Skip to content

fix(install): check permissions before npm link #517

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
MilanKiele wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(install): check permissions before npm link #517

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 17 additions & 2 deletions install.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,11 +222,26 @@ install_or_upgrade_ollama() {
install_nemoclaw() {
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 && npm link
npm install

# Check if we have write access to the npm prefix to avoid EACCES errors
local prefix
prefix="$(npm config get prefix 2>/dev/null || echo "/usr/local")"
if [[ -w "$prefix/lib/node_modules" ]]; then
npm link
Comment on lines +230 to +231
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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
   fi

Also 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.

else
warn "Insufficient permissions to link NemoClaw globally. Retrying with sudo…"
sudo npm link
Comment on lines +233 to +234
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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
+      fi

Also 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.

fi
else
info "Installing NemoClaw from GitHub…"
# Revert once https://github.com/NVIDIA/NemoClaw/issues/71 is complete and the package is published
npm install -g git+https://github.com/NVIDIA/NemoClaw.git
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
fi

refresh_path
Expand Down