fix(install): check permissions before npm link#517
Closed
MilanKiele wants to merge 1 commit into
Closed
Conversation
Contributor
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Important Merge conflicts detected (Beta)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment Tip You can enable review details to help with troubleshooting, context usage and more.Enable the |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@install.sh`:
- Around line 230-231: The pre-check using [[ -w "$prefix/lib/node_modules" ]]
only detects writability but doesn't handle runtime failures under set -e;
change the two spots that run npm commands (the `npm link` invocation
referencing "$prefix/lib/node_modules" and the similar npm global install block)
to run the npm command directly and, on failure, detect if the exit indicates a
permission error (EACCES) and retry with a fallback (e.g., using sudo or
changing install path) instead of exiting immediately; in practice wrap the `npm
link`/npm install command in a small retry block that captures the command exit
status, checks for permission-related failure, and performs the fallback retry
so actual command failure drives the retry logic rather than the initial
writable pre-check.
- Around line 233-234: The script calls "sudo npm link" directly (occurrences of
the literal string sudo npm link) which can block in non-interactive runs or
fail when sudo is not installed; update the install.sh logic to first detect if
sudo exists (e.g., command -v sudo) and whether the process is interactive
(e.g., test -t 1 or checking for CI env), and only invoke "sudo npm link" when
sudo is present and the session is interactive; otherwise fall back to running
"npm link" as the current user (or emit a clear non-fatal warning via warn) and
avoid prompting for a password or failing when sudo is absent.
Comment on lines
+230
to
+231
| if [[ -w "$prefix/lib/node_modules" ]]; then | ||
| npm link |
Contributor
There was a problem hiding this comment.
Retry should be driven by command failure, not only by pre-check.
Line 230 and Line 239 only check one path up front. With set -e, if the npm command still fails (including permission failures outside this pre-check), the script exits without retrying. Wrap the command and retry when it actually fails with EACCES.
Suggested fix
install_nemoclaw() {
+ run_npm_with_eacces_retry() {
+ local tmp
+ tmp="$(mktemp)"
+ if "$@" 2> >(tee "$tmp" >&2); then
+ rm -f "$tmp"
+ return 0
+ fi
+ if grep -q "EACCES" "$tmp"; then
+ warn "Permission denied for npm global operation. Retrying with sudo…"
+ rm -f "$tmp"
+ sudo "$@"
+ return $?
+ fi
+ rm -f "$tmp"
+ return 1
+ }
+
if [[ -f "./package.json" ]] && grep -q '"name": "nemoclaw"' ./package.json 2>/dev/null; then
info "NemoClaw package.json found in current directory — installing from source…"
npm install
- local prefix
- prefix="$(npm config get prefix 2>/dev/null || echo "/usr/local")"
- if [[ -w "$prefix/lib/node_modules" ]]; then
- npm link
- else
- warn "Insufficient permissions to link NemoClaw globally. Retrying with sudo…"
- sudo npm link
- fi
+ run_npm_with_eacces_retry npm link
else
info "Installing NemoClaw from GitHub…"
- if [[ -w "$(npm config get prefix 2>/dev/null || echo "/usr/local")/lib/node_modules" ]]; then
- npm install -g git+https://github.com/NVIDIA/NemoClaw.git
- else
- warn "Insufficient permissions for global install. Retrying with sudo…"
- sudo npm install -g git+https://github.com/NVIDIA/NemoClaw.git
- fi
+ run_npm_with_eacces_retry npm install -g git+https://github.com/NVIDIA/NemoClaw.git
fiAlso applies to: 239-240
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@install.sh` around lines 230 - 231, The pre-check using [[ -w
"$prefix/lib/node_modules" ]] only detects writability but doesn't handle
runtime failures under set -e; change the two spots that run npm commands (the
`npm link` invocation referencing "$prefix/lib/node_modules" and the similar npm
global install block) to run the npm command directly and, on failure, detect if
the exit indicates a permission error (EACCES) and retry with a fallback (e.g.,
using sudo or changing install path) instead of exiting immediately; in practice
wrap the `npm link`/npm install command in a small retry block that captures the
command exit status, checks for permission-related failure, and performs the
fallback retry so actual command failure drives the retry logic rather than the
initial writable pre-check.
Comment on lines
+233
to
+234
| warn "Insufficient permissions to link NemoClaw globally. Retrying with sudo…" | ||
| sudo npm link |
Contributor
There was a problem hiding this comment.
Guard sudo for non-interactive installs and missing sudo.
Line 234 and Line 243 call sudo directly. In non-interactive runs this can block on a password prompt, and on minimal environments sudo may not exist. Add explicit handling for both cases.
Suggested fix
- warn "Insufficient permissions to link NemoClaw globally. Retrying with sudo…"
- sudo npm link
+ warn "Insufficient permissions to link NemoClaw globally. Retrying with sudo…"
+ command_exists sudo || error "sudo is required for global link but is not installed."
+ if [[ "${NON_INTERACTIVE:-}" == "1" ]]; then
+ sudo -n npm link || error "sudo requires a password in non-interactive mode. Re-run interactively or configure npm prefix to a user-writable path."
+ else
+ sudo npm link
+ fi
@@
- warn "Insufficient permissions for global install. Retrying with sudo…"
- sudo npm install -g git+https://github.com/NVIDIA/NemoClaw.git
+ warn "Insufficient permissions for global install. Retrying with sudo…"
+ command_exists sudo || error "sudo is required for global install but is not installed."
+ if [[ "${NON_INTERACTIVE:-}" == "1" ]]; then
+ sudo -n npm install -g git+https://github.com/NVIDIA/NemoClaw.git || error "sudo requires a password in non-interactive mode. Re-run interactively or configure npm prefix to a user-writable path."
+ else
+ sudo npm install -g git+https://github.com/NVIDIA/NemoClaw.git
+ fiAlso applies to: 242-243
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@install.sh` around lines 233 - 234, The script calls "sudo npm link" directly
(occurrences of the literal string sudo npm link) which can block in
non-interactive runs or fail when sudo is not installed; update the install.sh
logic to first detect if sudo exists (e.g., command -v sudo) and whether the
process is interactive (e.g., test -t 1 or checking for CI env), and only invoke
"sudo npm link" when sudo is present and the session is interactive; otherwise
fall back to running "npm link" as the current user (or emit a clear non-fatal
warning via warn) and avoid prompting for a password or failing when sudo is
absent.
Contributor
|
Thanks for improving the handling of npm global permission issues, this could make the installation process more robust for users. |
13 tasks
Contributor
|
Thanks for working on this! I opened PR #821 with a no-sudo alternative: instead of retrying with elevated privileges, it detects a non-writable npm prefix and redirects it to |
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 -->
Contributor
|
Thanks for this — checking permissions before |
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 -->
Contributor
|
This PR still has merge conflicts against main. If the branch isn't updated in the next 7 days we'll close it to keep the queue healthy — feel free to reopen once it's rebased, or open a new PR. Thanks for contributing to NemoClaw! |
Collaborator
|
Closing due to inactivity |
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
4 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.
Checks for write permissions to npm prefix and retries with sudo if EACCES is encountered.
Summary by CodeRabbit