diff --git a/src/ApprovalOptionId.ts b/src/ApprovalOptionId.ts index ccec90e..969aa42 100644 --- a/src/ApprovalOptionId.ts +++ b/src/ApprovalOptionId.ts @@ -1,6 +1,9 @@ export const ApprovalOptionId = { AllowOnce: "allow_once", - AllowAlways: "allow_always", + AllowForSession: "allow_for_session", + AllowPersist: "allow_persist", + AllowCommandPrefixRule: "allow_command_prefix_rule", + ApplyNetworkPolicyAmendment: "apply_network_policy_amendment", RejectOnce: "reject_once", } as const; diff --git a/src/CodexAcpClient.ts b/src/CodexAcpClient.ts index 22cde0d..4822020 100644 --- a/src/CodexAcpClient.ts +++ b/src/CodexAcpClient.ts @@ -201,9 +201,10 @@ export class CodexAcpClient { async resumeSession(request: acp.ResumeSessionRequest): Promise { await this.refreshSkills(request.cwd, request._meta); + const sessionConfig = await this.createSessionConfig(request.cwd, request.mcpServers ?? []); const response = await this.codexClient.threadResume({ - config: await this.createSessionConfig(request.cwd, request.mcpServers ?? []), + config: sessionConfig.config, cwd: request.cwd, modelProvider: this.getResumeModelProvider(), threadId: request.sessionId, @@ -215,12 +216,14 @@ export class CodexAcpClient { currentModelId: currentModelId, models: codexModels, currentServiceTier: response.serviceTier ?? null, + configBackedMcpServerNames: sessionConfig.configBackedMcpServerNames, } } async loadSession(request: acp.LoadSessionRequest): Promise { + const sessionConfig = await this.createSessionConfig(request.cwd, request.mcpServers ?? []); const response = await this.codexClient.threadResume({ - config: await this.createSessionConfig(request.cwd, request.mcpServers ?? []), + config: sessionConfig.config, cwd: request.cwd, modelProvider: this.getResumeModelProvider(), threadId: request.sessionId, @@ -232,15 +235,17 @@ export class CodexAcpClient { currentModelId: currentModelId, models: codexModels, currentServiceTier: response.serviceTier ?? null, + configBackedMcpServerNames: sessionConfig.configBackedMcpServerNames, thread: response.thread, }; } async newSession(request: acp.NewSessionRequest): Promise { await this.refreshSkills(request.cwd, request._meta); + const sessionConfig = await this.createSessionConfig(request.cwd, request.mcpServers); const response = await this.codexClient.threadStart({ - config: await this.createSessionConfig(request.cwd, request.mcpServers), + config: sessionConfig.config, modelProvider: this.getModelProvider(), cwd: request.cwd, }); @@ -255,6 +260,7 @@ export class CodexAcpClient { currentModelId: currentModelId, models: codexModels, currentServiceTier: response.serviceTier ?? null, + configBackedMcpServerNames: sessionConfig.configBackedMcpServerNames, }; } @@ -266,7 +272,7 @@ export class CodexAcpClient { return this.codexClient.getMcpServerStartupVersion(); } - private async createSessionConfig(projectPath: string, mcpServers: Array): Promise { + private async createSessionConfig(projectPath: string, mcpServers: Array): Promise { const mergedConfig = { ...mergeGatewayConfig(this.config, this.gatewayConfig), projects: { @@ -275,21 +281,30 @@ export class CodexAcpClient { } }, }; + const configuredMcpServerNames = await this.getConfigMcpServerNames(projectPath); if (mcpServers.length === 0) { - return mergedConfig; + return { + config: mergedConfig, + configBackedMcpServerNames: configuredMcpServerNames, + }; } // Deduplicates new servers against existing config to prevent Codex from deep-merging // incompatible field types (e.g., mixing url and stdio schemas). - const existingNames = await this.getConfigMcpServerNames(projectPath); - const uniqueServers = mcpServers.filter(mcp => !existingNames.has(mcp.name)); + const uniqueServers = mcpServers.filter(mcp => !configuredMcpServerNames.has(mcp.name)); if (uniqueServers.length === 0) { - return mergedConfig; + return { + config: mergedConfig, + configBackedMcpServerNames: configuredMcpServerNames, + }; } return { - ...mergedConfig, - "mcp_servers": Object.fromEntries(uniqueServers.map(mcp => [mcp.name, this.createMcpSeverConfig(mcp)])), + config: { + ...mergedConfig, + "mcp_servers": Object.fromEntries(uniqueServers.map(mcp => [mcp.name, this.createMcpSeverConfig(mcp)])), + }, + configBackedMcpServerNames: configuredMcpServerNames, }; } @@ -569,12 +584,18 @@ export type SessionMetadata = { currentModelId: string, models: Model[], currentServiceTier?: ServiceTier | null, + configBackedMcpServerNames?: Set, } export type SessionMetadataWithThread = SessionMetadata & { thread: Thread, } +type SessionConfig = { + config: JsonObject, + configBackedMcpServerNames: Set, +} + function buildPromptItems(prompt: acp.ContentBlock[]): UserInput[] { return prompt.map((block): UserInput | null => { switch (block.type) { diff --git a/src/CodexAcpServer.ts b/src/CodexAcpServer.ts index 0865c98..aef60aa 100644 --- a/src/CodexAcpServer.ts +++ b/src/CodexAcpServer.ts @@ -57,7 +57,8 @@ export interface SessionState { cwd: string; fastModeEnabled: boolean; currentModelSupportsFast: boolean; - sessionMcpServers?: Array; + sessionMcpServers: Array; + configBackedMcpServerNames?: Set; } interface PendingMcpStartupSession { @@ -185,7 +186,6 @@ export class CodexAcpServer implements acp.Agent { const account = await this.getActiveAccount(); const {sessionId, currentModelId, models} = sessionMetadata; - const sessionMcpServers = this.resolveSessionMcpServers(requestedMcpServers, "sessionId" in request); const currentModel = this.findCurrentModel(models, currentModelId); const currentModelSupportsFast = modelSupportsFast(currentModel); const sessionState: SessionState = { @@ -203,7 +203,8 @@ export class CodexAcpServer implements acp.Agent { cwd: request.cwd, fastModeEnabled: sessionMetadata.currentServiceTier === "fast", currentModelSupportsFast: currentModelSupportsFast, - sessionMcpServers: sessionMcpServers, + sessionMcpServers: this.createSessionMcpServers(requestedMcpServers, "sessionId" in request), + configBackedMcpServerNames: sessionMetadata.configBackedMcpServerNames ?? new Set(), } this.sessions.set(sessionId, sessionState); @@ -452,7 +453,6 @@ export class CodexAcpServer implements acp.Agent { const account = await this.getActiveAccount(); const {sessionId, currentModelId, models, thread} = sessionMetadata; - const sessionMcpServers = this.resolveSessionMcpServers(requestedMcpServers, true); const currentModel = this.findCurrentModel(models, currentModelId); const currentModelSupportsFast = modelSupportsFast(currentModel); const sessionState: SessionState = { @@ -470,7 +470,8 @@ export class CodexAcpServer implements acp.Agent { cwd: request.cwd, fastModeEnabled: sessionMetadata.currentServiceTier === "fast", currentModelSupportsFast: currentModelSupportsFast, - sessionMcpServers: sessionMcpServers, + sessionMcpServers: this.createSessionMcpServers(requestedMcpServers, true), + configBackedMcpServerNames: sessionMetadata.configBackedMcpServerNames ?? new Set(), }; this.sessions.set(sessionId, sessionState); @@ -724,14 +725,13 @@ export class CodexAcpServer implements acp.Agent { return sessionState; } - private resolveSessionMcpServers( + private createSessionMcpServers( mcpServers: Array, recoverFromStartup: boolean, ): Array { // Explicit MCP servers from the request are the primary source of truth for the session. - const requestedServerNames = getRequestedMcpServerNames(mcpServers); - if (requestedServerNames.length > 0) { - return requestedServerNames; + if (mcpServers.length > 0) { + return mcpServers.map(server => server.name); } // Fresh sessions without MCP config should not inherit any session MCP state. if (!recoverFromStartup) { @@ -955,7 +955,3 @@ export class CodexAcpServer implements acp.Agent { } } } - -function getRequestedMcpServerNames(mcpServers: Array): Array { - return Array.from(new Set(mcpServers.map(server => server.name))); -} diff --git a/src/CodexAppServerClient.ts b/src/CodexAppServerClient.ts index 0d2d083..cbafd3e 100644 --- a/src/CodexAppServerClient.ts +++ b/src/CodexAppServerClient.ts @@ -8,6 +8,8 @@ import type { import type { ConfigReadParams, ConfigReadResponse, + ConfigBatchWriteParams, + ConfigWriteResponse, GetAccountParams, GetAccountResponse, ListMcpServerStatusParams, diff --git a/src/CodexApprovalHandler.ts b/src/CodexApprovalHandler.ts index 0eddf3a..618d39b 100644 --- a/src/CodexApprovalHandler.ts +++ b/src/CodexApprovalHandler.ts @@ -2,6 +2,7 @@ import * as acp from "@agentclientprotocol/sdk"; import type {SessionState} from "./CodexAcpServer"; import type {ApprovalHandler} from "./CodexAppServerClient"; import type { + CommandExecutionApprovalDecision, CommandExecutionRequestApprovalParams, CommandExecutionRequestApprovalResponse, FileChangeRequestApprovalParams, @@ -12,11 +13,13 @@ import {logger} from "./Logger"; import {stripShellPrefix} from "./CodexEventHandler"; import {ApprovalOptionId} from "./ApprovalOptionId"; -const APPROVAL_OPTIONS: acp.PermissionOption[] = [ - { optionId: ApprovalOptionId.AllowOnce, name: "Allow Once", kind: "allow_once" }, - { optionId: ApprovalOptionId.AllowAlways, name: "Allow for Session", kind: "allow_always" }, - { optionId: ApprovalOptionId.RejectOnce, name: "Reject", kind: "reject_once" }, -]; + +// Pair each displayed ACP option with the exact Codex decision it represents, +// so response conversion does not reconstruct decisions from labels or metadata. +type CommandDecisionOption = { + option: acp.PermissionOption; + decision: CommandExecutionApprovalDecision; +}; export class CodexApprovalHandler implements ApprovalHandler { private readonly connection: acp.AgentSideConnection; @@ -37,7 +40,7 @@ export class CodexApprovalHandler implements ApprovalHandler { const sessionId = this.sessionState.sessionId; const acpRequest = this.buildCommandPermissionRequest(sessionId, params); const response = await this.connection.requestPermission(acpRequest); - return this.convertCommandResponse(response); + return this.convertCommandResponse(response, params); } catch (error) { logger.error("Error requesting command execution permission", error); return { decision: "cancel" }; @@ -72,10 +75,156 @@ export class CodexApprovalHandler implements ApprovalHandler { content: reasonContent ? [reasonContent] : null, rawInput: params.command ? { command: stripShellPrefix(params.command), cwd: params.cwd } : null, }, - options: APPROVAL_OPTIONS, + options: this.buildCommandDecisionOptions(params).map(({ option }) => option), }; } + private buildCommandDecisionOptions( + params: CommandExecutionRequestApprovalParams + ): CommandDecisionOption[] { + const decisions = this.buildCommandDecisions(params); + let execAmendmentCount = 0; + let networkAmendmentCount = 0; + + return decisions.map((decision) => { + let amendmentIndex = 0; + if (typeof decision !== "string" && "acceptWithExecpolicyAmendment" in decision) { + amendmentIndex = execAmendmentCount++; + } else if (typeof decision !== "string" && "applyNetworkPolicyAmendment" in decision) { + amendmentIndex = networkAmendmentCount++; + } + return this.convertCommandDecisionToOption(params, decision, amendmentIndex); + }); + } + + private buildCommandDecisions( + params: CommandExecutionRequestApprovalParams + ): CommandExecutionApprovalDecision[] { + const decisions: CommandExecutionApprovalDecision[] = ["accept", "acceptForSession"]; + + if (params.proposedExecpolicyAmendment) { + decisions.push({ + acceptWithExecpolicyAmendment: { + execpolicy_amendment: params.proposedExecpolicyAmendment + } + }); + } + + for (const amendment of params.proposedNetworkPolicyAmendments ?? []) { + decisions.push({ + applyNetworkPolicyAmendment: { + network_policy_amendment: amendment + } + }); + } + + decisions.push("decline"); + return decisions; + } + + private convertCommandDecisionToOption( + params: CommandExecutionRequestApprovalParams, + decision: CommandExecutionApprovalDecision, + amendmentIndex: number + ): CommandDecisionOption { + if (decision === "accept") { + return { + option: { + optionId: ApprovalOptionId.AllowOnce, + name: params.networkApprovalContext ? "Yes, just this once" : "Yes, proceed", + kind: "allow_once" + }, + decision + }; + } + + if (decision === "acceptForSession") { + return { + option: { + optionId: ApprovalOptionId.AllowForSession, + name: params.networkApprovalContext + ? "Yes, and allow this host for this conversation" + : "Yes, and don't ask again for this command in this session", + kind: "allow_always" + }, + decision + }; + } + + if (decision === "decline") { + return { + option: { + optionId: ApprovalOptionId.RejectOnce, + name: "No, and tell Codex what to do differently", + kind: "reject_once" + }, + decision + }; + } + + if (decision === "cancel") { + return { + option: { + optionId: ApprovalOptionId.RejectOnce, + name: "No, and tell Codex what to do differently", + kind: "reject_once" + }, + decision + }; + } + + if ("acceptWithExecpolicyAmendment" in decision) { + // This amendment corresponds to a Codex exec-policy + // `prefix_rule(..., decision="allow")`, not session-scoped approval. + return { + option: { + optionId: this.indexedOptionId(ApprovalOptionId.AllowCommandPrefixRule, amendmentIndex), + name: this.commandPrefixApprovalLabel( + decision.acceptWithExecpolicyAmendment.execpolicy_amendment + ), + kind: "allow_always" + }, + decision + }; + } + + return { + option: { + optionId: this.indexedOptionId(ApprovalOptionId.ApplyNetworkPolicyAmendment, amendmentIndex), + name: this.networkPolicyApprovalLabel( + decision.applyNetworkPolicyAmendment.network_policy_amendment + ), + kind: decision.applyNetworkPolicyAmendment.network_policy_amendment.action === "deny" + ? "reject_always" + : "allow_always" + }, + decision + }; + } + + private commandPrefixApprovalLabel(execpolicyAmendment: string[]): string { + const commandPrefix = execpolicyAmendment.join(" "); + if (commandPrefix === "") { + return "Yes, and don't ask again for similar commands"; + } + return `Yes, and don't ask again for commands that start with \`${commandPrefix}\``; + } + + private networkPolicyApprovalLabel( + amendment: { host: string; action: "allow" | "deny" } + ): string { + return amendment.action === "deny" + ? "No, and block this host in the future" + : "Yes, and allow this host in the future"; + } + + // ACP responses only return the selected optionId. The network field is + // plural, so repeated amendments need unique IDs while the common + // single-amendment ID stays stable. + private indexedOptionId(optionId: ApprovalOptionId, index: number): ApprovalOptionId | string { + return index === 0 ? optionId : `${optionId}:${index}`; + } + private createContentFromReason(reason: string | null): ToolCallContent | null { if (reason === null || reason === "") { return null; @@ -102,25 +251,38 @@ export class CodexApprovalHandler implements ApprovalHandler { status: "pending", content: reasonContent ? [reasonContent] : null, }, - options: APPROVAL_OPTIONS, + options: [ + { optionId: ApprovalOptionId.AllowOnce, name: "Yes, proceed", kind: "allow_once" }, + { + optionId: ApprovalOptionId.AllowForSession, + name: "Yes, and don't ask again for these files", + kind: "allow_always" + }, + { + optionId: ApprovalOptionId.RejectOnce, + name: "No, and tell Codex what to do differently", + kind: "reject_once" + }, + ], }; } private convertCommandResponse( - response: acp.RequestPermissionResponse + response: acp.RequestPermissionResponse, + params: CommandExecutionRequestApprovalParams ): CommandExecutionRequestApprovalResponse { if (response.outcome.outcome === "cancelled") { return { decision: "cancel" }; } const optionId = response.outcome.optionId; - if (optionId === ApprovalOptionId.AllowOnce) { - return { decision: "accept" }; - } else if (optionId === ApprovalOptionId.AllowAlways) { - return { decision: "acceptForSession" }; - } else { - return { decision: "decline" }; + const selectedOption = this.buildCommandDecisionOptions(params) + .find(({ option }) => option.optionId === optionId); + if (selectedOption) { + return { decision: selectedOption.decision }; } + + return { decision: "decline" }; } private convertFileChangeResponse( @@ -133,7 +295,7 @@ export class CodexApprovalHandler implements ApprovalHandler { const optionId = response.outcome.optionId; if (optionId === ApprovalOptionId.AllowOnce) { return { decision: "accept" }; - } else if (optionId === ApprovalOptionId.AllowAlways) { + } else if (optionId === ApprovalOptionId.AllowForSession) { return { decision: "acceptForSession" }; } else { return { decision: "cancel" }; diff --git a/src/CodexCommands.ts b/src/CodexCommands.ts index a125d71..bc20a65 100644 --- a/src/CodexCommands.ts +++ b/src/CodexCommands.ts @@ -153,9 +153,7 @@ export class CodexCommands { const resourceCount = (server.resources ?? []).length; return `- ${server.name}: ${toolCount} tools, ${resourceCount} resources, auth=${server.authStatus}`; }); - const sessionServers = sessionState.sessionMcpServers - ? sessionState.sessionMcpServers.map(serverName => `- ${serverName}`) - : []; + const sessionServers = sessionState.sessionMcpServers.map(server => `- ${server}`); const lines = [...configuredServers, ...sessionServers]; const text = lines.length > 0 ? ["Configured MCP servers:", ...lines].join("\n") diff --git a/src/CodexElicitationHandler.ts b/src/CodexElicitationHandler.ts index 68a4994..e48dea0 100644 --- a/src/CodexElicitationHandler.ts +++ b/src/CodexElicitationHandler.ts @@ -9,16 +9,28 @@ import type { McpServerElicitationRequestResponse, } from "./app-server/v2"; import { logger } from "./Logger"; -import { McpApprovalOptionId } from "./McpApprovalOptionId"; +import { ApprovalOptionId } from "./ApprovalOptionId"; // Standard elicitation options (non-tool-call approval). const ELICITATION_OPTIONS: acp.PermissionOption[] = [ - { optionId: "accept", name: "Accept", kind: "allow_once" }, - { optionId: "decline", name: "Decline", kind: "reject_once" }, + { optionId: ApprovalOptionId.AllowOnce, name: "Accept", kind: "allow_once" }, + { optionId: ApprovalOptionId.RejectOnce, name: "Decline", kind: "reject_once" }, ]; type PersistValue = "session" | "always"; +function buildToolApprovalOption( + optionId: string, + name: string, + kind: acp.PermissionOption["kind"], +): acp.PermissionOption { + return { + optionId, + name, + kind + }; +} + /** * Parses the `persist` field from the elicitation request `_meta`. * Codex advertises which persistence options the client should show. @@ -49,19 +61,40 @@ function isMcpToolCallApproval(meta: unknown): boolean { /** * Builds the ACP permission options for an MCP tool call approval elicitation. - * Always includes "Allow Once"; adds session/always persist options when advertised. + * Always includes "Allow"; adds session/persistent approval options when advertised. */ -function buildToolApprovalOptions(persistOptions: Set): acp.PermissionOption[] { +function buildToolApprovalOptions( + persistOptions: Set, + allowPersistentApproval: boolean +): acp.PermissionOption[] { const options: acp.PermissionOption[] = [ - { optionId: McpApprovalOptionId.AllowOnce, name: "Allow", kind: "allow_once" }, + buildToolApprovalOption( + ApprovalOptionId.AllowOnce, + "Allow", + "allow_once" + ), ]; + // Codex advertises MCP tool approval persistence choices in request _meta.persist. + // Only surface scopes the server explicitly offered. if (persistOptions.has("session")) { - options.push({ optionId: McpApprovalOptionId.AllowSession, name: "Allow for This Session", kind: "allow_always" }); + options.push(buildToolApprovalOption( + ApprovalOptionId.AllowForSession, + "Allow for this session", + "allow_always" + )); } - if (persistOptions.has("always")) { - options.push({ optionId: McpApprovalOptionId.AllowAlways, name: "Allow and Don't Ask Again", kind: "allow_always" }); + if (persistOptions.has("always") && allowPersistentApproval) { + options.push(buildToolApprovalOption( + ApprovalOptionId.AllowPersist, + "Always allow", + "allow_always" + )); } - options.push({ optionId: McpApprovalOptionId.Decline, name: "Decline", kind: "reject_once" }); + options.push(buildToolApprovalOption( + ApprovalOptionId.RejectOnce, + "Cancel", + "reject_once" + )); return options; } @@ -114,14 +147,14 @@ export class CodexElicitationHandler implements ElicitationHandler { const response = await this.connection.requestPermission(request); if (correlatedCallId !== undefined && response.outcome.outcome !== "cancelled") { const optionId = response.outcome.optionId; - if (optionId !== McpApprovalOptionId.Decline) { + if (optionId !== ApprovalOptionId.RejectOnce) { await this.connection.sessionUpdate({ sessionId: this.sessionState.sessionId, update: { sessionUpdate: "tool_call_update", toolCallId: correlatedCallId, status: "in_progress" }, }); } } - return this.convertResponse(response); + return await this.convertResponse(response); } catch (error) { logger.error("Error handling MCP elicitation request", error); return { action: "cancel", content: null, _meta: null }; @@ -140,7 +173,10 @@ export class CodexElicitationHandler implements ElicitationHandler { const meta = params._meta; const isToolApproval = isMcpToolCallApproval(meta); const options = isToolApproval - ? buildToolApprovalOptions(parsePersistOptions(meta)) + ? buildToolApprovalOptions( + parsePersistOptions(meta), + this.serverSupportsPersistentApproval(params.serverName) + ) : ELICITATION_OPTIONS; if (params.mode === "form") { @@ -200,26 +236,34 @@ export class CodexElicitationHandler implements ElicitationHandler { } } - private convertResponse( + private async convertResponse( response: acp.RequestPermissionResponse - ): McpServerElicitationRequestResponse { + ): Promise { if (response.outcome.outcome === "cancelled") { return { action: "cancel", content: null, _meta: null }; } const optionId = response.outcome.optionId; - if (optionId === McpApprovalOptionId.AllowSession) { + if (optionId === ApprovalOptionId.AllowForSession) { + // This _meta is part of Codex's MCP tool approval response contract. + // It tells app-server to remember this approval for the current session. return { action: "accept", content: null, _meta: { persist: "session" } }; } - if (optionId === McpApprovalOptionId.AllowAlways) { + if (optionId === ApprovalOptionId.AllowPersist) { + // This _meta is part of Codex's MCP tool approval response contract. + // It tells app-server to persist this MCP tool approval across sessions. return { action: "accept", content: null, _meta: { persist: "always" } }; } - if (optionId === McpApprovalOptionId.AllowOnce || optionId === "accept") { + if (optionId === ApprovalOptionId.AllowOnce) { return { action: "accept", content: null, _meta: null }; } return { action: "decline", content: null, _meta: null }; } + private serverSupportsPersistentApproval(serverName: string): boolean { + return this.sessionState.configBackedMcpServerNames?.has(serverName) === true; + } + private handleItemStarted(event: ItemStartedNotification): void { if (event.item.type !== "mcpToolCall") { return; diff --git a/src/McpApprovalOptionId.ts b/src/McpApprovalOptionId.ts deleted file mode 100644 index 3728145..0000000 --- a/src/McpApprovalOptionId.ts +++ /dev/null @@ -1,8 +0,0 @@ -export const McpApprovalOptionId = { - AllowOnce: "allow_once", - AllowSession: "allow_session", - AllowAlways: "allow_always", - Decline: "decline", -} as const; - -export type McpApprovalOptionId = typeof McpApprovalOptionId[keyof typeof McpApprovalOptionId]; diff --git a/src/__tests__/CodexACPAgent/CodexAcpClient.test.ts b/src/__tests__/CodexACPAgent/CodexAcpClient.test.ts index a66a827..5638009 100644 --- a/src/__tests__/CodexACPAgent/CodexAcpClient.test.ts +++ b/src/__tests__/CodexACPAgent/CodexAcpClient.test.ts @@ -81,6 +81,7 @@ describe('ACP server test', { timeout: 40_000 }, () => { "account/login/start", "account/read", "account/updated", + "config/read", "thread/start", "model/list", "thread/started", diff --git a/src/__tests__/CodexACPAgent/approval-events.test.ts b/src/__tests__/CodexACPAgent/approval-events.test.ts index 7ee4374..bbe6dfa 100644 --- a/src/__tests__/CodexACPAgent/approval-events.test.ts +++ b/src/__tests__/CodexACPAgent/approval-events.test.ts @@ -50,7 +50,7 @@ describe('Approval Events', () => { describe('Command execution approval', () => { const commandApprovalCases = [ { optionId: 'allow_once', expectedDecision: 'accept', description: 'allow once' }, - { optionId: 'allow_always', expectedDecision: 'acceptForSession', description: 'allow for session' }, + { optionId: 'allow_for_session', expectedDecision: 'acceptForSession', description: 'allow for session' }, { optionId: 'reject_once', expectedDecision: 'decline', description: 'reject' }, ] as const; @@ -107,6 +107,74 @@ describe('Approval Events', () => { await promptPromise; }); + it('should map execpolicy amendment approval to Codex decision', async () => { + const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); + const execpolicyAmendment = ['npm', 'run']; + fixture.setPermissionResponse({ + outcome: { outcome: 'selected', optionId: 'allow_command_prefix_rule' } + }); + + const params: CommandExecutionRequestApprovalParams = { + threadId: sessionId, + turnId: 'turn-1', + itemId: 'item-prefix-rule-fallback', + reason: 'Run npm script', + proposedExecpolicyAmendment: execpolicyAmendment, + }; + + const response = await fixture.sendServerRequest( + 'item/commandExecution/requestApproval', + params + ); + + expect(response).toEqual({ + decision: { + acceptWithExecpolicyAmendment: { + execpolicy_amendment: execpolicyAmendment, + }, + }, + }); + + completeTurn(); + await promptPromise; + }); + + it('should map network policy amendment approval to Codex decision', async () => { + const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); + const networkPolicyAmendment = { host: 'registry.npmjs.org', action: 'allow' } as const; + fixture.setPermissionResponse({ + outcome: { outcome: 'selected', optionId: 'apply_network_policy_amendment' } + }); + + const params: CommandExecutionRequestApprovalParams = { + threadId: sessionId, + turnId: 'turn-1', + itemId: 'item-network-policy', + reason: 'Allow network access', + proposedExecpolicyAmendment: null, + proposedNetworkPolicyAmendments: [networkPolicyAmendment], + }; + + const response = await fixture.sendServerRequest( + 'item/commandExecution/requestApproval', + params + ); + + expect(response).toEqual({ + decision: { + applyNetworkPolicyAmendment: { + network_policy_amendment: networkPolicyAmendment, + }, + }, + }); + await expect(fixture.getAcpConnectionDump(['_meta'])).toMatchFileSnapshot( + 'data/approval-command-network-policy.json' + ); + + completeTurn(); + await promptPromise; + }); + it('should return cancel when no handler registered', async () => { const params: CommandExecutionRequestApprovalParams = { threadId: 'non-existent-session', @@ -221,7 +289,7 @@ describe('Approval Events', () => { describe('File change approval', () => { const fileChangeApprovalCases = [ { optionId: 'allow_once', expectedDecision: 'accept', description: 'allow once' }, - { optionId: 'allow_always', expectedDecision: 'acceptForSession', description: 'allow for session' }, + { optionId: 'allow_for_session', expectedDecision: 'acceptForSession', description: 'allow for session' }, { optionId: 'reject_once', expectedDecision: 'cancel', description: 'reject' }, ] as const; diff --git a/src/__tests__/CodexACPAgent/data/approval-command-allow-once.json b/src/__tests__/CodexACPAgent/data/approval-command-allow-once.json index 453ed87..d2a049b 100644 --- a/src/__tests__/CodexACPAgent/data/approval-command-allow-once.json +++ b/src/__tests__/CodexACPAgent/data/approval-command-allow-once.json @@ -21,17 +21,17 @@ "options": [ { "optionId": "allow_once", - "name": "Allow Once", + "name": "Yes, proceed", "kind": "allow_once" }, { - "optionId": "allow_always", - "name": "Allow for Session", + "optionId": "allow_for_session", + "name": "Yes, and don't ask again for this command in this session", "kind": "allow_always" }, { "optionId": "reject_once", - "name": "Reject", + "name": "No, and tell Codex what to do differently", "kind": "reject_once" } ] diff --git a/src/__tests__/CodexACPAgent/data/approval-command-network-policy.json b/src/__tests__/CodexACPAgent/data/approval-command-network-policy.json new file mode 100644 index 0000000..2d58ec6 --- /dev/null +++ b/src/__tests__/CodexACPAgent/data/approval-command-network-policy.json @@ -0,0 +1,45 @@ +{ + "method": "requestPermission", + "args": [ + { + "sessionId": "test-session-id", + "toolCall": { + "toolCallId": "item-network-policy", + "kind": "execute", + "status": "pending", + "content": [ + { + "type": "content", + "content": { + "type": "text", + "text": "Allow network access" + } + } + ], + "rawInput": null + }, + "options": [ + { + "optionId": "allow_once", + "name": "Yes, proceed", + "kind": "allow_once" + }, + { + "optionId": "allow_for_session", + "name": "Yes, and don't ask again for this command in this session", + "kind": "allow_always" + }, + { + "optionId": "apply_network_policy_amendment", + "name": "Yes, and allow this host in the future", + "kind": "allow_always" + }, + { + "optionId": "reject_once", + "name": "No, and tell Codex what to do differently", + "kind": "reject_once" + } + ] + } + ] +} \ No newline at end of file diff --git a/src/__tests__/CodexACPAgent/data/approval-command-with-rawInput.json b/src/__tests__/CodexACPAgent/data/approval-command-with-rawInput.json index 5be9602..5050084 100644 --- a/src/__tests__/CodexACPAgent/data/approval-command-with-rawInput.json +++ b/src/__tests__/CodexACPAgent/data/approval-command-with-rawInput.json @@ -24,17 +24,17 @@ "options": [ { "optionId": "allow_once", - "name": "Allow Once", + "name": "Yes, proceed", "kind": "allow_once" }, { - "optionId": "allow_always", - "name": "Allow for Session", + "optionId": "allow_for_session", + "name": "Yes, and don't ask again for this command in this session", "kind": "allow_always" }, { "optionId": "reject_once", - "name": "Reject", + "name": "No, and tell Codex what to do differently", "kind": "reject_once" } ] diff --git a/src/__tests__/CodexACPAgent/data/approval-file-change.json b/src/__tests__/CodexACPAgent/data/approval-file-change.json index b4c351f..e4271fb 100644 --- a/src/__tests__/CodexACPAgent/data/approval-file-change.json +++ b/src/__tests__/CodexACPAgent/data/approval-file-change.json @@ -20,17 +20,17 @@ "options": [ { "optionId": "allow_once", - "name": "Allow Once", + "name": "Yes, proceed", "kind": "allow_once" }, { - "optionId": "allow_always", - "name": "Allow for Session", + "optionId": "allow_for_session", + "name": "Yes, and don't ask again for these files", "kind": "allow_always" }, { "optionId": "reject_once", - "name": "Reject", + "name": "No, and tell Codex what to do differently", "kind": "reject_once" } ] diff --git a/src/__tests__/CodexACPAgent/data/elicitation-form-accept.json b/src/__tests__/CodexACPAgent/data/elicitation-form-accept.json index a2113d5..4483207 100644 --- a/src/__tests__/CodexACPAgent/data/elicitation-form-accept.json +++ b/src/__tests__/CodexACPAgent/data/elicitation-form-accept.json @@ -33,12 +33,12 @@ }, "options": [ { - "optionId": "accept", + "optionId": "allow_once", "name": "Accept", "kind": "allow_once" }, { - "optionId": "decline", + "optionId": "reject_once", "name": "Decline", "kind": "reject_once" } diff --git a/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-all-persist.json b/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-all-persist.json index d2f4080..1d0cabd 100644 --- a/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-all-persist.json +++ b/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-all-persist.json @@ -24,7 +24,9 @@ } } }, - "_meta": "_meta", + "_meta": { + "is_mcp_tool_approval": true + }, "options": [ { "optionId": "allow_once", @@ -32,18 +34,18 @@ "kind": "allow_once" }, { - "optionId": "allow_session", - "name": "Allow for This Session", + "optionId": "allow_for_session", + "name": "Allow for this session", "kind": "allow_always" }, { - "optionId": "allow_always", - "name": "Allow and Don't Ask Again", + "optionId": "allow_persist", + "name": "Always allow", "kind": "allow_always" }, { - "optionId": "decline", - "name": "Decline", + "optionId": "reject_once", + "name": "Cancel", "kind": "reject_once" } ] diff --git a/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-no-persist.json b/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-no-persist.json index b212b99..3aae8b2 100644 --- a/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-no-persist.json +++ b/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-no-persist.json @@ -24,7 +24,9 @@ } } }, - "_meta": "_meta", + "_meta": { + "is_mcp_tool_approval": true + }, "options": [ { "optionId": "allow_once", @@ -32,8 +34,8 @@ "kind": "allow_once" }, { - "optionId": "decline", - "name": "Decline", + "optionId": "reject_once", + "name": "Cancel", "kind": "reject_once" } ] diff --git a/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-session-only.json b/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-session-only.json index 50358be..c6a0d5f 100644 --- a/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-session-only.json +++ b/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-session-only.json @@ -24,7 +24,9 @@ } } }, - "_meta": "_meta", + "_meta": { + "is_mcp_tool_approval": true + }, "options": [ { "optionId": "allow_once", @@ -32,13 +34,13 @@ "kind": "allow_once" }, { - "optionId": "allow_session", - "name": "Allow for This Session", + "optionId": "allow_for_session", + "name": "Allow for this session", "kind": "allow_always" }, { - "optionId": "decline", - "name": "Decline", + "optionId": "reject_once", + "name": "Cancel", "kind": "reject_once" } ] diff --git a/src/__tests__/CodexACPAgent/data/elicitation-url-accept.json b/src/__tests__/CodexACPAgent/data/elicitation-url-accept.json index da7f9c0..ea36124 100644 --- a/src/__tests__/CodexACPAgent/data/elicitation-url-accept.json +++ b/src/__tests__/CodexACPAgent/data/elicitation-url-accept.json @@ -23,12 +23,12 @@ }, "options": [ { - "optionId": "accept", + "optionId": "allow_once", "name": "Accept", "kind": "allow_once" }, { - "optionId": "decline", + "optionId": "reject_once", "name": "Decline", "kind": "reject_once" } diff --git a/src/__tests__/CodexACPAgent/e2e/acp-e2e-mcp-approval.test.ts b/src/__tests__/CodexACPAgent/e2e/acp-e2e-mcp-approval.test.ts index e0ef576..600ba36 100644 --- a/src/__tests__/CodexACPAgent/e2e/acp-e2e-mcp-approval.test.ts +++ b/src/__tests__/CodexACPAgent/e2e/acp-e2e-mcp-approval.test.ts @@ -2,10 +2,9 @@ import type * as acp from "@agentclientprotocol/sdk"; import fs from "node:fs"; import path from "node:path"; import {afterEach, beforeEach, expect, it} from "vitest"; -import {McpApprovalOptionId, type McpApprovalOptionId as McpApprovalOptionIdValue} from "../../../McpApprovalOptionId"; +import {ApprovalOptionId} from "../../../ApprovalOptionId"; import { - createAuthenticatedFixture, - describeE2E, + createAuthenticatedFixture, describeE2E, expectEndTurn, type PermissionResponder, type SpawnedAgentFixture, @@ -31,18 +30,18 @@ function isMcpPermissionRequest(request: acp.RequestPermissionRequest): boolean return request.toolCall.kind === "execute" && request._meta?.["is_mcp_tool_approval"] === true; } -function createMcpPermissionResponse(optionId: McpApprovalOptionIdValue | null): acp.RequestPermissionResponse { +function createMcpPermissionResponse(optionId: ApprovalOptionId | null): acp.RequestPermissionResponse { if (optionId === null) { return {outcome: {outcome: "cancelled"}}; } return {outcome: {outcome: "selected", optionId}}; } -function createMcpPermissionResponder(...optionIds: McpApprovalOptionIdValue[]): PermissionResponder { +function createMcpPermissionResponder(...optionIds: ApprovalOptionId[]): PermissionResponder { const queue = [...optionIds]; return (request) => createMcpPermissionResponse( isMcpPermissionRequest(request) - ? queue.shift() ?? McpApprovalOptionId.Decline + ? queue.shift() ?? ApprovalOptionId.RejectOnce : null, ); } @@ -81,7 +80,7 @@ describeE2E("E2E MCP approval tests (configured in session)", () => { } it("executes an approved MCP tool call", async () => { - fixture.setPermissionResponder(createMcpPermissionResponder(McpApprovalOptionId.AllowOnce)); + fixture.setPermissionResponder(createMcpPermissionResponder(ApprovalOptionId.AllowOnce)); const {sessionId, invocationMarkerPath} = await createMcpSession(); await expectEchoToolReply(fixture, sessionId, MCP_ECHO_MESSAGE); @@ -90,7 +89,7 @@ describeE2E("E2E MCP approval tests (configured in session)", () => { }); it("ends turn when MCP tool call is rejected", async () => { - fixture.setPermissionResponder(createMcpPermissionResponder(McpApprovalOptionId.Decline)); + fixture.setPermissionResponder(createMcpPermissionResponder(ApprovalOptionId.RejectOnce)); const {sessionId, invocationMarkerPath} = await createMcpSession(); expectEndTurn(await fixture.connection.prompt({ @@ -105,7 +104,7 @@ describeE2E("E2E MCP approval tests (configured in session)", () => { }); it("skips subsequent approvals in the same session when allow_session is selected", async () => { - fixture.setPermissionResponder(createMcpPermissionResponder(McpApprovalOptionId.AllowSession)); + fixture.setPermissionResponder(createMcpPermissionResponder(ApprovalOptionId.AllowForSession)); const {sessionId, invocationMarkerPath} = await createMcpSession(); await expectEchoToolReply(fixture, sessionId, "session approval first"); @@ -116,7 +115,7 @@ describeE2E("E2E MCP approval tests (configured in session)", () => { }); it("requests subsequent approvals after session restart when allow_session is selected", async () => { - fixture.setPermissionResponder(createMcpPermissionResponder(McpApprovalOptionId.AllowSession, McpApprovalOptionId.AllowOnce)); + fixture.setPermissionResponder(createMcpPermissionResponder(ApprovalOptionId.AllowForSession, ApprovalOptionId.AllowOnce)); const {sessionId, invocationMarkerPath} = await createMcpSession(); await expectEchoToolReply(fixture, sessionId, MCP_ECHO_MESSAGE); @@ -154,9 +153,9 @@ describeE2E("E2E MCP approval tests (configured in toml)", () => { fixture = await createAuthenticatedFixture(undefined, [createMcpServer(invocationMarkerPath)]); }); - it("skips subsequent approvals in the same session when allow_always is selected", async () => { + it("skips subsequent approvals in the same session when allow_for_session is selected", async () => { fixture.setPermissionResponder( - createMcpPermissionResponder(McpApprovalOptionId.AllowAlways), + createMcpPermissionResponder(ApprovalOptionId.AllowForSession), ); const sessionId = (await fixture.createSession()).sessionId; @@ -167,9 +166,9 @@ describeE2E("E2E MCP approval tests (configured in toml)", () => { expectMcpPermissionRequestCount(fixture, sessionId, 1); }); - it.skip("skips subsequent approvals after session restart when allow_always is selected", async () => { + it("skips subsequent approvals after session restart when allow_persist is selected", async () => { fixture.setPermissionResponder( - createMcpPermissionResponder(McpApprovalOptionId.AllowAlways), + createMcpPermissionResponder(ApprovalOptionId.AllowPersist), ); const firstSessionId = (await fixture.createSession()).sessionId; diff --git a/src/__tests__/CodexACPAgent/e2e/acp-e2e-shell-approval.test.ts b/src/__tests__/CodexACPAgent/e2e/acp-e2e-shell-approval.test.ts index fdfcc55..6f7f5e4 100644 --- a/src/__tests__/CodexACPAgent/e2e/acp-e2e-shell-approval.test.ts +++ b/src/__tests__/CodexACPAgent/e2e/acp-e2e-shell-approval.test.ts @@ -57,8 +57,8 @@ describeE2E("E2E shell approval tests", () => { expectPermissionRequests(fixture, sessionId, {execute: 2, edit: 0}); }); - it("skips subsequent approvals when allow_always is selected", async () => { - fixture.setPermissionResponder(createPermissionResponder("execute", ApprovalOptionId.AllowAlways)); + it("skips subsequent approvals when allow_for_session is selected", async () => { + fixture.setPermissionResponder(createPermissionResponder("execute", ApprovalOptionId.AllowForSession)); await promptShellCommandTwice(); expect(fs.existsSync(path.join(fixture.workspaceDir, FIRST_FILE_NAME))).toBe(true); expect(fs.existsSync(path.join(fixture.workspaceDir, SECOND_FILE_NAME))).toBe(true); diff --git a/src/__tests__/CodexACPAgent/elicitation-events.test.ts b/src/__tests__/CodexACPAgent/elicitation-events.test.ts index 82d14a0..2cd132b 100644 --- a/src/__tests__/CodexACPAgent/elicitation-events.test.ts +++ b/src/__tests__/CodexACPAgent/elicitation-events.test.ts @@ -3,7 +3,6 @@ import type { McpServerElicitationRequestParams } from '../../app-server/v2'; import { createCodexMockTestFixture, createTestSessionState, type CodexMockTestFixture } from '../acp-test-utils'; import type { SessionState } from '../../CodexAcpServer'; import { AgentMode } from "../../AgentMode"; -import { McpApprovalOptionId } from "../../McpApprovalOptionId"; import type { ServerNotification } from "../../app-server"; describe('Elicitation Events', () => { @@ -15,7 +14,7 @@ describe('Elicitation Events', () => { vi.clearAllMocks(); }); - function setupSessionWithPendingPrompt() { + function setupSessionWithPendingPrompt(sessionOverrides?: Partial) { const codexAcpAgent = fixture.getCodexAcpAgent(); let resolveTurnCompleted: (value: { threadId: string; turn: { id: string; items: never[]; status: string; error: null } }) => void; @@ -31,7 +30,8 @@ describe('Elicitation Events', () => { const sessionState: SessionState = createTestSessionState({ sessionId, currentModelId: 'model-id[effort]', - agentMode: AgentMode.DEFAULT_AGENT_MODE + agentMode: AgentMode.DEFAULT_AGENT_MODE, + ...sessionOverrides, }); vi.spyOn(codexAcpAgent, 'getSessionState').mockReturnValue(sessionState); @@ -50,9 +50,9 @@ describe('Elicitation Events', () => { } describe('Form mode elicitation', () => { - it('should map accept to accept', async () => { + it('should map allow_once to accept', async () => { const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); - fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'accept' } }); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_once' } }); const params: McpServerElicitationRequestParams = { threadId: sessionId, turnId: 'turn-1', serverName: 'test-server', @@ -67,9 +67,9 @@ describe('Elicitation Events', () => { await promptPromise; }); - it('should map decline to decline', async () => { + it('should map reject_once to decline', async () => { const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); - fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'decline' } }); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'reject_once' } }); const params: McpServerElicitationRequestParams = { threadId: sessionId, turnId: 'turn-1', serverName: 'test-server', @@ -114,7 +114,7 @@ describe('Elicitation Events', () => { it('should build correct ACP permission request for form mode', async () => { const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); - fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'accept' } }); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_once' } }); const params: McpServerElicitationRequestParams = { threadId: sessionId, turnId: 'turn-1', serverName: 'my-mcp-server', @@ -131,9 +131,73 @@ describe('Elicitation Events', () => { }); describe('MCP tool call approval elicitation', () => { - it('should show Allow/session/always/Decline options when all persist values advertised', async () => { + it('should show Allow/session/always/Cancel options when all persist values advertised', async () => { + const { promptPromise, completeTurn } = setupSessionWithPendingPrompt({ + configBackedMcpServerNames: new Set(['tool-server']), + }); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_once' } }); + + const params: McpServerElicitationRequestParams = { + threadId: sessionId, turnId: 'turn-1', serverName: 'tool-server', + mode: 'form', + _meta: { codex_approval_kind: 'mcp_tool_call', persist: ['session', 'always'] }, + message: 'Allow tool call?', + requestedSchema: { type: 'object', properties: {} }, + }; + + await fixture.sendServerRequest('mcpServer/elicitation/request', params); + await expect(fixture.getAcpConnectionDump([])).toMatchFileSnapshot('data/elicitation-tool-approval-all-persist.json'); + + completeTurn(); + await promptPromise; + }); + + it('should include CLI-style descriptions for MCP tool approval options', async () => { + const { promptPromise, completeTurn } = setupSessionWithPendingPrompt({ + configBackedMcpServerNames: new Set(['tool-server']), + }); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_once' } }); + + const params: McpServerElicitationRequestParams = { + threadId: sessionId, turnId: 'turn-1', serverName: 'tool-server', + mode: 'form', + _meta: { codex_approval_kind: 'mcp_tool_call', persist: ['session', 'always'] }, + message: 'Allow tool call?', + requestedSchema: { type: 'object', properties: {} }, + }; + + await fixture.sendServerRequest('mcpServer/elicitation/request', params); + const [requestPermissionEvent] = fixture.getAcpConnectionEvents([]); + expect(requestPermissionEvent?.args[0].options).toEqual([ + { + optionId: 'allow_once', + name: 'Allow', + kind: 'allow_once', + }, + { + optionId: 'allow_for_session', + name: 'Allow for this session', + kind: 'allow_always', + }, + { + optionId: 'allow_persist', + name: 'Always allow', + kind: 'allow_always', + }, + { + optionId: 'reject_once', + name: 'Cancel', + kind: 'reject_once', + }, + ]); + + completeTurn(); + await promptPromise; + }); + + it('should not show persistent approval for MCP servers that are not configured', async () => { const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); - fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: McpApprovalOptionId.AllowOnce } }); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_once' } }); const params: McpServerElicitationRequestParams = { threadId: sessionId, turnId: 'turn-1', serverName: 'tool-server', @@ -144,7 +208,13 @@ describe('Elicitation Events', () => { }; await fixture.sendServerRequest('mcpServer/elicitation/request', params); - await expect(fixture.getAcpConnectionDump(['_meta'])).toMatchFileSnapshot('data/elicitation-tool-approval-all-persist.json'); + const [requestPermissionEvent] = fixture.getAcpConnectionEvents([]); + const optionIds = requestPermissionEvent?.args[0].options.map((option: { optionId: string }) => option.optionId); + expect(optionIds).toEqual([ + 'allow_once', + 'allow_for_session', + 'reject_once', + ]); completeTurn(); await promptPromise; @@ -152,7 +222,7 @@ describe('Elicitation Events', () => { it('should map allow_once to accept with null meta', async () => { const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); - fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: McpApprovalOptionId.AllowOnce } }); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_once' } }); const params: McpServerElicitationRequestParams = { threadId: sessionId, turnId: 'turn-1', serverName: 'tool-server', @@ -169,9 +239,9 @@ describe('Elicitation Events', () => { await promptPromise; }); - it('should map allow_session to accept with persist:session meta', async () => { + it('should map allow_for_session to accept with persist:session meta', async () => { const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); - fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: McpApprovalOptionId.AllowSession } }); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_for_session' } }); const params: McpServerElicitationRequestParams = { threadId: sessionId, turnId: 'turn-1', serverName: 'tool-server', @@ -188,9 +258,11 @@ describe('Elicitation Events', () => { await promptPromise; }); - it('should map allow_always to accept with persist:always meta', async () => { - const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); - fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: McpApprovalOptionId.AllowAlways } }); + it('should map persistent approval to accept with persist:always meta', async () => { + const { promptPromise, completeTurn } = setupSessionWithPendingPrompt({ + configBackedMcpServerNames: new Set(['tool-server']), + }); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_persist' } }); const params: McpServerElicitationRequestParams = { threadId: sessionId, turnId: 'turn-1', serverName: 'tool-server', @@ -209,7 +281,7 @@ describe('Elicitation Events', () => { it('should only show session option when persist is "session"', async () => { const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); - fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: McpApprovalOptionId.AllowOnce } }); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_once' } }); const params: McpServerElicitationRequestParams = { threadId: sessionId, turnId: 'turn-1', serverName: 'tool-server', @@ -220,15 +292,15 @@ describe('Elicitation Events', () => { }; await fixture.sendServerRequest('mcpServer/elicitation/request', params); - await expect(fixture.getAcpConnectionDump(['_meta'])).toMatchFileSnapshot('data/elicitation-tool-approval-session-only.json'); + await expect(fixture.getAcpConnectionDump([])).toMatchFileSnapshot('data/elicitation-tool-approval-session-only.json'); completeTurn(); await promptPromise; }); - it('should show only Allow and Decline when no persist options', async () => { + it('should show only Allow and Cancel when no persist options', async () => { const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); - fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: McpApprovalOptionId.AllowOnce } }); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_once' } }); const params: McpServerElicitationRequestParams = { threadId: sessionId, turnId: 'turn-1', serverName: 'tool-server', @@ -239,7 +311,7 @@ describe('Elicitation Events', () => { }; await fixture.sendServerRequest('mcpServer/elicitation/request', params); - await expect(fixture.getAcpConnectionDump(['_meta'])).toMatchFileSnapshot('data/elicitation-tool-approval-no-persist.json'); + await expect(fixture.getAcpConnectionDump([])).toMatchFileSnapshot('data/elicitation-tool-approval-no-persist.json'); completeTurn(); await promptPromise; @@ -247,7 +319,7 @@ describe('Elicitation Events', () => { it('should not reuse a completed auto-approved call id for a later approval request', async () => { const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); - fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: McpApprovalOptionId.AllowOnce } }); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_once' } }); const startedNotification: ServerNotification = { method: 'item/started', @@ -310,7 +382,7 @@ describe('Elicitation Events', () => { it('should not reuse a stale call id after serverRequest/resolved clears interrupted approval state', async () => { const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); - fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: McpApprovalOptionId.AllowOnce } }); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_once' } }); const startedNotification: ServerNotification = { method: 'item/started', @@ -362,9 +434,9 @@ describe('Elicitation Events', () => { }); describe('URL mode elicitation', () => { - it('should map accept to accept for URL mode', async () => { + it('should map allow_once to accept for URL mode', async () => { const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); - fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'accept' } }); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_once' } }); const params: McpServerElicitationRequestParams = { threadId: sessionId, turnId: 'turn-1', serverName: 'auth-server', @@ -398,7 +470,7 @@ describe('Elicitation Events', () => { it('should build correct ACP permission request for URL mode', async () => { const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); - fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'accept' } }); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'allow_once' } }); const params: McpServerElicitationRequestParams = { threadId: sessionId, turnId: 'turn-1', serverName: 'auth-server', diff --git a/src/__tests__/acp-test-utils.ts b/src/__tests__/acp-test-utils.ts index 9f82f16..4041c5a 100644 --- a/src/__tests__/acp-test-utils.ts +++ b/src/__tests__/acp-test-utils.ts @@ -345,6 +345,7 @@ export function createTestSessionState(overrides?: Partial): Sessi agentMode: AgentMode.DEFAULT_AGENT_MODE, fastModeEnabled: false, currentModelSupportsFast: false, + sessionMcpServers: [], ...overrides, }; }