Skip to content

Commit 0b89e93

Browse files
committed
Address MCP config filtering review comments
1 parent 900accd commit 0b89e93

2 files changed

Lines changed: 37 additions & 52 deletions

File tree

src/__tests__/CodexACPAgent/CodexAcpClient.test.ts

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

442-
it('passes conflicting ACP MCP servers through when config filtering is disabled', async () => {
443-
vi.stubEnv("DISABLE_MCP_CONFIG_FILTERING", "true");
444-
try {
445-
const mockFixture = createCodexMockTestFixture();
446-
const codexAcpClient = mockFixture.getCodexAcpClient();
447-
const codexAppServerClient = mockFixture.getCodexAppServerClient();
448-
449-
vi.spyOn(codexAppServerClient, "listSkills").mockResolvedValue({data: []});
450-
const configReadSpy = vi.spyOn(codexAppServerClient, "configRead").mockResolvedValue({
451-
config: {
452-
mcp_servers: {
453-
shared_mcp: {
454-
url: "https://example.com/mcp",
455-
},
456-
},
457-
},
458-
} as any);
459-
const threadStartSpy = vi.spyOn(codexAppServerClient, "threadStart").mockResolvedValue({
460-
thread: {id: "thread-id"} as any,
461-
model: "gpt-5",
462-
reasoningEffort: "medium",
463-
serviceTier: null,
464-
} as any);
465-
vi.spyOn(codexAppServerClient, "listModels").mockResolvedValue({
466-
data: [createTestModel({id: "gpt-5"})],
467-
nextCursor: null,
468-
});
469-
470-
await codexAcpClient.newSession({
471-
cwd: "/workspace",
472-
mcpServers: [{
473-
name: "shared mcp",
474-
command: "npx",
475-
args: ["shared"],
476-
env: [],
477-
}],
478-
});
479-
480-
const threadStartRequest = threadStartSpy.mock.calls[0]![0];
481-
expect(configReadSpy).not.toHaveBeenCalled();
482-
expect(threadStartRequest.config?.["mcp_servers"]).toEqual({
483-
shared_mcp: {
484-
command: "npx",
485-
args: ["shared"],
486-
env: {},
487-
},
488-
});
489-
} finally {
490-
vi.unstubAllEnvs();
491-
}
492-
});
493-
494442
it('waits for typed mcp startup status updates and returns terminal states', async () => {
495443
const mockFixture = createCodexMockTestFixture();
496444
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)