fix(install): detect and remove broken npm placeholder package#1606
Merged
miyoungc merged 1 commit intoNVIDIA:mainfrom Apr 9, 2026
Merged
fix(install): detect and remove broken npm placeholder package#1606miyoungc merged 1 commit intoNVIDIA:mainfrom
miyoungc merged 1 commit intoNVIDIA:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughInstaller now validates discovered Changes
Sequence Diagram(s)sequenceDiagram
participant Shell as Shell / PATH
participant Installer as Installer Script
participant NPM_BIN as $npm_prefix/bin/nemoclaw
participant NPM as npm (global pkg)
Shell->>Installer: execute install.sh
Installer->>Shell: command_exists("nemoclaw")?
alt found in PATH
Installer->>Shell: run `nemoclaw --version`
alt output matches "nemoclaw v<semver>"
Installer->>Shell: accept existing CLI
else output invalid
Installer->>NPM: run `npm uninstall -g nemoclaw`
NPM->>NPM_BIN: remove broken binary
Installer->>Shell: continue installer (install CLI)
end
else not in PATH
Installer->>NPM_BIN: check `$npm_prefix/bin/nemoclaw`
alt exists and `--version` valid
Installer->>Shell: set NEMOCLAW_RECOVERY_EXPORT_DIR (export path) and warn
else exists but invalid
Installer->>NPM: run `npm uninstall -g nemoclaw`
NPM->>NPM_BIN: remove broken binary
Installer->>Shell: proceed with installer (install CLI)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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: 2
🧹 Nitpick comments (2)
docs/reference/commands.md (1)
25-25: Split this into one-sentence lines and avoid passive voice.This change puts two sentences on one source line, and “It is installed automatically by ...” is passive. Please split the sentences and make the installer script the subject.
As per coding guidelines, "Active voice required. Flag passive constructions." and "One sentence per line in source (makes diffs readable). Flag paragraphs where multiple sentences appear on the same line."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/commands.md` at line 25, Split the current line into two lines so each sentence is on its own source line, and rewrite the second sentence to use active voice with the installer script as the subject; e.g., keep the first line describing the "nemoclaw CLI" as the primary interface for managing NemoClaw sandboxes, then add a second line starting with "The installer script installs the `nemoclaw` CLI..." to remove passive voice and satisfy the one-sentence-per-line rule referenced in the docs text about the `nemoclaw` CLI and the installer script.test/install-preflight.test.js (1)
385-390: Assert the post-remediation happy path too.This scenario currently proves the uninstall side effect, but not that the installer actually recovers and leaves a valid CLI behind. Adding a
result.status === 0assertion and a final--versioncheck on the replacement binary would keep the test aligned with the PR objective.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/install-preflight.test.js` around lines 385 - 390, Add post-remediation assertions to verify the installer recovered successfully: after the existing checks using output and npmLog, assert result.status === 0 to confirm the process exited successfully, then spawn or exec the replaced CLI binary (use the same path used earlier in the test) with the "--version" flag and assert its stdout matches the expected version string (or a semver regex). Reference the existing test variables result, output, npmLog and the fs-based log check to locate where to insert the new assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/install.sh`:
- Around line 935-938: The verify_nemoclaw helper currently calls
remove_broken_nemoclaw after install_nemoclaw (which runs npm link), so the
placeholder uninstall path can be skipped; move the broken-package cleanup to
run before any linking—either invoke remove_broken_nemoclaw at the start of
install_nemoclaw (before npm link) or change the call site so verify_nemoclaw is
executed before install_nemoclaw performs npm link; update references to
verify_nemoclaw, install_nemoclaw and remove_broken_nemoclaw accordingly so the
cleanup always runs prior to creating the linked CLI.
- Around line 918-933: The remove_broken_nemoclaw() function currently returns
non-zero on no-op paths which aborts callers under set -e; change its non-error
exits to return 0 so no-op/valid-binary cases are considered successful.
Specifically, update the checks inside remove_broken_nemoclaw: replace the early
exit after the executable test ([[ -x "$nemoclaw_bin" ]] || return 1) to return
0 instead, and change the final return 1 to return 0 so successful/no-op
outcomes return success; keep error/command-failure paths (like npm_prefix
assignment failures) returning non-zero as-is.
---
Nitpick comments:
In `@docs/reference/commands.md`:
- Line 25: Split the current line into two lines so each sentence is on its own
source line, and rewrite the second sentence to use active voice with the
installer script as the subject; e.g., keep the first line describing the
"nemoclaw CLI" as the primary interface for managing NemoClaw sandboxes, then
add a second line starting with "The installer script installs the `nemoclaw`
CLI..." to remove passive voice and satisfy the one-sentence-per-line rule
referenced in the docs text about the `nemoclaw` CLI and the installer script.
In `@test/install-preflight.test.js`:
- Around line 385-390: Add post-remediation assertions to verify the installer
recovered successfully: after the existing checks using output and npmLog,
assert result.status === 0 to confirm the process exited successfully, then
spawn or exec the replaced CLI binary (use the same path used earlier in the
test) with the "--version" flag and assert its stdout matches the expected
version string (or a semver regex). Reference the existing test variables
result, output, npmLog and the fs-based log check to locate where to insert the
new assertions.
🪄 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
Run ID: 8411efea-8f3c-4b6b-acd0-648b64b34d8e
📒 Files selected for processing (4)
.agents/skills/nemoclaw-reference/references/commands.mddocs/reference/commands.mdscripts/install.shtest/install-preflight.test.js
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/install-preflight.test.js (1)
391-397: Consider adding an assertion to verify the binary was actually removed.While the test checks that
npm uninstall -g nemoclawwas called, it would be more robust to also verify that the broken binary no longer exists after the test completes:// After the existing assertions: expect(fs.existsSync(path.join(prefix, "bin", "nemoclaw"))).toBe(false); // Or if npm link recreates it with a valid binary: const finalBinary = path.join(prefix, "bin", "nemoclaw"); if (fs.existsSync(finalBinary)) { const versionOutput = spawnSync(finalBinary, ["--version"], { encoding: "utf-8" }); expect(versionOutput.stdout).toMatch(/^nemoclaw v/); }This would verify the end-to-end behavior: broken binary detected → removed → valid binary installed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/install-preflight.test.js` around lines 391 - 397, Add an assertion after the existing expectations to verify the broken binary was actually removed: check fs.existsSync(path.join(prefix, "bin", "nemoclaw")) is false; if a reinstall may recreate the binary, instead check the recreated binary by calling spawnSync(finalBinary, ["--version"], { encoding: "utf-8" }) and assert the output matches the expected version pattern (e.g., /^nemoclaw v/); update the test block that references output/npmLog to include this existence/version assertion using the same prefix, fs, path and spawnSync identifiers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/install-preflight.test.js`:
- Around line 313-397: The test currently invokes the installer via
spawnSync("bash", [INSTALLER], ...) which causes the script to detect the repo
as a source checkout and skip the remove_broken_nemoclaw() path; fix by changing
the test invocation so the installer runs as a piped script or forcing bootstrap
mode: either pipe the installer contents into bash (like cat INSTALLER | bash
...) so SCRIPT_DIR isn't the repo, or set the environment variable
NEMOCLAW_BOOTSTRAP_PAYLOAD=1 in the spawnSync env to force the bootstrap/clone
path; ensure the test still asserts the uninstall log and references the same
INSTALLER and remove_broken_nemoclaw() behavior.
---
Nitpick comments:
In `@test/install-preflight.test.js`:
- Around line 391-397: Add an assertion after the existing expectations to
verify the broken binary was actually removed: check
fs.existsSync(path.join(prefix, "bin", "nemoclaw")) is false; if a reinstall may
recreate the binary, instead check the recreated binary by calling
spawnSync(finalBinary, ["--version"], { encoding: "utf-8" }) and assert the
output matches the expected version pattern (e.g., /^nemoclaw v/); update the
test block that references output/npmLog to include this existence/version
assertion using the same prefix, fs, path and spawnSync identifiers.
🪄 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
Run ID: a0c4473d-62d4-4ad2-a51e-934289ee9877
📒 Files selected for processing (4)
.agents/skills/nemoclaw-reference/references/commands.mddocs/reference/commands.mdscripts/install.shtest/install-preflight.test.js
✅ Files skipped from review due to trivial changes (2)
- .agents/skills/nemoclaw-reference/references/commands.md
- docs/reference/commands.md
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/install.sh
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
scripts/install.sh (1)
1174-1176: Add an order assertion for the cleanup-before-link fix.This wiring looks right now, but the current e2e in
test/install-preflight.test.js(Lines 313-370) only proves thatuninstall -g nemoclawhappened at some point. A future refactor could move this back belowinstall_nemoclawand still pass. Recording and assertinguninstall -g nemoclawbeforelinkwould lock the regression down.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install.sh` around lines 1174 - 1176, Add an explicit order assertion in the preflight test to lock the cleanup-before-link behavior: update the test that exercises remove_broken_nemoclaw / install_nemoclaw / verify_nemoclaw to capture the run logs/output and assert that the "uninstall -g nemoclaw" (the removal from remove_broken_nemoclaw) occurs before the linking step (the link operation invoked during install/linking), e.g. by finding the index/position of the uninstall message and the link message in the aggregated output and asserting uninstall_index < link_index so future refactors can't move the uninstall after link.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/install.sh`:
- Around line 975-977: The fallback messages that suggest running "npm install
-g git+https://github.com/NVIDIA/NemoClaw.git" are dangerous because
install_nemoclaw() intentionally avoids that path; update the three messages
(the warn/warn/error block that currently mentions the raw npm git install and
the "nemoclaw binary not found" error) to instead direct users to re-run the
project's installer entrypoint (e.g., the top-level install script or documented
installer command) and/or refer them to the install_nemoclaw() flow for
pinned/ref-aware installation; change the text for the warn lines to suggest
using the installer entrypoint and change the final error to indicate
installation failed and recommend running the installer entrypoint or opening an
issue rather than suggesting raw npm git install.
- Around line 906-912: The is_real_nemoclaw_cli() function currently uses a
loose regex; tighten it to only accept full semver outputs (e.g. "nemoclaw
vMAJOR.MINOR.PATCH" with optional pre-release/build) so malformed strings like
"nemoclaw v1 broken" fail; update the regex in is_real_nemoclaw_cli() to anchor
the end and require numeric MAJOR.MINOR.PATCH (optionally with -prerelease or
+build) so remove_broken_nemoclaw() and verify_nemoclaw() reliably detect real
installs.
---
Nitpick comments:
In `@scripts/install.sh`:
- Around line 1174-1176: Add an explicit order assertion in the preflight test
to lock the cleanup-before-link behavior: update the test that exercises
remove_broken_nemoclaw / install_nemoclaw / verify_nemoclaw to capture the run
logs/output and assert that the "uninstall -g nemoclaw" (the removal from
remove_broken_nemoclaw) occurs before the linking step (the link operation
invoked during install/linking), e.g. by finding the index/position of the
uninstall message and the link message in the aggregated output and asserting
uninstall_index < link_index so future refactors can't move the uninstall after
link.
🪄 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
Run ID: fec4dfe9-d061-4485-9666-f06f955bc6aa
📒 Files selected for processing (3)
.agents/skills/nemoclaw-reference/references/commands.mddocs/reference/commands.mdscripts/install.sh
✅ Files skipped from review due to trivial changes (2)
- .agents/skills/nemoclaw-reference/references/commands.md
- docs/reference/commands.md
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
scripts/install.sh (1)
955-957:⚠️ Potential issue | 🟠 MajorDon't point failures at raw
npm install -g git+...here.
install_nemoclaw()explicitly avoids that path so it can pre-extractopenclawbefore npm install and pin the resolved ref. This hint can reintroduce the GH-503 tarball failure or install default-branch HEAD instead of the tagged release. Point users back to the installer entrypoint instead.Suggested fix
warn "Could not locate the nemoclaw executable." -warn "Try running: npm install -g git+https://github.com/NVIDIA/NemoClaw.git" +warn "Try re-running: curl -fsSL https://www.nvidia.com/nemoclaw.sh | bash" error "Installation failed: nemoclaw binary not found."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install.sh` around lines 955 - 957, The displayed error message incorrectly suggests running a raw `npm install -g git+https://github.com/NVIDIA/NemoClaw.git`, which `install_nemoclaw()` intentionally avoids; update the three warning/error strings (the messages currently shown when the nemoclaw binary is not found) to direct users back to the installer entrypoint (e.g., run the provided installer script or follow the project's install instructions) instead of recommending a raw git-based npm install so we don't reintroduce the GH-503 tarball failure or install an unpinned HEAD; change the messages referenced in the failing branch where `nemoclaw` is checked (the messages surrounding the current "Could not locate the nemoclaw executable." / "Try running: npm install -g git+https://github.com/NVIDIA/NemoClaw.git" / "Installation failed: nemoclaw binary not found.") to a single clear hint pointing to the installer entrypoint.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@scripts/install.sh`:
- Around line 955-957: The displayed error message incorrectly suggests running
a raw `npm install -g git+https://github.com/NVIDIA/NemoClaw.git`, which
`install_nemoclaw()` intentionally avoids; update the three warning/error
strings (the messages currently shown when the nemoclaw binary is not found) to
direct users back to the installer entrypoint (e.g., run the provided installer
script or follow the project's install instructions) instead of recommending a
raw git-based npm install so we don't reintroduce the GH-503 tarball failure or
install an unpinned HEAD; change the messages referenced in the failing branch
where `nemoclaw` is checked (the messages surrounding the current "Could not
locate the nemoclaw executable." / "Try running: npm install -g
git+https://github.com/NVIDIA/NemoClaw.git" / "Installation failed: nemoclaw
binary not found.") to a single clear hint pointing to the installer entrypoint.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3be950f3-69ef-47c8-a24a-26f30d9c69e0
📒 Files selected for processing (2)
scripts/install.shtest/install-preflight.test.js
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/install-preflight.test.js (1)
434-441: Optional: extract the repeated inline fakenemoclawshim into a shared helper.The same shell-body pattern is duplicated across many tests; centralizing it would reduce churn for future behavior tweaks (e.g., version format changes).
♻️ Refactor sketch
+function nemoclawShim({ includeOnboardLog = false } = {}) { + return `#!/usr/bin/env bash +if [ "$1" = "--version" ]; then echo "nemoclaw v0.1.0-test"; exit 0; fi +if [ "$1" = "onboard" ]; then + ${includeOnboardLog ? `printf '%s\\n' "$*" >> "$NEMOCLAW_ONBOARD_LOG"` : `exit 0`} + exit 0 +fi +exit 0 +`; +} ... -if [ "$1" = "link" ]; then - cat > "$NPM_PREFIX/bin/nemoclaw" <<'EOS' -#!/usr/bin/env bash -if [ "$1" = "--version" ]; then echo "nemoclaw v0.1.0-test"; exit 0; fi -if [ "$1" = "onboard" ]; then exit 0; fi -exit 0 -EOS +if [ "$1" = "link" ]; then + cat > "$NPM_PREFIX/bin/nemoclaw" <<'EOS' +${nemoclawShim()} +EOS🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/install-preflight.test.js` around lines 434 - 441, Extract the repeated inline shim into a shared test helper (e.g., writeFakeNemoclawShim(npmPrefix, {version, onboardExitCode})) that writes the same script content to "$NPM_PREFIX/bin/nemoclaw", sets executable permissions, and returns any errors; replace in-test heredoc blocks with calls to writeFakeNemoclawShim and pass a configurable version string and behavior so tests can change version format or responses centrally; update tests that reference the inline shim to call this helper and remove duplicate chmod/exit handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/install-preflight.test.js`:
- Around line 434-441: Extract the repeated inline shim into a shared test
helper (e.g., writeFakeNemoclawShim(npmPrefix, {version, onboardExitCode})) that
writes the same script content to "$NPM_PREFIX/bin/nemoclaw", sets executable
permissions, and returns any errors; replace in-test heredoc blocks with calls
to writeFakeNemoclawShim and pass a configurable version string and behavior so
tests can change version format or responses centrally; update tests that
reference the inline shim to call this helper and remove duplicate chmod/exit
handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bdf31a1b-eeff-44a6-afc5-fbf28ba60672
📒 Files selected for processing (2)
scripts/install.shtest/install-preflight.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/install.sh
cv
requested changes
Apr 8, 2026
Contributor
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Security Review — WARNING (CI failing)
Security-wise the code is clean — detection heuristic is strict, no injection risks, removal scope is appropriate, fail-safe control flow.
Required fix
CI failure: The warn "Try re-running: curl -fsSL ... | bash" line in install.sh triggers the curl-pipe-to-shell guard in test/runner.test.js:631. The guard's exemption filter (line 607) only allows printf and echo prefixes, not warn.
Options:
- Add
warnto the exempted prefixes in the guard test - Reword the message to avoid the pattern (e.g.,
warn "Try re-running the installer: https://www.nvidia.com/nemoclaw.sh")
Security positives
is_real_nemoclaw_cliuses a strict anchored semver regex — good detectionnpm uninstall -g nemoclawis hardcoded, no variable interpolation"$bin_path" --versionis properly double-quoted- Fail-safe: uninstall + error message, never silently accepts a broken binary
wscurran
added
NemoClaw CLI
Dongni-Yang
force-pushed
the
fix/remove-bare-npm-install-506
branch
from
8444075 to
e24230f
Compare
cv
approved these changes
Apr 9, 2026
Contributor
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
LGTM — security review PASS. CI failure resolved.
is_real_nemoclaw_cli()uses strict anchored regex — only matches real CLI output- Both verification paths (command-v, npm-bin) now validate before accepting
npm uninstall -g nemoclawis hardcoded — no injection riskwarncorrectly added to curl-pipe-to-shell exemption (user-facing message, not an invocation)- Test shims all updated to match new
--versionformat - Docs updated to stop recommending
npm install -g nemoclaw(the broken placeholder)
No concerns.
…A#506) Signed-off-by: Dongni Yang <dongniy@nvidia.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Dongni-Yang
force-pushed
the
fix/remove-bare-npm-install-506
branch
from
87cce4f to
bece102
Compare
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
4 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
npm install -g nemoclawfrom docs — it points to a broken 249-byte placeholder on npmjs.org that only contains package.jsonis_real_nemoclaw_cli()behavioural validation toverify_nemoclaw()— runsnemoclaw --versionand verifies the output matchesnemoclaw v<semver>(mirrors theisOpenshellCLI()pattern from PR fix: validate openshell binary to prevent npm package shadowing #970)verify_nemoclaw()to validate every candidate binary before accepting it, and auto-uninstall if the binary fails the checkSupersedes #761 (docs-only fix, now stale with merge conflicts).
Fixes #506
Relates to #737, #967
Test plan
Updated all existing fake nemoclaw stubs to output
nemoclaw v<version>format so they passis_real_nemoclaw_cli(). Existing testprints the HTTPS GitHub remediation when the binary is missingstill asserts barenpm install -g nemoclawnever appears in output.Signed-off-by: Dongni Yang dongniy@nvidia.com
Summary by CodeRabbit
Documentation
Bug Fixes