Skip to content

Commit 511f92f

Browse files
fix: ignore MCP's with conflicted name to prevent config corruption and codex crash
1 parent fa6b0a8 commit 511f92f

2 files changed

Lines changed: 109 additions & 5 deletions

File tree

src/CodexAcpClient.ts

Lines changed: 23 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -206,7 +206,7 @@ export class CodexAcpClient {
206206
approvalPolicy: null,
207207
sandbox: null,
208208
baseInstructions: null,
209-
config: this.createSessionConfig(request.cwd, request.mcpServers ?? []),
209+
config: await this.createSessionConfig(request.cwd, request.mcpServers ?? []),
210210
cwd: request.cwd,
211211
developerInstructions: null,
212212
model: null,
@@ -228,7 +228,7 @@ export class CodexAcpClient {
228228
approvalPolicy: null,
229229
sandbox: null,
230230
baseInstructions: null,
231-
config: this.createSessionConfig(request.cwd, request.mcpServers ?? []),
231+
config: await this.createSessionConfig(request.cwd, request.mcpServers ?? []),
232232
cwd: request.cwd,
233233
developerInstructions: null,
234234
model: null,
@@ -250,7 +250,7 @@ export class CodexAcpClient {
250250
await this.refreshSkills(request.cwd, request._meta);
251251

252252
const response = await this.codexClient.threadStart({
253-
config: this.createSessionConfig(request.cwd, request.mcpServers),
253+
config: await this.createSessionConfig(request.cwd, request.mcpServers),
254254
modelProvider: this.getModelProvider(),
255255
model: null,
256256
cwd: request.cwd,
@@ -282,7 +282,7 @@ export class CodexAcpClient {
282282
return this.codexClient.getMcpServerStartupVersion();
283283
}
284284

285-
private createSessionConfig(projectPath: string, mcpServers: Array<McpServer>): JsonObject {
285+
private async createSessionConfig(projectPath: string, mcpServers: Array<McpServer>): Promise<JsonObject> {
286286
const mergedConfig = {
287287
...mergeGatewayConfig(this.config, this.gatewayConfig),
288288
projects: {
@@ -294,10 +294,28 @@ export class CodexAcpClient {
294294
if (mcpServers.length === 0) {
295295
return mergedConfig;
296296
}
297+
298+
// Deduplicates new servers against existing config to prevent Codex from deep-merging
299+
// incompatible field types (e.g., mixing url and stdio schemas).
300+
const existingNames = await this.getConfigMcpServerNames(projectPath);
301+
const uniqueServers = mcpServers.filter(mcp => !existingNames.has(mcp.name));
302+
if (uniqueServers.length === 0) {
303+
return mergedConfig;
304+
}
305+
297306
return {
298307
...mergedConfig,
299-
"mcp_servers": Object.fromEntries(mcpServers.map(mcp => [mcp.name, this.createMcpSeverConfig(mcp)]))
308+
"mcp_servers": Object.fromEntries(uniqueServers.map(mcp => [mcp.name, this.createMcpSeverConfig(mcp)])),
309+
};
310+
}
311+
312+
private async getConfigMcpServerNames(projectPath: string): Promise<Set<string>> {
313+
const response = await this.codexClient.configRead({ includeLayers: true, cwd: projectPath });
314+
const mcpServers = response?.config?.["mcp_servers"];
315+
if (!mcpServers || typeof mcpServers !== "object" || Array.isArray(mcpServers)) {
316+
return new Set();
300317
}
318+
return new Set(Object.keys(mcpServers));
301319
}
302320

303321
getModelProvider(): string | null {
Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
1+
// noinspection ES6RedundantAwait
2+
3+
import {afterEach, beforeEach, describe, expect, it, vi} from 'vitest';
4+
import path from "node:path";
5+
import fs from "node:fs";
6+
import os from "node:os";
7+
import type {McpServerStdio} from "@agentclientprotocol/sdk";
8+
import {startCodexConnection} from "../../CodexJsonRpcConnection";
9+
import {createBaseTestFixture, removeDirectoryWithRetry, type TestFixture} from "../acp-test-utils";
10+
11+
/**
12+
* Reproduces the bug in CodexAcpClient.createSessionConfig where it does not
13+
* resolve conflicts between MCP servers defined in the global codex config.toml
14+
* and MCP servers supplied through the ACP protocol.
15+
*
16+
* Setup:
17+
* 1. Write a url-based MCP server "shared-mcp" into the codex home config.toml
18+
* (acts as the user's globally configured MCP).
19+
* 2. Open a new ACP session passing a command-type MCP with the same name
20+
* "shared-mcp" via mcpServers.
21+
*
22+
* Expectation:
23+
* /mcp must still list "shared-mcp" as a single, well-formed entry. Currently
24+
* the override built by createSessionConfig replaces mcp_servers wholesale,
25+
* so the url-based config and the command-type override clash on the codex
26+
* side and produce a broken merged entry.
27+
*/
28+
describe('MCP config merge across global config and ACP request', { timeout: 40_000 }, () => {
29+
30+
let codexHome: string;
31+
let fixture: TestFixture;
32+
33+
beforeEach(() => {
34+
vi.clearAllMocks();
35+
36+
const configToml = `
37+
[mcp_servers.shared-mcp]
38+
url = "https://example.com/mcp"
39+
`;
40+
codexHome = fs.mkdtempSync(path.join(os.tmpdir(), "codex-acp-mcp-merge-"));
41+
fs.writeFileSync(path.join(codexHome, "config.toml"), configToml, "utf8");
42+
43+
const codexConnection = startCodexConnection(undefined, {
44+
...process.env,
45+
CODEX_HOME: codexHome,
46+
});
47+
48+
fixture = createBaseTestFixture({
49+
connection: codexConnection.connection,
50+
getExitCode: () => codexConnection.process.exitCode,
51+
});
52+
});
53+
54+
afterEach(() => {
55+
removeDirectoryWithRetry(codexHome);
56+
});
57+
58+
it('should preserve the global url-based MCP when ACP passes a command-type MCP with the same name', async () => {
59+
const codexAcpAgent = fixture.getCodexAcpAgent();
60+
await codexAcpAgent.initialize({protocolVersion: 1});
61+
62+
fixture.getCodexAcpClient().authRequired = vi.fn().mockResolvedValue(false);
63+
64+
const conflictingMcp: McpServerStdio = {
65+
name: "shared-mcp",
66+
command: "./node_modules/.bin/mcp-hello-world",
67+
args: ["example"],
68+
env: [{name: "example", value: "example"}],
69+
};
70+
71+
const newSessionResponse = await codexAcpAgent.newSession({
72+
cwd: "",
73+
mcpServers: [conflictingMcp],
74+
});
75+
fixture.clearAcpConnectionDump();
76+
77+
await codexAcpAgent.prompt({
78+
sessionId: newSessionResponse.sessionId,
79+
prompt: [{type: "text", text: "/mcp"}],
80+
});
81+
82+
const transportDump = fixture.getAcpConnectionDump([]);
83+
expect(transportDump).contain("Configured MCP servers:");
84+
expect(transportDump).contain("- shared-mcp");
85+
});
86+
});

0 commit comments

Comments
 (0)