fix(sandbox): continue destroy when pre-delete shields hardening fails (#7727) - #7886
Merged
Conversation
#7727) When `.config-hash` is removed from a locked OpenClaw sandbox, the config guard fails closed by design: `shields down` cannot unlock, its rollback cannot re-lock, and `shields up` refuses the tampered baseline. The pre-delete `shieldsUp` call in `wipeAndHardenLiveSandbox` ran with `throwOnError` and no failure handling, so that same failure escaped `executeSandboxDestroy` and `destroy --yes` exited 1. The sandbox stayed registered with no supported recovery path short of manually restoring trusted config state. Catch only that pre-delete hardening failure, warn with the guard's own detail, and continue with deletion. Tamper detection is unchanged: shields still fails closed, and `hardenedForDelete: false` keeps the delete-abort path from opening a bounded rollback window it never closed. A failed re-lock leaves the auto-restore timer as the only authority that can lock the config again, so `--force` no longer takes the local-cleanup shortcut when the gateway is unreachable after a failed re-lock. Discarding the record there would revoke that authority for a sandbox whose deletion the gateway never confirmed. Signed-off-by: Yimo Jiang <yimoj@nvidia.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
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:
📝 WalkthroughWalkthrough
ChangesSandbox destroy behavior
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant destroySandbox
participant shields
participant Sandbox
Operator->>destroySandbox: destroy sandbox
destroySandbox->>shields: re-lock configuration
shields-->>destroySandbox: re-lock failure
destroySandbox->>Sandbox: attempt deletion
Sandbox-->>destroySandbox: deletion result
destroySandbox-->>Operator: report deletion or preserved recovery state
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 docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 99315ca in the TypeScript / code-coverage/cliThe overall coverage in commit 99315ca in the Show a code coverage summary of the most impacted files.
Updated |
Contributor
|
🌿 Preview your docs: https://nvidia-preview-pr-7886.docs.buildwithfern.com/nemoclaw |
Contributor
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
6 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
4 additional E2E selections from the second opinionAdvisory only. The primary lane did not select these E2E jobs or targets.
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. 3 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. 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. |
…lock (#7727) The destroy reference and the CLI warning said the config stays unlocked until the sandbox is deleted or rebuilt when the delete fails after a failed pre-delete lock. That omitted the auto-restore timer this path deliberately preserves, which keeps retrying the lock and can restore it once the sandbox is reachable again (PR Review Advisor PRA-1). Signed-off-by: Yimo Jiang <yimoj@nvidia.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Each case in this suite spawns a Node child that loads the registry and MCP bridge module graph. `ci/cli-test-timing-hints.json` records the file at 5515ms, above the 5000ms default `testTimeout`, so the suite fails on timing alone whenever its shard is busy. Adding two destroy-flow cases to the same shard was enough to tip it, failing `cli-test-shards (4)` three times in a row while other pull requests passed. Declare the same 15s budget the sibling `mcp-bridge-status-removal` suite already uses for its child-process cases. Signed-off-by: Yimo Jiang <yimoj@nvidia.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…7727) The failed-hardening fallback described the invalid state and the immediate behavior but not why the missing integrity sidecar cannot be repaired at its source, or when the fallback can be removed (PR Review Advisor PRA-1). State the boundary in the repository's SOURCE_OF_TRUTH form: host root removes the sidecar out of band, the locked-posture refusal belongs to the config guard and is deliberate tamper evidence that #7727 keeps, and the fallback goes away once the guard gains a supported authenticated repair for a missing sidecar in the locked posture. Signed-off-by: Yimo Jiang <yimoj@nvidia.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Refresh the branch against origin/main so the pull request tests the current base. Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
Collaborator
Author
|
@coderabbitai resume |
Contributor
✅ Action performedReviews resumed. |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> (cherry picked from commit a4f2b3433f0647f0b69ca009f9722ac146fa1aec)
This was referenced Jul 31, 2026
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>
wscurran
added
area: sandbox
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
apurvvkumaria
approved these changes
Aug 3, 2026
apurvvkumaria
left a comment
apurvvkumaria
left a comment
Collaborator
There was a problem hiding this comment.
Approved after reviewing revision 4c6be5775.
Sensitive-path security review: PASS with no findings across the repository's nine categories:
- Secrets and credentials: no credential sources, values, or persistence change; the caught error remains redacted before logging.
- Input validation and sanitization: sandbox-name handling and existing validated timer-token handling are unchanged.
- Authentication and authorization: OpenShell deletion authority and runtime-provider cleanup authority remain required; the corrected early failure result performs no deletion.
- Dependencies and third-party libraries: no dependency or workflow changes.
- Error handling and logging: pre-delete re-lock failure is reported without leaking sensitive detail; the runtime-provider-authority failure now returns the complete typed failure shape.
- Cryptography and data protection: the config guard's tamper-evidence behavior remains fail-closed; no hash or cryptographic behavior changes.
- Configuration and security controls: deletion may proceed only after the explicit destroy request; if deletion is unconfirmed after re-lock failure, local shields state and the auto-restore timer remain authoritative and forced local cleanup is refused.
- Security testing: focused destroy, shields, MCP, and runtime-provider tests pass (96/96); CLI type-check and repository checks pass.
- System security: successful deletion removes the unguarded configuration with the sandbox, while failed deletion retains recovery authority and avoids creating an unbounded mutable window.
The documentation writer reviewed the completed change, and the documentation build passes with 0 errors. No administrator bypass or gate waiver is used.
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
23 tasks
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
senthilr-nv
approved these changes
Aug 4, 2026
senthilr-nv
left a comment
senthilr-nv
left a comment
Collaborator
There was a problem hiding this comment.
Approved at 99315ca. This is in scope as a fix to the existing destroy lifecycle. Focused tests, CLI type-checking, repository checks, docs build, security review, and documentation writer review pass.
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
4 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
When
.config-hashis removed from a locked OpenClaw sandbox, the config guard fails closed by design:shields downcannot unlock, its rollback cannot re-lock, andshields uprefuses the tampered baseline. The pre-deleteshieldsUpcall inwipeAndHardenLiveSandboxran withthrowOnErrorand no failure handling, so that same failure escapedexecuteSandboxDestroyandnemoclaw <name> destroy --yesexited 1 with the sandbox still registered. Destroy now warns and continues with deletion when that pre-delete re-lock fails, so the stranded sandbox is removable without manually reconstructing trusted config state. Shields tamper detection is unchanged.Related Issue
Fixes #7727
Changes
src/lib/actions/sandbox/destroy-execution.ts:wipeAndHardenLiveSandboxcatches only the pre-deleteshieldsUpfailure, warns with the guard's own redacted detail, and returnshardenedForDelete: falseso the delete-abort path does not open a bounded shields-down rollback window it never closed. The auto-restore timer stays authoritative until deletion succeeds, matching the reasonshieldsUpholds it through its own commit; revoking it earlier would turn a delete that then fails into an unbounded mutable window.src/lib/actions/sandbox/destroy-execution.ts/destroy.ts: a failed re-lock now also disables the--forcelocal-cleanup shortcut when the gateway is unreachable. The timer is then the only authority that can lock the config again, so discarding the local record would revoke it for a sandbox whose deletion the gateway never confirmed. The newshieldsRelockRequiresGatewayresult drives a specific recovery message instead of the generic "re-run with --force" hint.src/lib/actions/sandbox/destroy-flow.test.ts,test/helpers/destroy-flow-test-assertions.ts,test/helpers/destroy-flow-test-harness.ts: regression coverage for delete-proceeds-after-failed-hardening (including warning text andwipe → harden → delete → timer-cleanupordering), MCP restore without a rollback window when delete then fails, and the--forcerefusal above. The harness now exposeswarnSpy.docs/reference/commands.mdx: thedestroyreference documented the old refusal; it now states the warn-and-continue behavior, what happens if that deletion fails, and that a failed pre-delete lock disables the local-only--forcefallback.src/lib/actions/sandbox/mcp-bridge-status-state.test.ts: declare the 15 s budget the siblingmcp-bridge-status-removalsuite already uses. Every case there spawns a Node child that loads the registry and MCP bridge module graph;ci/cli-test-timing-hints.jsonrecords the file at 5515 ms, above the 5000 ms defaulttestTimeout. Adding two destroy-flow cases to the same shard was enough to tip it, failingcli-test-shards (4)three times in a row on this PR while other pull requests passed. Verified by running the whole shard locally (npx vitest run --project cli --shard=4/8→ 140 files / 1710 tests passed).No new flag, configuration, or fallback layer: the reporter's command is
destroy --yes, and it works unchanged.Type of Change
Quality Gates
shields up,shields down,verify-lock.ts,relock-reconfirm.ts, andscripts/openclaw-config-guard.pystill fail closed on a missing.config-hash, proven in the macOS E2E below where steps 4–6 keep failing closed on the fixed build. The only behavior change is that an explicit destroy of that sandbox is no longer blocked, and the--forceguard above prevents the failed-relock path from discarding the auto-restore authority for an unconfirmed deletion. Nine-category security review is PASS: the current-main sync and provider-neutral comment correction add no feature-scope command, secret, authentication, authorization, input, integrity, egress, or dependency risk; error detail remains redacted and unsafe forced cleanup remains fail-closed. Feature-diff fingerprint:6df29299cbacf9d01ba97c254872f5a02354dabe17d057ba58c2b86b6fd800c2.Documentation Writer Review
docs-updateddocs/reference/commands.mdxaccurately documents failed pre-delete re-lock behavior, timer and local-state preservation after failed destroy, lockdown retry behavior, and the disabled local-only fallback. The documentation writer review covered terminology, structure, voice, and code-sample presentation at exact head99315ca94. Focused changed suites passed 54/54.npm run docspassed with 0 errors and 2 pre-existing Fern warnings. CLI type-checking, repository checks, and commit hooks passed.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 unavailable —npm run validate:praftergit fetch origin main: all prek pre-commit hooks, commitlint, and pre-push hooks Passed (exit 0)npm run typecheck:cli,npm run checks, and changed-file hooks passednpm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm test→ 22655 passed, 15 failed, all pre-existing or load-induced and unrelated to this change. Re-running exactly those files with this change stashed reproduces 11 of them on the untouched baseline (test/install-station-dgx-os.test.ts,test/install-station-host-preparation.test.ts,test/install-station-package-transaction.test.ts,src/lib/onboard/docker-gpu-patch-rollback.test.ts,test/issue-5667-hosted-inference-model-namespace.test.ts). The other 4 are 17–30 s timeouts under full-suite parallel load that pass in isolation (test/langchain-deepagents-code-image-credentials.test.ts,test/rebuild-credential-preflight.test.ts×2,test/gateway-state-reconcile-2276.test.ts).npm run docsbuilds without warnings (doc changes only) — exit 0, 0 errors; 2 pre-existing environment warnings noted aboveReporter-workflow E2E (macOS arm64, real worktree CLI)
Host
h7yr45lq41.dyn.nvidia.com: macOS 26.5.2, arm64 (M4 Pro), Docker 29.3.1 (Colima), OpenShell 0.0.85, live OpenClaw sandbox on gatewaynemoclaw-8091. All commands run the worktree CLI (node ./bin/nemoclaw.js), never a globalnemoclaw.Before the fix (worktree built at
main4dcb89ea1), reporter steps 1–7 on live sandboxsb-7430-clone:nemoclaw listandopenshell sandbox listboth still showedsb-7430-clone— the reporter's Actual Result reproduced exactly.After the fix (worktree rebuilt from validated PR revision
ba66d0407, fresh sandboxsb-7727-finalrestored from a snapshot, identical steps 1–6 still failing closed):After that run
nemoclaw listandopenshell sandbox listshow only the unrelatedsb-7430, and the sandbox's shields state and timer marker are gone (leftover_shields_artifacts=0), confirming the post-delete timer/shields cleanup still runs. Steps 4–6 on the fixed build still exit 1 with the same tamper evidence, so the fail-closed boundary the issue explicitly preserves is intact.Signed-off-by: Yimo Jiang yimoj@nvidia.com
Summary by CodeRabbit
Bug Fixes
Tests