From 4f0f7ccd07312d15f473b5137f4becfcf3099d93 Mon Sep 17 00:00:00 2001 From: Yanyun Liao Date: Wed, 5 Aug 2026 12:37:35 +0800 Subject: [PATCH 01/11] fix(sandbox): catch McpBridgeError from MCP bridge prepare and finalize in destroy path (#8103) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `executeSandboxDestroy` called `prepareMcpDestroy` and `finalizeMcpDestroy` with no `McpBridgeError` guard. When a managed MCP server is present and the gateway becomes unreachable, `inspectExactMcpDestroyProvider` throws `McpBridgeError("Could not inspect OpenShell provider…")` which propagated uncaught, crashing `destroy --yes` with a stack trace instead of a clean exit-1 message. The same uncaught escape existed for `finalizeMcpDestroy`'s internal re-throw after post-delete cleanup fails. Both call sites now catch `McpBridgeError` and return `{ ok: false, … }`, letting the existing failure-path rendering in `destroy.ts` surface the error cleanly and exit with the error's own `exitCode`. Signed-off-by: yanyunl1991 --- src/lib/actions/sandbox/destroy-execution.ts | 39 ++++++++++++++++---- src/lib/actions/sandbox/destroy-flow.test.ts | 24 ++++++++++++ test/helpers/destroy-flow-test-assertions.ts | 17 +++++++++ test/helpers/destroy-flow-test-harness.ts | 18 +++++++-- 4 files changed, 87 insertions(+), 11 deletions(-) diff --git a/src/lib/actions/sandbox/destroy-execution.ts b/src/lib/actions/sandbox/destroy-execution.ts index 91bf4cd38a5..666eae7a353 100644 --- a/src/lib/actions/sandbox/destroy-execution.ts +++ b/src/lib/actions/sandbox/destroy-execution.ts @@ -20,6 +20,7 @@ import type { SandboxEntry } from "../../state/registry"; import type { DestroyRunOpenshell } from "./destroy-gateway"; import { finalizeMcpBridgesAfterSandboxDelete, + McpBridgeError, type McpDestroyPreparation, prepareMcpBridgesForAbsentSandboxDestroy, prepareMcpBridgesForDestroy, @@ -269,12 +270,22 @@ export async function executeSandboxDestroy({ }; } } - const mcpPreparation = await prepareMcpDestroy( - sandboxName, - sandbox, - sandboxConfirmedAbsent, - force, - ); + let mcpPreparation: McpDestroyPreparation; + try { + mcpPreparation = await prepareMcpDestroy(sandboxName, sandbox, sandboxConfirmedAbsent, force); + } catch (error) { + if (error instanceof McpBridgeError) { + return { + ok: false as const, + deleteOutput: redactDestroyError(error), + exitCode: error.exitCode, + gatewayUnreachable: false, + mcpOwnershipRequiresGateway: false, + shieldsRelockRequiresGateway: false, + }; + } + throw error; + } // Prepared-only/incomplete adds have no external resources and are safely // discarded during preparation. Remaining entries are the durable exact // provider ownership manifest and must survive an unconfirmed delete. @@ -329,7 +340,21 @@ export async function executeSandboxDestroy({ // stale timer state cannot target a same-name replacement. cleanupShieldsArtifacts(sandboxName); if (!forcedLocalCleanup) { - await finalizeMcpDestroy(sandboxName, mcpPreparation, force); + try { + await finalizeMcpDestroy(sandboxName, mcpPreparation, force); + } catch (error) { + if (error instanceof McpBridgeError) { + return { + ok: false as const, + deleteOutput: redactDestroyError(error), + exitCode: error.exitCode, + gatewayUnreachable: false, + mcpOwnershipRequiresGateway: false, + shieldsRelockRequiresGateway: false, + }; + } + throw error; + } } return { ok: true as const, diff --git a/src/lib/actions/sandbox/destroy-flow.test.ts b/src/lib/actions/sandbox/destroy-flow.test.ts index 22c68b57c15..c2e857fa713 100644 --- a/src/lib/actions/sandbox/destroy-flow.test.ts +++ b/src/lib/actions/sandbox/destroy-flow.test.ts @@ -12,7 +12,9 @@ import { expectFailedHardeningStillDeletes, expectFailedMcpFinalizePreservesRegistry, expectFailedMcpRestorePreservesDestroyFailure, + expectMcpFinalizeBridgeErrorReturnsFailure, expectMcpFinalizeAfterDelete, + expectMcpPrepareBridgeErrorAborts, expectMcpRestoreAfterDeleteFailure, expectShieldsUpRefusalBeforeMutation, expectStrictSandboxPresenceClassification, @@ -371,4 +373,26 @@ describe("destroySandbox flow", () => { expectAbsentSandboxMcpFinalize(harness); }); + + it("exits with code 1 when MCP bridge prepare throws McpBridgeError, gateway down (#8103)", async () => { + const harness = createDestroyHarness({ + mcpServers: ["github"], + prepareMcpBridgeError: "Could not inspect OpenShell provider: gateway unreachable", + }); + + await expect(harness.destroySandbox("alpha", { yes: true })).rejects.toThrow("process.exit(1)"); + + expectMcpPrepareBridgeErrorAborts(harness); + }); + + it("exits with code 1 when MCP bridge finalize throws McpBridgeError after sandbox delete (#8103)", async () => { + const harness = createDestroyHarness({ + mcpServers: ["github"], + finalizeMcpBridgeError: "Could not inspect OpenShell provider: gateway unreachable", + }); + + await expect(harness.destroySandbox("alpha", { yes: true })).rejects.toThrow("process.exit(1)"); + + expectMcpFinalizeBridgeErrorReturnsFailure(harness); + }); }); diff --git a/test/helpers/destroy-flow-test-assertions.ts b/test/helpers/destroy-flow-test-assertions.ts index 66b12c86aa0..a52c8983eb4 100644 --- a/test/helpers/destroy-flow-test-assertions.ts +++ b/test/helpers/destroy-flow-test-assertions.ts @@ -219,6 +219,23 @@ export function expectFailedMcpFinalizePreservesRegistry(harness: DestroyHarness expect(harness.cleanupGatewaySpy).not.toHaveBeenCalled(); } +export function expectMcpPrepareBridgeErrorAborts(harness: DestroyHarness): void { + expect(harness.prepareMcpBridgesForDestroySpy).toHaveBeenCalled(); + // No delete should happen when MCP prepare itself throws McpBridgeError. + expect(harness.runOpenshellSpy).not.toHaveBeenCalledWith( + expect.arrayContaining(["sandbox", "delete"]), + expect.anything(), + ); + expect(harness.removeSandboxSpy).not.toHaveBeenCalled(); +} + +export function expectMcpFinalizeBridgeErrorReturnsFailure(harness: DestroyHarness): void { + expect(harness.finalizeMcpBridgesAfterSandboxDeleteSpy).toHaveBeenCalled(); + // Registry must not be cleaned up when post-delete MCP finalize throws McpBridgeError. + expect(harness.removeSandboxSpy).not.toHaveBeenCalled(); + expect(harness.cleanupGatewaySpy).not.toHaveBeenCalled(); +} + export function expectAbsentSandboxMcpFinalize(harness: DestroyHarness): void { expect(harness.prepareMcpBridgesForDestroySpy).not.toHaveBeenCalled(); expect(harness.prepareMcpBridgesForAbsentSandboxDestroySpy).toHaveBeenCalledWith("alpha", { diff --git a/test/helpers/destroy-flow-test-harness.ts b/test/helpers/destroy-flow-test-harness.ts index ec7dd59f83b..2747c3a1090 100644 --- a/test/helpers/destroy-flow-test-harness.ts +++ b/test/helpers/destroy-flow-test-harness.ts @@ -50,12 +50,14 @@ type DestroyHarnessOptions = { deleteStatus?: number; dockerPsOutput?: string; endpointUrl?: string; + finalizeMcpBridgeError?: string; finalizeMcpError?: string; imageTag?: string | null; liveListOutput?: string; mcpAddState?: "prepared"; mcpServers?: string[]; openshellDriver?: string; + prepareMcpBridgeError?: string; promptResponses?: string[]; registeredSandboxCount?: number; restoreMcpError?: string; @@ -294,9 +296,14 @@ export function createDestroyHarness(options: DestroyHarnessOptions = {}): Destr destroyAlreadyPending: false, }; const gatewayPinsAtMcpPrepare: Array = []; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const { McpBridgeError } = mcpBridge as any; const prepareMcpBridgesForDestroySpy = vi .spyOn(mcpBridge, "prepareMcpBridgesForDestroy") .mockImplementation(async () => { + if (options.prepareMcpBridgeError !== undefined) { + throw new McpBridgeError(options.prepareMcpBridgeError); + } gatewayPinsAtMcpPrepare.push(process.env.OPENSHELL_GATEWAY); return mcpPreparation; }); @@ -316,11 +323,14 @@ export function createDestroyHarness(options: DestroyHarnessOptions = {}): Destr }); const finalizeMcpBridgesAfterSandboxDeleteSpy = vi .spyOn(mcpBridge, "finalizeMcpBridgesAfterSandboxDelete") - .mockImplementation(() => - options.finalizeMcpError + .mockImplementation(() => { + if (options.finalizeMcpBridgeError !== undefined) { + return Promise.reject(new McpBridgeError(options.finalizeMcpBridgeError)); + } + return options.finalizeMcpError ? Promise.reject(new Error(options.finalizeMcpError)) - : Promise.resolve(), - ); + : Promise.resolve(); + }); logSpy.mockClear(); From 16925c7a6f1011730e2398b16d7435bd8a2a5009 Mon Sep 17 00:00:00 2001 From: Carlos Villela Date: Wed, 5 Aug 2026 01:46:12 -0700 Subject: [PATCH 02/11] chore(architecture): ratchet source budgets Signed-off-by: Carlos Villela --- ci/source-architecture-budget.json | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ci/source-architecture-budget.json b/ci/source-architecture-budget.json index 4e3f1dacd40..dc2d40b9100 100644 --- a/ci/source-architecture-budget.json +++ b/ci/source-architecture-budget.json @@ -27,7 +27,7 @@ "src/lib/runner.ts": 88, "src/lib/security/redact.ts": 51, "src/lib/state/onboard-session.ts": 36, - "src/lib/state/registry.ts": 99, + "src/lib/state/registry.ts": 98, "src/lib/state/state-root.ts": 22, "src/lib/subprocess-env.ts": 24, "src/lib/validation.ts": 25 @@ -47,7 +47,7 @@ "src/lib/actions/uninstall/run-plan.ts": 26, "src/lib/inference/onboard-probes.ts": 21, "src/lib/inference/vllm.ts": 21, - "src/lib/onboard.ts": 222, + "src/lib/onboard.ts": 219, "src/lib/onboard/machine/handlers/sandbox.ts": 21, "src/lib/sandbox/config.ts": 22, "src/lib/shields/index.ts": 23 @@ -55,7 +55,7 @@ }, "allowedCycles": [], "maxRootFiles": { - "src/lib/onboard": 308, + "src/lib/onboard": 307, "src/lib/actions": 19, "src/lib/actions/sandbox": 184, "src/lib/state": 37, From a6f84d51f2df2cdf42571a0b40692759824de490 Mon Sep 17 00:00:00 2001 From: Carlos Villela Date: Wed, 5 Aug 2026 02:00:19 -0700 Subject: [PATCH 03/11] docs(test): state destroy ordering invariant Signed-off-by: Carlos Villela --- test/helpers/destroy-flow-test-assertions.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/helpers/destroy-flow-test-assertions.ts b/test/helpers/destroy-flow-test-assertions.ts index a52c8983eb4..2b467d67034 100644 --- a/test/helpers/destroy-flow-test-assertions.ts +++ b/test/helpers/destroy-flow-test-assertions.ts @@ -221,7 +221,7 @@ export function expectFailedMcpFinalizePreservesRegistry(harness: DestroyHarness export function expectMcpPrepareBridgeErrorAborts(harness: DestroyHarness): void { expect(harness.prepareMcpBridgesForDestroySpy).toHaveBeenCalled(); - // No delete should happen when MCP prepare itself throws McpBridgeError. + // MCP preparation must succeed before sandbox deletion begins. expect(harness.runOpenshellSpy).not.toHaveBeenCalledWith( expect.arrayContaining(["sandbox", "delete"]), expect.anything(), From 9efe01c14b0ead24e82d5615c11fb7fed3910603 Mon Sep 17 00:00:00 2001 From: Carlos Villela Date: Wed, 5 Aug 2026 03:20:59 -0700 Subject: [PATCH 04/11] fix(sandbox): harden MCP destroy recovery Signed-off-by: Carlos Villela --- src/lib/actions/sandbox/destroy-execution.ts | 2 +- src/lib/actions/sandbox/destroy-flow.test.ts | 30 ++++++++++++++++++-- test/helpers/destroy-flow-test-assertions.ts | 17 +++++++++-- test/helpers/destroy-flow-test-harness.ts | 7 ++++- 4 files changed, 50 insertions(+), 6 deletions(-) diff --git a/src/lib/actions/sandbox/destroy-execution.ts b/src/lib/actions/sandbox/destroy-execution.ts index 666eae7a353..865fae0773a 100644 --- a/src/lib/actions/sandbox/destroy-execution.ts +++ b/src/lib/actions/sandbox/destroy-execution.ts @@ -229,7 +229,7 @@ async function finalizeMcpDestroy( try { await finalizeMcpBridgesAfterSandboxDelete(sandboxName, preparation, { force }); } catch (error) { - const detail = error instanceof Error ? error.message : String(error); + const detail = redactDestroyError(error); console.error( ` Sandbox '${sandboxName}' is gone, but authenticated MCP provider cleanup is incomplete: ${detail}`, ); diff --git a/src/lib/actions/sandbox/destroy-flow.test.ts b/src/lib/actions/sandbox/destroy-flow.test.ts index c2e857fa713..a2c56bfb4ac 100644 --- a/src/lib/actions/sandbox/destroy-flow.test.ts +++ b/src/lib/actions/sandbox/destroy-flow.test.ts @@ -385,7 +385,19 @@ describe("destroySandbox flow", () => { expectMcpPrepareBridgeErrorAborts(harness); }); - it("exits with code 1 when MCP bridge finalize throws McpBridgeError after sandbox delete (#8103)", async () => { + it("redacts MCP bridge finalization errors after sandbox deletion (#8103)", async () => { + const secretMarker = "destroy-secret-marker"; + const harness = createDestroyHarness({ + mcpServers: ["github"], + finalizeMcpBridgeError: `Could not inspect OpenShell provider: OPENAI_API_KEY=${secretMarker}`, + }); + + await expect(harness.destroySandbox("alpha", { yes: true })).rejects.toThrow("process.exit(1)"); + + expectMcpFinalizeBridgeErrorReturnsFailure(harness, secretMarker); + }); + + it("retires retained MCP state when a destroy retry completes finalization (#8103)", async () => { const harness = createDestroyHarness({ mcpServers: ["github"], finalizeMcpBridgeError: "Could not inspect OpenShell provider: gateway unreachable", @@ -393,6 +405,20 @@ describe("destroySandbox flow", () => { await expect(harness.destroySandbox("alpha", { yes: true })).rejects.toThrow("process.exit(1)"); - expectMcpFinalizeBridgeErrorReturnsFailure(harness); + harness.setSandboxPresent(false); + harness.finalizeMcpBridgesAfterSandboxDeleteSpy.mockResolvedValue(undefined); + + await expect(harness.destroySandbox("alpha", { yes: true })).resolves.toBeUndefined(); + + expect(harness.prepareMcpBridgesForAbsentSandboxDestroySpy).toHaveBeenCalledWith("alpha", { + force: false, + }); + expect(harness.finalizeMcpBridgesAfterSandboxDeleteSpy).toHaveBeenCalledTimes(2); + expect(harness.removeSandboxSpy).toHaveBeenCalledWith("alpha"); + expect(harness.updateSessionSpy).toHaveBeenCalledOnce(); + expect(harness.cleanupGatewaySpy).toHaveBeenCalledWith( + "nemoclaw-19080", + harness.runOpenshellSpy, + ); }); }); diff --git a/test/helpers/destroy-flow-test-assertions.ts b/test/helpers/destroy-flow-test-assertions.ts index 2b467d67034..e4d56b961ec 100644 --- a/test/helpers/destroy-flow-test-assertions.ts +++ b/test/helpers/destroy-flow-test-assertions.ts @@ -229,9 +229,22 @@ export function expectMcpPrepareBridgeErrorAborts(harness: DestroyHarness): void expect(harness.removeSandboxSpy).not.toHaveBeenCalled(); } -export function expectMcpFinalizeBridgeErrorReturnsFailure(harness: DestroyHarness): void { +export function expectMcpFinalizeBridgeErrorReturnsFailure( + harness: DestroyHarness, + secretMarker: string, +): void { expect(harness.finalizeMcpBridgesAfterSandboxDeleteSpy).toHaveBeenCalled(); - // Registry must not be cleaned up when post-delete MCP finalize throws McpBridgeError. + const deleteCall = harness.runOpenshellSpy.mock.calls.findIndex( + (call) => Array.isArray(call[0]) && call[0].join(" ") === "sandbox delete alpha", + ); + expect(deleteCall).toBeGreaterThanOrEqual(0); + expect( + harness.finalizeMcpBridgesAfterSandboxDeleteSpy.mock.invocationCallOrder.at(-1), + ).toBeGreaterThan(harness.runOpenshellSpy.mock.invocationCallOrder[deleteCall]); + const errorOutput = harness.errorSpy.mock.calls.map((call) => String(call[0])).join("\n"); + expect(errorOutput).not.toContain(secretMarker); + expect(errorOutput).toContain(""); + // The sandbox registry must retain MCP ownership when provider finalization fails. expect(harness.removeSandboxSpy).not.toHaveBeenCalled(); expect(harness.cleanupGatewaySpy).not.toHaveBeenCalled(); } diff --git a/test/helpers/destroy-flow-test-harness.ts b/test/helpers/destroy-flow-test-harness.ts index 2747c3a1090..96cd7dd71dd 100644 --- a/test/helpers/destroy-flow-test-harness.ts +++ b/test/helpers/destroy-flow-test-harness.ts @@ -35,6 +35,7 @@ export type DestroyHarness = { restoreMcpBridgesAfterDestroyAbortSpy: MockInstance; runOpenshellSpy: MockInstance; selectGatewaySpy: MockInstance; + setSandboxPresent: (present: boolean) => void; shieldsDownSpy: MockInstance; stopAllSpy: MockInstance; stopNimByNameSpy: MockInstance; @@ -112,6 +113,7 @@ export function loadDestroySandboxPresenceClassifier(): DestroySandboxPresenceCl export function createDestroyHarness(options: DestroyHarnessOptions = {}): DestroyHarness { resetDestroyModuleCache(); const events: string[] = []; + let sandboxPresent = options.sandboxPresent !== false; const logSpy = vi.spyOn(console, "log").mockImplementation(() => undefined); const errorSpy = vi.spyOn(console, "error").mockImplementation(() => undefined); @@ -201,7 +203,7 @@ export function createDestroyHarness(options: DestroyHarnessOptions = {}): Destr gatewayPinsAtSandboxList.push(process.env.OPENSHELL_GATEWAY); return { status: 0, - stdout: sandboxListJson(options.sandboxPresent === false ? [] : ["alpha"]), + stdout: sandboxListJson(sandboxPresent ? ["alpha"] : []), stderr: "", }; case "sandbox:delete": @@ -356,6 +358,9 @@ export function createDestroyHarness(options: DestroyHarnessOptions = {}): Destr restoreMcpBridgesAfterDestroyAbortSpy, runOpenshellSpy, selectGatewaySpy, + setSandboxPresent: (present: boolean) => { + sandboxPresent = present; + }, shieldsDownSpy, stopAllSpy, stopNimByNameSpy, From d312465ba0a1d398efe2eefbafa58df6c134dac5 Mon Sep 17 00:00:00 2001 From: Carlos Villela Date: Wed, 5 Aug 2026 03:28:56 -0700 Subject: [PATCH 05/11] test(sandbox): clarify MCP destroy behavior Signed-off-by: Carlos Villela --- src/lib/actions/sandbox/destroy-flow.test.ts | 4 ++-- test/helpers/destroy-flow-test-assertions.ts | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/lib/actions/sandbox/destroy-flow.test.ts b/src/lib/actions/sandbox/destroy-flow.test.ts index a2c56bfb4ac..be988a104b7 100644 --- a/src/lib/actions/sandbox/destroy-flow.test.ts +++ b/src/lib/actions/sandbox/destroy-flow.test.ts @@ -374,7 +374,7 @@ describe("destroySandbox flow", () => { expectAbsentSandboxMcpFinalize(harness); }); - it("exits with code 1 when MCP bridge prepare throws McpBridgeError, gateway down (#8103)", async () => { + it("does not delete the sandbox when MCP preparation cannot reach the OpenShell gateway (#8103)", async () => { const harness = createDestroyHarness({ mcpServers: ["github"], prepareMcpBridgeError: "Could not inspect OpenShell provider: gateway unreachable", @@ -397,7 +397,7 @@ describe("destroySandbox flow", () => { expectMcpFinalizeBridgeErrorReturnsFailure(harness, secretMarker); }); - it("retires retained MCP state when a destroy retry completes finalization (#8103)", async () => { + it("completes registry and gateway cleanup after a destroy rerun finalizes MCP providers (#8103)", async () => { const harness = createDestroyHarness({ mcpServers: ["github"], finalizeMcpBridgeError: "Could not inspect OpenShell provider: gateway unreachable", diff --git a/test/helpers/destroy-flow-test-assertions.ts b/test/helpers/destroy-flow-test-assertions.ts index e4d56b961ec..26d00b82b55 100644 --- a/test/helpers/destroy-flow-test-assertions.ts +++ b/test/helpers/destroy-flow-test-assertions.ts @@ -221,7 +221,7 @@ export function expectFailedMcpFinalizePreservesRegistry(harness: DestroyHarness export function expectMcpPrepareBridgeErrorAborts(harness: DestroyHarness): void { expect(harness.prepareMcpBridgesForDestroySpy).toHaveBeenCalled(); - // MCP preparation must succeed before sandbox deletion begins. + // MCP preparation must succeed before NemoClaw asks OpenShell to delete the sandbox. expect(harness.runOpenshellSpy).not.toHaveBeenCalledWith( expect.arrayContaining(["sandbox", "delete"]), expect.anything(), From e386e386af1191217aa64ba1ab40568db17f983b Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Wed, 5 Aug 2026 02:57:32 -0700 Subject: [PATCH 06/11] test(sandbox): request gateway cleanup in retry flow Signed-off-by: Apurv Kumaria (cherry picked from commit 5934ec1a05ef0fde6115cb4e5cfaa5996ab12391) --- src/lib/actions/sandbox/destroy-flow.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/lib/actions/sandbox/destroy-flow.test.ts b/src/lib/actions/sandbox/destroy-flow.test.ts index be988a104b7..f7a5283ce15 100644 --- a/src/lib/actions/sandbox/destroy-flow.test.ts +++ b/src/lib/actions/sandbox/destroy-flow.test.ts @@ -408,7 +408,9 @@ describe("destroySandbox flow", () => { harness.setSandboxPresent(false); harness.finalizeMcpBridgesAfterSandboxDeleteSpy.mockResolvedValue(undefined); - await expect(harness.destroySandbox("alpha", { yes: true })).resolves.toBeUndefined(); + await expect( + harness.destroySandbox("alpha", { yes: true, cleanupGateway: true }), + ).resolves.toBeUndefined(); expect(harness.prepareMcpBridgesForAbsentSandboxDestroySpy).toHaveBeenCalledWith("alpha", { force: false, From 82946e691507d7d3abf34c2696e789cf847d83ce Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Wed, 5 Aug 2026 02:37:31 -0700 Subject: [PATCH 07/11] fix(sandbox): harden MCP destroy recovery Signed-off-by: Apurv Kumaria (cherry picked from commit adf49fd8c857b30be48d5f93d0f7968075fd3c5a) --- src/lib/actions/sandbox/destroy-execution.ts | 2 +- src/lib/actions/sandbox/destroy-flow.test.ts | 30 ++++++++++++++++++-- test/helpers/destroy-flow-test-assertions.ts | 15 +++++++++- test/helpers/destroy-flow-test-harness.ts | 7 ++++- 4 files changed, 49 insertions(+), 5 deletions(-) diff --git a/src/lib/actions/sandbox/destroy-execution.ts b/src/lib/actions/sandbox/destroy-execution.ts index 666eae7a353..865fae0773a 100644 --- a/src/lib/actions/sandbox/destroy-execution.ts +++ b/src/lib/actions/sandbox/destroy-execution.ts @@ -229,7 +229,7 @@ async function finalizeMcpDestroy( try { await finalizeMcpBridgesAfterSandboxDelete(sandboxName, preparation, { force }); } catch (error) { - const detail = error instanceof Error ? error.message : String(error); + const detail = redactDestroyError(error); console.error( ` Sandbox '${sandboxName}' is gone, but authenticated MCP provider cleanup is incomplete: ${detail}`, ); diff --git a/src/lib/actions/sandbox/destroy-flow.test.ts b/src/lib/actions/sandbox/destroy-flow.test.ts index c2e857fa713..6bbb071bca0 100644 --- a/src/lib/actions/sandbox/destroy-flow.test.ts +++ b/src/lib/actions/sandbox/destroy-flow.test.ts @@ -385,7 +385,19 @@ describe("destroySandbox flow", () => { expectMcpPrepareBridgeErrorAborts(harness); }); - it("exits with code 1 when MCP bridge finalize throws McpBridgeError after sandbox delete (#8103)", async () => { + it("redacts MCP bridge finalize errors after sandbox deletion (#8103)", async () => { + const secretMarker = "destroy-secret-marker"; + const harness = createDestroyHarness({ + mcpServers: ["github"], + finalizeMcpBridgeError: `Could not inspect OpenShell provider: OPENAI_API_KEY=${secretMarker}`, + }); + + await expect(harness.destroySandbox("alpha", { yes: true })).rejects.toThrow("process.exit(1)"); + + expectMcpFinalizeBridgeErrorReturnsFailure(harness, secretMarker); + }); + + it("retires retained MCP state when destroy retries after finalization failure (#8103)", async () => { const harness = createDestroyHarness({ mcpServers: ["github"], finalizeMcpBridgeError: "Could not inspect OpenShell provider: gateway unreachable", @@ -393,6 +405,20 @@ describe("destroySandbox flow", () => { await expect(harness.destroySandbox("alpha", { yes: true })).rejects.toThrow("process.exit(1)"); - expectMcpFinalizeBridgeErrorReturnsFailure(harness); + harness.setSandboxPresent(false); + harness.finalizeMcpBridgesAfterSandboxDeleteSpy.mockResolvedValue(undefined); + + await expect(harness.destroySandbox("alpha", { yes: true })).resolves.toBeUndefined(); + + expect(harness.prepareMcpBridgesForAbsentSandboxDestroySpy).toHaveBeenCalledWith("alpha", { + force: false, + }); + expect(harness.finalizeMcpBridgesAfterSandboxDeleteSpy).toHaveBeenCalledTimes(2); + expect(harness.removeSandboxSpy).toHaveBeenCalledWith("alpha"); + expect(harness.updateSessionSpy).toHaveBeenCalledOnce(); + expect(harness.cleanupGatewaySpy).toHaveBeenCalledWith( + "nemoclaw-19080", + harness.runOpenshellSpy, + ); }); }); diff --git a/test/helpers/destroy-flow-test-assertions.ts b/test/helpers/destroy-flow-test-assertions.ts index 2b467d67034..f6baae5c50b 100644 --- a/test/helpers/destroy-flow-test-assertions.ts +++ b/test/helpers/destroy-flow-test-assertions.ts @@ -229,8 +229,21 @@ export function expectMcpPrepareBridgeErrorAborts(harness: DestroyHarness): void expect(harness.removeSandboxSpy).not.toHaveBeenCalled(); } -export function expectMcpFinalizeBridgeErrorReturnsFailure(harness: DestroyHarness): void { +export function expectMcpFinalizeBridgeErrorReturnsFailure( + harness: DestroyHarness, + secretMarker: string, +): void { expect(harness.finalizeMcpBridgesAfterSandboxDeleteSpy).toHaveBeenCalled(); + const deleteCall = harness.runOpenshellSpy.mock.calls.findIndex( + (call) => Array.isArray(call[0]) && call[0].join(" ") === "sandbox delete alpha", + ); + expect(deleteCall).toBeGreaterThanOrEqual(0); + expect( + harness.finalizeMcpBridgesAfterSandboxDeleteSpy.mock.invocationCallOrder.at(-1), + ).toBeGreaterThan(harness.runOpenshellSpy.mock.invocationCallOrder[deleteCall]); + const errorOutput = harness.errorSpy.mock.calls.map((call) => String(call[0])).join("\n"); + expect(errorOutput).not.toContain(secretMarker); + expect(errorOutput).toContain(""); // Registry must not be cleaned up when post-delete MCP finalize throws McpBridgeError. expect(harness.removeSandboxSpy).not.toHaveBeenCalled(); expect(harness.cleanupGatewaySpy).not.toHaveBeenCalled(); diff --git a/test/helpers/destroy-flow-test-harness.ts b/test/helpers/destroy-flow-test-harness.ts index 2747c3a1090..96cd7dd71dd 100644 --- a/test/helpers/destroy-flow-test-harness.ts +++ b/test/helpers/destroy-flow-test-harness.ts @@ -35,6 +35,7 @@ export type DestroyHarness = { restoreMcpBridgesAfterDestroyAbortSpy: MockInstance; runOpenshellSpy: MockInstance; selectGatewaySpy: MockInstance; + setSandboxPresent: (present: boolean) => void; shieldsDownSpy: MockInstance; stopAllSpy: MockInstance; stopNimByNameSpy: MockInstance; @@ -112,6 +113,7 @@ export function loadDestroySandboxPresenceClassifier(): DestroySandboxPresenceCl export function createDestroyHarness(options: DestroyHarnessOptions = {}): DestroyHarness { resetDestroyModuleCache(); const events: string[] = []; + let sandboxPresent = options.sandboxPresent !== false; const logSpy = vi.spyOn(console, "log").mockImplementation(() => undefined); const errorSpy = vi.spyOn(console, "error").mockImplementation(() => undefined); @@ -201,7 +203,7 @@ export function createDestroyHarness(options: DestroyHarnessOptions = {}): Destr gatewayPinsAtSandboxList.push(process.env.OPENSHELL_GATEWAY); return { status: 0, - stdout: sandboxListJson(options.sandboxPresent === false ? [] : ["alpha"]), + stdout: sandboxListJson(sandboxPresent ? ["alpha"] : []), stderr: "", }; case "sandbox:delete": @@ -356,6 +358,9 @@ export function createDestroyHarness(options: DestroyHarnessOptions = {}): Destr restoreMcpBridgesAfterDestroyAbortSpy, runOpenshellSpy, selectGatewaySpy, + setSandboxPresent: (present: boolean) => { + sandboxPresent = present; + }, shieldsDownSpy, stopAllSpy, stopNimByNameSpy, From 8c6fcf0976a7ffca486b509f35b3821cf8e5f1dd Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Wed, 5 Aug 2026 02:57:32 -0700 Subject: [PATCH 08/11] test(sandbox): request gateway cleanup in retry flow Signed-off-by: Apurv Kumaria (cherry picked from commit 5934ec1a05ef0fde6115cb4e5cfaa5996ab12391) --- src/lib/actions/sandbox/destroy-flow.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/lib/actions/sandbox/destroy-flow.test.ts b/src/lib/actions/sandbox/destroy-flow.test.ts index 6bbb071bca0..08c3e01d3cc 100644 --- a/src/lib/actions/sandbox/destroy-flow.test.ts +++ b/src/lib/actions/sandbox/destroy-flow.test.ts @@ -408,7 +408,9 @@ describe("destroySandbox flow", () => { harness.setSandboxPresent(false); harness.finalizeMcpBridgesAfterSandboxDeleteSpy.mockResolvedValue(undefined); - await expect(harness.destroySandbox("alpha", { yes: true })).resolves.toBeUndefined(); + await expect( + harness.destroySandbox("alpha", { yes: true, cleanupGateway: true }), + ).resolves.toBeUndefined(); expect(harness.prepareMcpBridgesForAbsentSandboxDestroySpy).toHaveBeenCalledWith("alpha", { force: false, From 43bbc7130db6d62212aebd9b9b6a6870c0c38f1d Mon Sep 17 00:00:00 2001 From: Carlos Villela Date: Wed, 5 Aug 2026 03:28:56 -0700 Subject: [PATCH 09/11] test(sandbox): clarify MCP destroy behavior Signed-off-by: Carlos Villela (cherry picked from commit d312465ba0a1d398efe2eefbafa58df6c134dac5) Signed-off-by: Carlos Villela --- src/lib/actions/sandbox/destroy-flow.test.ts | 6 +++--- test/helpers/destroy-flow-test-assertions.ts | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/lib/actions/sandbox/destroy-flow.test.ts b/src/lib/actions/sandbox/destroy-flow.test.ts index 08c3e01d3cc..f7a5283ce15 100644 --- a/src/lib/actions/sandbox/destroy-flow.test.ts +++ b/src/lib/actions/sandbox/destroy-flow.test.ts @@ -374,7 +374,7 @@ describe("destroySandbox flow", () => { expectAbsentSandboxMcpFinalize(harness); }); - it("exits with code 1 when MCP bridge prepare throws McpBridgeError, gateway down (#8103)", async () => { + it("does not delete the sandbox when MCP preparation cannot reach the OpenShell gateway (#8103)", async () => { const harness = createDestroyHarness({ mcpServers: ["github"], prepareMcpBridgeError: "Could not inspect OpenShell provider: gateway unreachable", @@ -385,7 +385,7 @@ describe("destroySandbox flow", () => { expectMcpPrepareBridgeErrorAborts(harness); }); - it("redacts MCP bridge finalize errors after sandbox deletion (#8103)", async () => { + it("redacts MCP bridge finalization errors after sandbox deletion (#8103)", async () => { const secretMarker = "destroy-secret-marker"; const harness = createDestroyHarness({ mcpServers: ["github"], @@ -397,7 +397,7 @@ describe("destroySandbox flow", () => { expectMcpFinalizeBridgeErrorReturnsFailure(harness, secretMarker); }); - it("retires retained MCP state when destroy retries after finalization failure (#8103)", async () => { + it("completes registry and gateway cleanup after a destroy rerun finalizes MCP providers (#8103)", async () => { const harness = createDestroyHarness({ mcpServers: ["github"], finalizeMcpBridgeError: "Could not inspect OpenShell provider: gateway unreachable", diff --git a/test/helpers/destroy-flow-test-assertions.ts b/test/helpers/destroy-flow-test-assertions.ts index f6baae5c50b..26d00b82b55 100644 --- a/test/helpers/destroy-flow-test-assertions.ts +++ b/test/helpers/destroy-flow-test-assertions.ts @@ -221,7 +221,7 @@ export function expectFailedMcpFinalizePreservesRegistry(harness: DestroyHarness export function expectMcpPrepareBridgeErrorAborts(harness: DestroyHarness): void { expect(harness.prepareMcpBridgesForDestroySpy).toHaveBeenCalled(); - // MCP preparation must succeed before sandbox deletion begins. + // MCP preparation must succeed before NemoClaw asks OpenShell to delete the sandbox. expect(harness.runOpenshellSpy).not.toHaveBeenCalledWith( expect.arrayContaining(["sandbox", "delete"]), expect.anything(), @@ -244,7 +244,7 @@ export function expectMcpFinalizeBridgeErrorReturnsFailure( const errorOutput = harness.errorSpy.mock.calls.map((call) => String(call[0])).join("\n"); expect(errorOutput).not.toContain(secretMarker); expect(errorOutput).toContain(""); - // Registry must not be cleaned up when post-delete MCP finalize throws McpBridgeError. + // The sandbox registry must retain MCP ownership when provider finalization fails. expect(harness.removeSandboxSpy).not.toHaveBeenCalled(); expect(harness.cleanupGatewaySpy).not.toHaveBeenCalled(); } From b015859fbf3f6fc518ad576772f3c5f3a6ef337c Mon Sep 17 00:00:00 2001 From: Carlos Villela Date: Wed, 5 Aug 2026 03:57:40 -0700 Subject: [PATCH 10/11] test(sandbox): cover MCP destroy boundaries Signed-off-by: Carlos Villela --- src/lib/actions/sandbox/destroy-flow.test.ts | 12 +++++++++--- test/helpers/destroy-flow-test-assertions.ts | 8 +++++++- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/src/lib/actions/sandbox/destroy-flow.test.ts b/src/lib/actions/sandbox/destroy-flow.test.ts index f7a5283ce15..16b6175a2b6 100644 --- a/src/lib/actions/sandbox/destroy-flow.test.ts +++ b/src/lib/actions/sandbox/destroy-flow.test.ts @@ -374,15 +374,16 @@ describe("destroySandbox flow", () => { expectAbsentSandboxMcpFinalize(harness); }); - it("does not delete the sandbox when MCP preparation cannot reach the OpenShell gateway (#8103)", async () => { + it("redacts MCP bridge preparation errors and does not delete the sandbox (#8103)", async () => { + const secretMarker = "destroy-prepare-secret-marker"; const harness = createDestroyHarness({ mcpServers: ["github"], - prepareMcpBridgeError: "Could not inspect OpenShell provider: gateway unreachable", + prepareMcpBridgeError: `Could not inspect OpenShell provider: OPENAI_API_KEY=${secretMarker}`, }); await expect(harness.destroySandbox("alpha", { yes: true })).rejects.toThrow("process.exit(1)"); - expectMcpPrepareBridgeErrorAborts(harness); + expectMcpPrepareBridgeErrorAborts(harness, secretMarker); }); it("redacts MCP bridge finalization errors after sandbox deletion (#8103)", async () => { @@ -415,6 +416,11 @@ describe("destroySandbox flow", () => { expect(harness.prepareMcpBridgesForAbsentSandboxDestroySpy).toHaveBeenCalledWith("alpha", { force: false, }); + expect( + harness.runOpenshellSpy.mock.calls.filter( + (call) => Array.isArray(call[0]) && call[0].join(" ") === "sandbox delete alpha", + ), + ).toHaveLength(1); expect(harness.finalizeMcpBridgesAfterSandboxDeleteSpy).toHaveBeenCalledTimes(2); expect(harness.removeSandboxSpy).toHaveBeenCalledWith("alpha"); expect(harness.updateSessionSpy).toHaveBeenCalledOnce(); diff --git a/test/helpers/destroy-flow-test-assertions.ts b/test/helpers/destroy-flow-test-assertions.ts index 26d00b82b55..ab1743dab8c 100644 --- a/test/helpers/destroy-flow-test-assertions.ts +++ b/test/helpers/destroy-flow-test-assertions.ts @@ -219,13 +219,19 @@ export function expectFailedMcpFinalizePreservesRegistry(harness: DestroyHarness expect(harness.cleanupGatewaySpy).not.toHaveBeenCalled(); } -export function expectMcpPrepareBridgeErrorAborts(harness: DestroyHarness): void { +export function expectMcpPrepareBridgeErrorAborts( + harness: DestroyHarness, + secretMarker: string, +): void { expect(harness.prepareMcpBridgesForDestroySpy).toHaveBeenCalled(); // MCP preparation must succeed before NemoClaw asks OpenShell to delete the sandbox. expect(harness.runOpenshellSpy).not.toHaveBeenCalledWith( expect.arrayContaining(["sandbox", "delete"]), expect.anything(), ); + const errorOutput = harness.errorSpy.mock.calls.flat().map(String).join("\n"); + expect(errorOutput).not.toContain(secretMarker); + expect(errorOutput).toContain(""); expect(harness.removeSandboxSpy).not.toHaveBeenCalled(); } From db36b5a93eda1bc41cc23d630df327c33fbfb699 Mon Sep 17 00:00:00 2001 From: Carlos Villela Date: Wed, 5 Aug 2026 04:03:44 -0700 Subject: [PATCH 11/11] fix(sandbox): skip delete after confirmed absence Signed-off-by: Carlos Villela --- src/lib/actions/sandbox/destroy-execution.ts | 24 +++++++++++++------- src/lib/actions/sandbox/destroy.ts | 10 +++++--- 2 files changed, 23 insertions(+), 11 deletions(-) diff --git a/src/lib/actions/sandbox/destroy-execution.ts b/src/lib/actions/sandbox/destroy-execution.ts index 865fae0773a..d605f77bcbc 100644 --- a/src/lib/actions/sandbox/destroy-execution.ts +++ b/src/lib/actions/sandbox/destroy-execution.ts @@ -51,7 +51,7 @@ export type SandboxDestroyExecutionResult = ok: true; alreadyGone: boolean; deleteOutput: string; - deleteResult: ReturnType; + deleteSucceededOrAlreadyGone: boolean; detachOutcome: DetachSandboxProvidersResult; forcedLocalCleanup: boolean; } @@ -298,21 +298,28 @@ export async function executeSandboxDestroy({ : runtimeProvider?.cleanup.supported === true && sandbox ? runtimeProvider.cleanup.prepareDestroy({ sandbox, sandboxName }, { detachProviders }) : detachProviders(); - const deleteResult = runOpenshell(["sandbox", "delete", sandboxName], { - ignoreError: true, - stdio: ["ignore", "pipe", "pipe"], - }); + // Preflight already confirmed absence. A later delete could target a + // same-name sandbox created by another OpenShell client after that check. + const deleteResult = sandboxConfirmedAbsent + ? null + : runOpenshell(["sandbox", "delete", sandboxName], { + ignoreError: true, + stdio: ["ignore", "pipe", "pipe"], + }); const { output: deleteOutput, alreadyGone, gatewayUnreachable, - } = getSandboxDeleteOutcome(deleteResult); + } = deleteResult + ? getSandboxDeleteOutcome(deleteResult) + : { output: "", alreadyGone: true, gatewayUnreachable: false }; // #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 !== null && deleteResult.status !== 0 && !alreadyGone && gatewayUnreachable && @@ -320,7 +327,7 @@ export async function executeSandboxDestroy({ !hasMcpOwnership && !hardened.hardeningFailed; - if (deleteResult.status !== 0 && !alreadyGone && !forcedLocalCleanup) { + if (deleteResult !== null && deleteResult.status !== 0 && !alreadyGone && !forcedLocalCleanup) { const mcpRecoveryFailure = sandboxConfirmedAbsent ? undefined : await restoreMcpAfterDeleteAbort(sandboxName, mcpPreparation, hardened); @@ -360,7 +367,8 @@ export async function executeSandboxDestroy({ ok: true as const, detachOutcome, deleteOutput, - deleteResult, + deleteSucceededOrAlreadyGone: + deleteResult === null || deleteResult.status === 0 || alreadyGone, alreadyGone, forcedLocalCleanup, }; diff --git a/src/lib/actions/sandbox/destroy.ts b/src/lib/actions/sandbox/destroy.ts index 4a9ac783596..865c499369b 100644 --- a/src/lib/actions/sandbox/destroy.ts +++ b/src/lib/actions/sandbox/destroy.ts @@ -507,8 +507,13 @@ async function destroySandboxUnlocked( } process.exit(destructiveResult.exitCode); } - const { detachOutcome, deleteResult, alreadyGone, forcedLocalCleanup, deleteOutput } = - destructiveResult; + const { + detachOutcome, + alreadyGone, + deleteSucceededOrAlreadyGone, + forcedLocalCleanup, + deleteOutput, + } = destructiveResult; /** * SOURCE_OF_TRUTH @@ -543,7 +548,6 @@ async function destroySandboxUnlocked( // gateway. Gate that teardown on the *confirmed* delete state only — never on // forcedLocalCleanup — so a forced cleanup of the last registered sandbox does // not shut down services for a sandbox we never confirmed deleted (#6046). - const deleteSucceededOrAlreadyGone = deleteResult.status === 0 || alreadyGone; const shouldStopHostServices = shouldStopHostServicesAfterDestroy({ deleteSucceededOrAlreadyGone, registeredSandboxCount: registry.listSandboxes().sandboxes.length,