From 29015214a5b7af63ed06be4ad550749251464fc9 Mon Sep 17 00:00:00 2001 From: "Nikolai.Sviridov" Date: Fri, 17 Apr 2026 00:48:09 +0200 Subject: [PATCH 1/9] LLM-27035 [Codex] Add missing elicitation --- src/CodexAcpClient.ts | 6 +- src/CodexAcpServer.ts | 9 +- src/CodexAppServerClient.ts | 25 ++ src/CodexElicitationHandler.ts | 178 +++++++++++ src/CodexEventHandler.ts | 6 +- src/PendingMcpApprovals.ts | 27 ++ .../data/elicitation-form-accept.json | 48 +++ ...elicitation-tool-approval-all-persist.json | 51 +++ .../elicitation-tool-approval-no-persist.json | 41 +++ ...licitation-tool-approval-session-only.json | 46 +++ .../data/elicitation-url-accept.json | 38 +++ .../CodexACPAgent/elicitation-events.test.ts | 301 ++++++++++++++++++ 12 files changed, 771 insertions(+), 5 deletions(-) create mode 100644 src/CodexElicitationHandler.ts create mode 100644 src/PendingMcpApprovals.ts create mode 100644 src/__tests__/CodexACPAgent/data/elicitation-form-accept.json create mode 100644 src/__tests__/CodexACPAgent/data/elicitation-tool-approval-all-persist.json create mode 100644 src/__tests__/CodexACPAgent/data/elicitation-tool-approval-no-persist.json create mode 100644 src/__tests__/CodexACPAgent/data/elicitation-tool-approval-session-only.json create mode 100644 src/__tests__/CodexACPAgent/data/elicitation-url-accept.json create mode 100644 src/__tests__/CodexACPAgent/elicitation-events.test.ts diff --git a/src/CodexAcpClient.ts b/src/CodexAcpClient.ts index 39ed8998..a47a8cfb 100644 --- a/src/CodexAcpClient.ts +++ b/src/CodexAcpClient.ts @@ -2,7 +2,7 @@ import {isCodexAuthRequest} from "./CodexAuthMethod"; import type {EmbeddedResourceResource} from "@agentclientprotocol/sdk"; import * as acp from "@agentclientprotocol/sdk"; import {type McpServer, RequestError} from "@agentclientprotocol/sdk"; -import type {ApprovalHandler, CodexAppServerClient} from "./CodexAppServerClient"; +import type {ApprovalHandler, CodexAppServerClient, ElicitationHandler} from "./CodexAppServerClient"; import open from "open"; import type {Disposable} from "vscode-jsonrpc"; import type { @@ -360,10 +360,12 @@ export class CodexAcpClient { async subscribeToSessionEvents( sessionId: string, eventHandler: (result: ServerNotification) => void, - approvalHandler: ApprovalHandler + approvalHandler: ApprovalHandler, + elicitationHandler: ElicitationHandler ) { this.codexClient.onServerNotification(sessionId, eventHandler); this.codexClient.onApprovalRequest(sessionId, approvalHandler); + this.codexClient.onElicitationRequest(sessionId, elicitationHandler); } async sendPrompt( diff --git a/src/CodexAcpServer.ts b/src/CodexAcpServer.ts index e96576d5..c99d4abb 100644 --- a/src/CodexAcpServer.ts +++ b/src/CodexAcpServer.ts @@ -7,6 +7,8 @@ import { } from "@agentclientprotocol/sdk"; import {CodexEventHandler} from "./CodexEventHandler"; import {CodexApprovalHandler} from "./CodexApprovalHandler"; +import {CodexElicitationHandler} from "./CodexElicitationHandler"; +import {PendingMcpApprovals} from "./PendingMcpApprovals"; import {CodexAuthMethods, type CodexAuthRequest} from "./CodexAuthMethod"; import {CodexAcpClient, type SessionMetadata, type SessionMetadataWithThread} from "./CodexAcpClient"; import {ACPSessionConnection, type UpdateSessionEvent} from "./ACPSessionConnection"; @@ -705,11 +707,14 @@ export class CodexAcpServer implements acp.Agent { sessionState.lastTokenUsage = null; try { - const eventHandler = new CodexEventHandler(this.connection, sessionState); + const pendingMcpApprovals = new PendingMcpApprovals(); + const eventHandler = new CodexEventHandler(this.connection, sessionState, pendingMcpApprovals); const approvalHandler = new CodexApprovalHandler(this.connection, sessionState); + const elicitationHandler = new CodexElicitationHandler(this.connection, sessionState, pendingMcpApprovals); await this.codexAcpClient.subscribeToSessionEvents(params.sessionId, (event) => eventHandler.handleNotification(event), - approvalHandler); + approvalHandler, + elicitationHandler); if (await this.availableCommands.tryHandle(params.prompt, sessionState)) { logger.log("Prompt handled by a command"); diff --git a/src/CodexAppServerClient.ts b/src/CodexAppServerClient.ts index 8fb6ce71..376332af 100644 --- a/src/CodexAppServerClient.ts +++ b/src/CodexAppServerClient.ts @@ -28,6 +28,8 @@ import type { CommandExecutionRequestApprovalResponse, FileChangeRequestApprovalParams, FileChangeRequestApprovalResponse, + McpServerElicitationRequestParams, + McpServerElicitationRequestResponse, ThreadResumeParams, ThreadResumeResponse, SkillsListParams, @@ -41,6 +43,10 @@ export interface ApprovalHandler { handleFileChange(params: FileChangeRequestApprovalParams): Promise; } +export interface ElicitationHandler { + handleElicitation(params: McpServerElicitationRequestParams): Promise; +} + const CommandExecutionApprovalRequest = new RequestType< CommandExecutionRequestApprovalParams, CommandExecutionRequestApprovalResponse, @@ -53,6 +59,12 @@ const FileChangeApprovalRequest = new RequestType< void >('item/fileChange/requestApproval'); +const McpServerElicitationRequest = new RequestType< + McpServerElicitationRequestParams, + McpServerElicitationRequestResponse, + void +>('mcpServer/elicitation/request'); + /** * A type-safe client over the Codex App Server's JSON-RPC API. * Maps each request to its expected response and exposes clear, typed methods for supported JSON-RPC operations. @@ -60,6 +72,7 @@ const FileChangeApprovalRequest = new RequestType< export class CodexAppServerClient { readonly connection: MessageConnection; private approvalHandlers = new Map(); + private elicitationHandlers = new Map(); private mcpStartupCompleteVersion = 0; private lastMcpStartupComplete: McpStartupCompleteEvent | null = null; private readonly mcpStartupCompleteResolvers: Array> = []; @@ -98,12 +111,24 @@ export class CodexAppServerClient { } return await handler.handleFileChange(params); }); + + this.connection.onRequest(McpServerElicitationRequest, async (params) => { + const handler = this.elicitationHandlers.get(params.threadId); + if (!handler) { + return { action: "cancel", content: null, _meta: null }; + } + return await handler.handleElicitation(params); + }); } onApprovalRequest(threadId: string, handler: ApprovalHandler): void { this.approvalHandlers.set(threadId, handler); } + onElicitationRequest(threadId: string, handler: ElicitationHandler): void { + this.elicitationHandlers.set(threadId, handler); + } + async initialize(params: InitializeParams): Promise { return await this.sendRequest({ method: "initialize", params: params }); } diff --git a/src/CodexElicitationHandler.ts b/src/CodexElicitationHandler.ts new file mode 100644 index 00000000..0736dd78 --- /dev/null +++ b/src/CodexElicitationHandler.ts @@ -0,0 +1,178 @@ +import * as acp from "@agentclientprotocol/sdk"; +import type { SessionState } from "./CodexAcpServer"; +import type { ElicitationHandler } from "./CodexAppServerClient"; +import type { + McpServerElicitationRequestParams, + McpServerElicitationRequestResponse, +} from "./app-server/v2"; +import { logger } from "./Logger"; +import type { PendingMcpApprovals } from "./PendingMcpApprovals"; + +// 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" }, +]; + +// Option IDs used for MCP tool call approval persist choices. +const OPTION_ALLOW_ONCE = "allow_once"; +const OPTION_ALLOW_SESSION = "allow_session"; +const OPTION_ALLOW_ALWAYS = "allow_always"; + +type PersistValue = "session" | "always"; + +/** + * Parses the `persist` field from the elicitation request `_meta`. + * Codex advertises which persistence options the client should show. + * Returns a set of supported persist values. + */ +function parsePersistOptions(meta: unknown): Set { + const result = new Set(); + if (!meta || typeof meta !== "object") return result; + const persist = (meta as Record)["persist"]; + if (persist === "session") { + result.add("session"); + } else if (persist === "always") { + result.add("always"); + } else if (Array.isArray(persist)) { + if (persist.includes("session")) result.add("session"); + if (persist.includes("always")) result.add("always"); + } + return result; +} + +function isMcpToolCallApproval(meta: unknown): boolean { + return ( + meta !== null && + typeof meta === "object" && + (meta as Record)["codex_approval_kind"] === "mcp_tool_call" + ); +} + +/** + * Builds the ACP permission options for an MCP tool call approval elicitation. + * Always includes "Allow Once"; adds session/always persist options when advertised. + */ +function buildToolApprovalOptions(persistOptions: Set): acp.PermissionOption[] { + const options: acp.PermissionOption[] = [ + { optionId: OPTION_ALLOW_ONCE, name: "Allow", kind: "allow_once" }, + ]; + if (persistOptions.has("session")) { + options.push({ optionId: OPTION_ALLOW_SESSION, name: "Allow for This Session", kind: "allow_always" }); + } + if (persistOptions.has("always")) { + options.push({ optionId: OPTION_ALLOW_ALWAYS, name: "Allow and Don't Ask Again", kind: "allow_always" }); + } + options.push({ optionId: "decline", name: "Decline", kind: "reject_once" }); + return options; +} + +export class CodexElicitationHandler implements ElicitationHandler { + private readonly connection: acp.AgentSideConnection; + private readonly sessionState: SessionState; + private readonly pendingMcpApprovals: PendingMcpApprovals | undefined; + + constructor( + connection: acp.AgentSideConnection, + sessionState: SessionState, + pendingMcpApprovals?: PendingMcpApprovals + ) { + this.connection = connection; + this.sessionState = sessionState; + this.pendingMcpApprovals = pendingMcpApprovals; + } + + async handleElicitation( + params: McpServerElicitationRequestParams + ): Promise { + try { + const request = this.buildPermissionRequest(params); + const response = await this.connection.requestPermission(request); + return this.convertResponse(response); + } catch (error) { + logger.error("Error handling MCP elicitation request", error); + return { action: "cancel", content: null, _meta: null }; + } + } + + private buildPermissionRequest( + params: McpServerElicitationRequestParams + ): acp.RequestPermissionRequest { + const sessionId = this.sessionState.sessionId; + const messageContent: acp.ToolCallContent = { + type: "content", + content: { type: "text", text: params.message }, + }; + + const meta = params._meta; + const isToolApproval = isMcpToolCallApproval(meta); + const options = isToolApproval + ? buildToolApprovalOptions(parsePersistOptions(meta)) + : ELICITATION_OPTIONS; + + if (params.mode === "form") { + const correlatedCallId = isToolApproval + ? this.pendingMcpApprovals?.pop(params.threadId, params.serverName) + : undefined; + if (correlatedCallId !== undefined) { + // The tool call item is already visible in the IDE conversation history because + // item/started was emitted before the elicitation request. Sending content or + // rawInput here would duplicate that information in the approval widget. + return { + sessionId, + toolCall: { + toolCallId: correlatedCallId, + kind: "execute", + status: "pending", + // content: [messageContent], — omitted: already rendered via item/started + // rawInput: { ... } — omitted: same reason + }, + options, + }; + } + return { + sessionId, + toolCall: { + toolCallId: `elicitation-${params.serverName}`, + kind: isToolApproval ? "execute" : "other", + status: "pending", + content: [messageContent], + rawInput: { serverName: params.serverName, schema: params.requestedSchema }, + }, + options, + }; + } else { + return { + sessionId, + toolCall: { + toolCallId: `elicitation-${params.elicitationId}`, + kind: "fetch", + status: "pending", + content: [messageContent], + rawInput: { serverName: params.serverName, url: params.url }, + }, + options, + }; + } + } + + private convertResponse( + response: acp.RequestPermissionResponse + ): McpServerElicitationRequestResponse { + if (response.outcome.outcome === "cancelled") { + return { action: "cancel", content: null, _meta: null }; + } + + const optionId = response.outcome.optionId; + if (optionId === OPTION_ALLOW_SESSION) { + return { action: "accept", content: null, _meta: { persist: "session" } }; + } + if (optionId === OPTION_ALLOW_ALWAYS) { + return { action: "accept", content: null, _meta: { persist: "always" } }; + } + if (optionId === OPTION_ALLOW_ONCE || optionId === "accept") { + return { action: "accept", content: null, _meta: null }; + } + return { action: "decline", content: null, _meta: null }; + } +} diff --git a/src/CodexEventHandler.ts b/src/CodexEventHandler.ts index f771660b..dc2a3db6 100644 --- a/src/CodexEventHandler.ts +++ b/src/CodexEventHandler.ts @@ -34,6 +34,7 @@ import { fuzzyFileSearchToolCallId, } from "./CodexToolCallMapper"; import { stripShellPrefix } from "./CommandUtils"; +import type { PendingMcpApprovals } from "./PendingMcpApprovals"; export { stripShellPrefix }; @@ -43,10 +44,12 @@ export class CodexEventHandler { private readonly sessionState: SessionState; private failure: RequestError | null = null; private readonly activeFuzzyFileSearchSessions = new Set(); + private readonly pendingMcpApprovals: PendingMcpApprovals | undefined; - constructor(connection: acp.AgentSideConnection, sessionState: SessionState) { + constructor(connection: acp.AgentSideConnection, sessionState: SessionState, pendingMcpApprovals?: PendingMcpApprovals) { this.connection = connection; this.sessionState = sessionState; + this.pendingMcpApprovals = pendingMcpApprovals; } getFailure(): RequestError | null { @@ -193,6 +196,7 @@ export class CodexEventHandler { case "commandExecution": return await createCommandExecutionUpdate(event.item); case "mcpToolCall": + this.pendingMcpApprovals?.record(event.threadId, event.item.server, event.item.id); return await createMcpToolCallUpdate(event.item); case "dynamicToolCall": return await createDynamicToolCallUpdate(event.item); diff --git a/src/PendingMcpApprovals.ts b/src/PendingMcpApprovals.ts new file mode 100644 index 00000000..7192a65c --- /dev/null +++ b/src/PendingMcpApprovals.ts @@ -0,0 +1,27 @@ +// In Rust, the MCP elicitation handler receives ElicitationRequestEvent directly from the MCP +// protocol layer, where id is set to "mcp_tool_call_approval_" — the call ID is extracted +// by stripping that prefix. +// +// In TypeScript, Codex speaks the app-server JSON-RPC protocol (v2), where McpServerElicitationRequestParams +// omits elicitationId for form mode, so the MCP-level ID never reaches the client. +// +// Workaround: before requesting approval, Codex emits an item/started notification with an mcpToolCall +// item carrying the call id and server name. This class stores (threadId, serverName) → callId so the +// elicitation handler can retrieve it when the request arrives. +// +// Multiple calls are safe because Codex requests approval synchronously — it blocks on one tool call's +// elicitation before starting the next, so there is at most one pending approval per (threadId, serverName). +export class PendingMcpApprovals { + private readonly pending = new Map(); + + record(threadId: string, serverName: string, callId: string): void { + this.pending.set(`${threadId}:${serverName}`, callId); + } + + pop(threadId: string, serverName: string): string | undefined { + const key = `${threadId}:${serverName}`; + const callId = this.pending.get(key); + this.pending.delete(key); + return callId; + } +} diff --git a/src/__tests__/CodexACPAgent/data/elicitation-form-accept.json b/src/__tests__/CodexACPAgent/data/elicitation-form-accept.json new file mode 100644 index 00000000..a2113d5c --- /dev/null +++ b/src/__tests__/CodexACPAgent/data/elicitation-form-accept.json @@ -0,0 +1,48 @@ +{ + "method": "requestPermission", + "args": [ + { + "sessionId": "test-session-id", + "toolCall": { + "toolCallId": "elicitation-my-mcp-server", + "kind": "other", + "status": "pending", + "content": [ + { + "type": "content", + "content": { + "type": "text", + "text": "Please provide your GitHub username" + } + } + ], + "rawInput": { + "serverName": "my-mcp-server", + "schema": { + "type": "object", + "properties": { + "name": { + "type": "string" + } + }, + "required": [ + "name" + ] + } + } + }, + "options": [ + { + "optionId": "accept", + "name": "Accept", + "kind": "allow_once" + }, + { + "optionId": "decline", + "name": "Decline", + "kind": "reject_once" + } + ] + } + ] +} \ No newline at end of file diff --git a/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-all-persist.json b/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-all-persist.json new file mode 100644 index 00000000..409128e5 --- /dev/null +++ b/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-all-persist.json @@ -0,0 +1,51 @@ +{ + "method": "requestPermission", + "args": [ + { + "sessionId": "test-session-id", + "toolCall": { + "toolCallId": "elicitation-tool-server", + "kind": "other", + "status": "pending", + "content": [ + { + "type": "content", + "content": { + "type": "text", + "text": "Allow tool call?" + } + } + ], + "rawInput": { + "serverName": "tool-server", + "schema": { + "type": "object", + "properties": {} + } + } + }, + "options": [ + { + "optionId": "allow_once", + "name": "Allow", + "kind": "allow_once" + }, + { + "optionId": "allow_session", + "name": "Allow for This Session", + "kind": "allow_always" + }, + { + "optionId": "allow_always", + "name": "Allow and Don't Ask Again", + "kind": "allow_always" + }, + { + "optionId": "decline", + "name": "Decline", + "kind": "reject_once" + } + ] + } + ] +} \ No newline at end of file diff --git a/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-no-persist.json b/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-no-persist.json new file mode 100644 index 00000000..aecd7686 --- /dev/null +++ b/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-no-persist.json @@ -0,0 +1,41 @@ +{ + "method": "requestPermission", + "args": [ + { + "sessionId": "test-session-id", + "toolCall": { + "toolCallId": "elicitation-tool-server", + "kind": "other", + "status": "pending", + "content": [ + { + "type": "content", + "content": { + "type": "text", + "text": "Allow tool call?" + } + } + ], + "rawInput": { + "serverName": "tool-server", + "schema": { + "type": "object", + "properties": {} + } + } + }, + "options": [ + { + "optionId": "allow_once", + "name": "Allow", + "kind": "allow_once" + }, + { + "optionId": "decline", + "name": "Decline", + "kind": "reject_once" + } + ] + } + ] +} \ No newline at end of file diff --git a/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-session-only.json b/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-session-only.json new file mode 100644 index 00000000..d9bef72d --- /dev/null +++ b/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-session-only.json @@ -0,0 +1,46 @@ +{ + "method": "requestPermission", + "args": [ + { + "sessionId": "test-session-id", + "toolCall": { + "toolCallId": "elicitation-tool-server", + "kind": "other", + "status": "pending", + "content": [ + { + "type": "content", + "content": { + "type": "text", + "text": "Allow tool call?" + } + } + ], + "rawInput": { + "serverName": "tool-server", + "schema": { + "type": "object", + "properties": {} + } + } + }, + "options": [ + { + "optionId": "allow_once", + "name": "Allow", + "kind": "allow_once" + }, + { + "optionId": "allow_session", + "name": "Allow for This Session", + "kind": "allow_always" + }, + { + "optionId": "decline", + "name": "Decline", + "kind": "reject_once" + } + ] + } + ] +} \ No newline at end of file diff --git a/src/__tests__/CodexACPAgent/data/elicitation-url-accept.json b/src/__tests__/CodexACPAgent/data/elicitation-url-accept.json new file mode 100644 index 00000000..da7f9c0b --- /dev/null +++ b/src/__tests__/CodexACPAgent/data/elicitation-url-accept.json @@ -0,0 +1,38 @@ +{ + "method": "requestPermission", + "args": [ + { + "sessionId": "test-session-id", + "toolCall": { + "toolCallId": "elicitation-elicit-789", + "kind": "fetch", + "status": "pending", + "content": [ + { + "type": "content", + "content": { + "type": "text", + "text": "Please authorize access to your GitHub account" + } + } + ], + "rawInput": { + "serverName": "auth-server", + "url": "https://example.com/authorize?id=elicit-789" + } + }, + "options": [ + { + "optionId": "accept", + "name": "Accept", + "kind": "allow_once" + }, + { + "optionId": "decline", + "name": "Decline", + "kind": "reject_once" + } + ] + } + ] +} \ No newline at end of file diff --git a/src/__tests__/CodexACPAgent/elicitation-events.test.ts b/src/__tests__/CodexACPAgent/elicitation-events.test.ts new file mode 100644 index 00000000..b7450040 --- /dev/null +++ b/src/__tests__/CodexACPAgent/elicitation-events.test.ts @@ -0,0 +1,301 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +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"; + +describe('Elicitation Events', () => { + let fixture: CodexMockTestFixture; + const sessionId = 'test-session-id'; + + beforeEach(() => { + fixture = createCodexMockTestFixture(); + vi.clearAllMocks(); + }); + + function setupSessionWithPendingPrompt() { + const codexAcpAgent = fixture.getCodexAcpAgent(); + + let resolveTurnCompleted: (value: { threadId: string; turn: { id: string; items: never[]; status: string; error: null } }) => void; + const turnCompletedPromise = new Promise<{ threadId: string; turn: { id: string; items: never[]; status: string; error: null } }>((resolve) => { + resolveTurnCompleted = resolve; + }); + + fixture.getCodexAppServerClient().turnStart = vi.fn().mockResolvedValue({ + turn: { id: "turn-id", items: [], status: "inProgress", error: null } + }); + fixture.getCodexAppServerClient().awaitTurnCompleted = vi.fn().mockReturnValue(turnCompletedPromise); + + const sessionState: SessionState = createTestSessionState({ + sessionId, + currentModelId: 'model-id[effort]', + agentMode: AgentMode.DEFAULT_AGENT_MODE + }); + vi.spyOn(codexAcpAgent, 'getSessionState').mockReturnValue(sessionState); + + const promptPromise = codexAcpAgent.prompt({ + sessionId, + prompt: [{ type: 'text', text: 'Test prompt' }] + }); + + return { + promptPromise, + completeTurn: () => resolveTurnCompleted!({ + threadId: sessionId, + turn: { id: "turn-id", items: [], status: "completed", error: null } + }) + }; + } + + describe('Form mode elicitation', () => { + it('should map accept to accept', async () => { + const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'accept' } }); + + const params: McpServerElicitationRequestParams = { + threadId: sessionId, turnId: 'turn-1', serverName: 'test-server', + mode: 'form', _meta: null, message: 'Please provide your username', + requestedSchema: { type: 'object', properties: { username: { type: 'string' } }, required: ['username'] }, + }; + + const response = await fixture.sendServerRequest('mcpServer/elicitation/request', params); + expect(response).toEqual({ action: 'accept', content: null, _meta: null }); + + completeTurn(); + await promptPromise; + }); + + it('should map decline to decline', async () => { + const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'decline' } }); + + const params: McpServerElicitationRequestParams = { + threadId: sessionId, turnId: 'turn-1', serverName: 'test-server', + mode: 'form', _meta: null, message: 'Please provide info', + requestedSchema: { type: 'object', properties: {} }, + }; + + const response = await fixture.sendServerRequest('mcpServer/elicitation/request', params); + expect(response).toEqual({ action: 'decline', content: null, _meta: null }); + + completeTurn(); + await promptPromise; + }); + + it('should return cancel when user dismisses dialog', async () => { + const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); + fixture.setPermissionResponse({ outcome: { outcome: 'cancelled' } }); + + const params: McpServerElicitationRequestParams = { + threadId: sessionId, turnId: 'turn-1', serverName: 'test-server', + mode: 'form', _meta: null, message: 'Please provide info', + requestedSchema: { type: 'object', properties: {} }, + }; + + const response = await fixture.sendServerRequest('mcpServer/elicitation/request', params); + expect(response).toEqual({ action: 'cancel', content: null, _meta: null }); + + completeTurn(); + await promptPromise; + }); + + it('should return cancel when no handler registered', async () => { + const params: McpServerElicitationRequestParams = { + threadId: 'non-existent-session', turnId: null, serverName: 'test-server', + mode: 'form', _meta: null, message: 'Please provide info', + requestedSchema: { type: 'object', properties: {} }, + }; + + const response = await fixture.sendServerRequest('mcpServer/elicitation/request', params); + expect(response).toEqual({ action: 'cancel', content: null, _meta: null }); + }); + + it('should build correct ACP permission request for form mode', async () => { + const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'accept' } }); + + const params: McpServerElicitationRequestParams = { + threadId: sessionId, turnId: 'turn-1', serverName: 'my-mcp-server', + mode: 'form', _meta: null, message: 'Please provide your GitHub username', + requestedSchema: { type: 'object', properties: { name: { type: 'string' } }, required: ['name'] }, + }; + + await fixture.sendServerRequest('mcpServer/elicitation/request', params); + await expect(fixture.getAcpConnectionDump(['_meta'])).toMatchFileSnapshot('data/elicitation-form-accept.json'); + + completeTurn(); + await promptPromise; + }); + }); + + describe('MCP tool call approval elicitation', () => { + it('should show Allow/session/always/Decline options when all persist values advertised', async () => { + const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_once' } }); + + const params: McpServerElicitationRequestParams = { + threadId: sessionId, turnId: 'turn-1', serverName: 'tool-server', + mode: 'form', + _meta: { codex_approval_kind: 'mcp_tool_call', persist: ['session', 'always'] }, + message: 'Allow tool call?', + requestedSchema: { type: 'object', properties: {} }, + }; + + await fixture.sendServerRequest('mcpServer/elicitation/request', params); + await expect(fixture.getAcpConnectionDump(['_meta'])).toMatchFileSnapshot('data/elicitation-tool-approval-all-persist.json'); + + completeTurn(); + await promptPromise; + }); + + it('should map allow_once to accept with null meta', async () => { + const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_once' } }); + + const params: McpServerElicitationRequestParams = { + threadId: sessionId, turnId: 'turn-1', serverName: 'tool-server', + mode: 'form', + _meta: { codex_approval_kind: 'mcp_tool_call', persist: ['session', 'always'] }, + message: 'Allow tool call?', + requestedSchema: { type: 'object', properties: {} }, + }; + + const response = await fixture.sendServerRequest('mcpServer/elicitation/request', params); + expect(response).toEqual({ action: 'accept', content: null, _meta: null }); + + completeTurn(); + await promptPromise; + }); + + it('should map allow_session to accept with persist:session meta', async () => { + const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_session' } }); + + const params: McpServerElicitationRequestParams = { + threadId: sessionId, turnId: 'turn-1', serverName: 'tool-server', + mode: 'form', + _meta: { codex_approval_kind: 'mcp_tool_call', persist: ['session', 'always'] }, + message: 'Allow tool call?', + requestedSchema: { type: 'object', properties: {} }, + }; + + const response = await fixture.sendServerRequest('mcpServer/elicitation/request', params); + expect(response).toEqual({ action: 'accept', content: null, _meta: { persist: 'session' } }); + + completeTurn(); + 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' } }); + + const params: McpServerElicitationRequestParams = { + threadId: sessionId, turnId: 'turn-1', serverName: 'tool-server', + mode: 'form', + _meta: { codex_approval_kind: 'mcp_tool_call', persist: ['session', 'always'] }, + message: 'Allow tool call?', + requestedSchema: { type: 'object', properties: {} }, + }; + + const response = await fixture.sendServerRequest('mcpServer/elicitation/request', params); + expect(response).toEqual({ action: 'accept', content: null, _meta: { persist: 'always' } }); + + completeTurn(); + await promptPromise; + }); + + it('should only show session option when persist is "session"', async () => { + const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_once' } }); + + const params: McpServerElicitationRequestParams = { + threadId: sessionId, turnId: 'turn-1', serverName: 'tool-server', + mode: 'form', + _meta: { codex_approval_kind: 'mcp_tool_call', persist: 'session' }, + message: 'Allow tool call?', + requestedSchema: { type: 'object', properties: {} }, + }; + + await fixture.sendServerRequest('mcpServer/elicitation/request', params); + await expect(fixture.getAcpConnectionDump(['_meta'])).toMatchFileSnapshot('data/elicitation-tool-approval-session-only.json'); + + completeTurn(); + await promptPromise; + }); + + it('should show only Allow and Decline when no persist options', async () => { + const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_once' } }); + + const params: McpServerElicitationRequestParams = { + threadId: sessionId, turnId: 'turn-1', serverName: 'tool-server', + mode: 'form', + _meta: { codex_approval_kind: 'mcp_tool_call' }, + message: 'Allow tool call?', + requestedSchema: { type: 'object', properties: {} }, + }; + + await fixture.sendServerRequest('mcpServer/elicitation/request', params); + await expect(fixture.getAcpConnectionDump(['_meta'])).toMatchFileSnapshot('data/elicitation-tool-approval-no-persist.json'); + + completeTurn(); + await promptPromise; + }); + }); + + describe('URL mode elicitation', () => { + it('should map accept to accept for URL mode', async () => { + const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'accept' } }); + + const params: McpServerElicitationRequestParams = { + threadId: sessionId, turnId: 'turn-1', serverName: 'auth-server', + mode: 'url', _meta: null, message: 'Please authorize access', + url: 'https://example.com/authorize', elicitationId: 'elicit-123', + }; + + const response = await fixture.sendServerRequest('mcpServer/elicitation/request', params); + expect(response).toEqual({ action: 'accept', content: null, _meta: null }); + + completeTurn(); + await promptPromise; + }); + + it('should return cancel when user dismisses URL mode dialog', async () => { + const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); + fixture.setPermissionResponse({ outcome: { outcome: 'cancelled' } }); + + const params: McpServerElicitationRequestParams = { + threadId: sessionId, turnId: null, serverName: 'auth-server', + mode: 'url', _meta: null, message: 'Authorization required', + url: 'https://example.com/authorize', elicitationId: 'elicit-456', + }; + + const response = await fixture.sendServerRequest('mcpServer/elicitation/request', params); + expect(response).toEqual({ action: 'cancel', content: null, _meta: null }); + + completeTurn(); + await promptPromise; + }); + + it('should build correct ACP permission request for URL mode', async () => { + const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'accept' } }); + + const params: McpServerElicitationRequestParams = { + threadId: sessionId, turnId: 'turn-1', serverName: 'auth-server', + mode: 'url', _meta: null, + message: 'Please authorize access to your GitHub account', + url: 'https://example.com/authorize?id=elicit-789', + elicitationId: 'elicit-789', + }; + + await fixture.sendServerRequest('mcpServer/elicitation/request', params); + await expect(fixture.getAcpConnectionDump(['_meta'])).toMatchFileSnapshot('data/elicitation-url-accept.json'); + + completeTurn(); + await promptPromise; + }); + }); +}); From a5ea808ac518cc1e01fd5b60cdb14b0adb430f13 Mon Sep 17 00:00:00 2001 From: "Nikolai.Sviridov" Date: Fri, 17 Apr 2026 23:52:41 +0200 Subject: [PATCH 2/9] LLM-27036 [Codex] Improve mcp block detection --- src/CodexToolCallMapper.ts | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/CodexToolCallMapper.ts b/src/CodexToolCallMapper.ts index ab818662..f9bb2174 100644 --- a/src/CodexToolCallMapper.ts +++ b/src/CodexToolCallMapper.ts @@ -86,12 +86,15 @@ export async function createCommandExecutionUpdate( export async function createMcpToolCallUpdate( item: ThreadItem & { type: "mcpToolCall" } ): Promise { - return createExecuteToolCallUpdate( - item, - `mcp.${item.server}.${item.tool}`, - createMcpRawInput(item.server, item.tool, item.arguments), - createMcpRawOutput(item.result, item.error), - ); + return { + ...await createExecuteToolCallUpdate( + item, + `mcp.${item.server}.${item.tool}`, + createMcpRawInput(item.server, item.tool, item.arguments), + createMcpRawOutput(item.result, item.error), + ), + _meta: { is_mcp_tool_call: true }, + }; } export async function createDynamicToolCallUpdate( From 200ab41e0cf0237025732233ae9ba9774d646b38 Mon Sep 17 00:00:00 2001 From: "Nikolai.Sviridov" Date: Mon, 20 Apr 2026 13:27:58 +0200 Subject: [PATCH 3/9] fix: update mcp tool on approval --- src/CodexElicitationHandler.ts | 68 +++++++++++++++++++++------------- 1 file changed, 43 insertions(+), 25 deletions(-) diff --git a/src/CodexElicitationHandler.ts b/src/CodexElicitationHandler.ts index 0736dd78..e927c20c 100644 --- a/src/CodexElicitationHandler.ts +++ b/src/CodexElicitationHandler.ts @@ -86,8 +86,17 @@ export class CodexElicitationHandler implements ElicitationHandler { params: McpServerElicitationRequestParams ): Promise { try { - const request = this.buildPermissionRequest(params); + const { request, correlatedCallId } = this.buildPermissionRequest(params); const response = await this.connection.requestPermission(request); + if (correlatedCallId !== undefined && response.outcome.outcome !== "cancelled") { + const optionId = response.outcome.optionId; + if (optionId !== "decline") { + await this.connection.sessionUpdate({ + sessionId: this.sessionState.sessionId, + update: { sessionUpdate: "tool_call_update", toolCallId: correlatedCallId, status: "in_progress" }, + }); + } + } return this.convertResponse(response); } catch (error) { logger.error("Error handling MCP elicitation request", error); @@ -97,7 +106,7 @@ export class CodexElicitationHandler implements ElicitationHandler { private buildPermissionRequest( params: McpServerElicitationRequestParams - ): acp.RequestPermissionRequest { + ): { request: acp.RequestPermissionRequest; correlatedCallId: string | undefined } { const sessionId = this.sessionState.sessionId; const messageContent: acp.ToolCallContent = { type: "content", @@ -119,39 +128,48 @@ export class CodexElicitationHandler implements ElicitationHandler { // item/started was emitted before the elicitation request. Sending content or // rawInput here would duplicate that information in the approval widget. return { + request: { + sessionId, + toolCall: { + toolCallId: correlatedCallId, + kind: "execute", + status: "pending", + // content: [messageContent], — omitted: already rendered via item/started + // rawInput: { ... } — omitted: same reason + }, + options, + }, + correlatedCallId, + }; + } + return { + request: { sessionId, toolCall: { - toolCallId: correlatedCallId, - kind: "execute", + toolCallId: `elicitation-${params.serverName}`, + kind: isToolApproval ? "execute" : "other", status: "pending", - // content: [messageContent], — omitted: already rendered via item/started - // rawInput: { ... } — omitted: same reason + content: [messageContent], + rawInput: { serverName: params.serverName, schema: params.requestedSchema }, }, options, - }; - } - return { - sessionId, - toolCall: { - toolCallId: `elicitation-${params.serverName}`, - kind: isToolApproval ? "execute" : "other", - status: "pending", - content: [messageContent], - rawInput: { serverName: params.serverName, schema: params.requestedSchema }, }, - options, + correlatedCallId: undefined, }; } else { return { - sessionId, - toolCall: { - toolCallId: `elicitation-${params.elicitationId}`, - kind: "fetch", - status: "pending", - content: [messageContent], - rawInput: { serverName: params.serverName, url: params.url }, + request: { + sessionId, + toolCall: { + toolCallId: `elicitation-${params.elicitationId}`, + kind: "fetch", + status: "pending", + content: [messageContent], + rawInput: { serverName: params.serverName, url: params.url }, + }, + options, }, - options, + correlatedCallId: undefined, }; } } From 7c54ae3e0a2a88793b71f051092d356bc3000cc3 Mon Sep 17 00:00:00 2001 From: "Nikolai.Sviridov" Date: Mon, 20 Apr 2026 13:36:56 +0200 Subject: [PATCH 4/9] fix: update test snapshots --- .../data/elicitation-tool-approval-all-persist.json | 2 +- .../data/elicitation-tool-approval-no-persist.json | 2 +- .../data/elicitation-tool-approval-session-only.json | 2 +- src/__tests__/CodexACPAgent/data/load-session-history.json | 3 +++ .../CodexACPAgent/data/mcp-tool-completed-with-logs.json | 3 +++ src/__tests__/CodexACPAgent/data/mcp-tool-in-progress.json | 3 +++ .../CodexACPAgent/data/mcp-tool-repeated-progress.json | 3 +++ 7 files changed, 15 insertions(+), 3 deletions(-) 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 409128e5..61e97fc8 100644 --- a/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-all-persist.json +++ b/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-all-persist.json @@ -5,7 +5,7 @@ "sessionId": "test-session-id", "toolCall": { "toolCallId": "elicitation-tool-server", - "kind": "other", + "kind": "execute", "status": "pending", "content": [ { 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 aecd7686..c5aff8f2 100644 --- a/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-no-persist.json +++ b/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-no-persist.json @@ -5,7 +5,7 @@ "sessionId": "test-session-id", "toolCall": { "toolCallId": "elicitation-tool-server", - "kind": "other", + "kind": "execute", "status": "pending", "content": [ { 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 d9bef72d..dd19852c 100644 --- a/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-session-only.json +++ b/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-session-only.json @@ -5,7 +5,7 @@ "sessionId": "test-session-id", "toolCall": { "toolCallId": "elicitation-tool-server", - "kind": "other", + "kind": "execute", "status": "pending", "content": [ { diff --git a/src/__tests__/CodexACPAgent/data/load-session-history.json b/src/__tests__/CodexACPAgent/data/load-session-history.json index 097e7f02..8e962238 100644 --- a/src/__tests__/CodexACPAgent/data/load-session-history.json +++ b/src/__tests__/CodexACPAgent/data/load-session-history.json @@ -163,6 +163,9 @@ "server": "github", "tool": "search", "arguments": {} + }, + "_meta": { + "is_mcp_tool_call": true } } } diff --git a/src/__tests__/CodexACPAgent/data/mcp-tool-completed-with-logs.json b/src/__tests__/CodexACPAgent/data/mcp-tool-completed-with-logs.json index f2530266..0395df19 100644 --- a/src/__tests__/CodexACPAgent/data/mcp-tool-completed-with-logs.json +++ b/src/__tests__/CodexACPAgent/data/mcp-tool-completed-with-logs.json @@ -64,6 +64,9 @@ "start_line": 1, "max_lines": 200 } + }, + "_meta": { + "is_mcp_tool_call": true } } } diff --git a/src/__tests__/CodexACPAgent/data/mcp-tool-in-progress.json b/src/__tests__/CodexACPAgent/data/mcp-tool-in-progress.json index d6a5122c..a2e980c3 100644 --- a/src/__tests__/CodexACPAgent/data/mcp-tool-in-progress.json +++ b/src/__tests__/CodexACPAgent/data/mcp-tool-in-progress.json @@ -15,6 +15,9 @@ "arguments": { "argument": "example" } + }, + "_meta": { + "is_mcp_tool_call": true } } } diff --git a/src/__tests__/CodexACPAgent/data/mcp-tool-repeated-progress.json b/src/__tests__/CodexACPAgent/data/mcp-tool-repeated-progress.json index d60c6e28..395404fa 100644 --- a/src/__tests__/CodexACPAgent/data/mcp-tool-repeated-progress.json +++ b/src/__tests__/CodexACPAgent/data/mcp-tool-repeated-progress.json @@ -75,6 +75,9 @@ "arguments": { "argument": "example" } + }, + "_meta": { + "is_mcp_tool_call": true } } } From 7f8d7ca9eee12027f225a2e7dd15cc0df7a42c38 Mon Sep 17 00:00:00 2001 From: "Nikolai.Sviridov" Date: Mon, 20 Apr 2026 23:57:25 +0200 Subject: [PATCH 5/9] fix: pop() stale entries mcpToolCall --- src/CodexEventHandler.ts | 1 + .../CodexACPAgent/elicitation-events.test.ts | 64 +++++++++++++++++++ 2 files changed, 65 insertions(+) diff --git a/src/CodexEventHandler.ts b/src/CodexEventHandler.ts index dc2a3db6..38c11e19 100644 --- a/src/CodexEventHandler.ts +++ b/src/CodexEventHandler.ts @@ -226,6 +226,7 @@ export class CodexEventHandler { status: event.item.status === "completed" ? "completed" : "failed", } case "mcpToolCall": + this.pendingMcpApprovals?.pop(event.threadId, event.item.server); return { sessionUpdate: "tool_call_update", toolCallId: event.item.id, diff --git a/src/__tests__/CodexACPAgent/elicitation-events.test.ts b/src/__tests__/CodexACPAgent/elicitation-events.test.ts index b7450040..6a7067ee 100644 --- a/src/__tests__/CodexACPAgent/elicitation-events.test.ts +++ b/src/__tests__/CodexACPAgent/elicitation-events.test.ts @@ -3,6 +3,7 @@ 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 type { ServerNotification } from "../../app-server"; describe('Elicitation Events', () => { let fixture: CodexMockTestFixture; @@ -242,6 +243,69 @@ describe('Elicitation Events', () => { completeTurn(); await promptPromise; }); + + 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: 'allow_once' } }); + + const startedNotification: ServerNotification = { + method: 'item/started', + params: { + threadId: sessionId, + turnId: 'turn-1', + item: { + type: "mcpToolCall", + id: "completed-call-id", + server: "tool-server", + tool: "tool-name", + status: "inProgress", + arguments: { argument: "example" }, + result: null, + error: null, + durationMs: null, + }, + }, + }; + const completedNotification: ServerNotification = { + method: 'item/completed', + params: { + threadId: sessionId, + turnId: 'turn-1', + item: { + type: "mcpToolCall", + id: "completed-call-id", + server: "tool-server", + tool: "tool-name", + status: "completed", + arguments: { argument: "example" }, + result: { content: [], structuredContent: null, _meta: null }, + error: null, + durationMs: 15, + }, + }, + }; + + fixture.sendServerNotification(startedNotification); + fixture.sendServerNotification(completedNotification); + fixture.clearAcpConnectionDump(); + + const params: McpServerElicitationRequestParams = { + threadId: sessionId, turnId: 'turn-2', 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(['_meta']); + expect(requestPermissionEvent?.method).toBe('requestPermission'); + expect(requestPermissionEvent?.args[0].toolCall.toolCallId).toBe('elicitation-tool-server'); + + completeTurn(); + await promptPromise; + }); }); describe('URL mode elicitation', () => { From f7e743f3b31b739d76fe1a1de3f09649826aedf3 Mon Sep 17 00:00:00 2001 From: "Nikolai.Sviridov" Date: Tue, 21 Apr 2026 00:04:57 +0200 Subject: [PATCH 6/9] fix: cleanup on on serverRequest/resolved --- src/CodexEventHandler.ts | 4 +- src/PendingMcpApprovals.ts | 20 ++++++- .../CodexACPAgent/elicitation-events.test.ts | 52 +++++++++++++++++++ 3 files changed, 73 insertions(+), 3 deletions(-) diff --git a/src/CodexEventHandler.ts b/src/CodexEventHandler.ts index 38c11e19..84d8a3ec 100644 --- a/src/CodexEventHandler.ts +++ b/src/CodexEventHandler.ts @@ -106,11 +106,13 @@ export class CodexEventHandler { case "turn/diff/updated": case "item/commandExecution/terminalInteraction": case "item/fileChange/outputDelta": - case "serverRequest/resolved": case "account/updated": case "fs/changed": case "mcpServer/startupStatus/updated": return null; + case "serverRequest/resolved": + this.pendingMcpApprovals?.clearThread(notification.params.threadId); + return null; case "item/mcpToolCall/progress": return this.createMcpToolProgressEvent(notification.params); case "account/rateLimits/updated": diff --git a/src/PendingMcpApprovals.ts b/src/PendingMcpApprovals.ts index 7192a65c..7f272e60 100644 --- a/src/PendingMcpApprovals.ts +++ b/src/PendingMcpApprovals.ts @@ -15,13 +15,29 @@ export class PendingMcpApprovals { private readonly pending = new Map(); record(threadId: string, serverName: string, callId: string): void { - this.pending.set(`${threadId}:${serverName}`, callId); + this.pending.set(this.key(threadId, serverName), callId); } pop(threadId: string, serverName: string): string | undefined { - const key = `${threadId}:${serverName}`; + const key = this.key(threadId, serverName); const callId = this.pending.get(key); this.pending.delete(key); return callId; } + + clearThread(threadId: string): void { + for (const key of this.pending.keys()) { + if (this.belongsToThread(key, threadId)) { + this.pending.delete(key); + } + } + } + + private key(threadId: string, serverName: string): string { + return `${threadId}:${serverName}`; + } + + private belongsToThread(key: string, threadId: string): boolean { + return key.startsWith(`${threadId}:`); + } } diff --git a/src/__tests__/CodexACPAgent/elicitation-events.test.ts b/src/__tests__/CodexACPAgent/elicitation-events.test.ts index 6a7067ee..32548bee 100644 --- a/src/__tests__/CodexACPAgent/elicitation-events.test.ts +++ b/src/__tests__/CodexACPAgent/elicitation-events.test.ts @@ -306,6 +306,58 @@ describe('Elicitation Events', () => { completeTurn(); await promptPromise; }); + + 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: 'allow_once' } }); + + const startedNotification: ServerNotification = { + method: 'item/started', + params: { + threadId: sessionId, + turnId: 'turn-1', + item: { + type: "mcpToolCall", + id: "interrupted-call-id", + server: "tool-server", + tool: "tool-name", + status: "inProgress", + arguments: { argument: "example" }, + result: null, + error: null, + durationMs: null, + }, + }, + }; + const resolvedNotification: ServerNotification = { + method: 'serverRequest/resolved', + params: { + threadId: sessionId, + requestId: 'request-1', + }, + }; + + fixture.sendServerNotification(startedNotification); + fixture.sendServerNotification(resolvedNotification); + fixture.clearAcpConnectionDump(); + + const params: McpServerElicitationRequestParams = { + threadId: sessionId, turnId: 'turn-2', 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(['_meta']); + expect(requestPermissionEvent?.method).toBe('requestPermission'); + expect(requestPermissionEvent?.args[0].toolCall.toolCallId).toBe('elicitation-tool-server'); + + completeTurn(); + await promptPromise; + }); }); describe('URL mode elicitation', () => { From 6cc27ebc2de478b214afbb9c9280a7960106b2af Mon Sep 17 00:00:00 2001 From: "Nikolai.Sviridov" Date: Tue, 21 Apr 2026 00:08:57 +0200 Subject: [PATCH 7/9] fix: move pendingMcpApprovals to src/CodexElicitationHandler.ts --- src/CodexAcpServer.ts | 11 +++-- src/CodexElicitationHandler.ts | 83 ++++++++++++++++++++++++++++++---- src/CodexEventHandler.ts | 9 +--- src/PendingMcpApprovals.ts | 43 ------------------ 4 files changed, 81 insertions(+), 65 deletions(-) delete mode 100644 src/PendingMcpApprovals.ts diff --git a/src/CodexAcpServer.ts b/src/CodexAcpServer.ts index c99d4abb..f7425ff6 100644 --- a/src/CodexAcpServer.ts +++ b/src/CodexAcpServer.ts @@ -8,7 +8,6 @@ import { import {CodexEventHandler} from "./CodexEventHandler"; import {CodexApprovalHandler} from "./CodexApprovalHandler"; import {CodexElicitationHandler} from "./CodexElicitationHandler"; -import {PendingMcpApprovals} from "./PendingMcpApprovals"; import {CodexAuthMethods, type CodexAuthRequest} from "./CodexAuthMethod"; import {CodexAcpClient, type SessionMetadata, type SessionMetadataWithThread} from "./CodexAcpClient"; import {ACPSessionConnection, type UpdateSessionEvent} from "./ACPSessionConnection"; @@ -707,12 +706,14 @@ export class CodexAcpServer implements acp.Agent { sessionState.lastTokenUsage = null; try { - const pendingMcpApprovals = new PendingMcpApprovals(); - const eventHandler = new CodexEventHandler(this.connection, sessionState, pendingMcpApprovals); + const eventHandler = new CodexEventHandler(this.connection, sessionState); const approvalHandler = new CodexApprovalHandler(this.connection, sessionState); - const elicitationHandler = new CodexElicitationHandler(this.connection, sessionState, pendingMcpApprovals); + const elicitationHandler = new CodexElicitationHandler(this.connection, sessionState); await this.codexAcpClient.subscribeToSessionEvents(params.sessionId, - (event) => eventHandler.handleNotification(event), + (event) => { + elicitationHandler.handleNotification(event); + return eventHandler.handleNotification(event); + }, approvalHandler, elicitationHandler); diff --git a/src/CodexElicitationHandler.ts b/src/CodexElicitationHandler.ts index e927c20c..1b9b9d96 100644 --- a/src/CodexElicitationHandler.ts +++ b/src/CodexElicitationHandler.ts @@ -1,12 +1,14 @@ import * as acp from "@agentclientprotocol/sdk"; import type { SessionState } from "./CodexAcpServer"; import type { ElicitationHandler } from "./CodexAppServerClient"; +import type { ServerNotification } from "./app-server"; import type { + ItemCompletedNotification, + ItemStartedNotification, McpServerElicitationRequestParams, McpServerElicitationRequestResponse, } from "./app-server/v2"; import { logger } from "./Logger"; -import type { PendingMcpApprovals } from "./PendingMcpApprovals"; // Standard elicitation options (non-tool-call approval). const ELICITATION_OPTIONS: acp.PermissionOption[] = [ @@ -70,16 +72,42 @@ function buildToolApprovalOptions(persistOptions: Set): acp.Permis export class CodexElicitationHandler implements ElicitationHandler { private readonly connection: acp.AgentSideConnection; private readonly sessionState: SessionState; - private readonly pendingMcpApprovals: PendingMcpApprovals | undefined; + // In Rust, the MCP elicitation handler receives ElicitationRequestEvent directly from the MCP + // protocol layer, where id is set to "mcp_tool_call_approval_" — the call ID is extracted + // by stripping that prefix. + // + // In TypeScript, Codex speaks the app-server JSON-RPC protocol (v2), where + // McpServerElicitationRequestParams omits elicitationId for form mode, so the MCP-level ID never + // reaches the client. + // + // Workaround: before requesting approval, Codex emits an item/started notification with an + // mcpToolCall item carrying the call id and server name. We store (threadId, serverName) → callId + // here so the elicitation request can correlate back to the already-rendered tool call item. + // + // Multiple calls are safe because Codex requests approval synchronously — it blocks on one tool + // call's elicitation before starting the next, so there is at most one pending approval per + // (threadId, serverName). + private readonly pendingMcpApprovals = new Map(); - constructor( - connection: acp.AgentSideConnection, - sessionState: SessionState, - pendingMcpApprovals?: PendingMcpApprovals - ) { + constructor(connection: acp.AgentSideConnection, sessionState: SessionState) { this.connection = connection; this.sessionState = sessionState; - this.pendingMcpApprovals = pendingMcpApprovals; + } + + handleNotification(notification: ServerNotification): void { + switch (notification.method) { + case "item/started": + this.handleItemStarted(notification.params); + return; + case "item/completed": + this.handleItemCompleted(notification.params); + return; + case "serverRequest/resolved": + this.clearThread(notification.params.threadId); + return; + default: + return; + } } async handleElicitation( @@ -121,7 +149,7 @@ export class CodexElicitationHandler implements ElicitationHandler { if (params.mode === "form") { const correlatedCallId = isToolApproval - ? this.pendingMcpApprovals?.pop(params.threadId, params.serverName) + ? this.popPendingApproval(params.threadId, params.serverName) : undefined; if (correlatedCallId !== undefined) { // The tool call item is already visible in the IDE conversation history because @@ -193,4 +221,41 @@ export class CodexElicitationHandler implements ElicitationHandler { } return { action: "decline", content: null, _meta: null }; } + + private handleItemStarted(event: ItemStartedNotification): void { + if (event.item.type !== "mcpToolCall") { + return; + } + this.pendingMcpApprovals.set(this.key(event.threadId, event.item.server), event.item.id); + } + + private handleItemCompleted(event: ItemCompletedNotification): void { + if (event.item.type !== "mcpToolCall") { + return; + } + this.popPendingApproval(event.threadId, event.item.server); + } + + private popPendingApproval(threadId: string, serverName: string): string | undefined { + const key = this.key(threadId, serverName); + const callId = this.pendingMcpApprovals.get(key); + this.pendingMcpApprovals.delete(key); + return callId; + } + + private clearThread(threadId: string): void { + for (const key of this.pendingMcpApprovals.keys()) { + if (this.belongsToThread(key, threadId)) { + this.pendingMcpApprovals.delete(key); + } + } + } + + private key(threadId: string, serverName: string): string { + return `${threadId}:${serverName}`; + } + + private belongsToThread(key: string, threadId: string): boolean { + return key.startsWith(`${threadId}:`); + } } diff --git a/src/CodexEventHandler.ts b/src/CodexEventHandler.ts index 84d8a3ec..833b16a5 100644 --- a/src/CodexEventHandler.ts +++ b/src/CodexEventHandler.ts @@ -34,7 +34,6 @@ import { fuzzyFileSearchToolCallId, } from "./CodexToolCallMapper"; import { stripShellPrefix } from "./CommandUtils"; -import type { PendingMcpApprovals } from "./PendingMcpApprovals"; export { stripShellPrefix }; @@ -44,12 +43,10 @@ export class CodexEventHandler { private readonly sessionState: SessionState; private failure: RequestError | null = null; private readonly activeFuzzyFileSearchSessions = new Set(); - private readonly pendingMcpApprovals: PendingMcpApprovals | undefined; - constructor(connection: acp.AgentSideConnection, sessionState: SessionState, pendingMcpApprovals?: PendingMcpApprovals) { + constructor(connection: acp.AgentSideConnection, sessionState: SessionState) { this.connection = connection; this.sessionState = sessionState; - this.pendingMcpApprovals = pendingMcpApprovals; } getFailure(): RequestError | null { @@ -109,9 +106,7 @@ export class CodexEventHandler { case "account/updated": case "fs/changed": case "mcpServer/startupStatus/updated": - return null; case "serverRequest/resolved": - this.pendingMcpApprovals?.clearThread(notification.params.threadId); return null; case "item/mcpToolCall/progress": return this.createMcpToolProgressEvent(notification.params); @@ -198,7 +193,6 @@ export class CodexEventHandler { case "commandExecution": return await createCommandExecutionUpdate(event.item); case "mcpToolCall": - this.pendingMcpApprovals?.record(event.threadId, event.item.server, event.item.id); return await createMcpToolCallUpdate(event.item); case "dynamicToolCall": return await createDynamicToolCallUpdate(event.item); @@ -228,7 +222,6 @@ export class CodexEventHandler { status: event.item.status === "completed" ? "completed" : "failed", } case "mcpToolCall": - this.pendingMcpApprovals?.pop(event.threadId, event.item.server); return { sessionUpdate: "tool_call_update", toolCallId: event.item.id, diff --git a/src/PendingMcpApprovals.ts b/src/PendingMcpApprovals.ts deleted file mode 100644 index 7f272e60..00000000 --- a/src/PendingMcpApprovals.ts +++ /dev/null @@ -1,43 +0,0 @@ -// In Rust, the MCP elicitation handler receives ElicitationRequestEvent directly from the MCP -// protocol layer, where id is set to "mcp_tool_call_approval_" — the call ID is extracted -// by stripping that prefix. -// -// In TypeScript, Codex speaks the app-server JSON-RPC protocol (v2), where McpServerElicitationRequestParams -// omits elicitationId for form mode, so the MCP-level ID never reaches the client. -// -// Workaround: before requesting approval, Codex emits an item/started notification with an mcpToolCall -// item carrying the call id and server name. This class stores (threadId, serverName) → callId so the -// elicitation handler can retrieve it when the request arrives. -// -// Multiple calls are safe because Codex requests approval synchronously — it blocks on one tool call's -// elicitation before starting the next, so there is at most one pending approval per (threadId, serverName). -export class PendingMcpApprovals { - private readonly pending = new Map(); - - record(threadId: string, serverName: string, callId: string): void { - this.pending.set(this.key(threadId, serverName), callId); - } - - pop(threadId: string, serverName: string): string | undefined { - const key = this.key(threadId, serverName); - const callId = this.pending.get(key); - this.pending.delete(key); - return callId; - } - - clearThread(threadId: string): void { - for (const key of this.pending.keys()) { - if (this.belongsToThread(key, threadId)) { - this.pending.delete(key); - } - } - } - - private key(threadId: string, serverName: string): string { - return `${threadId}:${serverName}`; - } - - private belongsToThread(key: string, threadId: string): boolean { - return key.startsWith(`${threadId}:`); - } -} From d655361119435265cfafdd760dfdfd6e5278483f Mon Sep 17 00:00:00 2001 From: "Nikolai.Sviridov" Date: Tue, 21 Apr 2026 00:15:46 +0200 Subject: [PATCH 8/9] fix: add is_mcp_tool_approval marker --- src/CodexElicitationHandler.ts | 2 ++ .../data/elicitation-tool-approval-all-persist.json | 1 + .../data/elicitation-tool-approval-no-persist.json | 1 + .../data/elicitation-tool-approval-session-only.json | 1 + 4 files changed, 5 insertions(+) diff --git a/src/CodexElicitationHandler.ts b/src/CodexElicitationHandler.ts index 1b9b9d96..b536be60 100644 --- a/src/CodexElicitationHandler.ts +++ b/src/CodexElicitationHandler.ts @@ -165,6 +165,7 @@ export class CodexElicitationHandler implements ElicitationHandler { // content: [messageContent], — omitted: already rendered via item/started // rawInput: { ... } — omitted: same reason }, + _meta: { is_mcp_tool_approval: true }, options, }, correlatedCallId, @@ -180,6 +181,7 @@ export class CodexElicitationHandler implements ElicitationHandler { content: [messageContent], rawInput: { serverName: params.serverName, schema: params.requestedSchema }, }, + ...(isToolApproval ? { _meta: { is_mcp_tool_approval: true } } : {}), options, }, correlatedCallId: undefined, 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 61e97fc8..d2f40807 100644 --- a/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-all-persist.json +++ b/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-all-persist.json @@ -24,6 +24,7 @@ } } }, + "_meta": "_meta", "options": [ { "optionId": "allow_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 c5aff8f2..b212b996 100644 --- a/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-no-persist.json +++ b/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-no-persist.json @@ -24,6 +24,7 @@ } } }, + "_meta": "_meta", "options": [ { "optionId": "allow_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 dd19852c..50358bee 100644 --- a/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-session-only.json +++ b/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-session-only.json @@ -24,6 +24,7 @@ } } }, + "_meta": "_meta", "options": [ { "optionId": "allow_once", From 184954f0b6955669aa82f1900b63eef0627a9895 Mon Sep 17 00:00:00 2001 From: "Nikolai.Sviridov" Date: Tue, 21 Apr 2026 00:17:14 +0200 Subject: [PATCH 9/9] chore: document double-pop cleanup for MCP approval correlation --- src/CodexElicitationHandler.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/CodexElicitationHandler.ts b/src/CodexElicitationHandler.ts index b536be60..1b90bcf2 100644 --- a/src/CodexElicitationHandler.ts +++ b/src/CodexElicitationHandler.ts @@ -235,6 +235,9 @@ export class CodexElicitationHandler implements ElicitationHandler { if (event.item.type !== "mcpToolCall") { return; } + // This may run after the elicitation path already consumed the same entry. + // That double-pop is intentional: approvals pop on request correlation, while + // auto-approved or interrupted calls need completion-side cleanup. this.popPendingApproval(event.threadId, event.item.server); }