refactor: adding allow_prefix into ApprovedAllowPrefix

This commit is contained in:
kevin zhao
2025-11-20 20:25:25 -05:00
parent d4d293fcf0
commit 02c66be831
12 changed files with 38 additions and 76 deletions

View File

@@ -950,7 +950,6 @@ async fn on_exec_approval_response(
.submit(Op::ExecApproval {
id: event_turn_id,
decision: response.decision,
allow_prefix: None,
})
.await
{
@@ -1155,7 +1154,6 @@ async fn on_command_execution_request_approval_response(
.submit(Op::ExecApproval {
id: event_turn_id,
decision,
allow_prefix: None,
})
.await
{

View File

@@ -71,7 +71,7 @@ pub(crate) async fn apply_patch(
.await;
match rx_approve.await.unwrap_or_default() {
ReviewDecision::Approved
| ReviewDecision::ApprovedAllowPrefix
| ReviewDecision::ApprovedAllowPrefix { .. }
| ReviewDecision::ApprovedForSession => {
InternalApplyPatchInvocation::DelegateToExec(ApplyPatchExec {
action,

View File

@@ -1470,12 +1470,8 @@ async fn submission_loop(sess: Arc<Session>, config: Arc<Config>, rx_sub: Receiv
handlers::user_input_or_turn(&sess, sub.id.clone(), sub.op, &mut previous_context)
.await;
}
Op::ExecApproval {
id,
decision,
allow_prefix,
} => {
handlers::exec_approval(&sess, id, decision, allow_prefix).await;
Op::ExecApproval { id, decision } => {
handlers::exec_approval(&sess, id, decision).await;
}
Op::PatchApproval { id, decision } => {
handlers::patch_approval(&sess, id, decision).await;
@@ -1672,11 +1668,9 @@ mod handlers {
sess: &Arc<Session>,
id: String,
decision: ReviewDecision,
allow_prefix: Option<Vec<String>>,
) {
if let Some(prefix) = allow_prefix
&& matches!(decision, ReviewDecision::ApprovedAllowPrefix)
&& let Err(err) = sess.persist_command_allow_prefix(&prefix).await
if let ReviewDecision::ApprovedAllowPrefix { allow_prefix } = &decision
&& let Err(err) = sess.persist_command_allow_prefix(allow_prefix).await
{
let message = format!("Failed to update execpolicy allow list: {err}");
tracing::warn!("{message}");

View File

@@ -285,13 +285,7 @@ async fn handle_exec_approval(
)
.await;
let _ = codex
.submit(Op::ExecApproval {
id,
decision,
allow_prefix: None,
})
.await;
let _ = codex.submit(Op::ExecApproval { id, decision }).await;
}
/// Handle an ApplyPatchApprovalRequest by consulting the parent session and replying.

View File

@@ -88,14 +88,14 @@ impl ToolOrchestrator {
};
let decision = tool.start_approval_async(req, approval_ctx).await;
otel.tool_decision(otel_tn, otel_ci, decision, otel_user.clone());
otel.tool_decision(otel_tn, otel_ci, decision.clone(), otel_user.clone());
match decision {
ReviewDecision::Denied | ReviewDecision::Abort => {
return Err(ToolError::Rejected("rejected by user".to_string()));
}
ReviewDecision::Approved
| ReviewDecision::ApprovedAllowPrefix
| ReviewDecision::ApprovedAllowPrefix { .. }
| ReviewDecision::ApprovedForSession => {}
}
already_approved = true;
@@ -171,14 +171,14 @@ impl ToolOrchestrator {
};
let decision = tool.start_approval_async(req, approval_ctx).await;
otel.tool_decision(otel_tn, otel_ci, decision, otel_user);
otel.tool_decision(otel_tn, otel_ci, decision.clone(), otel_user);
match decision {
ReviewDecision::Denied | ReviewDecision::Abort => {
return Err(ToolError::Rejected("rejected by user".to_string()));
}
ReviewDecision::Approved
| ReviewDecision::ApprovedAllowPrefix
| ReviewDecision::ApprovedAllowPrefix { .. }
| ReviewDecision::ApprovedForSession => {}
}
}

View File

@@ -1523,8 +1523,7 @@ async fn run_scenario(scenario: &ScenarioSpec) -> Result<()> {
test.codex
.submit(Op::ExecApproval {
id: "0".into(),
decision: *decision,
allow_prefix: None,
decision: decision.clone(),
})
.await?;
wait_for_completion(&test).await;
@@ -1545,7 +1544,7 @@ async fn run_scenario(scenario: &ScenarioSpec) -> Result<()> {
test.codex
.submit(Op::PatchApproval {
id: "0".into(),
decision: *decision,
decision: decision.clone(),
})
.await?;
wait_for_completion(&test).await;

View File

@@ -95,7 +95,6 @@ async fn codex_delegate_forwards_exec_approval_and_proceeds_on_approval() {
.submit(Op::ExecApproval {
id: "0".into(),
decision: ReviewDecision::Approved,
allow_prefix: None,
})
.await
.expect("submit exec approval");

View File

@@ -843,7 +843,6 @@ async fn handle_container_exec_user_approved_records_tool_decision() {
.submit(Op::ExecApproval {
id: "0".into(),
decision: ReviewDecision::Approved,
allow_prefix: None,
})
.await
.unwrap();
@@ -902,7 +901,6 @@ async fn handle_container_exec_user_approved_for_session_records_tool_decision()
.submit(Op::ExecApproval {
id: "0".into(),
decision: ReviewDecision::ApprovedForSession,
allow_prefix: None,
})
.await
.unwrap();
@@ -961,7 +959,6 @@ async fn handle_sandbox_error_user_approves_retry_records_tool_decision() {
.submit(Op::ExecApproval {
id: "0".into(),
decision: ReviewDecision::Approved,
allow_prefix: None,
})
.await
.unwrap();
@@ -1020,7 +1017,6 @@ async fn handle_container_exec_user_denies_records_tool_decision() {
.submit(Op::ExecApproval {
id: "0".into(),
decision: ReviewDecision::Denied,
allow_prefix: None,
})
.await
.unwrap();
@@ -1079,7 +1075,6 @@ async fn handle_sandbox_error_user_approves_for_session_records_tool_decision()
.submit(Op::ExecApproval {
id: "0".into(),
decision: ReviewDecision::ApprovedForSession,
allow_prefix: None,
})
.await
.unwrap();
@@ -1139,7 +1134,6 @@ async fn handle_sandbox_error_user_denies_records_tool_decision() {
.submit(Op::ExecApproval {
id: "0".into(),
decision: ReviewDecision::Denied,
allow_prefix: None,
})
.await
.unwrap();

View File

@@ -150,7 +150,6 @@ async fn on_exec_approval_response(
.submit(Op::ExecApproval {
id: event_id,
decision: response.decision,
allow_prefix: None,
})
.await
{

View File

@@ -146,9 +146,6 @@ pub enum Op {
id: String,
/// The user's decision in response to the request.
decision: ReviewDecision,
/// When set, persist this prefix to the execpolicy allow list.
#[serde(default, skip_serializing_if = "Option::is_none")]
allow_prefix: Option<Vec<String>>,
},
/// Approve a code patch
@@ -1649,9 +1646,7 @@ pub struct SessionConfiguredEvent {
}
/// User's decision in response to an ExecApprovalRequest.
#[derive(
Debug, Default, Clone, Copy, Deserialize, Serialize, PartialEq, Eq, Display, JsonSchema, TS,
)]
#[derive(Debug, Default, Clone, Deserialize, Serialize, PartialEq, Eq, Display, JsonSchema, TS)]
#[serde(rename_all = "snake_case")]
pub enum ReviewDecision {
/// User has approved this command and the agent should execute it.
@@ -1659,7 +1654,7 @@ pub enum ReviewDecision {
/// User has approved this command and wants to add the command prefix to
/// the execpolicy allow list so future matching commands are permitted.
ApprovedAllowPrefix,
ApprovedAllowPrefix { allow_prefix: Vec<String> },
/// User has approved this command and wants to automatically approve any
/// future identical instances (`command` and `cwd` match exactly) for the

View File

@@ -162,11 +162,17 @@ impl ApprovalOverlay {
};
if let Some(variant) = self.current_variant.as_ref() {
match (variant, &option.decision) {
(ApprovalVariant::Exec { id, command, .. }, ApprovalDecision::Review(decision)) => {
self.handle_exec_decision(id, command, *decision, option.allow_prefix.clone());
(
ApprovalVariant::Exec { id, command, .. },
ApprovalDecision::Review(decision),
) => {
self.handle_exec_decision(id, command, decision.clone());
}
(ApprovalVariant::ApplyPatch { id, .. }, ApprovalDecision::Review(decision)) => {
self.handle_patch_decision(id, *decision);
(
ApprovalVariant::ApplyPatch { id, .. },
ApprovalDecision::Review(decision),
) => {
self.handle_patch_decision(id, decision.clone());
}
(
ApprovalVariant::McpElicitation {
@@ -185,19 +191,14 @@ impl ApprovalOverlay {
self.advance_queue();
}
fn handle_exec_decision(
&self,
id: &str,
command: &[String],
decision: ReviewDecision,
allow_prefix: Option<Vec<String>>,
) {
let cell = history_cell::new_approval_decision_cell(command.to_vec(), decision);
fn handle_exec_decision(&self, id: &str, command: &[String], decision: ReviewDecision) {
let decision_for_history = decision.clone();
let cell =
history_cell::new_approval_decision_cell(command.to_vec(), decision_for_history);
self.app_event_tx.send(AppEvent::InsertHistoryCell(cell));
self.app_event_tx.send(AppEvent::CodexOp(Op::ExecApproval {
id: id.to_string(),
decision,
allow_prefix,
}));
}
@@ -282,7 +283,7 @@ impl BottomPaneView for ApprovalOverlay {
{
match &variant {
ApprovalVariant::Exec { id, command, .. } => {
self.handle_exec_decision(id, command, ReviewDecision::Abort, None);
self.handle_exec_decision(id, command, ReviewDecision::Abort);
}
ApprovalVariant::ApplyPatch { id, .. } => {
self.handle_patch_decision(id, ReviewDecision::Abort);
@@ -467,7 +468,6 @@ struct ApprovalOption {
decision: ApprovalDecision,
display_shortcut: Option<KeyBinding>,
additional_shortcuts: Vec<KeyBinding>,
allow_prefix: Option<Vec<String>>,
}
impl ApprovalOption {
@@ -485,30 +485,26 @@ fn exec_options(allow_prefix: Option<Vec<String>>) -> Vec<ApprovalOption> {
decision: ApprovalDecision::Review(ReviewDecision::Approved),
display_shortcut: None,
additional_shortcuts: vec![key_hint::plain(KeyCode::Char('y'))],
allow_prefix: None,
},
ApprovalOption {
label: "Yes, and don't ask again for this command".to_string(),
decision: ApprovalDecision::Review(ReviewDecision::ApprovedForSession),
display_shortcut: None,
additional_shortcuts: vec![key_hint::plain(KeyCode::Char('a'))],
allow_prefix: None,
},
]
.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),
decision: ApprovalDecision::Review(ReviewDecision::ApprovedAllowPrefix { allow_prefix: prefix }),
display_shortcut: None,
additional_shortcuts: vec![key_hint::plain(KeyCode::Char('p'))],
allow_prefix: Some(prefix),
}))
.chain([ApprovalOption {
label: "No, and tell Codex what to do differently".to_string(),
decision: ApprovalDecision::Review(ReviewDecision::Abort),
display_shortcut: Some(key_hint::plain(KeyCode::Esc)),
additional_shortcuts: vec![key_hint::plain(KeyCode::Char('n'))],
allow_prefix: None,
}])
.collect()
}
@@ -520,14 +516,12 @@ fn patch_options() -> Vec<ApprovalOption> {
decision: ApprovalDecision::Review(ReviewDecision::Approved),
display_shortcut: None,
additional_shortcuts: vec![key_hint::plain(KeyCode::Char('y'))],
allow_prefix: None,
},
ApprovalOption {
label: "No, and tell Codex what to do differently".to_string(),
decision: ApprovalDecision::Review(ReviewDecision::Abort),
display_shortcut: Some(key_hint::plain(KeyCode::Esc)),
additional_shortcuts: vec![key_hint::plain(KeyCode::Char('n'))],
allow_prefix: None,
},
]
}
@@ -539,21 +533,18 @@ fn elicitation_options() -> Vec<ApprovalOption> {
decision: ApprovalDecision::McpElicitation(ElicitationAction::Accept),
display_shortcut: None,
additional_shortcuts: vec![key_hint::plain(KeyCode::Char('y'))],
allow_prefix: None,
},
ApprovalOption {
label: "No, but continue without it".to_string(),
decision: ApprovalDecision::McpElicitation(ElicitationAction::Decline),
display_shortcut: None,
additional_shortcuts: vec![key_hint::plain(KeyCode::Char('n'))],
allow_prefix: None,
},
ApprovalOption {
label: "Cancel this request".to_string(),
decision: ApprovalDecision::McpElicitation(ElicitationAction::Cancel),
display_shortcut: Some(key_hint::plain(KeyCode::Esc)),
additional_shortcuts: vec![key_hint::plain(KeyCode::Char('c'))],
allow_prefix: None,
},
]
}
@@ -621,14 +612,13 @@ mod tests {
view.handle_key_event(KeyEvent::new(KeyCode::Char('p'), KeyModifiers::NONE));
let mut saw_op = false;
while let Ok(ev) = rx.try_recv() {
if let AppEvent::CodexOp(Op::ExecApproval {
allow_prefix,
decision,
..
}) = ev
{
assert_eq!(decision, ReviewDecision::ApprovedAllowPrefix);
assert_eq!(allow_prefix, Some(vec!["echo".to_string()]));
if let AppEvent::CodexOp(Op::ExecApproval { decision, .. }) = ev {
assert_eq!(
decision,
ReviewDecision::ApprovedAllowPrefix {
allow_prefix: vec!["echo".to_string()]
}
);
saw_op = true;
break;
}

View File

@@ -408,7 +408,7 @@ pub fn new_approval_decision_cell(
],
)
}
ApprovedAllowPrefix => {
ApprovedAllowPrefix { .. } => {
let snippet = Span::from(exec_snippet(&command)).dim();
(
"".green(),