fix(install): add build:cli step and harden E2E destroy check#1277
Merged
Conversation
PR #1240 replaced bin/lib/ CJS modules with thin shims that re-export from dist/lib/, but install.sh was never updated to run `npm run build:cli`. Every fresh install now crashes on `nemoclaw onboard` with "Cannot find module '../../dist/lib/preflight'". Add the missing build:cli step to both install paths (local source and GitHub clone). Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
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:
📝 WalkthroughWalkthroughAdded CLI module building step to NemoClaw installation process and updated E2E test verification methods to check the registry file directly instead of parsing Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
The install-preflight test npm stubs only matched `npm run build` but not `npm run build:cli`, causing exit code 98 when install.sh invokes the new CLI build step. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
![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 `@install.sh`:
- Line 702: The installer calls npm run build:cli unconditionally via the spin
wrapper (e.g., spin "Building NemoClaw CLI modules" npm run build:cli), which
fails for refs where package.json lacks the build:cli script; modify install.sh
to check whether the script exists before running it: detect the npm script by
inspecting package.json (e.g., using jq or npm run | grep) in the current
NemoClaw checkout (respecting NEMOCLAW_INSTALL_TAG), and only invoke spin ...
npm run build:cli when the script is present; ensure you perform the same
guarded check for both occurrences referencing build:cli so installs on
tags/refs without that script don’t error out.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Installs targeting older tags via NEMOCLAW_INSTALL_TAG would fail if the ref's package.json lacks the build:cli script. Using npm run --if-present makes the step a no-op on those refs. Update test npm stubs to accept the --if-present flag. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
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 `@install.sh`:
- Line 702: The install.sh usage of "npm run --if-present build:cli" should be
changed to either remove the "--if-present" so npm fails if the script is
missing, or add an explicit detection+fallback: check for the presence of the
"build:cli" npm script (e.g., by listing available scripts) and if absent run a
fallback like "build" or fail with a clear error; update both occurrences that
call npm run --if-present build:cli (the spin "Building NemoClaw CLI modules"
invocations) to use this explicit check/fallback so missing scripts do not
silently succeed.
🪄 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: b57c6161-4327-426f-acab-e7d14d9851f4
📒 Files selected for processing (2)
install.shtest/install-preflight.test.js
✅ Files skipped from review due to trivial changes (1)
- test/install-preflight.test.js
nemoclaw list called recoverRegistryEntries() which could restart a destroyed gateway and re-import stale sandbox entries. This caused the E2E destroy verification to fail: destroy removes the sandbox, then list resurrects it by restarting the gateway and polling openshell sandbox list. list should just read the local registry. Recovery belongs in commands that need a live gateway (connect, status). Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
ericksoa
changed the title
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/cli.test.js (1)
646-646: Test may fail on systems without openshell installed.Since the registry contains
gamma,listSandboxes()will callcaptureOpenshell(["inference", "get"], ...)to query live inference. Ifopenshellis not in PATH,getOpenshellBinary()callsprocess.exit(1), causing test failure.Other tests (e.g., "prefers live inference model/provider over stale registry values" at line 1475) stub
openshellfor the same scenario. Consider adding a minimal stub here for test isolation.♻️ Suggested stub addition
+ const localBin = path.join(home, "bin"); + fs.mkdirSync(localBin, { recursive: true }); + fs.writeFileSync( + path.join(localBin, "openshell"), + [ + "#!/usr/bin/env bash", + 'if [ "$1" = "inference" ] && [ "$2" = "get" ]; then', + " exit 1", + "fi", + "exit 0", + ].join("\n"), + { mode: 0o755 }, + ); + - const r = runWithEnv("list", { HOME: home }); + const r = runWithEnv("list", { HOME: home, PATH: `${localBin}:${process.env.PATH || ""}` });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cli.test.js` at line 646, The test calls runWithEnv("list", { HOME: home }) which triggers listSandboxes() -> captureOpenshell(["inference","get"], ...) and getOpenshellBinary() may call process.exit(1) if openshell is missing; add a minimal openshell stub for this test by creating a tiny executable script (or otherwise stubbing getOpenshellBinary/captureOpenshell) and prepending its directory to PATH before invoking runWithEnv so the lookup succeeds and the test no longer exits; reference the helpers runWithEnv, listSandboxes, captureOpenshell, and getOpenshellBinary when making the change.
🤖 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/cli.test.js`:
- Line 646: The test calls runWithEnv("list", { HOME: home }) which triggers
listSandboxes() -> captureOpenshell(["inference","get"], ...) and
getOpenshellBinary() may call process.exit(1) if openshell is missing; add a
minimal openshell stub for this test by creating a tiny executable script (or
otherwise stubbing getOpenshellBinary/captureOpenshell) and prepending its
directory to PATH before invoking runWithEnv so the lookup succeeds and the test
no longer exits; reference the helpers runWithEnv, listSandboxes,
captureOpenshell, and getOpenshellBinary when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 03aad817-5508-4868-82a0-dded37bf02cc
📒 Files selected for processing (2)
bin/nemoclaw.jstest/cli.test.js
Revert the list-is-read-only change — the recovery behavior from #1187 is intentional and valuable after reboots. The E2E destroy check used `nemoclaw list` which triggers gateway recovery, potentially restarting a destroyed gateway and re-importing stale sandbox entries. Check the registry file directly instead. The list-recovery-after-destroy interaction is a real bug but belongs in a separate PR. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
ericksoa
changed the title
cv
approved these changes
Apr 1, 2026
laitingsheng
pushed a commit
that referenced
this pull request
Apr 2, 2026
## Summary - **install.sh missing build:cli**: PR #1240 replaced `bin/lib/` CJS modules with shims that re-export from `dist/lib/`, but `install.sh` never ran `npm run build:cli`. Every fresh install crashed on `nemoclaw onboard` with `Cannot find module '../../dist/lib/preflight'` - **E2E destroy verification**: The cleanup phase used `nemoclaw list` to verify a sandbox was gone, but `list` triggers gateway recovery which can restart a destroyed gateway and re-import the sandbox. Now checks the registry file directly instead - Uses `--if-present` so installs targeting older tags without `build:cli` don't break ## Test plan - [ ] Run `install.sh` on a clean VM and verify `nemoclaw onboard` no longer crashes - [ ] Verify `dist/lib/preflight.js` exists after install completes - [ ] Nightly E2E cloud-e2e passes the destroy verification phase --------- Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
lakamsani
pushed a commit
to lakamsani/NemoClaw
that referenced
this pull request
Apr 4, 2026
…#1277) ## Summary - **install.sh missing build:cli**: PR NVIDIA#1240 replaced `bin/lib/` CJS modules with shims that re-export from `dist/lib/`, but `install.sh` never ran `npm run build:cli`. Every fresh install crashed on `nemoclaw onboard` with `Cannot find module '../../dist/lib/preflight'` - **E2E destroy verification**: The cleanup phase used `nemoclaw list` to verify a sandbox was gone, but `list` triggers gateway recovery which can restart a destroyed gateway and re-import the sandbox. Now checks the registry file directly instead - Uses `--if-present` so installs targeting older tags without `build:cli` don't break ## Test plan - [ ] Run `install.sh` on a clean VM and verify `nemoclaw onboard` no longer crashes - [ ] Verify `dist/lib/preflight.js` exists after install completes - [ ] Nightly E2E cloud-e2e passes the destroy verification phase --------- Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
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
bin/lib/CJS modules with shims that re-export fromdist/lib/, butinstall.shnever rannpm run build:cli. Every fresh install crashed onnemoclaw onboardwithCannot find module '../../dist/lib/preflight'nemoclaw listto verify a sandbox was gone, butlisttriggers gateway recovery which can restart a destroyed gateway and re-import the sandbox. Now checks the registry file directly instead--if-presentso installs targeting older tags withoutbuild:clidon't breakTest plan
install.shon a clean VM and verifynemoclaw onboardno longer crashesdist/lib/preflight.jsexists after install completesSummary by CodeRabbit