mirror of
https://github.com/openai/codex.git
synced 2026-04-24 22:54:54 +00:00
execpolicy tui flow (#7543)
## Updating the `execpolicy` TUI flow In the TUI, when going through the command approval flow, codex will now ask the user if they would like to whitelist the FIRST unmatched command among a chain of commands. For example, let's say the agent wants to run `apple | pear` with an empty `execpolicy` Neither apple nor pear will match to an `execpolicy` rule. Thus, when prompting the user, codex tui will ask the user if they would like to whitelist `apple`. If the agent wants to run `apple | pear` again, they would be prompted again because pear is still unknown. when prompted, the user will now be asked if they'd like to whitelist `pear`. Here's a demo video of this flow: https://github.com/user-attachments/assets/fd160717-f6cb-46b0-9f4a-f0a974d4e710 This PR also removed the `allow for this session` option from the TUI.
This commit is contained in:
@@ -16,6 +16,8 @@ use crate::key_hint::KeyBinding;
|
||||
use crate::render::highlight::highlight_bash_to_lines;
|
||||
use crate::render::renderable::ColumnRenderable;
|
||||
use crate::render::renderable::Renderable;
|
||||
use codex_core::features::Feature;
|
||||
use codex_core::features::Features;
|
||||
use codex_core::protocol::ElicitationAction;
|
||||
use codex_core::protocol::ExecPolicyAmendment;
|
||||
use codex_core::protocol::FileChange;
|
||||
@@ -69,10 +71,11 @@ pub(crate) struct ApprovalOverlay {
|
||||
options: Vec<ApprovalOption>,
|
||||
current_complete: bool,
|
||||
done: bool,
|
||||
features: Features,
|
||||
}
|
||||
|
||||
impl ApprovalOverlay {
|
||||
pub fn new(request: ApprovalRequest, app_event_tx: AppEventSender) -> Self {
|
||||
pub fn new(request: ApprovalRequest, app_event_tx: AppEventSender, features: Features) -> Self {
|
||||
let mut view = Self {
|
||||
current_request: None,
|
||||
current_variant: None,
|
||||
@@ -82,6 +85,7 @@ impl ApprovalOverlay {
|
||||
options: Vec::new(),
|
||||
current_complete: false,
|
||||
done: false,
|
||||
features,
|
||||
};
|
||||
view.set_current(request);
|
||||
view
|
||||
@@ -96,7 +100,7 @@ impl ApprovalOverlay {
|
||||
let ApprovalRequestState { variant, header } = ApprovalRequestState::from(request);
|
||||
self.current_variant = Some(variant.clone());
|
||||
self.current_complete = false;
|
||||
let (options, params) = Self::build_options(variant, header);
|
||||
let (options, params) = Self::build_options(variant, header, &self.features);
|
||||
self.options = options;
|
||||
self.list = ListSelectionView::new(params, self.app_event_tx.clone());
|
||||
}
|
||||
@@ -104,13 +108,14 @@ impl ApprovalOverlay {
|
||||
fn build_options(
|
||||
variant: ApprovalVariant,
|
||||
header: Box<dyn Renderable>,
|
||||
features: &Features,
|
||||
) -> (Vec<ApprovalOption>, SelectionViewParams) {
|
||||
let (options, title) = match &variant {
|
||||
ApprovalVariant::Exec {
|
||||
proposed_execpolicy_amendment,
|
||||
..
|
||||
} => (
|
||||
exec_options(proposed_execpolicy_amendment.clone()),
|
||||
exec_options(proposed_execpolicy_amendment.clone(), features),
|
||||
"Would you like to run the following command?".to_string(),
|
||||
),
|
||||
ApprovalVariant::ApplyPatch { .. } => (
|
||||
@@ -474,30 +479,36 @@ impl ApprovalOption {
|
||||
}
|
||||
}
|
||||
|
||||
fn exec_options(proposed_execpolicy_amendment: Option<ExecPolicyAmendment>) -> Vec<ApprovalOption> {
|
||||
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(proposed_execpolicy_amendment.map(|prefix| ApprovalOption {
|
||||
label: "Yes, and don't ask again for commands with this prefix".to_string(),
|
||||
decision: ApprovalDecision::Review(ReviewDecision::ApprovedExecpolicyAmendment {
|
||||
proposed_execpolicy_amendment: prefix,
|
||||
}),
|
||||
fn exec_options(
|
||||
proposed_execpolicy_amendment: Option<ExecPolicyAmendment>,
|
||||
features: &Features,
|
||||
) -> Vec<ApprovalOption> {
|
||||
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(
|
||||
proposed_execpolicy_amendment
|
||||
.filter(|_| features.enabled(Feature::ExecPolicy))
|
||||
.map(|prefix| {
|
||||
let rendered_prefix = strip_bash_lc_and_escape(prefix.command());
|
||||
ApprovalOption {
|
||||
label: format!(
|
||||
"Yes, and don't ask again for commands that start with `{rendered_prefix}`"
|
||||
),
|
||||
decision: ApprovalDecision::Review(
|
||||
ReviewDecision::ApprovedExecpolicyAmendment {
|
||||
proposed_execpolicy_amendment: 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),
|
||||
@@ -568,7 +579,7 @@ mod tests {
|
||||
fn ctrl_c_aborts_and_clears_queue() {
|
||||
let (tx, _rx) = unbounded_channel::<AppEvent>();
|
||||
let tx = AppEventSender::new(tx);
|
||||
let mut view = ApprovalOverlay::new(make_exec_request(), tx);
|
||||
let mut view = ApprovalOverlay::new(make_exec_request(), tx, Features::with_defaults());
|
||||
view.enqueue_request(make_exec_request());
|
||||
assert_eq!(CancellationEvent::Handled, view.on_ctrl_c());
|
||||
assert!(view.queue.is_empty());
|
||||
@@ -579,7 +590,7 @@ mod tests {
|
||||
fn shortcut_triggers_selection() {
|
||||
let (tx, mut rx) = unbounded_channel::<AppEvent>();
|
||||
let tx = AppEventSender::new(tx);
|
||||
let mut view = ApprovalOverlay::new(make_exec_request(), tx);
|
||||
let mut view = ApprovalOverlay::new(make_exec_request(), tx, Features::with_defaults());
|
||||
assert!(!view.is_complete());
|
||||
view.handle_key_event(KeyEvent::new(KeyCode::Char('y'), KeyModifiers::NONE));
|
||||
// We expect at least one CodexOp message in the queue.
|
||||
@@ -608,6 +619,7 @@ mod tests {
|
||||
])),
|
||||
},
|
||||
tx,
|
||||
Features::with_defaults(),
|
||||
);
|
||||
view.handle_key_event(KeyEvent::new(KeyCode::Char('p'), KeyModifiers::NONE));
|
||||
let mut saw_op = false;
|
||||
@@ -631,6 +643,33 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn exec_prefix_option_hidden_when_execpolicy_disabled() {
|
||||
let (tx, mut rx) = unbounded_channel::<AppEvent>();
|
||||
let tx = AppEventSender::new(tx);
|
||||
let mut view = ApprovalOverlay::new(
|
||||
ApprovalRequest::Exec {
|
||||
id: "test".to_string(),
|
||||
command: vec!["echo".to_string()],
|
||||
reason: None,
|
||||
risk: None,
|
||||
proposed_execpolicy_amendment: Some(ExecPolicyAmendment::new(vec![
|
||||
"echo".to_string(),
|
||||
])),
|
||||
},
|
||||
tx,
|
||||
{
|
||||
let mut features = Features::with_defaults();
|
||||
features.disable(Feature::ExecPolicy);
|
||||
features
|
||||
},
|
||||
);
|
||||
assert_eq!(view.options.len(), 2);
|
||||
view.handle_key_event(KeyEvent::new(KeyCode::Char('p'), KeyModifiers::NONE));
|
||||
assert!(!view.is_complete());
|
||||
assert!(rx.try_recv().is_err());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn header_includes_command_snippet() {
|
||||
let (tx, _rx) = unbounded_channel::<AppEvent>();
|
||||
@@ -644,7 +683,7 @@ mod tests {
|
||||
proposed_execpolicy_amendment: None,
|
||||
};
|
||||
|
||||
let view = ApprovalOverlay::new(exec_request, tx);
|
||||
let view = ApprovalOverlay::new(exec_request, tx, Features::with_defaults());
|
||||
let mut buf = Buffer::empty(Rect::new(0, 0, 80, view.desired_height(80)));
|
||||
view.render(Rect::new(0, 0, 80, view.desired_height(80)), &mut buf);
|
||||
|
||||
@@ -694,8 +733,7 @@ mod tests {
|
||||
fn enter_sets_last_selected_index_without_dismissing() {
|
||||
let (tx_raw, mut rx) = unbounded_channel::<AppEvent>();
|
||||
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));
|
||||
let mut view = ApprovalOverlay::new(make_exec_request(), tx, Features::with_defaults());
|
||||
view.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE));
|
||||
|
||||
assert!(
|
||||
@@ -710,6 +748,6 @@ mod tests {
|
||||
break;
|
||||
}
|
||||
}
|
||||
assert_eq!(decision, Some(ReviewDecision::ApprovedForSession));
|
||||
assert_eq!(decision, Some(ReviewDecision::Approved));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -8,6 +8,7 @@ use crate::render::renderable::Renderable;
|
||||
use crate::render::renderable::RenderableItem;
|
||||
use crate::tui::FrameRequester;
|
||||
use bottom_pane_view::BottomPaneView;
|
||||
use codex_core::features::Features;
|
||||
use codex_core::skills::model::SkillMetadata;
|
||||
use codex_file_search::FileMatch;
|
||||
use crossterm::event::KeyCode;
|
||||
@@ -409,7 +410,7 @@ impl BottomPane {
|
||||
}
|
||||
|
||||
/// Called when the agent requests user approval.
|
||||
pub fn push_approval_request(&mut self, request: ApprovalRequest) {
|
||||
pub fn push_approval_request(&mut self, request: ApprovalRequest, features: &Features) {
|
||||
let request = if let Some(view) = self.view_stack.last_mut() {
|
||||
match view.try_consume_approval_request(request) {
|
||||
Some(request) => request,
|
||||
@@ -423,7 +424,7 @@ impl BottomPane {
|
||||
};
|
||||
|
||||
// Otherwise create a new approval modal overlay.
|
||||
let modal = ApprovalOverlay::new(request, self.app_event_tx.clone());
|
||||
let modal = ApprovalOverlay::new(request, self.app_event_tx.clone(), features.clone());
|
||||
self.pause_status_timer_for_modal();
|
||||
self.push_view(Box::new(modal));
|
||||
}
|
||||
@@ -578,6 +579,7 @@ mod tests {
|
||||
fn ctrl_c_on_modal_consumes_and_shows_quit_hint() {
|
||||
let (tx_raw, _rx) = unbounded_channel::<AppEvent>();
|
||||
let tx = AppEventSender::new(tx_raw);
|
||||
let features = Features::with_defaults();
|
||||
let mut pane = BottomPane::new(BottomPaneParams {
|
||||
app_event_tx: tx,
|
||||
frame_requester: FrameRequester::test_dummy(),
|
||||
@@ -588,7 +590,7 @@ mod tests {
|
||||
animations_enabled: true,
|
||||
skills: Some(Vec::new()),
|
||||
});
|
||||
pane.push_approval_request(exec_request());
|
||||
pane.push_approval_request(exec_request(), &features);
|
||||
assert_eq!(CancellationEvent::Handled, pane.on_ctrl_c());
|
||||
assert!(pane.ctrl_c_quit_hint_visible());
|
||||
assert_eq!(CancellationEvent::NotHandled, pane.on_ctrl_c());
|
||||
@@ -600,6 +602,7 @@ mod tests {
|
||||
fn overlay_not_shown_above_approval_modal() {
|
||||
let (tx_raw, _rx) = unbounded_channel::<AppEvent>();
|
||||
let tx = AppEventSender::new(tx_raw);
|
||||
let features = Features::with_defaults();
|
||||
let mut pane = BottomPane::new(BottomPaneParams {
|
||||
app_event_tx: tx,
|
||||
frame_requester: FrameRequester::test_dummy(),
|
||||
@@ -612,7 +615,7 @@ mod tests {
|
||||
});
|
||||
|
||||
// Create an approval modal (active view).
|
||||
pane.push_approval_request(exec_request());
|
||||
pane.push_approval_request(exec_request(), &features);
|
||||
|
||||
// Render and verify the top row does not include an overlay.
|
||||
let area = Rect::new(0, 0, 60, 6);
|
||||
@@ -633,6 +636,7 @@ mod tests {
|
||||
fn composer_shown_after_denied_while_task_running() {
|
||||
let (tx_raw, _rx) = unbounded_channel::<AppEvent>();
|
||||
let tx = AppEventSender::new(tx_raw);
|
||||
let features = Features::with_defaults();
|
||||
let mut pane = BottomPane::new(BottomPaneParams {
|
||||
app_event_tx: tx,
|
||||
frame_requester: FrameRequester::test_dummy(),
|
||||
@@ -648,7 +652,7 @@ mod tests {
|
||||
pane.set_task_running(true);
|
||||
|
||||
// Push an approval modal (e.g., command approval) which should hide the status view.
|
||||
pane.push_approval_request(exec_request());
|
||||
pane.push_approval_request(exec_request(), &features);
|
||||
|
||||
// Simulate pressing 'n' (No) on the modal.
|
||||
use crossterm::event::KeyCode;
|
||||
|
||||
@@ -1076,7 +1076,8 @@ impl ChatWidget {
|
||||
risk: ev.risk,
|
||||
proposed_execpolicy_amendment: ev.proposed_execpolicy_amendment,
|
||||
};
|
||||
self.bottom_pane.push_approval_request(request);
|
||||
self.bottom_pane
|
||||
.push_approval_request(request, &self.config.features);
|
||||
self.request_redraw();
|
||||
}
|
||||
|
||||
@@ -1093,7 +1094,8 @@ impl ChatWidget {
|
||||
changes: ev.changes.clone(),
|
||||
cwd: self.config.cwd.clone(),
|
||||
};
|
||||
self.bottom_pane.push_approval_request(request);
|
||||
self.bottom_pane
|
||||
.push_approval_request(request, &self.config.features);
|
||||
self.request_redraw();
|
||||
self.notify(Notification::EditApprovalRequested {
|
||||
cwd: self.config.cwd.clone(),
|
||||
@@ -1113,7 +1115,8 @@ impl ChatWidget {
|
||||
request_id: ev.id,
|
||||
message: ev.message,
|
||||
};
|
||||
self.bottom_pane.push_approval_request(request);
|
||||
self.bottom_pane
|
||||
.push_approval_request(request, &self.config.features);
|
||||
self.request_redraw();
|
||||
}
|
||||
|
||||
|
||||
@@ -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 commands that start with `echo hello world` (p)
|
||||
3. No, and tell Codex what to do differently (esc)
|
||||
|
||||
Press enter to confirm or esc to cancel
|
||||
|
||||
@@ -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 commands that start with `echo hello world` (p)
|
||||
3. No, and tell Codex what to do differently (esc)
|
||||
|
||||
Press enter to confirm or esc to cancel
|
||||
|
||||
@@ -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,
|
||||
]
|
||||
}
|
||||
|
||||
@@ -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 commands that start with `echo 'hello world'` (p) "
|
||||
" 3. No, and tell Codex what to do differently (esc) "
|
||||
" "
|
||||
" Press enter to confirm or esc to cancel "
|
||||
|
||||
@@ -2007,11 +2007,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
|
||||
@@ -2057,10 +2058,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)");
|
||||
@@ -2275,9 +2277,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");
|
||||
|
||||
Reference in New Issue
Block a user