fix: re-prompt on invalid sandbox name instead of exiting#1173
fix: re-prompt on invalid sandbox name instead of exiting#1173cv merged 6 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:
📝 WalkthroughWalkthrough
Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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)
test/onboard.test.js (1)
1578-1588: Source-level test verifies the retry loop exists, but the regex is formatting-sensitive.The regex
/isNonInteractive\(\)\)\s*\{\s*\n\s*process\.exit\(1\)/on line 1587 is tightly coupled to the current code formatting. If the code is reformatted (e.g., different whitespace between{andprocess.exit), the test will fail even though the behavior is correct.Consider a more resilient pattern that allows for any whitespace between the key tokens:
♻️ Suggested more flexible regex
- assert.match(source, /isNonInteractive\(\)\)\s*\{\s*\n\s*process\.exit\(1\)/); + assert.match(source, /isNonInteractive\(\)\)\s*\{[\s\S]*?process\.exit\(1\)/);Alternatively, consider splitting into two separate assertions (one for the condition, one for the exit call) to make failures more descriptive.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/onboard.test.js` around lines 1578 - 1588, The test's regex that checks for the non-interactive exit is fragile due to strict whitespace/newline matching; update the assertion in the "re-prompts on invalid sandbox names..." test to either (a) use a single, resilient pattern that allows any whitespace/newline between tokens (e.g., match isNonInteractive\(\).*process\.exit\(1\) with dot-all or [\s\S]* to span newlines) or (b) split into two assertions: one asserting the presence of isNonInteractive() and a second asserting process.exit(1) appears, so the test no longer depends on exact formatting; update the assertion referencing isNonInteractive() and process.exit(1) accordingly.
🤖 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/onboard.test.js`:
- Around line 1578-1588: The test's regex that checks for the non-interactive
exit is fragile due to strict whitespace/newline matching; update the assertion
in the "re-prompts on invalid sandbox names..." test to either (a) use a single,
resilient pattern that allows any whitespace/newline between tokens (e.g., match
isNonInteractive\(\).*process\.exit\(1\) with dot-all or [\s\S]* to span
newlines) or (b) split into two assertions: one asserting the presence of
isNonInteractive() and a second asserting process.exit(1) appears, so the test
no longer depends on exact formatting; update the assertion referencing
isNonInteractive() and process.exit(1) accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e85ef663-ac30-44df-ac4c-6e2b3a858df4
📒 Files selected for processing (2)
bin/lib/onboard.jstest/onboard.test.js
There was a problem hiding this comment.
Pull request overview
Updates the onboarding CLI flow to handle invalid sandbox-name input more gracefully by retrying interactively rather than terminating, addressing the “stuck on resume due to persisted bad name” problem described in #1120.
Changes:
- Updated
promptValidatedSandboxName()to retry on validation failure in interactive mode, while preservingprocess.exit(1)behavior for non-interactive runs. - Adjusted the temp-file test for sandbox sync script writing/cleanup.
- Added a source-level test asserting the new retry-loop behavior and non-interactive exit behavior remain present.
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
bin/lib/onboard.js |
Changes invalid sandbox-name handling to re-prompt interactively instead of exiting. |
test/onboard.test.js |
Updates temp script-file assertions/cleanup and adds a source-regex test for the new retry loop. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
test/onboard.test.js
Outdated
| // Verify the retry loop exists (while + re-prompt pattern) | ||
| assert.match(source, /while\s*\(true\)/); | ||
| assert.match(source, /Please try again/); | ||
| // Non-interactive still exits | ||
| assert.match(source, /isNonInteractive\(\)\)[\s\S]*?process\.exit\(1\)/); |
There was a problem hiding this comment.
This test is very brittle and can produce false positives/negatives:
/while\s*\(true\)/matches many loops inonboard.jsand doesn’t specifically assert thatpromptValidatedSandboxName()is the one that loops.- The
isNonInteractive()/process.exit(1)regex is not scoped to the sandbox-name branch and hard-codes\n, which can fail on CRLF checkouts.
To make this more reliable, scope the assertions to the promptValidatedSandboxName function body (e.g., extract/match a substring starting at async function promptValidatedSandboxName), and allow \r?\n (or avoid newline literals entirely).
| // Verify the retry loop exists (while + re-prompt pattern) | |
| assert.match(source, /while\s*\(true\)/); | |
| assert.match(source, /Please try again/); | |
| // Non-interactive still exits | |
| assert.match(source, /isNonInteractive\(\)\)[\s\S]*?process\.exit\(1\)/); | |
| const functionStart = source.indexOf("async function promptValidatedSandboxName"); | |
| assert.notEqual(functionStart, -1); | |
| const promptValidatedSandboxNameSource = source.slice(functionStart); | |
| // Verify the retry loop and re-prompt pattern exist within promptValidatedSandboxName | |
| assert.match( | |
| promptValidatedSandboxNameSource, | |
| /while\s*\(true\)[\s\S]*?Please try again/ | |
| ); | |
| // Non-interactive still exits within the same function | |
| assert.match( | |
| promptValidatedSandboxNameSource, | |
| /isNonInteractive\(\)[\s\S]*?process\.exit\(1\)/ | |
| ); |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/onboard.test.js (1)
1583-1588: Scope the retry-loop regex checks to the sandbox-name function block.Current assertions are file-wide and can pass on unrelated
while (true)orprocess.exit(1)occurrences, weakening this regression guard.♻️ Suggested tightening
- // Verify the retry loop exists (while + re-prompt pattern) - assert.match(source, /while\s*\(true\)/); - assert.match(source, /Please try again/); - // Non-interactive still exits - assert.match(source, /isNonInteractive\(\)\)[\s\S]*?process\.exit\(1\)/); + const sandboxNameFlow = source.match( + /(?:async\s+)?function\s+promptValidatedSandboxName\s*\([^)]*\)\s*\{[\s\S]*?\n\}/ + )?.[0]; + assert.ok(sandboxNameFlow, "expected promptValidatedSandboxName implementation"); + assert.match(sandboxNameFlow, /while\s*\(true\)/); + assert.match(sandboxNameFlow, /Please try again/); + assert.match( + sandboxNameFlow, + /if\s*\(isNonInteractive\(\)\)\s*\{[\s\S]*?process\.exit\(1\)/ + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/onboard.test.js` around lines 1583 - 1588, The three loose regex assertions operate on the whole file; restrict them to the "sandbox-name" function block by first extracting that block from source (e.g. locate the function or block labeled "sandbox-name" via a regex that captures its body into a substring) and then run the three assert.match checks against that substring instead of source; update the assertions that reference source to reference the extracted block and keep the same patterns (/while\s*\(true\)/, /Please try again/, /isNonInteractive\(\)\)[\s\S]*?process\.exit\(1\)/) so they only validate the retry-loop and non-interactive exit inside the sandbox-name function.
🤖 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/onboard.test.js`:
- Around line 1583-1588: The three loose regex assertions operate on the whole
file; restrict them to the "sandbox-name" function block by first extracting
that block from source (e.g. locate the function or block labeled "sandbox-name"
via a regex that captures its body into a substring) and then run the three
assert.match checks against that substring instead of source; update the
assertions that reference source to reference the extracted block and keep the
same patterns (/while\s*\(true\)/, /Please try again/,
/isNonInteractive\(\)\)[\s\S]*?process\.exit\(1\)/) so they only validate the
retry-loop and non-interactive exit inside the sandbox-name function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f30feffd-da47-4255-89d6-1c963d4fc28e
📒 Files selected for processing (1)
test/onboard.test.js
BenediktSchackenberg
force-pushed
the
fix/sandbox-name-validation-retry
branch
from
4efc26f to
fe6cc05
Compare
Head branch was pushed to by a user without write access
BenediktSchackenberg
force-pushed
the
fix/sandbox-name-validation-retry
branch
from
f64d49e to
9b13e79
Compare
|
@BenediktSchackenberg sorry about the formatting churn! We accidentally left prettier out of a good chunk of the code, and turning it on caused all sorts of merge issues. Hopefully that's not too painful to resolve! |
Add secureTempFile(prefix, ext) helper using fs.mkdtempSync() to create temp files inside OS-level unique directories, preventing symlink attacks on predictable /tmp paths. Add cleanupTempDir(filePath, expectedPrefix) guard that verifies the parent directory matches the expected mkdtemp prefix before calling fs.rmSync recursive, preventing accidental deletion of the system temp root on regression. Changes: - runCurlProbe: replace Date.now()+Math.random() with secureTempFile - writeSandboxConfigSyncFile: use secureTempFile, drop tmpDir param - All cleanup sites use cleanupTempDir guard - Test updated for new function signature and mkdtemp assertions Closes NVIDIA#1093
When a user enters an invalid sandbox name (e.g. starting with a digit), promptValidatedSandboxName() now shows the error and re-prompts instead of calling process.exit(1). This prevents users from getting stuck in a loop where rerunning the onboard script reuses the invalid name from the session state, requiring manual deletion of ~/.nemoclaw. In non-interactive mode (CI/CD), the function still exits with status 1 since re-prompting is not possible. Closes NVIDIA#1120
Address Copilot + CodeRabbit feedback: extract the function body first, then assert within it. Prevents false positives from unrelated while(true) or process.exit(1) elsewhere in the file.
Signed-off-by: Benedikt Schackenberg <[email protected]>
BenediktSchackenberg
force-pushed
the
fix/sandbox-name-validation-retry
branch
from
2fbee5f to
7c4805c
Compare
|
No worries! Rebased onto latest main and resolved the conflicts. The prettier formatting is now clean — only a few lines needed adjustment since most of the file was already formatted upstream. Also re-verified all commits are signed. Should be good to go! 🙂 |
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
CI checks job failed because the // eslint-disable-next-line no-constant-condition directive is unused — the current ESLint config doesn't flag while (true). When CI runs eslint --fix, it auto-removes the directive, which triggers the "Files were modified by following hooks" failure. See suggestion below.
## Summary When a user enters an invalid sandbox name (e.g. starting with a digit like `7racii`), `promptValidatedSandboxName()` now shows the error and **re-prompts** instead of calling `process.exit(1)`. This prevents users from getting stuck in a loop where rerunning the onboard script reuses the invalid name from the session state, requiring manual deletion of `~/.nemoclaw` to recover. ## Changes - **`bin/lib/onboard.js`**: Replace exit-on-invalid with a `while(true)` retry loop in interactive mode. Non-interactive mode (CI/CD) still exits with status 1. - **`test/onboard.test.js`**: Source-level test verifying the retry loop exists and non-interactive fallback is preserved. ## Before ``` Sandbox name: 7racii Invalid sandbox name: '7racii' Names must be lowercase... # process.exit(1) — rerun uses same name from session, stuck ``` ## After ``` Sandbox name: 7racii Invalid sandbox name: '7racii' Names must be lowercase... Please try again. Sandbox name: my-racii ✓ Valid name accepted ``` Closes #1120 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Sandbox name validation now allows interactive users to retry on invalid input with a clear "Please try again" prompt; non-interactive runs still terminate on invalid names. * **Tests** * Added and updated tests to verify interactive retry vs non-interactive exit and to strengthen temporary-file handling assertions (more flexible script naming and safer cleanup). <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Benedikt Schackenberg <[email protected]> --------- Signed-off-by: Benedikt Schackenberg <[email protected]> Co-authored-by: Carlos Villela <[email protected]> Co-authored-by: Carlos Villela <[email protected]>
Summary
When a user enters an invalid sandbox name (e.g. starting with a digit like
7racii),promptValidatedSandboxName()now shows the error and re-prompts instead of callingprocess.exit(1).This prevents users from getting stuck in a loop where rerunning the onboard script reuses the invalid name from the session state, requiring manual deletion of
~/.nemoclawto recover.Changes
bin/lib/onboard.js: Replace exit-on-invalid with awhile(true)retry loop in interactive mode. Non-interactive mode (CI/CD) still exits with status 1.test/onboard.test.js: Source-level test verifying the retry loop exists and non-interactive fallback is preserved.Before
After
Closes #1120
Summary by CodeRabbit
Bug Fixes
Tests
Signed-off-by: Benedikt Schackenberg [email protected]