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 6e0c394b..8d70b176 100644 --- a/src/CodexAcpServer.ts +++ b/src/CodexAcpServer.ts @@ -1,7 +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 {CodexApprovalHandler} from "./permissions/CodexApprovalHandler"; +import {PermissionLifecycleContext} from "./permissions/lifecycle"; +import { + planImplementationApproved, + planImplementationPermissionRequest, + planImplementationToolCallId, +} from "./permissions/plan-review"; import {CodexElicitationHandler} from "./CodexElicitationHandler"; import {type CodexAuthRequest, getCodexAuthMethods, isCodexAuthRequest} from "./CodexAuthMethod"; import {clientSupportsUrlElicitation} from "./ElicitationCapabilities"; @@ -113,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, @@ -255,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; @@ -276,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; @@ -1936,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, @@ -2280,10 +2294,18 @@ export class CodexAcpServer { this.sessionFailureEpoch, ); eventHandler = promptEventHandler; - const approvalHandler = new CodexApprovalHandler(this.connection, sessionState, activePrompt.signal); + const permissionLifecycle = this.permissionLifecycleContext(sessionState); + const permissionContext = permissionLifecycle.beginPrompt(); + const approvalHandler = new CodexApprovalHandler( + this.connection, + sessionState, + permissionContext, + activePrompt.signal, + ); const elicitationHandler = new CodexElicitationHandler( this.connection, sessionState, + permissionContext, this.clientCapabilities, activePrompt.signal, ); @@ -2295,6 +2317,7 @@ export class CodexAcpServer { } const completesActiveTurn = event.method === "turn/completed" && event.params.turn.id === sessionState.currentTurnId; + permissionContext.handleNotification(event); await elicitationHandler.handleNotification(event); await promptEventHandler.handleNotification(event); if (completesActiveTurn) { @@ -2646,42 +2669,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/CodexApprovalHandler.ts b/src/CodexApprovalHandler.ts deleted file mode 100644 index 7c2e08a9..00000000 --- a/src/CodexApprovalHandler.ts +++ /dev/null @@ -1,559 +0,0 @@ -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} : {}), - }, - } : {}), - }; -} - -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; - } - - async handleCommandExecution( - params: CommandExecutionRequestApprovalParams - ): Promise { - 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); - } catch (error) { - logger.error("Error requesting command execution permission", error); - return { decision: "cancel" }; - } - } - - async handleFileChange( - params: FileChangeRequestApprovalParams - ): Promise { - 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); - } catch (error) { - logger.error("Error requesting file change permission", error); - return { decision: "cancel" }; - } - } - - async handlePermissionsRequest( - params: PermissionsRequestApprovalParams - ): Promise { - 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); - } 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 convertFileChangeResponse( - params: FileChangeRequestApprovalParams, - response: acp.RequestPermissionResponse - ): FileChangeRequestApprovalResponse { - if (response.outcome.outcome === "cancelled") { - return { decision: "cancel" }; - } - - const optionId = response.outcome.optionId; - const decision = this.buildFileChangeOptions(params) - .find(({ option }) => option.optionId === optionId) - ?.decision; - return { decision: decision ?? "decline" }; - } - - private convertPermissionsResponse( - params: PermissionsRequestApprovalParams, - response: acp.RequestPermissionResponse, - ): PermissionsRequestApprovalResponse { - 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, - }; - 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( - 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 { - return { - type: "grant", - operation: "grant", - description: `Allow ${access} access to ${path}${suffix}`, - lifetime, - targets: [{ - type: "filesystem", - access: [access], - matcher: {type: "exact_path", path}, - }], - }; - } - - 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 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 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, - }, - }]; - } -} diff --git a/src/CodexElicitationHandler.ts b/src/CodexElicitationHandler.ts index b0ca0eab..f9e6c6c9 100644 --- a/src/CodexElicitationHandler.ts +++ b/src/CodexElicitationHandler.ts @@ -4,35 +4,27 @@ 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, 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"; +import type {PermissionPromptContext} from "./permissions/lifecycle"; +import {isRecord, normalizeJsonObject, normalizeJsonValue, recordOrNull} from "./permissions/json"; type AcpBackedMcpElicitationParams = Extract< McpServerElicitationRequestParams, { mode: "form" } | { mode: "url" } @@ -40,69 +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); -} - -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)) { @@ -151,48 +80,6 @@ function normalizeElicitationSchemaValue(value: unknown): unknown { return result; } -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 : {}; } @@ -212,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"]; @@ -250,27 +137,10 @@ 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; + 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 @@ -282,13 +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. // - // 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(); // 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>(); @@ -296,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: @@ -340,23 +201,33 @@ 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, + () => this.permissionContext.nextStandaloneMcpToolCallId(params.serverName), + ); 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 +307,22 @@ 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) + const correlatedCallId = isToolApproval + ? this.permissionContext.popPendingMcpApproval(params.threadId, params.serverName) : undefined; - return { isToolApproval, persistOptions, correlatedCallId }; + return { + isToolApproval, + persistOptions, + correlatedCallId, + }; } private shouldUseAcpElicitation( params: McpServerElicitationRequestParams ): params is AcpBackedMcpElicitationParams { + if (this.isMessageOnlyForm(params)) return false; switch (params.mode) { case "form": return clientSupportsFormElicitation(this.clientCapabilities); @@ -457,6 +333,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 @@ -465,21 +354,15 @@ 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) { 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 +448,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 +455,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 +469,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 +486,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 @@ -749,43 +559,4 @@ export class CodexElicitationHandler implements ElicitationHandler { } } - private handleItemStarted(event: ItemStartedNotification): void { - if (event.item.type !== "mcpToolCall") { - return; - } - this.pendingMcpApprovals.set(this.key(event.threadId, event.item.server), 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); - } - - private popPendingApproval(threadId: string, serverName: string): string | undefined { - const key = this.key(threadId, serverName); - const callId = this.pendingMcpApprovals.get(key); - 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/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 821f23bc..caa0783f 100644 --- a/src/__tests__/CodexACPAgent/approval-events.test.ts +++ b/src/__tests__/CodexACPAgent/approval-events.test.ts @@ -1,17 +1,24 @@ -import { describe, it, expect, vi, beforeEach } from 'vitest'; +import {beforeEach, describe, expect, it, vi} from "vitest"; import type { + AdditionalPermissionProfile, + 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"; +import {ApprovalOptionId} from "../../permissions/option-ids"; -describe('Approval Events', () => { +type CommandParams = CommandExecutionRequestApprovalParams & { + additionalPermissions?: AdditionalPermissionProfile | null; + availableDecisions?: unknown; +}; + +describe("Approval Events", () => { let fixture: CodexMockTestFixture; - const sessionId = 'test-session-id'; + const sessionId = "test-session-id"; beforeEach(() => { fixture = createCodexMockTestFixture(); @@ -20,659 +27,666 @@ 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 - ); + 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: expectedDecision }); + 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 handle cancelled permission dialog', async () => { - const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); - fixture.setPermissionResponse({ - outcome: { outcome: 'cancelled' } + 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.Decline}}); + 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 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 + const response = await fixture.sendServerRequest<{decision: unknown}>( + "item/commandExecution/requestApproval", + params, ); - expect(response).toEqual({ decision: 'cancel' }); + 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"}], + }, + options: [ + {optionId: "allow_once", name: "Yes, proceed", kind: "allow_once"}, + { + optionId: "allow_for_session", + name: "Yes, and don't ask again for this command in this session", + kind: "allow_always", + }, + {optionId: "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, + title: "Run command?", + description: "Needed to verify the changes.", + }}, + }); + 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); + }); - completeTurn(); - await promptPromise; + 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"]), + ); + expect(response).toEqual({decision: "cancel"}); + await finish(prompt); }); - it('should map execpolicy amendment approval to the exact app-server decision', async () => { - const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); + it("suppresses a multiline exec-policy option exactly like the Codex TUI", async () => { + const prompt = setupSessionWithPendingPrompt(); fixture.setPermissionResponse({ - outcome: { outcome: 'selected', optionId: ApprovalOptionId.AcceptWithExecpolicyAmendment } + outcome: {outcome: "selected", optionId: ApprovalOptionId.AcceptWithExecpolicyAmendment}, }); - - 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 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, + }), ); - - expect(response).toEqual({ - decision: { - acceptWithExecpolicyAmendment: { - execpolicy_amendment: proposedExecpolicyAmendment, - }, - }, - }); - - 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: { - 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, - }), - }, - }) + expect(response).toEqual({decision: "cancel"}); + const option = permissionRequest().options.find( + (candidate: {optionId: string}) => candidate.optionId === ApprovalOptionId.AcceptWithExecpolicyAmendment, ); - - completeTurn(); - await promptPromise; + expect(option).toBeUndefined(); + await finish(prompt); }); - it('should map network policy amendment approval to the exact app-server decision', async () => { - const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); - const optionId = `${ApprovalOptionId.ApplyNetworkPolicyAmendment}:0`; + 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 } + outcome: {outcome: "selected", optionId: ApprovalOptionId.AcceptWithExecpolicyAmendment}, }); - - 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, - }, - }, + expect(await fixture.sendServerRequest( + "item/commandExecution/requestApproval", + commandParams(["accept", decision, "cancel"], {proposedExecpolicyAmendment: amendment}), + )).toEqual({decision}); + expect(permissionRequest().options[1]).toEqual({ + optionId: ApprovalOptionId.AcceptWithExecpolicyAmendment, + name: "Yes, and don't ask again for commands that start with `npm run test:unit`", + kind: "allow_always", }); + await finish(prompt); + }); - 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("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('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, - }; + 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); + }); - const response = await fixture.sendServerRequest( - 'item/commandExecution/requestApproval', - params - ); + 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); + }); - expect(response).toEqual({ decision: 'cancel' }); + 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('should convert to ACP permission request format', async () => { - const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); + 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: 'allow_once' } + 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); + }); - const params: CommandExecutionRequestApprovalParams = { - threadId: sessionId, - turnId: 'turn-1', - startedAtMs: 0, - environmentId: null, - itemId: 'item-snapshot', - reason: 'Running npm install', - proposedExecpolicyAmendment: null, + 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); + }); - await fixture.sendServerRequest( - 'item/commandExecution/requestApproval', - params - ); - - await expect(fixture.getAcpConnectionDump([])).toMatchFileSnapshot( - 'data/approval-command-allow-once.json' - ); + 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); + }); - completeTurn(); - await promptPromise; + 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('should include rawInput with command and cwd', async () => { - const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); + 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: '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 + 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 expect(fixture.getAcpConnectionDump([])).toMatchFileSnapshot( - 'data/approval-command-with-rawInput.json' - ); - - completeTurn(); - await promptPromise; + await finish(prompt); }); - 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(); + 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: 'allow_once' } + outcome: {outcome: "selected", optionId: ApprovalOptionId.AcceptWithExecpolicyAmendment}, }); - - 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 + 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); + }); - const dump = fixture.getAcpConnectionDump(['_meta']); - const parsed = JSON.parse(dump); - expect(parsed.args[0].toolCall.rawInput.command).toBe(expected); - - completeTurn(); - await promptPromise; + 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); }); - }); - 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`}}], + ...(protocol === "http" || protocol === "https" + ? {rawInput: {url: `${protocol}://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: "No, and block this host in the future", + kind: "reject_always", }); - - 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([ + ["empty", []], + ["unknown", ["accept", "futureDecision", "decline"]], + ["duplicate option id", ["accept", "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(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' } - }); - - const params: FileChangeRequestApprovalParams = { - threadId: sessionId, - turnId: 'turn-1', - startedAtMs: 0, - itemId: 'file-change-snapshot', - reason: 'Modifying config file', - grantRoot: null, - }; - - await fixture.sendServerRequest( - 'item/fileChange/requestApproval', - params + it("rejects a stale-turn command before opening ACP permission UI", async () => { + const prompt = setupSessionWithPendingPrompt(); + fixture.getCodexAcpClient().markTurnStale({threadId: sessionId, turnId: "turn-1"}); + 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); + }); - await expect(fixture.getAcpConnectionDump([])).toMatchFileSnapshot( - 'data/approval-file-change.json' + 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"}), ); - - completeTurn(); - await promptPromise; + 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); }); }); - describe('Permissions approval', () => { - const requestedPermissions = { - network: { enabled: true }, - fileSystem: { - read: ['/home/user/project'], - write: ['/home/user/project/tmp'], - entries: [], - }, - }; - - 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 } - }); + describe("file change approvals", () => { + function fileParams(overrides: Partial = {}): FileChangeRequestApprovalParams { + return { + threadId: sessionId, + turnId: "turn-1", + itemId: "file-item", + startedAtMs: 0, + reason: "Apply the generated edits.", + grantRoot: "/workspace", + ...overrides, + }; + } - const params: PermissionsRequestApprovalParams = { + 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', - itemId: `permissions-${optionId}`, - environmentId: null, + turnId: "turn-1", 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' } + 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.AllowForSession}}); - const params: PermissionsRequestApprovalParams = { - threadId: sessionId, - turnId: 'turn-1', - itemId: 'permissions-cancelled', - environmentId: null, - startedAtMs: 0, - cwd: '/home/user/project', - reason: 'Need extra access', - permissions: requestedPermissions, - }; - - const response = await fixture.sendServerRequest( - 'item/permissions/requestApproval', - params + const response = await fixture.sendServerRequest<{decision: unknown}>( + "item/fileChange/requestApproval", + fileParams(), ); - expect(response).toEqual({ - permissions: {}, - scope: 'turn', - strictAutoReview: true, + 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.", + }}, }); - - completeTurn(); - await promptPromise; + expect(JSON.stringify(permissionRequest())).not.toContain("grantRoot"); + expect(JSON.stringify(permissionRequest())).not.toContain("writes under"); + 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({ - permissions: {}, - scope: 'turn', - strictAutoReview: true, - }); + it("distinguishes explicit file rejection from ACP cancellation", async () => { + const rejectPrompt = setupSessionWithPendingPrompt(); + fixture.setPermissionResponse({outcome: {outcome: "selected", optionId: ApprovalOptionId.Cancel}}); + expect(await fixture.sendServerRequest("item/fileChange/requestApproval", fileParams())) + .toEqual({decision: "cancel"}); + 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); }); + }); - it('should convert to ACP permission request format', async () => { - const { promptPromise, completeTurn } = setupSessionWithPendingPrompt(); - fixture.setPermissionResponse({ - outcome: { outcome: 'selected', optionId: ApprovalOptionId.AllowPermissionsForSession } - }); + describe("additional permission approvals", () => { + const permissions = { + network: {enabled: false}, + fileSystem: { + 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 params: PermissionsRequestApprovalParams = { + function params(): PermissionsRequestApprovalParams { + return { threadId: sessionId, - turnId: 'turn-1', - itemId: 'permissions-snapshot', - 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, }; + } - await fixture.sendServerRequest( - 'item/permissions/requestApproval', - params - ); - - await expect(fixture.getAcpConnectionDump([])).toMatchFileSnapshot( - 'data/approval-permissions-request.json' - ); + it.each([ + [ApprovalOptionId.AllowPermissionsForTurn, "turn", false], + [ApprovalOptionId.AllowPermissionsForTurnWithStrictAutoReview, "turn", true], + [ApprovalOptionId.AllowPermissionsForSession, "session", false], + [ApprovalOptionId.RejectPermissions, "turn", false], + ] 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.", + }}, + }); + 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); + }); - completeTurn(); - await promptPromise; + 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: 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/__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 new file mode 100644 index 00000000..3dab58f5 --- /dev/null +++ b/src/permissions/CodexApprovalHandler.ts @@ -0,0 +1,157 @@ +import * as acp from "@agentclientprotocol/sdk"; +import type {SessionState} from "../CodexAcpServer"; +import type {ApprovalHandler} from "../CodexAppServerClient"; +import type { + CommandExecutionRequestApprovalParams, + CommandExecutionRequestApprovalResponse, + FileChangeRequestApprovalParams, + FileChangeRequestApprovalResponse, + GrantedPermissionProfile, + PermissionsRequestApprovalParams, + PermissionsRequestApprovalResponse, + RequestPermissionProfile, +} from "../app-server/v2"; +import {logger} from "../Logger"; +import type {AcpClientConnection} from "../ACPSessionConnection"; +import { + commandDecisionOptions, + fileChangeDecisionOptions, + permissionProfileOptions, + type CommandParamsWithAvailableDecisions, + type DecisionOption, +} 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 "./metadata"; +import {additionalPermissionsToolCall, commandToolCall, fileChangeToolCall} from "./presentation"; +import type {PermissionPromptContext} from "./lifecycle"; + +export class CodexApprovalHandler implements ApprovalHandler { + constructor( + private readonly connection: AcpClientConnection, + private readonly sessionState: SessionState, + private readonly permissionContext: PermissionPromptContext, + private readonly cancellationSignal?: AbortSignal, + ) {} + + async handleCommandExecution( + params: CommandExecutionRequestApprovalParams, + ): Promise { + 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 response = await this.requestPermission({ + sessionId: this.sessionState.sessionId, + toolCall: commandToolCall(authoritativeParams), + 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"}; + } + } + + async handleFileChange(params: FileChangeRequestApprovalParams): Promise { + const decisions = fileChangeDecisionOptions(); + try { + const response = await this.requestPermission({ + sessionId: this.sessionState.sessionId, + toolCall: fileChangeToolCall(params, this.permissionContext), + 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"}; + } + } + + async handlePermissionsRequest( + params: PermissionsRequestApprovalParams, + ): Promise { + try { + 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 requestPermission(request: acp.RequestPermissionRequest): Promise { + return this.connection.request( + acp.methods.client.session.requestPermission, + request, + this.cancellationSignal ? {cancellationSignal: this.cancellationSignal} : 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; + } + + private permissionsResponse( + permissions: RequestPermissionProfile, + response: acp.RequestPermissionResponse, + ): PermissionsRequestApprovalResponse { + if (response.outcome.outcome === "cancelled") return this.rejectPermissionsResponse(); + switch (response.outcome.optionId) { + case ApprovalOptionId.AllowPermissionsForTurn: + return this.grantedPermissionsResponse(permissions, "turn", false); + case ApprovalOptionId.AllowPermissionsForTurnWithStrictAutoReview: + return this.grantedPermissionsResponse(permissions, "turn", true); + case ApprovalOptionId.AllowPermissionsForSession: + return this.grantedPermissionsResponse(permissions, "session", false); + case ApprovalOptionId.RejectPermissions: + default: + return this.rejectPermissionsResponse(); + } + } + + private grantedPermissionsResponse( + permissions: RequestPermissionProfile, + scope: "turn" | "session", + strictAutoReview: boolean, + ): PermissionsRequestApprovalResponse { + return {permissions: this.grantedPermissions(permissions), scope, strictAutoReview}; + } + + private rejectPermissionsResponse(): PermissionsRequestApprovalResponse { + return {permissions: {}, scope: "turn", strictAutoReview: false}; + } + + private grantedPermissions(permissions: RequestPermissionProfile): GrantedPermissionProfile { + return { + ...(permissions.network ? {network: permissions.network} : {}), + ...(permissions.fileSystem ? {fileSystem: permissions.fileSystem} : {}), + }; + } +} 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 new file mode 100644 index 00000000..13020259 --- /dev/null +++ b/src/permissions/mcp.ts @@ -0,0 +1,191 @@ +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"; +import {isRecord} from "./json"; + +export type PersistValue = "session" | "always"; + +export type McpElicitationContext = { + isToolApproval: boolean; + persistOptions: Set; + correlatedCallId: string | undefined; +}; + +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, + nextStandaloneToolCallId: () => string, +): {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: nextStandaloneToolCallId(), + 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}; +} diff --git a/src/permissions/metadata.ts b/src/permissions/metadata.ts new file mode 100644 index 00000000..2583ff78 --- /dev/null +++ b/src/permissions/metadata.ts @@ -0,0 +1,44 @@ +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/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..9f42e6ca --- /dev/null +++ b/src/permissions/options.ts @@ -0,0 +1,226 @@ +import type * as acp from "@agentclientprotocol/sdk"; +import type { + CommandExecutionApprovalDecision, + FileChangeApprovalDecision, +} from "../app-server/v2"; +import {ApprovalOptionId} from "./option-ids"; +import { + type CommandParamsWithAvailableDecisions, + parseAvailableCommandDecisions, +} from "./command-decision-contract"; + +export type {CommandParamsWithAvailableDecisions} from "./command-decision-contract"; + +export type DecisionOption = {option: acp.PermissionOption; decision: T}; + +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 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); +} 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/permissions/presentation.ts b/src/permissions/presentation.ts new file mode 100644 index 00000000..67704692 --- /dev/null +++ b/src/permissions/presentation.ts @@ -0,0 +1,158 @@ +import type * as acp from "@agentclientprotocol/sdk"; +import type { + AdditionalPermissionProfile, + CommandAction, + CommandExecutionRequestApprovalParams, + FileChangeRequestApprovalParams, + RequestPermissionProfile, + ThreadItem, +} from "../app-server/v2"; +import {stripShellPrefix} from "../CommandUtils"; +import type {PermissionPromptContext} from "./lifecycle"; + +type FileChangeItem = ThreadItem & {type: "fileChange"}; +type CommandPresentationParams = CommandExecutionRequestApprovalParams & { + additionalPermissions?: AdditionalPermissionProfile | null; +}; + +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", + status: "pending", + title: network + ? `${network.protocol} network access to ${network.host}` + : commandTitle(params.commandActions), + ...(Object.keys(rawInput).length > 0 ? {rawInput} : {}), + ...locationsField(unique([ + ...commandActionPaths(params.commandActions), + ...permissionProfilePaths(params.additionalPermissions), + ])), + ...(network + ? {content: [textContent(`${network.protocol} access to ${network.host}`), ...additionalPermissionContent]} + : additionalPermissionContent.length > 0 + ? {content: additionalPermissionContent} + : {}), + }; +} + +export function fileChangeToolCall( + params: FileChangeRequestApprovalParams, + permissionContext: PermissionPromptContext, +): acp.ToolCallUpdate { + const item = permissionContext.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 | AdditionalPermissionProfile | null): 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 | AdditionalPermissionProfile, +): 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)]; +}