Skip to content
Merged
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
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
10 changes: 9 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,12 @@ 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" &&
args[2] === "alpha-mcp-github" &&
args.length === 3:
providerResourceVersion += 1;
return { status: 0, stdout: "Updated provider", stderr: "" };
case args[0] === "provider" && args[1] === "delete":
providerExists = false;
attached = false;
Expand Down
23 changes: 18 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,28 @@ 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") {
const isCredentialUpdate =
args.length === 5 &&
args[2] === observedProviderName &&
args[3] === "--credential" &&
args[4] === "FAKE_MCP_SECRET";
const isCredentialFreeRefresh = args.length === 3 && args[2] === observedProviderName;
if (!isCredentialUpdate && !isCredentialFreeRefresh) {
throw new Error("Unexpected provider update: " + args.join(" "));
}
setProviderVersion(providerVersion() + 1);
if (isCredentialUpdate) mark("updated");
Comment thread
sandl99 marked this conversation as resolved.
}
mark("provider");
if (args[1] === "update") mark("updated");
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 +181,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
18 changes: 9 additions & 9 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,9 @@ 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 && testState.providers.has(args[2]):
testState.providers.get(args[2])!.resourceVersion = (testState.providers.get(args[2])!.resourceVersion ?? 1) + 1;
return { status: 0, stdout: "Updated provider", stderr: "" };
case args[0] === "provider" &&
args[1] === "delete" &&
testState.failProviderDelete === args[2]:
Expand Down Expand Up @@ -1236,15 +1238,12 @@ describe("authenticated MCP sandbox destroy lifecycle", () => {

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"),
).toBe(true);
expect(testState.calls.some((call) => /^provider (create|update) /.test(call))).toBe(false);
expect(testState.calls).toContain("sandbox provider attach alpha alpha-mcp-github");
expect(testState.providers.get("alpha-mcp-github")?.resourceVersion).toBe(2);
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(
testState.adapterCalls.some((call) => call.includes("openshell:resolve:env:GITHUB_TOKEN")),
).toBe(true);
expect(testState.adapterCalls.some((call) => call.includes("openshell:resolve:env:GITHUB_TOKEN"))).toBe(true);
expect(sandbox?.mcp?.bridges).toHaveProperty("github");
expect(sandbox?.mcp?.managedServerNames).toEqual(["github", "retired"]);
expect(sandbox?.mcp?.destroyPreparedAt).toBeUndefined();
Expand Down Expand Up @@ -1320,7 +1319,8 @@ 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.providers.get("alpha-mcp-github")?.resourceVersion).toBe(2);
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