From 511f92f21c080c762a1d12f66582907f3f96749f Mon Sep 17 00:00:00 2001 From: Alexandr Suhinin Date: Mon, 11 May 2026 12:07:37 +0300 Subject: [PATCH 1/2] fix: ignore MCP's with conflicted name to prevent config corruption and codex crash --- src/CodexAcpClient.ts | 28 ++++-- .../CodexACPAgent/mcp-config-merge.test.ts | 86 +++++++++++++++++++ 2 files changed, 109 insertions(+), 5 deletions(-) create mode 100644 src/__tests__/CodexACPAgent/mcp-config-merge.test.ts diff --git a/src/CodexAcpClient.ts b/src/CodexAcpClient.ts index b475513..d5b59f3 100644 --- a/src/CodexAcpClient.ts +++ b/src/CodexAcpClient.ts @@ -206,7 +206,7 @@ export class CodexAcpClient { approvalPolicy: null, sandbox: null, baseInstructions: null, - config: this.createSessionConfig(request.cwd, request.mcpServers ?? []), + config: await this.createSessionConfig(request.cwd, request.mcpServers ?? []), cwd: request.cwd, developerInstructions: null, model: null, @@ -228,7 +228,7 @@ export class CodexAcpClient { approvalPolicy: null, sandbox: null, baseInstructions: null, - config: this.createSessionConfig(request.cwd, request.mcpServers ?? []), + config: await this.createSessionConfig(request.cwd, request.mcpServers ?? []), cwd: request.cwd, developerInstructions: null, model: null, @@ -250,7 +250,7 @@ export class CodexAcpClient { await this.refreshSkills(request.cwd, request._meta); const response = await this.codexClient.threadStart({ - config: this.createSessionConfig(request.cwd, request.mcpServers), + config: await this.createSessionConfig(request.cwd, request.mcpServers), modelProvider: this.getModelProvider(), model: null, cwd: request.cwd, @@ -282,7 +282,7 @@ export class CodexAcpClient { return this.codexClient.getMcpServerStartupVersion(); } - private createSessionConfig(projectPath: string, mcpServers: Array): JsonObject { + private async createSessionConfig(projectPath: string, mcpServers: Array): Promise { const mergedConfig = { ...mergeGatewayConfig(this.config, this.gatewayConfig), projects: { @@ -294,10 +294,28 @@ export class CodexAcpClient { if (mcpServers.length === 0) { return mergedConfig; } + + // Deduplicates new servers against existing config to prevent Codex from deep-merging + // incompatible field types (e.g., mixing url and stdio schemas). + const existingNames = await this.getConfigMcpServerNames(projectPath); + const uniqueServers = mcpServers.filter(mcp => !existingNames.has(mcp.name)); + if (uniqueServers.length === 0) { + return mergedConfig; + } + return { ...mergedConfig, - "mcp_servers": Object.fromEntries(mcpServers.map(mcp => [mcp.name, this.createMcpSeverConfig(mcp)])) + "mcp_servers": Object.fromEntries(uniqueServers.map(mcp => [mcp.name, this.createMcpSeverConfig(mcp)])), + }; + } + + private async getConfigMcpServerNames(projectPath: string): Promise> { + const response = await this.codexClient.configRead({ includeLayers: true, cwd: projectPath }); + const mcpServers = response?.config?.["mcp_servers"]; + if (!mcpServers || typeof mcpServers !== "object" || Array.isArray(mcpServers)) { + return new Set(); } + return new Set(Object.keys(mcpServers)); } getModelProvider(): string | null { diff --git a/src/__tests__/CodexACPAgent/mcp-config-merge.test.ts b/src/__tests__/CodexACPAgent/mcp-config-merge.test.ts new file mode 100644 index 0000000..e59453a --- /dev/null +++ b/src/__tests__/CodexACPAgent/mcp-config-merge.test.ts @@ -0,0 +1,86 @@ +// noinspection ES6RedundantAwait + +import {afterEach, beforeEach, describe, expect, it, vi} from 'vitest'; +import path from "node:path"; +import fs from "node:fs"; +import os from "node:os"; +import type {McpServerStdio} from "@agentclientprotocol/sdk"; +import {startCodexConnection} from "../../CodexJsonRpcConnection"; +import {createBaseTestFixture, removeDirectoryWithRetry, type TestFixture} from "../acp-test-utils"; + +/** + * Reproduces the bug in CodexAcpClient.createSessionConfig where it does not + * resolve conflicts between MCP servers defined in the global codex config.toml + * and MCP servers supplied through the ACP protocol. + * + * Setup: + * 1. Write a url-based MCP server "shared-mcp" into the codex home config.toml + * (acts as the user's globally configured MCP). + * 2. Open a new ACP session passing a command-type MCP with the same name + * "shared-mcp" via mcpServers. + * + * Expectation: + * /mcp must still list "shared-mcp" as a single, well-formed entry. Currently + * the override built by createSessionConfig replaces mcp_servers wholesale, + * so the url-based config and the command-type override clash on the codex + * side and produce a broken merged entry. + */ +describe('MCP config merge across global config and ACP request', { timeout: 40_000 }, () => { + + let codexHome: string; + let fixture: TestFixture; + + beforeEach(() => { + vi.clearAllMocks(); + + const configToml = ` +[mcp_servers.shared-mcp] +url = "https://example.com/mcp" +`; + codexHome = fs.mkdtempSync(path.join(os.tmpdir(), "codex-acp-mcp-merge-")); + fs.writeFileSync(path.join(codexHome, "config.toml"), configToml, "utf8"); + + const codexConnection = startCodexConnection(undefined, { + ...process.env, + CODEX_HOME: codexHome, + }); + + fixture = createBaseTestFixture({ + connection: codexConnection.connection, + getExitCode: () => codexConnection.process.exitCode, + }); + }); + + afterEach(() => { + removeDirectoryWithRetry(codexHome); + }); + + it('should preserve the global url-based MCP when ACP passes a command-type MCP with the same name', async () => { + const codexAcpAgent = fixture.getCodexAcpAgent(); + await codexAcpAgent.initialize({protocolVersion: 1}); + + fixture.getCodexAcpClient().authRequired = vi.fn().mockResolvedValue(false); + + const conflictingMcp: McpServerStdio = { + name: "shared-mcp", + command: "./node_modules/.bin/mcp-hello-world", + args: ["example"], + env: [{name: "example", value: "example"}], + }; + + const newSessionResponse = await codexAcpAgent.newSession({ + cwd: "", + mcpServers: [conflictingMcp], + }); + fixture.clearAcpConnectionDump(); + + await codexAcpAgent.prompt({ + sessionId: newSessionResponse.sessionId, + prompt: [{type: "text", text: "/mcp"}], + }); + + const transportDump = fixture.getAcpConnectionDump([]); + expect(transportDump).contain("Configured MCP servers:"); + expect(transportDump).contain("- shared-mcp"); + }); +}); From 089a7753ecabb95dfad4da08dcca401a808b2c67 Mon Sep 17 00:00:00 2001 From: Alexandr Suhinin Date: Mon, 11 May 2026 12:16:49 +0300 Subject: [PATCH 2/2] clean: remove comment --- .../CodexACPAgent/mcp-config-merge.test.ts | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/src/__tests__/CodexACPAgent/mcp-config-merge.test.ts b/src/__tests__/CodexACPAgent/mcp-config-merge.test.ts index e59453a..ab54b8e 100644 --- a/src/__tests__/CodexACPAgent/mcp-config-merge.test.ts +++ b/src/__tests__/CodexACPAgent/mcp-config-merge.test.ts @@ -8,23 +8,6 @@ import type {McpServerStdio} from "@agentclientprotocol/sdk"; import {startCodexConnection} from "../../CodexJsonRpcConnection"; import {createBaseTestFixture, removeDirectoryWithRetry, type TestFixture} from "../acp-test-utils"; -/** - * Reproduces the bug in CodexAcpClient.createSessionConfig where it does not - * resolve conflicts between MCP servers defined in the global codex config.toml - * and MCP servers supplied through the ACP protocol. - * - * Setup: - * 1. Write a url-based MCP server "shared-mcp" into the codex home config.toml - * (acts as the user's globally configured MCP). - * 2. Open a new ACP session passing a command-type MCP with the same name - * "shared-mcp" via mcpServers. - * - * Expectation: - * /mcp must still list "shared-mcp" as a single, well-formed entry. Currently - * the override built by createSessionConfig replaces mcp_servers wholesale, - * so the url-based config and the command-type override clash on the codex - * side and produce a broken merged entry. - */ describe('MCP config merge across global config and ACP request', { timeout: 40_000 }, () => { let codexHome: string;