From 742feaf40f5fd03796afcf17f8f500c0706cc78c Mon Sep 17 00:00:00 2001 From: Jeremy Rose <172423086+nornagon-openai@users.noreply.github.com> Date: Thu, 4 Sep 2025 16:54:53 -0700 Subject: [PATCH] tui: fix approval dialog for large commands (#3087) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #### Summary - Emit a “Proposed Command” history cell when an ExecApprovalRequest arrives (parity with proposed patches). - Simplify the approval dialog: show only the reason/instructions; move the command preview into history. - Make approval/abort decision history concise: - Single line snippet; if multiline, show first line + " ...". - Truncate to 80 graphemes with ellipsis for very long commands. #### Details - History - Add `new_proposed_command` to render a header and indented command preview. - Use shared `prefix_lines` helper for first/subsequent line prefixes. - Approval UI - `UserApprovalWidget` no longer renders the command in the modal; shows optional `reason` text only. - Decision history renders an inline, dimmed snippet per rules above. - Tests (snapshot-based) - Proposed/decision flow for short command. - Proposed multi-line + aborted decision snippet with “ ...”. - Very long one-line command -> truncated snippet with “…”. - Updated existing exec approval snapshots and test reasons. Screenshot 2025-09-03 at 11 57 35 AM after approving: Screenshot 2025-09-03 at 11 58 18 AM rejection: Screenshot 2025-09-03 at 11 58 45 AM big command: https://github.com/user-attachments/assets/2dd976e5-799f-4af7-9682-a046e66cc494 --- codex-rs/tui/src/chatwidget.rs | 2 + ...hatwidget__tests__approval_modal_exec.snap | 4 +- ..._tests__approval_modal_exec_no_reason.snap | 2 - ...pproval_history_decision_aborted_long.snap | 5 + ...al_history_decision_aborted_multiline.snap | 6 + ...roval_history_decision_approved_short.snap | 6 + ...c_approval_history_proposed_multiline.snap | 7 + ..._exec_approval_history_proposed_short.snap | 6 + ...sts__status_widget_and_approval_modal.snap | 4 +- codex-rs/tui/src/chatwidget/tests.rs | 106 ++++++++++++++- codex-rs/tui/src/history_cell.rs | 49 ++++--- codex-rs/tui/src/render/line_utils.rs | 23 ++++ codex-rs/tui/src/user_approval_widget.rs | 126 +++++++----------- 13 files changed, 230 insertions(+), 116 deletions(-) create mode 100644 codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_decision_aborted_long.snap create mode 100644 codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_decision_aborted_multiline.snap create mode 100644 codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_decision_approved_short.snap create mode 100644 codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_proposed_multiline.snap create mode 100644 codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_proposed_short.snap diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index e8381c6f..7acb802c 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -524,6 +524,8 @@ impl ChatWidget { pub(crate) fn handle_exec_approval_now(&mut self, id: String, ev: ExecApprovalRequestEvent) { self.flush_answer_stream_with_separator(); + // Emit the proposed command into history (like proposed patches) + self.add_to_history(history_cell::new_proposed_command(&ev.command)); let request = ApprovalRequest::Exec { id, diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_exec.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_exec.snap index e190ef4d..f306381f 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_exec.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_exec.snap @@ -4,9 +4,7 @@ assertion_line: 728 expression: terminal.backend() --- " " -"? Codex wants to run echo hello world " -" " -"Model wants to run a command " +"this is a test reason such as one that would be produced by the model " " " "▌Allow command? " "▌ Yes Always No, provide feedback " diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_exec_no_reason.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_exec_no_reason.snap index f8aee639..fb1ecce7 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_exec_no_reason.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_exec_no_reason.snap @@ -3,8 +3,6 @@ source: tui/src/chatwidget/tests.rs expression: terminal.backend() --- " " -"? Codex wants to run echo hello world " -" " "▌Allow command? " "▌ Yes Always No, provide feedback " "▌ Approve and run the command " diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_decision_aborted_long.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_decision_aborted_long.snap new file mode 100644 index 00000000..524e0968 --- /dev/null +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_decision_aborted_long.snap @@ -0,0 +1,5 @@ +--- +source: tui/src/chatwidget/tests.rs +expression: lines_to_single_string(&aborted_long) +--- +✗ You canceled the request to run echo aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa... diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_decision_aborted_multiline.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_decision_aborted_multiline.snap new file mode 100644 index 00000000..d35cb175 --- /dev/null +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_decision_aborted_multiline.snap @@ -0,0 +1,6 @@ +--- +source: tui/src/chatwidget/tests.rs +expression: lines_to_single_string(&aborted_multi) +--- +✗ You canceled the request to run echo line1 ... + diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_decision_approved_short.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_decision_approved_short.snap new file mode 100644 index 00000000..1b18a23d --- /dev/null +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_decision_approved_short.snap @@ -0,0 +1,6 @@ +--- +source: tui/src/chatwidget/tests.rs +expression: lines_to_single_string(&decision) +--- +✔ You approved codex to run echo hello world this time + diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_proposed_multiline.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_proposed_multiline.snap new file mode 100644 index 00000000..119965df --- /dev/null +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_proposed_multiline.snap @@ -0,0 +1,7 @@ +--- +source: tui/src/chatwidget/tests.rs +expression: lines_to_single_string(&proposed_multi) +--- +• Proposed Command + └ echo line1 + echo line2 diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_proposed_short.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_proposed_short.snap new file mode 100644 index 00000000..dfc52c01 --- /dev/null +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_proposed_short.snap @@ -0,0 +1,6 @@ +--- +source: tui/src/chatwidget/tests.rs +expression: lines_to_single_string(&proposed) +--- +• Proposed Command + └ echo hello world diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__status_widget_and_approval_modal.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__status_widget_and_approval_modal.snap index 645ddb8e..63fb213e 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__status_widget_and_approval_modal.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__status_widget_and_approval_modal.snap @@ -4,9 +4,7 @@ assertion_line: 921 expression: terminal.backend() --- " " -"? Codex wants to run echo 'hello world' " -" " -"Codex wants to run a command " +"this is a test reason such as one that would be produced by the model " " " "▌Allow command? " "▌ Yes Always No, provide feedback " diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index 287ce706..555ce50d 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -265,6 +265,104 @@ fn lines_to_single_string(lines: &[ratatui::text::Line<'static>]) -> String { s } +// (removed experimental resize snapshot test) + +#[test] +fn exec_approval_emits_proposed_command_and_decision_history() { + let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(); + + // Trigger an exec approval request with a short, single-line command + let ev = ExecApprovalRequestEvent { + call_id: "call-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(), + ), + }; + chat.handle_codex_event(Event { + id: "sub-short".into(), + msg: EventMsg::ExecApprovalRequest(ev), + }); + + // Snapshot the Proposed Command cell emitted into history + let proposed = drain_insert_history(&mut rx) + .pop() + .expect("expected proposed command cell"); + assert_snapshot!( + "exec_approval_history_proposed_short", + lines_to_single_string(&proposed) + ); + + // Approve via keyboard and verify a concise decision history line is added + chat.handle_key_event(KeyEvent::new(KeyCode::Char('y'), KeyModifiers::NONE)); + let decision = drain_insert_history(&mut rx) + .pop() + .expect("expected decision cell in history"); + assert_snapshot!( + "exec_approval_history_decision_approved_short", + lines_to_single_string(&decision) + ); +} + +#[test] +fn exec_approval_decision_truncates_multiline_and_long_commands() { + let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(); + + // Multiline command: should render proposed command fully in history with prefixes + let ev_multi = ExecApprovalRequestEvent { + call_id: "call-multi".into(), + command: vec!["bash".into(), "-lc".into(), "echo line1\necho line2".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(), + ), + }; + chat.handle_codex_event(Event { + id: "sub-multi".into(), + msg: EventMsg::ExecApprovalRequest(ev_multi), + }); + let proposed_multi = drain_insert_history(&mut rx) + .pop() + .expect("expected proposed multiline command cell"); + assert_snapshot!( + "exec_approval_history_proposed_multiline", + lines_to_single_string(&proposed_multi) + ); + + // Deny via keyboard; decision snippet should be single-line and elided with " ..." + chat.handle_key_event(KeyEvent::new(KeyCode::Char('n'), KeyModifiers::NONE)); + let aborted_multi = drain_insert_history(&mut rx) + .pop() + .expect("expected aborted decision cell (multiline)"); + assert_snapshot!( + "exec_approval_history_decision_aborted_multiline", + lines_to_single_string(&aborted_multi) + ); + + // Very long single-line command: decision snippet should be truncated <= 80 chars with trailing ... + let long = format!("echo {}", "a".repeat(200)); + let ev_long = ExecApprovalRequestEvent { + call_id: "call-long".into(), + command: vec!["bash".into(), "-lc".into(), long.clone()], + cwd: std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")), + reason: None, + }; + chat.handle_codex_event(Event { + id: "sub-long".into(), + msg: EventMsg::ExecApprovalRequest(ev_long), + }); + drain_insert_history(&mut rx); // proposed cell not needed for this assertion + chat.handle_key_event(KeyEvent::new(KeyCode::Char('n'), KeyModifiers::NONE)); + let aborted_long = drain_insert_history(&mut rx) + .pop() + .expect("expected aborted decision cell (long)"); + assert_snapshot!( + "exec_approval_history_decision_aborted_long", + lines_to_single_string(&aborted_long) + ); +} + // --- Small helpers to tersely drive exec begin/end and snapshot active cell --- fn begin_exec(chat: &mut ChatWidget, call_id: &str, raw_cmd: &str) { // Build the full command vec and parse it using core's parser, @@ -714,7 +812,9 @@ fn approval_modal_exec_snapshot() { call_id: "call-approve-cmd".into(), command: vec!["bash".into(), "-lc".into(), "echo hello world".into()], cwd: std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")), - reason: Some("Model wants to run a command".into()), + reason: Some( + "this is a test reason such as one that would be produced by the model".into(), + ), }; chat.handle_codex_event(Event { id: "sub-approve".into(), @@ -907,7 +1007,9 @@ fn status_widget_and_approval_modal_snapshot() { call_id: "call-approve-exec".into(), command: vec!["echo".into(), "hello world".into()], cwd: std::path::PathBuf::from("/tmp"), - reason: Some("Codex wants to run a command".into()), + reason: Some( + "this is a test reason such as one that would be produced by the model".into(), + ), }; chat.handle_codex_event(Event { id: "sub-approve-exec".into(), diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index 798a87dc..ce048f48 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -2,6 +2,7 @@ use crate::diff_render::create_diff_summary; use crate::exec_command::relativize_to_home; use crate::exec_command::strip_bash_lc_and_escape; use crate::markdown::append_markdown; +use crate::render::line_utils::prefix_lines; use crate::slash_command::SlashCommand; use crate::text_formatting::format_and_truncate_tool_result; use base64::Engine; @@ -1140,33 +1141,9 @@ impl HistoryCell for PlanUpdateCell { .into_iter() .map(|s| s.to_string().set_style(step_style).into()) .collect(); - prefix_lines(step_text, &box_str.into(), &" ".into()) + prefix_lines(step_text, box_str.into(), " ".into()) }; - fn prefix_lines( - lines: Vec>, - initial_prefix: &Span<'static>, - subsequent_prefix: &Span<'static>, - ) -> Vec> { - lines - .into_iter() - .enumerate() - .map(|(i, l)| { - Line::from( - [ - vec![if i == 0 { - initial_prefix.clone() - } else { - subsequent_prefix.clone() - }], - l.spans, - ] - .concat(), - ) - }) - .collect() - } - let mut lines: Vec> = vec![]; lines.push(vec!["• ".into(), "Updated Plan".bold()].into()); @@ -1187,7 +1164,7 @@ impl HistoryCell for PlanUpdateCell { indented_lines.extend(render_step(status, step)); } } - lines.extend(prefix_lines(indented_lines, &" └ ".into(), &" ".into())); + lines.extend(prefix_lines(indented_lines, " └ ".into(), " ".into())); lines } @@ -1231,6 +1208,26 @@ pub(crate) fn new_patch_apply_failure(stderr: String) -> PlainHistoryCell { PlainHistoryCell { lines } } +/// Create a new history cell for a proposed command approval. +/// Renders a header and the command preview similar to how proposed patches +/// show a header and summary. +pub(crate) fn new_proposed_command(command: &[String]) -> PlainHistoryCell { + let cmd = strip_bash_lc_and_escape(command); + + let mut lines: Vec> = Vec::new(); + lines.push(Line::from(vec!["• ".into(), "Proposed Command".bold()])); + + let cmd_lines: Vec> = cmd + .lines() + .map(|part| Line::from(part.to_string())) + .collect(); + let initial_prefix: Span<'static> = " └ ".dim(); + let subsequent_prefix: Span<'static> = " ".into(); + lines.extend(prefix_lines(cmd_lines, initial_prefix, subsequent_prefix)); + + PlainHistoryCell { lines } +} + pub(crate) fn new_reasoning_block( full_reasoning_buffer: String, config: &Config, diff --git a/codex-rs/tui/src/render/line_utils.rs b/codex-rs/tui/src/render/line_utils.rs index 1cc9d5c9..5a8e0906 100644 --- a/codex-rs/tui/src/render/line_utils.rs +++ b/codex-rs/tui/src/render/line_utils.rs @@ -34,3 +34,26 @@ pub fn is_blank_line_spaces_only(line: &Line<'_>) -> bool { .iter() .all(|s| s.content.is_empty() || s.content.chars().all(|c| c == ' ')) } + +/// Prefix each line with `initial_prefix` for the first line and +/// `subsequent_prefix` for following lines. Returns a new Vec of owned lines. +pub fn prefix_lines( + lines: Vec>, + initial_prefix: Span<'static>, + subsequent_prefix: Span<'static>, +) -> Vec> { + lines + .into_iter() + .enumerate() + .map(|(i, l)| { + let mut spans = Vec::with_capacity(l.spans.len() + 1); + spans.push(if i == 0 { + initial_prefix.clone() + } else { + subsequent_prefix.clone() + }); + spans.extend(l.spans); + Line::from(spans) + }) + .collect() +} diff --git a/codex-rs/tui/src/user_approval_widget.rs b/codex-rs/tui/src/user_approval_widget.rs index 9ea2c0bc..4f173955 100644 --- a/codex-rs/tui/src/user_approval_widget.rs +++ b/codex-rs/tui/src/user_approval_widget.rs @@ -30,6 +30,7 @@ use crate::app_event::AppEvent; use crate::app_event_sender::AppEventSender; use crate::exec_command::strip_bash_lc_and_escape; use crate::history_cell; +use crate::text_formatting::truncate_text; /// Request coming from the agent that needs user approval. pub(crate) enum ApprovalRequest { @@ -110,45 +111,11 @@ pub(crate) struct UserApprovalWidget { done: bool, } -fn to_command_display<'a>( - first_line: Vec>, - cmd: String, - last_line: Vec>, -) -> Vec> { - let command_lines: Vec = cmd.lines().map(|line| line.to_string().dim()).collect(); - - let mut lines: Vec> = vec![]; - - let mut first_line = first_line.clone(); - if command_lines.len() == 1 { - first_line.push(command_lines[0].clone()); - first_line.extend(last_line); - } else { - for line in command_lines { - lines.push(vec![" ".into(), line].into()); - } - let last_line = last_line.clone(); - lines.push(Line::from(last_line)); - } - lines.insert(0, Line::from(first_line)); - - lines -} - impl UserApprovalWidget { pub(crate) fn new(approval_request: ApprovalRequest, app_event_tx: AppEventSender) -> Self { let confirmation_prompt = match &approval_request { - ApprovalRequest::Exec { - command, reason, .. - } => { - let cmd = strip_bash_lc_and_escape(command); - let mut contents: Vec = to_command_display( - vec!["? ".fg(Color::Cyan), "Codex wants to run ".bold()], - cmd, - vec![], - ); - - contents.push(Line::from("")); + ApprovalRequest::Exec { reason, .. } => { + let mut contents: Vec = vec![]; if let Some(reason) = reason { contents.push(Line::from(reason.clone().italic())); contents.push(Line::from("")); @@ -258,62 +225,61 @@ impl UserApprovalWidget { fn send_decision_with_feedback(&mut self, decision: ReviewDecision, feedback: String) { match &self.approval_request { ApprovalRequest::Exec { command, .. } => { - let cmd = strip_bash_lc_and_escape(command); - // TODO: move this rendering into history_cell. - let mut lines: Vec> = vec![]; + let full_cmd = strip_bash_lc_and_escape(command); + // Construct a concise, single-line summary of the command: + // - If multi-line, take the first line and append " ...". + // - Truncate to 80 graphemes. + let mut snippet = match full_cmd.split_once('\n') { + Some((first, _)) => format!("{first} ..."), + None => full_cmd.clone(), + }; + // Enforce the 80 character length limit. + snippet = truncate_text(&snippet, 80); - // Result line based on decision. + let mut result_spans: Vec> = Vec::new(); match decision { ReviewDecision::Approved => { - lines.extend(to_command_display( - vec![ - "✔ ".fg(Color::Green), - "You ".into(), - "approved".bold(), - " codex to run ".into(), - ], - cmd, - vec![" this time".bold()], - )); + result_spans.extend(vec![ + "✔ ".fg(Color::Green), + "You ".into(), + "approved".bold(), + " codex to run ".into(), + snippet.clone().dim(), + " this time".bold(), + ]); } ReviewDecision::ApprovedForSession => { - lines.extend(to_command_display( - vec![ - "✔ ".fg(Color::Green), - "You ".into(), - "approved".bold(), - " codex to run ".into(), - ], - cmd, - vec![" every time this session".bold()], - )); + result_spans.extend(vec![ + "✔ ".fg(Color::Green), + "You ".into(), + "approved".bold(), + " codex to run ".into(), + snippet.clone().dim(), + " every time this session".bold(), + ]); } ReviewDecision::Denied => { - lines.extend(to_command_display( - vec![ - "✗ ".fg(Color::Red), - "You ".into(), - "did not approve".bold(), - " codex to run ".into(), - ], - cmd, - vec![], - )); + result_spans.extend(vec![ + "✗ ".fg(Color::Red), + "You ".into(), + "did not approve".bold(), + " codex to run ".into(), + snippet.clone().dim(), + ]); } ReviewDecision::Abort => { - lines.extend(to_command_display( - vec![ - "✗ ".fg(Color::Red), - "You ".into(), - "canceled".bold(), - " the request to run ".into(), - ], - cmd, - vec![], - )); + result_spans.extend(vec![ + "✗ ".fg(Color::Red), + "You ".into(), + "canceled".bold(), + " the request to run ".into(), + snippet.clone().dim(), + ]); } } + let mut lines: Vec> = vec![Line::from(result_spans)]; + if !feedback.trim().is_empty() { lines.push(Line::from("feedback:")); for l in feedback.lines() {