diff --git a/docs/reference/commands.mdx b/docs/reference/commands.mdx index a65907c237c..0fdd6ca11ad 100644 --- a/docs/reference/commands.mdx +++ b/docs/reference/commands.mdx @@ -1782,7 +1782,11 @@ If sandbox deletion is refused after destroy has restored lockdown, NemoClaw ope If a shields auto-restore timer is active, `destroy` holds the same per-sandbox transition through state wipe and deletion. It restores and verifies lockdown and revokes the active timer before deletion. It clears the remaining local shields state only after deletion succeeds. -If hardening fails, the command refuses deletion and leaves the timer authority available to retry lockdown. +If the pre-delete re-lock fails, the command warns and attempts to destroy the sandbox. +If the destroy operation succeeds, it destroys the sandbox and deletes its unguarded configuration. +If the destroy operation fails, NemoClaw keeps the local shields state and the auto-restore timer. +The timer keeps retrying lockdown and can restore it after the sandbox is reachable again. +NemoClaw records Shields down until a retry verifies lockdown, or until you destroy or rebuild the sandbox. If deletion fails after hardening, the command keeps the surviving sandbox's locked shields state instead of cleaning it up as though deletion succeeded. By default, unattended final-sandbox destroys (`--yes`, `--force`, or `NEMOCLAW_NON_INTERACTIVE=1`) remove the shared NemoClaw gateway on macOS so the host listener is released, while Linux preserves it for reuse. Pass `--cleanup-gateway` to force removal, or `--no-cleanup-gateway` to force preservation. @@ -1804,6 +1808,7 @@ If the OpenShell gateway is unreachable and the sandbox has no managed MCP owner Gateway-side deletion remains unconfirmed, shared host-service and gateway teardown are skipped, and the sandbox and retained volume may still exist if the gateway returns. Start the gateway with `$$nemoclaw status` and retry destroy when you need a confirmed deletion. Managed MCP ownership disables the local-only fallback because exact provider cleanup requires the retained ownership state, and other delete failures remain fatal. +A failed pre-delete re-lock also disables the local-only fallback, because the auto-restore timer is then the only authority that can lock the configuration again after the gateway returns. ```bash $$nemoclaw my-assistant destroy [--yes|-y|--force] [--cleanup-gateway|--no-cleanup-gateway] diff --git a/src/lib/actions/sandbox/destroy-execution.ts b/src/lib/actions/sandbox/destroy-execution.ts index b5b0c94b66d..91bf4cd38a5 100644 --- a/src/lib/actions/sandbox/destroy-execution.ts +++ b/src/lib/actions/sandbox/destroy-execution.ts @@ -61,10 +61,12 @@ export type SandboxDestroyExecutionResult = gatewayUnreachable: boolean; mcpOwnershipRequiresGateway: boolean; mcpRecoveryFailure?: string; + shieldsRelockRequiresGateway: boolean; }; type HardenedDeleteState = { hardenedForDelete: boolean; + hardeningFailed: boolean; timerProcessToken?: string; }; @@ -103,23 +105,72 @@ function wipeAndHardenLiveSandbox( sandboxConfirmedAbsent: boolean, deps: NonNullable = {}, ): HardenedDeleteState { - if (sandboxConfirmedAbsent) return { hardenedForDelete: false }; + if (sandboxConfirmedAbsent) return { hardenedForDelete: false, hardeningFailed: false }; // Wipe before delete while the retained volume is still mounted. The caller // holds the timer-bound lock across this phase and all following teardown. (deps.wipeSandboxState ?? wipeSandboxState)(sandboxName); const timerMarker = (deps.readTimerMarker ?? readTimerMarker)(sandboxName); - if (!timerMarker) return { hardenedForDelete: false }; + if (!timerMarker) return { hardenedForDelete: false, hardeningFailed: false }; const timerProcessToken = /^[0-9a-f]{32}$/.test(timerMarker.processToken ?? "") ? timerMarker.processToken : undefined; const { shieldsUp } = require("../../shields") as typeof import("../../shields"); - shieldsUp(sandboxName, { - throwOnError: true, - allowLegacyHermesProtocol: true, - }); - return { hardenedForDelete: true, timerProcessToken }; + try { + shieldsUp(sandboxName, { + throwOnError: true, + allowLegacyHermesProtocol: true, + }); + } catch (error) { + /** + * SOURCE_OF_TRUTH + * Invalid state: the locked OpenClaw config pair lost its `.config-hash` + * integrity sidecar, so the guard refuses every lock transition and the + * sandbox cannot be re-hardened (#7727). + * Source boundary: the sidecar is removed out of band by host root inside + * the sandbox (the reporter used a privileged runtime command). The + * refusal itself belongs to `scripts/openclaw-config-guard.py`, which + * repairs an absent hash only in lock-from-mutable mode and fail-stops in + * the locked posture on purpose. + * Source-fix constraint: NemoClaw cannot stop host root from deleting a + * file inside the sandbox, and regenerating the hash from the current + * bytes in the locked posture is exactly the tamper laundering the guard + * exists to prevent — #7727 explicitly keeps that fail-closed. So destroy + * cannot repair this state at its source; it can only stop treating a + * best-effort hardening step as a precondition for removal. + * Regression proof: destroy-flow.test.ts covers delete-proceeds-after- + * failed-hardening, the emitted warning, timer cleanup ordering, and MCP + * restore when the delete then fails. + * Removal condition: drop this fallback when the config guard gains a + * supported authenticated repair for a missing sidecar in the locked + * posture, so a pre-delete `shieldsUp` can be required again. + * + * Hardening before delete narrows the open shields-down window; it is not + * a precondition for removal. Deleting removes the unguarded config along + * with the sandbox, so report the failure and continue. + * `hardenedForDelete: false` keeps the delete-abort path honest: it will + * not open a bounded shields-down rollback window it never closed. + */ + // The auto-restore timer stays authoritative until deletion succeeds, for + // the same reason `shieldsUp` keeps it through its own commit: revoking it + // here would turn a delete that then fails into an unbounded mutable + // window. At the absolute deadline it may stop this exact token-bound + // destroy owner and its subprocesses before reclaiming the transition + // lock, then restore lockdown. The token and process identity checks make + // a mismatched owner fail closed instead of signaling an unrelated process. + const detail = redact(error instanceof Error ? error.message : String(error)); + console.warn( + ` ${YW}⚠${R} Could not re-lock shields for '${sandboxName}' before delete: ${detail}`, + ); + console.warn( + ` Continuing with delete — '${sandboxName}' and its unguarded config are removed together. ` + + "If the delete fails, the auto-restore timer keeps retrying the lock until it succeeds " + + "or the sandbox is deleted or rebuilt.", + ); + return { hardenedForDelete: false, hardeningFailed: true, timerProcessToken }; + } + return { hardenedForDelete: true, hardeningFailed: false, timerProcessToken }; } async function restoreMcpAfterDeleteAbort( @@ -214,6 +265,7 @@ export async function executeSandboxDestroy({ exitCode: 1, gatewayUnreachable: false, mcpOwnershipRequiresGateway: false, + shieldsRelockRequiresGateway: false, }; } } @@ -244,8 +296,18 @@ export async function executeSandboxDestroy({ alreadyGone, gatewayUnreachable, } = getSandboxDeleteOutcome(deleteResult); + // #7727: a failed pre-delete re-lock leaves the auto-restore timer as the + // only authority that can lock the config again. Discarding the local + // record here would revoke it for a sandbox the gateway never confirmed + // deleting, so --force must not take the local-cleanup shortcut until the + // gateway is back and deletion is confirmed. const forcedLocalCleanup = - deleteResult.status !== 0 && !alreadyGone && gatewayUnreachable && force && !hasMcpOwnership; + deleteResult.status !== 0 && + !alreadyGone && + gatewayUnreachable && + force && + !hasMcpOwnership && + !hardened.hardeningFailed; if (deleteResult.status !== 0 && !alreadyGone && !forcedLocalCleanup) { const mcpRecoveryFailure = sandboxConfirmedAbsent @@ -258,6 +320,7 @@ export async function executeSandboxDestroy({ gatewayUnreachable, mcpOwnershipRequiresGateway: gatewayUnreachable && hasMcpOwnership, mcpRecoveryFailure, + shieldsRelockRequiresGateway: gatewayUnreachable && hardened.hardeningFailed, }; } diff --git a/src/lib/actions/sandbox/destroy-flow.test.ts b/src/lib/actions/sandbox/destroy-flow.test.ts index 8ff8c5ac761..22c68b57c15 100644 --- a/src/lib/actions/sandbox/destroy-flow.test.ts +++ b/src/lib/actions/sandbox/destroy-flow.test.ts @@ -7,7 +7,9 @@ import { expectAbsentSandboxMcpFinalize, expectActiveTimerDestroyOrder, expectFailedDeletePreservesHostState, - expectFailedHardeningStopsDelete, + expectFailedHardeningMcpRestore, + expectFailedHardeningRefusesForcedCleanup, + expectFailedHardeningStillDeletes, expectFailedMcpFinalizePreservesRegistry, expectFailedMcpRestorePreservesDestroyFailure, expectMcpFinalizeAfterDelete, @@ -265,17 +267,48 @@ describe("destroySandbox flow", () => { expectActiveTimerDestroyOrder(harness); }); - it("does not delete when active-window hardening fails after the wipe", async () => { + it("warns and still deletes when active-window hardening fails after the wipe (#7727)", async () => { const harness = createDestroyHarness({ activeTimer: true, shieldsUpError: new Error("injected hardening failure"), }); - await expect(harness.destroySandbox("alpha", { yes: true })).rejects.toThrow( - "injected hardening failure", + await expect(harness.destroySandbox("alpha", { yes: true })).resolves.toBeUndefined(); + + expectFailedHardeningStillDeletes(harness); + expect(exitSpy).not.toHaveBeenCalled(); + }); + + it("keeps the timer and local record when --force cannot confirm deletion after failed hardening (#7727)", async () => { + const harness = createDestroyHarness({ + activeTimer: true, + deleteStatus: 1, + deleteOutput: "error trying to connect: connection refused", + registeredSandboxCount: 1, + shieldsUpError: new Error("injected hardening failure"), + }); + + await expect(harness.destroySandbox("alpha", { force: true })).rejects.toThrow( + "process.exit(1)", ); - expectFailedHardeningStopsDelete(harness); + expectFailedHardeningRefusesForcedCleanup(harness); + expect(exitSpy).toHaveBeenCalledWith(1); + }); + + it("restores MCP runtime state without a rollback window when delete fails after failed hardening (#7727)", async () => { + const harness = createDestroyHarness({ + activeTimer: true, + deleteStatus: 7, + deleteOutput: "delete failed", + mcpServers: ["github"], + shieldsUpError: new Error("injected hardening failure"), + }); + + await expect(harness.destroySandbox("alpha", { yes: true })).rejects.toThrow("process.exit(7)"); + + expectFailedHardeningMcpRestore(harness); + expect(exitSpy).toHaveBeenCalledWith(7); }); it("detaches MCP providers before delete and finalizes them only after delete succeeds", async () => { diff --git a/src/lib/actions/sandbox/destroy.ts b/src/lib/actions/sandbox/destroy.ts index 2aac5e301b9..4a9ac783596 100644 --- a/src/lib/actions/sandbox/destroy.ts +++ b/src/lib/actions/sandbox/destroy.ts @@ -482,7 +482,14 @@ async function destroySandboxUnlocked( } console.error(` Failed to destroy sandbox '${sandboxName}'.`); if (destructiveResult.gatewayUnreachable) { - if (destructiveResult.mcpOwnershipRequiresGateway) { + if (destructiveResult.shieldsRelockRequiresGateway) { + console.error( + ` The OpenShell gateway is unreachable and shields could not be re-locked before delete. Local state was preserved so the auto-restore timer can still lock the config when the gateway returns.`, + ); + console.error( + ` Start the gateway (run '${CLI_NAME} ${sandboxName} status'), then retry destroy; --force cannot safely discard a record whose config lock is unconfirmed.`, + ); + } else if (destructiveResult.mcpOwnershipRequiresGateway) { console.error( ` The OpenShell gateway is unreachable. Local state was preserved because it contains MCP ownership required for exact provider cleanup.`, ); diff --git a/src/lib/actions/sandbox/mcp-bridge-status-state.test.ts b/src/lib/actions/sandbox/mcp-bridge-status-state.test.ts index f909f11d162..0cbc19b27a7 100644 --- a/src/lib/actions/sandbox/mcp-bridge-status-state.test.ts +++ b/src/lib/actions/sandbox/mcp-bridge-status-state.test.ts @@ -8,6 +8,8 @@ import path from "node:path"; import { afterEach, describe, expect, it } from "vitest"; +import { testTimeoutOptions } from "../../../../test/helpers/timeouts"; + const sourceRequireHook = path.resolve("test/helpers/onboard-script-mocks.cjs"); const sourceNodeOptions = [process.env.NODE_OPTIONS, `--require=${sourceRequireHook}`] .filter(Boolean) @@ -25,7 +27,11 @@ afterEach(() => { tempHomes.clear(); }); -describe("cross-agent MCP status state", () => { +// Each case spawns a Node child that loads the registry and MCP bridge module +// graph, which the recorded timing hints put at ~5.5s — over the 5s default. +// Use the same budget as the sibling `mcp-bridge-status-removal` suite so a +// loaded shard cannot fail these on timing alone. +describe("cross-agent MCP status state", testTimeoutOptions(15_000), () => { it("rejects duplicate static credential keys across bridges in one sandbox", () => { const home = createTempHome("nemoclaw-mcp-env-key-"); const script = ` diff --git a/src/lib/shields/flow.test.ts b/src/lib/shields/flow.test.ts index b28e8cec047..ac155fea696 100644 --- a/src/lib/shields/flow.test.ts +++ b/src/lib/shields/flow.test.ts @@ -810,6 +810,116 @@ describe("shields command flow", () => { } }); + it("lets an expired timer preempt its token-bound destroy owner and restore lockdown", { + timeout: 20_000, + }, async () => { + const transitionLockPath = path.join(import.meta.dirname, "transition-lock.ts"); + const stateDir = path.join(tmpDir, ".nemoclaw", "state"); + const sandboxName = "destroy-deadline"; + const processToken = "e".repeat(32); + const snapshotPath = path.join(stateDir, "policy-snapshot-destroy.yaml"); + const readyPath = path.join(stateDir, "destroy-owner.ready"); + const lockPath = path.join(stateDir, `shields-transition-lock-${sandboxName}.json`); + const markerPath = path.join(stateDir, `shields-timer-${sandboxName}.json`); + const statePath = path.join(stateDir, `shields-${sandboxName}.json`); + fs.mkdirSync(stateDir, { recursive: true }); + fs.writeFileSync(snapshotPath, "version: 1\nnetwork_policies:\n test: {}\n"); + fs.writeFileSync( + statePath, + JSON.stringify({ + shieldsDown: true, + shieldsDownAt: new Date(Date.now() - 60_000).toISOString(), + shieldsDownTimeout: 60, + shieldsDownReason: "destroy takeover coverage", + shieldsDownPolicy: "permissive", + shieldsPolicySnapshotPath: snapshotPath, + updatedAt: new Date().toISOString(), + }), + { mode: 0o600 }, + ); + fs.writeFileSync( + markerPath, + JSON.stringify({ + pid: 999_999, + sandboxName, + snapshotPath, + restoreAt: new Date(Date.now() - 1_000).toISOString(), + processToken, + }), + { mode: 0o600 }, + ); + + const owner = spawn( + process.execPath, + [ + "--import", + "tsx", + "-e", + [ + `const {withShieldsTransitionLock}=require(${JSON.stringify(transitionLockPath)})`, + "const fs=require('fs')", + "const [name,token,ready]=process.argv.slice(1)", + "withShieldsTransitionLock(name,'destroy sandbox',()=>{fs.writeFileSync(ready,'ready');Atomics.wait(new Int32Array(new SharedArrayBuffer(4)),0,0,10000)},{takeoverToken:token})", + ].join(";"), + sandboxName, + processToken, + readyPath, + ], + { env: { ...process.env, HOME: tmpDir }, stdio: "ignore" }, + ); + + try { + await vi.waitFor( + () => { + expect(fs.existsSync(readyPath)).toBe(true); + expect(fs.existsSync(lockPath)).toBe(true); + }, + { timeout: 5_000, interval: 10 }, + ); + const timerControl = requireDist("./timer-control.js"); + const ownerStartIdentity = timerControl.readProcessStartIdentity(owner.pid); + expect(ownerStartIdentity).toBeTypeOf("string"); + const harness = createHarness({ + dockerExecFileSync: (argv: unknown) => { + const args = Array.isArray(argv) ? argv.map(String) : []; + switch (true) { + case args.includes("sha256sum"): + return `${"a".repeat(64)} ${String(args.at(-1))}\n`; + case args.includes("lsattr"): + return `----i---------e----- ${String(args.at(-1))}\n`; + case args.includes("stat"): + return args.at(-1) === "/sandbox" + ? "1775 root:sandbox\n" + : args.at(-1) === "/sandbox/.openclaw" + ? "755 root:root\n" + : "444 root:root\n"; + default: + return ""; + } + }, + }); + + harness.shieldsStatus(sandboxName); + + const ownerState = timerControl.readProcessState(owner.pid); + expect(ownerState === null || ownerState.startsWith("Z")).toBe(true); + expect(fs.existsSync(lockPath)).toBe(false); + expect(JSON.parse(fs.readFileSync(statePath, "utf-8"))).toMatchObject({ + shieldsDown: false, + shieldsDownAt: null, + }); + expect(fs.existsSync(markerPath)).toBe(false); + expect(harness.runSpy).toHaveBeenCalledWith( + ["openshell", "policy", "set"], + expect.objectContaining({ ignoreError: true }), + ); + expect(harness.logSpy).toHaveBeenCalledWith(" Shields: UP (lockdown active)"); + } finally { + owner.kill("SIGCONT"); + owner.kill("SIGKILL"); + } + }); + it("publishes preparing recovery ownership before weakening and active only after unlock", () => { const stateDir = path.join(tmpDir, ".nemoclaw", "state"); let observedPreparingDuringPolicy = false; diff --git a/test/helpers/destroy-flow-test-assertions.ts b/test/helpers/destroy-flow-test-assertions.ts index ac9b1bb5508..66b12c86aa0 100644 --- a/test/helpers/destroy-flow-test-assertions.ts +++ b/test/helpers/destroy-flow-test-assertions.ts @@ -112,11 +112,49 @@ export function expectActiveTimerDestroyOrder(harness: DestroyHarness): void { expect(harness.events.indexOf("delete")).toBeLessThan(harness.events.indexOf("timer-cleanup")); } -export function expectFailedHardeningStopsDelete(harness: DestroyHarness): void { - expect(harness.events).toContain("wipe"); - expect(harness.events).toContain("harden"); - expect(harness.events).not.toContain("delete"); +export function expectFailedHardeningStillDeletes(harness: DestroyHarness): void { + expect(harness.events).toEqual( + expect.arrayContaining(["wipe", "harden", "delete", "timer-cleanup"]), + ); + expect(harness.events.indexOf("wipe")).toBeLessThan(harness.events.indexOf("harden")); + expect(harness.events.indexOf("harden")).toBeLessThan(harness.events.indexOf("delete")); + expect(harness.events.indexOf("delete")).toBeLessThan(harness.events.indexOf("timer-cleanup")); + expect(harness.removeSandboxSpy).toHaveBeenCalledWith("alpha"); + expect(harness.killTimerSpy).toHaveBeenCalledTimes(1); + const warnOutput = harness.warnSpy.mock.calls.map((call) => String(call[0])).join("\n"); + expect(warnOutput).toContain("Could not re-lock shields for 'alpha' before delete"); + expect(warnOutput).toContain("injected hardening failure"); + expect(warnOutput).toContain("Continuing with delete"); +} + +export function expectFailedHardeningRefusesForcedCleanup(harness: DestroyHarness): void { + expect(harness.events).toEqual(expect.arrayContaining(["harden", "delete"])); + // The auto-restore timer is the only remaining authority that can lock the + // config again, so an unconfirmed delete must keep it and the local record. expect(harness.killTimerSpy).not.toHaveBeenCalled(); + expect(harness.removeSandboxSpy).not.toHaveBeenCalled(); + expect(harness.stopAllSpy).not.toHaveBeenCalled(); + expect(harness.cleanupGatewaySpy).not.toHaveBeenCalled(); + const errorOutput = harness.errorSpy.mock.calls.map((call) => String(call[0])).join("\n"); + expect(errorOutput).toContain("shields could not be re-locked before delete"); + expect(errorOutput).toContain("--force cannot safely discard a record whose config lock"); + expect(errorOutput).not.toContain("re-run with --force to remove the local sandbox record"); +} + +export function expectFailedHardeningMcpRestore(harness: DestroyHarness): void { + expect(harness.events).toEqual(expect.arrayContaining(["harden", "delete", "mcp-restore"])); + expect(harness.events.indexOf("harden")).toBeLessThan(harness.events.indexOf("delete")); + expect(harness.events.indexOf("delete")).toBeLessThan(harness.events.indexOf("mcp-restore")); + // No lock was re-established, so destroy must not open a bounded + // shields-down rollback window it cannot close again. + expect(harness.events).not.toContain("unlock"); + expect(harness.shieldsDownSpy).not.toHaveBeenCalled(); + expect(harness.restoreMcpBridgesAfterDestroyAbortSpy).toHaveBeenCalledWith( + "alpha", + expect.objectContaining({ entries: [{ server: "github" }] }), + ); + expect(harness.finalizeMcpBridgesAfterSandboxDeleteSpy).not.toHaveBeenCalled(); + expect(harness.removeSandboxSpy).not.toHaveBeenCalled(); } export function expectMcpFinalizeAfterDelete(harness: DestroyHarness): void {