Skip to content

Commit 2aa37c9

Browse files
committed
Address MCP config filtering review comments
1 parent 6edaf22 commit 2aa37c9

3 files changed

Lines changed: 38 additions & 54 deletions

File tree

src/CodexAcpClient.ts

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -720,8 +720,7 @@ function formatUriAsLink(name: string | null | undefined, uri: string): string {
720720

721721
function shouldDeduplicateMcpConflicts(): boolean {
722722
const disabledByEnv = process.env["DISABLE_MCP_CONFIG_FILTERING"] === "true";
723-
const disabledByArg = process.argv.includes("--disable-mcp-config-filtering");
724-
return !disabledByEnv && !disabledByArg;
723+
return !disabledByEnv;
725724
}
726725

727726
interface GatewayConfig {

src/__tests__/CodexACPAgent/CodexAcpClient.test.ts

Lines changed: 0 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -328,58 +328,6 @@ describe('ACP server test', { timeout: 40_000 }, () => {
328328
});
329329
});
330330

331-
it('passes conflicting ACP MCP servers through when config filtering is disabled', async () => {
332-
vi.stubEnv("DISABLE_MCP_CONFIG_FILTERING", "true");
333-
try {
334-
const mockFixture = createCodexMockTestFixture();
335-
const codexAcpClient = mockFixture.getCodexAcpClient();
336-
const codexAppServerClient = mockFixture.getCodexAppServerClient();
337-
338-
vi.spyOn(codexAppServerClient, "listSkills").mockResolvedValue({data: []});
339-
const configReadSpy = vi.spyOn(codexAppServerClient, "configRead").mockResolvedValue({
340-
config: {
341-
mcp_servers: {
342-
shared_mcp: {
343-
url: "https://example.com/mcp",
344-
},
345-
},
346-
},
347-
} as any);
348-
const threadStartSpy = vi.spyOn(codexAppServerClient, "threadStart").mockResolvedValue({
349-
thread: {id: "thread-id"} as any,
350-
model: "gpt-5",
351-
reasoningEffort: "medium",
352-
serviceTier: null,
353-
} as any);
354-
vi.spyOn(codexAppServerClient, "listModels").mockResolvedValue({
355-
data: [createTestModel({id: "gpt-5"})],
356-
nextCursor: null,
357-
});
358-
359-
await codexAcpClient.newSession({
360-
cwd: "/workspace",
361-
mcpServers: [{
362-
name: "shared mcp",
363-
command: "npx",
364-
args: ["shared"],
365-
env: [],
366-
}],
367-
});
368-
369-
const threadStartRequest = threadStartSpy.mock.calls[0]![0];
370-
expect(configReadSpy).not.toHaveBeenCalled();
371-
expect(threadStartRequest.config?.["mcp_servers"]).toEqual({
372-
shared_mcp: {
373-
command: "npx",
374-
args: ["shared"],
375-
env: {},
376-
},
377-
});
378-
} finally {
379-
vi.unstubAllEnvs();
380-
}
381-
});
382-
383331
it('waits for typed mcp startup status updates and returns terminal states', async () => {
384332
const mockFixture = createCodexMockTestFixture();
385333
const codexAcpClient = mockFixture.getCodexAcpClient();

src/__tests__/CodexACPAgent/mcp-config-merge.test.ts

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ url = "https://example.com/mcp"
3535
});
3636

3737
afterEach(() => {
38+
vi.unstubAllEnvs();
3839
removeDirectoryWithRetry(codexHome);
3940
});
4041

@@ -66,4 +67,40 @@ url = "https://example.com/mcp"
6667
expect(transportDump).contain("Configured MCP servers:");
6768
expect(transportDump).contain("- shared-mcp");
6869
});
70+
71+
it('should pass the conflicting ACP MCP through when config filtering is disabled', async () => {
72+
vi.stubEnv("DISABLE_MCP_CONFIG_FILTERING", "true");
73+
const codexAcpAgent = fixture.getCodexAcpAgent();
74+
await codexAcpAgent.initialize({protocolVersion: 1});
75+
76+
fixture.getCodexAcpClient().authRequired = vi.fn().mockResolvedValue(false);
77+
78+
const conflictingMcp: McpServerStdio = {
79+
name: "shared-mcp",
80+
command: "./node_modules/.bin/mcp-hello-world",
81+
args: ["example"],
82+
env: [{name: "example", value: "example"}],
83+
};
84+
85+
await codexAcpAgent.newSession({
86+
cwd: "",
87+
mcpServers: [conflictingMcp],
88+
});
89+
90+
const threadStartRequest = fixture.getCodexConnectionEvents([])
91+
.find(event => event.eventType === "request" && event.method === "thread/start");
92+
expect(threadStartRequest).toMatchObject({
93+
params: {
94+
config: {
95+
mcp_servers: {
96+
"shared-mcp": {
97+
command: "./node_modules/.bin/mcp-hello-world",
98+
args: ["example"],
99+
env: {example: "example"},
100+
},
101+
},
102+
},
103+
},
104+
});
105+
});
69106
});

0 commit comments

Comments
 (0)