Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 22 additions & 1 deletion src/lib/actions/sandbox/mcp-bridge-remove.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,12 @@ import {
import {
assertMcpDestroyNotPending,
bridgeState,
clearMcpDestroyMarkersIfNoBridges,
ensureSandboxGatewaySelected,
getBridgeAdapter,
getSandboxAgent,
getSandboxOrThrow,
hasMcpDestroyMarkers,
removeBridgeEntry,
} from "./mcp-bridge-state";
import {
Expand Down Expand Up @@ -102,13 +104,26 @@ async function removeMcpBridgeUnlocked(
validateSandboxName(sandboxName);
validateMcpServerName(server);
const sandbox = getSandboxOrThrow(sandboxName);
assertMcpDestroyNotPending(sandbox);
// A forced remove is the documented non-destructive recovery from an
// incomplete destroy transaction (#6376): it adopts the durable cleanup
// manifest for this server instead of requiring full sandbox destruction.
const forcedDestroyRecovery = options.force === true && hasMcpDestroyMarkers(sandbox);
if (!forcedDestroyRecovery) {
assertMcpDestroyNotPending(sandbox);
} else {
console.log(
` Sandbox '${sandboxName}' has an incomplete MCP destroy transaction; --force adopts its cleanup for '${server}'.`,
);
}
const entry = bridgeState(sandbox)[server];
if (!entry) {
if (!options.force) {
throw new McpBridgeError(`MCP server '${server}' not found on sandbox '${sandboxName}'.`);
}
console.log(` No MCP server '${server}' is registered on sandbox '${sandboxName}'.`);
if (clearMcpDestroyMarkersIfNoBridges(sandboxName)) {
console.log(` Cleared the incomplete MCP destroy transaction on sandbox '${sandboxName}'.`);
}
return;
}
if (entry.addState === "prepared") {
Expand All @@ -118,6 +133,9 @@ async function removeMcpBridgeUnlocked(
// state another workflow may own.
removeBridgeEntry(sandboxName, server);
console.log(` Cancelled incomplete MCP add for '${server}' on sandbox '${sandboxName}'.`);
if (clearMcpDestroyMarkersIfNoBridges(sandboxName)) {
console.log(` Cleared the incomplete MCP destroy transaction on sandbox '${sandboxName}'.`);
}
return;
}
// Cleanup follows the adapter persisted with the bridge. Requiring the
Expand Down Expand Up @@ -337,4 +355,7 @@ async function removeMcpBridgeUnlocked(
}
removeBridgeEntry(sandboxName, server);
console.log(` Removed MCP server '${server}' from sandbox '${sandboxName}'.`);
if (clearMcpDestroyMarkersIfNoBridges(sandboxName)) {
console.log(` Cleared the incomplete MCP destroy transaction on sandbox '${sandboxName}'.`);
}
}
39 changes: 37 additions & 2 deletions src/lib/actions/sandbox/mcp-bridge-state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,13 +97,48 @@ export function setBridgeState(sandboxName: string, bridges: Record<string, McpB
}
}

export function hasMcpDestroyMarkers(sandbox: SandboxEntry): boolean {
return !!sandbox.mcp?.destroyPreparedAt || !!sandbox.mcp?.destroyPendingAt;
}

export function assertMcpDestroyNotPending(sandbox: SandboxEntry): void {
if (!sandbox.mcp?.destroyPreparedAt && !sandbox.mcp?.destroyPendingAt) return;
if (!hasMcpDestroyMarkers(sandbox)) return;
const servers = Object.values(bridgeState(sandbox)).map((entry) => entry.server);
const removeHint =
servers.length > 0
? ` or clear it with: nemoclaw ${sandbox.name} mcp remove ${servers[0]} --force`
: ` or clear it with: nemoclaw ${sandbox.name} mcp remove <server> --force`;
throw new McpBridgeError(
`Sandbox '${sandbox.name}' has an incomplete MCP destroy transaction. Re-run the sandbox destroy command to finish cleanup before using MCP commands.`,
`Sandbox '${sandbox.name}' has an incomplete MCP destroy transaction. Re-run the sandbox destroy command to finish cleanup,${removeHint}`,
);
}

/**
* Drop the durable destroy markers once no MCP bridge entries remain. A
* forced `mcp remove` of the last server adopts the incomplete destroy
* transaction's cleanup manifest, so keeping the markers would only block
* every MCP and rebuild command with nothing left to clean (#6376).
*/
export function clearMcpDestroyMarkersIfNoBridges(sandboxName: string): boolean {
const sandbox = getSandboxOrThrow(sandboxName);
if (!hasMcpDestroyMarkers(sandbox)) return false;
const bridges = bridgeState(sandbox);
if (Object.keys(bridges).length > 0) return false;
const managedServerNames = sandbox.mcp?.managedServerNames;
const updated = registry.updateSandbox(sandboxName, {
mcp:
managedServerNames && managedServerNames.length > 0
? { bridges: {}, managedServerNames }
: undefined,
});
if (!updated) {
throw new McpBridgeError(
`Could not clear the MCP destroy markers for sandbox '${sandboxName}'.`,
);
}
return true;
}

export function assertNoDerivedResourceCollision(
sandbox: SandboxEntry,
server: string,
Expand Down
26 changes: 26 additions & 0 deletions src/lib/actions/sandbox/rebuild-preflight-confirmation.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import * as sandboxSession from "../../state/sandbox-session";
import {
confirmSandboxRebuildIfNeeded,
countActiveSandboxSessionsForRebuild,
createRebuildCommandContext,
} from "./rebuild-preflight-confirmation";
import { isSingleAgentRebuildSupported } from "./rebuild-preflight-guards";

Expand Down Expand Up @@ -57,6 +58,31 @@ describe("rebuild confirmation", () => {
});
});

describe("rebuild bail reporting (#6376)", () => {
it("prints the failure reason on stderr before exiting in CLI mode", () => {
const errors: string[] = [];
const errorSpy = vi.spyOn(console, "error").mockImplementation((message: unknown) => {
errors.push(String(message));
});
const exitSpy = vi.spyOn(process, "exit").mockImplementation(((code?: number) => {
throw new Error(`exit:${code}`);
}) as never);

const { bail } = createRebuildCommandContext({}, {});
expect(() => bail("Failed to preserve MCP bridges before rebuild: destroy pending")).toThrow(
"exit:1",
);
expect(errors.join("\n")).toContain("Failed to preserve MCP bridges before rebuild");
expect(exitSpy).toHaveBeenCalledWith(1);
errorSpy.mockRestore();
});

it("keeps the throwing bail contract when throwOnError is set", () => {
const { bail } = createRebuildCommandContext({}, { throwOnError: true });
expect(() => bail("boom")).toThrow("boom");
});
});

describe("rebuild preflight guards", () => {
it("rejects a multi-agent sandbox before later rebuild work", () => {
const error = vi.spyOn(console, "error").mockImplementation(() => undefined);
Expand Down
11 changes: 9 additions & 2 deletions src/lib/actions/sandbox/rebuild-preflight-confirmation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

import { resolveOpenshell } from "../../adapters/openshell/resolve";
import * as agentRuntime from "../../agent/runtime";
import { B, D, R, YW } from "../../cli/terminal-style";
import { B, D, R, RD, YW } from "../../cli/terminal-style";
import { prompt as askPrompt } from "../../credentials/store";
import {
normalizeRebuildSandboxOptions,
Expand Down Expand Up @@ -46,7 +46,14 @@ export function createRebuildCommandContext(
? (message: string) => {
throw new Error(message);
}
: (_message: string, code = 1) => process.exit(code),
: (message: string, code = 1) => {
// Always surface the reason on stderr: several phases (notably the
// MCP destroy-marker guard) reach bail() without printing anything
// themselves, and a bare exit 1 gives the operator no diagnosis
// (#6376).
if (message) console.error(` ${RD}Rebuild failed:${R} ${redact(message)}`);
return process.exit(code);
},
};
}

Expand Down
102 changes: 102 additions & 0 deletions test/mcp-destroy-lifecycle.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -669,6 +669,108 @@ describe("authenticated MCP sandbox destroy lifecycle", () => {
expect([...testState.providers.keys()]).not.toContain("alpha-mcp-github");
});

it("refuses a plain mcp remove under destroy markers and names the forced recovery (#6376)", async () => {
registry.registerSandbox({
name: "alpha",
agent: "openclaw",
gatewayName: "nemoclaw",
mcp: {
bridges: { github: bridgeEntries.github },
destroyPreparedAt: "2026-07-02T22:49:42.000Z",
destroyPendingAt: "2026-07-02T22:49:43.000Z",
},
});
registry.addCustomPolicy("alpha", ownedPolicy("github"));

const message = await captureMessage(() => bridge.removeMcpBridge("alpha", "github"));
const sandbox = registry.getSandbox("alpha");

expect(message).toContain("incomplete MCP destroy transaction");
expect(message).toContain("mcp remove github --force");
expect(sandbox?.mcp?.bridges).toHaveProperty("github");
expect(sandbox?.mcp?.destroyPreparedAt).toBeTruthy();
});

it("clears destroy markers when a forced mcp remove adopts the last entry (#6376)", async () => {
registry.registerSandbox({
name: "alpha",
agent: "openclaw",
gatewayName: "nemoclaw",
mcp: {
bridges: { github: bridgeEntries.github },
destroyPreparedAt: "2026-07-02T22:49:42.000Z",
destroyPendingAt: "2026-07-02T22:49:43.000Z",
},
});
registry.addCustomPolicy("alpha", ownedPolicy("github"));
const removedPolicies = new Set<string>();
testState.removePreset.mockImplementation((_sandbox: string, policyName: string) => {
removedPolicies.add(policyName);
return true;
});
testState.getPresetContentGatewayState.mockImplementation(() =>
removedPolicies.size > 0 ? "absent" : "match",
);

await bridge.removeMcpBridge("alpha", "github", { force: true });
const sandbox = registry.getSandbox("alpha");

expect(sandbox?.mcp?.bridges ?? {}).not.toHaveProperty("github");
expect(sandbox?.mcp?.destroyPreparedAt).toBeUndefined();
expect(sandbox?.mcp?.destroyPendingAt).toBeUndefined();
expect([...testState.providers.keys()]).not.toContain("alpha-mcp-github");
});

it("keeps destroy markers while other MCP entries still need forced cleanup (#6376)", async () => {
registry.registerSandbox({
name: "alpha",
agent: "openclaw",
gatewayName: "nemoclaw",
mcp: {
bridges: { github: bridgeEntries.github, slack: bridgeEntries.slack },
destroyPreparedAt: "2026-07-02T22:49:42.000Z",
},
});
registry.addCustomPolicy("alpha", ownedPolicy("github"));
registry.addCustomPolicy("alpha", ownedPolicy("slack"));
const removedPolicies = new Set<string>();
testState.removePreset.mockImplementation((_sandbox: string, policyName: string) => {
removedPolicies.add(policyName);
return true;
});
testState.getPresetContentGatewayState.mockImplementation(() =>
removedPolicies.size > 0 ? "absent" : "match",
);

await bridge.removeMcpBridge("alpha", "github", { force: true });
const afterFirst = registry.getSandbox("alpha");
expect(afterFirst?.mcp?.bridges).toHaveProperty("slack");
expect(afterFirst?.mcp?.destroyPreparedAt).toBeTruthy();

await bridge.removeMcpBridge("alpha", "slack", { force: true });
const afterSecond = registry.getSandbox("alpha");
expect(afterSecond?.mcp?.destroyPreparedAt).toBeUndefined();
expect(afterSecond?.mcp?.destroyPendingAt).toBeUndefined();
});

it("clears bare destroy markers when a forced remove finds no entries (#6376)", async () => {
registry.registerSandbox({
name: "alpha",
agent: "openclaw",
gatewayName: "nemoclaw",
mcp: {
bridges: {},
destroyPendingAt: "2026-07-02T22:49:43.000Z",
},
});

await bridge.removeMcpBridge("alpha", "github", { force: true });
const sandbox = registry.getSandbox("alpha");

expect(sandbox?.mcp?.destroyPreparedAt).toBeUndefined();
expect(sandbox?.mcp?.destroyPendingAt).toBeUndefined();
});

it("does not let force delete a drifted global provider", async () => {
testState.providers.set("alpha-mcp-github", {
credential: "OTHER_TOKEN",
Expand Down
Loading