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.
This commit is contained in:
parent
6cb2f02ef8
commit
ddfa032eb8
3 changed files with 49 additions and 8 deletions
|
|
@ -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,
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue