fix(sandbox): recover managed gateway after protected start - #8682
fix(sandbox): recover managed gateway after protected start#8682souvikDevloper wants to merge 8 commits into
Conversation
Signed-off-by: souvikDevloper <138186578+souvikDevloper@users.noreply.github.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSandbox recovery now pins immutable container identity, preserves existing containers, validates managed gateway and OpenShell readiness, and returns structured failures. Docker lifecycle, Shields handling, forward recovery, tests, E2E evidence, and documentation were updated. ChangesProtected sandbox recovery
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 213-274: Update the parameterized test around startSandbox to
verify that the secret-boundary case also redacts its private detail. Use a
shared row-specific private-detail sentinel or assert each case’s corresponding
reason/detail is absent from the thrown error message, while preserving the
existing expected public-message assertions.
In `@src/lib/actions/sandbox/start.ts`:
- Around line 151-152: Update startSandbox so assertSandboxStartupRecovery is
invoked only when managed recovery is required, while preserving the default
OpenClaw path when resolved.sandbox.agent is unset. Keep custom-agent startup
flowing to verifyGateway and add coverage for an inconclusive recovery result
with a successful readiness probe.
🪄 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: c0a7f5b4-0d2f-4de0-b12a-0813b4bcbd95
📒 Files selected for processing (3)
src/lib/actions/sandbox/connect.tssrc/lib/actions/sandbox/start.test.tssrc/lib/actions/sandbox/start.ts
Signed-off-by: souvikDevloper <138186578+souvikDevloper@users.noreply.github.com>
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. 4 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: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
|
@apurvvkumaria can you please take a look. @cv can i get the ci now |
Signed-off-by: souvikDevloper <138186578+souvikDevloper@users.noreply.github.com>
Co-authored-by: Apurv Kumaria <akumaria@nvidia.com> Signed-off-by: souvikDevloper <138186578+souvikDevloper@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/lib/shields/index.ts (1)
2633-2655: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd pinned startup-access propagation coverage.
startSandboxpassesresult.runtimeHandlethroughprocessRecoverytorestoreStoppedSandboxStartupState. Add a test that passesexpectedContainerIdand asserts thatrestoreLockedStartupAccessreceives it. Current tests cover only theundefinedcase, whilestartSandboxtests replace the restoration bridge with a mock.🤖 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 2633 - 2655, Add test coverage for the pinned-container path through startSandbox, verifying that result.runtimeHandle is propagated by processRecovery into restoreStoppedSandboxStartupState and ultimately passed as expectedContainerId to restoreLockedStartupAccess. Extend the existing restoration-bridge mock assertions while preserving the current undefined-case coverage.Source: Path instructions
src/lib/actions/sandbox/connect.ts (1)
252-276: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winStrip control characters before redaction.
sanitizeSandboxStartupRecoveryDetailcallsredactFullfirst, then removes C0/C1 control characters. A secret-shaped token that contains an embedded control character therefore reachesredactFullin split form and can escape the redaction patterns. It then becomes contiguous after the control characters are replaced with spaces.Reverse the order so redaction runs on normalized text.
🛡️ Proposed reordering
export function sanitizeSandboxStartupRecoveryDetail(raw: string): string { - return redactFull(raw) - .replace(/[\u0000-\u001f\u007f-\u009f]/gu, " ") - .replace(/\s+/gu, " ") - .trim() - .slice(0, 240); + const normalized = raw + .replace(/[\u0000-\u001f\u007f-\u009f]/gu, " ") + .replace(/\s+/gu, " ") + .trim(); + return redactFull(normalized).slice(0, 240); }🤖 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/connect.ts` around lines 252 - 276, Update sanitizeSandboxStartupRecoveryDetail so C0/C1 control characters are removed or normalized before calling redactFull, ensuring secret-shaped tokens become contiguous before redaction; preserve the existing whitespace normalization, trimming, and length limit.src/lib/actions/sandbox/process-recovery.ts (1)
1150-1195: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the shared OpenShell readiness resolution.
This block and the relaunch readiness block at lines 1831-1852 repeat the same structure: build
RecreatedSandboxOpenShellReadyOptionswith abeforeProbemanaged-health guard, branch on whether the injected impl is the default to pickwaitForRecreatedSandboxOpenShellReadyResult, and then map the failure throughrecreatedSandboxOpenShellReadinessFailureDetail.The two copies differ only in the guard closure and the captured health detail. Extract one helper that accepts the guard and a health-detail accessor. This keeps the impl-identity branch in a single place.
🤖 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 1150 - 1195, Extract the duplicated OpenShell readiness resolution from managedStartupOpenShellReadinessFailureDetail and the relaunch readiness block into a shared helper. Have the helper accept the beforeProbe guard and managed-health-detail accessor, construct RecreatedSandboxOpenShellReadyOptions, centralize the waitForRecreatedSandboxOpenShellReady versus injected implementation branch, and map failures through recreatedSandboxOpenShellReadinessFailureDetail while preserving each caller’s distinct guard behavior and captured detail.test/process-recovery-supervisor-relaunch.test.ts (1)
736-741: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueImprove the failure output of the per-call assertion.
The
.every(...)form collapses six calls into one boolean. On failure Vitest reports onlyexpected false to be true. It does not name which call used the wrong action or the wrong container ID.Compare the projected tuples instead. The count assertion on Line 736 stays, and a failure then shows the actual action and container ID per call.
♻️ Proposed assertion 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 boolean .every assertion on requestPinnedGatewaySupervisorAction.mock.calls with an assertion comparing each call’s projected action and container-ID tuple against the expected tuples, while retaining the existing call-count assertion. Ensure failures display the actual values for each call.test/e2e/live/shields-restart-recovery-evidence.ts (1)
972-1130: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider splitting the restart assertion phases.
expectProtectedStopStartRecoveryruns about 160 lines and four distinct verification phases: pre-stop state, post-stop state, post-start state, and managed-control authentication. Each phase already has its own evidence object and assertion group.Extract
assertBeforeStopState,assertStoppedState, andassertAfterStartStatehelpers that receive the capturedStageEvidence. The diagnostics capture and the summary write stay in the orchestrator. This keeps each unit readable without changing behavior or coverage.🤖 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 972 - 1130, Split expectProtectedStopStartRecovery into assertBeforeStopState, assertStoppedState, and assertAfterStartState helpers, each accepting the relevant StageEvidence and required context needed by its existing assertions. Move only the pre-stop, post-stop, and post-start assertion groups into those helpers; keep command execution, diagnostics capture, managed-control authentication, and summary writing in the orchestrator, preserving behavior and coverage.test/e2e/live/shields-config.test.ts (1)
700-722: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMigrate the remaining phase-12 call site to
expectProtectedStopStartRecovery.expectStopStartRecoveryis still called attest/e2e/live/shields-config.test.ts:1132; delete the local helper after migration.🤖 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-config.test.ts` around lines 700 - 722, Update the remaining phase-12 recovery test call site around expectStopStartRecovery to use expectProtectedStopStartRecovery with the equivalent configuration and assertions. After all references are migrated, remove the now-unused local expectStopStartRecovery helper.Source: Path instructions
🤖 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.ts`:
- Around line 199-201: Update requiresManagedStartupRecovery to reuse the
authoritative shouldUseManagedStartupRecovery predicate so startup and process
recovery make the same decision for persisted managed agents with custom session
agents and null gateway inspection. Preserve the resulting managed-recovery
behavior and add a regression test covering this mismatch through startSandbox.
---
Nitpick comments:
In `@src/lib/actions/sandbox/connect.ts`:
- Around line 252-276: Update sanitizeSandboxStartupRecoveryDetail so C0/C1
control characters are removed or normalized before calling redactFull, ensuring
secret-shaped tokens become contiguous before redaction; preserve the existing
whitespace normalization, trimming, and length limit.
In `@src/lib/actions/sandbox/process-recovery.ts`:
- Around line 1150-1195: Extract the duplicated OpenShell readiness resolution
from managedStartupOpenShellReadinessFailureDetail and the relaunch readiness
block into a shared helper. Have the helper accept the beforeProbe guard and
managed-health-detail accessor, construct RecreatedSandboxOpenShellReadyOptions,
centralize the waitForRecreatedSandboxOpenShellReady versus injected
implementation branch, and map failures through
recreatedSandboxOpenShellReadinessFailureDetail while preserving each caller’s
distinct guard behavior and captured detail.
In `@src/lib/shields/index.ts`:
- Around line 2633-2655: Add test coverage for the pinned-container path through
startSandbox, verifying that result.runtimeHandle is propagated by
processRecovery into restoreStoppedSandboxStartupState and ultimately passed as
expectedContainerId to restoreLockedStartupAccess. Extend the existing
restoration-bridge mock assertions while preserving the current undefined-case
coverage.
In `@test/e2e/live/shields-config.test.ts`:
- Around line 700-722: Update the remaining phase-12 recovery test call site
around expectStopStartRecovery to use expectProtectedStopStartRecovery with the
equivalent configuration and assertions. After all references are migrated,
remove the now-unused local expectStopStartRecovery helper.
In `@test/e2e/live/shields-restart-recovery-evidence.ts`:
- Around line 972-1130: Split expectProtectedStopStartRecovery into
assertBeforeStopState, assertStoppedState, and assertAfterStartState helpers,
each accepting the relevant StageEvidence and required context needed by its
existing assertions. Move only the pre-stop, post-stop, and post-start assertion
groups into those helpers; keep command execution, diagnostics capture,
managed-control authentication, and summary writing in the orchestrator,
preserving behavior and coverage.
In `@test/process-recovery-supervisor-relaunch.test.ts`:
- Around line 736-741: Replace the boolean .every assertion on
requestPinnedGatewaySupervisorAction.mock.calls with an assertion comparing each
call’s projected action and container-ID tuple against the expected tuples,
while retaining the existing call-count assertion. Ensure failures display the
actual values for each call.
🪄 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: 9a2ac76e-eaab-40ec-9f1d-ecd9b2c7416f
📒 Files selected for processing (23)
docs/manage-sandboxes/recover-rebuild-sandboxes.mdxdocs/manage-sandboxes/run-sandboxes.mdxdocs/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
Signed-off-by: souvikDevloper <138186578+souvikDevloper@users.noreply.github.com>
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.ts`:
- Around line 263-268: Update the pinnedManagedStartupRecovery selection in the
startup flow to pass preserveContainer: true, then fail startup before
verifyStarted when managed recovery is selected but result.runtimeHandle is
absent. Preserve the existing managed identity checks for valid handles, and add
a regression test covering a managed OpenClaw or Hermes sandbox without a
runtime handle.
🪄 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: bfadb2b6-cdba-4390-b573-a7b0174f6d09
📒 Files selected for processing (5)
src/lib/actions/sandbox/connect.tssrc/lib/actions/sandbox/process-recovery.tssrc/lib/actions/sandbox/start.test.tssrc/lib/actions/sandbox/start.tstest/process-recovery-supervisor-relaunch.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- test/process-recovery-supervisor-relaunch.test.ts
- src/lib/actions/sandbox/start.test.ts
- src/lib/actions/sandbox/connect.ts
- src/lib/actions/sandbox/process-recovery.ts
Signed-off-by: souvikDevloper <138186578+souvikDevloper@users.noreply.github.com>
Signed-off-by: souvikDevloper <138186578+souvikDevloper@users.noreply.github.com>
|
@cv can i get the ci now |
1 similar comment
|
@cv can i get the ci now |
2 participants
Summary
Protected Docker stop/start now preserves one immutable container identity from lifecycle start through managed recovery, OpenShell readiness, required host-forward recovery, and a final authenticated managed-gateway probe.
The same change extends the existing OpenClaw and Hermes Shields live targets to exercise both Shields postures and emit redacted evidence for container identity, registered inference configuration, Shields receipts, protected filesystem state, readiness leases, PID 1, the managed process chain, and host forwards.
Related Issue
Fixes #8662
Changes
Ready, required host forwards, inference-route reconciliation, any applicable best-effort pairing pass, and a final pinned authenticated probe all succeed.recover,shields up,doctor, orrebuildguidance while redacting private diagnostics.shields-configandhermes-shields-configtargets for Shields up and Shields down.Acceptance Evidence
Readyshields-configtarget now runs the public stop/start path under Shields up and asserts the full return barrier. Runtime result is pending vetted Linux/Docker CI on13d6da02d.Readyhermes-shields-configtarget now runs the same protected lifecycle and evidence contract. Runtime result is pending vetted Linux/Docker CI on13d6da02d.startSandboxconsumes the real recovery result; connect-time verification requires OpenShellReadyand forwards; a final nonce-bound authenticated pinned probe runs after reconciliation. Unit tests prove ordering and fail-closed behavior.preserveContainerblocks relaunch, and live evidence asserts unchanged ID, name, creation timestamp, image, and workspace marker.start.test.tsdrives publicstartSandboxthrough productionrestoreSandboxStartupStatefor both agents with an unavailable initial probe and asserts that the real pinned controller result reaches the public failure/success boundary.13d6da02d; NVIDIA runner execution is still required because the contributor PR is awaiting vetting and this Windows host has no Docker Desktop Linux engine.Type of Change
Quality Gates
Documentation Writer Review
docs-updateddocs/manage-sandboxes/recover-rebuild-sandboxes.mdx,docs/manage-sandboxes/run-sandboxes.mdx, anddocs/reference/commands.mdx; independently verified against the pinned lifecycle, managed recovery, forward recovery, Docker identity, Shields startup-access, unit/regression, and four-case live evidence contracts. No blocking documentation finding or unsupported changed-page claim remains./root/documentation_writer_review)DGX Station Hardware Evidence
Verification
Signed-off-by:line and every new commit is SSH-signed; GitHub verification will be visible after pushpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpx tsc -p tsconfig.src.json, semantic E2E phase coverage (125 tests across 81 files), source-shape/test-size budgets, Biome, and the test-conditional scan passednpm run docsbuilds without warnings (doc changes only)Documentation validation passed agent-variant generation, published-route checks, and Fern with zero errors. The full docs/repository umbrella stops on pre-existing CRLF in untouched
docs/resources/starter-prompt.md. Local protected live runs could not start because Docker Desktop's Linux engine is unavailable on this Windows host; the same-commit NVIDIA runner results remain the final acceptance gate.Signed-off-by: souvikDevloper 138186578+souvikDevloper@users.noreply.github.com
Summary by CodeRabbit
New Features
Documentation
Tests