fix(sandbox): restore protected stopped sandboxes - #8683
Conversation
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
📝 WalkthroughWalkthroughSandbox startup recovery now returns structured, sanitized failure results. Managed recovery verifies gateway health and OpenShell readiness before forwarding. Failed sandboxes remain preserved and return actionable recovery guidance. ChangesSandbox recovery and startup reporting
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant SandboxStart
participant restoreSandboxStartupState
participant recoverSandboxProcesses
participant OpenShell
participant HostForward
SandboxStart->>restoreSandboxStartupState: restore startup state
restoreSandboxStartupState->>recoverSandboxProcesses: recover processes
recoverSandboxProcesses->>OpenShell: verify readiness
OpenShell-->>recoverSandboxProcesses: return readiness result
recoverSandboxProcesses->>HostForward: start forwarding after readiness
restoreSandboxStartupState-->>SandboxStart: return recovery result
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit f9cd443 in the TypeScript / code-coverage/cliThe overall coverage in commit f9cd443 in the Show a code coverage summary of the most impacted files.
Updated |
|
🌿 Preview your docs: https://nvidia-preview-pr-8683.docs.buildwithfern.com/nemoclaw |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
test/e2e/live/shields-restart-recovery-evidence.ts (1)
844-855: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the parsed probe output, not the constructed constants.
postStartManagedControlProbebuildsevidencewithdisposition,exitCode,phase,valid, andversionas hard-coded literals at Lines 639-648. The checks at Lines 849-855 therefore compare those literals to themselves and cannot fail. The real gate is thethrowat Lines 625-638.Either drop these checks or move the parsed values into
ManagedControlEvidenceand assert those. The remaining checks in this function, which compareprocessChainagainstpid1, do exercise observed state and should stay.As per path instructions: "Flag copied production algorithms, broad mocks that bypass the behavior under test, and conditionals that make a test pass without exercising its claim."
🤖 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 `@test/e2e/live/shields-restart-recovery-evidence.ts` around lines 844 - 855, The assertions in assertManagedControlReady currently validate hard-coded evidence rather than parsed probe output, so remove them or update postStartManagedControlProbe to store parsed disposition, exitCode, phase, valid, and version in ManagedControlEvidence and assert those fields here. Preserve the processChain-to-pid1 checks because they validate observed state.Source: Path instructions
test/process-recovery-supervisor-relaunch.test.ts (1)
736-741: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the pinned-probe assertion report which call diverged.
.every(...)collapses six calls into one boolean. A failure reports only "expected false to be true". It does not identify which call used the wrong action or the wrong container ID.Asserting the argument pairs directly gives the same guarantee and names the diverging call.
💚 Proposed change
expect(requestPinnedGatewaySupervisorAction).toHaveBeenCalledTimes(6); - expect( - requestPinnedGatewaySupervisorAction.mock.calls.every( - (call: unknown[]) => call[1] === "probe" && call[3] === "replacement-container-id", - ), - ).toBe(true); + expect( + requestPinnedGatewaySupervisorAction.mock.calls.map((call: unknown[]) => [call[1], call[3]]), + ).toEqual(Array.from({ length: 6 }, () => ["probe", "replacement-container-id"]));🤖 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 `@test/process-recovery-supervisor-relaunch.test.ts` around lines 736 - 741, Replace the aggregate .every assertion on requestPinnedGatewaySupervisorAction.mock.calls with a direct per-call argument assertion, such as expected argument pairs, so failures identify the specific call with an incorrect action or container ID; preserve validation of "probe" and "replacement-container-id" for all six calls.src/lib/onboard/docker-driver-sandbox-recovery.test.ts (1)
116-122: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider covering the remaining rejection branches.
The truncated-ID case proves the main path. The validation at
docker-driver-sandbox-recovery.tslines 206-213 has three more branches that no test exercises: an empty status, a name containing a control character, and an ID with uppercase hex characters.These branches guard a security boundary, so a regression that loosens one would pass the current suite.
🧪 Suggested additional cases
it("rejects container metadata with a control character in the name", () => { expect(() => findLabeledSandboxContainers("e2e-x", { dockerCapture: () => `${ORIGINAL_CONTAINER_ID}\topenshell-e2e\u0000x\tCreated\n`, }), ).toThrow(/malformed OpenShell sandbox container metadata/u); }); it("rejects container metadata with an empty status", () => { expect(() => findLabeledSandboxContainers("e2e-x", { dockerCapture: () => `${ORIGINAL_CONTAINER_ID}\topenshell-e2e-x\t\n`, }), ).toThrow(/malformed OpenShell sandbox container metadata/u); }); it("rejects an uppercase container identity", () => { expect(() => findLabeledSandboxContainers("e2e-x", { dockerCapture: () => `${"A".repeat(64)}\topenshell-e2e-x\tCreated\n`, }), ).toThrow(/malformed OpenShell sandbox container metadata/u); });As per path instructions: "Require negative-path tests that prove the boundary rejects bypasses".
🤖 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/docker-driver-sandbox-recovery.test.ts` around lines 116 - 122, Extend the tests around findLabeledSandboxContainers with negative cases for each remaining metadata-validation branch: a control character in the container name, an empty status, and an uppercase-hex container ID. Use valid surrounding metadata and assert each case throws the existing malformed OpenShell sandbox container metadata error, preserving coverage of the security boundary.Source: Path instructions
src/lib/actions/sandbox/forward-recovery.ts (1)
383-398: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the rejected-healthy-forward outcome with the messaging helper.
At line 390, a healthy Hermes forward whose
afterSuccess()returns false maps tonull. Insrc/lib/actions/sandbox/hermes-dashboard-recovery.tslines 44-47,nullis neithertruenor"occupied", so it falls through toensurePortForward. The code therefore tries to recreate a forward that was already healthy, immediately after the managed-health guard reported an unverified container identity.The final outcome is still fail-closed, because
beforeStartvetoes again at line 278 and returns false. The cost is an extra supervisor probe and a redundant health check on a guard path.
ensureMessagingHostForwardHealthyat line 416 handles the same situation directly by returning theafterSuccessresult. Returning"occupied"here maps straight tofalseand matches that behavior without the recreation round trip.♻️ Proposed change
isPortForwardHealthy: (name, port) => { const health = isSandboxPortForwardHealthy(name, port); - return health === true && options.afterSuccess && !options.afterSuccess() ? null : health; + return health === true && options.afterSuccess && !options.afterSuccess() + ? "occupied" + : health; },Confirm that
SandboxForwardHealthadmits"occupied"in this position before applying.🤖 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/actions/sandbox/forward-recovery.ts` around lines 383 - 398, Update the health callback in ensureHermesDashboardPortForwardIfEnabled so a healthy forward whose options.afterSuccess() returns false returns "occupied" instead of null. Confirm SandboxForwardHealth permits the "occupied" result, preserving the existing true/health behavior and ensuring hermes-dashboard-recovery handles this outcome without recreating the forward.src/lib/actions/sandbox/start.test.ts (3)
409-419: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the pinning assertion out of the mock implementation.
expect(expectedContainerId).toBe(ORIGINAL_CONTAINER_ID)runs inside the controller mock. If it fails, it throws from inside production code.checkAndRecoverSandboxProcessescan catch that throw and reclassify it as a managed-recovery failure. The test then reports a supervisor or gateway error instead of the pinning violation that actually occurred.Record the received IDs and assert on them after
startSandboxresolves.♻️ Proposed change
+ const observedContainerIds: (string | undefined)[] = []; const controller = vi.fn( ( name: string, action: "restart" | "recover" | "probe", _timeout: number, expectedContainerId?: string, ) => { - expect(expectedContainerId).toBe(ORIGINAL_CONTAINER_ID); + observedContainerIds.push(expectedContainerId); return successfulController(name, action); }, );Then assert after the call:
await expect(startSandbox("my-sandbox", h.deps)).resolves.toEqual({ exitCode: 0 }); + expect(observedContainerIds.every((id) => id === ORIGINAL_CONTAINER_ID)).toBe(true);🤖 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/actions/sandbox/start.test.ts` around lines 409 - 419, Update the controller mock in the startSandbox test to record each received expectedContainerId without asserting inside the mock; after startSandbox resolves, assert the recorded IDs equal ORIGINAL_CONTAINER_ID so the pinning violation is reported directly by the test.
226-228: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAssert that the raw token is absent, not only that
<REDACTED>is present.The regex proves
<REDACTED>appears. It does not proveopaque-post-connect-tokenwas removed. A partial redaction that emits<REDACTED>and still carries the token elsewhere in the message passes this test. Line 495 already uses the stronger form for the other redaction case.♻️ Proposed strengthening
- await expect(startSandbox("my-sandbox", h.deps)).rejects.toThrow( - /final managed gateway health.*container identity changed.*<REDACTED>/iu, - ); + const failure = await startSandbox("my-sandbox", h.deps).catch((error: unknown) => error); + expect(failure).toBeInstanceOf(Error); + const failureMessage = (failure as Error).message; + expect(failureMessage).toMatch( + /final managed gateway health.*container identity changed.*<REDACTED>/iu, + ); + expect(failureMessage).not.toContain("opaque-post-connect-token");🤖 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/actions/sandbox/start.test.ts` around lines 226 - 228, Strengthen the rejection assertion for startSandbox in the test around “final managed gateway health” so it verifies both that <REDACTED> appears and that the raw token opaque-post-connect-token is absent, matching the stronger redaction assertion already used at line 495.
340-604: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftSplit this test into separate cases.
One
itbody runs five independent scenarios: success, supervisor failure, readiness failure, already-running gateway, and identity swap. They are separated only bymockClear()and manual flag resets.Two consequences follow. If scenario 3 fails, scenarios 4 and 5 never run, so one regression hides four other behaviors. The failure report names only the parameterized title, so the failing scenario is not identifiable without reading the body.
State also carries across scenarios.
inspectGateway.mockReturnValue(true)at line 542 is never reset, so scenario 5 depends on a value set for scenario 4.Extract the setup into a helper and give each scenario its own
it.As per path instructions: "Review tests for behavioral confidence rather than implementation lock-in."
🤖 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/actions/sandbox/start.test.ts` around lines 340 - 604, The parameterized test currently combines five independent behaviors, shares mutable mocks and state, and produces ambiguous failures. Extract the common sandbox, spy, controller, and harness setup into a reusable helper, then split success, supervisor failure, readiness failure, already-running gateway, and identity-swap behavior into separate parameterized or nested tests, resetting scenario-specific mocks within each case. Ensure each test independently initializes flags and return values, especially inspectGateway, and retains behavioral assertions without coupling to unnecessary implementation details.Source: Path instructions
src/lib/shields/index.ts (1)
2646-2655: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDocument why
expectedContainerIdalso disables inline recovery.Line 2654 derives
allowInlineRecoveryfrom whether the caller supplied a container ID. The two concerns are independent: one pins the exec target, the other decides whether an expired auto-restore may be recovered in place. A future caller that wants pinning with inline recovery, or the fence without pinning, cannot express it and may not notice that supplying an ID silently changes the fail-closed posture.Add a short comment stating the rule: a pinned caller holds a lifecycle lock, so it must refuse rather than recover inline.
♻️ Proposed comment
}, + // A pinned caller (lifecycle `start`) already owns the container and its + // lifecycle lock, so it must refuse an expired auto-restore instead of + // recovering it inline. Unpinned callers keep the legacy inline path. { allowInlineRecovery: expectedContainerId === undefined }, );🤖 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/shields/index.ts` around lines 2646 - 2655, Add a concise comment immediately above the allowInlineRecovery assignment explaining that callers supplying expectedContainerId hold a lifecycle lock and therefore must refuse expired auto-restore rather than recover inline; leave the existing behavior unchanged.
🤖 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/actions/sandbox/process-recovery.ts`:
- Around line 493-511: Update pinnedManagedGatewayProbeFailure to catch
identity-change errors from requestPinnedGatewaySupervisorActionImpl and convert
them into the appropriate classified ManagedGatewayRecoveryFailure instead of
allowing them to escape. Match the existing handling in
createPreservedManagedForwardVerification and
managedStartupOpenShellReadinessFailureDetail, preserving the normal
managedSupervisorProbeFailure result for non-throwing paths.
In `@src/lib/actions/sandbox/start.ts`:
- Around line 274-283: Align the missing-container-identity message and its test
assertion. In src/lib/actions/sandbox/start.ts lines 274-283, choose the
operator-facing wording and ensure it includes the terms required by the
assertion if retained; in src/lib/actions/sandbox/start.test.ts lines 829-842,
update the regex to match the shipped wording, with no other behavior changes.
- Around line 307-323: Export the existing SHIELDS_STARTUP_AUTO_RESTORE_REQUIRED
constant from the Shields module, remove the duplicate local declaration in the
sandbox start action, and retrieve the shared sentinel through the existing lazy
loader used by start.ts so the catch branch matches the canonical value and
preserves the specialized shields up guidance.
In `@src/lib/onboard/docker-driver-sandbox-recovery.ts`:
- Around line 204-214: Update startSandbox and stopSandbox to catch errors from
findLabeledSandboxContainers, convert them into a concise lifecycle error
result, and route that result through applyExitResult so structured exit-1
recovery guidance is preserved.
---
Nitpick comments:
In `@src/lib/actions/sandbox/forward-recovery.ts`:
- Around line 383-398: Update the health callback in
ensureHermesDashboardPortForwardIfEnabled so a healthy forward whose
options.afterSuccess() returns false returns "occupied" instead of null. Confirm
SandboxForwardHealth permits the "occupied" result, preserving the existing
true/health behavior and ensuring hermes-dashboard-recovery handles this outcome
without recreating the forward.
In `@src/lib/actions/sandbox/start.test.ts`:
- Around line 409-419: Update the controller mock in the startSandbox test to
record each received expectedContainerId without asserting inside the mock;
after startSandbox resolves, assert the recorded IDs equal ORIGINAL_CONTAINER_ID
so the pinning violation is reported directly by the test.
- Around line 226-228: Strengthen the rejection assertion for startSandbox in
the test around “final managed gateway health” so it verifies both that
<REDACTED> appears and that the raw token opaque-post-connect-token is absent,
matching the stronger redaction assertion already used at line 495.
- Around line 340-604: The parameterized test currently combines five
independent behaviors, shares mutable mocks and state, and produces ambiguous
failures. Extract the common sandbox, spy, controller, and harness setup into a
reusable helper, then split success, supervisor failure, readiness failure,
already-running gateway, and identity-swap behavior into separate parameterized
or nested tests, resetting scenario-specific mocks within each case. Ensure each
test independently initializes flags and return values, especially
inspectGateway, and retains behavioral assertions without coupling to
unnecessary implementation details.
In `@src/lib/onboard/docker-driver-sandbox-recovery.test.ts`:
- Around line 116-122: Extend the tests around findLabeledSandboxContainers with
negative cases for each remaining metadata-validation branch: a control
character in the container name, an empty status, and an uppercase-hex container
ID. Use valid surrounding metadata and assert each case throws the existing
malformed OpenShell sandbox container metadata error, preserving coverage of the
security boundary.
In `@src/lib/shields/index.ts`:
- Around line 2646-2655: Add a concise comment immediately above the
allowInlineRecovery assignment explaining that callers supplying
expectedContainerId hold a lifecycle lock and therefore must refuse expired
auto-restore rather than recover inline; leave the existing behavior unchanged.
In `@test/e2e/live/shields-restart-recovery-evidence.ts`:
- Around line 844-855: The assertions in assertManagedControlReady currently
validate hard-coded evidence rather than parsed probe output, so remove them or
update postStartManagedControlProbe to store parsed disposition, exitCode,
phase, valid, and version in ManagedControlEvidence and assert those fields
here. Preserve the processChain-to-pid1 checks because they validate observed
state.
In `@test/process-recovery-supervisor-relaunch.test.ts`:
- Around line 736-741: Replace the aggregate .every assertion on
requestPinnedGatewaySupervisorAction.mock.calls with a direct per-call argument
assertion, such as expected argument pairs, so failures identify the specific
call with an incorrect action or container ID; preserve validation of "probe"
and "replacement-container-id" for all six calls.
🪄 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: 5dfb4958-d6f9-4c6c-950b-e00a62daeaa3
📒 Files selected for processing (21)
docs/reference/commands.mdxsrc/lib/actions/sandbox/connect-flow.test.tssrc/lib/actions/sandbox/connect.tssrc/lib/actions/sandbox/docker-health.test.tssrc/lib/actions/sandbox/forward-recovery.tssrc/lib/actions/sandbox/process-recovery.test.tssrc/lib/actions/sandbox/process-recovery.tssrc/lib/actions/sandbox/start.test.tssrc/lib/actions/sandbox/start.tssrc/lib/actions/sandbox/stop.test.tssrc/lib/onboard/docker-driver-sandbox-recovery.test.tssrc/lib/onboard/docker-driver-sandbox-recovery.tssrc/lib/onboard/runtime-provider/contract.tssrc/lib/onboard/runtime-provider/docker.tssrc/lib/onboard/runtime-provider/podman.test.tssrc/lib/shields/index.tssrc/lib/shields/startup-access.test.tstest/e2e/live/hermes-shields-config.test.tstest/e2e/live/shields-config.test.tstest/e2e/live/shields-restart-recovery-evidence.tstest/process-recovery-supervisor-relaunch.test.ts
| export function pinnedManagedGatewayProbeFailure( | ||
| sandboxName: string, | ||
| expectedContainerId: string, | ||
| { | ||
| requestPinnedGatewaySupervisorActionImpl = executeGatewaySupervisorActionPinned, | ||
| waitForManagedGatewaySupervisorImpl = waitForManagedGatewaySupervisor, | ||
| }: { | ||
| requestPinnedGatewaySupervisorActionImpl?: RequestPinnedGatewaySupervisorAction; | ||
| waitForManagedGatewaySupervisorImpl?: typeof waitForManagedGatewaySupervisor; | ||
| } = {}, | ||
| ): ManagedGatewayRecoveryFailure | null { | ||
| return managedSupervisorProbeFailure( | ||
| sandboxName, | ||
| waitForManagedGatewaySupervisorImpl, | ||
| (name, action, timeout = 210000) => | ||
| requestPinnedGatewaySupervisorActionImpl(name, action, timeout, expectedContainerId), | ||
| false, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Catch the identity-change throw so the final proof returns a classified failure.
executeGatewaySupervisorActionPinned reaches privilegedSandboxExecArgv, which throws when the resolved container differs from expectedContainerId. managedSupervisorProbeFailure does not wrap waitForSupervisor in a try/catch, so that throw propagates out of pinnedManagedGatewayProbeFailure.
The sibling guards handle the same throw and convert it into a ManagedGatewayRecoveryFailure: createPreservedManagedForwardVerification at lines 585-591 and managedStartupOpenShellReadinessFailureDetail at lines 1166-1170.
The consumer in src/lib/actions/sandbox/start.ts (verifyGateway) only formats a returned failure. It appends the recover then start remediation text. An escaping throw bypasses that formatting, so a same-name container swap detected at the final verification step produces a less actionable error than a swap detected earlier.
Issue #8662 requires specific actionable recovery errors on this path.
🛡️ Proposed fix
): ManagedGatewayRecoveryFailure | null {
- return managedSupervisorProbeFailure(
- sandboxName,
- waitForManagedGatewaySupervisorImpl,
- (name, action, timeout = 210000) =>
- requestPinnedGatewaySupervisorActionImpl(name, action, timeout, expectedContainerId),
- false,
- );
+ try {
+ return managedSupervisorProbeFailure(
+ sandboxName,
+ waitForManagedGatewaySupervisorImpl,
+ (name, action, timeout = 210000) =>
+ requestPinnedGatewaySupervisorActionImpl(name, action, timeout, expectedContainerId),
+ false,
+ );
+ } catch {
+ return {
+ layer: "privileged control unavailable",
+ detail: "the pinned container identity changed during final managed gateway verification",
+ };
+ }
}📝 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.
| export function pinnedManagedGatewayProbeFailure( | |
| sandboxName: string, | |
| expectedContainerId: string, | |
| { | |
| requestPinnedGatewaySupervisorActionImpl = executeGatewaySupervisorActionPinned, | |
| waitForManagedGatewaySupervisorImpl = waitForManagedGatewaySupervisor, | |
| }: { | |
| requestPinnedGatewaySupervisorActionImpl?: RequestPinnedGatewaySupervisorAction; | |
| waitForManagedGatewaySupervisorImpl?: typeof waitForManagedGatewaySupervisor; | |
| } = {}, | |
| ): ManagedGatewayRecoveryFailure | null { | |
| return managedSupervisorProbeFailure( | |
| sandboxName, | |
| waitForManagedGatewaySupervisorImpl, | |
| (name, action, timeout = 210000) => | |
| requestPinnedGatewaySupervisorActionImpl(name, action, timeout, expectedContainerId), | |
| false, | |
| ); | |
| } | |
| export function pinnedManagedGatewayProbeFailure( | |
| sandboxName: string, | |
| expectedContainerId: string, | |
| { | |
| requestPinnedGatewaySupervisorActionImpl = executeGatewaySupervisorActionPinned, | |
| waitForManagedGatewaySupervisorImpl = waitForManagedGatewaySupervisor, | |
| }: { | |
| requestPinnedGatewaySupervisorActionImpl?: RequestPinnedGatewaySupervisorAction; | |
| waitForManagedGatewaySupervisorImpl?: typeof waitForManagedGatewaySupervisor; | |
| } = {}, | |
| ): ManagedGatewayRecoveryFailure | null { | |
| try { | |
| return managedSupervisorProbeFailure( | |
| sandboxName, | |
| waitForManagedGatewaySupervisorImpl, | |
| (name, action, timeout = 210000) => | |
| requestPinnedGatewaySupervisorActionImpl(name, action, timeout, expectedContainerId), | |
| false, | |
| ); | |
| } catch { | |
| return { | |
| layer: "privileged control unavailable", | |
| detail: "the pinned container identity changed during final managed gateway verification", | |
| }; | |
| } | |
| } |
🤖 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/actions/sandbox/process-recovery.ts` around lines 493 - 511, Update
pinnedManagedGatewayProbeFailure to catch identity-change errors from
requestPinnedGatewaySupervisorActionImpl and convert them into the appropriate
classified ManagedGatewayRecoveryFailure instead of allowing them to escape.
Match the existing handling in createPreservedManagedForwardVerification and
managedStartupOpenShellReadinessFailureDetail, preserving the normal
managedSupervisorProbeFailure result for non-throwing paths.
| } catch (error) { | ||
| if ( | ||
| error && | ||
| typeof error === "object" && | ||
| "code" in error && | ||
| error.code === SHIELDS_STARTUP_AUTO_RESTORE_REQUIRED | ||
| ) { | ||
| throw new Error( | ||
| `Sandbox '${name}' started, but its expired Shields auto-restore must finish before startup recovery. ` + | ||
| `The existing sandbox was preserved. Run \`${cliName()} ${name} shields up\`, then retry \`${cliName()} ${name} start\`.`, | ||
| ); | ||
| } | ||
| throw new Error( | ||
| `Sandbox '${name}' started, but startup state restoration failed: ${sanitizedRecoveryDetail(error)}. ` + | ||
| `The existing sandbox was preserved. Run \`${cliName()} ${name} recover\` and retry \`${cliName()} ${name} start\`.`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Import the auto-restore sentinel instead of redeclaring the literal.
Line 16 declares SHIELDS_STARTUP_AUTO_RESTORE_REQUIRED with the same string value that src/lib/shields/index.ts line 1109 declares independently. The two constants are not linked. If one value changes, this branch stops matching, and the operator receives the generic recover guidance instead of the shields up guidance that can actually clear the state. Nothing fails at build time.
Export the constant from the Shields module and import it here so one definition governs both sides.
♻️ Proposed change
In src/lib/shields/index.ts, export the existing constant:
-const SHIELDS_STARTUP_AUTO_RESTORE_REQUIRED = "NEMOCLAW_SHIELDS_AUTO_RESTORE_REQUIRED";
+export const SHIELDS_STARTUP_AUTO_RESTORE_REQUIRED = "NEMOCLAW_SHIELDS_AUTO_RESTORE_REQUIRED";In src/lib/actions/sandbox/start.ts, drop the local copy and read it through the existing lazy loader:
-const SHIELDS_STARTUP_AUTO_RESTORE_REQUIRED = "NEMOCLAW_SHIELDS_AUTO_RESTORE_REQUIRED"; if (
error &&
typeof error === "object" &&
"code" in error &&
- error.code === SHIELDS_STARTUP_AUTO_RESTORE_REQUIRED
+ error.code ===
+ sandboxStartDependencies.loadShields().SHIELDS_STARTUP_AUTO_RESTORE_REQUIRED
) {📝 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.
| } catch (error) { | |
| if ( | |
| error && | |
| typeof error === "object" && | |
| "code" in error && | |
| error.code === SHIELDS_STARTUP_AUTO_RESTORE_REQUIRED | |
| ) { | |
| throw new Error( | |
| `Sandbox '${name}' started, but its expired Shields auto-restore must finish before startup recovery. ` + | |
| `The existing sandbox was preserved. Run \`${cliName()} ${name} shields up\`, then retry \`${cliName()} ${name} start\`.`, | |
| ); | |
| } | |
| throw new Error( | |
| `Sandbox '${name}' started, but startup state restoration failed: ${sanitizedRecoveryDetail(error)}. ` + | |
| `The existing sandbox was preserved. Run \`${cliName()} ${name} recover\` and retry \`${cliName()} ${name} start\`.`, | |
| ); | |
| } | |
| } catch (error) { | |
| if ( | |
| error && | |
| typeof error === "object" && | |
| "code" in error && | |
| error.code === | |
| sandboxStartDependencies.loadShields().SHIELDS_STARTUP_AUTO_RESTORE_REQUIRED | |
| ) { | |
| throw new Error( | |
| `Sandbox '${name}' started, but its expired Shields auto-restore must finish before startup recovery. ` + | |
| `The existing sandbox was preserved. Run \`${cliName()} ${name} shields up\`, then retry \`${cliName()} ${name} start\`.`, | |
| ); | |
| } | |
| throw new Error( | |
| `Sandbox '${name}' started, but startup state restoration failed: ${sanitizedRecoveryDetail(error)}. ` + | |
| `The existing sandbox was preserved. Run \`${cliName()} ${name} recover\` and retry \`${cliName()} ${name} start\`.`, | |
| ); | |
| } |
🤖 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/actions/sandbox/start.ts` around lines 307 - 323, Export the existing
SHIELDS_STARTUP_AUTO_RESTORE_REQUIRED constant from the Shields module, remove
the duplicate local declaration in the sandbox start action, and retrieve the
shared sentinel through the existing lazy loader used by start.ts so the catch
branch matches the canonical value and preserves the specialized shields up
guidance.
| const [containerId, name, ...rest] = line.split("\t"); | ||
| const status = rest.join("\t").trim(); | ||
| return { name, status, running: status.startsWith("Up") }; | ||
| if ( | ||
| !FULL_DOCKER_CONTAINER_ID_PATTERN.test(containerId) || | ||
| !name || | ||
| !status || | ||
| /[\u0000-\u001f\u007f]/u.test(name) | ||
| ) { | ||
| throw new Error("Docker returned malformed OpenShell sandbox container metadata."); | ||
| } | ||
| return { containerId, name, status, running: status.startsWith("Up") }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether lifecycle callers wrap container discovery in error handling.
set -euo pipefail
fd -t f 'docker.ts' src/lib/onboard/runtime-provider --exec rg -nP -C6 'findLabeledSandboxContainers|try\s*\{|catch'
rg -nP -C5 'runSandboxLifecycle|lifecycle\(|providers?\.(start|stop)\b' --type=ts src/lib/actions/sandboxRepository: NVIDIA/NemoClaw
Length of output: 3192
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- docker provider ---'
cat -n src/lib/onboard/runtime-provider/docker.ts | sed -n '120,235p'
printf '%s\n' '--- recovery caller ---'
rg -n -C12 'recoverDockerDriverSandbox|findLabeledSandboxContainers' src/lib/onboard
printf '%s\n' '--- lifecycle command boundaries ---'
rg -n -C10 'startDockerSandbox|stopDockerSandbox|recoverDockerDriverSandbox|RuntimeProviderLifecycleResult|exitCode|print.*Guidance|catch' src/lib --type=ts
printf '%s\n' '--- sandbox action files ---'
fd -t f . src/lib/actions/sandboxRepository: NVIDIA/NemoClaw
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- docker provider lifecycle implementation ---'
cat -n src/lib/onboard/runtime-provider/docker.ts | sed -n '130,225p'
printf '%s\n' '--- recovery implementation and callers ---'
rg -n -C8 'recoverDockerDriverSandbox|findLabeledSandboxContainers' src/lib/onboard --glob '*.ts' --glob '!*.test.ts'
printf '%s\n' '--- lifecycle result consumers ---'
rg -l 'RuntimeProviderLifecycleResult|startDockerSandbox|stopDockerSandbox|recoverDockerDriverSandbox' src/lib --glob '*.ts' --glob '!*.test.ts' |
while read -r file; do
echo "### $file"
rg -n -C8 'RuntimeProviderLifecycleResult|startDockerSandbox|stopDockerSandbox|recoverDockerDriverSandbox|catch|exitCode|message' "$file"
doneRepository: NVIDIA/NemoClaw
Length of output: 46992
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- lifecycle action call sites ---'
rg -n -C12 'resolveSandboxLifecycleProvider|\.lifecycle\.start|\.lifecycle\.stop|lifecycle\.start|lifecycle\.stop' \
src/lib/actions src/lib/cli src/lib --glob '*.ts' --glob '!*.test.ts' --glob '!src/lib/onboard/runtime-provider/docker.ts'
printf '%s\n' '--- lifecycle action files ---'
fd -t f . src/lib/actions/sandbox | sort
printf '%s\n' '--- CLI error boundary ---'
rg -n -C12 'uncaughtException|unhandledRejection|process\.exitCode|console\.error|run.*Action|execute.*Action|catch \(error\)' \
src --glob '*.ts' --glob '!*.test.ts' | head -n 500Repository: NVIDIA/NemoClaw
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- start action after lifecycle call ---'
cat -n src/lib/actions/sandbox/start.ts | sed -n '250,355p'
printf '%s\n' '--- stop action after lifecycle call ---'
cat -n src/lib/actions/sandbox/stop.ts | sed -n '46,145p'
printf '%s\n' '--- public command entrypoints ---'
rg -n -C10 'startSandbox\(|stopSandbox\(' src --glob '*.ts' --glob '!*.test.ts' --glob '!src/lib/actions/sandbox/start.ts' --glob '!src/lib/actions/sandbox/stop.ts'
printf '%s\n' '--- command result rendering ---'
rg -n -C10 'result\.message|exitCode.*result|process\.exitCode|throw error|catch \(error\)' src --glob '*.ts' --glob '!*.test.ts' | head -n 400Repository: NVIDIA/NemoClaw
Length of output: 34086
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- applyExitResult implementation ---'
rg -n -C12 'applyExitResult' src --glob '*.ts' --glob '!*.test.ts'
printf '%s\n' '--- command base error handling ---'
rg -n -C12 'class .*Command|extends Command|failWithLines|errorHandler|showHelpOnFail|parse.*error' src/commands src --glob '*.ts' --glob '!*.test.ts' | head -n 500
printf '%s\n' '--- lifecycle command tests ---'
rg -n -C10 'startSandbox|stopSandbox|findLabeledSandboxContainers|malformed|malformed metadata|Docker returned malformed' \
src/commands src/lib/actions src/lib/onboard --glob '*start*.test.ts' --glob '*stop*.test.ts' --glob '*.test.ts' | head -n 500Repository: NVIDIA/NemoClaw
Length of output: 50371
Convert Docker discovery errors into lifecycle results.
startSandbox and stopSandbox do not catch errors from findLabeledSandboxContainers. The exception bypasses applyExitResult and the structured exit-1 guidance. Return a concise lifecycle error with recovery guidance instead.
🤖 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/docker-driver-sandbox-recovery.ts` around lines 204 - 214,
Update startSandbox and stopSandbox to catch errors from
findLabeledSandboxContainers, convert them into a concise lifecycle error
result, and route that result through applyExitResult so structured exit-1
recovery guidance is preserved.
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 2 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite against this exact revision. Recommended E2E: 2 optional E2E recommendations
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
|
#8682 @apurvvkumaria take a look i already raised and its more compact. |
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
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/actions/sandbox/start.test.ts`:
- Around line 187-209: Update the Hermes fixture in the parameterized test for
startSandbox to include "opaque-token-8662" in forwardRecoveryFailureDetail, so
the existing failure assertion verifies that secret is redacted on the
forward-failure path.
In `@test/process-recovery-supervisor-relaunch.test.ts`:
- Around line 297-335: Require an explicit assertion that the “OpenShell
readiness” event was recorded before comparing its position with “host forward”
in the test around waitForRecreatedSandboxOpenShellReadyImpl. Add a
non-empty/order-presence check so the test fails when readiness never runs,
while preserving the existing beforeProbe assertion and ordering requirement.
🪄 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: f6c674b7-7d0f-4b7f-87c7-d4bb40c3f0e6
📒 Files selected for processing (7)
docs/manage-sandboxes/recover-rebuild-sandboxes.mdxsrc/lib/actions/sandbox/connect.tssrc/lib/actions/sandbox/process-recovery.tssrc/lib/actions/sandbox/start.test.tssrc/lib/actions/sandbox/start.tssrc/lib/onboard/runtime-provider/podman.test.tstest/process-recovery-supervisor-relaunch.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/onboard/runtime-provider/podman.test.ts
- src/lib/actions/sandbox/process-recovery.ts
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Pull request was converted to draft
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/actions/sandbox/start.test.ts`:
- Around line 171-196: Update the parameterized “propagates an actionable” test
for both recovery-detail fields to include stable non-secret markers in the
injected fixture details, then assert those markers appear in the user-facing
failure returned by startSandbox. Keep the existing recovery-command and
redacted-token assertions, and verify each detail survives sanitization without
inspecting private state or mock calls.
🪄 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: 5646c073-59be-4c20-904c-b15234582b34
📒 Files selected for processing (6)
docs/manage-sandboxes/recover-rebuild-sandboxes.mdxsrc/lib/actions/sandbox/connect.tssrc/lib/actions/sandbox/start.test.tssrc/lib/actions/sandbox/start.tssrc/lib/onboard/runtime-provider/podman.test.tstest/process-recovery-supervisor-relaunch.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/lib/onboard/runtime-provider/podman.test.ts
- docs/manage-sandboxes/recover-rebuild-sandboxes.mdx
- src/lib/actions/sandbox/connect.ts
- test/process-recovery-supervisor-relaunch.test.ts
- src/lib/actions/sandbox/start.ts
| { | ||
| ...FAILED_RECOVERY, | ||
| recoveryFailureLayer: "supervisor unavailable", | ||
| recoveryFailureDetail: `SUPERVISOR_UNAVAILABLE Authorization: Bearer ${REDACTED_TOKEN}`, | ||
| }, | ||
| /supervisor unavailable/iu, | ||
| ], | ||
| [ | ||
| "hermes", | ||
| "OpenShell readiness", | ||
| { | ||
| ...FAILED_RECOVERY, | ||
| forwardRecoveryFailed: true, | ||
| forwardRecoveryFailureDetail: `the sandbox did not become ready in OpenShell: token=${REDACTED_TOKEN}`, | ||
| }, | ||
| /did not become ready in OpenShell/iu, | ||
| ], | ||
| ] as const)("propagates an actionable %s %s failure (#8662)", async (agent, _layer, recovery, expected) => { | ||
| const h = harness(); | ||
| h.getSandbox.mockReturnValue(sandbox({ agent })); | ||
| h.restoreStartupState.mockReturnValue(recovery); | ||
|
|
||
| const failure = await startSandbox("my-sandbox", h.deps).catch((error) => String(error)); | ||
| expect(failure).toMatch(expected); | ||
| expect(failure).toMatch(/nemoclaw my-sandbox recover/iu); | ||
| expect(failure).not.toContain(REDACTED_TOKEN); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that sanitized failure details are retained.
Both fixtures now contain REDACTED_TOKEN, so the token assertion checks the redaction path. The test can still pass if the implementation drops recoveryFailureDetail and forwardRecoveryFailureDetail instead of sanitizing them. Add a stable, non-secret marker from each injected detail and assert that it remains in the user-facing error.
As per path instructions: "Prefer observable outcomes through the public boundary over source-text, private-shape, or mock-call assertions."
🤖 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/actions/sandbox/start.test.ts` around lines 171 - 196, Update the
parameterized “propagates an actionable” test for both recovery-detail fields to
include stable non-secret markers in the injected fixture details, then assert
those markers appear in the user-facing failure returned by startSandbox. Keep
the existing recovery-command and redacted-token assertions, and verify each
detail survives sanitization without inspecting private state or mock calls.
Source: Path instructions
wscurran
added
bug-fix
4 participants
Summary
startSandboxnow consumes the shared managed-recovery result instead of discarding it. OpenClaw and Hermes startup succeeds only after authenticated managed recovery, OpenShell readiness, and host-side port-forward verification; failures preserve the existing sandbox and return specific redacted recovery guidance.Related Issue
Fixes #8662
Changes
startSandboxpath closed on incomplete inspection, Shields or MCP refusals, readiness failures, forward failures, and managed gateway recovery failures before the final connect probe.startsuccess and remediation contract for OpenClaw and Hermes.The final diff against current
origin/mainis 205 additions and 34 deletions across seven files.Type of Change
Quality Gates
Documentation Writer Review
docs-updateddocs/manage-sandboxes/recover-rebuild-sandboxes.mdxDGX 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 unavailablef9cd4437b; signed commit and push hooks passed, including CLI typecheck.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)npm run docs:sync-agent-variantspassed. Protectedshields-configandhermes-shields-configlive targets have not been run againstf9cd4437b; this PR remains draft pending that evidence and broad CI.Signed-off-by: Apurv Kumaria akumaria@nvidia.com
Summary by CodeRabbit
Bug Fixes
Documentation
recoverbefore retrying.