diff --git a/package-lock.json b/package-lock.json index 9c64924..2fe670d 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 ccec90e..969aa42 100644 --- a/src/ApprovalOptionId.ts +++ b/src/ApprovalOptionId.ts @@ -1,6 +1,9 @@ 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", } as const; diff --git a/src/CodexAcpClient.ts b/src/CodexAcpClient.ts index 22cde0d..4822020 100644 --- a/src/CodexAcpClient.ts +++ b/src/CodexAcpClient.ts @@ -201,9 +201,10 @@ export class CodexAcpClient { async resumeSession(request: acp.ResumeSessionRequest): Promise { await this.refreshSkills(request.cwd, request._meta); + const sessionConfig = await this.createSessionConfig(request.cwd, request.mcpServers ?? []); const response = await this.codexClient.threadResume({ - config: await this.createSessionConfig(request.cwd, request.mcpServers ?? []), + config: sessionConfig.config, cwd: request.cwd, modelProvider: this.getResumeModelProvider(), threadId: request.sessionId, @@ -215,12 +216,14 @@ export class CodexAcpClient { currentModelId: currentModelId, models: codexModels, currentServiceTier: response.serviceTier ?? null, + configBackedMcpServerNames: sessionConfig.configBackedMcpServerNames, } } async loadSession(request: acp.LoadSessionRequest): Promise { + const sessionConfig = await this.createSessionConfig(request.cwd, request.mcpServers ?? []); const response = await this.codexClient.threadResume({ - config: await this.createSessionConfig(request.cwd, request.mcpServers ?? []), + config: sessionConfig.config, cwd: request.cwd, modelProvider: this.getResumeModelProvider(), threadId: request.sessionId, @@ -232,15 +235,17 @@ export class CodexAcpClient { currentModelId: currentModelId, models: codexModels, currentServiceTier: response.serviceTier ?? null, + configBackedMcpServerNames: sessionConfig.configBackedMcpServerNames, thread: response.thread, }; } async newSession(request: acp.NewSessionRequest): Promise { await this.refreshSkills(request.cwd, request._meta); + const sessionConfig = await this.createSessionConfig(request.cwd, request.mcpServers); const response = await this.codexClient.threadStart({ - config: await this.createSessionConfig(request.cwd, request.mcpServers), + config: sessionConfig.config, modelProvider: this.getModelProvider(), cwd: request.cwd, }); @@ -255,6 +260,7 @@ export class CodexAcpClient { currentModelId: currentModelId, models: codexModels, currentServiceTier: response.serviceTier ?? null, + configBackedMcpServerNames: sessionConfig.configBackedMcpServerNames, }; } @@ -266,7 +272,7 @@ export class CodexAcpClient { return this.codexClient.getMcpServerStartupVersion(); } - private async createSessionConfig(projectPath: string, mcpServers: Array): Promise { + private async createSessionConfig(projectPath: string, mcpServers: Array): Promise { const mergedConfig = { ...mergeGatewayConfig(this.config, this.gatewayConfig), projects: { @@ -275,21 +281,30 @@ export class CodexAcpClient { } }, }; + const configuredMcpServerNames = await this.getConfigMcpServerNames(projectPath); if (mcpServers.length === 0) { - return mergedConfig; + return { + config: mergedConfig, + configBackedMcpServerNames: configuredMcpServerNames, + }; } // Deduplicates new servers against existing config to prevent Codex from deep-merging // incompatible field types (e.g., mixing url and stdio schemas). - const existingNames = await this.getConfigMcpServerNames(projectPath); - const uniqueServers = mcpServers.filter(mcp => !existingNames.has(mcp.name)); + const uniqueServers = mcpServers.filter(mcp => !configuredMcpServerNames.has(mcp.name)); if (uniqueServers.length === 0) { - return mergedConfig; + return { + config: mergedConfig, + configBackedMcpServerNames: configuredMcpServerNames, + }; } return { - ...mergedConfig, - "mcp_servers": Object.fromEntries(uniqueServers.map(mcp => [mcp.name, this.createMcpSeverConfig(mcp)])), + config: { + ...mergedConfig, + "mcp_servers": Object.fromEntries(uniqueServers.map(mcp => [mcp.name, this.createMcpSeverConfig(mcp)])), + }, + configBackedMcpServerNames: configuredMcpServerNames, }; } @@ -569,12 +584,18 @@ export type SessionMetadata = { currentModelId: string, models: Model[], currentServiceTier?: ServiceTier | null, + configBackedMcpServerNames?: Set, } export type SessionMetadataWithThread = SessionMetadata & { thread: Thread, } +type SessionConfig = { + config: JsonObject, + configBackedMcpServerNames: Set, +} + function buildPromptItems(prompt: acp.ContentBlock[]): UserInput[] { return prompt.map((block): UserInput | null => { switch (block.type) { diff --git a/src/CodexAcpServer.ts b/src/CodexAcpServer.ts index 81e556c..a5977ca 100644 --- a/src/CodexAcpServer.ts +++ b/src/CodexAcpServer.ts @@ -57,7 +57,8 @@ export interface SessionState { cwd: string; fastModeEnabled: boolean; currentModelSupportsFast: boolean; - sessionMcpServers?: Array; + sessionMcpServers: Array; + configBackedMcpServerNames?: Set; } interface PendingMcpStartupSession { @@ -120,7 +121,6 @@ export class CodexAcpServer implements acp.Agent { list: { } }, mcpCapabilities: { - acp: false, http: true, sse: false } @@ -179,7 +179,6 @@ export class CodexAcpServer implements acp.Agent { const account = await this.getActiveAccount(); const {sessionId, currentModelId, models} = sessionMetadata; - const sessionMcpServers = this.resolveSessionMcpServers(requestedMcpServers, "sessionId" in request); const currentModel = this.findCurrentModel(models, currentModelId); const currentModelSupportsFast = modelSupportsFast(currentModel); const sessionState: SessionState = { @@ -197,7 +196,8 @@ export class CodexAcpServer implements acp.Agent { cwd: request.cwd, fastModeEnabled: sessionMetadata.currentServiceTier === "fast", currentModelSupportsFast: currentModelSupportsFast, - sessionMcpServers: sessionMcpServers, + sessionMcpServers: this.createSessionMcpServers(requestedMcpServers, "sessionId" in request), + configBackedMcpServerNames: sessionMetadata.configBackedMcpServerNames ?? new Set(), } this.sessions.set(sessionId, sessionState); @@ -439,7 +439,6 @@ export class CodexAcpServer implements acp.Agent { const account = await this.getActiveAccount(); const {sessionId, currentModelId, models, thread} = sessionMetadata; - const sessionMcpServers = this.resolveSessionMcpServers(requestedMcpServers, true); const currentModel = this.findCurrentModel(models, currentModelId); const currentModelSupportsFast = modelSupportsFast(currentModel); const sessionState: SessionState = { @@ -457,7 +456,8 @@ export class CodexAcpServer implements acp.Agent { cwd: request.cwd, fastModeEnabled: sessionMetadata.currentServiceTier === "fast", currentModelSupportsFast: currentModelSupportsFast, - sessionMcpServers: sessionMcpServers, + sessionMcpServers: this.createSessionMcpServers(requestedMcpServers, true), + configBackedMcpServerNames: sessionMetadata.configBackedMcpServerNames ?? new Set(), }; this.sessions.set(sessionId, sessionState); @@ -711,14 +711,13 @@ export class CodexAcpServer implements acp.Agent { return sessionState; } - private resolveSessionMcpServers( + private createSessionMcpServers( mcpServers: Array, recoverFromStartup: boolean, ): Array { // 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 mcpServers.map(server => server.name); } // Fresh sessions without MCP config should not inherit any session MCP state. if (!recoverFromStartup) { @@ -942,7 +941,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 0d2d083..cbafd3e 100644 --- a/src/CodexAppServerClient.ts +++ b/src/CodexAppServerClient.ts @@ -8,6 +8,8 @@ import type { import type { ConfigReadParams, ConfigReadResponse, + ConfigBatchWriteParams, + ConfigWriteResponse, GetAccountParams, GetAccountResponse, ListMcpServerStatusParams, diff --git a/src/CodexApprovalHandler.ts b/src/CodexApprovalHandler.ts index 0eddf3a..618d39b 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, @@ -12,11 +13,13 @@ import {logger} from "./Logger"; import {stripShellPrefix} from "./CodexEventHandler"; 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.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; @@ -37,7 +40,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 +75,156 @@ 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[] { + const decisions = this.buildCommandDecisions(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(params, decision, amendmentIndex); + }); + } + + private buildCommandDecisions( + 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( + params: CommandExecutionRequestApprovalParams, + decision: CommandExecutionApprovalDecision, + amendmentIndex: number + ): CommandDecisionOption { + if (decision === "accept") { + return { + option: { + optionId: ApprovalOptionId.AllowOnce, + name: params.networkApprovalContext ? "Yes, just this once" : "Yes, proceed", + kind: "allow_once" + }, + decision + }; + } + + if (decision === "acceptForSession") { + return { + option: { + optionId: ApprovalOptionId.AllowForSession, + name: params.networkApprovalContext + ? "Yes, and allow this host for this conversation" + : "Yes, and don't ask again for this command in this session", + 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.RejectOnce, + 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 { + return amendment.action === "deny" + ? "No, and block this host in the future" + : "Yes, and allow this host in the future"; + } + + // ACP responses only return the selected optionId. The network field is + // plural, 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; @@ -102,25 +251,38 @@ export class CodexApprovalHandler implements ApprovalHandler { status: "pending", content: reasonContent ? [reasonContent] : null, }, - options: APPROVAL_OPTIONS, + options: [ + { optionId: ApprovalOptionId.AllowOnce, name: "Yes, proceed", kind: "allow_once" }, + { + optionId: ApprovalOptionId.AllowForSession, + name: "Yes, and don't ask again for these files", + kind: "allow_always" + }, + { + optionId: ApprovalOptionId.RejectOnce, + name: "No, and tell Codex what to do differently", + kind: "reject_once" + }, + ], }; } 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 +295,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 a125d71..bc20a65 100644 --- a/src/CodexCommands.ts +++ b/src/CodexCommands.ts @@ -153,9 +153,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 = sessionState.sessionMcpServers.map(server => `- ${server}`); 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 732822b..59bfc38 100644 --- a/src/CodexElicitationHandler.ts +++ b/src/CodexElicitationHandler.ts @@ -13,15 +13,24 @@ import { ApprovalOptionId } from "./ApprovalOptionId"; // Standard elicitation options (non-tool-call approval). const ELICITATION_OPTIONS: acp.PermissionOption[] = [ - { optionId: "accept", name: "Accept", kind: "allow_once" }, - { optionId: "decline", name: "Decline", kind: "reject_once" }, + { optionId: ApprovalOptionId.AllowOnce, name: "Accept", kind: "allow_once" }, + { optionId: ApprovalOptionId.RejectOnce, name: "Decline", kind: "reject_once" }, ]; -// Option ID unique to elicitation persist choices — not part of the shared ApprovalOptionId set. -const OPTION_ALLOW_SESSION = "allow_session"; - type PersistValue = "session" | "always"; +function buildToolApprovalOption( + optionId: string, + name: string, + kind: acp.PermissionOption["kind"], +): acp.PermissionOption { + return { + optionId, + name, + kind + }; +} + /** * Parses the `persist` field from the elicitation request `_meta`. * Codex advertises which persistence options the client should show. @@ -52,19 +61,40 @@ 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[] { +function buildToolApprovalOptions( + persistOptions: Set, + allowPersistentApproval: boolean +): acp.PermissionOption[] { const options: acp.PermissionOption[] = [ - { optionId: ApprovalOptionId.AllowOnce, name: "Allow", kind: "allow_once" }, + buildToolApprovalOption( + ApprovalOptionId.AllowOnce, + "Allow", + "allow_once" + ), ]; + // 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( + ApprovalOptionId.AllowForSession, + "Allow for this session", + "allow_always" + )); } - if (persistOptions.has("always")) { - options.push({ optionId: ApprovalOptionId.AllowAlways, name: "Allow and Don't Ask Again", kind: "allow_always" }); + if (persistOptions.has("always") && allowPersistentApproval) { + options.push(buildToolApprovalOption( + ApprovalOptionId.AllowPersist, + "Always allow", + "allow_always" + )); } - options.push({ optionId: "decline", name: "Decline", kind: "reject_once" }); + options.push(buildToolApprovalOption( + ApprovalOptionId.RejectOnce, + "Cancel", + "reject_once" + )); return options; } @@ -88,7 +118,10 @@ export class CodexElicitationHandler implements ElicitationHandler { // (threadId, serverName). private readonly pendingMcpApprovals = new Map(); - constructor(connection: acp.AgentSideConnection, sessionState: SessionState) { + constructor( + connection: acp.AgentSideConnection, + sessionState: SessionState + ) { this.connection = connection; this.sessionState = sessionState; } @@ -117,14 +150,14 @@ export class CodexElicitationHandler implements ElicitationHandler { const response = await this.connection.requestPermission(request); if (correlatedCallId !== undefined && response.outcome.outcome !== "cancelled") { const optionId = response.outcome.optionId; - if (optionId !== "decline") { + if (optionId !== ApprovalOptionId.RejectOnce) { await this.connection.sessionUpdate({ sessionId: this.sessionState.sessionId, update: { sessionUpdate: "tool_call_update", toolCallId: correlatedCallId, status: "in_progress" }, }); } } - return this.convertResponse(response); + return await this.convertResponse(response); } catch (error) { logger.error("Error handling MCP elicitation request", error); return { action: "cancel", content: null, _meta: null }; @@ -143,7 +176,10 @@ export class CodexElicitationHandler implements ElicitationHandler { const meta = params._meta; const isToolApproval = isMcpToolCallApproval(meta); const options = isToolApproval - ? buildToolApprovalOptions(parsePersistOptions(meta)) + ? buildToolApprovalOptions( + parsePersistOptions(meta), + this.serverSupportsPersistentApproval(params.serverName) + ) : ELICITATION_OPTIONS; if (params.mode === "form") { @@ -203,26 +239,34 @@ export class CodexElicitationHandler implements ElicitationHandler { } } - private convertResponse( + private async convertResponse( response: acp.RequestPermissionResponse - ): McpServerElicitationRequestResponse { + ): Promise { if (response.outcome.outcome === "cancelled") { return { action: "cancel", content: null, _meta: null }; } const optionId = response.outcome.optionId; - if (optionId === OPTION_ALLOW_SESSION) { + if (optionId === ApprovalOptionId.AllowForSession) { + // 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) { + // 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") { + if (optionId === ApprovalOptionId.AllowOnce) { return { action: "accept", content: null, _meta: null }; } return { action: "decline", content: null, _meta: null }; } + private serverSupportsPersistentApproval(serverName: string): boolean { + return this.sessionState.configBackedMcpServerNames?.has(serverName) === true; + } + private handleItemStarted(event: ItemStartedNotification): void { if (event.item.type !== "mcpToolCall") { return; diff --git a/src/__tests__/CodexACPAgent/CodexAcpClient.test.ts b/src/__tests__/CodexACPAgent/CodexAcpClient.test.ts index a66a827..5638009 100644 --- a/src/__tests__/CodexACPAgent/CodexAcpClient.test.ts +++ b/src/__tests__/CodexACPAgent/CodexAcpClient.test.ts @@ -81,6 +81,7 @@ describe('ACP server test', { timeout: 40_000 }, () => { "account/login/start", "account/read", "account/updated", + "config/read", "thread/start", "model/list", "thread/started", diff --git a/src/__tests__/CodexACPAgent/approval-events.test.ts b/src/__tests__/CodexACPAgent/approval-events.test.ts index 7ee4374..bbe6dfa 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,74 @@ describe('Approval Events', () => { await promptPromise; }); + it('should map execpolicy amendment approval to Codex decision', 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 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, + proposedNetworkPolicyAmendments: [networkPolicyAmendment], + }; + + 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 return cancel when no handler registered', async () => { const params: CommandExecutionRequestApprovalParams = { threadId: 'non-existent-session', @@ -221,7 +289,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 453ed87..d2a049b 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 command in this session", "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 0000000..cab8edd --- /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 0000000..2d58ec6 --- /dev/null +++ b/src/__tests__/CodexACPAgent/data/approval-command-network-policy.json @@ -0,0 +1,45 @@ +{ + "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": "allow_for_session", + "name": "Yes, and don't ask again for this command in this session", + "kind": "allow_always" + }, + { + "optionId": "apply_network_policy_amendment", + "name": "Yes, and allow this host in the future", + "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-prefix-rule.json b/src/__tests__/CodexACPAgent/data/approval-command-prefix-rule.json new file mode 100644 index 0000000..73b5369 --- /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 5be9602..5050084 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 command in this session", "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 b4c351f..e4271fb 100644 --- a/src/__tests__/CodexACPAgent/data/approval-file-change.json +++ b/src/__tests__/CodexACPAgent/data/approval-file-change.json @@ -20,17 +20,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 these files", "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/elicitation-form-accept.json b/src/__tests__/CodexACPAgent/data/elicitation-form-accept.json index a2113d5..4483207 100644 --- a/src/__tests__/CodexACPAgent/data/elicitation-form-accept.json +++ b/src/__tests__/CodexACPAgent/data/elicitation-form-accept.json @@ -33,12 +33,12 @@ }, "options": [ { - "optionId": "accept", + "optionId": "allow_once", "name": "Accept", "kind": "allow_once" }, { - "optionId": "decline", + "optionId": "reject_once", "name": "Decline", "kind": "reject_once" } 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 d2f4080..1d0cabd 100644 --- a/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-all-persist.json +++ b/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-all-persist.json @@ -24,7 +24,9 @@ } } }, - "_meta": "_meta", + "_meta": { + "is_mcp_tool_approval": true + }, "options": [ { "optionId": "allow_once", @@ -32,18 +34,18 @@ "kind": "allow_once" }, { - "optionId": "allow_session", - "name": "Allow for This Session", + "optionId": "allow_for_session", + "name": "Allow for this session", "kind": "allow_always" }, { - "optionId": "allow_always", - "name": "Allow and Don't Ask Again", + "optionId": "allow_persist", + "name": "Always allow", "kind": "allow_always" }, { - "optionId": "decline", - "name": "Decline", + "optionId": "reject_once", + "name": "Cancel", "kind": "reject_once" } ] 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 b212b99..3aae8b2 100644 --- a/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-no-persist.json +++ b/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-no-persist.json @@ -24,7 +24,9 @@ } } }, - "_meta": "_meta", + "_meta": { + "is_mcp_tool_approval": true + }, "options": [ { "optionId": "allow_once", @@ -32,8 +34,8 @@ "kind": "allow_once" }, { - "optionId": "decline", - "name": "Decline", + "optionId": "reject_once", + "name": "Cancel", "kind": "reject_once" } ] 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 50358be..c6a0d5f 100644 --- a/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-session-only.json +++ b/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-session-only.json @@ -24,7 +24,9 @@ } } }, - "_meta": "_meta", + "_meta": { + "is_mcp_tool_approval": true + }, "options": [ { "optionId": "allow_once", @@ -32,13 +34,13 @@ "kind": "allow_once" }, { - "optionId": "allow_session", - "name": "Allow for This Session", + "optionId": "allow_for_session", + "name": "Allow for this session", "kind": "allow_always" }, { - "optionId": "decline", - "name": "Decline", + "optionId": "reject_once", + "name": "Cancel", "kind": "reject_once" } ] diff --git a/src/__tests__/CodexACPAgent/data/elicitation-url-accept.json b/src/__tests__/CodexACPAgent/data/elicitation-url-accept.json index da7f9c0..ea36124 100644 --- a/src/__tests__/CodexACPAgent/data/elicitation-url-accept.json +++ b/src/__tests__/CodexACPAgent/data/elicitation-url-accept.json @@ -23,12 +23,12 @@ }, "options": [ { - "optionId": "accept", + "optionId": "allow_once", "name": "Accept", "kind": "allow_once" }, { - "optionId": "decline", + "optionId": "reject_once", "name": "Decline", "kind": "reject_once" } diff --git a/src/__tests__/CodexACPAgent/e2e/acp-e2e-file-approval.test.ts b/src/__tests__/CodexACPAgent/e2e/acp-e2e-file-approval.test.ts index c2a6fee..20700cb 100644 --- a/src/__tests__/CodexACPAgent/e2e/acp-e2e-file-approval.test.ts +++ b/src/__tests__/CodexACPAgent/e2e/acp-e2e-file-approval.test.ts @@ -19,7 +19,7 @@ describeE2E("E2E file approval tests", () => { let fixture: SpawnedAgentFixture; beforeEach(async () => { - fixture = await createAuthenticatedFixture(AgentMode.ReadOnly); + fixture = await createAuthenticatedFixture({initialMode: AgentMode.ReadOnly}); }); afterEach(async () => { @@ -43,7 +43,7 @@ describeE2E("E2E Agent mode file permission tests", () => { let fixture: SpawnedAgentFixture; beforeEach(async () => { - fixture = await createAuthenticatedFixture(AgentMode.Agent); + fixture = await createAuthenticatedFixture({initialMode: AgentMode.Agent}); }); afterEach(async () => { @@ -65,7 +65,7 @@ describeE2E("E2E Agent with full access file permission tests", () => { let fixture: SpawnedAgentFixture; beforeEach(async () => { - fixture = await createAuthenticatedFixture(AgentMode.AgentFullAccess); + fixture = await createAuthenticatedFixture({initialMode: AgentMode.AgentFullAccess}); }); afterEach(async () => { diff --git a/src/__tests__/CodexACPAgent/e2e/acp-e2e-mcp-approval.test.ts b/src/__tests__/CodexACPAgent/e2e/acp-e2e-mcp-approval.test.ts index 42123a7..18a86d9 100644 --- a/src/__tests__/CodexACPAgent/e2e/acp-e2e-mcp-approval.test.ts +++ b/src/__tests__/CodexACPAgent/e2e/acp-e2e-mcp-approval.test.ts @@ -1,6 +1,6 @@ import type * as acp from "@agentclientprotocol/sdk"; import path from "node:path"; -import {afterEach, beforeEach, expect, it} from "vitest"; +import {afterEach, beforeEach, describe, expect, it} from "vitest"; import {ApprovalOptionId} from "../../../ApprovalOptionId"; import { createAuthenticatedFixture, @@ -13,6 +13,7 @@ import { const MCP_SERVER_NAME = "integration-mcp"; const MCP_ECHO_MESSAGE = "mcp approval e2e"; +const MCP_ECHO_PROMPT = `Use the ${MCP_SERVER_NAME} MCP echo tool with message "${MCP_ECHO_MESSAGE}". Reply with exactly the tool result and no extra text.`; function createMcpServer(): acp.McpServerStdio { return { @@ -31,6 +32,12 @@ function createMcpPermissionResponder(optionId: ApprovalOptionId): PermissionRes return (request) => createPermissionResponse(isMcpPermissionRequest(request) ? optionId : null); } +function failingPermissionResponder(label: string): PermissionResponder { + return (request) => { + throw new Error(`${label}: unexpected permission request (kind=${request.toolCall.kind})`); + }; +} + describeE2E("E2E MCP approval tests", () => { let fixture: SpawnedAgentFixture; let sessionId: string; @@ -55,7 +62,7 @@ describeE2E("E2E MCP approval tests", () => { await fixture.expectPromptText( sessionId, - `Use the ${MCP_SERVER_NAME} MCP echo tool with message "${MCP_ECHO_MESSAGE}". Reply with exactly the tool result and no extra text.`, + MCP_ECHO_PROMPT, (text) => expect(text).toContain(`You said: ${MCP_ECHO_MESSAGE}`), ); expectMcpToolPermissionRequest(); @@ -73,4 +80,97 @@ describeE2E("E2E MCP approval tests", () => { })); expectMcpToolPermissionRequest(); }); + + describe("persisted approvals", () => { + let beforeRestartFixture: SpawnedAgentFixture | null = null; + let afterRestartFixture: SpawnedAgentFixture | null = null; + + beforeEach(async () => { + // The outer beforeEach already created `fixture` without a config-backed MCP server. + // Persistence tests need the server in config.toml so Codex offers "Always allow", + // so dispose that fixture and replace it with a config-backed one. + await fixture.dispose(); + beforeRestartFixture = await createAuthenticatedFixture({ + configBackedMcpServers: [createMcpServer()], + }); + fixture = beforeRestartFixture; + sessionId = (await fixture.createSession()).sessionId; + }); + + afterEach(async () => { + await afterRestartFixture?.dispose(); + afterRestartFixture = null; + beforeRestartFixture = null; + }); + + it("does not re-prompt across agent restart when user picks Always allow", async () => { + fixture.setPermissionResponder(createMcpPermissionResponder(ApprovalOptionId.AllowPersist)); + + await fixture.expectPromptText( + sessionId, + MCP_ECHO_PROMPT, + (text) => expect(text).toContain(`You said: ${MCP_ECHO_MESSAGE}`), + ); + + const requests = fixture.readPermissionRequests(sessionId, "execute"); + expect(requests.length).toBe(1); + expect(isMcpPermissionRequest(requests[0]!)).toBe(true); + const optionIds = requests[0]!.options.map((option) => option.optionId); + expect(optionIds).toContain(ApprovalOptionId.AllowPersist); + + afterRestartFixture = await fixture.restart(); + // `fixture` is now stopped; route all subsequent calls through afterRestartFixture. + fixture = afterRestartFixture; + afterRestartFixture.setPermissionResponder(failingPermissionResponder("after restart")); + const resumedSessionId = (await afterRestartFixture.createSession()).sessionId; + + await afterRestartFixture.expectPromptText( + resumedSessionId, + MCP_ECHO_PROMPT, + (text) => expect(text).toContain(`You said: ${MCP_ECHO_MESSAGE}`), + ); + expect(afterRestartFixture.readPermissionRequests(resumedSessionId, "execute").length).toBe(0); + }); + + it("does not re-prompt within a session when user picks Allow for session, but re-prompts after restart", async () => { + let approvalsGranted = 0; + fixture.setPermissionResponder((request) => { + if (!isMcpPermissionRequest(request)) { + return createPermissionResponse(null); + } + approvalsGranted += 1; + if (approvalsGranted > 1) { + throw new Error("Allow-for-session approval should be reused within the same session"); + } + return createPermissionResponse(ApprovalOptionId.AllowForSession); + }); + + await fixture.expectPromptText( + sessionId, + MCP_ECHO_PROMPT, + (text) => expect(text).toContain(`You said: ${MCP_ECHO_MESSAGE}`), + ); + expect(fixture.readPermissionRequests(sessionId, "execute").length).toBe(1); + + await fixture.expectPromptText( + sessionId, + MCP_ECHO_PROMPT, + (text) => expect(text).toContain(`You said: ${MCP_ECHO_MESSAGE}`), + ); + // Still just the one approval recorded - the second call reused the session-scoped grant. + expect(fixture.readPermissionRequests(sessionId, "execute").length).toBe(1); + + afterRestartFixture = await fixture.restart(); + fixture = afterRestartFixture; + afterRestartFixture.setPermissionResponder(createMcpPermissionResponder(ApprovalOptionId.AllowOnce)); + const newSessionId = (await afterRestartFixture.createSession()).sessionId; + + await afterRestartFixture.expectPromptText( + newSessionId, + MCP_ECHO_PROMPT, + (text) => expect(text).toContain(`You said: ${MCP_ECHO_MESSAGE}`), + ); + expect(afterRestartFixture.readPermissionRequests(newSessionId, "execute").length).toBe(1); + }); + }); }); 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 fdfcc55..2c1de87 100644 --- a/src/__tests__/CodexACPAgent/e2e/acp-e2e-shell-approval.test.ts +++ b/src/__tests__/CodexACPAgent/e2e/acp-e2e-shell-approval.test.ts @@ -24,7 +24,7 @@ describeE2E("E2E shell approval tests", () => { let sessionId: string; beforeEach(async () => { - fixture = await createAuthenticatedFixture(AgentMode.ReadOnly); + fixture = await createAuthenticatedFixture({initialMode: AgentMode.ReadOnly}); sessionId = (await fixture.createSession()).sessionId; }); @@ -57,8 +57,8 @@ describeE2E("E2E shell approval tests", () => { expectPermissionRequests(fixture, sessionId, {execute: 2, edit: 0}); }); - 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); @@ -78,7 +78,7 @@ describeE2E("E2E Agent mode shell permission tests", () => { let fixture: SpawnedAgentFixture; beforeEach(async () => { - fixture = await createAuthenticatedFixture(AgentMode.Agent); + fixture = await createAuthenticatedFixture({initialMode: AgentMode.Agent}); }); afterEach(async () => { @@ -103,7 +103,7 @@ describeE2E("E2E Agent with full access shell permission tests", () => { let fixture: SpawnedAgentFixture; beforeEach(async () => { - fixture = await createAuthenticatedFixture(AgentMode.AgentFullAccess); + fixture = await createAuthenticatedFixture({initialMode: AgentMode.AgentFullAccess}); }); afterEach(async () => { @@ -135,7 +135,7 @@ describeE2E("E2E shell cancellation tests", () => { } it("cancels a running shell command", async () => { - fixture = await createAuthenticatedFixture(AgentMode.AgentFullAccess); + fixture = await createAuthenticatedFixture({initialMode: AgentMode.AgentFullAccess}); const sessionId = (await fixture.createSession()).sessionId; const pidFilePath = path.join(fixture.workspaceDir, "cancel-command.pid"); const command = `/bin/sh -c 'echo $$ > "${pidFilePath}"; exec sleep 100'`; diff --git a/src/__tests__/CodexACPAgent/e2e/acp-e2e-test-utils.ts b/src/__tests__/CodexACPAgent/e2e/acp-e2e-test-utils.ts index 429e79d..b625ad8 100644 --- a/src/__tests__/CodexACPAgent/e2e/acp-e2e-test-utils.ts +++ b/src/__tests__/CodexACPAgent/e2e/acp-e2e-test-utils.ts @@ -46,9 +46,14 @@ export function expectNoPermissionRequests(fixture: SpawnedAgentFixture, session expectPermissionRequests(fixture, sessionId, { edit: 0, execute: 0 }); } -export async function createAuthenticatedFixture(initialMode?: AgentMode): Promise { +export interface AuthenticatedFixtureOptions { + initialMode?: AgentMode; + configBackedMcpServers?: acp.McpServerStdio[]; +} + +export async function createAuthenticatedFixture(options: AuthenticatedFixtureOptions = {}): Promise { const apiKey = requireLiveApiKey(); - const extraEnv = initialMode ? {INITIAL_AGENT_MODE: initialMode.id} : undefined; + const extraEnv = options.initialMode ? {INITIAL_AGENT_MODE: options.initialMode.id} : undefined; return await createSpawnedFixture(async (connection, authMethods) => { if (!authMethods.some((method) => method.id === "api-key")) { throw new Error("API key authentication is not available."); @@ -67,7 +72,7 @@ export async function createAuthenticatedFixture(initialMode?: AgentMode): Promi if (authenticationStatus["type"] !== "api-key") { throw new Error(`Unexpected authentication status: ${JSON.stringify(authenticationStatus)}`); } - }, extraEnv); + }, extraEnv, options.configBackedMcpServers); } export async function createGatewayFixture( @@ -119,6 +124,7 @@ type Authenticator = (connection: acp.ClientSideConnection, authMethods: acp.Aut async function createSpawnedFixture( authenticate: Authenticator, extraEnv?: NodeJS.ProcessEnv, + configBackedMcpServers?: acp.McpServerStdio[], ): Promise { return await createSpawnedAgentFixture(async (connection) => { const initializeResponse = await connection.initialize({ @@ -135,7 +141,7 @@ async function createSpawnedFixture( } await authenticate(connection, initializeResponse.authMethods ?? []); - }, extraEnv); + }, extraEnv, undefined, configBackedMcpServers); } export function requireLiveApiKey(): string { diff --git a/src/__tests__/CodexACPAgent/e2e/acp-e2e.test.ts b/src/__tests__/CodexACPAgent/e2e/acp-e2e.test.ts index f625f0d..924b08e 100644 --- a/src/__tests__/CodexACPAgent/e2e/acp-e2e.test.ts +++ b/src/__tests__/CodexACPAgent/e2e/acp-e2e.test.ts @@ -79,7 +79,7 @@ describeE2E("E2E tests", () => { it("respects INITIAL_AGENT_MODE when seeding the initial session mode", async () => { const initialMode = AgentMode.ReadOnly; - fixture = await createAuthenticatedFixture(initialMode); + fixture = await createAuthenticatedFixture({initialMode}); const session = await fixture.createSession(); expect(session.modes?.currentModeId).toBe(initialMode.id); diff --git a/src/__tests__/CodexACPAgent/e2e/spawned-agent-fixture.ts b/src/__tests__/CodexACPAgent/e2e/spawned-agent-fixture.ts index 217c112..70cc06a 100644 --- a/src/__tests__/CodexACPAgent/e2e/spawned-agent-fixture.ts +++ b/src/__tests__/CodexACPAgent/e2e/spawned-agent-fixture.ts @@ -43,14 +43,16 @@ type ConnectionInitializer = (connection: acp.ClientSideConnection) => Promise { + const resolvedPaths = paths ?? RuntimePaths.createTemporary(configBackedMcpServers); const agentProcess = spawn("npm", ["run", "--silent", "start"], { cwd: process.cwd(), env: { ...process.env, - CODEX_HOME: paths.codexHome, - APP_SERVER_LOGS: paths.appServerLogsDir, + CODEX_HOME: resolvedPaths.codexHome, + APP_SERVER_LOGS: resolvedPaths.appServerLogsDir, ...extraEnv, }, stdio: ["pipe", "pipe", "pipe"], @@ -59,9 +61,10 @@ export async function createSpawnedAgentFixture( const fixture = new SpawnedAgentFixtureImpl( new RecordingClient(), agentProcess, - paths, + resolvedPaths, initializeConnection, extraEnv, + configBackedMcpServers, ); await initializeConnection(fixture.connection); return fixture; @@ -78,7 +81,7 @@ class RuntimePaths { this.appServerLogsDir = path.join(rootDir, "logs"); } - static createTemporary(): RuntimePaths { + static createTemporary(mcpServers: acp.McpServerStdio[] = []): RuntimePaths { const rootDir = path.join(process.cwd(), "tmp", crypto.randomUUID()); const paths = new RuntimePaths(rootDir); for (const dir of [paths.rootDir, paths.codexHome, paths.workspaceDir, paths.appServerLogsDir]) { @@ -88,7 +91,7 @@ class RuntimePaths { model: DEFAULT_TEST_MODEL_ID.model, model_reasoning_effort: DEFAULT_TEST_MODEL_ID.effort, web_search: "disabled", - }); + }, mcpServers); return paths; } } @@ -145,7 +148,8 @@ class SpawnedAgentFixtureImpl implements SpawnedAgentFixture { private readonly agentProcess: ChildProcessWithoutNullStreams, private readonly paths: RuntimePaths, private readonly initializeConnection: ConnectionInitializer, - private readonly extraEnv?: NodeJS.ProcessEnv, + private readonly extraEnv: NodeJS.ProcessEnv | undefined, + private readonly configBackedMcpServers: acp.McpServerStdio[], ) { const output = Readable.toWeb(agentProcess.stdout) as ReadableStream; this.connection = new acp.ClientSideConnection( @@ -167,7 +171,12 @@ class SpawnedAgentFixtureImpl implements SpawnedAgentFixture { async restart(): Promise { await this.stopProcess(false); - return await createSpawnedAgentFixture(this.initializeConnection, this.extraEnv, this.paths); + return await createSpawnedAgentFixture( + this.initializeConnection, + this.extraEnv, + this.paths, + this.configBackedMcpServers, + ); } writeSkill(skill: TestSkill): void { diff --git a/src/__tests__/CodexACPAgent/elicitation-events.test.ts b/src/__tests__/CodexACPAgent/elicitation-events.test.ts index 32548be..2cd132b 100644 --- a/src/__tests__/CodexACPAgent/elicitation-events.test.ts +++ b/src/__tests__/CodexACPAgent/elicitation-events.test.ts @@ -14,7 +14,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 +30,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); @@ -49,9 +50,9 @@ describe('Elicitation Events', () => { } describe('Form mode elicitation', () => { - it('should map accept to accept', async () => { + it('should map allow_once to accept', async () => { const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); - fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'accept' } }); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_once' } }); const params: McpServerElicitationRequestParams = { threadId: sessionId, turnId: 'turn-1', serverName: 'test-server', @@ -66,9 +67,9 @@ describe('Elicitation Events', () => { await promptPromise; }); - it('should map decline to decline', async () => { + it('should map reject_once to decline', async () => { const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); - fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'decline' } }); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'reject_once' } }); const params: McpServerElicitationRequestParams = { threadId: sessionId, turnId: 'turn-1', serverName: 'test-server', @@ -113,7 +114,7 @@ describe('Elicitation Events', () => { it('should build correct ACP permission request for form mode', async () => { const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); - fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'accept' } }); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_once' } }); const params: McpServerElicitationRequestParams = { threadId: sessionId, turnId: 'turn-1', serverName: 'my-mcp-server', @@ -130,7 +131,71 @@ 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({ + configBackedMcpServerNames: new Set(['tool-server']), + }); + 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); + await expect(fixture.getAcpConnectionDump([])).toMatchFileSnapshot('data/elicitation-tool-approval-all-persist.json'); + + completeTurn(); + await promptPromise; + }); + + it('should include CLI-style descriptions for MCP tool approval options', async () => { + const { promptPromise, completeTurn } = setupSessionWithPendingPrompt({ + configBackedMcpServerNames: new Set(['tool-server']), + }); + 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', + }, + { + optionId: 'allow_for_session', + name: 'Allow for this session', + kind: 'allow_always', + }, + { + optionId: 'allow_persist', + name: 'Always allow', + kind: 'allow_always', + }, + { + optionId: 'reject_once', + name: 'Cancel', + kind: 'reject_once', + }, + ]); + + completeTurn(); + await promptPromise; + }); + + it('should not show persistent approval for MCP servers that are not configured', async () => { const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_once' } }); @@ -143,7 +208,13 @@ describe('Elicitation Events', () => { }; await fixture.sendServerRequest('mcpServer/elicitation/request', params); - await expect(fixture.getAcpConnectionDump(['_meta'])).toMatchFileSnapshot('data/elicitation-tool-approval-all-persist.json'); + const [requestPermissionEvent] = fixture.getAcpConnectionEvents([]); + const optionIds = requestPermissionEvent?.args[0].options.map((option: { optionId: string }) => option.optionId); + expect(optionIds).toEqual([ + 'allow_once', + 'allow_for_session', + 'reject_once', + ]); completeTurn(); await promptPromise; @@ -168,9 +239,9 @@ describe('Elicitation Events', () => { await promptPromise; }); - it('should map allow_session to accept with persist:session meta', async () => { + it('should map allow_for_session to accept with persist:session meta', async () => { const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); - fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_session' } }); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_for_session' } }); const params: McpServerElicitationRequestParams = { threadId: sessionId, turnId: 'turn-1', serverName: 'tool-server', @@ -187,9 +258,11 @@ describe('Elicitation Events', () => { await promptPromise; }); - it('should map allow_always to accept with persist:always meta', async () => { - const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); - fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_always' } }); + it('should map persistent approval to accept with persist:always meta', async () => { + const { promptPromise, completeTurn } = setupSessionWithPendingPrompt({ + configBackedMcpServerNames: new Set(['tool-server']), + }); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_persist' } }); const params: McpServerElicitationRequestParams = { threadId: sessionId, turnId: 'turn-1', serverName: 'tool-server', @@ -219,13 +292,13 @@ describe('Elicitation Events', () => { }; await fixture.sendServerRequest('mcpServer/elicitation/request', params); - await expect(fixture.getAcpConnectionDump(['_meta'])).toMatchFileSnapshot('data/elicitation-tool-approval-session-only.json'); + await expect(fixture.getAcpConnectionDump([])).toMatchFileSnapshot('data/elicitation-tool-approval-session-only.json'); completeTurn(); 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' } }); @@ -238,7 +311,7 @@ describe('Elicitation Events', () => { }; await fixture.sendServerRequest('mcpServer/elicitation/request', params); - await expect(fixture.getAcpConnectionDump(['_meta'])).toMatchFileSnapshot('data/elicitation-tool-approval-no-persist.json'); + await expect(fixture.getAcpConnectionDump([])).toMatchFileSnapshot('data/elicitation-tool-approval-no-persist.json'); completeTurn(); await promptPromise; @@ -361,9 +434,9 @@ describe('Elicitation Events', () => { }); describe('URL mode elicitation', () => { - it('should map accept to accept for URL mode', async () => { + it('should map allow_once to accept for URL mode', async () => { const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); - fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'accept' } }); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_once' } }); const params: McpServerElicitationRequestParams = { threadId: sessionId, turnId: 'turn-1', serverName: 'auth-server', @@ -397,7 +470,7 @@ describe('Elicitation Events', () => { it('should build correct ACP permission request for URL mode', async () => { const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); - fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'accept' } }); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_once' } }); const params: McpServerElicitationRequestParams = { threadId: sessionId, turnId: 'turn-1', serverName: 'auth-server', diff --git a/src/__tests__/CodexACPAgent/initialize.test.ts b/src/__tests__/CodexACPAgent/initialize.test.ts index bf6e060..6b16455 100644 --- a/src/__tests__/CodexACPAgent/initialize.test.ts +++ b/src/__tests__/CodexACPAgent/initialize.test.ts @@ -51,7 +51,6 @@ describe('CodexACPAgent - initialize', () => { list: {}, }, mcpCapabilities: { - acp: false, http: true, sse: false, }, diff --git a/src/__tests__/acp-test-utils.ts b/src/__tests__/acp-test-utils.ts index ec510dc..79e2a4d 100644 --- a/src/__tests__/acp-test-utils.ts +++ b/src/__tests__/acp-test-utils.ts @@ -2,7 +2,7 @@ import {CodexAcpClient} from '../CodexAcpClient'; import {type CodexConnectionEvent, CodexAppServerClient} from '../CodexAppServerClient'; import {startCodexConnection} from "../CodexJsonRpcConnection"; import {CodexAcpServer, type SessionState} from "../CodexAcpServer"; -import type {AgentSideConnection, RequestPermissionResponse} from "@agentclientprotocol/sdk"; +import type {AgentSideConnection, McpServerStdio, RequestPermissionResponse} from "@agentclientprotocol/sdk"; import type {ServerNotification} from "../app-server"; import type {MessageConnection} from "vscode-jsonrpc/node"; import path from "node:path"; @@ -176,15 +176,37 @@ function createTestCodexHome(): string { return codexHome; } -export function writeCodexHomeConfig(codexHome: string, extras: Record = {}): void { +export function writeCodexHomeConfig( + codexHome: string, + extras: Record = {}, + mcpServers: McpServerStdio[] = [], +): void { const entries: Record = { cli_auth_credentials_store: "file", ...extras, }; - const body = Object.entries(entries) - .map(([key, value]) => `${key} = "${value}"`) - .join("\n"); - fs.writeFileSync(path.join(codexHome, "config.toml"), body, "utf8"); + const sections: string[] = [ + Object.entries(entries) + .map(([key, value]) => `${key} = "${value}"`) + .join("\n"), + ]; + for (const mcp of mcpServers) { + sections.push(stdioMcpServerSection(mcp)); + } + fs.writeFileSync(path.join(codexHome, "config.toml"), sections.join("\n\n") + "\n", "utf8"); +} + +function stdioMcpServerSection(mcp: McpServerStdio): string { + const lines: string[] = [`[mcp_servers.${mcp.name}]`]; + lines.push(`command = ${JSON.stringify(mcp.command)}`); + lines.push(`args = ${JSON.stringify(mcp.args)}`); + if (mcp.env.length > 0) { + const envEntries = mcp.env + .map(({name, value}) => `${JSON.stringify(name)} = ${JSON.stringify(value)}`) + .join(", "); + lines.push(`env = { ${envEntries} }`); + } + return lines.join("\n"); } export function removeDirectoryWithRetry(directory: string): void { @@ -325,6 +347,7 @@ export function createTestSessionState(overrides?: Partial): Sessi agentMode: AgentMode.DEFAULT_AGENT_MODE, fastModeEnabled: false, currentModelSupportsFast: false, + sessionMcpServers: [], ...overrides, }; }