Skip to content

fix(uninstall): source nvm in non-interactive shells to find npm#2072

Closed
Sanjays2402 wants to merge 1 commit intoNVIDIA:mainfrom
Sanjays2402:fix/uninstall-nvm-path
Closed

fix(uninstall): source nvm in non-interactive shells to find npm#2072
Sanjays2402 wants to merge 1 commit intoNVIDIA:mainfrom
Sanjays2402:fix/uninstall-nvm-path

Conversation

@Sanjays2402
Copy link
Copy Markdown

@Sanjays2402 Sanjays2402 commented Apr 19, 2026

Summary\n\nWhen running curl -fsSL .../uninstall.sh | bash, nvm is not sourced in the non-interactive shell, so npm is not on PATH. The remove_nemoclaw_cli() function skips CLI removal with "npm not found".\n\n## Changes\n\n1. Auto-detect nvm: Before the npm check, attempts to source $HOME/.nvm/nvm.sh or $NVM_DIR/nvm.sh and activate the default node version\n2. Fallback removal: After npm uninstall, if nemoclaw is still on PATH, directly removes the binary\n\nFixes #1959

Summary by CodeRabbit

  • Bug Fixes
    • Improved the uninstall process for the CLI tool to better handle Node/NVM environments, ensuring complete removal of all associated files and binaries.

When running `curl ... | bash`, nvm is not sourced in the non-interactive
shell, so npm is not on PATH and the CLI removal step is skipped.

This adds nvm auto-detection before the npm check and a fallback to
directly remove the nemoclaw binary if npm uninstall fails.

Fixes NVIDIA#1959
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 19, 2026

📝 Walkthrough

Walkthrough

Updated the remove_nemoclaw_cli() function in the uninstall script to handle environments where npm is unavailable in non-interactive shells. The function now sources NVM, activates a Node version, and adds a fallback to manually remove the nemoclaw binary if it persists after npm uninstall.

Changes

Cohort / File(s) Summary
NVM Environment Setup
uninstall.sh
Enhanced remove_nemoclaw_cli() to source nvm.sh, invoke nvm use default (with nvm use current fallback), and proceed with npm unlink/uninstall. Added post-removal fallback to detect and remove lingering nemoclaw binary via remove_path function.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 A script once left the CLI behind,
But now with NVM, we'll be more kind,
Sourcing paths and switching nodes with care,
Nemoclaw vanishes—poof—into the air! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely summarizes the main change: fixing the uninstall script by sourcing nvm in non-interactive shells to properly find and use npm.
Linked Issues check ✅ Passed The PR directly addresses issue #1959 by implementing both required fixes: sourcing nvm to find npm in non-interactive shells and adding a fallback to remove the nemoclaw binary if npm removal fails.
Out of Scope Changes check ✅ Passed All changes are scoped to the remove_nemoclaw_cli() function in uninstall.sh and directly address the documented objectives; no unrelated modifications are present.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

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 446-453: Check the located nemoclaw binary (the result of command
-v nemoclaw stored in nemoclaw_bin) to ensure it lives under installer-managed
prefixes (e.g., the installer’s install dir or a known list of managed PATH
prefixes) before calling remove_path; if it is outside those managed paths, log
and skip deletion. Also avoid hard-failing on permission errors by verifying
writability (-w) before attempting removal and by making the remove attempt
tolerant to failure (e.g., try remove_path but append a non-fatal fallback like
|| true or log the permission error and continue) so the script won’t exit under
set -e; keep references to the nemoclaw_bin variable, remove_path call, and info
logging when implementing these checks.
🪄 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: deeee923-9065-472b-9364-a81f976fbd43

📥 Commits

Reviewing files that changed from the base of the PR and between 4e6508d and c2615ff.

📒 Files selected for processing (1)
  • uninstall.sh

Comment thread uninstall.sh
Comment on lines +446 to +453
# Fallback: if nemoclaw binary is still on PATH, remove it directly
if command -v nemoclaw >/dev/null 2>&1; then
local nemoclaw_bin
nemoclaw_bin="$(command -v nemoclaw)"
if [ -f "$nemoclaw_bin" ]; then
remove_path "$nemoclaw_bin"
info "Removed leftover nemoclaw binary at $nemoclaw_bin"
fi
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Constrain fallback deletion to installer-managed paths and avoid hard-fail on permissions.

At Line 451, deleting command -v nemoclaw unconditionally is risky: it can remove a non-installer-managed binary and may terminate uninstall under set -e if the file is not writable.

Suggested fix
-  if command -v nemoclaw >/dev/null 2>&1; then
-    local nemoclaw_bin
-    nemoclaw_bin="$(command -v nemoclaw)"
-    if [ -f "$nemoclaw_bin" ]; then
-      remove_path "$nemoclaw_bin"
-      info "Removed leftover nemoclaw binary at $nemoclaw_bin"
-    fi
-  fi
+  if command -v nemoclaw >/dev/null 2>&1; then
+    local nemoclaw_bin
+    nemoclaw_bin="$(command -v nemoclaw)"
+    case "$nemoclaw_bin" in
+      "${NEMOCLAW_SHIM_DIR}/nemoclaw"|"$HOME"/.nvm/versions/node/*/bin/nemoclaw)
+        remove_file_with_optional_sudo "$nemoclaw_bin"
+        info "Removed leftover nemoclaw binary at $nemoclaw_bin"
+        ;;
+      *)
+        warn "Found nemoclaw at $nemoclaw_bin, but it is not an installer-managed path; leaving it in place."
+        ;;
+    esac
+  fi
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@uninstall.sh` around lines 446 - 453, Check the located nemoclaw binary (the
result of command -v nemoclaw stored in nemoclaw_bin) to ensure it lives under
installer-managed prefixes (e.g., the installer’s install dir or a known list of
managed PATH prefixes) before calling remove_path; if it is outside those
managed paths, log and skip deletion. Also avoid hard-failing on permission
errors by verifying writability (-w) before attempting removal and by making the
remove attempt tolerant to failure (e.g., try remove_path but append a non-fatal
fallback like || true or log the permission error and continue) so the script
won’t exit under set -e; keep references to the nemoclaw_bin variable,
remove_path call, and info logging when implementing these checks.

@wscurran wscurran added bug Something isn't working uninstall labels Apr 20, 2026
@wscurran
Copy link
Copy Markdown
Contributor

✨ Thanks for submitting this PR that proposes a fix for the uninstall process to properly remove the CLI tool, which could help improve the user experience.


Possibly related open issues:

@prekshivyas
Copy link
Copy Markdown
Contributor

prekshivyas commented Apr 21, 2026

Thanks for the contribution @Sanjays2402 — appreciate you picking up #1959!

Closing this in favor of #1965, which was opened 3 days earlier for the same issue and ended up more complete:

  • Scans all nvm prefixes via find ${NVM_DIR}/versions/node (catches leftovers from inactive node versions), while this PR only cleans the currently-active prefix.
  • Cleans both bin/nemoclaw and lib/node_modules/nemoclaw, vs. binary-only here.
  • Preserves the is_installer_managed_nemoclaw_shim guard — the fallback remove_path "$(command -v nemoclaw)" here can delete a user-managed ~/.local/bin/nemoclaw that the existing code deliberately protects.

Please do keep contributing — happy to see more PRs from you 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants