From ab193214c9f3dae6636b9d11521abaa8871046de Mon Sep 17 00:00:00 2001 From: Nikita Ashikhmin Date: Sat, 15 Aug 2026 01:47:54 +0400 Subject: [PATCH 1/3] feat: add ACP v1 permission presentation --- src/CodexAcpServer.ts | 10 +- src/CodexApprovalHandler.ts | 594 ++--------- src/CodexApprovalOptions.ts | 198 ++++ src/CodexApprovalPresentationStore.ts | 33 + src/CodexPermissionMetadata.ts | 49 + src/CodexPermissionPresentation.ts | 141 +++ .../CodexACPAgent/approval-events.test.ts | 938 +++++++----------- 7 files changed, 894 insertions(+), 1069 deletions(-) create mode 100644 src/CodexApprovalOptions.ts create mode 100644 src/CodexApprovalPresentationStore.ts create mode 100644 src/CodexPermissionMetadata.ts create mode 100644 src/CodexPermissionPresentation.ts diff --git a/src/CodexAcpServer.ts b/src/CodexAcpServer.ts index 6e0c394b..a74c14f8 100644 --- a/src/CodexAcpServer.ts +++ b/src/CodexAcpServer.ts @@ -2,6 +2,7 @@ import * as acp from "@agentclientprotocol/sdk"; import {RequestError, type SessionId, type SessionModeState} from "@agentclientprotocol/sdk"; import {CodexEventHandler, type CompletedPlan} from "./CodexEventHandler"; import {CodexApprovalHandler} from "./CodexApprovalHandler"; +import {CodexApprovalPresentationStore} from "./CodexApprovalPresentationStore"; import {CodexElicitationHandler} from "./CodexElicitationHandler"; import {type CodexAuthRequest, getCodexAuthMethods, isCodexAuthRequest} from "./CodexAuthMethod"; import {clientSupportsUrlElicitation} from "./ElicitationCapabilities"; @@ -2280,7 +2281,13 @@ export class CodexAcpServer { this.sessionFailureEpoch, ); eventHandler = promptEventHandler; - const approvalHandler = new CodexApprovalHandler(this.connection, sessionState, activePrompt.signal); + const approvalPresentationStore = new CodexApprovalPresentationStore(); + const approvalHandler = new CodexApprovalHandler( + this.connection, + sessionState, + approvalPresentationStore, + activePrompt.signal, + ); const elicitationHandler = new CodexElicitationHandler( this.connection, sessionState, @@ -2295,6 +2302,7 @@ export class CodexAcpServer { } const completesActiveTurn = event.method === "turn/completed" && event.params.turn.id === sessionState.currentTurnId; + approvalPresentationStore.handleNotification(event); await elicitationHandler.handleNotification(event); await promptEventHandler.handleNotification(event); if (completesActiveTurn) { diff --git a/src/CodexApprovalHandler.ts b/src/CodexApprovalHandler.ts index 7c2e08a9..761a3139 100644 --- a/src/CodexApprovalHandler.ts +++ b/src/CodexApprovalHandler.ts @@ -2,558 +2,176 @@ import * as acp from "@agentclientprotocol/sdk"; import type {SessionState} from "./CodexAcpServer"; import type {ApprovalHandler} from "./CodexAppServerClient"; import type { - CommandExecutionApprovalDecision, CommandExecutionRequestApprovalParams, CommandExecutionRequestApprovalResponse, - FileChangeApprovalDecision, FileChangeRequestApprovalParams, FileChangeRequestApprovalResponse, GrantedPermissionProfile, - NetworkPolicyAmendment, PermissionsRequestApprovalParams, PermissionsRequestApprovalResponse, RequestPermissionProfile, } from "./app-server/v2"; import {logger} from "./Logger"; -import {stripShellPrefix} from "./CodexEventHandler"; import {ApprovalOptionId} from "./ApprovalOptionId"; import type {AcpClientConnection} from "./ACPSessionConnection"; - -type CommandDecisionOption = { - option: acp.PermissionOption; - decision: CommandExecutionApprovalDecision; -}; - -type FileChangeDecisionOption = { - option: acp.PermissionOption; - decision: FileChangeApprovalDecision; -}; - -type PermissionMetadata = { - version: 1; - changes: Array>; -}; - -function permissionOption( - optionId: string, - name: string, - kind: acp.PermissionOptionKind, - codexMeta?: Record, - permission?: PermissionMetadata, -): acp.PermissionOption { - return { - optionId, - name, - kind, - ...((codexMeta || permission) ? { - _meta: { - ...(permission ? {permission} : {}), - ...(codexMeta ? {codex: codexMeta} : {}), - }, - } : {}), - }; -} +import { + commandDecisionOptions, + fileChangeDecisionOptions, + permissionProfileOptions, + type CommandParamsWithAvailableDecisions, + type DecisionOption, +} from "./CodexApprovalOptions"; +import { + CODEX_ADDITIONAL_PERMISSIONS_TITLE, + CODEX_COMMAND_PERMISSION_TITLE, + CODEX_FILE_CHANGE_PERMISSION_TITLE, + CODEX_NETWORK_PERMISSION_TITLE, + requestPermissionMeta, +} from "./CodexPermissionMetadata"; +import { + additionalPermissionsToolCall, + commandToolCall, + fileChangeToolCall, +} from "./CodexPermissionPresentation"; +import type {CodexApprovalPresentationStore} from "./CodexApprovalPresentationStore"; export class CodexApprovalHandler implements ApprovalHandler { - private readonly connection: AcpClientConnection; - private readonly sessionState: SessionState; - private readonly cancellationSignal: AbortSignal | undefined; - constructor( - connection: AcpClientConnection, - sessionState: SessionState, - cancellationSignal?: AbortSignal, - ) { - this.connection = connection; - this.sessionState = sessionState; - this.cancellationSignal = cancellationSignal; - } + private readonly connection: AcpClientConnection, + private readonly sessionState: SessionState, + private readonly presentationStore: CodexApprovalPresentationStore, + private readonly cancellationSignal?: AbortSignal, + ) {} async handleCommandExecution( - params: CommandExecutionRequestApprovalParams + params: CommandExecutionRequestApprovalParams, ): Promise { + if (this.isStale(params.turnId)) return {decision: "cancel"}; + + const authoritativeParams = params as CommandParamsWithAvailableDecisions; + const decisions = commandDecisionOptions(authoritativeParams); + if (!decisions) { + logger.error("Cancelling command approval without a complete authoritative decision set", undefined); + return {decision: "cancel"}; + } + try { - const sessionId = this.sessionState.sessionId; - const acpRequest = this.buildCommandPermissionRequest(sessionId, params); - const response = await this.connection.request( - acp.methods.client.session.requestPermission, - acpRequest, - this.requestOptions(), - ); - return this.convertCommandResponse(params, response); + const response = await this.requestPermission({ + sessionId: this.sessionState.sessionId, + toolCall: commandToolCall(params), + options: decisions.map(({option}) => option), + _meta: requestPermissionMeta( + params.networkApprovalContext + ? CODEX_NETWORK_PERMISSION_TITLE + : CODEX_COMMAND_PERMISSION_TITLE, + params.reason, + ), + }); + return {decision: this.selectedDecision(response, decisions) ?? "cancel"}; } catch (error) { logger.error("Error requesting command execution permission", error); - return { decision: "cancel" }; + return {decision: "cancel"}; } } async handleFileChange( - params: FileChangeRequestApprovalParams + params: FileChangeRequestApprovalParams, ): Promise { + if (this.isStale(params.turnId)) return {decision: "cancel"}; + const decisions = fileChangeDecisionOptions(); try { - const sessionId = this.sessionState.sessionId; - const acpRequest = this.buildFileChangePermissionRequest(sessionId, params); - const response = await this.connection.request( - acp.methods.client.session.requestPermission, - acpRequest, - this.requestOptions(), - ); - return this.convertFileChangeResponse(params, response); + const response = await this.requestPermission({ + sessionId: this.sessionState.sessionId, + toolCall: fileChangeToolCall(params, this.presentationStore), + options: decisions.map(({option}) => option), + _meta: requestPermissionMeta(CODEX_FILE_CHANGE_PERMISSION_TITLE, params.reason), + }); + return {decision: this.selectedDecision(response, decisions) ?? "cancel"}; } catch (error) { logger.error("Error requesting file change permission", error); - return { decision: "cancel" }; + return {decision: "cancel"}; } } async handlePermissionsRequest( - params: PermissionsRequestApprovalParams + params: PermissionsRequestApprovalParams, ): Promise { + if (this.isStale(params.turnId)) return this.rejectPermissionsResponse(); try { - const sessionId = this.sessionState.sessionId; - const acpRequest = this.buildPermissionsRequest(sessionId, params); - const response = await this.connection.request( - acp.methods.client.session.requestPermission, - acpRequest, - this.requestOptions(), - ); - return this.convertPermissionsResponse(params, response); + const response = await this.requestPermission({ + sessionId: this.sessionState.sessionId, + toolCall: additionalPermissionsToolCall( + params.itemId, + params.cwd, + params.environmentId, + params.permissions, + ), + options: permissionProfileOptions(), + _meta: requestPermissionMeta(CODEX_ADDITIONAL_PERMISSIONS_TITLE, params.reason), + }); + return this.permissionsResponse(params.permissions, response); } catch (error) { logger.error("Error requesting permissions", error); return this.rejectPermissionsResponse(); } } - private requestOptions(): acp.SendRequestOptions | undefined { - return this.cancellationSignal ? {cancellationSignal: this.cancellationSignal} : undefined; - } - - private buildCommandPermissionRequest( - sessionId: string, - params: CommandExecutionRequestApprovalParams - ): acp.RequestPermissionRequest { - const options = this.buildCommandOptions(params).map(({ option }) => option); - return { - sessionId, - toolCall: { - toolCallId: params.itemId, - kind: "execute", - status: "pending", - rawInput: params.command ? { command: stripShellPrefix(params.command), cwd: params.cwd } : null, - }, - options, - _meta: { codex: { params } } - }; - } - - private buildFileChangePermissionRequest( - sessionId: string, - params: FileChangeRequestApprovalParams - ): acp.RequestPermissionRequest { - const options = this.buildFileChangeOptions(params).map(({ option }) => option); - return { - sessionId, - toolCall: { - toolCallId: params.itemId, - kind: "edit", - status: "pending", - }, - options, - _meta: { codex: { params } } - }; - } - - private buildPermissionsRequest( - sessionId: string, - params: PermissionsRequestApprovalParams, - ): acp.RequestPermissionRequest { - const content = this.createContent([ - params.reason, - this.formatRequestedPermissions(params.permissions), - ]); - return { - sessionId, - toolCall: { - toolCallId: params.itemId, - kind: "other", - status: "pending", - title: params.reason ?? "Permissions Request", - rawInput: params, - ...(content ? { content } : {}), - }, - options: [ - permissionOption( - ApprovalOptionId.AllowPermissionsForSession, - "Allow for Session", - "allow_always", - { decision: "allowPermissionsForSession", permissions: params.permissions }, - this.permissionGrantMetadata(params.permissions, "session"), - ), - permissionOption( - ApprovalOptionId.AllowPermissionsForTurn, - "Allow Once", - "allow_once", - { decision: "allowPermissionsForTurn", permissions: params.permissions }, - this.permissionGrantMetadata(params.permissions, "turn"), - ), - permissionOption( - ApprovalOptionId.RejectPermissions, - "Reject", - "reject_once", - { decision: "rejectPermissions" }, - ), - ], - _meta: { codex: { params } }, - }; - } - - private convertCommandResponse( - params: CommandExecutionRequestApprovalParams, - response: acp.RequestPermissionResponse - ): CommandExecutionRequestApprovalResponse { - if (response.outcome.outcome === "cancelled") { - return { decision: "cancel" }; - } - - const optionId = response.outcome.optionId; - const decision = this.buildCommandOptions(params) - .find(({ option }) => option.optionId === optionId) - ?.decision; - return { decision: decision ?? "decline" }; + private requestPermission(request: acp.RequestPermissionRequest): Promise { + return this.connection.request( + acp.methods.client.session.requestPermission, + request, + this.cancellationSignal ? {cancellationSignal: this.cancellationSignal} : undefined, + ); } - private convertFileChangeResponse( - params: FileChangeRequestApprovalParams, - response: acp.RequestPermissionResponse - ): FileChangeRequestApprovalResponse { - if (response.outcome.outcome === "cancelled") { - return { decision: "cancel" }; - } - + private selectedDecision( + response: acp.RequestPermissionResponse, + decisions: DecisionOption[], + ): T | undefined { + if (response.outcome.outcome === "cancelled") return undefined; const optionId = response.outcome.optionId; - const decision = this.buildFileChangeOptions(params) - .find(({ option }) => option.optionId === optionId) - ?.decision; - return { decision: decision ?? "decline" }; + return decisions.find(({option}) => option.optionId === optionId)?.decision; } - private convertPermissionsResponse( - params: PermissionsRequestApprovalParams, + private permissionsResponse( + permissions: RequestPermissionProfile, response: acp.RequestPermissionResponse, ): PermissionsRequestApprovalResponse { - if (response.outcome.outcome === "cancelled") { - return this.rejectPermissionsResponse(); - } - + if (response.outcome.outcome === "cancelled") return this.rejectPermissionsResponse(); switch (response.outcome.optionId) { - case ApprovalOptionId.AllowPermissionsForSession: - case ApprovalOptionId.AllowAlways: - return { - permissions: this.grantedPermissions(params.permissions), - scope: "session", - strictAutoReview: false, - }; case ApprovalOptionId.AllowPermissionsForTurn: - case ApprovalOptionId.AllowOnce: - return { - permissions: this.grantedPermissions(params.permissions), - scope: "turn", - strictAutoReview: false, - }; + return this.grantedPermissionsResponse(permissions, "turn"); + case ApprovalOptionId.AllowPermissionsForSession: + return this.grantedPermissionsResponse(permissions, "session"); + case ApprovalOptionId.RejectPermissions: default: return this.rejectPermissionsResponse(); } } - private buildCommandOptions(params: CommandExecutionRequestApprovalParams): CommandDecisionOption[] { - const options: CommandDecisionOption[] = [ - { - option: permissionOption(ApprovalOptionId.AllowOnce, "Allow Once", "allow_once", { decision: "accept" }), - decision: "accept", - }, - { - option: permissionOption( - ApprovalOptionId.AllowAlways, - params.networkApprovalContext - ? "Allow Host for Session" - : "Allow for Session", - "allow_always", - { decision: "acceptForSession" }, - params.networkApprovalContext ? { - version: 1, - changes: [{ - type: "grant", - operation: "grant", - description: `Allow access to ${params.networkApprovalContext.host} for this session`, - lifetime: {scope: "session"}, - targets: [{ - type: "network", - matcher: { - type: "host", - host: params.networkApprovalContext.host, - protocol: params.networkApprovalContext.protocol, - }, - }], - }], - } : undefined, - ), - decision: "acceptForSession", - }, - ]; - - if (params.proposedExecpolicyAmendment && params.proposedExecpolicyAmendment.length > 0) { - options.push({ - option: permissionOption( - ApprovalOptionId.AcceptWithExecpolicyAmendment, - this.execpolicyAmendmentLabel(params.proposedExecpolicyAmendment), - "allow_always", - { - decision: "acceptWithExecpolicyAmendment", - execpolicyAmendment: params.proposedExecpolicyAmendment, - }, - { - version: 1, - changes: [{ - type: "policy_rule", - operation: "add", - ruleBehavior: "allow", - description: `Allow commands starting with ${params.proposedExecpolicyAmendment.join(" ")}`, - targets: [{ - type: "command", - matcher: { - type: "argv_prefix", - argv: params.proposedExecpolicyAmendment, - }, - }], - }], - }, - ), - decision: { - acceptWithExecpolicyAmendment: { - execpolicy_amendment: params.proposedExecpolicyAmendment, - }, - }, - }); - } - - params.proposedNetworkPolicyAmendments?.forEach((amendment, index) => { - options.push({ - option: permissionOption( - this.networkPolicyAmendmentOptionId(index), - this.networkPolicyAmendmentLabel(amendment), - amendment.action === "allow" ? "allow_always" : "reject_always", - { - decision: "applyNetworkPolicyAmendment", - networkPolicyAmendment: amendment, - }, - { - version: 1, - changes: [{ - type: "policy_rule", - operation: "add", - ruleBehavior: amendment.action, - description: amendment.action === "allow" - ? `Allow access to ${amendment.host}` - : `Block access to ${amendment.host}`, - targets: [{ - type: "network", - matcher: { - type: "host", - host: amendment.host, - }, - }], - }], - }, - ), - decision: { - applyNetworkPolicyAmendment: { - network_policy_amendment: amendment, - }, - }, - }); - }); - - options.push({ - option: permissionOption(ApprovalOptionId.RejectOnce, "Reject", "reject_once", { decision: "decline" }), - decision: "decline", - }); - - return options; - } - - private buildFileChangeOptions(params: FileChangeRequestApprovalParams): FileChangeDecisionOption[] { - return [ - { - option: permissionOption(ApprovalOptionId.AllowOnce, "Allow Once", "allow_once", { decision: "accept" }), - decision: "accept", - }, - { - option: permissionOption( - ApprovalOptionId.AllowAlways, - params.grantRoot ? "Allow Root for Session" : "Allow for Session", - "allow_always", - { decision: "acceptForSession", grantRoot: params.grantRoot ?? null }, - params.grantRoot ? { - version: 1, - changes: [{ - type: "grant", - operation: "grant", - description: `Allow writes under ${params.grantRoot} for this session`, - lifetime: {scope: "session"}, - targets: [{ - type: "filesystem", - access: ["write"], - matcher: {type: "directory", path: params.grantRoot}, - }], - }], - } : undefined, - ), - decision: "acceptForSession", - }, - { - option: permissionOption(ApprovalOptionId.RejectOnce, "Reject", "reject_once", { decision: "decline" }), - decision: "decline", - }, - ]; - } - - private rejectPermissionsResponse(): PermissionsRequestApprovalResponse { - return { - permissions: {}, - scope: "turn", - strictAutoReview: true, - }; - } - - private grantedPermissions(permissions: RequestPermissionProfile): GrantedPermissionProfile { - return { - ...(permissions.network ? { network: permissions.network } : {}), - ...(permissions.fileSystem ? { fileSystem: permissions.fileSystem } : {}), - }; - } - - private permissionGrantMetadata( + private grantedPermissionsResponse( permissions: RequestPermissionProfile, scope: "turn" | "session", - ): PermissionMetadata | undefined { - const changes: Array> = []; - const lifetime = {scope}; - const suffix = scope === "session" ? " for this session" : " for this turn"; - - if (permissions.network?.enabled !== null && permissions.network?.enabled !== undefined) { - const allowed = permissions.network.enabled; - changes.push({ - type: allowed ? "grant" : "policy_rule", - operation: allowed ? "grant" : "add", - ...(allowed ? {} : {ruleBehavior: "deny"}), - description: `${allowed ? "Allow" : "Deny"} network access${suffix}`, - lifetime, - targets: [{type: "network", matcher: {type: "any"}}], - }); - } - - const fileSystem = permissions.fileSystem; - for (const path of fileSystem?.read ?? []) { - changes.push(this.fileSystemGrantChange(path, "read", lifetime, suffix)); - } - for (const path of fileSystem?.write ?? []) { - changes.push(this.fileSystemGrantChange(path, "write", lifetime, suffix)); - } - for (const entry of fileSystem?.entries ?? []) { - const matcher = (() => { - switch (entry.path.type) { - case "path": - return {type: "exact_path", path: entry.path.path}; - case "glob_pattern": - return {type: "glob", pattern: entry.path.pattern}; - case "special": - return {type: "special", provider: "codex", value: entry.path.value}; - } - })(); - const pathDescription = entry.path.type === "path" - ? entry.path.path - : entry.path.type === "glob_pattern" ? entry.path.pattern : JSON.stringify(entry.path.value); - changes.push({ - type: entry.access === "deny" ? "policy_rule" : "grant", - operation: entry.access === "deny" ? "add" : "grant", - ...(entry.access === "deny" ? {ruleBehavior: "deny"} : {}), - description: entry.access === "deny" - ? `Deny filesystem access to ${pathDescription}${suffix}` - : `Allow ${entry.access} access to ${pathDescription}${suffix}`, - lifetime, - targets: [{ - type: "filesystem", - ...(entry.access === "deny" ? {} : {access: [entry.access]}), - matcher, - }], - }); - } - - return changes.length > 0 ? {version: 1, changes} : undefined; - } - - private fileSystemGrantChange( - path: string, - access: "read" | "write", - lifetime: {scope: "turn" | "session"}, - suffix: string, - ): Record { + ): PermissionsRequestApprovalResponse { return { - type: "grant", - operation: "grant", - description: `Allow ${access} access to ${path}${suffix}`, - lifetime, - targets: [{ - type: "filesystem", - access: [access], - matcher: {type: "exact_path", path}, - }], + permissions: this.grantedPermissions(permissions), + scope, + strictAutoReview: false, }; } - private networkPolicyAmendmentOptionId(index: number): string { - return `${ApprovalOptionId.ApplyNetworkPolicyAmendment}:${index}`; - } - - private execpolicyAmendmentLabel(amendment: string[]): string { - const commandPrefix = amendment.join(" "); - if (!commandPrefix || commandPrefix.includes("\n") || commandPrefix.includes("\r")) { - return "Allow and Remember Command Pattern"; - } - return `Allow Commands Starting With \`${commandPrefix}\``; - } - - private networkPolicyAmendmentLabel(amendment: NetworkPolicyAmendment): string { - return amendment.action === "allow" - ? `Allow ${amendment.host} in the Future` - : `Block ${amendment.host} in the Future`; + private rejectPermissionsResponse(): PermissionsRequestApprovalResponse { + return {permissions: {}, scope: "turn", strictAutoReview: true}; } - private formatRequestedPermissions(permissions: RequestPermissionProfile): string | null { - const content: string[] = []; - if (permissions.network?.enabled !== undefined && permissions.network.enabled !== null) { - content.push(`Network Access: ${permissions.network.enabled}`); - } - if (permissions.fileSystem?.read?.length) { - content.push(`File System Read Access: ${permissions.fileSystem.read.join(", ")}`); - } - if (permissions.fileSystem?.write?.length) { - content.push(`File System Write Access: ${permissions.fileSystem.write.join(", ")}`); - } - if (permissions.fileSystem?.entries?.length) { - content.push(`File System Entries: ${JSON.stringify(permissions.fileSystem.entries)}`); - } - return content.length > 0 ? content.join("\n\n") : null; + private grantedPermissions(permissions: RequestPermissionProfile): GrantedPermissionProfile { + return { + ...(permissions.network ? {network: permissions.network} : {}), + ...(permissions.fileSystem ? {fileSystem: permissions.fileSystem} : {}), + }; } - private createContent(lines: Array): acp.ToolCallContent[] | undefined { - const text = lines.filter((line): line is string => !!line).join("\n\n"); - if (!text) return undefined; - return [{ - type: "content", - content: { - type: "text", - text, - }, - }]; + private isStale(turnId: string): boolean { + return this.sessionState.currentTurnId !== null && this.sessionState.currentTurnId !== turnId; } } diff --git a/src/CodexApprovalOptions.ts b/src/CodexApprovalOptions.ts new file mode 100644 index 00000000..fd23fd1e --- /dev/null +++ b/src/CodexApprovalOptions.ts @@ -0,0 +1,198 @@ +import type * as acp from "@agentclientprotocol/sdk"; +import type { + CommandExecutionApprovalDecision, + CommandExecutionRequestApprovalParams, + FileChangeApprovalDecision, + NetworkPolicyAmendment, +} from "./app-server/v2"; +import {ApprovalOptionId} from "./ApprovalOptionId"; +import {optionPermissionMeta} from "./CodexPermissionMetadata"; + +export type DecisionOption = { + option: acp.PermissionOption; + decision: T; +}; + +export type CommandParamsWithAvailableDecisions = CommandExecutionRequestApprovalParams & { + availableDecisions?: unknown; +}; + +export function commandDecisionOptions( + params: CommandParamsWithAvailableDecisions, +): DecisionOption[] | undefined { + const decisions = parseAvailableCommandDecisions(params); + if (!decisions) return undefined; + + const options: DecisionOption[] = []; + let networkIndex = 0; + for (const decision of decisions) { + if (decision === "cancel") continue; + if (decision === "accept") { + options.push(decisionOption(ApprovalOptionId.AllowOnce, "Allow once", "allow_once", decision)); + continue; + } + if (decision === "acceptForSession") { + options.push(decisionOption( + ApprovalOptionId.AllowAlways, + "Allow for session", + "allow_always", + decision, + "Remember this approval until the Codex session ends", + )); + continue; + } + if (decision === "decline") { + options.push(decisionOption(ApprovalOptionId.RejectOnce, "Reject", "reject_once", decision)); + continue; + } + if ("acceptWithExecpolicyAmendment" in decision) { + options.push(decisionOption( + ApprovalOptionId.AcceptWithExecpolicyAmendment, + "Allow command pattern", + "allow_always", + decision, + "Add the proposed command-prefix rule to persistent Codex policy", + )); + continue; + } + options.push(decisionOption( + `${ApprovalOptionId.ApplyNetworkPolicyAmendment}:${networkIndex++}`, + decision.applyNetworkPolicyAmendment.network_policy_amendment.action === "allow" + ? "Allow in future" + : "Block in future", + decision.applyNetworkPolicyAmendment.network_policy_amendment.action === "allow" + ? "allow_always" + : "reject_always", + decision, + decision.applyNetworkPolicyAmendment.network_policy_amendment.action === "allow" + ? "Add the proposed allow rule to persistent Codex network policy" + : "Add the proposed block rule to persistent Codex network policy", + )); + } + + const hasAllow = options.some(({option}) => option.kind === "allow_once" || option.kind === "allow_always"); + const hasReject = options.some(({decision}) => decision === "decline"); + return hasAllow && hasReject ? options : undefined; +} + +export function fileChangeDecisionOptions(): DecisionOption[] { + return [ + decisionOption(ApprovalOptionId.AllowOnce, "Allow once", "allow_once", "accept"), + decisionOption( + ApprovalOptionId.AllowAlways, + "Allow for session", + "allow_always", + "acceptForSession", + "Remember this approval until the Codex session ends", + ), + decisionOption(ApprovalOptionId.RejectOnce, "Reject", "reject_once", "decline"), + ]; +} + +export function permissionProfileOptions(): acp.PermissionOption[] { + return [ + permissionOption( + ApprovalOptionId.AllowPermissionsForTurn, + "Allow once", + "allow_once", + "Grant the complete requested permission profile for this turn", + ), + permissionOption( + ApprovalOptionId.AllowPermissionsForSession, + "Allow for session", + "allow_always", + "Grant the complete requested permission profile until the Codex session ends", + ), + permissionOption(ApprovalOptionId.RejectPermissions, "Reject", "reject_once"), + ]; +} + +function parseAvailableCommandDecisions( + params: CommandParamsWithAvailableDecisions, +): CommandExecutionApprovalDecision[] | undefined { + if (!Array.isArray(params.availableDecisions) || params.availableDecisions.length === 0) { + return undefined; + } + const decisions: CommandExecutionApprovalDecision[] = []; + for (const candidate of params.availableDecisions) { + const decision = parseCommandDecision(candidate, params); + if (!decision) return undefined; + decisions.push(decision); + } + return decisions; +} + +function parseCommandDecision( + candidate: unknown, + params: CommandExecutionRequestApprovalParams, +): CommandExecutionApprovalDecision | undefined { + if (candidate === "accept" || candidate === "acceptForSession" || candidate === "decline" || candidate === "cancel") { + return candidate; + } + if (!isRecord(candidate)) return undefined; + + if ("acceptWithExecpolicyAmendment" in candidate) { + const value = candidate["acceptWithExecpolicyAmendment"]; + if (!isRecord(value)) return undefined; + const amendment = value["execpolicy_amendment"]; + if (!isStringArray(amendment) || amendment.length === 0) return undefined; + if (!sameStrings(amendment, params.proposedExecpolicyAmendment)) return undefined; + return {acceptWithExecpolicyAmendment: {execpolicy_amendment: [...amendment]}}; + } + + if ("applyNetworkPolicyAmendment" in candidate) { + const value = candidate["applyNetworkPolicyAmendment"]; + if (!isRecord(value)) return undefined; + const amendment = parseNetworkAmendment(value["network_policy_amendment"]); + if (!amendment || !params.networkApprovalContext) return undefined; + if (amendment.host !== params.networkApprovalContext.host) return undefined; + if (!(params.proposedNetworkPolicyAmendments ?? []).some(proposed => sameNetworkAmendment(proposed, amendment))) { + return undefined; + } + return {applyNetworkPolicyAmendment: {network_policy_amendment: amendment}}; + } + return undefined; +} + +function decisionOption( + optionId: string, + name: string, + kind: acp.PermissionOptionKind, + decision: T, + description?: string, +): DecisionOption { + return {option: permissionOption(optionId, name, kind, description), decision}; +} + +function permissionOption( + optionId: string, + name: string, + kind: acp.PermissionOptionKind, + description?: string, +): acp.PermissionOption { + const meta = optionPermissionMeta(description); + return {optionId, name, kind, ...(meta ? {_meta: meta} : {})}; +} + +function parseNetworkAmendment(value: unknown): NetworkPolicyAmendment | undefined { + if (!isRecord(value) || typeof value["host"] !== "string") return undefined; + const action = value["action"]; + if (action !== "allow" && action !== "deny") return undefined; + return {host: value["host"], action}; +} + +function isRecord(value: unknown): value is Record { + return value !== null && typeof value === "object" && !Array.isArray(value); +} + +function isStringArray(value: unknown): value is string[] { + return Array.isArray(value) && value.every(entry => typeof entry === "string"); +} + +function sameStrings(left: readonly string[], right?: readonly string[] | null): boolean { + return !!right && left.length === right.length && left.every((value, index) => value === right[index]); +} + +function sameNetworkAmendment(left: NetworkPolicyAmendment, right: NetworkPolicyAmendment): boolean { + return left.host === right.host && left.action === right.action; +} diff --git a/src/CodexApprovalPresentationStore.ts b/src/CodexApprovalPresentationStore.ts new file mode 100644 index 00000000..14158140 --- /dev/null +++ b/src/CodexApprovalPresentationStore.ts @@ -0,0 +1,33 @@ +import type {ServerNotification} from "./app-server"; +import type {ThreadItem} from "./app-server/v2"; + +type FileChangeItem = ThreadItem & {type: "fileChange"}; + +/** Prompt-lifetime presentation data that app-server approval params do not repeat. */ +export class CodexApprovalPresentationStore { + private readonly fileChanges = new Map(); + + handleNotification(notification: ServerNotification): void { + switch (notification.method) { + case "item/started": + if (notification.params.item.type === "fileChange") { + this.fileChanges.set(notification.params.item.id, notification.params.item); + } + return; + case "item/completed": + if (notification.params.item.type === "fileChange") { + this.fileChanges.delete(notification.params.item.id); + } + return; + case "turn/completed": + this.fileChanges.clear(); + return; + default: + return; + } + } + + fileChange(itemId: string): FileChangeItem | undefined { + return this.fileChanges.get(itemId); + } +} diff --git a/src/CodexPermissionMetadata.ts b/src/CodexPermissionMetadata.ts new file mode 100644 index 00000000..00b0b52a --- /dev/null +++ b/src/CodexPermissionMetadata.ts @@ -0,0 +1,49 @@ +import type * as acp from "@agentclientprotocol/sdk"; + +export const CODEX_COMMAND_PERMISSION_TITLE = "Run command?"; +export const CODEX_NETWORK_PERMISSION_TITLE = "Allow network access?"; +export const CODEX_FILE_CHANGE_PERMISSION_TITLE = "Make edits?"; +export const CODEX_ADDITIONAL_PERMISSIONS_TITLE = "Grant permissions?"; + +type RequestPermissionMetadata = { + version: 1; + title: string; + description?: string; +}; + +type OptionPermissionMetadata = { + version: 1; + description: string; +}; + +export function requestPermissionMeta( + title: string, + reason?: string | null, +): NonNullable { + const description = nonBlank(reason); + const permission: RequestPermissionMetadata = { + version: 1, + title, + ...(description ? {description} : {}), + }; + return {permission}; +} + +export function optionPermissionMeta( + description?: string | null, +): acp.PermissionOption["_meta"] | undefined { + const normalized = nonBlank(description); + if (!normalized) { + return undefined; + } + const permission: OptionPermissionMetadata = { + version: 1, + description: normalized, + }; + return {permission}; +} + +function nonBlank(value?: string | null): string | undefined { + const normalized = value?.trim(); + return normalized ? normalized : undefined; +} diff --git a/src/CodexPermissionPresentation.ts b/src/CodexPermissionPresentation.ts new file mode 100644 index 00000000..b06f9c3f --- /dev/null +++ b/src/CodexPermissionPresentation.ts @@ -0,0 +1,141 @@ +import type * as acp from "@agentclientprotocol/sdk"; +import type { + CommandAction, + CommandExecutionRequestApprovalParams, + FileChangeRequestApprovalParams, + RequestPermissionProfile, + ThreadItem, +} from "./app-server/v2"; +import {stripShellPrefix} from "./CodexEventHandler"; +import type {CodexApprovalPresentationStore} from "./CodexApprovalPresentationStore"; + +type FileChangeItem = ThreadItem & {type: "fileChange"}; + +export function commandToolCall( + params: CommandExecutionRequestApprovalParams, +): acp.ToolCallUpdate { + const network = params.networkApprovalContext; + const rawInput = { + ...(params.command ? {command: stripShellPrefix(params.command)} : {}), + ...(params.cwd ? {cwd: params.cwd} : {}), + }; + return { + toolCallId: params.itemId, + kind: "execute", + status: "pending", + title: network + ? `${network.protocol} network access to ${network.host}` + : commandTitle(params.commandActions), + ...(Object.keys(rawInput).length > 0 ? {rawInput} : {}), + ...locationsField(commandActionPaths(params.commandActions)), + ...(network ? {content: [textContent(`${network.protocol} access to ${network.host}`)]} : {}), + }; +} + +export function fileChangeToolCall( + params: FileChangeRequestApprovalParams, + store: CodexApprovalPresentationStore, +): acp.ToolCallUpdate { + const item = store.fileChange(params.itemId); + return { + toolCallId: params.itemId, + kind: "edit", + status: "pending", + title: "Edit files", + ...locationsField(fileChangePaths(item)), + }; +} + +export function additionalPermissionsToolCall( + itemId: string, + cwd: string, + environmentId: string | null, + permissions: RequestPermissionProfile, +): acp.ToolCallUpdate { + const content = permissionProfileContent(permissions); + return { + toolCallId: itemId, + kind: "other", + status: "pending", + title: "Additional sandbox permissions", + rawInput: {permissions, cwd, environmentId}, + ...locationsField(permissionProfilePaths(permissions)), + ...(content.length > 0 ? {content} : {}), + }; +} + +function commandTitle(actions?: CommandAction[] | null): string { + const first = actions?.[0]; + if (!first) return "Run command"; + switch (first.type) { + case "read": + return actions?.length === 1 ? "Read file" : "Run command with file reads"; + case "listFiles": + return "List files"; + case "search": + return "Search files"; + case "unknown": + return "Run command"; + } +} + +function commandActionPaths(actions?: CommandAction[] | null): string[] { + return unique((actions ?? []).flatMap(action => { + switch (action.type) { + case "read": + return [action.path]; + case "listFiles": + case "search": + return action.path ? [action.path] : []; + case "unknown": + return []; + } + })); +} + +function fileChangePaths(item?: FileChangeItem): string[] { + return unique(item?.changes.map(change => change.path) ?? []); +} + +function permissionProfilePaths(permissions: RequestPermissionProfile): string[] { + const fileSystem = permissions.fileSystem; + return unique([ + ...(fileSystem?.read ?? []), + ...(fileSystem?.write ?? []), + ...(fileSystem?.entries ?? []).flatMap(entry => + entry.path.type === "path" ? [entry.path.path] : []), + ]); +} + +function permissionProfileContent(permissions: RequestPermissionProfile): acp.ToolCallContent[] { + const lines: string[] = []; + const networkEnabled = permissions.network?.enabled; + if (networkEnabled !== null && networkEnabled !== undefined) { + lines.push(networkEnabled ? "Enable network access" : "Disable network access"); + } + for (const entry of permissions.fileSystem?.entries ?? []) { + switch (entry.path.type) { + case "glob_pattern": + lines.push(`${entry.access} filesystem pattern ${entry.path.pattern}`); + break; + case "special": + lines.push(`${entry.access} Codex filesystem scope ${JSON.stringify(entry.path.value)}`); + break; + case "path": + break; + } + } + return lines.length > 0 ? [textContent(lines.join("\n"))] : []; +} + +function locationsField(paths: string[]): Pick | object { + return paths.length > 0 ? {locations: paths.map(path => ({path}))} : {}; +} + +function textContent(text: string): acp.ToolCallContent { + return {type: "content", content: {type: "text", text}}; +} + +function unique(values: string[]): string[] { + return [...new Set(values)]; +} diff --git a/src/__tests__/CodexACPAgent/approval-events.test.ts b/src/__tests__/CodexACPAgent/approval-events.test.ts index 821f23bc..5ec73fae 100644 --- a/src/__tests__/CodexACPAgent/approval-events.test.ts +++ b/src/__tests__/CodexACPAgent/approval-events.test.ts @@ -1,17 +1,22 @@ -import { describe, it, expect, vi, beforeEach } from 'vitest'; +import {beforeEach, describe, expect, it, vi} from "vitest"; import type { + CommandExecutionApprovalDecision, CommandExecutionRequestApprovalParams, FileChangeRequestApprovalParams, PermissionsRequestApprovalParams, -} from '../../app-server/v2'; -import { createCodexMockTestFixture, createTestSessionState, type CodexMockTestFixture } from '../acp-test-utils'; -import type { SessionState } from '../../CodexAcpServer'; +} from "../../app-server/v2"; +import {createCodexMockTestFixture, createTestSessionState, type CodexMockTestFixture} from "../acp-test-utils"; +import type {SessionState} from "../../CodexAcpServer"; import {AgentMode} from "../../AgentMode"; import {ApprovalOptionId} from "../../ApprovalOptionId"; -describe('Approval Events', () => { +type CommandParams = CommandExecutionRequestApprovalParams & { + availableDecisions?: unknown; +}; + +describe("Approval Events", () => { let fixture: CodexMockTestFixture; - const sessionId = 'test-session-id'; + const sessionId = "test-session-id"; beforeEach(() => { fixture = createCodexMockTestFixture(); @@ -20,659 +25,432 @@ describe('Approval Events', () => { function setupSessionWithPendingPrompt() { const codexAcpAgent = fixture.getCodexAcpAgent(); - - let resolveTurnCompleted: (value: { threadId: string; turn: { id: string; items: never[]; status: string; error: null } }) => void; - const turnCompletedPromise = new Promise<{ threadId: string; turn: { id: string; items: never[]; status: string; error: null } }>((resolve) => { + let resolveTurnCompleted!: (value: { + threadId: string; + turn: {id: string; items: never[]; status: string; error: null}; + }) => void; + const turnCompletedPromise = new Promise<{ + threadId: string; + turn: {id: string; items: never[]; status: string; error: null}; + }>(resolve => { resolveTurnCompleted = resolve; }); - fixture.getCodexAppServerClient().turnStart = vi.fn().mockResolvedValue({ - turn: { id: "turn-id", items: [], status: "inProgress", error: null } + turn: {id: "turn-1", items: [], status: "inProgress", error: null}, }); fixture.getCodexAppServerClient().awaitTurnCompleted = vi.fn().mockReturnValue(turnCompletedPromise); - const sessionState: SessionState = createTestSessionState({ sessionId, - currentModelId: 'model-id[effort]', - agentMode: AgentMode.DEFAULT_AGENT_MODE + currentModelId: "model-id[effort]", + agentMode: AgentMode.DEFAULT_AGENT_MODE, }); - vi.spyOn(codexAcpAgent, 'getSessionState').mockReturnValue(sessionState); - + vi.spyOn(codexAcpAgent, "getSessionState").mockReturnValue(sessionState); const promptPromise = codexAcpAgent.prompt({ sessionId, - prompt: [{ type: 'text', text: 'Test prompt' }] + prompt: [{type: "text", text: "Test prompt"}], }); - return { + sessionState, promptPromise, - completeTurn: () => resolveTurnCompleted!({ + completeTurn: () => resolveTurnCompleted({ threadId: sessionId, - turn: { id: "turn-id", items: [], status: "completed", error: null } - }) + turn: {id: "turn-1", items: [], status: "completed", error: null}, + }), }; } - describe('Command execution approval', () => { - const commandApprovalCases = [ - { optionId: 'allow_once', expectedDecision: 'accept', description: 'allow once' }, - { optionId: 'allow_always', expectedDecision: 'acceptForSession', description: 'allow for session' }, - { optionId: 'reject_once', expectedDecision: 'decline', description: 'reject' }, - ] as const; - - it.each(commandApprovalCases)( - 'should map $optionId to $expectedDecision ($description)', - async ({ optionId, expectedDecision }) => { - const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); - fixture.setPermissionResponse({ - outcome: { outcome: 'selected', optionId } - }); - - const params: CommandExecutionRequestApprovalParams = { - threadId: sessionId, - turnId: 'turn-1', - itemId: `item-${optionId}`, - reason: 'Test command', - startedAtMs: 0, - environmentId: null, - proposedExecpolicyAmendment: null, - }; - - const response = await fixture.sendServerRequest( - 'item/commandExecution/requestApproval', - params - ); - - expect(response).toEqual({ decision: expectedDecision }); - - completeTurn(); - await promptPromise; - } - ); - - it('should handle cancelled permission dialog', async () => { - const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); - fixture.setPermissionResponse({ - outcome: { outcome: 'cancelled' } - }); - - const params: CommandExecutionRequestApprovalParams = { - threadId: sessionId, - turnId: 'turn-1', - startedAtMs: 0, - environmentId: null, - itemId: 'item-cancelled', - reason: null, - proposedExecpolicyAmendment: null, - }; - - const response = await fixture.sendServerRequest( - 'item/commandExecution/requestApproval', - params - ); + function commandParams( + availableDecisions: CommandExecutionApprovalDecision[] | unknown, + overrides: Partial = {}, + ): CommandParams { + return { + threadId: sessionId, + turnId: "turn-1", + itemId: "command-item", + startedAtMs: 0, + environmentId: "local", + command: "/bin/zsh -c npm test", + cwd: "/workspace", + reason: "Needed to verify the changes.", + availableDecisions, + ...overrides, + }; + } - expect(response).toEqual({ decision: 'cancel' }); + function permissionRequest() { + return fixture.getAcpConnectionEvents([]).find(event => event.method === "requestPermission")?.args[0]; + } - completeTurn(); - await promptPromise; - }); + async function finish(prompt: ReturnType): Promise { + prompt.completeTurn(); + await prompt.promptPromise; + } - it('should map execpolicy amendment approval to the exact app-server decision', async () => { - const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); - fixture.setPermissionResponse({ - outcome: { outcome: 'selected', optionId: ApprovalOptionId.AcceptWithExecpolicyAmendment } + describe("command approvals", () => { + it("emits an autonomous ACP v1 snapshot and maps explicit reject to decline", async () => { + const prompt = setupSessionWithPendingPrompt(); + fixture.setPermissionResponse({outcome: {outcome: "selected", optionId: ApprovalOptionId.RejectOnce}}); + const params = commandParams(["accept", "acceptForSession", "decline", "cancel"], { + commandActions: [ + {type: "read", command: "cat src/a.ts", name: "cat", path: "/workspace/src/a.ts"}, + {type: "search", command: "rg TODO src", query: "TODO", path: "/workspace/src"}, + ], }); - const proposedExecpolicyAmendment = ['npm', 'install']; - const params: CommandExecutionRequestApprovalParams = { - threadId: sessionId, - turnId: 'turn-1', - itemId: 'item-execpolicy-amendment', - startedAtMs: 0, - environmentId: null, - reason: 'Installing dependencies', - command: 'npm install', - cwd: '/home/user/project', - proposedExecpolicyAmendment, - }; - - const response = await fixture.sendServerRequest( - 'item/commandExecution/requestApproval', - params + const response = await fixture.sendServerRequest<{decision: unknown}>( + "item/commandExecution/requestApproval", + params, ); - expect(response).toEqual({ - decision: { - acceptWithExecpolicyAmendment: { - execpolicy_amendment: proposedExecpolicyAmendment, - }, + expect(response).toEqual({decision: "decline"}); + expect(permissionRequest()).toEqual({ + sessionId, + toolCall: { + toolCallId: "command-item", + kind: "execute", + status: "pending", + title: "Run command with file reads", + rawInput: {command: "npm test", cwd: "/workspace"}, + locations: [{path: "/workspace/src/a.ts"}, {path: "/workspace/src"}], }, - }); - - const requestEvent = fixture.getAcpConnectionEvents([])[0]; - expect(requestEvent).toBeDefined(); - const request = requestEvent!.args[0]; - expect(request.options).toContainEqual( - expect.objectContaining({ - optionId: ApprovalOptionId.AcceptWithExecpolicyAmendment, - kind: 'allow_always', - _meta: { - permission: { + options: [ + {optionId: "allow_once", name: "Allow once", kind: "allow_once"}, + { + optionId: "allow_always", + name: "Allow for session", + kind: "allow_always", + _meta: {permission: { version: 1, - changes: [{ - type: 'policy_rule', - operation: 'add', - ruleBehavior: 'allow', - description: 'Allow commands starting with npm install', - targets: [{ - type: 'command', - matcher: { - type: 'argv_prefix', - argv: proposedExecpolicyAmendment, - }, - }], - }], - }, - codex: expect.objectContaining({ - execpolicyAmendment: proposedExecpolicyAmendment, - }), + description: "Remember this approval until the Codex session ends", + }}, }, - }) - ); - - completeTurn(); - await promptPromise; - }); - - it('should map network policy amendment approval to the exact app-server decision', async () => { - const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); - const optionId = `${ApprovalOptionId.ApplyNetworkPolicyAmendment}:0`; - fixture.setPermissionResponse({ - outcome: { outcome: 'selected', optionId } + {optionId: "reject_once", name: "Reject", kind: "reject_once"}, + ], + _meta: {permission: { + version: 1, + title: "Run command?", + description: "Needed to verify the changes.", + }}, }); - - const networkPolicyAmendment = { host: 'registry.npmjs.org', action: 'allow' as const }; - const params: CommandExecutionRequestApprovalParams = { - threadId: sessionId, - turnId: 'turn-1', - itemId: 'item-network-policy-amendment', - startedAtMs: 0, - environmentId: null, - reason: 'Needs network access', - networkApprovalContext: { host: 'registry.npmjs.org', protocol: 'https' }, - proposedNetworkPolicyAmendments: [networkPolicyAmendment], - }; - - const response = await fixture.sendServerRequest( - 'item/commandExecution/requestApproval', - params - ); - - expect(response).toEqual({ - decision: { - applyNetworkPolicyAmendment: { - network_policy_amendment: networkPolicyAmendment, - }, - }, - }); - - const requestEvent = fixture.getAcpConnectionEvents([])[0]; - expect(requestEvent).toBeDefined(); - const request = requestEvent!.args[0]; - expect(request.options).toContainEqual( - expect.objectContaining({ - optionId, - kind: 'allow_always', - _meta: { - permission: { - version: 1, - changes: [{ - type: 'policy_rule', - operation: 'add', - ruleBehavior: 'allow', - description: 'Allow access to registry.npmjs.org', - targets: [{ - type: 'network', - matcher: { - type: 'host', - host: 'registry.npmjs.org', - }, - }], - }], - }, - codex: expect.objectContaining({ - networkPolicyAmendment, - }), - }, - }) - ); - - completeTurn(); - await promptPromise; - }); - - it('should return cancel when no handler registered', async () => { - const params: CommandExecutionRequestApprovalParams = { - threadId: 'non-existent-session', - turnId: 'turn-1', - startedAtMs: 0, - environmentId: null, - itemId: 'item-no-handler', - reason: null, - proposedExecpolicyAmendment: null, - }; - - const response = await fixture.sendServerRequest( - 'item/commandExecution/requestApproval', - params - ); - - expect(response).toEqual({ decision: 'cancel' }); + for (const option of permissionRequest().options) { + if (option._meta?.permission) { + expect(option._meta.permission).not.toHaveProperty("changes"); + } + } + expect(JSON.stringify(permissionRequest())).not.toContain("exact_command"); + expect(JSON.stringify(permissionRequest())).not.toContain("codex"); + await finish(prompt); }); - it('should convert to ACP permission request format', async () => { - const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); - fixture.setPermissionResponse({ - outcome: { outcome: 'selected', optionId: 'allow_once' } - }); - - const params: CommandExecutionRequestApprovalParams = { - threadId: sessionId, - turnId: 'turn-1', - startedAtMs: 0, - environmentId: null, - itemId: 'item-snapshot', - reason: 'Running npm install', - proposedExecpolicyAmendment: null, - }; - - await fixture.sendServerRequest( - 'item/commandExecution/requestApproval', - params + it("maps ACP cancellation to cancel, not decline", async () => { + const prompt = setupSessionWithPendingPrompt(); + fixture.setPermissionResponse({outcome: {outcome: "cancelled"}}); + const response = await fixture.sendServerRequest<{decision: unknown}>( + "item/commandExecution/requestApproval", + commandParams(["accept", "decline", "cancel"]), ); - - await expect(fixture.getAcpConnectionDump([])).toMatchFileSnapshot( - 'data/approval-command-allow-once.json' - ); - - completeTurn(); - await promptPromise; + expect(response).toEqual({decision: "cancel"}); + await finish(prompt); }); - it('should include rawInput with command and cwd', async () => { - const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); + it("retains and returns the exact proposed exec-policy payload without exposing argv in prose", async () => { + const prompt = setupSessionWithPendingPrompt(); fixture.setPermissionResponse({ - outcome: { outcome: 'selected', optionId: 'allow_once' } + outcome: {outcome: "selected", optionId: ApprovalOptionId.AcceptWithExecpolicyAmendment}, }); - - const params: CommandExecutionRequestApprovalParams = { - threadId: sessionId, - turnId: 'turn-1', - startedAtMs: 0, - environmentId: null, - itemId: 'item-with-command', - reason: 'Installing dependencies', - command: 'npm install', - cwd: '/home/user/project', - proposedExecpolicyAmendment: null, - }; - - await fixture.sendServerRequest( - 'item/commandExecution/requestApproval', - params + const amendment = ["C:\\Program Files\\PowerShell\\7\\pwsh.exe", "-Command", "@'\nsecret\n'@"]; + const decision = {acceptWithExecpolicyAmendment: {execpolicy_amendment: amendment}} as const; + const response = await fixture.sendServerRequest<{decision: unknown}>( + "item/commandExecution/requestApproval", + commandParams(["accept", decision, "decline", "cancel"], { + command: amendment.join(" "), + proposedExecpolicyAmendment: amendment, + }), ); - - await expect(fixture.getAcpConnectionDump([])).toMatchFileSnapshot( - 'data/approval-command-with-rawInput.json' + expect(response).toEqual({decision}); + const option = permissionRequest().options.find( + (candidate: {optionId: string}) => candidate.optionId === ApprovalOptionId.AcceptWithExecpolicyAmendment, ); - - completeTurn(); - await promptPromise; - }); - - it.each([ - { command: '/bin/zsh -c npm install', expected: 'npm install' }, - { command: '/bin/bash -lc npm install', expected: 'npm install' }, - { command: 'zsh npm install', expected: 'npm install' }, - { command: 'sh -c ls -la', expected: 'ls -la' }, - { command: 'npm install', expected: 'npm install' }, - { command: "/bin/bash -lc './tests.cmd -Darg=value'", expected: './tests.cmd -Darg=value' }, - { command: "/bin/zsh -c 'echo hello'", expected: 'echo hello' }, - ])('should strip shell prefix from "$command" in rawInput', async ({ command, expected }) => { - const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); - fixture.setPermissionResponse({ - outcome: { outcome: 'selected', optionId: 'allow_once' } + expect(option).toEqual({ + optionId: ApprovalOptionId.AcceptWithExecpolicyAmendment, + name: "Allow command pattern", + kind: "allow_always", + _meta: {permission: { + version: 1, + description: "Add the proposed command-prefix rule to persistent Codex policy", + }}, }); - - const params: CommandExecutionRequestApprovalParams = { - threadId: sessionId, - turnId: 'turn-1', - startedAtMs: 0, - environmentId: null, - itemId: 'item-shell-prefix', - reason: 'Installing dependencies', - command, - cwd: '/home/user/project', - proposedExecpolicyAmendment: null, - }; - - await fixture.sendServerRequest( - 'item/commandExecution/requestApproval', - params - ); - - const dump = fixture.getAcpConnectionDump(['_meta']); - const parsed = JSON.parse(dump); - expect(parsed.args[0].toolCall.rawInput.command).toBe(expected); - - completeTurn(); - await promptPromise; + expect(JSON.stringify(option)).not.toContain("secret"); + await finish(prompt); }); - }); - - describe('File change approval', () => { - const fileChangeApprovalCases = [ - { optionId: 'allow_once', expectedDecision: 'accept', description: 'allow once' }, - { optionId: 'allow_always', expectedDecision: 'acceptForSession', description: 'allow for session' }, - { optionId: 'reject_once', expectedDecision: 'decline', description: 'reject' }, - ] as const; - it.each(fileChangeApprovalCases)( - 'should map $optionId to $expectedDecision ($description)', - async ({ optionId, expectedDecision }) => { - const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); + it.each(["http", "https", "socks5Tcp", "socks5Udp"] as const)( + "keeps %s host/protocol in the tool subject and maps the exact network amendment", + async protocol => { + const prompt = setupSessionWithPendingPrompt(); + const amendment = {host: "example.test", action: "allow" as const}; + const decision = {applyNetworkPolicyAmendment: {network_policy_amendment: amendment}} as const; fixture.setPermissionResponse({ - outcome: { outcome: 'selected', optionId } + outcome: {outcome: "selected", optionId: `${ApprovalOptionId.ApplyNetworkPolicyAmendment}:0`}, }); - - const params: FileChangeRequestApprovalParams = { - threadId: sessionId, - turnId: 'turn-1', - startedAtMs: 0, - itemId: `file-change-${optionId}`, - reason: 'Test file change', - grantRoot: null, - }; - - const response = await fixture.sendServerRequest( - 'item/fileChange/requestApproval', - params + const response = await fixture.sendServerRequest<{decision: unknown}>( + "item/commandExecution/requestApproval", + commandParams(["accept", "acceptForSession", decision, "decline", "cancel"], { + networkApprovalContext: {host: amendment.host, protocol}, + proposedNetworkPolicyAmendments: [amendment], + }), ); - - expect(response).toEqual({ decision: expectedDecision }); - - completeTurn(); - await promptPromise; - } + expect(response).toEqual({decision}); + expect(permissionRequest()._meta).toEqual({permission: { + version: 1, + title: "Allow network access?", + description: "Needed to verify the changes.", + }}); + expect(permissionRequest().toolCall).toMatchObject({ + title: `${protocol} network access to example.test`, + content: [{type: "content", content: {type: "text", text: `${protocol} access to example.test`}}], + }); + const optionsJson = JSON.stringify(permissionRequest().options); + expect(optionsJson).not.toContain("example.test"); + expect(optionsJson).not.toContain("exact"); + await finish(prompt); + }, ); - it('should handle cancelled file change dialog', async () => { - const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); + it("maps a selected persistent network block to its exact provider decision", async () => { + const prompt = setupSessionWithPendingPrompt(); + const amendment = {host: "blocked.test", action: "deny" as const}; + const decision = {applyNetworkPolicyAmendment: {network_policy_amendment: amendment}} as const; fixture.setPermissionResponse({ - outcome: { outcome: 'cancelled' } + outcome: {outcome: "selected", optionId: `${ApprovalOptionId.ApplyNetworkPolicyAmendment}:0`}, }); - - const params: FileChangeRequestApprovalParams = { - threadId: sessionId, - turnId: 'turn-1', - startedAtMs: 0, - itemId: 'file-change-cancelled', - reason: null, - grantRoot: null, - }; - - const response = await fixture.sendServerRequest( - 'item/fileChange/requestApproval', - params + const response = await fixture.sendServerRequest<{decision: unknown}>( + "item/commandExecution/requestApproval", + commandParams(["accept", decision, "decline", "cancel"], { + networkApprovalContext: {host: amendment.host, protocol: "https"}, + proposedNetworkPolicyAmendments: [amendment], + }), ); - - expect(response).toEqual({ decision: 'cancel' }); - - completeTurn(); - await promptPromise; - }); - - it('should describe a session write-root grant with common permission metadata', async () => { - const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); - fixture.setPermissionResponse({ - outcome: { outcome: 'selected', optionId: ApprovalOptionId.AllowAlways } + expect(response).toEqual({decision}); + expect(permissionRequest().options[1]).toMatchObject({ + name: "Block in future", + kind: "reject_always", + _meta: {permission: {description: "Add the proposed block rule to persistent Codex network policy"}}, }); - - const params: FileChangeRequestApprovalParams = { - threadId: sessionId, - turnId: 'turn-1', - startedAtMs: 0, - itemId: 'file-change-grant-root', - reason: 'Write generated files', - grantRoot: '/workspace/generated', - }; - - await fixture.sendServerRequest('item/fileChange/requestApproval', params); - - const request = fixture.getAcpConnectionEvents([])[0]!.args[0]; - expect(request.options.find((option: { optionId: string }) => option.optionId === ApprovalOptionId.AllowAlways)?._meta) - .toMatchObject({ - permission: { - version: 1, - changes: [{ - type: 'grant', - operation: 'grant', - description: 'Allow writes under /workspace/generated for this session', - lifetime: {scope: 'session'}, - targets: [{ - type: 'filesystem', - access: ['write'], - matcher: {type: 'directory', path: '/workspace/generated'}, - }], - }], - }, - }); - - completeTurn(); - await promptPromise; + await finish(prompt); }); - it('should return cancel when no handler registered', async () => { - const params: FileChangeRequestApprovalParams = { - threadId: 'non-existent-session', - turnId: 'turn-1', - startedAtMs: 0, - itemId: 'file-change-no-handler', - reason: null, - grantRoot: null, - }; - - const response = await fixture.sendServerRequest( - 'item/fileChange/requestApproval', - params + it.each([ + ["missing", undefined], + ["empty", []], + ["unknown", ["accept", "futureDecision", "decline"]], + ["no explicit decline", ["accept", "cancel"]], + ["mismatched amendment", [ + "accept", + {acceptWithExecpolicyAmendment: {execpolicy_amendment: ["different"]}}, + "decline", + ]], + ])("fails closed for a %s authoritative decision contract", async (_name, availableDecisions) => { + const prompt = setupSessionWithPendingPrompt(); + const response = await fixture.sendServerRequest<{decision: unknown}>( + "item/commandExecution/requestApproval", + commandParams(availableDecisions, {proposedExecpolicyAmendment: ["expected"]}), ); + expect(response).toEqual({decision: "cancel"}); + expect(permissionRequest()).toBeUndefined(); + await finish(prompt); + }); - expect(response).toEqual({ decision: 'cancel' }); + it("rejects a stale-turn command before opening ACP permission UI", async () => { + const prompt = setupSessionWithPendingPrompt(); + prompt.sessionState.currentTurnId = "newer-turn"; + const response = await fixture.sendServerRequest<{decision: unknown}>( + "item/commandExecution/requestApproval", + commandParams(["accept", "decline", "cancel"]), + ); + expect(response).toEqual({decision: "cancel"}); + expect(permissionRequest()).toBeUndefined(); + await finish(prompt); }); - it('should convert to ACP permission request format', async () => { - const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); - fixture.setPermissionResponse({ - outcome: { outcome: 'selected', optionId: 'allow_once' } - }); + it("keeps concurrent approvalId callbacks request-local while reusing the item toolCallId", async () => { + const prompt = setupSessionWithPendingPrompt(); + fixture.setPermissionResponse({outcome: {outcome: "selected", optionId: ApprovalOptionId.AllowOnce}}); + const decisions: CommandExecutionApprovalDecision[] = ["accept", "decline", "cancel"]; + const first = await fixture.sendServerRequest<{decision: unknown}>( + "item/commandExecution/requestApproval", + commandParams(decisions, {approvalId: "approval-a"}), + ); + const second = await fixture.sendServerRequest<{decision: unknown}>( + "item/commandExecution/requestApproval", + commandParams(decisions, {approvalId: "approval-b"}), + ); + expect(first).toEqual({decision: "accept"}); + expect(second).toEqual({decision: "accept"}); + const requests = fixture.getAcpConnectionEvents([]).filter(event => event.method === "requestPermission"); + expect(requests).toHaveLength(2); + expect(requests.map(event => event.args[0].toolCall.toolCallId)).toEqual(["command-item", "command-item"]); + expect(JSON.stringify(requests)).not.toContain("approval-a"); + expect(JSON.stringify(requests)).not.toContain("approval-b"); + await finish(prompt); + }); + }); - const params: FileChangeRequestApprovalParams = { + describe("file change approvals", () => { + function fileParams(overrides: Partial = {}): FileChangeRequestApprovalParams { + return { threadId: sessionId, - turnId: 'turn-1', + turnId: "turn-1", + itemId: "file-item", startedAtMs: 0, - itemId: 'file-change-snapshot', - reason: 'Modifying config file', - grantRoot: null, + reason: "Apply the generated edits.", + grantRoot: "/workspace", + ...overrides, }; + } - await fixture.sendServerRequest( - 'item/fileChange/requestApproval', - params - ); + it("uses correlated file locations and never claims grantRoot coverage", async () => { + const prompt = setupSessionWithPendingPrompt(); + fixture.sendServerNotification({ + method: "item/started", + params: { + threadId: sessionId, + turnId: "turn-1", + startedAtMs: 0, + item: { + type: "fileChange", + id: "file-item", + status: "inProgress", + changes: [ + {path: "/workspace/a.ts", kind: {type: "update", move_path: null}, diff: "diff-a"}, + {path: "/workspace/b.ts", kind: {type: "add"}, diff: "diff-b"}, + ], + }, + }, + }); + await fixture.getCodexAcpClient().waitForSessionNotifications(sessionId); + fixture.clearAcpConnectionDump(); + fixture.setPermissionResponse({outcome: {outcome: "selected", optionId: ApprovalOptionId.AllowAlways}}); - await expect(fixture.getAcpConnectionDump([])).toMatchFileSnapshot( - 'data/approval-file-change.json' + const response = await fixture.sendServerRequest<{decision: unknown}>( + "item/fileChange/requestApproval", + fileParams(), ); - completeTurn(); - await promptPromise; + expect(response).toEqual({decision: "acceptForSession"}); + expect(permissionRequest()).toMatchObject({ + toolCall: { + toolCallId: "file-item", + kind: "edit", + status: "pending", + title: "Edit files", + locations: [{path: "/workspace/a.ts"}, {path: "/workspace/b.ts"}], + }, + _meta: {permission: { + version: 1, + title: "Make edits?", + description: "Apply the generated edits.", + }}, + }); + expect(JSON.stringify(permissionRequest())).not.toContain("grantRoot"); + expect(JSON.stringify(permissionRequest())).not.toContain("writes under"); + await finish(prompt); + }); + + it("distinguishes explicit file rejection from ACP cancellation", async () => { + const rejectPrompt = setupSessionWithPendingPrompt(); + fixture.setPermissionResponse({outcome: {outcome: "selected", optionId: ApprovalOptionId.RejectOnce}}); + expect(await fixture.sendServerRequest("item/fileChange/requestApproval", fileParams())) + .toEqual({decision: "decline"}); + await finish(rejectPrompt); + + fixture = createCodexMockTestFixture(); + const cancelPrompt = setupSessionWithPendingPrompt(); + fixture.setPermissionResponse({outcome: {outcome: "cancelled"}}); + expect(await fixture.sendServerRequest("item/fileChange/requestApproval", fileParams())) + .toEqual({decision: "cancel"}); + await finish(cancelPrompt); }); }); - describe('Permissions approval', () => { - const requestedPermissions = { - network: { enabled: true }, + describe("additional permission approvals", () => { + const permissions = { + network: {enabled: false}, fileSystem: { - read: ['/home/user/project'], - write: ['/home/user/project/tmp'], - entries: [], + read: ["/workspace/read"], + write: ["/workspace/write"], + globScanMaxDepth: 3, + entries: [ + {path: {type: "path" as const, path: "/workspace/exact"}, access: "read" as const}, + {path: {type: "glob_pattern" as const, pattern: "/workspace/**/*.key"}, access: "deny" as const}, + {path: {type: "special" as const, value: {kind: "project_roots" as const, subpath: "build"}}, access: "write" as const}, + ], }, }; - const permissionApprovalCases = [ - { - optionId: ApprovalOptionId.AllowPermissionsForTurn, - expectedResponse: { - permissions: requestedPermissions, - scope: 'turn', - strictAutoReview: false, - }, - description: 'allow for turn', - }, - { - optionId: ApprovalOptionId.AllowPermissionsForSession, - expectedResponse: { - permissions: requestedPermissions, - scope: 'session', - strictAutoReview: false, - }, - description: 'allow for session', - }, - { - optionId: ApprovalOptionId.RejectPermissions, - expectedResponse: { - permissions: {}, - scope: 'turn', - strictAutoReview: true, - }, - description: 'reject', - }, - ] as const; - - it.each(permissionApprovalCases)( - 'should map $optionId to app-server permissions response ($description)', - async ({ optionId, expectedResponse }) => { - const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); - fixture.setPermissionResponse({ - outcome: { outcome: 'selected', optionId } - }); - - const params: PermissionsRequestApprovalParams = { - threadId: sessionId, - turnId: 'turn-1', - itemId: `permissions-${optionId}`, - environmentId: null, - startedAtMs: 0, - cwd: '/home/user/project', - reason: 'Need extra access', - permissions: requestedPermissions, - }; - - const response = await fixture.sendServerRequest( - 'item/permissions/requestApproval', - params - ); - - expect(response).toEqual(expectedResponse); - - completeTurn(); - await promptPromise; - } - ); - - it('should map cancelled permission dialog to strict auto-review with no grants', async () => { - const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); - fixture.setPermissionResponse({ - outcome: { outcome: 'cancelled' } - }); - - const params: PermissionsRequestApprovalParams = { + function params(): PermissionsRequestApprovalParams { + return { threadId: sessionId, - turnId: 'turn-1', - itemId: 'permissions-cancelled', - environmentId: null, + turnId: "turn-1", + itemId: "permissions-item", + environmentId: "remote-env", startedAtMs: 0, - cwd: '/home/user/project', - reason: 'Need extra access', - permissions: requestedPermissions, + cwd: "/workspace", + reason: "The build needs generated output access.", + permissions, }; + } - const response = await fixture.sendServerRequest( - 'item/permissions/requestApproval', - params - ); - - expect(response).toEqual({ - permissions: {}, - scope: 'turn', - strictAutoReview: true, - }); - - completeTurn(); - await promptPromise; + it.each([ + [ApprovalOptionId.AllowPermissionsForTurn, "turn", false], + [ApprovalOptionId.AllowPermissionsForSession, "session", false], + [ApprovalOptionId.RejectPermissions, "turn", true], + ] as const)("maps %s atomically", async (optionId, scope, strictAutoReview) => { + const prompt = setupSessionWithPendingPrompt(); + fixture.setPermissionResponse({outcome: {outcome: "selected", optionId}}); + const response = await fixture.sendServerRequest("item/permissions/requestApproval", params()); + expect(response).toEqual(optionId === ApprovalOptionId.RejectPermissions + ? {permissions: {}, scope, strictAutoReview} + : {permissions, scope, strictAutoReview}); + + if (optionId === ApprovalOptionId.AllowPermissionsForTurn) { + expect(permissionRequest()).toMatchObject({ + toolCall: { + toolCallId: "permissions-item", + kind: "other", + status: "pending", + title: "Additional sandbox permissions", + rawInput: {permissions, cwd: "/workspace", environmentId: "remote-env"}, + locations: [ + {path: "/workspace/read"}, + {path: "/workspace/write"}, + {path: "/workspace/exact"}, + ], + content: [{type: "content", content: {type: "text", text: [ + "Disable network access", + "deny filesystem pattern /workspace/**/*.key", + 'write Codex filesystem scope {"kind":"project_roots","subpath":"build"}', + ].join("\n")}}], + }, + _meta: {permission: { + version: 1, + title: "Grant permissions?", + description: "The build needs generated output access.", + }}, + }); + } + await finish(prompt); }); - it('should return strict auto-review with no grants when no handler registered', async () => { - const params: PermissionsRequestApprovalParams = { - threadId: 'non-existent-session', - turnId: 'turn-1', - itemId: 'permissions-no-handler', - environmentId: null, - startedAtMs: 0, - cwd: '/home/user/project', - reason: 'Need extra access', - permissions: requestedPermissions, - }; - - const response = await fixture.sendServerRequest( - 'item/permissions/requestApproval', - params - ); - - expect(response).toEqual({ + it("maps ACP cancellation to an empty strict profile", async () => { + const prompt = setupSessionWithPendingPrompt(); + fixture.setPermissionResponse({outcome: {outcome: "cancelled"}}); + expect(await fixture.sendServerRequest("item/permissions/requestApproval", params())).toEqual({ permissions: {}, - scope: 'turn', + scope: "turn", strictAutoReview: true, }); - }); - - it('should convert to ACP permission request format', async () => { - const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); - fixture.setPermissionResponse({ - outcome: { outcome: 'selected', optionId: ApprovalOptionId.AllowPermissionsForSession } - }); - - const params: PermissionsRequestApprovalParams = { - threadId: sessionId, - turnId: 'turn-1', - itemId: 'permissions-snapshot', - environmentId: null, - startedAtMs: 0, - cwd: '/home/user/project', - reason: 'Need extra access', - permissions: requestedPermissions, - }; - - await fixture.sendServerRequest( - 'item/permissions/requestApproval', - params - ); - - await expect(fixture.getAcpConnectionDump([])).toMatchFileSnapshot( - 'data/approval-permissions-request.json' - ); - - completeTurn(); - await promptPromise; + await finish(prompt); }); }); }); From 0c11048f84c3edcf5a1f6a6dad512d91560c54dd Mon Sep 17 00:00:00 2001 From: Nikita Ashikhmin Date: Mon, 17 Aug 2026 16:28:57 +0400 Subject: [PATCH 2/3] feat: align approval options with native Codex permissions Map Codex approval requests to ACP v1 permission presentation and preserve the provider's real durable choices. Split approval, MCP, metadata, plan review, and presentation logic into focused permission modules with behavioral tests. --- README.md | 2 +- docs/permission-extension.md | 180 ++++++++++ package-lock.json | 6 +- src/CodexAcpServer.ts | 46 +-- src/CodexAppServerClient.ts | 4 +- src/CodexApprovalOptions.ts | 198 ----------- src/CodexElicitationHandler.ts | 306 +++++------------ src/McpApprovalOptionId.ts | 8 - .../CodexACPAgent/approval-events.test.ts | 294 ++++++++++++++-- .../data/elicitation-form-accept.json | 48 --- ...elicitation-tool-approval-all-persist.json | 22 +- .../elicitation-tool-approval-no-persist.json | 12 +- ...licitation-tool-approval-session-only.json | 17 +- .../data/elicitation-url-accept.json | 16 +- .../e2e/acp-e2e-file-approval.test.ts | 4 +- .../e2e/acp-e2e-mcp-approval.test.ts | 8 +- .../e2e/acp-e2e-shell-approval.test.ts | 14 +- .../e2e/permission-responders.ts | 2 +- .../CodexACPAgent/elicitation-events.test.ts | 169 +++++++-- src/{ => permissions}/CodexApprovalHandler.ts | 54 ++- src/permissions/mcp.ts | 194 +++++++++++ .../metadata.ts} | 9 +- .../option-ids.ts} | 16 +- src/permissions/options.ts | 322 ++++++++++++++++++ src/permissions/plan-review.ts | 38 +++ .../presentation-store.ts} | 4 +- .../presentation.ts} | 43 ++- 27 files changed, 1374 insertions(+), 662 deletions(-) create mode 100644 docs/permission-extension.md delete mode 100644 src/CodexApprovalOptions.ts delete mode 100644 src/McpApprovalOptionId.ts delete mode 100644 src/__tests__/CodexACPAgent/data/elicitation-form-accept.json rename src/{ => permissions}/CodexApprovalHandler.ts (82%) create mode 100644 src/permissions/mcp.ts rename src/{CodexPermissionMetadata.ts => permissions/metadata.ts} (87%) rename src/{ApprovalOptionId.ts => permissions/option-ids.ts} (52%) create mode 100644 src/permissions/options.ts create mode 100644 src/permissions/plan-review.ts rename src/{CodexApprovalPresentationStore.ts => permissions/presentation-store.ts} (91%) rename src/{CodexPermissionPresentation.ts => permissions/presentation.ts} (70%) diff --git a/README.md b/README.md index 5246590a..2bda2a26 100644 --- a/README.md +++ b/README.md @@ -11,7 +11,7 @@ Use [OpenAI Codex](https://github.com/openai/codex) from [Agent Client Protocol] - ChatGPT, API key, and client-provided custom gateway authentication. - Model, reasoning effort, fast mode, approval, and sandbox mode configuration. - Text prompts, embedded context, images, resource links, and additional workspace directories. -- Shell command, file change, permission request, MCP tool call, terminal output, reasoning, plan, web search, image generation, image view, token usage, and review events. +- Shell command, file change, [permission request](docs/permission-extension.md), MCP tool call, terminal output, reasoning, plan, web search, image generation, image view, token usage, and review events. - Subagent launches as standard ACP tool calls, with Codex thread identity and activity details in namespaced `_meta.codex.subagent` metadata. - Session-scoped long-running goals through the provider-neutral [goal extension](docs/goal-extension.md). - Client-provided MCP servers over command-based stdio config and HTTP transport. diff --git a/docs/permission-extension.md b/docs/permission-extension.md new file mode 100644 index 00000000..f23f3a04 --- /dev/null +++ b/docs/permission-extension.md @@ -0,0 +1,180 @@ +# Permission presentation extension + +For a user-facing summary of behavior changes, see +[`permission-changes.ru.md`](permission-changes.ru.md). + +This document defines the provider-neutral permission presentation implemented by `codex-acp`. Permission decisions use the standard ACP `session/request_permission` method. The optional `_meta.permission` extension adds display text only; it never changes which actions a client may approve. + +## Protocol contract + +Every permission request contains: + +- a `toolCall` describing the action that needs approval; +- an ordered `options` array containing every decision the user may select; +- optional request-level and option-level `_meta.permission` presentation data. + +Clients make a decision by returning one of the advertised `optionId` values. They must not derive a decision from the option label, `kind`, or metadata. `codex-acp` keeps the exact Codex decision associated with each option and returns that original value to Codex. + +```json +{ + "sessionId": "session-1", + "toolCall": { + "toolCallId": "command-7", + "kind": "execute", + "status": "pending", + "title": "Run command", + "rawInput": { + "command": "npm test", + "cwd": "/workspace" + } + }, + "options": [ + { + "optionId": "allow_once", + "name": "Yes, proceed", + "kind": "allow_once" + }, + { + "optionId": "cancel", + "name": "No, and tell Codex what to do differently", + "kind": "reject_once" + } + ], + "_meta": { + "permission": { + "version": 1, + "title": "Run command?", + "description": "The test suite needs to run outside the current sandbox." + } + } +} +``` + +The standard ACP fields are the compatibility contract. A client that ignores `_meta.permission` can still render the action, present every option, and return a correct decision. + +## Presentation metadata + +Request-level metadata has this shape: + +```json +{ + "_meta": { + "permission": { + "version": 1, + "title": "Allow network access?", + "description": "Download the requested dependency." + } + } +} +``` + +`version` and `title` are required. `description` is optional and contains the non-blank reason supplied by Codex. Action payloads are not copied into metadata. + +An individual option may provide a description: + +```json +{ + "optionId": "allow_session", + "name": "Allow for this session", + "kind": "allow_always", + "_meta": { + "permission": { + "version": 1, + "description": "Run the tool and remember this choice for this session." + } + } +} +``` + +No capability negotiation is required. The metadata is optional, additive, and safe for clients to ignore. + +## Action presentation + +The `toolCall` remains the authoritative description of the action: + +- `rawInput` contains structured command, working-directory, server, URL, or permission-profile data. +- `locations` contains affected filesystem paths when Codex provides them. +- `content` carries details that do not fit a location, such as a network host, filesystem glob, special Codex scope, or MCP message. +- `title`, `kind`, and `status` provide the standard ACP summary. + +Command approvals use `kind: execute`. File changes use `kind: edit`. Additional sandbox permissions use `kind: other`. URL authorization fallback uses `kind: fetch`. + +For file changes, locations come from the correlated Codex `fileChange` item. `grantRoot` is not presented as though every file below it will be modified. + +## Command and network decisions + +When Codex sends `availableDecisions`, that ordered list is authoritative. Older Codex versions that omit it use the native Codex fallback decision set. + +| Codex decision | ACP option kind | Meaning | +| --- | --- | --- | +| `accept` | `allow_once` | Approve this execution once. | +| `acceptForSession` | `allow_always` | Approve the command, host, or requested permissions for this session. | +| `acceptWithExecpolicyAmendment` | `allow_always` | Approve and install the exact proposed command-prefix rule. | +| network amendment with `allow` | `allow_always` | Approve and install the exact proposed allow rule. | +| network amendment with `deny` | `reject_always` | Reject and install the exact proposed deny rule. | +| `decline` | `reject_once` | Reject this execution and continue the turn. | +| `cancel` | `reject_once` | Reject this execution and abort the pending operation. | + +Exec-policy and network amendments are returned as the exact structured values supplied by Codex. An amendment is rejected if it does not match the corresponding proposal. An exec-policy option whose rendered prefix contains a line break is not shown, matching the native Codex UI. + +Unknown, malformed, empty, or internally inconsistent authoritative decision sets fail closed with `cancel`; the adapter does not invent replacement choices. + +## File changes + +File-change approvals expose the native Codex choices: + +| ACP option | Kind | Codex decision | +| --- | --- | --- | +| `Yes, proceed` | `allow_once` | `accept` | +| `Yes, and don't ask again for these files` | `allow_always` | `acceptForSession` | +| `No, and tell Codex what to do differently` | `reject_once` | `cancel` | + +Although the protocol decision enum also contains `decline`, the native Codex file-change prompt does not currently advertise it. + +## Additional sandbox permissions + +Codex may request a structured network and filesystem permission profile. `codex-acp` returns only permissions from that requested profile; Codex intersects the response with the original request before applying it. + +| User choice | Scope | `strictAutoReview` | +| --- | --- | --- | +| Grant for this turn | `turn` | `false` | +| Grant for this turn with strict auto review | `turn` | `true` | +| Grant for this session | `session` | `false` | +| Continue without permissions | `turn` | `false` | + +Strict auto review is intentionally turn-scoped. It causes subsequent actions in that turn to pass through Codex review even when ordinary sandbox policy would allow them. It is never combined with a session-scoped grant. + +Cancellation, an unknown option, a stale turn, or a missing handler returns an empty permission profile with turn scope and `strictAutoReview: false`. + +## MCP elicitation approvals + +Message-only MCP elicitations use `session/request_permission` so clients receive the same decision matrix as the native Codex UI. Codex advertises durable choices through request `_meta.persist`; `codex-acp` never creates a persistence scope that the server did not offer. + +| Advertised condition | ACP option | MCP response | +| --- | --- | --- | +| Always | `Allow` | `action: accept` | +| `persist` contains `session` | `Allow for this session` | `action: accept`, `_meta.persist: session` | +| `persist` contains `always` | `Always allow` | `action: accept`, `_meta.persist: always` | +| Non-tool request | `Deny` | `action: decline` | +| Always | `Cancel` | `action: cancel` | + +Tool-call approvals deliberately have no `Deny` choice: cancellation stops the tool call. For an ordinary MCP request, `Deny` declines the request while allowing the surrounding turn to continue, whereas `Cancel` aborts the request. + +Structured form and URL elicitations use the corresponding ACP elicitation capability when the client advertises it. A structured form that the client cannot render is cancelled rather than replaced with an approval that would omit required input. A message-only or URL request may use permission fallback because no structured field values are lost. + +The Codex app-server currently omits the MCP request identity from form-mode elicitation parameters. `codex-acp` correlates the request with an existing MCP tool call only when exactly one pending call for that thread and server is available. Ambiguous requests receive a unique standalone `toolCallId` and include the full message and schema. + +## Lifecycle and safety + +Permission prompts belong to the active Codex turn. Requests for a stale or interrupted turn are rejected without opening client UI. Cancelling an ACP request, returning an unadvertised `optionId`, transport failure, and malformed client responses all fail closed. + +The adapter does not reconstruct provider effects from ACP `kind` values. In particular, `allow_always` describes presentation intent but does not itself create a policy rule; only the exact Codex decision associated with the selected `optionId` can do that. + +The app-server v2 request methods are the active permission surface: + +- `item/commandExecution/requestApproval` +- `item/fileChange/requestApproval` +- `item/permissions/requestApproval` +- `mcpServer/elicitation/request` + +Deprecated `execCommandApproval` and `applyPatchApproval` methods are not exposed as a second permission pipeline. diff --git a/package-lock.json b/package-lock.json index 007d11c4..23b11398 100644 --- a/package-lock.json +++ b/package-lock.json @@ -3176,9 +3176,9 @@ "license": "MIT" }, "node_modules/nanoid": { - "version": "3.3.17", - "resolved": "https://registry.npmjs.org/nanoid/-/nanoid-3.3.17.tgz", - "integrity": "sha512-xQLf0A3HOMlgHq0n247/LRuAOYmB7dXJ/DvAxGvsSBij45XtBSmQycu+F8ODbHwns/XyFZagyL1+J0Offw1E0g==", + "version": "3.3.18", + "resolved": "https://registry.npmjs.org/nanoid/-/nanoid-3.3.18.tgz", + "integrity": "sha512-DTg4MJbGMWkfi6VZFdNt2/caMbQy4Ou+Op/hJQvGEWcnVfoA1QA+xzRKAzw9jD6+GVOOeYr/mIcuDSdug6F6+w==", "dev": true, "funding": [ { diff --git a/src/CodexAcpServer.ts b/src/CodexAcpServer.ts index a74c14f8..1c04edd2 100644 --- a/src/CodexAcpServer.ts +++ b/src/CodexAcpServer.ts @@ -1,8 +1,13 @@ import * as acp from "@agentclientprotocol/sdk"; import {RequestError, type SessionId, type SessionModeState} from "@agentclientprotocol/sdk"; import {CodexEventHandler, type CompletedPlan} from "./CodexEventHandler"; -import {CodexApprovalHandler} from "./CodexApprovalHandler"; -import {CodexApprovalPresentationStore} from "./CodexApprovalPresentationStore"; +import {CodexApprovalHandler} from "./permissions/CodexApprovalHandler"; +import {CodexApprovalPresentationStore} from "./permissions/presentation-store"; +import { + planImplementationApproved, + planImplementationPermissionRequest, + planImplementationToolCallId, +} from "./permissions/plan-review"; import {CodexElicitationHandler} from "./CodexElicitationHandler"; import {type CodexAuthRequest, getCodexAuthMethods, isCodexAuthRequest} from "./CodexAuthMethod"; import {clientSupportsUrlElicitation} from "./ElicitationCapabilities"; @@ -114,8 +119,6 @@ import { parseAgentFileChangeReportRequest, } from "./AgentFileChangeReport"; -const IMPLEMENT_PLAN_OPTION_ID = "implement_plan"; -const REVISE_PLAN_OPTION_ID = "revise_plan"; export interface SessionState { sessionId: string, @@ -145,6 +148,7 @@ export interface SessionState { sessionTitle: string | null; sessionTitleSource: "unset" | "fallback" | "explicit" | "unknown"; sessionFailure?: SessionFailure; + permissionRequestSequence?: number; } export type SessionFailureCategory = @@ -2654,42 +2658,14 @@ export class CodexAcpServer { plan: CompletedPlan, cancellationSignal: AbortSignal, ): Promise { - const toolCallId = `plan-review:${plan.itemId}`; + const toolCallId = planImplementationToolCallId(plan); try { const response = await this.connection.request( acp.methods.client.session.requestPermission, - { - sessionId: sessionState.sessionId, - toolCall: { - toolCallId, - title: "Implement this plan?", - kind: "switch_mode", - status: "pending", - rawInput: {plan: plan.text}, - }, - options: [ - { - optionId: IMPLEMENT_PLAN_OPTION_ID, - name: "Yes, implement this plan", - kind: "allow_once", - }, - { - optionId: REVISE_PLAN_OPTION_ID, - name: "No, and tell Codex what to do differently", - kind: "reject_once", - }, - ], - _meta: { - codex: { - kind: "plan_review", - planItemId: plan.itemId, - }, - }, - }, + planImplementationPermissionRequest(sessionState.sessionId, plan), {cancellationSignal}, ); - const approved = response.outcome.outcome === "selected" - && response.outcome.optionId === IMPLEMENT_PLAN_OPTION_ID; + const approved = planImplementationApproved(response); await this.connection.notify(acp.methods.client.session.update, { sessionId: sessionState.sessionId, update: { diff --git a/src/CodexAppServerClient.ts b/src/CodexAppServerClient.ts index 1e741eba..0f802d68 100644 --- a/src/CodexAppServerClient.ts +++ b/src/CodexAppServerClient.ts @@ -220,11 +220,11 @@ export class CodexAppServerClient { this.connection.onRequest(PermissionsApprovalRequest, async (params) => { if (this.isStaleTurn(params.threadId, params.turnId)) { - return { permissions: {}, scope: "turn", strictAutoReview: true }; + return { permissions: {}, scope: "turn", strictAutoReview: false }; } const handler = this.approvalHandlers.get(params.threadId); if (!handler) { - return { permissions: {}, scope: "turn", strictAutoReview: true }; + return { permissions: {}, scope: "turn", strictAutoReview: false }; } return await handler.handlePermissionsRequest(params); }); diff --git a/src/CodexApprovalOptions.ts b/src/CodexApprovalOptions.ts deleted file mode 100644 index fd23fd1e..00000000 --- a/src/CodexApprovalOptions.ts +++ /dev/null @@ -1,198 +0,0 @@ -import type * as acp from "@agentclientprotocol/sdk"; -import type { - CommandExecutionApprovalDecision, - CommandExecutionRequestApprovalParams, - FileChangeApprovalDecision, - NetworkPolicyAmendment, -} from "./app-server/v2"; -import {ApprovalOptionId} from "./ApprovalOptionId"; -import {optionPermissionMeta} from "./CodexPermissionMetadata"; - -export type DecisionOption = { - option: acp.PermissionOption; - decision: T; -}; - -export type CommandParamsWithAvailableDecisions = CommandExecutionRequestApprovalParams & { - availableDecisions?: unknown; -}; - -export function commandDecisionOptions( - params: CommandParamsWithAvailableDecisions, -): DecisionOption[] | undefined { - const decisions = parseAvailableCommandDecisions(params); - if (!decisions) return undefined; - - const options: DecisionOption[] = []; - let networkIndex = 0; - for (const decision of decisions) { - if (decision === "cancel") continue; - if (decision === "accept") { - options.push(decisionOption(ApprovalOptionId.AllowOnce, "Allow once", "allow_once", decision)); - continue; - } - if (decision === "acceptForSession") { - options.push(decisionOption( - ApprovalOptionId.AllowAlways, - "Allow for session", - "allow_always", - decision, - "Remember this approval until the Codex session ends", - )); - continue; - } - if (decision === "decline") { - options.push(decisionOption(ApprovalOptionId.RejectOnce, "Reject", "reject_once", decision)); - continue; - } - if ("acceptWithExecpolicyAmendment" in decision) { - options.push(decisionOption( - ApprovalOptionId.AcceptWithExecpolicyAmendment, - "Allow command pattern", - "allow_always", - decision, - "Add the proposed command-prefix rule to persistent Codex policy", - )); - continue; - } - options.push(decisionOption( - `${ApprovalOptionId.ApplyNetworkPolicyAmendment}:${networkIndex++}`, - decision.applyNetworkPolicyAmendment.network_policy_amendment.action === "allow" - ? "Allow in future" - : "Block in future", - decision.applyNetworkPolicyAmendment.network_policy_amendment.action === "allow" - ? "allow_always" - : "reject_always", - decision, - decision.applyNetworkPolicyAmendment.network_policy_amendment.action === "allow" - ? "Add the proposed allow rule to persistent Codex network policy" - : "Add the proposed block rule to persistent Codex network policy", - )); - } - - const hasAllow = options.some(({option}) => option.kind === "allow_once" || option.kind === "allow_always"); - const hasReject = options.some(({decision}) => decision === "decline"); - return hasAllow && hasReject ? options : undefined; -} - -export function fileChangeDecisionOptions(): DecisionOption[] { - return [ - decisionOption(ApprovalOptionId.AllowOnce, "Allow once", "allow_once", "accept"), - decisionOption( - ApprovalOptionId.AllowAlways, - "Allow for session", - "allow_always", - "acceptForSession", - "Remember this approval until the Codex session ends", - ), - decisionOption(ApprovalOptionId.RejectOnce, "Reject", "reject_once", "decline"), - ]; -} - -export function permissionProfileOptions(): acp.PermissionOption[] { - return [ - permissionOption( - ApprovalOptionId.AllowPermissionsForTurn, - "Allow once", - "allow_once", - "Grant the complete requested permission profile for this turn", - ), - permissionOption( - ApprovalOptionId.AllowPermissionsForSession, - "Allow for session", - "allow_always", - "Grant the complete requested permission profile until the Codex session ends", - ), - permissionOption(ApprovalOptionId.RejectPermissions, "Reject", "reject_once"), - ]; -} - -function parseAvailableCommandDecisions( - params: CommandParamsWithAvailableDecisions, -): CommandExecutionApprovalDecision[] | undefined { - if (!Array.isArray(params.availableDecisions) || params.availableDecisions.length === 0) { - return undefined; - } - const decisions: CommandExecutionApprovalDecision[] = []; - for (const candidate of params.availableDecisions) { - const decision = parseCommandDecision(candidate, params); - if (!decision) return undefined; - decisions.push(decision); - } - return decisions; -} - -function parseCommandDecision( - candidate: unknown, - params: CommandExecutionRequestApprovalParams, -): CommandExecutionApprovalDecision | undefined { - if (candidate === "accept" || candidate === "acceptForSession" || candidate === "decline" || candidate === "cancel") { - return candidate; - } - if (!isRecord(candidate)) return undefined; - - if ("acceptWithExecpolicyAmendment" in candidate) { - const value = candidate["acceptWithExecpolicyAmendment"]; - if (!isRecord(value)) return undefined; - const amendment = value["execpolicy_amendment"]; - if (!isStringArray(amendment) || amendment.length === 0) return undefined; - if (!sameStrings(amendment, params.proposedExecpolicyAmendment)) return undefined; - return {acceptWithExecpolicyAmendment: {execpolicy_amendment: [...amendment]}}; - } - - if ("applyNetworkPolicyAmendment" in candidate) { - const value = candidate["applyNetworkPolicyAmendment"]; - if (!isRecord(value)) return undefined; - const amendment = parseNetworkAmendment(value["network_policy_amendment"]); - if (!amendment || !params.networkApprovalContext) return undefined; - if (amendment.host !== params.networkApprovalContext.host) return undefined; - if (!(params.proposedNetworkPolicyAmendments ?? []).some(proposed => sameNetworkAmendment(proposed, amendment))) { - return undefined; - } - return {applyNetworkPolicyAmendment: {network_policy_amendment: amendment}}; - } - return undefined; -} - -function decisionOption( - optionId: string, - name: string, - kind: acp.PermissionOptionKind, - decision: T, - description?: string, -): DecisionOption { - return {option: permissionOption(optionId, name, kind, description), decision}; -} - -function permissionOption( - optionId: string, - name: string, - kind: acp.PermissionOptionKind, - description?: string, -): acp.PermissionOption { - const meta = optionPermissionMeta(description); - return {optionId, name, kind, ...(meta ? {_meta: meta} : {})}; -} - -function parseNetworkAmendment(value: unknown): NetworkPolicyAmendment | undefined { - if (!isRecord(value) || typeof value["host"] !== "string") return undefined; - const action = value["action"]; - if (action !== "allow" && action !== "deny") return undefined; - return {host: value["host"], action}; -} - -function isRecord(value: unknown): value is Record { - return value !== null && typeof value === "object" && !Array.isArray(value); -} - -function isStringArray(value: unknown): value is string[] { - return Array.isArray(value) && value.every(entry => typeof entry === "string"); -} - -function sameStrings(left: readonly string[], right?: readonly string[] | null): boolean { - return !!right && left.length === right.length && left.every((value, index) => value === right[index]); -} - -function sameNetworkAmendment(left: NetworkPolicyAmendment, right: NetworkPolicyAmendment): boolean { - return left.host === right.host && left.action === right.action; -} diff --git a/src/CodexElicitationHandler.ts b/src/CodexElicitationHandler.ts index b0ca0eab..84f2c6c6 100644 --- a/src/CodexElicitationHandler.ts +++ b/src/CodexElicitationHandler.ts @@ -12,27 +12,19 @@ import type { ToolRequestUserInputResponse, } from "./app-server/v2"; import { logger } from "./Logger"; -import { McpApprovalOptionId } from "./McpApprovalOptionId"; import type {AcpClientConnection} from "./ACPSessionConnection"; import { clientSupportsFormElicitation, clientSupportsUrlElicitation, } from "./ElicitationCapabilities"; - -// 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" }, -]; - -type PersistValue = "session" | "always"; -type ToolApprovalPersistValue = PersistValue | "once"; - -type McpElicitationContext = { - isToolApproval: boolean; - persistOptions: Set; - correlatedCallId: string | undefined; -}; +import { + buildMcpPermissionRequest, + convertMcpPermissionResponse, + isMcpToolCallApproval, + parsePersistOptions, + type PersistValue, + type McpElicitationContext, +} from "./permissions/mcp"; type AcpBackedMcpElicitationParams = Extract< McpServerElicitationRequestParams, { mode: "form" } | { mode: "url" } @@ -40,34 +32,6 @@ type AcpBackedMcpElicitationParams = Extract< const USER_INPUT_OTHER_FIELD_SUFFIX = "__other"; -/** - * Parses the `persist` field from the elicitation request `_meta`. - * Codex advertises which persistence options the client should show. - * Returns a set of supported persist values. - */ -function parsePersistOptions(meta: unknown): Set { - const result = new Set(); - if (!meta || typeof meta !== "object") return result; - const persist = (meta as Record)["persist"]; - if (persist === "session") { - result.add("session"); - } else if (persist === "always") { - result.add("always"); - } else if (Array.isArray(persist)) { - if (persist.includes("session")) result.add("session"); - if (persist.includes("always")) result.add("always"); - } - return result; -} - -function isMcpToolCallApproval(meta: unknown): boolean { - return ( - meta !== null && - typeof meta === "object" && - (meta as Record)["codex_approval_kind"] === "mcp_tool_call" - ); -} - function isRecord(value: unknown): value is Record { return value !== null && typeof value === "object" && !Array.isArray(value); } @@ -155,44 +119,6 @@ function metaRecord(meta: unknown): Record | null { return isRecord(meta) ? meta : null; } -function persistChoiceOption(value: ToolApprovalPersistValue): acp.EnumOption { - switch (value) { - case "once": - return { const: "once", title: "Allow once" }; - case "session": - return { const: "session", title: "Allow for this session" }; - case "always": - return { const: "always", title: "Allow and don't ask again" }; - } -} - -function addPersistChoiceToSchema( - schema: acp.ElicitationSchema, - persistOptions: Set -): acp.ElicitationSchema { - if (persistOptions.size === 0) { - return schema; - } - - const choices: ToolApprovalPersistValue[] = ["once"]; - if (persistOptions.has("session")) choices.push("session"); - if (persistOptions.has("always")) choices.push("always"); - - return { - ...schema, - properties: { - ...schema.properties, - persist: { - type: "string", - title: "Approval scope", - oneOf: choices.map(persistChoiceOption), - default: "once", - }, - }, - required: Array.from(new Set([...(schema.required ?? []), "persist"])), - }; -} - function contentRecord(content: unknown): Record { return isRecord(content) ? content as Record : {}; } @@ -250,24 +176,6 @@ function userInputResponseValue( return value; } -/** - * Builds the ACP permission options for an MCP tool call approval elicitation. - * Always includes "Allow Once"; adds session/always persist options when advertised. - */ -function buildToolApprovalOptions(persistOptions: Set): acp.PermissionOption[] { - const options: acp.PermissionOption[] = [ - { optionId: McpApprovalOptionId.AllowOnce, name: "Allow", kind: "allow_once" }, - ]; - if (persistOptions.has("session")) { - options.push({ optionId: McpApprovalOptionId.AllowSession, name: "Allow for This Session", kind: "allow_always" }); - } - if (persistOptions.has("always")) { - options.push({ optionId: McpApprovalOptionId.AllowAlways, name: "Allow and Don't Ask Again", kind: "allow_always" }); - } - options.push({ optionId: McpApprovalOptionId.Decline, name: "Decline", kind: "reject_once" }); - return options; -} - export class CodexElicitationHandler implements ElicitationHandler { private readonly connection: AcpClientConnection; private readonly sessionState: SessionState; @@ -285,10 +193,9 @@ export class CodexElicitationHandler implements ElicitationHandler { // mcpToolCall item carrying the call id and server name. We store (threadId, serverName) → callId // here so the elicitation request can correlate back to the already-rendered tool call item. // - // Multiple calls are safe because Codex requests approval synchronously — it blocks on one tool - // call's elicitation before starting the next, so there is at most one pending approval per - // (threadId, serverName). - private readonly pendingMcpApprovals = new Map(); + // App-server does not expose the MCP call id on form requests. Correlate only when there is one + // unambiguous pending call for the server; otherwise render a standalone request with its payload. + private readonly pendingMcpApprovals = new Map(); // The app-server handler exposes URL elicitationId, while serverRequest/resolved only exposes // threadId here, so accepted URL elicitations are completed at thread scope. private readonly pendingUrlElicitations = new Map>(); @@ -340,23 +247,32 @@ export class CodexElicitationHandler implements ElicitationHandler { await this.publishAcceptedMcpToolApproval(context, result.action === "accept"); return result; } + if (!this.canUsePermissionFallback(params)) { + return {action: "cancel", content: null, _meta: null}; + } - const { request, correlatedCallId } = this.buildPermissionRequest(params, context); + const {request, correlatedCallId} = buildMcpPermissionRequest( + this.sessionState.sessionId, + params, + context, + ); const response = await this.connection.request( acp.methods.client.session.requestPermission, request, this.requestOptions(), ); - if (correlatedCallId !== undefined && response.outcome.outcome !== "cancelled") { - const optionId = response.outcome.optionId; - if (optionId !== McpApprovalOptionId.Decline) { - await this.connection.notify(acp.methods.client.session.update, { - sessionId: this.sessionState.sessionId, - update: { sessionUpdate: "tool_call_update", toolCallId: correlatedCallId, status: "in_progress" }, - }); - } + const result = convertMcpPermissionResponse( + response, + context.isToolApproval, + context.persistOptions, + ); + if (correlatedCallId !== undefined && result.action === "accept") { + await this.connection.notify(acp.methods.client.session.update, { + sessionId: this.sessionState.sessionId, + update: { sessionUpdate: "tool_call_update", toolCallId: correlatedCallId, status: "in_progress" }, + }); } - return this.convertPermissionResponse(response); + return result; } catch (error) { logger.error("Error handling MCP elicitation request", error); return { action: "cancel", content: null, _meta: null }; @@ -436,17 +352,30 @@ export class CodexElicitationHandler implements ElicitationHandler { } private createMcpElicitationContext(params: McpServerElicitationRequestParams): McpElicitationContext { - const isToolApproval = isMcpToolCallApproval(params._meta); + const isToolApproval = isMcpToolCallApproval(params._meta) && this.isMessageOnlyForm(params); const persistOptions = parsePersistOptions(params._meta); const correlatedCallId = isToolApproval && (params.mode === "form" || params.mode === "openai/form") ? this.popPendingApproval(params.threadId, params.serverName) : undefined; - return { isToolApproval, persistOptions, correlatedCallId }; + const permissionRequestSequence = (this.sessionState.permissionRequestSequence ?? 0) + 1; + this.sessionState.permissionRequestSequence = permissionRequestSequence; + return { + isToolApproval, + persistOptions, + correlatedCallId, + standaloneToolCallId: [ + "elicitation", + this.sessionState.sessionId, + params.serverName, + permissionRequestSequence, + ].join(":"), + }; } private shouldUseAcpElicitation( params: McpServerElicitationRequestParams ): params is AcpBackedMcpElicitationParams { + if (this.isMessageOnlyForm(params)) return false; switch (params.mode) { case "form": return clientSupportsFormElicitation(this.clientCapabilities); @@ -457,6 +386,19 @@ export class CodexElicitationHandler implements ElicitationHandler { } } + private canUsePermissionFallback(params: McpServerElicitationRequestParams): boolean { + return params.mode === "url" || this.isMessageOnlyForm(params); + } + + private isMessageOnlyForm(params: McpServerElicitationRequestParams): boolean { + if (params.mode !== "form" && params.mode !== "openai/form") return false; + if (params.requestedSchema === null) return true; + if (!isRecord(params.requestedSchema)) return false; + return params.requestedSchema["type"] === "object" + && isRecord(params.requestedSchema["properties"]) + && Object.keys(params.requestedSchema["properties"]).length === 0; + } + private buildElicitationRequest( params: AcpBackedMcpElicitationParams, context: McpElicitationContext @@ -470,16 +412,10 @@ export class CodexElicitationHandler implements ElicitationHandler { switch (params.mode) { case "form": { - const requestedSchema = context.isToolApproval - ? addPersistChoiceToSchema( - normalizeElicitationSchema(params.requestedSchema), - context.persistOptions, - ) - : normalizeElicitationSchema(params.requestedSchema); return { ...base, mode: "form", - requestedSchema, + requestedSchema: normalizeElicitationSchema(params.requestedSchema), }; } case "url": @@ -565,94 +501,6 @@ export class CodexElicitationHandler implements ElicitationHandler { }; } - private buildPermissionRequest( - params: McpServerElicitationRequestParams, - context: McpElicitationContext - ): { request: acp.RequestPermissionRequest; correlatedCallId: string | undefined } { - const sessionId = this.sessionState.sessionId; - const messageContent: acp.ToolCallContent = { - type: "content", - content: { type: "text", text: params.message }, - }; - - const options = context.isToolApproval - ? buildToolApprovalOptions(context.persistOptions) - : ELICITATION_OPTIONS; - - if (params.mode === "form" || params.mode === "openai/form") { - if (context.correlatedCallId !== undefined) { - // The tool call item is already visible in the IDE conversation history because - // item/started was emitted before the elicitation request. Sending content or - // rawInput here would duplicate that information in the approval widget. - return { - request: { - sessionId, - toolCall: { - toolCallId: context.correlatedCallId, - kind: "execute", - status: "pending", - // content: [messageContent], — omitted: already rendered via item/started - // rawInput: { ... } — omitted: same reason - }, - _meta: { is_mcp_tool_approval: true }, - options, - }, - correlatedCallId: context.correlatedCallId, - }; - } - return { - request: { - sessionId, - toolCall: { - toolCallId: `elicitation-${params.serverName}`, - kind: context.isToolApproval ? "execute" : "other", - status: "pending", - content: [messageContent], - rawInput: { serverName: params.serverName, schema: params.requestedSchema }, - }, - ...(context.isToolApproval ? { _meta: { is_mcp_tool_approval: true } } : {}), - options, - }, - correlatedCallId: undefined, - }; - } else { - return { - request: { - sessionId, - toolCall: { - toolCallId: `elicitation-${params.elicitationId}`, - kind: "fetch", - status: "pending", - content: [messageContent], - rawInput: { serverName: params.serverName, url: params.url }, - }, - options, - }, - correlatedCallId: undefined, - }; - } - } - - private convertPermissionResponse( - response: acp.RequestPermissionResponse - ): McpServerElicitationRequestResponse { - if (response.outcome.outcome === "cancelled") { - return { action: "cancel", content: null, _meta: null }; - } - - const optionId = response.outcome.optionId; - if (optionId === McpApprovalOptionId.AllowSession) { - return { action: "accept", content: null, _meta: { persist: "session" } }; - } - if (optionId === McpApprovalOptionId.AllowAlways) { - return { action: "accept", content: null, _meta: { persist: "always" } }; - } - if (optionId === McpApprovalOptionId.AllowOnce || optionId === "accept") { - return { action: "accept", content: null, _meta: null }; - } - return { action: "decline", content: null, _meta: null }; - } - private convertElicitationResponse( response: acp.CreateElicitationResponse, context: McpElicitationContext @@ -660,6 +508,9 @@ export class CodexElicitationHandler implements ElicitationHandler { if (acp.CreateElicitationResponse.isAccept(response)) { const content = contentRecord(response.content); const persist = context.isToolApproval ? content["persist"] : undefined; + if (context.isToolApproval && !this.isAllowedToolApprovalPersist(persist, context.persistOptions)) { + return { action: "cancel", content: null, _meta: null }; + } if (persist === "session" || persist === "always" || persist === "once") { delete content["persist"]; } @@ -671,7 +522,9 @@ export class CodexElicitationHandler implements ElicitationHandler { } if (acp.CreateElicitationResponse.isDecline(response)) { - return { action: "decline", content: null, _meta: elicitationResponseMeta(response, context) }; + return context.isToolApproval + ? {action: "cancel", content: null, _meta: elicitationResponseMeta(response, context)} + : {action: "decline", content: null, _meta: elicitationResponseMeta(response, context)}; } if (acp.CreateElicitationResponse.isCancel(response)) { @@ -686,6 +539,16 @@ export class CodexElicitationHandler implements ElicitationHandler { return { action: "cancel", content: null, _meta: null }; } + private isAllowedToolApprovalPersist( + persist: acp.ElicitationContentValue | undefined, + persistOptions: ReadonlySet, + ): boolean { + return persist === undefined + || persist === "once" + || (persist === "session" && persistOptions.has("session")) + || (persist === "always" && persistOptions.has("always")); + } + private convertUserInputResponse( response: acp.CreateElicitationResponse, params: ToolRequestUserInputParams @@ -753,22 +616,29 @@ export class CodexElicitationHandler implements ElicitationHandler { if (event.item.type !== "mcpToolCall") { return; } - this.pendingMcpApprovals.set(this.key(event.threadId, event.item.server), event.item.id); + const key = this.key(event.threadId, event.item.server); + const pending = this.pendingMcpApprovals.get(key); + if (pending) pending.push(event.item.id); + else this.pendingMcpApprovals.set(key, [event.item.id]); } private handleItemCompleted(event: ItemCompletedNotification): void { if (event.item.type !== "mcpToolCall") { return; } - // This may run after the elicitation path already consumed the same entry. - // That double-pop is intentional: approvals pop on request correlation, while - // auto-approved or interrupted calls need completion-side cleanup. - this.popPendingApproval(event.threadId, event.item.server); + const key = this.key(event.threadId, event.item.server); + const pending = this.pendingMcpApprovals.get(key); + if (!pending) return; + const index = pending.indexOf(event.item.id); + if (index >= 0) pending.splice(index, 1); + if (pending.length === 0) this.pendingMcpApprovals.delete(key); } private popPendingApproval(threadId: string, serverName: string): string | undefined { const key = this.key(threadId, serverName); - const callId = this.pendingMcpApprovals.get(key); + const pending = this.pendingMcpApprovals.get(key); + if (pending?.length !== 1) return undefined; + const callId = pending.shift(); this.pendingMcpApprovals.delete(key); return callId; } diff --git a/src/McpApprovalOptionId.ts b/src/McpApprovalOptionId.ts deleted file mode 100644 index 3728145a..00000000 --- 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/approval-events.test.ts b/src/__tests__/CodexACPAgent/approval-events.test.ts index 5ec73fae..bde2e470 100644 --- a/src/__tests__/CodexACPAgent/approval-events.test.ts +++ b/src/__tests__/CodexACPAgent/approval-events.test.ts @@ -1,5 +1,6 @@ import {beforeEach, describe, expect, it, vi} from "vitest"; import type { + AdditionalPermissionProfile, CommandExecutionApprovalDecision, CommandExecutionRequestApprovalParams, FileChangeRequestApprovalParams, @@ -8,9 +9,10 @@ import type { import {createCodexMockTestFixture, createTestSessionState, type CodexMockTestFixture} from "../acp-test-utils"; import type {SessionState} from "../../CodexAcpServer"; import {AgentMode} from "../../AgentMode"; -import {ApprovalOptionId} from "../../ApprovalOptionId"; +import {ApprovalOptionId} from "../../permissions/option-ids"; type CommandParams = CommandExecutionRequestApprovalParams & { + additionalPermissions?: AdditionalPermissionProfile | null; availableDecisions?: unknown; }; @@ -89,7 +91,7 @@ describe("Approval Events", () => { describe("command approvals", () => { it("emits an autonomous ACP v1 snapshot and maps explicit reject to decline", async () => { const prompt = setupSessionWithPendingPrompt(); - fixture.setPermissionResponse({outcome: {outcome: "selected", optionId: ApprovalOptionId.RejectOnce}}); + fixture.setPermissionResponse({outcome: {outcome: "selected", optionId: ApprovalOptionId.Decline}}); const params = commandParams(["accept", "acceptForSession", "decline", "cancel"], { commandActions: [ {type: "read", command: "cat src/a.ts", name: "cat", path: "/workspace/src/a.ts"}, @@ -114,17 +116,18 @@ describe("Approval Events", () => { locations: [{path: "/workspace/src/a.ts"}, {path: "/workspace/src"}], }, options: [ - {optionId: "allow_once", name: "Allow once", kind: "allow_once"}, + {optionId: "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", - _meta: {permission: { - version: 1, - description: "Remember this approval until the Codex session ends", - }}, }, - {optionId: "reject_once", name: "Reject", kind: "reject_once"}, + {optionId: "decline", name: "No, continue without running it", kind: "reject_once"}, + { + optionId: "cancel", + name: "No, and tell Codex what to do differently", + kind: "reject_once", + }, ], _meta: {permission: { version: 1, @@ -153,7 +156,7 @@ describe("Approval Events", () => { await finish(prompt); }); - it("retains and returns the exact proposed exec-policy payload without exposing argv in prose", async () => { + it("suppresses a multiline exec-policy option exactly like the Codex TUI", async () => { const prompt = setupSessionWithPendingPrompt(); fixture.setPermissionResponse({ outcome: {outcome: "selected", optionId: ApprovalOptionId.AcceptWithExecpolicyAmendment}, @@ -167,20 +170,229 @@ describe("Approval Events", () => { proposedExecpolicyAmendment: amendment, }), ); - expect(response).toEqual({decision}); + expect(response).toEqual({decision: "cancel"}); const option = permissionRequest().options.find( (candidate: {optionId: string}) => candidate.optionId === ApprovalOptionId.AcceptWithExecpolicyAmendment, ); - expect(option).toEqual({ + expect(option).toBeUndefined(); + await finish(prompt); + }); + + it("returns the exact single-line exec-policy amendment selected by optionId", async () => { + const prompt = setupSessionWithPendingPrompt(); + const amendment = ["npm", "run", "test:unit"]; + const decision = {acceptWithExecpolicyAmendment: {execpolicy_amendment: amendment}} as const; + fixture.setPermissionResponse({ + outcome: {outcome: "selected", optionId: ApprovalOptionId.AcceptWithExecpolicyAmendment}, + }); + expect(await fixture.sendServerRequest( + "item/commandExecution/requestApproval", + commandParams(["accept", decision, "cancel"], {proposedExecpolicyAmendment: amendment}), + )).toEqual({decision}); + expect(permissionRequest().options[1]).toEqual({ optionId: ApprovalOptionId.AcceptWithExecpolicyAmendment, - name: "Allow command pattern", + name: "Yes, and don't ask again for commands that start with `npm run test:unit`", kind: "allow_always", - _meta: {permission: { - version: 1, - description: "Add the proposed command-prefix rule to persistent Codex policy", - }}, }); - expect(JSON.stringify(option)).not.toContain("secret"); + await finish(prompt); + }); + + it("maps selected decline and cancel to their distinct provider decisions", async () => { + const declinePrompt = setupSessionWithPendingPrompt(); + fixture.setPermissionResponse({outcome: {outcome: "selected", optionId: ApprovalOptionId.Decline}}); + expect(await fixture.sendServerRequest( + "item/commandExecution/requestApproval", + commandParams(["accept", "decline", "cancel"]), + )).toEqual({decision: "decline"}); + await finish(declinePrompt); + + fixture = createCodexMockTestFixture(); + const cancelPrompt = setupSessionWithPendingPrompt(); + fixture.setPermissionResponse({outcome: {outcome: "selected", optionId: ApprovalOptionId.Cancel}}); + expect(await fixture.sendServerRequest( + "item/commandExecution/requestApproval", + commandParams(["accept", "decline", "cancel"]), + )).toEqual({decision: "cancel"}); + await finish(cancelPrompt); + }); + + it("supports the native accept-plus-cancel decision set without inventing decline", async () => { + const prompt = setupSessionWithPendingPrompt(); + fixture.setPermissionResponse({outcome: {outcome: "selected", optionId: ApprovalOptionId.AllowOnce}}); + expect(await fixture.sendServerRequest( + "item/commandExecution/requestApproval", + commandParams(["accept", "cancel"]), + )).toEqual({decision: "accept"}); + expect(permissionRequest().options.map((option: {optionId: string}) => option.optionId)) + .toEqual([ApprovalOptionId.AllowOnce, ApprovalOptionId.Cancel]); + await finish(prompt); + }); + + it("orders native decisions as allow once, always allow, then deny", async () => { + const prompt = setupSessionWithPendingPrompt(); + fixture.setPermissionResponse({outcome: {outcome: "selected", optionId: ApprovalOptionId.AllowOnce}}); + expect(await fixture.sendServerRequest( + "item/commandExecution/requestApproval", + commandParams(["decline", "acceptForSession", "cancel", "accept"]), + )).toEqual({decision: "accept"}); + expect(permissionRequest().options.map((option: {optionId: string}) => option.optionId)).toEqual([ + ApprovalOptionId.AllowOnce, + ApprovalOptionId.AllowForSession, + ApprovalOptionId.Decline, + ApprovalOptionId.Cancel, + ]); + await finish(prompt); + }); + + it.each([undefined, null])( + "uses the native backwards-compatible decisions when availableDecisions is %s", + async availableDecisions => { + const prompt = setupSessionWithPendingPrompt(); + fixture.setPermissionResponse({outcome: {outcome: "selected", optionId: ApprovalOptionId.AllowOnce}}); + expect(await fixture.sendServerRequest( + "item/commandExecution/requestApproval", + commandParams(availableDecisions, {proposedExecpolicyAmendment: ["npm", "test"]}), + )).toEqual({decision: "accept"}); + expect(permissionRequest().options.map((option: {optionId: string}) => option.optionId)).toEqual([ + ApprovalOptionId.AllowOnce, + ApprovalOptionId.AcceptWithExecpolicyAmendment, + ApprovalOptionId.Cancel, + ]); + await finish(prompt); + }); + + it("uses only the first proposed allow amendment in the legacy network fallback", async () => { + const prompt = setupSessionWithPendingPrompt(); + const deny = {host: "example.test", action: "deny" as const}; + const firstAllow = {host: "example.test", action: "allow" as const}; + const secondAllow = {host: "example.test", action: "allow" as const}; + fixture.setPermissionResponse({ + outcome: {outcome: "selected", optionId: `${ApprovalOptionId.ApplyNetworkPolicyAmendment}:0`}, + }); + expect(await fixture.sendServerRequest( + "item/commandExecution/requestApproval", + commandParams(undefined, { + networkApprovalContext: {host: "example.test", protocol: "https"}, + proposedNetworkPolicyAmendments: [deny, firstAllow, secondAllow], + }), + )).toEqual({decision: { + applyNetworkPolicyAmendment: {network_policy_amendment: firstAllow}, + }}); + expect(permissionRequest().options.map((option: {name: string}) => option.name)).toEqual([ + "Yes, just this once", + "Yes, and allow this host for this conversation", + "Yes, and allow this host in the future", + "No, and tell Codex what to do differently", + ]); + await finish(prompt); + }); + + it("limits the legacy additional-permissions fallback to accept and cancel", async () => { + const prompt = setupSessionWithPendingPrompt(); + const additionalPermissions: AdditionalPermissionProfile = { + network: {enabled: true}, + fileSystem: null, + }; + fixture.setPermissionResponse({outcome: {outcome: "selected", optionId: ApprovalOptionId.AllowOnce}}); + expect(await fixture.sendServerRequest( + "item/commandExecution/requestApproval", + commandParams(undefined, {additionalPermissions}), + )).toEqual({decision: "accept"}); + expect(permissionRequest().options.map((option: {optionId: string}) => option.optionId)) + .toEqual([ApprovalOptionId.AllowOnce, ApprovalOptionId.Cancel]); + await finish(prompt); + }); + + it("cancels an unknown selected optionId instead of inferring a decision from kind", async () => { + const prompt = setupSessionWithPendingPrompt(); + fixture.setPermissionResponse({outcome: {outcome: "selected", optionId: "future-option"}}); + expect(await fixture.sendServerRequest( + "item/commandExecution/requestApproval", + commandParams(["accept", "cancel"]), + )).toEqual({decision: "cancel"}); + await finish(prompt); + }); + + it("fails closed when a network decision does not match the proposed host action", async () => { + const prompt = setupSessionWithPendingPrompt(); + const decision = { + applyNetworkPolicyAmendment: { + network_policy_amendment: {host: "other.test", action: "allow" as const}, + }, + }; + expect(await fixture.sendServerRequest( + "item/commandExecution/requestApproval", + commandParams(["accept", decision, "cancel"], { + networkApprovalContext: {host: "example.test", protocol: "https"}, + proposedNetworkPolicyAmendments: [{host: "example.test", action: "allow"}], + }), + )).toEqual({decision: "cancel"}); + expect(permissionRequest()).toBeUndefined(); + await finish(prompt); + }); + + it("renders exec-policy prefixes with the same shlex quoting as the Codex TUI", async () => { + const prompt = setupSessionWithPendingPrompt(); + const amendment = ["echo", "'foo bar'"]; + const decision = {acceptWithExecpolicyAmendment: {execpolicy_amendment: amendment}} as const; + fixture.setPermissionResponse({ + outcome: {outcome: "selected", optionId: ApprovalOptionId.AcceptWithExecpolicyAmendment}, + }); + expect(await fixture.sendServerRequest( + "item/commandExecution/requestApproval", + commandParams(["accept", decision, "cancel"], {proposedExecpolicyAmendment: amendment}), + )).toEqual({decision}); + expect(permissionRequest().options[1].name).toBe( + `Yes, and don't ask again for commands that start with \`echo "'foo bar'"\``, + ); + await finish(prompt); + }); + + it("uses Codex's case-sensitive recursive shell-name detection", async () => { + const prompt = setupSessionWithPendingPrompt(); + const amendment = ["BASH", "-lc", "echo hi"]; + const decision = {acceptWithExecpolicyAmendment: {execpolicy_amendment: amendment}} as const; + fixture.setPermissionResponse({ + outcome: {outcome: "selected", optionId: ApprovalOptionId.AcceptWithExecpolicyAmendment}, + }); + expect(await fixture.sendServerRequest( + "item/commandExecution/requestApproval", + commandParams(["accept", decision, "cancel"], {proposedExecpolicyAmendment: amendment}), + )).toEqual({decision}); + expect(permissionRequest().options[1].name).toBe( + "Yes, and don't ask again for commands that start with `BASH -lc 'echo hi'`", + ); + await finish(prompt); + }); + + it("presents experimental per-command additional permissions without copying them into prose", async () => { + const prompt = setupSessionWithPendingPrompt(); + fixture.setPermissionResponse({outcome: {outcome: "selected", optionId: ApprovalOptionId.AllowForSession}}); + const additionalPermissions: AdditionalPermissionProfile = { + network: {enabled: true}, + fileSystem: { + read: ["/outside/read"], + write: [], + entries: [{path: {type: "path", path: "/outside/read"}, access: "read"}], + }, + }; + expect(await fixture.sendServerRequest( + "item/commandExecution/requestApproval", + commandParams(["accept", "acceptForSession", "cancel"], {additionalPermissions}), + )).toEqual({decision: "acceptForSession"}); + expect(permissionRequest()).toMatchObject({ + toolCall: { + rawInput: {command: "npm test", cwd: "/workspace", additionalPermissions}, + locations: [{path: "/outside/read"}], + content: [{type: "content", content: {type: "text", text: "Enable network access"}}], + }, + options: [ + {name: "Yes, proceed"}, + {name: "Yes, and allow these permissions for this session"}, + {name: "No, and tell Codex what to do differently"}, + ], + }); + expect(permissionRequest()._meta.permission.description).toBe("Needed to verify the changes."); await finish(prompt); }); @@ -209,6 +421,9 @@ describe("Approval Events", () => { expect(permissionRequest().toolCall).toMatchObject({ title: `${protocol} network access to example.test`, content: [{type: "content", content: {type: "text", text: `${protocol} access to example.test`}}], + ...(protocol === "http" || protocol === "https" + ? {rawInput: {url: `${protocol}://example.test`}} + : {}), }); const optionsJson = JSON.stringify(permissionRequest().options); expect(optionsJson).not.toContain("example.test"); @@ -233,18 +448,16 @@ describe("Approval Events", () => { ); expect(response).toEqual({decision}); expect(permissionRequest().options[1]).toMatchObject({ - name: "Block in future", + name: "No, and block this host in the future", kind: "reject_always", - _meta: {permission: {description: "Add the proposed block rule to persistent Codex network policy"}}, }); await finish(prompt); }); it.each([ - ["missing", undefined], ["empty", []], ["unknown", ["accept", "futureDecision", "decline"]], - ["no explicit decline", ["accept", "cancel"]], + ["duplicate option id", ["accept", "accept", "cancel"]], ["mismatched amendment", [ "accept", {acceptWithExecpolicyAmendment: {execpolicy_amendment: ["different"]}}, @@ -330,7 +543,7 @@ describe("Approval Events", () => { }); await fixture.getCodexAcpClient().waitForSessionNotifications(sessionId); fixture.clearAcpConnectionDump(); - fixture.setPermissionResponse({outcome: {outcome: "selected", optionId: ApprovalOptionId.AllowAlways}}); + fixture.setPermissionResponse({outcome: {outcome: "selected", optionId: ApprovalOptionId.AllowForSession}}); const response = await fixture.sendServerRequest<{decision: unknown}>( "item/fileChange/requestApproval", @@ -359,9 +572,9 @@ describe("Approval Events", () => { it("distinguishes explicit file rejection from ACP cancellation", async () => { const rejectPrompt = setupSessionWithPendingPrompt(); - fixture.setPermissionResponse({outcome: {outcome: "selected", optionId: ApprovalOptionId.RejectOnce}}); + fixture.setPermissionResponse({outcome: {outcome: "selected", optionId: ApprovalOptionId.Cancel}}); expect(await fixture.sendServerRequest("item/fileChange/requestApproval", fileParams())) - .toEqual({decision: "decline"}); + .toEqual({decision: "cancel"}); await finish(rejectPrompt); fixture = createCodexMockTestFixture(); @@ -403,8 +616,9 @@ describe("Approval Events", () => { it.each([ [ApprovalOptionId.AllowPermissionsForTurn, "turn", false], + [ApprovalOptionId.AllowPermissionsForTurnWithStrictAutoReview, "turn", true], [ApprovalOptionId.AllowPermissionsForSession, "session", false], - [ApprovalOptionId.RejectPermissions, "turn", true], + [ApprovalOptionId.RejectPermissions, "turn", false], ] as const)("maps %s atomically", async (optionId, scope, strictAutoReview) => { const prompt = setupSessionWithPendingPrompt(); fixture.setPermissionResponse({outcome: {outcome: "selected", optionId}}); @@ -438,17 +652,39 @@ describe("Approval Events", () => { description: "The build needs generated output access.", }}, }); + expect(permissionRequest().options).toEqual([ + { + optionId: ApprovalOptionId.AllowPermissionsForTurn, + name: "Yes, grant these permissions for this turn", + kind: "allow_once", + }, + { + optionId: ApprovalOptionId.AllowPermissionsForTurnWithStrictAutoReview, + name: "Yes, grant for this turn with strict auto review", + kind: "allow_once", + }, + { + optionId: ApprovalOptionId.AllowPermissionsForSession, + name: "Yes, grant these permissions for this session", + kind: "allow_always", + }, + { + optionId: ApprovalOptionId.RejectPermissions, + name: "No, continue without permissions", + kind: "reject_once", + }, + ]); } await finish(prompt); }); - it("maps ACP cancellation to an empty strict profile", async () => { + it("maps ACP cancellation to the native empty non-strict profile", async () => { const prompt = setupSessionWithPendingPrompt(); fixture.setPermissionResponse({outcome: {outcome: "cancelled"}}); expect(await fixture.sendServerRequest("item/permissions/requestApproval", params())).toEqual({ permissions: {}, scope: "turn", - strictAutoReview: true, + strictAutoReview: false, }); await finish(prompt); }); diff --git a/src/__tests__/CodexACPAgent/data/elicitation-form-accept.json b/src/__tests__/CodexACPAgent/data/elicitation-form-accept.json deleted file mode 100644 index a2113d5c..00000000 --- a/src/__tests__/CodexACPAgent/data/elicitation-form-accept.json +++ /dev/null @@ -1,48 +0,0 @@ -{ - "method": "requestPermission", - "args": [ - { - "sessionId": "test-session-id", - "toolCall": { - "toolCallId": "elicitation-my-mcp-server", - "kind": "other", - "status": "pending", - "content": [ - { - "type": "content", - "content": { - "type": "text", - "text": "Please provide your GitHub username" - } - } - ], - "rawInput": { - "serverName": "my-mcp-server", - "schema": { - "type": "object", - "properties": { - "name": { - "type": "string" - } - }, - "required": [ - "name" - ] - } - } - }, - "options": [ - { - "optionId": "accept", - "name": "Accept", - "kind": "allow_once" - }, - { - "optionId": "decline", - "name": "Decline", - "kind": "reject_once" - } - ] - } - ] -} \ No newline at end of file diff --git a/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-all-persist.json b/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-all-persist.json index d2f40807..93bf950e 100644 --- a/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-all-persist.json +++ b/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-all-persist.json @@ -4,7 +4,7 @@ { "sessionId": "test-session-id", "toolCall": { - "toolCallId": "elicitation-tool-server", + "toolCallId": "elicitation:test-session-id:tool-server:1", "kind": "execute", "status": "pending", "content": [ @@ -29,22 +29,26 @@ { "optionId": "allow_once", "name": "Allow", - "kind": "allow_once" + "kind": "allow_once", + "_meta": "_meta" }, { "optionId": "allow_session", - "name": "Allow for This Session", - "kind": "allow_always" + "name": "Allow for this session", + "kind": "allow_always", + "_meta": "_meta" }, { "optionId": "allow_always", - "name": "Allow and Don't Ask Again", - "kind": "allow_always" + "name": "Always allow", + "kind": "allow_always", + "_meta": "_meta" }, { - "optionId": "decline", - "name": "Decline", - "kind": "reject_once" + "optionId": "cancel", + "name": "Cancel", + "kind": "reject_once", + "_meta": "_meta" } ] } 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 b212b996..d55ed702 100644 --- a/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-no-persist.json +++ b/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-no-persist.json @@ -4,7 +4,7 @@ { "sessionId": "test-session-id", "toolCall": { - "toolCallId": "elicitation-tool-server", + "toolCallId": "elicitation:test-session-id:tool-server:1", "kind": "execute", "status": "pending", "content": [ @@ -29,12 +29,14 @@ { "optionId": "allow_once", "name": "Allow", - "kind": "allow_once" + "kind": "allow_once", + "_meta": "_meta" }, { - "optionId": "decline", - "name": "Decline", - "kind": "reject_once" + "optionId": "cancel", + "name": "Cancel", + "kind": "reject_once", + "_meta": "_meta" } ] } 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 50358bee..db5ce031 100644 --- a/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-session-only.json +++ b/src/__tests__/CodexACPAgent/data/elicitation-tool-approval-session-only.json @@ -4,7 +4,7 @@ { "sessionId": "test-session-id", "toolCall": { - "toolCallId": "elicitation-tool-server", + "toolCallId": "elicitation:test-session-id:tool-server:1", "kind": "execute", "status": "pending", "content": [ @@ -29,17 +29,20 @@ { "optionId": "allow_once", "name": "Allow", - "kind": "allow_once" + "kind": "allow_once", + "_meta": "_meta" }, { "optionId": "allow_session", - "name": "Allow for This Session", - "kind": "allow_always" + "name": "Allow for this session", + "kind": "allow_always", + "_meta": "_meta" }, { - "optionId": "decline", - "name": "Decline", - "kind": "reject_once" + "optionId": "cancel", + "name": "Cancel", + "kind": "reject_once", + "_meta": "_meta" } ] } diff --git a/src/__tests__/CodexACPAgent/data/elicitation-url-accept.json b/src/__tests__/CodexACPAgent/data/elicitation-url-accept.json index da7f9c0b..c9c4de0a 100644 --- a/src/__tests__/CodexACPAgent/data/elicitation-url-accept.json +++ b/src/__tests__/CodexACPAgent/data/elicitation-url-accept.json @@ -24,13 +24,21 @@ "options": [ { "optionId": "accept", - "name": "Accept", - "kind": "allow_once" + "name": "Allow", + "kind": "allow_once", + "_meta": "_meta" }, { "optionId": "decline", - "name": "Decline", - "kind": "reject_once" + "name": "Deny", + "kind": "reject_once", + "_meta": "_meta" + }, + { + "optionId": "cancel", + "name": "Cancel", + "kind": "reject_once", + "_meta": "_meta" } ] } diff --git a/src/__tests__/CodexACPAgent/e2e/acp-e2e-file-approval.test.ts b/src/__tests__/CodexACPAgent/e2e/acp-e2e-file-approval.test.ts index 6edc1956..e5e4ff7f 100644 --- a/src/__tests__/CodexACPAgent/e2e/acp-e2e-file-approval.test.ts +++ b/src/__tests__/CodexACPAgent/e2e/acp-e2e-file-approval.test.ts @@ -2,7 +2,7 @@ import fs from "node:fs"; import path from "node:path"; import {afterEach, beforeEach, expect, it, onTestFinished} from "vitest"; import {AgentMode} from "../../../AgentMode"; -import {ApprovalOptionId} from "../../../ApprovalOptionId"; +import {ApprovalOptionId} from "../../../permissions/option-ids"; import { createAuthenticatedFixture, createPermissionResponder, @@ -33,7 +33,7 @@ describeE2E("E2E file approval tests", () => { }); it("does not apply rejected file edits", async () => { - fixture.setPermissionResponder(createPermissionResponder("edit", ApprovalOptionId.RejectOnce)); + fixture.setPermissionResponder(createPermissionResponder("edit", ApprovalOptionId.Cancel)); const sessionId = await editFileDirectly(fixture, path.join(fixture.workspaceDir, generateFileNameForTest()), false); expect(fixture.readPermissionRequests(sessionId, "edit").length).toBeGreaterThanOrEqual(1); expect(fixture.readPermissionRequests(sessionId, "execute")).toHaveLength(0); 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 e0ef5760..0cdb7fb3 100644 --- a/src/__tests__/CodexACPAgent/e2e/acp-e2e-mcp-approval.test.ts +++ b/src/__tests__/CodexACPAgent/e2e/acp-e2e-mcp-approval.test.ts @@ -2,7 +2,7 @@ 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 {McpApprovalOptionId, type McpApprovalOptionId as McpApprovalOptionIdValue} from "../../../permissions/option-ids"; import { createAuthenticatedFixture, describeE2E, @@ -42,7 +42,7 @@ function createMcpPermissionResponder(...optionIds: McpApprovalOptionIdValue[]): const queue = [...optionIds]; return (request) => createMcpPermissionResponse( isMcpPermissionRequest(request) - ? queue.shift() ?? McpApprovalOptionId.Decline + ? queue.shift() ?? McpApprovalOptionId.Cancel : null, ); } @@ -89,8 +89,8 @@ describeE2E("E2E MCP approval tests (configured in session)", () => { expectMcpPermissionRequestCount(fixture, sessionId, 1); }); - it("ends turn when MCP tool call is rejected", async () => { - fixture.setPermissionResponder(createMcpPermissionResponder(McpApprovalOptionId.Decline)); + it("ends turn when MCP tool call is cancelled", async () => { + fixture.setPermissionResponder(createMcpPermissionResponder(McpApprovalOptionId.Cancel)); const {sessionId, invocationMarkerPath} = await createMcpSession(); expectEndTurn(await fixture.connection.prompt({ 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 fdfcc55d..89387e5b 100644 --- a/src/__tests__/CodexACPAgent/e2e/acp-e2e-shell-approval.test.ts +++ b/src/__tests__/CodexACPAgent/e2e/acp-e2e-shell-approval.test.ts @@ -2,7 +2,7 @@ import fs from "node:fs"; import path from "node:path"; import {afterEach, beforeEach, expect, it, onTestFinished, vi} from "vitest"; import {AgentMode} from "../../../AgentMode"; -import {ApprovalOptionId} from "../../../ApprovalOptionId"; +import {ApprovalOptionId} from "../../../permissions/option-ids"; import { createAuthenticatedFixture, createPermissionResponder, @@ -45,10 +45,10 @@ describeE2E("E2E shell approval tests", () => { } it("prompts for every command when allow_once is selected", async () => { - const responses = [ApprovalOptionId.AllowOnce, ApprovalOptionId.RejectOnce]; + const responses = [ApprovalOptionId.AllowOnce, ApprovalOptionId.Cancel]; fixture.setPermissionResponder((request) => createPermissionResponse( request.toolCall.kind === "execute" - ? responses.shift() ?? ApprovalOptionId.RejectOnce + ? responses.shift() ?? ApprovalOptionId.Cancel : null )); await promptShellCommandTwice(); @@ -57,16 +57,16 @@ 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); expectPermissionRequests(fixture, sessionId, {execute: 1, edit: 0}); }); - it("prompts for every command when reject_once is selected", async () => { - fixture.setPermissionResponder(createPermissionResponder("execute", ApprovalOptionId.RejectOnce)); + it("cancels every command when cancel is selected", async () => { + fixture.setPermissionResponder(createPermissionResponder("execute", ApprovalOptionId.Cancel)); await promptShellCommandTwice(); expect(fs.existsSync(path.join(fixture.workspaceDir, FIRST_FILE_NAME))).toBe(false); expect(fs.existsSync(path.join(fixture.workspaceDir, SECOND_FILE_NAME))).toBe(false); diff --git a/src/__tests__/CodexACPAgent/e2e/permission-responders.ts b/src/__tests__/CodexACPAgent/e2e/permission-responders.ts index 79822a81..bd101068 100644 --- a/src/__tests__/CodexACPAgent/e2e/permission-responders.ts +++ b/src/__tests__/CodexACPAgent/e2e/permission-responders.ts @@ -1,5 +1,5 @@ import * as acp from "@agentclientprotocol/sdk"; -import {ApprovalOptionId} from "../../../ApprovalOptionId"; +import {ApprovalOptionId} from "../../../permissions/option-ids"; export type PermissionResponder = ( params: acp.RequestPermissionRequest, diff --git a/src/__tests__/CodexACPAgent/elicitation-events.test.ts b/src/__tests__/CodexACPAgent/elicitation-events.test.ts index 054c0cda..c3d44964 100644 --- a/src/__tests__/CodexACPAgent/elicitation-events.test.ts +++ b/src/__tests__/CodexACPAgent/elicitation-events.test.ts @@ -4,7 +4,7 @@ import type { McpServerElicitationRequestParams, ToolRequestUserInputParams } fr import { createCodexMockTestFixture, createTestSessionState, type CodexMockTestFixture } from '../acp-test-utils'; import type { SessionState } from '../../CodexAcpServer'; import { AgentMode } from "../../AgentMode"; -import { McpApprovalOptionId } from "../../McpApprovalOptionId"; +import { McpApprovalOptionId } from "../../permissions/option-ids"; import type { ServerNotification } from "../../app-server"; describe('Elicitation Events', () => { @@ -182,7 +182,7 @@ describe('Elicitation Events', () => { await promptPromise; }); - it('should map accept to accept', async () => { + it('should cancel a structured form when the client lacks ACP form support', async () => { const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'accept' } }); @@ -193,7 +193,8 @@ describe('Elicitation Events', () => { }; const response = await fixture.sendServerRequest('mcpServer/elicitation/request', params); - expect(response).toEqual({ action: 'accept', content: null, _meta: null }); + expect(response).toEqual({ action: 'cancel', content: null, _meta: null }); + expect(fixture.getAcpConnectionEvents([])).toEqual([]); completeTurn(); await promptPromise; @@ -216,6 +217,21 @@ describe('Elicitation Events', () => { await promptPromise; }); + it('should map the explicit non-tool Cancel option to cancel', async () => { + const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'cancel' } }); + const params: McpServerElicitationRequestParams = { + threadId: sessionId, turnId: 'turn-1', serverName: 'test-server', + mode: 'form', _meta: null, message: 'Please provide info', + requestedSchema: { type: 'object', properties: {} }, + }; + expect(await fixture.sendServerRequest('mcpServer/elicitation/request', params)) + .toEqual({ action: 'cancel', content: null, _meta: null }); + expect(fixture.getAcpConnectionEvents([]).filter(event => event.method === 'sessionUpdate')).toEqual([]); + completeTurn(); + await promptPromise; + }); + it('should return cancel when user dismisses dialog', async () => { const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); fixture.setPermissionResponse({ outcome: { outcome: 'cancelled' } }); @@ -244,7 +260,7 @@ describe('Elicitation Events', () => { expect(response).toEqual({ action: 'cancel', content: null, _meta: null }); }); - it('should build correct ACP permission request for form mode', async () => { + it('should not replace unsupported required form fields with permission buttons', async () => { const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: 'accept' } }); @@ -254,8 +270,9 @@ describe('Elicitation Events', () => { requestedSchema: { type: 'object', properties: { name: { type: 'string' } }, required: ['name'] }, }; - await fixture.sendServerRequest('mcpServer/elicitation/request', params); - await expect(fixture.getAcpConnectionDump(['_meta'])).toMatchFileSnapshot('data/elicitation-form-accept.json'); + expect(await fixture.sendServerRequest('mcpServer/elicitation/request', params)) + .toEqual({action: 'cancel', content: null, _meta: null}); + expect(fixture.getAcpConnectionEvents([])).toEqual([]); completeTurn(); await promptPromise; @@ -263,14 +280,12 @@ describe('Elicitation Events', () => { }); describe('MCP tool call approval elicitation', () => { - it('should use ACP form elicitation for MCP tool approval when supported', async () => { + it('should preserve the native permission options even when ACP form elicitation is supported', async () => { const { promptPromise, completeTurn } = await setupSessionWithPendingPromptAndCapabilities({ elicitation: { form: {} }, }); - fixture.setElicitationResponse({ - action: 'accept', - content: { persist: 'always' }, - _meta: { source: 'client' }, + fixture.setPermissionResponse({ + outcome: {outcome: 'selected', optionId: McpApprovalOptionId.AllowAlways}, }); fixture.sendServerNotification({ @@ -307,22 +322,18 @@ describe('Elicitation Events', () => { }; const response = await fixture.sendServerRequest('mcpServer/elicitation/request', params); - expect(response).toEqual({ action: 'accept', content: null, _meta: { source: 'client', persist: 'always' } }); + expect(response).toEqual({ action: 'accept', content: null, _meta: { persist: 'always' } }); const events = fixture.getAcpConnectionEvents(['_meta']); expect(events[0]).toMatchObject({ - method: 'createElicitation', + method: 'requestPermission', args: [{ sessionId, - toolCallId: 'call-id', - mode: 'form', - message: 'Allow tool call?', + toolCall: {toolCallId: 'call-id', kind: 'execute', status: 'pending'}, }], }); - expect(events[0]!.args[0].requestedSchema.properties.persist.oneOf).toEqual([ - { const: 'once', title: 'Allow once' }, - { const: 'session', title: 'Allow for this session' }, - { const: 'always', title: "Allow and don't ask again" }, + expect(events[0]!.args[0].options.map((option: {name: string}) => option.name)).toEqual([ + 'Allow', 'Allow for this session', 'Always allow', 'Cancel', ]); expect(events[1]).toEqual({ method: 'sessionUpdate', @@ -336,7 +347,25 @@ describe('Elicitation Events', () => { await promptPromise; }); - it('should show Allow/session/always/Decline options when all persist values advertised', async () => { + it('should not apply message-only tool approval semantics to a structured form', async () => { + const { promptPromise, completeTurn } = await setupSessionWithPendingPromptAndCapabilities({ + elicitation: { form: {} }, + }); + fixture.setElicitationResponse({action: 'decline'}); + const params: McpServerElicitationRequestParams = { + threadId: sessionId, turnId: 'turn-1', serverName: 'tool-server', + mode: 'form', + _meta: {codex_approval_kind: 'mcp_tool_call'}, + message: 'Collect fields', + requestedSchema: {type: 'object', properties: {value: {type: 'string'}}}, + }; + expect(await fixture.sendServerRequest('mcpServer/elicitation/request', params)) + .toEqual({action: 'decline', content: null, _meta: null}); + completeTurn(); + await promptPromise; + }); + + it('should show the native Allow/session/always/Cancel options when all persist values are advertised', async () => { const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: McpApprovalOptionId.AllowOnce } }); @@ -412,6 +441,41 @@ describe('Elicitation Events', () => { await promptPromise; }); + it('should cancel a durable permission response that Codex did not offer', async () => { + const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: McpApprovalOptionId.AllowAlways } }); + const params: McpServerElicitationRequestParams = { + threadId: sessionId, turnId: 'turn-1', serverName: 'tool-server', + mode: 'form', + _meta: { codex_approval_kind: 'mcp_tool_call' }, + message: 'Allow tool call?', + requestedSchema: { type: 'object', properties: {} }, + }; + expect(await fixture.sendServerRequest('mcpServer/elicitation/request', params)) + .toEqual({ action: 'cancel', content: null, _meta: null }); + expect(fixture.getAcpConnectionEvents([]).filter(event => event.method === 'sessionUpdate')).toEqual([]); + completeTurn(); + await promptPromise; + }); + + it('should cancel an ACP form persist value that Codex did not offer', async () => { + const { promptPromise, completeTurn } = await setupSessionWithPendingPromptAndCapabilities({ + elicitation: { form: {} }, + }); + fixture.setElicitationResponse({ action: 'accept', content: { persist: 'always' } }); + const params: McpServerElicitationRequestParams = { + threadId: sessionId, turnId: 'turn-1', serverName: 'tool-server', + mode: 'form', + _meta: { codex_approval_kind: 'mcp_tool_call' }, + message: 'Allow tool call?', + requestedSchema: { type: 'object', properties: {} }, + }; + expect(await fixture.sendServerRequest('mcpServer/elicitation/request', params)) + .toEqual({ action: 'cancel', content: null, _meta: null }); + completeTurn(); + await promptPromise; + }); + it('should only show session option when persist is "session"', async () => { const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: McpApprovalOptionId.AllowOnce } }); @@ -431,7 +495,7 @@ describe('Elicitation Events', () => { 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 } }); @@ -450,6 +514,63 @@ describe('Elicitation Events', () => { await promptPromise; }); + it('should map explicit tool approval Cancel to cancel without marking the tool in progress', async () => { + const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: McpApprovalOptionId.Cancel } }); + const params: McpServerElicitationRequestParams = { + threadId: sessionId, turnId: 'turn-1', serverName: 'tool-server', + mode: 'form', + _meta: { codex_approval_kind: 'mcp_tool_call' }, + message: 'Allow tool call?', + requestedSchema: { type: 'object', properties: {} }, + }; + expect(await fixture.sendServerRequest('mcpServer/elicitation/request', params)) + .toEqual({ action: 'cancel', content: null, _meta: null }); + expect(fixture.getAcpConnectionEvents([]).filter(event => event.method === 'sessionUpdate')).toEqual([]); + completeTurn(); + await promptPromise; + }); + + it('should render an ambiguous concurrent same-server approval as a standalone request', async () => { + const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); + fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: McpApprovalOptionId.Cancel } }); + for (const id of ['call-a', 'call-b']) { + fixture.sendServerNotification({ + method: 'item/started', + params: { + threadId: sessionId, + turnId: 'turn-1', + startedAtMs: 0, + item: { + type: 'mcpToolCall', id, server: 'tool-server', tool: 'tool-name', + status: 'inProgress', arguments: {id}, appContext: null, readOnlyHint: null, + pluginId: null, result: null, error: null, durationMs: null, + }, + }, + }); + } + await fixture.getCodexAcpClient().waitForSessionNotifications(sessionId); + fixture.clearAcpConnectionDump(); + const params: McpServerElicitationRequestParams = { + threadId: sessionId, turnId: 'turn-1', serverName: 'tool-server', + mode: 'form', _meta: {codex_approval_kind: 'mcp_tool_call'}, + message: 'Allow one of the concurrent calls?', + requestedSchema: {type: 'object', properties: {}}, + }; + expect(await fixture.sendServerRequest('mcpServer/elicitation/request', params)) + .toEqual({action: 'cancel', content: null, _meta: null}); + const request = fixture.getAcpConnectionEvents([]).find(event => event.method === 'requestPermission'); + expect(request?.args[0].toolCall).toMatchObject({ + toolCallId: 'elicitation:test-session-id:tool-server:1', + content: [{type: 'content', content: {type: 'text', text: 'Allow one of the concurrent calls?'}}], + rawInput: {serverName: 'tool-server', schema: {type: 'object', properties: {}}}, + }); + expect(request?.args[0].toolCall.toolCallId).not.toBe('call-a'); + expect(request?.args[0].toolCall.toolCallId).not.toBe('call-b'); + completeTurn(); + await promptPromise; + }); + it('should not reuse a completed auto-approved call id for a later approval request', async () => { const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); fixture.setPermissionResponse({ outcome: { outcome: 'selected', optionId: McpApprovalOptionId.AllowOnce } }); @@ -516,7 +637,7 @@ describe('Elicitation Events', () => { const [requestPermissionEvent] = fixture.getAcpConnectionEvents(['_meta']); expect(requestPermissionEvent?.method).toBe('requestPermission'); - expect(requestPermissionEvent?.args[0].toolCall.toolCallId).toBe('elicitation-tool-server'); + expect(requestPermissionEvent?.args[0].toolCall.toolCallId).toBe('elicitation:test-session-id:tool-server:1'); completeTurn(); await promptPromise; @@ -573,7 +694,7 @@ describe('Elicitation Events', () => { const [requestPermissionEvent] = fixture.getAcpConnectionEvents(['_meta']); expect(requestPermissionEvent?.method).toBe('requestPermission'); - expect(requestPermissionEvent?.args[0].toolCall.toolCallId).toBe('elicitation-tool-server'); + expect(requestPermissionEvent?.args[0].toolCall.toolCallId).toBe('elicitation:test-session-id:tool-server:1'); completeTurn(); await promptPromise; diff --git a/src/CodexApprovalHandler.ts b/src/permissions/CodexApprovalHandler.ts similarity index 82% rename from src/CodexApprovalHandler.ts rename to src/permissions/CodexApprovalHandler.ts index 761a3139..10bb3cf7 100644 --- a/src/CodexApprovalHandler.ts +++ b/src/permissions/CodexApprovalHandler.ts @@ -1,6 +1,6 @@ import * as acp from "@agentclientprotocol/sdk"; -import type {SessionState} from "./CodexAcpServer"; -import type {ApprovalHandler} from "./CodexAppServerClient"; +import type {SessionState} from "../CodexAcpServer"; +import type {ApprovalHandler} from "../CodexAppServerClient"; import type { CommandExecutionRequestApprovalParams, CommandExecutionRequestApprovalResponse, @@ -10,30 +10,26 @@ import type { PermissionsRequestApprovalParams, PermissionsRequestApprovalResponse, RequestPermissionProfile, -} from "./app-server/v2"; -import {logger} from "./Logger"; -import {ApprovalOptionId} from "./ApprovalOptionId"; -import type {AcpClientConnection} from "./ACPSessionConnection"; +} from "../app-server/v2"; +import {logger} from "../Logger"; +import type {AcpClientConnection} from "../ACPSessionConnection"; import { commandDecisionOptions, fileChangeDecisionOptions, permissionProfileOptions, type CommandParamsWithAvailableDecisions, type DecisionOption, -} from "./CodexApprovalOptions"; +} from "./options"; +import {ApprovalOptionId} from "./option-ids"; import { CODEX_ADDITIONAL_PERMISSIONS_TITLE, CODEX_COMMAND_PERMISSION_TITLE, CODEX_FILE_CHANGE_PERMISSION_TITLE, CODEX_NETWORK_PERMISSION_TITLE, requestPermissionMeta, -} from "./CodexPermissionMetadata"; -import { - additionalPermissionsToolCall, - commandToolCall, - fileChangeToolCall, -} from "./CodexPermissionPresentation"; -import type {CodexApprovalPresentationStore} from "./CodexApprovalPresentationStore"; +} from "./metadata"; +import {additionalPermissionsToolCall, commandToolCall, fileChangeToolCall} from "./presentation"; +import type {CodexApprovalPresentationStore} from "./presentation-store"; export class CodexApprovalHandler implements ApprovalHandler { constructor( @@ -58,12 +54,10 @@ export class CodexApprovalHandler implements ApprovalHandler { try { const response = await this.requestPermission({ sessionId: this.sessionState.sessionId, - toolCall: commandToolCall(params), + toolCall: commandToolCall(authoritativeParams), options: decisions.map(({option}) => option), _meta: requestPermissionMeta( - params.networkApprovalContext - ? CODEX_NETWORK_PERMISSION_TITLE - : CODEX_COMMAND_PERMISSION_TITLE, + params.networkApprovalContext ? CODEX_NETWORK_PERMISSION_TITLE : CODEX_COMMAND_PERMISSION_TITLE, params.reason, ), }); @@ -74,9 +68,7 @@ export class CodexApprovalHandler implements ApprovalHandler { } } - async handleFileChange( - params: FileChangeRequestApprovalParams, - ): Promise { + async handleFileChange(params: FileChangeRequestApprovalParams): Promise { if (this.isStale(params.turnId)) return {decision: "cancel"}; const decisions = fileChangeDecisionOptions(); try { @@ -124,10 +116,7 @@ export class CodexApprovalHandler implements ApprovalHandler { ); } - private selectedDecision( - response: acp.RequestPermissionResponse, - decisions: DecisionOption[], - ): T | undefined { + private selectedDecision(response: acp.RequestPermissionResponse, decisions: DecisionOption[]): T | undefined { if (response.outcome.outcome === "cancelled") return undefined; const optionId = response.outcome.optionId; return decisions.find(({option}) => option.optionId === optionId)?.decision; @@ -140,9 +129,11 @@ export class CodexApprovalHandler implements ApprovalHandler { if (response.outcome.outcome === "cancelled") return this.rejectPermissionsResponse(); switch (response.outcome.optionId) { case ApprovalOptionId.AllowPermissionsForTurn: - return this.grantedPermissionsResponse(permissions, "turn"); + return this.grantedPermissionsResponse(permissions, "turn", false); + case ApprovalOptionId.AllowPermissionsForTurnWithStrictAutoReview: + return this.grantedPermissionsResponse(permissions, "turn", true); case ApprovalOptionId.AllowPermissionsForSession: - return this.grantedPermissionsResponse(permissions, "session"); + return this.grantedPermissionsResponse(permissions, "session", false); case ApprovalOptionId.RejectPermissions: default: return this.rejectPermissionsResponse(); @@ -152,16 +143,13 @@ export class CodexApprovalHandler implements ApprovalHandler { private grantedPermissionsResponse( permissions: RequestPermissionProfile, scope: "turn" | "session", + strictAutoReview: boolean, ): PermissionsRequestApprovalResponse { - return { - permissions: this.grantedPermissions(permissions), - scope, - strictAutoReview: false, - }; + return {permissions: this.grantedPermissions(permissions), scope, strictAutoReview}; } private rejectPermissionsResponse(): PermissionsRequestApprovalResponse { - return {permissions: {}, scope: "turn", strictAutoReview: true}; + return {permissions: {}, scope: "turn", strictAutoReview: false}; } private grantedPermissions(permissions: RequestPermissionProfile): GrantedPermissionProfile { diff --git a/src/permissions/mcp.ts b/src/permissions/mcp.ts new file mode 100644 index 00000000..bba1a18c --- /dev/null +++ b/src/permissions/mcp.ts @@ -0,0 +1,194 @@ +import type * as acp from "@agentclientprotocol/sdk"; +import type {JsonValue} from "../app-server/serde_json/JsonValue"; +import type { + McpServerElicitationRequestParams, + McpServerElicitationRequestResponse, +} from "../app-server/v2"; +import {optionPermissionMeta} from "./metadata"; +import {McpApprovalOptionId} from "./option-ids"; + +export type PersistValue = "session" | "always"; + +export type McpElicitationContext = { + isToolApproval: boolean; + persistOptions: Set; + correlatedCallId: string | undefined; + standaloneToolCallId: string; +}; + +export function parsePersistOptions(meta: unknown): Set { + const result = new Set(); + if (!isRecord(meta)) return result; + const persist = meta["persist"]; + if (persist === "session") result.add("session"); + else if (persist === "always") result.add("always"); + else if (Array.isArray(persist)) { + if (persist.includes("session")) result.add("session"); + if (persist.includes("always")) result.add("always"); + } + return result; +} + +export function isMcpToolCallApproval(meta: unknown): boolean { + return isRecord(meta) && meta["codex_approval_kind"] === "mcp_tool_call"; +} + +export function buildMcpPermissionOptions( + isToolApproval: boolean, + persistOptions: Set, +): acp.PermissionOption[] { + const options: acp.PermissionOption[] = [permissionOption( + isToolApproval ? McpApprovalOptionId.AllowOnce : "accept", + "Allow", + "allow_once", + isToolApproval ? "Run the tool and continue." : "Allow this request and continue.", + )]; + if (persistOptions.has("session")) { + options.push(permissionOption( + McpApprovalOptionId.AllowSession, + "Allow for this session", + "allow_always", + isToolApproval + ? "Run the tool and remember this choice for this session." + : "Allow this request and remember this choice for this session.", + )); + } + if (persistOptions.has("always")) { + options.push(permissionOption( + McpApprovalOptionId.AllowAlways, + "Always allow", + "allow_always", + isToolApproval + ? "Run the tool and remember this choice for future tool calls." + : "Allow this request and remember this choice for future requests.", + )); + } + if (isToolApproval) { + options.push(permissionOption( + McpApprovalOptionId.Cancel, + "Cancel", + "reject_once", + "Cancel this tool call", + )); + } else { + options.push( + permissionOption( + McpApprovalOptionId.Decline, + "Deny", + "reject_once", + "Decline this request and continue.", + ), + permissionOption(McpApprovalOptionId.Cancel, "Cancel", "reject_once", "Cancel this request"), + ); + } + return options; +} + +export function buildMcpPermissionRequest( + sessionId: string, + params: McpServerElicitationRequestParams, + context: McpElicitationContext, +): {request: acp.RequestPermissionRequest; correlatedCallId: string | undefined} { + const messageContent: acp.ToolCallContent = { + type: "content", + content: {type: "text", text: params.message}, + }; + const options = buildMcpPermissionOptions(context.isToolApproval, context.persistOptions); + if (params.mode === "form" || params.mode === "openai/form") { + if (context.correlatedCallId !== undefined) { + return { + request: { + sessionId, + toolCall: { + toolCallId: context.correlatedCallId, + kind: "execute", + status: "pending", + }, + _meta: {is_mcp_tool_approval: true}, + options, + }, + correlatedCallId: context.correlatedCallId, + }; + } + return { + request: { + sessionId, + toolCall: { + toolCallId: context.standaloneToolCallId, + kind: context.isToolApproval ? "execute" : "other", + status: "pending", + content: [messageContent], + rawInput: {serverName: params.serverName, schema: params.requestedSchema}, + }, + ...(context.isToolApproval ? {_meta: {is_mcp_tool_approval: true}} : {}), + options, + }, + correlatedCallId: undefined, + }; + } + return { + request: { + sessionId, + toolCall: { + toolCallId: `elicitation-${params.elicitationId}`, + kind: "fetch", + status: "pending", + content: [messageContent], + rawInput: {serverName: params.serverName, url: params.url}, + }, + options, + }, + correlatedCallId: undefined, + }; +} + +export function convertMcpPermissionResponse( + response: acp.RequestPermissionResponse, + isToolApproval: boolean, + persistOptions: ReadonlySet, +): McpServerElicitationRequestResponse { + if (response.outcome.outcome === "cancelled") return cancelledResponse(); + switch (response.outcome.optionId) { + case McpApprovalOptionId.AllowSession: + return persistOptions.has("session") + ? {action: "accept", content: null, _meta: {persist: "session"}} + : cancelledResponse(); + case McpApprovalOptionId.AllowAlways: + return persistOptions.has("always") + ? {action: "accept", content: null, _meta: {persist: "always"}} + : cancelledResponse(); + case McpApprovalOptionId.AllowOnce: + return isToolApproval + ? {action: "accept", content: null, _meta: null} + : cancelledResponse(); + case "accept": + return !isToolApproval + ? {action: "accept", content: null, _meta: null} + : cancelledResponse(); + case McpApprovalOptionId.Decline: + return !isToolApproval + ? {action: "decline", content: null, _meta: null} + : cancelledResponse(); + case McpApprovalOptionId.Cancel: + default: + return cancelledResponse(); + } +} + +function permissionOption( + optionId: string, + name: string, + kind: acp.PermissionOptionKind, + description: string, +): acp.PermissionOption { + const meta = optionPermissionMeta(description); + return {optionId, name, kind, ...(meta ? {_meta: meta} : {})}; +} + +function cancelledResponse(): McpServerElicitationRequestResponse { + return {action: "cancel", content: null, _meta: null as JsonValue | null}; +} + +function isRecord(value: unknown): value is Record { + return value !== null && typeof value === "object" && !Array.isArray(value); +} diff --git a/src/CodexPermissionMetadata.ts b/src/permissions/metadata.ts similarity index 87% rename from src/CodexPermissionMetadata.ts rename to src/permissions/metadata.ts index 00b0b52a..2583ff78 100644 --- a/src/CodexPermissionMetadata.ts +++ b/src/permissions/metadata.ts @@ -33,13 +33,8 @@ export function optionPermissionMeta( description?: string | null, ): acp.PermissionOption["_meta"] | undefined { const normalized = nonBlank(description); - if (!normalized) { - return undefined; - } - const permission: OptionPermissionMetadata = { - version: 1, - description: normalized, - }; + if (!normalized) return undefined; + const permission: OptionPermissionMetadata = {version: 1, description: normalized}; return {permission}; } diff --git a/src/ApprovalOptionId.ts b/src/permissions/option-ids.ts similarity index 52% rename from src/ApprovalOptionId.ts rename to src/permissions/option-ids.ts index 45d05d90..4fc684cd 100644 --- a/src/ApprovalOptionId.ts +++ b/src/permissions/option-ids.ts @@ -1,12 +1,24 @@ export const ApprovalOptionId = { AllowOnce: "allow_once", - AllowAlways: "allow_always", - RejectOnce: "reject_once", + AllowForSession: "allow_for_session", + Decline: "decline", + Cancel: "cancel", AcceptWithExecpolicyAmendment: "accept_execpolicy_amendment", ApplyNetworkPolicyAmendment: "apply_network_policy_amendment", AllowPermissionsForTurn: "allow_permissions_turn", + AllowPermissionsForTurnWithStrictAutoReview: "allow_permissions_turn_strict_auto_review", AllowPermissionsForSession: "allow_permissions_session", RejectPermissions: "reject_permissions", } as const; export type ApprovalOptionId = typeof ApprovalOptionId[keyof typeof ApprovalOptionId]; + +export const McpApprovalOptionId = { + AllowOnce: "allow_once", + AllowSession: "allow_session", + AllowAlways: "allow_always", + Decline: "decline", + Cancel: "cancel", +} as const; + +export type McpApprovalOptionId = typeof McpApprovalOptionId[keyof typeof McpApprovalOptionId]; diff --git a/src/permissions/options.ts b/src/permissions/options.ts new file mode 100644 index 00000000..fe00c8e5 --- /dev/null +++ b/src/permissions/options.ts @@ -0,0 +1,322 @@ +import type * as acp from "@agentclientprotocol/sdk"; +import type { + AdditionalPermissionProfile, + CommandExecutionApprovalDecision, + CommandExecutionRequestApprovalParams, + FileChangeApprovalDecision, + NetworkPolicyAmendment, +} from "../app-server/v2"; +import {ApprovalOptionId} from "./option-ids"; + +export type DecisionOption = {option: acp.PermissionOption; decision: T}; + +export type CommandParamsWithAvailableDecisions = CommandExecutionRequestApprovalParams & { + additionalPermissions?: AdditionalPermissionProfile | null; + availableDecisions?: unknown; +}; + +export function commandDecisionOptions( + params: CommandParamsWithAvailableDecisions, +): DecisionOption[] | undefined { + const decisions = parseAvailableCommandDecisions(params); + if (!decisions) return undefined; + + const options: DecisionOption[] = []; + let networkIndex = 0; + for (const decision of decisions) { + if (decision === "accept") { + options.push(decisionOption( + ApprovalOptionId.AllowOnce, + params.networkApprovalContext ? "Yes, just this once" : "Yes, proceed", + "allow_once", + decision, + )); + } else if (decision === "acceptForSession") { + options.push(decisionOption( + ApprovalOptionId.AllowForSession, + params.networkApprovalContext + ? "Yes, and allow this host for this conversation" + : params.additionalPermissions + ? "Yes, and allow these permissions for this session" + : "Yes, and don't ask again for this command in this session", + "allow_always", + decision, + )); + } else if (decision === "decline") { + options.push(decisionOption( + ApprovalOptionId.Decline, + "No, continue without running it", + "reject_once", + decision, + )); + } else if (decision === "cancel") { + options.push(decisionOption( + ApprovalOptionId.Cancel, + "No, and tell Codex what to do differently", + "reject_once", + decision, + )); + } else if ("acceptWithExecpolicyAmendment" in decision) { + const prefix = renderExecPolicyPrefix(decision.acceptWithExecpolicyAmendment.execpolicy_amendment); + if (prefix.includes("\n") || prefix.includes("\r")) continue; + options.push(decisionOption( + ApprovalOptionId.AcceptWithExecpolicyAmendment, + `Yes, and don't ask again for commands that start with \`${prefix}\``, + "allow_always", + decision, + )); + } else { + const amendment = decision.applyNetworkPolicyAmendment.network_policy_amendment; + options.push(decisionOption( + `${ApprovalOptionId.ApplyNetworkPolicyAmendment}:${networkIndex++}`, + amendment.action === "allow" + ? "Yes, and allow this host in the future" + : "No, and block this host in the future", + amendment.action === "allow" ? "allow_always" : "reject_always", + decision, + )); + } + } + + const orderedOptions = [...options].sort( + (left, right) => permissionOptionOrder(left.option) - permissionOptionOrder(right.option), + ); + const hasAllow = orderedOptions.some(({option}) => option.kind === "allow_once" || option.kind === "allow_always"); + const hasReject = orderedOptions.some(({option}) => option.kind === "reject_once" || option.kind === "reject_always"); + const optionIds = orderedOptions.map(({option}) => option.optionId); + const hasUniqueOptionIds = new Set(optionIds).size === optionIds.length; + return hasAllow && hasReject && hasUniqueOptionIds ? orderedOptions : undefined; +} + +function permissionOptionOrder(option: acp.PermissionOption): number { + if (option.kind === "allow_once") return 0; + if (option.kind === "allow_always") return 1; + return 2; +} + +export function fileChangeDecisionOptions(): DecisionOption[] { + return [ + decisionOption(ApprovalOptionId.AllowOnce, "Yes, proceed", "allow_once", "accept"), + decisionOption( + ApprovalOptionId.AllowForSession, + "Yes, and don't ask again for these files", + "allow_always", + "acceptForSession", + ), + decisionOption( + ApprovalOptionId.Cancel, + "No, and tell Codex what to do differently", + "reject_once", + "cancel", + ), + ]; +} + +export function permissionProfileOptions(): acp.PermissionOption[] { + return [ + permissionOption( + ApprovalOptionId.AllowPermissionsForTurn, + "Yes, grant these permissions for this turn", + "allow_once", + ), + permissionOption( + ApprovalOptionId.AllowPermissionsForTurnWithStrictAutoReview, + "Yes, grant for this turn with strict auto review", + "allow_once", + ), + permissionOption( + ApprovalOptionId.AllowPermissionsForSession, + "Yes, grant these permissions for this session", + "allow_always", + ), + permissionOption( + ApprovalOptionId.RejectPermissions, + "No, continue without permissions", + "reject_once", + ), + ]; +} + +function parseAvailableCommandDecisions( + params: CommandParamsWithAvailableDecisions, +): CommandExecutionApprovalDecision[] | undefined { + if (params.availableDecisions === undefined || params.availableDecisions === null) { + return defaultCommandDecisions(params); + } + if (!Array.isArray(params.availableDecisions) || params.availableDecisions.length === 0) return undefined; + const decisions: CommandExecutionApprovalDecision[] = []; + for (const candidate of params.availableDecisions) { + const decision = parseCommandDecision(candidate, params); + if (!decision) return undefined; + decisions.push(decision); + } + return decisions; +} + +function defaultCommandDecisions( + params: CommandParamsWithAvailableDecisions, +): CommandExecutionApprovalDecision[] { + if (params.networkApprovalContext) { + const decisions: CommandExecutionApprovalDecision[] = ["accept", "acceptForSession"]; + const allowAmendment = params.proposedNetworkPolicyAmendments?.find(amendment => amendment.action === "allow"); + if (allowAmendment) { + decisions.push({applyNetworkPolicyAmendment: {network_policy_amendment: allowAmendment}}); + } + decisions.push("cancel"); + return decisions; + } + if (params.additionalPermissions) return ["accept", "cancel"]; + const decisions: CommandExecutionApprovalDecision[] = ["accept"]; + if (params.proposedExecpolicyAmendment) { + decisions.push({ + acceptWithExecpolicyAmendment: {execpolicy_amendment: params.proposedExecpolicyAmendment}, + }); + } + decisions.push("cancel"); + return decisions; +} + +function parseCommandDecision( + candidate: unknown, + params: CommandExecutionRequestApprovalParams, +): CommandExecutionApprovalDecision | undefined { + if (candidate === "accept" || candidate === "acceptForSession" || candidate === "decline" || candidate === "cancel") { + return candidate; + } + if (!isRecord(candidate)) return undefined; + + if ("acceptWithExecpolicyAmendment" in candidate) { + const value = candidate["acceptWithExecpolicyAmendment"]; + if (!isRecord(value)) return undefined; + const amendment = value["execpolicy_amendment"]; + if (!isStringArray(amendment) || amendment.length === 0) return undefined; + if (!sameStrings(amendment, params.proposedExecpolicyAmendment)) return undefined; + return {acceptWithExecpolicyAmendment: {execpolicy_amendment: [...amendment]}}; + } + + if ("applyNetworkPolicyAmendment" in candidate) { + const value = candidate["applyNetworkPolicyAmendment"]; + if (!isRecord(value)) return undefined; + const amendment = parseNetworkAmendment(value["network_policy_amendment"]); + if (!amendment || !params.networkApprovalContext) return undefined; + if (amendment.host !== params.networkApprovalContext.host) return undefined; + if (!(params.proposedNetworkPolicyAmendments ?? []).some(proposed => sameNetworkAmendment(proposed, amendment))) { + return undefined; + } + return {applyNetworkPolicyAmendment: {network_policy_amendment: amendment}}; + } + return undefined; +} + +function decisionOption( + optionId: string, + name: string, + kind: acp.PermissionOptionKind, + decision: T, +): DecisionOption { + return {option: permissionOption(optionId, name, kind), decision}; +} + +function permissionOption( + optionId: string, + name: string, + kind: acp.PermissionOptionKind, +): acp.PermissionOption { + return {optionId, name, kind}; +} + +function renderExecPolicyPrefix(command: readonly string[]): string { + const script = extractWrappedScript(command); + if (script !== undefined) return script; + if (command.some(value => value.includes("\0"))) return command.join(" "); + return command.map(shlexQuote).join(" "); +} + +function extractWrappedScript(command: readonly string[]): string | undefined { + const executable = executableName(command[0]); + if ((executable === "bash" || executable === "zsh" || executable === "sh") + && command.length === 3 + && (command[1] === "-lc" || command[1] === "-c")) { + return command[2]; + } + if (executable !== "pwsh" && executable !== "powershell") return undefined; + const allowedFlags = new Set(["-nologo", "-noprofile", "-command", "-c"]); + for (let index = 1; index + 1 < command.length; index++) { + const flag = command[index]?.toLowerCase(); + if (flag === undefined || !allowedFlags.has(flag)) return undefined; + if (flag === "-command" || flag === "-c") return command[index + 1]; + } + return undefined; +} + +function executableName(command: string | undefined): string | undefined { + let filename = command?.replaceAll("\\", "/").split("/").at(-1); + while (filename !== undefined) { + if (["bash", "zsh", "sh", "pwsh", "powershell"].includes(filename)) return filename; + const dot = filename.lastIndexOf("."); + if (dot <= 0) return undefined; + filename = filename.slice(0, dot); + } + return undefined; +} + +function shlexQuote(value: string): string { + if (value.length === 0) return "''"; + const UNQUOTED = 1; + const SINGLE_QUOTED = 2; + const DOUBLE_QUOTED = 4; + let offset = 0; + let result = ""; + while (offset < value.length) { + const start = offset; + let allowed = UNQUOTED | SINGLE_QUOTED | DOUBLE_QUOTED; + if (value[offset] === "^") { + allowed = SINGLE_QUOTED; + offset++; + } + while (offset < value.length) { + const character = value[offset]!; + let nextAllowed = allowed; + if (!isShlexUnquoted(character)) nextAllowed &= ~UNQUOTED; + if (character === "'" || character === "^" || character === "\\") nextAllowed &= ~SINGLE_QUOTED; + if (character === "`" || character === "$" || character === "!" || character === "^") { + nextAllowed &= ~DOUBLE_QUOTED; + } + if (nextAllowed === 0) break; + allowed = nextAllowed; + offset++; + } + const chunk = value.slice(start, offset); + if ((allowed & UNQUOTED) !== 0) result += chunk; + else if ((allowed & SINGLE_QUOTED) !== 0) result += `'${chunk}'`; + else result += `"${chunk.replace(/["\\]/g, "\\$&")}"`; + } + return result; +} + +function isShlexUnquoted(character: string): boolean { + return /^[0-9A-Za-z]$/.test(character) || "+-./:@]_".includes(character); +} + +function parseNetworkAmendment(value: unknown): NetworkPolicyAmendment | undefined { + if (!isRecord(value) || typeof value["host"] !== "string") return undefined; + const action = value["action"]; + if (action !== "allow" && action !== "deny") return undefined; + return {host: value["host"], action}; +} + +function isRecord(value: unknown): value is Record { + return value !== null && typeof value === "object" && !Array.isArray(value); +} + +function isStringArray(value: unknown): value is string[] { + return Array.isArray(value) && value.every(entry => typeof entry === "string"); +} + +function sameStrings(left: readonly string[], right?: readonly string[] | null): boolean { + return !!right && left.length === right.length && left.every((value, index) => value === right[index]); +} + +function sameNetworkAmendment(left: NetworkPolicyAmendment, right: NetworkPolicyAmendment): boolean { + return left.host === right.host && left.action === right.action; +} diff --git a/src/permissions/plan-review.ts b/src/permissions/plan-review.ts new file mode 100644 index 00000000..fc0f11f6 --- /dev/null +++ b/src/permissions/plan-review.ts @@ -0,0 +1,38 @@ +import type * as acp from "@agentclientprotocol/sdk"; +import type {CompletedPlan} from "../CodexEventHandler"; + +const IMPLEMENT_PLAN_OPTION_ID = "implement_plan"; +const REVISE_PLAN_OPTION_ID = "revise_plan"; + +export function planImplementationPermissionRequest( + sessionId: string, + plan: CompletedPlan, +): acp.RequestPermissionRequest { + return { + sessionId, + toolCall: { + toolCallId: planImplementationToolCallId(plan), + title: "Implement this plan?", + kind: "switch_mode", + status: "pending", + rawInput: {plan: plan.text}, + }, + options: [ + {optionId: IMPLEMENT_PLAN_OPTION_ID, name: "Yes, implement this plan", kind: "allow_once"}, + { + optionId: REVISE_PLAN_OPTION_ID, + name: "No, and tell Codex what to do differently", + kind: "reject_once", + }, + ], + _meta: {codex: {kind: "plan_review", planItemId: plan.itemId}}, + }; +} + +export function planImplementationApproved(response: acp.RequestPermissionResponse): boolean { + return response.outcome.outcome === "selected" && response.outcome.optionId === IMPLEMENT_PLAN_OPTION_ID; +} + +export function planImplementationToolCallId(plan: CompletedPlan): string { + return `plan-review:${plan.itemId}`; +} diff --git a/src/CodexApprovalPresentationStore.ts b/src/permissions/presentation-store.ts similarity index 91% rename from src/CodexApprovalPresentationStore.ts rename to src/permissions/presentation-store.ts index 14158140..742e88eb 100644 --- a/src/CodexApprovalPresentationStore.ts +++ b/src/permissions/presentation-store.ts @@ -1,5 +1,5 @@ -import type {ServerNotification} from "./app-server"; -import type {ThreadItem} from "./app-server/v2"; +import type {ServerNotification} from "../app-server"; +import type {ThreadItem} from "../app-server/v2"; type FileChangeItem = ThreadItem & {type: "fileChange"}; diff --git a/src/CodexPermissionPresentation.ts b/src/permissions/presentation.ts similarity index 70% rename from src/CodexPermissionPresentation.ts rename to src/permissions/presentation.ts index b06f9c3f..b0ea3b18 100644 --- a/src/CodexPermissionPresentation.ts +++ b/src/permissions/presentation.ts @@ -1,24 +1,33 @@ import type * as acp from "@agentclientprotocol/sdk"; import type { + AdditionalPermissionProfile, CommandAction, CommandExecutionRequestApprovalParams, FileChangeRequestApprovalParams, RequestPermissionProfile, ThreadItem, -} from "./app-server/v2"; -import {stripShellPrefix} from "./CodexEventHandler"; -import type {CodexApprovalPresentationStore} from "./CodexApprovalPresentationStore"; +} from "../app-server/v2"; +import {stripShellPrefix} from "../CommandUtils"; +import type {CodexApprovalPresentationStore} from "./presentation-store"; type FileChangeItem = ThreadItem & {type: "fileChange"}; +type CommandPresentationParams = CommandExecutionRequestApprovalParams & { + additionalPermissions?: AdditionalPermissionProfile | null; +}; -export function commandToolCall( - params: CommandExecutionRequestApprovalParams, -): acp.ToolCallUpdate { +export function commandToolCall(params: CommandPresentationParams): acp.ToolCallUpdate { const network = params.networkApprovalContext; const rawInput = { ...(params.command ? {command: stripShellPrefix(params.command)} : {}), ...(params.cwd ? {cwd: params.cwd} : {}), + ...(network?.protocol === "http" || network?.protocol === "https" + ? {url: `${network.protocol}://${network.host}`} + : {}), + ...(params.additionalPermissions ? {additionalPermissions: params.additionalPermissions} : {}), }; + const additionalPermissionContent = params.additionalPermissions + ? permissionProfileContent(params.additionalPermissions) + : []; return { toolCallId: params.itemId, kind: "execute", @@ -27,8 +36,15 @@ export function commandToolCall( ? `${network.protocol} network access to ${network.host}` : commandTitle(params.commandActions), ...(Object.keys(rawInput).length > 0 ? {rawInput} : {}), - ...locationsField(commandActionPaths(params.commandActions)), - ...(network ? {content: [textContent(`${network.protocol} access to ${network.host}`)]} : {}), + ...locationsField(unique([ + ...commandActionPaths(params.commandActions), + ...permissionProfilePaths(params.additionalPermissions), + ])), + ...(network + ? {content: [textContent(`${network.protocol} access to ${network.host}`), ...additionalPermissionContent]} + : additionalPermissionContent.length > 0 + ? {content: additionalPermissionContent} + : {}), }; } @@ -97,17 +113,18 @@ function fileChangePaths(item?: FileChangeItem): string[] { return unique(item?.changes.map(change => change.path) ?? []); } -function permissionProfilePaths(permissions: RequestPermissionProfile): string[] { - const fileSystem = permissions.fileSystem; +function permissionProfilePaths(permissions?: RequestPermissionProfile | AdditionalPermissionProfile | null): string[] { + const fileSystem = permissions?.fileSystem; return unique([ ...(fileSystem?.read ?? []), ...(fileSystem?.write ?? []), - ...(fileSystem?.entries ?? []).flatMap(entry => - entry.path.type === "path" ? [entry.path.path] : []), + ...(fileSystem?.entries ?? []).flatMap(entry => entry.path.type === "path" ? [entry.path.path] : []), ]); } -function permissionProfileContent(permissions: RequestPermissionProfile): acp.ToolCallContent[] { +function permissionProfileContent( + permissions: RequestPermissionProfile | AdditionalPermissionProfile, +): acp.ToolCallContent[] { const lines: string[] = []; const networkEnabled = permissions.network?.enabled; if (networkEnabled !== null && networkEnabled !== undefined) { From f86f25d2bcb6a9999d62b5b44b39a569b494eda9 Mon Sep 17 00:00:00 2001 From: Nikita Ashikhmin Date: Wed, 19 Aug 2026 12:10:01 +0400 Subject: [PATCH 3/3] refactor: isolate permission lifecycle state --- src/CodexAcpServer.ts | 21 ++- src/CodexElicitationHandler.ts | 123 ++------------- .../CodexACPAgent/approval-events.test.ts | 2 +- .../PermissionLifecycleContext.test.ts | 142 ++++++++++++++++++ src/permissions/CodexApprovalHandler.ts | 14 +- src/permissions/command-decision-contract.ts | 102 +++++++++++++ src/permissions/json.ts | 32 ++++ src/permissions/lifecycle.ts | 101 +++++++++++++ src/permissions/mcp.ts | 9 +- src/permissions/options.ts | 108 +------------ src/permissions/presentation-store.ts | 33 ---- src/permissions/presentation.ts | 6 +- 12 files changed, 421 insertions(+), 272 deletions(-) create mode 100644 src/__tests__/PermissionLifecycleContext.test.ts create mode 100644 src/permissions/command-decision-contract.ts create mode 100644 src/permissions/json.ts create mode 100644 src/permissions/lifecycle.ts delete mode 100644 src/permissions/presentation-store.ts diff --git a/src/CodexAcpServer.ts b/src/CodexAcpServer.ts index 1c04edd2..8d70b176 100644 --- a/src/CodexAcpServer.ts +++ b/src/CodexAcpServer.ts @@ -2,7 +2,7 @@ import * as acp from "@agentclientprotocol/sdk"; import {RequestError, type SessionId, type SessionModeState} from "@agentclientprotocol/sdk"; import {CodexEventHandler, type CompletedPlan} from "./CodexEventHandler"; import {CodexApprovalHandler} from "./permissions/CodexApprovalHandler"; -import {CodexApprovalPresentationStore} from "./permissions/presentation-store"; +import {PermissionLifecycleContext} from "./permissions/lifecycle"; import { planImplementationApproved, planImplementationPermissionRequest, @@ -148,7 +148,6 @@ export interface SessionState { sessionTitle: string | null; sessionTitleSource: "unset" | "fallback" | "explicit" | "unknown"; sessionFailure?: SessionFailure; - permissionRequestSequence?: number; } export type SessionFailureCategory = @@ -260,6 +259,7 @@ export class CodexAcpServer { private readonly sessionGenerations: Map; private readonly sessionOpenGenerations: Map; private readonly goalControlGenerations: Map; + private readonly permissionLifecycleContexts: WeakMap; private readonly codexProcessState: CodexProcessState | null; private initializeRequest: acp.InitializeRequest | null = null; private providerUpdate: Promise | null = null; @@ -281,6 +281,7 @@ export class CodexAcpServer { this.sessionGenerations = new Map(); this.sessionOpenGenerations = new Map(); this.goalControlGenerations = new Map(); + this.permissionLifecycleContexts = new WeakMap(); this.connection = connection; this.codexAcpClient = codexAcpClient; this.defaultAuthRequest = defaultAuthRequest ?? null; @@ -1941,6 +1942,14 @@ export class CodexAcpServer { return sessionState; } + private permissionLifecycleContext(sessionState: SessionState): PermissionLifecycleContext { + const existing = this.permissionLifecycleContexts.get(sessionState); + if (existing) return existing; + const context = new PermissionLifecycleContext(sessionState); + this.permissionLifecycleContexts.set(sessionState, context); + return context; + } + private resolveSessionMcpServers( mcpServers: Array, recoverFromStartup: boolean, @@ -2285,16 +2294,18 @@ export class CodexAcpServer { this.sessionFailureEpoch, ); eventHandler = promptEventHandler; - const approvalPresentationStore = new CodexApprovalPresentationStore(); + const permissionLifecycle = this.permissionLifecycleContext(sessionState); + const permissionContext = permissionLifecycle.beginPrompt(); const approvalHandler = new CodexApprovalHandler( this.connection, sessionState, - approvalPresentationStore, + permissionContext, activePrompt.signal, ); const elicitationHandler = new CodexElicitationHandler( this.connection, sessionState, + permissionContext, this.clientCapabilities, activePrompt.signal, ); @@ -2306,7 +2317,7 @@ export class CodexAcpServer { } const completesActiveTurn = event.method === "turn/completed" && event.params.turn.id === sessionState.currentTurnId; - approvalPresentationStore.handleNotification(event); + permissionContext.handleNotification(event); await elicitationHandler.handleNotification(event); await promptEventHandler.handleNotification(event); if (completesActiveTurn) { diff --git a/src/CodexElicitationHandler.ts b/src/CodexElicitationHandler.ts index 84f2c6c6..f9e6c6c9 100644 --- a/src/CodexElicitationHandler.ts +++ b/src/CodexElicitationHandler.ts @@ -4,8 +4,6 @@ import type { ElicitationHandler } from "./CodexAppServerClient"; import type { ServerNotification } from "./app-server"; import type {JsonValue} from "./app-server/serde_json/JsonValue"; import type { - ItemCompletedNotification, - ItemStartedNotification, McpServerElicitationRequestParams, McpServerElicitationRequestResponse, ToolRequestUserInputParams, @@ -25,6 +23,8 @@ import { type PersistValue, type McpElicitationContext, } from "./permissions/mcp"; +import type {PermissionPromptContext} from "./permissions/lifecycle"; +import {isRecord, normalizeJsonObject, normalizeJsonValue, recordOrNull} from "./permissions/json"; type AcpBackedMcpElicitationParams = Extract< McpServerElicitationRequestParams, { mode: "form" } | { mode: "url" } @@ -32,41 +32,6 @@ type AcpBackedMcpElicitationParams = Extract< const USER_INPUT_OTHER_FIELD_SUFFIX = "__other"; -function isRecord(value: unknown): value is Record { - return value !== null && typeof value === "object" && !Array.isArray(value); -} - -function normalizeJsonValue(value: unknown): JsonValue { - if (value === null || value === undefined) { - return null; - } - if (typeof value === "string" || typeof value === "number" || typeof value === "boolean") { - return value; - } - if (typeof value === "bigint") { - return Number(value); - } - if (Array.isArray(value)) { - return value.map(normalizeJsonValue); - } - if (typeof value === "object") { - return Object.fromEntries( - Object.entries(value) - .filter(([, nested]) => nested !== undefined) - .map(([key, nested]) => [key, normalizeJsonValue(nested)]) - ); - } - return String(value); -} - -function normalizeJsonObject(value: Record): Record { - return Object.fromEntries( - Object.entries(value) - .filter(([, nested]) => nested !== undefined) - .map(([key, nested]) => [key, normalizeJsonValue(nested)]) - ); -} - function normalizeElicitationSchema(value: unknown): acp.ElicitationSchema { const normalized = normalizeElicitationSchemaValue(value); if (!isRecord(normalized)) { @@ -115,10 +80,6 @@ function normalizeElicitationSchemaValue(value: unknown): unknown { return result; } -function metaRecord(meta: unknown): Record | null { - return isRecord(meta) ? meta : null; -} - function contentRecord(content: unknown): Record { return isRecord(content) ? content as Record : {}; } @@ -138,7 +99,7 @@ function elicitationResponseMeta( context: McpElicitationContext, persist: unknown = undefined ): JsonValue | null { - const responseMeta = metaRecord(response._meta); + const responseMeta = recordOrNull(response._meta); const meta = responseMeta ? normalizeJsonObject(responseMeta) : {}; if (context.isToolApproval) { delete meta["persist"]; @@ -179,6 +140,7 @@ function userInputResponseValue( export class CodexElicitationHandler implements ElicitationHandler { private readonly connection: AcpClientConnection; private readonly sessionState: SessionState; + private readonly permissionContext: PermissionPromptContext; private readonly clientCapabilities: acp.ClientCapabilities | null; private readonly cancellationSignal: AbortSignal | undefined; // In Rust, the MCP elicitation handler receives ElicitationRequestEvent directly from the MCP @@ -190,12 +152,9 @@ export class CodexElicitationHandler implements ElicitationHandler { // reaches the client. // // Workaround: before requesting approval, Codex emits an item/started notification with an - // mcpToolCall item carrying the call id and server name. We store (threadId, serverName) → callId - // here so the elicitation request can correlate back to the already-rendered tool call item. + // mcpToolCall item carrying the call id and server name. The shared permission lifecycle stores + // (threadId, serverName) → callId so this request can correlate to the rendered tool call item. // - // App-server does not expose the MCP call id on form requests. Correlate only when there is one - // unambiguous pending call for the server; otherwise render a standalone request with its payload. - private readonly pendingMcpApprovals = new Map(); // The app-server handler exposes URL elicitationId, while serverRequest/resolved only exposes // threadId here, so accepted URL elicitations are completed at thread scope. private readonly pendingUrlElicitations = new Map>(); @@ -203,25 +162,20 @@ export class CodexElicitationHandler implements ElicitationHandler { constructor( connection: AcpClientConnection, sessionState: SessionState, + permissionContext: PermissionPromptContext, clientCapabilities: acp.ClientCapabilities | null = null, cancellationSignal?: AbortSignal ) { this.connection = connection; this.sessionState = sessionState; + this.permissionContext = permissionContext; this.clientCapabilities = clientCapabilities; this.cancellationSignal = cancellationSignal; } async handleNotification(notification: ServerNotification): Promise { switch (notification.method) { - case "item/started": - this.handleItemStarted(notification.params); - return; - case "item/completed": - this.handleItemCompleted(notification.params); - return; case "serverRequest/resolved": - this.clearThread(notification.params.threadId); await this.completeUrlElicitations(notification.params.threadId); return; default: @@ -255,6 +209,7 @@ export class CodexElicitationHandler implements ElicitationHandler { this.sessionState.sessionId, params, context, + () => this.permissionContext.nextStandaloneMcpToolCallId(params.serverName), ); const response = await this.connection.request( acp.methods.client.session.requestPermission, @@ -354,21 +309,13 @@ export class CodexElicitationHandler implements ElicitationHandler { private createMcpElicitationContext(params: McpServerElicitationRequestParams): McpElicitationContext { const isToolApproval = isMcpToolCallApproval(params._meta) && this.isMessageOnlyForm(params); const persistOptions = parsePersistOptions(params._meta); - const correlatedCallId = isToolApproval && (params.mode === "form" || params.mode === "openai/form") - ? this.popPendingApproval(params.threadId, params.serverName) + const correlatedCallId = isToolApproval + ? this.permissionContext.popPendingMcpApproval(params.threadId, params.serverName) : undefined; - const permissionRequestSequence = (this.sessionState.permissionRequestSequence ?? 0) + 1; - this.sessionState.permissionRequestSequence = permissionRequestSequence; return { isToolApproval, persistOptions, correlatedCallId, - standaloneToolCallId: [ - "elicitation", - this.sessionState.sessionId, - params.serverName, - permissionRequestSequence, - ].join(":"), }; } @@ -407,7 +354,7 @@ export class CodexElicitationHandler implements ElicitationHandler { sessionId: this.sessionState.sessionId, ...(context.correlatedCallId ? { toolCallId: context.correlatedCallId } : {}), message: params.message, - _meta: metaRecord(params._meta), + _meta: recordOrNull(params._meta), }; switch (params.mode) { @@ -612,50 +559,4 @@ export class CodexElicitationHandler implements ElicitationHandler { } } - private handleItemStarted(event: ItemStartedNotification): void { - if (event.item.type !== "mcpToolCall") { - return; - } - const key = this.key(event.threadId, event.item.server); - const pending = this.pendingMcpApprovals.get(key); - if (pending) pending.push(event.item.id); - else this.pendingMcpApprovals.set(key, [event.item.id]); - } - - private handleItemCompleted(event: ItemCompletedNotification): void { - if (event.item.type !== "mcpToolCall") { - return; - } - const key = this.key(event.threadId, event.item.server); - const pending = this.pendingMcpApprovals.get(key); - if (!pending) return; - const index = pending.indexOf(event.item.id); - if (index >= 0) pending.splice(index, 1); - if (pending.length === 0) this.pendingMcpApprovals.delete(key); - } - - private popPendingApproval(threadId: string, serverName: string): string | undefined { - const key = this.key(threadId, serverName); - const pending = this.pendingMcpApprovals.get(key); - if (pending?.length !== 1) return undefined; - const callId = pending.shift(); - this.pendingMcpApprovals.delete(key); - return callId; - } - - private clearThread(threadId: string): void { - for (const key of this.pendingMcpApprovals.keys()) { - if (this.belongsToThread(key, threadId)) { - this.pendingMcpApprovals.delete(key); - } - } - } - - private key(threadId: string, serverName: string): string { - return `${threadId}:${serverName}`; - } - - private belongsToThread(key: string, threadId: string): boolean { - return key.startsWith(`${threadId}:`); - } } diff --git a/src/__tests__/CodexACPAgent/approval-events.test.ts b/src/__tests__/CodexACPAgent/approval-events.test.ts index bde2e470..caa0783f 100644 --- a/src/__tests__/CodexACPAgent/approval-events.test.ts +++ b/src/__tests__/CodexACPAgent/approval-events.test.ts @@ -476,7 +476,7 @@ describe("Approval Events", () => { it("rejects a stale-turn command before opening ACP permission UI", async () => { const prompt = setupSessionWithPendingPrompt(); - prompt.sessionState.currentTurnId = "newer-turn"; + fixture.getCodexAcpClient().markTurnStale({threadId: sessionId, turnId: "turn-1"}); const response = await fixture.sendServerRequest<{decision: unknown}>( "item/commandExecution/requestApproval", commandParams(["accept", "decline", "cancel"]), diff --git a/src/__tests__/PermissionLifecycleContext.test.ts b/src/__tests__/PermissionLifecycleContext.test.ts new file mode 100644 index 00000000..ece63118 --- /dev/null +++ b/src/__tests__/PermissionLifecycleContext.test.ts @@ -0,0 +1,142 @@ +import {describe, expect, it, vi} from "vitest"; +import type {SessionState} from "../CodexAcpServer"; +import {CodexElicitationHandler} from "../CodexElicitationHandler"; +import type {AcpClientConnection} from "../ACPSessionConnection"; +import type {ServerNotification} from "../app-server"; +import {PermissionLifecycleContext} from "../permissions/lifecycle"; + +function sessionState(): SessionState { + return { + sessionId: "session", + currentTurnId: "turn-1", + } as SessionState; +} + +function mcpStarted(id: string, turnId: string): ServerNotification { + return { + method: "item/started", + params: { + threadId: "thread", + turnId, + startedAtMs: 0, + item: { + type: "mcpToolCall", + id, + server: "server", + tool: "tool", + status: "inProgress", + arguments: {}, + appContext: null, + readOnlyHint: null, + pluginId: null, + result: null, + error: null, + durationMs: null, + }, + }, + }; +} + +describe("PermissionLifecycleContext", () => { + it("clears MCP correlation at the turn boundary", () => { + const lifecycle = new PermissionLifecycleContext(sessionState()); + const prompt = lifecycle.beginPrompt(); + prompt.handleNotification(mcpStarted("stale-call", "turn-1")); + prompt.handleNotification({ + method: "turn/completed", + params: { + threadId: "thread", + turn: { + id: "turn-1", + items: [], + itemsView: "full", + status: "completed", + error: null, + startedAt: 0, + completedAt: 1, + durationMs: 1_000, + }, + }, + }); + prompt.handleNotification(mcpStarted("current-call", "turn-2")); + + expect(prompt.popPendingMcpApproval("thread", "server")).toBe("current-call"); + }); + + it("keeps synthetic IDs session-scoped across prompt contexts", () => { + const lifecycle = new PermissionLifecycleContext(sessionState()); + expect(lifecycle.beginPrompt().nextStandaloneMcpToolCallId("server")) + .toBe("elicitation:session:server:1"); + expect(lifecycle.beginPrompt().nextStandaloneMcpToolCallId("server")) + .toBe("elicitation:session:server:2"); + }); + + it("isolates MCP correlation between prompt generations", () => { + const lifecycle = new PermissionLifecycleContext(sessionState()); + const stalePrompt = lifecycle.beginPrompt(); + const currentPrompt = lifecycle.beginPrompt(); + currentPrompt.handleNotification(mcpStarted("current-call", "turn-2")); + + expect(stalePrompt.popPendingMcpApproval("thread", "server")).toBeUndefined(); + expect(currentPrompt.popPendingMcpApproval("thread", "server")).toBe("current-call"); + }); + + it("does not allocate a synthetic ID for native ACP elicitation", async () => { + const state = sessionState(); + const lifecycle = new PermissionLifecycleContext(state); + const prompt = lifecycle.beginPrompt(); + const connection = { + request: vi.fn().mockResolvedValue({action: "decline"}), + } as unknown as AcpClientConnection; + const handler = new CodexElicitationHandler( + connection, + state, + prompt, + {elicitation: {form: {}}}, + ); + + await handler.handleElicitation({ + threadId: "thread", + turnId: "turn-1", + serverName: "server", + mode: "form", + _meta: null, + message: "Collect a value", + requestedSchema: {type: "object", properties: {value: {type: "string"}}}, + }); + + expect(prompt.nextStandaloneMcpToolCallId("server")).toBe("elicitation:session:server:1"); + }); + + it("does not allocate a synthetic ID for a correlated permission fallback", async () => { + const state = sessionState(); + const prompt = new PermissionLifecycleContext(state).beginPrompt(); + const requests: Array<{toolCall: {toolCallId: string}}> = []; + const connection = { + request: vi.fn().mockImplementation((_method, request) => { + requests.push(request); + return Promise.resolve({outcome: {outcome: "selected", optionId: "cancel"}}); + }), + notify: vi.fn(), + } as unknown as AcpClientConnection; + const handler = new CodexElicitationHandler(connection, state, prompt); + const approval = { + threadId: "thread", + turnId: "turn-1", + serverName: "server", + mode: "form" as const, + _meta: {codex_approval_kind: "mcp_tool_call"}, + message: "Allow?", + requestedSchema: {type: "object" as const, properties: {}}, + }; + + prompt.handleNotification(mcpStarted("correlated-call", "turn-1")); + await handler.handleElicitation(approval); + await handler.handleElicitation(approval); + + expect(requests.map(request => request.toolCall.toolCallId)).toEqual([ + "correlated-call", + "elicitation:session:server:1", + ]); + }); +}); diff --git a/src/permissions/CodexApprovalHandler.ts b/src/permissions/CodexApprovalHandler.ts index 10bb3cf7..3dab58f5 100644 --- a/src/permissions/CodexApprovalHandler.ts +++ b/src/permissions/CodexApprovalHandler.ts @@ -29,21 +29,19 @@ import { requestPermissionMeta, } from "./metadata"; import {additionalPermissionsToolCall, commandToolCall, fileChangeToolCall} from "./presentation"; -import type {CodexApprovalPresentationStore} from "./presentation-store"; +import type {PermissionPromptContext} from "./lifecycle"; export class CodexApprovalHandler implements ApprovalHandler { constructor( private readonly connection: AcpClientConnection, private readonly sessionState: SessionState, - private readonly presentationStore: CodexApprovalPresentationStore, + private readonly permissionContext: PermissionPromptContext, private readonly cancellationSignal?: AbortSignal, ) {} async handleCommandExecution( params: CommandExecutionRequestApprovalParams, ): Promise { - if (this.isStale(params.turnId)) return {decision: "cancel"}; - const authoritativeParams = params as CommandParamsWithAvailableDecisions; const decisions = commandDecisionOptions(authoritativeParams); if (!decisions) { @@ -69,12 +67,11 @@ export class CodexApprovalHandler implements ApprovalHandler { } async handleFileChange(params: FileChangeRequestApprovalParams): Promise { - if (this.isStale(params.turnId)) return {decision: "cancel"}; const decisions = fileChangeDecisionOptions(); try { const response = await this.requestPermission({ sessionId: this.sessionState.sessionId, - toolCall: fileChangeToolCall(params, this.presentationStore), + toolCall: fileChangeToolCall(params, this.permissionContext), options: decisions.map(({option}) => option), _meta: requestPermissionMeta(CODEX_FILE_CHANGE_PERMISSION_TITLE, params.reason), }); @@ -88,7 +85,6 @@ export class CodexApprovalHandler implements ApprovalHandler { async handlePermissionsRequest( params: PermissionsRequestApprovalParams, ): Promise { - if (this.isStale(params.turnId)) return this.rejectPermissionsResponse(); try { const response = await this.requestPermission({ sessionId: this.sessionState.sessionId, @@ -158,8 +154,4 @@ export class CodexApprovalHandler implements ApprovalHandler { ...(permissions.fileSystem ? {fileSystem: permissions.fileSystem} : {}), }; } - - private isStale(turnId: string): boolean { - return this.sessionState.currentTurnId !== null && this.sessionState.currentTurnId !== turnId; - } } diff --git a/src/permissions/command-decision-contract.ts b/src/permissions/command-decision-contract.ts new file mode 100644 index 00000000..3cd3bb24 --- /dev/null +++ b/src/permissions/command-decision-contract.ts @@ -0,0 +1,102 @@ +import type { + AdditionalPermissionProfile, + CommandExecutionApprovalDecision, + CommandExecutionRequestApprovalParams, + NetworkPolicyAmendment, +} from "../app-server/v2"; +import {isRecord} from "./json"; + +export type CommandParamsWithAvailableDecisions = CommandExecutionRequestApprovalParams & { + additionalPermissions?: AdditionalPermissionProfile | null; + availableDecisions?: unknown; +}; + +export function parseAvailableCommandDecisions( + params: CommandParamsWithAvailableDecisions, +): CommandExecutionApprovalDecision[] | undefined { + if (params.availableDecisions === undefined || params.availableDecisions === null) { + return defaultCommandDecisions(params); + } + if (!Array.isArray(params.availableDecisions) || params.availableDecisions.length === 0) return undefined; + const decisions: CommandExecutionApprovalDecision[] = []; + for (const candidate of params.availableDecisions) { + const decision = parseCommandDecision(candidate, params); + if (!decision) return undefined; + decisions.push(decision); + } + return decisions; +} + +function defaultCommandDecisions( + params: CommandParamsWithAvailableDecisions, +): CommandExecutionApprovalDecision[] { + if (params.networkApprovalContext) { + const decisions: CommandExecutionApprovalDecision[] = ["accept", "acceptForSession"]; + const allowAmendment = params.proposedNetworkPolicyAmendments?.find(amendment => amendment.action === "allow"); + if (allowAmendment) { + decisions.push({applyNetworkPolicyAmendment: {network_policy_amendment: allowAmendment}}); + } + decisions.push("cancel"); + return decisions; + } + if (params.additionalPermissions) return ["accept", "cancel"]; + const decisions: CommandExecutionApprovalDecision[] = ["accept"]; + if (params.proposedExecpolicyAmendment) { + decisions.push({ + acceptWithExecpolicyAmendment: {execpolicy_amendment: params.proposedExecpolicyAmendment}, + }); + } + decisions.push("cancel"); + return decisions; +} + +function parseCommandDecision( + candidate: unknown, + params: CommandExecutionRequestApprovalParams, +): CommandExecutionApprovalDecision | undefined { + if (candidate === "accept" || candidate === "acceptForSession" || candidate === "decline" || candidate === "cancel") { + return candidate; + } + if (!isRecord(candidate)) return undefined; + + if ("acceptWithExecpolicyAmendment" in candidate) { + const value = candidate["acceptWithExecpolicyAmendment"]; + if (!isRecord(value)) return undefined; + const amendment = value["execpolicy_amendment"]; + if (!isStringArray(amendment) || amendment.length === 0) return undefined; + if (!sameStrings(amendment, params.proposedExecpolicyAmendment)) return undefined; + return {acceptWithExecpolicyAmendment: {execpolicy_amendment: [...amendment]}}; + } + + if ("applyNetworkPolicyAmendment" in candidate) { + const value = candidate["applyNetworkPolicyAmendment"]; + if (!isRecord(value)) return undefined; + const amendment = parseNetworkAmendment(value["network_policy_amendment"]); + if (!amendment || !params.networkApprovalContext) return undefined; + if (amendment.host !== params.networkApprovalContext.host) return undefined; + if (!(params.proposedNetworkPolicyAmendments ?? []).some(proposed => sameNetworkAmendment(proposed, amendment))) { + return undefined; + } + return {applyNetworkPolicyAmendment: {network_policy_amendment: amendment}}; + } + return undefined; +} + +function parseNetworkAmendment(value: unknown): NetworkPolicyAmendment | undefined { + if (!isRecord(value) || typeof value["host"] !== "string") return undefined; + const action = value["action"]; + if (action !== "allow" && action !== "deny") return undefined; + return {host: value["host"], action}; +} + +function isStringArray(value: unknown): value is string[] { + return Array.isArray(value) && value.every(entry => typeof entry === "string"); +} + +function sameStrings(left: readonly string[], right?: readonly string[] | null): boolean { + return !!right && left.length === right.length && left.every((value, index) => value === right[index]); +} + +function sameNetworkAmendment(left: NetworkPolicyAmendment, right: NetworkPolicyAmendment): boolean { + return left.host === right.host && left.action === right.action; +} diff --git a/src/permissions/json.ts b/src/permissions/json.ts new file mode 100644 index 00000000..67a480e5 --- /dev/null +++ b/src/permissions/json.ts @@ -0,0 +1,32 @@ +import type {JsonValue} from "../app-server/serde_json/JsonValue"; + +export function isRecord(value: unknown): value is Record { + return value !== null && typeof value === "object" && !Array.isArray(value); +} + +export function recordOrNull(value: unknown): Record | null { + return isRecord(value) ? value : null; +} + +export function normalizeJsonValue(value: unknown): JsonValue { + if (value === null || value === undefined) return null; + if (typeof value === "string" || typeof value === "number" || typeof value === "boolean") return value; + if (typeof value === "bigint") return Number(value); + if (Array.isArray(value)) return value.map(normalizeJsonValue); + if (typeof value === "object") { + return Object.fromEntries( + Object.entries(value) + .filter(([, nested]) => nested !== undefined) + .map(([key, nested]) => [key, normalizeJsonValue(nested)]), + ); + } + return String(value); +} + +export function normalizeJsonObject(value: Record): Record { + return Object.fromEntries( + Object.entries(value) + .filter(([, nested]) => nested !== undefined) + .map(([key, nested]) => [key, normalizeJsonValue(nested)]), + ); +} diff --git a/src/permissions/lifecycle.ts b/src/permissions/lifecycle.ts new file mode 100644 index 00000000..18060764 --- /dev/null +++ b/src/permissions/lifecycle.ts @@ -0,0 +1,101 @@ +import type {SessionState} from "../CodexAcpServer"; +import type {ServerNotification} from "../app-server"; +import type {ThreadItem} from "../app-server/v2"; + +type FileChangeItem = ThreadItem & {type: "fileChange"}; + +/** Session-scoped permission state shared across prompt generations. */ +export class PermissionLifecycleContext { + private permissionRequestSequence = 0; + + constructor(private readonly session: Pick) {} + + beginPrompt(): PermissionPromptContext { + return new PermissionPromptContext(serverName => this.nextStandaloneMcpToolCallId(serverName)); + } + + private nextStandaloneMcpToolCallId(serverName: string): string { + this.permissionRequestSequence += 1; + return ["elicitation", this.session.sessionId, serverName, this.permissionRequestSequence].join(":"); + } +} + +/** Prompt-scoped permission presentation and MCP correlation state. */ +export class PermissionPromptContext { + private readonly fileChanges = new Map(); + private readonly pendingMcpApprovals = new Map>(); + + constructor(private readonly nextStandaloneId: (serverName: string) => string) {} + + handleNotification(notification: ServerNotification): void { + switch (notification.method) { + case "item/started": + this.handleItemStarted(notification.params.threadId, notification.params.item); + return; + case "item/completed": + this.handleItemCompleted(notification.params.threadId, notification.params.item); + return; + case "turn/completed": + this.clearTransientState(); + return; + case "serverRequest/resolved": + this.pendingMcpApprovals.delete(notification.params.threadId); + return; + default: + return; + } + } + + fileChange(itemId: string): FileChangeItem | undefined { + return this.fileChanges.get(itemId); + } + + popPendingMcpApproval(threadId: string, serverName: string): string | undefined { + const byServer = this.pendingMcpApprovals.get(threadId); + if (!byServer) return undefined; + const pending = byServer.get(serverName); + if (pending?.length !== 1) return undefined; + const callId = pending[0]; + byServer.delete(serverName); + if (byServer.size === 0) this.pendingMcpApprovals.delete(threadId); + return callId; + } + + nextStandaloneMcpToolCallId(serverName: string): string { + return this.nextStandaloneId(serverName); + } + + private handleItemStarted(threadId: string, item: ThreadItem): void { + if (item.type === "fileChange") { + this.fileChanges.set(item.id, item); + return; + } + if (item.type !== "mcpToolCall") return; + const byServer = this.pendingMcpApprovals.get(threadId) ?? new Map(); + const pending = byServer.get(item.server); + if (pending) pending.push(item.id); + else byServer.set(item.server, [item.id]); + this.pendingMcpApprovals.set(threadId, byServer); + } + + private handleItemCompleted(threadId: string, item: ThreadItem): void { + if (item.type === "fileChange") { + this.fileChanges.delete(item.id); + return; + } + if (item.type !== "mcpToolCall") return; + const byServer = this.pendingMcpApprovals.get(threadId); + if (!byServer) return; + const pending = byServer.get(item.server); + if (!pending) return; + const index = pending.indexOf(item.id); + if (index >= 0) pending.splice(index, 1); + if (pending.length === 0) byServer.delete(item.server); + if (byServer.size === 0) this.pendingMcpApprovals.delete(threadId); + } + + private clearTransientState(): void { + this.fileChanges.clear(); + this.pendingMcpApprovals.clear(); + } +} diff --git a/src/permissions/mcp.ts b/src/permissions/mcp.ts index bba1a18c..13020259 100644 --- a/src/permissions/mcp.ts +++ b/src/permissions/mcp.ts @@ -6,6 +6,7 @@ import type { } from "../app-server/v2"; import {optionPermissionMeta} from "./metadata"; import {McpApprovalOptionId} from "./option-ids"; +import {isRecord} from "./json"; export type PersistValue = "session" | "always"; @@ -13,7 +14,6 @@ export type McpElicitationContext = { isToolApproval: boolean; persistOptions: Set; correlatedCallId: string | undefined; - standaloneToolCallId: string; }; export function parsePersistOptions(meta: unknown): Set { @@ -88,6 +88,7 @@ export function buildMcpPermissionRequest( sessionId: string, params: McpServerElicitationRequestParams, context: McpElicitationContext, + nextStandaloneToolCallId: () => string, ): {request: acp.RequestPermissionRequest; correlatedCallId: string | undefined} { const messageContent: acp.ToolCallContent = { type: "content", @@ -114,7 +115,7 @@ export function buildMcpPermissionRequest( request: { sessionId, toolCall: { - toolCallId: context.standaloneToolCallId, + toolCallId: nextStandaloneToolCallId(), kind: context.isToolApproval ? "execute" : "other", status: "pending", content: [messageContent], @@ -188,7 +189,3 @@ function permissionOption( function cancelledResponse(): McpServerElicitationRequestResponse { return {action: "cancel", content: null, _meta: null as JsonValue | null}; } - -function isRecord(value: unknown): value is Record { - return value !== null && typeof value === "object" && !Array.isArray(value); -} diff --git a/src/permissions/options.ts b/src/permissions/options.ts index fe00c8e5..9f42e6ca 100644 --- a/src/permissions/options.ts +++ b/src/permissions/options.ts @@ -1,19 +1,17 @@ import type * as acp from "@agentclientprotocol/sdk"; import type { - AdditionalPermissionProfile, CommandExecutionApprovalDecision, - CommandExecutionRequestApprovalParams, FileChangeApprovalDecision, - NetworkPolicyAmendment, } from "../app-server/v2"; import {ApprovalOptionId} from "./option-ids"; +import { + type CommandParamsWithAvailableDecisions, + parseAvailableCommandDecisions, +} from "./command-decision-contract"; -export type DecisionOption = {option: acp.PermissionOption; decision: T}; +export type {CommandParamsWithAvailableDecisions} from "./command-decision-contract"; -export type CommandParamsWithAvailableDecisions = CommandExecutionRequestApprovalParams & { - additionalPermissions?: AdditionalPermissionProfile | null; - availableDecisions?: unknown; -}; +export type DecisionOption = {option: acp.PermissionOption; decision: T}; export function commandDecisionOptions( params: CommandParamsWithAvailableDecisions, @@ -137,77 +135,6 @@ export function permissionProfileOptions(): acp.PermissionOption[] { ]; } -function parseAvailableCommandDecisions( - params: CommandParamsWithAvailableDecisions, -): CommandExecutionApprovalDecision[] | undefined { - if (params.availableDecisions === undefined || params.availableDecisions === null) { - return defaultCommandDecisions(params); - } - if (!Array.isArray(params.availableDecisions) || params.availableDecisions.length === 0) return undefined; - const decisions: CommandExecutionApprovalDecision[] = []; - for (const candidate of params.availableDecisions) { - const decision = parseCommandDecision(candidate, params); - if (!decision) return undefined; - decisions.push(decision); - } - return decisions; -} - -function defaultCommandDecisions( - params: CommandParamsWithAvailableDecisions, -): CommandExecutionApprovalDecision[] { - if (params.networkApprovalContext) { - const decisions: CommandExecutionApprovalDecision[] = ["accept", "acceptForSession"]; - const allowAmendment = params.proposedNetworkPolicyAmendments?.find(amendment => amendment.action === "allow"); - if (allowAmendment) { - decisions.push({applyNetworkPolicyAmendment: {network_policy_amendment: allowAmendment}}); - } - decisions.push("cancel"); - return decisions; - } - if (params.additionalPermissions) return ["accept", "cancel"]; - const decisions: CommandExecutionApprovalDecision[] = ["accept"]; - if (params.proposedExecpolicyAmendment) { - decisions.push({ - acceptWithExecpolicyAmendment: {execpolicy_amendment: params.proposedExecpolicyAmendment}, - }); - } - decisions.push("cancel"); - return decisions; -} - -function parseCommandDecision( - candidate: unknown, - params: CommandExecutionRequestApprovalParams, -): CommandExecutionApprovalDecision | undefined { - if (candidate === "accept" || candidate === "acceptForSession" || candidate === "decline" || candidate === "cancel") { - return candidate; - } - if (!isRecord(candidate)) return undefined; - - if ("acceptWithExecpolicyAmendment" in candidate) { - const value = candidate["acceptWithExecpolicyAmendment"]; - if (!isRecord(value)) return undefined; - const amendment = value["execpolicy_amendment"]; - if (!isStringArray(amendment) || amendment.length === 0) return undefined; - if (!sameStrings(amendment, params.proposedExecpolicyAmendment)) return undefined; - return {acceptWithExecpolicyAmendment: {execpolicy_amendment: [...amendment]}}; - } - - if ("applyNetworkPolicyAmendment" in candidate) { - const value = candidate["applyNetworkPolicyAmendment"]; - if (!isRecord(value)) return undefined; - const amendment = parseNetworkAmendment(value["network_policy_amendment"]); - if (!amendment || !params.networkApprovalContext) return undefined; - if (amendment.host !== params.networkApprovalContext.host) return undefined; - if (!(params.proposedNetworkPolicyAmendments ?? []).some(proposed => sameNetworkAmendment(proposed, amendment))) { - return undefined; - } - return {applyNetworkPolicyAmendment: {network_policy_amendment: amendment}}; - } - return undefined; -} - function decisionOption( optionId: string, name: string, @@ -297,26 +224,3 @@ function shlexQuote(value: string): string { function isShlexUnquoted(character: string): boolean { return /^[0-9A-Za-z]$/.test(character) || "+-./:@]_".includes(character); } - -function parseNetworkAmendment(value: unknown): NetworkPolicyAmendment | undefined { - if (!isRecord(value) || typeof value["host"] !== "string") return undefined; - const action = value["action"]; - if (action !== "allow" && action !== "deny") return undefined; - return {host: value["host"], action}; -} - -function isRecord(value: unknown): value is Record { - return value !== null && typeof value === "object" && !Array.isArray(value); -} - -function isStringArray(value: unknown): value is string[] { - return Array.isArray(value) && value.every(entry => typeof entry === "string"); -} - -function sameStrings(left: readonly string[], right?: readonly string[] | null): boolean { - return !!right && left.length === right.length && left.every((value, index) => value === right[index]); -} - -function sameNetworkAmendment(left: NetworkPolicyAmendment, right: NetworkPolicyAmendment): boolean { - return left.host === right.host && left.action === right.action; -} diff --git a/src/permissions/presentation-store.ts b/src/permissions/presentation-store.ts deleted file mode 100644 index 742e88eb..00000000 --- a/src/permissions/presentation-store.ts +++ /dev/null @@ -1,33 +0,0 @@ -import type {ServerNotification} from "../app-server"; -import type {ThreadItem} from "../app-server/v2"; - -type FileChangeItem = ThreadItem & {type: "fileChange"}; - -/** Prompt-lifetime presentation data that app-server approval params do not repeat. */ -export class CodexApprovalPresentationStore { - private readonly fileChanges = new Map(); - - handleNotification(notification: ServerNotification): void { - switch (notification.method) { - case "item/started": - if (notification.params.item.type === "fileChange") { - this.fileChanges.set(notification.params.item.id, notification.params.item); - } - return; - case "item/completed": - if (notification.params.item.type === "fileChange") { - this.fileChanges.delete(notification.params.item.id); - } - return; - case "turn/completed": - this.fileChanges.clear(); - return; - default: - return; - } - } - - fileChange(itemId: string): FileChangeItem | undefined { - return this.fileChanges.get(itemId); - } -} diff --git a/src/permissions/presentation.ts b/src/permissions/presentation.ts index b0ea3b18..67704692 100644 --- a/src/permissions/presentation.ts +++ b/src/permissions/presentation.ts @@ -8,7 +8,7 @@ import type { ThreadItem, } from "../app-server/v2"; import {stripShellPrefix} from "../CommandUtils"; -import type {CodexApprovalPresentationStore} from "./presentation-store"; +import type {PermissionPromptContext} from "./lifecycle"; type FileChangeItem = ThreadItem & {type: "fileChange"}; type CommandPresentationParams = CommandExecutionRequestApprovalParams & { @@ -50,9 +50,9 @@ export function commandToolCall(params: CommandPresentationParams): acp.ToolCall export function fileChangeToolCall( params: FileChangeRequestApprovalParams, - store: CodexApprovalPresentationStore, + permissionContext: PermissionPromptContext, ): acp.ToolCallUpdate { - const item = store.fileChange(params.itemId); + const item = permissionContext.fileChange(params.itemId); return { toolCallId: params.itemId, kind: "edit",