fix(uninstall): source nvm before npm uninstall to find the correct p…#1965
Open
swqa-saiy wants to merge 1 commit intoNVIDIA:mainfrom
Open
fix(uninstall): source nvm before npm uninstall to find the correct p…#1965swqa-saiy wants to merge 1 commit intoNVIDIA:mainfrom
swqa-saiy wants to merge 1 commit 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>
Contributor
📝 WalkthroughWalkthroughThis PR fixes an incomplete uninstall process by adding NVM directory awareness to the uninstall script. It introduces a helper function to source NVM, then extends cleanup logic to remove leftover nemoclaw binaries and modules from NVM-managed Node.js directories. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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
Comment on lines
+383
to
+399
| # Belt-and-suspenders: if nemoclaw is still on PATH after npm uninstall | ||
| # (e.g. nvm resolved to a different node version than the one used during | ||
| # install), try to remove it from all nvm prefixes. | ||
| if command -v nemoclaw >/dev/null 2>&1; then | ||
| local remaining | ||
| remaining="$(command -v nemoclaw 2>/dev/null || true)" | ||
| if [ -n "$remaining" ] && [[ "$remaining" == */.nvm/* ]]; then | ||
| rm -f "$remaining" | ||
| info "Removed leftover nemoclaw binary at $remaining" | ||
| # Also clean the lib/node_modules symlink if present. | ||
| local mod_dir | ||
| mod_dir="$(dirname "$(dirname "$remaining")")/lib/node_modules/nemoclaw" | ||
| if [ -e "$mod_dir" ]; then | ||
| rm -rf "$mod_dir" | ||
| fi | ||
| fi | ||
| fi |
Contributor
There was a problem hiding this comment.
The fallback still misses the version-mismatch residue it is meant to clean.
This block only runs if command -v nemoclaw still succeeds. If npm uninstall -g removes the currently active prefix but leaves another nemoclaw under a different ${NVM_DIR}/versions/node/* tree, command -v is empty and the leftover is never touched. The */.nvm/* check also skips custom NVM_DIR locations.
Scan ${NVM_DIR:-$HOME/.nvm}/versions/node directly instead of gating cleanup on PATH resolution.
Suggested fix
- # Belt-and-suspenders: if nemoclaw is still on PATH after npm uninstall
- # (e.g. nvm resolved to a different node version than the one used during
- # install), try to remove it from all nvm prefixes.
- if command -v nemoclaw >/dev/null 2>&1; then
- local remaining
- remaining="$(command -v nemoclaw 2>/dev/null || true)"
- if [ -n "$remaining" ] && [[ "$remaining" == */.nvm/* ]]; then
- rm -f "$remaining"
- info "Removed leftover nemoclaw binary at $remaining"
- # Also clean the lib/node_modules symlink if present.
- local mod_dir
- mod_dir="$(dirname "$(dirname "$remaining")")/lib/node_modules/nemoclaw"
- if [ -e "$mod_dir" ]; then
- rm -rf "$mod_dir"
- fi
- fi
- fi
+ # Belt-and-suspenders: clean any leftover nemoclaw installs from nvm-managed
+ # node prefixes, even if the active PATH no longer points at them.
+ local nvm_dir="${NVM_DIR:-$HOME/.nvm}"
+ if [ -d "$nvm_dir/versions/node" ]; then
+ local remaining mod_dir
+ while IFS= read -r -d '' remaining; do
+ rm -f "$remaining"
+ info "Removed leftover nemoclaw binary at $remaining"
+ done < <(find "$nvm_dir/versions/node" -path '*/bin/nemoclaw' -print0 2>/dev/null)
+
+ while IFS= read -r -d '' mod_dir; do
+ rm -rf "$mod_dir"
+ info "Removed leftover nemoclaw module at $mod_dir"
+ done < <(find "$nvm_dir/versions/node" -path '*/lib/node_modules/nemoclaw' -print0 2>/dev/null)
+ fi📝 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.
Suggested change
| # Belt-and-suspenders: if nemoclaw is still on PATH after npm uninstall | |
| # (e.g. nvm resolved to a different node version than the one used during | |
| # install), try to remove it from all nvm prefixes. | |
| if command -v nemoclaw >/dev/null 2>&1; then | |
| local remaining | |
| remaining="$(command -v nemoclaw 2>/dev/null || true)" | |
| if [ -n "$remaining" ] && [[ "$remaining" == */.nvm/* ]]; then | |
| rm -f "$remaining" | |
| info "Removed leftover nemoclaw binary at $remaining" | |
| # Also clean the lib/node_modules symlink if present. | |
| local mod_dir | |
| mod_dir="$(dirname "$(dirname "$remaining")")/lib/node_modules/nemoclaw" | |
| if [ -e "$mod_dir" ]; then | |
| rm -rf "$mod_dir" | |
| fi | |
| fi | |
| fi | |
| # Belt-and-suspenders: clean any leftover nemoclaw installs from nvm-managed | |
| # node prefixes, even if the active PATH no longer points at them. | |
| local nvm_dir="${NVM_DIR:-$HOME/.nvm}" | |
| if [ -d "$nvm_dir/versions/node" ]; then | |
| local remaining mod_dir | |
| while IFS= read -r -d '' remaining; do | |
| rm -f "$remaining" | |
| info "Removed leftover nemoclaw binary at $remaining" | |
| done < <(find "$nvm_dir/versions/node" -path '*/bin/nemoclaw' -print0 2>/dev/null) | |
| while IFS= read -r -d '' mod_dir; do | |
| rm -rf "$mod_dir" | |
| info "Removed leftover nemoclaw module at $mod_dir" | |
| done < <(find "$nvm_dir/versions/node" -path '*/lib/node_modules/nemoclaw' -print0 2>/dev/null) | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@uninstall.sh` around lines 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.
Contributor
|
✨ 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: |
wscurran
added
NemoClaw CLI
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
2 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
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