fix: re-prompt on invalid sandbox name instead of exiting#1173
Open
BenediktSchackenberg wants to merge 4 commits intoNVIDIA:mainfrom
Open
fix: re-prompt on invalid sandbox name instead of exiting#1173BenediktSchackenberg wants to merge 4 commits intoNVIDIA:mainfrom
BenediktSchackenberg wants to merge 4 commits intoNVIDIA:mainfrom
Conversation
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
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 (1)
📝 WalkthroughWalkthroughUpdated onboarding sandbox-name validation to loop and re-prompt interactively on invalid RFC 1123 subdomain names; in non-interactive mode the script still exits immediately. Tests were updated to reflect retry behavior and to change temporary-file handling assertions. 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}
Contributor
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
Address CodeRabbit nitpick: use [\s\S]*? instead of strict whitespace/newline matching so the assertion survives reformatting.
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
Comment on lines
+1583
to
+1587
| // 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).
Suggested change
| // 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\)/ | |
| ); |
Contributor
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
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.
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
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
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