fix(sandbox): preserve structured MCP destroy failures (#8103)

src/lib/actions/sandbox/destroy-execution.ts

@@ -51,7 +51,7 @@ export type SandboxDestroyExecutionResult =
       ok: true;
       alreadyGone: boolean;
       deleteOutput: string;
-      deleteResult: ReturnType<DestroyRunOpenshell>;
+      deleteSucceededOrAlreadyGone: boolean;
       detachOutcome: DetachSandboxProvidersResult;
       forcedLocalCleanup: boolean;
     }
@@ -298,29 +298,36 @@ 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 &&
       force &&
       !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,
     };

src/lib/actions/sandbox/destroy-flow.test.ts

@@ -374,18 +374,19 @@ describe("destroySandbox flow", () => {
     expectAbsentSandboxMcpFinalize(harness);
   });
 
-  it("exits with code 1 when MCP bridge prepare throws McpBridgeError, gateway down (#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 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 +398,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",
@@ -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();

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,

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();
-  // No delete should happen when MCP prepare itself throws McpBridgeError.
+  // 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("<REDACTED>");
   expect(harness.removeSandboxSpy).not.toHaveBeenCalled();
 }
 
@@ -244,7 +250,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("<REDACTED>");
-  // 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();
 }