From b325f036a6b52df662bdbf01423733c23991a62c Mon Sep 17 00:00:00 2001 From: Christian Carollo Date: Thu, 24 Sep 2026 22:04:12 -0500 Subject: [PATCH] feat: add _meta.codex.strictMcpConfig for client-only MCP servers When a session request carries _meta.codex.strictMcpConfig = true, the session's MCP servers are exactly the client's: every server in the effective Codex configuration for cwd is disabled by name, and the apps, plugins and skill_mcp_dependency_install features are turned off. A client server whose name matches a configured one is rejected with invalid_params. Applies to session/new, load, resume and fork. Co-Authored-By: Claude Opus 5.5 --- README.md | 14 + src/CodexAcpClient.ts | 96 ++++++- .../CodexACPAgent/mcp-config-merge.test.ts | 69 +++++ .../CodexACPAgent/strict-mcp-config.test.ts | 243 ++++++++++++++++++ 4 files changed, 413 insertions(+), 9 deletions(-) create mode 100644 src/__tests__/CodexACPAgent/strict-mcp-config.test.ts diff --git a/README.md b/README.md index 35bf5d3a1..33a9ae04f 100644 --- a/README.md +++ b/README.md @@ -100,6 +100,20 @@ Codex can keep a shell command running after a turn continues. AIR clients can s See [docs/async-tasks.md](docs/async-tasks.md) for the capability, lifecycle events, and stop request. +### Client-only MCP servers + +By default, a session gets the MCP servers in the user's Codex configuration plus the client's `mcpServers`. A client that needs a session to have exactly its own servers sets `strictMcpConfig` in the request's `_meta`, on `session/new`, `session/load`, `session/resume` and `session/fork`: + +```json +{ + "cwd": "/path/to/project", + "mcpServers": [], + "_meta": { "codex": { "strictMcpConfig": true } } +} +``` + +The adapter then disables every MCP server that Codex's configuration would load for `cwd`, and turns off the `apps`, `plugins` and `skill_mcp_dependency_install` features, which add MCP servers of their own. The rest of the configuration still applies. A client server whose name matches a configured one is rejected with `invalid_params`, because Codex would merge the two entries. Pass it under another name. + ## License By contributing, you agree that your contributions will be licensed under the Apache 2.0 License. diff --git a/src/CodexAcpClient.ts b/src/CodexAcpClient.ts index 19c81e9f9..9593bf5af 100644 --- a/src/CodexAcpClient.ts +++ b/src/CodexAcpClient.ts @@ -588,7 +588,7 @@ export class CodexAcpClient { const response = await this.resumeThread({ excludeTurns: true, - config: await this.createSessionConfig(request.cwd, additionalDirectories, request.mcpServers ?? []), + config: await this.createSessionConfig(request.cwd, additionalDirectories, request.mcpServers ?? [], readStrictMcpConfig(request._meta)), cwd: request.cwd, modelProvider: await this.getResumeModelProvider(), threadId: request.sessionId, @@ -609,11 +609,12 @@ export class CodexAcpClient { async forkSession(request: acp.ForkSessionRequest): Promise { const additionalDirectories = readAdditionalDirectories(request.cwd, request.additionalDirectories, request._meta); + const strictMcpConfig = readStrictMcpConfig(request._meta); return await runForkSession(request, additionalDirectories, { codexClient: this.codexClient, refreshSkills: (cwd, directories) => this.refreshSkills(cwd, directories), createSessionConfig: (cwd, directories, mcpServers) => - this.createSessionConfig(cwd, directories, mcpServers), + this.createSessionConfig(cwd, directories, mcpServers, strictMcpConfig), getResumeModelProvider: () => this.getResumeModelProvider(), fetchAvailableModels: () => this.fetchAvailableModels(), createCurrentModelId: (models, model, reasoningEffort) => @@ -628,7 +629,7 @@ export class CodexAcpClient { const response = await this.resumeThread({ excludeTurns: true, - config: await this.createSessionConfig(request.cwd, additionalDirectories, request.mcpServers ?? []), + config: await this.createSessionConfig(request.cwd, additionalDirectories, request.mcpServers ?? [], readStrictMcpConfig(request._meta)), cwd: request.cwd, modelProvider: await this.getResumeModelProvider(), threadId: request.sessionId, @@ -669,7 +670,7 @@ export class CodexAcpClient { await this.refreshSkills(request.cwd, additionalDirectories); const response = await this.codexClient.threadStart({ - config: await this.createSessionConfig(request.cwd, additionalDirectories, request.mcpServers), + config: await this.createSessionConfig(request.cwd, additionalDirectories, request.mcpServers, readStrictMcpConfig(request._meta)), modelProvider: this.getModelProvider(), cwd: request.cwd, }); @@ -807,7 +808,8 @@ export class CodexAcpClient { private async createSessionConfig( projectPath: string, additionalDirectories: string[], - mcpServers: Array + mcpServers: Array, + strictMcpConfig: boolean, ): Promise { const sessionRoots = [projectPath, ...additionalDirectories]; const activeProvider = this.gatewayConfig @@ -830,14 +832,19 @@ export class CodexAcpClient { }])), }; const configWithWorkspaceRoots = mergeSandboxWorkspaceWriteRoots(mergedConfig, additionalDirectories); - if (mcpServers.length === 0) { - return configWithWorkspaceRoots; - } - const requestedServers = mcpServers.map(mcp => ({ name: sanitizeMcpServerName(mcp.name), server: mcp, })); + + if (strictMcpConfig) { + return await this.createStrictMcpSessionConfig(projectPath, configWithWorkspaceRoots, requestedServers); + } + + if (mcpServers.length === 0) { + return configWithWorkspaceRoots; + } + let serversToConfigure = requestedServers; if (shouldDeduplicateMcpConflicts()) { // Prevents Codex from deep-merging incompatible field types, such as url and stdio schemas. @@ -854,6 +861,54 @@ export class CodexAcpClient { }; } + /** + * Session config for `_meta.codex.strictMcpConfig`: the session's MCP servers are + * exactly the client's. Every server Codex has configured is disabled by name, and + * the features that add MCP servers of their own are turned off. + */ + private async createStrictMcpSessionConfig( + projectPath: string, + config: JsonObject, + requestedServers: Array<{ name: string, server: McpServer }>, + ): Promise { + const configuredNames = await this.getEffectiveMcpServerNames(projectPath); + // A client server cannot replace a configured one: Codex deep-merges the two + // entries, so fields of the configured server would leak into the client's. + const conflicts = requestedServers + .map(mcp => mcp.name) + .filter(name => configuredNames.has(name)); + if (conflicts.length > 0) { + throw RequestError.invalidParams( + undefined, + `_meta.codex.strictMcpConfig: MCP server name(s) ${conflicts.join(", ")} are also configured in Codex; pass them under different names`, + ); + } + + const features = isJsonObject(config["features"]) ? config["features"] : {}; + return { + ...config, + features: { + ...features, + ...STRICT_MCP_DISABLED_FEATURES, + }, + "mcp_servers": { + ...Object.fromEntries([...configuredNames].map(name => [name, {enabled: false}])), + ...Object.fromEntries(requestedServers.map(mcp => [mcp.name, this.createMcpSeverConfig(mcp.server)])), + }, + }; + } + + /** + * Names of the MCP servers Codex will load for `projectPath`: the effective config + * only. Servers in disabled layers (an untrusted project, for example) never load, + * and naming them in the session config would create entries with no transport. + */ + private async getEffectiveMcpServerNames(projectPath: string): Promise> { + const response = await this.codexClient.configRead({ includeLayers: false, cwd: projectPath }); + const effectiveMcpServers = response?.config?.["mcp_servers"]; + return new Set(isJsonObject(effectiveMcpServers) ? Object.keys(effectiveMcpServers) : []); + } + private async getConfigMcpServerNames(projectPath: string): Promise> { const response = await this.codexClient.configRead({ includeLayers: true, cwd: projectPath }); const effectiveMcpServers = response?.config?.["mcp_servers"]; @@ -1314,6 +1369,29 @@ interface GatewayConfig { } } +/** + * Features that add MCP servers of their own. `strictMcpConfig` turns them off so a + * session's servers are exactly the ones the client passed. + */ +const STRICT_MCP_DISABLED_FEATURES = { + apps: false, + plugins: false, + skill_mcp_dependency_install: false, +} as const; + +/** Reads `_meta.codex.strictMcpConfig`. Absent means false; any non-boolean is rejected. */ +function readStrictMcpConfig(meta?: Record | null): boolean { + const codexMeta = meta?.["codex"] as JsonValue | undefined; + if (!isJsonObject(codexMeta) || !("strictMcpConfig" in codexMeta)) { + return false; + } + const value = codexMeta["strictMcpConfig"]; + if (typeof value !== "boolean") { + throw RequestError.invalidParams(undefined, "_meta.codex.strictMcpConfig must be a boolean"); + } + return value; +} + function readMetaAdditionalRoots(meta?: Record | null): string[] | undefined { const rawRoots = meta?.["additionalRoots"]; if (!Array.isArray(rawRoots)) { diff --git a/src/__tests__/CodexACPAgent/mcp-config-merge.test.ts b/src/__tests__/CodexACPAgent/mcp-config-merge.test.ts index 94a92d710..0570dbefb 100644 --- a/src/__tests__/CodexACPAgent/mcp-config-merge.test.ts +++ b/src/__tests__/CodexACPAgent/mcp-config-merge.test.ts @@ -97,6 +97,75 @@ url = "https://example.com/mcp" })).resolves.toBeDefined(); }); + const clientMcp: McpServerStdio = { + name: "client-mcp", + command: path.resolve("node_modules/.bin/mcp-hello-world"), + args: [], + env: [], + }; + + /** MCP servers the session will run: every server its thread reports, less the disabled ones. */ + async function sessionMcpServerNames(meta?: Record): Promise { + const codexAcpAgent = fixture.getCodexAcpAgent(); + const newSessionResponse = await codexAcpAgent.newSession({ + cwd: projectPath, + mcpServers: [clientMcp], + ...(meta && {_meta: meta}), + }); + const status = await fixture.getCodexAcpClient().appServerClient.listMcpServerStatus({ + threadId: newSessionResponse.sessionId, + }); + return status.data + .filter(server => server.runtimeStatus !== "disabled") + .map(server => server.name) + .sort(); + } + + it('should give a strictMcpConfig session only the ACP MCP servers', async () => { + const codexAcpAgent = fixture.getCodexAcpAgent(); + await codexAcpAgent.initialize({protocolVersion: 1}); + fixture.getCodexAcpClient().authRequired = vi.fn().mockResolvedValue(false); + + expect(await sessionMcpServerNames()).toEqual(["client-mcp", "shared-mcp"]); + expect(await sessionMcpServerNames({codex: {strictMcpConfig: true}})).toEqual(["client-mcp"]); + }); + + it('should disable a trusted project MCP server in a strictMcpConfig session', async () => { + fs.appendFileSync( + path.join(codexHome, "config.toml"), + `\n[projects.${JSON.stringify(fs.realpathSync(projectPath))}]\ntrust_level = "trusted"\n`, + "utf8", + ); + const codexAcpAgent = fixture.getCodexAcpAgent(); + await codexAcpAgent.initialize({protocolVersion: 1}); + fixture.getCodexAcpClient().authRequired = vi.fn().mockResolvedValue(false); + + expect(await sessionMcpServerNames()).toEqual(["client-mcp", "project-mcp", "shared-mcp"]); + expect(await sessionMcpServerNames({codex: {strictMcpConfig: true}})).toEqual(["client-mcp"]); + }); + + it('should reject a strictMcpConfig session whose ACP MCP server shares a configured 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: [], + env: [], + }; + + await expect(codexAcpAgent.newSession({ + cwd: projectPath, + mcpServers: [conflictingMcp], + _meta: {codex: {strictMcpConfig: true}}, + })).rejects.toMatchObject({ + code: -32602, + message: expect.stringContaining("shared-mcp"), + }); + }); + 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(); diff --git a/src/__tests__/CodexACPAgent/strict-mcp-config.test.ts b/src/__tests__/CodexACPAgent/strict-mcp-config.test.ts new file mode 100644 index 000000000..e55e9e0f0 --- /dev/null +++ b/src/__tests__/CodexACPAgent/strict-mcp-config.test.ts @@ -0,0 +1,243 @@ +import {beforeEach, describe, expect, it, vi} from 'vitest'; +import type {McpServerStdio} from "@agentclientprotocol/sdk"; +import {createCodexMockTestFixture, createTestModel} from "../acp-test-utils"; + +const clientServer: McpServerStdio = { + name: "client-mcp", + command: "/usr/local/bin/client-mcp", + args: ["--stdio"], + env: [{name: "TOKEN_ENV", value: "client"}], +}; + +const strictMeta = {codex: {strictMcpConfig: true}}; + +/** + * The effective Codex config: a user-level and a trusted project-level MCP server. + * A disabled layer's server never loads, so it is not in the effective config. + */ +const configReadResponse = { + config: { + mcp_servers: { + "user-mcp": {url: "https://example.com/user"}, + "project-mcp": {command: "project-mcp"}, + }, + }, + origins: {}, + layers: [ + { + name: {type: "project", dotCodexFolder: "/elsewhere/.codex"}, + version: "1", + config: {mcp_servers: {"untrusted-mcp": {command: "untrusted-mcp"}}}, + disabledReason: "untrusted project", + }, + ], +}; + +function setUp() { + const fixture = createCodexMockTestFixture(); + const codexAcpClient = fixture.getCodexAcpClient(); + const codexAppServerClient = fixture.getCodexAppServerClient(); + + vi.spyOn(codexAppServerClient, "skillsExtraRootsSet").mockResolvedValue(undefined); + vi.spyOn(codexAppServerClient, "listSkills").mockResolvedValue({data: []}); + const configReadSpy = vi.spyOn(codexAppServerClient, "configRead").mockResolvedValue(configReadResponse as any); + const threadStartSpy = vi.spyOn(codexAppServerClient, "threadStart").mockResolvedValue({ + thread: {id: "thread-id"} as any, + model: "gpt-5", + reasoningEffort: "medium", + serviceTier: null, + } as any); + const threadResumeSpy = vi.spyOn(codexAppServerClient, "threadResume").mockResolvedValue({ + thread: {id: "thread-id"} as any, + model: "gpt-5", + reasoningEffort: "medium", + serviceTier: null, + } as any); + vi.spyOn(codexAppServerClient, "threadReadWithHistory").mockResolvedValue({ + thread: {id: "thread-id"} as any, + }); + vi.spyOn(codexAppServerClient, "listModels").mockResolvedValue({ + data: [createTestModel({id: "gpt-5"})], + nextCursor: null, + }); + + return {codexAcpClient, configReadSpy, threadStartSpy, threadResumeSpy}; +} + +describe('_meta.codex.strictMcpConfig', () => { + + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('gives a session only the client MCP servers and disables every configured one', async () => { + const {codexAcpClient, threadStartSpy} = setUp(); + + await codexAcpClient.newSession({ + cwd: "/workspace", + mcpServers: [clientServer], + _meta: strictMeta, + }); + + const config = threadStartSpy.mock.calls[0]![0].config!; + expect(config["mcp_servers"]).toEqual({ + "user-mcp": {enabled: false}, + "project-mcp": {enabled: false}, + "client-mcp": { + command: "/usr/local/bin/client-mcp", + args: ["--stdio"], + env: {TOKEN_ENV: "client"}, + }, + }); + }); + + it('turns off the features that add MCP servers of their own', async () => { + const {codexAcpClient, threadStartSpy} = setUp(); + + await codexAcpClient.newSession({ + cwd: "/workspace", + mcpServers: [clientServer], + _meta: strictMeta, + }); + + expect(threadStartSpy.mock.calls[0]![0].config?.["features"]).toEqual({ + cwd_relative_turn_diffs: false, + apps: false, + plugins: false, + skill_mcp_dependency_install: false, + }); + }); + + it('disables configured servers even when the client passes none', async () => { + const {codexAcpClient, threadStartSpy} = setUp(); + + await codexAcpClient.newSession({ + cwd: "/workspace", + mcpServers: [], + _meta: strictMeta, + }); + + expect(threadStartSpy.mock.calls[0]![0].config?.["mcp_servers"]).toEqual({ + "user-mcp": {enabled: false}, + "project-mcp": {enabled: false}, + }); + }); + + it('does not name servers from disabled config layers', async () => { + const {codexAcpClient, configReadSpy, threadStartSpy} = setUp(); + + await codexAcpClient.newSession({ + cwd: "/workspace", + mcpServers: [{...clientServer, name: "untrusted-mcp"}], + _meta: strictMeta, + }); + + expect(configReadSpy).toHaveBeenCalledWith({includeLayers: false, cwd: "/workspace"}); + expect(threadStartSpy.mock.calls[0]![0].config?.["mcp_servers"]).toEqual({ + "user-mcp": {enabled: false}, + "project-mcp": {enabled: false}, + "untrusted-mcp": expect.objectContaining({command: "/usr/local/bin/client-mcp"}), + }); + }); + + it('rejects a client server whose name is also configured', async () => { + const {codexAcpClient, threadStartSpy} = setUp(); + + await expect(codexAcpClient.newSession({ + cwd: "/workspace", + mcpServers: [{...clientServer, name: "user-mcp"}], + _meta: strictMeta, + })).rejects.toMatchObject({ + code: -32602, + message: expect.stringContaining("user-mcp"), + }); + expect(threadStartSpy).not.toHaveBeenCalled(); + }); + + it('rejects a value that is not a boolean', async () => { + const {codexAcpClient, threadStartSpy} = setUp(); + + await expect(codexAcpClient.newSession({ + cwd: "/workspace", + mcpServers: [clientServer], + _meta: {codex: {strictMcpConfig: "yes"}}, + })).rejects.toMatchObject({code: -32602}); + expect(threadStartSpy).not.toHaveBeenCalled(); + }); + + it('leaves configured servers and features alone when absent or false', async () => { + const {codexAcpClient, threadStartSpy} = setUp(); + + await codexAcpClient.newSession({cwd: "/workspace", mcpServers: [clientServer]}); + await codexAcpClient.newSession({ + cwd: "/workspace", + mcpServers: [clientServer], + _meta: {codex: {strictMcpConfig: false}}, + }); + + for (const [params] of threadStartSpy.mock.calls) { + expect(Object.keys(params.config?.["mcp_servers"] as object)).toEqual(["client-mcp"]); + expect(params.config?.["features"]).toEqual({cwd_relative_turn_diffs: false}); + } + }); + + it('applies to loaded and resumed sessions', async () => { + const {codexAcpClient, threadResumeSpy} = setUp(); + + await codexAcpClient.loadSession({ + sessionId: "load-id", + cwd: "/workspace", + mcpServers: [clientServer], + _meta: strictMeta, + }); + await codexAcpClient.resumeSession({ + sessionId: "resume-id", + cwd: "/workspace", + mcpServers: [clientServer], + _meta: strictMeta, + }); + + for (const [params] of threadResumeSpy.mock.calls) { + expect(params.config?.["mcp_servers"]).toMatchObject({ + "user-mcp": {enabled: false}, + "project-mcp": {enabled: false}, + "client-mcp": {command: "/usr/local/bin/client-mcp"}, + }); + expect(params.config?.["features"]).toMatchObject({apps: false, plugins: false}); + } + expect(threadResumeSpy).toHaveBeenCalledTimes(2); + }); + + it('applies to forked sessions', async () => { + const fixture = createCodexMockTestFixture(); + const codexAcpClient = fixture.getCodexAcpClient(); + const codexAppServerClient = fixture.getCodexAppServerClient(); + vi.spyOn(codexAppServerClient, "skillsExtraRootsSet").mockResolvedValue(undefined); + vi.spyOn(codexAppServerClient, "listSkills").mockResolvedValue({data: []}); + vi.spyOn(codexAppServerClient, "configRead").mockResolvedValue(configReadResponse as any); + const threadForkSpy = vi.spyOn(codexAppServerClient, "threadFork").mockResolvedValue({ + thread: {id: "fork-id"} as any, + model: "gpt-5", + modelProvider: "openai", + reasoningEffort: "medium", + serviceTier: null, + } as any); + vi.spyOn(codexAppServerClient, "threadUnsubscribe").mockResolvedValue({status: "unsubscribed"}); + vi.spyOn(codexAppServerClient, "listModels").mockResolvedValue({ + data: [createTestModel({id: "gpt-5"})], + nextCursor: null, + }); + + await codexAcpClient.forkSession({ + sessionId: "source-id", + cwd: "/workspace", + mcpServers: [clientServer], + _meta: strictMeta, + }); + + const config = threadForkSpy.mock.calls[0]![0].config!; + expect(Object.keys(config["mcp_servers"] as object).sort()).toEqual(["client-mcp", "project-mcp", "user-mcp"]); + expect(config["mcp_servers"]).toMatchObject({"user-mcp": {enabled: false}, "project-mcp": {enabled: false}}); + expect(config["features"]).toMatchObject({apps: false, plugins: false, skill_mcp_dependency_install: false}); + }); +});