From 109dc3a178a819ee27b001268c91657b58b5970f Mon Sep 17 00:00:00 2001 From: kevin zhao Date: Mon, 1 Dec 2025 14:08:34 -0500 Subject: [PATCH] improved behavior for dont ask again for this prefix --- codex-rs/core/src/exec_policy.rs | 46 +++++++++++++++--- .../tui/src/bottom_pane/approval_overlay.rs | 48 +++++++++---------- ...hatwidget__tests__approval_modal_exec.snap | 8 ++-- ..._tests__approval_modal_exec_no_reason.snap | 5 +- ...dget__tests__exec_approval_modal_exec.snap | 13 ++--- ...sts__status_widget_and_approval_modal.snap | 28 +++++------ codex-rs/tui/src/chatwidget/tests.rs | 20 ++++---- 7 files changed, 99 insertions(+), 69 deletions(-) diff --git a/codex-rs/core/src/exec_policy.rs b/codex-rs/core/src/exec_policy.rs index 44328de14a..817d792df5 100644 --- a/codex-rs/core/src/exec_policy.rs +++ b/codex-rs/core/src/exec_policy.rs @@ -160,14 +160,21 @@ fn requirement_from_decision( /// unrolled command (multi-part scripts shouldn’t be whitelisted via prefix) and /// when execpolicy feature is enabled. fn allow_prefix_if_applicable( + policy: &Policy, commands: &[Vec], features: &Features, ) -> Option> { - if features.enabled(Feature::ExecPolicy) && commands.len() == 1 { - Some(commands[0].clone()) - } else { - None + if !features.enabled(Feature::ExecPolicy) { + return None; } + + // Only offer a prefix when the prompt is driven by heuristics (policy has no matches). + // For multi-command scripts, choose the first command segment that also has no policy + // match (i.e. the first segment that could plausibly be the prompt trigger). + commands + .iter() + .find(|cmd| matches!(policy.check(cmd), Evaluation::NoMatch)) + .cloned() } pub(crate) async fn create_approval_requirement_for_command( @@ -193,7 +200,7 @@ pub(crate) async fn create_approval_requirement_for_command( ) { ApprovalRequirement::NeedsApproval { reason: None, - allow_prefix: allow_prefix_if_applicable(&commands, features), + allow_prefix: allow_prefix_if_applicable(policy, &commands, features), } } else { ApprovalRequirement::Skip { @@ -597,8 +604,35 @@ prefix_rule(pattern=["rm"], decision="forbidden") requirement, ApprovalRequirement::NeedsApproval { reason: None, - allow_prefix: None, + allow_prefix: Some(vec!["python".to_string()]), } ); } + + #[test] + fn allow_prefix_uses_first_no_match_in_multi_command_scripts() { + let policy_src = r#"prefix_rule(pattern=["python"], decision="allow")"#; + let mut parser = PolicyParser::new(); + parser + .parse("test.codexpolicy", policy_src) + .expect("parse policy"); + let policy = parser.build(); + + let command = vec![ + "bash".to_string(), + "-lc".to_string(), + "python && echo ok".to_string(), + ]; + + let requirement = create_approval_requirement_for_command( + &policy, + &Features::with_defaults(), + &command, + AskForApproval::UnlessTrusted, + &SandboxPolicy::ReadOnly, + SandboxPermissions::UseDefault, + ); + + assert_eq!(requirement, ApprovalRequirement::Skip); + } } diff --git a/codex-rs/tui/src/bottom_pane/approval_overlay.rs b/codex-rs/tui/src/bottom_pane/approval_overlay.rs index 3defdbf9e3..6b36175848 100644 --- a/codex-rs/tui/src/bottom_pane/approval_overlay.rs +++ b/codex-rs/tui/src/bottom_pane/approval_overlay.rs @@ -471,29 +471,30 @@ impl ApprovalOption { } fn exec_options(allow_prefix: Option>) -> Vec { - vec![ - ApprovalOption { - label: "Yes, proceed".to_string(), - decision: ApprovalDecision::Review(ReviewDecision::Approved), - display_shortcut: None, - additional_shortcuts: vec![key_hint::plain(KeyCode::Char('y'))], - }, - ApprovalOption { - label: "Yes, and don't ask again this session".to_string(), - decision: ApprovalDecision::Review(ReviewDecision::ApprovedForSession), - display_shortcut: None, - additional_shortcuts: vec![key_hint::plain(KeyCode::Char('a'))], - }, - ] - .into_iter() - .chain(allow_prefix.map(|prefix| ApprovalOption { - label: "Yes, and don't ask again for commands with this prefix".to_string(), - decision: ApprovalDecision::Review(ReviewDecision::ApprovedAllowPrefix { - allow_prefix: prefix, - }), + let allow_prefix_label = allow_prefix.as_ref().map(|prefix| { + let rendered_prefix = strip_bash_lc_and_escape(prefix); + format!("Yes, and don't ask again for `{rendered_prefix}` commands") + }); + + vec![ApprovalOption { + label: "Yes, proceed".to_string(), + decision: ApprovalDecision::Review(ReviewDecision::Approved), display_shortcut: None, - additional_shortcuts: vec![key_hint::plain(KeyCode::Char('p'))], - })) + additional_shortcuts: vec![key_hint::plain(KeyCode::Char('y'))], + }] + .into_iter() + .chain( + allow_prefix + .zip(allow_prefix_label) + .map(|(prefix, label)| ApprovalOption { + label, + decision: ApprovalDecision::Review(ReviewDecision::ApprovedAllowPrefix { + allow_prefix: prefix, + }), + display_shortcut: None, + additional_shortcuts: vec![key_hint::plain(KeyCode::Char('p'))], + }), + ) .chain([ApprovalOption { label: "No, and tell Codex what to do differently".to_string(), decision: ApprovalDecision::Review(ReviewDecision::Abort), @@ -687,7 +688,6 @@ mod tests { let (tx_raw, mut rx) = unbounded_channel::(); let tx = AppEventSender::new(tx_raw); let mut view = ApprovalOverlay::new(make_exec_request(), tx); - view.handle_key_event(KeyEvent::new(KeyCode::Down, KeyModifiers::NONE)); view.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); assert!( @@ -702,6 +702,6 @@ mod tests { break; } } - assert_eq!(decision, Some(ReviewDecision::ApprovedForSession)); + assert_eq!(decision, Some(ReviewDecision::Approved)); } } 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 eaf0fed3a1..748537beee 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,14 +4,12 @@ expression: terminal.backend().vt100().screen().contents() --- Would you like to run the following command? - Reason: this is a test reason such as one that would be produced by the - model + Reason: this is a test reason such as one that would be produced by the model $ echo hello world › 1. Yes, proceed (y) - 2. Yes, and don't ask again this session (a) - 3. Yes, and don't ask again for commands with this prefix (p) - 4. No, and tell Codex what to do differently (esc) + 2. Yes, and don't ask again for `echo hello world` commands (p) + 3. No, and tell Codex what to do differently (esc) Press enter to confirm or esc to cancel 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 ce2277ce62..8f3d27fe7c 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 @@ -7,8 +7,7 @@ expression: terminal.backend().vt100().screen().contents() $ echo hello world › 1. Yes, proceed (y) - 2. Yes, and don't ask again this session (a) - 3. Yes, and don't ask again for commands with this prefix (p) - 4. No, and tell Codex what to do differently (esc) + 2. Yes, and don't ask again for `echo hello world` commands (p) + 3. No, and tell Codex what to do differently (esc) Press enter to confirm or esc to cancel diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_modal_exec.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_modal_exec.snap index ff70d7d492..1c6a3ef136 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_modal_exec.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_modal_exec.snap @@ -3,7 +3,7 @@ source: tui/src/chatwidget/tests.rs expression: "format!(\"{buf:?}\")" --- Buffer { - area: Rect { x: 0, y: 0, width: 80, height: 14 }, + area: Rect { x: 0, y: 0, width: 80, height: 13 }, content: [ " ", " ", @@ -15,8 +15,7 @@ Buffer { " $ echo hello world ", " ", "› 1. Yes, proceed (y) ", - " 2. Yes, and don't ask again this session (a) ", - " 3. No, and tell Codex what to do differently (esc) ", + " 2. No, and tell Codex what to do differently (esc) ", " ", " Press enter to confirm or esc to cancel ", ], @@ -30,10 +29,8 @@ Buffer { x: 7, y: 5, fg: Reset, bg: Reset, underline: Reset, modifier: NONE, x: 0, y: 9, fg: Cyan, bg: Reset, underline: Reset, modifier: BOLD, x: 21, y: 9, fg: Reset, bg: Reset, underline: Reset, modifier: NONE, - x: 44, y: 10, fg: Reset, bg: Reset, underline: Reset, modifier: DIM, - x: 45, y: 10, fg: Reset, bg: Reset, underline: Reset, modifier: NONE, - x: 48, y: 11, fg: Reset, bg: Reset, underline: Reset, modifier: DIM, - x: 51, y: 11, fg: Reset, bg: Reset, underline: Reset, modifier: NONE, - x: 2, y: 13, fg: Reset, bg: Reset, underline: Reset, modifier: DIM, + x: 48, y: 10, fg: Reset, bg: Reset, underline: Reset, modifier: DIM, + x: 51, y: 10, fg: Reset, bg: Reset, underline: Reset, modifier: NONE, + x: 2, y: 12, fg: Reset, bg: Reset, underline: Reset, modifier: DIM, ] } 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 5ce388c87b..459ad2c2d9 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 @@ -2,18 +2,16 @@ source: tui/src/chatwidget/tests.rs expression: terminal.backend() --- -" " -" " -" Would you like to run the following command? " -" " -" Reason: this is a test reason such as one that would be produced by the " -" model " -" " -" $ echo 'hello world' " -" " -"› 1. Yes, proceed (y) " -" 2. Yes, and don't ask again this session (a) " -" 3. Yes, and don't ask again for commands with this prefix (p) " -" 4. No, and tell Codex what to do differently (esc) " -" " -" Press enter to confirm or esc to cancel " +" " +" " +" Would you like to run the following command? " +" " +" Reason: this is a test reason such as one that would be produced by the model " +" " +" $ echo 'hello world' " +" " +"› 1. Yes, proceed (y) " +" 2. Yes, and don't ask again for `echo 'hello world'` commands (p) " +" 3. No, and tell Codex what to do differently (esc) " +" " +" Press enter to confirm or esc to cancel " diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index c25355a759..5202908ade 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -1981,11 +1981,12 @@ fn approval_modal_exec_snapshot() { }); // Render to a fixed-size test terminal and snapshot. // Call desired_height first and use that exact height for rendering. - let height = chat.desired_height(80); + let width = 100; + let height = chat.desired_height(width); let mut terminal = - crate::custom_terminal::Terminal::with_options(VT100Backend::new(80, height)) + crate::custom_terminal::Terminal::with_options(VT100Backend::new(width, height)) .expect("create terminal"); - let viewport = Rect::new(0, 0, 80, height); + let viewport = Rect::new(0, 0, width, height); terminal.set_viewport_area(viewport); terminal @@ -2027,10 +2028,11 @@ fn approval_modal_exec_without_reason_snapshot() { msg: EventMsg::ExecApprovalRequest(ev), }); - let height = chat.desired_height(80); + let width = 100; + let height = chat.desired_height(width); let mut terminal = - ratatui::Terminal::new(VT100Backend::new(80, height)).expect("create terminal"); - terminal.set_viewport_area(Rect::new(0, 0, 80, height)); + ratatui::Terminal::new(VT100Backend::new(width, height)).expect("create terminal"); + terminal.set_viewport_area(Rect::new(0, 0, width, height)); terminal .draw(|f| chat.render(f.area(), f.buffer_mut())) .expect("draw approval modal (no reason)"); @@ -2242,9 +2244,11 @@ fn status_widget_and_approval_modal_snapshot() { }); // Render at the widget's desired height and snapshot. - let height = chat.desired_height(80); - let mut terminal = ratatui::Terminal::new(ratatui::backend::TestBackend::new(80, height)) + let width: u16 = 100; + let height = chat.desired_height(width); + let mut terminal = ratatui::Terminal::new(ratatui::backend::TestBackend::new(width, height)) .expect("create terminal"); + terminal.set_viewport_area(Rect::new(0, 0, width, height)); terminal .draw(|f| chat.render(f.area(), f.buffer_mut())) .expect("draw status + approval modal");