diff --git a/src/CodexAcpClient.ts b/src/CodexAcpClient.ts index 8a5af9e7..f7474e9d 100644 --- a/src/CodexAcpClient.ts +++ b/src/CodexAcpClient.ts @@ -758,9 +758,15 @@ export class CodexAcpClient { })); let serversToConfigure = requestedServers; if (shouldDeduplicateMcpConflicts()) { - // Prevents Codex from deep-merging incompatible field types, such as url and stdio schemas. - const existingNames = await this.getConfigMcpServerNames(projectPath); - serversToConfigure = requestedServers.filter(mcp => !existingNames.has(mcp.name)); + // Codex deep-merges session config into its persisted config. Let the session replace + // connection details when both definitions use the same transport, but keep filtering + // incompatible transports because merging e.g. `url` and `command` produces an invalid + // hybrid definition. + const existingServers = await this.getConfigMcpServers(projectPath); + serversToConfigure = requestedServers.filter(mcp => { + const existing = existingServers.get(mcp.name); + return existing === undefined || haveCompatibleMcpTransports(existing, mcp.server); + }); } if (serversToConfigure.length === 0) { return configWithWorkspaceRoots; @@ -772,7 +778,7 @@ export class CodexAcpClient { }; } - private async getConfigMcpServerNames(projectPath: string): Promise> { + private async getConfigMcpServers(projectPath: string): Promise> { const response = await this.codexClient.configRead({ includeLayers: true, cwd: projectPath }); const effectiveMcpServers = response?.config?.["mcp_servers"]; const configLayers = response?.layers ?? []; @@ -780,10 +786,15 @@ export class CodexAcpClient { return isJsonObject(layer.config) ? layer.config["mcp_servers"] : undefined; }); const configuredMcpServers = [effectiveMcpServers, ...layerMcpServers].filter(isJsonObject); - if (configuredMcpServers.length === 0) { - return new Set(); + const servers = new Map(); + for (const configured of configuredMcpServers) { + for (const [name, server] of Object.entries(configured)) { + if (!servers.has(name) && isJsonObject(server)) { + servers.set(name, server); + } + } } - return new Set(configuredMcpServers.flatMap(server => Object.keys(server))); + return servers; } getModelProvider(): string | null { @@ -1218,6 +1229,16 @@ function shouldDeduplicateMcpConflicts(): boolean { return !disabledByEnv; } +function haveCompatibleMcpTransports(configured: JsonObject, requested: McpServer): boolean { + const configuredTransport = "command" in configured && !("url" in configured) + ? "stdio" + : "url" in configured && !("command" in configured) + ? "http" + : null; + const requestedTransport = "type" in requested ? requested.type : "stdio"; + return configuredTransport === requestedTransport; +} + type WireApi = "responses"; type GatewayConfigSource = "authentication" | "acpProviders"; diff --git a/src/__tests__/CodexACPAgent/CodexAcpClient.test.ts b/src/__tests__/CodexACPAgent/CodexAcpClient.test.ts index 77aa7af7..be118fe7 100644 --- a/src/__tests__/CodexACPAgent/CodexAcpClient.test.ts +++ b/src/__tests__/CodexACPAgent/CodexAcpClient.test.ts @@ -886,7 +886,7 @@ describe('ACP server test', { timeout: 40_000 }, () => { } as unknown as acp.NewSessionRequest)).rejects.toThrow("additionalDirectories entries must be strings"); }); - it('sanitizes whitespace in ACP MCP server names before adding them to Codex config', async () => { + it('adds same-transport ACP MCP overrides while filtering incompatible conflicts', async () => { const mockFixture = createCodexMockTestFixture(); const codexAcpClient = mockFixture.getCodexAcpClient(); const codexAppServerClient = mockFixture.getCodexAppServerClient(); @@ -898,6 +898,13 @@ describe('ACP server test', { timeout: 40_000 }, () => { shared_mcp: { url: "https://example.com/mcp", }, + stdio_server_one: { + command: "stale-command", + args: ["stale"], + }, + global_only: { + command: "global-command", + }, }, }, } as any); diff --git a/src/__tests__/CodexACPAgent/mcp-config-merge.test.ts b/src/__tests__/CodexACPAgent/mcp-config-merge.test.ts index 94a92d71..190c3c1f 100644 --- a/src/__tests__/CodexACPAgent/mcp-config-merge.test.ts +++ b/src/__tests__/CodexACPAgent/mcp-config-merge.test.ts @@ -17,9 +17,18 @@ describe('MCP config merge across configured MCP servers and ACP request', { tim beforeEach(() => { vi.clearAllMocks(); + const node = JSON.stringify(process.execPath); + const mcpServer = JSON.stringify(path.resolve(process.cwd(), "node_modules/mcp-hello-world/build/stdio.js")); const globalConfig = ` [mcp_servers.shared-mcp] url = "https://example.com/mcp" + +[mcp_servers.same-transport] +command = "missing-global-command" + +[mcp_servers.global-only] +command = ${node} +args = [${mcpServer}] `; const projectConfig = ` @@ -97,6 +106,37 @@ url = "https://example.com/mcp" })).resolves.toBeDefined(); }); + it('should prefer a same-transport ACP MCP and preserve unrelated configured servers', async () => { + const codexAcpAgent = fixture.getCodexAcpAgent(); + await codexAcpAgent.initialize({protocolVersion: 1}); + fixture.getCodexAcpClient().authRequired = vi.fn().mockResolvedValue(false); + + const codexAcpClient = fixture.getCodexAcpClient(); + const startupVersion = codexAcpClient.getMcpServerStartupVersion(); + const mcpServer = path.resolve(process.cwd(), "node_modules/mcp-hello-world/build/stdio.js"); + await codexAcpAgent.newSession({ + cwd: "", + mcpServers: [{ + name: "same-transport", + command: process.execPath, + args: [mcpServer], + env: [], + }], + }); + + await expect(codexAcpClient.awaitMcpServerStartup(["same-transport"], startupVersion)).resolves.toEqual({ + ready: ["same-transport"], + failed: [], + cancelled: [], + }); + await expect(codexAcpClient.listMcpServers()).resolves.toMatchObject({ + data: expect.arrayContaining([ + expect.objectContaining({name: "same-transport"}), + expect.objectContaining({name: "global-only"}), + ]), + }); + }); + it('should not filter the conflicting ACP MCP when config filtering is disabled', async () => { vi.stubEnv("DISABLE_MCP_CONFIG_FILTERING", "true"); const codexAcpAgent = fixture.getCodexAcpAgent();