fix(tui): simplify approval confirmation flow
Harvested from PR #2143 by @reidliu41. Co-authored-by: reidliu41 <reid201711@gmail.com>
This commit is contained in:
+52
-167
@@ -16,10 +16,9 @@
|
||||
//! `2` / `a` approves for the session.
|
||||
//! - **Destructive** (`RiskLevel::Destructive`) — file writes, shell,
|
||||
//! patches, MCP actions, unclassified tools, and any "fetch arbitrary
|
||||
//! content" surface. The first approve press *stages* a decision and
|
||||
//! the second matching press commits — muscle-memory `Enter` cannot
|
||||
//! accidentally land on an approval. Any non-approve key clears the
|
||||
//! staging and keeps the user in selection mode.
|
||||
//! content" surface. The takeover keeps the destructive badge and
|
||||
//! impact summary visible, then lets `Enter` commit the highlighted
|
||||
//! option or `y` / `a` / `d` commit directly.
|
||||
//!
|
||||
//! The decision events emitted upstream are unchanged
|
||||
//! (`ViewEvent::ApprovalDecision`), so `ui.rs` and the engine handle
|
||||
@@ -102,8 +101,8 @@ pub enum ToolCategory {
|
||||
/// Stakes-based variant for the takeover modal.
|
||||
///
|
||||
/// `RiskLevel::Benign` lets a single keystroke commit the approval.
|
||||
/// `RiskLevel::Destructive` requires an explicit second confirmation
|
||||
/// keypress so muscle-memory `Enter` never lands on an irreversible op.
|
||||
/// `RiskLevel::Destructive` keeps stronger warning copy and styling
|
||||
/// around approvals that can touch files, shell, or remote state.
|
||||
///
|
||||
/// Routing rules live in [`classify_risk`] — when in doubt, route to
|
||||
/// `Destructive`.
|
||||
@@ -228,13 +227,12 @@ pub fn get_tool_category(name: &str) -> ToolCategory {
|
||||
/// The bias is conservative: a category we don't recognise routes to
|
||||
/// `Destructive`, and any shell command that `command_safety` flags as
|
||||
/// `Dangerous` is forced to `Destructive` even when the rest of the
|
||||
/// request looks calm. The split lets the modal swap muscle-memory
|
||||
/// approval for an explicit two-key confirmation on anything that can
|
||||
/// touch state outside this turn.
|
||||
/// request looks calm. The split lets the modal render stronger warning
|
||||
/// copy on anything that can touch state outside this turn.
|
||||
#[must_use]
|
||||
pub fn classify_risk(tool_name: &str, category: ToolCategory, params: &Value) -> RiskLevel {
|
||||
match category {
|
||||
// Read paths and discovery — never staged.
|
||||
// Read paths and discovery.
|
||||
ToolCategory::Safe | ToolCategory::McpRead => RiskLevel::Benign,
|
||||
// Query-only network is benign; opening a URL pulls arbitrary
|
||||
// remote content, so it stays destructive.
|
||||
@@ -448,9 +446,7 @@ fn build_impact_summary_zh_hans(
|
||||
}
|
||||
}
|
||||
|
||||
/// Indices into the option list shared by both variants. Visible to
|
||||
/// the widget module so it can render the staged-confirmation banner
|
||||
/// without re-deriving the variant from the request.
|
||||
/// Indices into the option list shared by both variants.
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
||||
pub enum ApprovalOption {
|
||||
ApproveOnce,
|
||||
@@ -486,16 +482,6 @@ impl ApprovalOption {
|
||||
ApprovalOption::Abort => ReviewDecision::Abort,
|
||||
}
|
||||
}
|
||||
|
||||
/// Whether this option needs an explicit second-key confirmation in
|
||||
/// the destructive variant. Deny/Abort are never staged.
|
||||
fn requires_confirm(self, risk: RiskLevel) -> bool {
|
||||
matches!(risk, RiskLevel::Destructive)
|
||||
&& matches!(
|
||||
self,
|
||||
ApprovalOption::ApproveOnce | ApprovalOption::ApproveAlways
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
/// Approval overlay state managed by the modal view stack
|
||||
@@ -504,10 +490,6 @@ pub struct ApprovalView {
|
||||
request: ApprovalRequest,
|
||||
selected: usize,
|
||||
locale: Locale,
|
||||
/// When `Some`, the destructive variant has staged this approval and
|
||||
/// is waiting for the user to press the same key (or `Enter`) again.
|
||||
/// Any other key clears the staging.
|
||||
pending_confirm: Option<ApprovalOption>,
|
||||
timeout: Option<Duration>,
|
||||
requested_at: Instant,
|
||||
/// Whether the approval card is collapsed to a single-line banner.
|
||||
@@ -525,7 +507,6 @@ impl ApprovalView {
|
||||
request,
|
||||
selected: 0,
|
||||
locale,
|
||||
pending_confirm: None,
|
||||
timeout: None,
|
||||
requested_at: Instant::now(),
|
||||
collapsed: false,
|
||||
@@ -534,22 +515,17 @@ impl ApprovalView {
|
||||
|
||||
fn select_prev(&mut self) {
|
||||
self.selected = self.selected.saturating_sub(1);
|
||||
// Moving the selection abandons any staged confirmation; the
|
||||
// user is reconsidering.
|
||||
self.pending_confirm = None;
|
||||
}
|
||||
|
||||
fn select_next(&mut self) {
|
||||
self.selected = (self.selected + 1).min(ApprovalOption::ORDER.len() - 1);
|
||||
self.pending_confirm = None;
|
||||
}
|
||||
|
||||
fn current_option(&self) -> ApprovalOption {
|
||||
ApprovalOption::from_index(self.selected)
|
||||
}
|
||||
|
||||
/// Test-only accessor — the widget reads decisions through
|
||||
/// `commit_or_stage` instead of polling.
|
||||
/// Test-only accessor for the selected option's decision.
|
||||
#[cfg(test)]
|
||||
fn current_decision(&self) -> ReviewDecision {
|
||||
self.current_option().decision()
|
||||
@@ -566,33 +542,13 @@ impl ApprovalView {
|
||||
self.request.risk
|
||||
}
|
||||
|
||||
/// The staged option, if any. `None` in the benign variant or when
|
||||
/// no approve key has been pressed yet.
|
||||
pub(crate) fn pending_confirm(&self) -> Option<ApprovalOption> {
|
||||
self.pending_confirm
|
||||
}
|
||||
|
||||
pub(crate) fn locale(&self) -> Locale {
|
||||
self.locale
|
||||
}
|
||||
|
||||
/// Try to commit (or stage) the given option respecting the
|
||||
/// variant's confirmation policy. Returns the action the modal
|
||||
/// stack should apply.
|
||||
fn commit_or_stage(&mut self, option: ApprovalOption) -> ViewAction {
|
||||
if option.requires_confirm(self.request.risk) {
|
||||
// Two-step destructive flow: first press stages, second
|
||||
// press of the same option commits.
|
||||
if self.pending_confirm == Some(option) {
|
||||
self.pending_confirm = None;
|
||||
return self.emit_decision(option.decision(), false);
|
||||
}
|
||||
self.pending_confirm = Some(option);
|
||||
self.selected = option.index();
|
||||
return ViewAction::None;
|
||||
}
|
||||
// Benign variant or non-approve options commit immediately.
|
||||
self.pending_confirm = None;
|
||||
/// Commit the given option and close the approval modal.
|
||||
fn commit_option(&mut self, option: ApprovalOption) -> ViewAction {
|
||||
self.selected = option.index();
|
||||
self.emit_decision(option.decision(), false)
|
||||
}
|
||||
|
||||
@@ -647,31 +603,23 @@ impl ModalView for ApprovalView {
|
||||
self.select_next();
|
||||
ViewAction::None
|
||||
}
|
||||
KeyCode::Enter => self.commit_or_stage(self.current_option()),
|
||||
KeyCode::Enter => self.commit_option(self.current_option()),
|
||||
// Direct shortcuts; '1' / '2' map to the first two options
|
||||
// so a numeric pad still works for benign approve flows.
|
||||
// so a numeric pad still works for approve flows.
|
||||
KeyCode::Char('y') | KeyCode::Char('Y') | KeyCode::Char('1') => {
|
||||
self.commit_or_stage(ApprovalOption::ApproveOnce)
|
||||
self.commit_option(ApprovalOption::ApproveOnce)
|
||||
}
|
||||
KeyCode::Char('a') | KeyCode::Char('A') | KeyCode::Char('2') => {
|
||||
self.commit_or_stage(ApprovalOption::ApproveAlways)
|
||||
self.commit_option(ApprovalOption::ApproveAlways)
|
||||
}
|
||||
KeyCode::Char('n')
|
||||
| KeyCode::Char('N')
|
||||
| KeyCode::Char('d')
|
||||
| KeyCode::Char('D')
|
||||
| KeyCode::Char('3') => self.commit_or_stage(ApprovalOption::Deny),
|
||||
KeyCode::Char('v') | KeyCode::Char('V') => {
|
||||
self.pending_confirm = None;
|
||||
self.emit_params_pager()
|
||||
}
|
||||
| KeyCode::Char('3') => self.commit_option(ApprovalOption::Deny),
|
||||
KeyCode::Char('v') | KeyCode::Char('V') => self.emit_params_pager(),
|
||||
KeyCode::Esc => self.emit_decision(ReviewDecision::Abort, false),
|
||||
_ => {
|
||||
// Any unrecognised key cancels a staged confirmation —
|
||||
// the user is no longer aiming at "approve".
|
||||
self.pending_confirm = None;
|
||||
ViewAction::None
|
||||
}
|
||||
_ => ViewAction::None,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1030,13 +978,13 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn risk_query_only_network_is_benign_but_fetch_is_destructive() {
|
||||
// web_search is read-only enough to skip the two-key dance.
|
||||
// web_search is read-only enough to use the benign variant.
|
||||
let cat = ToolCategory::Network;
|
||||
assert_eq!(
|
||||
classify_risk("web_search", cat, &json!({"q": "rust"})),
|
||||
RiskLevel::Benign
|
||||
);
|
||||
// fetch_url pulls arbitrary remote content; never staged.
|
||||
// fetch_url pulls arbitrary remote content, so it stays destructive.
|
||||
assert_eq!(
|
||||
classify_risk("fetch_url", cat, &json!({"url": "https://example.com"})),
|
||||
RiskLevel::Destructive
|
||||
@@ -1163,7 +1111,6 @@ mod tests {
|
||||
let view = ApprovalView::new(benign_request());
|
||||
assert_eq!(view.selected, 0);
|
||||
assert!(view.timeout.is_none());
|
||||
assert_eq!(view.pending_confirm(), None);
|
||||
assert_eq!(view.risk(), RiskLevel::Benign);
|
||||
}
|
||||
|
||||
@@ -1376,7 +1323,7 @@ mod tests {
|
||||
}
|
||||
|
||||
// ========================================================================
|
||||
// ApprovalView Tests — Destructive Variant (two-key confirm)
|
||||
// ApprovalView Tests — Destructive Variant (one-step approve with warning)
|
||||
// ========================================================================
|
||||
|
||||
#[test]
|
||||
@@ -1386,16 +1333,10 @@ mod tests {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn destructive_y_first_press_stages_then_second_commits() {
|
||||
fn destructive_y_first_press_approves_once() {
|
||||
for code in [KeyCode::Char('y'), KeyCode::Char('Y')] {
|
||||
let mut view = ApprovalView::new(destructive_request());
|
||||
|
||||
// First press stages — no decision emitted yet.
|
||||
let action = view.handle_key(create_key_event(code));
|
||||
assert!(matches!(action, ViewAction::None));
|
||||
assert_eq!(view.pending_confirm(), Some(ApprovalOption::ApproveOnce));
|
||||
|
||||
// Second press of the same key commits.
|
||||
let action = view.handle_key(create_key_event(code));
|
||||
assert!(
|
||||
matches!(
|
||||
@@ -1411,15 +1352,10 @@ mod tests {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn destructive_enter_first_press_stages_then_second_commits() {
|
||||
fn destructive_enter_approves_selected_option() {
|
||||
let mut view = ApprovalView::new(destructive_request());
|
||||
|
||||
// Selection starts at ApproveOnce — Enter stages.
|
||||
let action = view.handle_key(create_key_event(KeyCode::Enter));
|
||||
assert!(matches!(action, ViewAction::None));
|
||||
assert_eq!(view.pending_confirm(), Some(ApprovalOption::ApproveOnce));
|
||||
|
||||
// Second Enter on the same selection commits.
|
||||
// Selection starts at ApproveOnce — Enter commits the selected option.
|
||||
let action = view.handle_key(create_key_event(KeyCode::Enter));
|
||||
assert!(matches!(
|
||||
action,
|
||||
@@ -1431,39 +1367,33 @@ mod tests {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn destructive_navigation_clears_staged_confirmation() {
|
||||
fn destructive_navigation_then_enter_commits_highlighted_option() {
|
||||
let mut view = ApprovalView::new(destructive_request());
|
||||
|
||||
view.handle_key(create_key_event(KeyCode::Char('y')));
|
||||
assert_eq!(view.pending_confirm(), Some(ApprovalOption::ApproveOnce));
|
||||
|
||||
// Moving the selection abandons the staging.
|
||||
view.handle_key(create_key_event(KeyCode::Down));
|
||||
assert_eq!(view.pending_confirm(), None);
|
||||
let action = view.handle_key(create_key_event(KeyCode::Enter));
|
||||
assert!(matches!(
|
||||
action,
|
||||
ViewAction::EmitAndClose(ViewEvent::ApprovalDecision {
|
||||
decision: ReviewDecision::ApprovedForSession,
|
||||
..
|
||||
})
|
||||
));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn destructive_unrelated_key_clears_staged_confirmation() {
|
||||
fn destructive_unrelated_key_keeps_modal_open() {
|
||||
let mut view = ApprovalView::new(destructive_request());
|
||||
|
||||
view.handle_key(create_key_event(KeyCode::Char('y')));
|
||||
assert_eq!(view.pending_confirm(), Some(ApprovalOption::ApproveOnce));
|
||||
|
||||
// A key with no mapped action clears the staging.
|
||||
let action = view.handle_key(create_key_event(KeyCode::Char('q')));
|
||||
assert!(matches!(action, ViewAction::None));
|
||||
assert_eq!(view.pending_confirm(), None);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn destructive_a_first_press_stages_then_second_commits_session() {
|
||||
fn destructive_a_first_press_approves_for_session() {
|
||||
for code in [KeyCode::Char('a'), KeyCode::Char('A')] {
|
||||
let mut view = ApprovalView::new(destructive_request());
|
||||
|
||||
let action = view.handle_key(create_key_event(code));
|
||||
assert!(matches!(action, ViewAction::None));
|
||||
assert_eq!(view.pending_confirm(), Some(ApprovalOption::ApproveAlways));
|
||||
|
||||
let action = view.handle_key(create_key_event(code));
|
||||
assert!(
|
||||
matches!(
|
||||
@@ -1479,23 +1409,8 @@ mod tests {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn destructive_y_then_a_does_not_commit_either() {
|
||||
// Pressing 'y' then 'a' must NOT commit ApproveAlways — the
|
||||
// second key is a different option, so it re-stages instead.
|
||||
let mut view = ApprovalView::new(destructive_request());
|
||||
|
||||
let action = view.handle_key(create_key_event(KeyCode::Char('y')));
|
||||
assert!(matches!(action, ViewAction::None));
|
||||
assert_eq!(view.pending_confirm(), Some(ApprovalOption::ApproveOnce));
|
||||
|
||||
let action = view.handle_key(create_key_event(KeyCode::Char('a')));
|
||||
assert!(matches!(action, ViewAction::None));
|
||||
assert_eq!(view.pending_confirm(), Some(ApprovalOption::ApproveAlways));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn destructive_deny_does_not_require_confirmation() {
|
||||
// Deny / Abort skip the two-key dance — the user is bailing.
|
||||
fn destructive_deny_commits_immediately() {
|
||||
// Deny commits immediately — the user is rejecting the tool.
|
||||
for code in [
|
||||
KeyCode::Char('n'),
|
||||
KeyCode::Char('N'),
|
||||
@@ -1520,9 +1435,6 @@ mod tests {
|
||||
#[test]
|
||||
fn destructive_esc_aborts_immediately() {
|
||||
let mut view = ApprovalView::new(destructive_request());
|
||||
// Stage something first.
|
||||
view.handle_key(create_key_event(KeyCode::Char('y')));
|
||||
// Esc still aborts in one press.
|
||||
let action = view.handle_key(create_key_event(KeyCode::Esc));
|
||||
assert!(matches!(
|
||||
action,
|
||||
@@ -1557,20 +1469,21 @@ mod tests {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn render_benign_includes_review_badge_and_one_step_hint() {
|
||||
fn render_benign_includes_review_badge_and_selection_hint() {
|
||||
let view = ApprovalView::new(benign_request());
|
||||
let lines = render_lines(&view, 100, 40);
|
||||
let joined = lines.join("\n");
|
||||
assert!(joined.contains("REVIEW"), "missing REVIEW badge:\n{joined}");
|
||||
assert!(joined.contains("Choose"), "benign hint missing:\n{joined}");
|
||||
assert!(
|
||||
joined.contains("Single key approves"),
|
||||
"benign hint missing:\n{joined}"
|
||||
joined.contains("Enter selected option"),
|
||||
"benign selection hint missing:\n{joined}"
|
||||
);
|
||||
assert!(joined.contains("read_file"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn render_destructive_shows_warning_badge_and_two_step_hint() {
|
||||
fn render_destructive_shows_warning_badge_and_one_step_hint() {
|
||||
let view = ApprovalView::new(destructive_request());
|
||||
let lines = render_lines(&view, 100, 40);
|
||||
let joined = lines.join("\n");
|
||||
@@ -1579,31 +1492,15 @@ mod tests {
|
||||
"missing DESTRUCTIVE badge:\n{joined}"
|
||||
);
|
||||
assert!(
|
||||
joined.contains("Two keys to approve"),
|
||||
joined.contains("Enter selected option"),
|
||||
"destructive hint missing:\n{joined}"
|
||||
);
|
||||
assert!(joined.contains("write_file"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn render_destructive_after_stage_shows_confirm_banner() {
|
||||
let mut view = ApprovalView::new(destructive_request());
|
||||
view.handle_key(create_key_event(KeyCode::Char('y')));
|
||||
let lines = render_lines(&view, 100, 40);
|
||||
let joined = lines.join("\n");
|
||||
assert!(
|
||||
joined.contains("Confirm destructive action"),
|
||||
"confirm banner missing:\n{joined}"
|
||||
);
|
||||
assert!(
|
||||
joined.contains("(staged)"),
|
||||
"stage marker missing:\n{joined}"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn render_destructive_zh_hans_localizes_security_copy() {
|
||||
let mut view = ApprovalView::new_for_locale(destructive_request(), Locale::ZhHans);
|
||||
let view = ApprovalView::new_for_locale(destructive_request(), Locale::ZhHans);
|
||||
let lines = render_lines(&view, 100, 40);
|
||||
let joined = compact_rendered_text(&lines);
|
||||
assert!(
|
||||
@@ -1611,8 +1508,12 @@ mod tests {
|
||||
"missing zh risk badge:\n{joined}"
|
||||
);
|
||||
assert!(
|
||||
joined.contains("两次按键确认"),
|
||||
"missing zh two-step hint:\n{joined}"
|
||||
joined.contains("选择:"),
|
||||
"missing zh selection prefix:\n{joined}"
|
||||
);
|
||||
assert!(
|
||||
joined.contains("Enter执行选中项,或直接按y/a/d"),
|
||||
"missing zh one-step hint:\n{joined}"
|
||||
);
|
||||
assert!(
|
||||
joined.contains("文件写入"),
|
||||
@@ -1630,22 +1531,6 @@ mod tests {
|
||||
joined.contains("仅本次批准"),
|
||||
"missing zh approve option:\n{joined}"
|
||||
);
|
||||
|
||||
view.handle_key(create_key_event(KeyCode::Char('y')));
|
||||
let lines = render_lines(&view, 100, 40);
|
||||
let joined = compact_rendered_text(&lines);
|
||||
assert!(
|
||||
joined.contains("确认破坏性操作"),
|
||||
"missing zh confirm banner:\n{joined}"
|
||||
);
|
||||
assert!(
|
||||
joined.contains("(待确认)"),
|
||||
"missing zh staged marker:\n{joined}"
|
||||
);
|
||||
assert!(
|
||||
joined.contains("Enter或y"),
|
||||
"missing zh confirm key:\n{joined}"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
+59
-25
@@ -6036,31 +6036,19 @@ async fn handle_view_events(
|
||||
approval_key,
|
||||
approval_grouping_key,
|
||||
} => {
|
||||
if decision == ReviewDecision::ApprovedForSession {
|
||||
// Store the tool name (backward compat) and the lossy
|
||||
// grouping key so later flag variants of the same
|
||||
// command family are also auto-approved (v0.8.37).
|
||||
app.approval_session_approved.insert(tool_name.clone());
|
||||
app.approval_session_approved
|
||||
.insert(approval_grouping_key.clone());
|
||||
}
|
||||
|
||||
match decision {
|
||||
ReviewDecision::Approved | ReviewDecision::ApprovedForSession => {
|
||||
let _ = engine_handle.approve_tool_call(tool_id).await;
|
||||
}
|
||||
ReviewDecision::Denied | ReviewDecision::Abort => {
|
||||
// Cache the denial so the model retry-loop doesn't
|
||||
// re-prompt for the exact same approval_key (#360).
|
||||
// Only the key (per-call unique) is stored — NOT
|
||||
// the tool_name, which would block all future
|
||||
// invocations of the same tool type (#1377).
|
||||
if !timed_out {
|
||||
app.approval_session_denied.insert(approval_key);
|
||||
}
|
||||
let _ = engine_handle.deny_tool_call(tool_id).await;
|
||||
}
|
||||
}
|
||||
apply_approval_decision(
|
||||
app,
|
||||
engine_handle,
|
||||
ApprovalDecisionEvent {
|
||||
tool_id,
|
||||
tool_name,
|
||||
decision,
|
||||
timed_out,
|
||||
approval_key,
|
||||
approval_grouping_key,
|
||||
},
|
||||
)
|
||||
.await;
|
||||
|
||||
if timed_out {
|
||||
app.add_message(HistoryCell::System {
|
||||
@@ -6333,6 +6321,52 @@ async fn handle_view_events(
|
||||
Ok(false)
|
||||
}
|
||||
|
||||
struct ApprovalDecisionEvent {
|
||||
tool_id: String,
|
||||
tool_name: String,
|
||||
decision: ReviewDecision,
|
||||
timed_out: bool,
|
||||
approval_key: String,
|
||||
approval_grouping_key: String,
|
||||
}
|
||||
|
||||
async fn apply_approval_decision(
|
||||
app: &mut App,
|
||||
engine_handle: &mut EngineHandle,
|
||||
event: ApprovalDecisionEvent,
|
||||
) {
|
||||
if event.decision == ReviewDecision::ApprovedForSession {
|
||||
// Store the tool name (backward compat) and the lossy grouping key so
|
||||
// later flag variants of the same command family are also auto-approved
|
||||
// (v0.8.37).
|
||||
app.approval_session_approved
|
||||
.insert(event.tool_name.clone());
|
||||
app.approval_session_approved
|
||||
.insert(event.approval_grouping_key.clone());
|
||||
}
|
||||
|
||||
match event.decision {
|
||||
ReviewDecision::Approved | ReviewDecision::ApprovedForSession => {
|
||||
let _ = engine_handle.approve_tool_call(event.tool_id).await;
|
||||
}
|
||||
ReviewDecision::Denied => {
|
||||
// Cache the denial so the model retry-loop doesn't re-prompt for
|
||||
// the exact same approval_key (#360). Only the key (per-call
|
||||
// unique) is stored — NOT the tool_name, which would block all
|
||||
// future invocations of the same tool type (#1377).
|
||||
if !event.timed_out {
|
||||
app.approval_session_denied.insert(event.approval_key);
|
||||
}
|
||||
let _ = engine_handle.deny_tool_call(event.tool_id).await;
|
||||
}
|
||||
ReviewDecision::Abort => {
|
||||
engine_handle.cancel();
|
||||
mark_active_turn_cancelled_locally(app);
|
||||
app.status_message = Some("Request cancelled".to_string());
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
fn mark_active_turn_cancelled_locally(app: &mut App) {
|
||||
app.is_loading = false;
|
||||
app.dispatch_started_at = None;
|
||||
|
||||
@@ -1015,13 +1015,10 @@ impl Renderable for ComposerWidget<'_> {
|
||||
|
||||
/// Codex-style full-screen approval takeover (#129).
|
||||
///
|
||||
/// The widget reads its mutable state (selected option, staged
|
||||
/// confirmation) directly from the [`ApprovalView`] so the destructive
|
||||
/// variant can render its "Press Y again to confirm" banner without
|
||||
/// touching internal fields. Rendering reflows to fill most of the
|
||||
/// transcript area instead of a centered popup; on small terminals it
|
||||
/// falls back to a 65×22 card so existing snapshot tests still see a
|
||||
/// coherent layout.
|
||||
/// The widget reads its selected option and locale directly from the
|
||||
/// [`ApprovalView`]. Rendering reflows to fill most of the transcript
|
||||
/// area instead of a centered popup; on small terminals it falls back to
|
||||
/// a 65×22 card so existing snapshot tests still see a coherent layout.
|
||||
pub struct ApprovalWidget<'a> {
|
||||
request: &'a ApprovalRequest,
|
||||
view: &'a ApprovalView,
|
||||
@@ -1038,8 +1035,8 @@ impl<'a> ApprovalWidget<'a> {
|
||||
/// terminal can hold.
|
||||
const APPROVAL_CARD_HORIZONTAL_PAD: u16 = 6;
|
||||
const APPROVAL_CARD_VERTICAL_PAD: u16 = 2;
|
||||
/// Minimum card height — anything tighter and the destructive variant's
|
||||
/// confirmation banner overlaps the option list.
|
||||
/// Minimum card height — anything tighter and the approval controls
|
||||
/// overlap the option list.
|
||||
const APPROVAL_CARD_MIN_HEIGHT: u16 = 18;
|
||||
/// Minimum card width — anything tighter makes approval copy wrap too
|
||||
/// aggressively on small terminals.
|
||||
@@ -1164,120 +1161,49 @@ impl Renderable for ApprovalWidget<'_> {
|
||||
lines.push(Line::from(""));
|
||||
|
||||
let options = approval_options_for(risk, locale);
|
||||
let pending = self.view.pending_confirm();
|
||||
|
||||
for (i, opt) in options.iter().enumerate() {
|
||||
let is_selected = i == self.view.selected();
|
||||
let staged = pending.is_some_and(|p| p == opt.option);
|
||||
let label_color = if opt.dangerous {
|
||||
palette_colors.accent
|
||||
} else {
|
||||
palette::TEXT_BODY
|
||||
};
|
||||
|
||||
let row_style = if is_selected {
|
||||
Style::default()
|
||||
.fg(palette::SELECTION_TEXT)
|
||||
.bg(palette::SELECTION_BG)
|
||||
} else {
|
||||
Style::default()
|
||||
};
|
||||
let option_style = approval_option_style(is_selected, label_color);
|
||||
let shortcut_style = approval_option_style(is_selected, palette_colors.shortcut);
|
||||
|
||||
let mut spans = vec![
|
||||
let spans = vec![
|
||||
Span::raw(" "),
|
||||
Span::styled(
|
||||
format!("[{}] ", opt.key_hint),
|
||||
Style::default()
|
||||
.fg(palette_colors.shortcut)
|
||||
.add_modifier(Modifier::BOLD),
|
||||
shortcut_style.add_modifier(Modifier::BOLD),
|
||||
),
|
||||
Span::styled(opt.label.to_string(), row_style.fg(label_color)),
|
||||
Span::styled(opt.label.to_string(), option_style),
|
||||
];
|
||||
if staged {
|
||||
spans.push(Span::raw(" "));
|
||||
spans.push(Span::styled(
|
||||
staged_marker(locale),
|
||||
Style::default()
|
||||
.fg(palette_colors.accent)
|
||||
.add_modifier(Modifier::BOLD),
|
||||
));
|
||||
}
|
||||
lines.push(Line::from(spans));
|
||||
}
|
||||
|
||||
// Variant-specific footer: benign nudges single-key approve;
|
||||
// destructive shows either the standing prompt or the
|
||||
// confirmation banner when an approve key has been staged.
|
||||
// Footer: Enter commits the highlighted row; y/a/d remain direct
|
||||
// shortcuts for users who do not want to move the selection.
|
||||
lines.push(Line::from(""));
|
||||
match (risk, pending) {
|
||||
(RiskLevel::Benign, _) => {
|
||||
lines.push(Line::from(vec![
|
||||
Span::raw(" "),
|
||||
Span::styled(
|
||||
single_key_prefix(locale),
|
||||
Style::default().fg(palette::TEXT_HINT),
|
||||
),
|
||||
Span::styled(
|
||||
single_key_value(locale),
|
||||
Style::default()
|
||||
.fg(palette_colors.accent)
|
||||
.add_modifier(Modifier::BOLD),
|
||||
),
|
||||
Span::styled(
|
||||
footer_controls(locale),
|
||||
Style::default().fg(palette::TEXT_HINT),
|
||||
),
|
||||
]));
|
||||
}
|
||||
(RiskLevel::Destructive, Some(opt)) => {
|
||||
let again_key = match opt {
|
||||
crate::tui::approval::ApprovalOption::ApproveOnce => confirm_key_once(locale),
|
||||
crate::tui::approval::ApprovalOption::ApproveAlways => {
|
||||
confirm_key_always(locale)
|
||||
}
|
||||
_ => "Enter",
|
||||
};
|
||||
lines.push(Line::from(vec![
|
||||
Span::raw(" "),
|
||||
Span::styled(
|
||||
destructive_confirm_prefix(locale),
|
||||
Style::default()
|
||||
.fg(palette_colors.accent)
|
||||
.add_modifier(Modifier::BOLD),
|
||||
),
|
||||
Span::styled(
|
||||
again_key.to_string(),
|
||||
Style::default()
|
||||
.fg(palette::DEEPSEEK_INK)
|
||||
.bg(palette_colors.accent)
|
||||
.add_modifier(Modifier::BOLD),
|
||||
),
|
||||
Span::styled(
|
||||
destructive_confirm_suffix(locale),
|
||||
Style::default().fg(palette::TEXT_HINT),
|
||||
),
|
||||
]));
|
||||
}
|
||||
(RiskLevel::Destructive, None) => {
|
||||
lines.push(Line::from(vec![
|
||||
Span::raw(" "),
|
||||
Span::styled(
|
||||
two_key_prefix(locale),
|
||||
Style::default().fg(palette::TEXT_HINT),
|
||||
),
|
||||
Span::styled(
|
||||
two_key_value(locale),
|
||||
Style::default()
|
||||
.fg(palette_colors.accent)
|
||||
.add_modifier(Modifier::BOLD),
|
||||
),
|
||||
Span::styled(
|
||||
footer_controls(locale),
|
||||
Style::default().fg(palette::TEXT_HINT),
|
||||
),
|
||||
]));
|
||||
}
|
||||
}
|
||||
lines.push(Line::from(vec![
|
||||
Span::raw(" "),
|
||||
Span::styled(
|
||||
selection_hint_prefix(locale),
|
||||
Style::default().fg(palette::TEXT_HINT),
|
||||
),
|
||||
Span::styled(
|
||||
selection_hint_value(locale),
|
||||
Style::default()
|
||||
.fg(palette_colors.accent)
|
||||
.add_modifier(Modifier::BOLD),
|
||||
),
|
||||
Span::styled(
|
||||
footer_controls(locale),
|
||||
Style::default().fg(palette::TEXT_HINT),
|
||||
),
|
||||
]));
|
||||
|
||||
let title = format!(
|
||||
" {} {} — {} ",
|
||||
@@ -1375,6 +1301,21 @@ fn approval_palette(risk: RiskLevel) -> ApprovalColors {
|
||||
}
|
||||
}
|
||||
|
||||
fn approval_selected_style() -> Style {
|
||||
Style::default()
|
||||
.fg(palette::SELECTION_TEXT)
|
||||
.bg(palette::DEEPSEEK_BLUE)
|
||||
.add_modifier(Modifier::BOLD)
|
||||
}
|
||||
|
||||
fn approval_option_style(is_selected: bool, color: Color) -> Style {
|
||||
if is_selected {
|
||||
approval_selected_style()
|
||||
} else {
|
||||
Style::default().fg(color)
|
||||
}
|
||||
}
|
||||
|
||||
fn risk_badge_text(risk: RiskLevel, locale: Locale) -> &'static str {
|
||||
match (locale, risk) {
|
||||
(Locale::ZhHans, RiskLevel::Benign) => "审查",
|
||||
@@ -1438,24 +1379,6 @@ fn label_params(locale: Locale) -> &'static str {
|
||||
}
|
||||
}
|
||||
|
||||
fn staged_marker(locale: Locale) -> &'static str {
|
||||
match locale {
|
||||
Locale::ZhHans => "(待确认)",
|
||||
_ => "(staged)",
|
||||
}
|
||||
}
|
||||
|
||||
fn single_key_prefix(locale: Locale) -> &'static str {
|
||||
match locale {
|
||||
Locale::ZhHans => "单键批准:",
|
||||
_ => "Single key approves: ",
|
||||
}
|
||||
}
|
||||
|
||||
fn single_key_value(_locale: Locale) -> &'static str {
|
||||
"Enter / 1 / y"
|
||||
}
|
||||
|
||||
fn footer_controls(locale: Locale) -> &'static str {
|
||||
match locale {
|
||||
Locale::ZhHans => " · v:完整参数 · Esc:终止",
|
||||
@@ -1463,79 +1386,45 @@ fn footer_controls(locale: Locale) -> &'static str {
|
||||
}
|
||||
}
|
||||
|
||||
fn destructive_confirm_prefix(locale: Locale) -> &'static str {
|
||||
fn selection_hint_prefix(locale: Locale) -> &'static str {
|
||||
match locale {
|
||||
Locale::ZhHans => "确认破坏性操作:再次按 ",
|
||||
_ => "Confirm destructive action — press ",
|
||||
Locale::ZhHans => "选择:",
|
||||
_ => "Choose: ",
|
||||
}
|
||||
}
|
||||
|
||||
fn destructive_confirm_suffix(locale: Locale) -> &'static str {
|
||||
fn selection_hint_value(locale: Locale) -> &'static str {
|
||||
match locale {
|
||||
Locale::ZhHans => " 执行;按其他键取消。",
|
||||
_ => " again to commit, anything else cancels.",
|
||||
}
|
||||
}
|
||||
|
||||
fn confirm_key_once(locale: Locale) -> &'static str {
|
||||
match locale {
|
||||
Locale::ZhHans => "Enter 或 y",
|
||||
_ => "Enter or y",
|
||||
}
|
||||
}
|
||||
|
||||
fn confirm_key_always(locale: Locale) -> &'static str {
|
||||
match locale {
|
||||
Locale::ZhHans => "Enter 或 a",
|
||||
_ => "Enter or a",
|
||||
}
|
||||
}
|
||||
|
||||
fn two_key_prefix(locale: Locale) -> &'static str {
|
||||
match locale {
|
||||
Locale::ZhHans => "两次按键确认:",
|
||||
_ => "Two keys to approve: ",
|
||||
}
|
||||
}
|
||||
|
||||
fn two_key_value(locale: Locale) -> &'static str {
|
||||
match locale {
|
||||
Locale::ZhHans => "先按 y/a,再按一次 y/a",
|
||||
_ => "y/a then y/a again",
|
||||
Locale::ZhHans => "Enter 执行选中项,或直接按 y/a/d",
|
||||
_ => "Enter selected option, or press y/a/d directly",
|
||||
}
|
||||
}
|
||||
|
||||
struct ApprovalOptionRow {
|
||||
option: crate::tui::approval::ApprovalOption,
|
||||
label: &'static str,
|
||||
key_hint: &'static str,
|
||||
dangerous: bool,
|
||||
}
|
||||
|
||||
fn approval_options_for(risk: RiskLevel, locale: Locale) -> [ApprovalOptionRow; 4] {
|
||||
use crate::tui::approval::ApprovalOption as O;
|
||||
let dangerous = matches!(risk, RiskLevel::Destructive);
|
||||
[
|
||||
ApprovalOptionRow {
|
||||
option: O::ApproveOnce,
|
||||
label: option_approve_once(locale),
|
||||
key_hint: "1 / y",
|
||||
dangerous,
|
||||
},
|
||||
ApprovalOptionRow {
|
||||
option: O::ApproveAlways,
|
||||
label: option_approve_always(locale),
|
||||
key_hint: "2 / a",
|
||||
dangerous,
|
||||
},
|
||||
ApprovalOptionRow {
|
||||
option: O::Deny,
|
||||
label: option_deny(locale),
|
||||
key_hint: "3 / d / n",
|
||||
dangerous: false,
|
||||
},
|
||||
ApprovalOptionRow {
|
||||
option: O::Abort,
|
||||
label: option_abort(locale),
|
||||
key_hint: "Esc",
|
||||
dangerous: false,
|
||||
@@ -3479,6 +3368,43 @@ mod tests {
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn approval_selected_destructive_option_uses_contrasting_highlight() {
|
||||
let request = crate::tui::approval::ApprovalRequest::new(
|
||||
"approval-1",
|
||||
"exec_shell",
|
||||
"Run git commit",
|
||||
&serde_json::json!({ "command": "git commit -m fix" }),
|
||||
"exec_shell:git commit",
|
||||
);
|
||||
let view = crate::tui::approval::ApprovalView::new(request.clone());
|
||||
let widget = ApprovalWidget::new(&request, &view);
|
||||
let area = Rect::new(0, 0, 100, 30);
|
||||
let mut buf = Buffer::empty(area);
|
||||
|
||||
widget.render(area, &mut buf);
|
||||
|
||||
let selected_row = (area.y..area.y.saturating_add(area.height))
|
||||
.find(|&y| {
|
||||
(area.x..area.x.saturating_add(area.width))
|
||||
.any(|x| buf[(x, y)].bg == palette::DEEPSEEK_BLUE)
|
||||
})
|
||||
.expect("selected approval row should use blue background");
|
||||
let highlighted_cells = (area.x..area.x.saturating_add(area.width))
|
||||
.filter(|&x| {
|
||||
let cell = &buf[(x, selected_row)];
|
||||
!cell.symbol().trim().is_empty()
|
||||
&& cell.bg == palette::DEEPSEEK_BLUE
|
||||
&& cell.fg == palette::SELECTION_TEXT
|
||||
})
|
||||
.count();
|
||||
|
||||
assert!(
|
||||
highlighted_cells >= 4,
|
||||
"selected destructive option should render visible blue/white text"
|
||||
);
|
||||
}
|
||||
|
||||
/// Regression for issue #65: after `App::handle_resize`, the chat widget
|
||||
/// must produce a clean render at the new width — no stale wrapping,
|
||||
/// no panic, no content exceeding the requested width. Cycling through
|
||||
|
||||
Reference in New Issue
Block a user