mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
feat: Constrain values for approval_policy (#7778)
Constrain `approval_policy` through new `admin_policy` config. This PR will: 1. Add a `admin_policy` section to config, with a single field (for now) `allowed_approval_policies`. This list constrains the set of user-settable `approval_policy`s. 2. Introduce a new `Constrained<T>` type, which combines a current value and a validator function. The validator function ensures disallowed values are not set. 3. Change the type of `approval_policy` on `Config` and `SessionConfiguration` from `AskForApproval` to `Constrained<AskForApproval>`. The validator function is set by the values passed into `allowed_approval_policies`. 4. `GenericDisplayRow`: add a `disabled_reason: Option<String>`. When set, it disables selection of the value and indicates as such in the menu. This also makes it unselectable with arrow keys or numbers. This is used in the `/approvals` menu. Follow ups are: 1. Do the same thing to `sandbox_policy`. 2. Propagate the allowed set of values through app-server for the extension (though already this should prevent app-server from setting this values, it's just that we want to disable UI elements that are unsettable). Happy to split this PR up if you prefer, into the logical numbered areas above. Especially if there are parts we want to gavel on separately (e.g. admin_policy). Disabled full access: <img width="1680" height="380" alt="image" src="https://github.com/user-attachments/assets/1fb61c8c-1fcb-4dc4-8355-2293edb52ba0" /> Disabled `--yolo` on startup: <img width="749" height="76" alt="image" src="https://github.com/user-attachments/assets/0a1211a0-6eb1-40d6-a1d7-439c41e94ddb" /> CODEX-4087
This commit is contained in:
@@ -185,6 +185,7 @@ impl CommandPopup {
|
||||
display_shortcut: None,
|
||||
description: Some(description),
|
||||
wrap_indent: None,
|
||||
disabled_reason: None,
|
||||
}
|
||||
})
|
||||
.collect()
|
||||
|
||||
@@ -132,6 +132,7 @@ impl WidgetRef for &FileSearchPopup {
|
||||
display_shortcut: None,
|
||||
description: None,
|
||||
wrap_indent: None,
|
||||
disabled_reason: None,
|
||||
})
|
||||
.collect()
|
||||
};
|
||||
|
||||
@@ -44,6 +44,7 @@ pub(crate) struct SelectionItem {
|
||||
pub actions: Vec<SelectionAction>,
|
||||
pub dismiss_on_select: bool,
|
||||
pub search_value: Option<String>,
|
||||
pub disabled_reason: Option<String>,
|
||||
}
|
||||
|
||||
pub(crate) struct SelectionViewParams {
|
||||
@@ -217,6 +218,7 @@ impl ListSelectionView {
|
||||
match_indices: None,
|
||||
description,
|
||||
wrap_indent,
|
||||
disabled_reason: item.disabled_reason.clone(),
|
||||
}
|
||||
})
|
||||
})
|
||||
@@ -228,6 +230,7 @@ impl ListSelectionView {
|
||||
self.state.move_up_wrap(len);
|
||||
let visible = Self::max_visible_rows(len);
|
||||
self.state.ensure_visible(len, visible);
|
||||
self.skip_disabled_up();
|
||||
}
|
||||
|
||||
fn move_down(&mut self) {
|
||||
@@ -235,12 +238,14 @@ impl ListSelectionView {
|
||||
self.state.move_down_wrap(len);
|
||||
let visible = Self::max_visible_rows(len);
|
||||
self.state.ensure_visible(len, visible);
|
||||
self.skip_disabled_down();
|
||||
}
|
||||
|
||||
fn accept(&mut self) {
|
||||
if let Some(idx) = self.state.selected_idx
|
||||
&& let Some(actual_idx) = self.filtered_indices.get(idx)
|
||||
&& let Some(item) = self.items.get(*actual_idx)
|
||||
&& item.disabled_reason.is_none()
|
||||
{
|
||||
self.last_selected_actual_idx = Some(*actual_idx);
|
||||
for act in &item.actions {
|
||||
@@ -267,6 +272,40 @@ impl ListSelectionView {
|
||||
fn rows_width(total_width: u16) -> u16 {
|
||||
total_width.saturating_sub(2)
|
||||
}
|
||||
|
||||
fn skip_disabled_down(&mut self) {
|
||||
let len = self.visible_len();
|
||||
for _ in 0..len {
|
||||
if let Some(idx) = self.state.selected_idx
|
||||
&& let Some(actual_idx) = self.filtered_indices.get(idx)
|
||||
&& self
|
||||
.items
|
||||
.get(*actual_idx)
|
||||
.is_some_and(|item| item.disabled_reason.is_some())
|
||||
{
|
||||
self.state.move_down_wrap(len);
|
||||
} else {
|
||||
break;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
fn skip_disabled_up(&mut self) {
|
||||
let len = self.visible_len();
|
||||
for _ in 0..len {
|
||||
if let Some(idx) = self.state.selected_idx
|
||||
&& let Some(actual_idx) = self.filtered_indices.get(idx)
|
||||
&& self
|
||||
.items
|
||||
.get(*actual_idx)
|
||||
.is_some_and(|item| item.disabled_reason.is_some())
|
||||
{
|
||||
self.state.move_up_wrap(len);
|
||||
} else {
|
||||
break;
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
impl BottomPaneView for ListSelectionView {
|
||||
@@ -348,6 +387,10 @@ impl BottomPaneView for ListSelectionView {
|
||||
.map(|d| d as usize)
|
||||
.and_then(|d| d.checked_sub(1))
|
||||
&& idx < self.items.len()
|
||||
&& self
|
||||
.items
|
||||
.get(idx)
|
||||
.is_some_and(|item| item.disabled_reason.is_none())
|
||||
{
|
||||
self.state.selected_idx = Some(idx);
|
||||
self.accept();
|
||||
|
||||
@@ -20,6 +20,7 @@ pub(crate) struct GenericDisplayRow {
|
||||
pub display_shortcut: Option<KeyBinding>,
|
||||
pub match_indices: Option<Vec<usize>>, // indices to bold (char positions)
|
||||
pub description: Option<String>, // optional grey text after the name
|
||||
pub disabled_reason: Option<String>, // optional disabled message
|
||||
pub wrap_indent: Option<usize>, // optional indent for wrapped lines
|
||||
}
|
||||
|
||||
@@ -37,7 +38,13 @@ fn compute_desc_col(
|
||||
.iter()
|
||||
.enumerate()
|
||||
.filter(|(i, _)| visible_range.contains(i))
|
||||
.map(|(_, r)| Line::from(r.name.clone()).width())
|
||||
.map(|(_, r)| {
|
||||
let mut spans: Vec<Span> = vec![r.name.clone().into()];
|
||||
if r.disabled_reason.is_some() {
|
||||
spans.push(" (disabled)".dim());
|
||||
}
|
||||
Line::from(spans).width()
|
||||
})
|
||||
.max()
|
||||
.unwrap_or(0);
|
||||
let mut desc_col = max_name_width.saturating_add(2);
|
||||
@@ -51,7 +58,7 @@ fn compute_desc_col(
|
||||
fn wrap_indent(row: &GenericDisplayRow, desc_col: usize, max_width: u16) -> usize {
|
||||
let max_indent = max_width.saturating_sub(1) as usize;
|
||||
let indent = row.wrap_indent.unwrap_or_else(|| {
|
||||
if row.description.is_some() {
|
||||
if row.description.is_some() || row.disabled_reason.is_some() {
|
||||
desc_col
|
||||
} else {
|
||||
0
|
||||
@@ -64,10 +71,16 @@ fn wrap_indent(row: &GenericDisplayRow, desc_col: usize, max_width: u16) -> usiz
|
||||
/// at `desc_col`. Applies fuzzy-match bolding when indices are present and
|
||||
/// dims the description.
|
||||
fn build_full_line(row: &GenericDisplayRow, desc_col: usize) -> Line<'static> {
|
||||
let combined_description = match (&row.description, &row.disabled_reason) {
|
||||
(Some(desc), Some(reason)) => Some(format!("{desc} (disabled: {reason})")),
|
||||
(Some(desc), None) => Some(desc.clone()),
|
||||
(None, Some(reason)) => Some(format!("disabled: {reason}")),
|
||||
(None, None) => None,
|
||||
};
|
||||
|
||||
// Enforce single-line name: allow at most desc_col - 2 cells for name,
|
||||
// reserving two spaces before the description column.
|
||||
let name_limit = row
|
||||
.description
|
||||
let name_limit = combined_description
|
||||
.as_ref()
|
||||
.map(|_| desc_col.saturating_sub(2))
|
||||
.unwrap_or(usize::MAX);
|
||||
@@ -113,6 +126,10 @@ fn build_full_line(row: &GenericDisplayRow, desc_col: usize) -> Line<'static> {
|
||||
name_spans.push("…".into());
|
||||
}
|
||||
|
||||
if row.disabled_reason.is_some() {
|
||||
name_spans.push(" (disabled)".dim());
|
||||
}
|
||||
|
||||
let this_name_width = Line::from(name_spans.clone()).width();
|
||||
let mut full_spans: Vec<Span> = name_spans;
|
||||
if let Some(display_shortcut) = row.display_shortcut {
|
||||
@@ -120,7 +137,7 @@ fn build_full_line(row: &GenericDisplayRow, desc_col: usize) -> Line<'static> {
|
||||
full_spans.push(display_shortcut.into());
|
||||
full_spans.push(")".into());
|
||||
}
|
||||
if let Some(desc) = row.description.as_ref() {
|
||||
if let Some(desc) = combined_description.as_ref() {
|
||||
let gap = desc_col.saturating_sub(this_name_width);
|
||||
if gap > 0 {
|
||||
full_spans.push(" ".repeat(gap).into());
|
||||
|
||||
@@ -92,6 +92,7 @@ impl SkillPopup {
|
||||
match_indices: indices,
|
||||
display_shortcut: None,
|
||||
description: Some(description),
|
||||
disabled_reason: None,
|
||||
wrap_indent: None,
|
||||
}
|
||||
})
|
||||
|
||||
@@ -2556,7 +2556,7 @@ impl ChatWidget {
|
||||
|
||||
/// Open a popup to choose the approvals mode (ask for approval policy + sandbox policy).
|
||||
pub(crate) fn open_approvals_popup(&mut self) {
|
||||
let current_approval = self.config.approval_policy;
|
||||
let current_approval = self.config.approval_policy.value();
|
||||
let current_sandbox = self.config.sandbox_policy.clone();
|
||||
let mut items: Vec<SelectionItem> = Vec::new();
|
||||
let presets: Vec<ApprovalPreset> = builtin_approval_presets();
|
||||
@@ -2564,8 +2564,11 @@ impl ChatWidget {
|
||||
let is_current =
|
||||
Self::preset_matches_current(current_approval, ¤t_sandbox, &preset);
|
||||
let name = preset.label.to_string();
|
||||
let description_text = preset.description;
|
||||
let description = Some(description_text.to_string());
|
||||
let description = Some(preset.description.to_string());
|
||||
let disabled_reason = match self.config.approval_policy.can_set(&preset.approval) {
|
||||
Ok(()) => None,
|
||||
Err(err) => Some(err.to_string()),
|
||||
};
|
||||
let requires_confirmation = preset.id == "full-access"
|
||||
&& !self
|
||||
.config
|
||||
@@ -2618,6 +2621,7 @@ impl ChatWidget {
|
||||
is_current,
|
||||
actions,
|
||||
dismiss_on_select: true,
|
||||
disabled_reason,
|
||||
..Default::default()
|
||||
});
|
||||
}
|
||||
@@ -2954,7 +2958,9 @@ impl ChatWidget {
|
||||
|
||||
/// Set the approval policy in the widget's config copy.
|
||||
pub(crate) fn set_approval_policy(&mut self, policy: AskForApproval) {
|
||||
self.config.approval_policy = policy;
|
||||
if let Err(err) = self.config.approval_policy.set(policy) {
|
||||
tracing::warn!(%err, "failed to set approval_policy on chat config");
|
||||
}
|
||||
}
|
||||
|
||||
/// Set the sandbox policy in the widget's config copy.
|
||||
|
||||
@@ -10,6 +10,8 @@ use codex_core::CodexAuth;
|
||||
use codex_core::config::Config;
|
||||
use codex_core::config::ConfigOverrides;
|
||||
use codex_core::config::ConfigToml;
|
||||
use codex_core::config::Constrained;
|
||||
use codex_core::config::ConstraintError;
|
||||
use codex_core::openai_models::models_manager::ModelsManager;
|
||||
use codex_core::protocol::AgentMessageDeltaEvent;
|
||||
use codex_core::protocol::AgentMessageEvent;
|
||||
@@ -2039,17 +2041,125 @@ fn disabled_slash_command_while_task_running_snapshot() {
|
||||
assert_snapshot!(blob);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn approvals_popup_shows_disabled_presets() {
|
||||
let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None);
|
||||
|
||||
chat.config.approval_policy =
|
||||
Constrained::new(AskForApproval::OnRequest, |candidate| match candidate {
|
||||
AskForApproval::OnRequest => Ok(()),
|
||||
_ => Err(ConstraintError {
|
||||
message: "this message should be printed in the description".to_string(),
|
||||
}),
|
||||
})
|
||||
.expect("construct constrained approval policy");
|
||||
|
||||
chat.open_approvals_popup();
|
||||
|
||||
let width = 80;
|
||||
let height = chat.desired_height(width);
|
||||
let mut terminal =
|
||||
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("render approvals popup");
|
||||
|
||||
let screen = terminal.backend().vt100().screen().contents();
|
||||
let collapsed = screen.split_whitespace().collect::<Vec<_>>().join(" ");
|
||||
assert!(
|
||||
collapsed.contains("(disabled)"),
|
||||
"disabled preset label should be shown"
|
||||
);
|
||||
assert!(
|
||||
collapsed.contains("this message should be printed in the description"),
|
||||
"disabled preset reason should be shown"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn approvals_popup_navigation_skips_disabled() {
|
||||
let (mut chat, mut rx, mut op_rx) = make_chatwidget_manual(None);
|
||||
|
||||
chat.config.approval_policy =
|
||||
Constrained::new(AskForApproval::OnRequest, |candidate| match candidate {
|
||||
AskForApproval::OnRequest => Ok(()),
|
||||
_ => Err(ConstraintError {
|
||||
message: "disabled preset".to_string(),
|
||||
}),
|
||||
})
|
||||
.expect("construct constrained approval policy");
|
||||
|
||||
chat.open_approvals_popup();
|
||||
|
||||
// The approvals popup is the active bottom-pane view; drive navigation via chat handle_key_event.
|
||||
// Start selected at idx 0 (enabled), move down twice; the disabled option should be skipped
|
||||
// and selection should wrap back to idx 0 (also enabled).
|
||||
chat.handle_key_event(KeyEvent::from(KeyCode::Down));
|
||||
chat.handle_key_event(KeyEvent::from(KeyCode::Down));
|
||||
|
||||
// Press numeric shortcut for the disabled row (3 => idx 2); should not close or accept.
|
||||
chat.handle_key_event(KeyEvent::from(KeyCode::Char('3')));
|
||||
|
||||
// Ensure the popup remains open and no selection actions were sent.
|
||||
let width = 80;
|
||||
let height = chat.desired_height(width);
|
||||
let mut terminal =
|
||||
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("render approvals popup after disabled selection");
|
||||
let screen = terminal.backend().vt100().screen().contents();
|
||||
assert!(
|
||||
screen.contains("Select Approval Mode"),
|
||||
"popup should remain open after selecting a disabled entry"
|
||||
);
|
||||
assert!(
|
||||
op_rx.try_recv().is_err(),
|
||||
"no actions should be dispatched yet"
|
||||
);
|
||||
assert!(rx.try_recv().is_err(), "no history should be emitted");
|
||||
|
||||
// Press Enter; selection should land on an enabled preset and dispatch updates.
|
||||
chat.handle_key_event(KeyEvent::from(KeyCode::Enter));
|
||||
let mut app_events = Vec::new();
|
||||
while let Ok(ev) = rx.try_recv() {
|
||||
app_events.push(ev);
|
||||
}
|
||||
assert!(
|
||||
app_events.iter().any(|ev| matches!(
|
||||
ev,
|
||||
AppEvent::CodexOp(Op::OverrideTurnContext {
|
||||
approval_policy: Some(AskForApproval::OnRequest),
|
||||
..
|
||||
})
|
||||
)),
|
||||
"enter should select an enabled preset"
|
||||
);
|
||||
assert!(
|
||||
!app_events.iter().any(|ev| matches!(
|
||||
ev,
|
||||
AppEvent::CodexOp(Op::OverrideTurnContext {
|
||||
approval_policy: Some(AskForApproval::Never),
|
||||
..
|
||||
})
|
||||
)),
|
||||
"disabled preset should not be selected"
|
||||
);
|
||||
}
|
||||
|
||||
//
|
||||
// Snapshot test: command approval modal
|
||||
//
|
||||
// Synthesizes a Codex ExecApprovalRequest event to trigger the approval modal
|
||||
// and snapshots the visual output using the ratatui TestBackend.
|
||||
#[test]
|
||||
fn approval_modal_exec_snapshot() {
|
||||
fn approval_modal_exec_snapshot() -> anyhow::Result<()> {
|
||||
// Build a chat widget with manual channels to avoid spawning the agent.
|
||||
let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None);
|
||||
// Ensure policy allows surfacing approvals explicitly (not strictly required for direct event).
|
||||
chat.config.approval_policy = AskForApproval::OnRequest;
|
||||
chat.config.approval_policy.set(AskForApproval::OnRequest)?;
|
||||
// Inject an exec approval request to display the approval modal.
|
||||
let ev = ExecApprovalRequestEvent {
|
||||
call_id: "call-approve-cmd".into(),
|
||||
@@ -2095,14 +2205,16 @@ fn approval_modal_exec_snapshot() {
|
||||
"approval_modal_exec",
|
||||
terminal.backend().vt100().screen().contents()
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
// Snapshot test: command approval modal without a reason
|
||||
// Ensures spacing looks correct when no reason text is provided.
|
||||
#[test]
|
||||
fn approval_modal_exec_without_reason_snapshot() {
|
||||
fn approval_modal_exec_without_reason_snapshot() -> anyhow::Result<()> {
|
||||
let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None);
|
||||
chat.config.approval_policy = AskForApproval::OnRequest;
|
||||
chat.config.approval_policy.set(AskForApproval::OnRequest)?;
|
||||
|
||||
let ev = ExecApprovalRequestEvent {
|
||||
call_id: "call-approve-cmd-noreason".into(),
|
||||
@@ -2134,13 +2246,15 @@ fn approval_modal_exec_without_reason_snapshot() {
|
||||
"approval_modal_exec_no_reason",
|
||||
terminal.backend().vt100().screen().contents()
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
// Snapshot test: patch approval modal
|
||||
#[test]
|
||||
fn approval_modal_patch_snapshot() {
|
||||
fn approval_modal_patch_snapshot() -> anyhow::Result<()> {
|
||||
let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None);
|
||||
chat.config.approval_policy = AskForApproval::OnRequest;
|
||||
chat.config.approval_policy.set(AskForApproval::OnRequest)?;
|
||||
|
||||
// Build a small changeset and a reason/grant_root to exercise the prompt text.
|
||||
let mut changes = HashMap::new();
|
||||
@@ -2174,6 +2288,8 @@ fn approval_modal_patch_snapshot() {
|
||||
"approval_modal_patch",
|
||||
terminal.backend().vt100().screen().contents()
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -2736,10 +2852,10 @@ fn apply_patch_full_flow_integration_like() {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn apply_patch_untrusted_shows_approval_modal() {
|
||||
fn apply_patch_untrusted_shows_approval_modal() -> anyhow::Result<()> {
|
||||
let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None);
|
||||
// Ensure approval policy is untrusted (OnRequest)
|
||||
chat.config.approval_policy = AskForApproval::OnRequest;
|
||||
chat.config.approval_policy.set(AskForApproval::OnRequest)?;
|
||||
|
||||
// Simulate a patch approval request from backend
|
||||
let mut changes = HashMap::new();
|
||||
@@ -2778,14 +2894,16 @@ fn apply_patch_untrusted_shows_approval_modal() {
|
||||
contains_title,
|
||||
"expected approval modal to be visible with title 'Would you like to make the following edits?'"
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn apply_patch_request_shows_diff_summary() {
|
||||
fn apply_patch_request_shows_diff_summary() -> anyhow::Result<()> {
|
||||
let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(None);
|
||||
|
||||
// Ensure we are in OnRequest so an approval is surfaced
|
||||
chat.config.approval_policy = AskForApproval::OnRequest;
|
||||
chat.config.approval_policy.set(AskForApproval::OnRequest)?;
|
||||
|
||||
// Simulate backend asking to apply a patch adding two lines to README.md
|
||||
let mut changes = HashMap::new();
|
||||
@@ -2844,6 +2962,8 @@ fn apply_patch_request_shows_diff_summary() {
|
||||
saw_line1 && saw_line2,
|
||||
"expected modal to show per-line diff summary"
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
@@ -25,6 +25,7 @@ pub struct VT100Backend {
|
||||
impl VT100Backend {
|
||||
/// Creates a new `TestBackend` with the specified width and height.
|
||||
pub fn new(width: u16, height: u16) -> Self {
|
||||
crossterm::style::force_color_output(true);
|
||||
Self {
|
||||
crossterm_backend: CrosstermBackend::new(vt100::Parser::new(height, width, 0)),
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user