refactor(cli): extract sandbox create stream from onboard.js#1516
Merged
cv merged 10 commits intoNVIDIA:mainfrom Apr 6, 2026
Merged
refactor(cli): extract sandbox create stream from onboard.js#1516cv merged 10 commits intoNVIDIA:mainfrom
cv merged 10 commits intoNVIDIA:mainfrom
Conversation
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughExtracted the inline sandbox creation streaming logic from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
![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: 5
🧹 Nitpick comments (4)
test/credential-exposure.test.js (1)
85-95: Keep the timeout assertions in thehttp-probesuite.This file is a credential-leak regression, but these checks are now about curl-timeout wiring. They duplicate
src/lib/http-probe.test.tsand make unrelated probe refactors fail a security-focused test. I’d move the timeout expectations there and keep this file centered on argv leakage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/credential-exposure.test.js` around lines 85 - 95, The timeout assertions in credential-exposure.test.js are misplaced: remove the two expect(probeSrc).toMatch(/"--connect-timeout", "10"/) and expect(probeSrc).toMatch(/"--max-time", "60"/) checks from the "onboard curl probes use explicit timeouts" test (which reads ONBOARD_JS and probeSrc) and add equivalent assertions into the http-probe test suite (src/lib/http-probe.test.ts) so that http-probe timeout wiring is validated there while credential-exposure.test.js remains focused on argv/credential leakage.bin/lib/onboard.js (1)
1164-1165: ImportBACK_TO_SELECTIONfrommodel-promptsas well.
onboard.jsstill compares prompt results against its own local sentinel. Keeping that magic value duplicated across the module boundary means a future change insrc/lib/model-prompts.tssilently breaks every=== BACK_TO_SELECTIONbranch here. Single-source it from the extracted module.♻️ Collapse the sentinel to one source of truth
-const BACK_TO_SELECTION = "__NEMOCLAW_BACK_TO_SELECTION__"; @@ -const { promptManualModelId, promptCloudModel, promptRemoteModel, promptInputModel } = modelPrompts; +const { BACK_TO_SELECTION, promptManualModelId, promptCloudModel, promptRemoteModel, promptInputModel } = modelPrompts;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 1164 - 1165, Import BACK_TO_SELECTION from modelPrompts and stop using a local duplicate sentinel: update the destructuring that currently reads "const { promptManualModelId, promptCloudModel, promptRemoteModel, promptInputModel } = modelPrompts;" to also include BACK_TO_SELECTION, remove any local BACK_TO_SELECTION constant, and change all direct comparisons that used the local magic value to compare against the imported BACK_TO_SELECTION (e.g., in handlers that check prompt results). This ensures the sentinel is single-sourced from model-prompts and prevents future breakage when the sentinel value changes.test/onboard.test.js (1)
1243-1259: Avoid source-scanning raw TS internals from the CLI regression tests.These assertions hard-code exact implementation text from
src/lib/*, so harmless refactors or formatting changes fail the test while the compileddist/lib/*code thatonboard.jsactually requires still goes unexercised. The new module-level tests already cover the behavior; here I’d prefer wiring checks against exported helpers or the builtdistmodules.Also applies to: 1302-1320
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/onboard.test.js` around lines 1243 - 1259, The test is brittle because it reads raw TS source (onboardSource, probeSource, recoverySource) and matches exact internals; instead load and assert against the built/consumed artifacts or exported helpers: require/import the compiled dist modules (the runtime onboard.js and dist/lib/http-probe.js / dist/lib/validation-recovery.js) or directly import the exported functions from src/lib/http-probe.ts and src/lib/validation-recovery.ts and assert their observable behavior or exported values (e.g., call the timeout-producing helper in http-probe and assert it returns the expected args, call the recovery helper to assert it checks/handles curl status 2 or returns the expected error marker) rather than matching source text; replace assert.match(onboardSource/probeSource/recoverySource, ...) with assertions against those exported functions or the dist module strings.src/lib/http-probe.ts (1)
96-97: Keep the complexity rule enabled here.This new helper starts with the repo’s
complexitycheck suppressed, which weakens the guardrail on exactly the kind of orchestration code that tends to accumulate branches over time. If lint is actually tripping here, please split the spawn/error formatting into a small helper instead of disabling the rule.As per coding guidelines, "Enforce cyclomatic complexity limit of 20 (ratcheting down to 15) in JavaScript/TypeScript code via ESLint".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/http-probe.ts` around lines 96 - 97, Remove the "// eslint-disable-next-line complexity" suppression from the runCurlProbe function and refactor to reduce cyclomatic complexity: extract conditional branches and error/exit formatting into one or more small helper functions (e.g., spawnCurlProcess, formatCurlError, parseCurlResult) and call them from runCurlProbe so the main function stays under the complexity threshold while preserving existing behavior and options handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/model-prompts.test.ts`:
- Around line 14-17: The helper promptSequence returns an empty string when the
queue is exhausted which can mask extra prompt cycles; update promptSequence
(the queue variable and the vi.fn async callback) to throw a clear error when
queue.shift() would be undefined (e.g., throw new Error("promptSequence
exhausted")) instead of returning "", so tests fail fast on unexpected extra
prompts.
In `@src/lib/model-prompts.ts`:
- Around line 27-30: The validator's optional message
(PromptValidationResult.message) is interpolated directly in multiple places
causing "undefined" to appear when a validator returns { ok: false } without a
message; update the code paths that render validator output (notably
promptInputModel and the other validator branches) to guard the message with a
fallback (e.g. use a default string like an empty string or a generic error
message when message is undefined) before interpolation so the UI never prints
"undefined"; locate usages of PromptValidationResult, promptInputModel, and the
validator result handling and replace direct interpolation of result.message
with a guarded expression that falls back to a safe string.
In `@src/lib/provider-models.ts`:
- Around line 17-30: ValidateModelResult currently drops structured http/curl
status info (it only has message/validated), and FetchModelsFailure renames
httpStatus to status; restore and propagate httpStatus?: number and curlStatus?:
number (or keep the same field names used by runCurlProbe/ValidationFailureLike)
on ValidateModelResult and ensure all code paths that return ok: false for
validation or fetch include those fields instead of flattening into message;
update places that construct FetchModelsFailure/ValidateModelResult to pass
through httpStatus/curlStatus (and map any existing status -> httpStatus) so
downstream callers can handle 429/5xx/curl failures cleanly.
In `@src/lib/sandbox-create-stream.ts`:
- Around line 161-168: The finish function currently flushes pending but
resolves using a pre-flush snapshot (output: lines.join("\n") and sawProgress),
which can drop the final partial line; modify finish (and the other resolve
paths around the same pattern at the other two locations) so that after calling
flushLine(pending) you rebuild the result payload from the current buffer (e.g.,
recompute output as lines.join("\n") and recompute sawProgress) and then pass
that rebuilt StreamSandboxCreateResult into resolvePromise; ensure you reference
the existing symbols finish, pending, flushLine, lines, sawProgress, and
resolvePromise when making the change.
- Around line 69-70: currentPhase is initialized to "build" which makes the
later setPhase("build") call a no-op and prevents the initial progress banner
from printing; change the initial currentPhase sentinel to a non-"build" value
(e.g., null/undefined or a dedicated "none" state) so setPhase("build") will run
its logic and print the banner. Update the declaration of currentPhase (type
CreatePhase | null if using null) and ensure any other places that re-initialize
phase (the similar blocks around the other setPhase calls referenced) use the
same sentinel approach so initial setPhase invocations are not skipped.
---
Nitpick comments:
In `@bin/lib/onboard.js`:
- Around line 1164-1165: Import BACK_TO_SELECTION from modelPrompts and stop
using a local duplicate sentinel: update the destructuring that currently reads
"const { promptManualModelId, promptCloudModel, promptRemoteModel,
promptInputModel } = modelPrompts;" to also include BACK_TO_SELECTION, remove
any local BACK_TO_SELECTION constant, and change all direct comparisons that
used the local magic value to compare against the imported BACK_TO_SELECTION
(e.g., in handlers that check prompt results). This ensures the sentinel is
single-sourced from model-prompts and prevents future breakage when the sentinel
value changes.
In `@src/lib/http-probe.ts`:
- Around line 96-97: Remove the "// eslint-disable-next-line complexity"
suppression from the runCurlProbe function and refactor to reduce cyclomatic
complexity: extract conditional branches and error/exit formatting into one or
more small helper functions (e.g., spawnCurlProcess, formatCurlError,
parseCurlResult) and call them from runCurlProbe so the main function stays
under the complexity threshold while preserving existing behavior and options
handling.
In `@test/credential-exposure.test.js`:
- Around line 85-95: The timeout assertions in credential-exposure.test.js are
misplaced: remove the two expect(probeSrc).toMatch(/"--connect-timeout", "10"/)
and expect(probeSrc).toMatch(/"--max-time", "60"/) checks from the "onboard curl
probes use explicit timeouts" test (which reads ONBOARD_JS and probeSrc) and add
equivalent assertions into the http-probe test suite
(src/lib/http-probe.test.ts) so that http-probe timeout wiring is validated
there while credential-exposure.test.js remains focused on argv/credential
leakage.
In `@test/onboard.test.js`:
- Around line 1243-1259: The test is brittle because it reads raw TS source
(onboardSource, probeSource, recoverySource) and matches exact internals;
instead load and assert against the built/consumed artifacts or exported
helpers: require/import the compiled dist modules (the runtime onboard.js and
dist/lib/http-probe.js / dist/lib/validation-recovery.js) or directly import the
exported functions from src/lib/http-probe.ts and src/lib/validation-recovery.ts
and assert their observable behavior or exported values (e.g., call the
timeout-producing helper in http-probe and assert it returns the expected args,
call the recovery helper to assert it checks/handles curl status 2 or returns
the expected error marker) rather than matching source text; replace
assert.match(onboardSource/probeSource/recoverySource, ...) with assertions
against those exported functions or the dist module strings.
🪄 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: bfd87508-deed-4ed1-a0ee-7d76caa61e07
📒 Files selected for processing (13)
bin/lib/onboard.jssrc/lib/http-probe.test.tssrc/lib/http-probe.tssrc/lib/model-prompts.test.tssrc/lib/model-prompts.tssrc/lib/provider-models.test.tssrc/lib/provider-models.tssrc/lib/sandbox-create-stream.test.tssrc/lib/sandbox-create-stream.tssrc/lib/validation-recovery.test.tssrc/lib/validation-recovery.tstest/credential-exposure.test.jstest/onboard.test.js
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/lib/http-probe.ts (2)
146-158: Consider extracting duplicated status extraction logic.Lines 151 and 155 repeat the same pattern for extracting status from the error object.
♻️ Extract to helper variable
} catch (error) { const detail = error instanceof Error ? error.message : String(error); + const errStatus = typeof error === "object" && error && "status" in error ? Number(error.status) || 1 : 1; return { ok: false, httpStatus: 0, - curlStatus: typeof error === "object" && error && "status" in error ? Number(error.status) || 1 : 1, + curlStatus: errStatus, body: "", stderr: detail, - message: summarizeCurlFailure( - typeof error === "object" && error && "status" in error ? Number(error.status) || 1 : 1, - detail, - ), + message: summarizeCurlFailure(errStatus, detail), }; } finally {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/http-probe.ts` around lines 146 - 158, In the catch block of src/lib/http-probe.ts where you build the failure response, extract the duplicated logic that computes the numeric status from the thrown error into a single helper variable (e.g., const curlStatusFromError = typeof error === "object" && error && "status" in error ? Number(error.status) || 1 : 1) and then reuse that variable for both the curlStatus field and the call to summarizeCurlFailure; update the returned object fields (curlStatus and the summarizeCurlFailure invocation) to reference this new variable and remove the duplicate expressions.
54-57: Document the URL-last convention or make it explicit.
splitCurlArgsassumes the URL is always the last element inargv. While current callers follow this pattern (per context snippets), this implicit contract could lead to subtle bugs if future callers place URL elsewhere.Consider adding a JSDoc comment documenting this requirement, or accepting the URL as a separate parameter.
📝 Option: Document the contract
+/** + * Splits curl arguments, extracting the URL which must be the last element. + * Callers must ensure URL is passed as the final argv element. + */ function splitCurlArgs(argv: readonly string[]): { args: string[]; url: string } { const args = [...argv]; return { args, url: String(args.pop() || "") }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/http-probe.ts` around lines 54 - 57, splitCurlArgs currently assumes the URL is the last element of argv which is an implicit contract that can break callers; either document that contract with a JSDoc on splitCurlArgs stating "argv must have the URL as the last element" and/or validate the last item (e.g., simple URL pattern check and throw a clear error if missing), or change the signature to accept the URL as an explicit second parameter (e.g., splitCurlArgs(argv, url) or splitCurlArgsWithUrl) and update its callers to pass the URL explicitly so the contract is explicit and enforced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/lib/http-probe.ts`:
- Around line 146-158: In the catch block of src/lib/http-probe.ts where you
build the failure response, extract the duplicated logic that computes the
numeric status from the thrown error into a single helper variable (e.g., const
curlStatusFromError = typeof error === "object" && error && "status" in error ?
Number(error.status) || 1 : 1) and then reuse that variable for both the
curlStatus field and the call to summarizeCurlFailure; update the returned
object fields (curlStatus and the summarizeCurlFailure invocation) to reference
this new variable and remove the duplicate expressions.
- Around line 54-57: splitCurlArgs currently assumes the URL is the last element
of argv which is an implicit contract that can break callers; either document
that contract with a JSDoc on splitCurlArgs stating "argv must have the URL as
the last element" and/or validate the last item (e.g., simple URL pattern check
and throw a clear error if missing), or change the signature to accept the URL
as an explicit second parameter (e.g., splitCurlArgs(argv, url) or
splitCurlArgsWithUrl) and update its callers to pass the URL explicitly so the
contract is explicit and enforced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 84f88838-7171-4edc-ad3c-6bc0e6f24b02
📒 Files selected for processing (4)
bin/lib/onboard.jssrc/lib/http-probe.test.tssrc/lib/http-probe.tssrc/lib/provider-models.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/http-probe.test.ts
Contributor
Author
|
Merged latest |
brandonpelfrey
approved these changes
Apr 6, 2026
tranzmatt
pushed a commit
to tranzmatt/NemoClaw
that referenced
this pull request
Apr 6, 2026
…1516) ## Summary - move sandbox-create progress streaming into `src/lib/sandbox-create-stream.ts` - keep `onboard.js` focused on sandbox orchestration and recovery flow - add focused tests for immediate phase feedback, buffered-tail flushing, forced-ready resolution, and spawn-error handling ## Notes - merged `main` into this branch in `4b6be62`, so the PR now contains only the sandbox-create-stream extraction - fixed the extracted helper so the initial build banner prints immediately - fixed the helper to rebuild the returned result after flushing any pending partial line - updated the CLI regression test to assert delegation to the extracted helper module instead of matching raw helper internals in `onboard.js` ## Testing - `npm run build:cli` - `npx vitest run --project cli src/lib/sandbox-create-stream.test.ts test/onboard.test.js` - `npm test` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Refactor** * Extracted sandbox-creation streaming behavior into a shared, reusable module to centralize progress, readiness, heartbeat, and error handling. * **Tests** * Added a comprehensive test suite covering progress parsing, readiness detection, heartbeat messages, partial-line handling, and spawn errors. * Updated onboarding tests to align with the new shared implementation. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
ericksoa
added a commit
that referenced
this pull request
Apr 8, 2026
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>
6 tasks
ericksoa
added a commit
that referenced
this pull request
Apr 8, 2026
…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>
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
src/lib/sandbox-create-stream.tsonboard.jsfocused on sandbox orchestration and recovery flowNotes
maininto this branch in4b6be62, so the PR now contains only the sandbox-create-stream extractiononboard.jsTesting
npm run build:clinpx vitest run --project cli src/lib/sandbox-create-stream.test.ts test/onboard.test.jsnpm testSummary by CodeRabbit
Refactor
Tests