fix(cli): prompt before reusing existing sandbox on re-onboard#1527
fix(cli): prompt before reusing existing sandbox on re-onboard#1527ericksoa merged 3 commits intoNVIDIA:mainfrom
Conversation
|
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:
📝 WalkthroughWalkthroughAdds a Changes
Sequence DiagramsequenceDiagram
participant User as User/CLI
participant CLI as nemoclaw.js
participant Onboard as onboard.js
participant Sandbox as Sandbox System
User->>CLI: Run `nemoclaw onboard` (maybe --recreate-sandbox)
CLI->>CLI: Parse args → {recreateSandbox, nonInteractive, ...}
CLI->>Onboard: runOnboard(opts)
Onboard->>Onboard: Determine isRecreateSandbox() from opts/env
Onboard->>Sandbox: Check existing sandbox state & needsProviderMigration
alt isRecreateSandbox() OR needsProviderMigration
Onboard->>User: note "Deleting and recreating sandbox '${sandboxName}'..."
Onboard->>Sandbox: Delete existing sandbox
Onboard->>Sandbox: Create sandbox
else !isRecreateSandbox() AND !needsProviderMigration
alt Interactive
Onboard->>User: Prompt "Reuse existing sandbox? [Y/n]"
alt User = n
Onboard->>User: note "Deleting and recreating sandbox '${sandboxName}'..."
Onboard->>Sandbox: Delete and Create
else User = y
Onboard->>Sandbox: Upsert messaging providers
Onboard->>Sandbox: Forward dashboard
Onboard-->>User: Return sandbox name
end
else Non-Interactive
alt Sandbox state == ready
Onboard->>Sandbox: Upsert messaging providers
Onboard->>Sandbox: Forward dashboard
Onboard-->>User: Return sandbox name
else Sandbox not ready
Onboard->>User: Print error & hint about --recreate-sandbox
Onboard->>Onboard: process.exit(1)
end
end
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 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}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
bin/lib/onboard.js (1)
180-188: Minor redundancy inisRecreateSandbox()env check.At line 3647,
RECREATE_SANDBOXis set fromopts.recreateSandbox || process.env.NEMOCLAW_RECREATE_SANDBOX === "1", so the env var check inisRecreateSandbox()is redundant—if the env is"1",RECREATE_SANDBOXis alreadytrue.The pattern matches
isNonInteractive(), so this is consistent within the file. Consider simplifying if you want to reduce duplication, but it's not harmful as-is.🔧 Optional simplification
function isRecreateSandbox() { - return RECREATE_SANDBOX || process.env.NEMOCLAW_RECREATE_SANDBOX === "1"; + return RECREATE_SANDBOX; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 180 - 188, The isRecreateSandbox() helper redundantly re-checks process.env.NEMOCLAW_RECREATE_SANDBOX === "1" even though RECREATE_SANDBOX is already initialized from opts.recreateSandbox || process.env.NEMOCLAW_RECREATE_SANDBOX === "1"; update isRecreateSandbox() to simply return RECREATE_SANDBOX (remove the duplicate env lookup) so the function mirrors isNonInteractive()’s simplicity and relies on the single source of truth set when RECREATE_SANDBOX is initialized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@bin/lib/onboard.js`:
- Around line 180-188: The isRecreateSandbox() helper redundantly re-checks
process.env.NEMOCLAW_RECREATE_SANDBOX === "1" even though RECREATE_SANDBOX is
already initialized from opts.recreateSandbox ||
process.env.NEMOCLAW_RECREATE_SANDBOX === "1"; update isRecreateSandbox() to
simply return RECREATE_SANDBOX (remove the duplicate env lookup) so the function
mirrors isNonInteractive()’s simplicity and relies on the single source of truth
set when RECREATE_SANDBOX is initialized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cf0f4bda-ad50-47c8-883d-a2c0a79c08a6
📒 Files selected for processing (2)
bin/lib/onboard.jsbin/nemoclaw.js
wscurran
added
Getting Started
|
✨ Thanks for submitting this fix, which proposes a way to prompt before reusing an existing sandbox during re-onboarding, improving safety and clarity in the setup flow. |
paritoshd-nv
force-pushed
the
fix/1102-recreate-sandbox-prompt
branch
from
058101f to
0e9bded
Compare
ericksoa
left a comment
ericksoa
left a comment
There was a problem hiding this comment.
Thanks for tackling this, @paritoshd-nv — the non-interactive error-on-non-ready behavior is a solid improvement over the silent overwrite. A few things to consider:
Interactive path and sandbox state
The non-interactive path correctly distinguishes ready vs not-ready, but the interactive prompt offers "Reuse existing sandbox?" regardless of state. What happens if the user says "yes" to reuse a sandbox that isn't ready? Would it be worth either skipping the prompt (go straight to recreation) or surfacing the state so the user can make an informed choice?
Input parsing
The check answer.trim().toLowerCase() !== "n" means typing "no" is treated as reuse. Would .startsWith("n") be safer here?
Unrelated change in isNonInteractive()
The added process.env.NEMOCLAW_NON_INTERACTIVE === "1" check is already handled at the top of onboard() where NON_INTERACTIVE is set. Could you revert this to keep the PR scoped to the sandbox prompt change?
Tests
Could you add unit tests for the new branching logic? Specifically the interactive ready/not-ready paths and the non-interactive error exit.
The tests call createSandbox() directly without going through onboard(), so NON_INTERACTIVE stays false. |
paritoshd-nv
force-pushed
the
fix/1102-recreate-sandbox-prompt
branch
from
6ffc19b to
79feed8
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/lib/onboard.js`:
- Around line 2009-2011: Update the non-interactive reuse message to include a
recreate hint: when the branch that calls note(` [non-interactive] Sandbox
'${sandboxName}' exists and is ready — reusing it`) executes (the same place
that calls ensureDashboardForward(sandboxName, chatUiUrl) and returns
sandboxName), append or alter the note text to mention the CLI flag and env var
for forcing recreation (e.g. `--recreate-sandbox` /
`NEMOCLAW_RECREATE_SANDBOX=1`) so users know how to recreate the sandbox in
non-interactive mode.
- Around line 2020-2022: The prompt handling for sandbox reuse only treats "n"
as a negative; update the logic around the promptOrDefault call so answers of
"no" (case-insensitive, trimmed) are also treated as negative. Locate the
variable answer from promptOrDefault and change the conditional that currently
checks answer.trim().toLowerCase() !== "n" to instead reject both "n" and "no"
(for example by normalizing answer and checking it is not "n" and not "no" or by
using an inclusion check), so upsertMessagingProviders(messagingTokenDefs) is
only called when the user really chose to reuse the sandbox.
In `@test/onboard.test.js`:
- Around line 2215-2219: The test currently spreads process.env into the child
env when calling spawnSync (see the env: { ...process.env, HOME: tmpDir, PATH:
`${fakeBin}:${process.env.PATH || ""}` } block), which can inherit
NEMOCLAW_NON_INTERACTIVE or NEMOCLAW_RECREATE_SANDBOX and break interactive path
coverage; fix by constructing a fresh env object (copy needed keys or start from
Object.assign({}, process.env)), then explicitly delete
env.NEMOCLAW_NON_INTERACTIVE and env.NEMOCLAW_RECREATE_SANDBOX before using it
in the spawnSync invocation so the child process always runs the interactive
branch.
- Around line 2010-2056: The test currently only asserts exit code and hint,
which can miss destructive side-effects; update the test so runner.run throws
when called with a "sandbox delete" command (patch runner.run to throw on
delete), stub child_process.spawn (or spawnSync) used by createSandbox to fail
on any sandbox creation attempt, and ensure the env passed to the spawned script
removes NEMOCLAW_RECREATE_SANDBOX so the test exercises the non-recreate branch;
target the runner.run, runner.runCapture, createSandbox invocation and the
child_process.spawn stub to enforce immediate failure on destructive commands
and prevent masked deletions.
🪄 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: 1ec2f39c-1fad-4c94-b98d-ddca48485726
📒 Files selected for processing (3)
bin/lib/onboard.jsbin/nemoclaw.jstest/onboard.test.js
paritoshd-nv
force-pushed
the
fix/1102-recreate-sandbox-prompt
branch
from
d5cf62b to
ed401f4
Compare
Hi @ericksoa , thanks for the review. Added tests for the new branching logic. Please note that new tests add ~1min of time in overall tests. In the last comment, I have mentioned about the need for : |
ericksoa
left a comment
ericksoa
left a comment
There was a problem hiding this comment.
Clean PR — the --recreate-sandbox flag, non-interactive error-on-not-ready, and interactive prompt are all good additions. Tests are thorough and follow existing patterns. One thing worth discussing:
Interactive mode doesn't guard on sandbox readiness
The original code only offered reuse when existingSandboxState === "ready". The new code drops that guard in the interactive path:
if (!isRecreateSandbox() && !needsProviderMigration) {
if (isNonInteractive()) {
if (existingSandboxState === "ready") { ... } // ← guarded
// not ready → exit(1) // ← guarded
}
// interactive: prompts for ANY existing sandbox, including not-ready
console.log(` Sandbox '${sandboxName}' already exists.`);
const answer = await promptOrDefault(" Reuse existing sandbox? [Y/n]: ", ...);
if (answer !== "n") {
return sandboxName; // ← returns a possibly non-ready sandbox
}
}Non-interactive handles this correctly (errors if not ready). But in interactive mode, a user could say "y" to reuse a sandbox stuck in NotReady/Creating — which would return without a working sandbox.
Question: Should the interactive prompt also check existingSandboxState === "ready" before offering reuse? Or at minimum show the current state so the user knows what they're agreeing to?
There's also no test for the interactive + not-ready edge case (the other three quadrants are covered). Adding one would close that gap.
paritoshd-nv
force-pushed
the
fix/1102-recreate-sandbox-prompt
branch
2 times, most recently
from
6416db0 to
0f9eefa
Compare
That was miss from my part. Now, added the "interactive and not-ready sandbox" case with explicitly confirmation from user. |
Add --recreate-sandbox CLI flag and interactive confirmation prompt when a sandbox with the same name already exists during onboarding. In non-interactive mode, ready sandboxes are reused with a hint about the new flag; non-ready sandboxes now exit with an error instead of silently recreating. Closes NVIDIA#1102 Signed-off-by: Paritosh Dixit <paritoshd@nvidia.com>
Guard the interactive path on sandbox readiness. When a sandbox exists but is not ready, prompt the user to confirm deletion and recreation instead of silently proceeding. Selecting n aborts onboarding. Add test for the interactive + not-ready edge case. Signed-off-by: Paritosh Dixit <paritoshd@nvidia.com>
paritoshd-nv
force-pushed
the
fix/1102-recreate-sandbox-prompt
branch
from
0f9eefa to
e134f9a
Compare
|
Hi @ericksoa, Due to new PR merges in the main, there were merge conflicts in this PR today morning. |
ericksoa
left a comment
ericksoa
left a comment
There was a problem hiding this comment.
Looks good — the interactive readiness guard, "no" handling, and test coverage all address the earlier feedback. Approving.
One nit for a follow-up PR: the isNonInteractive() change (adding || process.env.NEMOCLAW_NON_INTERACTIVE === "1") is still here — that's already handled where NON_INTERACTIVE is initialized in onboard(), so it's redundant and out of scope for this PR. Same applies to the matching pattern in isRecreateSandbox(). Could you clean both up in a follow-up?
It was needed to run the tests (without this change tests were failing): |
When openshell sandbox create exits with SSH 255 after printing "Created sandbox:", NemoClaw previously hard-exited instead of checking whether the sandbox reached Ready state. This was a regression from the create-stream extraction (#1516) combined with the messaging provider migration path (#1081, #1527) that forces sandbox recreation. Two fixes: - streamSandboxCreate: do one final readyCheck on non-zero close to catch the race where the sandbox is already Ready when SSH dies. - onboard.js: when failure is sandbox_create_incomplete, fall through to the existing ready-wait loop (60s polling) instead of exiting. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
…1598) ## Summary - When `openshell sandbox create` exits with SSH 255 after printing "Created sandbox:", NemoClaw now treats this as recoverable instead of hard-exiting - Added a final `readyCheck` in `streamSandboxCreate` on non-zero close to catch the race where the sandbox is already Ready when SSH dies - In `onboard.js`, `sandbox_create_incomplete` failures now fall through to the existing 60s ready-wait loop instead of calling `process.exit()` ## Root cause Regression from #1516 (create-stream extraction) combined with #1081/#1527 (messaging provider migration forcing sandbox recreation). The create stream returns non-zero after "Created sandbox:" and NemoClaw exits before checking if the sandbox reached Ready state. ## Test plan - [x] New unit tests: stream recovers when readyCheck is true at close time (SSH 255) - [x] New unit test: stream still returns non-zero when readyCheck is false at close time - [x] All existing `sandbox-create-stream` tests pass (7/7) - [x] All onboard integration tests pass (83/83) - [x] All src unit tests pass (538/538) - [ ] Manual: trigger SSH 255 during sandbox create and verify recovery <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Enhanced sandbox creation error handling with improved failure classification and recovery messaging. * Improved sandbox readiness detection to prevent unnecessary creation failures when the sandbox is operational. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
3 participants
Add --recreate-sandbox CLI flag and interactive confirmation prompt when a sandbox with the same name already exists during onboarding. In non-interactive mode, ready sandboxes are reused with a hint about the new flag; non-ready sandboxes now exit with an error instead of silently recreating.
Closes #1102
Summary
Related Issue
Changes
Type of Change
Testing
npx prek run --all-filespasses (or equivalentlymake check).npm testpasses.make docsbuilds without warnings. (for doc-only changes)Checklist
General
Code Changes
npx prek run --all-filesauto-fixes formatting (ormake formatfor targeted runs).Doc Changes
update-docsagent skill to draft changes while complying with the style guide. For example, prompt your agent with "/update-docscatch up the docs for the new changes I made in this PR."Signed-off-by: Your Name your-email@example.com
Summary by CodeRabbit
New Features
Improvements
Tests