fix(installer): resolve npm prefix when host nvm is present#485
Merged
cv merged 8 commits intoNVIDIA:mainfrom Apr 22, 2026
Merged
fix(installer): resolve npm prefix when host nvm is present#485cv merged 8 commits intoNVIDIA:mainfrom
cv merged 8 commits intoNVIDIA:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughExtracted npm global-bin resolution into Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Installer
participant npm
participant nvm
participant FS as Filesystem
User->>Installer: run install script
Installer->>npm: probe npm on PATH
alt npm present
npm-->>Installer: responds
else npm absent
Installer->>nvm: source NVM_DIR to load npm
nvm-->>Installer: nvm loaded
Installer->>npm: probe npm after loading nvm
npm-->>Installer: responds or fails
end
Installer->>npm: npm config get prefix
npm-->>Installer: prefix
Installer->>Installer: compute npm_bin = prefix/bin
Installer->>FS: ensure npm_bin is in PATH / create shim
Installer->>FS: check writability of prefix/bin and prefix/lib/node_modules
alt writable
Installer->>npm: npm link / npm install -g (no sudo)
else not writable
Installer->>FS: invoke sudo for npm link / install
end
Installer->>User: emit verification ("Verified: nemoclaw is available")
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
wscurran
added
enhancement
Contributor
|
Thanks for tackling the installer issues, especially around npm resolution and PATH refresh - this could make a big difference for users with nvm installed. |
afurm
force-pushed
the
fix/installer-nvm-path-refresh
branch
from
bee4afc to
3ef8fef
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/install.sh (1)
431-439: Good fix for avoiding unnecessary sudo, with a minor robustness consideration.The writability check correctly avoids
sudo npm linkwhen the npm prefix is writable, fixing the stdin issue in CI pipelines. However,npm linkwrites to both$npm_prefix/binand$npm_prefix/lib/node_modules. If these subdirectories have different permissions than the prefix itself, the link could still fail.For typical nodesource setups or CI containers, checking just the prefix is likely sufficient. If edge cases arise, consider checking
$npm_prefix/binwritability as well.💡 Optional: More precise writability check
npm_prefix="$(npm config get prefix 2>/dev/null || true)" - if [ -z "$npm_prefix" ] || [ ! -w "$npm_prefix" ]; then + if [ -z "$npm_prefix" ] || [ ! -w "$npm_prefix/bin" ]; then SUDO="sudo" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install.sh` around lines 431 - 439, The current SUDO detection (variables NODE_MGR, SUDO, npm_prefix) only checks writability of npm_prefix which can miss differing permissions on npm_prefix/bin or npm_prefix/lib/node_modules; update the logic in scripts/install.sh so after obtaining npm_prefix you test writability of "$npm_prefix/bin" and "$npm_prefix/lib/node_modules" (falling back to testing npm_prefix if those paths are missing) and only leave SUDO empty when those target dirs are writable (otherwise set SUDO="sudo"); ensure the same early-root check (id -u) and NODE_MGR="nodesource" condition are preserved and reference SUDO, NODE_MGR, and npm_prefix in your changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/install.sh`:
- Around line 431-439: The current SUDO detection (variables NODE_MGR, SUDO,
npm_prefix) only checks writability of npm_prefix which can miss differing
permissions on npm_prefix/bin or npm_prefix/lib/node_modules; update the logic
in scripts/install.sh so after obtaining npm_prefix you test writability of
"$npm_prefix/bin" and "$npm_prefix/lib/node_modules" (falling back to testing
npm_prefix if those paths are missing) and only leave SUDO empty when those
target dirs are writable (otherwise set SUDO="sudo"); ensure the same early-root
check (id -u) and NODE_MGR="nodesource" condition are preserved and reference
SUDO, NODE_MGR, and npm_prefix in your changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 95455142-b500-4be1-a488-c62ece65b323
📒 Files selected for processing (3)
install.shscripts/install.shtest/install-preflight.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- test/install-preflight.test.js
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/install-preflight.test.js (1)
451-595: Test cleanup consideration: read-only directories may persist.The test sets
prefixBinandprefixNodeModulesto mode0o555(read-only). While the fakenpm linkrestores writability when running under fake sudo, if the test fails early or assertions fail, these directories remain read-only in the temp directory, which could cause issues during cleanup on some systems.Consider adding a cleanup step or using
t.after()to restore permissions:♻️ Optional: Ensure cleanup restores permissions
// At the start of the test, after creating the directories: const cleanup = () => { try { fs.chmodSync(prefixBin, 0o755); fs.chmodSync(prefixNodeModules, 0o755); } catch (e) { /* ignore */ } }; // Node test runner cleanup hook would go here if available🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/install-preflight.test.js` around lines 451 - 595, The test "uses sudo for npm link when nodesource target directories are not writable" leaves prefixBin and prefixNodeModules as mode 0o555 which can block cleanup on failures; add a teardown that restores permissions (chmodSync back to 0o755) for prefixBin and prefixNodeModules and invoke it in a finally block or the test framework's after/teardown hook (e.g., t.after or equivalent) so permissions are always reset even if assertions fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/install.sh`:
- Around line 126-149: The npm_link_targets_writable function doesn't verify
writability of the intermediate "$npm_prefix/lib" when
"$npm_prefix/lib/node_modules" is absent; update the logic inside
npm_link_targets_writable to check the parent directory of npm_lib_dir (e.g.,
"$npm_prefix/lib") and return failure if that parent exists but is not writable,
so that attempting to create "node_modules" won't later fail; keep existing
checks for npm_bin_dir and npm_prefix and use the variables npm_prefix and
npm_lib_dir to locate and test the intermediate dir.
---
Nitpick comments:
In `@test/install-preflight.test.js`:
- Around line 451-595: The test "uses sudo for npm link when nodesource target
directories are not writable" leaves prefixBin and prefixNodeModules as mode
0o555 which can block cleanup on failures; add a teardown that restores
permissions (chmodSync back to 0o755) for prefixBin and prefixNodeModules and
invoke it in a finally block or the test framework's after/teardown hook (e.g.,
t.after or equivalent) so permissions are always reset even if assertions fail.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: baae8255-8b26-4093-b869-c05cb5223e8a
📒 Files selected for processing (2)
scripts/install.shtest/install-preflight.test.js
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/install-preflight.test.js (1)
451-740: Consider extracting shared fake-runtime setup helpers.The two new sudo-focused tests duplicate a large setup surface (fake
node,git,npm,sudo,docker,openshell,uname). A small fixture builder would reduce drift and make future behavior changes easier to apply consistently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/install-preflight.test.js` around lines 451 - 740, Both tests duplicate a large fake runtime setup (creating tmp, fakeBin, prefix, writing node/git/npm/sudo/docker/openshell/uname via writeExecutable and using installerEnv), so extract that logic into a small fixture builder (e.g., createFakeRuntime or buildFakeBinWithExecutables) that accepts overrides for permissions and NPM_PREFIX/SUDO_LOG_PATH; update the two tests to call this helper and only mutate what differs (like chmod on prefix/lib or prefix/bin) and keep using writeExecutable and installerEnv in the helper to locate the code to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/install-preflight.test.js`:
- Around line 594-595: The current assertion using
assert.match(fs.readFileSync(sudoLog, "utf-8"), /^npm link$/m) allows "npm link"
to appear anywhere in sudo.log and misses extra sudo commands; update both
occurrences to assert the log equals exactly "npm link" (e.g., read sudoLog,
trim the content and compare with "npm link") so any additional sudo activity
fails the test; locate and replace the assert.match calls that reference sudoLog
in test/install-preflight.test.js accordingly.
---
Nitpick comments:
In `@test/install-preflight.test.js`:
- Around line 451-740: Both tests duplicate a large fake runtime setup (creating
tmp, fakeBin, prefix, writing node/git/npm/sudo/docker/openshell/uname via
writeExecutable and using installerEnv), so extract that logic into a small
fixture builder (e.g., createFakeRuntime or buildFakeBinWithExecutables) that
accepts overrides for permissions and NPM_PREFIX/SUDO_LOG_PATH; update the two
tests to call this helper and only mutate what differs (like chmod on prefix/lib
or prefix/bin) and keep using writeExecutable and installerEnv in the helper to
locate the code to change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ee692bc0-14be-4df9-964b-70f535fe2663
📒 Files selected for processing (2)
scripts/install.shtest/install-preflight.test.js
✅ Files skipped from review due to trivial changes (1)
- scripts/install.sh
2 tasks
kjw3
added a commit
that referenced
this pull request
Apr 4, 2026
## Summary Fixes a CLI install regression where `nemoclaw` was reported as installed but the command was broken after install completed. This changes the installer so: - user shells get a stable `~/.local/bin/nemoclaw` wrapper - the wrapper prepends the resolved Node bin to `PATH` before execing the installed CLI - bootstrap `curl | bash` installs no longer `npm link` a temporary cloned checkout - bootstrap payload clones are treated as ephemeral, not as persistent source checkouts ## Root Cause There were two installer-side problems: 1. The thin bootstrap `install.sh` cloned the selected ref into a temporary directory and then executed `scripts/install.sh` from that clone. The payload installer treated that temp clone as a source checkout and ran `npm link`, which created a global package symlink into the temp directory. Once the bootstrap temp directory was cleaned up, `nemoclaw` was broken. 2. Even when the CLI binary existed, future shells could still fail to run it because the shebang relied on `node` being on `PATH`. On hosts using `nvm`, sourcing `nvm.sh` was not sufficient to guarantee that a Node version was active in later shells. ## Related Issues Related: #569, #989, #1429 Notes: - `#569` is the closest installer/runtime-path issue. - `#989` and `#1429` track the developer-doc path that requires `npm link`. - This PR does not update docs, so it does not close `#989` or `#1429`. ## Related Prior Work This overlaps with earlier installer investigation/work in: - `#485` by @afurm - `#517` by @MilanKiele Those PRs helped surface the same general npm/prefix/PATH fragility from adjacent angles. ## What Changed - `install.sh` - marks bootstrap payload execution as ephemeral via `NEMOCLAW_BOOTSTRAP_PAYLOAD=1` - `scripts/install.sh` - adds a stable `~/.local/bin/nemoclaw` wrapper - wrapper exports the resolved Node bin onto `PATH` and execs the installed `nemoclaw` - `is_source_checkout()` now rejects bootstrap payload clones - `test/install-preflight.test.js` - adds coverage for source checkout detection and bootstrap payload handling - updates shim expectations to match the wrapper behavior ## Supported Paths This PR fixes the fully supported installer paths: - `curl | bash` - repo checkout + `./install.sh` Developer repo usage remains: - `git clone` - `npm install` - `npm link` - `nemoclaw onboard` Plain `npm install` alone is not treated as a supported CLI install path. ## Validation Local: - `npm test -- test/install-preflight.test.js` Live: - `brev-cpu` `kj-nemoclaw-cpu-test` - repo checkout + `./install.sh --non-interactive` - raw GitHub bootstrap installer equivalent to `curl | bash` - verified fresh-shell `command -v nemoclaw` and `nemoclaw --version` - `spark` `spark-d8c8` - reproduced broken pre-fix binary state - repo checkout + `./install.sh --non-interactive` - verified fresh-shell `command -v nemoclaw` and `nemoclaw --version` ## Notes I only live-tested non-interactive installs on this branch. Interactive install behavior was not live-retested here. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes / Improvements** * Improved installer reliability and shim creation so the CLI runs correctly even when Node is outside PATH. * Installer better distinguishes source checkouts vs non-persistent payload installs and emits clearer informational messages. * Uninstaller now recognizes and removes installer-generated wrapper shims while preserving unrelated user files. * **Tests** * Added and updated tests covering shim behavior, uninstall handling, and installation-type detection. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
wscurran
added
the
status: rebase
When a host nvm environment is present on the system, sourcing nvm.sh during installer PATH refresh could switch to a different npm than the one that performed the install, making nemoclaw appear missing even after a successful install. Add a resolve_npm_bin() helper that resolves the npm global bin from the active npm on PATH before falling back to nvm-loaded npm. Reuse this helper in refresh_path(), ensure_nemoclaw_shim(), and verify_nemoclaw() for consistent resolution across all installer code paths. refresh_path() no longer unconditionally calls ensure_nvm_loaded() first — it now uses resolve_npm_bin() which respects an already-active npm while still loading nvm as a fallback when needed. Drop obsolete test/install-preflight.test.js (replaced by .ts version in main).
afurm
force-pushed
the
fix/installer-nvm-path-refresh
branch
from
b06c2ed to
e1f37cc
Compare
Contributor
Author
|
Thanks for the review! I've rebased against main and updated the PR description to reflect the actual changes. The diff now adds a resolve_npm_bin() helper that uses the active npm on PATH before falling back to nvm — avoiding the hostile nvm.sh override issue. Also dropped the obsolete test/install-preflight.test.js since main now has the TypeScript version. Ready for another look when you have time. |
gemini2026
pushed a commit
to gemini2026/NemoClaw
that referenced
this pull request
Apr 14, 2026
## Summary Fixes a CLI install regression where `nemoclaw` was reported as installed but the command was broken after install completed. This changes the installer so: - user shells get a stable `~/.local/bin/nemoclaw` wrapper - the wrapper prepends the resolved Node bin to `PATH` before execing the installed CLI - bootstrap `curl | bash` installs no longer `npm link` a temporary cloned checkout - bootstrap payload clones are treated as ephemeral, not as persistent source checkouts ## Root Cause There were two installer-side problems: 1. The thin bootstrap `install.sh` cloned the selected ref into a temporary directory and then executed `scripts/install.sh` from that clone. The payload installer treated that temp clone as a source checkout and ran `npm link`, which created a global package symlink into the temp directory. Once the bootstrap temp directory was cleaned up, `nemoclaw` was broken. 2. Even when the CLI binary existed, future shells could still fail to run it because the shebang relied on `node` being on `PATH`. On hosts using `nvm`, sourcing `nvm.sh` was not sufficient to guarantee that a Node version was active in later shells. ## Related Issues Related: NVIDIA#569, NVIDIA#989, NVIDIA#1429 Notes: - `NVIDIA#569` is the closest installer/runtime-path issue. - `NVIDIA#989` and `NVIDIA#1429` track the developer-doc path that requires `npm link`. - This PR does not update docs, so it does not close `NVIDIA#989` or `NVIDIA#1429`. ## Related Prior Work This overlaps with earlier installer investigation/work in: - `NVIDIA#485` by @afurm - `NVIDIA#517` by @MilanKiele Those PRs helped surface the same general npm/prefix/PATH fragility from adjacent angles. ## What Changed - `install.sh` - marks bootstrap payload execution as ephemeral via `NEMOCLAW_BOOTSTRAP_PAYLOAD=1` - `scripts/install.sh` - adds a stable `~/.local/bin/nemoclaw` wrapper - wrapper exports the resolved Node bin onto `PATH` and execs the installed `nemoclaw` - `is_source_checkout()` now rejects bootstrap payload clones - `test/install-preflight.test.js` - adds coverage for source checkout detection and bootstrap payload handling - updates shim expectations to match the wrapper behavior ## Supported Paths This PR fixes the fully supported installer paths: - `curl | bash` - repo checkout + `./install.sh` Developer repo usage remains: - `git clone` - `npm install` - `npm link` - `nemoclaw onboard` Plain `npm install` alone is not treated as a supported CLI install path. ## Validation Local: - `npm test -- test/install-preflight.test.js` Live: - `brev-cpu` `kj-nemoclaw-cpu-test` - repo checkout + `./install.sh --non-interactive` - raw GitHub bootstrap installer equivalent to `curl | bash` - verified fresh-shell `command -v nemoclaw` and `nemoclaw --version` - `spark` `spark-d8c8` - reproduced broken pre-fix binary state - repo checkout + `./install.sh --non-interactive` - verified fresh-shell `command -v nemoclaw` and `nemoclaw --version` ## Notes I only live-tested non-interactive installs on this branch. Interactive install behavior was not live-retested here. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes / Improvements** * Improved installer reliability and shim creation so the CLI runs correctly even when Node is outside PATH. * Installer better distinguishes source checkouts vs non-persistent payload installs and emits clearer informational messages. * Uninstaller now recognizes and removes installer-generated wrapper shims while preserving unrelated user files. * **Tests** * Added and updated tests covering shim behavior, uninstall handling, and installation-type detection. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
wscurran
added
v0.0.22
Contributor
|
Thanks for the quick turnaround on the rebase and updated PR description — the |
Contributor
|
@afurm can I ask you add the DCO tag to the PR body, please? |
Contributor
Author
Done, added the DCO sign-off to the PR body. |
cv
changed the title
cv
approved these changes
Apr 22, 2026
13 tasks
ericksoa
added a commit
that referenced
this pull request
Apr 22, 2026
## Summary This PR fixes the post-merge main-CI regression introduced after #485 by making the installer npm-resolution permission test match non-root installer semantics on root-backed runners. The production installer logic is unchanged; only the test harness is adjusted so the assertion no longer depends on root-only writability behavior. ## Changes - update `test/install-npm-resolution.test.ts` so the permission-sensitive `npm_link_targets_writable()` assertion runs as `nobody` on Linux root runners - keep the temporary fixture directories traversable while making the lib path the actual unwritable target under test - preserve existing coverage for active-PATH npm preference, nvm fallback, and writable target detection without changing installer code ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Verification - [x] `npx prek run --all-files` passes - [x] `npm test` passes - [x] Tests added or updated for new or changed behavior - [x] No secrets, API keys, or credentials committed - [ ] Docs updated for user-facing behavior changes - [ ] `make docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) ## AI Disclosure - [x] AI-assisted — tool: pi agent --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Installer tests now support executing provided snippets "raw" (no automatic sourcing) so wrappers can control execution. * Tests simulate privilege-drop execution (running as an unprivileged user) to validate installer behavior under lowered privileges. * Improved handling of writable/unwritable install targets with conditional permission adjustments when running as root. * No public interfaces were changed; updates are test-only. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Aaron Erickson <aerickson@nvidia.com> Co-authored-by: Aaron Erickson <aerickson@nvidia.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes an installer regression where a host nvm environment could override the active npm during PATH refresh, causing nemoclaw to appear missing even after a successful install.
Changes
Testing
DCO
Signed-off-by: Andrii Furmanets furmanets.andriy@gmail.com