From ddfa032eb8506eeb21b4f00f12f0836f06e8a6b0 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Tue, 24 Feb 2026 22:27:05 -0800 Subject: [PATCH] fix: chatwidget was not honoring approval_id for an ExecApprovalRequestEvent (#12746) ## Why `ExecApprovalRequestEvent` can carry a distinct `approval_id` for subcommand approvals, including the `execve`-intercepted zsh-fork path. The session registers the pending approval callback under `approval_id` when one is present, but `ChatWidget` was stashing `call_id` in the approval modal state. When the user approved the command in the TUI, the response was sent back with the wrong identifier, so the pending approval could not be matched and the approval callback would not resolve. Note `approval_id` was introduced in https://github.com/openai/codex/pull/12051. ## What changed - In `tui/src/chatwidget.rs`, `ChatWidget` now uses `ExecApprovalRequestEvent::effective_approval_id()` when constructing `ApprovalRequest::Exec`. - That preserves the existing behavior for normal shell and `unified_exec` approvals, where `approval_id` is absent and the effective id still falls back to `call_id`. - For subcommand approvals that provide a distinct `approval_id`, the TUI now sends back the same key that `Session::request_command_approval()` registered. ## Verification - Traced the approval flow end to end to confirm the same effective approval id is now used on both sides of the round trip: - `Session::request_command_approval()` registers the pending callback under `approval_id.unwrap_or(call_id)`. - `ChatWidget` now emits `Op::ExecApproval` with that same effective id. --- .../tui/src/app/pending_interactive_replay.rs | 18 +++++---- codex-rs/tui/src/chatwidget.rs | 2 +- codex-rs/tui/src/chatwidget/tests.rs | 37 +++++++++++++++++++ 3 files changed, 49 insertions(+), 8 deletions(-) diff --git a/codex-rs/tui/src/app/pending_interactive_replay.rs b/codex-rs/tui/src/app/pending_interactive_replay.rs index bc7fed644..14d9f13c7 100644 --- a/codex-rs/tui/src/app/pending_interactive_replay.rs +++ b/codex-rs/tui/src/app/pending_interactive_replay.rs @@ -111,11 +111,12 @@ impl PendingInteractiveReplayState { pub(super) fn note_event(&mut self, event: &Event) { match &event.msg { EventMsg::ExecApprovalRequest(ev) => { - self.exec_approval_call_ids.insert(ev.call_id.clone()); + let approval_id = ev.effective_approval_id(); + self.exec_approval_call_ids.insert(approval_id.clone()); self.exec_approval_call_ids_by_turn_id .entry(ev.turn_id.clone()) .or_default() - .push(ev.call_id.clone()); + .push(approval_id); } EventMsg::ExecCommandBegin(ev) => { self.exec_approval_call_ids.remove(&ev.call_id); @@ -173,11 +174,12 @@ impl PendingInteractiveReplayState { pub(super) fn note_evicted_event(&mut self, event: &Event) { match &event.msg { EventMsg::ExecApprovalRequest(ev) => { - self.exec_approval_call_ids.remove(&ev.call_id); + let approval_id = ev.effective_approval_id(); + self.exec_approval_call_ids.remove(&approval_id); Self::remove_call_id_from_turn_map_entry( &mut self.exec_approval_call_ids_by_turn_id, &ev.turn_id, - &ev.call_id, + &approval_id, ); } EventMsg::ApplyPatchApprovalRequest(ev) => { @@ -218,7 +220,9 @@ impl PendingInteractiveReplayState { pub(super) fn should_replay_snapshot_event(&self, event: &Event) -> bool { match &event.msg { - EventMsg::ExecApprovalRequest(ev) => self.exec_approval_call_ids.contains(&ev.call_id), + EventMsg::ExecApprovalRequest(ev) => self + .exec_approval_call_ids + .contains(&ev.effective_approval_id()), EventMsg::ApplyPatchApprovalRequest(ev) => { self.patch_approval_call_ids.contains(&ev.call_id) } @@ -362,7 +366,7 @@ mod tests { } #[test] - fn thread_event_snapshot_drops_resolved_exec_approval_after_outbound_approval_call_id() { + fn thread_event_snapshot_drops_resolved_exec_approval_after_outbound_approval_id() { let mut store = ThreadEventStore::new(8); store.push_event(Event { id: "ev-1".to_string(), @@ -384,7 +388,7 @@ mod tests { }); store.note_outbound_op(&Op::ExecApproval { - id: "call-1".to_string(), + id: "approval-1".to_string(), turn_id: Some("turn-1".to_string()), decision: codex_protocol::protocol::ReviewDecision::Approved, }); diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 15a6a58d6..9f4148b9d 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -2569,7 +2569,7 @@ impl ChatWidget { self.notify(Notification::ExecApprovalRequested { command }); let request = ApprovalRequest::Exec { - id: ev.call_id, + id: ev.effective_approval_id(), command: ev.command, reason: ev.reason, network_approval_context: ev.network_approval_context, diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index 1e938e2d4..575b860ba 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -2811,6 +2811,43 @@ async fn exec_approval_emits_proposed_command_and_decision_history() { ); } +#[tokio::test] +async fn exec_approval_uses_approval_id_when_present() { + let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(None).await; + + chat.handle_codex_event(Event { + id: "sub-short".into(), + msg: EventMsg::ExecApprovalRequest(ExecApprovalRequestEvent { + call_id: "call-parent".into(), + approval_id: Some("approval-subcommand".into()), + turn_id: "turn-short".into(), + command: vec!["bash".into(), "-lc".into(), "echo hello world".into()], + cwd: std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")), + reason: Some( + "this is a test reason such as one that would be produced by the model".into(), + ), + network_approval_context: None, + proposed_execpolicy_amendment: None, + proposed_network_policy_amendments: None, + additional_permissions: None, + parsed_cmd: vec![], + }), + }); + + chat.handle_key_event(KeyEvent::new(KeyCode::Char('y'), KeyModifiers::NONE)); + + let mut found = false; + while let Ok(app_ev) = rx.try_recv() { + if let AppEvent::CodexOp(Op::ExecApproval { id, decision, .. }) = app_ev { + assert_eq!(id, "approval-subcommand"); + assert_matches!(decision, codex_protocol::protocol::ReviewDecision::Approved); + found = true; + break; + } + } + assert!(found, "expected ExecApproval op to be sent"); +} + #[tokio::test] async fn exec_approval_decision_truncates_multiline_and_long_commands() { let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(None).await;