From 157b54f667e160d17fee32f7a0499acb69c70440 Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Sun, 19 Jul 2026 03:25:36 -0700 Subject: [PATCH 01/18] fix(rebuild): recover managed MCP after exec loss Use host-side MCP recovery only when an explicit forced rebuild cannot run a sandbox no-op. Keep policy, ownership, target, provider, and transaction errors fail-closed. Closes NVIDIA/NemoClaw#7062 Co-authored-by: kagura-agent Signed-off-by: Apurv Kumaria --- .../recover-rebuild-sandboxes.mdx | 2 + docs/reference/commands.mdx | 5 +- .../sandbox/rebuild-destroy-phase.test.ts | 50 ++++++++ .../actions/sandbox/rebuild-destroy-phase.ts | 10 +- .../actions/sandbox/rebuild-mcp-phase.test.ts | 116 +++++++++++++++++- src/lib/actions/sandbox/rebuild-mcp-phase.ts | 22 +++- src/lib/actions/sandbox/rebuild-pipeline.ts | 1 + 7 files changed, 201 insertions(+), 5 deletions(-) diff --git a/docs/manage-sandboxes/recover-rebuild-sandboxes.mdx b/docs/manage-sandboxes/recover-rebuild-sandboxes.mdx index cfbe1eab2a2..3274cf1f968 100644 --- a/docs/manage-sandboxes/recover-rebuild-sandboxes.mdx +++ b/docs/manage-sandboxes/recover-rebuild-sandboxes.mdx @@ -121,6 +121,8 @@ If every state directory fails, NemoClaw stops before deleting the original sand `rebuild --force` can continue when no state directory was preserved. NemoClaw restores any loose files captured in the partial backup; if nothing usable was captured, it recreates the sandbox from recorded registry metadata without restoring prior sandbox state. Use this recovery path only when losing the state that could not be backed up is acceptable. +When a sandbox with managed MCP servers cannot run a pre-mutation no-op, explicit `--force` uses the exact host-side registry and provider identities to preserve MCP intent without scrubbing the unreachable in-sandbox adapter. +This recovery remains fail-closed for ambiguous ownership, target, policy-registration, or provider state; an error after a successful no-op does not fall back to the host-side path. When rebuild starts with shields up, NemoClaw opens a 30-minute shields-down window for backup and recreation. diff --git a/docs/reference/commands.mdx b/docs/reference/commands.mdx index 0e8c3fed80f..d1e839e7f30 100644 --- a/docs/reference/commands.mdx +++ b/docs/reference/commands.mdx @@ -2475,7 +2475,7 @@ $$nemoclaw my-assistant rebuild [--yes|-y|--force] [--verbose|-v] [--tool-disclo | Flag | Description | |------|-------------| | `--yes`, `-y` | Skip the confirmation prompt. | -| `--force` | Skip the confirmation prompt and continue when no state directory was preserved. NemoClaw restores any captured loose files; after a total failure, it recreates from registry metadata only. | +| `--force` | Skip the confirmation prompt and continue when no state directory was preserved. NemoClaw restores any captured loose files; after a total failure, it recreates from registry metadata only. If a pre-mutation no-op cannot execute in a sandbox with managed MCP servers, it may preserve the exact registered MCP intent through host-side recovery. | | `--verbose`, `-v` | Log SSH commands, exit codes, and session state (also enabled by `NEMOCLAW_REBUILD_VERBOSE=1`) | | `--tool-disclosure ` | Change the model-visible tool catalog during this transactional rebuild. Use this path for sandboxes with managed MCP servers so their providers and adapter state are preserved. | | `--dcode-auto-approval ` | Change the managed Deep Agents Code thread auto-approval capability. `thread-opt-in` is accepted only for managed Deep Agents Code sandboxes and is rejected for other agents or custom images. Enabling prints a warning, and either value requires sandbox recreation. | @@ -2490,6 +2490,9 @@ If every state directory fails, `rebuild` exits before destroying the original s With `--force`, NemoClaw preserves any captured loose files in the partial manifest and restores them after recreation. If the backup produced nothing usable, it continues from recorded registry metadata without restoring prior sandbox state. Use this recovery path only when losing the state that could not be backed up is acceptable. +For a sandbox with managed MCP servers, `--force` probes sandbox execution before MCP teardown. +If that no-op cannot run, NemoClaw uses the durable registry and host-side provider identities to preserve MCP intent without trying an in-sandbox adapter scrub. +Ownership, target, or provider validation failures still stop before sandbox deletion, and failures after a successful exec probe do not switch to the host-side path. Before backup or deletion, `rebuild` also refuses an incomplete MCP destroy transaction. For a prepared-only transaction, the redacted diagnostic points to `$$nemoclaw mcp remove --force` when the sandbox is still live. For a pending or both-marker transaction, it points to `$$nemoclaw destroy` because the registry records that OpenShell deletion was already confirmed. diff --git a/src/lib/actions/sandbox/rebuild-destroy-phase.test.ts b/src/lib/actions/sandbox/rebuild-destroy-phase.test.ts index b7dbe99098c..e29ca0e0d0c 100644 --- a/src/lib/actions/sandbox/rebuild-destroy-phase.test.ts +++ b/src/lib/actions/sandbox/rebuild-destroy-phase.test.ts @@ -7,6 +7,11 @@ const mocks = vi.hoisted(() => ({ prepareMcpForRebuild: vi.fn(), reattachMcpAfterDeleteFailure: vi.fn(), warnUnpreservedUserManagedFiles: vi.fn(), + runOpenshell: vi.fn(() => ({ status: 0, stdout: "", stderr: "" })), + getSandboxDeleteOutcome: vi.fn(() => ({ alreadyGone: false })), + getSandbox: vi.fn(() => null), + listSandboxes: vi.fn(() => ({ sandboxes: [] })), + removeSandboxRegistryEntryWithReceipt: vi.fn(() => null), })); vi.mock("./rebuild-flow-helpers", async (importOriginal) => ({ @@ -20,6 +25,24 @@ vi.mock("./rebuild-mcp-phase", async (importOriginal) => ({ reattachMcpAfterDeleteFailure: mocks.reattachMcpAfterDeleteFailure, })); +vi.mock("../../adapters/openshell/runtime", () => ({ + runOpenshell: mocks.runOpenshell, +})); + +vi.mock("../../domain/sandbox/destroy", () => ({ + getSandboxDeleteOutcome: mocks.getSandboxDeleteOutcome, +})); + +vi.mock("../../state/registry", async (importOriginal) => ({ + ...(await importOriginal()), + getSandbox: mocks.getSandbox, + listSandboxes: mocks.listSandboxes, +})); + +vi.mock("./destroy", () => ({ + removeSandboxRegistryEntryWithReceipt: mocks.removeSandboxRegistryEntryWithReceipt, +})); + import { runRebuildDestroyPhase } from "./rebuild-destroy-phase"; describe("rebuild destroy validation diagnostics", () => { @@ -70,4 +93,31 @@ describe("rebuild destroy validation diagnostics", () => { expect(mocks.reattachMcpAfterDeleteFailure).toHaveBeenCalledOnce(); expect(relockShieldsIfNeeded).toHaveBeenCalledWith(true); }); + + it("passes force=true to prepareMcpForRebuild when input.force is set (#7062)", async () => { + const log = vi.fn(); + const bail = vi.fn((message: string): never => { + throw new Error(message); + }); + + await runRebuildDestroyPhase({ + sandboxName: "alpha", + sandboxEntry: { name: "alpha", agent: "openclaw" }, + staleRecovery: false, + backupManifest: null, + force: true, + log, + bail, + relockShieldsIfNeeded: vi.fn(() => true), + onDeleted: vi.fn(), + }); + + expect(mocks.prepareMcpForRebuild).toHaveBeenCalledWith( + "alpha", + false, + true, + expect.any(Function), + expect.any(Function), + ); + }); }); diff --git a/src/lib/actions/sandbox/rebuild-destroy-phase.ts b/src/lib/actions/sandbox/rebuild-destroy-phase.ts index 70c7dd71a83..45cb98ddce7 100644 --- a/src/lib/actions/sandbox/rebuild-destroy-phase.ts +++ b/src/lib/actions/sandbox/rebuild-destroy-phase.ts @@ -30,6 +30,7 @@ export interface RebuildDestroyPhaseInput { log: RebuildLog; bail: RebuildBail; relockShieldsIfNeeded: (sandboxStillExists: boolean) => boolean; + force?: boolean; validateAfterMcpPreparation?: () => Promise; onDeleted: () => void; } @@ -66,7 +67,14 @@ export async function runRebuildDestroyPhase( `Registry entry: agent=${sbMeta?.agent}, agentVersion=${sbMeta?.agentVersion}, nimContainer=${sbMeta?.nimContainer}`, ); const mcpPreparation = await prepareMcpBeforeBestEffortNimStop({ - prepareMcp: () => prepareMcpForRebuild(sandboxName, staleRecovery, relockShieldsIfNeeded, bail), + prepareMcp: () => + prepareMcpForRebuild( + sandboxName, + staleRecovery, + input.force === true, + relockShieldsIfNeeded, + bail, + ), afterPrepare: async (preparation) => { // MCP preparation removes only adapter entries whose exact ownership // fingerprints match the registry. Probe afterward so a Deep Agents diff --git a/src/lib/actions/sandbox/rebuild-mcp-phase.test.ts b/src/lib/actions/sandbox/rebuild-mcp-phase.test.ts index 77c4f17c6aa..05d872c0c33 100644 --- a/src/lib/actions/sandbox/rebuild-mcp-phase.test.ts +++ b/src/lib/actions/sandbox/rebuild-mcp-phase.test.ts @@ -1,14 +1,126 @@ // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 -import { afterEach, describe, expect, it, vi } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; -import { printMcpRebuildRetryCommand } from "./rebuild-mcp-phase"; +const mocks = vi.hoisted(() => ({ + executeSandboxCommand: vi.fn(), + prepareAbsent: vi.fn(), + prepareLive: vi.fn(), +})); + +vi.mock("./mcp-bridge", () => ({ + prepareMcpBridgesForAbsentSandboxRebuild: mocks.prepareAbsent, + prepareMcpBridgesForRebuild: mocks.prepareLive, + reattachMcpProvidersAfterRebuildAbort: vi.fn(), + restoreMcpBridgesAfterRebuild: vi.fn(), +})); + +vi.mock("./process-recovery", () => ({ + executeSandboxCommand: mocks.executeSandboxCommand, +})); + +import { prepareMcpForRebuild, printMcpRebuildRetryCommand } from "./rebuild-mcp-phase"; + +const emptyPreparation = { + entries: [], + detachedProviderEntries: [], + scrubbedAdapterEntries: [], +}; afterEach(() => { vi.restoreAllMocks(); }); +describe("forced rebuild MCP preparation", () => { + beforeEach(() => { + vi.clearAllMocks(); + vi.spyOn(console, "error").mockImplementation(() => undefined); + mocks.executeSandboxCommand.mockReturnValue({ status: 0, stdout: "", stderr: "" }); + mocks.prepareAbsent.mockResolvedValue(emptyPreparation); + mocks.prepareLive.mockResolvedValue(emptyPreparation); + }); + + it("uses host-side recovery when the pre-mutation exec probe cannot run (#7062)", async () => { + mocks.executeSandboxCommand.mockReturnValue(null); + const relock = vi.fn(() => true); + const bail = vi.fn((message: string): never => { + throw new Error(message); + }); + + await expect(prepareMcpForRebuild("alpha", false, true, relock, bail)).resolves.toEqual( + emptyPreparation, + ); + + expect(mocks.executeSandboxCommand).toHaveBeenCalledWith("alpha", ":"); + expect(mocks.prepareAbsent).toHaveBeenCalledWith("alpha"); + expect(mocks.prepareLive).not.toHaveBeenCalled(); + expect(relock).not.toHaveBeenCalled(); + }); + + it("does not mask a live-path safety failure after a successful exec probe (#7062)", async () => { + mocks.prepareLive.mockRejectedValue(new Error("generated policy drifted")); + const relock = vi.fn(() => true); + const bail = vi.fn((message: string): never => { + throw new Error(message); + }); + + await expect(prepareMcpForRebuild("alpha", false, true, relock, bail)).rejects.toThrow( + "Failed to preserve MCP bridges before rebuild: generated policy drifted", + ); + + expect(mocks.prepareLive).toHaveBeenCalledWith("alpha"); + expect(mocks.prepareAbsent).not.toHaveBeenCalled(); + expect(relock).toHaveBeenCalledWith(true); + }); + + it("fails closed when host-side recovery cannot prove durable ownership (#7062)", async () => { + mocks.executeSandboxCommand.mockReturnValue({ status: 255, stdout: "", stderr: "relay EOF" }); + mocks.prepareAbsent.mockRejectedValue(new Error("provider ownership is ambiguous")); + const relock = vi.fn(() => true); + const bail = vi.fn((message: string): never => { + throw new Error(message); + }); + + await expect(prepareMcpForRebuild("alpha", false, true, relock, bail)).rejects.toThrow( + "Failed to preserve MCP bridges before rebuild (--force host-side recovery): provider ownership is ambiguous", + ); + + expect(mocks.prepareLive).not.toHaveBeenCalled(); + expect(relock).toHaveBeenCalledWith(true); + }); + + it("does not probe or use host-side recovery without explicit force (#7062)", async () => { + const relock = vi.fn(() => true); + const bail = vi.fn((message: string): never => { + throw new Error(message); + }); + + await expect(prepareMcpForRebuild("alpha", false, false, relock, bail)).resolves.toEqual( + emptyPreparation, + ); + + expect(mocks.executeSandboxCommand).not.toHaveBeenCalled(); + expect(mocks.prepareLive).toHaveBeenCalledWith("alpha"); + expect(mocks.prepareAbsent).not.toHaveBeenCalled(); + }); + + it("keeps already-absent stale recovery on its established host-side path (#7062)", async () => { + const relock = vi.fn(() => true); + const bail = vi.fn((message: string): never => { + throw new Error(message); + }); + + await expect(prepareMcpForRebuild("alpha", true, true, relock, bail)).resolves.toEqual( + emptyPreparation, + ); + + expect(mocks.executeSandboxCommand).not.toHaveBeenCalled(); + expect(mocks.prepareAbsent).toHaveBeenCalledWith("alpha"); + expect(mocks.prepareLive).not.toHaveBeenCalled(); + }); +}); + describe("MCP rebuild retry guidance", () => { it.each([ [true, "--observability"], diff --git a/src/lib/actions/sandbox/rebuild-mcp-phase.ts b/src/lib/actions/sandbox/rebuild-mcp-phase.ts index 7c25568d3db..c96e9eb8456 100644 --- a/src/lib/actions/sandbox/rebuild-mcp-phase.ts +++ b/src/lib/actions/sandbox/rebuild-mcp-phase.ts @@ -13,17 +13,37 @@ import { reattachMcpProvidersAfterRebuildAbort, restoreMcpBridgesAfterRebuild, } from "./mcp-bridge"; +import { executeSandboxCommand } from "./process-recovery"; import type { RebuildBail } from "./rebuild-credential-preflight"; import type { RebuildSandboxEntry } from "./rebuild-flow-helpers"; export type McpRebuildPreparation = Awaited>; +function canExecuteSandboxNoop(sandboxName: string): boolean { + const probe = executeSandboxCommand(sandboxName, ":"); + return probe !== null && probe.status === 0; +} + export async function prepareMcpForRebuild( sandboxName: string, staleRecovery: boolean, + force: boolean, relockShieldsIfNeeded: (sandboxStillExists: boolean) => boolean, - bail: (message: string, code?: number) => never, + bail: RebuildBail, ): Promise { + if (force && !staleRecovery && !canExecuteSandboxNoop(sandboxName)) { + console.error(` ${YW}⚠${R} Sandbox exec probe failed; --force using host-side MCP recovery`); + try { + return await prepareMcpBridgesForAbsentSandboxRebuild(sandboxName); + } catch (error) { + relockShieldsIfNeeded(true); + bail( + `Failed to preserve MCP bridges before rebuild (--force host-side recovery): ${error instanceof Error ? error.message : String(error)}`, + ); + return null; + } + } + try { return await (staleRecovery ? prepareMcpBridgesForAbsentSandboxRebuild(sandboxName) diff --git a/src/lib/actions/sandbox/rebuild-pipeline.ts b/src/lib/actions/sandbox/rebuild-pipeline.ts index d2f4942f779..49e221816e4 100644 --- a/src/lib/actions/sandbox/rebuild-pipeline.ts +++ b/src/lib/actions/sandbox/rebuild-pipeline.ts @@ -203,6 +203,7 @@ async function rebuildSandboxUnlocked( sandboxEntry, staleRecovery, backupManifest: backup.backupManifest, + force: normalized.force, log, bail, relockShieldsIfNeeded, From ad20e0e90ae8a914bff0d8b60685cfe632bfbbd1 Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Sun, 19 Jul 2026 15:14:59 -0700 Subject: [PATCH 02/18] fix(rebuild): preserve MCP state when sandbox exec fails Co-authored-by: kagura-agent Signed-off-by: Apurv Kumaria --- .../gateway-state-owning-gateway.test.ts | 17 + src/lib/actions/sandbox/gateway-state.ts | 11 +- .../actions/sandbox/mcp-bridge-policy.test.ts | 155 +++++- src/lib/actions/sandbox/mcp-bridge-policy.ts | 77 ++- .../mcp-bridge-rebuild-exec-unavailable.ts | 230 ++++++++ src/lib/actions/sandbox/mcp-bridge-rebuild.ts | 6 + src/lib/actions/sandbox/mcp-bridge.ts | 11 + .../sandbox/rebuild-destroy-phase.test.ts | 289 +++++++++- .../actions/sandbox/rebuild-destroy-phase.ts | 183 ++++-- .../actions/sandbox/rebuild-mcp-phase.test.ts | 30 +- src/lib/actions/sandbox/rebuild-mcp-phase.ts | 15 +- src/lib/actions/sandbox/rebuild-pipeline.ts | 8 +- test/helpers/rebuild-flow-recovery-cases.ts | 14 +- test/mcp-destroy-lifecycle.test.ts | 527 +++++++++++++++++- 14 files changed, 1518 insertions(+), 55 deletions(-) create mode 100644 src/lib/actions/sandbox/mcp-bridge-rebuild-exec-unavailable.ts diff --git a/src/lib/actions/sandbox/gateway-state-owning-gateway.test.ts b/src/lib/actions/sandbox/gateway-state-owning-gateway.test.ts index b15098f6260..fec4dc749b1 100644 --- a/src/lib/actions/sandbox/gateway-state-owning-gateway.test.ts +++ b/src/lib/actions/sandbox/gateway-state-owning-gateway.test.ts @@ -55,6 +55,23 @@ describe("getReconciledSandboxGatewayState owning-gateway guard", () => { ); }); + it("classifies the owner-scoped Internal no-spec response as missing", () => { + vi.spyOn(gatewayDrift, "detectOpenShellStateRpcPreflightIssue").mockReturnValue(null); + vi.spyOn(gatewayDrift, "detectOpenShellStateRpcResultIssue").mockReturnValue(null); + const capture = vi.spyOn(openshellRuntime, "captureOpenshell").mockReturnValue({ + status: 1, + output: 'status: Internal, message: "sandbox has no spec"', + } as never); + + expect(getSandboxGatewayState("beta", "nemoclaw-8091")).toMatchObject({ + state: "missing", + }); + expect(capture).toHaveBeenCalledWith( + ["sandbox", "get", "-g", "nemoclaw-8091", "beta"], + expect.anything(), + ); + }); + it("pins the async status RPC to the recorded owner", async () => { vi.spyOn(gatewayDrift, "detectOpenShellStateRpcPreflightIssue").mockReturnValue(null); vi.spyOn(gatewayDrift, "detectOpenShellStateRpcResultIssue").mockReturnValue(null); diff --git a/src/lib/actions/sandbox/gateway-state.ts b/src/lib/actions/sandbox/gateway-state.ts index 22f04fe31dd..f1ef8799cc9 100644 --- a/src/lib/actions/sandbox/gateway-state.ts +++ b/src/lib/actions/sandbox/gateway-state.ts @@ -92,6 +92,13 @@ function gatewayEndpointOverrideState(): SandboxGatewayState | null { } } +/** Canonical OpenShell response classifier for an absent sandbox record. */ +export function isMissingSandboxGatewayOutput(output = ""): boolean { + return /\bNotFound\b|\bNot Found\b|sandbox not found|sandbox has no spec/i.test( + stripAnsi(String(output)), + ); +} + function formatGatewaySchemaMismatchOutput( issue: OpenShellStateRpcIssue, action: string, @@ -181,7 +188,7 @@ export function getSandboxGatewayState( // sibling; an owner-scoped lookup means the sandbox is genuinely absent // from its recorded gateway. Both remain `missing`, and reconciliation uses // the presence of the explicit owner pin to distinguish those cases. - if (/\bNotFound\b|\bNot Found\b|sandbox not found|sandbox has no spec/i.test(output)) { + if (isMissingSandboxGatewayOutput(output)) { return { state: "missing", output }; } if ( @@ -248,7 +255,7 @@ export async function getSandboxGatewayStateForStatus( } return { state: "present", output }; } - if (/\bNotFound\b|\bNot Found\b|sandbox not found|sandbox has no spec/i.test(output)) { + if (isMissingSandboxGatewayOutput(output)) { return { state: "missing", output }; } if ( diff --git a/src/lib/actions/sandbox/mcp-bridge-policy.test.ts b/src/lib/actions/sandbox/mcp-bridge-policy.test.ts index 424e9613106..4da98350065 100644 --- a/src/lib/actions/sandbox/mcp-bridge-policy.test.ts +++ b/src/lib/actions/sandbox/mcp-bridge-policy.test.ts @@ -5,6 +5,7 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import YAML from "yaml"; import * as policies from "../../policy"; +import type { McpBridgeEntry } from "../../state/registry"; import * as registry from "../../state/registry"; import { buildMcpBridgePolicyName, @@ -12,8 +13,23 @@ import { buildMcpBridgeProviderName, MCP_BRIDGE_ALLOWED_METHODS, MCP_BRIDGE_POLICY_MAX_BODY_BYTES, + MCP_BRIDGE_POLICY_SOURCE, } from "./mcp-bridge"; -import { applyGeneratedPolicy } from "./mcp-bridge-policy"; +import { applyGeneratedPolicy, assertGeneratedPolicyExactReadOnly } from "./mcp-bridge-policy"; + +function githubBridgeEntry(overrides: Partial = {}): McpBridgeEntry { + return { + server: "github", + agent: "openclaw", + adapter: "mcporter", + url: "https://api.githubcopilot.com/mcp", + env: ["GITHUB_MCP_TOKEN"], + providerName: "alpha-mcp-github-0123456789abcdef", + policyName: "mcp-bridge-github", + addedAt: "2026-06-01T00:00:00.000Z", + ...overrides, + }; +} describe("MCP OpenShell policy", () => { afterEach(() => { @@ -136,6 +152,143 @@ describe("MCP OpenShell policy", () => { }); }); + it("accepts only the canonical generated policy for the exact bridge and DNS pins", () => { + const entry = githubBridgeEntry(); + const pins = ["8.8.8.8", "2606:4700:4700::1111"]; + const content = buildMcpBridgePolicyYaml(entry.server, entry.url, "mcporter", pins); + const registration = { + name: entry.policyName, + content, + sourcePath: MCP_BRIDGE_POLICY_SOURCE, + }; + vi.spyOn(registry, "getCustomPolicies").mockReturnValue([registration]); + const gatewayState = vi + .spyOn(policies, "getPresetContentGatewayState") + .mockReturnValue("match"); + + expect(assertGeneratedPolicyExactReadOnly("alpha", entry, "mcporter", pins)).toEqual( + registration, + ); + expect(gatewayState).toHaveBeenCalledWith("alpha", content); + }); + + it.each([ + "owned-first", + "unowned-first", + ])("rejects duplicate same-name ownership records regardless of order (%s)", (order) => { + const entry = githubBridgeEntry(); + const pins = ["8.8.8.8"]; + const content = buildMcpBridgePolicyYaml(entry.server, entry.url, "mcporter", pins); + const owned = { + name: entry.policyName, + content, + sourcePath: MCP_BRIDGE_POLICY_SOURCE, + }; + const unowned = { + name: entry.policyName, + content: `${content}\n# conflicting duplicate`, + sourcePath: "/tmp/operator-policy.yaml", + }; + vi.spyOn(registry, "getCustomPolicies").mockReturnValue( + order === "owned-first" ? [owned, unowned] : [unowned, owned], + ); + const gatewayState = vi.spyOn(policies, "getPresetContentGatewayState"); + + expect(() => assertGeneratedPolicyExactReadOnly("alpha", entry, "mcporter", pins)).toThrow( + /ownership is missing or ambiguous/, + ); + expect(gatewayState).not.toHaveBeenCalled(); + }); + + it("rejects individually valid policy records that disagree with their bridge definition", () => { + const entry = githubBridgeEntry(); + const pins = ["8.8.8.8"]; + const canonical = buildMcpBridgePolicyYaml(entry.server, entry.url, "mcporter", pins); + const wrongKeyDocument = YAML.parse(canonical) as { + network_policies: Record; + }; + wrongKeyDocument.network_policies.mcp_bridge_other = { + ...wrongKeyDocument.network_policies.mcp_bridge_github, + name: "mcp_bridge_other", + }; + delete wrongKeyDocument.network_policies.mcp_bridge_github; + + const mismatches: Array<{ + label: string; + candidateEntry?: McpBridgeEntry; + candidateName?: string; + content: string; + }> = [ + { + label: "host", + content: buildMcpBridgePolicyYaml( + entry.server, + "https://mcp.example.test/mcp", + "mcporter", + pins, + ), + }, + { + label: "path", + content: buildMcpBridgePolicyYaml( + entry.server, + "https://api.githubcopilot.com/other", + "mcporter", + pins, + ), + }, + { + label: "adapter", + content: buildMcpBridgePolicyYaml(entry.server, entry.url, "hermes-config", pins), + }, + { label: "network policy key", content: YAML.stringify(wrongKeyDocument) }, + { + label: "resolved address pins", + content: buildMcpBridgePolicyYaml(entry.server, entry.url, "mcporter", ["1.1.1.1"]), + }, + { + label: "policy name", + candidateEntry: githubBridgeEntry({ policyName: "mcp-bridge-other" }), + candidateName: "mcp-bridge-other", + content: canonical, + }, + ]; + + for (const mismatch of mismatches) { + vi.restoreAllMocks(); + const candidateEntry = mismatch.candidateEntry ?? entry; + vi.spyOn(registry, "getCustomPolicies").mockReturnValue([ + { + name: mismatch.candidateName ?? candidateEntry.policyName, + content: mismatch.content, + sourcePath: MCP_BRIDGE_POLICY_SOURCE, + }, + ]); + const gatewayState = vi.spyOn(policies, "getPresetContentGatewayState"); + + expect( + () => assertGeneratedPolicyExactReadOnly("alpha", candidateEntry, "mcporter", pins), + mismatch.label, + ).toThrow(/not canonical for its recorded bridge definition/); + expect(gatewayState, mismatch.label).not.toHaveBeenCalled(); + } + }); + + it("does not expose malformed persisted URLs in canonical ownership errors", () => { + const secret = `nvapi-${"a".repeat(32)}`; + const entry = githubBridgeEntry({ url: `not-a-url-${secret}` }); + vi.spyOn(registry, "getCustomPolicies").mockReturnValue([]); + + let message = ""; + try { + assertGeneratedPolicyExactReadOnly("alpha", entry, "mcporter", ["8.8.8.8"]); + } catch (error) { + message = error instanceof Error ? error.message : String(error); + } + expect(message).toContain("not canonical for its recorded bridge definition"); + expect(message).not.toContain(secret); + }); + it("pins the current OpenShell main client-to-server MCP method profile", () => { expect(MCP_BRIDGE_ALLOWED_METHODS).toEqual([ "initialize", diff --git a/src/lib/actions/sandbox/mcp-bridge-policy.ts b/src/lib/actions/sandbox/mcp-bridge-policy.ts index 8a99f478341..88a5e024655 100644 --- a/src/lib/actions/sandbox/mcp-bridge-policy.ts +++ b/src/lib/actions/sandbox/mcp-bridge-policy.ts @@ -1,6 +1,7 @@ // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 +import type { AgentMcpAdapter } from "../../agent/defs"; import * as policies from "../../policy"; import type { McpBridgeEntry } from "../../state/registry"; import * as registry from "../../state/registry"; @@ -9,7 +10,11 @@ import { MCP_BRIDGE_POLICY_SOURCE, McpBridgeError, } from "./mcp-bridge-contracts"; -import { buildMcpBridgePolicyKey, buildMcpBridgePolicyYaml } from "./mcp-bridge-policy-render"; +import { + buildMcpBridgePolicyKey, + buildMcpBridgePolicyName, + buildMcpBridgePolicyYaml, +} from "./mcp-bridge-policy-render"; export { buildMcpBridgePolicyKey, @@ -235,6 +240,76 @@ export function assertGeneratedPolicyRegistrationMutationSafe( return owned ? registeredPolicy : undefined; } +/** + * Prove that a complete bridge still owns its exact live policy without + * reconciling crash markers or writing registry state. This is intentionally + * stricter than the mutation preflight: a still-live sandbox whose adapter + * cannot be inspected may cross the rebuild delete boundary only from a fully + * committed policy registration generated for the authoritative recorded-agent + * adapter and exactly matching the gateway. + */ +export function assertGeneratedPolicyExactReadOnly( + sandboxName: string, + entry: McpBridgeEntry, + adapter: AgentMcpAdapter, + resolvedAddresses: readonly string[], +): registry.CustomPolicyEntry { + const canonicalOwnershipError = (): McpBridgeError => + new McpBridgeError( + "Generated MCP policy ownership is not canonical for its recorded bridge definition. Refusing host-side rebuild recovery.", + ); + let expectedPolicyName: string; + try { + expectedPolicyName = buildMcpBridgePolicyName(entry.server); + } catch { + throw canonicalOwnershipError(); + } + if ( + entry.policyName !== expectedPolicyName || + entry.adapter !== adapter || + resolvedAddresses.length === 0 + ) { + throw canonicalOwnershipError(); + } + let expectedContent: string; + try { + expectedContent = buildMcpBridgePolicyYaml(entry.server, entry.url, adapter, resolvedAddresses); + } catch { + // Registry entries are untrusted local state. Keep malformed URLs and any + // credential-shaped material out of the recovery diagnostic. + throw canonicalOwnershipError(); + } + const sameNamePolicies = registry + .getCustomPolicies(sandboxName) + .filter((policy) => policy.name === expectedPolicyName); + if (sameNamePolicies.length !== 1) { + throw new McpBridgeError( + "Generated MCP policy ownership is missing or ambiguous. Refusing host-side rebuild recovery for a still-live sandbox.", + ); + } + const [registeredPolicy] = sameNamePolicies; + if (registeredPolicy?.sourcePath !== MCP_BRIDGE_POLICY_SOURCE) { + throw new McpBridgeError( + "Generated MCP policy has no exact NemoClaw ownership record. Refusing host-side rebuild recovery for a still-live sandbox.", + ); + } + if (registeredPolicy.pendingContent !== undefined) { + throw new McpBridgeError( + "Generated MCP policy has an incomplete registry transition. Refusing read-only host-side rebuild recovery.", + ); + } + if (registeredPolicy.content !== expectedContent) { + throw canonicalOwnershipError(); + } + const state = policies.getPresetContentGatewayState(sandboxName, registeredPolicy.content); + if (state !== "match") { + throw new McpBridgeError( + "Generated MCP policy is absent, unreachable, or drifted from its exact ownership record. Refusing host-side rebuild recovery.", + ); + } + return { ...registeredPolicy }; +} + export function removeGeneratedPolicy( sandboxName: string, entry: McpBridgeEntry, diff --git a/src/lib/actions/sandbox/mcp-bridge-rebuild-exec-unavailable.ts b/src/lib/actions/sandbox/mcp-bridge-rebuild-exec-unavailable.ts new file mode 100644 index 00000000000..5d28f84d46b --- /dev/null +++ b/src/lib/actions/sandbox/mcp-bridge-rebuild-exec-unavailable.ts @@ -0,0 +1,230 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import type { AgentMcpAdapter } from "../../agent/defs"; +import { resolveSandboxGatewayName } from "../../onboard/gateway-binding"; +import type { McpBridgeEntry, SandboxEntry } from "../../state/registry"; +import { McpBridgeError } from "./mcp-bridge-contracts"; +import { + assertMcpDestroySnapshotCurrent, + cloneMcpBridgeEntry, + inspectExactMcpDestroyProvider, +} from "./mcp-bridge-destroy-preflight"; +import { assertGeneratedPolicyExactReadOnly } from "./mcp-bridge-policy"; +import { preflightMcpEntryTargets } from "./mcp-bridge-provider"; +import { + assertMcpDestroyNotPending, + bridgeState, + ensureSandboxGatewaySelected, + getBridgeAdapter, + getSandboxAgent, + getSandboxOrThrow, +} from "./mcp-bridge-state"; +import { validateSandboxName } from "./mcp-bridge-validation"; + +type ReadOnlyValidationSnapshot = { + policyByServer: Map; + providerByServer: Map; + targetsByServer: Map; +}; + +type ExplicitAdapterMcpBridgeEntry = McpBridgeEntry & { adapter: AgentMcpAdapter }; + +export interface ExecUnavailableMcpRebuildPreparation { + entries: McpBridgeEntry[]; + detachedProviderEntries: McpBridgeEntry[]; + scrubbedAdapterEntries: McpBridgeEntry[]; + revalidateBeforeDelete: () => Promise; + assertDeleteEdgeUnchanged: () => void; +} + +function snapshotCompleteEntries(sandboxName: string): { + entries: ExplicitAdapterMcpBridgeEntry[]; + gatewayName: string; + agentName: string; + adapter: AgentMcpAdapter; +} { + validateSandboxName(sandboxName); + const sandbox = getSandboxOrThrow(sandboxName); + assertMcpDestroyNotPending(sandbox); + const entries = Object.values(bridgeState(sandbox)).map(cloneMcpBridgeEntry); + const incomplete = entries.find((entry) => entry.addState !== undefined); + if (incomplete) { + throw new McpBridgeError( + `MCP server '${incomplete.server}' has an incomplete add transaction (${incomplete.addState}). Read-only host-side rebuild recovery cannot discard or adopt it; re-run the original mcp add command or remove it with --force before rebuilding the sandbox.`, + ); + } + const agent = getSandboxAgent(sandbox); + const adapter = getBridgeAdapter(agent); + const incompatible = entries.find( + (entry) => entry.agent !== agent.name || entry.adapter !== adapter, + ); + if (incompatible) { + throw new McpBridgeError( + "Managed MCP adapter identity is missing or incompatible with the sandbox's recorded agent. Refusing read-only host-side rebuild recovery.", + ); + } + return { + entries: entries.map((entry) => ({ ...entry, adapter })), + gatewayName: resolveSandboxGatewayName(sandbox), + agentName: agent.name, + adapter, + }; +} + +function policyFingerprint(policy: ReturnType): string { + return JSON.stringify({ + name: policy.name, + content: policy.content, + pendingContent: policy.pendingContent, + sourcePath: policy.sourcePath, + appliedAt: policy.appliedAt, + }); +} + +function providerFingerprint(provider: ReturnType): string { + return JSON.stringify({ + exists: provider.exists, + id: provider.id, + resourceVersion: provider.resourceVersion, + type: provider.type, + credentialKeys: provider.credentialKeys, + }); +} + +function targetFingerprint(addresses: readonly string[] | undefined): string { + if (!addresses || addresses.length === 0) { + throw new McpBridgeError( + "Resolved MCP target validation returned no exact public address pins. Refusing host-side rebuild recovery.", + ); + } + return JSON.stringify([...addresses].sort()); +} + +async function inspectReadOnlyRecoveryState( + sandboxName: string, + entries: readonly McpBridgeEntry[], + adapter: AgentMcpAdapter, +): Promise { + const resolvedTargets = await preflightMcpEntryTargets(entries); + if (entries.length > 0) await ensureSandboxGatewaySelected(sandboxName); + + const policyByServer = new Map(); + const providerByServer = new Map(); + const targetsByServer = new Map(); + for (const entry of entries) { + const resolvedAddresses = resolvedTargets.get(entry.server); + const policy = assertGeneratedPolicyExactReadOnly( + sandboxName, + entry, + adapter, + resolvedAddresses ?? [], + ); + policyByServer.set(entry.server, policyFingerprint(policy)); + const provider = inspectExactMcpDestroyProvider(entry, { allowMissing: false }); + providerByServer.set(entry.server, providerFingerprint(provider)); + targetsByServer.set(entry.server, targetFingerprint(resolvedAddresses)); + } + return { policyByServer, providerByServer, targetsByServer }; +} + +function assertValidationSnapshotCurrent( + entries: readonly McpBridgeEntry[], + expected: ReadOnlyValidationSnapshot, + current: ReadOnlyValidationSnapshot, +): void { + const drifted = entries.find( + (entry) => + current.policyByServer.get(entry.server) !== expected.policyByServer.get(entry.server) || + current.providerByServer.get(entry.server) !== expected.providerByServer.get(entry.server) || + current.targetsByServer.get(entry.server) !== expected.targetsByServer.get(entry.server), + ); + if (drifted) { + throw new McpBridgeError( + `MCP server '${drifted.server}' changed after host-side rebuild preflight. Refusing to delete the still-live sandbox; retry after its target, policy, and provider state is stable.`, + ); + } +} + +function assertDeleteEdgeUnchanged( + sandboxName: string, + expectedEntries: readonly McpBridgeEntry[], + expectedGatewayName: string, + expectedAgentName: string, + expectedAdapter: AgentMcpAdapter, +): void { + const sandbox: SandboxEntry = assertMcpDestroySnapshotCurrent(sandboxName, expectedEntries); + assertMcpDestroyNotPending(sandbox); + try { + const agent = getSandboxAgent(sandbox); + if (agent.name !== expectedAgentName || getBridgeAdapter(agent) !== expectedAdapter) { + throw new Error("adapter binding changed"); + } + } catch { + throw new McpBridgeError( + `Sandbox '${sandboxName}' changed its recorded agent or MCP adapter after host-side rebuild preflight. Refusing to delete it.`, + ); + } + if (resolveSandboxGatewayName(sandbox) !== expectedGatewayName) { + throw new McpBridgeError( + `Sandbox '${sandboxName}' changed its recorded gateway after host-side rebuild preflight. Refusing to delete it.`, + ); + } +} + +async function revalidateBeforeDelete( + sandboxName: string, + expectedEntries: readonly McpBridgeEntry[], + expectedGatewayName: string, + expectedAgentName: string, + expectedAdapter: AgentMcpAdapter, + expectedValidation: ReadOnlyValidationSnapshot, +): Promise { + assertDeleteEdgeUnchanged( + sandboxName, + expectedEntries, + expectedGatewayName, + expectedAgentName, + expectedAdapter, + ); + const currentValidation = await inspectReadOnlyRecoveryState( + sandboxName, + expectedEntries, + expectedAdapter, + ); + assertValidationSnapshotCurrent(expectedEntries, expectedValidation, currentValidation); +} + +/** + * Preserve complete MCP intent when sandbox exec is unavailable but OpenShell + * still reports the sandbox live. Unlike absent-sandbox recovery, this path is + * read-only: it never discards add markers, scrubs adapters, detaches providers, + * reconciles policy records, or otherwise mutates MCP ownership before delete. + */ +export async function prepareMcpBridgesForExecUnavailableRebuild( + sandboxName: string, +): Promise { + const { entries, gatewayName, agentName, adapter } = snapshotCompleteEntries(sandboxName); + const expectedEntries = entries.map(cloneMcpBridgeEntry); + const expectedValidation = await inspectReadOnlyRecoveryState( + sandboxName, + expectedEntries, + adapter, + ); + return { + entries: entries.map(cloneMcpBridgeEntry), + detachedProviderEntries: [], + scrubbedAdapterEntries: [], + revalidateBeforeDelete: () => + revalidateBeforeDelete( + sandboxName, + expectedEntries, + gatewayName, + agentName, + adapter, + expectedValidation, + ), + assertDeleteEdgeUnchanged: () => + assertDeleteEdgeUnchanged(sandboxName, expectedEntries, gatewayName, agentName, adapter), + }; +} diff --git a/src/lib/actions/sandbox/mcp-bridge-rebuild.ts b/src/lib/actions/sandbox/mcp-bridge-rebuild.ts index f8321b2fcbe..215a38d0274 100644 --- a/src/lib/actions/sandbox/mcp-bridge-rebuild.ts +++ b/src/lib/actions/sandbox/mcp-bridge-rebuild.ts @@ -42,8 +42,14 @@ export interface McpRebuildPreparation { entries: McpBridgeEntry[]; detachedProviderEntries: McpBridgeEntry[]; scrubbedAdapterEntries: McpBridgeEntry[]; + /** Full read-only target, policy, provider, and registry proof before delete. */ + revalidateBeforeDelete?: () => Promise; + /** Final synchronous registry-only proof immediately before delete. */ + assertDeleteEdgeUnchanged?: () => void; } +export { prepareMcpBridgesForExecUnavailableRebuild } from "./mcp-bridge-rebuild-exec-unavailable"; + async function getCompleteMcpRebuildEntries( sandboxName: string, options: { sandboxAbsent?: boolean } = {}, diff --git a/src/lib/actions/sandbox/mcp-bridge.ts b/src/lib/actions/sandbox/mcp-bridge.ts index d31fdfaae1b..047048dfc04 100644 --- a/src/lib/actions/sandbox/mcp-bridge.ts +++ b/src/lib/actions/sandbox/mcp-bridge.ts @@ -17,6 +17,7 @@ import { import { redactBridgeSecretsForDisplay } from "./mcp-bridge-output"; import { prepareMcpBridgesForAbsentSandboxRebuild as prepareMcpBridgesForAbsentSandboxRebuildLifecycle, + prepareMcpBridgesForExecUnavailableRebuild as prepareMcpBridgesForExecUnavailableRebuildLifecycle, prepareMcpBridgesForRebuild as prepareMcpBridgesForRebuildLifecycle, reattachMcpProvidersAfterRebuildAbort as reattachMcpProvidersAfterRebuildAbortLifecycle, restoreMcpBridgesAfterRebuild as restoreMcpBridgesAfterRebuildLifecycle, @@ -94,6 +95,10 @@ export interface McpRebuildPreparation { entries: McpBridgeEntry[]; detachedProviderEntries: McpBridgeEntry[]; scrubbedAdapterEntries: McpBridgeEntry[]; + /** Full read-only target, policy, provider, and registry proof before delete. */ + revalidateBeforeDelete?: () => Promise; + /** Final synchronous registry-only proof immediately before delete. */ + assertDeleteEdgeUnchanged?: () => void; } export async function addMcpBridge( @@ -149,6 +154,12 @@ export async function prepareMcpBridgesForAbsentSandboxRebuild( return prepareMcpBridgesForAbsentSandboxRebuildLifecycle(sandboxName); } +export async function prepareMcpBridgesForExecUnavailableRebuild( + sandboxName: string, +): Promise { + return prepareMcpBridgesForExecUnavailableRebuildLifecycle(sandboxName); +} + export async function prepareMcpBridgesForRebuild( sandboxName: string, ): Promise { diff --git a/src/lib/actions/sandbox/rebuild-destroy-phase.test.ts b/src/lib/actions/sandbox/rebuild-destroy-phase.test.ts index e29ca0e0d0c..5995a1d08d9 100644 --- a/src/lib/actions/sandbox/rebuild-destroy-phase.test.ts +++ b/src/lib/actions/sandbox/rebuild-destroy-phase.test.ts @@ -7,11 +7,21 @@ const mocks = vi.hoisted(() => ({ prepareMcpForRebuild: vi.fn(), reattachMcpAfterDeleteFailure: vi.fn(), warnUnpreservedUserManagedFiles: vi.fn(), - runOpenshell: vi.fn(() => ({ status: 0, stdout: "", stderr: "" })), - getSandboxDeleteOutcome: vi.fn(() => ({ alreadyGone: false })), - getSandbox: vi.fn(() => null), + runOpenshell: vi.fn( + (): { + status: number | null; + stdout: string; + stderr: string; + error?: NodeJS.ErrnoException; + } => ({ status: 0, stdout: "", stderr: "" }), + ), + getSandbox: vi.fn( + (_name: string): { name: string; agent: string; nimContainer?: string | null } | null => null, + ), listSandboxes: vi.fn(() => ({ sandboxes: [] })), removeSandboxRegistryEntryWithReceipt: vi.fn(() => null), + stopNimContainer: vi.fn(), + stopNimContainerByName: vi.fn(), })); vi.mock("./rebuild-flow-helpers", async (importOriginal) => ({ @@ -29,8 +39,9 @@ vi.mock("../../adapters/openshell/runtime", () => ({ runOpenshell: mocks.runOpenshell, })); -vi.mock("../../domain/sandbox/destroy", () => ({ - getSandboxDeleteOutcome: mocks.getSandboxDeleteOutcome, +vi.mock("../../inference/nim", () => ({ + stopNimContainer: mocks.stopNimContainer, + stopNimContainerByName: mocks.stopNimContainerByName, })); vi.mock("../../state/registry", async (importOriginal) => ({ @@ -120,4 +131,272 @@ describe("rebuild destroy validation diagnostics", () => { expect.any(Function), ); }); + + it("refuses sandbox deletion when read-only MCP state drifts at the delete edge (#7062)", async () => { + const revalidateBeforeDelete = vi.fn().mockRejectedValue(new Error("live policy drifted")); + mocks.prepareMcpForRebuild.mockResolvedValue({ + entries: [{}], + detachedProviderEntries: [], + scrubbedAdapterEntries: [], + revalidateBeforeDelete, + }); + const relockShieldsIfNeeded = vi.fn(() => true); + const bail = vi.fn((message: string): never => { + throw new Error(message); + }); + + await expect( + runRebuildDestroyPhase({ + sandboxName: "alpha", + sandboxEntry: { name: "alpha", agent: "openclaw" }, + staleRecovery: false, + backupManifest: null, + force: true, + log: vi.fn(), + bail, + relockShieldsIfNeeded, + onDeleted: vi.fn(), + }), + ).rejects.toThrow( + "Failed to revalidate read-only MCP recovery before sandbox deletion: live policy drifted", + ); + + expect(revalidateBeforeDelete).toHaveBeenCalledOnce(); + expect(mocks.runOpenshell).not.toHaveBeenCalled(); + expect(mocks.removeSandboxRegistryEntryWithReceipt).not.toHaveBeenCalled(); + expect(mocks.reattachMcpAfterDeleteFailure).not.toHaveBeenCalled(); + expect(mocks.stopNimContainer).not.toHaveBeenCalled(); + expect(mocks.stopNimContainerByName).not.toHaveBeenCalled(); + expect(relockShieldsIfNeeded).toHaveBeenCalledWith(true); + }); + + it("retains read-only MCP ownership when sandbox deletion fails (#7062)", async () => { + const revalidateBeforeDelete = vi.fn().mockResolvedValue(undefined); + const entry = { server: "github" }; + mocks.prepareMcpForRebuild.mockResolvedValue({ + entries: [entry], + detachedProviderEntries: [], + scrubbedAdapterEntries: [], + revalidateBeforeDelete, + }); + mocks.runOpenshell + .mockReturnValueOnce({ status: 9, stdout: "", stderr: "delete failed" }) + .mockReturnValueOnce({ status: 0, stdout: "Phase: Ready\n", stderr: "" }); + const onDeleted = vi.fn(); + const relockShieldsIfNeeded = vi.fn(() => true); + const bail = vi.fn((message: string): never => { + throw new Error(message); + }); + + await expect( + runRebuildDestroyPhase({ + sandboxName: "alpha", + sandboxEntry: { name: "alpha", agent: "openclaw" }, + staleRecovery: false, + backupManifest: null, + force: true, + log: vi.fn(), + bail, + relockShieldsIfNeeded, + onDeleted, + }), + ).rejects.toThrow("Failed to delete sandbox."); + + expect(revalidateBeforeDelete).toHaveBeenCalledOnce(); + expect(revalidateBeforeDelete.mock.invocationCallOrder[0]).toBeLessThan( + mocks.runOpenshell.mock.invocationCallOrder[0] ?? Number.POSITIVE_INFINITY, + ); + expect(mocks.reattachMcpAfterDeleteFailure).toHaveBeenCalledWith("alpha", [], []); + expect(mocks.removeSandboxRegistryEntryWithReceipt).not.toHaveBeenCalled(); + expect(onDeleted).not.toHaveBeenCalled(); + expect(mocks.stopNimContainer).not.toHaveBeenCalled(); + expect(mocks.stopNimContainerByName).not.toHaveBeenCalled(); + expect(relockShieldsIfNeeded).toHaveBeenCalledWith(true); + expect(mocks.runOpenshell).toHaveBeenNthCalledWith( + 2, + ["sandbox", "get", "-g", "nemoclaw", "alpha"], + expect.any(Object), + ); + }); + + it("converges as deleted when a nonzero delete is followed by exact NotFound (#7062)", async () => { + mocks.getSandbox.mockReturnValueOnce({ + name: "alpha", + agent: "openclaw", + nimContainer: "nim-alpha", + }); + mocks.prepareMcpForRebuild.mockResolvedValue({ + entries: [{ server: "github" }], + detachedProviderEntries: [{ server: "github" }], + scrubbedAdapterEntries: [], + }); + mocks.runOpenshell + .mockReturnValueOnce({ status: 9, stdout: "", stderr: "delete interrupted" }) + .mockReturnValueOnce({ + status: 1, + stdout: "", + stderr: 'status: Internal, message: "sandbox has no spec"', + }); + const onDeleted = vi.fn(); + const relockShieldsIfNeeded = vi.fn(() => true); + + const result = await runRebuildDestroyPhase({ + sandboxName: "alpha", + sandboxEntry: { name: "alpha", agent: "openclaw", gatewayName: "nemoclaw" }, + staleRecovery: false, + backupManifest: null, + force: true, + log: vi.fn(), + bail: vi.fn((message: string): never => { + throw new Error(message); + }), + relockShieldsIfNeeded, + onDeleted, + }); + + expect(result?.entries).toEqual([{ server: "github" }]); + expect(onDeleted).toHaveBeenCalledOnce(); + expect(mocks.stopNimContainerByName).toHaveBeenCalledWith("nim-alpha"); + expect(mocks.reattachMcpAfterDeleteFailure).not.toHaveBeenCalled(); + expect(relockShieldsIfNeeded).not.toHaveBeenCalled(); + }); + + it("preserves recovery ownership when post-delete state is partial or ambiguous (#7062)", async () => { + mocks.getSandbox.mockReturnValueOnce({ + name: "alpha", + agent: "openclaw", + nimContainer: "nim-alpha", + }); + mocks.prepareMcpForRebuild.mockResolvedValue({ + entries: [{ server: "github" }], + detachedProviderEntries: [{ server: "github" }], + scrubbedAdapterEntries: [], + }); + mocks.runOpenshell + .mockReturnValueOnce({ status: 9, stdout: "", stderr: "delete interrupted" }) + .mockReturnValueOnce({ status: 0, stdout: "Phase: Terminating\n", stderr: "" }); + const onDeleted = vi.fn(); + const onDeleteStateAmbiguous = vi.fn(); + const relockShieldsIfNeeded = vi.fn(() => true); + + await expect( + runRebuildDestroyPhase({ + sandboxName: "alpha", + sandboxEntry: { name: "alpha", agent: "openclaw", gatewayName: "nemoclaw" }, + staleRecovery: false, + backupManifest: null, + force: true, + log: vi.fn(), + bail: vi.fn((message: string): never => { + throw new Error(message); + }), + relockShieldsIfNeeded, + onDeleted, + onDeleteStateAmbiguous, + }), + ).rejects.toThrow(/exact post-delete state is ambiguous.*recovery state was preserved/i); + + expect(onDeleted).not.toHaveBeenCalled(); + expect(onDeleteStateAmbiguous).toHaveBeenCalledOnce(); + expect(mocks.stopNimContainer).not.toHaveBeenCalled(); + expect(mocks.stopNimContainerByName).not.toHaveBeenCalled(); + expect(mocks.reattachMcpAfterDeleteFailure).not.toHaveBeenCalled(); + expect(relockShieldsIfNeeded).not.toHaveBeenCalled(); + }); + + it("does not treat missing-looking partial output from a timed-out probe as deleted (#7062)", async () => { + mocks.getSandbox.mockReturnValueOnce({ + name: "alpha", + agent: "openclaw", + nimContainer: "nim-alpha", + }); + mocks.prepareMcpForRebuild.mockResolvedValue({ + entries: [{ server: "github" }], + detachedProviderEntries: [{ server: "github" }], + scrubbedAdapterEntries: [], + }); + mocks.runOpenshell + .mockReturnValueOnce({ status: 9, stdout: "", stderr: "delete interrupted" }) + .mockReturnValueOnce({ + status: null, + stdout: "", + stderr: 'status: Internal, message: "sandbox has no spec"', + error: Object.assign(new Error("probe timed out"), { code: "ETIMEDOUT" }), + }); + const onDeleted = vi.fn(); + const onDeleteStateAmbiguous = vi.fn(); + const relockShieldsIfNeeded = vi.fn(() => true); + + await expect( + runRebuildDestroyPhase({ + sandboxName: "alpha", + sandboxEntry: { name: "alpha", agent: "openclaw", gatewayName: "nemoclaw" }, + staleRecovery: false, + backupManifest: null, + force: true, + log: vi.fn(), + bail: vi.fn((message: string): never => { + throw new Error(message); + }), + relockShieldsIfNeeded, + onDeleted, + onDeleteStateAmbiguous, + }), + ).rejects.toThrow(/exact post-delete state is ambiguous.*recovery state was preserved/i); + + expect(mocks.runOpenshell).toHaveBeenNthCalledWith( + 2, + ["sandbox", "get", "-g", "nemoclaw", "alpha"], + expect.objectContaining({ timeout: 15_000 }), + ); + expect(onDeleted).not.toHaveBeenCalled(); + expect(onDeleteStateAmbiguous).toHaveBeenCalledOnce(); + expect(mocks.removeSandboxRegistryEntryWithReceipt).not.toHaveBeenCalled(); + expect(mocks.stopNimContainer).not.toHaveBeenCalled(); + expect(mocks.stopNimContainerByName).not.toHaveBeenCalled(); + expect(mocks.reattachMcpAfterDeleteFailure).not.toHaveBeenCalled(); + expect(relockShieldsIfNeeded).not.toHaveBeenCalled(); + }); + + it("stops local NIM only after a read-only MCP rebuild deletes the sandbox (#7062)", async () => { + const revalidateBeforeDelete = vi.fn().mockResolvedValue(undefined); + const assertDeleteEdgeUnchanged = vi.fn(); + mocks.getSandbox.mockReturnValueOnce({ + name: "alpha", + agent: "openclaw", + nimContainer: "nim-alpha", + }); + mocks.prepareMcpForRebuild.mockResolvedValue({ + entries: [{ server: "github" }], + detachedProviderEntries: [], + scrubbedAdapterEntries: [], + revalidateBeforeDelete, + assertDeleteEdgeUnchanged, + }); + + await runRebuildDestroyPhase({ + sandboxName: "alpha", + sandboxEntry: { name: "alpha", agent: "openclaw" }, + staleRecovery: false, + backupManifest: null, + force: true, + log: vi.fn(), + bail: vi.fn((message: string): never => { + throw new Error(message); + }), + relockShieldsIfNeeded: vi.fn(() => true), + onDeleted: vi.fn(), + }); + + expect(revalidateBeforeDelete).toHaveBeenCalledOnce(); + expect(assertDeleteEdgeUnchanged).toHaveBeenCalledOnce(); + expect(mocks.stopNimContainer).not.toHaveBeenCalled(); + expect(mocks.stopNimContainerByName).toHaveBeenCalledWith("nim-alpha"); + expect(assertDeleteEdgeUnchanged.mock.invocationCallOrder[0]).toBeLessThan( + mocks.runOpenshell.mock.invocationCallOrder[0] ?? Number.POSITIVE_INFINITY, + ); + expect(mocks.runOpenshell.mock.invocationCallOrder[0]).toBeLessThan( + mocks.stopNimContainerByName.mock.invocationCallOrder[0] ?? Number.POSITIVE_INFINITY, + ); + }); }); diff --git a/src/lib/actions/sandbox/rebuild-destroy-phase.ts b/src/lib/actions/sandbox/rebuild-destroy-phase.ts index 45cb98ddce7..a76ffc5c105 100644 --- a/src/lib/actions/sandbox/rebuild-destroy-phase.ts +++ b/src/lib/actions/sandbox/rebuild-destroy-phase.ts @@ -2,12 +2,16 @@ // SPDX-License-Identifier: Apache-2.0 import { runOpenshell } from "../../adapters/openshell/runtime"; +import { OPENSHELL_PROBE_TIMEOUT_MS } from "../../adapters/openshell/timeouts"; import { G, R } from "../../cli/terminal-style"; import { getSandboxDeleteOutcome } from "../../domain/sandbox/destroy"; import * as nim from "../../inference/nim"; +import { resolveSandboxGatewayName } from "../../onboard/gateway-binding"; import { redactFull } from "../../security/redact"; +import { parseSandboxPhase } from "../../state/gateway"; import * as registry from "../../state/registry"; import { removeSandboxRegistryEntryWithReceipt } from "./destroy"; +import { isMissingSandboxGatewayOutput } from "./gateway-state"; import type { RebuildBackupManifest } from "./rebuild-backup-phase"; import type { RebuildBail, RebuildLog } from "./rebuild-credential-preflight"; import { type RebuildSandboxEntry, warnUnpreservedUserManagedFiles } from "./rebuild-flow-helpers"; @@ -33,14 +37,70 @@ export interface RebuildDestroyPhaseInput { force?: boolean; validateAfterMcpPreparation?: () => Promise; onDeleted: () => void; + onDeleteStateAmbiguous?: () => void; } export type RebuildDestroyPhaseResult = McpRebuildPreparation & { removalReceipt: registry.SandboxRemovalReceipt | null; }; +type PostDeleteReconciliation = + | { state: "deleted"; phase: null; status: number | null } + | { state: "intact"; phase: "Ready" | "Running"; status: 0 } + | { state: "ambiguous"; phase: string | null; status: number | null }; + +/** + * A nonzero delete may be reported after OpenShell has already changed the + * sandbox. Query the exact recorded gateway and classify only an explicit + * NotFound as deleted or a live Ready/Running phase as intact. Everything else + * stays ambiguous so recovery never invents an ownership boundary. + */ +function reconcileFailedSandboxDelete( + sandboxName: string, + sandboxEntry: RebuildSandboxEntry, + log: RebuildLog, +): PostDeleteReconciliation { + let gatewayName: string; + try { + gatewayName = resolveSandboxGatewayName(sandboxEntry); + } catch { + log("Post-delete reconciliation could not resolve the recorded sandbox gateway."); + return { state: "ambiguous", phase: null, status: null }; + } + + let probe: ReturnType; + try { + probe = runOpenshell(["sandbox", "get", "-g", gatewayName, sandboxName], { + ignoreError: true, + stdio: ["ignore", "pipe", "pipe"], + timeout: OPENSHELL_PROBE_TIMEOUT_MS, + }); + } catch { + log(`Post-delete reconciliation could not query recorded gateway '${gatewayName}'.`); + return { state: "ambiguous", phase: null, status: null }; + } + if (probe.error) { + log(`Post-delete reconciliation could not complete on recorded gateway '${gatewayName}'.`); + return { state: "ambiguous", phase: null, status: probe.status }; + } + const probeOutput = `${probe.stdout || ""}\n${probe.stderr || ""}`; + if (probe.status !== 0 && isMissingSandboxGatewayOutput(probeOutput)) { + log(`Post-delete reconciliation on '${gatewayName}': sandbox is absent.`); + return { state: "deleted", phase: null, status: probe.status }; + } + const phase = probe.status === 0 ? parseSandboxPhase(probeOutput) : null; + if (probe.status === 0 && (phase === "Ready" || phase === "Running")) { + log(`Post-delete reconciliation on '${gatewayName}': sandbox remains ${phase}.`); + return { state: "intact", phase, status: 0 }; + } + log( + `Post-delete reconciliation on '${gatewayName}' is ambiguous: exit=${probe.status}, phase=${phase ?? "unknown"}.`, + ); + return { state: "ambiguous", phase, status: probe.status }; +} + /** - * Detach owned MCP state, stop inference, and delete the old sandbox. + * Detach owned MCP state, delete the old sandbox, and then stop inference. * Boundary coverage: rebuild-flow.test.ts exercises success, stale recovery, * delete failure, provider reattach failure, and MCP-bearing registry retention. */ @@ -66,15 +126,34 @@ export async function runRebuildDestroyPhase( log( `Registry entry: agent=${sbMeta?.agent}, agentVersion=${sbMeta?.agentVersion}, nimContainer=${sbMeta?.nimContainer}`, ); + const stopNimBestEffort = (): void => { + try { + if (sbMeta && sbMeta.nimContainer) { + log(`Stopping NIM container: ${sbMeta.nimContainer}`); + nim.stopNimContainerByName(sbMeta.nimContainer); + } else { + // Best-effort cleanup — see comment in sandboxDestroy. + nim.stopNimContainer(sandboxName, { silent: true }); + } + } catch (error) { + // Keep the established best-effort contract if the local runtime throws; + // recreate force-removes the old name after a successful sandbox delete. + log( + `Best-effort NIM stop failed; continuing rebuild: ${error instanceof Error ? error.message : String(error)}`, + ); + } + }; const mcpPreparation = await prepareMcpBeforeBestEffortNimStop({ - prepareMcp: () => - prepareMcpForRebuild( + prepareMcp: async () => { + const preparation = await prepareMcpForRebuild( sandboxName, staleRecovery, input.force === true, relockShieldsIfNeeded, bail, - ), + ); + return preparation; + }, afterPrepare: async (preparation) => { // MCP preparation removes only adapter entries whose exact ownership // fingerprints match the registry. Probe afterward so a Deep Agents @@ -108,15 +187,10 @@ export async function runRebuildDestroyPhase( ); } }, - stopNim: () => { - if (sbMeta && sbMeta.nimContainer) { - log(`Stopping NIM container: ${sbMeta.nimContainer}`); - nim.stopNimContainerByName(sbMeta.nimContainer); - } else { - // Best-effort cleanup — see comment in sandboxDestroy. - nim.stopNimContainer(sandboxName, { silent: true }); - } - }, + // A nonzero OpenShell delete may arrive after partial mutation. Keep local + // inference alive until deletion is positively confirmed for every rebuild + // path, not only read-only MCP recovery. + stopNim: () => undefined, log, }); if (!mcpPreparation) return null; @@ -124,6 +198,24 @@ export async function runRebuildDestroyPhase( const rebuildDetachedMcpProviderEntries = mcpPreparation.detachedProviderEntries; const rebuildScrubbedMcpAdapterEntries = mcpPreparation.scrubbedAdapterEntries; + // Exec-unavailable recovery deliberately made no MCP mutation during + // preparation. Re-prove target, policy, provider, and registry state while + // the original sandbox and local NIM are still intact. Then run one final + // synchronous registry check at the no-await edge immediately before delete. + if (mcpPreparation.revalidateBeforeDelete || mcpPreparation.assertDeleteEdgeUnchanged) { + try { + await mcpPreparation.revalidateBeforeDelete?.(); + mcpPreparation.assertDeleteEdgeUnchanged?.(); + } catch (error) { + relockShieldsIfNeeded(true); + const detail = error instanceof Error ? error.message : String(error); + bail( + `Failed to revalidate read-only MCP recovery before sandbox deletion: ${redactFull(detail)}`, + ); + return null; + } + } + log(`Running: openshell sandbox delete ${sandboxName}`); const deleteResult = runOpenshell(["sandbox", "delete", sandboxName], { ignoreError: true, @@ -131,30 +223,55 @@ export async function runRebuildDestroyPhase( }); const { alreadyGone } = getSandboxDeleteOutcome(deleteResult); log(`Delete result: exit=${deleteResult.status}, alreadyGone=${alreadyGone}`); - if (deleteResult.status !== 0 && !alreadyGone) { - console.error(" Failed to delete sandbox. Aborting rebuild."); - const mcpRecoveryFailure = await reattachMcpAfterDeleteFailure( - sandboxName, - rebuildDetachedMcpProviderEntries, - rebuildScrubbedMcpAdapterEntries, - ); - if (mcpRecoveryFailure) { + if (deleteResult.status !== 0) { + const reconciledDelete = reconcileFailedSandboxDelete(sandboxName, input.sandboxEntry, log); + if (reconciledDelete.state === "deleted") { + log("Delete returned nonzero, but exact post-delete state confirms sandbox removal."); + } else if (reconciledDelete.state === "intact") { + console.error(" Failed to delete sandbox. Aborting rebuild."); console.error( - ` Failed to reattach MCP providers to the existing sandbox: ${mcpRecoveryFailure}`, + ` Exact post-delete verification confirms the original sandbox remains ${reconciledDelete.phase}.`, ); + const mcpRecoveryFailure = await reattachMcpAfterDeleteFailure( + sandboxName, + rebuildDetachedMcpProviderEntries, + rebuildScrubbedMcpAdapterEntries, + ); + if (mcpRecoveryFailure) { + console.error( + ` Failed to reattach MCP providers to the existing sandbox: ${mcpRecoveryFailure}`, + ); + } + if (backupManifest) { + console.error(" State backup is preserved at: " + backupManifest.backupPath); + } + relockShieldsIfNeeded(true); + bail( + mcpRecoveryFailure + ? `Failed to delete sandbox; MCP provider recovery also failed: ${mcpRecoveryFailure}` + : "Failed to delete sandbox.", + deleteResult.status || 1, + ); + return null; + } else { + console.error( + " Sandbox deletion returned an error, and exact post-delete state is ambiguous.", + ); + console.error( + " MCP ownership and recovery metadata were preserved; local NIM was not stopped.", + ); + if (backupManifest) { + console.error(" State backup is preserved at: " + backupManifest.backupPath); + } + input.onDeleteStateAmbiguous?.(); + bail( + "Sandbox delete failed and exact post-delete state is ambiguous; recovery state was preserved.", + deleteResult.status || 1, + ); + return null; } - if (backupManifest) { - console.error(" State backup is preserved at: " + backupManifest.backupPath); - } - relockShieldsIfNeeded(true); - bail( - mcpRecoveryFailure - ? `Failed to delete sandbox; MCP provider recovery also failed: ${mcpRecoveryFailure}` - : "Failed to delete sandbox.", - deleteResult.status || 1, - ); - return null; } + stopNimBestEffort(); onDeleted(); let removalReceipt: registry.SandboxRemovalReceipt | null = null; if (rebuildMcpEntries.length === 0) { diff --git a/src/lib/actions/sandbox/rebuild-mcp-phase.test.ts b/src/lib/actions/sandbox/rebuild-mcp-phase.test.ts index 05d872c0c33..adbc3a2bf76 100644 --- a/src/lib/actions/sandbox/rebuild-mcp-phase.test.ts +++ b/src/lib/actions/sandbox/rebuild-mcp-phase.test.ts @@ -6,11 +6,13 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; const mocks = vi.hoisted(() => ({ executeSandboxCommand: vi.fn(), prepareAbsent: vi.fn(), + prepareExecUnavailable: vi.fn(), prepareLive: vi.fn(), })); vi.mock("./mcp-bridge", () => ({ prepareMcpBridgesForAbsentSandboxRebuild: mocks.prepareAbsent, + prepareMcpBridgesForExecUnavailableRebuild: mocks.prepareExecUnavailable, prepareMcpBridgesForRebuild: mocks.prepareLive, reattachMcpProvidersAfterRebuildAbort: vi.fn(), restoreMcpBridgesAfterRebuild: vi.fn(), @@ -38,6 +40,7 @@ describe("forced rebuild MCP preparation", () => { vi.spyOn(console, "error").mockImplementation(() => undefined); mocks.executeSandboxCommand.mockReturnValue({ status: 0, stdout: "", stderr: "" }); mocks.prepareAbsent.mockResolvedValue(emptyPreparation); + mocks.prepareExecUnavailable.mockResolvedValue(emptyPreparation); mocks.prepareLive.mockResolvedValue(emptyPreparation); }); @@ -53,8 +56,28 @@ describe("forced rebuild MCP preparation", () => { ); expect(mocks.executeSandboxCommand).toHaveBeenCalledWith("alpha", ":"); - expect(mocks.prepareAbsent).toHaveBeenCalledWith("alpha"); + expect(mocks.prepareExecUnavailable).toHaveBeenCalledWith("alpha"); + expect(mocks.prepareAbsent).not.toHaveBeenCalled(); + expect(mocks.prepareLive).not.toHaveBeenCalled(); + expect(relock).not.toHaveBeenCalled(); + }); + + it.each([ + 1, 64, 126, 127, 255, + ])("routes every nonzero exec result (%i) through explicit force recovery (#7062)", async (status) => { + mocks.executeSandboxCommand.mockReturnValue({ status, stdout: "", stderr: "exec failed" }); + const relock = vi.fn(() => true); + const bail = vi.fn((message: string): never => { + throw new Error(message); + }); + + await expect(prepareMcpForRebuild("alpha", false, true, relock, bail)).resolves.toEqual( + emptyPreparation, + ); + + expect(mocks.prepareExecUnavailable).toHaveBeenCalledWith("alpha"); expect(mocks.prepareLive).not.toHaveBeenCalled(); + expect(mocks.prepareAbsent).not.toHaveBeenCalled(); expect(relock).not.toHaveBeenCalled(); }); @@ -76,7 +99,7 @@ describe("forced rebuild MCP preparation", () => { it("fails closed when host-side recovery cannot prove durable ownership (#7062)", async () => { mocks.executeSandboxCommand.mockReturnValue({ status: 255, stdout: "", stderr: "relay EOF" }); - mocks.prepareAbsent.mockRejectedValue(new Error("provider ownership is ambiguous")); + mocks.prepareExecUnavailable.mockRejectedValue(new Error("provider ownership is ambiguous")); const relock = vi.fn(() => true); const bail = vi.fn((message: string): never => { throw new Error(message); @@ -87,6 +110,7 @@ describe("forced rebuild MCP preparation", () => { ); expect(mocks.prepareLive).not.toHaveBeenCalled(); + expect(mocks.prepareAbsent).not.toHaveBeenCalled(); expect(relock).toHaveBeenCalledWith(true); }); @@ -102,6 +126,7 @@ describe("forced rebuild MCP preparation", () => { expect(mocks.executeSandboxCommand).not.toHaveBeenCalled(); expect(mocks.prepareLive).toHaveBeenCalledWith("alpha"); + expect(mocks.prepareExecUnavailable).not.toHaveBeenCalled(); expect(mocks.prepareAbsent).not.toHaveBeenCalled(); }); @@ -117,6 +142,7 @@ describe("forced rebuild MCP preparation", () => { expect(mocks.executeSandboxCommand).not.toHaveBeenCalled(); expect(mocks.prepareAbsent).toHaveBeenCalledWith("alpha"); + expect(mocks.prepareExecUnavailable).not.toHaveBeenCalled(); expect(mocks.prepareLive).not.toHaveBeenCalled(); }); }); diff --git a/src/lib/actions/sandbox/rebuild-mcp-phase.ts b/src/lib/actions/sandbox/rebuild-mcp-phase.ts index c96e9eb8456..c7c0adf8314 100644 --- a/src/lib/actions/sandbox/rebuild-mcp-phase.ts +++ b/src/lib/actions/sandbox/rebuild-mcp-phase.ts @@ -9,6 +9,7 @@ import * as registry from "../../state/registry"; import type { ToolDisclosure } from "../../tool-disclosure"; import { prepareMcpBridgesForAbsentSandboxRebuild, + prepareMcpBridgesForExecUnavailableRebuild, prepareMcpBridgesForRebuild, reattachMcpProvidersAfterRebuildAbort, restoreMcpBridgesAfterRebuild, @@ -31,10 +32,22 @@ export async function prepareMcpForRebuild( relockShieldsIfNeeded: (sandboxStillExists: boolean) => boolean, bail: RebuildBail, ): Promise { + // invalidState: OpenShell still reports a live sandbox, but even the + // side-effect-free `:` command cannot cross its exec transport. Every + // nonzero result is non-authoritative, so interpreting selected exit codes + // as proof that in-sandbox MCP teardown can run would cross the delete edge. + // sourceBoundary: the pinned OpenShell sandbox-exec transport owns this + // liveness signal; NemoClaw owns only the explicit --force recovery policy. + // whyNotSourceFix: an unreachable retained image cannot be repaired before + // rebuild, and OpenShell v0.0.85 exposes no stronger adapter-health proof. + // regressionTest: rebuild-mcp-phase.test.ts exercises null and representative + // nonzero results through this exact force-only branch. + // removalCondition: remove this fallback only when OpenShell exposes an + // attested read-only adapter snapshot that is safe without sandbox exec. if (force && !staleRecovery && !canExecuteSandboxNoop(sandboxName)) { console.error(` ${YW}⚠${R} Sandbox exec probe failed; --force using host-side MCP recovery`); try { - return await prepareMcpBridgesForAbsentSandboxRebuild(sandboxName); + return await prepareMcpBridgesForExecUnavailableRebuild(sandboxName); } catch (error) { relockShieldsIfNeeded(true); bail( diff --git a/src/lib/actions/sandbox/rebuild-pipeline.ts b/src/lib/actions/sandbox/rebuild-pipeline.ts index 49e221816e4..0c7eb519564 100644 --- a/src/lib/actions/sandbox/rebuild-pipeline.ts +++ b/src/lib/actions/sandbox/rebuild-pipeline.ts @@ -137,6 +137,7 @@ async function rebuildSandboxUnlocked( relock: relockShieldsIfNeeded, } = shieldsPhase; let sandboxStillExists = true; + let sandboxExistenceAmbiguous = false; try { const preDeleteRecovery = revalidatePreparedRecoveryBeforeDelete( @@ -242,6 +243,9 @@ async function rebuildSandboxUnlocked( onDeleted: () => { sandboxStillExists = false; }, + onDeleteStateAmbiguous: () => { + sandboxExistenceAmbiguous = true; + }, }); if (!mcpPreparation) return; registryRollback.recordRemoval(mcpPreparation.removalReceipt); @@ -333,7 +337,9 @@ async function rebuildSandboxUnlocked( bail, }); } finally { - if (!rebuildShieldsWindow.relocked) relockShieldsIfNeeded(sandboxStillExists); + if (!rebuildShieldsWindow.relocked && !sandboxExistenceAmbiguous) { + relockShieldsIfNeeded(sandboxStillExists); + } } } finally { dcodePreflight.cleanup(); diff --git a/test/helpers/rebuild-flow-recovery-cases.ts b/test/helpers/rebuild-flow-recovery-cases.ts index 61d8dfb588f..0b0602a37aa 100644 --- a/test/helpers/rebuild-flow-recovery-cases.ts +++ b/test/helpers/rebuild-flow-recovery-cases.ts @@ -462,10 +462,16 @@ export function registerRebuildFlowRecoveryTests(): void { entries: [attached, alreadyDetached], detachedProviderEntries: [attached], }, - runOpenshell: (args) => - args.join(" ") === "sandbox delete alpha" - ? { status: 7, output: "delete failed", stderr: "delete failed" } - : { status: 0, output: "" }, + runOpenshell: (args) => { + const command = args.join(" "); + if (command === "sandbox delete alpha") { + return { status: 7, output: "delete failed", stderr: "delete failed" }; + } + if (command === "sandbox get -g nemoclaw alpha") { + return { status: 0, output: "Phase: Ready", stdout: "Phase: Ready", stderr: "" }; + } + return { status: 0, output: "" }; + }, }); await expect( diff --git a/test/mcp-destroy-lifecycle.test.ts b/test/mcp-destroy-lifecycle.test.ts index 9372ab20284..445135023a3 100644 --- a/test/mcp-destroy-lifecycle.test.ts +++ b/test/mcp-destroy-lifecycle.test.ts @@ -6,6 +6,7 @@ import path from "node:path"; import { afterAll, beforeEach, describe, expect, it, vi } from "vitest"; +import type { AgentMcpAdapter } from "../src/lib/agent/defs"; import type { McpBridgeEntry } from "../src/lib/state/registry"; const testState = vi.hoisted(() => { @@ -33,11 +34,16 @@ const testState = vi.hoisted(() => { home, originalEnv, policyApplyCalls: 0, - providers: new Map(), + providers: new Map(), + resolveHostAddresses: vi.fn(), attachedProviders: new Set(), recoverNamedGatewayRuntime: vi.fn(), removePreset: vi.fn(), + runOpenshell: vi.fn(), runOpenshellProviderCommand: vi.fn(), + stopNimContainer: vi.fn(), + stopNimContainerByName: vi.fn(), + warnUnpreservedUserManagedFiles: vi.fn(), }; }); @@ -45,6 +51,15 @@ vi.mock("../src/lib/actions/global", () => ({ runOpenshellProviderCommand: testState.runOpenshellProviderCommand, })); +vi.mock("../src/lib/adapters/dns/resolve", () => ({ + resolveHostAddresses: testState.resolveHostAddresses, +})); + +vi.mock("../src/lib/adapters/openshell/runtime", async (importOriginal) => ({ + ...(await importOriginal()), + runOpenshell: testState.runOpenshell, +})); + vi.mock("../src/lib/gateway-runtime-action", () => ({ recoverNamedGatewayRuntime: testState.recoverNamedGatewayRuntime, })); @@ -61,7 +76,19 @@ vi.mock("../src/lib/actions/sandbox/process-recovery", () => ({ executeSandboxExecCommand: testState.executeSandboxExecCommand, })); +vi.mock("../src/lib/actions/sandbox/rebuild-flow-helpers", async (importOriginal) => ({ + ...(await importOriginal()), + warnUnpreservedUserManagedFiles: testState.warnUnpreservedUserManagedFiles, +})); + +vi.mock("../src/lib/inference/nim", () => ({ + stopNimContainer: testState.stopNimContainer, + stopNimContainerByName: testState.stopNimContainerByName, +})); + import * as bridge from "../src/lib/actions/sandbox/mcp-bridge"; +import { isAgentMcpAdapter } from "../src/lib/actions/sandbox/mcp-bridge-contracts"; +import { runRebuildDestroyPhase } from "../src/lib/actions/sandbox/rebuild-destroy-phase"; import * as registry from "../src/lib/state/registry"; const MATCHING_OPENSHELL = path.resolve("test/fixtures/openshell-v0.0.85"); @@ -91,10 +118,23 @@ const bridgeEntries: Record<"github" | "slack", McpBridgeEntry> = { }, }; -function ownedPolicy(server: "github" | "slack") { +function ownedPolicy( + server: "github" | "slack", + options: { + adapter?: AgentMcpAdapter; + entry?: McpBridgeEntry; + resolvedAddresses?: readonly string[]; + } = {}, +) { + const entry = options.entry ?? bridgeEntries[server]; + const adapter = options.adapter ?? entry.adapter; + if (!isAgentMcpAdapter(adapter)) { + throw new Error("MCP policy fixture requires an explicit adapter"); + } + const resolvedAddresses = options.resolvedAddresses ?? [new URL(entry.url).hostname]; return { - name: `mcp-bridge-${server}`, - content: "network_policies: {}\n", + name: entry.policyName, + content: bridge.buildMcpBridgePolicyYaml(entry.server, entry.url, adapter, resolvedAddresses), sourcePath: "generated:nemoclaw-mcp-bridge", }; } @@ -158,6 +198,10 @@ beforeEach(() => { }); testState.getPresetContentGatewayState.mockReturnValue("match"); testState.removePreset.mockReturnValue(true); + testState.runOpenshell.mockReturnValue({ status: 0, stdout: "", stderr: "" }); + testState.resolveHostAddresses.mockImplementation(async (hostname: string) => [ + { address: hostname }, + ]); testState.runOpenshellProviderCommand.mockImplementation((args: string[]) => { testState.calls.push(args.join(" ")); @@ -171,7 +215,7 @@ beforeEach(() => { return provider ? { status: 0, - stdout: `Id: ${provider.id}\nType: generic\nResource version: 1\nCredential keys: ${provider.credential}\n`, + stdout: `Id: ${provider.id}\nType: generic\nResource version: ${provider.resourceVersion ?? 1}\nCredential keys: ${provider.credential}\n`, stderr: "", } : { status: 1, stdout: "", stderr: "Provider not found" }; @@ -367,6 +411,479 @@ describe("authenticated MCP sandbox destroy lifecycle", () => { expect([...testState.providers.keys()]).toContain("alpha-mcp-github"); }); + it("retains a providerless preflighted add when exec-unavailable recovery refuses it (#7062)", async () => { + testState.providers.delete("alpha-mcp-github"); + testState.attachedProviders.delete("alpha-mcp-github"); + const pending: McpBridgeEntry = { ...bridgeEntries.github, addState: "preflighted" }; + delete pending.providerId; + registry.registerSandbox({ + name: "alpha", + agent: "openclaw", + gatewayName: "nemoclaw", + mcp: { bridges: { github: pending } }, + }); + registry.addCustomPolicy("alpha", ownedPolicy("github")); + const before = registry.getSandbox("alpha"); + + const message = await captureMessage(() => + bridge.prepareMcpBridgesForExecUnavailableRebuild("alpha"), + ); + + expect(message).toMatch(/incomplete add transaction.*cannot discard or adopt/i); + expect(registry.getSandbox("alpha")).toEqual(before); + expect(testState.calls).toEqual([]); + expect(testState.adapterCalls).toEqual([]); + expect(testState.getPresetContentGatewayState).not.toHaveBeenCalled(); + expect(testState.recoverNamedGatewayRuntime).not.toHaveBeenCalled(); + expect(testState.applyPresetContent).not.toHaveBeenCalled(); + expect(testState.removePreset).not.toHaveBeenCalled(); + }); + + it("rejects a missing adapter before exec-unavailable recovery can mutate state (#7062)", async () => { + const missingAdapter: McpBridgeEntry = { ...bridgeEntries.github }; + delete missingAdapter.adapter; + registry.registerSandbox({ + name: "alpha", + agent: "openclaw", + gatewayName: "nemoclaw", + mcp: { bridges: { github: missingAdapter } }, + }); + registry.addCustomPolicy( + "alpha", + ownedPolicy("github", { adapter: "mcporter", entry: missingAdapter }), + ); + const before = registry.getSandbox("alpha"); + + const message = await captureMessage(() => + bridge.prepareMcpBridgesForExecUnavailableRebuild("alpha"), + ); + + expect(message).toMatch(/adapter identity is missing or incompatible/i); + expect(registry.getSandbox("alpha")).toEqual(before); + expect(testState.resolveHostAddresses).not.toHaveBeenCalled(); + expect(testState.calls).toEqual([]); + expect(testState.adapterCalls).toEqual([]); + expect(testState.getPresetContentGatewayState).not.toHaveBeenCalled(); + expect(testState.recoverNamedGatewayRuntime).not.toHaveBeenCalled(); + expect(testState.applyPresetContent).not.toHaveBeenCalled(); + expect(testState.removePreset).not.toHaveBeenCalled(); + }); + + it("rejects a cross-agent adapter before exec-unavailable recovery can mutate state (#7062)", async () => { + const crossAgentEntry: McpBridgeEntry = { + ...bridgeEntries.github, + agent: "hermes", + adapter: "hermes-config", + }; + registry.registerSandbox({ + name: "alpha", + agent: "openclaw", + gatewayName: "nemoclaw", + mcp: { bridges: { github: crossAgentEntry } }, + }); + registry.addCustomPolicy("alpha", ownedPolicy("github", { entry: crossAgentEntry })); + const before = registry.getSandbox("alpha"); + + const message = await captureMessage(() => + bridge.prepareMcpBridgesForExecUnavailableRebuild("alpha"), + ); + + expect(message).toMatch(/adapter identity is missing or incompatible/i); + expect(registry.getSandbox("alpha")).toEqual(before); + expect(testState.resolveHostAddresses).not.toHaveBeenCalled(); + expect(testState.calls).toEqual([]); + expect(testState.adapterCalls).toEqual([]); + expect(testState.getPresetContentGatewayState).not.toHaveBeenCalled(); + expect(testState.recoverNamedGatewayRuntime).not.toHaveBeenCalled(); + expect(testState.applyPresetContent).not.toHaveBeenCalled(); + expect(testState.removePreset).not.toHaveBeenCalled(); + }); + + it("rejects live policy drift during exec-unavailable recovery without MCP mutations (#7062)", async () => { + registry.registerSandbox({ + name: "alpha", + agent: "openclaw", + gatewayName: "nemoclaw", + mcp: { bridges: { github: bridgeEntries.github } }, + }); + registry.addCustomPolicy("alpha", ownedPolicy("github")); + const before = registry.getSandbox("alpha"); + testState.getPresetContentGatewayState.mockReturnValue("drift"); + + const message = await captureMessage(() => + bridge.prepareMcpBridgesForExecUnavailableRebuild("alpha"), + ); + + expect(message).toMatch(/policy.*drifted.*host-side rebuild recovery/i); + expect(registry.getSandbox("alpha")).toEqual(before); + expect(testState.calls).toEqual([]); + expect(testState.adapterCalls).toEqual([]); + expect(testState.applyPresetContent).not.toHaveBeenCalled(); + expect(testState.removePreset).not.toHaveBeenCalled(); + }); + + it("does not reconcile an incomplete policy registration during read-only recovery (#7062)", async () => { + registry.registerSandbox({ + name: "alpha", + agent: "openclaw", + gatewayName: "nemoclaw", + mcp: { bridges: { github: bridgeEntries.github } }, + }); + registry.addCustomPolicy("alpha", { + ...ownedPolicy("github"), + pendingContent: "network_policies: {}\n", + }); + const before = registry.getSandbox("alpha"); + + const message = await captureMessage(() => + bridge.prepareMcpBridgesForExecUnavailableRebuild("alpha"), + ); + + expect(message).toMatch(/incomplete registry transition.*read-only/i); + expect(registry.getSandbox("alpha")).toEqual(before); + expect(testState.calls).toEqual([]); + expect(testState.adapterCalls).toEqual([]); + expect(testState.applyPresetContent).not.toHaveBeenCalled(); + expect(testState.removePreset).not.toHaveBeenCalled(); + }); + + it("permits read-only host recovery only while complete MCP state stays exact (#7062)", async () => { + registry.registerSandbox({ + name: "alpha", + agent: "openclaw", + gatewayName: "nemoclaw", + mcp: { bridges: { github: bridgeEntries.github } }, + }); + registry.addCustomPolicy("alpha", ownedPolicy("github")); + const before = registry.getSandbox("alpha"); + + const preparation = await bridge.prepareMcpBridgesForExecUnavailableRebuild("alpha"); + await preparation.revalidateBeforeDelete?.(); + preparation.assertDeleteEdgeUnchanged?.(); + + expect(preparation.entries).toEqual([bridgeEntries.github]); + expect(preparation.detachedProviderEntries).toEqual([]); + expect(preparation.scrubbedAdapterEntries).toEqual([]); + expect(registry.getSandbox("alpha")).toEqual(before); + expect(testState.calls).toEqual([ + "provider get alpha-mcp-github", + "provider get alpha-mcp-github", + ]); + expect(testState.recoverNamedGatewayRuntime).toHaveBeenCalledTimes(2); + expect(testState.getPresetContentGatewayState).toHaveBeenCalledTimes(2); + expect(testState.adapterCalls).toEqual([]); + expect(testState.applyPresetContent).not.toHaveBeenCalled(); + expect(testState.removePreset).not.toHaveBeenCalled(); + }); + + it("fails the delete-edge proof when live MCP policy drifts after host preflight (#7062)", async () => { + registry.registerSandbox({ + name: "alpha", + agent: "openclaw", + gatewayName: "nemoclaw", + mcp: { bridges: { github: bridgeEntries.github } }, + }); + registry.addCustomPolicy("alpha", ownedPolicy("github")); + const preparation = await bridge.prepareMcpBridgesForExecUnavailableRebuild("alpha"); + const before = registry.getSandbox("alpha"); + testState.getPresetContentGatewayState.mockReturnValue("drift"); + + const message = await captureMessage(async () => preparation.revalidateBeforeDelete?.()); + + expect(message).toMatch(/policy.*drifted.*host-side rebuild recovery/i); + expect(registry.getSandbox("alpha")).toEqual(before); + expect(testState.adapterCalls).toEqual([]); + expect(testState.calls).toEqual(["provider get alpha-mcp-github"]); + }); + + it("fails the delete-edge proof when the exact provider identity changes (#7062)", async () => { + registry.registerSandbox({ + name: "alpha", + agent: "openclaw", + gatewayName: "nemoclaw", + mcp: { bridges: { github: bridgeEntries.github } }, + }); + registry.addCustomPolicy("alpha", ownedPolicy("github")); + const preparation = await bridge.prepareMcpBridgesForExecUnavailableRebuild("alpha"); + const before = registry.getSandbox("alpha"); + testState.providers.set("alpha-mcp-github", { + credential: "GITHUB_TOKEN", + id: "99999999-2222-4333-8444-555555555555", + }); + + const message = await captureMessage(async () => preparation.revalidateBeforeDelete?.()); + + expect(message).toMatch(/no longer exactly matches.*stable provider ID/i); + expect(registry.getSandbox("alpha")).toEqual(before); + expect(testState.adapterCalls).toEqual([]); + expect(testState.calls).toEqual([ + "provider get alpha-mcp-github", + "provider get alpha-mcp-github", + ]); + }); + + it("rejects valid provider resource-version drift through the exact snapshot comparator (#7062)", async () => { + registry.registerSandbox({ + name: "alpha", + agent: "openclaw", + gatewayName: "nemoclaw", + mcp: { bridges: { github: bridgeEntries.github } }, + }); + registry.addCustomPolicy("alpha", ownedPolicy("github")); + const preparation = await bridge.prepareMcpBridgesForExecUnavailableRebuild("alpha"); + const before = registry.getSandbox("alpha"); + testState.providers.set("alpha-mcp-github", { + credential: "GITHUB_TOKEN", + id: "11111111-2222-4333-8444-555555555555", + resourceVersion: 2, + }); + + const message = await captureMessage(async () => preparation.revalidateBeforeDelete?.()); + + expect(message).toMatch(/changed after host-side rebuild preflight/i); + expect(registry.getSandbox("alpha")).toEqual(before); + expect(testState.calls).toEqual([ + "provider get alpha-mcp-github", + "provider get alpha-mcp-github", + ]); + expect(testState.adapterCalls).toEqual([]); + expect(testState.applyPresetContent).not.toHaveBeenCalled(); + expect(testState.removePreset).not.toHaveBeenCalled(); + }); + + it("rejects valid-to-valid DNS drift from the canonical policy pins (#7062)", async () => { + const dnsEntry = { + ...bridgeEntries.github, + url: "https://mcp.example.com/github", + }; + testState.resolveHostAddresses + .mockResolvedValueOnce([{ address: "8.8.8.8" }]) + .mockResolvedValueOnce([{ address: "1.1.1.1" }]); + registry.registerSandbox({ + name: "alpha", + agent: "openclaw", + gatewayName: "nemoclaw", + mcp: { bridges: { github: dnsEntry } }, + }); + registry.addCustomPolicy( + "alpha", + ownedPolicy("github", { entry: dnsEntry, resolvedAddresses: ["8.8.8.8"] }), + ); + const preparation = await bridge.prepareMcpBridgesForExecUnavailableRebuild("alpha"); + const before = registry.getSandbox("alpha"); + + const message = await captureMessage(async () => preparation.revalidateBeforeDelete?.()); + + expect(message).toMatch(/not canonical for its recorded bridge definition/i); + expect(registry.getSandbox("alpha")).toEqual(before); + expect(testState.resolveHostAddresses).toHaveBeenNthCalledWith(1, "mcp.example.com"); + expect(testState.resolveHostAddresses).toHaveBeenNthCalledWith(2, "mcp.example.com"); + expect(testState.adapterCalls).toEqual([]); + }); + + it("rejects bridge-definition drift at the final no-await delete edge (#7062)", async () => { + registry.registerSandbox({ + name: "alpha", + agent: "openclaw", + gatewayName: "nemoclaw", + mcp: { bridges: { github: bridgeEntries.github } }, + }); + registry.addCustomPolicy("alpha", ownedPolicy("github")); + const preparation = await bridge.prepareMcpBridgesForExecUnavailableRebuild("alpha"); + const before = registry.getSandbox("alpha"); + registry.updateSandbox("alpha", { + mcp: { + ...before?.mcp, + bridges: { + github: { ...bridgeEntries.github, url: "https://1.1.1.1/github" }, + }, + }, + }); + + expect(() => preparation.assertDeleteEdgeUnchanged?.()).toThrow( + /MCP bridge definitions changed/i, + ); + expect(testState.adapterCalls).toEqual([]); + }); + + it("rejects a new destroy marker at the final no-await delete edge (#7062)", async () => { + registry.registerSandbox({ + name: "alpha", + agent: "openclaw", + gatewayName: "nemoclaw", + mcp: { bridges: { github: bridgeEntries.github } }, + }); + registry.addCustomPolicy("alpha", ownedPolicy("github")); + const preparation = await bridge.prepareMcpBridgesForExecUnavailableRebuild("alpha"); + const before = registry.getSandbox("alpha"); + registry.updateSandbox("alpha", { + mcp: { + ...before?.mcp, + bridges: { github: bridgeEntries.github }, + destroyPreparedAt: "2026-07-19T00:00:00.000Z", + }, + }); + + expect(() => preparation.assertDeleteEdgeUnchanged?.()).toThrow( + /incomplete MCP destroy transaction/i, + ); + expect(registry.getSandbox("alpha")?.mcp?.destroyPreparedAt).toBe("2026-07-19T00:00:00.000Z"); + expect(testState.adapterCalls).toEqual([]); + }); + + it("rejects recorded-gateway drift at the final no-await delete edge (#7062)", async () => { + registry.registerSandbox({ + name: "alpha", + agent: "openclaw", + gatewayName: "nemoclaw", + mcp: { bridges: { github: bridgeEntries.github } }, + }); + registry.addCustomPolicy("alpha", ownedPolicy("github")); + const preparation = await bridge.prepareMcpBridgesForExecUnavailableRebuild("alpha"); + registry.updateSandbox("alpha", { gatewayPort: 19080 }); + + expect(() => preparation.assertDeleteEdgeUnchanged?.()).toThrow( + /changed its recorded gateway/i, + ); + expect(registry.getSandbox("alpha")?.gatewayPort).toBe(19080); + expect(testState.adapterCalls).toEqual([]); + }); + + it("rejects recorded-agent adapter drift at the final no-await delete edge (#7062)", async () => { + registry.registerSandbox({ + name: "alpha", + agent: "openclaw", + gatewayName: "nemoclaw", + mcp: { bridges: { github: bridgeEntries.github } }, + }); + registry.addCustomPolicy("alpha", ownedPolicy("github")); + const preparation = await bridge.prepareMcpBridgesForExecUnavailableRebuild("alpha"); + registry.updateSandbox("alpha", { agent: "hermes" }); + + expect(() => preparation.assertDeleteEdgeUnchanged?.()).toThrow( + /changed its recorded agent or MCP adapter/i, + ); + expect(registry.getSandbox("alpha")?.agent).toBe("hermes"); + expect(testState.adapterCalls).toEqual([]); + expect(testState.applyPresetContent).not.toHaveBeenCalled(); + expect(testState.removePreset).not.toHaveBeenCalled(); + }); + + it("runs failed exec through real read-only preparation and deletes before stopping NIM (#7062)", async () => { + registry.registerSandbox({ + name: "alpha", + agent: "openclaw", + gatewayName: "nemoclaw", + nimContainer: "nim-alpha", + mcp: { bridges: { github: bridgeEntries.github } }, + }); + registry.addCustomPolicy("alpha", ownedPolicy("github")); + const before = registry.getSandbox("alpha"); + testState.executeSandboxCommand.mockImplementation((_sandbox: string, command: string) => { + testState.adapterCalls.push(command); + return null; + }); + const onDeleted = vi.fn(); + + const result = await runRebuildDestroyPhase({ + sandboxName: "alpha", + sandboxEntry: before ?? { name: "alpha", agent: "openclaw" }, + staleRecovery: false, + backupManifest: null, + force: true, + log: vi.fn(), + bail: vi.fn((message: string): never => { + throw new Error(message); + }), + relockShieldsIfNeeded: vi.fn(() => true), + onDeleted, + }); + + expect(result?.entries).toEqual([bridgeEntries.github]); + expect(testState.executeSandboxCommand).toHaveBeenCalledOnce(); + expect(testState.executeSandboxCommand).toHaveBeenCalledWith("alpha", ":"); + expect(testState.runOpenshell).toHaveBeenCalledWith( + ["sandbox", "delete", "alpha"], + expect.any(Object), + ); + expect(testState.stopNimContainer).not.toHaveBeenCalled(); + expect(testState.stopNimContainerByName).toHaveBeenCalledWith("nim-alpha"); + expect(testState.runOpenshellProviderCommand).toHaveBeenCalledTimes(2); + expect(testState.getPresetContentGatewayState).toHaveBeenCalledTimes(2); + expect(testState.recoverNamedGatewayRuntime).toHaveBeenCalledTimes(2); + expect(testState.executeSandboxCommand.mock.invocationCallOrder[0]).toBeLessThan( + testState.runOpenshellProviderCommand.mock.invocationCallOrder[0] ?? Number.POSITIVE_INFINITY, + ); + expect(testState.runOpenshellProviderCommand.mock.invocationCallOrder[1]).toBeLessThan( + testState.runOpenshell.mock.invocationCallOrder[0] ?? Number.POSITIVE_INFINITY, + ); + expect(testState.runOpenshell.mock.invocationCallOrder[0]).toBeLessThan( + testState.stopNimContainerByName.mock.invocationCallOrder[0] ?? Number.POSITIVE_INFINITY, + ); + expect(registry.getSandbox("alpha")).toEqual(before); + expect([...testState.providers.keys()]).toContain("alpha-mcp-github"); + expect([...testState.attachedProviders]).toContain("alpha-mcp-github"); + expect(testState.adapterRegistered).toBe(true); + expect(testState.adapterCalls).toEqual([":"]); + expect(testState.applyPresetContent).not.toHaveBeenCalled(); + expect(testState.removePreset).not.toHaveBeenCalled(); + expect(onDeleted).toHaveBeenCalledOnce(); + }); + + it("preserves real MCP ownership and running NIM when sandbox deletion fails (#7062)", async () => { + registry.registerSandbox({ + name: "alpha", + agent: "openclaw", + gatewayName: "nemoclaw", + nimContainer: "nim-alpha", + mcp: { bridges: { github: bridgeEntries.github } }, + }); + registry.addCustomPolicy("alpha", ownedPolicy("github")); + const beforeRegistry = registry.getSandbox("alpha"); + const beforeProviders = [...testState.providers.entries()]; + const beforeAttachments = [...testState.attachedProviders]; + const beforeAdapterRegistered = testState.adapterRegistered; + testState.executeSandboxCommand.mockImplementation((_sandbox: string, command: string) => { + testState.adapterCalls.push(command); + return null; + }); + testState.runOpenshell + .mockReturnValueOnce({ + status: 9, + stdout: "", + stderr: "delete failed", + }) + .mockReturnValueOnce({ status: 0, stdout: "Phase: Ready\n", stderr: "" }); + const onDeleted = vi.fn(); + + await expect( + runRebuildDestroyPhase({ + sandboxName: "alpha", + sandboxEntry: beforeRegistry ?? { name: "alpha", agent: "openclaw" }, + staleRecovery: false, + backupManifest: null, + force: true, + log: vi.fn(), + bail: vi.fn((message: string): never => { + throw new Error(message); + }), + relockShieldsIfNeeded: vi.fn(() => true), + onDeleted, + }), + ).rejects.toThrow("Failed to delete sandbox."); + + expect(registry.getSandbox("alpha")).toEqual(beforeRegistry); + expect([...testState.providers.entries()]).toEqual(beforeProviders); + expect([...testState.attachedProviders]).toEqual(beforeAttachments); + expect(testState.adapterRegistered).toBe(beforeAdapterRegistered); + expect(testState.adapterCalls).toEqual([":"]); + expect(testState.applyPresetContent).not.toHaveBeenCalled(); + expect(testState.removePreset).not.toHaveBeenCalled(); + expect(testState.stopNimContainer).not.toHaveBeenCalled(); + expect(testState.stopNimContainerByName).not.toHaveBeenCalled(); + expect(onDeleted).not.toHaveBeenCalled(); + }); + it("rejects policy drift before prepareMcpBridgesForRebuild mutates adapter or provider state", async () => { registry.registerSandbox({ name: "alpha", From dedfcce40739d6dcc1291c0178f212216fdae0d7 Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Sun, 19 Jul 2026 15:16:09 -0700 Subject: [PATCH 03/18] docs(rebuild): document host-side MCP recovery Co-authored-by: kagura-agent Signed-off-by: Apurv Kumaria --- docs/manage-sandboxes/recover-rebuild-sandboxes.mdx | 11 +++++++++-- docs/reference/commands.mdx | 10 ++++++++-- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/docs/manage-sandboxes/recover-rebuild-sandboxes.mdx b/docs/manage-sandboxes/recover-rebuild-sandboxes.mdx index e251096c808..05dcf93cb66 100644 --- a/docs/manage-sandboxes/recover-rebuild-sandboxes.mdx +++ b/docs/manage-sandboxes/recover-rebuild-sandboxes.mdx @@ -134,8 +134,15 @@ If every state directory fails, NemoClaw stops before deleting the original sand `rebuild --force` can continue when no state directory was preserved. NemoClaw restores any loose files captured in the partial backup; if nothing usable was captured, it recreates the sandbox from recorded registry metadata without restoring prior sandbox state. Use this recovery path only when losing the state that could not be backed up is acceptable. -When a sandbox with managed MCP servers cannot run a pre-mutation no-op, explicit `--force` uses the exact host-side registry and provider identities to preserve MCP intent without scrubbing the unreachable in-sandbox adapter. -This recovery remains fail-closed for ambiguous ownership, target, policy-registration, or provider state; an error after a successful no-op does not fall back to the host-side path. +When a sandbox with managed MCP servers cannot run a pre-mutation no-op, explicit `--force` uses its complete registry entries plus the exact live generated policies and provider identities to preserve MCP intent without scrubbing the unreachable in-sandbox adapter. +Every bridge entry must record an explicit adapter that matches the sandbox's recorded agent, and the registered policy must be the canonical generated policy for that adapter, server name, URL endpoint, and current resolved-address pins. +NemoClaw rechecks that read-only snapshot immediately before deletion and stops if the target, registry, policy, provider, or recorded gateway changed. +Across every rebuild path, NemoClaw does not attempt to stop the local NIM through the delete attempt, and cleanup is attempted on a best-effort basis only after deletion is positively confirmed. +After a nonzero delete, NemoClaw queries the sandbox name on its exact recorded gateway: an explicit missing result converges as deleted, while a `Ready` or `Running` result triggers an attempt to restore prepared MCP state and any shields lockdown that rebuild temporarily opened. +That MCP and shields restoration can fail and is reported rather than presented as a successful rollback. +Any partial or unreachable result remains ambiguous: NemoClaw preserves the MCP ownership and rebuild-recovery records, does not attempt to stop NIM, skips the rebuild process's immediate shields relock, and does not claim that the original sandbox is intact. +Inspect the live sandbox and gateway state before retrying recovery. +This recovery also stops for incomplete MCP adds or ambiguous ownership; an error after a successful no-op does not fall back to the host-side path. When rebuild starts with shields up, NemoClaw opens a 30-minute shields-down window for backup and recreation. diff --git a/docs/reference/commands.mdx b/docs/reference/commands.mdx index 91e0f91bfec..357335f9e59 100644 --- a/docs/reference/commands.mdx +++ b/docs/reference/commands.mdx @@ -2500,8 +2500,14 @@ With `--force`, NemoClaw preserves any captured loose files in the partial manif If the backup produced nothing usable, it continues from recorded registry metadata without restoring prior sandbox state. Use this recovery path only when losing the state that could not be backed up is acceptable. For a sandbox with managed MCP servers, `--force` probes sandbox execution before MCP teardown. -If that no-op cannot run, NemoClaw uses the durable registry and host-side provider identities to preserve MCP intent without trying an in-sandbox adapter scrub. -Ownership, target, or provider validation failures still stop before sandbox deletion, and failures after a successful exec probe do not switch to the host-side path. +If that no-op cannot run, NemoClaw requires complete bridge entries and exact live policy and provider identities, without trying an in-sandbox adapter scrub or changing MCP ownership state. +Each bridge must carry an explicit adapter matching the sandbox's recorded agent, and the registered policy must equal the canonical generated policy for that adapter, server name, URL endpoint, and current resolved-address pins. +It rechecks the registry, recorded gateway, resolved targets, live generated policies, and provider identities immediately before deletion; incomplete adds, drift, or ambiguous ownership stop before deletion. +Across every rebuild path, NemoClaw does not attempt to stop local NIM until sandbox deletion is positively confirmed, then attempts NIM cleanup on a best-effort basis. +When `openshell sandbox delete` exits nonzero, an exact owner-gateway lookup distinguishes explicit absence from a confirmed `Ready` or `Running` sandbox; any other phase or probe failure is ambiguous. +Explicit absence continues the rebuild, confirmed intact state triggers an attempt to restore prepared MCP state and any shields lockdown that rebuild temporarily opened, and any restoration failure is reported. +Ambiguous state preserves MCP ownership and recovery metadata without attempting to stop NIM or claiming the original sandbox remains intact, and the rebuild process skips its immediate shields relock. +Failures after a successful exec probe do not switch to the host-side path. Before backup or deletion, `rebuild` also refuses an incomplete MCP destroy transaction. For a prepared-only transaction, the redacted diagnostic points to `$$nemoclaw mcp remove --force` when the sandbox is still live. For a pending or both-marker transaction, it points to `$$nemoclaw destroy` because the registry records that OpenShell deletion was already confirmed. From f1c564361756fdbcaea7571f3293056612403709 Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Sun, 19 Jul 2026 15:26:00 -0700 Subject: [PATCH 04/18] test(rebuild): keep MCP policy fixture branchless Co-authored-by: kagura-agent Signed-off-by: Apurv Kumaria --- test/mcp-destroy-lifecycle.test.ts | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/test/mcp-destroy-lifecycle.test.ts b/test/mcp-destroy-lifecycle.test.ts index 445135023a3..abc4d41c1f8 100644 --- a/test/mcp-destroy-lifecycle.test.ts +++ b/test/mcp-destroy-lifecycle.test.ts @@ -128,13 +128,16 @@ function ownedPolicy( ) { const entry = options.entry ?? bridgeEntries[server]; const adapter = options.adapter ?? entry.adapter; - if (!isAgentMcpAdapter(adapter)) { - throw new Error("MCP policy fixture requires an explicit adapter"); - } + expect(isAgentMcpAdapter(adapter), "MCP policy fixture requires an explicit adapter").toBe(true); const resolvedAddresses = options.resolvedAddresses ?? [new URL(entry.url).hostname]; return { name: entry.policyName, - content: bridge.buildMcpBridgePolicyYaml(entry.server, entry.url, adapter, resolvedAddresses), + content: bridge.buildMcpBridgePolicyYaml( + entry.server, + entry.url, + adapter as AgentMcpAdapter, + resolvedAddresses, + ), sourcePath: "generated:nemoclaw-mcp-bridge", }; } From fb28ee9fac31fd7c34df9a8d8fdf59e2e0e72e3f Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Sun, 19 Jul 2026 19:40:57 -0700 Subject: [PATCH 05/18] fix(rebuild): require explicit sandbox absence Co-authored-by: kagura-agent Signed-off-by: Apurv Kumaria --- src/lib/actions/sandbox/gateway-state.ts | 25 ++++ .../sandbox/rebuild-destroy-phase.test.ts | 123 ++++++++++++++++++ .../actions/sandbox/rebuild-destroy-phase.ts | 6 +- 3 files changed, 151 insertions(+), 3 deletions(-) diff --git a/src/lib/actions/sandbox/gateway-state.ts b/src/lib/actions/sandbox/gateway-state.ts index f1ef8799cc9..5de51b5095f 100644 --- a/src/lib/actions/sandbox/gateway-state.ts +++ b/src/lib/actions/sandbox/gateway-state.ts @@ -99,6 +99,31 @@ export function isMissingSandboxGatewayOutput(output = ""): boolean { ); } +/** + * Strict absence classifier for destructive owner-gateway reconciliation. + * Bare NotFound is not sufficient because OpenShell uses it for missing + * gateways and providers as well as sandboxes. + */ +export function isExplicitMissingSandboxGatewayOutput( + output: string, + sandboxName: string, +): boolean { + const clean = stripAnsi(String(output)).replace(/\r/g, "").trim(); + const exactNoSpec = + /^(?:error:\s*)?status:\s*Internal,\s*message:\s*["']sandbox has no spec["'](?:,\s*details:\s*\[\])?(?:,\s*metadata:\s*MetadataMap\s*\{\s*\})?$/i; + if (exactNoSpec.test(clean)) return true; + + const escapedName = sandboxName.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); + const namedSandbox = `(?:['\"]${escapedName}['\"]|${escapedName})`; + return ( + new RegExp( + `^(?:error:\\s*)?sandbox\\s+${namedSandbox}\\s+(?:(?:is\\s+)?not\\s+(?:found|present)|does\\s+not\\s+exist)[.!]?$`, + "i", + ).test(clean) || + new RegExp(`^(?:error:\\s*)?no\\s+such\\s+sandbox\\s+${namedSandbox}[.!]?$`, "i").test(clean) + ); +} + function formatGatewaySchemaMismatchOutput( issue: OpenShellStateRpcIssue, action: string, diff --git a/src/lib/actions/sandbox/rebuild-destroy-phase.test.ts b/src/lib/actions/sandbox/rebuild-destroy-phase.test.ts index 5995a1d08d9..f89b1f3234d 100644 --- a/src/lib/actions/sandbox/rebuild-destroy-phase.test.ts +++ b/src/lib/actions/sandbox/rebuild-destroy-phase.test.ts @@ -13,6 +13,7 @@ const mocks = vi.hoisted(() => ({ stdout: string; stderr: string; error?: NodeJS.ErrnoException; + signal?: NodeJS.Signals | null; } => ({ status: 0, stdout: "", stderr: "" }), ), getSandbox: vi.fn( @@ -261,6 +262,128 @@ describe("rebuild destroy validation diagnostics", () => { expect(relockShieldsIfNeeded).not.toHaveBeenCalled(); }); + it.each([ + [ + "bare NotFound output", + { + status: 1, + stdout: "", + stderr: "NotFound", + }, + ], + [ + "generic sandbox NotFound output", + { + status: 1, + stdout: "", + stderr: 'status: NotFound, message: "sandbox not found"', + }, + ], + [ + "gateway NotFound output", + { + status: 1, + stdout: "", + stderr: 'status: NotFound, message: "gateway nemoclaw not found"', + }, + ], + [ + "provider NotFound output", + { + status: 1, + stdout: "", + stderr: 'status: NotFound, message: "provider alpha-mcp-github not found"', + }, + ], + [ + "signal-terminated sandbox absence output", + { + status: null, + signal: "SIGTERM", + stdout: "", + stderr: 'status: Internal, message: "sandbox has no spec"', + }, + ], + [ + "null-status sandbox absence output", + { + status: null, + signal: null, + stdout: "", + stderr: 'status: Internal, message: "sandbox has no spec"', + }, + ], + [ + "mixed gateway and sandbox absence output", + { + status: 1, + stdout: "", + stderr: + 'status: NotFound, message: "gateway nemoclaw not found"\nstatus: Internal, message: "sandbox has no spec"', + }, + ], + [ + "absence output for a different sandbox", + { + status: 1, + stdout: "", + stderr: "sandbox beta not found", + }, + ], + ] satisfies ReadonlyArray< + readonly [ + string, + { + status: number | null; + signal?: NodeJS.Signals | null; + stdout: string; + stderr: string; + }, + ] + >)("does not treat %s as proof of sandbox deletion (#7062)", async (_label, probe) => { + mocks.getSandbox.mockReturnValueOnce({ + name: "alpha", + agent: "openclaw", + nimContainer: "nim-alpha", + }); + mocks.prepareMcpForRebuild.mockResolvedValue({ + entries: [{ server: "github" }], + detachedProviderEntries: [{ server: "github" }], + scrubbedAdapterEntries: [], + }); + mocks.runOpenshell + .mockReturnValueOnce({ status: 9, stdout: "", stderr: "delete interrupted" }) + .mockReturnValueOnce(probe); + const onDeleted = vi.fn(); + const onDeleteStateAmbiguous = vi.fn(); + const relockShieldsIfNeeded = vi.fn(() => true); + + await expect( + runRebuildDestroyPhase({ + sandboxName: "alpha", + sandboxEntry: { name: "alpha", agent: "openclaw", gatewayName: "nemoclaw" }, + staleRecovery: false, + backupManifest: null, + force: true, + log: vi.fn(), + bail: vi.fn((message: string): never => { + throw new Error(message); + }), + relockShieldsIfNeeded, + onDeleted, + onDeleteStateAmbiguous, + }), + ).rejects.toThrow(/exact post-delete state is ambiguous.*recovery state was preserved/i); + + expect(onDeleted).not.toHaveBeenCalled(); + expect(onDeleteStateAmbiguous).toHaveBeenCalledOnce(); + expect(mocks.removeSandboxRegistryEntryWithReceipt).not.toHaveBeenCalled(); + expect(mocks.stopNimContainer).not.toHaveBeenCalled(); + expect(mocks.stopNimContainerByName).not.toHaveBeenCalled(); + expect(mocks.reattachMcpAfterDeleteFailure).not.toHaveBeenCalled(); + expect(relockShieldsIfNeeded).not.toHaveBeenCalled(); + }); + it("preserves recovery ownership when post-delete state is partial or ambiguous (#7062)", async () => { mocks.getSandbox.mockReturnValueOnce({ name: "alpha", diff --git a/src/lib/actions/sandbox/rebuild-destroy-phase.ts b/src/lib/actions/sandbox/rebuild-destroy-phase.ts index a76ffc5c105..90be56b4e70 100644 --- a/src/lib/actions/sandbox/rebuild-destroy-phase.ts +++ b/src/lib/actions/sandbox/rebuild-destroy-phase.ts @@ -11,7 +11,7 @@ import { redactFull } from "../../security/redact"; import { parseSandboxPhase } from "../../state/gateway"; import * as registry from "../../state/registry"; import { removeSandboxRegistryEntryWithReceipt } from "./destroy"; -import { isMissingSandboxGatewayOutput } from "./gateway-state"; +import { isExplicitMissingSandboxGatewayOutput } from "./gateway-state"; import type { RebuildBackupManifest } from "./rebuild-backup-phase"; import type { RebuildBail, RebuildLog } from "./rebuild-credential-preflight"; import { type RebuildSandboxEntry, warnUnpreservedUserManagedFiles } from "./rebuild-flow-helpers"; @@ -79,12 +79,12 @@ function reconcileFailedSandboxDelete( log(`Post-delete reconciliation could not query recorded gateway '${gatewayName}'.`); return { state: "ambiguous", phase: null, status: null }; } - if (probe.error) { + if (probe.error || probe.signal || probe.status === null) { log(`Post-delete reconciliation could not complete on recorded gateway '${gatewayName}'.`); return { state: "ambiguous", phase: null, status: probe.status }; } const probeOutput = `${probe.stdout || ""}\n${probe.stderr || ""}`; - if (probe.status !== 0 && isMissingSandboxGatewayOutput(probeOutput)) { + if (probe.status !== 0 && isExplicitMissingSandboxGatewayOutput(probeOutput, sandboxName)) { log(`Post-delete reconciliation on '${gatewayName}': sandbox is absent.`); return { state: "deleted", phase: null, status: probe.status }; } From 98c23d9926a2f70dd4241ecefec286df43d6d8f6 Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Sun, 19 Jul 2026 19:41:01 -0700 Subject: [PATCH 06/18] fix(rebuild): reject ambiguous MCP ownership Co-authored-by: kagura-agent Signed-off-by: Apurv Kumaria --- .../mcp-bridge-rebuild-exec-unavailable.ts | 33 ++++++++- test/mcp-destroy-lifecycle.test.ts | 68 +++++++++++++++++++ 2 files changed, 100 insertions(+), 1 deletion(-) diff --git a/src/lib/actions/sandbox/mcp-bridge-rebuild-exec-unavailable.ts b/src/lib/actions/sandbox/mcp-bridge-rebuild-exec-unavailable.ts index 5d28f84d46b..9cf64a2b83c 100644 --- a/src/lib/actions/sandbox/mcp-bridge-rebuild-exec-unavailable.ts +++ b/src/lib/actions/sandbox/mcp-bridge-rebuild-exec-unavailable.ts @@ -20,7 +20,7 @@ import { getSandboxAgent, getSandboxOrThrow, } from "./mcp-bridge-state"; -import { validateSandboxName } from "./mcp-bridge-validation"; +import { assertAuthenticatedBridgeEntry, validateSandboxName } from "./mcp-bridge-validation"; type ReadOnlyValidationSnapshot = { policyByServer: Map; @@ -30,6 +30,11 @@ type ReadOnlyValidationSnapshot = { type ExplicitAdapterMcpBridgeEntry = McpBridgeEntry & { adapter: AgentMcpAdapter }; +type McpOwnershipField = { + label: string; + value: (entry: McpBridgeEntry) => string | undefined; +}; + export interface ExecUnavailableMcpRebuildPreparation { entries: McpBridgeEntry[]; detachedProviderEntries: McpBridgeEntry[]; @@ -38,6 +43,31 @@ export interface ExecUnavailableMcpRebuildPreparation { assertDeleteEdgeUnchanged: () => void; } +function assertUniqueMcpOwnership(entries: readonly McpBridgeEntry[]): void { + for (const entry of entries) assertAuthenticatedBridgeEntry(entry); + + const ownershipFields: readonly McpOwnershipField[] = [ + { label: "credential key", value: (entry) => entry.env[0] }, + { label: "provider name", value: (entry) => entry.providerName }, + { label: "provider ID", value: (entry) => entry.providerId }, + { label: "generated policy name", value: (entry) => entry.policyName }, + ]; + for (const field of ownershipFields) { + const ownerByValue = new Map(); + for (const entry of entries) { + const value = field.value(entry); + if (!value) continue; + if (ownerByValue.has(value)) { + const priorOwner = ownerByValue.get(value) ?? ""; + throw new McpBridgeError( + `MCP servers '${priorOwner}' and '${entry.server}' reuse the same ${field.label} '${value}'. Refusing read-only host-side rebuild recovery.`, + ); + } + ownerByValue.set(value, entry.server); + } + } +} + function snapshotCompleteEntries(sandboxName: string): { entries: ExplicitAdapterMcpBridgeEntry[]; gatewayName: string; @@ -64,6 +94,7 @@ function snapshotCompleteEntries(sandboxName: string): { "Managed MCP adapter identity is missing or incompatible with the sandbox's recorded agent. Refusing read-only host-side rebuild recovery.", ); } + assertUniqueMcpOwnership(entries); return { entries: entries.map((entry) => ({ ...entry, adapter })), gatewayName: resolveSandboxGatewayName(sandbox), diff --git a/test/mcp-destroy-lifecycle.test.ts b/test/mcp-destroy-lifecycle.test.ts index abc4d41c1f8..8d00574304d 100644 --- a/test/mcp-destroy-lifecycle.test.ts +++ b/test/mcp-destroy-lifecycle.test.ts @@ -502,6 +502,74 @@ describe("authenticated MCP sandbox destroy lifecycle", () => { expect(testState.removePreset).not.toHaveBeenCalled(); }); + it.each([ + [ + "credential key", + /reuse the same credential key/i, + (entry: McpBridgeEntry): McpBridgeEntry => ({ + ...entry, + env: [...bridgeEntries.github.env], + }), + ], + [ + "provider name", + /reuse the same provider name/i, + (entry: McpBridgeEntry): McpBridgeEntry => ({ + ...entry, + providerName: bridgeEntries.github.providerName, + }), + ], + [ + "provider ID", + /reuse the same provider ID/i, + (entry: McpBridgeEntry): McpBridgeEntry => ({ + ...entry, + providerId: bridgeEntries.github.providerId, + }), + ], + [ + "generated policy name", + /reuse the same generated policy name/i, + (entry: McpBridgeEntry): McpBridgeEntry => ({ + ...entry, + policyName: bridgeEntries.github.policyName, + }), + ], + ] satisfies ReadonlyArray< + readonly [string, RegExp, (entry: McpBridgeEntry) => McpBridgeEntry] + >)("rejects a cross-entry %s collision before exec-unavailable recovery can inspect or mutate state (#7062)", async (_label, expected, collide) => { + const collidingSlack = collide(bridgeEntries.slack); + registry.registerSandbox({ + name: "alpha", + agent: "openclaw", + gatewayName: "nemoclaw", + mcp: { + bridges: { + github: bridgeEntries.github, + slack: collidingSlack, + }, + }, + }); + registry.addCustomPolicy("alpha", ownedPolicy("github")); + registry.addCustomPolicy("alpha", ownedPolicy("slack", { entry: collidingSlack })); + const before = registry.getSandbox("alpha"); + + const message = await captureMessage(() => + bridge.prepareMcpBridgesForExecUnavailableRebuild("alpha"), + ); + + expect(message).toMatch(expected); + expect(registry.getSandbox("alpha")).toEqual(before); + expect(testState.resolveHostAddresses).not.toHaveBeenCalled(); + expect(testState.calls).toEqual([]); + expect(testState.adapterCalls).toEqual([]); + expect(testState.runOpenshell).not.toHaveBeenCalled(); + expect(testState.getPresetContentGatewayState).not.toHaveBeenCalled(); + expect(testState.recoverNamedGatewayRuntime).not.toHaveBeenCalled(); + expect(testState.applyPresetContent).not.toHaveBeenCalled(); + expect(testState.removePreset).not.toHaveBeenCalled(); + }); + it("rejects live policy drift during exec-unavailable recovery without MCP mutations (#7062)", async () => { registry.registerSandbox({ name: "alpha", From 21695d1ac8e9ea9f91142a3e96aa286f999bc576 Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Sun, 19 Jul 2026 19:41:05 -0700 Subject: [PATCH 07/18] refactor(rebuild): canonicalize MCP preparation Co-authored-by: kagura-agent Signed-off-by: Apurv Kumaria --- src/lib/actions/sandbox/mcp-bridge.ts | 20 +++----------------- 1 file changed, 3 insertions(+), 17 deletions(-) diff --git a/src/lib/actions/sandbox/mcp-bridge.ts b/src/lib/actions/sandbox/mcp-bridge.ts index 047048dfc04..8cf40611100 100644 --- a/src/lib/actions/sandbox/mcp-bridge.ts +++ b/src/lib/actions/sandbox/mcp-bridge.ts @@ -16,8 +16,8 @@ import { } from "./mcp-bridge-destroy"; import { redactBridgeSecretsForDisplay } from "./mcp-bridge-output"; import { + type McpRebuildPreparation, prepareMcpBridgesForAbsentSandboxRebuild as prepareMcpBridgesForAbsentSandboxRebuildLifecycle, - prepareMcpBridgesForExecUnavailableRebuild as prepareMcpBridgesForExecUnavailableRebuildLifecycle, prepareMcpBridgesForRebuild as prepareMcpBridgesForRebuildLifecycle, reattachMcpProvidersAfterRebuildAbort as reattachMcpProvidersAfterRebuildAbortLifecycle, restoreMcpBridgesAfterRebuild as restoreMcpBridgesAfterRebuildLifecycle, @@ -70,6 +70,7 @@ export { parseMcpProviderMetadata, providerDetachChangedState, } from "./mcp-bridge-provider"; +export { prepareMcpBridgesForExecUnavailableRebuild } from "./mcp-bridge-rebuild"; export { buildMcpBridgeProviderName, MCP_SERVER_URL_MAX_LENGTH, @@ -79,6 +80,7 @@ export { validateMcpCredentialEnvName, validateMcpServerName, } from "./mcp-bridge-validation"; +export type { McpRebuildPreparation }; export { statusMcpBridge }; export interface McpDestroyPreparation { @@ -91,16 +93,6 @@ export interface McpDestroyPreparation { destroyAlreadyPending: boolean; } -export interface McpRebuildPreparation { - entries: McpBridgeEntry[]; - detachedProviderEntries: McpBridgeEntry[]; - scrubbedAdapterEntries: McpBridgeEntry[]; - /** Full read-only target, policy, provider, and registry proof before delete. */ - revalidateBeforeDelete?: () => Promise; - /** Final synchronous registry-only proof immediately before delete. */ - assertDeleteEdgeUnchanged?: () => void; -} - export async function addMcpBridge( sandboxName: string, options: McpBridgeAddOptions, @@ -154,12 +146,6 @@ export async function prepareMcpBridgesForAbsentSandboxRebuild( return prepareMcpBridgesForAbsentSandboxRebuildLifecycle(sandboxName); } -export async function prepareMcpBridgesForExecUnavailableRebuild( - sandboxName: string, -): Promise { - return prepareMcpBridgesForExecUnavailableRebuildLifecycle(sandboxName); -} - export async function prepareMcpBridgesForRebuild( sandboxName: string, ): Promise { From 5293739bef2c8a7b567461db8baed5a4c94e94fe Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Sun, 19 Jul 2026 20:21:07 -0700 Subject: [PATCH 08/18] fix(rebuild): probe both MCP teardown transports Co-authored-by: kagura-agent Signed-off-by: Apurv Kumaria --- .../mcp-bridge-rebuild-exec-unavailable.ts | 9 ++- .../actions/sandbox/rebuild-destroy-phase.ts | 4 ++ .../actions/sandbox/rebuild-mcp-phase.test.ts | 70 ++++++++++++++++++- src/lib/actions/sandbox/rebuild-mcp-phase.ts | 33 +++++---- test/mcp-destroy-lifecycle.test.ts | 18 +++-- 5 files changed, 106 insertions(+), 28 deletions(-) diff --git a/src/lib/actions/sandbox/mcp-bridge-rebuild-exec-unavailable.ts b/src/lib/actions/sandbox/mcp-bridge-rebuild-exec-unavailable.ts index 9cf64a2b83c..bf2c4b88411 100644 --- a/src/lib/actions/sandbox/mcp-bridge-rebuild-exec-unavailable.ts +++ b/src/lib/actions/sandbox/mcp-bridge-rebuild-exec-unavailable.ts @@ -138,6 +138,9 @@ async function inspectReadOnlyRecoveryState( adapter: AgentMcpAdapter, ): Promise { const resolvedTargets = await preflightMcpEntryTargets(entries); + // This may start or recover the sandbox's recorded host gateway and select + // it in CLI context. It does not mutate MCP ownership or sandbox contents; + // the provider, policy, and target checks below remain inspection-only. if (entries.length > 0) await ensureSandboxGatewaySelected(sandboxName); const policyByServer = new Map(); @@ -229,8 +232,10 @@ async function revalidateBeforeDelete( /** * Preserve complete MCP intent when sandbox exec is unavailable but OpenShell * still reports the sandbox live. Unlike absent-sandbox recovery, this path is - * read-only: it never discards add markers, scrubs adapters, detaches providers, - * reconciles policy records, or otherwise mutates MCP ownership before delete. + * read-only with respect to MCP ownership and sandbox contents: it may recover + * and select the recorded host gateway for inspection, but it never discards + * add markers, scrubs adapters, detaches providers, reconciles policy records, + * or otherwise mutates MCP ownership before delete. */ export async function prepareMcpBridgesForExecUnavailableRebuild( sandboxName: string, diff --git a/src/lib/actions/sandbox/rebuild-destroy-phase.ts b/src/lib/actions/sandbox/rebuild-destroy-phase.ts index 90be56b4e70..8844f245b5f 100644 --- a/src/lib/actions/sandbox/rebuild-destroy-phase.ts +++ b/src/lib/actions/sandbox/rebuild-destroy-phase.ts @@ -202,6 +202,10 @@ export async function runRebuildDestroyPhase( // preparation. Re-prove target, policy, provider, and registry state while // the original sandbox and local NIM are still intact. Then run one final // synchronous registry check at the no-await edge immediately before delete. + // External control-plane state can still change after the awaited proof; the + // final synchronous check covers registry state only and minimizes that + // window. Durable MCP intent remains preserved, and restoration rechecks the + // external state and fails closed if later control-plane drift is observed. if (mcpPreparation.revalidateBeforeDelete || mcpPreparation.assertDeleteEdgeUnchanged) { try { await mcpPreparation.revalidateBeforeDelete?.(); diff --git a/src/lib/actions/sandbox/rebuild-mcp-phase.test.ts b/src/lib/actions/sandbox/rebuild-mcp-phase.test.ts index adbc3a2bf76..5ec7cf3ef24 100644 --- a/src/lib/actions/sandbox/rebuild-mcp-phase.test.ts +++ b/src/lib/actions/sandbox/rebuild-mcp-phase.test.ts @@ -5,6 +5,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; const mocks = vi.hoisted(() => ({ executeSandboxCommand: vi.fn(), + executeSandboxExecCommand: vi.fn(), prepareAbsent: vi.fn(), prepareExecUnavailable: vi.fn(), prepareLive: vi.fn(), @@ -20,6 +21,7 @@ vi.mock("./mcp-bridge", () => ({ vi.mock("./process-recovery", () => ({ executeSandboxCommand: mocks.executeSandboxCommand, + executeSandboxExecCommand: mocks.executeSandboxExecCommand, })); import { prepareMcpForRebuild, printMcpRebuildRetryCommand } from "./rebuild-mcp-phase"; @@ -39,12 +41,34 @@ describe("forced rebuild MCP preparation", () => { vi.clearAllMocks(); vi.spyOn(console, "error").mockImplementation(() => undefined); mocks.executeSandboxCommand.mockReturnValue({ status: 0, stdout: "", stderr: "" }); + mocks.executeSandboxExecCommand.mockReturnValue({ status: 0, stdout: "", stderr: "" }); mocks.prepareAbsent.mockResolvedValue(emptyPreparation); mocks.prepareExecUnavailable.mockResolvedValue(emptyPreparation); mocks.prepareLive.mockResolvedValue(emptyPreparation); }); - it("uses host-side recovery when the pre-mutation exec probe cannot run (#7062)", async () => { + it("uses host-side recovery when OpenShell exec fails even while SSH is healthy (#7062)", async () => { + mocks.executeSandboxExecCommand.mockReturnValue(null); + const relock = vi.fn(() => true); + const bail = vi.fn((message: string): never => { + throw new Error(message); + }); + + await expect(prepareMcpForRebuild("alpha", false, true, relock, bail)).resolves.toEqual( + emptyPreparation, + ); + + expect(mocks.executeSandboxExecCommand).toHaveBeenCalledWith("alpha", ":", undefined, { + allowLocalDockerFallback: false, + }); + expect(mocks.executeSandboxCommand).toHaveBeenCalledWith("alpha", ":"); + expect(mocks.prepareExecUnavailable).toHaveBeenCalledWith("alpha"); + expect(mocks.prepareAbsent).not.toHaveBeenCalled(); + expect(mocks.prepareLive).not.toHaveBeenCalled(); + expect(relock).not.toHaveBeenCalled(); + }); + + it("uses host-side recovery when SSH fails even while OpenShell exec is healthy (#7062)", async () => { mocks.executeSandboxCommand.mockReturnValue(null); const relock = vi.fn(() => true); const bail = vi.fn((message: string): never => { @@ -56,8 +80,34 @@ describe("forced rebuild MCP preparation", () => { ); expect(mocks.executeSandboxCommand).toHaveBeenCalledWith("alpha", ":"); + expect(mocks.executeSandboxExecCommand).toHaveBeenCalledWith("alpha", ":", undefined, { + allowLocalDockerFallback: false, + }); expect(mocks.prepareExecUnavailable).toHaveBeenCalledWith("alpha"); + expect(mocks.prepareLive).not.toHaveBeenCalled(); expect(mocks.prepareAbsent).not.toHaveBeenCalled(); + expect(relock).not.toHaveBeenCalled(); + }); + + it("uses host-side recovery when SSH exits nonzero even while OpenShell exec is healthy (#7062)", async () => { + mocks.executeSandboxCommand.mockReturnValue({ + status: 255, + stdout: "", + stderr: "relay EOF", + }); + const relock = vi.fn(() => true); + const bail = vi.fn((message: string): never => { + throw new Error(message); + }); + + await expect(prepareMcpForRebuild("alpha", false, true, relock, bail)).resolves.toEqual( + emptyPreparation, + ); + + expect(mocks.executeSandboxExecCommand).toHaveBeenCalledWith("alpha", ":", undefined, { + allowLocalDockerFallback: false, + }); + expect(mocks.prepareExecUnavailable).toHaveBeenCalledWith("alpha"); expect(mocks.prepareLive).not.toHaveBeenCalled(); expect(relock).not.toHaveBeenCalled(); }); @@ -65,7 +115,11 @@ describe("forced rebuild MCP preparation", () => { it.each([ 1, 64, 126, 127, 255, ])("routes every nonzero exec result (%i) through explicit force recovery (#7062)", async (status) => { - mocks.executeSandboxCommand.mockReturnValue({ status, stdout: "", stderr: "exec failed" }); + mocks.executeSandboxExecCommand.mockReturnValue({ + status, + stdout: "", + stderr: "exec failed", + }); const relock = vi.fn(() => true); const bail = vi.fn((message: string): never => { throw new Error(message); @@ -93,12 +147,20 @@ describe("forced rebuild MCP preparation", () => { ); expect(mocks.prepareLive).toHaveBeenCalledWith("alpha"); + expect(mocks.executeSandboxCommand).toHaveBeenCalledWith("alpha", ":"); + expect(mocks.executeSandboxExecCommand).toHaveBeenCalledWith("alpha", ":", undefined, { + allowLocalDockerFallback: false, + }); expect(mocks.prepareAbsent).not.toHaveBeenCalled(); expect(relock).toHaveBeenCalledWith(true); }); it("fails closed when host-side recovery cannot prove durable ownership (#7062)", async () => { - mocks.executeSandboxCommand.mockReturnValue({ status: 255, stdout: "", stderr: "relay EOF" }); + mocks.executeSandboxExecCommand.mockReturnValue({ + status: 255, + stdout: "", + stderr: "relay EOF", + }); mocks.prepareExecUnavailable.mockRejectedValue(new Error("provider ownership is ambiguous")); const relock = vi.fn(() => true); const bail = vi.fn((message: string): never => { @@ -124,6 +186,7 @@ describe("forced rebuild MCP preparation", () => { emptyPreparation, ); + expect(mocks.executeSandboxExecCommand).not.toHaveBeenCalled(); expect(mocks.executeSandboxCommand).not.toHaveBeenCalled(); expect(mocks.prepareLive).toHaveBeenCalledWith("alpha"); expect(mocks.prepareExecUnavailable).not.toHaveBeenCalled(); @@ -140,6 +203,7 @@ describe("forced rebuild MCP preparation", () => { emptyPreparation, ); + expect(mocks.executeSandboxExecCommand).not.toHaveBeenCalled(); expect(mocks.executeSandboxCommand).not.toHaveBeenCalled(); expect(mocks.prepareAbsent).toHaveBeenCalledWith("alpha"); expect(mocks.prepareExecUnavailable).not.toHaveBeenCalled(); diff --git a/src/lib/actions/sandbox/rebuild-mcp-phase.ts b/src/lib/actions/sandbox/rebuild-mcp-phase.ts index c7c0adf8314..7cb1ed1497b 100644 --- a/src/lib/actions/sandbox/rebuild-mcp-phase.ts +++ b/src/lib/actions/sandbox/rebuild-mcp-phase.ts @@ -14,15 +14,22 @@ import { reattachMcpProvidersAfterRebuildAbort, restoreMcpBridgesAfterRebuild, } from "./mcp-bridge"; -import { executeSandboxCommand } from "./process-recovery"; +import { executeSandboxCommand, executeSandboxExecCommand } from "./process-recovery"; import type { RebuildBail } from "./rebuild-credential-preflight"; import type { RebuildSandboxEntry } from "./rebuild-flow-helpers"; export type McpRebuildPreparation = Awaited>; -function canExecuteSandboxNoop(sandboxName: string): boolean { - const probe = executeSandboxCommand(sandboxName, ":"); - return probe !== null && probe.status === 0; +function canExecuteMcpPreparation(sandboxName: string): boolean { + // Live MCP preparation uses both transports: SSH-backed adapter + // inspection/mutation and OpenShell-mediated adapter/provider operations. + // Prove both before any mutation. A direct Docker fallback would not prove + // that the OpenShell transport itself can run. + const sshProbe = executeSandboxCommand(sandboxName, ":"); + const execProbe = executeSandboxExecCommand(sandboxName, ":", undefined, { + allowLocalDockerFallback: false, + }); + return sshProbe !== null && sshProbe.status === 0 && execProbe !== null && execProbe.status === 0; } export async function prepareMcpForRebuild( @@ -32,20 +39,20 @@ export async function prepareMcpForRebuild( relockShieldsIfNeeded: (sandboxStillExists: boolean) => boolean, bail: RebuildBail, ): Promise { - // invalidState: OpenShell still reports a live sandbox, but even the - // side-effect-free `:` command cannot cross its exec transport. Every - // nonzero result is non-authoritative, so interpreting selected exit codes - // as proof that in-sandbox MCP teardown can run would cross the delete edge. - // sourceBoundary: the pinned OpenShell sandbox-exec transport owns this - // liveness signal; NemoClaw owns only the explicit --force recovery policy. + // invalidState: OpenShell still reports a live sandbox, but the + // side-effect-free `:` command cannot cross every transport required by live + // MCP preparation. Every nonzero result is non-authoritative, so interpreting + // selected exit codes as proof teardown can run would cross the delete edge. + // sourceBoundary: the pinned OpenShell sandbox-exec and SSH transports own + // these liveness signals; NemoClaw owns only explicit --force recovery policy. // whyNotSourceFix: an unreachable retained image cannot be repaired before // rebuild, and OpenShell v0.0.85 exposes no stronger adapter-health proof. // regressionTest: rebuild-mcp-phase.test.ts exercises null and representative // nonzero results through this exact force-only branch. // removalCondition: remove this fallback only when OpenShell exposes an - // attested read-only adapter snapshot that is safe without sandbox exec. - if (force && !staleRecovery && !canExecuteSandboxNoop(sandboxName)) { - console.error(` ${YW}⚠${R} Sandbox exec probe failed; --force using host-side MCP recovery`); + // attested read-only adapter snapshot that is safe without sandbox transport. + if (force && !staleRecovery && !canExecuteMcpPreparation(sandboxName)) { + console.error(` ${YW}⚠${R} MCP transport probe failed; --force using host-side MCP recovery`); try { return await prepareMcpBridgesForExecUnavailableRebuild(sandboxName); } catch (error) { diff --git a/test/mcp-destroy-lifecycle.test.ts b/test/mcp-destroy-lifecycle.test.ts index 8d00574304d..bbdea7a8447 100644 --- a/test/mcp-destroy-lifecycle.test.ts +++ b/test/mcp-destroy-lifecycle.test.ts @@ -296,6 +296,7 @@ beforeEach(() => { }); testState.executeSandboxExecCommand.mockImplementation((_sandbox: string, command: string) => { + if (command === ":") return { status: 0, stdout: "", stderr: "" }; const encoded = command.match(/printf '%s' '([A-Za-z0-9+/=]+)' \| base64 -d/)?.[1] ?? ""; const proof = encoded ? Buffer.from(encoded, "base64").toString("utf8") : command; const isRevisionObservation = proof.includes("printf '%s\\n' absent"); @@ -850,10 +851,7 @@ describe("authenticated MCP sandbox destroy lifecycle", () => { }); registry.addCustomPolicy("alpha", ownedPolicy("github")); const before = registry.getSandbox("alpha"); - testState.executeSandboxCommand.mockImplementation((_sandbox: string, command: string) => { - testState.adapterCalls.push(command); - return null; - }); + testState.executeSandboxExecCommand.mockReturnValue(null); const onDeleted = vi.fn(); const result = await runRebuildDestroyPhase({ @@ -871,7 +869,10 @@ describe("authenticated MCP sandbox destroy lifecycle", () => { }); expect(result?.entries).toEqual([bridgeEntries.github]); - expect(testState.executeSandboxCommand).toHaveBeenCalledOnce(); + expect(testState.executeSandboxExecCommand).toHaveBeenCalledOnce(); + expect(testState.executeSandboxExecCommand).toHaveBeenCalledWith("alpha", ":", undefined, { + allowLocalDockerFallback: false, + }); expect(testState.executeSandboxCommand).toHaveBeenCalledWith("alpha", ":"); expect(testState.runOpenshell).toHaveBeenCalledWith( ["sandbox", "delete", "alpha"], @@ -882,7 +883,7 @@ describe("authenticated MCP sandbox destroy lifecycle", () => { expect(testState.runOpenshellProviderCommand).toHaveBeenCalledTimes(2); expect(testState.getPresetContentGatewayState).toHaveBeenCalledTimes(2); expect(testState.recoverNamedGatewayRuntime).toHaveBeenCalledTimes(2); - expect(testState.executeSandboxCommand.mock.invocationCallOrder[0]).toBeLessThan( + expect(testState.executeSandboxExecCommand.mock.invocationCallOrder[0]).toBeLessThan( testState.runOpenshellProviderCommand.mock.invocationCallOrder[0] ?? Number.POSITIVE_INFINITY, ); expect(testState.runOpenshellProviderCommand.mock.invocationCallOrder[1]).toBeLessThan( @@ -914,10 +915,7 @@ describe("authenticated MCP sandbox destroy lifecycle", () => { const beforeProviders = [...testState.providers.entries()]; const beforeAttachments = [...testState.attachedProviders]; const beforeAdapterRegistered = testState.adapterRegistered; - testState.executeSandboxCommand.mockImplementation((_sandbox: string, command: string) => { - testState.adapterCalls.push(command); - return null; - }); + testState.executeSandboxExecCommand.mockReturnValue(null); testState.runOpenshell .mockReturnValueOnce({ status: 9, From 6340766bc0fa09a3e5b6f8ac663cd0c7665143fc Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Sun, 19 Jul 2026 20:28:08 -0700 Subject: [PATCH 09/18] test(rebuild): keep transport mock linear Co-authored-by: kagura-agent Signed-off-by: Apurv Kumaria --- test/mcp-destroy-lifecycle.test.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/mcp-destroy-lifecycle.test.ts b/test/mcp-destroy-lifecycle.test.ts index bbdea7a8447..c7cfb8609c7 100644 --- a/test/mcp-destroy-lifecycle.test.ts +++ b/test/mcp-destroy-lifecycle.test.ts @@ -296,7 +296,7 @@ beforeEach(() => { }); testState.executeSandboxExecCommand.mockImplementation((_sandbox: string, command: string) => { - if (command === ":") return { status: 0, stdout: "", stderr: "" }; + const isNoopProbe = command === ":"; const encoded = command.match(/printf '%s' '([A-Za-z0-9+/=]+)' \| base64 -d/)?.[1] ?? ""; const proof = encoded ? Buffer.from(encoded, "base64").toString("utf8") : command; const isRevisionObservation = proof.includes("printf '%s\\n' absent"); @@ -312,6 +312,7 @@ beforeEach(() => { ); return { status: + isNoopProbe || proof.includes("allow_all_known_mcp_methods") || proof.includes('[ -z "${') || proof.includes("openshell:resolve:env:GITHUB_TOKEN") || From b83a56b64f8b877028e75ee961d24ea00abdef44 Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Sun, 19 Jul 2026 20:42:49 -0700 Subject: [PATCH 10/18] fix(rebuild): pin sandbox delete gateway Co-authored-by: kagura-agent Signed-off-by: Apurv Kumaria --- .../sandbox/rebuild-destroy-phase.test.ts | 29 +++++++++++++++++++ .../actions/sandbox/rebuild-destroy-phase.ts | 5 ++-- 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/src/lib/actions/sandbox/rebuild-destroy-phase.test.ts b/src/lib/actions/sandbox/rebuild-destroy-phase.test.ts index f89b1f3234d..40fd4772ab1 100644 --- a/src/lib/actions/sandbox/rebuild-destroy-phase.test.ts +++ b/src/lib/actions/sandbox/rebuild-destroy-phase.test.ts @@ -70,6 +70,7 @@ describe("rebuild destroy validation diagnostics", () => { }); afterEach(() => { + vi.unstubAllEnvs(); vi.restoreAllMocks(); }); @@ -133,6 +134,34 @@ describe("rebuild destroy validation diagnostics", () => { ); }); + it("pins deletion to the recorded gateway when ambient selection changes (#7062)", async () => { + vi.stubEnv("OPENSHELL_GATEWAY", "nemoclaw-29080"); + + await runRebuildDestroyPhase({ + sandboxName: "alpha", + sandboxEntry: { + name: "alpha", + agent: "openclaw", + gatewayName: "nemoclaw-19080", + gatewayPort: 19080, + }, + staleRecovery: false, + backupManifest: null, + force: true, + log: vi.fn(), + bail: vi.fn((message: string): never => { + throw new Error(message); + }), + relockShieldsIfNeeded: vi.fn(() => true), + onDeleted: vi.fn(), + }); + + expect(mocks.runOpenshell).toHaveBeenCalledWith( + ["sandbox", "delete", "-g", "nemoclaw-19080", "alpha"], + expect.objectContaining({ ignoreError: true }), + ); + }); + it("refuses sandbox deletion when read-only MCP state drifts at the delete edge (#7062)", async () => { const revalidateBeforeDelete = vi.fn().mockRejectedValue(new Error("live policy drifted")); mocks.prepareMcpForRebuild.mockResolvedValue({ diff --git a/src/lib/actions/sandbox/rebuild-destroy-phase.ts b/src/lib/actions/sandbox/rebuild-destroy-phase.ts index 8844f245b5f..ac14baf2663 100644 --- a/src/lib/actions/sandbox/rebuild-destroy-phase.ts +++ b/src/lib/actions/sandbox/rebuild-destroy-phase.ts @@ -117,6 +117,7 @@ export async function runRebuildDestroyPhase( validateAfterMcpPreparation, onDeleted, } = input; + const gatewayName = resolveSandboxGatewayName(input.sandboxEntry); // Step 3: Delete sandbox without tearing down gateway or session. // sandboxDestroy() cleans up the gateway when it's the last sandbox and @@ -220,8 +221,8 @@ export async function runRebuildDestroyPhase( } } - log(`Running: openshell sandbox delete ${sandboxName}`); - const deleteResult = runOpenshell(["sandbox", "delete", sandboxName], { + log(`Running: openshell sandbox delete -g ${gatewayName} ${sandboxName}`); + const deleteResult = runOpenshell(["sandbox", "delete", "-g", gatewayName, sandboxName], { ignoreError: true, stdio: ["ignore", "pipe", "pipe"], }); From b951d1f6ade1fec8222bd051e56537e3d789d2c3 Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Sun, 19 Jul 2026 21:30:42 -0700 Subject: [PATCH 11/18] test(rebuild): align gateway-pinned delete assertions Co-authored-by: kagura-agent Signed-off-by: Apurv Kumaria --- .../rebuild-dcode-artifact-drift.test.ts | 16 +++------ .../rebuild-dcode-mutation-edge.test.ts | 8 ++--- .../rebuild-dcode-pre-delete-drift.test.ts | 11 ++---- .../sandbox/rebuild-dcode-recovery.test.ts | 2 +- .../rebuild-local-provider-recreate.test.ts | 2 +- .../sandbox/rebuild-prepared-recovery.test.ts | 35 +++++-------------- test/helpers/rebuild-dcode-flow-helpers.ts | 6 ++-- test/helpers/rebuild-dcode-flow-support.ts | 6 ++-- test/helpers/rebuild-delete-assertions.ts | 11 ++++++ ...rebuild-flow-credential-preflight-cases.ts | 16 +++------ test/helpers/rebuild-flow-lifecycle-cases.ts | 12 +++---- test/helpers/rebuild-flow-recovery-cases.ts | 32 +++++------------ .../rebuild-flow-target-credentials-cases.ts | 11 ++---- .../rebuild-flow-target-image-cases.ts | 15 +++----- .../rebuild-flow-target-session-cases.ts | 6 ++-- test/mcp-destroy-lifecycle.test.ts | 2 +- test/rebuild-stale-recovery.test.ts | 11 ++---- 17 files changed, 68 insertions(+), 134 deletions(-) create mode 100644 test/helpers/rebuild-delete-assertions.ts diff --git a/src/lib/actions/sandbox/rebuild-dcode-artifact-drift.test.ts b/src/lib/actions/sandbox/rebuild-dcode-artifact-drift.test.ts index e172220aaf6..5802553222c 100644 --- a/src/lib/actions/sandbox/rebuild-dcode-artifact-drift.test.ts +++ b/src/lib/actions/sandbox/rebuild-dcode-artifact-drift.test.ts @@ -6,6 +6,7 @@ import { configureDcodeSession, makeDcodeSandboxEntry, } from "../../../../test/helpers/rebuild-dcode-flow-support"; +import { expectNoSandboxDelete } from "../../../../test/helpers/rebuild-delete-assertions"; import { createRebuildFlowHarness, resetRebuildFlowTestEnvironment, @@ -32,10 +33,7 @@ describe("rebuildSandbox DCode flow: prepared artifact drift", () => { expect(harness.openShieldsSpy).toHaveBeenCalledOnce(); expect(harness.backupSandboxStateSpy).toHaveBeenCalledOnce(); - expect(harness.runOpenshellSpy).not.toHaveBeenCalledWith( - ["sandbox", "delete", "alpha"], - expect.anything(), - ); + expectNoSandboxDelete(harness.runOpenshellSpy); expect(harness.removeSandboxRegistryEntrySpy).not.toHaveBeenCalled(); expect(harness.onboardSpy).not.toHaveBeenCalled(); expect(harness.relockSpy).toHaveBeenCalledWith("alpha", expect.any(Object), true, "nemoclaw"); @@ -58,10 +56,7 @@ describe("rebuildSandbox DCode flow: prepared artifact drift", () => { expect(harness.openShieldsSpy).toHaveBeenCalledOnce(); expect(harness.backupSandboxStateSpy).toHaveBeenCalledOnce(); - expect(harness.runOpenshellSpy).not.toHaveBeenCalledWith( - ["sandbox", "delete", "alpha"], - expect.anything(), - ); + expectNoSandboxDelete(harness.runOpenshellSpy); expect(harness.removeSandboxRegistryEntrySpy).not.toHaveBeenCalled(); expect(harness.onboardSpy).not.toHaveBeenCalled(); expect(harness.relockSpy).toHaveBeenCalledWith("alpha", expect.any(Object), true, "nemoclaw"); @@ -95,10 +90,7 @@ describe("rebuildSandbox DCode flow: prepared artifact drift", () => { expect(harness.prepareManagedDcodeRebuildImageSpy).toHaveBeenCalledOnce(); expect(harness.openShieldsSpy).toHaveBeenCalledOnce(); expect(harness.backupSandboxStateSpy).not.toHaveBeenCalled(); - expect(harness.runOpenshellSpy).not.toHaveBeenCalledWith( - ["sandbox", "delete", "alpha"], - expect.anything(), - ); + expectNoSandboxDelete(harness.runOpenshellSpy); expect(harness.onboardSpy).not.toHaveBeenCalled(); expect(harness.disposePreparedDcodeRebuildImageSpy).toHaveBeenCalledWith( harness.preparedDcodeBuildContext, diff --git a/src/lib/actions/sandbox/rebuild-dcode-mutation-edge.test.ts b/src/lib/actions/sandbox/rebuild-dcode-mutation-edge.test.ts index 3c60d85cfc1..5926f651f9a 100644 --- a/src/lib/actions/sandbox/rebuild-dcode-mutation-edge.test.ts +++ b/src/lib/actions/sandbox/rebuild-dcode-mutation-edge.test.ts @@ -6,6 +6,7 @@ import { configureDcodeSession, makeDcodeSandboxEntry, } from "../../../../test/helpers/rebuild-dcode-flow-support"; +import { expectNoSandboxDelete } from "../../../../test/helpers/rebuild-delete-assertions"; import { createRebuildFlowHarness, resetRebuildFlowTestEnvironment, @@ -73,7 +74,7 @@ describe("rebuildSandbox DCode flow: mutation edge", () => { const warningProbeOrder = harness.warnUnpreservedUserManagedFilesSpy.mock.invocationCallOrder[0]; const deleteCall = harness.runOpenshellSpy.mock.calls.findIndex( - ([args]) => Array.isArray(args) && args.join(" ") === "sandbox delete alpha", + ([args]) => Array.isArray(args) && args.join(" ") === "sandbox delete -g nemoclaw alpha", ); const deleteOrder = harness.runOpenshellSpy.mock.invocationCallOrder[deleteCall]; const onboardOrder = harness.onboardSpy.mock.invocationCallOrder[0]; @@ -119,10 +120,7 @@ describe("rebuildSandbox DCode flow: mutation edge", () => { [detached], [scrubbed], ); - expect(harness.runOpenshellSpy).not.toHaveBeenCalledWith( - ["sandbox", "delete", "alpha"], - expect.anything(), - ); + expectNoSandboxDelete(harness.runOpenshellSpy); expect(harness.onboardSpy).not.toHaveBeenCalled(); expect(harness.relockSpy).toHaveBeenCalledWith("alpha", expect.any(Object), true, "nemoclaw"); }); diff --git a/src/lib/actions/sandbox/rebuild-dcode-pre-delete-drift.test.ts b/src/lib/actions/sandbox/rebuild-dcode-pre-delete-drift.test.ts index 91affb1c1a5..4d3b841bfea 100644 --- a/src/lib/actions/sandbox/rebuild-dcode-pre-delete-drift.test.ts +++ b/src/lib/actions/sandbox/rebuild-dcode-pre-delete-drift.test.ts @@ -7,6 +7,7 @@ import { expectNoDcodeMutation, makeDcodeSandboxEntry, } from "../../../../test/helpers/rebuild-dcode-flow-support"; +import { expectNoSandboxDelete } from "../../../../test/helpers/rebuild-delete-assertions"; import { createRebuildFlowHarness, resetRebuildFlowTestEnvironment, @@ -194,10 +195,7 @@ describe("rebuildSandbox DCode flow: pre-delete drift", () => { expect(harness.openShieldsSpy).toHaveBeenCalledOnce(); expect(harness.backupSandboxStateSpy).toHaveBeenCalledOnce(); - expect(harness.runOpenshellSpy).not.toHaveBeenCalledWith( - ["sandbox", "delete", "alpha"], - expect.anything(), - ); + expectNoSandboxDelete(harness.runOpenshellSpy); expect(harness.removeSandboxRegistryEntrySpy).not.toHaveBeenCalled(); expect(harness.onboardSpy).not.toHaveBeenCalled(); expect(harness.relockSpy).toHaveBeenCalledWith("alpha", expect.any(Object), true, "nemoclaw"); @@ -224,10 +222,7 @@ describe("rebuildSandbox DCode flow: pre-delete drift", () => { expect(harness.preflightDcodeRouteSpy).toHaveBeenCalledTimes(3); expect(harness.openShieldsSpy).toHaveBeenCalledOnce(); expect(harness.backupSandboxStateSpy).toHaveBeenCalledOnce(); - expect(harness.runOpenshellSpy).not.toHaveBeenCalledWith( - ["sandbox", "delete", "alpha"], - expect.anything(), - ); + expectNoSandboxDelete(harness.runOpenshellSpy); expect(harness.removeSandboxRegistryEntrySpy).not.toHaveBeenCalled(); expect(harness.onboardSpy).not.toHaveBeenCalled(); expect(harness.relockSpy).toHaveBeenCalledWith("alpha", expect.any(Object), true, "nemoclaw"); diff --git a/src/lib/actions/sandbox/rebuild-dcode-recovery.test.ts b/src/lib/actions/sandbox/rebuild-dcode-recovery.test.ts index 76fd49f550f..dac36d5609c 100644 --- a/src/lib/actions/sandbox/rebuild-dcode-recovery.test.ts +++ b/src/lib/actions/sandbox/rebuild-dcode-recovery.test.ts @@ -43,7 +43,7 @@ describe("rebuildSandbox DCode flow: recovery", () => { expect(harness.prepareManagedDcodeRebuildImageSpy).toHaveBeenCalledOnce(); expect(harness.backupSandboxStateSpy).not.toHaveBeenCalled(); expect(harness.runOpenshellSpy).toHaveBeenCalledWith( - ["sandbox", "delete", "alpha"], + ["sandbox", "delete", "-g", "nemoclaw", "alpha"], expect.objectContaining({ ignoreError: true }), ); expect(harness.onboardSpy).toHaveBeenCalledOnce(); diff --git a/src/lib/actions/sandbox/rebuild-local-provider-recreate.test.ts b/src/lib/actions/sandbox/rebuild-local-provider-recreate.test.ts index e7439fe3e36..bdcb9017548 100644 --- a/src/lib/actions/sandbox/rebuild-local-provider-recreate.test.ts +++ b/src/lib/actions/sandbox/rebuild-local-provider-recreate.test.ts @@ -175,7 +175,7 @@ describe("rebuild local-provider recreation", () => { const calls = harness.runOpenshellSpy.mock.calls.map((call) => call[0] as string[]); const deleteCall = calls.findIndex( - (args) => args[0] === "sandbox" && args[1] === "delete" && args[2] === "alpha", + (args) => args.join(" ") === "sandbox delete -g nemoclaw alpha", ); const providerLookup = calls.findIndex( (args) => args[0] === "provider" && args[1] === "get" && args[2] === provider, diff --git a/src/lib/actions/sandbox/rebuild-prepared-recovery.test.ts b/src/lib/actions/sandbox/rebuild-prepared-recovery.test.ts index 1cdd09251c9..ff683736b44 100644 --- a/src/lib/actions/sandbox/rebuild-prepared-recovery.test.ts +++ b/src/lib/actions/sandbox/rebuild-prepared-recovery.test.ts @@ -2,6 +2,7 @@ // SPDX-License-Identifier: Apache-2.0 import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { expectNoSandboxDelete } from "../../../../test/helpers/rebuild-delete-assertions"; import { createRebuildFlowHarness, makePreparedRecoveryManifest, @@ -44,7 +45,7 @@ describe("prepared rebuild recovery", () => { expect.objectContaining({ deferInferenceRouteUntilOnboard: true }), ); expect(harness.runOpenshellSpy).toHaveBeenCalledWith( - ["sandbox", "delete", "alpha"], + ["sandbox", "delete", "-g", "nemoclaw", "alpha"], expect.objectContaining({ ignoreError: true }), ); expect(harness.restoreSandboxStateSpy).toHaveBeenCalledWith( @@ -85,7 +86,7 @@ describe("prepared rebuild recovery", () => { expect(harness.backupSandboxStateSpy).not.toHaveBeenCalled(); expect(harness.runOpenshellSpy).toHaveBeenCalledWith( - ["sandbox", "delete", "alpha"], + ["sandbox", "delete", "-g", "nemoclaw", "alpha"], expect.objectContaining({ ignoreError: true }), ); expect(harness.restoreSandboxStateSpy).toHaveBeenCalledWith( @@ -109,10 +110,7 @@ describe("prepared rebuild recovery", () => { }), ).rejects.toThrow("no NemoClaw-managed image fingerprint"); - expect(harness.runOpenshellSpy).not.toHaveBeenCalledWith( - ["sandbox", "delete", "alpha"], - expect.anything(), - ); + expectNoSandboxDelete(harness.runOpenshellSpy); expect(harness.onboardSpy).not.toHaveBeenCalled(); }); @@ -134,10 +132,7 @@ describe("prepared rebuild recovery", () => { }), ).rejects.toThrow("no NemoClaw-managed image fingerprint"); - expect(harness.runOpenshellSpy).not.toHaveBeenCalledWith( - ["sandbox", "delete", "alpha"], - expect.anything(), - ); + expectNoSandboxDelete(harness.runOpenshellSpy); }); it("rejects a mismatched prepared manifest before deleting the sandbox (#6114)", async () => { @@ -156,10 +151,7 @@ describe("prepared rebuild recovery", () => { ).rejects.toThrow("Invalid recovery manifest"); expect(harness.backupSandboxStateSpy).not.toHaveBeenCalled(); - expect(harness.runOpenshellSpy).not.toHaveBeenCalledWith( - ["sandbox", "delete", "alpha"], - expect.anything(), - ); + expectNoSandboxDelete(harness.runOpenshellSpy); expect(harness.onboardSpy).not.toHaveBeenCalled(); }); @@ -183,10 +175,7 @@ describe("prepared rebuild recovery", () => { expect(validationCount).toBe(2); expect(harness.backupSandboxStateSpy).not.toHaveBeenCalled(); - expect(harness.runOpenshellSpy).not.toHaveBeenCalledWith( - ["sandbox", "delete", "alpha"], - expect.anything(), - ); + expectNoSandboxDelete(harness.runOpenshellSpy); expect(harness.onboardSpy).not.toHaveBeenCalled(); }); @@ -211,10 +200,7 @@ describe("prepared rebuild recovery", () => { ).rejects.toThrow("Recovery registry configuration changed during preflight"); expect(harness.backupSandboxStateSpy).not.toHaveBeenCalled(); - expect(harness.runOpenshellSpy).not.toHaveBeenCalledWith( - ["sandbox", "delete", "alpha"], - expect.anything(), - ); + expectNoSandboxDelete(harness.runOpenshellSpy); }); it("uses the single refreshed registry snapshot for recreate rollback (#6114)", async () => { @@ -255,10 +241,7 @@ describe("prepared rebuild recovery", () => { ).rejects.toThrow("Recovery backup identity changed during preflight"); expect(harness.backupSandboxStateSpy).not.toHaveBeenCalled(); - expect(harness.runOpenshellSpy).not.toHaveBeenCalledWith( - ["sandbox", "delete", "alpha"], - expect.anything(), - ); + expectNoSandboxDelete(harness.runOpenshellSpy); }); it("restores the registry entry when prepared-backup recreation fails (#6114)", async () => { diff --git a/test/helpers/rebuild-dcode-flow-helpers.ts b/test/helpers/rebuild-dcode-flow-helpers.ts index 9cf23863de0..40a5fcd5ee1 100644 --- a/test/helpers/rebuild-dcode-flow-helpers.ts +++ b/test/helpers/rebuild-dcode-flow-helpers.ts @@ -3,6 +3,7 @@ import { expect } from "vitest"; +import { expectNoSandboxDelete } from "./rebuild-delete-assertions"; import type { RebuildFlowHarness } from "./rebuild-flow-harness"; export function makeDcodeSandboxEntry(): Record { @@ -42,10 +43,7 @@ export function configureDcodeSession(harness: RebuildFlowHarness): void { export function expectNoDcodeMutation(harness: RebuildFlowHarness): void { expect(harness.openShieldsSpy).not.toHaveBeenCalled(); expect(harness.backupSandboxStateSpy).not.toHaveBeenCalled(); - expect(harness.runOpenshellSpy).not.toHaveBeenCalledWith( - ["sandbox", "delete", "alpha"], - expect.anything(), - ); + expectNoSandboxDelete(harness.runOpenshellSpy); expect(harness.removeSandboxRegistryEntrySpy).not.toHaveBeenCalled(); expect(harness.onboardSpy).not.toHaveBeenCalled(); } diff --git a/test/helpers/rebuild-dcode-flow-support.ts b/test/helpers/rebuild-dcode-flow-support.ts index 8ac165c830b..0a24c392d0d 100644 --- a/test/helpers/rebuild-dcode-flow-support.ts +++ b/test/helpers/rebuild-dcode-flow-support.ts @@ -3,6 +3,7 @@ import { expect } from "vitest"; +import { expectNoSandboxDelete } from "./rebuild-delete-assertions"; import { type RebuildFlowHarness } from "./rebuild-flow-harness"; export function makeDcodeSandboxEntry(): Record { @@ -42,10 +43,7 @@ export function configureDcodeSession(harness: RebuildFlowHarness): void { export function expectNoDcodeMutation(harness: RebuildFlowHarness): void { expect(harness.openShieldsSpy).not.toHaveBeenCalled(); expect(harness.backupSandboxStateSpy).not.toHaveBeenCalled(); - expect(harness.runOpenshellSpy).not.toHaveBeenCalledWith( - ["sandbox", "delete", "alpha"], - expect.anything(), - ); + expectNoSandboxDelete(harness.runOpenshellSpy); expect(harness.removeSandboxRegistryEntrySpy).not.toHaveBeenCalled(); expect(harness.onboardSpy).not.toHaveBeenCalled(); } diff --git a/test/helpers/rebuild-delete-assertions.ts b/test/helpers/rebuild-delete-assertions.ts new file mode 100644 index 00000000000..e8a23e9d7b7 --- /dev/null +++ b/test/helpers/rebuild-delete-assertions.ts @@ -0,0 +1,11 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { expect, type MockInstance } from "vitest"; + +export function expectNoSandboxDelete(runOpenshellSpy: MockInstance): void { + const sandboxDeleteWasCalled = runOpenshellSpy.mock.calls.some( + ([args]) => Array.isArray(args) && args[0] === "sandbox" && args[1] === "delete", + ); + expect(sandboxDeleteWasCalled).toBe(false); +} diff --git a/test/helpers/rebuild-flow-credential-preflight-cases.ts b/test/helpers/rebuild-flow-credential-preflight-cases.ts index 9bfb10ac9b4..1512970ac93 100644 --- a/test/helpers/rebuild-flow-credential-preflight-cases.ts +++ b/test/helpers/rebuild-flow-credential-preflight-cases.ts @@ -3,6 +3,7 @@ import { describe, expect, it } from "vitest"; import { makePreparedRecoveryManifest } from "../../src/lib/actions/sandbox/rebuild-flow-test-fixtures"; +import { expectNoSandboxDelete } from "./rebuild-delete-assertions"; import { createRebuildFlowHarness, installRebuildFlowTestHooks } from "./rebuild-flow-test-harness"; type Harness = ReturnType; @@ -225,10 +226,7 @@ export function registerRebuildFlowCredentialPreflightTests(): void { }), ).rejects.toThrow("changed during rebuild preflight"); - expect(harness.runOpenshellSpy).not.toHaveBeenCalledWith( - ["sandbox", "delete", "alpha"], - expect.anything(), - ); + expectNoSandboxDelete(harness.runOpenshellSpy); expect(harness.onboardSpy).not.toHaveBeenCalled(); }); @@ -261,10 +259,7 @@ export function registerRebuildFlowCredentialPreflightTests(): void { }), ).rejects.toThrow("became unavailable before sandbox deletion"); - expect(harness.runOpenshellSpy).not.toHaveBeenCalledWith( - ["sandbox", "delete", "alpha"], - expect.anything(), - ); + expectNoSandboxDelete(harness.runOpenshellSpy); expect(harness.onboardSpy).not.toHaveBeenCalled(); }); @@ -301,10 +296,7 @@ export function registerRebuildFlowCredentialPreflightTests(): void { }), ).rejects.toThrow("could not be verified before sandbox deletion"); - expect(harness.runOpenshellSpy).not.toHaveBeenCalledWith( - ["sandbox", "delete", "alpha"], - expect.anything(), - ); + expectNoSandboxDelete(harness.runOpenshellSpy); expect(harness.onboardSpy).not.toHaveBeenCalled(); }); diff --git a/test/helpers/rebuild-flow-lifecycle-cases.ts b/test/helpers/rebuild-flow-lifecycle-cases.ts index 8a0c57c2c5d..03575983959 100644 --- a/test/helpers/rebuild-flow-lifecycle-cases.ts +++ b/test/helpers/rebuild-flow-lifecycle-cases.ts @@ -3,6 +3,7 @@ import { describe, expect, it } from "vitest"; import { makePreparedRecoveryManifest } from "../../src/lib/actions/sandbox/rebuild-flow-test-fixtures"; +import { expectNoSandboxDelete } from "./rebuild-delete-assertions"; import { createRebuildFlowHarness, installRebuildFlowTestHooks, @@ -26,11 +27,7 @@ export function registerRebuildFlowLifecycleTests(): void { expect(harness.backupSandboxStateSpy).not.toHaveBeenCalled(); expect(harness.onboardSpy).not.toHaveBeenCalled(); expect(harness.removeSandboxRegistryEntryWithReceiptSpy).not.toHaveBeenCalled(); - expect( - harness.runOpenshellSpy.mock.calls.some( - ([args]) => Array.isArray(args) && args.join(" ") === "sandbox delete alpha", - ), - ).toBe(false); + expectNoSandboxDelete(harness.runOpenshellSpy); }); it("backs up, recreates, restores, reapplies policy, and relocks on a successful OpenClaw rebuild", async () => { @@ -63,7 +60,7 @@ export function registerRebuildFlowLifecycleTests(): void { harness.warnUnpreservedUserManagedFilesSpy.mock.invocationCallOrder[0], ); expect(harness.runOpenshellSpy).toHaveBeenCalledWith( - ["sandbox", "delete", "alpha"], + ["sandbox", "delete", "-g", "nemoclaw", "alpha"], expect.objectContaining({ ignoreError: true }), ); expect(harness.onboardSpy).toHaveBeenCalledWith( @@ -86,7 +83,8 @@ export function registerRebuildFlowLifecycleTests(): void { }), ); const deleteCall = harness.runOpenshellSpy.mock.calls.findIndex( - (call) => Array.isArray(call[0]) && call[0].join(" ") === "sandbox delete alpha", + (call) => + Array.isArray(call[0]) && call[0].join(" ") === "sandbox delete -g nemoclaw alpha", ); expect(harness.registryUpdateSpy.mock.invocationCallOrder[0]).toBeLessThan( harness.runOpenshellSpy.mock.invocationCallOrder[deleteCall], diff --git a/test/helpers/rebuild-flow-recovery-cases.ts b/test/helpers/rebuild-flow-recovery-cases.ts index 0b0602a37aa..5d4267a922a 100644 --- a/test/helpers/rebuild-flow-recovery-cases.ts +++ b/test/helpers/rebuild-flow-recovery-cases.ts @@ -8,6 +8,7 @@ import { makeActiveTeamsMessagingPlan, makePreparedRecoveryManifest, } from "../../src/lib/actions/sandbox/rebuild-flow-test-fixtures"; +import { expectNoSandboxDelete } from "./rebuild-delete-assertions"; import { createRebuildFlowHarness, installRebuildFlowTestHooks } from "./rebuild-flow-test-harness"; export function registerRebuildFlowRecoveryTests(): void { @@ -27,7 +28,7 @@ export function registerRebuildFlowRecoveryTests(): void { expect(harness.backupSandboxStateSpy).not.toHaveBeenCalled(); expect(harness.runOpenshellSpy).toHaveBeenCalledWith( - ["sandbox", "delete", "alpha"], + ["sandbox", "delete", "-g", "nemoclaw", "alpha"], expect.objectContaining({ ignoreError: true }), ); expect(harness.restoreSandboxStateSpy).toHaveBeenCalledWith( @@ -63,7 +64,7 @@ export function registerRebuildFlowRecoveryTests(): void { expect(harness.backupSandboxStateSpy).not.toHaveBeenCalled(); expect(harness.runOpenshellSpy).toHaveBeenCalledWith( - ["sandbox", "delete", "alpha"], + ["sandbox", "delete", "-g", "nemoclaw", "alpha"], expect.objectContaining({ ignoreError: true }), ); expect(harness.restoreSandboxStateSpy).toHaveBeenCalledWith( @@ -89,10 +90,7 @@ export function registerRebuildFlowRecoveryTests(): void { ).rejects.toThrow("Invalid recovery manifest"); expect(harness.backupSandboxStateSpy).not.toHaveBeenCalled(); - expect(harness.runOpenshellSpy).not.toHaveBeenCalledWith( - ["sandbox", "delete", "alpha"], - expect.anything(), - ); + expectNoSandboxDelete(harness.runOpenshellSpy); expect(harness.onboardSpy).not.toHaveBeenCalled(); }); @@ -116,10 +114,7 @@ export function registerRebuildFlowRecoveryTests(): void { expect(validationCount).toBe(2); expect(harness.backupSandboxStateSpy).not.toHaveBeenCalled(); - expect(harness.runOpenshellSpy).not.toHaveBeenCalledWith( - ["sandbox", "delete", "alpha"], - expect.anything(), - ); + expectNoSandboxDelete(harness.runOpenshellSpy); }); it("rejects registry configuration drift before prepared recovery deletion (#6114)", async () => { @@ -143,10 +138,7 @@ export function registerRebuildFlowRecoveryTests(): void { ).rejects.toThrow("Recovery registry configuration changed during preflight"); expect(harness.backupSandboxStateSpy).not.toHaveBeenCalled(); - expect(harness.runOpenshellSpy).not.toHaveBeenCalledWith( - ["sandbox", "delete", "alpha"], - expect.anything(), - ); + expectNoSandboxDelete(harness.runOpenshellSpy); }); it("uses the refreshed registry snapshot for prepared-recovery rollback (#6114)", async () => { @@ -188,10 +180,7 @@ export function registerRebuildFlowRecoveryTests(): void { ).rejects.toThrow("Recovery backup identity changed during preflight"); expect(harness.backupSandboxStateSpy).not.toHaveBeenCalled(); - expect(harness.runOpenshellSpy).not.toHaveBeenCalledWith( - ["sandbox", "delete", "alpha"], - expect.anything(), - ); + expectNoSandboxDelete(harness.runOpenshellSpy); }); it("restores the registry entry when prepared-backup recreation fails (#6114)", async () => { @@ -441,10 +430,7 @@ export function registerRebuildFlowRecoveryTests(): void { expect(errors).toContain("messaging manifest plan could not be staged"); expect(harness.releaseOnboardLockSpy).toHaveBeenCalledOnce(); expect(harness.backupSandboxStateSpy).not.toHaveBeenCalled(); - expect(harness.runOpenshellSpy).not.toHaveBeenCalledWith( - ["sandbox", "delete", "alpha"], - expect.anything(), - ); + expectNoSandboxDelete(harness.runOpenshellSpy); expect(harness.onboardSpy).not.toHaveBeenCalled(); }); @@ -464,7 +450,7 @@ export function registerRebuildFlowRecoveryTests(): void { }, runOpenshell: (args) => { const command = args.join(" "); - if (command === "sandbox delete alpha") { + if (command === "sandbox delete -g nemoclaw alpha") { return { status: 7, output: "delete failed", stderr: "delete failed" }; } if (command === "sandbox get -g nemoclaw alpha") { diff --git a/test/helpers/rebuild-flow-target-credentials-cases.ts b/test/helpers/rebuild-flow-target-credentials-cases.ts index ee53b749e0b..8832466fd7b 100644 --- a/test/helpers/rebuild-flow-target-credentials-cases.ts +++ b/test/helpers/rebuild-flow-target-credentials-cases.ts @@ -6,6 +6,7 @@ import os from "node:os"; import path from "node:path"; import { describe, expect, it } from "vitest"; +import { expectNoSandboxDelete } from "./rebuild-delete-assertions"; import { createRebuildFlowHarness, installRebuildFlowTestHooks, @@ -29,10 +30,7 @@ export function registerRebuildFlowTargetCredentialsTests(): void { ).rejects.toThrow("Brave Search credential preflight failed"); expect(harness.backupSandboxStateSpy).not.toHaveBeenCalled(); - expect(harness.runOpenshellSpy).not.toHaveBeenCalledWith( - ["sandbox", "delete", "alpha"], - expect.anything(), - ); + expectNoSandboxDelete(harness.runOpenshellSpy); }); it("rejects recorded web search when the target agent does not support it", async () => { @@ -73,10 +71,7 @@ export function registerRebuildFlowTargetCredentialsTests(): void { expect(harness.backupSandboxStateSpy).not.toHaveBeenCalled(); expect(harness.prepareMcpBridgesForRebuildSpy).not.toHaveBeenCalled(); - expect(harness.runOpenshellSpy).not.toHaveBeenCalledWith( - ["sandbox", "delete", "alpha"], - expect.anything(), - ); + expectNoSandboxDelete(harness.runOpenshellSpy); }); it("preserves legacy Brave web search during a nonmatching-session rebuild", async () => { diff --git a/test/helpers/rebuild-flow-target-image-cases.ts b/test/helpers/rebuild-flow-target-image-cases.ts index ecbb2aa7d3e..ad8b85624da 100644 --- a/test/helpers/rebuild-flow-target-image-cases.ts +++ b/test/helpers/rebuild-flow-target-image-cases.ts @@ -8,6 +8,7 @@ import path from "node:path"; import { describe, expect, it, vi } from "vitest"; import { createBuildContextVerifier } from "../../src/lib/actions/sandbox/rebuild-prepared-image-context"; import { fingerprintBuildContext } from "../../src/lib/adapters/fs/build-context-fingerprint"; +import { expectNoSandboxDelete } from "./rebuild-delete-assertions"; import { createRebuildFlowHarness, installRebuildFlowTestHooks, @@ -211,10 +212,7 @@ export function registerRebuildFlowTargetImageTests(): void { harness.rebuildSandbox("alpha", ["--yes"], { throwOnError: true }), ).rejects.toThrow("Replacement sandbox image context changed before delete"); - expect(harness.runOpenshellSpy).not.toHaveBeenCalledWith( - ["sandbox", "delete", "alpha"], - expect.anything(), - ); + expectNoSandboxDelete(harness.runOpenshellSpy); expect(harness.onboardSpy).not.toHaveBeenCalled(); expect(cleanupBuildCtx).toHaveBeenCalledOnce(); } finally { @@ -273,10 +271,7 @@ export function registerRebuildFlowTargetImageTests(): void { await expect( harness.rebuildSandbox("alpha", ["--yes"], { throwOnError: true }), ).rejects.toThrow("Replacement sandbox image context changed before delete"); - expect(harness.runOpenshellSpy).not.toHaveBeenCalledWith( - ["sandbox", "delete", "alpha"], - expect.anything(), - ); + expectNoSandboxDelete(harness.runOpenshellSpy); expect(harness.onboardSpy).not.toHaveBeenCalled(); expect(cleanupBuildCtx).toHaveBeenCalledOnce(); } finally { @@ -322,7 +317,7 @@ export function registerRebuildFlowTargetImageTests(): void { expect(harness.session.policyPresets).toEqual(["npm", "bad", "throw"]); expect(harness.session.gpuPassthrough).toBe(false); expect(harness.runOpenshellSpy).toHaveBeenCalledWith( - ["sandbox", "delete", "alpha"], + ["sandbox", "delete", "-g", "nemoclaw", "alpha"], expect.objectContaining({ ignoreError: true }), ); } finally { @@ -344,7 +339,7 @@ export function registerRebuildFlowTargetImageTests(): void { ).resolves.toBeUndefined(); expect(harness.runOpenshellSpy).toHaveBeenCalledWith( - ["sandbox", "delete", "alpha"], + ["sandbox", "delete", "-g", "nemoclaw", "alpha"], expect.objectContaining({ ignoreError: true }), ); expect(harness.onboardSpy).toHaveBeenCalled(); diff --git a/test/helpers/rebuild-flow-target-session-cases.ts b/test/helpers/rebuild-flow-target-session-cases.ts index 94b82fde158..7a6131d6534 100644 --- a/test/helpers/rebuild-flow-target-session-cases.ts +++ b/test/helpers/rebuild-flow-target-session-cases.ts @@ -2,6 +2,7 @@ // SPDX-License-Identifier: Apache-2.0 import { describe, expect, it } from "vitest"; +import { expectNoSandboxDelete } from "./rebuild-delete-assertions"; import { createRebuildFlowHarness, installRebuildFlowTestHooks, @@ -186,10 +187,7 @@ export function registerRebuildFlowTargetSessionTests(): void { expect(errors).toContain("cannot determine the inference endpoint"); expect(errors).toContain("Sandbox is untouched"); expect(harness.backupSandboxStateSpy).not.toHaveBeenCalled(); - expect(harness.runOpenshellSpy).not.toHaveBeenCalledWith( - ["sandbox", "delete", "alpha"], - expect.anything(), - ); + expectNoSandboxDelete(harness.runOpenshellSpy); expect(harness.onboardSpy).not.toHaveBeenCalled(); } finally { restoreEnv(); diff --git a/test/mcp-destroy-lifecycle.test.ts b/test/mcp-destroy-lifecycle.test.ts index c7cfb8609c7..5711da927e9 100644 --- a/test/mcp-destroy-lifecycle.test.ts +++ b/test/mcp-destroy-lifecycle.test.ts @@ -876,7 +876,7 @@ describe("authenticated MCP sandbox destroy lifecycle", () => { }); expect(testState.executeSandboxCommand).toHaveBeenCalledWith("alpha", ":"); expect(testState.runOpenshell).toHaveBeenCalledWith( - ["sandbox", "delete", "alpha"], + ["sandbox", "delete", "-g", "nemoclaw", "alpha"], expect.any(Object), ); expect(testState.stopNimContainer).not.toHaveBeenCalled(); diff --git a/test/rebuild-stale-recovery.test.ts b/test/rebuild-stale-recovery.test.ts index e8157f3f158..e6a7348121c 100644 --- a/test/rebuild-stale-recovery.test.ts +++ b/test/rebuild-stale-recovery.test.ts @@ -28,6 +28,7 @@ import fs from "node:fs"; import os from "node:os"; import path from "node:path"; import { afterEach, describe, expect, it } from "vitest"; +import { expectNoSandboxDelete } from "./helpers/rebuild-delete-assertions"; import { createRebuildFlowHarness, installRebuildFlowTestHooks, @@ -303,10 +304,7 @@ describe("stale sandbox rebuild recovery (#4497)", () => { // Must surface the wrong-gateway guidance and preserve the registry entry. expect(output).toContain("NOT been removed"); expect(harness.backupSandboxStateSpy).not.toHaveBeenCalled(); - expect(harness.runOpenshellSpy).not.toHaveBeenCalledWith( - ["sandbox", "delete", "alpha"], - expect.anything(), - ); + expectNoSandboxDelete(harness.runOpenshellSpy); expect(harness.removeSandboxRegistryEntryWithReceiptSpy).not.toHaveBeenCalled(); expect(harness.onboardSpy).not.toHaveBeenCalled(); }); @@ -338,10 +336,7 @@ describe("stale sandbox rebuild recovery (#4497)", () => { expect(output).not.toContain("Creating new sandbox with current image"); expect(output).toContain("openshell gateway select nemoclaw-9000"); expect(harness.backupSandboxStateSpy).not.toHaveBeenCalled(); - expect(harness.runOpenshellSpy).not.toHaveBeenCalledWith( - ["sandbox", "delete", "alpha"], - expect.anything(), - ); + expectNoSandboxDelete(harness.runOpenshellSpy); expect(harness.removeSandboxRegistryEntryWithReceiptSpy).not.toHaveBeenCalled(); expect(harness.onboardSpy).not.toHaveBeenCalled(); }); From 390bf2637e8cd2a066358dfc3c1f88b502e251f9 Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Fri, 24 Jul 2026 20:54:40 -0700 Subject: [PATCH 12/18] docs(rebuild): clarify recorded-gateway deletion Signed-off-by: Apurv Kumaria --- docs/manage-sandboxes/recover-rebuild-sandboxes.mdx | 9 ++++++--- docs/reference/commands.mdx | 8 ++++++-- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/docs/manage-sandboxes/recover-rebuild-sandboxes.mdx b/docs/manage-sandboxes/recover-rebuild-sandboxes.mdx index 2a92a61e3b1..4d1ec536979 100644 --- a/docs/manage-sandboxes/recover-rebuild-sandboxes.mdx +++ b/docs/manage-sandboxes/recover-rebuild-sandboxes.mdx @@ -171,10 +171,13 @@ Use this recovery path only when losing the state that could not be backed up is When a sandbox with managed MCP servers cannot run a pre-mutation no-op, explicit `--force` uses its complete registry entries plus the exact live generated policies and provider identities to preserve MCP intent without scrubbing the unreachable in-sandbox adapter. Every bridge entry must record an explicit adapter that matches the sandbox's recorded agent, and the registered policy must be the canonical generated policy for that adapter, server name, URL endpoint, and current resolved-address pins. NemoClaw rechecks that read-only snapshot immediately before deletion and stops if the target, registry, policy, provider, or recorded gateway changed. +NemoClaw sends the delete request and every deletion-confirmation lookup to the sandbox's exact recorded gateway. Across every rebuild path, NemoClaw does not attempt to stop the local NIM through the delete attempt, and cleanup is attempted on a best-effort basis only after deletion is positively confirmed. -After a nonzero delete, NemoClaw queries the sandbox name on its exact recorded gateway: an explicit missing result converges as deleted, while a `Ready` or `Running` result triggers an attempt to restore prepared MCP state and any shields lockdown that rebuild temporarily opened. -That MCP and shields restoration can fail and is reported rather than presented as a successful rollback. -Any partial or unreachable result remains ambiguous: NemoClaw preserves the MCP ownership and rebuild-recovery records, does not attempt to stop NIM, skips the rebuild process's immediate shields relock, and does not claim that the original sandbox is intact. +After a nonzero delete, an explicit missing result converges as deleted. +A `Ready` or `Running` result triggers an attempt to restore prepared MCP state and any shields lockdown that rebuild temporarily opened. +NemoClaw reports any MCP or shields restoration failure and does not present the operation as a successful rollback. +Any partial or unreachable result remains ambiguous. +NemoClaw preserves the MCP ownership and rebuild-recovery records, does not attempt to stop NIM, skips the rebuild process's immediate shields relock, and does not claim that the original sandbox is intact. Inspect the live sandbox and gateway state before retrying recovery. This recovery also stops for incomplete MCP adds or ambiguous ownership; an error after a successful no-op does not fall back to the host-side path. diff --git a/docs/reference/commands.mdx b/docs/reference/commands.mdx index e54494f7650..a102f59d1a9 100644 --- a/docs/reference/commands.mdx +++ b/docs/reference/commands.mdx @@ -2579,9 +2579,13 @@ For a sandbox with managed MCP servers, `--force` probes sandbox execution befor If that no-op cannot run, NemoClaw requires complete bridge entries and exact live policy and provider identities, without trying an in-sandbox adapter scrub or changing MCP ownership state. Each bridge must carry an explicit adapter matching the sandbox's recorded agent, and the registered policy must equal the canonical generated policy for that adapter, server name, URL endpoint, and current resolved-address pins. It rechecks the registry, recorded gateway, resolved targets, live generated policies, and provider identities immediately before deletion; incomplete adds, drift, or ambiguous ownership stop before deletion. +NemoClaw sends the delete request and every deletion-confirmation lookup to the sandbox's exact recorded gateway. Across every rebuild path, NemoClaw does not attempt to stop local NIM until sandbox deletion is positively confirmed, then attempts NIM cleanup on a best-effort basis. -When `openshell sandbox delete` exits nonzero, an exact owner-gateway lookup distinguishes explicit absence from a confirmed `Ready` or `Running` sandbox; any other phase or probe failure is ambiguous. -Explicit absence continues the rebuild, confirmed intact state triggers an attempt to restore prepared MCP state and any shields lockdown that rebuild temporarily opened, and any restoration failure is reported. +When `openshell sandbox delete` exits nonzero, an exact recorded-gateway lookup distinguishes explicit absence from a confirmed `Ready` or `Running` sandbox. +Any other phase or probe failure is ambiguous. +Explicit absence continues the rebuild. +Confirmed intact state triggers an attempt to restore prepared MCP state and any shields lockdown that rebuild temporarily opened. +NemoClaw reports any MCP or shields restoration failure and does not present the operation as a successful rollback. Ambiguous state preserves MCP ownership and recovery metadata without attempting to stop NIM or claiming the original sandbox remains intact, and the rebuild process skips its immediate shields relock. Failures after a successful exec probe do not switch to the host-side path. Before backup or deletion, `rebuild` also refuses an incomplete MCP destroy transaction. From 59bac28ea335826aa35a220e9b33559350ec6501 Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Fri, 24 Jul 2026 21:15:15 -0700 Subject: [PATCH 13/18] test(rebuild): model recorded-gateway deletion probes Co-authored-by: kagura-agent Signed-off-by: Apurv Kumaria --- src/lib/actions/sandbox/rebuild-resume-snapshot.test.ts | 3 ++- test/helpers/rebuild-flow-harness.ts | 5 ++++- test/helpers/rebuild-flow-test-harness.ts | 5 ++++- 3 files changed, 10 insertions(+), 3 deletions(-) diff --git a/src/lib/actions/sandbox/rebuild-resume-snapshot.test.ts b/src/lib/actions/sandbox/rebuild-resume-snapshot.test.ts index f7a2bbdcbc9..a00ed2fe15a 100644 --- a/src/lib/actions/sandbox/rebuild-resume-snapshot.test.ts +++ b/src/lib/actions/sandbox/rebuild-resume-snapshot.test.ts @@ -157,7 +157,8 @@ describe("rebuild resume snapshot repair", () => { } as never), vi.spyOn(openshellRuntime, "runOpenshell").mockImplementation((args: unknown) => { const argv = Array.isArray(args) ? args.map(String) : []; - return argv.join(" ") === "sandbox get alpha" + return argv.join(" ") === "sandbox get alpha" || + argv.join(" ") === "sandbox get -g nemoclaw alpha" ? ({ status: 1, output: "sandbox alpha not found", diff --git a/test/helpers/rebuild-flow-harness.ts b/test/helpers/rebuild-flow-harness.ts index 9507b22bc3e..801b6320a3d 100644 --- a/test/helpers/rebuild-flow-harness.ts +++ b/test/helpers/rebuild-flow-harness.ts @@ -527,7 +527,10 @@ export function createRebuildFlowHarness(overrides: RebuildFlowOverrides = {}): ); const runOpenshellSpy = vi.spyOn(openshellRuntime, "runOpenshell").mockImplementation((args) => { const argv = args as string[]; - if (argv.join(" ") === "sandbox get alpha") { + if ( + argv.join(" ") === "sandbox get alpha" || + argv.join(" ") === "sandbox get -g nemoclaw alpha" + ) { return { status: 1, output: "sandbox alpha not found", diff --git a/test/helpers/rebuild-flow-test-harness.ts b/test/helpers/rebuild-flow-test-harness.ts index 25e64166524..92e5cd6200c 100644 --- a/test/helpers/rebuild-flow-test-harness.ts +++ b/test/helpers/rebuild-flow-test-harness.ts @@ -382,7 +382,10 @@ export function createRebuildFlowHarness(overrides: RebuildFlowOverrides = {}): .spyOn(openshellRuntime, "runOpenshell") .mockImplementation((args: unknown) => { const argv = Array.isArray(args) ? args.map(String) : []; - if (argv.join(" ") === "sandbox get alpha") { + if ( + argv.join(" ") === "sandbox get alpha" || + argv.join(" ") === "sandbox get -g nemoclaw alpha" + ) { return { status: 1, output: "sandbox alpha not found", From 280c81e08b606d5aab82556a91991cb31c8c9d85 Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Fri, 24 Jul 2026 21:34:22 -0700 Subject: [PATCH 14/18] test(rebuild): preserve explicit OpenShell fixtures Co-authored-by: kagura-agent Signed-off-by: Apurv Kumaria --- .../rebuild-flow-credential-preflight-cases.ts | 12 +++++++++--- test/helpers/rebuild-flow-recovery-cases.ts | 2 +- test/helpers/rebuild-flow-test-harness.ts | 4 +++- test/helpers/rebuild-flow-test-support.ts | 14 ++++++++------ 4 files changed, 21 insertions(+), 11 deletions(-) diff --git a/test/helpers/rebuild-flow-credential-preflight-cases.ts b/test/helpers/rebuild-flow-credential-preflight-cases.ts index 1512970ac93..4ff020a2e1e 100644 --- a/test/helpers/rebuild-flow-credential-preflight-cases.ts +++ b/test/helpers/rebuild-flow-credential-preflight-cases.ts @@ -31,7 +31,7 @@ function providerRuntime( ) { return (args: string[]) => { if (args[0] !== "provider" || args[1] !== "get") { - return { status: 0, output: "", stdout: "", stderr: "" }; + return undefined; } const provider = args[2]; if (!registeredProviders.includes(provider)) { @@ -211,7 +211,10 @@ export function registerRebuildFlowCredentialPreflightTests(): void { preferredInferenceApi: "openai-completions", }, hydrateCredentialEnv: () => "host-provider-key", - runOpenshell: (args) => (providerLookups.shift() ?? registeredProvider)(args), + runOpenshell: (args) => + args[0] === "provider" + ? (providerLookups.shift() ?? registeredProvider)(args) + : undefined, staleRecovery: true, }); configureSession(harness, "compatible-endpoint", "COMPATIBLE_API_KEY", { @@ -281,7 +284,10 @@ export function registerRebuildFlowCredentialPreflightTests(): void { preferredInferenceApi: "openai-completions", }, hydrateCredentialEnv: () => "host-provider-key", - runOpenshell: (args) => (providerLookups.shift() ?? indeterminateProvider)(args), + runOpenshell: (args) => + args[0] === "provider" + ? (providerLookups.shift() ?? indeterminateProvider)(args) + : undefined, staleRecovery: true, }); configureSession(harness, "compatible-endpoint", "COMPATIBLE_API_KEY", { diff --git a/test/helpers/rebuild-flow-recovery-cases.ts b/test/helpers/rebuild-flow-recovery-cases.ts index 5d4267a922a..1aba1431732 100644 --- a/test/helpers/rebuild-flow-recovery-cases.ts +++ b/test/helpers/rebuild-flow-recovery-cases.ts @@ -456,7 +456,7 @@ export function registerRebuildFlowRecoveryTests(): void { if (command === "sandbox get -g nemoclaw alpha") { return { status: 0, output: "Phase: Ready", stdout: "Phase: Ready", stderr: "" }; } - return { status: 0, output: "" }; + return undefined; }, }); diff --git a/test/helpers/rebuild-flow-test-harness.ts b/test/helpers/rebuild-flow-test-harness.ts index 92e5cd6200c..a2976ffe4c2 100644 --- a/test/helpers/rebuild-flow-test-harness.ts +++ b/test/helpers/rebuild-flow-test-harness.ts @@ -382,6 +382,8 @@ export function createRebuildFlowHarness(overrides: RebuildFlowOverrides = {}): .spyOn(openshellRuntime, "runOpenshell") .mockImplementation((args: unknown) => { const argv = Array.isArray(args) ? args.map(String) : []; + const overrideResult = overrides.runOpenshell?.(argv); + if (overrideResult) return overrideResult; if ( argv.join(" ") === "sandbox get alpha" || argv.join(" ") === "sandbox get -g nemoclaw alpha" @@ -393,7 +395,7 @@ export function createRebuildFlowHarness(overrides: RebuildFlowOverrides = {}): stderr: "sandbox alpha not found", }; } - return overrides.runOpenshell ? overrides.runOpenshell(argv) : { status: 0, output: "" }; + return { status: 0, output: "" }; }); const defaultRemovalReceipt = { entry: preDeleteSandboxEntry, diff --git a/test/helpers/rebuild-flow-test-support.ts b/test/helpers/rebuild-flow-test-support.ts index 4753e5c8f39..0939a9c2670 100644 --- a/test/helpers/rebuild-flow-test-support.ts +++ b/test/helpers/rebuild-flow-test-support.ts @@ -83,12 +83,14 @@ export type RebuildFlowOverrides = { detachedProviderEntries: Array>; scrubbedAdapterEntries?: Array>; }; - runOpenshell?: (args: string[]) => { - status: number; - output: string; - stdout?: string; - stderr?: string; - }; + runOpenshell?: (args: string[]) => + | { + status: number; + output: string; + stdout?: string; + stderr?: string; + } + | undefined; backupPolicyPresets?: string[]; ensureValidatedBraveSearchCredential?: () => Promise; ensureValidatedWebSearchCredential?: () => Promise; From 5288cd2ad366ca3fc737b31c6f455543ef8b0f95 Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Sat, 25 Jul 2026 17:18:36 -0700 Subject: [PATCH 15/18] test(rebuild): expect recorded-gateway convergence Signed-off-by: Apurv Kumaria --- test/helpers/rebuild-flow-lifecycle-cases.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/helpers/rebuild-flow-lifecycle-cases.ts b/test/helpers/rebuild-flow-lifecycle-cases.ts index b1841ea2b5d..b8f0d3aa9d8 100644 --- a/test/helpers/rebuild-flow-lifecycle-cases.ts +++ b/test/helpers/rebuild-flow-lifecycle-cases.ts @@ -274,7 +274,7 @@ network_policies: expect(events).toEqual(["stale-live", "absent", "onboard"]); expect( harness.captureOpenshellSpy.mock.calls.filter( - ([args]) => Array.isArray(args) && args.join(" ") === "sandbox get alpha", + ([args]) => Array.isArray(args) && args.join(" ") === "sandbox get -g nemoclaw alpha", ), ).toHaveLength(2); }); From 61834f2899df691ff3b8ae243f2d2aa34395fe1b Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Sat, 25 Jul 2026 17:47:22 -0700 Subject: [PATCH 16/18] fix(rebuild): require named delete convergence Signed-off-by: Apurv Kumaria --- .../sandbox/rebuild-destroy-phase.test.ts | 61 +++++++++++++++++++ .../actions/sandbox/rebuild-destroy-phase.ts | 6 +- 2 files changed, 64 insertions(+), 3 deletions(-) diff --git a/src/lib/actions/sandbox/rebuild-destroy-phase.test.ts b/src/lib/actions/sandbox/rebuild-destroy-phase.test.ts index a45d2815c7c..cd623ba688f 100644 --- a/src/lib/actions/sandbox/rebuild-destroy-phase.test.ts +++ b/src/lib/actions/sandbox/rebuild-destroy-phase.test.ts @@ -652,6 +652,67 @@ describe("rebuild destroy phase", () => { expect(currentMs).toBeLessThanOrEqual(15_000); }); + it.each([ + ["another sandbox", { status: 1, stderr: "sandbox beta not found" }], + [ + "a missing gateway", + { status: 1, stderr: 'status: NotFound, message: "gateway nemoclaw not found"' }, + ], + [ + "a missing provider", + { status: 1, stderr: 'status: NotFound, message: "provider alpha-mcp-github not found"' }, + ], + [ + "mixed gateway and sandbox diagnostics", + { + status: 1, + stderr: + 'status: NotFound, message: "gateway nemoclaw not found"\nstatus: Internal, message: "sandbox has no spec"', + }, + ], + [ + "a signal-terminated probe with a missing-sandbox diagnostic", + { + status: 1, + signal: "SIGTERM" as NodeJS.Signals, + stderr: "Error: sandbox alpha not found", + }, + ], + ])("does not continue deletion after convergence reports %s", async (_label, probe) => { + mocks.getSandbox.mockReturnValue({ + name: "alpha", + agent: "openclaw", + nimContainer: "nim-alpha", + }); + mocks.runOpenshell.mockReturnValue({ status: 0, stdout: "deleted", stderr: "" }); + mocks.captureOpenshell.mockReturnValue({ + stdout: "", + ...probe, + }); + const onDeleted = vi.fn(); + + await expect( + runRebuildDestroyPhase({ + sandboxName: "alpha", + sandboxEntry: { name: "alpha", agent: "openclaw", gatewayName: "nemoclaw" }, + staleRecovery: false, + backupManifest: null, + log: vi.fn(), + bail: vi.fn((message: string): never => { + throw new Error(message); + }), + relockShieldsIfNeeded: vi.fn(() => true), + onDeleted, + }), + ).rejects.toThrow("Sandbox deletion could not be confirmed."); + + expect(onDeleted).not.toHaveBeenCalled(); + expect(mocks.stopNimContainer).not.toHaveBeenCalled(); + expect(mocks.stopNimContainerByName).not.toHaveBeenCalled(); + expect(mocks.removeSandboxRegistryEntryWithReceipt).not.toHaveBeenCalled(); + expect(mocks.listSandboxes).not.toHaveBeenCalled(); + }); + it("removes registry state only after the gateway reports the deleted sandbox missing", async () => { const events: string[] = []; let getAttempts = 0; diff --git a/src/lib/actions/sandbox/rebuild-destroy-phase.ts b/src/lib/actions/sandbox/rebuild-destroy-phase.ts index 464b22d0bb9..f96d74e5819 100644 --- a/src/lib/actions/sandbox/rebuild-destroy-phase.ts +++ b/src/lib/actions/sandbox/rebuild-destroy-phase.ts @@ -61,6 +61,7 @@ interface RebuildDeleteAbsenceDeps { stdout?: string; stderr?: string; error?: Error; + signal?: NodeJS.Signals | null; }; now?: () => number; sleep?: (milliseconds: number) => void; @@ -69,8 +70,6 @@ interface RebuildDeleteAbsenceDeps { const REBUILD_DELETE_ABSENCE_MAX_ATTEMPTS = 20; const REBUILD_DELETE_ABSENCE_INITIAL_INTERVAL_MS = 250; const REBUILD_DELETE_ABSENCE_MAX_INTERVAL_MS = 1_000; -const MISSING_SANDBOX_GET_OUTPUT = - /\b(?:no such sandbox|sandbox(?:\s+['"`]?[A-Za-z0-9._-]+['"`]?)?\s+(?:(?:was|is)\s+)?(?:not found|not present|does not exist|has no spec))\b/i; /** Wait for explicit absence from the same `sandbox get` boundary used by inner onboard. */ export function waitForRebuildDeleteAbsence( @@ -103,9 +102,10 @@ export function waitForRebuildDeleteAbsence( const combinedOutput = `${stdout}\n${String(probe.stderr ?? probe.output ?? "")}`.trim(); const state = !probe.error && + !probe.signal && probe.status !== null && probe.status !== 0 && - MISSING_SANDBOX_GET_OUTPUT.test(combinedOutput) + isExplicitMissingSandboxGatewayOutput(combinedOutput, sandboxName) ? "absent" : probe.status === 0 && stdout.length > 0 ? "present" From e92c7f060896fe3ea5fb84cc4876ddba26c8fcf9 Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Sat, 25 Jul 2026 18:04:25 -0700 Subject: [PATCH 17/18] test(rebuild): name delete convergence fixtures Signed-off-by: Apurv Kumaria --- src/lib/actions/sandbox/rebuild-local-provider-recreate.test.ts | 2 +- src/lib/actions/sandbox/rebuild-resume-snapshot.test.ts | 2 +- test/helpers/rebuild-flow-harness.ts | 2 +- test/helpers/rebuild-flow-lifecycle-cases.ts | 2 +- test/helpers/rebuild-flow-test-harness.ts | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/lib/actions/sandbox/rebuild-local-provider-recreate.test.ts b/src/lib/actions/sandbox/rebuild-local-provider-recreate.test.ts index 9adc4da76d6..0eb5dcef86d 100644 --- a/src/lib/actions/sandbox/rebuild-local-provider-recreate.test.ts +++ b/src/lib/actions/sandbox/rebuild-local-provider-recreate.test.ts @@ -152,7 +152,7 @@ describe("rebuild local-provider recreation", () => { status: 1, output: "", stdout: "", - stderr: "Not Found: sandbox not found", + stderr: "Error: sandbox alpha not found", }); let harness!: RebuildFlowHarness; let setupResult: SetupResult | undefined; diff --git a/src/lib/actions/sandbox/rebuild-resume-snapshot.test.ts b/src/lib/actions/sandbox/rebuild-resume-snapshot.test.ts index 8f0ca4238ed..36e0aae14ff 100644 --- a/src/lib/actions/sandbox/rebuild-resume-snapshot.test.ts +++ b/src/lib/actions/sandbox/rebuild-resume-snapshot.test.ts @@ -162,7 +162,7 @@ describe("rebuild resume snapshot repair", () => { status: 1, output: "", stdout: "", - stderr: "Not Found: sandbox not found", + stderr: "Error: sandbox alpha not found", } as never), vi.spyOn(destroy, "removeSandboxRegistryEntry").mockReturnValue(true), vi.spyOn(nim, "stopNimContainer").mockImplementation(() => undefined), diff --git a/test/helpers/rebuild-flow-harness.ts b/test/helpers/rebuild-flow-harness.ts index d4a1642ad7c..3600c49f0f1 100644 --- a/test/helpers/rebuild-flow-harness.ts +++ b/test/helpers/rebuild-flow-harness.ts @@ -546,7 +546,7 @@ export function createRebuildFlowHarness(overrides: RebuildFlowOverrides = {}): status: 1, output: "", stdout: "", - stderr: "Not Found: sandbox not found", + stderr: "Error: sandbox alpha not found", }; }); const runOpenshellSpy = vi.spyOn(openshellRuntime, "runOpenshell").mockImplementation((args) => { diff --git a/test/helpers/rebuild-flow-lifecycle-cases.ts b/test/helpers/rebuild-flow-lifecycle-cases.ts index b8f0d3aa9d8..af32d4ade64 100644 --- a/test/helpers/rebuild-flow-lifecycle-cases.ts +++ b/test/helpers/rebuild-flow-lifecycle-cases.ts @@ -252,7 +252,7 @@ network_policies: }, { event: "absent", - result: { status: 1, output: "", stderr: "Not Found: sandbox not found" }, + result: { status: 1, output: "", stderr: "Error: sandbox alpha not found" }, }, ]; const harness = createRebuildFlowHarness({ diff --git a/test/helpers/rebuild-flow-test-harness.ts b/test/helpers/rebuild-flow-test-harness.ts index 8d1ce416dd3..672441e2353 100644 --- a/test/helpers/rebuild-flow-test-harness.ts +++ b/test/helpers/rebuild-flow-test-harness.ts @@ -411,7 +411,7 @@ export function createRebuildFlowHarness(overrides: RebuildFlowOverrides = {}): const argv = Array.isArray(args) ? args.map(String) : []; return overrides.captureOpenshell ? overrides.captureOpenshell(argv, options as Record | undefined) - : { status: 1, output: "", stderr: "Not Found: sandbox not found" }; + : { status: 1, output: "", stderr: "Error: sandbox alpha not found" }; }); const defaultRemovalReceipt = { entry: preDeleteSandboxEntry, From 768e87c20a6f676e51bf6890884d724d76565a3a Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Sat, 25 Jul 2026 18:34:54 -0700 Subject: [PATCH 18/18] test(rebuild): name remaining absence fixtures Signed-off-by: Apurv Kumaria --- test/gateway-state-reconcile-2276.test.ts | 2 +- test/rebuild-credential-preflight.test.ts | 2 +- test/rebuild-stale-recovery.test.ts | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/test/gateway-state-reconcile-2276.test.ts b/test/gateway-state-reconcile-2276.test.ts index 40679dd55d8..00317cda23a 100644 --- a/test/gateway-state-reconcile-2276.test.ts +++ b/test/gateway-state-reconcile-2276.test.ts @@ -27,7 +27,7 @@ const GATEWAY_INFO_NEMOCLAW = const STATUS_CONNECTED_NEMOCLAW = "Server Status\n\nGateway: nemoclaw\nServer: https://127.0.0.1:8080/\nStatus: Connected\n"; -const SANDBOX_GET_NOT_FOUND = "Error: × Not Found: sandbox not found"; +const SANDBOX_GET_NOT_FOUND = "Error: sandbox my-assistant not found"; interface ScenarioScript { // sandbox get responses, one per call (cycled / stops at last) diff --git a/test/rebuild-credential-preflight.test.ts b/test/rebuild-credential-preflight.test.ts index aa499cd40c0..860f2465111 100644 --- a/test/rebuild-credential-preflight.test.ts +++ b/test/rebuild-credential-preflight.test.ts @@ -173,7 +173,7 @@ if (a[0] === "sandbox" && a[1] === "list") { process.stdout.write("${sandboxName if (a[0] === "sandbox" && a[1] === "ssh-config") { process.stdout.write("${sshConfig}\\n"); process.exit(0); } if (a[0] === "sandbox" && a[1] === "get") { if (fs.existsSync(${JSON.stringify(deleteMarker)})) { - process.stderr.write("Not Found: sandbox not found\\n"); + process.stderr.write("sandbox ${sandboxName} not found\\n"); process.exit(1); } process.stdout.write("Sandbox: ${sandboxName}\\nPhase: Ready\\n"); diff --git a/test/rebuild-stale-recovery.test.ts b/test/rebuild-stale-recovery.test.ts index e6a7348121c..42e2e5163cc 100644 --- a/test/rebuild-stale-recovery.test.ts +++ b/test/rebuild-stale-recovery.test.ts @@ -141,7 +141,7 @@ if (a[0]==="-V" || a[0]==="--version") { process.stdout.write("openshell 0 if (a[0]==="sandbox" && a[1]==="list") { process.stdout.write("\\n"); process.exit(0); } if (a[0]==="sandbox" && a[1]==="delete") { process.exit(0); } if (a[0]==="sandbox" && a[1]==="create" && ${JSON.stringify(failSandboxCreate)}) { process.stderr.write("injected sandbox create failure\\n"); process.exit(1); } -if (a[0]==="sandbox" && a[1]==="get") { process.stderr.write("Error: × Not Found: sandbox not found\\n"); process.exit(1); } +if (a[0]==="sandbox" && a[1]==="get") { process.stderr.write("Error: sandbox ${sandboxName} not found\\n"); process.exit(1); } if (a[0]==="status") { ${healthyTargetStatus} } if (a[0]==="gateway" && a[1]==="info") { process.stdout.write("Gateway Info\\n\\nGateway: ${targetGatewayName}\\nGateway endpoint: https://127.0.0.1:${targetGatewayPort}/\\n"); process.exit(0); } if (a[0]==="gateway" && a[1]==="select") { process.exit(0); }