From 306ff95020e201c7f352540b9722a0d60c5afd09 Mon Sep 17 00:00:00 2001 From: Eason Goodale Date: Sat, 26 Jul 2025 11:48:10 -0700 Subject: [PATCH] fix invisible approvals --- .../src/event_processor_with_human_output.rs | 83 ++++++++++++++++++- codex-rs/tui/src/app_event.rs | 2 +- codex-rs/tui/src/app_event_sender.rs | 6 +- codex-rs/tui/src/lib.rs | 5 ++ codex-rs/tui/src/user_approval_widget.rs | 10 +-- codex-rs/tui/tests/approval_widget.rs | 79 ++++++++++++++++++ 6 files changed, 172 insertions(+), 13 deletions(-) create mode 100644 codex-rs/tui/tests/approval_widget.rs diff --git a/codex-rs/exec/src/event_processor_with_human_output.rs b/codex-rs/exec/src/event_processor_with_human_output.rs index bc647c683e..c878f4b63a 100644 --- a/codex-rs/exec/src/event_processor_with_human_output.rs +++ b/codex-rs/exec/src/event_processor_with_human_output.rs @@ -3,10 +3,12 @@ use codex_core::config::Config; use codex_core::protocol::AgentMessageDeltaEvent; use codex_core::protocol::AgentMessageEvent; use codex_core::protocol::AgentReasoningDeltaEvent; +use codex_core::protocol::ApplyPatchApprovalRequestEvent; use codex_core::protocol::BackgroundEventEvent; use codex_core::protocol::ErrorEvent; use codex_core::protocol::Event; use codex_core::protocol::EventMsg; +use codex_core::protocol::ExecApprovalRequestEvent; use codex_core::protocol::ExecCommandBeginEvent; use codex_core::protocol::ExecCommandEndEvent; use codex_core::protocol::FileChange; @@ -474,11 +476,45 @@ impl EventProcessor for EventProcessorWithHumanOutput { println!("{}", line.style(self.dimmed)); } } - EventMsg::ExecApprovalRequest(_) => { - // Should we exit? + EventMsg::ExecApprovalRequest(ExecApprovalRequestEvent { + command, + cwd, + reason, + .. + }) => { + ts_println!( + self, + "{} {} in {}", + "approval required for".style(self.magenta), + escape_command(&command).style(self.bold), + cwd.to_string_lossy(), + ); + if let Some(r) = reason { + ts_println!(self, "{r}"); + } + return CodexStatus::InitiateShutdown; } - EventMsg::ApplyPatchApprovalRequest(_) => { - // Should we exit? + EventMsg::ApplyPatchApprovalRequest(ApplyPatchApprovalRequestEvent { + changes, + reason, + .. + }) => { + ts_println!( + self, + "{}:", + "approval required for apply_patch".style(self.magenta), + ); + for (path, change) in changes.iter() { + println!( + " {} {}", + format_file_change(change).style(self.cyan), + path.to_string_lossy(), + ); + } + if let Some(r) = reason { + ts_println!(self, "{r}"); + } + return CodexStatus::InitiateShutdown; } EventMsg::AgentReasoning(agent_reasoning_event) => { if self.show_agent_reasoning { @@ -538,3 +574,42 @@ fn format_file_change(change: &FileChange) -> &'static str { } => "M", } } + +#[cfg(test)] +mod tests { + use super::*; + use codex_core::config::Config; + use codex_core::config::ConfigOverrides; + use codex_core::config::ConfigToml; + use codex_core::protocol::Event; + use codex_core::protocol::EventMsg; + use codex_core::protocol::ExecApprovalRequestEvent; + + fn test_config() -> Config { + Config::load_from_base_config_with_overrides( + ConfigToml::default(), + ConfigOverrides::default(), + std::env::temp_dir(), + ) + .unwrap() + } + + #[test] + fn exec_approval_request_displays_command() { + let config = test_config(); + let mut processor = EventProcessorWithHumanOutput::create_with_ansi(false, &config, None); + + let event = Event { + id: "1".into(), + msg: EventMsg::ExecApprovalRequest(ExecApprovalRequestEvent { + call_id: "c1".into(), + command: vec!["rm".into(), "-rf".into(), "/".into()], + cwd: PathBuf::from("/tmp"), + reason: None, + }), + }; + + let status = processor.process_event(event); + assert!(matches!(status, CodexStatus::InitiateShutdown)); + } +} diff --git a/codex-rs/tui/src/app_event.rs b/codex-rs/tui/src/app_event.rs index a1f304fe42..c144c30918 100644 --- a/codex-rs/tui/src/app_event.rs +++ b/codex-rs/tui/src/app_event.rs @@ -6,7 +6,7 @@ use ratatui::text::Line; use crate::slash_command::SlashCommand; #[allow(clippy::large_enum_variant)] -pub(crate) enum AppEvent { +pub enum AppEvent { CodexEvent(Event), /// Request a redraw which will be debounced by the [`App`]. diff --git a/codex-rs/tui/src/app_event_sender.rs b/codex-rs/tui/src/app_event_sender.rs index 9d838273ef..9752a3c415 100644 --- a/codex-rs/tui/src/app_event_sender.rs +++ b/codex-rs/tui/src/app_event_sender.rs @@ -3,18 +3,18 @@ use std::sync::mpsc::Sender; use crate::app_event::AppEvent; #[derive(Clone, Debug)] -pub(crate) struct AppEventSender { +pub struct AppEventSender { app_event_tx: Sender, } impl AppEventSender { - pub(crate) fn new(app_event_tx: Sender) -> Self { + pub fn new(app_event_tx: Sender) -> Self { Self { app_event_tx } } /// Send an event to the app event channel. If it fails, we swallow the /// error and log it. - pub(crate) fn send(&self, event: AppEvent) { + pub fn send(&self, event: AppEvent) { if let Err(e) = self.app_event_tx.send(event) { tracing::error!("failed to send event: {e}"); } diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index 905f0aaf0b..f35e1f5d34 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -45,6 +45,11 @@ mod text_formatting; mod tui; mod user_approval_widget; +// Re-export a small subset of internal types used by integration tests. +pub use app_event::AppEvent; +pub use app_event_sender::AppEventSender; +pub use user_approval_widget::{ApprovalRequest, UserApprovalWidget}; + pub use cli::Cli; pub fn run_main( diff --git a/codex-rs/tui/src/user_approval_widget.rs b/codex-rs/tui/src/user_approval_widget.rs index 431f85a268..7883228680 100644 --- a/codex-rs/tui/src/user_approval_widget.rs +++ b/codex-rs/tui/src/user_approval_widget.rs @@ -33,7 +33,7 @@ use crate::exec_command::relativize_to_home; use crate::exec_command::strip_bash_lc_and_escape; /// Request coming from the agent that needs user approval. -pub(crate) enum ApprovalRequest { +pub enum ApprovalRequest { Exec { id: String, command: Vec, @@ -97,7 +97,7 @@ enum Mode { } /// A modal prompting the user to approve or deny the pending request. -pub(crate) struct UserApprovalWidget<'a> { +pub struct UserApprovalWidget<'a> { approval_request: ApprovalRequest, app_event_tx: AppEventSender, confirmation_prompt: Paragraph<'a>, @@ -117,7 +117,7 @@ pub(crate) struct UserApprovalWidget<'a> { } impl UserApprovalWidget<'_> { - pub(crate) fn new(approval_request: ApprovalRequest, app_event_tx: AppEventSender) -> Self { + pub fn new(approval_request: ApprovalRequest, app_event_tx: AppEventSender) -> Self { let input = Input::default(); let confirmation_prompt = match &approval_request { ApprovalRequest::Exec { @@ -196,7 +196,7 @@ impl UserApprovalWidget<'_> { /// Process a key event originating from crossterm. As the modal fully /// captures input while visible, we don’t need to report whether the event /// was consumed—callers can assume it always is. - pub(crate) fn handle_key_event(&mut self, key: KeyEvent) { + pub fn handle_key_event(&mut self, key: KeyEvent) { match self.mode { Mode::Select => self.handle_select_key(key), Mode::Input => self.handle_input_key(key), @@ -289,7 +289,7 @@ impl UserApprovalWidget<'_> { /// Returns `true` once the user has made a decision and the widget no /// longer needs to be displayed. - pub(crate) fn is_complete(&self) -> bool { + pub fn is_complete(&self) -> bool { self.done } } diff --git a/codex-rs/tui/tests/approval_widget.rs b/codex-rs/tui/tests/approval_widget.rs new file mode 100644 index 0000000000..45a65a5444 --- /dev/null +++ b/codex-rs/tui/tests/approval_widget.rs @@ -0,0 +1,79 @@ +use std::path::PathBuf; +use std::sync::mpsc::channel; + +use codex_core::protocol::ReviewDecision; // keep this import to ensure the enum is linked +use ratatui::buffer::Buffer; +use ratatui::layout::Rect; +use ratatui::widgets::WidgetRef; + +use codex_tui::{AppEvent, AppEventSender, ApprovalRequest, UserApprovalWidget}; + +/// Regression test: ensure that the exec command line (cwd + command) is always +/// visible when rendering the approval modal, even when the available viewport +/// is very small and the confirmation prompt needs to be truncated. +#[test] +fn exec_command_is_visible_in_small_viewport() { + // Construct a long reason to inflate the confirmation prompt, so we can + // verify that it gets truncated rather than pushing the command or the + // response options out of view. + let long_reason = "This is a very long explanatory reason that would normally occupy many lines in the confirmation prompt. \ +It should not cause the actual command or the response options to be scrolled out of the visible area.".to_string(); + + // Minimal AppEventSender – the widget will send an event when a decision is + // made, but we will not interact with it in this rendering test. + let (tx, _rx) = channel::(); + let app_tx = AppEventSender::new(tx); + + let cwd = PathBuf::from("/home/alice/project"); + let command = vec![ + "bash".to_string(), + "-lc".to_string(), + "echo 123 && printf 'hello'".to_string(), + ]; + + let widget = UserApprovalWidget::new( + ApprovalRequest::Exec { + id: "test-id".to_string(), + command: command.clone(), + cwd: cwd.clone(), + reason: Some(long_reason), + }, + app_tx, + ); + + // Render into a deliberately small area to force truncation of the prompt. + // The outer border consumes one row/column on each side, leaving very + // little space inside – this is fine, we just want to see that the command + // line is still present in the buffer. + let area = Rect::new(0, 0, 50, 12); + let mut buf = Buffer::empty(area); + (&widget).render_ref(area, &mut buf); + + // Extract the contents of the buffer into a single string for searching. + let mut rendered = String::new(); + for y in 0..area.height { + for x in 0..area.width { + let cell = buf.get(x, y); + rendered.push(cell.symbol().chars().next().unwrap_or('\0')); + } + rendered.push('\n'); + } + + // The command is displayed after stripping the leading "bash -lc" wrapper + // and shell-escaping. For our input this should result in the literal + // command text below. The cwd is printed as a prefix ending with a '$', but + // to keep the assertion robust we only verify that the command itself is + // present in the rendered buffer. + assert!( + rendered.contains("echo 123 && printf 'hello'"), + "rendered buffer did not contain the command.\n--- buffer ---\n{rendered}\n----------------" + ); + + // Additionally assert that at least one of the response options is visible + // to ensure the interactive section has not been pushed out of view. + assert!(rendered.contains("Yes (y)")); + + // Silence unused import warning for ReviewDecision (ensures the crate is + // linked with the protocol module and avoids accidental dead-code removal). + let _ = ReviewDecision::Approved; +}