From f167869695ac8a8637156b4787e9f2c7491e8fc0 Mon Sep 17 00:00:00 2001 From: Edwin Date: Sat, 6 Jun 2026 08:47:04 -0700 Subject: [PATCH] Make approval mode badge clickable --- crates/cli/src/app.rs | 45 ++++++++++++-- crates/cli/src/ui.rs | 117 ++++++++++++++++++++++++++++++++--- specs/0015-approval-modes.md | 2 + 3 files changed, 149 insertions(+), 15 deletions(-) diff --git a/crates/cli/src/app.rs b/crates/cli/src/app.rs index 3251b05e..a8c73549 100644 --- a/crates/cli/src/app.rs +++ b/crates/cli/src/app.rs @@ -1260,6 +1260,20 @@ pub struct TerminalScrollbarHit { pub max_scrollback: usize, } +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct ModelineApprovalModeHit { + pub row: u16, + pub start_col: u16, + /// Exclusive end column. + pub end_col: u16, +} + +impl ModelineApprovalModeHit { + pub fn contains(&self, col: u16, row: u16) -> bool { + row == self.row && col >= self.start_col && col < self.end_col + } +} + #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub struct WindowPaneHit { pub id: u64, @@ -1286,6 +1300,8 @@ pub struct LayoutSnapshot { pub pin_strip_area: Option, pub matrix_rain_area: Option, pub minibuffer_area: Option, + /// Clickable approval-mode badge in the modeline for the selected session. + pub modeline_approval_mode_hit: Option, /// Number of rows of the list pane currently in use (so a click /// past the last row is a no-op rather than selecting an /// out-of-range item). Mirrors `app.list_items().len()`. @@ -3744,8 +3760,18 @@ impl App { /// Cycle the selected session's approval mode. pub async fn cycle_approval_mode(&mut self) { + self.cycle_approval_mode_with_status(true).await; + } + + pub async fn cycle_approval_mode_silent(&mut self) { + self.cycle_approval_mode_with_status(false).await; + } + + async fn cycle_approval_mode_with_status(&mut self, show_status: bool) { let Some(s) = self.selected_session() else { - self.set_status("no session selected".into()); + if show_status { + self.set_status("no session selected".into()); + } return; }; let id = s.id.clone(); @@ -3755,10 +3781,10 @@ impl App { agentd_protocol::ApprovalMode::UnsafeAuto => agentd_protocol::ApprovalMode::Manual, }; match self.client.set_approval_mode(&id, next).await { - Ok(()) => self.set_status(format!( - "approval mode {}", - next.badge().unwrap_or("manual") - )), + Ok(()) if show_status => { + self.set_status(format!("approval mode {}", next.badge().unwrap_or("manual"))) + } + Ok(()) => {} Err(e) => self.set_status(format!("set_approval_mode failed: {e}")), } } @@ -4490,6 +4516,14 @@ impl App { if self.handle_dynamic_ui_overlay_click(col, row).await { return; } + if self + .layout + .modeline_approval_mode_hit + .is_some_and(|hit| hit.contains(col, row)) + { + self.cycle_approval_mode_silent().await; + return; + } // Matrix-rain horizontal reveal word: jump to the session that // produced it (issue #140). Checked before the pane hit-tests — // the rain panel is its own region, so this never shadows a real @@ -7562,6 +7596,7 @@ mod tests { pin_strip_area: Some(Rect::new(20, 20, 80, 8)), matrix_rain_area: None, minibuffer_area: Some(Rect::new(0, 29, 100, 4)), + modeline_approval_mode_hit: None, list_row_count: 0, list_items_area: None, list_scroll_offset: 0, diff --git a/crates/cli/src/ui.rs b/crates/cli/src/ui.rs index 7e4767eb..c188d928 100644 --- a/crates/cli/src/ui.rs +++ b/crates/cli/src/ui.rs @@ -107,6 +107,7 @@ pub fn render(f: &mut Frame, app: &mut App) { app.layout.dynamic_ui_trigger = None; app.layout.dynamic_ui_triggers.clear(); app.layout.shortcut_hints.clear(); + app.layout.modeline_approval_mode_hit = None; app.layout.main_window_areas.clear(); app.layout.main_window_dividers.clear(); app.window_pane_sizes.clear(); @@ -253,6 +254,7 @@ pub fn render(f: &mut Frame, app: &mut App) { render_list_title_button_tooltips(f, app); render_view_uncollapse_tooltip(f, app); render_harness_unavailable_tooltip(f, app); + render_modeline_approval_mode_tooltip(f, app); render_tasks_popup(f, app); render_remote_control_popup(f, app); if app.help_visible { @@ -5172,7 +5174,7 @@ fn render_chat(f: &mut Frame, area: Rect, app: &App) { f.render_widget(para, area); } -fn render_modeline(f: &mut Frame, area: Rect, app: &App) { +fn render_modeline(f: &mut Frame, area: Rect, app: &mut App) { let s = app.selected_session(); let conn = if app.connected { "" } else { " disconnected!" }; let focus_label = match app.focus { @@ -5184,10 +5186,8 @@ fn render_modeline(f: &mut Frame, area: Rect, app: &App) { } else { String::new() }; - let approval_mode_badge = match s.and_then(|s| s.approval_mode.badge()) { - Some(badge) => format!("[{badge}] "), - None => String::new(), - }; + let approval_mode_label = s.and_then(approval_mode_modeline_label); + let approval_mode_badge = approval_mode_label.map(|badge| format!("[{badge}]")); // "● remote: N" badge when at least one phone / remote client is // attached to the daemon. Visible signal that another surface // is also driving sessions, so the local user doesn't get @@ -5203,11 +5203,9 @@ fn render_modeline(f: &mut Frame, area: Rect, app: &App) { } else { "" }; - let modeline = format!( - " agentd focus:{focus} {sel} {model} {remote}{approval_mode}{scrollback}{chord}{empty_hint}{status}{conn} ", + let modeline_before_approval_mode = format!( + " agentd focus:{focus} {sel} {model} {remote}", focus = focus_label, - scrollback = scrollback_label, - approval_mode = approval_mode_badge, remote = remote_badge, sel = match s { Some(s) => format!("\"{}\"", primary_label(s)), @@ -5217,6 +5215,32 @@ fn render_modeline(f: &mut Frame, area: Rect, app: &App) { Some(s) => s.model.clone().unwrap_or_else(|| "-".into()), None => "-".into(), }, + ); + if approval_mode_label.is_some() { + let start_col = area + .x + .saturating_add(UnicodeWidthStr::width(modeline_before_approval_mode.as_str()) as u16); + let width = approval_mode_badge + .as_deref() + .map(UnicodeWidthStr::width) + .unwrap_or(0) as u16; + if width > 0 && start_col < area.x.saturating_add(area.width) { + let end_col = start_col + .saturating_add(width) + .min(area.x.saturating_add(area.width)); + if end_col > start_col { + app.layout.modeline_approval_mode_hit = + Some(crate::app::ModelineApprovalModeHit { + row: area.y, + start_col, + end_col, + }); + } + } + } + let modeline_after_approval_mode = format!( + "{scrollback}{chord}{empty_hint}{status}{conn} ", + scrollback = scrollback_label, chord = if app.chord_label.is_empty() { String::new() } else { @@ -5225,7 +5249,30 @@ fn render_modeline(f: &mut Frame, area: Rect, app: &App) { empty_hint = empty_hint, status = status, ); - let para = Paragraph::new(modeline).style( + let mut spans = Vec::new(); + spans.push(Span::raw(modeline_before_approval_mode)); + if let Some(badge) = approval_mode_badge { + let hovered = app + .mouse_pos + .zip(app.layout.modeline_approval_mode_hit) + .is_some_and(|((col, row), hit)| hit.contains(col, row)); + let badge_style = Style::default() + .bg(app.theme.modeline_bg) + .fg(if hovered { + app.theme.text + } else { + app.theme.modeline_fg + }) + .add_modifier(if hovered { + Modifier::BOLD | Modifier::UNDERLINED + } else { + Modifier::UNDERLINED + }); + spans.push(Span::styled(badge, badge_style)); + spans.push(Span::raw(" ")); + } + spans.push(Span::raw(modeline_after_approval_mode)); + let para = Paragraph::new(Line::from(spans)).style( Style::default() .bg(app.theme.modeline_bg) .fg(app.theme.modeline_fg), @@ -5256,6 +5303,37 @@ fn render_modeline(f: &mut Frame, area: Rect, app: &App) { } } +fn approval_mode_modeline_label(s: &SessionSummary) -> Option<&'static str> { + s.approval_mode + .badge() + .or_else(|| (s.harness == "zarvis").then_some("manual")) +} + +fn render_modeline_approval_mode_tooltip(f: &mut Frame, app: &App) { + let Some(hit) = app.layout.modeline_approval_mode_hit else { + return; + }; + let Some((mx, my)) = app.mouse_pos else { + return; + }; + if !hit.contains(mx, my) { + return; + } + let Some(s) = app.selected_session() else { + return; + }; + let Some(label) = approval_mode_modeline_label(s) else { + return; + }; + render_button_tooltip( + f, + &app.theme, + &format!(" Approval mode: {label}. Click to cycle "), + hit.start_col, + hit.row.saturating_sub(2), + ); +} + /// Compute how many rows the minibuffer footer occupies this frame. /// The default footer is 1 row (palette / hints / intent prompts). /// When the orchestrator panel is focused (its `MinibufferIntent` @@ -7680,6 +7758,25 @@ mod tests { assert_eq!(harness_label(&summary_with_mode("shell", None)), "shell"); } + #[test] + fn approval_mode_modeline_label_shows_manual_for_zarvis() { + let s = summary_with_mode("zarvis", Some("interactive")); + assert_eq!(approval_mode_modeline_label(&s), Some("manual")); + } + + #[test] + fn approval_mode_modeline_label_hides_manual_for_shell() { + let s = summary_with_mode("shell", Some("interactive")); + assert_eq!(approval_mode_modeline_label(&s), None); + } + + #[test] + fn approval_mode_modeline_label_uses_non_manual_badge() { + let mut s = summary_with_mode("zarvis", Some("interactive")); + s.approval_mode = agentd_protocol::ApprovalMode::UnsafeAuto; + assert_eq!(approval_mode_modeline_label(&s), Some("unsafe-auto")); + } + #[test] fn zarvis_running_animates_only_while_agent_active() { let mut s = summary_with_mode("zarvis", Some("interactive")); diff --git a/specs/0015-approval-modes.md b/specs/0015-approval-modes.md index 1bcafdc1..a7ac5af2 100644 --- a/specs/0015-approval-modes.md +++ b/specs/0015-approval-modes.md @@ -25,6 +25,8 @@ A boolean automode conflated two different needs: high-throughput trusted operat Clients should present approval mode as a session mode and use `unsafe_auto` terminology instead of generic “automode.” Adapters that do not gate tools may ignore approval-mode changes. `auto_review` is not a security boundary; it is a convenience review layer that must fall back to asking the user when uncertain. The reviewer prompt should encourage approving bounded routine development work, including ordinary file edits inside the active git worktree, while still asking the user for broad, ambiguous, unrelated, outside-worktree, or sensitive actions. +Interactive clients should keep the selected tool-gating session's current mode visible and allow direct mode changes from that status surface when the UI has one. Cycling order is `manual` → `auto_review` → `unsafe_auto` → `manual`. + One approval decision applies to the whole pending tool call (including every hunk of a batched edit). The prompt conveys this through the call summary rather than the action labels: for batched edit calls the summary should include affected file paths and edit-level hints rather than only aggregate counts. Action labels stay simple verbs (`approve` / `deny` / `auto-review`) — not `approve all` / `deny all`, which read as approving or denying all *future* calls (the role of `unsafe_auto`) rather than the parts of the current one. Fleet observers and operator/minibuffer surfaces should not duplicate inline approval prompts as proactive observations. The requesting session remains the canonical interaction surface for that approval.