From db4d2599b589d24e30eb491d63d1a834c5807258 Mon Sep 17 00:00:00 2001 From: Owen Lin Date: Tue, 17 Feb 2026 17:55:57 -0800 Subject: [PATCH] feat(core): plumb distinct approval ids for command approvals (#12051) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit zsh fork PR stack: - https://github.com/openai/codex/pull/12051 πŸ‘ˆ - https://github.com/openai/codex/pull/12052 With upcoming support for a fork of zsh that allows us to intercept `execve` and run execpolicy checks for each subcommand as part of a `CommandExecution`, it will be possible for there to be multiple approval requests for a shell command like `/path/to/zsh -lc 'git status && rg \"TODO\" src && make test'`. To support that, this PR introduces a new `approval_id` field across core, protocol, and app-server so that we can associate approvals properly for subcommands. --- ...CommandExecutionRequestApprovalParams.json | 7 + .../schema/json/EventMsg.json | 18 +- .../json/ExecCommandApprovalParams.json | 7 + .../schema/json/ServerNotification.json | 9 +- .../schema/json/ServerRequest.json | 14 ++ .../codex_app_server_protocol.schemas.json | 23 ++- .../json/v1/ForkConversationResponse.json | 9 +- .../json/v1/ResumeConversationResponse.json | 9 +- .../v1/SessionConfiguredNotification.json | 9 +- .../typescript/ExecApprovalRequestEvent.ts | 9 +- .../typescript/ExecCommandApprovalParams.ts | 6 +- .../CommandExecutionRequestApprovalParams.ts | 10 + .../src/protocol/common.rs | 2 + .../app-server-protocol/src/protocol/v1.rs | 2 + .../app-server-protocol/src/protocol/v2.rs | 12 ++ codex-rs/app-server-test-client/src/lib.rs | 4 +- codex-rs/app-server/README.md | 2 +- .../app-server/src/bespoke_event_handling.rs | 185 +++++++++++------- codex-rs/app-server/src/thread_state.rs | 1 + .../suite/codex_message_processor_flow.rs | 1 + codex-rs/core/src/codex.rs | 12 +- codex-rs/core/src/codex_delegate.rs | 15 +- codex-rs/core/src/tools/network_approval.rs | 1 + codex-rs/core/src/tools/runtimes/shell.rs | 1 + .../core/src/tools/runtimes/unified_exec.rs | 1 + codex-rs/core/tests/suite/approvals.rs | 6 +- codex-rs/core/tests/suite/codex_delegate.rs | 2 +- codex-rs/core/tests/suite/otel.rs | 12 +- codex-rs/docs/codex_mcp_interface.md | 2 +- codex-rs/mcp-server/src/codex_tool_runner.rs | 28 +-- codex-rs/mcp-server/src/exec_approval.rs | 2 +- codex-rs/protocol/src/approvals.rs | 17 +- codex-rs/tui/src/chatwidget/tests.rs | 7 + 33 files changed, 331 insertions(+), 114 deletions(-) diff --git a/codex-rs/app-server-protocol/schema/json/CommandExecutionRequestApprovalParams.json b/codex-rs/app-server-protocol/schema/json/CommandExecutionRequestApprovalParams.json index 9257aec83..193870195 100644 --- a/codex-rs/app-server-protocol/schema/json/CommandExecutionRequestApprovalParams.json +++ b/codex-rs/app-server-protocol/schema/json/CommandExecutionRequestApprovalParams.json @@ -113,6 +113,13 @@ } }, "properties": { + "approvalId": { + "description": "Unique identifier for this specific approval callback.\n\nFor regular shell/unified_exec approvals, this is null.\n\nFor zsh-exec-bridge subcommand approvals, multiple callbacks can belong to one parent `itemId`, so `approvalId` is a distinct opaque callback id (a UUID) used to disambiguate routing.", + "type": [ + "string", + "null" + ] + }, "command": { "description": "The command to be executed.", "type": [ diff --git a/codex-rs/app-server-protocol/schema/json/EventMsg.json b/codex-rs/app-server-protocol/schema/json/EventMsg.json index 5ec61285c..f38c9b994 100644 --- a/codex-rs/app-server-protocol/schema/json/EventMsg.json +++ b/codex-rs/app-server-protocol/schema/json/EventMsg.json @@ -1452,8 +1452,15 @@ }, { "properties": { + "approval_id": { + "description": "Identifier for this specific approval callback.\n\nWhen absent, the approval is for the command item itself (`call_id`). This is present for subcommand approvals (via execve intercept).", + "type": [ + "string", + "null" + ] + }, "call_id": { - "description": "Identifier for the associated exec call, if available.", + "description": "Identifier for the associated command execution item.", "type": "string" }, "command": { @@ -6392,8 +6399,15 @@ }, { "properties": { + "approval_id": { + "description": "Identifier for this specific approval callback.\n\nWhen absent, the approval is for the command item itself (`call_id`). This is present for subcommand approvals (via execve intercept).", + "type": [ + "string", + "null" + ] + }, "call_id": { - "description": "Identifier for the associated exec call, if available.", + "description": "Identifier for the associated command execution item.", "type": "string" }, "command": { diff --git a/codex-rs/app-server-protocol/schema/json/ExecCommandApprovalParams.json b/codex-rs/app-server-protocol/schema/json/ExecCommandApprovalParams.json index 977b1626a..3855168e0 100644 --- a/codex-rs/app-server-protocol/schema/json/ExecCommandApprovalParams.json +++ b/codex-rs/app-server-protocol/schema/json/ExecCommandApprovalParams.json @@ -117,6 +117,13 @@ } }, "properties": { + "approvalId": { + "description": "Identifier for this specific approval callback.", + "type": [ + "string", + "null" + ] + }, "callId": { "description": "Use to correlate this with [codex_core::protocol::ExecCommandBeginEvent] and [codex_core::protocol::ExecCommandEndEvent].", "type": "string" diff --git a/codex-rs/app-server-protocol/schema/json/ServerNotification.json b/codex-rs/app-server-protocol/schema/json/ServerNotification.json index 540e03756..b6d87cd44 100644 --- a/codex-rs/app-server-protocol/schema/json/ServerNotification.json +++ b/codex-rs/app-server-protocol/schema/json/ServerNotification.json @@ -2268,8 +2268,15 @@ }, { "properties": { + "approval_id": { + "description": "Identifier for this specific approval callback.\n\nWhen absent, the approval is for the command item itself (`call_id`). This is present for subcommand approvals (via execve intercept).", + "type": [ + "string", + "null" + ] + }, "call_id": { - "description": "Identifier for the associated exec call, if available.", + "description": "Identifier for the associated command execution item.", "type": "string" }, "command": { diff --git a/codex-rs/app-server-protocol/schema/json/ServerRequest.json b/codex-rs/app-server-protocol/schema/json/ServerRequest.json index 3fa487385..6be3bebc5 100644 --- a/codex-rs/app-server-protocol/schema/json/ServerRequest.json +++ b/codex-rs/app-server-protocol/schema/json/ServerRequest.json @@ -179,6 +179,13 @@ }, "CommandExecutionRequestApprovalParams": { "properties": { + "approvalId": { + "description": "Unique identifier for this specific approval callback.\n\nFor regular shell/unified_exec approvals, this is null.\n\nFor zsh-exec-bridge subcommand approvals, multiple callbacks can belong to one parent `itemId`, so `approvalId` is a distinct opaque callback id (a UUID) used to disambiguate routing.", + "type": [ + "string", + "null" + ] + }, "command": { "description": "The command to be executed.", "type": [ @@ -264,6 +271,13 @@ }, "ExecCommandApprovalParams": { "properties": { + "approvalId": { + "description": "Identifier for this specific approval callback.", + "type": [ + "string", + "null" + ] + }, "callId": { "description": "Use to correlate this with [codex_core::protocol::ExecCommandBeginEvent] and [codex_core::protocol::ExecCommandEndEvent].", "type": "string" diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json index 82f5c3b7d..b6002dabd 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json @@ -2051,6 +2051,13 @@ "CommandExecutionRequestApprovalParams": { "$schema": "http://json-schema.org/draft-07/schema#", "properties": { + "approvalId": { + "description": "Unique identifier for this specific approval callback.\n\nFor regular shell/unified_exec approvals, this is null.\n\nFor zsh-exec-bridge subcommand approvals, multiple callbacks can belong to one parent `itemId`, so `approvalId` is a distinct opaque callback id (a UUID) used to disambiguate routing.", + "type": [ + "string", + "null" + ] + }, "command": { "description": "The command to be executed.", "type": [ @@ -3465,8 +3472,15 @@ }, { "properties": { + "approval_id": { + "description": "Identifier for this specific approval callback.\n\nWhen absent, the approval is for the command item itself (`call_id`). This is present for subcommand approvals (via execve intercept).", + "type": [ + "string", + "null" + ] + }, "call_id": { - "description": "Identifier for the associated exec call, if available.", + "description": "Identifier for the associated command execution item.", "type": "string" }, "command": { @@ -4939,6 +4953,13 @@ "ExecCommandApprovalParams": { "$schema": "http://json-schema.org/draft-07/schema#", "properties": { + "approvalId": { + "description": "Identifier for this specific approval callback.", + "type": [ + "string", + "null" + ] + }, "callId": { "description": "Use to correlate this with [codex_core::protocol::ExecCommandBeginEvent] and [codex_core::protocol::ExecCommandEndEvent].", "type": "string" diff --git a/codex-rs/app-server-protocol/schema/json/v1/ForkConversationResponse.json b/codex-rs/app-server-protocol/schema/json/v1/ForkConversationResponse.json index a73c550f6..f460c4a28 100644 --- a/codex-rs/app-server-protocol/schema/json/v1/ForkConversationResponse.json +++ b/codex-rs/app-server-protocol/schema/json/v1/ForkConversationResponse.json @@ -1452,8 +1452,15 @@ }, { "properties": { + "approval_id": { + "description": "Identifier for this specific approval callback.\n\nWhen absent, the approval is for the command item itself (`call_id`). This is present for subcommand approvals (via execve intercept).", + "type": [ + "string", + "null" + ] + }, "call_id": { - "description": "Identifier for the associated exec call, if available.", + "description": "Identifier for the associated command execution item.", "type": "string" }, "command": { diff --git a/codex-rs/app-server-protocol/schema/json/v1/ResumeConversationResponse.json b/codex-rs/app-server-protocol/schema/json/v1/ResumeConversationResponse.json index 6dc02934d..a39f1fd8e 100644 --- a/codex-rs/app-server-protocol/schema/json/v1/ResumeConversationResponse.json +++ b/codex-rs/app-server-protocol/schema/json/v1/ResumeConversationResponse.json @@ -1452,8 +1452,15 @@ }, { "properties": { + "approval_id": { + "description": "Identifier for this specific approval callback.\n\nWhen absent, the approval is for the command item itself (`call_id`). This is present for subcommand approvals (via execve intercept).", + "type": [ + "string", + "null" + ] + }, "call_id": { - "description": "Identifier for the associated exec call, if available.", + "description": "Identifier for the associated command execution item.", "type": "string" }, "command": { diff --git a/codex-rs/app-server-protocol/schema/json/v1/SessionConfiguredNotification.json b/codex-rs/app-server-protocol/schema/json/v1/SessionConfiguredNotification.json index 69967e685..20f4771e5 100644 --- a/codex-rs/app-server-protocol/schema/json/v1/SessionConfiguredNotification.json +++ b/codex-rs/app-server-protocol/schema/json/v1/SessionConfiguredNotification.json @@ -1452,8 +1452,15 @@ }, { "properties": { + "approval_id": { + "description": "Identifier for this specific approval callback.\n\nWhen absent, the approval is for the command item itself (`call_id`). This is present for subcommand approvals (via execve intercept).", + "type": [ + "string", + "null" + ] + }, "call_id": { - "description": "Identifier for the associated exec call, if available.", + "description": "Identifier for the associated command execution item.", "type": "string" }, "command": { diff --git a/codex-rs/app-server-protocol/schema/typescript/ExecApprovalRequestEvent.ts b/codex-rs/app-server-protocol/schema/typescript/ExecApprovalRequestEvent.ts index 542ed879b..d0a7fef5c 100644 --- a/codex-rs/app-server-protocol/schema/typescript/ExecApprovalRequestEvent.ts +++ b/codex-rs/app-server-protocol/schema/typescript/ExecApprovalRequestEvent.ts @@ -7,9 +7,16 @@ import type { ParsedCommand } from "./ParsedCommand"; export type ExecApprovalRequestEvent = { /** - * Identifier for the associated exec call, if available. + * Identifier for the associated command execution item. */ call_id: string, +/** + * Identifier for this specific approval callback. + * + * When absent, the approval is for the command item itself (`call_id`). + * This is present for subcommand approvals (via execve intercept). + */ +approval_id?: string, /** * Turn ID that this command belongs to. * Uses `#[serde(default)]` for backwards compatibility. diff --git a/codex-rs/app-server-protocol/schema/typescript/ExecCommandApprovalParams.ts b/codex-rs/app-server-protocol/schema/typescript/ExecCommandApprovalParams.ts index b427337a8..5637d1305 100644 --- a/codex-rs/app-server-protocol/schema/typescript/ExecCommandApprovalParams.ts +++ b/codex-rs/app-server-protocol/schema/typescript/ExecCommandApprovalParams.ts @@ -9,4 +9,8 @@ export type ExecCommandApprovalParams = { conversationId: ThreadId, * Use to correlate this with [codex_core::protocol::ExecCommandBeginEvent] * and [codex_core::protocol::ExecCommandEndEvent]. */ -callId: string, command: Array, cwd: string, reason: string | null, parsedCmd: Array, }; +callId: string, +/** + * Identifier for this specific approval callback. + */ +approvalId: string | null, command: Array, cwd: string, reason: string | null, parsedCmd: Array, }; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/CommandExecutionRequestApprovalParams.ts b/codex-rs/app-server-protocol/schema/typescript/v2/CommandExecutionRequestApprovalParams.ts index 12b252143..290ce5669 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/CommandExecutionRequestApprovalParams.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/CommandExecutionRequestApprovalParams.ts @@ -5,6 +5,16 @@ import type { CommandAction } from "./CommandAction"; import type { ExecPolicyAmendment } from "./ExecPolicyAmendment"; export type CommandExecutionRequestApprovalParams = { threadId: string, turnId: string, itemId: string, +/** + * Unique identifier for this specific approval callback. + * + * For regular shell/unified_exec approvals, this is null. + * + * For zsh-exec-bridge subcommand approvals, multiple callbacks can belong to + * one parent `itemId`, so `approvalId` is a distinct opaque callback id + * (a UUID) used to disambiguate routing. + */ +approvalId?: string | null, /** * Optional explanatory reason (e.g. request for network access). */ diff --git a/codex-rs/app-server-protocol/src/protocol/common.rs b/codex-rs/app-server-protocol/src/protocol/common.rs index 64e5d2c68..d3f906bca 100644 --- a/codex-rs/app-server-protocol/src/protocol/common.rs +++ b/codex-rs/app-server-protocol/src/protocol/common.rs @@ -1003,6 +1003,7 @@ mod tests { let params = v1::ExecCommandApprovalParams { conversation_id, call_id: "call-42".to_string(), + approval_id: Some("approval-42".to_string()), command: vec!["echo".to_string(), "hello".to_string()], cwd: PathBuf::from("/tmp"), reason: Some("because tests".to_string()), @@ -1022,6 +1023,7 @@ mod tests { "params": { "conversationId": "67e55044-10b1-426f-9247-bb680e5fe0c8", "callId": "call-42", + "approvalId": "approval-42", "command": ["echo", "hello"], "cwd": "/tmp", "reason": "because tests", diff --git a/codex-rs/app-server-protocol/src/protocol/v1.rs b/codex-rs/app-server-protocol/src/protocol/v1.rs index 25e60ca75..13308ffaf 100644 --- a/codex-rs/app-server-protocol/src/protocol/v1.rs +++ b/codex-rs/app-server-protocol/src/protocol/v1.rs @@ -258,6 +258,8 @@ pub struct ExecCommandApprovalParams { /// Use to correlate this with [codex_core::protocol::ExecCommandBeginEvent] /// and [codex_core::protocol::ExecCommandEndEvent]. pub call_id: String, + /// Identifier for this specific approval callback. + pub approval_id: Option, pub command: Vec, pub cwd: PathBuf, pub reason: Option, diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index 96c14ef5f..52eeaf883 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -3181,7 +3181,18 @@ pub struct CommandExecutionRequestApprovalParams { pub thread_id: String, pub turn_id: String, pub item_id: String, + /// Unique identifier for this specific approval callback. + /// + /// For regular shell/unified_exec approvals, this is null. + /// + /// For zsh-exec-bridge subcommand approvals, multiple callbacks can belong to + /// one parent `itemId`, so `approvalId` is a distinct opaque callback id + /// (a UUID) used to disambiguate routing. + #[serde(default, skip_serializing_if = "Option::is_none")] + #[ts(optional = nullable)] + pub approval_id: Option, /// Optional explanatory reason (e.g. request for network access). + #[serde(default, skip_serializing_if = "Option::is_none")] #[ts(optional = nullable)] pub reason: Option, /// The command to be executed. @@ -3197,6 +3208,7 @@ pub struct CommandExecutionRequestApprovalParams { #[ts(optional = nullable)] pub command_actions: Option>, /// Optional proposed execpolicy amendment to allow similar commands without prompting. + #[serde(default, skip_serializing_if = "Option::is_none")] #[ts(optional = nullable)] pub proposed_execpolicy_amendment: Option, } diff --git a/codex-rs/app-server-test-client/src/lib.rs b/codex-rs/app-server-test-client/src/lib.rs index 7b55c30a3..3f2dafbfe 100644 --- a/codex-rs/app-server-test-client/src/lib.rs +++ b/codex-rs/app-server-test-client/src/lib.rs @@ -1302,6 +1302,7 @@ impl CodexClient { thread_id, turn_id, item_id, + approval_id, reason, command, cwd, @@ -1310,7 +1311,8 @@ impl CodexClient { } = params; println!( - "\n< commandExecution approval requested for thread {thread_id}, turn {turn_id}, item {item_id}" + "\n< commandExecution approval requested for thread {thread_id}, turn {turn_id}, item {item_id}, approval {}", + approval_id.as_deref().unwrap_or("") ); if let Some(reason) = reason.as_deref() { println!("< reason: {reason}"); diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index e414f1da6..bb378264e 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -637,7 +637,7 @@ Certain actions (shell commands or modifying files) may require explicit user ap Order of messages: 1. `item/started` β€” shows the pending `commandExecution` item with `command`, `cwd`, and other fields so you can render the proposed action. -2. `item/commandExecution/requestApproval` (request) β€” carries the same `itemId`, `threadId`, `turnId`, optionally `reason`, plus `command`, `cwd`, and `commandActions` for friendly display. +2. `item/commandExecution/requestApproval` (request) β€” carries the same `itemId`, `threadId`, `turnId`, optionally `approvalId` (for subcommand callbacks), `reason`, plus `command`, `cwd`, and `commandActions` for friendly display. 3. Client response β€” `{ "decision": "accept", "acceptSettings": { "forSession": false } }` or `{ "decision": "decline" }`. 4. `item/completed` β€” final `commandExecution` item with `status: "completed" | "failed" | "declined"` and execution output. Render this as the authoritative result. diff --git a/codex-rs/app-server/src/bespoke_event_handling.rs b/codex-rs/app-server/src/bespoke_event_handling.rs index 3acdefc73..33bbb9c34 100644 --- a/codex-rs/app-server/src/bespoke_event_handling.rs +++ b/codex-rs/app-server/src/bespoke_event_handling.rs @@ -214,76 +214,88 @@ pub(crate) async fn apply_bespoke_event_handling( }); } }, - EventMsg::ExecApprovalRequest(ExecApprovalRequestEvent { - call_id, - turn_id, - command, - cwd, - reason, - proposed_execpolicy_amendment, - parsed_cmd, - .. - }) => match api_version { - ApiVersion::V1 => { - let params = ExecCommandApprovalParams { - conversation_id, - call_id: call_id.clone(), - command, - cwd, - reason, - parsed_cmd, - }; - let rx = outgoing - .send_request(ServerRequestPayload::ExecCommandApproval(params)) - .await; - tokio::spawn(async move { - on_exec_approval_response(call_id, event_turn_id, rx, conversation).await; - }); - } - ApiVersion::V2 => { - let item_id = call_id.clone(); - let command_actions = parsed_cmd - .iter() - .cloned() - .map(V2ParsedCommand::from) - .collect::>(); - let command_string = shlex_join(&command); - let proposed_execpolicy_amendment_v2 = - proposed_execpolicy_amendment.map(V2ExecPolicyAmendment::from); - - let params = CommandExecutionRequestApprovalParams { - thread_id: conversation_id.to_string(), - turn_id: turn_id.clone(), - // Until we migrate the core to be aware of a first class CommandExecutionItem - // and emit the corresponding EventMsg, we repurpose the call_id as the item_id. - item_id: item_id.clone(), - reason, - command: Some(command_string.clone()), - cwd: Some(cwd.clone()), - command_actions: Some(command_actions.clone()), - proposed_execpolicy_amendment: proposed_execpolicy_amendment_v2, - }; - let rx = outgoing - .send_request(ServerRequestPayload::CommandExecutionRequestApproval( - params, - )) - .await; - tokio::spawn(async move { - on_command_execution_request_approval_response( - event_turn_id, + EventMsg::ExecApprovalRequest(ev) => { + let approval_id_for_op = ev.effective_approval_id(); + let ExecApprovalRequestEvent { + call_id, + approval_id, + turn_id, + command, + cwd, + reason, + proposed_execpolicy_amendment, + parsed_cmd, + .. + } = ev; + match api_version { + ApiVersion::V1 => { + let params = ExecCommandApprovalParams { conversation_id, - item_id, - command_string, + call_id: call_id.clone(), + approval_id, + command, cwd, - command_actions, - rx, - conversation, - outgoing, - ) - .await; - }); + reason, + parsed_cmd, + }; + let rx = outgoing + .send_request(ServerRequestPayload::ExecCommandApproval(params)) + .await; + tokio::spawn(async move { + on_exec_approval_response( + approval_id_for_op, + event_turn_id, + rx, + conversation, + ) + .await; + }); + } + ApiVersion::V2 => { + let command_actions = parsed_cmd + .iter() + .cloned() + .map(V2ParsedCommand::from) + .collect::>(); + let command_string = shlex_join(&command); + let proposed_execpolicy_amendment_v2 = + proposed_execpolicy_amendment.map(V2ExecPolicyAmendment::from); + + let params = CommandExecutionRequestApprovalParams { + thread_id: conversation_id.to_string(), + turn_id: turn_id.clone(), + item_id: call_id.clone(), + approval_id: approval_id.clone(), + reason, + command: Some(command_string.clone()), + cwd: Some(cwd.clone()), + command_actions: Some(command_actions.clone()), + proposed_execpolicy_amendment: proposed_execpolicy_amendment_v2, + }; + let rx = outgoing + .send_request(ServerRequestPayload::CommandExecutionRequestApproval( + params, + )) + .await; + tokio::spawn(async move { + on_command_execution_request_approval_response( + event_turn_id, + conversation_id, + approval_id, + call_id, + command_string, + cwd, + command_actions, + rx, + conversation, + outgoing, + thread_state.clone(), + ) + .await; + }); + } } - }, + } EventMsg::RequestUserInput(request) => { if matches!(api_version, ApiVersion::V2) { let questions = request @@ -933,6 +945,14 @@ pub(crate) async fn apply_bespoke_event_handling( let cwd = exec_command_begin_event.cwd; let process_id = exec_command_begin_event.process_id; + { + let mut state = thread_state.lock().await; + state + .turn_summary + .command_execution_started + .insert(item_id.clone()); + } + let item = ThreadItem::CommandExecution { id: item_id, command, @@ -1020,6 +1040,14 @@ pub(crate) async fn apply_bespoke_event_handling( .. } = exec_command_end_event; + { + let mut state = thread_state.lock().await; + state + .turn_summary + .command_execution_started + .remove(&call_id); + } + let status: CommandExecutionStatus = (&status).into(); let command_actions = parsed_cmd .into_iter() @@ -1739,6 +1767,7 @@ async fn on_file_change_request_approval_response( async fn on_command_execution_request_approval_response( event_turn_id: String, conversation_id: ThreadId, + approval_id: Option, item_id: String, command: String, cwd: PathBuf, @@ -1746,6 +1775,7 @@ async fn on_command_execution_request_approval_response( receiver: oneshot::Receiver, conversation: Arc, outgoing: ThreadScopedOutgoingMessageSender, + thread_state: Arc>, ) { let response = receiver.await; let (decision, completion_status) = match response { @@ -1794,7 +1824,24 @@ async fn on_command_execution_request_approval_response( } }; - if let Some(status) = completion_status { + let suppress_subcommand_completion_item = { + // For regular shell/unified_exec approvals, approval_id is null. + // For zsh-fork subcommand approvals, approval_id is present and + // item_id points to the parent command item. + if approval_id.is_some() { + let state = thread_state.lock().await; + state + .turn_summary + .command_execution_started + .contains(&item_id) + } else { + false + } + }; + + if let Some(status) = completion_status + && !suppress_subcommand_completion_item + { complete_command_execution_item( conversation_id, event_turn_id.clone(), @@ -1811,7 +1858,7 @@ async fn on_command_execution_request_approval_response( if let Err(err) = conversation .submit(Op::ExecApproval { - id: item_id, + id: approval_id.unwrap_or_else(|| item_id.clone()), turn_id: Some(event_turn_id), decision, }) diff --git a/codex-rs/app-server/src/thread_state.rs b/codex-rs/app-server/src/thread_state.rs index 2ccb7eaae..c7fd963c3 100644 --- a/codex-rs/app-server/src/thread_state.rs +++ b/codex-rs/app-server/src/thread_state.rs @@ -20,6 +20,7 @@ type PendingInterruptQueue = Vec<( #[derive(Default, Clone)] pub(crate) struct TurnSummary { pub(crate) file_change_started: HashSet, + pub(crate) command_execution_started: HashSet, pub(crate) last_error: Option, } diff --git a/codex-rs/app-server/tests/suite/codex_message_processor_flow.rs b/codex-rs/app-server/tests/suite/codex_message_processor_flow.rs index 863a7bfc4..e6f887dba 100644 --- a/codex-rs/app-server/tests/suite/codex_message_processor_flow.rs +++ b/codex-rs/app-server/tests/suite/codex_message_processor_flow.rs @@ -298,6 +298,7 @@ async fn test_send_user_turn_changes_approval_policy_behavior() -> Result<()> { ExecCommandApprovalParams { conversation_id, call_id: "call1".to_string(), + approval_id: None, command: format_with_current_shell("python3 -c 'print(42)'"), cwd: working_directory.clone(), reason: None, diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 210809a81..7744480ab 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -2155,7 +2155,7 @@ impl Session { /// Emit an exec approval request event and await the user's decision. /// - /// The request is keyed by `call_id` so matching responses are delivered + /// The request is keyed by `call_id` + `approval_id` so matching responses are delivered /// to the correct in-flight turn. If the task is aborted, this returns the /// default `ReviewDecision` (`Denied`). #[allow(clippy::too_many_arguments)] @@ -2163,32 +2163,36 @@ impl Session { &self, turn_context: &TurnContext, call_id: String, + approval_id: Option, command: Vec, cwd: PathBuf, reason: Option, network_approval_context: Option, proposed_execpolicy_amendment: Option, ) -> ReviewDecision { + // command-level approvals use `call_id`. + // `approval_id` is only present for subcommand callbacks (execve intercept) + let effective_approval_id = approval_id.clone().unwrap_or_else(|| call_id.clone()); // Add the tx_approve callback to the map before sending the request. let (tx_approve, rx_approve) = oneshot::channel(); - let approval_id = call_id.clone(); let prev_entry = { let mut active = self.active_turn.lock().await; match active.as_mut() { Some(at) => { let mut ts = at.turn_state.lock().await; - ts.insert_pending_approval(approval_id.clone(), tx_approve) + ts.insert_pending_approval(effective_approval_id.clone(), tx_approve) } None => None, } }; if prev_entry.is_some() { - warn!("Overwriting existing pending approval for call_id: {approval_id}"); + warn!("Overwriting existing pending approval for call_id: {effective_approval_id}"); } let parsed_cmd = parse_command(&command); let event = EventMsg::ExecApprovalRequest(ExecApprovalRequestEvent { call_id, + approval_id, turn_id: turn_context.sub_id.clone(), command, cwd, diff --git a/codex-rs/core/src/codex_delegate.rs b/codex-rs/core/src/codex_delegate.rs index 69efeefec..fb0f5ad93 100644 --- a/codex-rs/core/src/codex_delegate.rs +++ b/codex-rs/core/src/codex_delegate.rs @@ -311,8 +311,10 @@ async fn handle_exec_approval( event: ExecApprovalRequestEvent, cancel_token: &CancellationToken, ) { + let approval_id_for_op = event.effective_approval_id(); let ExecApprovalRequestEvent { call_id, + approval_id, command, cwd, reason, @@ -320,23 +322,28 @@ async fn handle_exec_approval( proposed_execpolicy_amendment, .. } = event; - let approval_id = call_id.clone(); // Race approval with cancellation and timeout to avoid hangs. let approval_fut = parent_session.request_command_approval( parent_ctx, call_id, + approval_id, command, cwd, reason, network_approval_context, proposed_execpolicy_amendment, ); - let decision = - await_approval_with_cancel(approval_fut, parent_session, &approval_id, cancel_token).await; + let decision = await_approval_with_cancel( + approval_fut, + parent_session, + &approval_id_for_op, + cancel_token, + ) + .await; let _ = codex .submit(Op::ExecApproval { - id: approval_id, + id: approval_id_for_op, turn_id: Some(turn_id), decision, }) diff --git a/codex-rs/core/src/tools/network_approval.rs b/codex-rs/core/src/tools/network_approval.rs index 51e6e3c98..d5643e4fe 100644 --- a/codex-rs/core/src/tools/network_approval.rs +++ b/codex-rs/core/src/tools/network_approval.rs @@ -247,6 +247,7 @@ impl NetworkApprovalService { .request_command_approval( turn_context.as_ref(), attempt.call_id.clone(), + None, attempt.command.clone(), attempt.cwd.clone(), Some(format!( diff --git a/codex-rs/core/src/tools/runtimes/shell.rs b/codex-rs/core/src/tools/runtimes/shell.rs index 3d7a61ef7..a277e48a5 100644 --- a/codex-rs/core/src/tools/runtimes/shell.rs +++ b/codex-rs/core/src/tools/runtimes/shell.rs @@ -109,6 +109,7 @@ impl Approvable for ShellRuntime { .request_command_approval( turn, call_id, + None, command, cwd, reason, diff --git a/codex-rs/core/src/tools/runtimes/unified_exec.rs b/codex-rs/core/src/tools/runtimes/unified_exec.rs index 89b159c5b..be049d093 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -110,6 +110,7 @@ impl Approvable for UnifiedExecRuntime<'_> { .request_command_approval( turn, call_id, + None, command, cwd, reason, diff --git a/codex-rs/core/tests/suite/approvals.rs b/codex-rs/core/tests/suite/approvals.rs index 09d6755d3..14d9c79e5 100644 --- a/codex-rs/core/tests/suite/approvals.rs +++ b/codex-rs/core/tests/suite/approvals.rs @@ -1603,7 +1603,7 @@ async fn run_scenario(scenario: &ScenarioSpec) -> Result<()> { } test.codex .submit(Op::ExecApproval { - id: approval.call_id, + id: approval.effective_approval_id(), turn_id: None, decision: decision.clone(), }) @@ -1823,7 +1823,7 @@ async fn approving_execpolicy_amendment_persists_policy_and_skips_future_prompts test.codex .submit(Op::ExecApproval { - id: approval.call_id, + id: approval.effective_approval_id(), turn_id: None, decision: ReviewDecision::ApprovedExecpolicyAmendment { proposed_execpolicy_amendment: expected_execpolicy_amendment.clone(), @@ -1988,7 +1988,7 @@ async fn compound_command_with_one_safe_command_still_requires_approval() -> Res let approval = expect_exec_approval(&test, expected_command.as_str()).await; test.codex .submit(Op::ExecApproval { - id: approval.call_id, + id: approval.effective_approval_id(), turn_id: None, decision: ReviewDecision::Denied, }) diff --git a/codex-rs/core/tests/suite/codex_delegate.rs b/codex-rs/core/tests/suite/codex_delegate.rs index 0cbdcef06..841dbbb2c 100644 --- a/codex-rs/core/tests/suite/codex_delegate.rs +++ b/codex-rs/core/tests/suite/codex_delegate.rs @@ -100,7 +100,7 @@ async fn codex_delegate_forwards_exec_approval_and_proceeds_on_approval() { // Approve via parent using the emitted approval call ID. test.codex .submit(Op::ExecApproval { - id: approval.call_id, + id: approval.effective_approval_id(), turn_id: None, decision: ReviewDecision::Approved, }) diff --git a/codex-rs/core/tests/suite/otel.rs b/codex-rs/core/tests/suite/otel.rs index 38f223769..fd3d1ca16 100644 --- a/codex-rs/core/tests/suite/otel.rs +++ b/codex-rs/core/tests/suite/otel.rs @@ -1042,7 +1042,7 @@ async fn handle_container_exec_user_approved_records_tool_decision() { codex .submit(Op::ExecApproval { - id: approval.call_id, + id: approval.effective_approval_id(), turn_id: None, decision: ReviewDecision::Approved, }) @@ -1108,7 +1108,7 @@ async fn handle_container_exec_user_approved_for_session_records_tool_decision() codex .submit(Op::ExecApproval { - id: approval.call_id, + id: approval.effective_approval_id(), turn_id: None, decision: ReviewDecision::ApprovedForSession, }) @@ -1174,7 +1174,7 @@ async fn handle_sandbox_error_user_approves_retry_records_tool_decision() { codex .submit(Op::ExecApproval { - id: approval.call_id, + id: approval.effective_approval_id(), turn_id: None, decision: ReviewDecision::Approved, }) @@ -1240,7 +1240,7 @@ async fn handle_container_exec_user_denies_records_tool_decision() { codex .submit(Op::ExecApproval { - id: approval.call_id, + id: approval.effective_approval_id(), turn_id: None, decision: ReviewDecision::Denied, }) @@ -1306,7 +1306,7 @@ async fn handle_sandbox_error_user_approves_for_session_records_tool_decision() codex .submit(Op::ExecApproval { - id: approval.call_id, + id: approval.effective_approval_id(), turn_id: None, decision: ReviewDecision::ApprovedForSession, }) @@ -1373,7 +1373,7 @@ async fn handle_sandbox_error_user_denies_records_tool_decision() { codex .submit(Op::ExecApproval { - id: approval.call_id, + id: approval.effective_approval_id(), turn_id: None, decision: ReviewDecision::Denied, }) diff --git a/codex-rs/docs/codex_mcp_interface.md b/codex-rs/docs/codex_mcp_interface.md index 73512c03c..503891a49 100644 --- a/codex-rs/docs/codex_mcp_interface.md +++ b/codex-rs/docs/codex_mcp_interface.md @@ -146,7 +146,7 @@ Example: When Codex needs approval to apply changes or run commands, the server issues JSON‑RPC requests to the client: - `applyPatchApproval { conversationId, callId, fileChanges, reason?, grantRoot? }` -- `execCommandApproval { conversationId, callId, command, cwd, reason? }` +- `execCommandApproval { conversationId, callId, approvalId?, command, cwd, reason? }` The client must reply with `{ decision: "allow" | "deny" }` for each request. diff --git a/codex-rs/mcp-server/src/codex_tool_runner.rs b/codex-rs/mcp-server/src/codex_tool_runner.rs index 04fa65801..c20c877c4 100644 --- a/codex-rs/mcp-server/src/codex_tool_runner.rs +++ b/codex-rs/mcp-server/src/codex_tool_runner.rs @@ -213,16 +213,19 @@ async fn run_codex_tool_session_inner( .await; match event.msg { - EventMsg::ExecApprovalRequest(ExecApprovalRequestEvent { - turn_id: _, - command, - cwd, - call_id, - reason: _, - proposed_execpolicy_amendment: _, - parsed_cmd, - network_approval_context: _, - }) => { + EventMsg::ExecApprovalRequest(ev) => { + let approval_id = ev.effective_approval_id(); + let ExecApprovalRequestEvent { + turn_id: _, + command, + cwd, + call_id, + approval_id: _, + reason: _, + proposed_execpolicy_amendment: _, + parsed_cmd, + network_approval_context: _, + } = ev; handle_exec_approval_request( command, cwd, @@ -232,6 +235,7 @@ async fn run_codex_tool_session_inner( request_id_str.clone(), event.id.clone(), call_id, + approval_id, parsed_cmd, thread_id, ) @@ -254,9 +258,6 @@ async fn run_codex_tool_session_inner( EventMsg::Warning(_) => { continue; } - EventMsg::ModelReroute(_) => { - continue; - } EventMsg::ElicitationRequest(_) => { // TODO: forward elicitation requests to the client? continue; @@ -361,6 +362,7 @@ async fn run_codex_tool_session_inner( | EventMsg::RequestUserInput(_) | EventMsg::DynamicToolCallRequest(_) | EventMsg::ContextCompacted(_) + | EventMsg::ModelReroute(_) | EventMsg::ThreadRolledBack(_) | EventMsg::CollabAgentSpawnBegin(_) | EventMsg::CollabAgentSpawnEnd(_) diff --git a/codex-rs/mcp-server/src/exec_approval.rs b/codex-rs/mcp-server/src/exec_approval.rs index f4b0a3fe6..54994f9fd 100644 --- a/codex-rs/mcp-server/src/exec_approval.rs +++ b/codex-rs/mcp-server/src/exec_approval.rs @@ -57,10 +57,10 @@ pub(crate) async fn handle_exec_approval_request( tool_call_id: String, event_id: String, call_id: String, + approval_id: String, codex_parsed_cmd: Vec, thread_id: ThreadId, ) { - let approval_id = call_id.clone(); let escaped_command = shlex::try_join(command.iter().map(String::as_str)).unwrap_or_else(|_| command.join(" ")); let message = format!( diff --git a/codex-rs/protocol/src/approvals.rs b/codex-rs/protocol/src/approvals.rs index 4ee7bd8b3..f275ca66d 100644 --- a/codex-rs/protocol/src/approvals.rs +++ b/codex-rs/protocol/src/approvals.rs @@ -57,8 +57,15 @@ pub struct NetworkApprovalContext { #[derive(Debug, Clone, Deserialize, Serialize, JsonSchema, TS)] pub struct ExecApprovalRequestEvent { - /// Identifier for the associated exec call, if available. + /// Identifier for the associated command execution item. pub call_id: String, + /// Identifier for this specific approval callback. + /// + /// When absent, the approval is for the command item itself (`call_id`). + /// This is present for subcommand approvals (via execve intercept). + #[serde(default, skip_serializing_if = "Option::is_none")] + #[ts(optional)] + pub approval_id: Option, /// Turn ID that this command belongs to. /// Uses `#[serde(default)]` for backwards compatibility. #[serde(default)] @@ -81,6 +88,14 @@ pub struct ExecApprovalRequestEvent { pub parsed_cmd: Vec, } +impl ExecApprovalRequestEvent { + pub fn effective_approval_id(&self) -> String { + self.approval_id + .clone() + .unwrap_or_else(|| self.call_id.clone()) + } +} + #[derive(Debug, Clone, Deserialize, Serialize, JsonSchema, TS)] pub struct ElicitationRequestEvent { pub server_name: String, diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index bfae9581b..79232bddc 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -2472,6 +2472,7 @@ async fn exec_approval_emits_proposed_command_and_decision_history() { // Trigger an exec approval request with a short, single-line command let ev = ExecApprovalRequestEvent { call_id: "call-short".into(), + approval_id: Some("call-short".into()), turn_id: "turn-short".into(), command: vec!["bash".into(), "-lc".into(), "echo hello world".into()], cwd: std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")), @@ -2517,6 +2518,7 @@ async fn exec_approval_decision_truncates_multiline_and_long_commands() { // Multiline command: modal should show full command, history records decision only let ev_multi = ExecApprovalRequestEvent { call_id: "call-multi".into(), + approval_id: Some("call-multi".into()), turn_id: "turn-multi".into(), command: vec!["bash".into(), "-lc".into(), "echo line1\necho line2".into()], cwd: std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")), @@ -2570,6 +2572,7 @@ async fn exec_approval_decision_truncates_multiline_and_long_commands() { let long = format!("echo {}", "a".repeat(200)); let ev_long = ExecApprovalRequestEvent { call_id: "call-long".into(), + approval_id: Some("call-long".into()), turn_id: "turn-long".into(), command: vec!["bash".into(), "-lc".into(), long], cwd: std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")), @@ -5684,6 +5687,7 @@ async fn approval_modal_exec_snapshot() -> anyhow::Result<()> { // Inject an exec approval request to display the approval modal. let ev = ExecApprovalRequestEvent { call_id: "call-approve-cmd".into(), + approval_id: Some("call-approve-cmd".into()), turn_id: "turn-approve-cmd".into(), command: vec!["bash".into(), "-lc".into(), "echo hello world".into()], cwd: std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")), @@ -5743,6 +5747,7 @@ async fn approval_modal_exec_without_reason_snapshot() -> anyhow::Result<()> { let ev = ExecApprovalRequestEvent { call_id: "call-approve-cmd-noreason".into(), + approval_id: Some("call-approve-cmd-noreason".into()), turn_id: "turn-approve-cmd-noreason".into(), command: vec!["bash".into(), "-lc".into(), "echo hello world".into()], cwd: std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")), @@ -5791,6 +5796,7 @@ async fn approval_modal_exec_multiline_prefix_hides_execpolicy_option_snapshot() let command = vec!["bash".into(), "-lc".into(), script]; let ev = ExecApprovalRequestEvent { call_id: "call-approve-cmd-multiline-trunc".into(), + approval_id: Some("call-approve-cmd-multiline-trunc".into()), turn_id: "turn-approve-cmd-multiline-trunc".into(), command: command.clone(), cwd: std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")), @@ -6149,6 +6155,7 @@ async fn status_widget_and_approval_modal_snapshot() { // Now show an approval modal (e.g. exec approval). let ev = ExecApprovalRequestEvent { call_id: "call-approve-exec".into(), + approval_id: Some("call-approve-exec".into()), turn_id: "turn-approve-exec".into(), command: vec!["echo".into(), "hello world".into()], cwd: PathBuf::from("/tmp"),