diff --git a/README.md b/README.md index 7d03989f4..c0dbdfe75 100644 --- a/README.md +++ b/README.md @@ -108,6 +108,20 @@ Codex can keep a shell command running after a turn continues. AIR clients can s See [AIR extensions](docs/air-extensions.md#async-tasks) 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 b7271800b..76d536071 100644 --- a/src/CodexAcpClient.ts +++ b/src/CodexAcpClient.ts @@ -32,7 +32,7 @@ import {isDeepStrictEqual} from "node:util"; import {logger} from "./Logger"; import {isAccountReadAuthFailureError, isAccountReadUnavailableError} from "./CodexThreadErrors"; import {listedSessionTitle, normalizeSessionTitle} from "./SessionTitle"; -import {type AcpMcpServer, type WithAcpMcpServers, normalizeMcpServers, toCodexMcpServerConfig} from "./McpServerConfig"; +import {type AcpMcpServer, type NormalizedMcpServer, type WithAcpMcpServers, normalizeMcpServers, toCodexMcpServerConfig} from "./McpServerConfig"; import type { AccountUpdatedNotification, ApprovalsReviewer, @@ -694,7 +694,7 @@ export class CodexAcpClient { await this.refreshSkills(request.cwd, additionalDirectories); const {config, skippedMcpServers, defaultPermissionProfileId, ...permissionProfileMetadata} = - await this.createSessionConfig(request.cwd, additionalDirectories, request.mcpServers ?? []); + await this.createSessionConfig(request.cwd, additionalDirectories, request.mcpServers ?? [], readStrictMcpConfig(request._meta)); const response = await this.resumeThreadWithInstructions({ excludeTurns: true, config, @@ -732,11 +732,12 @@ export class CodexAcpClient { ): Promise { const instructions = await this.resolveSessionInstructions(request, previousInstructions); 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) => @@ -757,7 +758,7 @@ export class CodexAcpClient { await this.refreshSkills(request.cwd, additionalDirectories); const {config, skippedMcpServers, defaultPermissionProfileId, ...permissionProfileMetadata} = - await this.createSessionConfig(request.cwd, additionalDirectories, request.mcpServers ?? []); + await this.createSessionConfig(request.cwd, additionalDirectories, request.mcpServers ?? [], readStrictMcpConfig(request._meta)); const response = await this.resumeThreadWithInstructions({ excludeTurns: true, config, @@ -864,7 +865,7 @@ export class CodexAcpClient { await this.refreshSkills(request.cwd, additionalDirectories); const {config, skippedMcpServers, defaultPermissionProfileId: _defaultPermissionProfileId, ...permissionProfileMetadata} = - await this.createSessionConfig(request.cwd, additionalDirectories, request.mcpServers ?? []); + await this.createSessionConfig(request.cwd, additionalDirectories, request.mcpServers ?? [], readStrictMcpConfig(request._meta)); const response = await this.codexClient.threadStart({ config, modelProvider: this.getModelProvider(), @@ -1139,7 +1140,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 @@ -1172,6 +1174,14 @@ export class CodexAcpClient { ...(effectiveConfig?.config?.approval_policy != null && {approvalPolicy: effectiveConfig.config.approval_policy}), ...(effectiveConfig?.config?.approvals_reviewer != null && {approvalsReviewer: effectiveConfig.config.approvals_reviewer}), }; + if (strictMcpConfig) { + const requested = normalizeMcpServers(mcpServers).map(server => ({name: server.name, server})); + return { + config: this.createStrictMcpSessionConfig(effectiveConfig, configWithWorkspaceRoots, requested), + skippedMcpServers: [], + ...permissionProfileMetadata, + }; + } if (mcpServers.length === 0) { return {config: configWithWorkspaceRoots, skippedMcpServers: [], ...permissionProfileMetadata}; } @@ -1206,6 +1216,43 @@ 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 createStrictMcpSessionConfig( + effectiveConfig: ConfigReadResponse, + config: JsonObject, + requestedServers: Array<{ name: string, server: NormalizedMcpServer }>, + ): JsonObject { + const configuredNames = getEffectiveMcpServerNames(effectiveConfig); + // 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, toCodexMcpServerConfig(mcp.server)])), + }, + }; + } + private getConfigMcpServerNames(response: ConfigReadResponse): Set { const effectiveMcpServers = response?.config?.["mcp_servers"]; const configLayers = response?.layers ?? []; @@ -1738,6 +1785,39 @@ function sameProviderConfig(left: GatewayConfig, right: GatewayConfig): boolean return left.modelProvider === right.modelProvider && isDeepStrictEqual(left.config, right.config); } +/** + * 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; + +/** + * Names of the MCP servers Codex will load: 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. + */ +function getEffectiveMcpServerNames(response: ConfigReadResponse): Set { + const effectiveMcpServers = response?.config?.["mcp_servers"]; + return new Set(isJsonObject(effectiveMcpServers) ? Object.keys(effectiveMcpServers) : []); +} + +/** 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 baf4d439b..1692090ed 100644 --- a/src/__tests__/CodexACPAgent/mcp-config-merge.test.ts +++ b/src/__tests__/CodexACPAgent/mcp-config-merge.test.ts @@ -101,6 +101,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().readAuthRequirement = vi.fn().mockResolvedValue({required: false, account: null}); + + 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().readAuthRequirement = vi.fn().mockResolvedValue({required: false, account: null}); + + 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().readAuthRequirement = vi.fn().mockResolvedValue({required: false, account: null}); + + 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 wait for an ACP MCP that a disabled MCP of the same name in the config replaces', async () => { const codexAcpAgent = fixture.getCodexAcpAgent(); await codexAcpAgent.initialize({protocolVersion: 1}); 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..2f77ef2f8 --- /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", turns: []} 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: true, 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}); + }); +});