Fix approval workflow (#1696)

(Hopefully) temporary solution to the invisible approvals problem -
prints commands to history when they need approval and then also prints
the result of the approval. In the near future we should be able to do
some fancy stuff with updating commands before writing them to permanent
history.

Also, ctr-c while in the approval modal now acts as esc (aborts command)
and puts the TUI in the state where one additional ctr-c will exit.

codex-rs/tui/src/bottom_pane/approval_modal_view.rs

@@ -9,6 +9,7 @@ use crate::user_approval_widget::UserApprovalWidget;
 
 use super::BottomPane;
 use super::BottomPaneView;
+use super::CancellationEvent;
 
 /// Modal overlay asking the user to approve/deny a sequence of requests.
 pub(crate) struct ApprovalModalView<'a> {
@@ -46,6 +47,12 @@ impl<'a> BottomPaneView<'a> for ApprovalModalView<'a> {
         self.maybe_advance();
     }
 
+    fn on_ctrl_c(&mut self, _pane: &mut BottomPane<'a>) -> CancellationEvent {
+        self.current.on_ctrl_c();
+        self.queue.clear();
+        CancellationEvent::Handled
+    }
+
     fn is_complete(&self) -> bool {
         self.current.is_complete() && self.queue.is_empty()
     }
@@ -59,3 +66,39 @@ impl<'a> BottomPaneView<'a> for ApprovalModalView<'a> {
         None
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::app_event::AppEvent;
+    use std::path::PathBuf;
+    use std::sync::mpsc::channel;
+
+    fn make_exec_request() -> ApprovalRequest {
+        ApprovalRequest::Exec {
+            id: "test".to_string(),
+            command: vec!["echo".to_string(), "hi".to_string()],
+            cwd: PathBuf::from("/tmp"),
+            reason: None,
+        }
+    }
+
+    #[test]
+    fn ctrl_c_aborts_and_clears_queue() {
+        let (tx_raw, _rx) = channel::<AppEvent>();
+        let tx = AppEventSender::new(tx_raw);
+        let first = make_exec_request();
+        let mut view = ApprovalModalView::new(first, tx);
+        view.enqueue_request(make_exec_request());
+
+        let (tx_raw2, _rx2) = channel::<AppEvent>();
+        let mut pane = BottomPane::new(super::super::BottomPaneParams {
+            app_event_tx: AppEventSender::new(tx_raw2),
+            has_input_focus: true,
+        });
+        assert_eq!(CancellationEvent::Handled, view.on_ctrl_c(&mut pane));
+        assert!(view.queue.is_empty());
+        assert!(view.current.is_complete());
+        assert!(view.is_complete());
+    }
+}

codex-rs/tui/src/bottom_pane/bottom_pane_view.rs

@@ -4,6 +4,7 @@ use ratatui::buffer::Buffer;
 use ratatui::layout::Rect;
 
 use super::BottomPane;
+use super::CancellationEvent;
 
 /// Type to use for a method that may require a redraw of the UI.
 pub(crate) enum ConditionalUpdate {
@@ -22,6 +23,11 @@ pub(crate) trait BottomPaneView<'a> {
         false
     }
 
+    /// Handle Ctrl-C while this view is active.
+    fn on_ctrl_c(&mut self, _pane: &mut BottomPane<'a>) -> CancellationEvent {
+        CancellationEvent::Ignored
+    }
+
     /// Render the view: this will be displayed in place of the composer.
     fn render(&self, area: Rect, buf: &mut Buffer);
 

codex-rs/tui/src/bottom_pane/mod.rs

@@ -20,6 +20,12 @@ mod command_popup;
 mod file_search_popup;
 mod status_indicator_view;
 
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub(crate) enum CancellationEvent {
+    Ignored,
+    Handled,
+}
+
 pub(crate) use chat_composer::ChatComposer;
 pub(crate) use chat_composer::InputResult;
 
@@ -80,6 +86,33 @@ impl BottomPane<'_> {
         }
     }
 
+    /// Handle Ctrl-C in the bottom pane. If a modal view is active it gets a
+    /// chance to consume the event (e.g. to dismiss itself).
+    pub(crate) fn on_ctrl_c(&mut self) -> CancellationEvent {
+        let mut view = match self.active_view.take() {
+            Some(view) => view,
+            None => return CancellationEvent::Ignored,
+        };
+
+        let event = view.on_ctrl_c(self);
+        match event {
+            CancellationEvent::Handled => {
+                if !view.is_complete() {
+                    self.active_view = Some(view);
+                } else if self.is_task_running {
+                    self.active_view = Some(Box::new(StatusIndicatorView::new(
+                        self.app_event_tx.clone(),
+                    )));
+                }
+                self.show_ctrl_c_quit_hint();
+            }
+            CancellationEvent::Ignored => {
+                self.active_view = Some(view);
+            }
+        }
+        event
+    }
+
     pub fn handle_paste(&mut self, pasted: String) {
         if self.active_view.is_none() {
             let needs_redraw = self.composer.handle_paste(pasted);
@@ -234,3 +267,34 @@ impl WidgetRef for &BottomPane<'_> {
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::app_event::AppEvent;
+    use std::path::PathBuf;
+    use std::sync::mpsc::channel;
+
+    fn exec_request() -> ApprovalRequest {
+        ApprovalRequest::Exec {
+            id: "1".to_string(),
+            command: vec!["echo".into(), "ok".into()],
+            cwd: PathBuf::from("."),
+            reason: None,
+        }
+    }
+
+    #[test]
+    fn ctrl_c_on_modal_consumes_and_shows_quit_hint() {
+        let (tx_raw, _rx) = channel::<AppEvent>();
+        let tx = AppEventSender::new(tx_raw);
+        let mut pane = BottomPane::new(BottomPaneParams {
+            app_event_tx: tx,
+            has_input_focus: true,
+        });
+        pane.push_approval_request(exec_request());
+        assert_eq!(CancellationEvent::Handled, pane.on_ctrl_c());
+        assert!(pane.ctrl_c_quit_hint_visible());
+        assert_eq!(CancellationEvent::Ignored, pane.on_ctrl_c());
+    }
+}

codex-rs/tui/src/chatwidget.rs

@@ -34,8 +34,10 @@ use crate::app_event::AppEvent;
 use crate::app_event_sender::AppEventSender;
 use crate::bottom_pane::BottomPane;
 use crate::bottom_pane::BottomPaneParams;
+use crate::bottom_pane::CancellationEvent;
 use crate::bottom_pane::InputResult;
 use crate::conversation_history_widget::ConversationHistoryWidget;
+use crate::exec_command::strip_bash_lc_and_escape;
 use crate::history_cell::PatchEventType;
 use crate::user_approval_widget::ApprovalRequest;
 use codex_file_search::FileMatch;
@@ -301,6 +303,20 @@ impl ChatWidget<'_> {
                 cwd,
                 reason,
             }) => {
+                // Print the command to the history so it is visible in the
+                // transcript *before* the modal asks for approval.
+                let cmdline = strip_bash_lc_and_escape(&command);
+                let text = format!(
+                    "command requires approval:\n$ {cmdline}{reason}",
+                    reason = reason
+                        .as_ref()
+                        .map(|r| format!("\n{r}"))
+                        .unwrap_or_default()
+                );
+                self.conversation_history.add_background_event(text);
+                self.emit_last_history_entry();
+                self.conversation_history.scroll_to_bottom();
+
                 let request = ApprovalRequest::Exec {
                     id,
                     command,
@@ -308,6 +324,7 @@ impl ChatWidget<'_> {
                     reason,
                 };
                 self.bottom_pane.push_approval_request(request);
+                self.request_redraw();
             }
             EventMsg::ApplyPatchApprovalRequest(ApplyPatchApprovalRequestEvent {
                 call_id: _,
@@ -453,21 +470,25 @@ impl ChatWidget<'_> {
     }
 
     /// Handle Ctrl-C key press.
-    /// Returns true if the key press was handled, false if it was not.
-    /// If the key press was not handled, the caller should handle it (likely by exiting the process).
-    pub(crate) fn on_ctrl_c(&mut self) -> bool {
+    /// Returns CancellationEvent::Handled if the event was consumed by the UI, or
+    /// CancellationEvent::Ignored if the caller should handle it (e.g. exit).
+    pub(crate) fn on_ctrl_c(&mut self) -> CancellationEvent {
+        match self.bottom_pane.on_ctrl_c() {
+            CancellationEvent::Handled => return CancellationEvent::Handled,
+            CancellationEvent::Ignored => {}
+        }
         if self.bottom_pane.is_task_running() {
             self.bottom_pane.clear_ctrl_c_quit_hint();
             self.submit_op(Op::Interrupt);
             self.answer_buffer.clear();
             self.reasoning_buffer.clear();
-            false
+            CancellationEvent::Ignored
         } else if self.bottom_pane.ctrl_c_quit_hint_visible() {
             self.submit_op(Op::Shutdown);
-            true
+            CancellationEvent::Handled
         } else {
             self.bottom_pane.show_ctrl_c_quit_hint();
-            false
+            CancellationEvent::Ignored
         }
     }
 

codex-rs/tui/src/user_approval_widget.rs

@@ -203,6 +203,12 @@ impl UserApprovalWidget<'_> {
         }
     }
 
+    /// Handle Ctrl-C pressed by the user while the modal is visible.
+    /// Behaves like pressing Escape: abort the request and close the modal.
+    pub(crate) fn on_ctrl_c(&mut self) {
+        self.send_decision(ReviewDecision::Abort);
+    }
+
     fn handle_select_key(&mut self, key_event: KeyEvent) {
         match key_event.code {
             KeyCode::Up => {
@@ -265,7 +271,28 @@ impl UserApprovalWidget<'_> {
         self.send_decision_with_feedback(decision, String::new())
     }
 
-    fn send_decision_with_feedback(&mut self, decision: ReviewDecision, _feedback: String) {
+    fn send_decision_with_feedback(&mut self, decision: ReviewDecision, feedback: String) {
+        let mut lines: Vec<Line<'static>> = Vec::new();
+        match &self.approval_request {
+            ApprovalRequest::Exec { command, .. } => {
+                let cmd = strip_bash_lc_and_escape(command);
+                lines.push(Line::from("approval decision"));
+                lines.push(Line::from(format!("$ {cmd}")));
+                lines.push(Line::from(format!("decision: {decision:?}")));
+            }
+            ApprovalRequest::ApplyPatch { .. } => {
+                lines.push(Line::from(format!("patch approval decision: {decision:?}")));
+            }
+        }
+        if !feedback.trim().is_empty() {
+            lines.push(Line::from("feedback:"));
+            for l in feedback.lines() {
+                lines.push(Line::from(l.to_string()));
+            }
+        }
+        lines.push(Line::from(""));
+        self.app_event_tx.send(AppEvent::InsertHistory(lines));
+
         let op = match &self.approval_request {
             ApprovalRequest::Exec { id, .. } => Op::ExecApproval {
                 id: id.clone(),
@@ -277,12 +304,6 @@ impl UserApprovalWidget<'_> {
             },
         };
 
-        // Ignore feedback for now – the current `Op` variants do not carry it.
-
-        // Forward the Op to the agent. The caller (ChatWidget) will trigger a
-        // redraw after it processes the resulting state change, so we avoid
-        // issuing an extra Redraw here to prevent a transient frame where the
-        // modal is still visible.
         self.app_event_tx.send(AppEvent::CodexOp(op));
         self.done = true;
     }