From 734db45cb16f93241d80fee8506e155a43da531e Mon Sep 17 00:00:00 2001 From: Charles Cunningham Date: Tue, 24 Mar 2026 16:16:14 -0700 Subject: [PATCH] Preserve timeout approval outcomes Co-authored-by: Codex --- .../app-server/src/bespoke_event_handling.rs | 16 ++-- codex-rs/core/src/codex.rs | 47 ++++++---- codex-rs/core/src/codex_delegate.rs | 83 +++++++++++------ codex-rs/core/src/codex_delegate_tests.rs | 88 ++++++++++++++++++- codex-rs/core/src/guardian/review.rs | 10 +++ codex-rs/core/src/mcp_tool_call.rs | 6 ++ codex-rs/core/src/mcp_tool_call_tests.rs | 20 +++++ codex-rs/core/src/state/turn.rs | 10 +-- codex-rs/core/src/tools/network_approval.rs | 35 ++++++-- codex-rs/core/src/tools/orchestrator.rs | 66 ++++++++------ .../core/src/tools/runtimes/apply_patch.rs | 13 +-- codex-rs/core/src/tools/runtimes/shell.rs | 3 +- .../tools/runtimes/shell/unix_escalation.rs | 37 ++++---- .../core/src/tools/runtimes/unified_exec.rs | 3 +- codex-rs/core/src/tools/sandboxing.rs | 51 +++-------- codex-rs/core/tests/suite/approvals.rs | 29 +++--- codex-rs/core/tests/suite/codex_delegate.rs | 5 +- codex-rs/core/tests/suite/otel.rs | 17 ++-- .../core/tests/suite/request_permissions.rs | 25 +++--- .../tests/suite/request_permissions_tool.rs | 3 +- codex-rs/core/tests/suite/skill_approval.rs | 9 +- codex-rs/mcp-server/src/exec_approval.rs | 2 +- codex-rs/mcp-server/src/patch_approval.rs | 6 +- codex-rs/otel/src/events/session_telemetry.rs | 6 +- .../tests/suite/otel_export_routing_policy.rs | 48 ++++++++++ codex-rs/protocol/src/protocol.rs | 47 +++++++++- .../tui/src/app/pending_interactive_replay.rs | 12 ++- .../tui/src/bottom_pane/approval_overlay.rs | 16 +++- codex-rs/tui/src/chatwidget/tests.rs | 18 +++- .../src/app/app_server_requests.rs | 32 ++++--- .../src/app/pending_interactive_replay.rs | 6 +- codex-rs/tui_app_server/src/app_command.rs | 18 ++-- .../src/bottom_pane/approval_overlay.rs | 12 ++- .../tui_app_server/src/chatwidget/tests.rs | 18 +++- 34 files changed, 574 insertions(+), 243 deletions(-) diff --git a/codex-rs/app-server/src/bespoke_event_handling.rs b/codex-rs/app-server/src/bespoke_event_handling.rs index 5f6a9c552e..e14f081b36 100644 --- a/codex-rs/app-server/src/bespoke_event_handling.rs +++ b/codex-rs/app-server/src/bespoke_event_handling.rs @@ -2213,7 +2213,9 @@ async fn on_patch_approval_response( if let Err(submit_err) = codex .submit(Op::PatchApproval { id: call_id.clone(), - decision: ReviewDecision::Denied, + outcome: codex_protocol::protocol::ApprovalOutcome::from( + ReviewDecision::Denied, + ), }) .await { @@ -2226,7 +2228,9 @@ async fn on_patch_approval_response( if let Err(submit_err) = codex .submit(Op::PatchApproval { id: call_id.clone(), - decision: ReviewDecision::Denied, + outcome: codex_protocol::protocol::ApprovalOutcome::from( + ReviewDecision::Denied, + ), }) .await { @@ -2247,7 +2251,7 @@ async fn on_patch_approval_response( if let Err(err) = codex .submit(Op::PatchApproval { id: call_id, - decision: response.decision, + outcome: codex_protocol::protocol::ApprovalOutcome::from(response.decision), }) .await { @@ -2290,7 +2294,7 @@ async fn on_exec_approval_response( .submit(Op::ExecApproval { id: call_id, turn_id: Some(turn_id), - decision: response.decision, + outcome: codex_protocol::protocol::ApprovalOutcome::from(response.decision), }) .await { @@ -2612,7 +2616,7 @@ async fn on_file_change_request_approval_response( if let Err(err) = codex .submit(Op::PatchApproval { id: item_id, - decision, + outcome: codex_protocol::protocol::ApprovalOutcome::from(decision), }) .await { @@ -2736,7 +2740,7 @@ async fn on_command_execution_request_approval_response( .submit(Op::ExecApproval { id: approval_id.unwrap_or_else(|| item_id.clone()), turn_id: Some(event_turn_id), - decision, + outcome: codex_protocol::protocol::ApprovalOutcome::from(decision), }) .await { diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index b299bec369..512977e0a1 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -265,6 +265,7 @@ use crate::project_doc::get_user_instructions; use crate::protocol::AgentMessageContentDeltaEvent; use crate::protocol::AgentReasoningSectionBreakEvent; use crate::protocol::ApplyPatchApprovalRequestEvent; +use crate::protocol::ApprovalOutcome; use crate::protocol::AskForApproval; use crate::protocol::BackgroundEventEvent; use crate::protocol::CompactedItem; @@ -2889,7 +2890,7 @@ impl Session { additional_permissions: Option, skill_metadata: Option, available_decisions: Option>, - ) -> ReviewDecision { + ) -> ApprovalOutcome { // 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()); @@ -2946,7 +2947,9 @@ impl Session { parsed_cmd, }); self.send_event(turn_context, event).await; - rx_approve.await.unwrap_or(ReviewDecision::Abort) + rx_approve + .await + .unwrap_or_else(|_| ApprovalOutcome::from(ReviewDecision::Abort)) } pub async fn request_patch_approval( @@ -2956,7 +2959,7 @@ impl Session { changes: HashMap, reason: Option, grant_root: Option, - ) -> oneshot::Receiver { + ) -> oneshot::Receiver { // 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(); @@ -3250,7 +3253,7 @@ impl Session { } } - pub async fn notify_approval(&self, approval_id: &str, decision: ReviewDecision) { + pub async fn notify_approval(&self, approval_id: &str, outcome: ApprovalOutcome) { let entry = { let mut active = self.active_turn.lock().await; match active.as_mut() { @@ -3263,7 +3266,7 @@ impl Session { }; match entry { Some(tx_approve) => { - tx_approve.send(decision).ok(); + tx_approve.send(outcome).ok(); } None => { warn!("No pending approval found for call_id: {approval_id}"); @@ -4339,13 +4342,13 @@ async fn submission_loop(sess: Arc, config: Arc, rx_sub: Receiv Op::ExecApproval { id: approval_id, turn_id, - decision, + outcome, } => { - handlers::exec_approval(&sess, approval_id, turn_id, decision).await; + handlers::exec_approval(&sess, approval_id, turn_id, outcome).await; false } - Op::PatchApproval { id, decision } => { - handlers::patch_approval(&sess, id, decision).await; + Op::PatchApproval { id, outcome } => { + handlers::patch_approval(&sess, id, outcome).await; false } Op::UserInputAnswer { id, response } => { @@ -4511,6 +4514,7 @@ mod handlers { use crate::tasks::UserShellCommandTask; use crate::tasks::execute_user_shell_command; use codex_protocol::custom_prompts::CustomPrompt; + use codex_protocol::protocol::ApprovalOutcome; use codex_protocol::protocol::CodexErrorInfo; use codex_protocol::protocol::ErrorEvent; use codex_protocol::protocol::Event; @@ -4755,12 +4759,15 @@ mod handlers { sess: &Arc, approval_id: String, turn_id: Option, - decision: ReviewDecision, + outcome: ApprovalOutcome, ) { let event_turn_id = turn_id.unwrap_or_else(|| approval_id.clone()); - if let ReviewDecision::ApprovedExecpolicyAmendment { - proposed_execpolicy_amendment, - } = &decision + if let ApprovalOutcome::Decision { + decision: + ReviewDecision::ApprovedExecpolicyAmendment { + proposed_execpolicy_amendment, + }, + } = &outcome { match sess .persist_execpolicy_amendment(proposed_execpolicy_amendment) @@ -4785,17 +4792,21 @@ mod handlers { } } } - match decision { - ReviewDecision::Abort => { + match outcome { + ApprovalOutcome::Decision { + decision: ReviewDecision::Abort, + } => { sess.interrupt_task().await; } other => sess.notify_approval(&approval_id, other).await, } } - pub async fn patch_approval(sess: &Arc, id: String, decision: ReviewDecision) { - match decision { - ReviewDecision::Abort => { + pub async fn patch_approval(sess: &Arc, id: String, outcome: ApprovalOutcome) { + match outcome { + ApprovalOutcome::Decision { + decision: ReviewDecision::Abort, + } => { sess.interrupt_task().await; } other => sess.notify_approval(&id, other).await, diff --git a/codex-rs/core/src/codex_delegate.rs b/codex-rs/core/src/codex_delegate.rs index c59e429c53..2ef9372722 100644 --- a/codex-rs/core/src/codex_delegate.rs +++ b/codex-rs/core/src/codex_delegate.rs @@ -5,6 +5,7 @@ use async_channel::Receiver; use async_channel::Sender; use codex_async_utils::OrCancelExt; use codex_protocol::protocol::ApplyPatchApprovalRequestEvent; +use codex_protocol::protocol::ApprovalOutcome; use codex_protocol::protocol::Event; use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::ExecApprovalRequestEvent; @@ -45,6 +46,7 @@ use crate::guardian::routes_approval_to_guardian; use crate::mcp_tool_call::MCP_TOOL_APPROVAL_ACCEPT; use crate::mcp_tool_call::MCP_TOOL_APPROVAL_ACCEPT_FOR_SESSION; use crate::mcp_tool_call::MCP_TOOL_APPROVAL_DECLINE_SYNTHETIC; +use crate::mcp_tool_call::MCP_TOOL_APPROVAL_TIMED_OUT_SYNTHETIC; use crate::mcp_tool_call::build_guardian_mcp_tool_review_request; use crate::mcp_tool_call::is_mcp_tool_approval_question_id; use crate::mcp_tool_call::lookup_mcp_tool_metadata; @@ -437,7 +439,7 @@ async fn handle_exec_approval( available_decisions, .. } = event; - let decision = if routes_approval_to_guardian(parent_ctx) { + let outcome = if routes_approval_to_guardian(parent_ctx) { let review_cancel = cancel_token.child_token(); let review_rx = spawn_guardian_review( Arc::clone(parent_session), @@ -458,7 +460,11 @@ async fn handle_exec_approval( review_cancel.clone(), ); await_approval_with_cancel( - async move { review_rx.await.unwrap_or_default() }, + async move { + review_rx + .await + .unwrap_or_else(|_| ApprovalOutcome::from(ReviewDecision::Denied)) + }, parent_session, &approval_id_for_op, cancel_token, @@ -492,7 +498,7 @@ async fn handle_exec_approval( .submit(Op::ExecApproval { id: approval_id_for_op, turn_id: Some(turn_id), - decision, + outcome, }) .await; } @@ -514,7 +520,7 @@ async fn handle_patch_approval( .. } = event; let approval_id = call_id.clone(); - let guardian_decision = if routes_approval_to_guardian(parent_ctx) { + let guardian_outcome = if routes_approval_to_guardian(parent_ctx) { let change_count = changes.len(); let maybe_files = changes .keys() @@ -564,7 +570,11 @@ async fn handle_patch_approval( ); Some( await_approval_with_cancel( - async move { review_rx.await.unwrap_or_default() }, + async move { + review_rx + .await + .unwrap_or_else(|_| ApprovalOutcome::from(ReviewDecision::Denied)) + }, parent_session, &approval_id, cancel_token, @@ -578,14 +588,18 @@ async fn handle_patch_approval( } else { None }; - let decision = if let Some(decision) = guardian_decision { - decision + let outcome = if let Some(outcome) = guardian_outcome { + outcome } else { let decision_rx = parent_session .request_patch_approval(parent_ctx, call_id, changes, reason, grant_root) .await; await_approval_with_cancel( - async move { decision_rx.await.unwrap_or_default() }, + async move { + decision_rx + .await + .unwrap_or_else(|_| ApprovalOutcome::from(ReviewDecision::Abort)) + }, parent_session, &approval_id, cancel_token, @@ -596,7 +610,7 @@ async fn handle_patch_approval( let _ = codex .submit(Op::PatchApproval { id: approval_id, - decision, + outcome, }) .await; } @@ -680,16 +694,22 @@ async fn maybe_auto_review_mcp_request_user_input( /*retry_reason*/ None, review_cancel.clone(), ); - let decision = await_approval_with_cancel( - async move { review_rx.await.unwrap_or_default() }, + let outcome = await_approval_with_cancel( + async move { + review_rx + .await + .unwrap_or_else(|_| ApprovalOutcome::from(ReviewDecision::Denied)) + }, parent_session, &event.call_id, cancel_token, Some(&review_cancel), ) .await; - let selected_label = match decision { - ReviewDecision::ApprovedForSession => question + let selected_label = match outcome { + ApprovalOutcome::Decision { + decision: ReviewDecision::ApprovedForSession, + } => question .options .as_ref() .and_then(|options| { @@ -699,12 +719,16 @@ async fn maybe_auto_review_mcp_request_user_input( }) .map(|option| option.label.clone()) .unwrap_or_else(|| MCP_TOOL_APPROVAL_ACCEPT.to_string()), - ReviewDecision::Approved - | ReviewDecision::ApprovedExecpolicyAmendment { .. } - | ReviewDecision::NetworkPolicyAmendment { .. } => MCP_TOOL_APPROVAL_ACCEPT.to_string(), - ReviewDecision::Denied | ReviewDecision::Abort => { - MCP_TOOL_APPROVAL_DECLINE_SYNTHETIC.to_string() - } + ApprovalOutcome::Decision { + decision: + ReviewDecision::Approved + | ReviewDecision::ApprovedExecpolicyAmendment { .. } + | ReviewDecision::NetworkPolicyAmendment { .. }, + } => MCP_TOOL_APPROVAL_ACCEPT.to_string(), + ApprovalOutcome::TimedOut => MCP_TOOL_APPROVAL_TIMED_OUT_SYNTHETIC.to_string(), + ApprovalOutcome::Decision { + decision: ReviewDecision::Denied | ReviewDecision::Abort, + } => MCP_TOOL_APPROVAL_DECLINE_SYNTHETIC.to_string(), }; Some(RequestUserInputResponse { answers: HashMap::from([( @@ -722,14 +746,14 @@ fn spawn_guardian_review( request: GuardianApprovalRequest, retry_reason: Option, cancel_token: CancellationToken, -) -> oneshot::Receiver { +) -> oneshot::Receiver { let (tx, rx) = oneshot::channel(); std::thread::spawn(move || { let Ok(runtime) = tokio::runtime::Builder::new_current_thread() .enable_all() .build() else { - let _ = tx.send(ReviewDecision::Denied); + let _ = tx.send(ApprovalOutcome::from(ReviewDecision::Denied)); return; }; let decision = runtime.block_on(review_approval_request_with_cancel( @@ -739,7 +763,7 @@ fn spawn_guardian_review( retry_reason, cancel_token, )); - let _ = tx.send(decision.into_review_decision()); + let _ = tx.send(decision.into()); }); rx } @@ -822,16 +846,16 @@ where } } -/// Await an approval decision, aborting on cancellation. +/// Await an approval outcome, aborting on cancellation. async fn await_approval_with_cancel( fut: F, parent_session: &Session, approval_id: &str, cancel_token: &CancellationToken, review_cancel_token: Option<&CancellationToken>, -) -> codex_protocol::protocol::ReviewDecision +) -> ApprovalOutcome where - F: core::future::Future, + F: core::future::Future, { tokio::select! { biased; @@ -839,13 +863,14 @@ where if let Some(review_cancel_token) = review_cancel_token { review_cancel_token.cancel(); } + let abort = ApprovalOutcome::from(ReviewDecision::Abort); parent_session - .notify_approval(approval_id, codex_protocol::protocol::ReviewDecision::Abort) + .notify_approval(approval_id, abort.clone()) .await; - codex_protocol::protocol::ReviewDecision::Abort + abort } - decision = fut => { - decision + outcome = fut => { + outcome } } } diff --git a/codex-rs/core/src/codex_delegate_tests.rs b/codex-rs/core/src/codex_delegate_tests.rs index 8201424d8e..bbec40f50e 100644 --- a/codex-rs/core/src/codex_delegate_tests.rs +++ b/codex-rs/core/src/codex_delegate_tests.rs @@ -6,6 +6,7 @@ use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::models::NetworkPermissions; use codex_protocol::models::ResponseItem; use codex_protocol::protocol::AgentStatus; +use codex_protocol::protocol::ApprovalOutcome; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::ExecApprovalRequestEvent; @@ -342,7 +343,92 @@ async fn handle_exec_approval_uses_call_id_for_guardian_review_and_approval_id_f Op::ExecApproval { id: "callback-approval-1".to_string(), turn_id: Some("child-turn-1".to_string()), - decision: ReviewDecision::Abort, + outcome: ApprovalOutcome::from(ReviewDecision::Abort), + } + ); +} + +#[tokio::test] +async fn handle_exec_approval_preserves_timed_out_outcome_for_reply() { + let (parent_session, parent_ctx, rx_events) = + crate::codex::make_session_and_context_with_rx().await; + *parent_session.active_turn.lock().await = Some(crate::state::ActiveTurn::default()); + + let (tx_sub, rx_sub) = bounded(SUBMISSION_CHANNEL_CAPACITY); + let (_tx_events, rx_events_child) = bounded(SUBMISSION_CHANNEL_CAPACITY); + let (_agent_status_tx, agent_status) = watch::channel(AgentStatus::PendingInit); + let codex = Arc::new(Codex { + tx_sub, + rx_event: rx_events_child, + agent_status, + session: Arc::clone(&parent_session), + session_loop_termination: completed_session_loop_termination(), + }); + + let cancel_token = CancellationToken::new(); + let handle = tokio::spawn({ + let codex = Arc::clone(&codex); + let parent_session = Arc::clone(&parent_session); + let parent_ctx = Arc::clone(&parent_ctx); + let cancel_token = cancel_token.clone(); + async move { + handle_exec_approval( + codex.as_ref(), + "child-turn-1".to_string(), + &parent_session, + &parent_ctx, + ExecApprovalRequestEvent { + call_id: "command-item-1".to_string(), + approval_id: Some("callback-approval-1".to_string()), + turn_id: "child-turn-1".to_string(), + command: vec!["rm".to_string(), "-rf".to_string(), "tmp".to_string()], + cwd: PathBuf::from("/tmp"), + reason: Some("unsafe subcommand".to_string()), + network_approval_context: None, + proposed_execpolicy_amendment: None, + proposed_network_policy_amendments: None, + additional_permissions: None, + skill_metadata: None, + available_decisions: Some(vec![ + ReviewDecision::Approved, + ReviewDecision::Abort, + ]), + parsed_cmd: Vec::new(), + }, + &cancel_token, + ) + .await; + } + }); + + let request_event = timeout(Duration::from_secs(2), rx_events.recv()) + .await + .expect("timed out waiting for exec approval request") + .expect("exec approval request missing"); + let EventMsg::ExecApprovalRequest(request) = request_event.msg else { + panic!("expected ExecApprovalRequest event"); + }; + assert_eq!(request.effective_approval_id(), "callback-approval-1"); + + parent_session + .notify_approval("callback-approval-1", ApprovalOutcome::TimedOut) + .await; + + timeout(Duration::from_secs(2), handle) + .await + .expect("handle_exec_approval hung") + .expect("handle_exec_approval join error"); + + let submission = timeout(Duration::from_secs(2), rx_sub.recv()) + .await + .expect("exec approval response timed out") + .expect("exec approval response missing"); + assert_eq!( + submission.op, + Op::ExecApproval { + id: "callback-approval-1".to_string(), + turn_id: Some("child-turn-1".to_string()), + outcome: ApprovalOutcome::TimedOut, } ); } diff --git a/codex-rs/core/src/guardian/review.rs b/codex-rs/core/src/guardian/review.rs index 63b202c117..c21f8990e9 100644 --- a/codex-rs/core/src/guardian/review.rs +++ b/codex-rs/core/src/guardian/review.rs @@ -1,6 +1,7 @@ use std::sync::Arc; use codex_protocol::config_types::ApprovalsReviewer; +use codex_protocol::protocol::ApprovalOutcome; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::GuardianAssessmentEvent; @@ -62,6 +63,15 @@ impl GuardianApprovalDecision { } } +impl From for ApprovalOutcome { + fn from(value: GuardianApprovalDecision) -> Self { + match value { + GuardianApprovalDecision::TimedOut => Self::TimedOut, + decision => Self::from(decision.into_review_decision()), + } + } +} + #[derive(Debug)] pub(super) enum GuardianReviewOutcome { Completed(anyhow::Result), diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index fc0bcd2663..9d8a7acbe7 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -455,6 +455,7 @@ pub(crate) const MCP_TOOL_APPROVAL_ACCEPT_FOR_SESSION: &str = "Allow for this se // real "Decline" answer, so this lets guardian denials round-trip distinctly from user cancel. // This is not a user-facing option. pub(crate) const MCP_TOOL_APPROVAL_DECLINE_SYNTHETIC: &str = "__codex_mcp_decline__"; +pub(crate) const MCP_TOOL_APPROVAL_TIMED_OUT_SYNTHETIC: &str = "__codex_mcp_timed_out__"; const MCP_TOOL_APPROVAL_ACCEPT_AND_REMEMBER: &str = "Allow and don't ask me again"; const MCP_TOOL_APPROVAL_CANCEL: &str = "Cancel"; const MCP_TOOL_APPROVAL_KIND_KEY: &str = "codex_approval_kind"; @@ -1195,6 +1196,11 @@ fn parse_mcp_tool_approval_response( .any(|answer| answer == MCP_TOOL_APPROVAL_DECLINE_SYNTHETIC) { McpToolApprovalDecision::Decline + } else if answers + .iter() + .any(|answer| answer == MCP_TOOL_APPROVAL_TIMED_OUT_SYNTHETIC) + { + McpToolApprovalDecision::TimedOut(GUARDIAN_TIMEOUT_MESSAGE.to_string()) } else if answers .iter() .any(|answer| answer == MCP_TOOL_APPROVAL_ACCEPT_FOR_SESSION) diff --git a/codex-rs/core/src/mcp_tool_call_tests.rs b/codex-rs/core/src/mcp_tool_call_tests.rs index 0c36e46cdb..8531000738 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -847,6 +847,26 @@ fn synthetic_decline_request_user_input_response_stays_decline() { assert_eq!(response, McpToolApprovalDecision::Decline); } +#[test] +fn synthetic_timeout_request_user_input_response_stays_timed_out() { + let response = parse_mcp_tool_approval_response( + Some(RequestUserInputResponse { + answers: HashMap::from([( + "approval".to_string(), + RequestUserInputAnswer { + answers: vec![MCP_TOOL_APPROVAL_TIMED_OUT_SYNTHETIC.to_string()], + }, + )]), + }), + "approval", + ); + + assert_eq!( + response, + McpToolApprovalDecision::TimedOut(GUARDIAN_TIMEOUT_MESSAGE.to_string()) + ); +} + #[test] fn accepted_elicitation_response_uses_always_persist_meta() { let response = parse_mcp_tool_approval_elicitation_response( diff --git a/codex-rs/core/src/state/turn.rs b/codex-rs/core/src/state/turn.rs index ebfa5a8bb8..be01cbd32d 100644 --- a/codex-rs/core/src/state/turn.rs +++ b/codex-rs/core/src/state/turn.rs @@ -18,7 +18,7 @@ use rmcp::model::RequestId; use tokio::sync::oneshot; use crate::codex::TurnContext; -use crate::protocol::ReviewDecision; +use crate::protocol::ApprovalOutcome; use crate::protocol::TokenUsage; use crate::tasks::SessionTask; use codex_protocol::models::PermissionProfile; @@ -75,7 +75,7 @@ impl ActiveTurn { /// Mutable state for a single turn. #[derive(Default)] pub(crate) struct TurnState { - pending_approvals: HashMap>, + pending_approvals: HashMap>, pending_request_permissions: HashMap>, pending_user_input: HashMap>, pending_elicitations: HashMap<(String, RequestId), oneshot::Sender>, @@ -90,15 +90,15 @@ impl TurnState { pub(crate) fn insert_pending_approval( &mut self, key: String, - tx: oneshot::Sender, - ) -> Option> { + tx: oneshot::Sender, + ) -> Option> { self.pending_approvals.insert(key, tx) } pub(crate) fn remove_pending_approval( &mut self, key: &str, - ) -> Option> { + ) -> Option> { self.pending_approvals.remove(key) } diff --git a/codex-rs/core/src/tools/network_approval.rs b/codex-rs/core/src/tools/network_approval.rs index b1bd2de657..e467e525a5 100644 --- a/codex-rs/core/src/tools/network_approval.rs +++ b/codex-rs/core/src/tools/network_approval.rs @@ -17,6 +17,7 @@ use codex_network_proxy::NetworkProxy; use codex_protocol::approvals::NetworkApprovalContext; use codex_protocol::approvals::NetworkApprovalProtocol; use codex_protocol::approvals::NetworkPolicyRuleAction; +use codex_protocol::protocol::ApprovalOutcome; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::Event; use codex_protocol::protocol::EventMsg; @@ -420,12 +421,18 @@ impl NetworkApprovalService { ) .await { - ReviewDecision::Approved | ReviewDecision::ApprovedExecpolicyAmendment { .. } => { - PendingApprovalDecision::AllowOnce - } - ReviewDecision::ApprovedForSession => PendingApprovalDecision::AllowForSession, - ReviewDecision::NetworkPolicyAmendment { - network_policy_amendment, + ApprovalOutcome::Decision { + decision: + ReviewDecision::Approved | ReviewDecision::ApprovedExecpolicyAmendment { .. }, + } => PendingApprovalDecision::AllowOnce, + ApprovalOutcome::Decision { + decision: ReviewDecision::ApprovedForSession, + } => PendingApprovalDecision::AllowForSession, + ApprovalOutcome::Decision { + decision: + ReviewDecision::NetworkPolicyAmendment { + network_policy_amendment, + }, } => match network_policy_amendment.action { NetworkPolicyRuleAction::Allow => { match session @@ -496,7 +503,9 @@ impl NetworkApprovalService { PendingApprovalDecision::Deny } }, - ReviewDecision::Denied | ReviewDecision::Abort => { + ApprovalOutcome::Decision { + decision: ReviewDecision::Denied | ReviewDecision::Abort, + } => { if let Some(owner_call) = owner_call.as_ref() { self.record_call_outcome( &owner_call.registration_id, @@ -506,6 +515,18 @@ impl NetworkApprovalService { } PendingApprovalDecision::Deny } + ApprovalOutcome::TimedOut => { + if let Some(owner_call) = owner_call.as_ref() { + self.record_call_outcome( + &owner_call.registration_id, + NetworkApprovalOutcome::TimedOutByReviewer( + GUARDIAN_TIMEOUT_MESSAGE.to_string(), + ), + ) + .await; + } + PendingApprovalDecision::Deny + } } }; diff --git a/codex-rs/core/src/tools/orchestrator.rs b/codex-rs/core/src/tools/orchestrator.rs index a5b1aea37e..dc49a2e8b8 100644 --- a/codex-rs/core/src/tools/orchestrator.rs +++ b/codex-rs/core/src/tools/orchestrator.rs @@ -19,7 +19,6 @@ use crate::tools::network_approval::begin_network_approval; use crate::tools::network_approval::finish_deferred_network_approval; use crate::tools::network_approval::finish_immediate_network_approval; use crate::tools::sandboxing::ApprovalCtx; -use crate::tools::sandboxing::ApprovalOutcome; use crate::tools::sandboxing::ExecApprovalRequirement; use crate::tools::sandboxing::SandboxAttempt; use crate::tools::sandboxing::SandboxOverride; @@ -28,6 +27,7 @@ use crate::tools::sandboxing::ToolError; use crate::tools::sandboxing::ToolRuntime; use crate::tools::sandboxing::default_exec_approval_requirement; use codex_otel::ToolDecisionSource; +use codex_protocol::protocol::ApprovalOutcome; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::NetworkPolicyRuleAction; use codex_protocol::protocol::ReviewDecision; @@ -135,7 +135,12 @@ impl ToolOrchestrator { }); match requirement { ExecApprovalRequirement::Skip { .. } => { - otel.tool_decision(otel_tn, otel_ci, &ReviewDecision::Approved, otel_cfg); + otel.tool_decision( + otel_tn, + otel_ci, + &ApprovalOutcome::from(ReviewDecision::Approved), + otel_cfg, + ); } ExecApprovalRequirement::Forbidden { reason } => { return Err(ToolError::Rejected(reason)); @@ -154,26 +159,29 @@ impl ToolOrchestrator { } else { otel_user.clone() }; - let decision = approval_outcome.decision_for_telemetry(); - - otel.tool_decision(otel_tn, otel_ci, &decision, otel_source); + otel.tool_decision(otel_tn, otel_ci, &approval_outcome, otel_source); match approval_outcome { ApprovalOutcome::TimedOut => { return Err(ToolError::Message(GUARDIAN_TIMEOUT_MESSAGE.to_string())); } - ApprovalOutcome::Decision(ReviewDecision::Denied) - | ApprovalOutcome::Decision(ReviewDecision::Abort) => { + ApprovalOutcome::Decision { + decision: ReviewDecision::Denied | ReviewDecision::Abort, + } => { return Err(approval_denial_tool_error(turn_ctx)); } - ApprovalOutcome::Decision( - ReviewDecision::Approved - | ReviewDecision::ApprovedExecpolicyAmendment { .. } - | ReviewDecision::ApprovedForSession, - ) => {} - ApprovalOutcome::Decision(ReviewDecision::NetworkPolicyAmendment { - network_policy_amendment, - }) => match network_policy_amendment.action { + ApprovalOutcome::Decision { + decision: + ReviewDecision::Approved + | ReviewDecision::ApprovedExecpolicyAmendment { .. } + | ReviewDecision::ApprovedForSession, + } => {} + ApprovalOutcome::Decision { + decision: + ReviewDecision::NetworkPolicyAmendment { + network_policy_amendment, + }, + } => match network_policy_amendment.action { NetworkPolicyRuleAction::Allow => {} NetworkPolicyRuleAction::Deny => { return Err(ToolError::Rejected("rejected by user".to_string())); @@ -310,25 +318,29 @@ impl ToolOrchestrator { } else { otel_user }; - let decision = approval_outcome.decision_for_telemetry(); - otel.tool_decision(otel_tn, otel_ci, &decision, otel_source); + otel.tool_decision(otel_tn, otel_ci, &approval_outcome, otel_source); match approval_outcome { ApprovalOutcome::TimedOut => { return Err(ToolError::Message(GUARDIAN_TIMEOUT_MESSAGE.to_string())); } - ApprovalOutcome::Decision(ReviewDecision::Denied) - | ApprovalOutcome::Decision(ReviewDecision::Abort) => { + ApprovalOutcome::Decision { + decision: ReviewDecision::Denied | ReviewDecision::Abort, + } => { return Err(approval_denial_tool_error(turn_ctx)); } - ApprovalOutcome::Decision( - ReviewDecision::Approved - | ReviewDecision::ApprovedExecpolicyAmendment { .. } - | ReviewDecision::ApprovedForSession, - ) => {} - ApprovalOutcome::Decision(ReviewDecision::NetworkPolicyAmendment { - network_policy_amendment, - }) => match network_policy_amendment.action { + ApprovalOutcome::Decision { + decision: + ReviewDecision::Approved + | ReviewDecision::ApprovedExecpolicyAmendment { .. } + | ReviewDecision::ApprovedForSession, + } => {} + ApprovalOutcome::Decision { + decision: + ReviewDecision::NetworkPolicyAmendment { + network_policy_amendment, + }, + } => match network_policy_amendment.action { NetworkPolicyRuleAction::Allow => {} NetworkPolicyRuleAction::Deny => { return Err(ToolError::Rejected("rejected by user".to_string())); diff --git a/codex-rs/core/src/tools/runtimes/apply_patch.rs b/codex-rs/core/src/tools/runtimes/apply_patch.rs index c821035fd6..6f51642115 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch.rs @@ -14,7 +14,6 @@ use crate::sandboxing::SandboxPermissions; use crate::sandboxing::execute_env; use crate::tools::sandboxing::Approvable; use crate::tools::sandboxing::ApprovalCtx; -use crate::tools::sandboxing::ApprovalOutcome; use crate::tools::sandboxing::ExecApprovalRequirement; use crate::tools::sandboxing::SandboxAttempt; use crate::tools::sandboxing::Sandboxable; @@ -25,6 +24,7 @@ use crate::tools::sandboxing::with_cached_approval; use codex_apply_patch::ApplyPatchAction; use codex_apply_patch::CODEX_CORE_APPLY_PATCH_ARG1; use codex_protocol::models::PermissionProfile; +use codex_protocol::protocol::ApprovalOutcome; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::FileChange; use codex_protocol::protocol::ReviewDecision; @@ -144,7 +144,7 @@ impl Approvable for ApplyPatchRuntime { .into(); } if req.permissions_preapproved && retry_reason.is_none() { - return ApprovalOutcome::Decision(ReviewDecision::Approved); + return ApprovalOutcome::from(ReviewDecision::Approved); } if let Some(reason) = retry_reason { let rx_approve = session @@ -156,7 +156,9 @@ impl Approvable for ApplyPatchRuntime { /*grant_root*/ None, ) .await; - return ApprovalOutcome::Decision(rx_approve.await.unwrap_or_default()); + return rx_approve + .await + .unwrap_or_else(|_| ApprovalOutcome::from(ReviewDecision::Abort)); } with_cached_approval( @@ -169,11 +171,12 @@ impl Approvable for ApplyPatchRuntime { turn, call_id, changes, /*reason*/ None, /*grant_root*/ None, ) .await; - rx_approve.await.unwrap_or_default() + rx_approve + .await + .unwrap_or_else(|_| ApprovalOutcome::from(ReviewDecision::Abort)) }, ) .await - .into() }) } diff --git a/codex-rs/core/src/tools/runtimes/shell.rs b/codex-rs/core/src/tools/runtimes/shell.rs index b55b7ac7e5..dbef9a07e4 100644 --- a/codex-rs/core/src/tools/runtimes/shell.rs +++ b/codex-rs/core/src/tools/runtimes/shell.rs @@ -25,7 +25,6 @@ use crate::tools::runtimes::build_sandbox_command; use crate::tools::runtimes::maybe_wrap_shell_lc_with_snapshot; use crate::tools::sandboxing::Approvable; use crate::tools::sandboxing::ApprovalCtx; -use crate::tools::sandboxing::ApprovalOutcome; use crate::tools::sandboxing::ExecApprovalRequirement; use crate::tools::sandboxing::SandboxAttempt; use crate::tools::sandboxing::SandboxOverride; @@ -37,6 +36,7 @@ use crate::tools::sandboxing::sandbox_override_for_first_attempt; use crate::tools::sandboxing::with_cached_approval; use codex_network_proxy::NetworkProxy; use codex_protocol::models::PermissionProfile; +use codex_protocol::protocol::ApprovalOutcome; use codex_sandboxing::SandboxablePreference; use futures::future::BoxFuture; use std::collections::HashMap; @@ -191,7 +191,6 @@ impl Approvable for ShellRuntime { .await }) .await - .into() }) } 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 b9c3bdb089..2db7128c7b 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs @@ -16,7 +16,6 @@ use crate::shell::ShellType; use crate::skills::SkillMetadata; use crate::tools::runtimes::ExecveSessionApproval; use crate::tools::runtimes::build_sandbox_command; -use crate::tools::sandboxing::ApprovalOutcome; use crate::tools::sandboxing::SandboxAttempt; use crate::tools::sandboxing::ToolCtx; use crate::tools::sandboxing::ToolError; @@ -31,6 +30,7 @@ use codex_protocol::models::MacOsSeatbeltProfileExtensions; use codex_protocol::models::PermissionProfile; use codex_protocol::permissions::FileSystemSandboxPolicy; use codex_protocol::permissions::NetworkSandboxPolicy; +use codex_protocol::protocol::ApprovalOutcome; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::ExecApprovalRequestSkillMetadata; use codex_protocol::protocol::NetworkPolicyRuleAction; @@ -488,7 +488,6 @@ impl CoreShellActionProvider { Some(available_decisions), ) .await - .into() }) .await) } @@ -555,17 +554,20 @@ impl CoreShellActionProvider { ApprovalOutcome::TimedOut => { EscalationDecision::deny(Some(GUARDIAN_TIMEOUT_MESSAGE.to_string())) } - ApprovalOutcome::Decision( - ReviewDecision::Approved - | ReviewDecision::ApprovedExecpolicyAmendment { .. }, - ) => { + ApprovalOutcome::Decision { + decision: + ReviewDecision::Approved + | ReviewDecision::ApprovedExecpolicyAmendment { .. }, + } => { if needs_escalation { EscalationDecision::escalate(escalation_execution.clone()) } else { EscalationDecision::run() } } - ApprovalOutcome::Decision(ReviewDecision::ApprovedForSession) => { + ApprovalOutcome::Decision { + decision: ReviewDecision::ApprovedForSession, + } => { // Currently, we only add session approvals for // skill scripts because we are storing only the // `program` whereas prefix rules may be restricted by a longer prefix. @@ -592,9 +594,12 @@ impl CoreShellActionProvider { EscalationDecision::run() } } - ApprovalOutcome::Decision(ReviewDecision::NetworkPolicyAmendment { - network_policy_amendment, - }) => match network_policy_amendment.action { + ApprovalOutcome::Decision { + decision: + ReviewDecision::NetworkPolicyAmendment { + network_policy_amendment, + }, + } => match network_policy_amendment.action { NetworkPolicyRuleAction::Allow => { if needs_escalation { EscalationDecision::escalate(escalation_execution.clone()) @@ -606,12 +611,12 @@ impl CoreShellActionProvider { EscalationDecision::deny(Some("User denied execution".to_string())) } }, - ApprovalOutcome::Decision(ReviewDecision::Denied) => { - EscalationDecision::deny(Some("User denied execution".to_string())) - } - ApprovalOutcome::Decision(ReviewDecision::Abort) => { - EscalationDecision::deny(Some("User cancelled execution".to_string())) - } + ApprovalOutcome::Decision { + decision: ReviewDecision::Denied, + } => EscalationDecision::deny(Some("User denied execution".to_string())), + ApprovalOutcome::Decision { + decision: ReviewDecision::Abort, + } => EscalationDecision::deny(Some("User cancelled execution".to_string())), } } } diff --git a/codex-rs/core/src/tools/runtimes/unified_exec.rs b/codex-rs/core/src/tools/runtimes/unified_exec.rs index 3de535b1fa..f928a04cb9 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -23,7 +23,6 @@ use crate::tools::runtimes::maybe_wrap_shell_lc_with_snapshot; use crate::tools::runtimes::shell::zsh_fork_backend; use crate::tools::sandboxing::Approvable; use crate::tools::sandboxing::ApprovalCtx; -use crate::tools::sandboxing::ApprovalOutcome; use crate::tools::sandboxing::ExecApprovalRequirement; use crate::tools::sandboxing::SandboxAttempt; use crate::tools::sandboxing::SandboxOverride; @@ -40,6 +39,7 @@ use crate::unified_exec::UnifiedExecProcess; use crate::unified_exec::UnifiedExecProcessManager; use codex_network_proxy::NetworkProxy; use codex_protocol::models::PermissionProfile; +use codex_protocol::protocol::ApprovalOutcome; use codex_sandboxing::SandboxablePreference; use futures::future::BoxFuture; use std::collections::HashMap; @@ -160,7 +160,6 @@ impl Approvable for UnifiedExecRuntime<'_> { .await }) .await - .into() }) } diff --git a/codex-rs/core/src/tools/sandboxing.rs b/codex-rs/core/src/tools/sandboxing.rs index b8ec58a82c..a570e01726 100644 --- a/codex-rs/core/src/tools/sandboxing.rs +++ b/codex-rs/core/src/tools/sandboxing.rs @@ -7,7 +7,6 @@ use crate::codex::Session; use crate::codex::TurnContext; use crate::error::CodexErr; -use crate::guardian::GuardianApprovalDecision; #[cfg(test)] use crate::protocol::SandboxPolicy; use crate::sandboxing::ExecOptions; @@ -20,6 +19,7 @@ use codex_protocol::approvals::NetworkApprovalContext; use codex_protocol::permissions::FileSystemSandboxKind; use codex_protocol::permissions::FileSystemSandboxPolicy; use codex_protocol::permissions::NetworkSandboxPolicy; +use codex_protocol::protocol::ApprovalOutcome; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::ReviewDecision; use codex_sandboxing::SandboxCommand; @@ -74,11 +74,11 @@ pub(crate) async fn with_cached_approval( tool_name: &str, keys: Vec, fetch: F, -) -> ReviewDecision +) -> ApprovalOutcome where K: Serialize, F: FnOnce() -> Fut, - Fut: Future, + Fut: Future, { // To be defensive here, don't bother with checking the cache if keys are empty. if keys.is_empty() { @@ -92,58 +92,33 @@ where }; if already_approved { - return ReviewDecision::ApprovedForSession; + return ApprovalOutcome::from(ReviewDecision::ApprovedForSession); } - let decision = fetch().await; + let outcome = fetch().await; services.session_telemetry.counter( "codex.approval.requested", /*inc*/ 1, &[ ("tool", tool_name), - ("approved", decision.to_opaque_string()), + ("approved", outcome.to_opaque_string()), ], ); - if matches!(decision, ReviewDecision::ApprovedForSession) { + if matches!( + outcome, + ApprovalOutcome::Decision { + decision: ReviewDecision::ApprovedForSession, + } + ) { let mut store = services.tool_approvals.lock().await; for key in keys { store.put(key, ReviewDecision::ApprovedForSession); } } - decision -} - -#[derive(Debug)] -pub(crate) enum ApprovalOutcome { - Decision(ReviewDecision), - TimedOut, -} - -impl ApprovalOutcome { - pub(crate) fn decision_for_telemetry(&self) -> ReviewDecision { - match self { - Self::Decision(decision) => decision.clone(), - Self::TimedOut => ReviewDecision::Denied, - } - } -} - -impl From for ApprovalOutcome { - fn from(value: ReviewDecision) -> Self { - Self::Decision(value) - } -} - -impl From for ApprovalOutcome { - fn from(value: GuardianApprovalDecision) -> Self { - match value { - GuardianApprovalDecision::TimedOut => Self::TimedOut, - decision => Self::Decision(decision.into_review_decision()), - } - } + outcome } #[derive(Clone)] diff --git a/codex-rs/core/tests/suite/approvals.rs b/codex-rs/core/tests/suite/approvals.rs index 6ec05bedc2..5075c55303 100644 --- a/codex-rs/core/tests/suite/approvals.rs +++ b/codex-rs/core/tests/suite/approvals.rs @@ -15,6 +15,7 @@ use codex_protocol::approvals::NetworkApprovalProtocol; use codex_protocol::approvals::NetworkPolicyAmendment; use codex_protocol::approvals::NetworkPolicyRuleAction; use codex_protocol::protocol::ApplyPatchApprovalRequestEvent; +use codex_protocol::protocol::ApprovalOutcome; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::ExecApprovalRequestEvent; @@ -1715,7 +1716,7 @@ async fn run_scenario(scenario: &ScenarioSpec) -> Result<()> { .submit(Op::ExecApproval { id: approval.effective_approval_id(), turn_id: None, - decision: decision.clone(), + outcome: ApprovalOutcome::from(decision.clone()), }) .await?; wait_for_completion(&test).await; @@ -1736,7 +1737,7 @@ async fn run_scenario(scenario: &ScenarioSpec) -> Result<()> { test.codex .submit(Op::PatchApproval { id: approval.call_id, - decision: decision.clone(), + outcome: ApprovalOutcome::from(decision.clone()), }) .await?; wait_for_completion(&test).await; @@ -1819,7 +1820,7 @@ async fn approving_apply_patch_for_session_skips_future_prompts_for_same_file() test.codex .submit(Op::PatchApproval { id: approval.call_id, - decision: ReviewDecision::ApprovedForSession, + outcome: ApprovalOutcome::from(ReviewDecision::ApprovedForSession), }) .await?; wait_for_completion(&test).await; @@ -1939,9 +1940,9 @@ async fn approving_execpolicy_amendment_persists_policy_and_skips_future_prompts .submit(Op::ExecApproval { id: approval.effective_approval_id(), turn_id: None, - decision: ReviewDecision::ApprovedExecpolicyAmendment { + outcome: ApprovalOutcome::from(ReviewDecision::ApprovedExecpolicyAmendment { proposed_execpolicy_amendment: expected_execpolicy_amendment.clone(), - }, + }), }) .await?; wait_for_completion(&test).await; @@ -2178,9 +2179,9 @@ async fn spawned_subagent_execpolicy_amendment_propagates_to_parent_session() -> .submit(Op::ExecApproval { id: approval.effective_approval_id(), turn_id: None, - decision: ReviewDecision::ApprovedExecpolicyAmendment { + outcome: ApprovalOutcome::from(ReviewDecision::ApprovedExecpolicyAmendment { proposed_execpolicy_amendment: expected_execpolicy_amendment, - }, + }), }) .await?; @@ -2403,9 +2404,9 @@ async fn approving_fallback_rule_for_compound_command_works() -> Result<()> { .submit(Op::ExecApproval { id: approval_id, turn_id: None, - decision: ReviewDecision::ApprovedExecpolicyAmendment { + outcome: ApprovalOutcome::from(ReviewDecision::ApprovedExecpolicyAmendment { proposed_execpolicy_amendment: amendment.clone(), - }, + }), }) .await?; wait_for_completion(&test).await; @@ -2599,7 +2600,7 @@ allow_local_binding = true .submit(Op::ExecApproval { id: approval.effective_approval_id(), turn_id: None, - decision: ReviewDecision::Approved, + outcome: ApprovalOutcome::from(ReviewDecision::Approved), }) .await?; } @@ -2637,9 +2638,9 @@ allow_local_binding = true .submit(Op::ExecApproval { id: approval.effective_approval_id(), turn_id: None, - decision: ReviewDecision::NetworkPolicyAmendment { + outcome: ApprovalOutcome::from(ReviewDecision::NetworkPolicyAmendment { network_policy_amendment: deny_network_amendment.clone(), - }, + }), }) .await?; wait_for_completion(&test).await; @@ -2742,7 +2743,7 @@ allow_local_binding = true .submit(Op::ExecApproval { id: approval.effective_approval_id(), turn_id: None, - decision: ReviewDecision::Approved, + outcome: ApprovalOutcome::from(ReviewDecision::Approved), }) .await?; } @@ -2826,7 +2827,7 @@ async fn compound_command_with_one_safe_command_still_requires_approval() -> Res .submit(Op::ExecApproval { id: approval.effective_approval_id(), turn_id: None, - decision: ReviewDecision::Denied, + outcome: ApprovalOutcome::from(ReviewDecision::Denied), }) .await?; wait_for_completion(&test).await; diff --git a/codex-rs/core/tests/suite/codex_delegate.rs b/codex-rs/core/tests/suite/codex_delegate.rs index e71d99b620..785b85c59c 100644 --- a/codex-rs/core/tests/suite/codex_delegate.rs +++ b/codex-rs/core/tests/suite/codex_delegate.rs @@ -1,5 +1,6 @@ use codex_core::config::Constrained; use codex_core::sandboxing::SandboxPermissions; +use codex_protocol::protocol::ApprovalOutcome; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::Op; @@ -102,7 +103,7 @@ async fn codex_delegate_forwards_exec_approval_and_proceeds_on_approval() { .submit(Op::ExecApproval { id: approval.effective_approval_id(), turn_id: None, - decision: ReviewDecision::Approved, + outcome: ApprovalOutcome::from(ReviewDecision::Approved), }) .await .expect("submit exec approval"); @@ -181,7 +182,7 @@ async fn codex_delegate_forwards_patch_approval_and_proceeds_on_decision() { test.codex .submit(Op::PatchApproval { id: approval.call_id, - decision: ReviewDecision::Denied, + outcome: ApprovalOutcome::from(ReviewDecision::Denied), }) .await .expect("submit patch approval"); diff --git a/codex-rs/core/tests/suite/otel.rs b/codex-rs/core/tests/suite/otel.rs index 96df3fd2f1..20a14552fd 100644 --- a/codex-rs/core/tests/suite/otel.rs +++ b/codex-rs/core/tests/suite/otel.rs @@ -1,5 +1,6 @@ use codex_core::config::Constrained; use codex_features::Feature; +use codex_protocol::protocol::ApprovalOutcome; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::Op; @@ -766,7 +767,7 @@ async fn handle_response_item_records_tool_result_for_custom_tool_call() { .await .unwrap(); - wait_for_event(&codex, |ev| matches!(ev, EventMsg::TokenCount(_))).await; + wait_for_event(&codex, |ev| matches!(ev, EventMsg::TurnComplete(_))).await; logs_assert(|lines: &[&str]| { let line = lines @@ -839,7 +840,7 @@ async fn handle_response_item_records_tool_result_for_function_call() { .await .unwrap(); - wait_for_event(&codex, |ev| matches!(ev, EventMsg::TokenCount(_))).await; + wait_for_event(&codex, |ev| matches!(ev, EventMsg::TurnComplete(_))).await; logs_assert(|lines: &[&str]| { let line = lines @@ -1159,7 +1160,7 @@ async fn handle_container_exec_user_approved_records_tool_decision() { .submit(Op::ExecApproval { id: approval.effective_approval_id(), turn_id: None, - decision: ReviewDecision::Approved, + outcome: ApprovalOutcome::from(ReviewDecision::Approved), }) .await .unwrap(); @@ -1225,7 +1226,7 @@ async fn handle_container_exec_user_approved_for_session_records_tool_decision() .submit(Op::ExecApproval { id: approval.effective_approval_id(), turn_id: None, - decision: ReviewDecision::ApprovedForSession, + outcome: ApprovalOutcome::from(ReviewDecision::ApprovedForSession), }) .await .unwrap(); @@ -1291,7 +1292,7 @@ async fn handle_sandbox_error_user_approves_retry_records_tool_decision() { .submit(Op::ExecApproval { id: approval.effective_approval_id(), turn_id: None, - decision: ReviewDecision::Approved, + outcome: ApprovalOutcome::from(ReviewDecision::Approved), }) .await .unwrap(); @@ -1357,7 +1358,7 @@ async fn handle_container_exec_user_denies_records_tool_decision() { .submit(Op::ExecApproval { id: approval.effective_approval_id(), turn_id: None, - decision: ReviewDecision::Denied, + outcome: ApprovalOutcome::from(ReviewDecision::Denied), }) .await .unwrap(); @@ -1423,7 +1424,7 @@ async fn handle_sandbox_error_user_approves_for_session_records_tool_decision() .submit(Op::ExecApproval { id: approval.effective_approval_id(), turn_id: None, - decision: ReviewDecision::ApprovedForSession, + outcome: ApprovalOutcome::from(ReviewDecision::ApprovedForSession), }) .await .unwrap(); @@ -1490,7 +1491,7 @@ async fn handle_sandbox_error_user_denies_records_tool_decision() { .submit(Op::ExecApproval { id: approval.effective_approval_id(), turn_id: None, - decision: ReviewDecision::Denied, + outcome: ApprovalOutcome::from(ReviewDecision::Denied), }) .await .unwrap(); diff --git a/codex-rs/core/tests/suite/request_permissions.rs b/codex-rs/core/tests/suite/request_permissions.rs index 7c16599bf0..022391ceaf 100644 --- a/codex-rs/core/tests/suite/request_permissions.rs +++ b/codex-rs/core/tests/suite/request_permissions.rs @@ -6,6 +6,7 @@ use codex_core::sandboxing::SandboxPermissions; use codex_features::Feature; use codex_protocol::models::FileSystemPermissions; use codex_protocol::models::PermissionProfile; +use codex_protocol::protocol::ApprovalOutcome; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::ExecApprovalRequestEvent; @@ -378,7 +379,7 @@ async fn with_additional_permissions_requires_approval_under_on_request() -> Res .submit(Op::ExecApproval { id: approval.effective_approval_id(), turn_id: None, - decision: ReviewDecision::Approved, + outcome: ApprovalOutcome::from(ReviewDecision::Approved), }) .await?; wait_for_completion(&test).await; @@ -566,7 +567,7 @@ async fn relative_additional_permissions_resolve_against_tool_workdir() -> Resul .submit(Op::ExecApproval { id: approval.effective_approval_id(), turn_id: None, - decision: ReviewDecision::Approved, + outcome: ApprovalOutcome::from(ReviewDecision::Approved), }) .await?; wait_for_completion(&test).await; @@ -660,7 +661,7 @@ async fn read_only_with_additional_permissions_does_not_widen_to_unrequested_cwd .submit(Op::ExecApproval { id: approval.effective_approval_id(), turn_id: None, - decision: ReviewDecision::Approved, + outcome: ApprovalOutcome::from(ReviewDecision::Approved), }) .await?; wait_for_completion(&test).await; @@ -761,7 +762,7 @@ async fn read_only_with_additional_permissions_does_not_widen_to_unrequested_tmp .submit(Op::ExecApproval { id: approval.effective_approval_id(), turn_id: None, - decision: ReviewDecision::Approved, + outcome: ApprovalOutcome::from(ReviewDecision::Approved), }) .await?; wait_for_completion(&test).await; @@ -869,7 +870,7 @@ async fn workspace_write_with_additional_permissions_can_write_outside_cwd() -> .submit(Op::ExecApproval { id: approval.effective_approval_id(), turn_id: None, - decision: ReviewDecision::Approved, + outcome: ApprovalOutcome::from(ReviewDecision::Approved), }) .await?; wait_for_completion(&test).await; @@ -971,7 +972,7 @@ async fn with_additional_permissions_denied_approval_blocks_execution() -> Resul .submit(Op::ExecApproval { id: approval.effective_approval_id(), turn_id: None, - decision: ReviewDecision::Denied, + outcome: ApprovalOutcome::from(ReviewDecision::Denied), }) .await?; wait_for_completion(&test).await; @@ -1100,7 +1101,7 @@ async fn request_permissions_grants_apply_to_later_exec_command_calls() -> Resul .submit(Op::ExecApproval { id: approval.effective_approval_id(), turn_id: None, - decision: ReviewDecision::Approved, + outcome: ApprovalOutcome::from(ReviewDecision::Approved), }) .await?; wait_for_completion(&test).await; @@ -1210,7 +1211,7 @@ async fn request_permissions_preapprove_explicit_exec_permissions_outside_on_req .submit(Op::ExecApproval { id: approval.effective_approval_id(), turn_id: None, - decision: ReviewDecision::Approved, + outcome: ApprovalOutcome::from(ReviewDecision::Approved), }) .await?; wait_for_completion(&test).await; @@ -1323,7 +1324,7 @@ async fn request_permissions_grants_apply_to_later_shell_command_calls() -> Resu .submit(Op::ExecApproval { id: approval.effective_approval_id(), turn_id: None, - decision: ReviewDecision::Approved, + outcome: ApprovalOutcome::from(ReviewDecision::Approved), }) .await?; wait_for_completion(&test).await; @@ -1432,7 +1433,7 @@ async fn request_permissions_grants_apply_to_later_shell_command_calls_without_i .submit(Op::ExecApproval { id: approval.effective_approval_id(), turn_id: None, - decision: ReviewDecision::Approved, + outcome: ApprovalOutcome::from(ReviewDecision::Approved), }) .await?; wait_for_completion(&test).await; @@ -1601,7 +1602,7 @@ async fn partial_request_permissions_grants_do_not_preapprove_new_permissions() .submit(Op::ExecApproval { id: approval.effective_approval_id(), turn_id: None, - decision: ReviewDecision::Approved, + outcome: ApprovalOutcome::from(ReviewDecision::Approved), }) .await?; wait_for_completion(&test).await; @@ -1846,7 +1847,7 @@ async fn request_permissions_session_grants_carry_across_turns() -> Result<()> { .submit(Op::ExecApproval { id: approval.effective_approval_id(), turn_id: None, - decision: ReviewDecision::Approved, + outcome: ApprovalOutcome::from(ReviewDecision::Approved), }) .await?; wait_for_completion(&test).await; diff --git a/codex-rs/core/tests/suite/request_permissions_tool.rs b/codex-rs/core/tests/suite/request_permissions_tool.rs index 14506f4a41..a1f7066987 100644 --- a/codex-rs/core/tests/suite/request_permissions_tool.rs +++ b/codex-rs/core/tests/suite/request_permissions_tool.rs @@ -5,6 +5,7 @@ use anyhow::Result; use codex_core::config::Constrained; use codex_features::Feature; use codex_protocol::models::FileSystemPermissions; +use codex_protocol::protocol::ApprovalOutcome; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::Op; @@ -277,7 +278,7 @@ async fn approved_folder_write_request_permissions_unblocks_later_exec_without_s .submit(Op::ExecApproval { id: approval.effective_approval_id(), turn_id: None, - decision: ReviewDecision::Approved, + outcome: ApprovalOutcome::from(ReviewDecision::Approved), }) .await?; wait_for_event(&test.codex, |event| { diff --git a/codex-rs/core/tests/suite/skill_approval.rs b/codex-rs/core/tests/suite/skill_approval.rs index 5a50e09cee..38e32c5f89 100644 --- a/codex-rs/core/tests/suite/skill_approval.rs +++ b/codex-rs/core/tests/suite/skill_approval.rs @@ -4,6 +4,7 @@ use anyhow::Result; use codex_protocol::models::FileSystemPermissions; use codex_protocol::models::PermissionProfile; +use codex_protocol::protocol::ApprovalOutcome; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::ExecApprovalRequestEvent; @@ -257,7 +258,7 @@ permissions: .submit(Op::ExecApproval { id: approval.effective_approval_id(), turn_id: None, - decision: ReviewDecision::Denied, + outcome: ApprovalOutcome::from(ReviewDecision::Denied), }) .await?; @@ -351,7 +352,7 @@ permissions: .submit(Op::ExecApproval { id: approval.effective_approval_id(), turn_id: None, - decision: ReviewDecision::Denied, + outcome: ApprovalOutcome::from(ReviewDecision::Denied), }) .await?; @@ -446,7 +447,7 @@ permissions: .submit(Op::ExecApproval { id: approval.effective_approval_id(), turn_id: None, - decision: ReviewDecision::Denied, + outcome: ApprovalOutcome::from(ReviewDecision::Denied), }) .await?; @@ -895,7 +896,7 @@ async fn shell_zsh_fork_skill_session_approval_enforces_skill_permissions() -> R .submit(Op::ExecApproval { id: approval.effective_approval_id(), turn_id: None, - decision: ReviewDecision::ApprovedForSession, + outcome: ApprovalOutcome::from(ReviewDecision::ApprovedForSession), }) .await?; diff --git a/codex-rs/mcp-server/src/exec_approval.rs b/codex-rs/mcp-server/src/exec_approval.rs index 3b5ef87cdb..03099b0196 100644 --- a/codex-rs/mcp-server/src/exec_approval.rs +++ b/codex-rs/mcp-server/src/exec_approval.rs @@ -138,7 +138,7 @@ async fn on_exec_approval_response( .submit(Op::ExecApproval { id: approval_id, turn_id: Some(event_id), - decision: response.decision, + outcome: codex_protocol::protocol::ApprovalOutcome::from(response.decision), }) .await { diff --git a/codex-rs/mcp-server/src/patch_approval.rs b/codex-rs/mcp-server/src/patch_approval.rs index e392694d3a..a27384923e 100644 --- a/codex-rs/mcp-server/src/patch_approval.rs +++ b/codex-rs/mcp-server/src/patch_approval.rs @@ -113,7 +113,9 @@ pub(crate) async fn on_patch_approval_response( if let Err(submit_err) = codex .submit(Op::PatchApproval { id: approval_id.clone(), - decision: ReviewDecision::Denied, + outcome: codex_protocol::protocol::ApprovalOutcome::from( + ReviewDecision::Denied, + ), }) .await { @@ -133,7 +135,7 @@ pub(crate) async fn on_patch_approval_response( if let Err(err) = codex .submit(Op::PatchApproval { id: approval_id, - decision: response.decision, + outcome: codex_protocol::protocol::ApprovalOutcome::from(response.decision), }) .await { diff --git a/codex-rs/otel/src/events/session_telemetry.rs b/codex-rs/otel/src/events/session_telemetry.rs index 42fd6e50c3..fa37a8b5f5 100644 --- a/codex-rs/otel/src/events/session_telemetry.rs +++ b/codex-rs/otel/src/events/session_telemetry.rs @@ -35,8 +35,8 @@ use codex_protocol::ThreadId; use codex_protocol::config_types::ReasoningSummary; use codex_protocol::models::ResponseItem; use codex_protocol::openai_models::ReasoningEffort; +use codex_protocol::protocol::ApprovalOutcome; use codex_protocol::protocol::AskForApproval; -use codex_protocol::protocol::ReviewDecision; use codex_protocol::protocol::SandboxPolicy; use codex_protocol::protocol::SessionSource; use codex_protocol::user_input::UserInput; @@ -865,7 +865,7 @@ impl SessionTelemetry { &self, tool_name: &str, call_id: &str, - decision: &ReviewDecision, + outcome: &ApprovalOutcome, source: ToolDecisionSource, ) { log_event!( @@ -873,7 +873,7 @@ impl SessionTelemetry { event.name = "codex.tool_decision", tool_name = %tool_name, call_id = %call_id, - decision = %decision.clone().to_string().to_lowercase(), + decision = %outcome.to_opaque_string(), source = %source.to_string(), ); } diff --git a/codex-rs/otel/tests/suite/otel_export_routing_policy.rs b/codex-rs/otel/tests/suite/otel_export_routing_policy.rs index b7fce86145..3a31439a5f 100644 --- a/codex-rs/otel/tests/suite/otel_export_routing_policy.rs +++ b/codex-rs/otel/tests/suite/otel_export_routing_policy.rs @@ -2,6 +2,7 @@ use codex_otel::AuthEnvTelemetryMetadata; use codex_otel::OtelProvider; use codex_otel::SessionTelemetry; use codex_otel::TelemetryAuthMode; +use codex_otel::ToolDecisionSource; use opentelemetry::KeyValue; use opentelemetry::logs::AnyValue; use opentelemetry::trace::TracerProvider as _; @@ -20,6 +21,7 @@ use tracing_subscriber::layer::SubscriberExt; use codex_protocol::ThreadId; use codex_protocol::config_types::ReasoningSummary; +use codex_protocol::protocol::ApprovalOutcome; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::SandboxPolicy; use codex_protocol::protocol::SessionSource; @@ -200,6 +202,52 @@ fn otel_export_routing_policy_routes_user_prompt_log_and_trace_events() { assert!(!prompt_trace_attrs.contains_key("user.account_id")); } +#[test] +fn otel_export_routing_policy_logs_tool_timeout_decision_as_timed_out() { + let log_exporter = InMemoryLogExporter::default(); + let logger_provider = SdkLoggerProvider::builder() + .with_simple_exporter(log_exporter.clone()) + .build(); + + let subscriber = tracing_subscriber::registry().with( + opentelemetry_appender_tracing::layer::OpenTelemetryTracingBridge::new(&logger_provider) + .with_filter(filter_fn(OtelProvider::log_export_filter)), + ); + + tracing::subscriber::with_default(subscriber, || { + tracing::callsite::rebuild_interest_cache(); + let manager = SessionTelemetry::new( + ThreadId::new(), + "gpt-5.1", + "gpt-5.1", + Some("account-id".to_string()), + None, + Some(TelemetryAuthMode::ApiKey), + "codex_exec".to_string(), + true, + "tty".to_string(), + SessionSource::Cli, + ); + manager.tool_decision( + "local_shell", + "call-timeout", + &ApprovalOutcome::TimedOut, + ToolDecisionSource::AutomatedReviewer, + ); + }); + + logger_provider.force_flush().expect("flush logs"); + + let logs = log_exporter.get_emitted_logs().expect("log export"); + let tool_log = find_log_by_event_name(&logs, "codex.tool_decision"); + let tool_log_attrs = log_attributes(&tool_log.record); + + assert_eq!( + tool_log_attrs.get("decision").map(String::as_str), + Some("timed_out") + ); +} + #[test] fn otel_export_routing_policy_routes_tool_result_log_and_trace_events() { let log_exporter = InMemoryLogExporter::default(); diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index c40514e3db..a73288398b 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -373,16 +373,16 @@ pub enum Op { /// Turn id associated with the approval event, when available. #[serde(default, skip_serializing_if = "Option::is_none")] turn_id: Option, - /// The user's decision in response to the request. - decision: ReviewDecision, + /// The outcome of resolving the request. + outcome: ApprovalOutcome, }, /// Approve a code patch PatchApproval { /// The id of the submission we are approving id: String, - /// The user's decision in response to the request. - decision: ReviewDecision, + /// The outcome of resolving the request. + outcome: ApprovalOutcome, }, /// Resolve an MCP elicitation request. @@ -3290,6 +3290,45 @@ impl ReviewDecision { } } +/// Result of resolving an approval request. +/// +/// `ReviewDecision` represents the generic approval choices available to a +/// user or policy surface. Some approval flows, such as guardian review, can +/// also end without an intentional rejection. Those cases are represented as a +/// richer outcome here so timeout information can survive hops through session +/// state and delegated subagents. +#[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq, JsonSchema)] +#[serde(tag = "type", rename_all = "snake_case")] +pub enum ApprovalOutcome { + Decision { decision: ReviewDecision }, + TimedOut, +} + +impl ApprovalOutcome { + /// Returns an opaque version of the outcome without PII. We can't use an + /// ignored flag on `serde` because the serialization is required by some + /// surfaces. + pub fn to_opaque_string(&self) -> &'static str { + match self { + Self::Decision { decision } => decision.to_opaque_string(), + Self::TimedOut => "timed_out", + } + } + + pub fn into_decision(self) -> Option { + match self { + Self::Decision { decision } => Some(decision), + Self::TimedOut => None, + } + } +} + +impl From for ApprovalOutcome { + fn from(decision: ReviewDecision) -> Self { + Self::Decision { decision } + } +} + #[derive(Debug, Clone, Deserialize, Serialize, PartialEq, JsonSchema, TS)] #[serde(tag = "type", rename_all = "snake_case")] #[ts(tag = "type")] diff --git a/codex-rs/tui/src/app/pending_interactive_replay.rs b/codex-rs/tui/src/app/pending_interactive_replay.rs index 02efb2ec1b..9b7db2b761 100644 --- a/codex-rs/tui/src/app/pending_interactive_replay.rs +++ b/codex-rs/tui/src/app/pending_interactive_replay.rs @@ -463,7 +463,9 @@ mod tests { store.note_outbound_op(&Op::ExecApproval { id: "approval-1".to_string(), turn_id: Some("turn-1".to_string()), - decision: codex_protocol::protocol::ReviewDecision::Approved, + outcome: codex_protocol::protocol::ApprovalOutcome::from( + codex_protocol::protocol::ReviewDecision::Approved, + ), }); let snapshot = store.snapshot(); @@ -570,7 +572,9 @@ mod tests { store.note_outbound_op(&Op::PatchApproval { id: "call-1".to_string(), - decision: codex_protocol::protocol::ReviewDecision::Approved, + outcome: codex_protocol::protocol::ApprovalOutcome::from( + codex_protocol::protocol::ReviewDecision::Approved, + ), }); let snapshot = store.snapshot(); @@ -699,7 +703,9 @@ mod tests { store.note_outbound_op(&Op::ExecApproval { id: "call-1".to_string(), turn_id: Some("turn-1".to_string()), - decision: codex_protocol::protocol::ReviewDecision::Approved, + outcome: codex_protocol::protocol::ApprovalOutcome::from( + codex_protocol::protocol::ReviewDecision::Approved, + ), }); assert_eq!(store.has_pending_thread_approvals(), false); diff --git a/codex-rs/tui/src/bottom_pane/approval_overlay.rs b/codex-rs/tui/src/bottom_pane/approval_overlay.rs index 1b403c251f..df56979579 100644 --- a/codex-rs/tui/src/bottom_pane/approval_overlay.rs +++ b/codex-rs/tui/src/bottom_pane/approval_overlay.rs @@ -269,7 +269,7 @@ impl ApprovalOverlay { op: Op::ExecApproval { id: id.to_string(), turn_id: None, - decision, + outcome: codex_protocol::protocol::ApprovalOutcome::from(decision), }, }); } @@ -331,7 +331,7 @@ impl ApprovalOverlay { thread_id, op: Op::PatchApproval { id: id.to_string(), - decision, + outcome: codex_protocol::protocol::ApprovalOutcome::from(decision), }, }); } @@ -1093,7 +1093,11 @@ mod tests { let mut saw_op = false; while let Ok(ev) = rx.try_recv() { if let AppEvent::SubmitThreadOp { - op: Op::ExecApproval { decision, .. }, + op: + Op::ExecApproval { + outcome: codex_protocol::protocol::ApprovalOutcome::Decision { decision }, + .. + }, .. } = ev { @@ -1543,7 +1547,11 @@ mod tests { let mut decision = None; while let Ok(ev) = rx.try_recv() { if let AppEvent::SubmitThreadOp { - op: Op::ExecApproval { decision: d, .. }, + op: + Op::ExecApproval { + outcome: codex_protocol::protocol::ApprovalOutcome::Decision { decision: d }, + .. + }, .. } = ev { diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index db0e13e4e5..130c14e7a1 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -3520,7 +3520,12 @@ async fn exec_approval_uses_approval_id_when_present() { let mut found = false; while let Ok(app_ev) = rx.try_recv() { if let AppEvent::SubmitThreadOp { - op: Op::ExecApproval { id, decision, .. }, + op: + Op::ExecApproval { + id, + outcome: codex_protocol::protocol::ApprovalOutcome::Decision { decision }, + .. + }, .. } = app_ev { @@ -10983,7 +10988,11 @@ async fn apply_patch_approval_sends_op_with_call_id() { let mut found = false; while let Ok(app_ev) = rx.try_recv() { if let AppEvent::SubmitThreadOp { - op: Op::PatchApproval { id, decision }, + op: + Op::PatchApproval { + id, + outcome: codex_protocol::protocol::ApprovalOutcome::Decision { decision }, + }, .. } = app_ev { @@ -11034,7 +11043,10 @@ async fn apply_patch_full_flow_integration_like() { .try_recv() .expect("expected op forwarded to codex channel"); match forwarded { - Op::PatchApproval { id, decision } => { + Op::PatchApproval { + id, + outcome: codex_protocol::protocol::ApprovalOutcome::Decision { decision }, + } => { assert_eq!(id, "call-1"); assert_matches!(decision, codex_protocol::protocol::ReviewDecision::Approved); } diff --git a/codex-rs/tui_app_server/src/app/app_server_requests.rs b/codex-rs/tui_app_server/src/app/app_server_requests.rs index 4381e883c0..00f4c982d7 100644 --- a/codex-rs/tui_app_server/src/app/app_server_requests.rs +++ b/codex-rs/tui_app_server/src/app/app_server_requests.rs @@ -119,14 +119,18 @@ impl PendingAppServerRequests { { let op: AppCommand = op.into(); let resolution = match op.view() { - AppCommandView::ExecApproval { id, decision, .. } => self + AppCommandView::ExecApproval { id, outcome, .. } => self .exec_approvals .remove(id) .map(|request_id| { + let decision = outcome.clone().into_decision().ok_or_else(|| { + "cannot serialize timed-out exec approval as an app-server decision" + .to_string() + })?; Ok::(AppServerRequestResolution { request_id, result: serde_json::to_value(CommandExecutionRequestApprovalResponse { - decision: decision.clone().into(), + decision: decision.into(), }) .map_err(|err| { format!("failed to serialize command execution approval response: {err}") @@ -134,14 +138,18 @@ impl PendingAppServerRequests { }) }) .transpose()?, - AppCommandView::PatchApproval { id, decision } => self + AppCommandView::PatchApproval { id, outcome } => self .file_change_approvals .remove(id) .map(|request_id| { + let decision = outcome.clone().into_decision().ok_or_else(|| { + "cannot serialize timed-out patch approval as an app-server decision" + .to_string() + })?; Ok::(AppServerRequestResolution { request_id, result: serde_json::to_value(FileChangeRequestApprovalResponse { - decision: file_change_decision(decision)?, + decision: file_change_decision(&decision)?, }) .map_err(|err| { format!("failed to serialize file change approval response: {err}") @@ -331,7 +339,7 @@ mod tests { .take_resolution(&Op::ExecApproval { id: "approval-1".to_string(), turn_id: None, - decision: ReviewDecision::Approved, + outcome: codex_protocol::protocol::ApprovalOutcome::from(ReviewDecision::Approved), }) .expect("resolution should serialize") .expect("request should be pending"); @@ -537,12 +545,14 @@ mod tests { let error = pending .take_resolution(&Op::PatchApproval { id: "patch-1".to_string(), - decision: ReviewDecision::ApprovedExecpolicyAmendment { - proposed_execpolicy_amendment: ExecPolicyAmendment::new(vec![ - "echo".to_string(), - "hi".to_string(), - ]), - }, + outcome: codex_protocol::protocol::ApprovalOutcome::from( + ReviewDecision::ApprovedExecpolicyAmendment { + proposed_execpolicy_amendment: ExecPolicyAmendment::new(vec![ + "echo".to_string(), + "hi".to_string(), + ]), + }, + ), }) .expect_err("invalid patch decision should fail"); diff --git a/codex-rs/tui_app_server/src/app/pending_interactive_replay.rs b/codex-rs/tui_app_server/src/app/pending_interactive_replay.rs index 67c88d5f90..05dd2c5868 100644 --- a/codex-rs/tui_app_server/src/app/pending_interactive_replay.rs +++ b/codex-rs/tui_app_server/src/app/pending_interactive_replay.rs @@ -760,7 +760,7 @@ mod tests { store.note_outbound_op(&Op::ExecApproval { id: "approval-1".to_string(), turn_id: Some("turn-1".to_string()), - decision: ReviewDecision::Approved, + outcome: codex_protocol::protocol::ApprovalOutcome::from(ReviewDecision::Approved), }); let snapshot = store.snapshot(); @@ -847,7 +847,7 @@ mod tests { store.note_outbound_op(&Op::PatchApproval { id: "call-1".to_string(), - decision: ReviewDecision::Approved, + outcome: codex_protocol::protocol::ApprovalOutcome::from(ReviewDecision::Approved), }); let snapshot = store.snapshot(); @@ -911,7 +911,7 @@ mod tests { store.note_outbound_op(&Op::ExecApproval { id: "call-1".to_string(), turn_id: Some("turn-1".to_string()), - decision: ReviewDecision::Approved, + outcome: codex_protocol::protocol::ApprovalOutcome::from(ReviewDecision::Approved), }); assert_eq!(store.has_pending_thread_approvals(), false); diff --git a/codex-rs/tui_app_server/src/app_command.rs b/codex-rs/tui_app_server/src/app_command.rs index e01a250274..1fdcdfaa61 100644 --- a/codex-rs/tui_app_server/src/app_command.rs +++ b/codex-rs/tui_app_server/src/app_command.rs @@ -9,6 +9,7 @@ use codex_protocol::config_types::ServiceTier; use codex_protocol::config_types::WindowsSandboxLevel; use codex_protocol::mcp::RequestId as McpRequestId; use codex_protocol::openai_models::ReasoningEffort as ReasoningEffortConfig; +use codex_protocol::protocol::ApprovalOutcome; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::ConversationAudioParams; use codex_protocol::protocol::ConversationStartParams; @@ -68,11 +69,11 @@ pub(crate) enum AppCommandView<'a> { ExecApproval { id: &'a str, turn_id: &'a Option, - decision: &'a ReviewDecision, + outcome: &'a ApprovalOutcome, }, PatchApproval { id: &'a str, - decision: &'a ReviewDecision, + outcome: &'a ApprovalOutcome, }, ResolveElicitation { server_name: &'a str, @@ -209,12 +210,15 @@ impl AppCommand { Self(Op::ExecApproval { id, turn_id, - decision, + outcome: ApprovalOutcome::from(decision), }) } pub(crate) fn patch_approval(id: String, decision: ReviewDecision) -> Self { - Self(Op::PatchApproval { id, decision }) + Self(Op::PatchApproval { + id, + outcome: ApprovalOutcome::from(decision), + }) } pub(crate) fn resolve_elicitation( @@ -356,13 +360,13 @@ impl AppCommand { Op::ExecApproval { id, turn_id, - decision, + outcome, } => AppCommandView::ExecApproval { id, turn_id, - decision, + outcome, }, - Op::PatchApproval { id, decision } => AppCommandView::PatchApproval { id, decision }, + Op::PatchApproval { id, outcome } => AppCommandView::PatchApproval { id, outcome }, Op::ResolveElicitation { server_name, request_id, diff --git a/codex-rs/tui_app_server/src/bottom_pane/approval_overlay.rs b/codex-rs/tui_app_server/src/bottom_pane/approval_overlay.rs index f5d1cee621..33827ea99d 100644 --- a/codex-rs/tui_app_server/src/bottom_pane/approval_overlay.rs +++ b/codex-rs/tui_app_server/src/bottom_pane/approval_overlay.rs @@ -1079,7 +1079,11 @@ mod tests { let mut saw_op = false; while let Ok(ev) = rx.try_recv() { if let AppEvent::SubmitThreadOp { - op: Op::ExecApproval { decision, .. }, + op: + Op::ExecApproval { + outcome: codex_protocol::protocol::ApprovalOutcome::Decision { decision }, + .. + }, .. } = ev { @@ -1529,7 +1533,11 @@ mod tests { let mut decision = None; while let Ok(ev) = rx.try_recv() { if let AppEvent::SubmitThreadOp { - op: Op::ExecApproval { decision: d, .. }, + op: + Op::ExecApproval { + outcome: codex_protocol::protocol::ApprovalOutcome::Decision { decision: d }, + .. + }, .. } = ev { diff --git a/codex-rs/tui_app_server/src/chatwidget/tests.rs b/codex-rs/tui_app_server/src/chatwidget/tests.rs index 6e4a60dfa4..f38611de25 100644 --- a/codex-rs/tui_app_server/src/chatwidget/tests.rs +++ b/codex-rs/tui_app_server/src/chatwidget/tests.rs @@ -3529,7 +3529,12 @@ async fn exec_approval_uses_approval_id_when_present() { let mut found = false; while let Ok(app_ev) = rx.try_recv() { if let AppEvent::SubmitThreadOp { - op: Op::ExecApproval { id, decision, .. }, + op: + Op::ExecApproval { + id, + outcome: codex_protocol::protocol::ApprovalOutcome::Decision { decision }, + .. + }, .. } = app_ev { @@ -11647,7 +11652,11 @@ async fn apply_patch_approval_sends_op_with_call_id() { let mut found = false; while let Ok(app_ev) = rx.try_recv() { if let AppEvent::SubmitThreadOp { - op: Op::PatchApproval { id, decision }, + op: + Op::PatchApproval { + id, + outcome: codex_protocol::protocol::ApprovalOutcome::Decision { decision }, + }, .. } = app_ev { @@ -11698,7 +11707,10 @@ async fn apply_patch_full_flow_integration_like() { .try_recv() .expect("expected op forwarded to codex channel"); match forwarded { - Op::PatchApproval { id, decision } => { + Op::PatchApproval { + id, + outcome: codex_protocol::protocol::ApprovalOutcome::Decision { decision }, + } => { assert_eq!(id, "call-1"); assert_matches!(decision, codex_protocol::protocol::ReviewDecision::Approved); }