diff --git a/docs/deployment/set-up-mcp-bridge.mdx b/docs/deployment/set-up-mcp-bridge.mdx index 2238928f42e..e155b5f858c 100644 --- a/docs/deployment/set-up-mcp-bridge.mdx +++ b/docs/deployment/set-up-mcp-bridge.mdx @@ -275,6 +275,28 @@ Provider deletion and registry cleanup happen only after OpenShell confirms that NemoClaw prechecks the recorded provider ID and credential-key shape before mutation and uses a random per-add provider-name suffix to avoid accidental name reuse. The stable OpenShell limitations section describes why these ownership checks do not form an atomic identity binding. +An interrupted MCP destroy can leave durable transaction state. +A prepared-only transaction means deletion is not durably confirmed. +If the sandbox is still live, recover without destroying it by removing the affected server with `--force`. + +```bash +$$nemoclaw my-sandbox mcp remove --force +``` + +NemoClaw clears the prepared marker only after cleanup succeeds without residuals and no bridge entries remain. +For a sandbox with multiple bridge entries, repeat the command with each registered server name until the manifest is empty. +A failed cleanup, a wrong server name, residual resources, or any remaining bridge entry preserves the marker for another retry. + +A pending marker, including a transaction with both prepared and pending markers, means the registry records that OpenShell deletion was already confirmed. +`mcp remove --force` refuses this state because provider or policy cleanup can still be owed. +Finish the idempotent destroy instead. + +```bash +$$nemoclaw my-sandbox destroy +``` + +While either marker remains, `rebuild` refuses during preflight before backup or deletion and prints a redacted diagnostic with the applicable recovery command. + `remove --force` may remove a modified same-name agent adapter entry so an operator can clear local config. Provider deletion still requires the exact recorded provider ID, type, and credential key, and policy deletion still requires exact owned content; force never claims an unowned or drifted provider or same-key live policy. If any cleanup step leaves a residual, the command exits nonzero and preserves the managed MCP registry entry so cleanup can be retried. diff --git a/docs/reference/commands.mdx b/docs/reference/commands.mdx index 82d599bb40a..23705ec9e33 100644 --- a/docs/reference/commands.mdx +++ b/docs/reference/commands.mdx @@ -1855,13 +1855,22 @@ The command fails closed on observed drift. Residuals preserve registry state. OpenShell `0.0.72` mutates providers by name, so do not concurrently replace a managed provider through another OpenShell client during this command. +When an interrupted destroy leaves a prepared-only transaction, deletion is not durably confirmed. +If the sandbox is still live, run `$$nemoclaw mcp remove --force` with the affected server name. +NemoClaw clears the prepared marker only after cleanup succeeds without residuals and no bridge entries remain. +A failed cleanup, a wrong server name, residual resources, or any remaining bridge entry preserves the marker for another retry. + +A pending marker, including a transaction with both prepared and pending markers, means the registry records that OpenShell deletion was already confirmed. +`mcp remove --force` refuses this state. +Run `$$nemoclaw destroy` to finish the idempotent provider and policy cleanup. + ```bash $$nemoclaw my-assistant mcp remove github [--force] ``` | Flag | Description | |------|-------------| -| `--force` | Remove same-name adapter config and continue exact-ownership provider/policy cleanup; preserve registry state when residuals remain | +| `--force` | Remove same-name adapter config and continue exact-ownership provider and policy cleanup. For a prepared-only destroy, attempt recovery when the sandbox is still live and clear the marker only after residual-free cleanup drains every bridge entry. | ### `$$nemoclaw skill install ` @@ -2235,6 +2244,9 @@ Pass `--yes`, `-y`, or `--force` to skip the prompt in scripted workflows. The sandbox must be running for the backup step to succeed. If an archive command reports partial output while still producing usable data, `rebuild` keeps the captured backup entries and reports only the manifest-defined paths that could not be archived. If any required state path still cannot be backed up, `rebuild` exits before destroying the original sandbox. +Before backup or deletion, `rebuild` also refuses an incomplete MCP destroy transaction. +For a prepared-only transaction, the redacted diagnostic points to `$$nemoclaw mcp remove --force` when the sandbox is still live. +For a pending or both-marker transaction, it points to `$$nemoclaw destroy` because the registry records that OpenShell deletion was already confirmed. Before backup or deletion, rebuild checks the staged messaging configuration for credentials or channel resources already used by another registered sandbox. A conflict aborts with the original sandbox registered and intact so you can resolve the conflict before retrying. When rebuild starts with shields up, NemoClaw opens a 30-minute shields-down window for backup and recreation. diff --git a/src/lib/actions/sandbox/mcp-bridge-remove.ts b/src/lib/actions/sandbox/mcp-bridge-remove.ts index afe9722b5c1..1a4c48f315a 100644 --- a/src/lib/actions/sandbox/mcp-bridge-remove.ts +++ b/src/lib/actions/sandbox/mcp-bridge-remove.ts @@ -24,6 +24,7 @@ import { import { assertMcpDestroyNotPending, bridgeState, + clearMcpDestroyMarkers, ensureSandboxGatewaySelected, getBridgeAdapter, getSandboxAgent, @@ -84,32 +85,100 @@ function assertExactMcpRemoveProvider( } } +// Discriminated outcome of a single `mcp remove` so the caller can prove the +// requested entry was handled. Clearing the transaction marker additionally +// requires that no bridge entries remain: phase one scrubbed/detached every +// bridge, so one successful removal cannot mark a multi-bridge recovery done. +type McpRemovalOutcome = + | "removedTarget" // the requested committed server was cleaned up + | "cancelledPreparedAdd" // an incomplete add for the requested server was cancelled + | "markerOnlyNoEntries" // no bridges remain; a --force removal is a marker-only recovery + | "noMatchingEntry" // bridges remain but not the requested server (no-op / wrong server) + | "residualPreserved"; // force cleanup left residual resources; the entry is preserved + +const PROVEN_MCP_RECOVERY_OUTCOMES: ReadonlySet = new Set([ + "removedTarget", + "cancelledPreparedAdd", + "markerOnlyNoEntries", +]); + +function completedPreparedDestroyRecovery( + sandboxName: string, + outcome: McpRemovalOutcome, +): boolean { + if (!PROVEN_MCP_RECOVERY_OUTCOMES.has(outcome)) return false; + return Object.keys(bridgeState(getSandboxOrThrow(sandboxName))).length === 0; +} + export async function removeMcpBridge( sandboxName: string, server: string, options: { force?: boolean; allowResidual?: boolean } = {}, ): Promise { - return withMcpLifecycleLock(sandboxName, () => - removeMcpBridgeUnlocked(sandboxName, server, options), - ); + return withMcpLifecycleLock(sandboxName, async () => { + // #6376: capture the recoverable prepared-destroy phase BEFORE the removal. + const before = getSandboxOrThrow(sandboxName).mcp; + const recoverPreparedDestroy = + !!options.force && !!before?.destroyPreparedAt && !before?.destroyPendingAt; + const outcome = await removeMcpBridgeUnlocked(sandboxName, server, options); + // Clear the phase-one destroy marker only after the requested entry was + // handled AND no bridge entries remain. A failed removal, wrong-server + // no-op, residual-preserving cleanup, or partial multi-bridge recovery must + // retain the durable marker. `setBridgeState` preserves it across each + // removal until this explicit, phase-aware clear. + if ( + recoverPreparedDestroy && + completedPreparedDestroyRecovery(sandboxName, outcome) && + clearMcpDestroyMarkers(sandboxName) + ) { + console.log( + ` Cleared incomplete MCP destroy transaction on sandbox '${sandboxName}' (--force).`, + ); + } + }); } async function removeMcpBridgeUnlocked( sandboxName: string, server: string, options: { force?: boolean; allowResidual?: boolean } = {}, -): Promise { +): Promise { validateSandboxName(sandboxName); validateMcpServerName(server); const sandbox = getSandboxOrThrow(sandboxName); - assertMcpDestroyNotPending(sandbox); - const entry = bridgeState(sandbox)[server]; + // #6376: `--force` on `mcp remove` is the documented non-destructive recovery + // for a stuck MCP destroy transaction. It is PHASE-AWARE: only the prepared + // (phase-one) marker — in-sandbox scrub + provider detach done, deletion not + // durably confirmed — is recoverable here when the sandbox is still live. In + // that case skip the guard and attempt the requested removal; removeMcpBridge + // clears the marker only after the full bridge manifest is drained, so a + // failure or an absent sandbox preserves the durable retry state. Every other + // state still hits the guard: + // - no `--force`: refuse (the guard's normal behavior); + // - the pending (phase-two) marker: the registry records confirmed OpenShell + // deletion, so the guard points at `nemoclaw destroy`; clearing that + // marker would abandon still-owed provider/policy cleanup. + const recoverPreparedDestroy = + !!options.force && !!sandbox.mcp?.destroyPreparedAt && !sandbox.mcp?.destroyPendingAt; + if (!recoverPreparedDestroy) { + assertMcpDestroyNotPending(sandbox); + } + const currentBridges = bridgeState(sandbox); + const entry = currentBridges[server]; if (!entry) { if (!options.force) { throw new McpBridgeError(`MCP server '${server}' not found on sandbox '${sandboxName}'.`); } + // Distinguish a genuine marker-only recovery (no bridge entries remain, so + // the requested `--force` removal has nothing to clean up beyond the stuck + // marker) from a wrong-server no-op (other entries exist but not this one). + // Only the former is a proven recovery that may clear the destroy marker. + if (Object.keys(currentBridges).length === 0) { + console.log(` No MCP servers are registered on sandbox '${sandboxName}'.`); + return "markerOnlyNoEntries"; + } console.log(` No MCP server '${server}' is registered on sandbox '${sandboxName}'.`); - return; + return "noMatchingEntry"; } if (entry.addState === "prepared") { // `prepared` is persisted before gateway selection and is advanced only @@ -118,7 +187,7 @@ async function removeMcpBridgeUnlocked( // state another workflow may own. removeBridgeEntry(sandboxName, server); console.log(` Cancelled incomplete MCP add for '${server}' on sandbox '${sandboxName}'.`); - return; + return "cancelledPreparedAdd"; } // Cleanup follows the adapter persisted with the bridge. Requiring the // sandbox's current agent to still advertise MCP support would strand old @@ -333,8 +402,11 @@ async function removeMcpBridgeUnlocked( `MCP force cleanup left residual resources for '${server}'. The registry entry was preserved so cleanup can be retried.`, ); } - return; + // allowResidual: the caller accepted leftover resources. This is NOT a proven + // recovery — residual state remains — so the destroy marker must be preserved. + return "residualPreserved"; } removeBridgeEntry(sandboxName, server); console.log(` Removed MCP server '${server}' from sandbox '${sandboxName}'.`); + return "removedTarget"; } diff --git a/src/lib/actions/sandbox/mcp-bridge-state.ts b/src/lib/actions/sandbox/mcp-bridge-state.ts index 8a128683376..bafb6335c8e 100644 --- a/src/lib/actions/sandbox/mcp-bridge-state.ts +++ b/src/lib/actions/sandbox/mcp-bridge-state.ts @@ -11,6 +11,7 @@ import { MCP_BRIDGE_POLICY_SOURCE, McpBridgeError, } from "./mcp-bridge-contracts"; +import { validateSandboxName } from "./mcp-bridge-validation"; export function nowIso(): string { return new Date().toISOString(); @@ -99,11 +100,96 @@ export function setBridgeState(sandboxName: string, bridges: Record --force\`.`, ); } +/** + * Non-destructive recovery for a stuck MCP destroy transaction — PHASE-AWARE. + * + * When a prior destroy leaves a `destroyPreparedAt` marker behind (phase one: + * in-sandbox scrub + provider detach done, deletion not durably confirmed) + * every MCP command is refused by `assertMcpDestroyNotPending`, and rebuild + * refuses up front with the same guard. Before #6376 the only advertised + * recovery was `nemoclaw destroy` — full sandbox destruction. + * + * This helper clears ONLY the prepared (phase-one) marker, in place, so a + * `--force` caller can attempt the requested removal if the sandbox still + * exists. It deliberately refuses the pending (phase-two) marker: that marker + * records confirmed OpenShell deletion and is the durable retry state that + * keeps still-owed provider/policy cleanup idempotent. Erasing it would silently + * abandon that cleanup, so a pending transaction must be finished with + * `nemoclaw destroy`, not cleared. + * + * Callers clear the prepared marker only AFTER the requested removal succeeds + * (see removeMcpBridge), so a failed recovery preserves the retry marker. + * `setBridgeState` preserves the marker across the removal's own writes until + * then. + * + * Returns whether the marker was actually cleared, so callers can log + * accurately (no-op vs. cleared). + * + * Product contract (#6376), intentionally narrow: + * invalidState: a crash/abort mid-destroy leaves durable `destroyPreparedAt` + * and/or `destroyPendingAt` markers that fail every MCP command and rebuild. + * sourceBoundary: the markers are host-owned registry state; the sandbox does + * not write them. `destroyPreparedAt` = deletion is not durably confirmed + * (recoverable if still live); `destroyPendingAt` = the registry records + * confirmed OpenShell deletion (not recoverable in place — global + * provider/policy cleanup is still owed). + * sourceFixConstraint: there is no safe non-destructive reconciliation for the + * pending/both-marker live state, so this helper refuses it rather than + * guess. Prepared-only markers are recoverable with `mcp remove --force`; + * pending/both-marker state must finish `nemoclaw destroy`. + * regressionTest: mcp-bridge-destroy-marker-recovery.test.ts (phase-aware + * clear/refuse, clear-only-after-proven-recovery, preserve-on-failure) and + * mcp-destroy-lifecycle.test.ts (phase-aware guard message). + * removalCondition: revisit if a safe pending-phase reconciliation is designed + * (proving the still-owed provider/policy cleanup is complete) — then this + * refusal could be relaxed. + */ +export function clearMcpDestroyMarkers(sandboxName: string): boolean { + // Validate the name before any registry read/update — this helper mutates + // durable state and must not trust an unvalidated identifier. + validateSandboxName(sandboxName); + const sandbox = registry.getSandbox(sandboxName); + const mcpState = sandbox?.mcp; + if (!mcpState?.destroyPreparedAt && !mcpState?.destroyPendingAt) return false; + if (mcpState.destroyPendingAt) { + throw new McpBridgeError( + `Sandbox '${sandboxName}' is mid-destroy past the point of no return — the registry records that OpenShell deletion was already confirmed. Run \`nemoclaw ${sandboxName} destroy\` to finish cleanup; the pending-destroy marker cannot be cleared non-destructively.`, + ); + } + const bridges = mcpState.bridges ?? {}; + const managedServerNames = mcpState.managedServerNames ?? []; + const updated = registry.updateSandbox(sandboxName, { + mcp: + Object.keys(bridges).length > 0 || managedServerNames.length > 0 + ? { + bridges, + ...(managedServerNames.length > 0 ? { managedServerNames } : {}), + } + : undefined, + }); + if (!updated) { + throw new McpBridgeError( + `Could not clear incomplete MCP destroy markers for sandbox '${sandboxName}'.`, + ); + } + return true; +} + export function assertNoDerivedResourceCollision( sandbox: SandboxEntry, server: string, diff --git a/src/lib/actions/sandbox/rebuild-preflight-confirmation.test.ts b/src/lib/actions/sandbox/rebuild-preflight-confirmation.test.ts index bcec96372d6..04ce99bffc3 100644 --- a/src/lib/actions/sandbox/rebuild-preflight-confirmation.test.ts +++ b/src/lib/actions/sandbox/rebuild-preflight-confirmation.test.ts @@ -3,10 +3,12 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import * as openshellResolve from "../../adapters/openshell/resolve"; +import { redact } from "../../security/redact"; import * as sandboxSession from "../../state/sandbox-session"; import { confirmSandboxRebuildIfNeeded, countActiveSandboxSessionsForRebuild, + createRebuildCommandContext, } from "./rebuild-preflight-confirmation"; import { isSingleAgentRebuildSupported } from "./rebuild-preflight-guards"; @@ -57,6 +59,83 @@ describe("rebuild confirmation", () => { }); }); +describe("createRebuildCommandContext bail behaviour (#6376)", () => { + it("prints the bail message to stderr before exiting (non-throw mode)", () => { + // Regression for #6376: the non-throw bail path used to discard its + // `message` argument and just call `process.exit(code)`, so an actionable + // reason (e.g. `Failed to preserve MCP bridges before rebuild: Sandbox + // 'X' has an incomplete MCP destroy transaction ...`) exited 1 with NO + // output, leaving the user without a diagnosis. + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => undefined); + const exitSpy = vi.spyOn(process, "exit").mockImplementation(((code?: number) => { + throw new Error(`process.exit(${code ?? 0})`); + }) as never); + + const ctx = createRebuildCommandContext([], { throwOnError: false }); + expect(() => ctx.bail("Failed to preserve MCP bridges before rebuild: reason X", 1)).toThrow( + "process.exit(1)", + ); + + // The message must reach stderr; `console.error` inherits the pipeline's + // leading two-space rebuild-diagnostic prefix. + expect(errorSpy).toHaveBeenCalledWith( + " Failed to preserve MCP bridges before rebuild: reason X", + ); + expect(exitSpy).toHaveBeenCalledWith(1); + }); + + it("still exits with the requested code even when the message is empty (backward compat)", () => { + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => undefined); + const exitSpy = vi.spyOn(process, "exit").mockImplementation(((code?: number) => { + throw new Error(`process.exit(${code ?? 0})`); + }) as never); + + const ctx = createRebuildCommandContext([], { throwOnError: false }); + expect(() => ctx.bail("", 2)).toThrow("process.exit(2)"); + + // Empty messages must NOT be printed as a bare two-space line — + // silence is fine when the caller passed nothing meaningful. + expect(errorSpy).not.toHaveBeenCalled(); + expect(exitSpy).toHaveBeenCalledWith(2); + }); + + it("keeps the throw-on-error path lossless (bail message becomes the Error)", () => { + // Belt-and-suspenders: the other consumer of createRebuildCommandContext + // (test / in-process callers) still gets the message via `throw new Error`. + const ctx = createRebuildCommandContext([], { throwOnError: true }); + expect(() => ctx.bail("carried reason", 1)).toThrow("carried reason"); + }); + + it("routes the surfaced bail message through the redaction boundary (#6376)", () => { + // The bail message can wrap a lower-level error (`bail("...: " + err.message)`) + // that carries a URL/token; the new stderr path must not become the one place + // rebuild leaks a secret. It must apply the same `redact` boundary `log` uses. + const raw = + "Failed to preserve MCP bridges before rebuild: probe https://hub.example.test/v1?api_key=SUPERSECRETTOKEN123 failed"; + const redacted = redact(raw); + // Sanity: the chosen message actually contains something the boundary scrubs, + // so this test is meaningful regardless of redact's exact patterns. + expect(redacted).not.toBe(raw); + expect(redacted).not.toContain("SUPERSECRETTOKEN123"); + + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => undefined); + const exitSpy = vi.spyOn(process, "exit").mockImplementation(((code?: number) => { + throw new Error(`process.exit(${code ?? 0})`); + }) as never); + + const ctx = createRebuildCommandContext([], { throwOnError: false }); + expect(() => ctx.bail(raw, 1)).toThrow("process.exit(1)"); + + // What reached stderr is the redacted form, with the two-space prefix ... + expect(errorSpy).toHaveBeenCalledWith(` ${redacted}`); + // ... and the raw secret never surfaced. + for (const call of errorSpy.mock.calls) { + expect(String(call[0])).not.toContain("SUPERSECRETTOKEN123"); + } + expect(exitSpy).toHaveBeenCalledWith(1); + }); +}); + describe("rebuild preflight guards", () => { it("rejects a multi-agent sandbox before later rebuild work", () => { const error = vi.spyOn(console, "error").mockImplementation(() => undefined); diff --git a/src/lib/actions/sandbox/rebuild-preflight-confirmation.ts b/src/lib/actions/sandbox/rebuild-preflight-confirmation.ts index ca551e78ebe..1d648180d4d 100644 --- a/src/lib/actions/sandbox/rebuild-preflight-confirmation.ts +++ b/src/lib/actions/sandbox/rebuild-preflight-confirmation.ts @@ -44,7 +44,23 @@ export function createRebuildCommandContext( ? (message: string) => { throw new Error(message); } - : (_message: string, code = 1) => process.exit(code), + : // #6376: previously discarded `message` entirely, so any bail() call + // that raised an actionable reason (e.g. `Failed to preserve MCP + // bridges before rebuild: Sandbox 'X' has an incomplete MCP destroy + // transaction. Re-run the sandbox destroy command …`) exited 1 with + // no output at all — leaving the user with the last stage's spinner + // line and no diagnosis. Emit the reason on stderr before exit so + // `$?`-gated automation and interactive users see WHY rebuild + // aborted. The message can carry a wrapped lower-level error + // (`bail("...: " + error.message)`), so route it through the same + // `redact` boundary `log` already uses (line above) before surfacing — + // a bailed rebuild must not be the one path that leaks a URL/token. + // `console.error` inherits the rebuild-diagnostic formatting (leading + // two spaces). + (message: string, code = 1) => { + if (message) console.error(` ${redact(message)}`); + process.exit(code); + }, }; } diff --git a/src/lib/actions/sandbox/rebuild-preflight-phase.ts b/src/lib/actions/sandbox/rebuild-preflight-phase.ts index 0b54620e08a..a874840fd24 100644 --- a/src/lib/actions/sandbox/rebuild-preflight-phase.ts +++ b/src/lib/actions/sandbox/rebuild-preflight-phase.ts @@ -5,6 +5,7 @@ import type { RebuildSandboxOptions } from "../../domain/lifecycle/options"; import type { SandboxMessagingPlan } from "../../messaging"; import { hydrateCredentialEnv } from "../../onboard/credential-env"; import type { RebuildManifest } from "../../state/sandbox"; +import { assertMcpDestroyNotPending } from "./mcp-bridge-state"; import { preflightRebuildCredentials, type RebuildBail, @@ -83,6 +84,19 @@ export async function runRebuildPreflightPhase( const activeSessionCount = countActiveSandboxSessionsForRebuild(sandboxName); const sandboxEntry = getRebuildSandboxEntryOrBail(sandboxName, bail); if (!sandboxEntry) return null; + // #6376: refuse a stuck MCP destroy transaction up front — before backup, + // image prep, or the old-sandbox delete. The only MCP marker check used to + // live inside the destroy phase, which runs AFTER the backup phase, so a + // stuck sandbox paid destructive/backup cost before the guard fired. Moving + // it here fails closed before any destructive work; the guard's message is + // phase-aware (prepared -> non-destructive `mcp remove --force`; pending -> + // finish the destroy). + try { + assertMcpDestroyNotPending(sandboxEntry); + } catch (error) { + bail(error instanceof Error ? error.message : String(error)); + return null; + } const confirmedEntrySnapshot = JSON.stringify(sandboxEntry); const allowLegacyManagedImageRecovery = opts.recoveryManifest !== undefined && opts.allowLegacyManagedImageRecovery === true; diff --git a/test/mcp-bridge-destroy-marker-recovery.test.ts b/test/mcp-bridge-destroy-marker-recovery.test.ts new file mode 100644 index 00000000000..cd11147a393 --- /dev/null +++ b/test/mcp-bridge-destroy-marker-recovery.test.ts @@ -0,0 +1,753 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +// Regression coverage for #6376: `nemoclaw mcp remove --force` +// must recover an incomplete-destroy transaction non-destructively — but only +// the PREPARED (phase-one) marker, where deletion is not durably confirmed. The +// PENDING (phase-two) marker records confirmed OpenShell deletion, so it must +// NOT be cleared here (that would abandon still-owed provider/policy cleanup). +// The prepared marker is cleared only AFTER the removal succeeds, so a failed +// recovery preserves the durable retry state. + +import { spawnSync } from "node:child_process"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; + +import { afterEach, describe, expect, it } from "vitest"; + +const sourceRequireHook = path.resolve("test/helpers/onboard-script-mocks.cjs"); +const sourceNodeOptions = [process.env.NODE_OPTIONS, `--require=${sourceRequireHook}`] + .filter(Boolean) + .join(" "); +const tempHomes = new Set(); + +function createTempHome(prefix: string): string { + const home = fs.mkdtempSync(path.join(os.tmpdir(), prefix)); + tempHomes.add(home); + return home; +} + +afterEach(() => { + tempHomes.forEach((home) => fs.rmSync(home, { recursive: true, force: true })); + tempHomes.clear(); +}); + +interface SandboxMcpSnapshot { + bridges: Record; + managedServerNames?: readonly string[]; + destroyPreparedAt?: string; + destroyPendingAt?: string; +} + +function runNodeScript( + home: string, + script: string, +): { status: number | null; stdout: string; stderr: string } { + const result = spawnSync(process.execPath, ["-e", script], { + cwd: process.cwd(), + encoding: "utf8", + env: { ...process.env, HOME: home, NODE_OPTIONS: sourceNodeOptions }, + }); + return { status: result.status, stdout: result.stdout || "", stderr: result.stderr || "" }; +} + +const GITHUB_BRIDGE = `{ + server: "github", + agent: "openclaw", + adapter: "mcporter", + url: "https://mcp.example.test/mcp", + env: [], + policyName: "mcp-bridge-github", + addedAt: "2026-06-01T00:00:00.000Z", +}`; + +const SLACK_BRIDGE = `{ + server: "slack", + agent: "openclaw", + adapter: "mcporter", + url: "https://mcp.example.test/slack", + env: [], + policyName: "mcp-bridge-slack", + addedAt: "2026-06-01T00:00:00.000Z", +}`; + +describe("clearMcpDestroyMarkers — phase-aware (#6376)", () => { + it("clears the prepared (phase-one) marker in place, preserving bridges + managedServerNames", () => { + const home = createTempHome("nemoclaw-clear-prepared-"); + const script = ` +process.env.HOME = ${JSON.stringify(home)}; +const registry = require("./src/lib/state/registry.js"); +const state = require("./src/lib/actions/sandbox/mcp-bridge-state.js"); +registry.registerSandbox({ + name: "stuck-sandbox", + agent: "openclaw", + mcp: { + bridges: { github: ${GITHUB_BRIDGE} }, + managedServerNames: ["github"], + destroyPreparedAt: "2026-06-27T01:00:00.000Z", + }, +}); +const changed = state.clearMcpDestroyMarkers("stuck-sandbox"); +const after = registry.getSandbox("stuck-sandbox"); +process.stdout.write(JSON.stringify({ changed, mcp: after && after.mcp })); +`; + const result = runNodeScript(home, script); + expect(result.status).toBe(0); + const parsed = JSON.parse(result.stdout) as { + changed: boolean; + mcp: SandboxMcpSnapshot | undefined; + }; + expect(parsed.changed).toBe(true); + expect(parsed.mcp?.destroyPreparedAt).toBeUndefined(); + // Marker-only surgery: bridges and managedServerNames are preserved. + expect(parsed.mcp?.bridges).toHaveProperty("github"); + expect(parsed.mcp?.managedServerNames).toEqual(["github"]); + }); + + it("returns false without mutating the registry when no markers are set", () => { + const home = createTempHome("nemoclaw-clear-noop-"); + const script = ` +process.env.HOME = ${JSON.stringify(home)}; +const registry = require("./src/lib/state/registry.js"); +const state = require("./src/lib/actions/sandbox/mcp-bridge-state.js"); +registry.registerSandbox({ + name: "healthy-sandbox", + agent: "openclaw", + mcp: { bridges: { github: ${GITHUB_BRIDGE} }, managedServerNames: ["github"] }, +}); +const before = JSON.stringify(registry.getSandbox("healthy-sandbox")); +const changed = state.clearMcpDestroyMarkers("healthy-sandbox"); +const after = JSON.stringify(registry.getSandbox("healthy-sandbox")); +process.stdout.write(JSON.stringify({ changed, mutated: before !== after })); +`; + const result = runNodeScript(home, script); + expect(result.status).toBe(0); + const parsed = JSON.parse(result.stdout) as { changed: boolean; mutated: boolean }; + expect(parsed.changed).toBe(false); + expect(parsed.mutated).toBe(false); + }); + + it("refuses to clear a pending marker and preserves the complete destroy transaction", () => { + const home = createTempHome("nemoclaw-clear-pending-refuse-"); + const script = ` +process.env.HOME = ${JSON.stringify(home)}; +const registry = require("./src/lib/state/registry.js"); +const state = require("./src/lib/actions/sandbox/mcp-bridge-state.js"); +registry.registerSandbox({ + name: "deleted-sandbox", + agent: "openclaw", + mcp: { + bridges: { github: ${GITHUB_BRIDGE} }, + managedServerNames: ["github"], + destroyPreparedAt: "2026-06-27T01:00:00.000Z", + destroyPendingAt: "2026-06-27T01:05:00.000Z", + }, +}); +let threw = ""; +try { + state.clearMcpDestroyMarkers("deleted-sandbox"); +} catch (error) { + threw = String(error && error.message || error); +} +const after = registry.getSandbox("deleted-sandbox"); +process.stdout.write(JSON.stringify({ threw, mcp: after && after.mcp })); +`; + const result = runNodeScript(home, script); + expect(result.status).toBe(0); + const parsed = JSON.parse(result.stdout) as { + threw: string; + mcp: SandboxMcpSnapshot | undefined; + }; + expect(parsed.threw).toContain("past the point of no return"); + expect(parsed.threw).toContain("nemoclaw deleted-sandbox destroy"); + expect(parsed.mcp?.destroyPreparedAt).toBe("2026-06-27T01:00:00.000Z"); + expect(parsed.mcp?.destroyPendingAt).toBe("2026-06-27T01:05:00.000Z"); + expect(parsed.mcp?.managedServerNames).toEqual(["github"]); + expect(parsed.mcp?.bridges).toHaveProperty("github"); + }); +}); + +describe("mcp remove --force — phase-aware recovery (#6376)", () => { + it("recovers a prepared-only stuck destroy and clears the marker after the removal succeeds", async () => { + const home = createTempHome("nemoclaw-force-prepared-recover-"); + const script = ` +process.env.HOME = ${JSON.stringify(home)}; +const registry = require("./src/lib/state/registry.js"); +registry.registerSandbox({ + name: "stuck-sandbox", + agent: "openclaw", + mcp: { + // Empty bridges — a destroy interrupted after the bridge entry was purged + // but before the marker was cleared. Deletion is not durably confirmed. + bridges: {}, + destroyPreparedAt: "2026-06-27T01:00:00.000Z", + }, +}); +const bridge = require("./src/lib/actions/sandbox/mcp-bridge.js"); +bridge.removeMcpBridge("stuck-sandbox", "github", { force: true }).then( + () => { + const after = registry.getSandbox("stuck-sandbox"); + process.stdout.write("\\n<>" + JSON.stringify({ ok: true, mcp: after && after.mcp })); + process.exit(0); + }, + (error) => { + process.stderr.write(String(error && error.message || error)); + process.exit(1); + }, +); +`; + const result = runNodeScript(home, script); + expect(result.status).toBe(0); + expect(result.stdout).toContain( + "Cleared incomplete MCP destroy transaction on sandbox 'stuck-sandbox'", + ); + const jsonMarker = "<>"; + const jsonPayload = result.stdout.slice(result.stdout.indexOf(jsonMarker) + jsonMarker.length); + const parsed = JSON.parse(jsonPayload) as { + ok: boolean; + mcp: SandboxMcpSnapshot | undefined; + }; + expect(parsed.ok).toBe(true); + expect(parsed.mcp?.destroyPreparedAt).toBeUndefined(); + expect(parsed.mcp?.destroyPendingAt).toBeUndefined(); + }); + + it("clears the prepared marker after removing the final committed bridge (#6376)", () => { + const home = createTempHome("nemoclaw-force-final-bridge-"); + const script = String.raw` +process.env.HOME = ${JSON.stringify(home)}; +const registry = require("./src/lib/state/registry.js"); +const state = require("./src/lib/actions/sandbox/mcp-bridge-state.js"); +const globalActions = require("./src/lib/actions/global.js"); +const policies = require("./src/lib/policy/index.js"); +const processRecovery = require("./src/lib/actions/sandbox/process-recovery.js"); +const expectedId = "11111111-2222-4333-8444-555555555555"; +const providerName = "stuck-sandbox-mcp-github"; +let providerExists = true; +let attached = true; +let policyState = "match"; +const events = []; +const commands = []; +state.ensureSandboxGatewaySelected = async () => {}; +globalActions.runOpenshellProviderCommand = (args) => { + const command = args.join(" "); + commands.push(command); + if (args[0] === "provider" && args[1] === "get") { + events.push(providerExists ? "provider:get:present" : "provider:get:absent"); + return providerExists + ? { + status: 0, + stdout: "Id: " + expectedId + "\nType: generic\nResource version: 4\nCredential keys: EXPECTED_TOKEN\n", + stderr: "", + } + : { status: 1, stdout: "", stderr: "NotFound: provider" }; + } + if (args[0] === "sandbox" && args[1] === "provider" && args[2] === "list") { + events.push(attached ? "provider:list:attached" : "provider:list:detached"); + return { + status: 0, + stdout: attached + ? "NAME TYPE CREDENTIAL_KEYS CONFIG_KEYS\n" + providerName + " generic 1 0\n" + : "No providers attached to sandbox stuck-sandbox.\n", + stderr: "", + }; + } + if (args[0] === "sandbox" && args[1] === "provider" && args[2] === "detach") { + events.push("provider:detach"); + attached = false; + return { + status: 0, + stdout: "Detached provider " + providerName + " from sandbox stuck-sandbox.", + stderr: "", + }; + } + if (args[0] === "provider" && args[1] === "delete") { + events.push("provider:delete"); + providerExists = false; + return { status: 0, stdout: "deleted", stderr: "" }; + } + throw new Error("unexpected OpenShell provider command: " + command); +}; +processRecovery.executeSandboxCommand = (_sandboxName, command) => { + if (!command.includes('spawnSync("mcporter", ["config", "remove"')) { + throw new Error("unexpected sandbox command: " + command); + } + events.push("adapter:remove"); + return { status: 0, stdout: "", stderr: "" }; +}; +processRecovery.executeSandboxExecCommand = (_sandboxName, 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 expectedProof = '[ -z "' + '$' + '{EXPECTED_TOKEN+x}" ]'; + if (!proof.includes(expectedProof)) { + throw new Error("unexpected fresh-exec credential proof: " + proof); + } + events.push("credential:revoked"); + return { status: 0, stdout: "", stderr: "" }; +}; +policies.getPresetContentGatewayState = () => policyState; +policies.removePreset = () => { + events.push("policy:remove"); + policyState = "absent"; + return true; +}; +const entry = { + server: "github", + agent: "openclaw", + adapter: "mcporter", + url: "https://mcp.example.test/mcp", + env: ["EXPECTED_TOKEN"], + providerName, + providerId: expectedId, + policyName: "mcp-bridge-github", + addedAt: "2026-06-01T00:00:00.000Z", +}; +registry.registerSandbox({ + name: "stuck-sandbox", + agent: "openclaw", + mcp: { + bridges: { github: entry }, + managedServerNames: ["github"], + destroyPreparedAt: "2026-06-27T01:00:00.000Z", + }, +}); +registry.addCustomPolicy("stuck-sandbox", { + name: entry.policyName, + content: "network_policies: {}", + sourcePath: "generated:nemoclaw-mcp-bridge", +}); +const bridge = require("./src/lib/actions/sandbox/mcp-bridge.js"); +bridge.removeMcpBridge("stuck-sandbox", "github", { force: true }).then( + () => { + const after = registry.getSandbox("stuck-sandbox"); + process.stdout.write("<>" + JSON.stringify({ + mcp: after && after.mcp, + customPolicies: after && after.customPolicies || [], + events, + commands, + providerExists, + attached, + policyState, + })); + process.exit(0); + }, + (error) => { + process.stderr.write(String(error && error.message || error)); + process.exit(1); + }, +); +`; + const result = runNodeScript(home, script); + expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(0); + const removedLog = result.stdout.indexOf("Removed MCP server 'github'"); + const clearedLog = result.stdout.indexOf("Cleared incomplete MCP destroy transaction"); + expect(removedLog).toBeGreaterThanOrEqual(0); + expect(clearedLog).toBeGreaterThan(removedLog); + expect(result.stderr).not.toContain("MCP force cleanup warnings"); + const jsonMarker = "<>"; + const parsed = JSON.parse( + result.stdout.slice(result.stdout.indexOf(jsonMarker) + jsonMarker.length), + ) as { + mcp: SandboxMcpSnapshot | undefined; + customPolicies: unknown[]; + events: string[]; + commands: string[]; + providerExists: boolean; + attached: boolean; + policyState: string; + }; + expect(parsed.attached).toBe(false); + expect(parsed.providerExists).toBe(false); + expect(parsed.policyState).toBe("absent"); + expect(parsed.customPolicies).toEqual([]); + expect(parsed.mcp?.bridges).toEqual({}); + expect(parsed.mcp?.managedServerNames).toEqual(["github"]); + expect(parsed.mcp?.destroyPreparedAt).toBeUndefined(); + expect(parsed.mcp?.destroyPendingAt).toBeUndefined(); + + const initialProviderInspection = parsed.events.indexOf("provider:get:present"); + const adapterRemoval = parsed.events.indexOf("adapter:remove"); + const providerDetach = parsed.events.indexOf("provider:detach"); + const credentialRevocation = parsed.events.indexOf("credential:revoked"); + const policyRemoval = parsed.events.indexOf("policy:remove"); + const providerDelete = parsed.events.indexOf("provider:delete"); + expect(initialProviderInspection).toBeGreaterThanOrEqual(0); + expect(adapterRemoval).toBeGreaterThan(initialProviderInspection); + expect(providerDetach).toBeGreaterThan(adapterRemoval); + expect(credentialRevocation).toBeGreaterThan(providerDetach); + expect(policyRemoval).toBeGreaterThan(credentialRevocation); + expect(providerDelete).toBeGreaterThan(policyRemoval); + expect( + parsed.events + .slice(policyRemoval + 1, providerDelete) + .filter((event) => event === "provider:get:present"), + ).toHaveLength(2); + expect(parsed.events.slice(providerDelete + 1)).toContain("provider:get:absent"); + expect( + parsed.commands.filter((command) => command === "provider get stuck-sandbox-mcp-github"), + ).toHaveLength(5); + }); + + it("does NOT clear the prepared marker on a wrong-server --force no-op (other entries remain)", async () => { + const home = createTempHome("nemoclaw-force-wrong-server-"); + // PRA-2: `--force` removing a server that is NOT registered (while other + // entries exist) is a no-op, not a proven recovery. The prepared marker must + // survive — clearing it here would drop the retry state on a mistyped name. + const script = ` +process.env.HOME = ${JSON.stringify(home)}; +const registry = require("./src/lib/state/registry.js"); +registry.registerSandbox({ + name: "stuck-sandbox", + agent: "openclaw", + mcp: { + bridges: { github: ${GITHUB_BRIDGE} }, + managedServerNames: ["github"], + destroyPreparedAt: "2026-06-27T01:00:00.000Z", + }, +}); +const bridge = require("./src/lib/actions/sandbox/mcp-bridge.js"); +bridge.removeMcpBridge("stuck-sandbox", "not-registered", { force: true }).then( + () => { + const after = registry.getSandbox("stuck-sandbox"); + process.stdout.write("<>" + JSON.stringify({ ok: true, mcp: after && after.mcp })); + process.exit(0); + }, + (error) => { + process.stderr.write(String(error && error.message || error)); + process.exit(1); + }, +); +`; + const result = runNodeScript(home, script); + expect(result.status).toBe(0); + const jsonMarker = "<>"; + const parsed = JSON.parse( + result.stdout.slice(result.stdout.indexOf(jsonMarker) + jsonMarker.length), + ) as { ok: boolean; mcp: SandboxMcpSnapshot | undefined }; + expect(parsed.ok).toBe(true); + // The no-op did not clear the durable retry marker ... + expect(parsed.mcp?.destroyPreparedAt).toBe("2026-06-27T01:00:00.000Z"); + // ... and the recovery log must NOT have printed. + expect(result.stdout).not.toContain("Cleared incomplete MCP destroy transaction"); + }); + + it("refuses --force on a pending destroy and preserves both markers", async () => { + const home = createTempHome("nemoclaw-force-pending-refuse-"); + const script = ` +process.env.HOME = ${JSON.stringify(home)}; +const registry = require("./src/lib/state/registry.js"); +registry.registerSandbox({ + name: "deleted-sandbox", + agent: "openclaw", + mcp: { + bridges: { github: ${GITHUB_BRIDGE} }, + managedServerNames: ["github"], + destroyPreparedAt: "2026-06-27T01:00:00.000Z", + destroyPendingAt: "2026-06-27T01:05:00.000Z", + }, +}); +const bridge = require("./src/lib/actions/sandbox/mcp-bridge.js"); +bridge.removeMcpBridge("deleted-sandbox", "github", { force: true }).then( + () => { + process.stdout.write("UNEXPECTED_OK"); + process.exit(0); + }, + (error) => { + const after = registry.getSandbox("deleted-sandbox"); + process.stdout.write(JSON.stringify({ error: String(error && error.message || error), mcp: after && after.mcp })); + process.exit(0); + }, +); +`; + const result = runNodeScript(home, script); + expect(result.status).toBe(0); + const parsed = JSON.parse(result.stdout) as { + error: string; + mcp: SandboxMcpSnapshot | undefined; + }; + expect(parsed.error).toContain("past the point of no return"); + expect(parsed.error).toContain("nemoclaw deleted-sandbox destroy"); + expect(parsed.mcp?.destroyPreparedAt).toBe("2026-06-27T01:00:00.000Z"); + expect(parsed.mcp?.destroyPendingAt).toBe("2026-06-27T01:05:00.000Z"); + expect(parsed.mcp?.managedServerNames).toEqual(["github"]); + expect(parsed.mcp?.bridges).toHaveProperty("github"); + // And the fix's "Cleared incomplete MCP destroy transaction" log must NOT appear. + expect(result.stdout).not.toContain("Cleared incomplete MCP destroy transaction"); + }); + + it("PRESERVES the prepared marker when the --force removal itself fails (durable retry state)", async () => { + const home = createTempHome("nemoclaw-force-fail-preserve-"); + // Deterministically fail the removal at the gateway-selection step (before + // any provider/openshell work) by stubbing ensureSandboxGatewaySelected to + // throw. Because the prepared marker is cleared only AFTER a successful + // removal, it must survive this failure — the #6376 blocker was that the + // earlier code cleared markers up front and lost the retry state. + const script = ` +process.env.HOME = ${JSON.stringify(home)}; +const registry = require("./src/lib/state/registry.js"); +const state = require("./src/lib/actions/sandbox/mcp-bridge-state.js"); +// CJS interop: mcp-bridge-remove calls this via the module object, so the +// override is observed at call time. +state.ensureSandboxGatewaySelected = async () => { + throw new Error("gateway unavailable (injected)"); +}; +registry.registerSandbox({ + name: "stuck-sandbox", + agent: "openclaw", + mcp: { + bridges: { + github: { + server: "github", + agent: "openclaw", + adapter: "mcporter", + url: "https://mcp.example.test/mcp", + env: [], + policyName: "mcp-bridge-github", + addedAt: "2026-06-01T00:00:00.000Z", + }, + }, + managedServerNames: ["github"], + destroyPreparedAt: "2026-06-27T01:00:00.000Z", + }, +}); +const bridge = require("./src/lib/actions/sandbox/mcp-bridge.js"); +bridge.removeMcpBridge("stuck-sandbox", "github", { force: true }).then( + () => { + const after = registry.getSandbox("stuck-sandbox"); + process.stdout.write("<>" + JSON.stringify({ threw: false, mcp: after && after.mcp })); + process.exit(0); + }, + (error) => { + const after = registry.getSandbox("stuck-sandbox"); + process.stdout.write("<>" + JSON.stringify({ threw: true, error: String(error && error.message || error), mcp: after && after.mcp })); + process.exit(0); + }, +); +`; + const result = runNodeScript(home, script); + expect(result.status).toBe(0); + const jsonMarker = "<>"; + const jsonPayload = result.stdout.slice(result.stdout.indexOf(jsonMarker) + jsonMarker.length); + const parsed = JSON.parse(jsonPayload) as { + threw: boolean; + error?: string; + mcp: SandboxMcpSnapshot | undefined; + }; + // The removal failed ... + expect(parsed.threw).toBe(true); + expect(parsed.error).toContain("gateway unavailable (injected)"); + // ... so the durable prepared retry marker MUST be preserved for a later + // `sandbox destroy` or retry. + expect(parsed.mcp?.destroyPreparedAt).toBe("2026-06-27T01:00:00.000Z"); + // The success log must NOT have printed. + expect(result.stdout).not.toContain("Cleared incomplete MCP destroy transaction"); + }); + + it("preserves the prepared marker and manifest when forced cleanup tolerates residuals", async () => { + const home = createTempHome("nemoclaw-force-residual-preserve-"); + const script = ` +process.env.HOME = ${JSON.stringify(home)}; +const registry = require("./src/lib/state/registry.js"); +const state = require("./src/lib/actions/sandbox/mcp-bridge-state.js"); +const adapters = require("./src/lib/actions/sandbox/mcp-bridge-adapters.js"); +const policy = require("./src/lib/actions/sandbox/mcp-bridge-policy.js"); +state.ensureSandboxGatewaySelected = async () => {}; +adapters.assertAgentMcpConfigMutationAllowed = () => {}; +adapters.assertAgentMcpTeardownRuntimeCapability = () => {}; +adapters.unregisterAgentAdapter = () => { + throw new Error("adapter cleanup failed (injected)"); +}; +policy.assertGeneratedPolicyMutationSafe = () => {}; +registry.registerSandbox({ + name: "stuck-sandbox", + agent: "openclaw", + mcp: { + bridges: { github: ${GITHUB_BRIDGE} }, + managedServerNames: ["github"], + destroyPreparedAt: "2026-06-27T01:00:00.000Z", + }, +}); +const bridge = require("./src/lib/actions/sandbox/mcp-bridge.js"); +bridge.removeMcpBridge("stuck-sandbox", "github", { force: true, allowResidual: true }).then( + () => { + const after = registry.getSandbox("stuck-sandbox"); + process.stdout.write("<>" + JSON.stringify({ mcp: after && after.mcp })); + process.exit(0); + }, + (error) => { + process.stderr.write(String(error && error.message || error)); + process.exit(1); + }, +); +`; + const result = runNodeScript(home, script); + expect(result.status).toBe(0); + expect(result.stderr).toContain("adapter cleanup failed (injected)"); + const jsonMarker = "<>"; + const parsed = JSON.parse( + result.stdout.slice(result.stdout.indexOf(jsonMarker) + jsonMarker.length), + ) as { mcp: SandboxMcpSnapshot | undefined }; + expect(parsed.mcp?.destroyPreparedAt).toBe("2026-06-27T01:00:00.000Z"); + expect(parsed.mcp?.managedServerNames).toEqual(["github"]); + expect(parsed.mcp?.bridges).toHaveProperty("github"); + expect(result.stdout).not.toContain("Cleared incomplete MCP destroy transaction"); + }); + + it("keeps the prepared marker until every bridge entry is removed", async () => { + const home = createTempHome("nemoclaw-force-multi-bridge-"); + const script = ` +process.env.HOME = ${JSON.stringify(home)}; +const registry = require("./src/lib/state/registry.js"); +const state = require("./src/lib/actions/sandbox/mcp-bridge-state.js"); +const adapters = require("./src/lib/actions/sandbox/mcp-bridge-adapters.js"); +const policy = require("./src/lib/actions/sandbox/mcp-bridge-policy.js"); +state.ensureSandboxGatewaySelected = async () => {}; +adapters.assertAgentMcpConfigMutationAllowed = () => {}; +adapters.assertAgentMcpTeardownRuntimeCapability = () => {}; +adapters.unregisterAgentAdapter = () => "removed"; +policy.assertGeneratedPolicyMutationSafe = () => {}; +policy.removeGeneratedPolicy = () => {}; +registry.registerSandbox({ + name: "stuck-sandbox", + agent: "openclaw", + mcp: { + bridges: { github: ${GITHUB_BRIDGE}, slack: ${SLACK_BRIDGE} }, + managedServerNames: ["github", "slack"], + destroyPreparedAt: "2026-06-27T01:00:00.000Z", + }, +}); +const bridge = require("./src/lib/actions/sandbox/mcp-bridge.js"); +bridge.removeMcpBridge("stuck-sandbox", "github", { force: true }).then( + () => { + const after = registry.getSandbox("stuck-sandbox"); + process.stdout.write("<>" + JSON.stringify({ mcp: after && after.mcp })); + process.exit(0); + }, + (error) => { + process.stderr.write(String(error && error.message || error)); + process.exit(1); + }, +); +`; + const result = runNodeScript(home, script); + expect(result.status).toBe(0); + const jsonMarker = "<>"; + const parsed = JSON.parse( + result.stdout.slice(result.stdout.indexOf(jsonMarker) + jsonMarker.length), + ) as { mcp: SandboxMcpSnapshot | undefined }; + expect(parsed.mcp?.destroyPreparedAt).toBe("2026-06-27T01:00:00.000Z"); + expect(parsed.mcp?.managedServerNames).toEqual(["github", "slack"]); + expect(parsed.mcp?.bridges).not.toHaveProperty("github"); + expect(parsed.mcp?.bridges).toHaveProperty("slack"); + expect(result.stdout).not.toContain("Cleared incomplete MCP destroy transaction"); + }); + + it("rebuild refuses a stuck destroy in the PREFLIGHT phase, before any destructive/backup work (#6376)", async () => { + const home = createTempHome("nemoclaw-rebuild-preflight-marker-"); + // runRebuildPreflightPhase runs before the pipeline's backup and delete + // phases. A stuck marker must throw here (throwOnError mode) so the pipeline + // never reaches backup/delete — recovery is no longer "too late". + const script = ` +process.env.HOME = ${JSON.stringify(home)}; +const registry = require("./src/lib/state/registry.js"); +registry.registerSandbox({ + name: "stuck-sandbox", + agent: "openclaw", + mcp: { bridges: {}, destroyPreparedAt: "2026-06-27T01:00:00.000Z" }, +}); +const preflight = require("./src/lib/actions/sandbox/rebuild-preflight-phase.js"); +preflight.runRebuildPreflightPhase("stuck-sandbox", [], { throwOnError: true }).then( + () => { + process.stdout.write("UNEXPECTED_OK"); + process.exit(0); + }, + (error) => { + process.stdout.write(String(error && error.message || error)); + process.exit(0); + }, +); +`; + const result = runNodeScript(home, script); + expect(result.status).toBe(0); + expect(result.stdout).not.toContain("UNEXPECTED_OK"); + expect(result.stdout).toContain("incomplete MCP destroy transaction"); + // Destructive markers from a real backup/delete must not appear — the guard + // fired first. + expect(result.stdout).not.toContain("Deleting old sandbox"); + }); + + it("refuses a both-marker rebuild before destructive work and preserves destroy guidance", async () => { + const home = createTempHome("nemoclaw-rebuild-preflight-pending-"); + const script = ` +process.env.HOME = ${JSON.stringify(home)}; +const registry = require("./src/lib/state/registry.js"); +registry.registerSandbox({ + name: "stuck-sandbox", + agent: "openclaw", + mcp: { + bridges: { github: ${GITHUB_BRIDGE} }, + managedServerNames: ["github"], + destroyPreparedAt: "2026-06-27T01:00:00.000Z", + destroyPendingAt: "2026-06-27T01:05:00.000Z", + }, +}); +const preflight = require("./src/lib/actions/sandbox/rebuild-preflight-phase.js"); +preflight.runRebuildPreflightPhase("stuck-sandbox", [], { throwOnError: true }).then( + () => { + process.stdout.write("UNEXPECTED_OK"); + process.exit(0); + }, + (error) => { + const after = registry.getSandbox("stuck-sandbox"); + process.stdout.write("<>" + JSON.stringify({ + error: String(error && error.message || error), + mcp: after && after.mcp, + })); + process.exit(0); + }, +); +`; + const result = runNodeScript(home, script); + expect(result.status).toBe(0); + const jsonMarker = "<>"; + const parsed = JSON.parse( + result.stdout.slice(result.stdout.indexOf(jsonMarker) + jsonMarker.length), + ) as { error: string; mcp: SandboxMcpSnapshot | undefined }; + expect(parsed.error).toContain("past the point of no return"); + expect(parsed.error).toContain("nemoclaw stuck-sandbox destroy"); + expect(parsed.mcp?.destroyPreparedAt).toBe("2026-06-27T01:00:00.000Z"); + expect(parsed.mcp?.destroyPendingAt).toBe("2026-06-27T01:05:00.000Z"); + expect(parsed.mcp?.managedServerNames).toEqual(["github"]); + expect(parsed.mcp?.bridges).toHaveProperty("github"); + expect(result.stdout).not.toContain("Deleting old sandbox"); + }); + + it("WITHOUT --force still refuses (prepared phase points at the --force recovery)", async () => { + const home = createTempHome("nemoclaw-noforce-guard-"); + const script = ` +process.env.HOME = ${JSON.stringify(home)}; +const registry = require("./src/lib/state/registry.js"); +registry.registerSandbox({ + name: "stuck-sandbox", + agent: "openclaw", + mcp: { bridges: {}, destroyPreparedAt: "2026-06-27T01:00:00.000Z" }, +}); +const bridge = require("./src/lib/actions/sandbox/mcp-bridge.js"); +bridge.removeMcpBridge("stuck-sandbox", "github", {}).then( + () => { + process.stdout.write("UNEXPECTED_OK"); + process.exit(0); + }, + (error) => { + process.stdout.write(String(error && error.message || error)); + process.exit(0); + }, +); +`; + const result = runNodeScript(home, script); + expect(result.status).toBe(0); + expect(result.stdout).toContain("incomplete MCP destroy transaction"); + expect(result.stdout).toContain("mcp remove --force"); + }); +}); diff --git a/test/mcp-destroy-lifecycle.test.ts b/test/mcp-destroy-lifecycle.test.ts index 9f4100a2b17..7f8cc1492e2 100644 --- a/test/mcp-destroy-lifecycle.test.ts +++ b/test/mcp-destroy-lifecycle.test.ts @@ -330,7 +330,14 @@ describe("authenticated MCP sandbox destroy lifecycle", () => { const message = await captureMessage(() => bridge[method]("alpha")); const sandbox = registry.getSandbox("alpha"); - expect(message).toContain("incomplete MCP destroy transaction"); + // #6376: the guard message is phase-aware — the pending (phase-two) + // marker records confirmed sandbox deletion, so it points at finishing + // the destroy rather than the in-place `mcp remove --force` recovery. + expect(message).toContain( + marker === "destroyPendingAt" + ? "past the point of no return" + : "incomplete MCP destroy transaction", + ); expect(sandbox?.mcp).toHaveProperty(marker); expect(testState.calls).toEqual([]); expect(testState.adapterCalls).toEqual([]); @@ -441,7 +448,7 @@ describe("authenticated MCP sandbox destroy lifecycle", () => { await bridge.restoreMcpBridgesAfterDestroyAbort("alpha", preparation); const sandbox = registry.getSandbox("alpha"); - expect(Object.hasOwn(process.env, "GITHUB_TOKEN")).toBe(true); + expect(process.env.GITHUB_TOKEN).toBe("ambient-value-that-must-not-rotate"); expect([...testState.providers.keys()]).toContain("alpha-mcp-github"); expect( testState.calls.some((call) => call === "sandbox provider attach alpha alpha-mcp-github"), @@ -528,6 +535,7 @@ describe("authenticated MCP sandbox destroy lifecycle", () => { await bridge.restoreMcpBridgesAfterRebuild("alpha", [bridgeEntries.github]); + expect(process.env.GITHUB_TOKEN).toBe("ambient-value-that-must-not-rotate"); expect(testState.calls.some((call) => /^provider (create|update) /.test(call))).toBe(false); expect([...testState.attachedProviders]).toContain("alpha-mcp-github"); expect(testState.adapterRegistered).toBe(true);