refactor(cli): delete setup.sh, route all setup through nemoclaw onboard#1235
Merged
refactor(cli): delete setup.sh, route all setup through nemoclaw onboard#1235
Conversation
scripts/setup.sh (324 lines) duplicated every step that `nemoclaw onboard` already handles with better error recovery, exponential backoff, interactive prompts, and registry management. - Delete scripts/setup.sh and its dedicated test file - `nemoclaw setup` now delegates to onboard instead of shelling out to the deleted script - brev-setup.sh installs the CLI and runs `nemoclaw onboard --non-interactive` instead of calling setup.sh - Remove setup.sh assertions from gateway-cleanup and runner tests - Update comments in walkthrough.sh and brev-e2e.test.js Relates to #924 (shell consolidation). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe changes replace the legacy Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 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 docstrings
🧪 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
🧹 Nitpick comments (2)
test/e2e/brev-e2e.test.js (2)
172-187: Drop the second CLI install and just verify visibility.
scripts/brev-setup.shnow performs the samenpm_config_prefix/npm linksequence, so this re-runs a networked install for no new state and adds more flake to an already long E2E bootstrap. Awhich nemoclaw && nemoclaw --versionsanity check should be enough here.💡 Suggested simplification
- // Install nemoclaw CLI into ~/.local so test helper scripts can find it. - // brev-setup.sh already installs and runs onboard, but npm link there uses - // the default prefix; re-linking here with npm_config_prefix ensures the - // binary lands on the PATH used by runRemoteTest. - console.log(`[${elapsed()}] Installing nemoclaw CLI...`); + // Verify the CLI installed by brev-setup.sh is visible to the non-login + // SSH sessions used by runRemoteTest. + console.log(`[${elapsed()}] Verifying nemoclaw CLI...`); ssh( [ `export npm_config_prefix=$HOME/.local`, `export PATH=$HOME/.local/bin:$PATH`, - `cd ${remoteDir}/nemoclaw && npm install && npm run build`, - `cd ${remoteDir} && npm install --ignore-scripts && npm link`, `which nemoclaw && nemoclaw --version`, ].join(" && "), { timeout: 120_000 }, ); - console.log(`[${elapsed()}] nemoclaw CLI installed`); + console.log(`[${elapsed()}] nemoclaw CLI verified`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/brev-e2e.test.js` around lines 172 - 187, Remove the redundant second CLI install in the SSH command sequence inside the E2E test: the ssh(...) call currently runs `cd ${remoteDir}/nemoclaw && npm install && npm run build` plus a separate `cd ${remoteDir} && npm install --ignore-scripts && npm link`; since brev-setup.sh already performs the npm_config_prefix/npm link step, drop the latter install/link commands and only run the visibility check (`which nemoclaw && nemoclaw --version`) after the build step (or replace the whole ssh command block with a simpler visibility-only check), updating the ssh invocation in test/e2e/brev-e2e.test.js accordingly so the test no longer re-runs networked installs.
189-211: Assert the onboard-written registry instead of replacing it.
bin/lib/onboard.jsalready writes~/.nemoclaw/sandboxes.jsonduring successful onboarding. Replacing the file here means the suite stops checking that the new onboard-based bootstrap actually persisted the sandbox entry. If you need determinism, normalize only the volatile fields likecreatedAt.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/brev-e2e.test.js` around lines 189 - 211, Replace the hard overwrite of ~/.nemoclaw/sandboxes.json with an assertion that the onboard step actually persisted the registry: use the existing ssh(...) helper to cat ~/.nemoclaw/sandboxes.json, parse the JSON, assert that json.sandboxes['e2e-test'] exists and json.defaultSandbox === 'e2e-test', and when comparing to an expected object normalize volatile fields (e.g., set createdAt, model, nimContainer to fixed values or delete them) before doing deep-equality checks; update the code around the ssh(...) call and the elapsed() log to perform these reads/assertions instead of writing the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/nemoclaw.js`:
- Around line 623-629: Change setup so it forwards the CLI arguments into the
existing onboard parser instead of dropping them: update async function setup()
to accept an argv parameter (or use process.argv.slice(2) if not passed) and
call the onboard entry (the runOnboard export from ./lib/onboard) with that argv
so the onboard parser handles --non-interactive, --resume and unknown flags
(e.g. await runOnboard({ argv }) or await runOnboard({ argv, nonInteractive:
false, resume: false }) depending on runOnboard's signature); make the same
change to the duplicate setup alias at the other location so both deprecated
aliases forward args to runOnboard.
---
Nitpick comments:
In `@test/e2e/brev-e2e.test.js`:
- Around line 172-187: Remove the redundant second CLI install in the SSH
command sequence inside the E2E test: the ssh(...) call currently runs `cd
${remoteDir}/nemoclaw && npm install && npm run build` plus a separate `cd
${remoteDir} && npm install --ignore-scripts && npm link`; since brev-setup.sh
already performs the npm_config_prefix/npm link step, drop the latter
install/link commands and only run the visibility check (`which nemoclaw &&
nemoclaw --version`) after the build step (or replace the whole ssh command
block with a simpler visibility-only check), updating the ssh invocation in
test/e2e/brev-e2e.test.js accordingly so the test no longer re-runs networked
installs.
- Around line 189-211: Replace the hard overwrite of ~/.nemoclaw/sandboxes.json
with an assertion that the onboard step actually persisted the registry: use the
existing ssh(...) helper to cat ~/.nemoclaw/sandboxes.json, parse the JSON,
assert that json.sandboxes['e2e-test'] exists and json.defaultSandbox ===
'e2e-test', and when comparing to an expected object normalize volatile fields
(e.g., set createdAt, model, nimContainer to fixed values or delete them) before
doing deep-equality checks; update the code around the ssh(...) call and the
elapsed() log to perform these reads/assertions instead of writing the file.
🪄 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: 1c47e23f-9a56-42ff-8ef1-4f892e87570b
📒 Files selected for processing (8)
bin/nemoclaw.jsscripts/brev-setup.shscripts/setup.shscripts/walkthrough.shtest/e2e/brev-e2e.test.jstest/gateway-cleanup.test.jstest/runner.test.jstest/setup-sandbox-name.test.js
💤 Files with no reviewable changes (4)
- test/gateway-cleanup.test.js
- test/runner.test.js
- test/setup-sandbox-name.test.js
- scripts/setup.sh
jyaunches
approved these changes
Apr 1, 2026
wscurran
added
NemoClaw CLI
laitingsheng
pushed a commit
that referenced
this pull request
Apr 2, 2026
…ard (#1235) ## Summary - Delete `scripts/setup.sh` (324 lines) — every step it performed is already handled by `nemoclaw onboard` with better error recovery (exponential backoff via pRetry), interactive prompts, registry management, and WSL2-aware Ollama binding - `nemoclaw setup` now delegates to `onboard` instead of shelling out to the deleted script - `brev-setup.sh` installs the nemoclaw CLI and runs `nemoclaw onboard --non-interactive` instead of calling setup.sh - Remove setup.sh-specific tests and assertions (3 test removals across 3 files); all equivalent assertions against `onboard.js` already exist ## Test plan - [x] 541 CLI tests pass (`vitest run --project cli`) - [ ] Verify `nemoclaw setup` prints deprecation notice and runs onboard - [ ] Verify `brev-setup.sh` bootstraps correctly on a fresh Brev VM (E2E) Relates to #924 (shell consolidation). Supersedes #1230. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Refactor** * Migrated legacy setup to a unified onboarding flow and removed the old top-level setup orchestration. * **Chores** * Updated bootstrap to build/install the CLI locally and run onboarding non-interactively. * Adjusted setup walkthrough instructions to reference onboarding. * **Tests** * Removed tests tied to the removed setup script and sandbox-name behavior. * Added CLI dispatch tests verifying setup argument forwarding into onboarding. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Apr 2, 2026
lakamsani
pushed a commit
to lakamsani/NemoClaw
that referenced
this pull request
Apr 4, 2026
…ard (NVIDIA#1235) ## Summary - Delete `scripts/setup.sh` (324 lines) — every step it performed is already handled by `nemoclaw onboard` with better error recovery (exponential backoff via pRetry), interactive prompts, registry management, and WSL2-aware Ollama binding - `nemoclaw setup` now delegates to `onboard` instead of shelling out to the deleted script - `brev-setup.sh` installs the nemoclaw CLI and runs `nemoclaw onboard --non-interactive` instead of calling setup.sh - Remove setup.sh-specific tests and assertions (3 test removals across 3 files); all equivalent assertions against `onboard.js` already exist ## Test plan - [x] 541 CLI tests pass (`vitest run --project cli`) - [ ] Verify `nemoclaw setup` prints deprecation notice and runs onboard - [ ] Verify `brev-setup.sh` bootstraps correctly on a fresh Brev VM (E2E) Relates to NVIDIA#924 (shell consolidation). Supersedes NVIDIA#1230. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Refactor** * Migrated legacy setup to a unified onboarding flow and removed the old top-level setup orchestration. * **Chores** * Updated bootstrap to build/install the CLI locally and run onboarding non-interactively. * Adjusted setup walkthrough instructions to reference onboarding. * **Tests** * Removed tests tied to the removed setup script and sandbox-name behavior. * Added CLI dispatch tests verifying setup argument forwarding into onboarding. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.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
3 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
scripts/setup.sh(324 lines) — every step it performed is already handled bynemoclaw onboardwith better error recovery (exponential backoff via pRetry), interactive prompts, registry management, and WSL2-aware Ollama bindingnemoclaw setupnow delegates toonboardinstead of shelling out to the deleted scriptbrev-setup.shinstalls the nemoclaw CLI and runsnemoclaw onboard --non-interactiveinstead of calling setup.shonboard.jsalready existTest plan
vitest run --project cli)nemoclaw setupprints deprecation notice and runs onboardbrev-setup.shbootstraps correctly on a fresh Brev VM (E2E)Relates to #924 (shell consolidation). Supersedes #1230.
🤖 Generated with Claude Code
Summary by CodeRabbit
Refactor
Chores
Tests