fix(install): make prek optional in prepare script (fixes #731)#746
fix(install): make prek optional in prepare script (fixes #731)#746cv merged 2 commits intoNVIDIA:mainfrom
Conversation
The prepare script runs 'prek install' to set up git hooks, but prek is a devDependency. When end users install NemoClaw from a git checkout without --include=dev (or when the installer runs 'npm install --ignore-scripts' then later triggers prepare), prek is not available and npm exits with code 127. Fix: Guard the prek invocation with 'command -v prek' so it only runs when the binary is actually installed (i.e., in a dev environment). End-user installs skip hook setup gracefully.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Thanks for submitting this PR, it makes prek optional in the prepare script, which could improve the CI/CD pipeline and user experience by providing more flexibility and preventing potential issues. |
cv
left a comment
There was a problem hiding this comment.
Fixes the right problem, but as-is this silently skips hook setup even for developers where prek should be available (e.g. devDeps installed but binary somehow broken). Consider distinguishing "prek intentionally absent" (deploy/CI) from "prek should be here but isn't" (dev) — see inline suggestion.
…NVIDIA#746) * fix(install): make prek optional in prepare script (fixes NVIDIA#731) The prepare script runs 'prek install' to set up git hooks, but prek is a devDependency. When end users install NemoClaw from a git checkout without --include=dev (or when the installer runs 'npm install --ignore-scripts' then later triggers prepare), prek is not available and npm exits with code 127. Fix: Guard the prek invocation with 'command -v prek' so it only runs when the binary is actually installed (i.e., in a dev environment). End-user installs skip hook setup gracefully. * fix: less forgiving prepare script --------- Co-authored-by: Kagura Chen <daniyuu19@sjtu.edu.cn> Co-authored-by: Carlos Villela <cv@lixo.org>
…NVIDIA#746) * fix(install): make prek optional in prepare script (fixes NVIDIA#731) The prepare script runs 'prek install' to set up git hooks, but prek is a devDependency. When end users install NemoClaw from a git checkout without --include=dev (or when the installer runs 'npm install --ignore-scripts' then later triggers prepare), prek is not available and npm exits with code 127. Fix: Guard the prek invocation with 'command -v prek' so it only runs when the binary is actually installed (i.e., in a dev environment). End-user installs skip hook setup gracefully. * fix: less forgiving prepare script --------- Co-authored-by: Kagura Chen <daniyuu19@sjtu.edu.cn> Co-authored-by: Carlos Villela <cv@lixo.org>
|
What's the intended behaviour after this PR? I just tried to install this on a clean Ubuntu VM and it failed and stopped with the error:
This seems to be essentially the same issue described in #731 which this PR was intended to fix, only there's a slightly different error message. Was that the intention, or was installation supposed to continue? |
|
Thanks for the detailed report @DanTup — you're right, this is a bug in my fix. The The correct behavior should be: if |
|
Fix submitted in #1502 — removes the |
## Problem PR #746 (merged) made `prek` optional in the prepare script, but introduced a regression: when `node_modules/@j178/prek` directory exists but the `prek` binary is not in PATH, the script errors with `exit 1`. This happens in practice when running `npm install --omit=dev` (as the NemoClaw install script does) — the package directory may be present from a previous full install, but the binary won't be linked to PATH. Reported by @DanTup on a clean Ubuntu VM: #746 (comment) ## Root Cause The prepare script tried to distinguish between "prek intentionally absent" and "prek should be here but isn't" by checking for the `node_modules/@j178/prek` directory. However, this distinction is unreliable — the directory can exist without the binary being available (e.g., after `--omit=dev` with a stale `node_modules`). ## Fix Remove the `elif [ -d node_modules/@j178/prek ]` branch entirely. If `prek` is not in PATH, simply skip hook setup regardless of whether the package directory exists. **Before:** ```bash if [ -d .git ]; then if command -v prek >/dev/null 2>&1; then prek install; elif [ -d node_modules/@j178/prek ]; then echo "ERROR: ..." && exit 1; # ← BUG else echo "Skipping git hook setup (prek not installed)"; fi fi ``` **After:** ```bash if [ -d .git ]; then if command -v prek >/dev/null 2>&1; then prek install; else echo "Skipping git hook setup (prek not installed)"; fi fi ``` ## Test Scenarios | Scenario | Expected | Result | |----------|----------|--------| | `.git` exists + `prek` in PATH | Runs `prek install` | ✅ | | `.git` exists + `prek` not in PATH + `node_modules/@j178/prek` exists | Skip (no error) | ✅ (was ❌ `exit 1`) | | `.git` exists + `prek` not in PATH + no `node_modules` | Skip | ✅ | | No `.git` directory | Skip | ✅ | Fixes regression from #746. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Simplified the installation preparation script to streamline git hook setup. When the required tooling isn't available, the script now gracefully skips hook configuration (and logs a skip) instead of failing, reducing setup friction for users. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
## Problem PR NVIDIA#746 (merged) made `prek` optional in the prepare script, but introduced a regression: when `node_modules/@j178/prek` directory exists but the `prek` binary is not in PATH, the script errors with `exit 1`. This happens in practice when running `npm install --omit=dev` (as the NemoClaw install script does) — the package directory may be present from a previous full install, but the binary won't be linked to PATH. Reported by @DanTup on a clean Ubuntu VM: NVIDIA#746 (comment) ## Root Cause The prepare script tried to distinguish between "prek intentionally absent" and "prek should be here but isn't" by checking for the `node_modules/@j178/prek` directory. However, this distinction is unreliable — the directory can exist without the binary being available (e.g., after `--omit=dev` with a stale `node_modules`). ## Fix Remove the `elif [ -d node_modules/@j178/prek ]` branch entirely. If `prek` is not in PATH, simply skip hook setup regardless of whether the package directory exists. **Before:** ```bash if [ -d .git ]; then if command -v prek >/dev/null 2>&1; then prek install; elif [ -d node_modules/@j178/prek ]; then echo "ERROR: ..." && exit 1; # ← BUG else echo "Skipping git hook setup (prek not installed)"; fi fi ``` **After:** ```bash if [ -d .git ]; then if command -v prek >/dev/null 2>&1; then prek install; else echo "Skipping git hook setup (prek not installed)"; fi fi ``` ## Test Scenarios | Scenario | Expected | Result | |----------|----------|--------| | `.git` exists + `prek` in PATH | Runs `prek install` | ✅ | | `.git` exists + `prek` not in PATH + `node_modules/@j178/prek` exists | Skip (no error) | ✅ (was ❌ `exit 1`) | | `.git` exists + `prek` not in PATH + no `node_modules` | Skip | ✅ | | No `.git` directory | Skip | ✅ | Fixes regression from NVIDIA#746. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Simplified the installation preparation script to streamline git hook setup. When the required tooling isn't available, the script now gracefully skips hook configuration (and logs a skip) instead of failing, reducing setup friction for users. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
## Problem PR NVIDIA#746 (merged) made `prek` optional in the prepare script, but introduced a regression: when `node_modules/@j178/prek` directory exists but the `prek` binary is not in PATH, the script errors with `exit 1`. This happens in practice when running `npm install --omit=dev` (as the NemoClaw install script does) — the package directory may be present from a previous full install, but the binary won't be linked to PATH. Reported by @DanTup on a clean Ubuntu VM: NVIDIA#746 (comment) ## Root Cause The prepare script tried to distinguish between "prek intentionally absent" and "prek should be here but isn't" by checking for the `node_modules/@j178/prek` directory. However, this distinction is unreliable — the directory can exist without the binary being available (e.g., after `--omit=dev` with a stale `node_modules`). ## Fix Remove the `elif [ -d node_modules/@j178/prek ]` branch entirely. If `prek` is not in PATH, simply skip hook setup regardless of whether the package directory exists. **Before:** ```bash if [ -d .git ]; then if command -v prek >/dev/null 2>&1; then prek install; elif [ -d node_modules/@j178/prek ]; then echo "ERROR: ..." && exit 1; # ← BUG else echo "Skipping git hook setup (prek not installed)"; fi fi ``` **After:** ```bash if [ -d .git ]; then if command -v prek >/dev/null 2>&1; then prek install; else echo "Skipping git hook setup (prek not installed)"; fi fi ``` ## Test Scenarios | Scenario | Expected | Result | |----------|----------|--------| | `.git` exists + `prek` in PATH | Runs `prek install` | ✅ | | `.git` exists + `prek` not in PATH + `node_modules/@j178/prek` exists | Skip (no error) | ✅ (was ❌ `exit 1`) | | `.git` exists + `prek` not in PATH + no `node_modules` | Skip | ✅ | | No `.git` directory | Skip | ✅ | Fixes regression from NVIDIA#746. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Simplified the installation preparation script to streamline git hook setup. When the required tooling isn't available, the script now gracefully skips hook configuration (and logs a skip) instead of failing, reducing setup friction for users. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
Fixes #731
Problem
The
preparescript inpackage.jsonunconditionally runsprek installto set up git hooks. Sinceprekis a devDependency (@j178/prek), it's not installed when:npm installwithout--include=devnpm install --ignore-scriptsthen later triggersprepareThis causes
npm error code 127(sh: 1: prek: not found) and blocks installation.Solution
Guard the
prekinvocation withcommand -v prekso the prepare hook only runs when prek is actually available:Developers with devDependencies installed get hooks as before. End-user installs skip hook setup gracefully.
One-line change
Only
package.jsonis modified.Summary by CodeRabbit