From 3185b6a319c8c17625c2f57ffdff9df4d6ee7742 Mon Sep 17 00:00:00 2001 From: Yanyun Liao Date: Wed, 5 Aug 2026 12:37:35 +0800 Subject: [PATCH 1/4] 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 Signed-off-by: Apurv Kumaria --- 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 91bf4cd38a..666eae7a35 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 22c68b57c1..c2e857fa71 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 66b12c86aa..a52c8983eb 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 ec7dd59f83..2747c3a109 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 28e1df05690ea10fa5b9b76d4f6ce79d89299b9e Mon Sep 17 00:00:00 2001 From: Carlos Villela Date: Wed, 5 Aug 2026 01:44:48 -0700 Subject: [PATCH 2/4] chore(architecture): ratchet source budgets Signed-off-by: Carlos Villela Signed-off-by: Apurv Kumaria --- 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 4e3f1dacd4..dc2d40b910 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 adf49fd8c857b30be48d5f93d0f7968075fd3c5a Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Wed, 5 Aug 2026 02:37:31 -0700 Subject: [PATCH 3/4] fix(sandbox): harden MCP destroy recovery Signed-off-by: Apurv Kumaria --- 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 666eae7a35..865fae0773 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 c2e857fa71..6bbb071bca 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 a52c8983eb..28e2b73dc0 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 2747c3a109..96cd7dd71d 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 5934ec1a05ef0fde6115cb4e5cfaa5996ab12391 Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Wed, 5 Aug 2026 02:57:32 -0700 Subject: [PATCH 4/4] test(sandbox): request gateway cleanup in retry flow Signed-off-by: Apurv Kumaria --- 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 6bbb071bca..08c3e01d3c 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,