fix(onboard): ensure nemoclaw CLI is on PATH after nvm-based install - #4191
fix(onboard): ensure nemoclaw CLI is on PATH after nvm-based install#4191Kartalops wants to merge 1 commit into
Conversation
|
✨ Thanks for submitting this detailed PR about ensuring the nemoclaw CLI is on PATH after an nvm-based install. This proposes a fix to modify the install process to include the CLI in the system PATH. |
wscurran
added
area: ci
After install.sh installs Node via nvm, npm global bins land in a non-standard directory that isn't on PATH. This adds the npm global bin dir to PATH immediately and persists it to .bashrc/.zshrc so nemoclaw works in the current session and future shells. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
6bd05a2 to
581e225
Compare
📝 WalkthroughWalkthroughinstall.sh is rewritten from a curl|bash bootstrap wrapper (repo cloning, ref resolution, payload verification) into a standalone installer that installs Node.js via nvm, conditionally installs Ollama and pulls a GPU/VRAM-sized model, installs NemoClaw, wires shell PATH, and runs onboarding. ChangesStandalone Installer Rewrite
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant InstallScript as install.sh
participant Nvm as nvm/Node
participant Ollama
participant Npm as npm/NemoClaw
User->>InstallScript: run install.sh
InstallScript->>Nvm: check node, install nvm + Node 24 if missing
Nvm-->>InstallScript: node installed
InstallScript->>InstallScript: detect GPU via nvidia-smi, detect VRAM
alt GPU present
InstallScript->>Ollama: install/upgrade if below OLLAMA_MIN_VERSION
Ollama-->>InstallScript: version confirmed
InstallScript->>Ollama: pull model sized to VRAM
end
InstallScript->>Npm: install NemoClaw (local or global)
Npm-->>InstallScript: persist PATH to ~/.bashrc or ~/.zshrc
InstallScript->>Npm: npx nemoclaw onboard
Npm-->>User: onboarding complete
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Rebased onto current Branch — sent via hermes-pr-bot. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
install.sh (1)
157-157: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRun the validated installed CLI directly.
Line 146 validates
nemoclawonPATH; onboarding should use that same binary instead of going back throughnpx.Proposed fix
- npx nemoclaw onboard + nemoclaw onboard🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@install.sh` at line 157, The onboarding step is invoking the CLI through npx instead of the validated installed binary. Update the onboarding command in install.sh to use the same nemoclaw executable that was just checked on PATH, so the install flow consistently runs the local installed CLI rather than re-resolving via npx.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@install.sh`:
- Line 167: The Ollama install path is advertised but never executed from main,
leaving a dead code path. Either call install_or_upgrade_ollama from the main
installer flow after the existing verification steps, or remove the
Ollama-related header/docs and helper functions if Ollama is no longer in scope.
Use the existing main entrypoint and install_or_upgrade_ollama symbol to locate
the wiring point.
- Line 40: The nvm install flow is executing a remote script directly via curl |
bash, which skips integrity checks. Update the install logic in the nvm
bootstrap section to first download the installer to a temporary file, verify it
against a pinned checksum or signature, and only then run it; apply the same
pattern to the other curl | bash/sh usages referenced in the install script so
all remote executables are verified before execution.
- Around line 56-62: The GPU detection logic in detect_gpu only checks
nvidia-smi, so macOS/Apple Silicon hosts are treated as GPU-less even though
get_vram_mb can fall back to unified memory. Update detect_gpu to recognize the
macOS path used by get_vram_mb (or otherwise treat Apple Silicon/macOS as
GPU-capable) so Ollama is not skipped before the VRAM fallback can run, and make
sure the related install flow that consumes detect_gpu follows the same logic.
- Around line 51-53: The version parsing in get_ollama_version() is too brittle
under pipefail, causing install.sh to exit before the current:-unknown upgrade
path can run. Update get_ollama_version() so the ollama --version pipeline is
non-fatal when parsing fails, and make it return an empty/unknown result instead
of aborting; use the existing get_ollama_version symbol and the current:-unknown
fallback path to keep the installer progressing.
- Around line 138-144: The npm global bin persistence logic only writes to one
shell profile based on which file exists, but it should update both supported
profiles. In the install.sh profile-handling block, adjust the shell setup so
the PATH export is appended to both .bashrc and .zshrc when present, instead of
choosing one via the profile variable. Keep the existing duplicate-check
behavior, and make sure the change is localized around the npm global bin
persistence logic.
- Around line 34-42: The install flow in install.sh currently exits early in the
node-check path, but it should verify both node and npm before skipping nvm.
Update the existing command_exists logic around the node version check so it
only returns when both node and npm are present and the Node version meets the
minimum required version; otherwise continue through the nvm install/upgrade
path in the same install block.
---
Nitpick comments:
In `@install.sh`:
- Line 157: The onboarding step is invoking the CLI through npx instead of the
validated installed binary. Update the onboarding command in install.sh to use
the same nemoclaw executable that was just checked on PATH, so the install flow
consistently runs the local installed CLI rather than re-resolving via npx.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e666ba7f-41c4-4741-9640-556de1928aec
📒 Files selected for processing (1)
install.sh
| if command_exists node; then | ||
| info "Node.js found: $(node --version)" | ||
| return | ||
| fi | ||
| printf "%s" "${NEMOCLAW_INSTALL_TAG:-$DEFAULT_INSTALL_REF}" | ||
|
|
||
| info "Node.js not found — installing via nvm…" | ||
| curl -o- https://raw.githubusercontent.com/nvm-sh/nvm/v0.40.4/install.sh | bash | ||
| \. "$HOME/.nvm/nvm.sh" | ||
| nvm install 24 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the target script and related helpers
git ls-files | rg '(^|/)install\.sh$|(^|/)README\.md$|(^|/)\.sh$'
# Show the install.sh file with line numbers
wc -l install.sh
cat -n install.sh | sed -n '1,220p'
# Search for helper functions and later npm/node usage
rg -n "command_exists|version_gte|install_nodejs|install_nemoclaw|npm|node --version|nvm install|nvm use" install.shRepository: NVIDIA/NemoClaw
Length of output: 9025
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect declared runtime requirements
wc -l package.json 2>/dev/null || true
cat -n package.json 2>/dev/null | sed -n '1,220p' || true
# Look for Node engine/version constraints and install docs
rg -n '"engines"|node|npm|npx|install_nodejs|install_nemoclaw|v24|24\.0\.0' package.json README.md install.sh scripts/install.shRepository: NVIDIA/NemoClaw
Length of output: 24212
Check both node and npm before skipping nvm. Any existing node currently bypasses the upgrade path, so Node < 22.16.0 or Node-without-npm setups still reach npm install later and fail.
🧰 Tools
🪛 ast-grep (0.44.0)
[error] 39-39: Remote content fetched with curl/wget is piped directly into a shell interpreter, so any server compromise, MITM, or tampered mirror results in arbitrary code execution on this host. Download the script to a file first, verify its integrity (checksum/signature) and inspect it, then run the verified local copy.
Context: curl -o- https://raw.githubusercontent.com/nvm-sh/nvm/v0.40.4/install.sh | bash
Note: [CWE-494] Download of Code Without Integrity Check.
(curl-pipe-to-shell-bash)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@install.sh` around lines 34 - 42, The install flow in install.sh currently
exits early in the node-check path, but it should verify both node and npm
before skipping nvm. Update the existing command_exists logic around the node
version check so it only returns when both node and npm are present and the Node
version meets the minimum required version; otherwise continue through the nvm
install/upgrade path in the same install block.
| printf "%s" "${NEMOCLAW_INSTALL_TAG:-$DEFAULT_INSTALL_REF}" | ||
|
|
||
| info "Node.js not found — installing via nvm…" | ||
| curl -o- https://raw.githubusercontent.com/nvm-sh/nvm/v0.40.4/install.sh | bash |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Avoid executing remote installer scripts without integrity verification.
These curl | bash/sh paths execute mutable network content as the current user. Download to a temp file, verify a pinned checksum/signature, then execute the verified file.
Also applies to: 89-96
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@install.sh` at line 40, The nvm install flow is executing a remote script
directly via curl | bash, which skips integrity checks. Update the install logic
in the nvm bootstrap section to first download the installer to a temporary
file, verify it against a pinned checksum or signature, and only then run it;
apply the same pattern to the other curl | bash/sh usages referenced in the
install script so all remote executables are verified before execution.
Source: Linters/SAST tools
| get_ollama_version() { | ||
| # `ollama --version` outputs something like "ollama version 0.18.0" | ||
| ollama --version 2>/dev/null | grep -oE '[0-9]+\.[0-9]+\.[0-9]+' | head -1 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make version parsing non-fatal.
With pipefail, an unparseable ollama --version exits the installer before the current:-unknown upgrade path can run.
Proposed fix
get_ollama_version() {
# `ollama --version` outputs something like "ollama version 0.18.0"
- ollama --version 2>/dev/null | grep -oE '[0-9]+\.[0-9]+\.[0-9]+' | head -1
+ ollama --version 2>/dev/null | grep -oE '[0-9]+\.[0-9]+\.[0-9]+' | head -1 || true
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| get_ollama_version() { | |
| # `ollama --version` outputs something like "ollama version 0.18.0" | |
| ollama --version 2>/dev/null | grep -oE '[0-9]+\.[0-9]+\.[0-9]+' | head -1 | |
| get_ollama_version() { | |
| # `ollama --version` outputs something like "ollama version 0.18.0" | |
| ollama --version 2>/dev/null | grep -oE '[0-9]+\.[0-9]+\.[0-9]+' | head -1 || true | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@install.sh` around lines 51 - 53, The version parsing in get_ollama_version()
is too brittle under pipefail, causing install.sh to exit before the
current:-unknown upgrade path can run. Update get_ollama_version() so the ollama
--version pipeline is non-fatal when parsing fails, and make it return an
empty/unknown result instead of aborting; use the existing get_ollama_version
symbol and the current:-unknown fallback path to keep the installer progressing.
| detect_gpu() { | ||
| # Returns 0 if a GPU is detected | ||
| if command_exists nvidia-smi; then | ||
| nvidia-smi &>/dev/null && return 0 | ||
| fi | ||
| if ! head -1 "$file" | grep -qE '^#!.*(sh|bash)'; then | ||
| printf "[ERROR] %s does not start with a shell shebang\n" "$label" >&2 | ||
| exit 1 | ||
| return 1 | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Align GPU detection with the macOS VRAM fallback.
get_vram_mb supports macOS unified memory, but detect_gpu only recognizes nvidia-smi; Apple Silicon/macOS hosts will always skip Ollama before that fallback is used.
Proposed fix
detect_gpu() {
# Returns 0 if a GPU is detected
if command_exists nvidia-smi; then
nvidia-smi &>/dev/null && return 0
fi
+ if [[ "$(uname -s)" == "Darwin" ]] && command_exists sysctl; then
+ return 0
+ fi
return 1
}Also applies to: 92-100
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@install.sh` around lines 56 - 62, The GPU detection logic in detect_gpu only
checks nvidia-smi, so macOS/Apple Silicon hosts are treated as GPU-less even
though get_vram_mb can fall back to unified memory. Update detect_gpu to
recognize the macOS path used by get_vram_mb (or otherwise treat Apple
Silicon/macOS as GPU-capable) so Ollama is not skipped before the VRAM fallback
can run, and make sure the related install flow that consumes detect_gpu follows
the same logic.
| # Persist npm global bin for future shells | ||
| local profile="$HOME/.bashrc" | ||
| [ -f "$HOME/.zshrc" ] && profile="$HOME/.zshrc" | ||
| if ! grep -q 'npm config get prefix' "$profile" 2>/dev/null; then | ||
| echo 'export PATH="$(npm config get prefix)/bin:$PATH"' >> "$profile" | ||
| info "Added npm global bin to $profile" | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Persist the npm global bin to both supported shell profiles.
The PR objective says .bashrc and .zshrc are updated, but this code only updates .zshrc when it exists, otherwise .bashrc.
Proposed fix
- local profile="$HOME/.bashrc"
- [ -f "$HOME/.zshrc" ] && profile="$HOME/.zshrc"
- if ! grep -q 'npm config get prefix' "$profile" 2>/dev/null; then
- echo 'export PATH="$(npm config get prefix)/bin:$PATH"' >> "$profile"
- info "Added npm global bin to $profile"
- fi
+ local profile
+ for profile in "$HOME/.bashrc" "$HOME/.zshrc"; do
+ touch "$profile"
+ if ! grep -Fqx 'export PATH="$(npm config get prefix)/bin:$PATH"' "$profile"; then
+ echo 'export PATH="$(npm config get prefix)/bin:$PATH"' >> "$profile"
+ info "Added npm global bin to $profile"
+ fi
+ done📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Persist npm global bin for future shells | |
| local profile="$HOME/.bashrc" | |
| [ -f "$HOME/.zshrc" ] && profile="$HOME/.zshrc" | |
| if ! grep -q 'npm config get prefix' "$profile" 2>/dev/null; then | |
| echo 'export PATH="$(npm config get prefix)/bin:$PATH"' >> "$profile" | |
| info "Added npm global bin to $profile" | |
| fi | |
| # Persist npm global bin for future shells | |
| local profile | |
| for profile in "$HOME/.bashrc" "$HOME/.zshrc"; do | |
| touch "$profile" | |
| if ! grep -Fqx 'export PATH="$(npm config get prefix)/bin:$PATH"' "$profile"; then | |
| echo 'export PATH="$(npm config get prefix)/bin:$PATH"' >> "$profile" | |
| info "Added npm global bin to $profile" | |
| fi | |
| done |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@install.sh` around lines 138 - 144, The npm global bin persistence logic only
writes to one shell profile based on which file exists, but it should update
both supported profiles. In the install.sh profile-handling block, adjust the
shell setup so the PATH export is appended to both .bashrc and .zshrc when
present, instead of choosing one via the profile variable. Keep the existing
duplicate-check behavior, and make sure the change is localized around the npm
global bin persistence logic.
| esac | ||
| done | ||
| install_nodejs | ||
| # install_or_upgrade_ollama |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Either wire in Ollama setup or remove the advertised Ollama install path.
The installer header and Ollama functions say GPU hosts get Ollama/model setup, but main never calls it. Re-enable this after addressing the installer verification issues above, or update the installer scope to avoid a dead path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@install.sh` at line 167, The Ollama install path is advertised but never
executed from main, leaving a dead code path. Either call
install_or_upgrade_ollama from the main installer flow after the existing
verification steps, or remove the Ollama-related header/docs and helper
functions if Ollama is no longer in scope. Use the existing main entrypoint and
install_or_upgrade_ollama symbol to locate the wiring point.
|
Closing as superseded during the The PATH outcome landed in #228 and was subsequently hardened in #1468 with the stable Thank you for identifying the original PATH problem; the maintained implementation is now on |
4 participants
Description
Ensure nemoclaw CLI is on PATH after nvm-based install.
Changes
fix: ensure nemoclaw CLI is on PATH after nvm-based installTesting
PATH verification after nvm-based install.
Summary by CodeRabbit