Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 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
2 changes: 2 additions & 0 deletions src/lib/actions/sandbox/mcp-bridge-add-restart.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ import {
observeMcpCredentialRevision,
providerMatchesCredential,
providerShapeDetail,
refreshMcpProviderEnvironment,
upsertMcpProvider,
waitForAttachedMcpCredential,
waitForDetachedMcpCredential,
Expand Down Expand Up @@ -434,6 +435,7 @@ async function addMcpBridgeUnlocked(
providerAttachAttempted = true;
attachProvider(sandboxName, entry);
applyGeneratedPolicy(sandboxName, entry, target);
refreshMcpProviderEnvironment(entry);
waitForAttachedMcpCredential(sandboxName, entry, {
...(providerResult.action === "updated"
? {
Expand Down
1 change: 1 addition & 0 deletions src/lib/actions/sandbox/mcp-bridge-input-targets.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,7 @@ replace(provider, "upsertMcpProvider", () => ({
},
}));
replace(provider, "attachProvider", () => {});
replace(provider, "refreshMcpProviderEnvironment", () => {});
replace(provider, "waitForAttachedMcpCredential", () => {});
registry.registerSandbox({ name: "alpha", agent: "openclaw" });
require("./src/lib/actions/sandbox/mcp-bridge.js").addMcpBridge("alpha", {
Expand Down
43 changes: 43 additions & 0 deletions src/lib/actions/sandbox/mcp-bridge-provider-mutation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,49 @@ export function upsertMcpProvider(
return { action: action === "create" ? "created" : "updated", inspection: after };
}

/**
* Republish an attached provider after its endpointless credential binding is
* active. OpenShell's Docker sidecar can observe the provider mutation before
* the bound policy generation; a no-field update advances the provider
* revision without reading or rotating the stored credential, giving the
* sidecar a post-policy generation to synchronize.
*/
export function refreshMcpProviderEnvironment(entry: McpBridgeEntry): McpProviderInspection {
assertPersistedAuthenticatedBridgeEntry(entry);
if (!entry.providerName || !entry.providerId) {
throw new McpBridgeError(
`MCP server '${entry.server}' has no stable OpenShell provider identity for credential synchronization.`,
);
}
const before = inspectMcpProvider(entry.providerName);
if (!providerMatchesCredential(before, entry.env[0], entry.providerId)) {
throw new McpBridgeError(
`OpenShell provider '${entry.providerName}' changed before credential synchronization. ${providerShapeDetail(before, entry.env[0], entry.providerId)} Refusing to mutate it.`,
);
}
const result = runOpenshellProviderCommand(["provider", "update", entry.providerName], {
ignoreError: true,
stdio: ["ignore", "pipe", "pipe"],
}) as OpenShellCommandResult;
if (result.status !== 0) {
throw new McpBridgeError(
commandOutput(result) ||
`Failed to synchronize MCP provider '${entry.providerName}' after policy binding.`,
);
}
const after = inspectMcpProvider(entry.providerName);
if (
!providerMatchesCredential(after, entry.env[0], entry.providerId) ||
!after.resourceVersion ||
after.resourceVersion <= (before.resourceVersion ?? 0)
) {
throw new McpBridgeError(
`OpenShell provider '${entry.providerName}' changed during credential synchronization. ${providerShapeDetail(after, entry.env[0], entry.providerId)} Refusing later MCP side effects.`,
);
}
return after;
}

function inspectMcpProviderForDeletion(
entry: McpBridgeEntry,
options: { allowLegacyGeneric?: boolean; allowMissing?: boolean; bestEffort?: boolean } = {},
Expand Down
45 changes: 45 additions & 0 deletions src/lib/actions/sandbox/mcp-bridge-provider.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import {
import {
assertMcpProviderRecoverable,
observeMcpCredentialRevision,
refreshMcpProviderEnvironment,
waitForAttachedMcpCredential,
waitForDetachedMcpCredential,
} from "./mcp-bridge-provider";
Expand Down Expand Up @@ -139,6 +140,50 @@ Provider:
);
});

it("republishes an exact provider only after policy binding without reading its credential", () => {
const id = "11111111-2222-4333-8444-555555555555";
const providerResult = (resourceVersion: number) => ({
pid: 1234,
status: 0,
signal: null,
output: [
null,
`Id: ${id}\nType: nemoclaw-mcp-v1\nResource version: ${resourceVersion}\nCredential keys: GITHUB_TOKEN\n`,
"",
],
stdout: `Id: ${id}\nType: nemoclaw-mcp-v1\nResource version: ${resourceVersion}\nCredential keys: GITHUB_TOKEN\n`,
stderr: "",
});
const run = vi
.spyOn(providerCommand, "runOpenshellProviderCommand")
.mockReturnValueOnce(providerResult(7))
.mockReturnValueOnce({
pid: 1234,
status: 0,
signal: null,
output: [null, "", ""],
stdout: "",
stderr: "",
})
.mockReturnValueOnce(providerResult(8));

expect(
refreshMcpProviderEnvironment({
server: "github",
agent: "openclaw",
adapter: "mcporter",
url: "https://api.githubcopilot.com/mcp",
env: ["GITHUB_TOKEN"],
providerName: "alpha-mcp-github",
providerId: id,
policyName: "mcp-bridge-github",
addedAt: "2026-08-19T00:00:00.000Z",
}),
).toMatchObject({ resourceVersion: 8 });
expect(run.mock.calls[1]?.[0]).toEqual(["provider", "update", "alpha-mcp-github"]);
expect(run.mock.calls[1]?.[0]).not.toContain("--credential");
});

it("distinguishes a real detach from OpenShell's idempotent success", () => {
expect(
providerDetachChangedState(0, "✓ Detached provider alpha-mcp-github from sandbox alpha"),
Expand Down
1 change: 1 addition & 0 deletions src/lib/actions/sandbox/mcp-bridge-provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ export {
detachMissingProviderReference,
detachProvider,
ensureMcpBridgeProviderProfile,
refreshMcpProviderEnvironment,
providerDetachChangedState,
upsertMcpProvider,
} from "./mcp-bridge-provider-mutation";
Expand Down
5 changes: 4 additions & 1 deletion src/lib/actions/sandbox/mcp-bridge-restart.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,9 @@ import {
assertNoAttachedProviderCredentialCollisions,
assertNoProviderCredentialCollisions,
attachProvider,
ensureMcpBridgeProviderProfile,
detachMissingProviderReference,
ensureMcpBridgeProviderProfile,
refreshMcpProviderEnvironment,
type McpCredentialRevisionObservation,
type McpProviderInspection,
observeMcpCredentialRevision,
Expand Down Expand Up @@ -171,6 +172,7 @@ async function restartMcpBridgeUnlocked(sandboxName: string, server?: string): P
}
attachProvider(sandboxName, entry);
applyGeneratedPolicy(sandboxName, entry, target);
refreshMcpProviderEnvironment(entry);
waitForAttachedMcpCredential(sandboxName, entry, {
...(providerResult.action === "updated"
? { previousRevision: previousCredentialRevision }
Expand Down Expand Up @@ -239,6 +241,7 @@ export async function restoreExistingMcpBridgeRuntime(
});
attachProvider(sandboxName, entry);
applyGeneratedPolicy(sandboxName, entry, resolvedTargetPins(resolvedByServer, entry));
refreshMcpProviderEnvironment(entry);
waitForAttachedMcpCredential(sandboxName, entry);
const adapter = (entry.adapter as AgentMcpAdapter | undefined) ?? defaultAdapter;
registerAgentAdapter(
Expand Down
7 changes: 6 additions & 1 deletion test/deepagents-mcp-legacy-lifecycle.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ type ProviderType = "generic" | "nemoclaw-mcp-v1";

let providerExists = true;
let providerType: ProviderType = "generic";
let providerResourceVersion = 1;
let attached = true;
let adapterRegistered = true;
let adapterRemovalOutcome = "";
Expand Down Expand Up @@ -101,6 +102,7 @@ beforeEach(() => {

providerExists = true;
providerType = "generic";
providerResourceVersion = 1;
attached = true;
adapterRegistered = true;
adapterRemovalOutcome = "";
Expand All @@ -120,7 +122,7 @@ beforeEach(() => {
return providerExists
? {
status: 0,
stdout: `Id: ${providerId}\nType: ${providerType}\nResource version: 1\nCredential keys: GITHUB_TOKEN\n`,
stdout: `Id: ${providerId}\nType: ${providerType}\nResource version: ${providerResourceVersion}\nCredential keys: GITHUB_TOKEN\n`,
stderr: "",
}
: { status: 1, stdout: "", stderr: "Provider not found" };
Expand All @@ -138,6 +140,9 @@ beforeEach(() => {
case args[0] === "sandbox" && args[1] === "provider" && args[2] === "attach":
attached = true;
return { status: 0, stdout: "Attached provider", stderr: "" };
case args[0] === "provider" && args[1] === "update":
providerResourceVersion += 1;
return { status: 0, stdout: "Updated provider", stderr: "" };
case args[0] === "provider" && args[1] === "delete":
providerExists = false;
attached = false;
Expand Down
12 changes: 7 additions & 5 deletions test/mcp-add-crash-consistency.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ const crashAfter = ${JSON.stringify(crashAfter)};
const marker = (name) => path.join(process.env.HOME, name + ".marker");
const mark = (name) => fs.writeFileSync(marker(name), "yes\n", { mode: 0o600 });
const marked = (name) => fs.existsSync(marker(name));
const providerVersion = () => Number.parseInt(marked("provider-version") ? fs.readFileSync(marker("provider-version"), "utf8") : "1", 10);
const setProviderVersion = (version) => fs.writeFileSync(marker("provider-version"), String(version), { mode: 0o600 });
const providerPresentAtStart = marked("provider");
const providerId = "11111111-2222-4333-8444-555555555555";
const foreignProviderId = "99999999-8888-4777-8666-555555555555";
Expand Down Expand Up @@ -84,17 +86,17 @@ providerCommands.runOpenshellProviderCommand = (args) => {
if (crashAfter === "race" && providerGetCount === 2) mark("provider");
if (crashAfter === "late-race" && providerGetCount === 3) mark("provider");
return marked("provider")
? { status: 0, stdout: "Id: " + (marked("foreign-provider") ? foreignProviderId : providerId) + "\nType: nemoclaw-mcp-v1\nResource version: " + (marked("updated") ? "2" : "1") + "\nCredential keys: FAKE_MCP_SECRET\n", stderr: "" }
? { status: 0, stdout: "Id: " + (marked("foreign-provider") ? foreignProviderId : providerId) + "\nType: nemoclaw-mcp-v1\nResource version: " + providerVersion() + "\nCredential keys: FAKE_MCP_SECRET\n", stderr: "" }
: { status: 1, stdout: "", stderr: "NotFound: provider" };
}
if (args[0] === "provider" && (args[1] === "create" || args[1] === "update")) {
if (!marked("policy")) {
return { status: 1, stdout: "", stderr: "provider mutation preceded policy attestation" };
}
if (args[1] === "create") observedProviderName = args[args.indexOf("--name") + 1];
if (args[1] === "update") observedProviderName = args[2];
if (args[1] === "create") observedProviderName = args[args.indexOf("--name") + 1], setProviderVersion(1);
if (args[1] === "update") observedProviderName = args[2], setProviderVersion(providerVersion() + 1);
mark("provider");
if (args[1] === "update") mark("updated");
if (args[1] === "update" && args.includes("--credential")) mark("updated");
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
if (crashAfter === "registered-late-collision") registry.addExtraProvider("foreign-registered");
if (crashAfter === "provider") process.exit(86);
return { status: 0, stdout: args[1] === "create" ? "Created provider" : "Updated provider", stderr: "" };
Expand Down Expand Up @@ -168,7 +170,7 @@ processRecovery.executeSandboxExecCommand = (_sandbox, command) => {
isPreupdateObservation && mark("observation");
return {
status: crashAfter === "preupdate-observation-forbidden" && isPreupdateObservation ? 1 : 0,
stdout: isObservation ? (marked("updated") ? "v2" : marked("provider") ? "v1" : "absent") : "",
stdout: isObservation ? (marked("updated") ? "v" + providerVersion() : marked("provider") ? "v1" : "absent") : "",
stderr: "",
};
};
Expand Down
6 changes: 3 additions & 3 deletions test/mcp-destroy-lifecycle.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,6 @@ async function captureMessage(action: () => Promise<unknown>): Promise<string> {
return error instanceof Error ? error.message : String(error);
}
}

function registerAlphaGithubBridge(): void {
registry.registerSandbox({
name: "alpha",
Expand Down Expand Up @@ -303,6 +302,7 @@ beforeEach(() => {
case args[0] === "sandbox" && args[1] === "provider" && args[2] === "attach":
testState.attachedProviders.add(args[4]);
return { status: 0, stdout: "Attached provider", stderr: "" };
case args[0] === "provider" && args[1] === "update" && args.length === 3: return Object.assign(testState.providers.get(args[2])!, { resourceVersion: 2 }) && { status: 0, stdout: "Updated provider", stderr: "" };
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
case args[0] === "provider" &&
args[1] === "delete" &&
testState.failProviderDelete === args[2]:
Expand Down Expand Up @@ -1239,7 +1239,7 @@ describe("authenticated MCP sandbox destroy lifecycle", () => {
expect(
testState.calls.some((call) => call === "sandbox provider attach alpha alpha-mcp-github"),
).toBe(true);
expect(testState.calls.some((call) => /^provider (create|update) /.test(call))).toBe(false);
expect(testState.calls.some((call) => /^provider (create|update) .*--credential/.test(call))).toBe(false);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
expect(testState.policyApplyCalls).toBe(2);
expect(testState.adapterCalls).toContain("command -v mcporter");
expect(
Expand Down Expand Up @@ -1320,7 +1320,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.calls.some((call) => /^provider (create|update) .*--credential/.test(call))).toBe(false);
expect([...testState.attachedProviders]).toContain("alpha-mcp-github");
expect(testState.adapterRegistered).toBe(true);
expect(testState.policyApplyCalls).toBe(2);
Expand Down
5 changes: 3 additions & 2 deletions test/mcp-restart-policy-order.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,7 @@ providerCommands.runOpenshellProviderCommand = (args) => {
}
if (args[0] === "provider" && args[1] === "update") {
providerCalls.push(command);
resourceVersion = 2;
resourceVersion += 1;
return { status: 0, stdout: "Updated provider", stderr: "" };
}
if (args[0] === "sandbox" && args[1] === "provider" && args[2] === "list") {
Expand Down Expand Up @@ -296,9 +296,10 @@ bridge.restartMcpBridge("alpha", "example").then(
providerCalls: string[];
registeredProviderGets: number;
};
expect(payload.observations).toEqual(["v1", "v2"]);
expect(payload.observations).toEqual(["v1", "v3"]);
expect(payload.providerCalls).toEqual([
"provider update alpha-mcp-example --credential MCP_TOKEN",
"provider update alpha-mcp-example",
]);
expect(payload.registeredProviderGets).toBe(1);
expect(payload.proofScripts).toHaveLength(2);
Expand Down
Loading