From e2a2d1588d09b285b56328747f6874f8c053badc Mon Sep 17 00:00:00 2001 From: Mohamed Boudra Date: Thu, 29 Oct 2026 09:14:58 +0100 Subject: [PATCH] fix(agents): validate optional MCP requirements before plugin injection Keep optional approval decisions in the provider registry and preserve required plugin servers. Remove requirements alongside stripped internal servers so persistence cannot retain dangling metadata. --- .../src/server/agent/agent-manager.test.ts | 41 ++++++++++++++----- .../server/src/server/agent/agent-manager.ts | 14 +++++-- .../server/agent/runtime-mcp-config.test.ts | 14 ++++++- .../src/server/agent/runtime-mcp-config.ts | 5 +++ 4 files changed, 59 insertions(+), 15 deletions(-) diff --git a/packages/server/src/server/agent/agent-manager.test.ts b/packages/server/src/server/agent/agent-manager.test.ts index da3f9bb2c2..fe38bd92c0 100644 --- a/packages/server/src/server/agent/agent-manager.test.ts +++ b/packages/server/src/server/agent/agent-manager.test.ts @@ -3850,15 +3850,12 @@ test.each([ clients: { [provider]: client, }, - providerDefinitions: - provider === "pi" - ? { - pi: { - enabled: true, - applyToolPolicy: buildProviderRegistry(logger).pi.applyToolPolicy, - }, - } - : undefined, + providerDefinitions: { + [provider]: { + enabled: true, + applyToolPolicy: buildProviderRegistry(logger)[provider]!.applyToolPolicy, + }, + }, registry: storage, logger, mcpBaseUrl: "http://127.0.0.1:6767/mcp/agents", @@ -3898,8 +3895,12 @@ test.each([ expect(manager.getAgent(snapshot.id)?.config.mcpServerRequirements).toEqual({ paseo: "optional", }); - expect(snapshot.config.toolPolicy).toBeUndefined(); - expect(client.lastConfig?.toolPolicy).toBeUndefined(); + const expectedPolicy = + provider === "codex" + ? { preapproved: [{ kind: "mcp", server: "paseo", tool: "finish_execution" }] } + : undefined; + expect(snapshot.config.toolPolicy).toEqual(expectedPolicy); + expect(client.lastConfig?.toolPolicy).toEqual(expectedPolicy); expect(client.lastConfig?.mcpServers).toEqual(snapshot.config.mcpServers); }, ); @@ -12608,3 +12609,21 @@ test("an out-of-band command racing idle cleanup waits for the resumed runtime", expect(h.commands).toEqual(["/maintenance"]); expect(h.prompts).toEqual([]); }); + +test("MCP requirements cannot mark a later plugin server optional", async () => { + const workdir = mkdtempSync(join(tmpdir(), "agent-manager-test-")); + const manager = new AgentManager({ + clients: { codex: new TestAgentClient() }, + registry: new AgentStorage(join(workdir, "agents"), logger), + logger, + }); + await expect( + manager.createAgent( + { provider: "codex", cwd: workdir, mcpServerRequirements: { plugin: "optional" } }, + undefined, + { workspaceId: undefined }, + ), + ).rejects.toThrow( + "MCP server requirement 'plugin' requires MCP server 'plugin' in the same agent request", + ); +}); diff --git a/packages/server/src/server/agent/agent-manager.ts b/packages/server/src/server/agent/agent-manager.ts index a0a1246c6a..edfbcc8f21 100644 --- a/packages/server/src/server/agent/agent-manager.ts +++ b/packages/server/src/server/agent/agent-manager.ts @@ -5465,10 +5465,8 @@ export class AgentManager { private applyProviderConfiguration(config: AgentSessionConfig): AgentSessionConfig { const definition = this.providerDefinitions.get(config.provider); this.validateToolPolicyServers(config); + this.validateMcpServerRequirements(config); if (config.toolPolicy && !definition?.applyToolPolicy) { - if (config.toolPolicy.enforcement === "if-supported") { - return { ...config, toolPolicy: undefined }; - } throw new Error( `Provider '${config.provider}' cannot preapprove exact MCP tools for unattended execution`, ); @@ -5478,6 +5476,16 @@ export class AgentManager { : config; } + private validateMcpServerRequirements(config: AgentSessionConfig): void { + for (const name of Object.keys(config.mcpServerRequirements ?? {})) { + if (!Object.hasOwn(config.mcpServers ?? {}, name)) { + throw new Error( + `MCP server requirement '${name}' requires MCP server '${name}' in the same agent request`, + ); + } + } + } + private validateToolPolicyServers(config: AgentSessionConfig): void { if (!config.toolPolicy) return; const serverNames = new Set(Object.keys(config.mcpServers ?? {})); diff --git a/packages/server/src/server/agent/runtime-mcp-config.test.ts b/packages/server/src/server/agent/runtime-mcp-config.test.ts index 4704d131bb..c740e301c8 100644 --- a/packages/server/src/server/agent/runtime-mcp-config.test.ts +++ b/packages/server/src/server/agent/runtime-mcp-config.test.ts @@ -1,7 +1,7 @@ import { describe, expect, test } from "vitest"; import type { AgentSessionConfig } from "./agent-sdk-types.js"; -import { withRuntimePaseoMcpServer } from "./runtime-mcp-config.js"; +import { stripInternalPaseoMcpServer, withRuntimePaseoMcpServer } from "./runtime-mcp-config.js"; const BASE_CONFIG: AgentSessionConfig = { provider: "claude", @@ -49,3 +49,15 @@ describe("withRuntimePaseoMcpServer", () => { expect(result.mcpServers).toBeUndefined(); }); }); + +test("stripping an internal MCP server also strips its launch requirement", () => { + const result = stripInternalPaseoMcpServer({ + ...BASE_CONFIG, + mcpServerRequirements: { paseo: "optional", hub: "optional" }, + mcpServers: { + paseo: { type: "http", url: "http://127.0.0.1/mcp/agents?callerAgentId=agent-1" }, + hub: { type: "http", url: "https://hub.test/mcp" }, + }, + }); + expect(result.mcpServerRequirements).toEqual({ hub: "optional" }); +}); diff --git a/packages/server/src/server/agent/runtime-mcp-config.ts b/packages/server/src/server/agent/runtime-mcp-config.ts index 711638ce2f..0573efcdcf 100644 --- a/packages/server/src/server/agent/runtime-mcp-config.ts +++ b/packages/server/src/server/agent/runtime-mcp-config.ts @@ -18,6 +18,11 @@ export function stripInternalPaseoMcpServer(config: AgentSessionConfig): AgentSe delete nextMcpServers[PASEO_MCP_SERVER_NAME]; const next = { ...config }; + if (next.mcpServerRequirements?.[PASEO_MCP_SERVER_NAME]) { + next.mcpServerRequirements = { ...next.mcpServerRequirements }; + delete next.mcpServerRequirements[PASEO_MCP_SERVER_NAME]; + if (Object.keys(next.mcpServerRequirements).length === 0) delete next.mcpServerRequirements; + } if (Object.keys(nextMcpServers).length > 0) { next.mcpServers = nextMcpServers; } else {