Skip to content

Commit a971527

Browse files
ishulginishulgin
committed
Fix ApprovalOptionId mixed with mcp approval options
1 parent 97b01f0 commit a971527

4 files changed

Lines changed: 38 additions & 26 deletions

File tree

‎src/CodexElicitationHandler.ts‎

Lines changed: 9 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -9,17 +9,14 @@ import type {
99
McpServerElicitationRequestResponse,
1010
} from "./app-server/v2";
1111
import { logger } from "./Logger";
12-
import { ApprovalOptionId } from "./ApprovalOptionId";
12+
import { McpApprovalOptionId } from "./McpApprovalOptionId";
1313

1414
// Standard elicitation options (non-tool-call approval).
1515
const ELICITATION_OPTIONS: acp.PermissionOption[] = [
1616
{ optionId: "accept", name: "Accept", kind: "allow_once" },
1717
{ optionId: "decline", name: "Decline", kind: "reject_once" },
1818
];
1919

20-
// Option ID unique to elicitation persist choices — not part of the shared ApprovalOptionId set.
21-
const OPTION_ALLOW_SESSION = "allow_session";
22-
2320
type PersistValue = "session" | "always";
2421

2522
/**
@@ -56,15 +53,15 @@ function isMcpToolCallApproval(meta: unknown): boolean {
5653
*/
5754
function buildToolApprovalOptions(persistOptions: Set<PersistValue>): acp.PermissionOption[] {
5855
const options: acp.PermissionOption[] = [
59-
{ optionId: ApprovalOptionId.AllowOnce, name: "Allow", kind: "allow_once" },
56+
{ optionId: McpApprovalOptionId.AllowOnce, name: "Allow", kind: "allow_once" },
6057
];
6158
if (persistOptions.has("session")) {
62-
options.push({ optionId: OPTION_ALLOW_SESSION, name: "Allow for This Session", kind: "allow_always" });
59+
options.push({ optionId: McpApprovalOptionId.AllowSession, name: "Allow for This Session", kind: "allow_always" });
6360
}
6461
if (persistOptions.has("always")) {
65-
options.push({ optionId: ApprovalOptionId.AllowAlways, name: "Allow and Don't Ask Again", kind: "allow_always" });
62+
options.push({ optionId: McpApprovalOptionId.AllowAlways, name: "Allow and Don't Ask Again", kind: "allow_always" });
6663
}
67-
options.push({ optionId: "decline", name: "Decline", kind: "reject_once" });
64+
options.push({ optionId: McpApprovalOptionId.Decline, name: "Decline", kind: "reject_once" });
6865
return options;
6966
}
7067

@@ -117,7 +114,7 @@ export class CodexElicitationHandler implements ElicitationHandler {
117114
const response = await this.connection.requestPermission(request);
118115
if (correlatedCallId !== undefined && response.outcome.outcome !== "cancelled") {
119116
const optionId = response.outcome.optionId;
120-
if (optionId !== "decline") {
117+
if (optionId !== McpApprovalOptionId.Decline) {
121118
await this.connection.sessionUpdate({
122119
sessionId: this.sessionState.sessionId,
123120
update: { sessionUpdate: "tool_call_update", toolCallId: correlatedCallId, status: "in_progress" },
@@ -211,13 +208,13 @@ export class CodexElicitationHandler implements ElicitationHandler {
211208
}
212209

213210
const optionId = response.outcome.optionId;
214-
if (optionId === OPTION_ALLOW_SESSION) {
211+
if (optionId === McpApprovalOptionId.AllowSession) {
215212
return { action: "accept", content: null, _meta: { persist: "session" } };
216213
}
217-
if (optionId === ApprovalOptionId.AllowAlways) {
214+
if (optionId === McpApprovalOptionId.AllowAlways) {
218215
return { action: "accept", content: null, _meta: { persist: "always" } };
219216
}
220-
if (optionId === ApprovalOptionId.AllowOnce || optionId === "accept") {
217+
if (optionId === McpApprovalOptionId.AllowOnce || optionId === "accept") {
221218
return { action: "accept", content: null, _meta: null };
222219
}
223220
return { action: "decline", content: null, _meta: null };

‎src/McpApprovalOptionId.ts‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
export const McpApprovalOptionId = {
2+
AllowOnce: "allow_once",
3+
AllowSession: "allow_session",
4+
AllowAlways: "allow_always",
5+
Decline: "decline",
6+
} as const;
7+
8+
export type McpApprovalOptionId = typeof McpApprovalOptionId[keyof typeof McpApprovalOptionId];

‎src/__tests__/CodexACPAgent/e2e/acp-e2e-mcp-approval.test.ts‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,9 @@ import type * as acp from "@agentclientprotocol/sdk";
22
import fs from "node:fs";
33
import path from "node:path";
44
import {afterEach, beforeEach, expect, it} from "vitest";
5-
import {ApprovalOptionId} from "../../../ApprovalOptionId";
5+
import {McpApprovalOptionId, type McpApprovalOptionId as McpApprovalOptionIdValue} from "../../../McpApprovalOptionId";
66
import {
77
createAuthenticatedFixture,
8-
createPermissionResponse,
98
describeE2E,
109
expectEndTurn,
1110
type PermissionResponder,
@@ -31,8 +30,15 @@ function isMcpPermissionRequest(request: acp.RequestPermissionRequest): boolean
3130
return request.toolCall.kind === "execute" && request._meta?.["is_mcp_tool_approval"] === true;
3231
}
3332

34-
function createMcpPermissionResponder(optionId: ApprovalOptionId): PermissionResponder {
35-
return (request) => createPermissionResponse(isMcpPermissionRequest(request) ? optionId : null);
33+
function createMcpPermissionResponse(optionId: McpApprovalOptionIdValue | null): acp.RequestPermissionResponse {
34+
if (optionId === null) {
35+
return {outcome: {outcome: "cancelled"}};
36+
}
37+
return {outcome: {outcome: "selected", optionId}};
38+
}
39+
40+
function createMcpPermissionResponder(optionId: McpApprovalOptionIdValue): PermissionResponder {
41+
return (request) => createMcpPermissionResponse(isMcpPermissionRequest(request) ? optionId : null);
3642
}
3743

3844
describeE2E("E2E MCP approval tests", () => {
@@ -53,7 +59,7 @@ describeE2E("E2E MCP approval tests", () => {
5359
}
5460

5561
it("executes an approved MCP tool call", async () => {
56-
fixture.setPermissionResponder(createMcpPermissionResponder(ApprovalOptionId.AllowOnce));
62+
fixture.setPermissionResponder(createMcpPermissionResponder(McpApprovalOptionId.AllowOnce));
5763
const invocationMarkerPath = path.join(fixture.workspaceDir, `mcp-tool-invocation-${crypto.randomUUID()}.txt`);
5864
const sessionId = (await fixture.createSession([createMcpServer(invocationMarkerPath)])).sessionId;
5965

@@ -67,7 +73,7 @@ describeE2E("E2E MCP approval tests", () => {
6773
});
6874

6975
it("ends turn when MCP tool call is rejected", async () => {
70-
fixture.setPermissionResponder(createMcpPermissionResponder(ApprovalOptionId.RejectOnce));
76+
fixture.setPermissionResponder(createMcpPermissionResponder(McpApprovalOptionId.Decline));
7177
const invocationMarkerPath = path.join(fixture.workspaceDir, `mcp-tool-invocation-${crypto.randomUUID()}.txt`);
7278
const sessionId = (await fixture.createSession([createMcpServer(invocationMarkerPath)])).sessionId;
7379

‎src/__tests__/CodexACPAgent/elicitation-events.test.ts‎

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import type { McpServerElicitationRequestParams } from '../../app-server/v2';
33
import { createCodexMockTestFixture, createTestSessionState, type CodexMockTestFixture } from '../acp-test-utils';
44
import type { SessionState } from '../../CodexAcpServer';
55
import { AgentMode } from "../../AgentMode";
6+
import { McpApprovalOptionId } from "../../McpApprovalOptionId";
67
import type { ServerNotification } from "../../app-server";
78

89
describe('Elicitation Events', () => {
@@ -132,7 +133,7 @@ describe('Elicitation Events', () => {
132133
describe('MCP tool call approval elicitation', () => {
133134
it('should show Allow/session/always/Decline options when all persist values advertised', async () => {
134135
const { promptPromise, completeTurn } = setupSessionWithPendingPrompt();
135-
fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_once' } });
136+
fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: McpApprovalOptionId.AllowOnce } });
136137

137138
const params: McpServerElicitationRequestParams = {
138139
threadId: sessionId, turnId: 'turn-1', serverName: 'tool-server',
@@ -151,7 +152,7 @@ describe('Elicitation Events', () => {
151152

152153
it('should map allow_once to accept with null meta', async () => {
153154
const { promptPromise, completeTurn } = setupSessionWithPendingPrompt();
154-
fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_once' } });
155+
fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: McpApprovalOptionId.AllowOnce } });
155156

156157
const params: McpServerElicitationRequestParams = {
157158
threadId: sessionId, turnId: 'turn-1', serverName: 'tool-server',
@@ -170,7 +171,7 @@ describe('Elicitation Events', () => {
170171

171172
it('should map allow_session to accept with persist:session meta', async () => {
172173
const { promptPromise, completeTurn } = setupSessionWithPendingPrompt();
173-
fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_session' } });
174+
fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: McpApprovalOptionId.AllowSession } });
174175

175176
const params: McpServerElicitationRequestParams = {
176177
threadId: sessionId, turnId: 'turn-1', serverName: 'tool-server',
@@ -189,7 +190,7 @@ describe('Elicitation Events', () => {
189190

190191
it('should map allow_always to accept with persist:always meta', async () => {
191192
const { promptPromise, completeTurn } = setupSessionWithPendingPrompt();
192-
fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_always' } });
193+
fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: McpApprovalOptionId.AllowAlways } });
193194

194195
const params: McpServerElicitationRequestParams = {
195196
threadId: sessionId, turnId: 'turn-1', serverName: 'tool-server',
@@ -208,7 +209,7 @@ describe('Elicitation Events', () => {
208209

209210
it('should only show session option when persist is "session"', async () => {
210211
const { promptPromise, completeTurn } = setupSessionWithPendingPrompt();
211-
fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_once' } });
212+
fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: McpApprovalOptionId.AllowOnce } });
212213

213214
const params: McpServerElicitationRequestParams = {
214215
threadId: sessionId, turnId: 'turn-1', serverName: 'tool-server',
@@ -227,7 +228,7 @@ describe('Elicitation Events', () => {
227228

228229
it('should show only Allow and Decline when no persist options', async () => {
229230
const { promptPromise, completeTurn } = setupSessionWithPendingPrompt();
230-
fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_once' } });
231+
fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: McpApprovalOptionId.AllowOnce } });
231232

232233
const params: McpServerElicitationRequestParams = {
233234
threadId: sessionId, turnId: 'turn-1', serverName: 'tool-server',
@@ -246,7 +247,7 @@ describe('Elicitation Events', () => {
246247

247248
it('should not reuse a completed auto-approved call id for a later approval request', async () => {
248249
const { promptPromise, completeTurn } = setupSessionWithPendingPrompt();
249-
fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_once' } });
250+
fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: McpApprovalOptionId.AllowOnce } });
250251

251252
const startedNotification: ServerNotification = {
252253
method: 'item/started',
@@ -309,7 +310,7 @@ describe('Elicitation Events', () => {
309310

310311
it('should not reuse a stale call id after serverRequest/resolved clears interrupted approval state', async () => {
311312
const { promptPromise, completeTurn } = setupSessionWithPendingPrompt();
312-
fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_once' } });
313+
fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: McpApprovalOptionId.AllowOnce } });
313314

314315
const startedNotification: ServerNotification = {
315316
method: 'item/started',

0 commit comments

Comments
 (0)