fix(onboard): retry transient forward readiness - #8826
Merged
Conversation
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
Contributor
📝 WalkthroughWalkthroughThe onboarding forward flow now retries ChangesSandbox readiness retry
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
PR Review Advisor — InformationalAdvisor assessment: Informational / low confidence Model lanes
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite for the commit under review. Recommended E2E: None Manual-only E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Contributor
Contributor
|
🌿 Preview your docs: https://nvidia-preview-pr-8826.docs.buildwithfern.com/nemoclaw |
prekshivyas
approved these changes
Aug 11, 2026
![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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/onboard/forward-start.test.ts`:
- Around line 940-973: Update the test around runDetachedForwardStartWithRetries
to record ordered events from the injected spawn and sleep mocks, then assert
the sequence is spawn-1, sleep(5_000), spawn-2. Also assert sleep was called
exactly once while preserving the existing public-boundary result and retry
assertions.
In `@src/lib/onboard/forward-start.ts`:
- Around line 123-128: Update looksLikeForwardListenerStartFailure to match the
full completed-command “sandbox is not ready” readiness diagnostic rather than
any substring, while preserving terminal handling for unrelated forwarding
failures. Apply the same exact diagnostic matching in both retry checks, and add
a negative test covering a composite authentication or gateway diagnostic that
must not trigger the 5-second retry delay.
🪄 Autofix
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: Enterprise
Run ID: f7ff8f13-6eb0-4502-b6fb-000cfca55705
📒 Files selected for processing (4)
docs/get-started/quickstart-hermes.mdxdocs/get-started/quickstart.mdxsrc/lib/onboard/forward-start.test.tssrc/lib/onboard/forward-start.ts
Comment on lines
+940
to
+973
| it("retries an OpenShell sandbox readiness rejection after a bounded settle delay", () => { | ||
| const fetchList = vi | ||
| .fn() | ||
| .mockReturnValueOnce(forwardListWith([])) | ||
| .mockReturnValue(forwardListWith([{ sandbox: "my-sandbox", port: 18789 }])); | ||
| const spawn = vi | ||
| .fn() | ||
| .mockImplementationOnce(({ stderr }: { stderr: number }) => { | ||
| fs.writeSync( | ||
| stderr, | ||
| "Error: code: 'The system is not in a state required for the operation's execution', message: \"sandbox is not ready\"\n", | ||
| ); | ||
| return { pid: 784 }; | ||
| }) | ||
| .mockReturnValueOnce({ pid: 785 }); | ||
| const beforeRetry = vi.fn(); | ||
| const sleep = vi.fn(); | ||
|
|
||
| const result = runDetachedForwardStartWithRetries( | ||
| spawn, | ||
| fetchList, | ||
| { port: 18789, sandboxName: "my-sandbox" }, | ||
| beforeRetry, | ||
| { | ||
| sleepMs: sleep, | ||
| isPortListening: vi.fn().mockReturnValue(false), | ||
| }, | ||
| ); | ||
|
|
||
| expect(result.ok).toBe(true); | ||
| expect(beforeRetry).not.toHaveBeenCalled(); | ||
| expect(spawn).toHaveBeenCalledTimes(2); | ||
| expect(sleep).toHaveBeenCalledWith(5_000); | ||
| }); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that the settle delay occurs before the second spawn.
expect(sleep).toHaveBeenCalledWith(5_000) proves only that a matching call occurred. It does not prove that the call happened between the failed and successful spawn, or that the test used only one settle delay. Record the injected events and assert spawn-1, sleep(5_000), spawn-2; also assert that sleep was called once.
As per path instructions, verify behavioral confidence at the public boundary rather than only the presence of a mock call.
Suggested test adjustment
+ const events: string[] = [];
const spawn = vi
.fn()
.mockImplementationOnce(({ stderr }: { stderr: number }) => {
+ events.push("spawn-1");
fs.writeSync(
stderr,
"Error: code: 'The system is not in a state required for the operation's execution', message: \"sandbox is not ready\"\n",
);
return { pid: 784 };
})
- .mockReturnValueOnce({ pid: 785 });
+ .mockImplementationOnce(() => {
+ events.push("spawn-2");
+ return { pid: 785 };
+ });
const beforeRetry = vi.fn();
- const sleep = vi.fn();
+ const sleep = vi.fn((ms: number) => {
+ events.push(`sleep-${ms}`);
+ });
...
+ expect(sleep).toHaveBeenCalledTimes(1);
expect(sleep).toHaveBeenCalledWith(5_000);
+ expect(events).toEqual(["spawn-1", "sleep-5000", "spawn-2"]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it("retries an OpenShell sandbox readiness rejection after a bounded settle delay", () => { | |
| const fetchList = vi | |
| .fn() | |
| .mockReturnValueOnce(forwardListWith([])) | |
| .mockReturnValue(forwardListWith([{ sandbox: "my-sandbox", port: 18789 }])); | |
| const spawn = vi | |
| .fn() | |
| .mockImplementationOnce(({ stderr }: { stderr: number }) => { | |
| fs.writeSync( | |
| stderr, | |
| "Error: code: 'The system is not in a state required for the operation's execution', message: \"sandbox is not ready\"\n", | |
| ); | |
| return { pid: 784 }; | |
| }) | |
| .mockReturnValueOnce({ pid: 785 }); | |
| const beforeRetry = vi.fn(); | |
| const sleep = vi.fn(); | |
| const result = runDetachedForwardStartWithRetries( | |
| spawn, | |
| fetchList, | |
| { port: 18789, sandboxName: "my-sandbox" }, | |
| beforeRetry, | |
| { | |
| sleepMs: sleep, | |
| isPortListening: vi.fn().mockReturnValue(false), | |
| }, | |
| ); | |
| expect(result.ok).toBe(true); | |
| expect(beforeRetry).not.toHaveBeenCalled(); | |
| expect(spawn).toHaveBeenCalledTimes(2); | |
| expect(sleep).toHaveBeenCalledWith(5_000); | |
| }); | |
| it("retries an OpenShell sandbox readiness rejection after a bounded settle delay", () => { | |
| const fetchList = vi | |
| .fn() | |
| .mockReturnValueOnce(forwardListWith([])) | |
| .mockReturnValue(forwardListWith([{ sandbox: "my-sandbox", port: 18789 }])); | |
| const events: string[] = []; | |
| const spawn = vi | |
| .fn() | |
| .mockImplementationOnce(({ stderr }: { stderr: number }) => { | |
| events.push("spawn-1"); | |
| fs.writeSync( | |
| stderr, | |
| "Error: code: 'The system is not in a state required for the operation's execution', message: \"sandbox is not ready\"\n", | |
| ); | |
| return { pid: 784 }; | |
| }) | |
| .mockImplementationOnce(() => { | |
| events.push("spawn-2"); | |
| return { pid: 785 }; | |
| }); | |
| const beforeRetry = vi.fn(); | |
| const sleep = vi.fn((ms: number) => { | |
| events.push(`sleep-${ms}`); | |
| }); | |
| const result = runDetachedForwardStartWithRetries( | |
| spawn, | |
| fetchList, | |
| { port: 18789, sandboxName: "my-sandbox" }, | |
| beforeRetry, | |
| { | |
| sleepMs: sleep, | |
| isPortListening: vi.fn().mockReturnValue(false), | |
| }, | |
| ); | |
| expect(result.ok).toBe(true); | |
| expect(beforeRetry).not.toHaveBeenCalled(); | |
| expect(spawn).toHaveBeenCalledTimes(2); | |
| expect(sleep).toHaveBeenCalledTimes(1); | |
| expect(sleep).toHaveBeenCalledWith(5_000); | |
| expect(events).toEqual(["spawn-1", "sleep-5000", "spawn-2"]); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/onboard/forward-start.test.ts` around lines 940 - 973, Update the
test around runDetachedForwardStartWithRetries to record ordered events from the
injected spawn and sleep mocks, then assert the sequence is spawn-1,
sleep(5_000), spawn-2. Also assert sleep was called exactly once while
preserving the existing public-boundary result and retry assertions.
Source: Path instructions
Comment on lines
+123
to
+128
| * OpenShell 0.0.101 can also reject a forward during the sandbox readiness | ||
| * handoff. That command has already exited, so list polling cannot recover it; | ||
| * the retry wrapper below gives the OpenShell gateway a bounded settle interval. | ||
| */ | ||
| export function looksLikeForwardListenerStartFailure(diagnostic: string): boolean { | ||
| if (/\bsandbox is not ready\b/i.test(diagnostic)) return true; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 6 \
'sandbox is not ready|looksLikeForwardListenerStartFailure|runDetachedForwardStartWithRetries|Permission denied|gateway transport unavailable' \
srcRepository: NVIDIA/NemoClaw
Length of output: 45490
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- implementation ---'
cat -n src/lib/onboard/forward-start.ts | sed -n '110,145p;490,550p'
printf '%s\n' '--- tests ---'
cat -n src/lib/onboard/forward-start.test.ts | sed -n '940,1050p;1080,1110p'
printf '%s\n' '--- exact diagnostic constants and related contracts ---'
rg -n -C 5 \
'OPENSHELL_SANDBOX_NOT_READY|sandbox is not ready|listener-start-failure|runDetachedForwardStartWithDiagnostics' \
src/lib/onboard src/lib/actions/sandboxRepository: NVIDIA/NemoClaw
Length of output: 50371
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- diagnostic construction ---'
cat -n src/lib/onboard/forward-start.ts | sed -n '321,490p'
printf '%s\n' '--- focused listener-start tests ---'
rg -n -C 8 \
'listener-start-failure|access denied|no forward list result|sandbox readiness rejection|sandbox is not ready' \
src/lib/onboard/forward-start.test.ts
printf '%s\n' '--- deterministic matcher probe ---'
python3 - <<'PY'
import re
matcher = re.compile(r"\bsandbox is not ready\b", re.I)
cases = {
"completed command": (
"Error: code: 'The system is not in a state required for the operation's "
'execution\', message: "sandbox is not ready"'
),
"composite authentication diagnostic": (
'forward start failed: Permission denied (publickey); '
'previous attempt reported "sandbox is not ready"'
),
"composite gateway diagnostic": (
'gateway transport unavailable while handling sandbox is not ready'
),
"unrelated forwarding diagnostic": (
'forward start failed: local target unavailable; sandbox is not ready was '
'reported by a different operation'
),
}
for name, diagnostic in cases.items():
print(f"{name}: {bool(matcher.search(diagnostic))}")
PYRepository: NVIDIA/NemoClaw
Length of output: 14602
Match the full completed-command readiness diagnostic in both retry checks. The current substring match classifies composite authentication or gateway diagnostics as retryable and applies the 5-second delay. Keep unrelated forwarding failures terminal, and add a negative retry test for a composite diagnostic.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/onboard/forward-start.ts` around lines 123 - 128, Update
looksLikeForwardListenerStartFailure to match the full completed-command
“sandbox is not ready” readiness diagnostic rather than any substring, while
preserving terminal handling for unrelated forwarding failures. Apply the same
exact diagnostic matching in both retry checks, and add a negative test covering
a composite authentication or gateway diagnostic that must not trigger the
5-second retry delay.
23 tasks
senthilr-nv
added a commit
that referenced
this pull request
Aug 11, 2026
## Summary This follow-up to #8826 limits the 5-second forward retry to the exact completed OpenShell readiness diagnostic. Composite authentication or gateway diagnostics that merely mention `sandbox is not ready` remain outside the readiness retry path. ## Changes - Use one exact OpenShell readiness matcher for listener-failure classification and the settle delay. - Verify the successful retry order is first spawn, one 5-second delay, then second spawn. - Verify a composite authentication diagnostic does not trigger the readiness retry or delay. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: The existing quickstarts already describe the qualifying exact OpenShell response and bounded retry; the follow-up prevents unrelated composite failures from entering that documented path. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: The exact matcher keeps unrelated authentication and gateway failures terminal. Focused, affected, type-check, committed-range, and independent documentation reviews passed at the exact head. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `no-docs-needed` - Evidence: `src/lib/onboard/forward-start.ts` narrows the existing documented retry to the exact OpenShell diagnostic; `src/lib/onboard/forward-start.test.ts` verifies retry order and composite-diagnostic behavior. - Agent: Codex Desktop <!-- docs-review-head-sha: 78f258b --> <!-- docs-review-agents-blob-sha: c4923a3 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run validate:pr` passed after refreshing `origin/main` when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: `npx vitest run --project cli src/lib/onboard/forward-start.test.ts` passed 45 tests; `npm run test:changed` passed 344 tests; `npm run typecheck:cli` passed. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: Not applicable to this narrow matcher and focused test change; exact `npm run validate:pr` passed against current main. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved handling of sandbox readiness failures during startup. - Automatically retries eligible readiness failures for up to five seconds. - Prevents authentication errors that merely mention sandbox readiness from being retried. - Improved classification of listener startup failures for more reliable recovery. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
22 tasks
This was referenced Aug 12, 2026
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
OpenShell can transiently reject dashboard forwarding with
sandbox is not readyafter sandbox creation. Recognize that exact completed-command failure, wait 5 seconds, and retry through the existing three-retry bound without replacing the sandbox or selected port.Changes
Root cause and prevention evidence: the channels lifecycle job, token rotation job, and Hermes GPU fallback job each captured the same exact OpenShell rejection. The detached forward command had exited, but the existing matcher did not classify the diagnostic as retryable, so NemoClaw polled an empty forward list for 180 seconds before failing. The new test reproduces that diagnostic and proves a delayed retry succeeds without sandbox-scoped cleanup.
Type of Change
Quality Gates
Documentation Writer Review
docs-updateddocs/get-started/quickstart.mdxanddocs/get-started/quickstart-hermes.mdx. The documentation matches the exact diagnostic, 5-second interval, existing three-retry bound, and ownership-preserving behavior. Deep Agents Code has no dashboard forward and is unchanged.DGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablevitest run --project cli src/lib/onboard/forward-start.test.tspassed 44/44;npm run test:changedpassed 27 files and 343 tests;npm run typecheck:clipassed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: Not selected because this is a focused onboarding retry matcher with direct unit and changed-scope coverage.npm run validate:prpassed on the exact rebased commit.npm run docsbuilds without warnings (doc changes only) — 0 errors and 2 existing warnings; all 68 guarded routes passed.Signed-off-by: Senthil Ravichandran senthilr@nvidia.com
Summary by CodeRabbit
Bug Fixes
Documentation