From 76f9ea2e58f870f5e2968933815b567f79ce3c8a Mon Sep 17 00:00:00 2001 From: "Nikolai.Sviridov" Date: Wed, 29 Apr 2026 12:09:26 +0200 Subject: [PATCH 1/5] feat: LLM-25394 support Codex persist approval --- package-lock.json | 9 +- src/ApprovalOptionId.ts | 5 +- src/CodexAcpClient.ts | 41 +++- src/CodexAcpServer.ts | 23 +-- src/CodexAppServerClient.ts | 2 + src/CodexApprovalHandler.ts | 194 ++++++++++++++++-- src/CodexCommands.ts | 4 +- src/CodexElicitationHandler.ts | 80 ++++++-- .../CodexACPAgent/CodexAcpClient.test.ts | 1 + .../CodexACPAgent/approval-events.test.ts | 72 ++++++- .../data/approval-command-allow-once.json | 8 +- ...nd-available-decisions-without-prefix.json | 35 ++++ .../data/approval-command-network-policy.json | 45 ++++ .../data/approval-command-prefix-rule.json | 40 ++++ .../data/approval-command-with-rawInput.json | 8 +- .../data/approval-file-change.json | 8 +- .../data/elicitation-form-accept.json | 4 +- ...elicitation-tool-approval-all-persist.json | 16 +- .../elicitation-tool-approval-no-persist.json | 8 +- ...licitation-tool-approval-session-only.json | 12 +- .../data/elicitation-url-accept.json | 4 +- .../e2e/acp-e2e-file-approval.test.ts | 6 +- .../e2e/acp-e2e-mcp-approval.test.ts | 103 +++++++++- .../e2e/acp-e2e-shell-approval.test.ts | 12 +- .../CodexACPAgent/e2e/acp-e2e.test.ts | 2 +- .../CodexACPAgent/elicitation-events.test.ts | 126 +++++++++--- .../CodexACPAgent/initialize.test.ts | 1 - src/__tests__/acp-test-utils.ts | 1 + 28 files changed, 734 insertions(+), 136 deletions(-) create mode 100644 src/__tests__/CodexACPAgent/data/approval-command-available-decisions-without-prefix.json create mode 100644 src/__tests__/CodexACPAgent/data/approval-command-network-policy.json create mode 100644 src/__tests__/CodexACPAgent/data/approval-command-prefix-rule.json diff --git a/package-lock.json b/package-lock.json index 9c64924e..2fe670d9 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..969aa420 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 22cde0da..48220203 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 0865c984..22635c88 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 { @@ -126,7 +127,6 @@ export class CodexAcpServer implements acp.Agent { list: { } }, mcpCapabilities: { - acp: false, http: true, sse: false } @@ -185,7 +185,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 = { @@ -203,7 +202,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); @@ -452,7 +452,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 = { @@ -470,7 +469,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); @@ -724,14 +724,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) { @@ -955,7 +954,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 0d2d0832..cbafd3e0 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 0eddf3ac..618d39b6 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 a125d71d..bc20a65f 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 68a49947..e48dea06 100644 --- a/src/CodexElicitationHandler.ts +++ b/src/CodexElicitationHandler.ts @@ -9,16 +9,28 @@ import type { McpServerElicitationRequestResponse, } from "./app-server/v2"; import { logger } from "./Logger"; -import { McpApprovalOptionId } from "./McpApprovalOptionId"; +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" }, ]; 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. @@ -49,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: McpApprovalOptionId.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: McpApprovalOptionId.AllowSession, 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: McpApprovalOptionId.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: McpApprovalOptionId.Decline, name: "Decline", kind: "reject_once" }); + options.push(buildToolApprovalOption( + ApprovalOptionId.RejectOnce, + "Cancel", + "reject_once" + )); return options; } @@ -114,14 +147,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 !== McpApprovalOptionId.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 }; @@ -140,7 +173,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") { @@ -200,26 +236,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 === McpApprovalOptionId.AllowSession) { + 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 === McpApprovalOptionId.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 === McpApprovalOptionId.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 a66a8276..56380098 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 7ee43746..bbe6dfa8 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 453ed87b..d2a049b3 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 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..2d58ec67 --- /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 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..5050084f 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 b4c351f1..e4271fb4 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 a2113d5c..4483207b 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 d2f40807..1d0cabd9 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 b212b996..3aae8b2a 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 50358bee..c6a0d5f5 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 da7f9c0b..ea361244 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 c2a6fee2..20700cbf 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 e0ef5760..095a3384 100644 --- a/src/__tests__/CodexACPAgent/e2e/acp-e2e-mcp-approval.test.ts +++ b/src/__tests__/CodexACPAgent/e2e/acp-e2e-mcp-approval.test.ts @@ -1,8 +1,9 @@ import type * as acp from "@agentclientprotocol/sdk"; import fs from "node:fs"; import path from "node:path"; -import {afterEach, beforeEach, expect, it} from "vitest"; +import {afterEach, beforeEach, describe, expect, it} from "vitest"; import {McpApprovalOptionId, type McpApprovalOptionId as McpApprovalOptionIdValue} from "../../../McpApprovalOptionId"; +import {ApprovalOptionId} from "../../../ApprovalOptionId"; import { createAuthenticatedFixture, describeE2E, @@ -14,6 +15,7 @@ import os from "node:os"; 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(invocationMarkerPath: string): acp.McpServerStdio { return { @@ -63,6 +65,12 @@ function expectMcpPermissionRequestCount(fixture: SpawnedAgentFixture, sessionId } } +function failingPermissionResponder(label: string): PermissionResponder { + return (request) => { + throw new Error(`${label}: unexpected permission request (kind=${request.toolCall.kind})`); + }; +} + describeE2E("E2E MCP approval tests (configured in session)", () => { let fixture: SpawnedAgentFixture; @@ -188,4 +196,97 @@ describeE2E("E2E MCP approval tests (configured in toml)", () => { expect(fs.readFileSync(invocationMarkerPath, "utf8")).toBe("always approval second"); expect(fixture.readPermissionRequests(newSessionId, "execute").length).toBe(0); }); + + 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 fdfcc55d..2c1de875 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.ts b/src/__tests__/CodexACPAgent/e2e/acp-e2e.test.ts index a098976d..7cb7aa84 100644 --- a/src/__tests__/CodexACPAgent/e2e/acp-e2e.test.ts +++ b/src/__tests__/CodexACPAgent/e2e/acp-e2e.test.ts @@ -80,7 +80,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/elicitation-events.test.ts b/src/__tests__/CodexACPAgent/elicitation-events.test.ts index 82d14a09..2cd132b6 100644 --- a/src/__tests__/CodexACPAgent/elicitation-events.test.ts +++ b/src/__tests__/CodexACPAgent/elicitation-events.test.ts @@ -3,7 +3,6 @@ import type { McpServerElicitationRequestParams } from '../../app-server/v2'; import { createCodexMockTestFixture, createTestSessionState, type CodexMockTestFixture } from '../acp-test-utils'; import type { SessionState } from '../../CodexAcpServer'; import { AgentMode } from "../../AgentMode"; -import { McpApprovalOptionId } from "../../McpApprovalOptionId"; import type { ServerNotification } from "../../app-server"; describe('Elicitation Events', () => { @@ -15,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; @@ -31,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); @@ -50,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', @@ -67,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', @@ -114,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', @@ -131,9 +131,73 @@ 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: McpApprovalOptionId.AllowOnce } }); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_once' } }); const params: McpServerElicitationRequestParams = { threadId: sessionId, turnId: 'turn-1', serverName: 'tool-server', @@ -144,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; @@ -152,7 +222,7 @@ describe('Elicitation Events', () => { it('should map allow_once to accept with null meta', async () => { const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); - fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: McpApprovalOptionId.AllowOnce } }); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_once' } }); const params: McpServerElicitationRequestParams = { threadId: sessionId, turnId: 'turn-1', serverName: 'tool-server', @@ -169,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: McpApprovalOptionId.AllowSession } }); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_for_session' } }); const params: McpServerElicitationRequestParams = { threadId: sessionId, turnId: 'turn-1', serverName: 'tool-server', @@ -188,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: McpApprovalOptionId.AllowAlways } }); + 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', @@ -209,7 +281,7 @@ describe('Elicitation Events', () => { it('should only show session option when persist is "session"', async () => { const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); - fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: McpApprovalOptionId.AllowOnce } }); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_once' } }); const params: McpServerElicitationRequestParams = { threadId: sessionId, turnId: 'turn-1', serverName: 'tool-server', @@ -220,15 +292,15 @@ 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: McpApprovalOptionId.AllowOnce } }); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_once' } }); const params: McpServerElicitationRequestParams = { threadId: sessionId, turnId: 'turn-1', serverName: 'tool-server', @@ -239,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; @@ -247,7 +319,7 @@ describe('Elicitation Events', () => { it('should not reuse a completed auto-approved call id for a later approval request', async () => { const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); - fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: McpApprovalOptionId.AllowOnce } }); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_once' } }); const startedNotification: ServerNotification = { method: 'item/started', @@ -310,7 +382,7 @@ describe('Elicitation Events', () => { it('should not reuse a stale call id after serverRequest/resolved clears interrupted approval state', async () => { const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); - fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: McpApprovalOptionId.AllowOnce } }); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_once' } }); const startedNotification: ServerNotification = { method: 'item/started', @@ -362,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', @@ -398,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 bf6e0604..6b164550 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 9f82f16a..8e14b0d8 100644 --- a/src/__tests__/acp-test-utils.ts +++ b/src/__tests__/acp-test-utils.ts @@ -3,6 +3,7 @@ import {CodexAcpClient} from '../CodexAcpClient'; import {CodexAppServerClient, type CodexConnectionEvent} from '../CodexAppServerClient'; import {startCodexConnection} from "../CodexJsonRpcConnection"; import {CodexAcpServer, type SessionState} from "../CodexAcpServer"; +import type {AgentSideConnection, RequestPermissionResponse} from "@agentclientprotocol/sdk"; import type {ServerNotification} from "../app-server"; import type {MessageConnection} from "vscode-jsonrpc/node"; import path from "node:path"; From da653e45d48ff917a45409024dbebc5e5196e9af Mon Sep 17 00:00:00 2001 From: "Nikolai.Sviridov" Date: Wed, 27 May 2026 11:52:38 +0200 Subject: [PATCH 2/5] fix: rebase --- package-lock.json | 9 ++----- src/CodexAcpServer.ts | 1 + src/McpApprovalOptionId.ts | 8 ------ .../e2e/acp-e2e-file-approval.test.ts | 6 ++--- .../e2e/acp-e2e-mcp-approval.test.ts | 27 +++++++++---------- .../e2e/acp-e2e-shell-approval.test.ts | 8 +++--- .../CodexACPAgent/e2e/acp-e2e.test.ts | 2 +- .../CodexACPAgent/initialize.test.ts | 1 + src/__tests__/acp-test-utils.ts | 2 +- 9 files changed, 26 insertions(+), 38 deletions(-) delete mode 100644 src/McpApprovalOptionId.ts diff --git a/package-lock.json b/package-lock.json index 2fe670d9..9c64924e 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1284,7 +1284,6 @@ "integrity": "sha512-GNWcUTRBgIRJD5zj+Tq0fKOJ5XZajIiBroOF0yvj2bSU1WvNdYS/dn9UxwsujGW4JX06dnHyjV2y9rRaybH0iQ==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "undici-types": "~7.16.0" } @@ -1931,7 +1930,6 @@ "integrity": "sha512-F2X8g9P1X7uCPZMA3MVf9wcTqlyNp7IhH5qPCI0izhaOIYXaW9L535tGA3qmjRzpH+bZczqq7hVKxTR4NWnu+g==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "accepts": "~1.3.8", "array-flatten": "1.1.1", @@ -2634,7 +2632,6 @@ "integrity": "sha512-5gTmgEY/sqK6gFXLIsQNH19lWb4ebPDLA4SdLP7dsWkIXHWlG66oPuVvXSGFPppYZz8ZDZq0dYYrbHfBCVUb1Q==", "dev": true, "license": "MIT", - "peer": true, "engines": { "node": ">=12" }, @@ -2926,7 +2923,8 @@ "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" + "license": "MIT", + "peer": true }, "node_modules/send": { "version": "0.19.2", @@ -3190,7 +3188,6 @@ "integrity": "sha512-ytQKuwgmrrkDTFP4LjR0ToE2nqgy886GpvRSpU0JAnrdBYppuY5rLkRUYPU1yCryb24SsKBTL/hlDQAEFVwtZg==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "esbuild": "~0.25.0", "get-tsconfig": "^4.7.5" @@ -3276,7 +3273,6 @@ "integrity": "sha512-BxAKBWmIbrDgrokdGZH1IgkIk/5mMHDreLDmCJ0qpyJaAteP8NvMhkwr/ZCQNqNH97bw/dANTE9PDzqwJghfMQ==", "dev": true, "license": "MIT", - "peer": true, "dependencies": { "esbuild": "^0.25.0", "fdir": "^6.5.0", @@ -3494,7 +3490,6 @@ "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/CodexAcpServer.ts b/src/CodexAcpServer.ts index 22635c88..aef60aac 100644 --- a/src/CodexAcpServer.ts +++ b/src/CodexAcpServer.ts @@ -127,6 +127,7 @@ export class CodexAcpServer implements acp.Agent { list: { } }, mcpCapabilities: { + acp: false, http: true, sse: false } diff --git a/src/McpApprovalOptionId.ts b/src/McpApprovalOptionId.ts deleted file mode 100644 index 3728145a..00000000 --- a/src/McpApprovalOptionId.ts +++ /dev/null @@ -1,8 +0,0 @@ -export const McpApprovalOptionId = { - AllowOnce: "allow_once", - AllowSession: "allow_session", - AllowAlways: "allow_always", - Decline: "decline", -} as const; - -export type McpApprovalOptionId = typeof McpApprovalOptionId[keyof typeof McpApprovalOptionId]; 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 20700cbf..c2a6fee2 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({initialMode: AgentMode.ReadOnly}); + fixture = await createAuthenticatedFixture(AgentMode.ReadOnly); }); afterEach(async () => { @@ -43,7 +43,7 @@ describeE2E("E2E Agent mode file permission tests", () => { let fixture: SpawnedAgentFixture; beforeEach(async () => { - fixture = await createAuthenticatedFixture({initialMode: AgentMode.Agent}); + fixture = await createAuthenticatedFixture(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({initialMode: AgentMode.AgentFullAccess}); + fixture = await createAuthenticatedFixture(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 095a3384..b4857367 100644 --- a/src/__tests__/CodexACPAgent/e2e/acp-e2e-mcp-approval.test.ts +++ b/src/__tests__/CodexACPAgent/e2e/acp-e2e-mcp-approval.test.ts @@ -2,10 +2,9 @@ import type * as acp from "@agentclientprotocol/sdk"; import fs from "node:fs"; import path from "node:path"; import {afterEach, beforeEach, describe, expect, it} from "vitest"; -import {McpApprovalOptionId, type McpApprovalOptionId as McpApprovalOptionIdValue} from "../../../McpApprovalOptionId"; import {ApprovalOptionId} from "../../../ApprovalOptionId"; import { - createAuthenticatedFixture, + createAuthenticatedFixture, createPermissionResponse, describeE2E, expectEndTurn, type PermissionResponder, @@ -33,18 +32,18 @@ function isMcpPermissionRequest(request: acp.RequestPermissionRequest): boolean return request.toolCall.kind === "execute" && request._meta?.["is_mcp_tool_approval"] === true; } -function createMcpPermissionResponse(optionId: McpApprovalOptionIdValue | null): acp.RequestPermissionResponse { +function createMcpPermissionResponse(optionId: ApprovalOptionId | null): acp.RequestPermissionResponse { if (optionId === null) { return {outcome: {outcome: "cancelled"}}; } return {outcome: {outcome: "selected", optionId}}; } -function createMcpPermissionResponder(...optionIds: McpApprovalOptionIdValue[]): PermissionResponder { +function createMcpPermissionResponder(...optionIds: ApprovalOptionId[]): PermissionResponder { const queue = [...optionIds]; return (request) => createMcpPermissionResponse( isMcpPermissionRequest(request) - ? queue.shift() ?? McpApprovalOptionId.Decline + ? queue.shift() ?? ApprovalOptionId.RejectOnce : null, ); } @@ -89,7 +88,7 @@ describeE2E("E2E MCP approval tests (configured in session)", () => { } it("executes an approved MCP tool call", async () => { - fixture.setPermissionResponder(createMcpPermissionResponder(McpApprovalOptionId.AllowOnce)); + fixture.setPermissionResponder(createMcpPermissionResponder(ApprovalOptionId.AllowOnce)); const {sessionId, invocationMarkerPath} = await createMcpSession(); await expectEchoToolReply(fixture, sessionId, MCP_ECHO_MESSAGE); @@ -98,7 +97,7 @@ describeE2E("E2E MCP approval tests (configured in session)", () => { }); it("ends turn when MCP tool call is rejected", async () => { - fixture.setPermissionResponder(createMcpPermissionResponder(McpApprovalOptionId.Decline)); + fixture.setPermissionResponder(createMcpPermissionResponder(ApprovalOptionId.RejectOnce)); const {sessionId, invocationMarkerPath} = await createMcpSession(); expectEndTurn(await fixture.connection.prompt({ @@ -113,7 +112,7 @@ describeE2E("E2E MCP approval tests (configured in session)", () => { }); it("skips subsequent approvals in the same session when allow_session is selected", async () => { - fixture.setPermissionResponder(createMcpPermissionResponder(McpApprovalOptionId.AllowSession)); + fixture.setPermissionResponder(createMcpPermissionResponder(ApprovalOptionId.AllowForSession)); const {sessionId, invocationMarkerPath} = await createMcpSession(); await expectEchoToolReply(fixture, sessionId, "session approval first"); @@ -124,7 +123,7 @@ describeE2E("E2E MCP approval tests (configured in session)", () => { }); it("requests subsequent approvals after session restart when allow_session is selected", async () => { - fixture.setPermissionResponder(createMcpPermissionResponder(McpApprovalOptionId.AllowSession, McpApprovalOptionId.AllowOnce)); + fixture.setPermissionResponder(createMcpPermissionResponder(ApprovalOptionId.AllowForSession, ApprovalOptionId.AllowOnce)); const {sessionId, invocationMarkerPath} = await createMcpSession(); await expectEchoToolReply(fixture, sessionId, MCP_ECHO_MESSAGE); @@ -162,9 +161,10 @@ describeE2E("E2E MCP approval tests (configured in toml)", () => { fixture = await createAuthenticatedFixture(undefined, [createMcpServer(invocationMarkerPath)]); }); + //TODO: recheck allow_always == persist? it("skips subsequent approvals in the same session when allow_always is selected", async () => { fixture.setPermissionResponder( - createMcpPermissionResponder(McpApprovalOptionId.AllowAlways), + createMcpPermissionResponder(ApprovalOptionId.AllowPersist), ); const sessionId = (await fixture.createSession()).sessionId; @@ -177,7 +177,7 @@ describeE2E("E2E MCP approval tests (configured in toml)", () => { it.skip("skips subsequent approvals after session restart when allow_always is selected", async () => { fixture.setPermissionResponder( - createMcpPermissionResponder(McpApprovalOptionId.AllowAlways), + createMcpPermissionResponder(ApprovalOptionId.AllowPersist), ); const firstSessionId = (await fixture.createSession()).sessionId; @@ -200,15 +200,14 @@ describeE2E("E2E MCP approval tests (configured in toml)", () => { describe("persisted approvals", () => { let beforeRestartFixture: SpawnedAgentFixture | null = null; let afterRestartFixture: SpawnedAgentFixture | null = null; + let sessionId: string; 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()], - }); + beforeRestartFixture = await createAuthenticatedFixture(undefined, [createMcpServer(invocationMarkerPath)]); fixture = beforeRestartFixture; sessionId = (await fixture.createSession()).sessionId; }); 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 2c1de875..6f7f5e41 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({initialMode: AgentMode.ReadOnly}); + fixture = await createAuthenticatedFixture(AgentMode.ReadOnly); sessionId = (await fixture.createSession()).sessionId; }); @@ -78,7 +78,7 @@ describeE2E("E2E Agent mode shell permission tests", () => { let fixture: SpawnedAgentFixture; beforeEach(async () => { - fixture = await createAuthenticatedFixture({initialMode: AgentMode.Agent}); + fixture = await createAuthenticatedFixture(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({initialMode: AgentMode.AgentFullAccess}); + fixture = await createAuthenticatedFixture(AgentMode.AgentFullAccess); }); afterEach(async () => { @@ -135,7 +135,7 @@ describeE2E("E2E shell cancellation tests", () => { } it("cancels a running shell command", async () => { - fixture = await createAuthenticatedFixture({initialMode: AgentMode.AgentFullAccess}); + fixture = await createAuthenticatedFixture(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.ts b/src/__tests__/CodexACPAgent/e2e/acp-e2e.test.ts index 7cb7aa84..a098976d 100644 --- a/src/__tests__/CodexACPAgent/e2e/acp-e2e.test.ts +++ b/src/__tests__/CodexACPAgent/e2e/acp-e2e.test.ts @@ -80,7 +80,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/initialize.test.ts b/src/__tests__/CodexACPAgent/initialize.test.ts index 6b164550..bf6e0604 100644 --- a/src/__tests__/CodexACPAgent/initialize.test.ts +++ b/src/__tests__/CodexACPAgent/initialize.test.ts @@ -51,6 +51,7 @@ 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 8e14b0d8..4041c5a9 100644 --- a/src/__tests__/acp-test-utils.ts +++ b/src/__tests__/acp-test-utils.ts @@ -3,7 +3,6 @@ import {CodexAcpClient} from '../CodexAcpClient'; import {CodexAppServerClient, type CodexConnectionEvent} from '../CodexAppServerClient'; import {startCodexConnection} from "../CodexJsonRpcConnection"; import {CodexAcpServer, type SessionState} from "../CodexAcpServer"; -import type {AgentSideConnection, RequestPermissionResponse} from "@agentclientprotocol/sdk"; import type {ServerNotification} from "../app-server"; import type {MessageConnection} from "vscode-jsonrpc/node"; import path from "node:path"; @@ -346,6 +345,7 @@ export function createTestSessionState(overrides?: Partial): Sessi agentMode: AgentMode.DEFAULT_AGENT_MODE, fastModeEnabled: false, currentModelSupportsFast: false, + sessionMcpServers: [], ...overrides, }; } From b1db93dffba2192ef58f2cfdfecc51f6d9a67356 Mon Sep 17 00:00:00 2001 From: "Nikolai.Sviridov" Date: Wed, 27 May 2026 12:21:55 +0200 Subject: [PATCH 3/5] fix: non persist test, remove skip for persist test --- .../CodexACPAgent/e2e/acp-e2e-mcp-approval.test.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) 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 b4857367..a53253d9 100644 --- a/src/__tests__/CodexACPAgent/e2e/acp-e2e-mcp-approval.test.ts +++ b/src/__tests__/CodexACPAgent/e2e/acp-e2e-mcp-approval.test.ts @@ -161,10 +161,9 @@ describeE2E("E2E MCP approval tests (configured in toml)", () => { fixture = await createAuthenticatedFixture(undefined, [createMcpServer(invocationMarkerPath)]); }); - //TODO: recheck allow_always == persist? - it("skips subsequent approvals in the same session when allow_always is selected", async () => { + it("skips subsequent approvals in the same session when allow_for_session is selected", async () => { fixture.setPermissionResponder( - createMcpPermissionResponder(ApprovalOptionId.AllowPersist), + createMcpPermissionResponder(ApprovalOptionId.AllowForSession), ); const sessionId = (await fixture.createSession()).sessionId; @@ -175,7 +174,7 @@ describeE2E("E2E MCP approval tests (configured in toml)", () => { expectMcpPermissionRequestCount(fixture, sessionId, 1); }); - it.skip("skips subsequent approvals after session restart when allow_always is selected", async () => { + it("skips subsequent approvals after session restart when allow_persist is selected", async () => { fixture.setPermissionResponder( createMcpPermissionResponder(ApprovalOptionId.AllowPersist), ); From 123433db74de0c527f56bcbb33f3d034047c49e8 Mon Sep 17 00:00:00 2001 From: "Nikolai.Sviridov" Date: Wed, 27 May 2026 12:37:13 +0200 Subject: [PATCH 4/5] chore: remove duplicate persist tests (existing in master after rebase) --- .../e2e/acp-e2e-mcp-approval.test.ts | 104 +----------------- 1 file changed, 2 insertions(+), 102 deletions(-) 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 a53253d9..600ba36f 100644 --- a/src/__tests__/CodexACPAgent/e2e/acp-e2e-mcp-approval.test.ts +++ b/src/__tests__/CodexACPAgent/e2e/acp-e2e-mcp-approval.test.ts @@ -1,11 +1,10 @@ import type * as acp from "@agentclientprotocol/sdk"; import fs from "node:fs"; import path from "node:path"; -import {afterEach, beforeEach, describe, expect, it} from "vitest"; +import {afterEach, beforeEach, expect, it} from "vitest"; import {ApprovalOptionId} from "../../../ApprovalOptionId"; import { - createAuthenticatedFixture, createPermissionResponse, - describeE2E, + createAuthenticatedFixture, describeE2E, expectEndTurn, type PermissionResponder, type SpawnedAgentFixture, @@ -14,7 +13,6 @@ import os from "node:os"; 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(invocationMarkerPath: string): acp.McpServerStdio { return { @@ -64,12 +62,6 @@ function expectMcpPermissionRequestCount(fixture: SpawnedAgentFixture, sessionId } } -function failingPermissionResponder(label: string): PermissionResponder { - return (request) => { - throw new Error(`${label}: unexpected permission request (kind=${request.toolCall.kind})`); - }; -} - describeE2E("E2E MCP approval tests (configured in session)", () => { let fixture: SpawnedAgentFixture; @@ -195,96 +187,4 @@ describeE2E("E2E MCP approval tests (configured in toml)", () => { expect(fs.readFileSync(invocationMarkerPath, "utf8")).toBe("always approval second"); expect(fixture.readPermissionRequests(newSessionId, "execute").length).toBe(0); }); - - describe("persisted approvals", () => { - let beforeRestartFixture: SpawnedAgentFixture | null = null; - let afterRestartFixture: SpawnedAgentFixture | null = null; - let sessionId: string; - - 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(undefined, [createMcpServer(invocationMarkerPath)]); - 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); - }); - }); }); From 57faddbf0e5a6ca935872bbb3defdd560745323f Mon Sep 17 00:00:00 2001 From: "Nikolai.Sviridov" Date: Wed, 27 May 2026 13:00:42 +0200 Subject: [PATCH 5/5] chore: remove stale snapshots --- ...nd-available-decisions-without-prefix.json | 35 ---------------- .../data/approval-command-prefix-rule.json | 40 ------------------- 2 files changed, 75 deletions(-) delete mode 100644 src/__tests__/CodexACPAgent/data/approval-command-available-decisions-without-prefix.json delete mode 100644 src/__tests__/CodexACPAgent/data/approval-command-prefix-rule.json 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 deleted file mode 100644 index cab8eddf..00000000 --- a/src/__tests__/CodexACPAgent/data/approval-command-available-decisions-without-prefix.json +++ /dev/null @@ -1,35 +0,0 @@ -{ - "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-prefix-rule.json b/src/__tests__/CodexACPAgent/data/approval-command-prefix-rule.json deleted file mode 100644 index 73b53697..00000000 --- a/src/__tests__/CodexACPAgent/data/approval-command-prefix-rule.json +++ /dev/null @@ -1,40 +0,0 @@ -{ - "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