fix(uninstall): source nvm before npm uninstall to find the correct p…#1965
fix(uninstall): source nvm before npm uninstall to find the correct p…#1965cv merged 5 commits intoNVIDIA:mainfrom
Conversation
…refix When running uninstall.sh via `curl | bash`, nvm is not loaded into the shell environment. This means `command -v npm` either fails (npm not on PATH) or finds a system npm whose global prefix differs from the nvm-managed one used during install. In both cases, `npm uninstall -g nemoclaw` does not remove the binary installed under the nvm prefix (e.g. ~/.nvm/versions/node/v22.x/bin/nemoclaw). Source nvm.sh at the start of remove_nemoclaw_cli() so npm resolves to the same prefix that originally installed nemoclaw. Running `nemoclaw uninstall` was not affected because the command itself runs from the nvm-installed Node.js, which already has the correct npm on PATH. Fixes NVIDIA#1959 Signed-off-by: swqa-saiy <swqa-saiy@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds NVM sourcing to the uninstall flow and extends CLI removal to scan and delete leftover Changes
Sequence Diagram(s)sequenceDiagram
participant User as "User"
participant Script as "uninstall.sh"
participant NVM as "NVM (nvm.sh)"
participant NPM as "npm/unlink"
participant FS as "Filesystem (~/.nvm/versions)"
rect rgba(200,230,255,0.5)
User->>Script: run uninstall script
Script->>NVM: ensure_nvm_for_uninstall() — source nvm.sh if present
end
rect rgba(200,255,200,0.5)
Script->>NPM: run npm unlink/uninstall (uses nvm-managed prefix)
NPM-->>Script: uninstall result
end
rect rgba(255,230,200,0.5)
Script->>FS: scan NVM_DIR/versions/node/** for bin/nemoclaw and lib/node_modules/nemoclaw
FS-->>Script: list of matches
Script->>FS: remove matching files/directories
end
Script->>User: report completion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@uninstall.sh`:
- Around line 383-399: The current cleanup only runs when command -v nemoclaw
finds a binary and therefore misses leftover installs in other NVM prefixes;
change the logic in the block using remaining/mod_dir to instead scan
${NVM_DIR:-$HOME/.nvm}/versions/node/* for versions containing bin/nemoclaw and
lib/node_modules/nemoclaw and remove them regardless of PATH. Concretely,
replace the outer "if command -v nemoclaw" guard with a loop over
"${NVM_DIR:-$HOME/.nvm}/versions/node" entries, for each candidate check for
"$candidate/bin/nemoclaw" and remove it (logging via info "Removed leftover
nemoclaw binary at ...") and then check and rm -rf
"$candidate/lib/node_modules/nemoclaw" (logging as well); preserve the existing
variable names (remaining/mod_dir) or use descriptive equivalents and keep
redirects like 2>/dev/null to avoid errors when directories are absent.
🪄 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: Pro Plus
Run ID: ade8f5cf-70f8-4659-a509-7e39b5b34d22
📒 Files selected for processing (1)
uninstall.sh
|
✨ Thanks for submitting this PR that proposes a fix for the uninstall command not removing the NemoClaw CLI, which could help improve the uninstall process and is a good first issue for new contributors. Possibly related open issues: |
The previous fallback only cleaned leftovers when `command -v nemoclaw`
still resolved to a binary. If npm uninstall removed the active prefix
but a different nvm node version still had nemoclaw installed, the
leftover was never touched. The `*/.nvm/*` glob also missed custom
NVM_DIR locations.
Scan ${NVM_DIR:-$HOME/.nvm}/versions/node directly with find(1) to
remove bin/nemoclaw and lib/node_modules/nemoclaw from every nvm-managed
prefix, regardless of which version is currently active on PATH.
Addresses CodeRabbit review on NVIDIA#1965.
Signed-off-by: swqa-saiy <swqa-saiy@users.noreply.github.com>
## Summary Refreshes user-facing docs for the last 24 hours of merged NemoClaw history and bumps the docs metadata to 0.0.29, the next version after v0.0.28. The updates are limited to behavior supported by merged PR descriptions and diffs. ## Changes - `docs/reference/commands.md`: documented `nemoclaw <name> policy-add --from-file` and `--from-dir`, including custom preset review guidance, from #2077 / commit `7720b175`. - `docs/deployment/deploy-to-remote-gpu.md`: clarified that non-loopback `CHAT_UI_URL` disables OpenClaw device pairing for remote browser-only deployments, from #2449 / commit `f5ee8a4d`. - `docs/inference/inference-options.md`: documented provider-aware credential retry validation and the NVIDIA-only `nvapi-` prefix check, from #2389 / commit `6f7f0c6d`. - `docs/inference/switch-inference-providers.md`: documented `NEMOCLAW_INFERENCE_INPUTS` for text/image-capable model metadata baked into `openclaw.json`, from #2441 / commit `f4391892`. - `docs/reference/troubleshooting.md`: added the Git certificate verification entry for proxy CA propagation through `GIT_SSL_CAINFO`, `GIT_SSL_CAPATH`, `CURL_CA_BUNDLE`, and `REQUESTS_CA_BUNDLE`, from #2345 / commit `fa0dc1ab`. - `docs/versions1.json` and `docs/project.json`: promoted docs version `0.0.29`; `docs/versions1.json` omits unpublished `0.0.26`, `0.0.27`, and `0.0.28` entries. - `.agents/skills/nemoclaw-user-*`: regenerated derived user skill references from the updated docs. - Reviewed with no extra doc changes: #2575 / `d392ec07`, #2565 / `a3231049`, #1965 / `db1ef3ca`, #1990 / `db665834`, #2495 / `7da86fa3`, #2496 / `3192f4f4`, #2490 / `8c209058`, #2487 / `1f615e2f`, #2483 / `5653d33a`, #2482 / `31c782c0`, #2464 / `23bb5703`, #2472 / `a54f9a34`, and #2437 / `6bc860d7`. - Skipped per docs policy: #2420 / `7b76df6b` touched the experimental sandbox config path listed in `docs/.docs-skip`; #2466 / `cc15689c` touched a skipped term and CI-only sandbox image files. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [x] Doc only (includes code sample changes) ## Verification <!-- Check each item you ran and confirmed. Leave unchecked items you skipped. --> - [x] `npx prek run --all-files` passes - [ ] `npm test` passes — failed locally in installer-integration tests and one onboard helper timeout; the doc-scoped hook test projects passed under `prek`. - [ ] Tests added or updated for new or changed behavior - [x] No secrets, API keys, or credentials committed - [x] Docs updated for user-facing behavior changes - [ ] `make docs` builds without warnings (doc changes only) — build succeeded, but local Sphinx emitted the existing version-switcher file read message. - [x] 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 <!-- If an AI agent authored or co-authored this PR, check the box and name the tool. Remove this section for fully human-authored PRs. --> - [x] AI-assisted — tool: Codex --- <!-- DCO sign-off required by CI. Run: git config user.name && git config user.email --> Signed-off-by: Miyoung Choi <miyoungc@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Support for custom YAML presets in policy configuration via --from-file and --from-dir. * New build-time inference input option to declare accepted modalities (text or text,image). * **Improvements** * Credential validation now offers interactive recovery: re-enter key, retry, choose another provider, or exit. * Clarified provider-specific API key prefix handling (nvapi- only applies to NVIDIA keys). * **Documentation** * TLS certificate troubleshooting for inspected networks. * Clarified remote dashboard security/device-pairing behavior; command docs updated; docs version bumped. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Miyoung Choi <miyoungc@nvidia.com>
Summary
Source
nvm.shbefore runningnpm uninstall -g nemoclawin the curl-based uninstaller, and add a fallback that removes any leftover nemoclaw binary found under an nvm prefix.Related Issue
Fixes #1959
Changes
ensure_nvm_for_uninstall()that sources~/.nvm/nvm.shso npm resolves to the nvm-managed prefixremove_nemoclaw_cli()before any npm operations~/.nvm/*/bin/, remove the binary and itslib/node_modules/nemoclawentry directlyRoot cause:
curl | bashruns in a non-interactive shell that does not source.bashrc/.zshrc, so nvm is not loaded.npm uninstall -geither fails (npm not on PATH) or targets the wrong global prefix (system npm vs nvm npm), leaving~/.nvm/versions/node/v22.x/bin/nemoclawbehind.nemoclaw uninstall --yeswas unaffected because it runs from the nvm-installed Node.js which already has the correct npm on PATH.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesManual test results
Tested on local machine (nvm v22.22.1, Ubuntu 22.04):
env -iclean shell (no nvm) + old codeenv -iclean shell + new code (source nvm)rmremoves leftoverAI Disclosure
Signed-off-by: swqa-saiy swqa-saiy@users.noreply.github.com
Summary by CodeRabbit