diff --git a/package-lock.json b/package-lock.json index 41235615..a7db50e5 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1284,6 +1284,7 @@ "integrity": "sha512-GNWcUTRBgIRJD5zj+Tq0fKOJ5XZajIiBroOF0yvj2bSU1WvNdYS/dn9UxwsujGW4JX06dnHyjV2y9rRaybH0iQ==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "undici-types": "~7.16.0" } @@ -1930,6 +1931,7 @@ "integrity": "sha512-F2X8g9P1X7uCPZMA3MVf9wcTqlyNp7IhH5qPCI0izhaOIYXaW9L535tGA3qmjRzpH+bZczqq7hVKxTR4NWnu+g==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "accepts": "~1.3.8", "array-flatten": "1.1.1", @@ -2632,6 +2634,7 @@ "integrity": "sha512-5gTmgEY/sqK6gFXLIsQNH19lWb4ebPDLA4SdLP7dsWkIXHWlG66oPuVvXSGFPppYZz8ZDZq0dYYrbHfBCVUb1Q==", "dev": true, "license": "MIT", + "peer": true, "engines": { "node": ">=12" }, @@ -2923,8 +2926,7 @@ "resolved": "https://registry.npmjs.org/scheduler/-/scheduler-0.27.0.tgz", "integrity": "sha512-eNv+WrVbKu1f3vbYJT/xtiF5syA5HPIMtf9IgY/nKg0sWqzAUEvqY/xm7OcZc/qafLx/iO9FgOmeSAp4v5ti/Q==", "dev": true, - "license": "MIT", - "peer": true + "license": "MIT" }, "node_modules/send": { "version": "0.19.2", @@ -3188,6 +3190,7 @@ "integrity": "sha512-ytQKuwgmrrkDTFP4LjR0ToE2nqgy886GpvRSpU0JAnrdBYppuY5rLkRUYPU1yCryb24SsKBTL/hlDQAEFVwtZg==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "esbuild": "~0.25.0", "get-tsconfig": "^4.7.5" @@ -3273,6 +3276,7 @@ "integrity": "sha512-BxAKBWmIbrDgrokdGZH1IgkIk/5mMHDreLDmCJ0qpyJaAteP8NvMhkwr/ZCQNqNH97bw/dANTE9PDzqwJghfMQ==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "esbuild": "^0.25.0", "fdir": "^6.5.0", @@ -3490,6 +3494,7 @@ "resolved": "https://registry.npmjs.org/zod/-/zod-3.25.76.tgz", "integrity": "sha512-gzUt/qt81nXsFGKIFcC3YnfEAx5NkunCfnDlvuBSSFS02bcXu4Lmea0AFIUwbLWxWPx3d9p8S5QoaujKcNQxcQ==", "license": "MIT", + "peer": true, "funding": { "url": "https://github.com/sponsors/colinhacks" } diff --git a/src/ApprovalOptionId.ts b/src/ApprovalOptionId.ts index ccec90ed..817fe7c2 100644 --- a/src/ApprovalOptionId.ts +++ b/src/ApprovalOptionId.ts @@ -1,7 +1,11 @@ export const ApprovalOptionId = { AllowOnce: "allow_once", - AllowAlways: "allow_always", + AllowForSession: "allow_for_session", + AllowPersist: "allow_persist", + AllowCommandPrefixRule: "allow_command_prefix_rule", + ApplyNetworkPolicyAmendment: "apply_network_policy_amendment", RejectOnce: "reject_once", + Cancel: "cancel", } as const; export type ApprovalOptionId = typeof ApprovalOptionId[keyof typeof ApprovalOptionId]; diff --git a/src/CodexAcpClient.ts b/src/CodexAcpClient.ts index 514e53b5..2ece9360 100644 --- a/src/CodexAcpClient.ts +++ b/src/CodexAcpClient.ts @@ -23,6 +23,7 @@ import {logger} from "./Logger"; import type { AccountLoginCompletedNotification, AccountUpdatedNotification, + ConfigEdit, GetAccountResponse, ListMcpServerStatusParams, ListMcpServerStatusResponse, @@ -309,6 +310,24 @@ export class CodexAcpClient { } } + // Makes an ACP-provided MCP server durable so Codex core can append + // mcp_servers..tools..approval_mode afterward: + // https://github.com/openai/codex/blob/main/codex-rs/app-server/src/config_api.rs + async persistMcpServer(mcpServer: McpServer): Promise { + const edits: ConfigEdit[] = [{ + keyPath: `mcp_servers.${mcpServer.name}`, + value: this.createMcpSeverConfig(mcpServer), + mergeStrategy: "upsert", + }]; + + await this.codexClient.configBatchWrite({ + edits, + filePath: null, + expectedVersion: null, + reloadUserConfig: true, + }); + } + private getModelProvider(): string | null { return this.gatewayConfig?.modelProvider ?? this.modelProvider; } diff --git a/src/CodexAcpServer.ts b/src/CodexAcpServer.ts index 4159656a..1536a5a5 100644 --- a/src/CodexAcpServer.ts +++ b/src/CodexAcpServer.ts @@ -51,7 +51,7 @@ export interface SessionState { rateLimits: RateLimitsMap | null; account: Account | null; cwd: string; - sessionMcpServers?: Array; + sessionMcpServers: Map; } interface PendingMcpStartupSession { @@ -161,7 +161,6 @@ export class CodexAcpServer implements acp.Agent { const accountResponse = await this.runWithProcessCheck(() => this.codexAcpClient.getAccount()); const {sessionId, currentModelId, models} = sessionMetadata; - const sessionMcpServers = this.resolveSessionMcpServers(requestedMcpServers, "sessionId" in request); const currentModel = this.findCurrentModel(models, currentModelId); const sessionState: SessionState = { sessionId: sessionId, @@ -176,7 +175,7 @@ export class CodexAcpServer implements acp.Agent { rateLimits: null, account: accountResponse.account, cwd: request.cwd, - sessionMcpServers: sessionMcpServers, + sessionMcpServers: this.createSessionMcpServers(requestedMcpServers, "sessionId" in request), } this.sessions.set(sessionId, sessionState); @@ -372,7 +371,6 @@ export class CodexAcpServer implements acp.Agent { const accountResponse = await this.runWithProcessCheck(() => this.codexAcpClient.getAccount()); const {sessionId, currentModelId, models, thread} = sessionMetadata; - const sessionMcpServers = this.resolveSessionMcpServers(requestedMcpServers, true); const currentModel = this.findCurrentModel(models, currentModelId); const sessionState: SessionState = { sessionId: sessionId, @@ -387,7 +385,7 @@ export class CodexAcpServer implements acp.Agent { rateLimits: null, account: accountResponse.account, cwd: request.cwd, - sessionMcpServers: sessionMcpServers, + sessionMcpServers: this.createSessionMcpServers(requestedMcpServers, true), }; this.sessions.set(sessionId, sessionState); @@ -641,24 +639,23 @@ export class CodexAcpServer implements acp.Agent { return sessionState; } - private resolveSessionMcpServers( + private createSessionMcpServers( mcpServers: Array, recoverFromStartup: boolean, - ): Array { + ): Map { // Explicit MCP servers from the request are the primary source of truth for the session. - const requestedServerNames = getRequestedMcpServerNames(mcpServers); - if (requestedServerNames.length > 0) { - return requestedServerNames; + if (mcpServers.length > 0) { + return new Map(mcpServers.map(server => [server.name, server])); } // Fresh sessions without MCP config should not inherit any session MCP state. if (!recoverFromStartup) { - return []; + return new Map(); } // Without a thread-scoped startup completion event, loadSession/resumeSession can no longer // recover omitted session MCP server names. Treat the session set as unknown unless ACP // explicitly provided mcpServers in the request. logger.log("Skipping MCP server recovery for load/resume without explicit mcpServers"); - return []; + return new Map(); } private publishMcpStartupStatusAsync(sessionId: string): void { @@ -719,7 +716,7 @@ export class CodexAcpServer implements acp.Agent { try { const eventHandler = new CodexEventHandler(this.connection, sessionState); const approvalHandler = new CodexApprovalHandler(this.connection, sessionState); - const elicitationHandler = new CodexElicitationHandler(this.connection, sessionState); + const elicitationHandler = new CodexElicitationHandler(this.connection, sessionState, this.codexAcpClient); await this.codexAcpClient.subscribeToSessionEvents(params.sessionId, (event) => { elicitationHandler.handleNotification(event); @@ -868,7 +865,3 @@ export class CodexAcpServer implements acp.Agent { } } } - -function getRequestedMcpServerNames(mcpServers: Array): Array { - return Array.from(new Set(mcpServers.map(server => server.name))); -} diff --git a/src/CodexAppServerClient.ts b/src/CodexAppServerClient.ts index afd333ff..3df3ce96 100644 --- a/src/CodexAppServerClient.ts +++ b/src/CodexAppServerClient.ts @@ -9,6 +9,8 @@ import type { AccountLoginCompletedNotification, AccountUpdatedNotification, ConfigReadParams, ConfigReadResponse, + ConfigBatchWriteParams, + ConfigWriteResponse, GetAccountParams, GetAccountResponse, ListMcpServerStatusParams, @@ -195,6 +197,10 @@ export class CodexAppServerClient { return await this.sendRequest({ method: "config/read", params: params }); } + async configBatchWrite(params: ConfigBatchWriteParams): Promise { + return await this.sendRequest({ method: "config/batchWrite", params }); + } + getMcpServerStartupVersion(): number { return this.mcpServerStartupVersion; } diff --git a/src/CodexApprovalHandler.ts b/src/CodexApprovalHandler.ts index 0eddf3ac..2978891c 100644 --- a/src/CodexApprovalHandler.ts +++ b/src/CodexApprovalHandler.ts @@ -2,6 +2,7 @@ import * as acp from "@agentclientprotocol/sdk"; import type {SessionState} from "./CodexAcpServer"; import type {ApprovalHandler} from "./CodexAppServerClient"; import type { + CommandExecutionApprovalDecision, CommandExecutionRequestApprovalParams, CommandExecutionRequestApprovalResponse, FileChangeRequestApprovalParams, @@ -14,10 +15,17 @@ import {ApprovalOptionId} from "./ApprovalOptionId"; const APPROVAL_OPTIONS: acp.PermissionOption[] = [ { optionId: ApprovalOptionId.AllowOnce, name: "Allow Once", kind: "allow_once" }, - { optionId: ApprovalOptionId.AllowAlways, name: "Allow for Session", kind: "allow_always" }, + { optionId: ApprovalOptionId.AllowForSession, name: "Allow for Session", kind: "allow_always" }, { optionId: ApprovalOptionId.RejectOnce, name: "Reject", kind: "reject_once" }, ]; +// Pair each displayed ACP option with the exact Codex decision it represents, +// so response conversion does not reconstruct decisions from labels or metadata. +type CommandDecisionOption = { + option: acp.PermissionOption; + decision: CommandExecutionApprovalDecision; +}; + export class CodexApprovalHandler implements ApprovalHandler { private readonly connection: acp.AgentSideConnection; private readonly sessionState: SessionState; @@ -37,7 +45,7 @@ export class CodexApprovalHandler implements ApprovalHandler { const sessionId = this.sessionState.sessionId; const acpRequest = this.buildCommandPermissionRequest(sessionId, params); const response = await this.connection.requestPermission(acpRequest); - return this.convertCommandResponse(response); + return this.convertCommandResponse(response, params); } catch (error) { logger.error("Error requesting command execution permission", error); return { decision: "cancel" }; @@ -72,10 +80,151 @@ export class CodexApprovalHandler implements ApprovalHandler { content: reasonContent ? [reasonContent] : null, rawInput: params.command ? { command: stripShellPrefix(params.command), cwd: params.cwd } : null, }, - options: APPROVAL_OPTIONS, + options: this.buildCommandDecisionOptions(params).map(({ option }) => option), + }; + } + + private buildCommandDecisionOptions( + params: CommandExecutionRequestApprovalParams + ): CommandDecisionOption[] { + // Older app-server versions did not send availableDecisions; they only + // sent proposed amendment fields. Reconstruct that older decision list + // as a compatibility fallback. + const decisions = params.availableDecisions ?? this.buildLegacyFallbackCommandDecisions(params); + let execAmendmentCount = 0; + let networkAmendmentCount = 0; + + return decisions.map((decision) => { + let amendmentIndex = 0; + if (typeof decision !== "string" && "acceptWithExecpolicyAmendment" in decision) { + amendmentIndex = execAmendmentCount++; + } else if (typeof decision !== "string" && "applyNetworkPolicyAmendment" in decision) { + amendmentIndex = networkAmendmentCount++; + } + return this.convertCommandDecisionToOption(decision, amendmentIndex); + }); + } + + private buildLegacyFallbackCommandDecisions( + params: CommandExecutionRequestApprovalParams + ): CommandExecutionApprovalDecision[] { + const decisions: CommandExecutionApprovalDecision[] = ["accept", "acceptForSession"]; + + if (params.proposedExecpolicyAmendment) { + decisions.push({ + acceptWithExecpolicyAmendment: { + execpolicy_amendment: params.proposedExecpolicyAmendment + } + }); + } + + for (const amendment of params.proposedNetworkPolicyAmendments ?? []) { + decisions.push({ + applyNetworkPolicyAmendment: { + network_policy_amendment: amendment + } + }); + } + + decisions.push("decline"); + return decisions; + } + + private convertCommandDecisionToOption( + decision: CommandExecutionApprovalDecision, + amendmentIndex: number + ): CommandDecisionOption { + if (decision === "accept") { + return { + option: { optionId: ApprovalOptionId.AllowOnce, name: "Yes, proceed", kind: "allow_once" }, + decision + }; + } + + if (decision === "acceptForSession") { + return { + option: { + optionId: ApprovalOptionId.AllowForSession, + name: "Yes, and don't ask again for this exact command", + kind: "allow_always" + }, + decision + }; + } + + if (decision === "decline") { + return { + option: { + optionId: ApprovalOptionId.RejectOnce, + name: "No, and tell Codex what to do differently", + kind: "reject_once" + }, + decision + }; + } + + if (decision === "cancel") { + return { + option: { + optionId: ApprovalOptionId.Cancel, + name: "No, and tell Codex what to do differently", + kind: "reject_once" + }, + decision + }; + } + + if ("acceptWithExecpolicyAmendment" in decision) { + // This amendment corresponds to a Codex exec-policy + // `prefix_rule(..., decision="allow")`, not session-scoped approval. + return { + option: { + optionId: this.indexedOptionId(ApprovalOptionId.AllowCommandPrefixRule, amendmentIndex), + name: this.commandPrefixApprovalLabel( + decision.acceptWithExecpolicyAmendment.execpolicy_amendment + ), + kind: "allow_always" + }, + decision + }; + } + + return { + option: { + optionId: this.indexedOptionId(ApprovalOptionId.ApplyNetworkPolicyAmendment, amendmentIndex), + name: this.networkPolicyApprovalLabel( + decision.applyNetworkPolicyAmendment.network_policy_amendment + ), + kind: decision.applyNetworkPolicyAmendment.network_policy_amendment.action === "deny" + ? "reject_always" + : "allow_always" + }, + decision }; } + private commandPrefixApprovalLabel(execpolicyAmendment: string[]): string { + const commandPrefix = execpolicyAmendment.join(" "); + if (commandPrefix === "") { + return "Yes, and don't ask again for similar commands"; + } + return `Yes, and don't ask again for commands that start with \`${commandPrefix}\``; + } + + private networkPolicyApprovalLabel( + amendment: { host: string; action: "allow" | "deny" } + ): string { + const decision = amendment.action === "deny" ? "No" : "Yes"; + return `${decision}, and don't ask again for network access to \`${amendment.host}\``; + } + + // ACP responses only return the selected optionId. The legacy network field + // is plural and availableDecisions is an array, so repeated amendments need + // unique IDs while the common single-amendment ID stays stable. + private indexedOptionId(optionId: ApprovalOptionId, index: number): ApprovalOptionId | string { + return index === 0 ? optionId : `${optionId}:${index}`; + } + private createContentFromReason(reason: string | null): ToolCallContent | null { if (reason === null || reason === "") { return null; @@ -107,20 +256,21 @@ export class CodexApprovalHandler implements ApprovalHandler { } private convertCommandResponse( - response: acp.RequestPermissionResponse + response: acp.RequestPermissionResponse, + params: CommandExecutionRequestApprovalParams ): CommandExecutionRequestApprovalResponse { if (response.outcome.outcome === "cancelled") { return { decision: "cancel" }; } const optionId = response.outcome.optionId; - if (optionId === ApprovalOptionId.AllowOnce) { - return { decision: "accept" }; - } else if (optionId === ApprovalOptionId.AllowAlways) { - return { decision: "acceptForSession" }; - } else { - return { decision: "decline" }; + const selectedOption = this.buildCommandDecisionOptions(params) + .find(({ option }) => option.optionId === optionId); + if (selectedOption) { + return { decision: selectedOption.decision }; } + + return { decision: "decline" }; } private convertFileChangeResponse( @@ -133,7 +283,7 @@ export class CodexApprovalHandler implements ApprovalHandler { const optionId = response.outcome.optionId; if (optionId === ApprovalOptionId.AllowOnce) { return { decision: "accept" }; - } else if (optionId === ApprovalOptionId.AllowAlways) { + } else if (optionId === ApprovalOptionId.AllowForSession) { return { decision: "acceptForSession" }; } else { return { decision: "cancel" }; diff --git a/src/CodexCommands.ts b/src/CodexCommands.ts index c81e6429..f30fa278 100644 --- a/src/CodexCommands.ts +++ b/src/CodexCommands.ts @@ -165,9 +165,7 @@ export class CodexCommands { const resourceCount = (server.resources ?? []).length; return `- ${server.name}: ${toolCount} tools, ${resourceCount} resources, auth=${server.authStatus}`; }); - const sessionServers = sessionState.sessionMcpServers - ? sessionState.sessionMcpServers.map(serverName => `- ${serverName}`) - : []; + const sessionServers = Array.from(sessionState.sessionMcpServers.keys()).map(serverName => `- ${serverName}`); const lines = [...configuredServers, ...sessionServers]; const text = lines.length > 0 ? ["Configured MCP servers:", ...lines].join("\n") diff --git a/src/CodexElicitationHandler.ts b/src/CodexElicitationHandler.ts index 732822bd..c0beaf0e 100644 --- a/src/CodexElicitationHandler.ts +++ b/src/CodexElicitationHandler.ts @@ -1,6 +1,7 @@ import * as acp from "@agentclientprotocol/sdk"; import type { SessionState } from "./CodexAcpServer"; import type { ElicitationHandler } from "./CodexAppServerClient"; +import type { CodexAcpClient } from "./CodexAcpClient"; import type { ServerNotification } from "./app-server"; import type { ItemCompletedNotification, @@ -22,6 +23,20 @@ const OPTION_ALLOW_SESSION = "allow_session"; type PersistValue = "session" | "always"; +function buildToolApprovalOption( + optionId: string, + name: string, + kind: acp.PermissionOption["kind"], + description: string +): acp.PermissionOption { + return { + optionId, + name, + kind, + _meta: { description }, + }; +} + /** * Parses the `persist` field from the elicitation request `_meta`. * Codex advertises which persistence options the client should show. @@ -52,25 +67,48 @@ function isMcpToolCallApproval(meta: unknown): boolean { /** * Builds the ACP permission options for an MCP tool call approval elicitation. - * Always includes "Allow Once"; adds session/always persist options when advertised. + * Always includes "Allow"; adds session/persistent approval options when advertised. */ function buildToolApprovalOptions(persistOptions: Set): acp.PermissionOption[] { const options: acp.PermissionOption[] = [ - { optionId: ApprovalOptionId.AllowOnce, name: "Allow", kind: "allow_once" }, + buildToolApprovalOption( + ApprovalOptionId.AllowOnce, + "Allow", + "allow_once", + "Run the tool and continue." + ), ]; + // Codex advertises MCP tool approval persistence choices in request _meta.persist. + // Only surface scopes the server explicitly offered. if (persistOptions.has("session")) { - options.push({ optionId: OPTION_ALLOW_SESSION, name: "Allow for This Session", kind: "allow_always" }); + options.push(buildToolApprovalOption( + OPTION_ALLOW_SESSION, + "Allow for this session", + "allow_always", + "Run the tool and remember this choice for this session." + )); } if (persistOptions.has("always")) { - options.push({ optionId: ApprovalOptionId.AllowAlways, name: "Allow and Don't Ask Again", kind: "allow_always" }); + options.push(buildToolApprovalOption( + ApprovalOptionId.AllowPersist, + "Always allow", + "allow_always", + "Run the tool and remember this choice for future tool calls." + )); } - options.push({ optionId: "decline", name: "Decline", kind: "reject_once" }); + options.push(buildToolApprovalOption( + "decline", + "Cancel", + "reject_once", + "Cancel this tool call." + )); return options; } export class CodexElicitationHandler implements ElicitationHandler { private readonly connection: acp.AgentSideConnection; private readonly sessionState: SessionState; + private readonly codexAcpClient: CodexAcpClient; // In Rust, the MCP elicitation handler receives ElicitationRequestEvent directly from the MCP // protocol layer, where id is set to "mcp_tool_call_approval_" — the call ID is extracted // by stripping that prefix. @@ -88,9 +126,14 @@ export class CodexElicitationHandler implements ElicitationHandler { // (threadId, serverName). private readonly pendingMcpApprovals = new Map(); - constructor(connection: acp.AgentSideConnection, sessionState: SessionState) { + constructor( + connection: acp.AgentSideConnection, + sessionState: SessionState, + codexAcpClient: CodexAcpClient + ) { this.connection = connection; this.sessionState = sessionState; + this.codexAcpClient = codexAcpClient; } handleNotification(notification: ServerNotification): void { @@ -124,7 +167,7 @@ export class CodexElicitationHandler implements ElicitationHandler { }); } } - return this.convertResponse(response); + return await this.convertResponse(response, params); } catch (error) { logger.error("Error handling MCP elicitation request", error); return { action: "cancel", content: null, _meta: null }; @@ -203,18 +246,29 @@ export class CodexElicitationHandler implements ElicitationHandler { } } - private convertResponse( - response: acp.RequestPermissionResponse - ): McpServerElicitationRequestResponse { + private async convertResponse( + response: acp.RequestPermissionResponse, + params: McpServerElicitationRequestParams + ): Promise { if (response.outcome.outcome === "cancelled") { return { action: "cancel", content: null, _meta: null }; } const optionId = response.outcome.optionId; if (optionId === OPTION_ALLOW_SESSION) { + // This _meta is part of Codex's MCP tool approval response contract. + // It tells app-server to remember this approval for the current session. return { action: "accept", content: null, _meta: { persist: "session" } }; } - if (optionId === ApprovalOptionId.AllowAlways) { + if (optionId === ApprovalOptionId.AllowPersist) { + if (isMcpToolCallApproval(params._meta)) { + // Codex persists tool approval under an existing config.toml server: + // https://github.com/openai/codex/blob/main/codex-rs/core/src/mcp_tool_call.rs + // ACP servers are session-only, so write this server before returning persist:"always". + await this.persistMcpServerForToolApproval(params.serverName); + } + // This _meta is part of Codex's MCP tool approval response contract. + // It tells app-server to persist this MCP tool approval across sessions. return { action: "accept", content: null, _meta: { persist: "always" } }; } if (optionId === ApprovalOptionId.AllowOnce || optionId === "accept") { @@ -223,6 +277,19 @@ export class CodexElicitationHandler implements ElicitationHandler { return { action: "decline", content: null, _meta: null }; } + private async persistMcpServerForToolApproval(serverName: string): Promise { + const mcpServer = this.sessionState.sessionMcpServers.get(serverName); + if (mcpServer === undefined) { + return; + } + + try { + await this.codexAcpClient.persistMcpServer(mcpServer); + } catch (error) { + logger.error("Failed to persist ACP MCP server before MCP tool approval", error); + } + } + private handleItemStarted(event: ItemStartedNotification): void { if (event.item.type !== "mcpToolCall") { return; diff --git a/src/__tests__/CodexACPAgent/approval-events.test.ts b/src/__tests__/CodexACPAgent/approval-events.test.ts index 7ee43746..2b708fdb 100644 --- a/src/__tests__/CodexACPAgent/approval-events.test.ts +++ b/src/__tests__/CodexACPAgent/approval-events.test.ts @@ -50,7 +50,7 @@ describe('Approval Events', () => { describe('Command execution approval', () => { const commandApprovalCases = [ { optionId: 'allow_once', expectedDecision: 'accept', description: 'allow once' }, - { optionId: 'allow_always', expectedDecision: 'acceptForSession', description: 'allow for session' }, + { optionId: 'allow_for_session', expectedDecision: 'acceptForSession', description: 'allow for session' }, { optionId: 'reject_once', expectedDecision: 'decline', description: 'reject' }, ] as const; @@ -107,6 +107,172 @@ describe('Approval Events', () => { await promptPromise; }); + it('should map available execpolicy amendment approval to Codex decision', async () => { + const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); + const execpolicyAmendment = ['env', 'GRADLE_USER_HOME=/workspace/.gradle-home', './gradlew', 'build']; + fixture.setPermissionResponse({ + outcome: { outcome: 'selected', optionId: 'allow_command_prefix_rule' } + }); + + const params: CommandExecutionRequestApprovalParams = { + threadId: sessionId, + turnId: 'turn-1', + itemId: 'item-prefix-rule', + reason: 'Build the project', + proposedExecpolicyAmendment: ['ignored-fallback'], + availableDecisions: [ + 'accept', + { acceptWithExecpolicyAmendment: { execpolicy_amendment: execpolicyAmendment } }, + 'decline', + ], + }; + + const response = await fixture.sendServerRequest( + 'item/commandExecution/requestApproval', + params + ); + + expect(response).toEqual({ + decision: { + acceptWithExecpolicyAmendment: { + execpolicy_amendment: execpolicyAmendment, + }, + }, + }); + await expect(fixture.getAcpConnectionDump(['_meta'])).toMatchFileSnapshot( + 'data/approval-command-prefix-rule.json' + ); + + completeTurn(); + await promptPromise; + }); + + it('should add fallback execpolicy amendment option when availableDecisions is absent', async () => { + const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); + const execpolicyAmendment = ['npm', 'run']; + fixture.setPermissionResponse({ + outcome: { outcome: 'selected', optionId: 'allow_command_prefix_rule' } + }); + + const params: CommandExecutionRequestApprovalParams = { + threadId: sessionId, + turnId: 'turn-1', + itemId: 'item-prefix-rule-fallback', + reason: 'Run npm script', + proposedExecpolicyAmendment: execpolicyAmendment, + }; + + const response = await fixture.sendServerRequest( + 'item/commandExecution/requestApproval', + params + ); + + expect(response).toEqual({ + decision: { + acceptWithExecpolicyAmendment: { + execpolicy_amendment: execpolicyAmendment, + }, + }, + }); + + completeTurn(); + await promptPromise; + }); + + it('should respect availableDecisions when prefix-rule approval is omitted', async () => { + const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); + fixture.setPermissionResponse({ + outcome: { outcome: 'selected', optionId: 'allow_once' } + }); + + const params: CommandExecutionRequestApprovalParams = { + threadId: sessionId, + turnId: 'turn-1', + itemId: 'item-no-prefix-rule', + reason: 'Run exact command only', + proposedExecpolicyAmendment: ['npm', 'run'], + availableDecisions: ['accept', 'decline'], + }; + + await fixture.sendServerRequest( + 'item/commandExecution/requestApproval', + params + ); + + await expect(fixture.getAcpConnectionDump(['_meta'])).toMatchFileSnapshot( + 'data/approval-command-available-decisions-without-prefix.json' + ); + + completeTurn(); + await promptPromise; + }); + + it('should map network policy amendment approval to Codex decision', async () => { + const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); + const networkPolicyAmendment = { host: 'registry.npmjs.org', action: 'allow' } as const; + fixture.setPermissionResponse({ + outcome: { outcome: 'selected', optionId: 'apply_network_policy_amendment' } + }); + + const params: CommandExecutionRequestApprovalParams = { + threadId: sessionId, + turnId: 'turn-1', + itemId: 'item-network-policy', + reason: 'Allow network access', + proposedExecpolicyAmendment: null, + availableDecisions: [ + 'accept', + { applyNetworkPolicyAmendment: { network_policy_amendment: networkPolicyAmendment } }, + 'cancel', + ], + }; + + const response = await fixture.sendServerRequest( + 'item/commandExecution/requestApproval', + params + ); + + expect(response).toEqual({ + decision: { + applyNetworkPolicyAmendment: { + network_policy_amendment: networkPolicyAmendment, + }, + }, + }); + await expect(fixture.getAcpConnectionDump(['_meta'])).toMatchFileSnapshot( + 'data/approval-command-network-policy.json' + ); + + completeTurn(); + await promptPromise; + }); + + it('should map explicit cancel option to cancel when advertised', async () => { + const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); + fixture.setPermissionResponse({ + outcome: { outcome: 'selected', optionId: 'cancel' } + }); + + const params: CommandExecutionRequestApprovalParams = { + threadId: sessionId, + turnId: 'turn-1', + itemId: 'item-explicit-cancel', + reason: 'Test command', + proposedExecpolicyAmendment: null, + availableDecisions: ['accept', 'cancel'], + }; + + const response = await fixture.sendServerRequest( + 'item/commandExecution/requestApproval', + params + ); + + expect(response).toEqual({ decision: 'cancel' }); + + completeTurn(); + await promptPromise; + }); + it('should return cancel when no handler registered', async () => { const params: CommandExecutionRequestApprovalParams = { threadId: 'non-existent-session', @@ -221,7 +387,7 @@ describe('Approval Events', () => { describe('File change approval', () => { const fileChangeApprovalCases = [ { optionId: 'allow_once', expectedDecision: 'accept', description: 'allow once' }, - { optionId: 'allow_always', expectedDecision: 'acceptForSession', description: 'allow for session' }, + { optionId: 'allow_for_session', expectedDecision: 'acceptForSession', description: 'allow for session' }, { optionId: 'reject_once', expectedDecision: 'cancel', description: 'reject' }, ] as const; diff --git a/src/__tests__/CodexACPAgent/data/approval-command-allow-once.json b/src/__tests__/CodexACPAgent/data/approval-command-allow-once.json index 453ed87b..d74caf9d 100644 --- a/src/__tests__/CodexACPAgent/data/approval-command-allow-once.json +++ b/src/__tests__/CodexACPAgent/data/approval-command-allow-once.json @@ -21,17 +21,17 @@ "options": [ { "optionId": "allow_once", - "name": "Allow Once", + "name": "Yes, proceed", "kind": "allow_once" }, { - "optionId": "allow_always", - "name": "Allow for Session", + "optionId": "allow_for_session", + "name": "Yes, and don't ask again for this exact command", "kind": "allow_always" }, { "optionId": "reject_once", - "name": "Reject", + "name": "No, and tell Codex what to do differently", "kind": "reject_once" } ] diff --git a/src/__tests__/CodexACPAgent/data/approval-command-available-decisions-without-prefix.json b/src/__tests__/CodexACPAgent/data/approval-command-available-decisions-without-prefix.json new file mode 100644 index 00000000..cab8eddf --- /dev/null +++ b/src/__tests__/CodexACPAgent/data/approval-command-available-decisions-without-prefix.json @@ -0,0 +1,35 @@ +{ + "method": "requestPermission", + "args": [ + { + "sessionId": "test-session-id", + "toolCall": { + "toolCallId": "item-no-prefix-rule", + "kind": "execute", + "status": "pending", + "content": [ + { + "type": "content", + "content": { + "type": "text", + "text": "Run exact command only" + } + } + ], + "rawInput": null + }, + "options": [ + { + "optionId": "allow_once", + "name": "Yes, proceed", + "kind": "allow_once" + }, + { + "optionId": "reject_once", + "name": "No, and tell Codex what to do differently", + "kind": "reject_once" + } + ] + } + ] +} \ No newline at end of file diff --git a/src/__tests__/CodexACPAgent/data/approval-command-network-policy.json b/src/__tests__/CodexACPAgent/data/approval-command-network-policy.json new file mode 100644 index 00000000..f8c1d3bb --- /dev/null +++ b/src/__tests__/CodexACPAgent/data/approval-command-network-policy.json @@ -0,0 +1,40 @@ +{ + "method": "requestPermission", + "args": [ + { + "sessionId": "test-session-id", + "toolCall": { + "toolCallId": "item-network-policy", + "kind": "execute", + "status": "pending", + "content": [ + { + "type": "content", + "content": { + "type": "text", + "text": "Allow network access" + } + } + ], + "rawInput": null + }, + "options": [ + { + "optionId": "allow_once", + "name": "Yes, proceed", + "kind": "allow_once" + }, + { + "optionId": "apply_network_policy_amendment", + "name": "Yes, and don't ask again for network access to `registry.npmjs.org`", + "kind": "allow_always" + }, + { + "optionId": "cancel", + "name": "No, and tell Codex what to do differently", + "kind": "reject_once" + } + ] + } + ] +} \ No newline at end of file diff --git a/src/__tests__/CodexACPAgent/data/approval-command-prefix-rule.json b/src/__tests__/CodexACPAgent/data/approval-command-prefix-rule.json new file mode 100644 index 00000000..73b53697 --- /dev/null +++ b/src/__tests__/CodexACPAgent/data/approval-command-prefix-rule.json @@ -0,0 +1,40 @@ +{ + "method": "requestPermission", + "args": [ + { + "sessionId": "test-session-id", + "toolCall": { + "toolCallId": "item-prefix-rule", + "kind": "execute", + "status": "pending", + "content": [ + { + "type": "content", + "content": { + "type": "text", + "text": "Build the project" + } + } + ], + "rawInput": null + }, + "options": [ + { + "optionId": "allow_once", + "name": "Yes, proceed", + "kind": "allow_once" + }, + { + "optionId": "allow_command_prefix_rule", + "name": "Yes, and don't ask again for commands that start with `env GRADLE_USER_HOME=/workspace/.gradle-home ./gradlew build`", + "kind": "allow_always" + }, + { + "optionId": "reject_once", + "name": "No, and tell Codex what to do differently", + "kind": "reject_once" + } + ] + } + ] +} \ No newline at end of file diff --git a/src/__tests__/CodexACPAgent/data/approval-command-with-rawInput.json b/src/__tests__/CodexACPAgent/data/approval-command-with-rawInput.json index 5be9602b..6d2cd764 100644 --- a/src/__tests__/CodexACPAgent/data/approval-command-with-rawInput.json +++ b/src/__tests__/CodexACPAgent/data/approval-command-with-rawInput.json @@ -24,17 +24,17 @@ "options": [ { "optionId": "allow_once", - "name": "Allow Once", + "name": "Yes, proceed", "kind": "allow_once" }, { - "optionId": "allow_always", - "name": "Allow for Session", + "optionId": "allow_for_session", + "name": "Yes, and don't ask again for this exact command", "kind": "allow_always" }, { "optionId": "reject_once", - "name": "Reject", + "name": "No, and tell Codex what to do differently", "kind": "reject_once" } ] diff --git a/src/__tests__/CodexACPAgent/data/approval-file-change.json b/src/__tests__/CodexACPAgent/data/approval-file-change.json index b4c351f1..f9c7e226 100644 --- a/src/__tests__/CodexACPAgent/data/approval-file-change.json +++ b/src/__tests__/CodexACPAgent/data/approval-file-change.json @@ -24,7 +24,7 @@ "kind": "allow_once" }, { - "optionId": "allow_always", + "optionId": "allow_for_session", "name": "Allow for Session", "kind": "allow_always" }, diff --git a/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-all-persist.json b/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-all-persist.json index d2f40807..8ceee1a3 100644 --- a/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-all-persist.json +++ b/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-all-persist.json @@ -29,22 +29,26 @@ { "optionId": "allow_once", "name": "Allow", - "kind": "allow_once" + "kind": "allow_once", + "_meta": "_meta" }, { "optionId": "allow_session", - "name": "Allow for This Session", - "kind": "allow_always" + "name": "Allow for this session", + "kind": "allow_always", + "_meta": "_meta" }, { - "optionId": "allow_always", - "name": "Allow and Don't Ask Again", - "kind": "allow_always" + "optionId": "allow_persist", + "name": "Always allow", + "kind": "allow_always", + "_meta": "_meta" }, { "optionId": "decline", - "name": "Decline", - "kind": "reject_once" + "name": "Cancel", + "kind": "reject_once", + "_meta": "_meta" } ] } diff --git a/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-no-persist.json b/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-no-persist.json index b212b996..f94f57ca 100644 --- a/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-no-persist.json +++ b/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-no-persist.json @@ -29,12 +29,14 @@ { "optionId": "allow_once", "name": "Allow", - "kind": "allow_once" + "kind": "allow_once", + "_meta": "_meta" }, { "optionId": "decline", - "name": "Decline", - "kind": "reject_once" + "name": "Cancel", + "kind": "reject_once", + "_meta": "_meta" } ] } diff --git a/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-session-only.json b/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-session-only.json index 50358bee..3827c82a 100644 --- a/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-session-only.json +++ b/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-session-only.json @@ -29,17 +29,20 @@ { "optionId": "allow_once", "name": "Allow", - "kind": "allow_once" + "kind": "allow_once", + "_meta": "_meta" }, { "optionId": "allow_session", - "name": "Allow for This Session", - "kind": "allow_always" + "name": "Allow for this session", + "kind": "allow_always", + "_meta": "_meta" }, { "optionId": "decline", - "name": "Decline", - "kind": "reject_once" + "name": "Cancel", + "kind": "reject_once", + "_meta": "_meta" } ] } diff --git a/src/__tests__/CodexACPAgent/e2e/acp-e2e-shell-approval.test.ts b/src/__tests__/CodexACPAgent/e2e/acp-e2e-shell-approval.test.ts index da026f52..f116be83 100644 --- a/src/__tests__/CodexACPAgent/e2e/acp-e2e-shell-approval.test.ts +++ b/src/__tests__/CodexACPAgent/e2e/acp-e2e-shell-approval.test.ts @@ -53,8 +53,8 @@ describeE2E("E2E shell approval tests", () => { expect(fixture.readPermissionRequests(sessionId, "execute").length).toBe(2); }); - it("skips subsequent approvals when allow_always is selected", async () => { - fixture.setPermissionResponder(createPermissionResponder("execute", ApprovalOptionId.AllowAlways)); + it("skips subsequent approvals when allow_for_session is selected", async () => { + fixture.setPermissionResponder(createPermissionResponder("execute", ApprovalOptionId.AllowForSession)); await promptShellCommandTwice(); expect(fs.existsSync(path.join(fixture.workspaceDir, FIRST_FILE_NAME))).toBe(true); expect(fs.existsSync(path.join(fixture.workspaceDir, SECOND_FILE_NAME))).toBe(true); diff --git a/src/__tests__/CodexACPAgent/elicitation-events.test.ts b/src/__tests__/CodexACPAgent/elicitation-events.test.ts index 32548bee..6c010fb7 100644 --- a/src/__tests__/CodexACPAgent/elicitation-events.test.ts +++ b/src/__tests__/CodexACPAgent/elicitation-events.test.ts @@ -1,4 +1,5 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; +import type { McpServerStdio } from '@agentclientprotocol/sdk'; import type { McpServerElicitationRequestParams } from '../../app-server/v2'; import { createCodexMockTestFixture, createTestSessionState, type CodexMockTestFixture } from '../acp-test-utils'; import type { SessionState } from '../../CodexAcpServer'; @@ -14,7 +15,7 @@ describe('Elicitation Events', () => { vi.clearAllMocks(); }); - function setupSessionWithPendingPrompt() { + function setupSessionWithPendingPrompt(sessionOverrides?: Partial) { const codexAcpAgent = fixture.getCodexAcpAgent(); let resolveTurnCompleted: (value: { threadId: string; turn: { id: string; items: never[]; status: string; error: null } }) => void; @@ -30,7 +31,8 @@ describe('Elicitation Events', () => { const sessionState: SessionState = createTestSessionState({ sessionId, currentModelId: 'model-id[effort]', - agentMode: AgentMode.DEFAULT_AGENT_MODE + agentMode: AgentMode.DEFAULT_AGENT_MODE, + ...sessionOverrides, }); vi.spyOn(codexAcpAgent, 'getSessionState').mockReturnValue(sessionState); @@ -130,7 +132,7 @@ describe('Elicitation Events', () => { }); describe('MCP tool call approval elicitation', () => { - it('should show Allow/session/always/Decline options when all persist values advertised', async () => { + it('should show Allow/session/always/Cancel options when all persist values advertised', async () => { const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_once' } }); @@ -149,6 +151,51 @@ describe('Elicitation Events', () => { await promptPromise; }); + it('should include CLI-style descriptions for MCP tool approval options', async () => { + const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_once' } }); + + const params: McpServerElicitationRequestParams = { + threadId: sessionId, turnId: 'turn-1', serverName: 'tool-server', + mode: 'form', + _meta: { codex_approval_kind: 'mcp_tool_call', persist: ['session', 'always'] }, + message: 'Allow tool call?', + requestedSchema: { type: 'object', properties: {} }, + }; + + await fixture.sendServerRequest('mcpServer/elicitation/request', params); + const [requestPermissionEvent] = fixture.getAcpConnectionEvents([]); + expect(requestPermissionEvent?.args[0].options).toEqual([ + { + optionId: 'allow_once', + name: 'Allow', + kind: 'allow_once', + _meta: { description: 'Run the tool and continue.' }, + }, + { + optionId: 'allow_session', + name: 'Allow for this session', + kind: 'allow_always', + _meta: { description: 'Run the tool and remember this choice for this session.' }, + }, + { + optionId: 'allow_persist', + name: 'Always allow', + kind: 'allow_always', + _meta: { description: 'Run the tool and remember this choice for future tool calls.' }, + }, + { + optionId: 'decline', + name: 'Cancel', + kind: 'reject_once', + _meta: { description: 'Cancel this tool call.' }, + }, + ]); + + completeTurn(); + await promptPromise; + }); + it('should map allow_once to accept with null meta', async () => { const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_once' } }); @@ -187,9 +234,51 @@ describe('Elicitation Events', () => { await promptPromise; }); - it('should map allow_always to accept with persist:always meta', async () => { + it('should map persistent approval to accept with persist:always meta', async () => { const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); - fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_always' } }); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_persist' } }); + + const params: McpServerElicitationRequestParams = { + threadId: sessionId, turnId: 'turn-1', serverName: 'tool-server', + mode: 'form', + _meta: { codex_approval_kind: 'mcp_tool_call', persist: ['session', 'always'] }, + message: 'Allow tool call?', + requestedSchema: { type: 'object', properties: {} }, + }; + + const response = await fixture.sendServerRequest('mcpServer/elicitation/request', params); + expect(response).toEqual({ action: 'accept', content: null, _meta: { persist: 'always' } }); + + completeTurn(); + await promptPromise; + }); + + it('should persist only the approved MCP server before returning persistent approval meta', async () => { + const mcpServer: McpServerStdio = { + name: 'tool-server', + command: 'npx', + args: ['tool-server'], + env: [{ name: 'FOO', value: 'bar' }], + }; + const otherServer: McpServerStdio = { + name: 'other-server', + command: 'npx', + args: ['other-server'], + env: [], + }; + const { promptPromise, completeTurn } = setupSessionWithPendingPrompt({ + sessionMcpServers: new Map([ + [mcpServer.name, mcpServer], + [otherServer.name, otherServer], + ]), + }); + const configBatchWriteSpy = vi.spyOn(fixture.getCodexAppServerClient(), 'configBatchWrite').mockResolvedValue({ + status: 'ok', + version: '1', + filePath: '/tmp/config.toml', + overriddenMetadata: null, + }); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_persist' } }); const params: McpServerElicitationRequestParams = { threadId: sessionId, turnId: 'turn-1', serverName: 'tool-server', @@ -201,6 +290,21 @@ describe('Elicitation Events', () => { const response = await fixture.sendServerRequest('mcpServer/elicitation/request', params); expect(response).toEqual({ action: 'accept', content: null, _meta: { persist: 'always' } }); + expect(configBatchWriteSpy).toHaveBeenCalledTimes(1); + expect(configBatchWriteSpy).toHaveBeenCalledWith({ + edits: [{ + keyPath: 'mcp_servers.tool-server', + value: { + command: 'npx', + args: ['tool-server'], + env: { FOO: 'bar' }, + }, + mergeStrategy: 'upsert', + }], + filePath: null, + expectedVersion: null, + reloadUserConfig: true, + }); completeTurn(); await promptPromise; @@ -225,7 +329,7 @@ describe('Elicitation Events', () => { await promptPromise; }); - it('should show only Allow and Decline when no persist options', async () => { + it('should show only Allow and Cancel when no persist options', async () => { const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_once' } }); diff --git a/src/__tests__/CodexACPAgent/load-session.test.ts b/src/__tests__/CodexACPAgent/load-session.test.ts index 43aaa09a..9ac7643b 100644 --- a/src/__tests__/CodexACPAgent/load-session.test.ts +++ b/src/__tests__/CodexACPAgent/load-session.test.ts @@ -238,7 +238,7 @@ describe("CodexACPAgent - loadSession", () => { mcpServers: [], }); - expect(codexAcpAgent.getSessionState("session-1").sessionMcpServers).toEqual([]); + expect(Array.from(codexAcpAgent.getSessionState("session-1").sessionMcpServers.keys())).toEqual([]); }); it("publishes MCP startup failure for explicitly requested servers during loadSession", async () => { @@ -317,7 +317,7 @@ describe("CodexACPAgent - loadSession", () => { }); await vi.waitFor(() => { - expect(codexAcpAgent.getSessionState("session-1").sessionMcpServers).toEqual(["broken-mcp"]); + expect(Array.from(codexAcpAgent.getSessionState("session-1").sessionMcpServers.keys())).toEqual(["broken-mcp"]); }); fixture.sendServerNotification({ diff --git a/src/__tests__/acp-test-utils.ts b/src/__tests__/acp-test-utils.ts index a1629214..c084ca95 100644 --- a/src/__tests__/acp-test-utils.ts +++ b/src/__tests__/acp-test-utils.ts @@ -322,6 +322,7 @@ export function createTestSessionState(overrides?: Partial): Sessi supportedReasoningEfforts: [], supportedInputModalities: ["text", "image"], agentMode: AgentMode.DEFAULT_AGENT_MODE, + sessionMcpServers: new Map(), ...overrides, }; }