diff --git a/docs/manage-sandboxes/recover-rebuild-sandboxes.mdx b/docs/manage-sandboxes/recover-rebuild-sandboxes.mdx index e941110406..4d1ec53697 100644 --- a/docs/manage-sandboxes/recover-rebuild-sandboxes.mdx +++ b/docs/manage-sandboxes/recover-rebuild-sandboxes.mdx @@ -168,6 +168,18 @@ If every state directory fails, NemoClaw stops before deleting the original sand `rebuild --force` can continue when no state directory was preserved or a manifest-declared state file failed. NemoClaw restores any entries 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 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, 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. 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 fc07770498..00ee75610b 100644 --- a/docs/reference/commands.mdx +++ b/docs/reference/commands.mdx @@ -2616,7 +2616,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 or a manifest-declared state file failed. NemoClaw restores any captured entries; after a total failure, it recreates from registry metadata only. | +| `--force` | Skip the confirmation prompt and continue when no state directory was preserved or a manifest-declared state file failed. NemoClaw restores any captured entries; 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. | @@ -2632,6 +2632,19 @@ 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 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 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. It also refuses a pending baseline exclusion transaction before opening a shields-down window, starting backup, or deleting the sandbox, and prints the exact `policy exclude` or `policy restore` command to rerun. For a prepared-only transaction, the redacted diagnostic points to `$$nemoclaw mcp remove --force` when the sandbox is still live. 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 b15098f626..fec4dc749b 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 08baed5c53..55c5f5c4b0 100644 --- a/src/lib/actions/sandbox/gateway-state.ts +++ b/src/lib/actions/sandbox/gateway-state.ts @@ -94,6 +94,38 @@ 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)), + ); +} + +/** + * 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, @@ -183,7 +215,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 ( @@ -250,7 +282,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 424e961310..4da9835006 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 8a99f47834..88a5e02465 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 0000000000..bf2c4b8841 --- /dev/null +++ b/src/lib/actions/sandbox/mcp-bridge-rebuild-exec-unavailable.ts @@ -0,0 +1,266 @@ +// 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 { assertAuthenticatedBridgeEntry, validateSandboxName } from "./mcp-bridge-validation"; + +type ReadOnlyValidationSnapshot = { + policyByServer: Map; + providerByServer: Map; + targetsByServer: Map; +}; + +type ExplicitAdapterMcpBridgeEntry = McpBridgeEntry & { adapter: AgentMcpAdapter }; + +type McpOwnershipField = { + label: string; + value: (entry: McpBridgeEntry) => string | undefined; +}; + +export interface ExecUnavailableMcpRebuildPreparation { + entries: McpBridgeEntry[]; + detachedProviderEntries: McpBridgeEntry[]; + scrubbedAdapterEntries: McpBridgeEntry[]; + revalidateBeforeDelete: () => Promise; + 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; + 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.", + ); + } + assertUniqueMcpOwnership(entries); + 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); + // 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(); + 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 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, +): 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 f8321b2fcb..215a38d027 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 d31fdfaae1..8cf4061110 100644 --- a/src/lib/actions/sandbox/mcp-bridge.ts +++ b/src/lib/actions/sandbox/mcp-bridge.ts @@ -16,6 +16,7 @@ import { } from "./mcp-bridge-destroy"; import { redactBridgeSecretsForDisplay } from "./mcp-bridge-output"; import { + type McpRebuildPreparation, prepareMcpBridgesForAbsentSandboxRebuild as prepareMcpBridgesForAbsentSandboxRebuildLifecycle, prepareMcpBridgesForRebuild as prepareMcpBridgesForRebuildLifecycle, reattachMcpProvidersAfterRebuildAbort as reattachMcpProvidersAfterRebuildAbortLifecycle, @@ -69,6 +70,7 @@ export { parseMcpProviderMetadata, providerDetachChangedState, } from "./mcp-bridge-provider"; +export { prepareMcpBridgesForExecUnavailableRebuild } from "./mcp-bridge-rebuild"; export { buildMcpBridgeProviderName, MCP_SERVER_URL_MAX_LENGTH, @@ -78,6 +80,7 @@ export { validateMcpCredentialEnvName, validateMcpServerName, } from "./mcp-bridge-validation"; +export type { McpRebuildPreparation }; export { statusMcpBridge }; export interface McpDestroyPreparation { @@ -90,12 +93,6 @@ export interface McpDestroyPreparation { destroyAlreadyPending: boolean; } -export interface McpRebuildPreparation { - entries: McpBridgeEntry[]; - detachedProviderEntries: McpBridgeEntry[]; - scrubbedAdapterEntries: McpBridgeEntry[]; -} - export async function addMcpBridge( sandboxName: string, options: McpBridgeAddOptions, 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 e172220aaf..5802553222 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 3c60d85cfc..5926f651f9 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 91affb1c1a..4d3b841bfe 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 76fd49f550..dac36d5609 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-destroy-phase.test.ts b/src/lib/actions/sandbox/rebuild-destroy-phase.test.ts index 3118635a74..cd623ba688 100644 --- a/src/lib/actions/sandbox/rebuild-destroy-phase.test.ts +++ b/src/lib/actions/sandbox/rebuild-destroy-phase.test.ts @@ -5,16 +5,28 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; const mocks = vi.hoisted(() => ({ captureOpenshell: vi.fn(), - getSandbox: vi.fn(), - listSandboxes: vi.fn(), + getSandbox: vi.fn( + (_name: string): { name: string; agent: string; nimContainer?: string | null } | null => null, + ), + listSandboxes: vi.fn(() => ({ sandboxes: [] })), prepareMcpForRebuild: vi.fn(), reattachMcpAfterDeleteFailure: vi.fn(), - removeSandboxRegistryEntryWithReceipt: vi.fn(), - runOpenshell: vi.fn(), - stopNimContainer: vi.fn(), - stopNimContainerByName: vi.fn(), + removeSandboxRegistryEntryWithReceipt: vi.fn(() => null), waitUntil: vi.fn(), warnUnpreservedUserManagedFiles: vi.fn(), + runOpenshell: vi.fn( + ( + _args: string[], + ): { + status: number | null; + stdout: string; + stderr: string; + error?: NodeJS.ErrnoException; + signal?: NodeJS.Signals | null; + } => ({ status: 0, stdout: "", stderr: "" }), + ), + stopNimContainer: vi.fn(), + stopNimContainerByName: vi.fn(), })); vi.mock("../../adapters/openshell/runtime", () => ({ @@ -31,7 +43,8 @@ vi.mock("../../inference/nim", () => ({ stopNimContainerByName: mocks.stopNimContainerByName, })); -vi.mock("../../state/registry", () => ({ +vi.mock("../../state/registry", async (importOriginal) => ({ + ...(await importOriginal()), getSandbox: mocks.getSandbox, listSandboxes: mocks.listSandboxes, })); @@ -56,7 +69,7 @@ describe("rebuild destroy phase", () => { vi.clearAllMocks(); vi.spyOn(console, "log").mockImplementation(() => undefined); vi.spyOn(console, "error").mockImplementation(() => undefined); - mocks.getSandbox.mockReturnValue(undefined); + mocks.getSandbox.mockReturnValue(null); mocks.listSandboxes.mockReturnValue({ sandboxes: [] }); mocks.prepareMcpForRebuild.mockResolvedValue({ entries: [], @@ -65,12 +78,24 @@ describe("rebuild destroy phase", () => { }); mocks.reattachMcpAfterDeleteFailure.mockResolvedValue(undefined); mocks.removeSandboxRegistryEntryWithReceipt.mockReturnValue(null); + mocks.captureOpenshell.mockReturnValue({ + status: 1, + output: "", + stdout: "", + stderr: "Error: sandbox alpha not found", + }); + mocks.runOpenshell.mockImplementation((args: string[]) => + args[1] === "get" + ? { status: 1, stdout: "", stderr: "Error: sandbox alpha not found" } + : { status: 0, stdout: "", stderr: "" }, + ); mocks.waitUntil.mockImplementation( (condition: () => boolean) => condition() || condition() || condition(), ); }); afterEach(() => { + vi.unstubAllEnvs(); vi.restoreAllMocks(); }); @@ -146,6 +171,451 @@ describe("rebuild destroy phase", () => { 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), + ); + }); + + 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({ + 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.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", + 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, + ); + }); + it("bounds delete convergence without treating timeout or gateway errors as absence (#7194)", async () => { const { waitUntil: realWaitUntil } = await vi.importActual("../../core/wait"); @@ -169,7 +639,7 @@ describe("rebuild destroy phase", () => { }); expect( - waitForRebuildDeleteAbsence("alpha", vi.fn(), { + waitForRebuildDeleteAbsence("alpha", "nemoclaw", vi.fn(), { captureSandboxGet, now: () => currentMs, sleep, @@ -182,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; @@ -204,7 +735,7 @@ describe("rebuild destroy phase", () => { const result = await runRebuildDestroyPhase({ sandboxName: "alpha", - sandboxEntry: { name: "alpha", agent: "openclaw" }, + sandboxEntry: { name: "alpha", agent: "openclaw", gatewayName: "nemoclaw" }, staleRecovery: false, backupManifest: null, log: vi.fn(), @@ -218,6 +749,11 @@ describe("rebuild destroy phase", () => { expect(result).not.toBeNull(); expect(events).toEqual(["delete", "get-live", "get-missing", "on-deleted", "remove-registry"]); expect(mocks.waitUntil).toHaveBeenCalledOnce(); + expect(mocks.captureOpenshell).toHaveBeenNthCalledWith( + 1, + ["sandbox", "get", "-g", "nemoclaw", "alpha"], + expect.objectContaining({ timeout: expect.any(Number) }), + ); expect(mocks.removeSandboxRegistryEntryWithReceipt).toHaveBeenCalledWith("alpha"); }); @@ -233,7 +769,7 @@ describe("rebuild destroy phase", () => { await expect( runRebuildDestroyPhase({ sandboxName: "alpha", - sandboxEntry: { name: "alpha", agent: "openclaw" }, + sandboxEntry: { name: "alpha", agent: "openclaw", gatewayName: "nemoclaw" }, staleRecovery: false, backupManifest: { backupPath: "/tmp/rebuild-backups/alpha/backup" } as never, log: vi.fn(), diff --git a/src/lib/actions/sandbox/rebuild-destroy-phase.ts b/src/lib/actions/sandbox/rebuild-destroy-phase.ts index e28493d4f0..f96d74e581 100644 --- a/src/lib/actions/sandbox/rebuild-destroy-phase.ts +++ b/src/lib/actions/sandbox/rebuild-destroy-phase.ts @@ -7,9 +7,12 @@ import { G, R } from "../../cli/terminal-style"; import { waitUntil } from "../../core/wait"; 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 { 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"; @@ -33,14 +36,21 @@ export interface RebuildDestroyPhaseInput { log: RebuildLog; bail: RebuildBail; relockShieldsIfNeeded: (sandboxStillExists: boolean) => boolean; + 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 }; + interface RebuildDeleteAbsenceDeps { captureSandboxGet?: ( sandboxName: string, @@ -51,6 +61,7 @@ interface RebuildDeleteAbsenceDeps { stdout?: string; stderr?: string; error?: Error; + signal?: NodeJS.Signals | null; }; now?: () => number; sleep?: (milliseconds: number) => void; @@ -59,12 +70,11 @@ 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( sandboxName: string, + gatewayName: string, log: RebuildLog, deps: RebuildDeleteAbsenceDeps = {}, ): boolean { @@ -73,7 +83,7 @@ export function waitForRebuildDeleteAbsence( const captureSandboxGet = deps.captureSandboxGet ?? ((name: string, timeoutMs: number) => { - const probe = captureOpenshell(["sandbox", "get", name], { + const probe = captureOpenshell(["sandbox", "get", "-g", gatewayName, name], { ignoreError: true, includeStderr: true, includeStreams: true, @@ -92,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" @@ -114,7 +125,57 @@ export function waitForRebuildDeleteAbsence( } /** - * Detach owned MCP state, stop inference, and delete the old sandbox. + * 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 || 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 && isExplicitMissingSandboxGatewayOutput(probeOutput, sandboxName)) { + 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, 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. */ @@ -131,6 +192,7 @@ export async function runRebuildDestroyPhase( validateAfterMcpPreparation, onDeleted, } = input; + const gatewayName = resolveSandboxGatewayName(input.sandboxEntry); if (blockRebuildOnPendingBaselineTransition(input.sandboxEntry, sandboxName, bail)) return null; @@ -142,8 +204,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(sandboxName, staleRecovery, relockShieldsIfNeeded, bail), + 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 @@ -177,15 +265,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; @@ -193,38 +276,86 @@ export async function runRebuildDestroyPhase( const rebuildDetachedMcpProviderEntries = mcpPreparation.detachedProviderEntries; const rebuildScrubbedMcpAdapterEntries = mcpPreparation.scrubbedAdapterEntries; - log(`Running: openshell sandbox delete ${sandboxName}`); - const deleteResult = runOpenshell(["sandbox", "delete", sandboxName], { + // 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. + // 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?.(); + 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 -g ${gatewayName} ${sandboxName}`); + const deleteResult = runOpenshell(["sandbox", "delete", "-g", gatewayName, sandboxName], { ignoreError: true, stdio: ["ignore", "pipe", "pipe"], }); 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) { + let deletionConfirmed = alreadyGone; + 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."); + deletionConfirmed = true; + } 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; } - const deletionConfirmed = alreadyGone || waitForRebuildDeleteAbsence(sandboxName, log); + deletionConfirmed ||= waitForRebuildDeleteAbsence(sandboxName, gatewayName, log); if (!deletionConfirmed) { console.error( " Sandbox delete was accepted, but OpenShell did not confirm that the sandbox is absent.", @@ -236,6 +367,7 @@ export async function runRebuildDestroyPhase( bail("Sandbox deletion could not be confirmed."); return null; } + stopNimBestEffort(); onDeleted(); let removalReceipt: registry.SandboxRemovalReceipt | null = null; const hasBaselineExclusions = (input.sandboxEntry.baselineExclusions?.length ?? 0) > 0; 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 47c56f4f78..0eb5dcef86 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; @@ -193,7 +193,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-mcp-phase.test.ts b/src/lib/actions/sandbox/rebuild-mcp-phase.test.ts index 77c4f17c6a..5ec7cf3ef2 100644 --- a/src/lib/actions/sandbox/rebuild-mcp-phase.test.ts +++ b/src/lib/actions/sandbox/rebuild-mcp-phase.test.ts @@ -1,14 +1,216 @@ // 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(), + executeSandboxExecCommand: 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(), +})); + +vi.mock("./process-recovery", () => ({ + executeSandboxCommand: mocks.executeSandboxCommand, + executeSandboxExecCommand: mocks.executeSandboxExecCommand, +})); + +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.executeSandboxExecCommand.mockReturnValue({ status: 0, stdout: "", stderr: "" }); + mocks.prepareAbsent.mockResolvedValue(emptyPreparation); + mocks.prepareExecUnavailable.mockResolvedValue(emptyPreparation); + mocks.prepareLive.mockResolvedValue(emptyPreparation); + }); + + 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 => { + throw new Error(message); + }); + + await expect(prepareMcpForRebuild("alpha", false, true, relock, bail)).resolves.toEqual( + emptyPreparation, + ); + + 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(); + }); + + it.each([ + 1, 64, 126, 127, 255, + ])("routes every nonzero exec result (%i) through explicit force recovery (#7062)", async (status) => { + mocks.executeSandboxExecCommand.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(); + }); + + 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.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.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 => { + 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(mocks.prepareAbsent).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.executeSandboxExecCommand).not.toHaveBeenCalled(); + expect(mocks.executeSandboxCommand).not.toHaveBeenCalled(); + expect(mocks.prepareLive).toHaveBeenCalledWith("alpha"); + expect(mocks.prepareExecUnavailable).not.toHaveBeenCalled(); + 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.executeSandboxExecCommand).not.toHaveBeenCalled(); + expect(mocks.executeSandboxCommand).not.toHaveBeenCalled(); + expect(mocks.prepareAbsent).toHaveBeenCalledWith("alpha"); + expect(mocks.prepareExecUnavailable).not.toHaveBeenCalled(); + 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 7c25568d3d..7cb1ed1497 100644 --- a/src/lib/actions/sandbox/rebuild-mcp-phase.ts +++ b/src/lib/actions/sandbox/rebuild-mcp-phase.ts @@ -9,21 +9,61 @@ import * as registry from "../../state/registry"; import type { ToolDisclosure } from "../../tool-disclosure"; import { prepareMcpBridgesForAbsentSandboxRebuild, + prepareMcpBridgesForExecUnavailableRebuild, prepareMcpBridgesForRebuild, reattachMcpProvidersAfterRebuildAbort, restoreMcpBridgesAfterRebuild, } from "./mcp-bridge"; +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 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( sandboxName: string, staleRecovery: boolean, + force: boolean, relockShieldsIfNeeded: (sandboxStillExists: boolean) => boolean, - bail: (message: string, code?: number) => never, + bail: RebuildBail, ): Promise { + // 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 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) { + 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 6db604bd0f..f28d95f83c 100644 --- a/src/lib/actions/sandbox/rebuild-pipeline.ts +++ b/src/lib/actions/sandbox/rebuild-pipeline.ts @@ -148,6 +148,7 @@ async function rebuildSandboxUnlocked( relock: relockShieldsIfNeeded, } = shieldsPhase; let sandboxStillExists = true; + let sandboxExistenceAmbiguous = false; try { const preDeleteRecovery = revalidatePreparedRecoveryBeforeDelete( @@ -214,6 +215,7 @@ async function rebuildSandboxUnlocked( sandboxEntry, staleRecovery, backupManifest: backup.backupManifest, + force: normalized.force, log, bail, relockShieldsIfNeeded, @@ -252,6 +254,9 @@ async function rebuildSandboxUnlocked( onDeleted: () => { sandboxStillExists = false; }, + onDeleteStateAmbiguous: () => { + sandboxExistenceAmbiguous = true; + }, }); if (!mcpPreparation) return; registryRollback.recordRemoval(mcpPreparation.removalReceipt); @@ -343,7 +348,9 @@ async function rebuildSandboxUnlocked( bail, }); } finally { - if (!rebuildShieldsWindow.relocked) relockShieldsIfNeeded(sandboxStillExists); + if (!rebuildShieldsWindow.relocked && !sandboxExistenceAmbiguous) { + relockShieldsIfNeeded(sandboxStillExists); + } } } finally { runBestEffortRebuildCleanup( diff --git a/src/lib/actions/sandbox/rebuild-prepared-recovery.test.ts b/src/lib/actions/sandbox/rebuild-prepared-recovery.test.ts index 1cdd09251c..ff683736b4 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/src/lib/actions/sandbox/rebuild-resume-snapshot.test.ts b/src/lib/actions/sandbox/rebuild-resume-snapshot.test.ts index 8f0ca4238e..36e0aae14f 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/gateway-state-reconcile-2276.test.ts b/test/gateway-state-reconcile-2276.test.ts index 40679dd55d..00317cda23 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/helpers/rebuild-dcode-flow-helpers.ts b/test/helpers/rebuild-dcode-flow-helpers.ts index ecca0cf758..1b8a783726 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 8ac165c830..0a24c392d0 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 0000000000..e8a23e9d7b --- /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 9bfb10ac9b..4ff020a2e1 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; @@ -30,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)) { @@ -210,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", { @@ -225,10 +229,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 +262,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(); }); @@ -286,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", { @@ -301,10 +302,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-harness.ts b/test/helpers/rebuild-flow-harness.ts index d1b92dd0f2..3600c49f0f 100644 --- a/test/helpers/rebuild-flow-harness.ts +++ b/test/helpers/rebuild-flow-harness.ts @@ -546,12 +546,15 @@ 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) => { 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-lifecycle-cases.ts b/test/helpers/rebuild-flow-lifecycle-cases.ts index 0a9540643a..af32d4ade6 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, vi } 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 once, recreates, restores, reapplies policy, and relocks on a successful OpenClaw rebuild", async ({ @@ -73,7 +70,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( @@ -98,7 +95,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], @@ -254,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({ @@ -276,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); }); diff --git a/test/helpers/rebuild-flow-recovery-cases.ts b/test/helpers/rebuild-flow-recovery-cases.ts index 61d8dfb588..1aba143173 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(); }); @@ -462,10 +448,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 -g nemoclaw 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 undefined; + }, }); await expect( diff --git a/test/helpers/rebuild-flow-target-credentials-cases.ts b/test/helpers/rebuild-flow-target-credentials-cases.ts index ee53b749e0..8832466fd7 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 a0ab3cb796..7249cc78c4 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, @@ -237,10 +238,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 { @@ -299,10 +297,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 { @@ -348,7 +343,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 { @@ -370,7 +365,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 94b82fde15..7a6131d653 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/helpers/rebuild-flow-test-harness.ts b/test/helpers/rebuild-flow-test-harness.ts index 60fcf420cd..672441e235 100644 --- a/test/helpers/rebuild-flow-test-harness.ts +++ b/test/helpers/rebuild-flow-test-harness.ts @@ -390,7 +390,12 @@ 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") { + const overrideResult = overrides.runOpenshell?.(argv); + if (overrideResult) return overrideResult; + if ( + argv.join(" ") === "sandbox get alpha" || + argv.join(" ") === "sandbox get -g nemoclaw alpha" + ) { return { status: 1, output: "sandbox alpha not found", @@ -398,7 +403,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 captureOpenshellSpy = vi .spyOn(openshellRuntime, "captureOpenshell") @@ -406,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, diff --git a/test/helpers/rebuild-flow-test-support.ts b/test/helpers/rebuild-flow-test-support.ts index 9dff69238e..195f3a5e8f 100644 --- a/test/helpers/rebuild-flow-test-support.ts +++ b/test/helpers/rebuild-flow-test-support.ts @@ -86,12 +86,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; captureOpenshell?: ( args: string[], options?: Record, diff --git a/test/mcp-destroy-lifecycle.test.ts b/test/mcp-destroy-lifecycle.test.ts index 9372ab2028..b47fae403a 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(() => { @@ -24,6 +25,7 @@ const testState = vi.hoisted(() => { adapterRegistered: true, applyPresetContent: vi.fn(), calls: [] as string[], + captureOpenshell: vi.fn(), executeGatewaySupervisorAction: vi.fn(), executeSandboxCommand: vi.fn(), executeSandboxExecCommand: vi.fn(), @@ -33,11 +35,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 +52,16 @@ 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()), + captureOpenshell: testState.captureOpenshell, + runOpenshell: testState.runOpenshell, +})); + vi.mock("../src/lib/gateway-runtime-action", () => ({ recoverNamedGatewayRuntime: testState.recoverNamedGatewayRuntime, })); @@ -61,7 +78,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 +120,26 @@ 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; + expect(isAgentMcpAdapter(adapter), "MCP policy fixture requires an explicit adapter").toBe(true); + 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 as AgentMcpAdapter, + resolvedAddresses, + ), sourcePath: "generated:nemoclaw-mcp-bridge", }; } @@ -158,6 +203,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 +220,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" }; @@ -249,6 +298,7 @@ beforeEach(() => { }); testState.executeSandboxExecCommand.mockImplementation((_sandbox: string, command: string) => { + 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"); @@ -264,6 +314,7 @@ beforeEach(() => { ); return { status: + isNoopProbe || proof.includes("allow_all_known_mcp_methods") || proof.includes('[ -z "${') || proof.includes("openshell:resolve:env:GITHUB_TOKEN") || @@ -367,6 +418,559 @@ 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.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", + 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.executeSandboxExecCommand.mockReturnValue(null); + testState.runOpenshell.mockImplementation((args: string[]) => + args[1] === "delete" + ? { status: 0, stdout: "", stderr: "" } + : { status: 1, stdout: "", stderr: "Error: sandbox alpha not found" }, + ); + testState.captureOpenshell.mockReturnValue({ + status: 1, + output: "", + stdout: "", + stderr: "Error: sandbox alpha not found", + }); + 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.executeSandboxExecCommand).toHaveBeenCalledOnce(); + expect(testState.executeSandboxExecCommand).toHaveBeenCalledWith("alpha", ":", undefined, { + allowLocalDockerFallback: false, + }); + expect(testState.executeSandboxCommand).toHaveBeenCalledWith("alpha", ":"); + expect(testState.runOpenshell).toHaveBeenCalledWith( + ["sandbox", "delete", "-g", "nemoclaw", "alpha"], + expect.any(Object), + ); + expect(testState.captureOpenshell).toHaveBeenCalledWith( + ["sandbox", "get", "-g", "nemoclaw", "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.executeSandboxExecCommand.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.executeSandboxExecCommand.mockReturnValue(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", diff --git a/test/rebuild-credential-preflight.test.ts b/test/rebuild-credential-preflight.test.ts index aa499cd40c..860f246511 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 e8157f3f15..42e2e5163c 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, @@ -140,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); } @@ -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(); });