Skip to content

Commit 2798159

Browse files
LLM-27035 [Codex] Add missing elicitation, LLM-27036 [Codex] Improve mcp block detection (#100)
* LLM-27035 [Codex] Add missing elicitation * LLM-27036 [Codex] Improve mcp block detection * fix: update mcp tool on approval * fix: update test snapshots * fix: pop() stale entries mcpToolCall * fix: cleanup on on serverRequest/resolved * fix: move pendingMcpApprovals to src/CodexElicitationHandler.ts * fix: add is_mcp_tool_approval marker * chore: document double-pop cleanup for MCP approval correlation
1 parent 2ba0bc1 commit 2798159

16 files changed

Lines changed: 969 additions & 11 deletions

src/CodexAcpClient.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import {isCodexAuthRequest} from "./CodexAuthMethod";
22
import type {EmbeddedResourceResource} from "@agentclientprotocol/sdk";
33
import * as acp from "@agentclientprotocol/sdk";
44
import {type McpServer, RequestError} from "@agentclientprotocol/sdk";
5-
import type {ApprovalHandler, CodexAppServerClient} from "./CodexAppServerClient";
5+
import type {ApprovalHandler, CodexAppServerClient, ElicitationHandler} from "./CodexAppServerClient";
66
import open from "open";
77
import type {Disposable} from "vscode-jsonrpc";
88
import type {
@@ -360,10 +360,12 @@ export class CodexAcpClient {
360360
async subscribeToSessionEvents(
361361
sessionId: string,
362362
eventHandler: (result: ServerNotification) => void,
363-
approvalHandler: ApprovalHandler
363+
approvalHandler: ApprovalHandler,
364+
elicitationHandler: ElicitationHandler
364365
) {
365366
this.codexClient.onServerNotification(sessionId, eventHandler);
366367
this.codexClient.onApprovalRequest(sessionId, approvalHandler);
368+
this.codexClient.onElicitationRequest(sessionId, elicitationHandler);
367369
}
368370

369371
async sendPrompt(

src/CodexAcpServer.ts

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import {
77
} from "@agentclientprotocol/sdk";
88
import {CodexEventHandler} from "./CodexEventHandler";
99
import {CodexApprovalHandler} from "./CodexApprovalHandler";
10+
import {CodexElicitationHandler} from "./CodexElicitationHandler";
1011
import {CodexAuthMethods, type CodexAuthRequest} from "./CodexAuthMethod";
1112
import {CodexAcpClient, type SessionMetadata, type SessionMetadataWithThread} from "./CodexAcpClient";
1213
import {ACPSessionConnection, type UpdateSessionEvent} from "./ACPSessionConnection";
@@ -707,9 +708,14 @@ export class CodexAcpServer implements acp.Agent {
707708
try {
708709
const eventHandler = new CodexEventHandler(this.connection, sessionState);
709710
const approvalHandler = new CodexApprovalHandler(this.connection, sessionState);
711+
const elicitationHandler = new CodexElicitationHandler(this.connection, sessionState);
710712
await this.codexAcpClient.subscribeToSessionEvents(params.sessionId,
711-
(event) => eventHandler.handleNotification(event),
712-
approvalHandler);
713+
(event) => {
714+
elicitationHandler.handleNotification(event);
715+
return eventHandler.handleNotification(event);
716+
},
717+
approvalHandler,
718+
elicitationHandler);
713719

714720
if (await this.availableCommands.tryHandle(params.prompt, sessionState)) {
715721
logger.log("Prompt handled by a command");

src/CodexAppServerClient.ts

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,8 @@ import type {
2828
CommandExecutionRequestApprovalResponse,
2929
FileChangeRequestApprovalParams,
3030
FileChangeRequestApprovalResponse,
31+
McpServerElicitationRequestParams,
32+
McpServerElicitationRequestResponse,
3133
ThreadResumeParams,
3234
ThreadResumeResponse,
3335
SkillsListParams,
@@ -41,6 +43,10 @@ export interface ApprovalHandler {
4143
handleFileChange(params: FileChangeRequestApprovalParams): Promise<FileChangeRequestApprovalResponse>;
4244
}
4345

46+
export interface ElicitationHandler {
47+
handleElicitation(params: McpServerElicitationRequestParams): Promise<McpServerElicitationRequestResponse>;
48+
}
49+
4450
const CommandExecutionApprovalRequest = new RequestType<
4551
CommandExecutionRequestApprovalParams,
4652
CommandExecutionRequestApprovalResponse,
@@ -53,13 +59,20 @@ const FileChangeApprovalRequest = new RequestType<
5359
void
5460
>('item/fileChange/requestApproval');
5561

62+
const McpServerElicitationRequest = new RequestType<
63+
McpServerElicitationRequestParams,
64+
McpServerElicitationRequestResponse,
65+
void
66+
>('mcpServer/elicitation/request');
67+
5668
/**
5769
* A type-safe client over the Codex App Server's JSON-RPC API.
5870
* Maps each request to its expected response and exposes clear, typed methods for supported JSON-RPC operations.
5971
*/
6072
export class CodexAppServerClient {
6173
readonly connection: MessageConnection;
6274
private approvalHandlers = new Map<string, ApprovalHandler>();
75+
private elicitationHandlers = new Map<string, ElicitationHandler>();
6376
private mcpStartupCompleteVersion = 0;
6477
private lastMcpStartupComplete: McpStartupCompleteEvent | null = null;
6578
private readonly mcpStartupCompleteResolvers: Array<SignalResolver<McpStartupCompleteEvent>> = [];
@@ -98,12 +111,24 @@ export class CodexAppServerClient {
98111
}
99112
return await handler.handleFileChange(params);
100113
});
114+
115+
this.connection.onRequest(McpServerElicitationRequest, async (params) => {
116+
const handler = this.elicitationHandlers.get(params.threadId);
117+
if (!handler) {
118+
return { action: "cancel", content: null, _meta: null };
119+
}
120+
return await handler.handleElicitation(params);
121+
});
101122
}
102123

103124
onApprovalRequest(threadId: string, handler: ApprovalHandler): void {
104125
this.approvalHandlers.set(threadId, handler);
105126
}
106127

128+
onElicitationRequest(threadId: string, handler: ElicitationHandler): void {
129+
this.elicitationHandlers.set(threadId, handler);
130+
}
131+
107132
async initialize(params: InitializeParams): Promise<InitializeResponse> {
108133
return await this.sendRequest({ method: "initialize", params: params });
109134
}

src/CodexElicitationHandler.ts

Lines changed: 266 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,266 @@
1+
import * as acp from "@agentclientprotocol/sdk";
2+
import type { SessionState } from "./CodexAcpServer";
3+
import type { ElicitationHandler } from "./CodexAppServerClient";
4+
import type { ServerNotification } from "./app-server";
5+
import type {
6+
ItemCompletedNotification,
7+
ItemStartedNotification,
8+
McpServerElicitationRequestParams,
9+
McpServerElicitationRequestResponse,
10+
} from "./app-server/v2";
11+
import { logger } from "./Logger";
12+
13+
// Standard elicitation options (non-tool-call approval).
14+
const ELICITATION_OPTIONS: acp.PermissionOption[] = [
15+
{ optionId: "accept", name: "Accept", kind: "allow_once" },
16+
{ optionId: "decline", name: "Decline", kind: "reject_once" },
17+
];
18+
19+
// Option IDs used for MCP tool call approval persist choices.
20+
const OPTION_ALLOW_ONCE = "allow_once";
21+
const OPTION_ALLOW_SESSION = "allow_session";
22+
const OPTION_ALLOW_ALWAYS = "allow_always";
23+
24+
type PersistValue = "session" | "always";
25+
26+
/**
27+
* Parses the `persist` field from the elicitation request `_meta`.
28+
* Codex advertises which persistence options the client should show.
29+
* Returns a set of supported persist values.
30+
*/
31+
function parsePersistOptions(meta: unknown): Set<PersistValue> {
32+
const result = new Set<PersistValue>();
33+
if (!meta || typeof meta !== "object") return result;
34+
const persist = (meta as Record<string, unknown>)["persist"];
35+
if (persist === "session") {
36+
result.add("session");
37+
} else if (persist === "always") {
38+
result.add("always");
39+
} else if (Array.isArray(persist)) {
40+
if (persist.includes("session")) result.add("session");
41+
if (persist.includes("always")) result.add("always");
42+
}
43+
return result;
44+
}
45+
46+
function isMcpToolCallApproval(meta: unknown): boolean {
47+
return (
48+
meta !== null &&
49+
typeof meta === "object" &&
50+
(meta as Record<string, unknown>)["codex_approval_kind"] === "mcp_tool_call"
51+
);
52+
}
53+
54+
/**
55+
* Builds the ACP permission options for an MCP tool call approval elicitation.
56+
* Always includes "Allow Once"; adds session/always persist options when advertised.
57+
*/
58+
function buildToolApprovalOptions(persistOptions: Set<PersistValue>): acp.PermissionOption[] {
59+
const options: acp.PermissionOption[] = [
60+
{ optionId: OPTION_ALLOW_ONCE, name: "Allow", kind: "allow_once" },
61+
];
62+
if (persistOptions.has("session")) {
63+
options.push({ optionId: OPTION_ALLOW_SESSION, name: "Allow for This Session", kind: "allow_always" });
64+
}
65+
if (persistOptions.has("always")) {
66+
options.push({ optionId: OPTION_ALLOW_ALWAYS, name: "Allow and Don't Ask Again", kind: "allow_always" });
67+
}
68+
options.push({ optionId: "decline", name: "Decline", kind: "reject_once" });
69+
return options;
70+
}
71+
72+
export class CodexElicitationHandler implements ElicitationHandler {
73+
private readonly connection: acp.AgentSideConnection;
74+
private readonly sessionState: SessionState;
75+
// In Rust, the MCP elicitation handler receives ElicitationRequestEvent directly from the MCP
76+
// protocol layer, where id is set to "mcp_tool_call_approval_<call_id>" — the call ID is extracted
77+
// by stripping that prefix.
78+
//
79+
// In TypeScript, Codex speaks the app-server JSON-RPC protocol (v2), where
80+
// McpServerElicitationRequestParams omits elicitationId for form mode, so the MCP-level ID never
81+
// reaches the client.
82+
//
83+
// Workaround: before requesting approval, Codex emits an item/started notification with an
84+
// mcpToolCall item carrying the call id and server name. We store (threadId, serverName) → callId
85+
// here so the elicitation request can correlate back to the already-rendered tool call item.
86+
//
87+
// Multiple calls are safe because Codex requests approval synchronously — it blocks on one tool
88+
// call's elicitation before starting the next, so there is at most one pending approval per
89+
// (threadId, serverName).
90+
private readonly pendingMcpApprovals = new Map<string, string>();
91+
92+
constructor(connection: acp.AgentSideConnection, sessionState: SessionState) {
93+
this.connection = connection;
94+
this.sessionState = sessionState;
95+
}
96+
97+
handleNotification(notification: ServerNotification): void {
98+
switch (notification.method) {
99+
case "item/started":
100+
this.handleItemStarted(notification.params);
101+
return;
102+
case "item/completed":
103+
this.handleItemCompleted(notification.params);
104+
return;
105+
case "serverRequest/resolved":
106+
this.clearThread(notification.params.threadId);
107+
return;
108+
default:
109+
return;
110+
}
111+
}
112+
113+
async handleElicitation(
114+
params: McpServerElicitationRequestParams
115+
): Promise<McpServerElicitationRequestResponse> {
116+
try {
117+
const { request, correlatedCallId } = this.buildPermissionRequest(params);
118+
const response = await this.connection.requestPermission(request);
119+
if (correlatedCallId !== undefined && response.outcome.outcome !== "cancelled") {
120+
const optionId = response.outcome.optionId;
121+
if (optionId !== "decline") {
122+
await this.connection.sessionUpdate({
123+
sessionId: this.sessionState.sessionId,
124+
update: { sessionUpdate: "tool_call_update", toolCallId: correlatedCallId, status: "in_progress" },
125+
});
126+
}
127+
}
128+
return this.convertResponse(response);
129+
} catch (error) {
130+
logger.error("Error handling MCP elicitation request", error);
131+
return { action: "cancel", content: null, _meta: null };
132+
}
133+
}
134+
135+
private buildPermissionRequest(
136+
params: McpServerElicitationRequestParams
137+
): { request: acp.RequestPermissionRequest; correlatedCallId: string | undefined } {
138+
const sessionId = this.sessionState.sessionId;
139+
const messageContent: acp.ToolCallContent = {
140+
type: "content",
141+
content: { type: "text", text: params.message },
142+
};
143+
144+
const meta = params._meta;
145+
const isToolApproval = isMcpToolCallApproval(meta);
146+
const options = isToolApproval
147+
? buildToolApprovalOptions(parsePersistOptions(meta))
148+
: ELICITATION_OPTIONS;
149+
150+
if (params.mode === "form") {
151+
const correlatedCallId = isToolApproval
152+
? this.popPendingApproval(params.threadId, params.serverName)
153+
: undefined;
154+
if (correlatedCallId !== undefined) {
155+
// The tool call item is already visible in the IDE conversation history because
156+
// item/started was emitted before the elicitation request. Sending content or
157+
// rawInput here would duplicate that information in the approval widget.
158+
return {
159+
request: {
160+
sessionId,
161+
toolCall: {
162+
toolCallId: correlatedCallId,
163+
kind: "execute",
164+
status: "pending",
165+
// content: [messageContent], — omitted: already rendered via item/started
166+
// rawInput: { ... } — omitted: same reason
167+
},
168+
_meta: { is_mcp_tool_approval: true },
169+
options,
170+
},
171+
correlatedCallId,
172+
};
173+
}
174+
return {
175+
request: {
176+
sessionId,
177+
toolCall: {
178+
toolCallId: `elicitation-${params.serverName}`,
179+
kind: isToolApproval ? "execute" : "other",
180+
status: "pending",
181+
content: [messageContent],
182+
rawInput: { serverName: params.serverName, schema: params.requestedSchema },
183+
},
184+
...(isToolApproval ? { _meta: { is_mcp_tool_approval: true } } : {}),
185+
options,
186+
},
187+
correlatedCallId: undefined,
188+
};
189+
} else {
190+
return {
191+
request: {
192+
sessionId,
193+
toolCall: {
194+
toolCallId: `elicitation-${params.elicitationId}`,
195+
kind: "fetch",
196+
status: "pending",
197+
content: [messageContent],
198+
rawInput: { serverName: params.serverName, url: params.url },
199+
},
200+
options,
201+
},
202+
correlatedCallId: undefined,
203+
};
204+
}
205+
}
206+
207+
private convertResponse(
208+
response: acp.RequestPermissionResponse
209+
): McpServerElicitationRequestResponse {
210+
if (response.outcome.outcome === "cancelled") {
211+
return { action: "cancel", content: null, _meta: null };
212+
}
213+
214+
const optionId = response.outcome.optionId;
215+
if (optionId === OPTION_ALLOW_SESSION) {
216+
return { action: "accept", content: null, _meta: { persist: "session" } };
217+
}
218+
if (optionId === OPTION_ALLOW_ALWAYS) {
219+
return { action: "accept", content: null, _meta: { persist: "always" } };
220+
}
221+
if (optionId === OPTION_ALLOW_ONCE || optionId === "accept") {
222+
return { action: "accept", content: null, _meta: null };
223+
}
224+
return { action: "decline", content: null, _meta: null };
225+
}
226+
227+
private handleItemStarted(event: ItemStartedNotification): void {
228+
if (event.item.type !== "mcpToolCall") {
229+
return;
230+
}
231+
this.pendingMcpApprovals.set(this.key(event.threadId, event.item.server), event.item.id);
232+
}
233+
234+
private handleItemCompleted(event: ItemCompletedNotification): void {
235+
if (event.item.type !== "mcpToolCall") {
236+
return;
237+
}
238+
// This may run after the elicitation path already consumed the same entry.
239+
// That double-pop is intentional: approvals pop on request correlation, while
240+
// auto-approved or interrupted calls need completion-side cleanup.
241+
this.popPendingApproval(event.threadId, event.item.server);
242+
}
243+
244+
private popPendingApproval(threadId: string, serverName: string): string | undefined {
245+
const key = this.key(threadId, serverName);
246+
const callId = this.pendingMcpApprovals.get(key);
247+
this.pendingMcpApprovals.delete(key);
248+
return callId;
249+
}
250+
251+
private clearThread(threadId: string): void {
252+
for (const key of this.pendingMcpApprovals.keys()) {
253+
if (this.belongsToThread(key, threadId)) {
254+
this.pendingMcpApprovals.delete(key);
255+
}
256+
}
257+
}
258+
259+
private key(threadId: string, serverName: string): string {
260+
return `${threadId}:${serverName}`;
261+
}
262+
263+
private belongsToThread(key: string, threadId: string): boolean {
264+
return key.startsWith(`${threadId}:`);
265+
}
266+
}

src/CodexEventHandler.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -103,10 +103,10 @@ export class CodexEventHandler {
103103
case "turn/diff/updated":
104104
case "item/commandExecution/terminalInteraction":
105105
case "item/fileChange/outputDelta":
106-
case "serverRequest/resolved":
107106
case "account/updated":
108107
case "fs/changed":
109108
case "mcpServer/startupStatus/updated":
109+
case "serverRequest/resolved":
110110
return null;
111111
case "item/mcpToolCall/progress":
112112
return this.createMcpToolProgressEvent(notification.params);

0 commit comments

Comments
 (0)