From 90b2b3c717c42ccc03d66cc8bb71b5dacce1fbd6 Mon Sep 17 00:00:00 2001 From: Chris Bookholt Date: Thu, 2 Jul 2026 20:36:58 -0700 Subject: [PATCH] Add explicit command approval purpose --- .../analytics/src/analytics_client_tests.rs | 1 + codex-rs/analytics/src/events.rs | 2 +- codex-rs/analytics/src/reducer.rs | 160 ++++++++++++------ ...CommandExecutionRequestApprovalParams.json | 19 +++ .../schema/json/ServerRequest.json | 19 +++ .../codex_app_server_protocol.schemas.json | 19 +++ .../v2/CommandExecutionApprovalPurpose.ts | 5 + .../CommandExecutionRequestApprovalParams.ts | 4 + .../schema/typescript/v2/index.ts | 1 + .../src/protocol/common.rs | 1 + .../src/protocol/v2/item.rs | 13 ++ codex-rs/app-server-test-client/src/lib.rs | 1 + codex-rs/app-server/README.md | 2 +- .../app-server/src/bespoke_event_handling.rs | 58 +++---- codex-rs/app-server/src/outgoing_message.rs | 1 + codex-rs/app-server/src/transport_tests.rs | 2 + .../tests/suite/v2/turn_start_zsh_fork.rs | 5 + codex-rs/core/src/codex_delegate.rs | 2 + codex-rs/core/src/codex_delegate_tests.rs | 58 ++++++- codex-rs/core/src/session/mod.rs | 18 +- codex-rs/core/src/session/tests.rs | 82 +++++++++ codex-rs/core/src/tools/network_approval.rs | 2 + codex-rs/core/src/tools/runtimes/shell.rs | 1 + .../tools/runtimes/shell/unix_escalation.rs | 2 + .../core/src/tools/runtimes/unified_exec.rs | 1 + codex-rs/mcp-server/src/codex_tool_runner.rs | 1 + codex-rs/protocol/src/approvals.rs | 110 ++++++++++-- codex-rs/protocol/src/protocol.rs | 1 + codex-rs/tui/src/app.rs | 4 +- codex-rs/tui/src/app/app_server_requests.rs | 2 + .../tui/src/app/pending_interactive_replay.rs | 1 + codex-rs/tui/src/app/tests.rs | 58 +++++-- codex-rs/tui/src/app/thread_events.rs | 1 + codex-rs/tui/src/app/thread_routing.rs | 1 + codex-rs/tui/src/approval_events.rs | 19 ++- codex-rs/tui/src/chatwidget.rs | 1 + codex-rs/tui/src/chatwidget/interrupts.rs | 1 + .../src/chatwidget/tests/approval_requests.rs | 9 + .../tui/src/chatwidget/tests/exec_flow.rs | 45 ++++- .../src/chatwidget/tests/status_and_layout.rs | 1 + .../src/chatwidget/tests/terminal_title.rs | 4 + 41 files changed, 608 insertions(+), 130 deletions(-) create mode 100644 codex-rs/app-server-protocol/schema/typescript/v2/CommandExecutionApprovalPurpose.ts diff --git a/codex-rs/analytics/src/analytics_client_tests.rs b/codex-rs/analytics/src/analytics_client_tests.rs index 0f759fd938..489bfc7217 100644 --- a/codex-rs/analytics/src/analytics_client_tests.rs +++ b/codex-rs/analytics/src/analytics_client_tests.rs @@ -893,6 +893,7 @@ fn sample_command_approval_request(request_id: i64, approval_id: Option<&str>) - item_id: "item-1".to_string(), started_at_ms: 1_000, approval_id: approval_id.map(str::to_string), + approval_purpose: None, environment_id: None, reason: None, network_approval_context: None, diff --git a/codex-rs/analytics/src/events.rs b/codex-rs/analytics/src/events.rs index ad8adf14cb..50db868a0f 100644 --- a/codex-rs/analytics/src/events.rs +++ b/codex-rs/analytics/src/events.rs @@ -564,7 +564,7 @@ pub(crate) enum Reviewer { } #[allow(dead_code)] -#[derive(Clone, Copy, Debug, Serialize)] +#[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize)] #[serde(rename_all = "snake_case")] pub(crate) enum ReviewTrigger { Initial, diff --git a/codex-rs/analytics/src/reducer.rs b/codex-rs/analytics/src/reducer.rs index a076a37c3d..f5c4f49de6 100644 --- a/codex-rs/analytics/src/reducer.rs +++ b/codex-rs/analytics/src/reducer.rs @@ -106,6 +106,7 @@ use codex_app_server_protocol::CollabAgentTool; use codex_app_server_protocol::CollabAgentToolCallStatus; use codex_app_server_protocol::CommandAction; use codex_app_server_protocol::CommandExecutionApprovalDecision; +use codex_app_server_protocol::CommandExecutionApprovalPurpose; use codex_app_server_protocol::CommandExecutionSource; use codex_app_server_protocol::CommandExecutionStatus; use codex_app_server_protocol::DynamicToolCallOutputContentItem; @@ -970,6 +971,7 @@ impl AnalyticsReducer { .unwrap_or(false); let requested_additional_permissions = params.additional_permissions.is_some(); let trigger = command_execution_review_trigger( + params.approval_purpose, params.approval_id.as_deref(), params.reason.as_deref(), requested_additional_permissions, @@ -2318,6 +2320,7 @@ fn guardian_review_subject_metadata( } fn command_execution_review_trigger( + approval_purpose: Option, approval_id: Option<&str>, reason: Option<&str>, requested_additional_permissions: bool, @@ -2327,20 +2330,22 @@ fn command_execution_review_trigger( return ReviewTrigger::NetworkPolicyDenial; } - // Both execve interception and a command's unsandboxed retry use a - // callback-specific approval ID. A retry also carries the sandbox denial - // reason, while an execve interception does not. Initial command prompts - // can carry a justification, but do not have a callback-specific ID. - if approval_id.is_some() { - if reason.is_some() { - ReviewTrigger::SandboxDenial + let purpose = approval_purpose.unwrap_or_else(|| { + if approval_id.is_none() { + CommandExecutionApprovalPurpose::Initial + } else if reason.is_some() { + CommandExecutionApprovalPurpose::SandboxRetry } else { - ReviewTrigger::ExecveIntercept + CommandExecutionApprovalPurpose::Execve } - } else if requested_additional_permissions { - ReviewTrigger::SandboxDenial - } else { - ReviewTrigger::Initial + }); + match purpose { + CommandExecutionApprovalPurpose::SandboxRetry => ReviewTrigger::SandboxDenial, + CommandExecutionApprovalPurpose::Execve => ReviewTrigger::ExecveIntercept, + CommandExecutionApprovalPurpose::Initial if requested_additional_permissions => { + ReviewTrigger::SandboxDenial + } + CommandExecutionApprovalPurpose::Initial => ReviewTrigger::Initial, } } @@ -2869,48 +2874,103 @@ mod tests { } #[test] - fn command_execution_review_trigger_distinguishes_retry_from_execve() { - assert!(matches!( - command_execution_review_trigger( - Some("retry-id"), - Some("sandbox denied"), + fn command_execution_review_trigger_uses_explicit_purpose_and_precedence() { + use CommandExecutionApprovalPurpose as Purpose; + let cases = [ + ( + Some(Purpose::Initial), + Some("id"), + None, false, - false + false, + ReviewTrigger::Initial, ), - ReviewTrigger::SandboxDenial - )); - assert!(matches!( - command_execution_review_trigger(Some("execve-id"), None, false, false), - ReviewTrigger::ExecveIntercept - )); - assert!(matches!( - command_execution_review_trigger(None, Some("initial justification"), false, false), - ReviewTrigger::Initial - )); - } - - #[test] - fn command_execution_review_trigger_uses_semantic_precedence() { - assert!(matches!( - command_execution_review_trigger( - Some("network-id"), - Some("network denied"), + ( + Some(Purpose::Initial), + Some("id"), + Some("reason"), + false, + false, + ReviewTrigger::Initial, + ), + ( + Some(Purpose::Execve), + Some("id"), + None, + false, + false, + ReviewTrigger::ExecveIntercept, + ), + ( + Some(Purpose::Execve), + Some("id"), + Some("reason"), + true, + false, + ReviewTrigger::ExecveIntercept, + ), + ( + Some(Purpose::SandboxRetry), + Some("id"), + None, + false, + false, + ReviewTrigger::SandboxDenial, + ), + ( + Some(Purpose::SandboxRetry), + Some("id"), + Some("reason"), + false, + false, + ReviewTrigger::SandboxDenial, + ), + ( + Some(Purpose::Initial), + None, + None, + true, + false, + ReviewTrigger::SandboxDenial, + ), + ( + Some(Purpose::Execve), + Some("id"), + None, true, true, + ReviewTrigger::NetworkPolicyDenial, ), - ReviewTrigger::NetworkPolicyDenial - )); - assert!(matches!( - command_execution_review_trigger(Some("permissions-id"), None, true, false), - ReviewTrigger::ExecveIntercept - )); - assert!(matches!( - command_execution_review_trigger(None, None, true, false), - ReviewTrigger::SandboxDenial - )); - assert!(matches!( - command_execution_review_trigger(None, None, false, false), - ReviewTrigger::Initial - )); + ( + None, + Some("id"), + Some("reason"), + false, + false, + ReviewTrigger::SandboxDenial, + ), + ( + None, + Some("id"), + None, + false, + false, + ReviewTrigger::ExecveIntercept, + ), + ( + None, + None, + Some("reason"), + false, + false, + ReviewTrigger::Initial, + ), + ]; + for (purpose, id, reason, permissions, network, expected) in cases { + assert_eq!( + command_execution_review_trigger(purpose, id, reason, permissions, network), + expected + ); + } } } diff --git a/codex-rs/app-server-protocol/schema/json/CommandExecutionRequestApprovalParams.json b/codex-rs/app-server-protocol/schema/json/CommandExecutionRequestApprovalParams.json index 84dc4a8cf4..c45011bcdf 100644 --- a/codex-rs/app-server-protocol/schema/json/CommandExecutionRequestApprovalParams.json +++ b/codex-rs/app-server-protocol/schema/json/CommandExecutionRequestApprovalParams.json @@ -273,6 +273,14 @@ } ] }, + "CommandExecutionApprovalPurpose": { + "enum": [ + "initial", + "execve", + "sandboxRetry" + ], + "type": "string" + }, "FileSystemAccessMode": { "enum": [ "read", @@ -527,6 +535,17 @@ "null" ] }, + "approvalPurpose": { + "anyOf": [ + { + "$ref": "#/definitions/CommandExecutionApprovalPurpose" + }, + { + "type": "null" + } + ], + "description": "Semantic purpose of this approval request. Compatibility events may omit it." + }, "command": { "description": "The command to be executed.", "type": [ diff --git a/codex-rs/app-server-protocol/schema/json/ServerRequest.json b/codex-rs/app-server-protocol/schema/json/ServerRequest.json index 5cf3c61711..227ecf94e7 100644 --- a/codex-rs/app-server-protocol/schema/json/ServerRequest.json +++ b/codex-rs/app-server-protocol/schema/json/ServerRequest.json @@ -342,6 +342,14 @@ } ] }, + "CommandExecutionApprovalPurpose": { + "enum": [ + "initial", + "execve", + "sandboxRetry" + ], + "type": "string" + }, "CommandExecutionRequestApprovalParams": { "properties": { "approvalId": { @@ -351,6 +359,17 @@ "null" ] }, + "approvalPurpose": { + "anyOf": [ + { + "$ref": "#/definitions/CommandExecutionApprovalPurpose" + }, + { + "type": "null" + } + ], + "description": "Semantic purpose of this approval request. Compatibility events may omit it." + }, "command": { "description": "The command to be executed.", "type": [ 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 0dfb9a8de5..6fb793d0ed 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 @@ -2371,6 +2371,14 @@ } ] }, + "CommandExecutionApprovalPurpose": { + "enum": [ + "initial", + "execve", + "sandboxRetry" + ], + "type": "string" + }, "CommandExecutionRequestApprovalParams": { "$schema": "http://json-schema.org/draft-07/schema#", "properties": { @@ -2381,6 +2389,17 @@ "null" ] }, + "approvalPurpose": { + "anyOf": [ + { + "$ref": "#/definitions/CommandExecutionApprovalPurpose" + }, + { + "type": "null" + } + ], + "description": "Semantic purpose of this approval request. Compatibility events may omit it." + }, "command": { "description": "The command to be executed.", "type": [ diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/CommandExecutionApprovalPurpose.ts b/codex-rs/app-server-protocol/schema/typescript/v2/CommandExecutionApprovalPurpose.ts new file mode 100644 index 0000000000..1807c0d9f6 --- /dev/null +++ b/codex-rs/app-server-protocol/schema/typescript/v2/CommandExecutionApprovalPurpose.ts @@ -0,0 +1,5 @@ +// GENERATED CODE! DO NOT MODIFY BY HAND! + +// This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. + +export type CommandExecutionApprovalPurpose = "initial" | "execve" | "sandboxRetry"; 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 8a77e5ffe0..3a18e7514c 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/CommandExecutionRequestApprovalParams.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/CommandExecutionRequestApprovalParams.ts @@ -3,6 +3,7 @@ // This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. import type { LegacyAppPathString } from "../LegacyAppPathString"; import type { CommandAction } from "./CommandAction"; +import type { CommandExecutionApprovalPurpose } from "./CommandExecutionApprovalPurpose"; import type { ExecPolicyAmendment } from "./ExecPolicyAmendment"; import type { NetworkApprovalContext } from "./NetworkApprovalContext"; import type { NetworkPolicyAmendment } from "./NetworkPolicyAmendment"; @@ -18,6 +19,9 @@ startedAtMs: number, /** * present. Absence is a legacy/cacheable compatibility path. */ approvalId?: string | null, /** + * Semantic purpose of this approval request. Compatibility events may omit it. + */ +approvalPurpose?: CommandExecutionApprovalPurpose | null, /** * Environment in which the command will run. */ environmentId: string | null, /** diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/index.ts b/codex-rs/app-server-protocol/schema/typescript/v2/index.ts index c405082c4c..cc2953e23a 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/index.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/index.ts @@ -63,6 +63,7 @@ export type { CommandExecTerminateResponse } from "./CommandExecTerminateRespons export type { CommandExecWriteParams } from "./CommandExecWriteParams"; export type { CommandExecWriteResponse } from "./CommandExecWriteResponse"; export type { CommandExecutionApprovalDecision } from "./CommandExecutionApprovalDecision"; +export type { CommandExecutionApprovalPurpose } from "./CommandExecutionApprovalPurpose"; export type { CommandExecutionOutputDeltaNotification } from "./CommandExecutionOutputDeltaNotification"; export type { CommandExecutionRequestApprovalParams } from "./CommandExecutionRequestApprovalParams"; export type { CommandExecutionRequestApprovalResponse } from "./CommandExecutionRequestApprovalResponse"; diff --git a/codex-rs/app-server-protocol/src/protocol/common.rs b/codex-rs/app-server-protocol/src/protocol/common.rs index d8db99d4ff..9e95990841 100644 --- a/codex-rs/app-server-protocol/src/protocol/common.rs +++ b/codex-rs/app-server-protocol/src/protocol/common.rs @@ -3693,6 +3693,7 @@ mod tests { item_id: "call_123".to_string(), started_at_ms: 0, approval_id: None, + approval_purpose: None, environment_id: None, reason: None, network_approval_context: None, diff --git a/codex-rs/app-server-protocol/src/protocol/v2/item.rs b/codex-rs/app-server-protocol/src/protocol/v2/item.rs index 3157aee0b2..fcdb325fd8 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2/item.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2/item.rs @@ -11,6 +11,7 @@ use super::shared::v2_enum_from_core; use crate::protocol::item_builders::command_actions_for_path_uri; use crate::protocol::item_builders::convert_patch_changes; use codex_experimental_api_macros::ExperimentalApi; +use codex_protocol::approvals::ExecApprovalPurpose as CoreExecApprovalPurpose; use codex_protocol::approvals::GuardianAssessmentAction as CoreGuardianAssessmentAction; use codex_protocol::approvals::GuardianAssessmentDecisionSource as CoreGuardianAssessmentDecisionSource; use codex_protocol::approvals::GuardianCommandSource as CoreGuardianCommandSource; @@ -72,6 +73,14 @@ pub enum CommandExecutionApprovalDecision { Cancel, } +v2_enum_from_core! { + pub enum CommandExecutionApprovalPurpose from CoreExecApprovalPurpose { + Initial, + Execve, + SandboxRetry, + } +} + impl From for CommandExecutionApprovalDecision { fn from(value: CoreReviewDecision) -> Self { match value { @@ -1452,6 +1461,10 @@ pub struct CommandExecutionRequestApprovalParams { #[serde(default, skip_serializing_if = "Option::is_none")] #[ts(optional = nullable)] pub approval_id: Option, + /// Semantic purpose of this approval request. Compatibility events may omit it. + #[serde(default, skip_serializing_if = "Option::is_none")] + #[ts(optional = nullable)] + pub approval_purpose: Option, /// Environment in which the command will run. #[serde(default)] pub environment_id: Option, diff --git a/codex-rs/app-server-test-client/src/lib.rs b/codex-rs/app-server-test-client/src/lib.rs index a70945c468..401c450b0f 100644 --- a/codex-rs/app-server-test-client/src/lib.rs +++ b/codex-rs/app-server-test-client/src/lib.rs @@ -2065,6 +2065,7 @@ impl CodexClient { item_id, started_at_ms: _, approval_id, + approval_purpose: _, environment_id, reason, network_approval_context, diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index 9eb7ae1598..84bf2b205e 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -1453,7 +1453,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`, the nullable `environmentId` where the command will run, optionally `approvalId`, and `reason`. `itemId` remains the stable command-item identity. Use the server-initiated JSON-RPC request ID for the response. When tracking approval UI or state outside that envelope, `approvalId`, when present, is the opaque identity of this callback and distinguishes it from other callbacks on the same `itemId`; compatibility flows may omit it. A follow-up callback may reuse an active item without another `item/started`; if the prior attempt already completed, the server can restart the same `itemId` before requesting retry approval. New shell and unified-exec approvals set `environmentId`; older events that do not provide one are exposed as `null`. For normal command approvals, the request also includes `command`, `cwd`, and `commandActions` for friendly display. When `initialize.params.capabilities.experimentalApi = true`, it may also include experimental `additionalPermissions` describing requested per-command sandbox access; any filesystem paths in that payload are absolute on the wire, and network access is represented as `additionalPermissions.network.enabled`. For network-only approvals, those command fields may be omitted and `networkApprovalContext` is provided instead. Optional persistence hints may also be included via `proposedExecpolicyAmendment` and `proposedNetworkPolicyAmendments`. Clients can prefer `availableDecisions` when present to render the exact set of choices the server wants to expose, while still falling back to the older heuristics if it is omitted. +2. `item/commandExecution/requestApproval` (request) — carries the same `itemId`, `threadId`, `turnId`, the nullable `environmentId` where the command will run, optionally `approvalId`, `approvalPurpose`, and `reason`. `itemId` remains the stable command-item identity. Use the server-initiated JSON-RPC request ID for the response. When tracking approval UI or state outside that envelope, `approvalId`, when present, is the opaque identity of this callback and distinguishes it from other callbacks on the same `itemId`; compatibility flows may omit it. `approvalPurpose`, when present, distinguishes an initial prompt, execve/subcommand callback, or sandbox retry without relying on display text; compatibility events may omit it. A follow-up callback may reuse an active item without another `item/started`; if the prior attempt already completed, the server can restart the same `itemId` before requesting retry approval. New shell and unified-exec approvals set `environmentId`; older events that do not provide one are exposed as `null`. For normal command approvals, the request also includes `command`, `cwd`, and `commandActions` for friendly display. When `initialize.params.capabilities.experimentalApi = true`, it may also include experimental `additionalPermissions` describing requested per-command sandbox access; any filesystem paths in that payload are absolute on the wire, and network access is represented as `additionalPermissions.network.enabled`. For network-only approvals, those command fields may be omitted and `networkApprovalContext` is provided instead. Optional persistence hints may also be included via `proposedExecpolicyAmendment` and `proposedNetworkPolicyAmendments`. Clients can prefer `availableDecisions` when present to render the exact set of choices the server wants to expose, while still falling back to the older heuristics if it is omitted. 3. Client response — for example `{ "decision": "accept" }`, `{ "decision": "acceptForSession" }`, `{ "decision": { "acceptWithExecpolicyAmendment": { "execpolicy_amendment": [...] } } }`, `{ "decision": { "applyNetworkPolicyAmendment": { "network_policy_amendment": { "host": "example.com", "action": "allow" } } } }`, `{ "decision": "decline" }`, or `{ "decision": "cancel" }`. 4. `serverRequest/resolved` — `{ threadId, requestId }` confirms the pending request has been resolved or cleared, including lifecycle cleanup on turn start/complete/interrupt. 5. `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 aab6f61ea9..f1386db1d4 100644 --- a/codex-rs/app-server/src/bespoke_event_handling.rs +++ b/codex-rs/app-server/src/bespoke_event_handling.rs @@ -98,6 +98,7 @@ use codex_protocol::plan_tool::UpdatePlanArgs; use codex_protocol::protocol::CodexErrorInfo as CoreCodexErrorInfo; use codex_protocol::protocol::Event; use codex_protocol::protocol::EventMsg; +use codex_protocol::protocol::ExecApprovalPurpose; use codex_protocol::protocol::ExecApprovalRequestEvent; use codex_protocol::protocol::Op; use codex_protocol::protocol::RealtimeEvent; @@ -561,6 +562,8 @@ pub(crate) async fn apply_bespoke_event_handling( .into_iter() .map(CommandExecutionApprovalDecision::from) .collect::>(); + let effective_approval_purpose = ev.effective_approval_purpose(); + let approval_purpose = ev.approval_purpose.map(Into::into); let ExecApprovalRequestEvent { call_id, approval_id, @@ -635,17 +638,13 @@ pub(crate) async fn apply_bespoke_event_handling( }); let additional_permissions = additional_permissions.map(V2AdditionalPermissionProfile::from); - // Today execve callbacks omit `reason`, while sandbox retries carry - // one. If execve starts carrying a reason, replace this narrow - // compatibility discriminator with an explicit callback purpose. - let is_sandbox_retry = approval_id.is_some() && reason.is_some(); - let params = CommandExecutionRequestApprovalParams { thread_id: conversation_id.to_string(), turn_id: turn_id.clone(), item_id: call_id.clone(), started_at_ms, approval_id: approval_id.clone(), + approval_purpose, environment_id, reason, network_approval_context, @@ -670,7 +669,7 @@ pub(crate) async fn apply_bespoke_event_handling( call_id, completion_item, started_now, - is_sandbox_retry, + effective_approval_purpose, pending_request_id, rx, conversation, @@ -2088,7 +2087,7 @@ async fn on_command_execution_request_approval_response( item_id: String, completion_item: Option, started_now: bool, - is_sandbox_retry: bool, + approval_purpose: ExecApprovalPurpose, pending_request_id: RequestId, receiver: oneshot::Receiver, conversation: Arc, @@ -2172,18 +2171,14 @@ async fn on_command_execution_request_approval_response( } }; - // A non-null approval_id can identify either an execve callback for a - // command item that is still active (for example, zsh-exec-bridge) or an - // unsandboxed retry. Suppress duplicate completion only for a non-canceling - // callback of the former kind. A declined retry must complete the item even - // when its initial approval already started it and no begin/end event ran - // before the sandbox denial; cancel must also close the item before aborting. - let suppress_repeated_prompt_completion_item = suppress_repeated_prompt_completion_item( - approval_id.as_deref(), - started_now, - is_sandbox_retry, - cancel_requested, - ); + // An execve callback can target a command item that is still active (for + // example, zsh-exec-bridge), while a sandbox retry represents a new attempt. + // Suppress duplicate completion only for a non-canceling callback of the + // former kind. A declined retry must complete the item even when its initial + // approval already started it and no begin/end event ran before the sandbox + // denial; cancel must also close the item before aborting. + let suppress_repeated_prompt_completion_item = + suppress_repeated_prompt_completion_item(started_now, approval_purpose, cancel_requested); if let Some(status) = completion_status && !suppress_repeated_prompt_completion_item @@ -2215,12 +2210,11 @@ async fn on_command_execution_request_approval_response( } fn suppress_repeated_prompt_completion_item( - approval_id: Option<&str>, started_now: bool, - is_sandbox_retry: bool, + approval_purpose: ExecApprovalPurpose, cancel_requested: bool, ) -> bool { - approval_id.is_some() && !started_now && !is_sandbox_retry && !cancel_requested + !started_now && approval_purpose == ExecApprovalPurpose::Execve && !cancel_requested } fn now_unix_timestamp_ms() -> i64 { @@ -2728,20 +2722,16 @@ mod tests { #[test] fn repeated_approval_response_lifecycle_truth_table() { let cases = [ - (None, false, false, false, false), - (Some("callback"), false, false, false, true), - (Some("callback"), false, false, true, false), - (Some("callback"), false, true, false, false), - (Some("callback"), true, false, false, false), + (ExecApprovalPurpose::Initial, false, false, false), + (ExecApprovalPurpose::Initial, false, true, false), + (ExecApprovalPurpose::SandboxRetry, false, false, false), + (ExecApprovalPurpose::Execve, false, false, true), + (ExecApprovalPurpose::Execve, false, true, false), + (ExecApprovalPurpose::Execve, true, false, false), ]; - for (approval_id, started_now, is_sandbox_retry, cancel_requested, expected) in cases { + for (purpose, started_now, cancel, expected) in cases { assert_eq!( - suppress_repeated_prompt_completion_item( - approval_id, - started_now, - is_sandbox_retry, - cancel_requested, - ), + suppress_repeated_prompt_completion_item(started_now, purpose, cancel), expected ); } diff --git a/codex-rs/app-server/src/outgoing_message.rs b/codex-rs/app-server/src/outgoing_message.rs index 16d5c2ff9d..9a30a6f694 100644 --- a/codex-rs/app-server/src/outgoing_message.rs +++ b/codex-rs/app-server/src/outgoing_message.rs @@ -1013,6 +1013,7 @@ mod tests { item_id: "item-1".to_string(), started_at_ms: 0, approval_id: None, + approval_purpose: None, environment_id: None, reason: None, network_approval_context: None, diff --git a/codex-rs/app-server/src/transport_tests.rs b/codex-rs/app-server/src/transport_tests.rs index 439284a176..780aebd639 100644 --- a/codex-rs/app-server/src/transport_tests.rs +++ b/codex-rs/app-server/src/transport_tests.rs @@ -250,6 +250,7 @@ async fn command_execution_request_approval_strips_additional_permissions_withou item_id: "call_123".to_string(), started_at_ms: 0, approval_id: None, + approval_purpose: None, environment_id: None, reason: Some("Need extra read access".to_string()), network_approval_context: None, @@ -316,6 +317,7 @@ async fn command_execution_request_approval_keeps_additional_permissions_with_ca item_id: "call_123".to_string(), started_at_ms: 0, approval_id: None, + approval_purpose: None, environment_id: None, reason: Some("Need extra read access".to_string()), network_approval_context: None, diff --git a/codex-rs/app-server/tests/suite/v2/turn_start_zsh_fork.rs b/codex-rs/app-server/tests/suite/v2/turn_start_zsh_fork.rs index a70e61be09..1644f9c1d3 100644 --- a/codex-rs/app-server/tests/suite/v2/turn_start_zsh_fork.rs +++ b/codex-rs/app-server/tests/suite/v2/turn_start_zsh_fork.rs @@ -15,6 +15,7 @@ use app_test_support::create_shell_command_sse_response; use app_test_support::to_response; use codex_app_server_protocol::CommandAction; use codex_app_server_protocol::CommandExecutionApprovalDecision; +use codex_app_server_protocol::CommandExecutionApprovalPurpose; use codex_app_server_protocol::CommandExecutionRequestApprovalResponse; use codex_app_server_protocol::CommandExecutionStatus; use codex_app_server_protocol::ItemCompletedNotification; @@ -595,6 +596,10 @@ async fn turn_start_shell_zsh_fork_subcommand_decline_marks_parent_declined_v2() (has_first_file != has_second_file) && (has_rm_action || mentions_rm_binary); if is_target_subcommand { + assert_eq!( + params.approval_purpose, + Some(CommandExecutionApprovalPurpose::Execve) + ); approved_subcommand_ids.push( params .approval_id diff --git a/codex-rs/core/src/codex_delegate.rs b/codex-rs/core/src/codex_delegate.rs index 8fc0ba0cab..e842481ef8 100644 --- a/codex-rs/core/src/codex_delegate.rs +++ b/codex-rs/core/src/codex_delegate.rs @@ -497,6 +497,7 @@ async fn handle_exec_approval( cancel_token: &CancellationToken, ) { let approval_id_for_op = event.effective_approval_id(); + let approval_purpose = event.approval_purpose; let ExecApprovalRequestEvent { call_id, approval_id, @@ -547,6 +548,7 @@ async fn handle_exec_approval( parent_ctx, call_id, approval_id, + approval_purpose, environment_id, command, cwd, diff --git a/codex-rs/core/src/codex_delegate_tests.rs b/codex-rs/core/src/codex_delegate_tests.rs index 565b7be388..dd71ae7174 100644 --- a/codex-rs/core/src/codex_delegate_tests.rs +++ b/codex-rs/core/src/codex_delegate_tests.rs @@ -9,6 +9,7 @@ use codex_protocol::models::ResponseItem; use codex_protocol::protocol::AgentStatus; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EventMsg; +use codex_protocol::protocol::ExecApprovalPurpose; use codex_protocol::protocol::ExecApprovalRequestEvent; use codex_protocol::protocol::GuardianAssessmentAction; use codex_protocol::protocol::GuardianAssessmentStatus; @@ -322,6 +323,7 @@ async fn handle_exec_approval_uses_call_id_for_guardian_review_and_approval_id_f ExecApprovalRequestEvent { call_id: "command-item-1".to_string(), approval_id: Some("callback-approval-1".to_string()), + approval_purpose: Some(ExecApprovalPurpose::SandboxRetry), turn_id: "child-turn-1".to_string(), environment_id: Some("remote".to_string()), started_at_ms: 0, @@ -397,7 +399,7 @@ async fn handle_exec_approval_uses_call_id_for_guardian_review_and_approval_id_f } #[tokio::test] -async fn cancelling_delegated_exec_removes_retry_waiter_and_late_response_is_inert() { +async fn delegated_exec_preserves_raw_purpose_and_cancel_cleans_retry_waiter() { let (parent_session, parent_ctx, rx_events) = crate::session::tests::make_session_and_context_with_rx().await; *parent_session.active_turn.lock().await = Some(crate::state::ActiveTurn::default()); @@ -418,6 +420,55 @@ async fn cancelling_delegated_exec_removes_retry_waiter_and_late_response_is_ine session_loop_termination: completed_session_loop_termination(), }); + let legacy_handle = tokio::spawn({ + let codex = Arc::clone(&codex); + let parent_session = Arc::clone(&parent_session); + let parent_ctx = Arc::clone(&parent_ctx); + async move { + let mut event = delegated_retry_exec_approval_event(); + event.call_id = "legacy-command-item".to_string(); + event.approval_id = None; + event.approval_purpose = None; + handle_exec_approval( + codex.as_ref(), + "legacy-child-turn".to_string(), + &parent_session, + &parent_ctx, + event, + &CancellationToken::new(), + ) + .await; + } + }); + let legacy_request = timeout(Duration::from_secs(1), rx_events.recv()) + .await + .expect("legacy exec approval event timed out") + .expect("legacy exec approval event missing"); + let EventMsg::ExecApprovalRequest(legacy_request) = legacy_request.msg else { + panic!("expected legacy ExecApprovalRequest event"); + }; + assert_eq!(legacy_request.call_id, "legacy-command-item"); + assert_eq!(legacy_request.approval_purpose, None); + parent_session + .notify_exec_approval("legacy-command-item", ReviewDecision::Approved) + .await; + timeout(Duration::from_secs(1), legacy_handle) + .await + .expect("legacy handle_exec_approval hung") + .expect("legacy handle_exec_approval join error"); + let legacy_submission = timeout(Duration::from_secs(1), rx_sub.recv()) + .await + .expect("legacy exec approval response timed out") + .expect("legacy exec approval response missing"); + assert_eq!( + legacy_submission.op, + Op::ExecApproval { + id: "legacy-command-item".to_string(), + turn_id: Some("legacy-child-turn".to_string()), + decision: ReviewDecision::Approved, + } + ); + let cancel_token = CancellationToken::new(); let handle = tokio::spawn({ let codex = Arc::clone(&codex); @@ -446,6 +497,10 @@ async fn cancelling_delegated_exec_removes_retry_waiter_and_late_response_is_ine }; assert_eq!(request.call_id, "command-item-1"); assert_eq!(request.approval_id.as_deref(), Some("retry-id")); + assert_eq!( + request.approval_purpose, + Some(ExecApprovalPurpose::SandboxRetry) + ); cancel_token.cancel(); timeout(Duration::from_secs(1), handle) @@ -493,6 +548,7 @@ fn delegated_retry_exec_approval_event() -> ExecApprovalRequestEvent { ExecApprovalRequestEvent { call_id: "command-item-1".to_string(), approval_id: Some("retry-id".to_string()), + approval_purpose: Some(ExecApprovalPurpose::SandboxRetry), turn_id: "child-turn-1".to_string(), environment_id: Some("remote".to_string()), started_at_ms: 0, diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index 69719710c3..4cc2c1e035 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -354,6 +354,7 @@ use codex_protocol::protocol::DeprecationNoticeEvent; use codex_protocol::protocol::ErrorEvent; use codex_protocol::protocol::Event; use codex_protocol::protocol::EventMsg; +use codex_protocol::protocol::ExecApprovalPurpose; use codex_protocol::protocol::ExecApprovalRequestEvent; use codex_protocol::protocol::InitialHistory; use codex_protocol::protocol::McpServerRefreshConfig; @@ -2131,9 +2132,9 @@ impl Session { /// cleared before a response arrives, treat it as an abort so interrupted /// turns do not continue on a synthetic denial. /// - /// If `available_decisions` is `None`, callback-scoped requests with an - /// `approval_id` offer only approve or abort. Other requests derive their - /// decisions via [ExecApprovalRequestEvent::default_available_decisions]. + /// If `available_decisions` is `None`, callback-scoped requests use the + /// one-shot approve/abort set; other requests derive their decisions via + /// [ExecApprovalRequestEvent::default_available_decisions]. #[allow(clippy::too_many_arguments)] #[expect( clippy::await_holding_invalid_type, @@ -2144,6 +2145,7 @@ impl Session { turn_context: &TurnContext, call_id: String, approval_id: Option, + approval_purpose: Option, environment_id: Option, command: Vec, cwd: AbsolutePathBuf, @@ -2153,8 +2155,8 @@ impl Session { additional_permissions: Option, available_decisions: Option>, ) -> ReviewDecision { - // command-level approvals use `call_id`. - // `approval_id` is only present for subcommand callbacks (execve intercept) + // Command-level approvals use `call_id`; callback-scoped approvals can + // supply a distinct ID for matching the response. let effective_approval_id = approval_id.clone().unwrap_or_else(|| call_id.clone()); let proposed_network_policy_amendments = network_approval_context.as_ref().map(|context| { vec![ @@ -2169,7 +2171,10 @@ impl Session { ] }); let available_decisions = available_decisions.unwrap_or_else(|| { - if approval_id.is_some() { + if ExecApprovalRequestEvent::is_callback_scoped( + approval_id.as_deref(), + approval_purpose, + ) { vec![ReviewDecision::Approved, ReviewDecision::Abort] } else { ExecApprovalRequestEvent::default_available_decisions( @@ -2211,6 +2216,7 @@ impl Session { let event = EventMsg::ExecApprovalRequest(ExecApprovalRequestEvent { call_id, approval_id, + approval_purpose, turn_id: turn_context.sub_id.clone(), environment_id, started_at_ms: now_unix_timestamp_ms(), diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index 9bcc262891..ce12808b36 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -806,6 +806,7 @@ async fn callback_without_explicit_decisions_is_one_shot() { turn_context.as_ref(), "command-item".to_string(), Some("callback".to_string()), + /*approval_purpose*/ None, /*environment_id*/ None, vec!["echo".to_string(), "hi".to_string()], test_path_buf("/tmp").abs(), @@ -846,6 +847,7 @@ async fn callback_without_explicit_decisions_is_one_shot() { turn_context.as_ref(), "command-item".to_string(), Some("explicit-callback".to_string()), + /*approval_purpose*/ None, /*environment_id*/ None, vec!["echo".to_string(), "hi".to_string()], test_path_buf("/tmp").abs(), @@ -878,6 +880,84 @@ async fn callback_without_explicit_decisions_is_one_shot() { ); } +#[tokio::test] +async fn default_command_decisions_respect_explicit_callback_purpose() { + use codex_protocol::protocol::ExecApprovalPurpose as Purpose; + let (session, turn_context, rx_events) = make_session_and_context_with_rx().await; + *session.active_turn.lock().await = Some(ActiveTurn::default()); + + for (index, approval_id, purpose, expected) in [ + ( + 0, + None, + Purpose::Initial, + vec![ + ReviewDecision::Approved, + ReviewDecision::ApprovedForSession, + ReviewDecision::Abort, + ], + ), + ( + 1, + Some("callback-initial"), + Purpose::Initial, + vec![ReviewDecision::Approved, ReviewDecision::Abort], + ), + ( + 2, + None, + Purpose::Execve, + vec![ReviewDecision::Approved, ReviewDecision::Abort], + ), + ( + 3, + None, + Purpose::SandboxRetry, + vec![ReviewDecision::Approved, ReviewDecision::Abort], + ), + ] { + let call_id = format!("purpose-{index}"); + let response_id = approval_id + .map(str::to_string) + .unwrap_or_else(|| call_id.clone()); + let request = tokio::spawn({ + let session = Arc::clone(&session); + let turn_context = Arc::clone(&turn_context); + let call_id = call_id.clone(); + async move { + session + .request_command_approval( + turn_context.as_ref(), + call_id, + approval_id.map(str::to_string), + Some(purpose), + /*environment_id*/ None, + vec!["true".to_string()], + test_path_buf("/tmp").abs(), + /*reason*/ None, + /*network_approval_context*/ None, + /*proposed_execpolicy_amendment*/ None, + /*additional_permissions*/ None, + /*available_decisions*/ None, + ) + .await + } + }); + let event = rx_events.recv().await.expect("exec approval event"); + let EventMsg::ExecApprovalRequest(event) = event.msg else { + panic!("expected exec approval request"); + }; + assert_eq!(event.available_decisions, Some(expected)); + session + .notify_exec_approval(&response_id, ReviewDecision::Approved) + .await; + assert_eq!( + request.await.expect("approval task"), + ReviewDecision::Approved + ); + } +} + #[derive(Clone, Copy, Debug)] enum DuplicateApprovalRequestKind { Exec, @@ -906,6 +986,7 @@ async fn duplicate_same_kind_approval_request_aborts_without_replacing_or_emitti turn_context.as_ref(), approval_id, /*approval_id*/ None, + Some(codex_protocol::protocol::ExecApprovalPurpose::Initial), /*environment_id*/ None, vec!["first".to_string()], test_path_buf("/tmp").abs(), @@ -934,6 +1015,7 @@ async fn duplicate_same_kind_approval_request_aborts_without_replacing_or_emitti turn_context.as_ref(), approval_id.clone(), /*approval_id*/ None, + Some(codex_protocol::protocol::ExecApprovalPurpose::Initial), /*environment_id*/ None, vec!["second".to_string()], test_path_buf("/tmp").abs(), diff --git a/codex-rs/core/src/tools/network_approval.rs b/codex-rs/core/src/tools/network_approval.rs index 2b63164b4d..82b0e4cf36 100644 --- a/codex-rs/core/src/tools/network_approval.rs +++ b/codex-rs/core/src/tools/network_approval.rs @@ -25,6 +25,7 @@ use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::Event; use codex_protocol::protocol::EventMsg; +use codex_protocol::protocol::ExecApprovalPurpose; use codex_protocol::protocol::ReviewDecision; use codex_protocol::protocol::WarningEvent; use indexmap::IndexMap; @@ -608,6 +609,7 @@ impl NetworkApprovalService { turn_context.as_ref(), guardian_approval_id, /*approval_id*/ None, + Some(ExecApprovalPurpose::Initial), Some(environment_id), prompt_command, cwd, diff --git a/codex-rs/core/src/tools/runtimes/shell.rs b/codex-rs/core/src/tools/runtimes/shell.rs index 0d386103db..551e5dc142 100644 --- a/codex-rs/core/src/tools/runtimes/shell.rs +++ b/codex-rs/core/src/tools/runtimes/shell.rs @@ -176,6 +176,7 @@ impl Approvable for ShellRuntime { turn, call_id, /*approval_id*/ None, + /*approval_purpose*/ None, environment_id, command, cwd, diff --git a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs index 10cb460205..d5523ad0ab 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs @@ -41,6 +41,7 @@ use codex_protocol::models::PermissionProfile; use codex_protocol::permissions::FileSystemSandboxPolicy; use codex_protocol::permissions::NetworkSandboxPolicy; use codex_protocol::protocol::AskForApproval; +use codex_protocol::protocol::ExecApprovalPurpose; use codex_protocol::protocol::GuardianCommandSource; use codex_protocol::protocol::NetworkPolicyRuleAction; use codex_protocol::protocol::ReviewDecision; @@ -519,6 +520,7 @@ impl CoreShellActionProvider { &turn, call_id, approval_id, + Some(ExecApprovalPurpose::Execve), environment_id, command, workdir.clone(), diff --git a/codex-rs/core/src/tools/runtimes/unified_exec.rs b/codex-rs/core/src/tools/runtimes/unified_exec.rs index 5df42cc979..e00192cdab 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -217,6 +217,7 @@ impl Approvable for UnifiedExecRuntime<'_> { turn, call_id, /*approval_id*/ None, + /*approval_purpose*/ None, environment_id, command, native_cwd, diff --git a/codex-rs/mcp-server/src/codex_tool_runner.rs b/codex-rs/mcp-server/src/codex_tool_runner.rs index cbc113ff79..0202ccb526 100644 --- a/codex-rs/mcp-server/src/codex_tool_runner.rs +++ b/codex-rs/mcp-server/src/codex_tool_runner.rs @@ -226,6 +226,7 @@ async fn run_codex_tool_session_inner( cwd, call_id, approval_id: _, + approval_purpose: _, reason: _, proposed_execpolicy_amendment: _, proposed_network_policy_amendments: _, diff --git a/codex-rs/protocol/src/approvals.rs b/codex-rs/protocol/src/approvals.rs index fc7f9a5a9d..e5bcf7e8b0 100644 --- a/codex-rs/protocol/src/approvals.rs +++ b/codex-rs/protocol/src/approvals.rs @@ -214,6 +214,14 @@ pub struct GuardianAssessmentEvent { pub action: GuardianAssessmentAction, } +#[derive(Debug, Clone, Copy, Deserialize, Serialize, PartialEq, Eq, JsonSchema, TS)] +#[serde(rename_all = "snake_case")] +pub enum ExecApprovalPurpose { + Initial, + Execve, + SandboxRetry, +} + #[derive(Debug, Clone, Deserialize, Serialize, JsonSchema, TS)] pub struct ExecApprovalRequestEvent { /// Identifier for the associated command execution item. @@ -225,6 +233,13 @@ pub struct ExecApprovalRequestEvent { #[serde(default, skip_serializing_if = "Option::is_none")] #[ts(optional)] pub approval_id: Option, + /// Semantic purpose of this approval request. + /// + /// Compatibility senders may omit this field; consumers should then use + /// [`Self::effective_approval_purpose`] for compatibility. + #[serde(default, skip_serializing_if = "Option::is_none")] + #[ts(optional)] + pub approval_purpose: Option, /// Turn ID that this command belongs to. /// Uses `#[serde(default)]` for backwards compatibility. #[serde(default)] @@ -286,7 +301,11 @@ impl ExecApprovalRequestEvent { // senders, so we fall back to the legacy logic if it's not present. match &self.available_decisions { Some(decisions) => decisions.clone(), - None if self.approval_id.is_some() => { + None if Self::is_callback_scoped( + self.approval_id.as_deref(), + self.approval_purpose, + ) => + { vec![ReviewDecision::Approved, ReviewDecision::Abort] } None => Self::default_available_decisions( @@ -298,6 +317,29 @@ impl ExecApprovalRequestEvent { } } + pub fn is_callback_scoped( + approval_id: Option<&str>, + approval_purpose: Option, + ) -> bool { + approval_id.is_some() + || matches!( + approval_purpose, + Some(ExecApprovalPurpose::Execve | ExecApprovalPurpose::SandboxRetry) + ) + } + + pub fn effective_approval_purpose(&self) -> ExecApprovalPurpose { + self.approval_purpose.unwrap_or_else(|| { + if self.approval_id.is_none() { + ExecApprovalPurpose::Initial + } else if self.reason.is_some() { + ExecApprovalPurpose::SandboxRetry + } else { + ExecApprovalPurpose::Execve + } + }) + } + pub fn default_available_decisions( network_approval_context: Option<&NetworkApprovalContext>, proposed_execpolicy_amendment: Option<&ExecPolicyAmendment>, @@ -476,29 +518,71 @@ mod tests { } #[test] - fn callback_without_explicit_decisions_is_one_shot() { - let value = serde_json::json!({ + fn exec_approval_purpose_is_optional_and_overrides_legacy_inference() { + let mut value = serde_json::json!({ "call_id": "command-item", "approval_id": "callback", "turn_id": "turn", "started_at_ms": 0, "command": ["echo", "hi"], "cwd": test_path_buf("/tmp"), - "reason": null, + "reason": "display text", "parsed_cmd": [], }); - let mut event: ExecApprovalRequestEvent = - serde_json::from_value(value).expect("callback event"); + let legacy: ExecApprovalRequestEvent = + serde_json::from_value(value.clone()).expect("legacy event"); assert_eq!( - event.effective_available_decisions(), - vec![ReviewDecision::Approved, ReviewDecision::Abort] + ( + legacy.effective_approval_purpose(), + legacy.effective_available_decisions(), + ), + ( + ExecApprovalPurpose::SandboxRetry, + vec![ReviewDecision::Approved, ReviewDecision::Abort], + ) ); - event.available_decisions = Some(vec![ReviewDecision::ApprovedForSession]); - assert_eq!( - event.effective_available_decisions(), - vec![ReviewDecision::ApprovedForSession] - ); + for (wire, expected) in [ + ("initial", ExecApprovalPurpose::Initial), + ("execve", ExecApprovalPurpose::Execve), + ("sandbox_retry", ExecApprovalPurpose::SandboxRetry), + ] { + value["approval_purpose"] = serde_json::Value::String(wire.to_string()); + let event: ExecApprovalRequestEvent = + serde_json::from_value(value.clone()).expect("event with explicit purpose"); + assert_eq!(event.effective_approval_purpose(), expected); + } + + for (approval_id, purpose, callback_scoped) in [ + (None, Some("initial"), false), + (Some("callback"), Some("initial"), true), + (None, Some("execve"), true), + (None, Some("sandbox_retry"), true), + ] { + value["approval_id"] = approval_id + .map(|id| serde_json::Value::String(id.to_string())) + .unwrap_or(serde_json::Value::Null); + value["approval_purpose"] = purpose + .map(|purpose| serde_json::Value::String(purpose.to_string())) + .unwrap_or(serde_json::Value::Null); + let mut event: ExecApprovalRequestEvent = + serde_json::from_value(value.clone()).expect("callback scope event"); + let expected = if callback_scoped { + vec![ReviewDecision::Approved, ReviewDecision::Abort] + } else { + vec![ + ReviewDecision::Approved, + ReviewDecision::ApprovedForSession, + ReviewDecision::Abort, + ] + }; + assert_eq!(event.effective_available_decisions(), expected); + event.available_decisions = Some(vec![ReviewDecision::ApprovedForSession]); + assert_eq!( + event.effective_available_decisions(), + vec![ReviewDecision::ApprovedForSession] + ); + } } #[test] diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index 96a793bcd3..7098baec44 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -70,6 +70,7 @@ use ts_rs::TS; pub use crate::approvals::ApplyPatchApprovalRequestEvent; pub use crate::approvals::ElicitationAction; +pub use crate::approvals::ExecApprovalPurpose; pub use crate::approvals::ExecApprovalRequestEvent; pub use crate::approvals::ExecPolicyAmendment; pub use crate::approvals::GuardianAssessmentAction; diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index fb98c57bc5..3de85bee96 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -22,6 +22,7 @@ use crate::app_server_session::AppServerSession; use crate::app_server_session::AppServerStartedThread; use crate::app_server_session::TurnPermissionsOverride; use crate::app_server_session::app_server_rate_limit_snapshots; +use crate::approval_events::is_callback_scoped; use crate::bottom_pane::AppLinkViewParams; use crate::bottom_pane::ApprovalRequest; use crate::bottom_pane::FeedbackAudience; @@ -304,6 +305,7 @@ fn collab_receiver_is_not_found( fn default_exec_approval_decisions( approval_id: Option<&str>, + approval_purpose: Option, network_approval_context: Option<&codex_app_server_protocol::NetworkApprovalContext>, proposed_execpolicy_amendment: Option<&codex_app_server_protocol::ExecPolicyAmendment>, proposed_network_policy_amendments: Option< @@ -314,7 +316,7 @@ fn default_exec_approval_decisions( use codex_app_server_protocol::CommandExecutionApprovalDecision; use codex_app_server_protocol::NetworkPolicyRuleAction; - if approval_id.is_some() { + if is_callback_scoped(approval_id, approval_purpose) { return vec![ CommandExecutionApprovalDecision::Accept, CommandExecutionApprovalDecision::Cancel, diff --git a/codex-rs/tui/src/app/app_server_requests.rs b/codex-rs/tui/src/app/app_server_requests.rs index 0b72897685..356b983836 100644 --- a/codex-rs/tui/src/app/app_server_requests.rs +++ b/codex-rs/tui/src/app/app_server_requests.rs @@ -459,6 +459,7 @@ mod tests { item_id: "call-1".to_string(), started_at_ms: 0, approval_id: Some("approval-1".to_string()), + approval_purpose: None, environment_id: None, reason: None, network_approval_context: None, @@ -799,6 +800,7 @@ mod tests { item_id: "call-1".to_string(), started_at_ms: 0, approval_id: Some("approval-1".to_string()), + approval_purpose: None, environment_id: None, reason: None, network_approval_context: None, diff --git a/codex-rs/tui/src/app/pending_interactive_replay.rs b/codex-rs/tui/src/app/pending_interactive_replay.rs index bf1c004165..aed9a0a5c6 100644 --- a/codex-rs/tui/src/app/pending_interactive_replay.rs +++ b/codex-rs/tui/src/app/pending_interactive_replay.rs @@ -615,6 +615,7 @@ mod tests { item_id: call_id.to_string(), started_at_ms: 0, approval_id: approval_id.map(str::to_string), + approval_purpose: None, environment_id: None, reason: None, network_approval_context: None, diff --git a/codex-rs/tui/src/app/tests.rs b/codex-rs/tui/src/app/tests.rs index 35d611ca6d..a476a3ebb8 100644 --- a/codex-rs/tui/src/app/tests.rs +++ b/codex-rs/tui/src/app/tests.rs @@ -2525,34 +2525,56 @@ async fn inactive_thread_exec_approval_preserves_context() { } #[tokio::test] -async fn callback_exec_approval_falls_back_to_one_shot_decisions() -> std::io::Result<()> { +async fn exec_approval_fallback_is_callback_scope_aware() -> std::io::Result<()> { use codex_app_server_protocol::CommandExecutionApprovalDecision as Decision; + use codex_app_server_protocol::CommandExecutionApprovalPurpose as Purpose; let app = make_test_app().await; let thread_id = ThreadId::new(); - let request = exec_approval_request( + for (approval_id, purpose, callback_scoped) in [ + (None, None, false), + (None, Some(Purpose::Initial), false), + (Some("callback"), None, true), + (Some("callback"), Some(Purpose::Initial), true), + (None, Some(Purpose::Execve), true), + (None, Some(Purpose::SandboxRetry), true), + ] { + let mut request = + exec_approval_request(thread_id, "turn-approval", "call-approval", approval_id); + let ServerRequest::CommandExecutionRequestApproval { params, .. } = &mut request else { + unreachable!(); + }; + params.approval_purpose = purpose; + let Some(ThreadInteractiveRequest::Approval(ApprovalRequest::Exec { + available_decisions, + .. + })) = app + .interactive_request_for_thread_request(thread_id, &request) + .await? + else { + panic!("expected exec approval request"); + }; + let expected = if callback_scoped { + vec![Decision::Accept, Decision::Cancel] + } else { + vec![ + Decision::Accept, + Decision::AcceptForSession, + Decision::Cancel, + ] + }; + assert_eq!(available_decisions, expected); + } + + let mut request = exec_approval_request( thread_id, "turn-approval", "call-approval", Some("callback"), ); - let Some(ThreadInteractiveRequest::Approval(ApprovalRequest::Exec { - available_decisions, - .. - })) = app - .interactive_request_for_thread_request(thread_id, &request) - .await? - else { - panic!("expected exec approval request"); - }; - assert_eq!( - available_decisions, - vec![Decision::Accept, Decision::Cancel] - ); - - let mut request = request; let ServerRequest::CommandExecutionRequestApproval { params, .. } = &mut request else { unreachable!(); }; + params.approval_purpose = Some(Purpose::Execve); params.available_decisions = Some(vec![Decision::AcceptForSession]); let Some(ThreadInteractiveRequest::Approval(ApprovalRequest::Exec { available_decisions, @@ -2564,6 +2586,7 @@ async fn callback_exec_approval_falls_back_to_one_shot_decisions() -> std::io::R panic!("expected exec approval request"); }; assert_eq!(available_decisions, vec![Decision::AcceptForSession]); + Ok(()) } @@ -4746,6 +4769,7 @@ fn exec_approval_request( item_id: item_id.to_string(), started_at_ms: 0, approval_id: approval_id.map(str::to_string), + approval_purpose: None, environment_id: None, reason: Some("needs approval".to_string()), network_approval_context: None, diff --git a/codex-rs/tui/src/app/thread_events.rs b/codex-rs/tui/src/app/thread_events.rs index 8d41f1c052..e6d69bdf44 100644 --- a/codex-rs/tui/src/app/thread_events.rs +++ b/codex-rs/tui/src/app/thread_events.rs @@ -488,6 +488,7 @@ mod tests { item_id: item_id.to_string(), started_at_ms: 0, approval_id: approval_id.map(str::to_string), + approval_purpose: None, environment_id: None, reason: Some("needs approval".to_string()), network_approval_context: None, diff --git a/codex-rs/tui/src/app/thread_routing.rs b/codex-rs/tui/src/app/thread_routing.rs index 40e5e02bb5..2f8ad2bcc1 100644 --- a/codex-rs/tui/src/app/thread_routing.rs +++ b/codex-rs/tui/src/app/thread_routing.rs @@ -242,6 +242,7 @@ impl App { available_decisions: params.available_decisions.clone().unwrap_or_else(|| { default_exec_approval_decisions( params.approval_id.as_deref(), + params.approval_purpose, network_approval_context.as_ref(), proposed_execpolicy_amendment.as_ref(), proposed_network_policy_amendments.as_deref(), diff --git a/codex-rs/tui/src/approval_events.rs b/codex-rs/tui/src/approval_events.rs index f56d86ea3d..413010dc6f 100644 --- a/codex-rs/tui/src/approval_events.rs +++ b/codex-rs/tui/src/approval_events.rs @@ -11,6 +11,7 @@ use std::path::PathBuf; use crate::diff_model::FileChange; use codex_app_server_protocol::AdditionalPermissionProfile; use codex_app_server_protocol::CommandExecutionApprovalDecision; +use codex_app_server_protocol::CommandExecutionApprovalPurpose; use codex_app_server_protocol::ExecPolicyAmendment; use codex_app_server_protocol::NetworkApprovalContext; use codex_app_server_protocol::NetworkPolicyAmendment; @@ -24,6 +25,8 @@ pub(crate) struct ExecApprovalRequestEvent { pub(crate) call_id: String, #[serde(default, skip_serializing_if = "Option::is_none")] pub(crate) approval_id: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub(crate) approval_purpose: Option, #[serde(default)] pub(crate) turn_id: String, #[serde(default, skip_serializing_if = "Option::is_none")] @@ -53,7 +56,7 @@ impl ExecApprovalRequestEvent { pub(crate) fn effective_available_decisions(&self) -> Vec { match &self.available_decisions { Some(decisions) => decisions.clone(), - None if self.approval_id.is_some() => vec![ + None if is_callback_scoped(self.approval_id.as_deref(), self.approval_purpose) => vec![ CommandExecutionApprovalDecision::Accept, CommandExecutionApprovalDecision::Cancel, ], @@ -115,6 +118,20 @@ impl ExecApprovalRequestEvent { } } +pub(crate) fn is_callback_scoped( + approval_id: Option<&str>, + approval_purpose: Option, +) -> bool { + approval_id.is_some() + || matches!( + approval_purpose, + Some( + CommandExecutionApprovalPurpose::Execve + | CommandExecutionApprovalPurpose::SandboxRetry + ) + ) +} + #[derive(Debug, Clone, Serialize, Deserialize)] pub(crate) struct ApplyPatchApprovalRequestEvent { pub(crate) call_id: String, diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 42903d2e7f..631fe197f3 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -873,6 +873,7 @@ fn exec_approval_request_from_params( additional_permissions: params.additional_permissions, turn_id: params.turn_id, approval_id: params.approval_id, + approval_purpose: params.approval_purpose, environment_id: params.environment_id, proposed_execpolicy_amendment: params.proposed_execpolicy_amendment, proposed_network_policy_amendments: params.proposed_network_policy_amendments, diff --git a/codex-rs/tui/src/chatwidget/interrupts.rs b/codex-rs/tui/src/chatwidget/interrupts.rs index 7b4c23e273..44fc450061 100644 --- a/codex-rs/tui/src/chatwidget/interrupts.rs +++ b/codex-rs/tui/src/chatwidget/interrupts.rs @@ -160,6 +160,7 @@ mod tests { ExecApprovalRequestEvent { call_id: call_id.to_string(), approval_id: approval_id.map(str::to_string), + approval_purpose: None, turn_id: "turn".to_string(), environment_id: None, command: vec!["true".to_string()], diff --git a/codex-rs/tui/src/chatwidget/tests/approval_requests.rs b/codex-rs/tui/src/chatwidget/tests/approval_requests.rs index 29b2cb80b5..7e152628be 100644 --- a/codex-rs/tui/src/chatwidget/tests/approval_requests.rs +++ b/codex-rs/tui/src/chatwidget/tests/approval_requests.rs @@ -12,6 +12,7 @@ async fn exec_approval_emits_proposed_command_and_decision_history() { let ev = ExecApprovalRequestEvent { call_id: "call-short".into(), approval_id: None, + approval_purpose: None, turn_id: "turn-short".into(), environment_id: Some("remote".to_string()), command: vec!["bash".into(), "-lc".into(), "echo hello world".into()], @@ -58,6 +59,7 @@ fn app_server_exec_approval_request_splits_shell_wrapped_command() { item_id: "item-1".to_string(), started_at_ms: 0, approval_id: Some("approval-1".to_string()), + approval_purpose: None, environment_id: None, reason: None, network_approval_context: None, @@ -100,6 +102,7 @@ fn app_server_exec_approval_request_preserves_permissions_context() { item_id: "item-1".to_string(), started_at_ms: 0, approval_id: Some("approval-1".to_string()), + approval_purpose: None, environment_id: None, reason: None, network_approval_context: Some(codex_app_server_protocol::NetworkApprovalContext { @@ -160,6 +163,7 @@ async fn network_exec_approval_history_describes_session_host_allowance() { item_id: "item-1".to_string(), started_at_ms: 0, approval_id: Some("approval-1".to_string()), + approval_purpose: None, environment_id: None, reason: None, network_approval_context: Some(codex_app_server_protocol::NetworkApprovalContext { @@ -202,6 +206,7 @@ async fn network_exec_approval_history_describes_one_time_host_allowance() { item_id: "item-1".to_string(), started_at_ms: 0, approval_id: Some("approval-1".to_string()), + approval_purpose: None, environment_id: None, reason: None, network_approval_context: Some(codex_app_server_protocol::NetworkApprovalContext { @@ -244,6 +249,7 @@ async fn network_exec_approval_history_describes_canceled_host_request() { item_id: "item-1".to_string(), started_at_ms: 0, approval_id: Some("approval-1".to_string()), + approval_purpose: None, environment_id: None, reason: None, network_approval_context: Some(codex_app_server_protocol::NetworkApprovalContext { @@ -335,6 +341,7 @@ async fn exec_approval_uses_approval_id_when_present() { ExecApprovalRequestEvent { call_id: "call-parent".into(), approval_id: Some("approval-subcommand".into()), + approval_purpose: None, turn_id: "turn-short".into(), environment_id: None, command: vec!["bash".into(), "-lc".into(), "echo hello world".into()], @@ -378,6 +385,7 @@ async fn exec_approval_decision_truncates_multiline_and_long_commands() { let ev_multi = ExecApprovalRequestEvent { call_id: "call-multi".into(), approval_id: None, + approval_purpose: None, turn_id: "turn-multi".into(), environment_id: None, command: vec!["bash".into(), "-lc".into(), "echo line1\necho line2".into()], @@ -430,6 +438,7 @@ async fn exec_approval_decision_truncates_multiline_and_long_commands() { let ev_long = ExecApprovalRequestEvent { call_id: "call-long".into(), approval_id: None, + approval_purpose: None, turn_id: "turn-long".into(), environment_id: None, command: vec!["bash".into(), "-lc".into(), long], diff --git a/codex-rs/tui/src/chatwidget/tests/exec_flow.rs b/codex-rs/tui/src/chatwidget/tests/exec_flow.rs index f4e5212173..3482965c6b 100644 --- a/codex-rs/tui/src/chatwidget/tests/exec_flow.rs +++ b/codex-rs/tui/src/chatwidget/tests/exec_flow.rs @@ -9,6 +9,7 @@ async fn exec_approval_emits_proposed_command_and_decision_history() { let ev = ExecApprovalRequestEvent { call_id: "call-short".into(), approval_id: None, + approval_purpose: None, turn_id: "turn-short".into(), environment_id: None, command: vec!["bash".into(), "-lc".into(), "echo hello world".into()], @@ -57,6 +58,7 @@ fn app_server_exec_approval_request_splits_shell_wrapped_command() { item_id: "item-1".to_string(), started_at_ms: 0, approval_id: Some("approval-1".to_string()), + approval_purpose: None, environment_id: None, reason: None, network_approval_context: None, @@ -91,6 +93,7 @@ async fn exec_approval_uses_approval_id_when_present() { let event = ExecApprovalRequestEvent { call_id: "call-parent".into(), approval_id: Some("approval-subcommand".into()), + approval_purpose: None, turn_id: "turn-short".into(), environment_id: None, command: vec!["bash".into(), "-lc".into(), "echo hello world".into()], @@ -104,13 +107,40 @@ async fn exec_approval_uses_approval_id_when_present() { additional_permissions: None, available_decisions: None, }; + use codex_app_server_protocol::CommandExecutionApprovalDecision as Decision; + use codex_app_server_protocol::CommandExecutionApprovalPurpose as Purpose; + for (approval_id, purpose, callback_scoped) in [ + (None, None, false), + (None, Some(Purpose::Initial), false), + (Some("callback"), None, true), + (Some("callback"), Some(Purpose::Initial), true), + (None, Some(Purpose::Execve), true), + (None, Some(Purpose::SandboxRetry), true), + ] { + let mut candidate = event.clone(); + candidate.approval_id = approval_id.map(str::to_string); + candidate.approval_purpose = purpose; + let expected = if callback_scoped { + vec![Decision::Accept, Decision::Cancel] + } else { + vec![ + Decision::Accept, + Decision::AcceptForSession, + Decision::Cancel, + ] + }; + assert_eq!(candidate.effective_available_decisions(), expected); + } + + let mut explicit = event.clone(); + explicit.approval_id = None; + explicit.approval_purpose = Some(Purpose::Execve); + explicit.available_decisions = Some(vec![Decision::AcceptForSession]); assert_eq!( - event.effective_available_decisions(), - vec![ - codex_app_server_protocol::CommandExecutionApprovalDecision::Accept, - codex_app_server_protocol::CommandExecutionApprovalDecision::Cancel, - ] + explicit.effective_available_decisions(), + vec![Decision::AcceptForSession] ); + handle_exec_approval_request(&mut chat, "sub-short", event); chat.handle_key_event(KeyEvent::new(KeyCode::Char('y'), KeyModifiers::NONE)); @@ -142,6 +172,7 @@ async fn exec_approval_decision_truncates_multiline_and_long_commands() { let ev_multi = ExecApprovalRequestEvent { call_id: "call-multi".into(), approval_id: None, + approval_purpose: None, turn_id: "turn-multi".into(), environment_id: None, command: vec!["bash".into(), "-lc".into(), "echo line1\necho line2".into()], @@ -196,6 +227,7 @@ async fn exec_approval_decision_truncates_multiline_and_long_commands() { let ev_long = ExecApprovalRequestEvent { call_id: "call-long".into(), approval_id: None, + approval_purpose: None, turn_id: "turn-long".into(), environment_id: None, command: vec!["bash".into(), "-lc".into(), long], @@ -1121,6 +1153,7 @@ async fn approval_modal_exec_snapshot() -> anyhow::Result<()> { let ev = ExecApprovalRequestEvent { call_id: "call-approve-cmd".into(), approval_id: None, + approval_purpose: None, turn_id: "turn-approve-cmd".into(), environment_id: None, command: vec!["bash".into(), "-lc".into(), "echo hello world".into()], @@ -1179,6 +1212,7 @@ async fn approval_modal_exec_without_reason_snapshot() -> anyhow::Result<()> { let ev = ExecApprovalRequestEvent { call_id: "call-approve-cmd-noreason".into(), approval_id: None, + approval_purpose: None, turn_id: "turn-approve-cmd-noreason".into(), environment_id: None, command: vec!["bash".into(), "-lc".into(), "echo hello world".into()], @@ -1226,6 +1260,7 @@ async fn approval_modal_exec_multiline_prefix_hides_execpolicy_option_snapshot() let ev = ExecApprovalRequestEvent { call_id: "call-approve-cmd-multiline-trunc".into(), approval_id: None, + approval_purpose: None, turn_id: "turn-approve-cmd-multiline-trunc".into(), environment_id: None, command: command.clone(), diff --git a/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs b/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs index bd11c703a0..e121a56fe4 100644 --- a/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs +++ b/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs @@ -2045,6 +2045,7 @@ async fn status_widget_and_approval_modal_snapshot() { let ev = ExecApprovalRequestEvent { call_id: "call-approve-exec".into(), approval_id: None, + approval_purpose: None, turn_id: "turn-approve-exec".into(), environment_id: None, command: vec!["echo".into(), "hello world".into()], diff --git a/codex-rs/tui/src/chatwidget/tests/terminal_title.rs b/codex-rs/tui/src/chatwidget/tests/terminal_title.rs index 7445162759..678d086cf8 100644 --- a/codex-rs/tui/src/chatwidget/tests/terminal_title.rs +++ b/codex-rs/tui/src/chatwidget/tests/terminal_title.rs @@ -12,6 +12,7 @@ async fn terminal_title_shows_action_required_while_exec_approval_is_pending() { let request = ExecApprovalRequestEvent { call_id: "call-action-required".into(), approval_id: Some("call-action-required".into()), + approval_purpose: None, turn_id: "turn-action-required".into(), environment_id: None, command: vec!["bash".into(), "-lc".into(), "echo hello".into()], @@ -55,6 +56,7 @@ async fn terminal_title_action_required_respects_spinner_setting() { let request = ExecApprovalRequestEvent { call_id: "call-no-spinner".into(), approval_id: Some("call-no-spinner".into()), + approval_purpose: None, turn_id: "turn-no-spinner".into(), environment_id: None, command: vec!["bash".into(), "-lc".into(), "echo hello".into()], @@ -84,6 +86,7 @@ async fn terminal_title_action_required_blinks_when_animations_are_enabled() { let request = ExecApprovalRequestEvent { call_id: "call-blink".into(), approval_id: Some("call-blink".into()), + approval_purpose: None, turn_id: "turn-blink".into(), environment_id: None, command: vec!["bash".into(), "-lc".into(), "echo hello".into()], @@ -120,6 +123,7 @@ async fn terminal_title_activity_indicators_do_not_animate_when_animations_are_d let request = ExecApprovalRequestEvent { call_id: "call-no-animations".into(), approval_id: Some("call-no-animations".into()), + approval_purpose: None, turn_id: "turn-no-animations".into(), environment_id: None, command: vec!["bash".into(), "-lc".into(), "echo hello".into()],