diff --git a/src/CodexElicitationHandler.ts b/src/CodexElicitationHandler.ts index 732822b..68a4994 100644 --- a/src/CodexElicitationHandler.ts +++ b/src/CodexElicitationHandler.ts @@ -9,7 +9,7 @@ import type { McpServerElicitationRequestResponse, } from "./app-server/v2"; import { logger } from "./Logger"; -import { ApprovalOptionId } from "./ApprovalOptionId"; +import { McpApprovalOptionId } from "./McpApprovalOptionId"; // Standard elicitation options (non-tool-call approval). const ELICITATION_OPTIONS: acp.PermissionOption[] = [ @@ -17,9 +17,6 @@ const ELICITATION_OPTIONS: acp.PermissionOption[] = [ { optionId: "decline", name: "Decline", kind: "reject_once" }, ]; -// Option ID unique to elicitation persist choices — not part of the shared ApprovalOptionId set. -const OPTION_ALLOW_SESSION = "allow_session"; - type PersistValue = "session" | "always"; /** @@ -56,15 +53,15 @@ function isMcpToolCallApproval(meta: unknown): boolean { */ function buildToolApprovalOptions(persistOptions: Set): acp.PermissionOption[] { const options: acp.PermissionOption[] = [ - { optionId: ApprovalOptionId.AllowOnce, name: "Allow", kind: "allow_once" }, + { optionId: McpApprovalOptionId.AllowOnce, name: "Allow", kind: "allow_once" }, ]; if (persistOptions.has("session")) { - options.push({ optionId: OPTION_ALLOW_SESSION, name: "Allow for This Session", kind: "allow_always" }); + options.push({ optionId: McpApprovalOptionId.AllowSession, name: "Allow for This Session", kind: "allow_always" }); } if (persistOptions.has("always")) { - options.push({ optionId: ApprovalOptionId.AllowAlways, name: "Allow and Don't Ask Again", kind: "allow_always" }); + options.push({ optionId: McpApprovalOptionId.AllowAlways, name: "Allow and Don't Ask Again", kind: "allow_always" }); } - options.push({ optionId: "decline", name: "Decline", kind: "reject_once" }); + options.push({ optionId: McpApprovalOptionId.Decline, name: "Decline", kind: "reject_once" }); return options; } @@ -117,7 +114,7 @@ export class CodexElicitationHandler implements ElicitationHandler { const response = await this.connection.requestPermission(request); if (correlatedCallId !== undefined && response.outcome.outcome !== "cancelled") { const optionId = response.outcome.optionId; - if (optionId !== "decline") { + if (optionId !== McpApprovalOptionId.Decline) { await this.connection.sessionUpdate({ sessionId: this.sessionState.sessionId, update: { sessionUpdate: "tool_call_update", toolCallId: correlatedCallId, status: "in_progress" }, @@ -211,13 +208,13 @@ export class CodexElicitationHandler implements ElicitationHandler { } const optionId = response.outcome.optionId; - if (optionId === OPTION_ALLOW_SESSION) { + if (optionId === McpApprovalOptionId.AllowSession) { return { action: "accept", content: null, _meta: { persist: "session" } }; } - if (optionId === ApprovalOptionId.AllowAlways) { + if (optionId === McpApprovalOptionId.AllowAlways) { return { action: "accept", content: null, _meta: { persist: "always" } }; } - if (optionId === ApprovalOptionId.AllowOnce || optionId === "accept") { + if (optionId === McpApprovalOptionId.AllowOnce || optionId === "accept") { return { action: "accept", content: null, _meta: null }; } return { action: "decline", content: null, _meta: null }; diff --git a/src/McpApprovalOptionId.ts b/src/McpApprovalOptionId.ts new file mode 100644 index 0000000..3728145 --- /dev/null +++ b/src/McpApprovalOptionId.ts @@ -0,0 +1,8 @@ +export const McpApprovalOptionId = { + AllowOnce: "allow_once", + AllowSession: "allow_session", + AllowAlways: "allow_always", + Decline: "decline", +} as const; + +export type McpApprovalOptionId = typeof McpApprovalOptionId[keyof typeof McpApprovalOptionId]; diff --git a/src/__tests__/CodexACPAgent/e2e/acp-e2e-mcp-approval.test.ts b/src/__tests__/CodexACPAgent/e2e/acp-e2e-mcp-approval.test.ts index 42123a7..1481041 100644 --- a/src/__tests__/CodexACPAgent/e2e/acp-e2e-mcp-approval.test.ts +++ b/src/__tests__/CodexACPAgent/e2e/acp-e2e-mcp-approval.test.ts @@ -1,10 +1,10 @@ import type * as acp from "@agentclientprotocol/sdk"; +import fs from "node:fs"; import path from "node:path"; import {afterEach, beforeEach, expect, it} from "vitest"; -import {ApprovalOptionId} from "../../../ApprovalOptionId"; +import {McpApprovalOptionId, type McpApprovalOptionId as McpApprovalOptionIdValue} from "../../../McpApprovalOptionId"; import { createAuthenticatedFixture, - createPermissionResponse, describeE2E, expectEndTurn, type PermissionResponder, @@ -14,12 +14,15 @@ import { const MCP_SERVER_NAME = "integration-mcp"; const MCP_ECHO_MESSAGE = "mcp approval e2e"; -function createMcpServer(): acp.McpServerStdio { +function createMcpServer(invocationMarkerPath: string): acp.McpServerStdio { return { name: MCP_SERVER_NAME, command: process.execPath, - args: [path.join(process.cwd(), "node_modules/mcp-hello-world/build/stdio.js")], - env: [], + args: [path.join(process.cwd(), "src/__tests__/CodexACPAgent/e2e/fixtures/invocation-aware-mcp-server.mjs")], + env: [{ + name: "MCP_TOOL_INVOCATION_MARKER_PATH", + value: invocationMarkerPath, + }], }; } @@ -27,42 +30,67 @@ function isMcpPermissionRequest(request: acp.RequestPermissionRequest): boolean return request.toolCall.kind === "execute" && request._meta?.["is_mcp_tool_approval"] === true; } -function createMcpPermissionResponder(optionId: ApprovalOptionId): PermissionResponder { - return (request) => createPermissionResponse(isMcpPermissionRequest(request) ? optionId : null); +function createMcpPermissionResponse(optionId: McpApprovalOptionIdValue | null): acp.RequestPermissionResponse { + if (optionId === null) { + return {outcome: {outcome: "cancelled"}}; + } + return {outcome: {outcome: "selected", optionId}}; +} + +function createMcpPermissionResponder(...optionIds: McpApprovalOptionIdValue[]): PermissionResponder { + const queue = [...optionIds]; + return (request) => createMcpPermissionResponse( + isMcpPermissionRequest(request) + ? queue.shift() ?? McpApprovalOptionId.Decline + : null, + ); } describeE2E("E2E MCP approval tests", () => { let fixture: SpawnedAgentFixture; - let sessionId: string; beforeEach(async () => { fixture = await createAuthenticatedFixture(); - sessionId = (await fixture.createSession([createMcpServer()])).sessionId; }); afterEach(async () => { await fixture.dispose(); }); - function expectMcpToolPermissionRequest(): void { - const requests = fixture.readPermissionRequests(sessionId, "execute"); - expect(requests.length).toBe(1); - expect(isMcpPermissionRequest(requests[0]!)).toBe(true); + async function createMcpSession(): Promise<{ sessionId: string; invocationMarkerPath: string }> { + const invocationMarkerPath = path.join(fixture.workspaceDir, `mcp-tool-invocation-${crypto.randomUUID()}.txt`); + const sessionId = (await fixture.createSession([createMcpServer(invocationMarkerPath)])).sessionId; + return {sessionId, invocationMarkerPath}; } - it("executes an approved MCP tool call", async () => { - fixture.setPermissionResponder(createMcpPermissionResponder(ApprovalOptionId.AllowOnce)); - + async function expectEchoToolReply(sessionId: string, message: string): Promise { await fixture.expectPromptText( sessionId, - `Use the ${MCP_SERVER_NAME} MCP echo tool with message "${MCP_ECHO_MESSAGE}". Reply with exactly the tool result and no extra text.`, - (text) => expect(text).toContain(`You said: ${MCP_ECHO_MESSAGE}`), + `Use the ${MCP_SERVER_NAME} MCP echo tool with message "${message}". Reply with exactly the tool result and no extra text.`, + (text) => expect(text).toContain(`You said: ${message}`), ); - expectMcpToolPermissionRequest(); + } + + function expectMcpPermissionRequestCount(sessionId: string, count: number): void { + const requests = fixture.readPermissionRequests(sessionId, "execute"); + expect(requests.length).toBe(count); + for (const request of requests) { + expect(isMcpPermissionRequest(request)).toBe(true); + } + } + + it("executes an approved MCP tool call", async () => { + fixture.setPermissionResponder(createMcpPermissionResponder(McpApprovalOptionId.AllowOnce)); + const {sessionId, invocationMarkerPath} = await createMcpSession(); + + await expectEchoToolReply(sessionId, MCP_ECHO_MESSAGE); + expect(fs.readFileSync(invocationMarkerPath, "utf8")).toBe(MCP_ECHO_MESSAGE); + expectMcpPermissionRequestCount(sessionId, 1); }); it("ends turn when MCP tool call is rejected", async () => { - fixture.setPermissionResponder(createMcpPermissionResponder(ApprovalOptionId.RejectOnce)); + fixture.setPermissionResponder(createMcpPermissionResponder(McpApprovalOptionId.Decline)); + const {sessionId, invocationMarkerPath} = await createMcpSession(); expectEndTurn(await fixture.connection.prompt({ sessionId, @@ -71,6 +99,35 @@ describeE2E("E2E MCP approval tests", () => { text: `Use the ${MCP_SERVER_NAME} MCP echo tool with message "${MCP_ECHO_MESSAGE}". Stop if the tool call is rejected.`, }], })); - expectMcpToolPermissionRequest(); + expect(fs.existsSync(invocationMarkerPath)).toBe(false); + expectMcpPermissionRequestCount(sessionId, 1); + }); + + it("skips subsequent approvals in the same session when allow_session is selected", async () => { + fixture.setPermissionResponder( + createMcpPermissionResponder(McpApprovalOptionId.AllowSession, McpApprovalOptionId.Decline), + ); + const {sessionId, invocationMarkerPath} = await createMcpSession(); + + await expectEchoToolReply(sessionId, "session approval first"); + await expectEchoToolReply(sessionId, "session approval second"); + + expect(fs.readFileSync(invocationMarkerPath, "utf8")).toBe("session approval second"); + expectMcpPermissionRequestCount(sessionId, 1); + }); + + // This case only asserts observed same-session approval. + // codex-acp adapter produces persist metadata for MCP tool approvals, but its persistence behavior depends on upstream. + it("skips subsequent approvals in the same session when allow_always is selected", async () => { + fixture.setPermissionResponder( + createMcpPermissionResponder(McpApprovalOptionId.AllowAlways, McpApprovalOptionId.Decline), + ); + const {sessionId, invocationMarkerPath} = await createMcpSession(); + + await expectEchoToolReply(sessionId, "always approval first"); + await expectEchoToolReply(sessionId, "always approval second"); + + expect(fs.readFileSync(invocationMarkerPath, "utf8")).toBe("always approval second"); + expectMcpPermissionRequestCount(sessionId, 1); }); }); diff --git a/src/__tests__/CodexACPAgent/e2e/fixtures/invocation-aware-mcp-server.mjs b/src/__tests__/CodexACPAgent/e2e/fixtures/invocation-aware-mcp-server.mjs new file mode 100644 index 0000000..828986c --- /dev/null +++ b/src/__tests__/CodexACPAgent/e2e/fixtures/invocation-aware-mcp-server.mjs @@ -0,0 +1,32 @@ +import fs from "node:fs/promises"; +import {McpServer} from "@modelcontextprotocol/sdk/server/mcp.js"; +import {StdioServerTransport} from "@modelcontextprotocol/sdk/server/stdio.js"; +import {z} from "zod"; + +const markerPath = process.env["MCP_TOOL_INVOCATION_MARKER_PATH"]; + +if (!markerPath) { + throw new Error("MCP_TOOL_INVOCATION_MARKER_PATH is required."); +} + +const server = new McpServer({ + name: "integration-mcp", + version: "1.0.0", +}); + +server.tool( + "echo", + "Echoes back a message with a side effect for test assertions.", + {message: z.string().describe("The message to echo")}, + async ({message}) => { + await fs.writeFile(markerPath, message, "utf8"); + return { + content: [{ + type: "text", + text: `You said: ${message}`, + }], + }; + }, +); + +await server.connect(new StdioServerTransport()); diff --git a/src/__tests__/CodexACPAgent/elicitation-events.test.ts b/src/__tests__/CodexACPAgent/elicitation-events.test.ts index 32548be..82d14a0 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 { McpApprovalOptionId } from "../../McpApprovalOptionId"; import type { ServerNotification } from "../../app-server"; describe('Elicitation Events', () => { @@ -132,7 +133,7 @@ describe('Elicitation Events', () => { 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' } }); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: McpApprovalOptionId.AllowOnce } }); const params: McpServerElicitationRequestParams = { threadId: sessionId, turnId: 'turn-1', serverName: 'tool-server', @@ -151,7 +152,7 @@ describe('Elicitation Events', () => { it('should map allow_once to accept with null meta', async () => { const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); - fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_once' } }); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: McpApprovalOptionId.AllowOnce } }); const params: McpServerElicitationRequestParams = { threadId: sessionId, turnId: 'turn-1', serverName: 'tool-server', @@ -170,7 +171,7 @@ describe('Elicitation Events', () => { it('should map allow_session to accept with persist:session meta', async () => { const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); - fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_session' } }); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: McpApprovalOptionId.AllowSession } }); const params: McpServerElicitationRequestParams = { threadId: sessionId, turnId: 'turn-1', serverName: 'tool-server', @@ -189,7 +190,7 @@ describe('Elicitation Events', () => { it('should map allow_always to accept with persist:always meta', async () => { const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); - fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_always' } }); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: McpApprovalOptionId.AllowAlways } }); const params: McpServerElicitationRequestParams = { threadId: sessionId, turnId: 'turn-1', serverName: 'tool-server', @@ -208,7 +209,7 @@ describe('Elicitation Events', () => { it('should only show session option when persist is "session"', async () => { const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); - fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_once' } }); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: McpApprovalOptionId.AllowOnce } }); const params: McpServerElicitationRequestParams = { threadId: sessionId, turnId: 'turn-1', serverName: 'tool-server', @@ -227,7 +228,7 @@ describe('Elicitation Events', () => { it('should show only Allow and Decline when no persist options', async () => { const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); - fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_once' } }); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: McpApprovalOptionId.AllowOnce } }); const params: McpServerElicitationRequestParams = { threadId: sessionId, turnId: 'turn-1', serverName: 'tool-server', @@ -246,7 +247,7 @@ describe('Elicitation Events', () => { it('should not reuse a completed auto-approved call id for a later approval request', async () => { const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); - fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_once' } }); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: McpApprovalOptionId.AllowOnce } }); const startedNotification: ServerNotification = { method: 'item/started', @@ -309,7 +310,7 @@ describe('Elicitation Events', () => { it('should not reuse a stale call id after serverRequest/resolved clears interrupted approval state', async () => { const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); - fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_once' } }); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: McpApprovalOptionId.AllowOnce } }); const startedNotification: ServerNotification = { method: 'item/started',