From 3c20c1efc2f19a1329a181b8cb353252ba18e030 Mon Sep 17 00:00:00 2001 From: Stan Fan Date: Thu, 6 Aug 2026 20:46:20 +1000 Subject: [PATCH 1/6] feat(tui): semantic colour layer, folded pickers, and reveal-before-run (#556) Adds an ANSI-16 theme with NO_COLOR support and migrates the frame to styled Lines. Also bounds the left column: the command list and both pickers now window and scroll, and the first Enter reveals folded options instead of running. Co-Authored-By: Claude Opus 5 --- tui/src/main.rs | 23 + tui/src/renderer.rs | 2021 +++++++++++++++++- tui/src/results_pane.rs | 548 ++++- tui/src/theme.rs | 608 ++++++ tui/tests/hermeticity_tripwire.rs | 88 +- tui/tests/no_backend_attach_call.rs | 16 +- tui/tests/no_colour_literal_outside_theme.rs | 558 +++++ 7 files changed, 3752 insertions(+), 110 deletions(-) create mode 100644 tui/src/theme.rs create mode 100644 tui/tests/no_colour_literal_outside_theme.rs diff --git a/tui/src/main.rs b/tui/src/main.rs index ead6f1041..451224112 100644 --- a/tui/src/main.rs +++ b/tui/src/main.rs @@ -44,6 +44,12 @@ mod results_pane; /// Six methods, six error variants, the 21-route table, and no subprocess anywhere (ADR-02). /// (#321) mod server; +/// The semantic colour layer (#556): six roles, an ANSI-16 palette, and `NO_COLOR`. **The only +/// module permitted to name a `Color`** — every other module says `theme.error`, and +/// `tests/no_colour_literal_outside_theme.rs` makes that a failing test rather than a convention. +/// Colour is decoration only: every state this crate shows is already textual, which is NFR-3 of +/// #321 and the reason `NO_COLOR` costs nothing to honour. (#556) +mod theme; /// The wire vocabulary six later units share. Declared here so every consumer imports the /// types from one place rather than redeclaring the server's shapes locally. (#321) mod types; @@ -255,6 +261,12 @@ fn run_app() -> Result<(), TuiError> { let mut shell = renderer::Renderer::new(server.as_ref(), &host, cols, rows) .with_concurrent_pickers(Arc::clone(&server)); + // `NO_COLOR` is read exactly ONCE, here, and the resolved palette is threaded down (#556). + // Re-reading it per frame would let a mid-session change produce a half-coloured screen, and + // reading it deeper in the call tree would make every unit test's output depend on the ambient + // environment. This is the only `from_env` call in the crate. + shell.set_theme(theme::Theme::from_env()); + // A `Fatal` here exits non-zero with one styled line — never a traceback (SR-1). Mapped into // `TuiError` because this function's signature is the boundary contract, and `Fatal`'s own // `Display` already carries the whole operator-facing sentence. @@ -268,6 +280,17 @@ fn run_app() -> Result<(), TuiError> { if !interactive { let frame = shell.render(); let mut out = io::stdout().lock(); + // `{line}` on a `Line` writes its spans' content and **no SGR codes** — checked in + // ratatui-core 0.1.2, `Span`'s `Display` is a plain `write!` of `content`. That is what + // keeps a pipe free of escapes now that these are styled values (SR-1); it is relied upon + // here rather than merely true, so it is written down. Guarded by + // `renderer::tests::a_styled_line_displays_without_escape_codes` — nothing asserted this + // from a real piped process, and a dependency on an upstream `Display` impl with no test + // behind it is what silently breaks on a minor-version bump. + // + // Still header+footer only, deliberately: this is the pipe frame from #321, and widening it + // to `plain_lines()` would change what `cao-tui | ...` prints under cover of a colour + // change. (#556) for line in frame.header.iter().chain(&frame.footer) { writeln!(out, "{line}")?; } diff --git a/tui/src/renderer.rs b/tui/src/renderer.rs index 938615646..94a8cd2e1 100644 --- a/tui/src/renderer.rs +++ b/tui/src/renderer.rs @@ -66,6 +66,7 @@ use ratatui::backend::CrosstermBackend; use ratatui::buffer::Buffer; use ratatui::layout::{Constraint, Layout, Rect}; use ratatui::style::Style; +use ratatui::text::{Line, Span}; use ratatui::widgets::{Paragraph, Widget, Wrap}; use ratatui::Terminal as RatatuiTerminal; @@ -74,6 +75,7 @@ use crate::error::TuiError; use crate::guided_flow::{self, Field, FieldKind, GuidedFlow, PickerState, UNLOADABLE_MARKER}; use crate::handoff::{HandoffDriver, Host, ServerRead}; use crate::results_pane::{PaneState, ResultsPane}; +use crate::theme::Theme; use crate::types::{Health, Profile, Provider, Readiness, SessionParams, Terminal, TerminalStatus}; /// The minimum terminal the two-column layout needs (NFR-6). Below either bound the layout @@ -149,8 +151,12 @@ pub struct Fatal(pub String); /// Which region has keyboard focus. Focus order follows FR-2.1's step order. /// /// A closed enum with an exhaustive `match` in [`Renderer::on_key`], following `catalog.rs`'s -/// idiom: a sixth region is a compile error rather than a region that silently cannot be reached +/// idiom: a further region is a compile error rather than a region that silently cannot be reached /// by `Tab` — which, for a keyboard-only UI (NFR-3), is the same as not existing. (#321) +/// +/// [`Self::AgentPicker`] and [`Self::ProviderPicker`] were added when the pickers became foldable: +/// a fold the operator cannot focus is a fold they cannot open, so the two regions are what make +/// collapsed-by-default safe rather than lossy. #[allow(dead_code)] // every variant is constructed by `on_key`'s focus ring. (#321) #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum Focus { @@ -160,6 +166,10 @@ pub enum Focus { RequiredFields, /// The collapsed-by-default optional section header (FR-2.3). OptionalSection, + /// The collapsed-by-default agent picker. `Enter` folds, `Up`/`Down` scroll when expanded. + AgentPicker, + /// The collapsed-by-default provider picker. Same keys as [`Self::AgentPicker`]. + ProviderPicker, /// The results pane. `[k]` cancels here, and **only while running**. Results, } @@ -171,10 +181,12 @@ impl Focus { /// A `const` array rather than an `impl` with four arms so the *order* is readable as data — /// FR-2.1 specifies a step order, and a chain of `match` arms makes it something a reader has /// to reconstruct. - const ORDER: [Self; 4] = [ + const ORDER: [Self; 6] = [ Self::CommandList, Self::RequiredFields, Self::OptionalSection, + Self::AgentPicker, + Self::ProviderPicker, Self::Results, ]; @@ -426,32 +438,46 @@ enum PendingAction { /// is blank when every rendered string is empty or whitespace. So [`Self::is_blank`] asks "is /// there at least one non-whitespace glyph", not `frame != Frame::default()` — the latter passes /// while the screen is visually empty. (#321) +/// # Why the regions are `Line<'static>` and not `String` (#556) +/// +/// The semantic colour layer has to attach a style to *part* of a line — the `>` focus marker, an +/// unset field's `(required)` — and a `Vec` has nowhere to put one. Carrying the style in +/// the `Frame` rather than applying it in [`Renderer::draw`] keeps the property this type exists +/// for: the widget path and the assertion path see the same data, styles included, so a styling +/// mistake is assertable without a terminal. +/// +/// `'static` and not `'_`: a borrowed lifetime would tie `Frame` to `&self`, which +/// `render(&self) -> Frame` cannot return. Every string in here is already owned, so this costs +/// nothing. +/// +/// **Every pre-#556 test asserts on content, not style**, and they were repointed at +/// [`Self::plain_lines`] rather than rewritten — a rewritten assertion is a chance to weaken one. #[derive(Debug, Clone, PartialEq, Eq)] pub struct Frame { /// The layout NFR-6 chose for the current size. pub layout: LayoutMode, /// App title plus the server-status indicator, textual. - pub header: Vec, + pub header: Vec>, /// The navigable command list — **HIDE rows already excluded** by `commands()` (FR-4.3). - pub command_list: Vec, + pub command_list: Vec>, /// The required fields, always visible. - pub required_fields: Vec, + pub required_fields: Vec>, /// The optional section: its header always, its fields only when expanded (FR-2.3). - pub optional_section: Vec, + pub optional_section: Vec>, /// The two pickers' states, each stating cause and remedy when failed (FR-6.1). - pub pickers: Vec, + pub pickers: Vec>, /// The results pane's own lines, pulled from the pane. - pub results: Vec, + pub results: Vec>, /// The banner, when there is one. - pub banner: Vec, + pub banner: Vec>, /// Gating reason | key hints (FR-6.2, NFR-3). - pub footer: Vec, + pub footer: Vec>, } #[allow(dead_code)] // `is_blank` is SR-2's predicate, asserted by this module's tests. (#321) impl Frame { /// Every line in the frame, in render order. - fn all_lines(&self) -> impl Iterator { + fn all_lines(&self) -> impl Iterator> { self.header .iter() .chain(&self.command_list) @@ -463,12 +489,32 @@ impl Frame { .chain(&self.footer) } + /// Every line's **text**, styles discarded, in render order. + /// + /// This is the strip-styling seam (FR-5.2) and it does double duty. It is what the pre-#556 + /// tests assert on, so the `Vec` → `Vec` migration did not have to touch them; + /// and it is what the FR-5.2 guards use to prove no state is conveyed by colour alone, because + /// what it returns is exactly what an operator on a monochrome terminal reads. + /// + /// Uses `Line::to_string`, which concatenates the spans' content — so a line assembled from + /// three styled spans yields the same string as the single unstyled span it replaced. That + /// equivalence is what `the_frames_plain_text_survived_the_line_migration` pins. + pub fn plain_lines(&self) -> Vec { + self.all_lines().map(Line::to_string).collect() + } + /// Is there **no** non-whitespace glyph anywhere in the frame? /// /// The definition `frontend-components.md:167` insists on, because a blank screen is /// indistinguishable from a hang (SR-2) and a weaker definition makes the guard vacuous. + /// + /// **Unchanged by #556, deliberately.** This is SR-2's predicate, and it still asks about + /// *glyphs*: a styled-but-empty `Line` is blank, exactly as an empty `String` was. Letting a + /// style count as content would retire a safety property silently (FR-4.3). pub fn is_blank(&self) -> bool { - !self.all_lines().any(|line| !line.trim().is_empty()) + !self + .all_lines() + .any(|line| !line.to_string().trim().is_empty()) } } @@ -497,6 +543,21 @@ pub struct Renderer<'a, S: ServerApi, H: Host> { focus: Focus, /// `false` by default (FR-2.3): the optional section is collapsed but **present**. optional_expanded: bool, + /// `false` by default: the agent list is collapsed to a counted header but **present**. + /// + /// The left column does not scroll — `draw` renders it as one `Paragraph`, so anything past the + /// last row is simply clipped. With 25 agents the picker pushed the *banner* off-screen, which + /// meant a failure the operator needed to read was invisible. Collapsing by default is what + /// keeps the column bounded regardless of how many profiles the machine has. + agents_expanded: bool, + /// `false` by default, for the same reason as [`Self::agents_expanded`]. + providers_expanded: bool, + /// First visible row within the expanded agent list — the viewport offset, not a selection. + /// + /// A separate scroll offset per picker, because scrolling one must not move the other. + agent_scroll: usize, + /// First visible row within the expanded provider list. + provider_scroll: usize, cols: u16, rows: u16, banner: Option, @@ -527,6 +588,14 @@ pub struct Renderer<'a, S: ServerApi, H: Host> { edit_buffer: Option<(usize, String)>, /// Set by `[q]` (or a confirmed quit) and read by the event loop. should_quit: bool, + /// The semantic palette (#556). The **only** presentation-only field on this struct. + /// + /// Held rather than read from the environment at each `render()`: `Theme::from_env` is resolved + /// once at startup (`main`), so a `NO_COLOR` change mid-session cannot make the screen + /// half-coloured, and `render()` stays a pure function of state. It is also what makes + /// [`Self::set_theme`] enough to test both palettes without touching process environment — + /// which matters, because `std::env::set_var` is racy across Rust's threaded test harness. + theme: Theme, } #[allow(dead_code)] // `main` reaches `new`/`run`/`render`; the rest await the event loop. (#321) @@ -551,6 +620,10 @@ impl<'a, S: ServerApi, H: Host> Renderer<'a, S, H> { field_cursor: 0, focus: Focus::CommandList, optional_expanded: false, + agents_expanded: false, + providers_expanded: false, + agent_scroll: 0, + provider_scroll: 0, cols, rows, banner: None, @@ -563,9 +636,24 @@ impl<'a, S: ServerApi, H: Host> Renderer<'a, S, H> { confirm_quit: false, edit_buffer: None, should_quit: false, + // Colour by default, and `main` overrides it with `Theme::from_env()`. The default is + // deliberately the LOUD one: a forgotten `set_theme` then shows up as colour with + // `NO_COLOR` set, which somebody reports, rather than as a permanently monochrome TUI, + // which looks like working code. (#556) + theme: Theme::default(), } } + /// Installs the palette, once, at startup. + /// + /// Separate from [`Self::new`] because `render()`'s signature cannot take a theme — it is + /// `Widget::render(self, area, buf)` downstream — and because reading the environment inside a + /// constructor would make every test's rendering depend on the ambient `NO_COLOR`. (#556) + pub fn set_theme(&mut self, theme: Theme) { + self.theme = theme; + self.pane.set_theme(theme); + } + /// Installs the production concurrent picker source. /// /// Kept as a constructor modifier so the orchestration fake need not be `Sync`: production @@ -680,22 +768,116 @@ impl<'a, S: ServerApi, H: Host> Renderer<'a, S, H> { /// unconditional — that alone makes [`Frame::is_blank`] false even with the server down, both /// pickers failed and no command selected, which is the total-failure case the never-blank /// test exercises. + /// The producers below still return `Vec`, and #556 converts at **this** boundary + /// rather than retyping each of them. Two reasons: a producer that builds plain text cannot + /// accidentally apply a colour, so the "only `theme.rs` names a `Color`" property is easier to + /// hold; and it kept the `Vec` → `Vec` migration to one place, which is what let + /// `the_frames_plain_text_survived_the_line_migration` isolate a wrapping regression from a + /// styling mistake. `style_*` below then re-splits the lines that need a styled span. pub fn render(&self) -> Frame { let layout = LayoutMode::of(self.cols, self.rows); Frame { layout, - header: self.header_lines(), - command_list: self.command_list_lines(), - required_fields: self.field_lines(true), - optional_section: self.optional_section_lines(), - pickers: self.picker_lines(), - results: self.results_lines(), - banner: self.banner.as_ref().map(Banner::lines).unwrap_or_default(), - footer: self.footer_lines(), + // Unstyled, deliberately. The header's server line already carries its own cause and + // remedy as words (FR-6.1); colouring it `error` would add nothing an operator acts on + // and would put a second, redundant signal on the crate's most-read line. + header: plain(self.header_lines()), + command_list: self.style_focus_marker(self.command_list_lines()), + required_fields: self.style_form(self.field_lines(true)), + optional_section: self.style_form(self.optional_section_lines()), + pickers: style_pickers(self.picker_lines(), &self.theme), + // Unstyled here: the pane styles ITSELF (T-4). These lines are read back out of the + // pane's own rendered buffer, so restyling them would be a second opinion about a + // decision the pane already made — and the two could disagree. + results: plain(self.results_lines()), + banner: plain(self.banner.as_ref().map(Banner::lines).unwrap_or_default()), + footer: self.style_footer(self.footer_lines()), } } + /// `theme.focus` on the focus marker **and its whole line** (FR-4.4, FR-4.5). + /// + /// The line, not just the `>`: a one-character cue is what NFR-3 item 7 calls insufficient + /// visible focus, and the marker is already structural — the colour is a second channel on the + /// same fact, which is exactly what decoration-only colour means. + /// + /// There is **no border and no selected-row widget to style instead** (design C-2, P-22): this + /// crate renders one `Block::new()`, borderless, and uses no `List`/`Table`/`highlight_style`. + /// The issue's palette table says focus applies to the "focused region border, selected row"; + /// applying it to the existing `>` marker is the honest translation of that intent. + fn style_focus_marker(&self, lines: Vec) -> Vec> { + lines + .into_iter() + .map(|line| { + if line.starts_with('>') { + Line::styled(line, self.theme.focus) + } else if line.contains(SCROLL_RESIDUE_MARKER) { + // The windowed list's residue line is navigational chrome, same as a picker's. + Line::styled(line, self.theme.dim) + } else { + Line::raw(line) + } + }) + .collect() + } + + /// The form's two regions: `focus` on the marked row, `required` on an **unset** `(required)`, + /// `dim` on the `[not sent — …]` marker (FR-4.4). + /// + /// # Why `(required)` is only styled while the field is unset + /// + /// A satisfied required field is not a thing the operator has to act on, and colouring it + /// keeps a warning on screen after the warning is answered — which trains the operator to + /// ignore the colour. The "unset" test is the rendered `: —` value that [`render_field`] writes + /// for `None`, read back rather than re-derived from `GuidedFlow`: re-deriving would let the + /// style disagree with the text on the same line. + fn style_form(&self, lines: Vec) -> Vec> { + lines + .into_iter() + .map(|line| { + // A focused row takes `focus` for the whole line, so the two roles cannot fight + // over the same cells. Focus is the more urgent of the two — it says where the + // keyboard is. + if line.starts_with('>') { + return Line::styled(line, self.theme.focus); + } + + let unset_required = + line.contains(REQUIREMENT_SUFFIX) && line.trim_end().ends_with(UNSET_VALUE); + if unset_required { + return split_styled(&line, REQUIREMENT_SUFFIX, self.theme.required); + } + + let not_sent = format!("[{}]", guided_flow::NOT_SENT_MARKER); + if line.contains(¬_sent) { + return split_styled(&line, ¬_sent, self.theme.dim); + } + + Line::raw(line) + }) + .collect() + } + + /// The footer's blocked reason takes `required` (FR-4.4). + /// + /// `required` and not `error`: a blocked form is not a failure, it is an unfinished one, and the + /// reason names the field the operator still has to fill. Reaching for `error` here would report + /// a problem where there is only an incomplete step — and the palette already has a role that + /// means "you need to supply this". + fn style_footer(&self, lines: Vec) -> Vec> { + lines + .into_iter() + .map(|line| { + if line.starts_with(BLOCKED_PREFIX) { + Line::styled(line, self.theme.required) + } else { + Line::raw(line) + } + }) + .collect() + } + /// Title plus the server indicator, textual (NFR-3). fn header_lines(&self) -> Vec { let status = match &self.health { @@ -719,12 +901,77 @@ impl<'a, S: ServerApi, H: Host> Renderer<'a, S, H> { vec![format!("cao-tui {}", env!("CARGO_PKG_VERSION")), status] } - /// The command list. `Hidden` rows are **absent**, not disabled (FR-4.3, SR-3). + /// The command list, **windowed around the cursor**. `Hidden` rows are absent (FR-4.3, SR-3). /// /// The focus marker is a structural `>` rather than a colour, per NFR-3 item 1 — a screen /// whose focus is only a hue is unnavigable for anyone who cannot see it, and item 7 makes /// visible focus a hard requirement. + /// + /// # Why this is windowed, and why folding the pickers alone was not enough + /// + /// The catalog offers **42 commands**, and at 120 columns several wrap, so the full list needs + /// roughly 55 screen rows. The left column is one non-scrolling `Paragraph`, so on any terminal + /// shorter than that the list consumed the whole column and the form, the pickers and the + /// banner were all clipped away — measured in a real pty at both 40 and 70 rows, where the + /// pickers never appeared at all regardless of how few agents the machine had. + /// + /// That means the reported "can't see the agents" had **two** independent causes: a long agent + /// list (fixed by the fold) and this. Folding the pickers while leaving the list unbounded + /// would have produced a fold the operator still could not see, so the fix is only complete + /// with both. + /// + /// The window follows the cursor rather than being a fixed slice, because the list is what + /// `Up`/`Down` move through: a fixed slice would let the cursor walk off the visible rows, + /// which is the same invisible-focus defect one region over. fn command_list_lines(&self) -> Vec { + let rows: Vec = self.command_rows(); + let window = self.command_list_window(); + if rows.len() <= window { + return rows; + } + + // Centre the cursor, then clamp to the ends so the first and last screens are full rather + // than half-empty — a half-empty last screen reads as the end of a shorter list. + let half = window / 2; + let start = self + .cursor + .saturating_sub(half) + .min(rows.len().saturating_sub(window)); + let end = (start + window).min(rows.len()); + + let mut lines: Vec = Vec::with_capacity(window + 1); + lines.extend(rows[start..end].iter().cloned()); + // The residue names what is off-screen in each direction, so a partial list never reads as + // the whole catalog. + let (above, below) = (start, rows.len() - end); + if above > 0 || below > 0 { + let mut parts = Vec::new(); + if above > 0 { + parts.push(format!("{above} above")); + } + if below > 0 { + parts.push(format!("{below} below")); + } + lines.push(format!( + " … {} of {} commands, {SCROLL_RESIDUE_MARKER}", + parts.join(", "), + rows.len() + )); + } + lines + } + + /// How many rows the command list may occupy. + /// + /// A third of the terminal, floored at 3: the list shares the column with the form, the two + /// picker folds and the banner, and a list that takes the whole height is the defect being + /// fixed. The floor keeps it navigable at the smallest size the crate renders at. + fn command_list_window(&self) -> usize { + usize::from(self.rows / 3).max(3) + } + + /// Every command as a line, unwindowed — [`Self::command_list_lines`] slices this. + fn command_rows(&self) -> Vec { self.commands .iter() .enumerate() @@ -773,7 +1020,10 @@ impl<'a, S: ServerApi, H: Host> Renderer<'a, S, H> { Focus::OptionalSection => { !guided && self.optional_expanded && self.field_cursor == offset + index } - Focus::CommandList | Focus::Results => false, + Focus::CommandList + | Focus::AgentPicker + | Focus::ProviderPicker + | Focus::Results => false, }; render_field(field, focused, self.flow.current()) }) @@ -833,21 +1083,36 @@ impl<'a, S: ServerApi, H: Host> Renderer<'a, S, H> { lines.push("agents: none found on this machine".to_string()); } PickerState::Loaded(profiles) => { - lines.push(format!("agents ({}):", profiles.len())); - for profile in profiles { - // The unselectable marker travels WITH its row (FR-1.5): an unloadable profile - // is shown and explained, never filtered out. - if profile.loadable { - lines.push(format!(" {}", profile.name)); - } else { - lines.push(format!(" {} [{UNLOADABLE_MARKER}]", profile.name)); - } - } + // The unselectable marker travels WITH its row (FR-1.5): an unloadable profile + // is shown and explained, never filtered out. + let rows: Vec = profiles + .iter() + .map(|profile| { + if profile.loadable { + format!(" {}", profile.name) + } else { + format!(" {} [{UNLOADABLE_MARKER}]", profile.name) + } + }) + .collect(); + let unavailable = profiles.iter().filter(|p| !p.loadable).count(); + lines.extend(self.fold_lines( + "agents", + &rows, + unavailable, + "unloadable", + self.agents_expanded, + self.agent_scroll, + self.focus == Focus::AgentPicker, + )); } PickerState::Failed(error) => { // `[ctrl+r]`, not `[r]`: a plain `r` is TEXT in a field, so naming it here would // promise an affordance that types instead of retrying — the `[c] clear` failure // again, where a documented key did nothing. (#321) + // + // NEVER FOLDED, in any state: a failure the operator has to act on must not need a + // keystroke to become visible, and this line already fits in one row. lines.push(format!( "agents: unavailable — {error}. Press [ctrl+r] to retry" )); @@ -860,17 +1125,29 @@ impl<'a, S: ServerApi, H: Host> Renderer<'a, S, H> { lines.push("providers: none reported".to_string()); } PickerState::Loaded(providers) => { - lines.push(format!("providers ({}):", providers.len())); - for provider in providers { - // `installed` is DISPLAY information, never a filter (FR-1.7): the endpoint - // serves a hard-coded nine-entry map against a ten-value enum, so hiding an - // uninstalled provider would hide real drift. - if provider.installed { - lines.push(format!(" {}", provider.name)); - } else { - lines.push(format!(" {} (not installed)", provider.name)); - } - } + // `installed` is DISPLAY information, never a filter (FR-1.7): the endpoint + // serves a hard-coded nine-entry map against a ten-value enum, so hiding an + // uninstalled provider would hide real drift. + let rows: Vec = providers + .iter() + .map(|provider| { + if provider.installed { + format!(" {}", provider.name) + } else { + format!(" {} {NOT_INSTALLED_MARKER}", provider.name) + } + }) + .collect(); + let unavailable = providers.iter().filter(|p| !p.installed).count(); + lines.extend(self.fold_lines( + "providers", + &rows, + unavailable, + "not installed", + self.providers_expanded, + self.provider_scroll, + self.focus == Focus::ProviderPicker, + )); } PickerState::Failed(error) => lines.push(format!( "providers: unavailable — {error}. Press [ctrl+r] to retry" @@ -880,6 +1157,89 @@ impl<'a, S: ServerApi, H: Host> Renderer<'a, S, H> { lines } + /// One picker as a fold: a counted header, plus a scrolled window of `rows` when expanded. + /// + /// # Why the count of unavailable rows is in the *collapsed* header + /// + /// FR-1.5 and FR-1.7 keep an unloadable profile and an uninstalled provider **listed** so the + /// operator learns the thing exists and why it is unavailable. A fold that simply hid them + /// would undo exactly that, and would do it silently. So the header carries the diagnosis + /// forward — `agents (25, 2 unloadable)` — and the per-row explanation is one keystroke away + /// rather than gone. Hiding the *rows* is a layout decision; hiding the *fact* would be a + /// regression against those two requirements. + /// + /// # Why a scrolled window rather than a `… N more` cap + /// + /// A cap leaves the tail unreachable on screen, which is the defect being fixed one level in: + /// the operator with 25 agents still could not see agent 25. The window moves, so every row is + /// reachable by `Up`/`Down`, and the residue count names what is off-window in each direction + /// so a partial view never reads as a complete one. + #[allow(clippy::too_many_arguments)] // seven small values, all of them the caller's own state. + fn fold_lines( + &self, + label: &str, + rows: &[String], + unavailable: usize, + unavailable_word: &str, + expanded: bool, + scroll: usize, + focused: bool, + ) -> Vec { + let marker = if focused { ">" } else { " " }; + let glyph = if expanded { "▾" } else { "▸" }; + let diagnosis = if unavailable > 0 { + format!(", {unavailable} {unavailable_word}") + } else { + String::new() + }; + let action = if expanded { "collapse" } else { "expand" }; + let mut lines = vec![format!( + "{marker} {glyph} {label} ({count}{diagnosis}) — [enter] {action}", + count = rows.len() + )]; + + if !expanded { + return lines; + } + + let window = self.picker_window(); + // `min` against a saturating end, so a scroll offset left over from a longer list cannot + // index past a shorter one — the pickers are re-fetched by `[ctrl+r]` and the new answer + // may be shorter than the old. + let start = scroll.min(rows.len().saturating_sub(1)); + let end = (start + window).min(rows.len()); + lines.extend(rows[start..end].iter().cloned()); + + // The residue in BOTH directions: a window showing rows 10-20 of 25 with no note reads + // exactly like a complete list of 11. + let hidden_above = start; + let hidden_below = rows.len().saturating_sub(end); + if hidden_above > 0 || hidden_below > 0 { + let mut parts = Vec::new(); + if hidden_above > 0 { + parts.push(format!("{hidden_above} above")); + } + if hidden_below > 0 { + parts.push(format!("{hidden_below} below")); + } + lines.push(format!( + " … {}, {SCROLL_RESIDUE_MARKER}", + parts.join(", ") + )); + } + lines + } + + /// How many rows an expanded picker may occupy. + /// + /// Derived from the terminal height rather than a constant: a fixed window that fits an 80x24 + /// wastes two thirds of a tall terminal, and one sized for a tall terminal re-creates the + /// clipping on a short one. The `2` floor keeps the window non-empty at the smallest size the + /// crate renders at, so an expanded picker always shows *something*. + fn picker_window(&self) -> usize { + usize::from(self.rows / 4).max(2) + } + /// The pane's region, as the operator sees it, plus its state as text. /// /// # Why this renders the pane rather than reading `ResultsPane::lines()` @@ -937,6 +1297,13 @@ impl<'a, S: ServerApi, H: Host> Renderer<'a, S, H> { match self.flow.blocked_reason() { Some(reason) => lines.push(reason), + // The gating line must name what `[enter]` ACTUALLY does right now. While anything is + // folded the first press reveals rather than runs, and "ready — [enter] run" would be + // the same broken promise as a documented key that does nothing — the operator presses + // it expecting a run, gets an expansion, and learns to distrust the footer. + None if self.has_folded_options() => { + lines.push("ready — [enter] show all options, then [enter] to run".to_string()); + } None => lines.push("ready — [enter] run".to_string()), } @@ -1036,6 +1403,28 @@ impl<'a, S: ServerApi, H: Host> Renderer<'a, S, H> { } _ => false, }, + // `rows` and `window` are read into locals FIRST: both are `&self` reads, and taking + // them inline would overlap the `&mut` borrows of the two fields below. + Focus::AgentPicker => { + let (rows, window) = (self.agent_row_count(), self.picker_window()); + Self::on_key_fold( + key, + &mut self.agents_expanded, + &mut self.agent_scroll, + rows, + window, + ) + } + Focus::ProviderPicker => { + let (rows, window) = (self.provider_row_count(), self.picker_window()); + Self::on_key_fold( + key, + &mut self.providers_expanded, + &mut self.provider_scroll, + rows, + window, + ) + } // `[k]` cancels **only while running**, and `cancel()`'s `NotRunning` is NOT // surfaced — `handle_key` swallows it and reports `false`, so pressing `[k]` with // nothing running does nothing visible. A key that does not apply is not an @@ -1110,6 +1499,11 @@ impl<'a, S: ServerApi, H: Host> Renderer<'a, S, H> { Focus::RequiredFields => true, // Text only once expanded; collapsed, it is a single activatable control. Focus::OptionalSection => self.optional_expanded, + // The picker folds are navigation, never text entry: nothing is typed into them in + // either state, so `[q]` and `[r]` must keep working while one is focused. This is the + // same reasoning that made `RequiredFields` unconditional above, applied to a region + // that happens to be foldable — foldable is not the same as editable. + Focus::AgentPicker | Focus::ProviderPicker => false, Focus::CommandList | Focus::Results => false, } } @@ -1163,6 +1557,13 @@ impl<'a, S: ServerApi, H: Host> Renderer<'a, S, H> { // so field 0 of the NEW form would otherwise inherit the old form's partial text. (#321) self.edit_buffer = None; self.optional_expanded = false; + // The folds reset with the form. Carrying an expansion across a command change would make + // the first `Enter` run immediately for the second command but not the first, so the + // reveal-then-run shape would depend on history rather than on what is on screen. + self.agents_expanded = false; + self.providers_expanded = false; + self.agent_scroll = 0; + self.provider_scroll = 0; self.populate_pickers(); } @@ -1183,7 +1584,7 @@ impl<'a, S: ServerApi, H: Host> Renderer<'a, S, H> { if start == end { return if key == KeyCode::Enter { - self.run_selected() + self.reveal_options_or_run() } else { false }; @@ -1199,7 +1600,7 @@ impl<'a, S: ServerApi, H: Host> Renderer<'a, S, H> { self.field_cursor = (self.field_cursor + 1).min(end - 1); true } - KeyCode::Enter => self.run_selected(), + KeyCode::Enter => self.reveal_options_or_run(), KeyCode::Char(' ') if self.focused_field_kind() == Some(FieldKind::Flag) => { self.toggle_focused_flag() } @@ -1215,6 +1616,136 @@ impl<'a, S: ServerApi, H: Host> Renderer<'a, S, H> { } } + /// A fold's keys: `Enter`/`Space` toggles, `Up`/`Down` scroll, `Esc` collapses. + /// + /// An associated function taking `&mut` to the two pieces of state rather than a method, so + /// the agent and provider arms share one implementation instead of two copies that can drift — + /// a scroll clamp fixed in one and not the other is precisely the kind of divergence this + /// avoids. It cannot be a `&mut self` method because the caller already holds `&mut` borrows of + /// the individual fields. + /// + /// Scrolling is clamped to the last *window*, not the last row: allowing `scroll` to reach + /// `len - 1` would let the operator scroll into a view holding a single row with empty space + /// below it, which reads as the end of a shorter list. + fn on_key_fold( + key: KeyCode, + expanded: &mut bool, + scroll: &mut usize, + rows: usize, + window: usize, + ) -> bool { + // NOTHING TO FOLD: a `Loading`, `Failed` or empty picker renders one unfoldable line, so + // every key here is a no-op. Reporting `false` rather than toggling an invisible flag + // follows this module's rule that "a key that does not apply is not an operator-facing + // error" — and it keeps `[enter]` reaching nothing rather than appearing to work. Without + // this the flag flipped, the screen did not change, and the keypress claimed success. + if rows == 0 { + return false; + } + + match key { + KeyCode::Enter | KeyCode::Char(' ') => { + *expanded = !*expanded; + // Collapsing resets the viewport: re-expanding to a remembered offset shows the + // middle of the list with no indication that the top was skipped. + if !*expanded { + *scroll = 0; + } + true + } + KeyCode::Up if *expanded => { + *scroll = scroll.saturating_sub(1); + true + } + KeyCode::Down if *expanded => { + let last = rows.saturating_sub(window); + *scroll = (*scroll + 1).min(last); + true + } + KeyCode::Esc if *expanded => { + *expanded = false; + *scroll = 0; + true + } + _ => false, + } + } + + /// How many rows the expanded agent list holds. `0` unless the fetch succeeded. + /// + /// `Loading` and `Failed` are 0 because neither renders a scrollable list — they render one + /// line each, and a scroll clamp computed against a phantom row count would let the viewport + /// move over content that is not there. + fn agent_row_count(&self) -> usize { + match self.flow.agent_choices() { + PickerState::Loaded(profiles) => profiles.len(), + PickerState::Loading | PickerState::Failed(_) => 0, + } + } + + /// How many rows the expanded provider list holds. `0` unless the fetch succeeded. + fn provider_row_count(&self) -> usize { + match self.flow.provider_choices() { + PickerState::Loaded(providers) => providers.len(), + PickerState::Loading | PickerState::Failed(_) => 0, + } + } + + /// `Enter` in the form: **reveal the options first, run on the second press.** + /// + /// # The defect this fixes + /// + /// `Enter` used to call [`Self::run_selected`] directly. Combined with the optional section + /// being collapsed by default (FR-2.3), that made the *first* `Enter` after choosing a command + /// execute it — so the nine optional parameters behind the fold were unreachable in the one + /// flow every operator takes. The operator asking for this described running the CLI by + /// accident while trying to open the options, which is the failure exactly: a keystroke that + /// looks like "show me more" performed an irreversible side effect instead. + /// + /// So the first `Enter` expands whatever is still folded and the second runs. Returning to the + /// command list and re-selecting re-collapses everything ([`Self::select_at_cursor`]), which + /// keeps the two-press shape stable rather than depending on what the operator opened last time. + /// + /// # Why this does not simply gate on "has the operator pressed Enter once" + /// + /// A press counter would be a second source of truth about the same thing and would drift from + /// what is on screen — an operator who expanded the section with `Space` would still owe a + /// wasted `Enter`. The condition is therefore the *visible state*: if anything is folded, this + /// press unfolds it. Once the screen shows everything, `Enter` means run, which is what the + /// footer advertises. + fn reveal_options_or_run(&mut self) -> bool { + if self.has_folded_options() { + self.expand_all_options(); + return true; + } + self.run_selected() + } + + /// Is any part of the form still folded away? + /// + /// The pickers count only when they actually hold rows: a `Failed` or empty picker renders one + /// unfoldable line, so treating it as "folded" would make the first `Enter` a no-op that never + /// becomes a run — the operator would press `Enter` forever with the server down. + fn has_folded_options(&self) -> bool { + let optional = !self.optional_expanded && !self.field_lines(false).is_empty(); + let agents = !self.agents_expanded && self.agent_row_count() > 0; + let providers = !self.providers_expanded && self.provider_row_count() > 0; + optional || agents || providers + } + + /// Unfolds every foldable region, so the second `Enter` runs against a fully visible form. + fn expand_all_options(&mut self) { + if !self.field_lines(false).is_empty() { + self.optional_expanded = true; + } + if self.agent_row_count() > 0 { + self.agents_expanded = true; + } + if self.provider_row_count() > 0 { + self.providers_expanded = true; + } + } + /// Runs the selected command through the policy-specific production path. fn run_selected(&mut self) -> bool { let Some(id) = self.flow.current() else { @@ -1881,7 +2412,7 @@ impl<'a, S: ServerApi, H: Host> Renderer<'a, S, H> { paragraph(&frame.header).render(header, buf); paragraph(&frame.footer).render(footer, buf); - let mut left: Vec = Vec::new(); + let mut left: Vec> = Vec::new(); left.extend(frame.command_list.iter().cloned()); left.extend(frame.required_fields.iter().cloned()); left.extend(frame.optional_section.iter().cloned()); @@ -1937,9 +2468,27 @@ impl<'a, S: ServerApi, H: Host> Renderer<'a, S, H> { } } +/// Owned strings as unstyled [`Line`]s — the identity conversion for a region with no roles. +/// +/// A region that carries no styling still has to be `Vec>`, and going through this +/// rather than `Line::from` at each call site makes the unstyled regions greppable: `plain(` marks +/// every place #556 deliberately left alone. +fn plain(lines: Vec) -> Vec> { + lines.into_iter().map(Line::raw).collect() +} + /// A `Paragraph` over owned lines, wrapping rather than truncating (NFR-6). -fn paragraph(lines: &[String]) -> Paragraph<'_> { - Paragraph::new(lines.join("\n")).wrap(Wrap { trim: false }) +/// +/// # Why `Text::from(Vec)` and not `lines.join("\n")` (#556, OQ-4) +/// +/// The pre-#556 path handed `Paragraph` a single string with embedded newlines. Whether that wraps +/// identically to a `Vec` under `Wrap { trim: false }` was an **open question in the design, +/// not an assumption** — a wrapping regression here would be an NFR-6 violation, and NFR-6 is +/// "wrap, never truncate". It was settled by measurement: a full buffer dump at 100x40, 70x20 and +/// 40x12 across three shell states was captured before the type change and compared after. +/// `the_frames_plain_text_survived_the_line_migration` is the standing form of that check. +fn paragraph(lines: &[Line<'static>]) -> Paragraph<'static> { + Paragraph::new(lines.to_vec()).wrap(Wrap { trim: false }) } /// The pane's state as a word, so no state is conveyed by colour alone (NFR-3). @@ -1954,6 +2503,113 @@ fn pane_state_word(state: PaneState) -> &'static str { } } +/// The `(required)` suffix [`render_field`] writes, and the marker `style_form` looks for. +/// +/// One constant read by both, so the style cannot drift from the wording. Retyping it at the +/// styling site is how a role silently stops applying after a copy edit. (#556) +const REQUIREMENT_SUFFIX: &str = " (required)"; + +/// What [`render_field`] renders for a field with no value — the em dash, not an empty string. +/// +/// `style_form` tests for this to decide whether a `(required)` suffix is still outstanding, which +/// keeps the style agreeing with the text on its own line rather than re-deriving "unset" from +/// `GuidedFlow`. (#556) +const UNSET_VALUE: &str = "—"; + +/// The prefix `GuidedFlow::blocked_reason` puts on the footer's gating reason. +/// +/// Matched rather than reconstructed, for the same reason as [`REQUIREMENT_SUFFIX`]. The wording +/// itself lives in `guided_flow.rs` with the rule that produces it. (#556) +const BLOCKED_PREFIX: &str = "blocked:"; + +/// The substring that marks a picker line as a FAILURE, as `picker_lines` writes it. +/// +/// `": unavailable — "` and not bare `"unavailable"`: the word also occurs inside +/// `UNLOADABLE_MARKER`'s explanatory text and in a provider row, and matching it loosely would +/// paint a working picker's row as a region failure. (#556) +const PICKER_FAILURE_MARKER: &str = ": unavailable — "; + +/// The suffix `picker_lines` puts on an uninstalled provider row. (#556) +const NOT_INSTALLED_MARKER: &str = "(not installed)"; + +/// The substring marking an expanded picker's off-window residue line, as `fold_lines` writes it. +/// +/// A named constant rather than a literal in both the producer and `style_pickers`, so the two +/// cannot drift into a residue line that is never styled — the failure mode that a hand-matched +/// marker always eventually reaches. +const SCROLL_RESIDUE_MARKER: &str = "[↑↓] scroll"; + +/// A line split into three spans, the middle one styled: `before`, `needle`, `after`. +/// +/// The concatenation is **exactly** the input, so [`Frame::plain_lines`] is unchanged by styling — +/// which is what lets the FR-5.2 guards assert on plain text and the pre-#556 tests keep passing. +/// A line whose plain text changed when a style was added would be a content change wearing a +/// styling change's clothes. +/// +/// Falls back to an unstyled line when the needle is absent rather than panicking: a caller that +/// checked `contains` first cannot reach that branch, and a missing needle means "nothing to +/// style", not a broken invariant. `styling_a_line_preserves_its_plain_text` covers both. (#556) +fn split_styled(line: &str, needle: &str, style: Style) -> Line<'static> { + match line.split_once(needle) { + Some((before, after)) => Line::from(vec![ + Span::raw(before.to_string()), + Span::styled(needle.to_string(), style), + Span::raw(after.to_string()), + ]), + None => Line::raw(line.to_string()), + } +} + +/// A failed picker's line takes `error`; a listed-but-unusable *row* takes `dim` (FR-4.4). +/// +/// # The one place FR-4.4 needed a ruling +/// +/// FR-4.4 asks for both "a picker's failure line (`theme.error`)" and "an `unavailable` line +/// (`theme.dim`)" — and in this crate those are **the same string**: a failed picker renders +/// `agents: unavailable — {error}. Press [ctrl+r] to retry`. One line cannot carry two roles. +/// +/// Resolved by the design's own role docs, which gloss `dim` as `[unavailable]` — a bracketed *row* +/// marker, not a region failure. So the split is by what the line reports: +/// +/// - the picker **failed** → `error`. The whole read is unavailable and there is a remedy to press. +/// - a **row** is listed but unusable (`[unloadable — …]`, `(not installed)`) → `dim`, on the marker +/// only. The picker worked; this entry is informational, and FR-1.5/FR-1.7 require it be *shown* +/// rather than filtered. Dimming the whole row would work against that. +/// +/// A `Loading` or `none reported` line is left unstyled: neither is a failure, and `none reported` +/// is a valid answer from a machine with no profiles — colouring it `error` would report a fault +/// where there is none, which is the exact conflation `picker_lines` is written to avoid. +fn style_pickers(lines: Vec, theme: &Theme) -> Vec> { + let unloadable = format!("[{UNLOADABLE_MARKER}]"); + + lines + .into_iter() + .map(|line| { + if line.contains(PICKER_FAILURE_MARKER) { + return Line::styled(line, theme.error); + } + if line.contains(&unloadable) { + return split_styled(&line, &unloadable, theme.dim); + } + if line.contains(NOT_INSTALLED_MARKER) { + return split_styled(&line, NOT_INSTALLED_MARKER, theme.dim); + } + // A focused fold header, same `>`-prefix convention and same `focus` role as every + // other region's marked row (FR-4.4/FR-4.5). Checked AFTER the markers above so a + // focused header that also carries a dim marker keeps the diagnosis visible — but a + // header only ever holds a count, so in practice these do not overlap. + if line.starts_with('>') { + return Line::styled(line, theme.focus); + } + // The scroll residue is navigational chrome, not content the operator acts on. + if line.contains(SCROLL_RESIDUE_MARKER) { + return Line::styled(line, theme.dim); + } + Line::raw(line) + }) + .collect() +} + /// `cao session list`, from a catalog row. `None` parent means a top-level leaf. fn render_command_path(command: &Command) -> String { match command.parent { @@ -2356,13 +3012,58 @@ mod tests { use crate::guided_flow::PickerState; use crate::handoff::{Host, ServerRead}; use crate::results_pane::PaneState; + use crate::theme::Theme; use crate::types::{Health, Profile, Provider, SessionParams, Terminal, TerminalStatus}; use crossterm::event::{KeyCode, KeyModifiers}; + use ratatui::style::Color; + use ratatui::text::Line; use std::cell::{Cell, RefCell}; use std::collections::VecDeque; use std::io::Write; use std::time::Duration; + /// One region's text, styles discarded — the `Vec` replacement for `region.join(sep)`. + /// + /// #556 retyped `Frame`'s regions from `Vec` to `Vec>`, which broke the + /// pre-existing `.join()` calls. The tests were **repointed here, not rewritten**: every one of + /// them asserts on content, and a rewritten assertion is a chance to weaken one. This discards + /// exactly what those tests never looked at. + fn joined(region: &[Line<'static>], separator: &str) -> String { + region + .iter() + .map(Line::to_string) + .collect::>() + .join(separator) + } + + /// Presses `[enter]` until it means "run" — i.e. consumes the reveal press. + /// + /// `[enter]` on the form reveals the folded options on its first press and runs on the second + /// (`Renderer::reveal_options_or_run`). Tests that care about *running* went through this rather + /// than gaining a second bare `on_key(Enter)` each: a literal extra press states nothing about + /// why it is there, and the next person to change the fold count has to find every one of them. + /// + /// Asserts the reveal press was CONSUMED, so this cannot silently degrade into a no-op if the + /// two-press behaviour is removed — it would then run on the first press and this helper's + /// second press would land on a running form. + fn press_enter_to_run(shell: &mut Renderer<'_, S, H>) { + if shell.has_folded_options() { + assert!( + shell.on_key(KeyCode::Enter), + "the reveal press must be handled — an unhandled [enter] leaves the options folded" + ); + assert!( + !shell.has_folded_options(), + "one reveal press must unfold EVERYTHING, or the operator owes an unpredictable \ + number of presses before a run" + ); + } + assert!( + shell.on_key(KeyCode::Enter), + "[enter] must be handled on a fully revealed form" + ); + } + /// **This module's own source text**, embedded at compile time. /// /// FR-3.2 is a claim about **where** the pane is called from, and that is a property of the @@ -2918,7 +3619,7 @@ mod tests { // The RENDERED region, not the ring buffer: the outcome line comes from the pane's own // `Widget` impl and is absent from `lines()`. Asserting on `lines()` here reported an empty // pane after a successful hand-off — the defect this frame accessor was corrected for. - let rendered = shell.render().results.join("\n"); + let rendered = joined(&shell.render().results, "\n"); assert!( rendered.contains("launched in new window") && rendered.contains("session: work"), "HANDOFF's completion shape is the STRUCTURED OUTCOME LINE, because the command's \ @@ -2941,7 +3642,7 @@ mod tests { "the FR-5.3 refusal arm must leave the pane `refused`. Pane: {:?}", shell.pane() ); - let rendered = shell.render().results.join("\n"); + let rendered = joined(&shell.render().results, "\n"); assert!( rendered.contains("$TMUX is unset"), "the refusal must carry the reason. Got: {rendered:?}" @@ -3146,10 +3847,7 @@ mod tests { // The footer carries it too, since that is where the operator looks before pressing enter. let frame = shell.render(); assert!( - frame - .footer - .iter() - .any(|line| line.contains("blocked: --agents required")), + joined(&frame.footer, "\n").contains("blocked: --agents required"), "the gating reason must be in the footer as text. Footer: {:?}", frame.footer ); @@ -3192,13 +3890,11 @@ mod tests { "a blank screen is indistinguishable from a hang (SR-2). Frame: {frame:?}" ); - let all = frame - .header - .iter() - .chain(&frame.pickers) - .cloned() - .collect::>() - .join("\n"); + let all = format!( + "{}\n{}", + joined(&frame.header, "\n"), + joined(&frame.pickers, "\n") + ); assert!( all.contains("unreachable") || all.contains("unavailable"), "the state must say WHAT failed. Got: {all:?}" @@ -3640,16 +4336,28 @@ mod tests { // And the definition itself is not vacuous: a frame of empty and whitespace-only strings // IS blank by it. Without this, `is_blank` could be `|| false` and every case above would // still pass. + // + // #556 note: the whitespace lines are **STYLED**, which is the new way to get this wrong. + // `is_blank` asks about glyphs, so a bold empty line is still blank; had the migration let + // a style count as content, this fixture would stop being blank and SR-2's predicate would + // have been retired silently (FR-4.3). That is why the styles are here rather than in a + // separate test — the anti-vacuity fixture is the right place for the harder case. + let styled_blank = |text: &str| { + Line::styled( + text.to_string(), + ratatui::style::Style::new().add_modifier(ratatui::style::Modifier::BOLD), + ) + }; let whitespace_only = Frame { layout: LayoutMode::Stacked, - header: vec![String::new(), " ".to_string()], - command_list: vec!["\t".to_string()], + header: vec![styled_blank(""), styled_blank(" ")], + command_list: vec![styled_blank("\t")], required_fields: Vec::new(), optional_section: Vec::new(), - pickers: vec!["\n".to_string()], + pickers: vec![styled_blank("\n")], results: Vec::new(), banner: Vec::new(), - footer: vec![" ".to_string()], + footer: vec![styled_blank(" ")], }; assert!( whitespace_only.is_blank(), @@ -3761,7 +4469,7 @@ mod tests { PaneState::Cancelled, "`[k]` while running must cancel (stop following)" ); - let rendered = shell.render().results.join("\n"); + let rendered = joined(&shell.render().results, "\n"); assert!( rendered.contains("still running"), "SR-4: the cancelled wording must not claim the command stopped — `[k]` stops FOLLOWING \ @@ -3794,6 +4502,21 @@ mod tests { Some(&crate::guided_flow::FieldValue::Text("planner".to_string())), "keyboard entry must mutate the production GuidedFlow, not a renderer-only buffer" ); + // FIRST Enter reveals the folded options; it must NOT run. An operator reported the old + // behaviour as running the CLI by accident while trying to open the options. + assert!(shell.on_key(KeyCode::Enter)); + assert_eq!( + server.create_session_calls.get(), + 0, + "the first [enter] must reveal the folded options, never reach the network" + ); + assert!( + shell.pending_action.is_none(), + "the first [enter] must not even QUEUE a run — a queued launch executes on the next \ + tick, so queueing here is the same defect one frame later" + ); + + // SECOND Enter runs, because nothing is folded any more. assert!(shell.on_key(KeyCode::Enter)); assert_eq!( server.create_session_calls.get(), @@ -3818,7 +4541,7 @@ mod tests { let mut shell = Renderer::new(&server, &host, 100, 40); assert!(shell.focus_command(CommandId::SessionList)); assert!(shell.on_key(KeyCode::Enter)); - assert!(shell.on_key(KeyCode::Enter)); + press_enter_to_run(&mut shell); assert!( server.run_calls.borrow().is_empty(), "the keypress must queue the in-app run until after the pending draw" @@ -3856,7 +4579,7 @@ mod tests { let host = FakeHost::outside_tmux(); let mut shell = Renderer::new(&server, &host, 200, 60); - let rendered = shell.render().command_list.join("\n"); + let rendered = joined(&shell.render().command_list, "\n"); let mut hidden_seen = Vec::new(); let mut offered_missing = Vec::new(); @@ -3925,20 +4648,32 @@ mod tests { "{id:?} must be unreachable: {why}" ); } + // ABSENCE IS ASSERTED AGAINST THE UNWINDOWED ROWS, not the visible slice. + // + // The list is now windowed around the cursor (42 commands do not fit a column), so a needle + // missing from `render()` proves nothing — it may simply be below the window. Checking + // `command_rows()` asks the question FR-4.3 actually cares about: is the row generated at + // all? Asserting on the windowed text here would have been an absence check satisfied by + // scrolling, which is the weakest kind of green. + let all_rows = shell.command_rows().join("\n"); + for needle in ["cao tui", "cao shutdown"] { + assert!( + !all_rows.contains(needle), + "{needle:?} must not appear ANYWHERE in the list's rows, windowed or not. \ + Got: {all_rows:?}" + ); + } + // Positive control on the same string, so the absence checks are not passing because the + // rows are empty. assert!( - !rendered.contains("cao tui"), - "`cao tui` must not appear in the rendered list. Got: {rendered:?}" - ); - assert!( - !rendered.contains("cao shutdown"), - "`cao shutdown` must not appear in the rendered list. Got: {rendered:?}" + all_rows.contains("cao launch") && all_rows.contains("cao session list"), + "the rows must include the commands the catalog DOES offer, or the absence checks are \ + vacuous. Got: {all_rows:?}" ); - // Positive control on the rendering itself, so the two assertions above are not passing - // because the list renders nothing at all. + // And the windowed rendering is non-empty, so the region the operator sees is populated. assert!( - rendered.contains("cao launch") && rendered.contains("cao session list"), - "the list must render the commands it DOES offer, or the absence checks are vacuous. \ - Got: {rendered:?}" + rendered.contains("cao install"), + "the rendered window must show the rows at the cursor. Got: {rendered:?}" ); // A programmatic caller holding a HIDE id is refused rather than silently doing nothing. @@ -4154,7 +4889,7 @@ mod tests { // And the footer must ADVERTISE it: an escape hatch nobody can discover is not an escape. let shell = Renderer::new(&server, &host, 100, 40); - let footer = shell.render().footer.join(" "); + let footer = joined(&shell.render().footer, " "); assert!( footer.contains("[←]"), "the footer must name `[←]`, or the operator has no way to learn the key exists — the \ @@ -4285,7 +5020,7 @@ mod tests { // is the pane's own render path — the same accessor a prior test was // corrected to use, because `lines()` omits the completion line and reported an empty pane // after a successful run. - let cells = shell.render().results.join("\n"); + let cells = joined(&shell.render().results, "\n"); assert!( !cells.contains("has no HTTP route"), "`profile find` must NOT report a missing route — it is served client-side per OQ-6 Q2. \ @@ -4460,7 +5195,7 @@ mod tests { assert!(shell.focus_command(CommandId::Launch)); assert!(shell.on_key(KeyCode::Enter)); - let header = shell.render().header.join("\n"); + let header = joined(&shell.render().header, "\n"); assert!( header.to_lowercase().contains(expected), "HTTP {status} must be described as an auth failure — the header is the operator's \ @@ -4777,7 +5512,7 @@ mod tests { } assert!(shell.on_key(KeyCode::Enter), "[enter] expands the section"); - let optional = shell.render().optional_section.join("\n"); + let optional = joined(&shell.render().optional_section, "\n"); let yolo = optional .lines() @@ -4807,6 +5542,7 @@ mod tests { .required_fields .iter() .chain(&whole_form.optional_section) + .map(Line::to_string) .find(|line| line.contains("message")); if let Some(line) = message_line { assert!( @@ -4907,7 +5643,7 @@ mod tests { "`[q]` while running must raise a confirmation" ); - let footer = shell.render().footer.join("\n"); + let footer = joined(&shell.render().footer, "\n"); assert!( footer.contains("still running"), "the confirmation must say a command is still running. Got: {footer:?}" @@ -4973,7 +5709,7 @@ mod tests { let mut shell = Renderer::new(&server, &host, 100, 40); assert!(shell.focus_command(CommandId::Launch)); assert!(shell.on_key(KeyCode::Enter)); - assert!(shell.on_key(KeyCode::Enter), "Enter must queue the launch"); + press_enter_to_run(&mut shell); assert!( shell.run_pending_action(), "the queued launch must execute on the next tick" @@ -5026,7 +5762,7 @@ mod tests { .set(field, value) .expect("the field is declared by this command"); } - assert!(shell.on_key(KeyCode::Enter)); + press_enter_to_run(&mut shell); assert!(shell.run_pending_action()); assert!( server.run_calls.borrow().is_empty(), @@ -5583,4 +6319,1117 @@ mod tests { server.run_calls.borrow() ); } + + // ── #556: the semantic colour layer ────────────────────────────────────────────────────── + + /// Renders `shell` and returns every cell as `(symbol, fg)`. + fn drawn_cells( + shell: &Renderer<'_, S, H>, + width: u16, + height: u16, + ) -> Vec<(String, Color)> { + let area = ratatui::layout::Rect::new(0, 0, width, height); + let mut buffer = ratatui::buffer::Buffer::empty(area); + shell.draw(area, &mut buffer); + (0..height) + .flat_map(|row| { + (0..width) + .map(move |column| (column, row)) + .collect::>() + }) + .map(|(column, row)| { + let cell = &buffer[(column, row)]; + (cell.symbol().to_string(), cell.fg) + }) + .collect() + } + + /// The foreground `needle` is drawn in, located by scanning rendered rows. Panics if absent, so + /// "the style is right" cannot degrade into "the text was not there". + fn drawn_foreground( + shell: &Renderer<'_, S, H>, + width: u16, + height: u16, + needle: &str, + ) -> Color { + assert!(needle.is_ascii(), "row indexing here is by byte offset"); + let cells = drawn_cells(shell, width, height); + for row in cells.chunks(width as usize) { + let text: String = row.iter().map(|(symbol, _)| symbol.as_str()).collect(); + if let Some(start) = text.find(needle) { + let colours: Vec = row[start..start + needle.len()] + .iter() + .map(|(_, fg)| *fg) + .collect(); + let first = colours[0]; + assert!( + colours.iter().all(|fg| *fg == first), + "{needle:?} is drawn in more than one colour ({colours:?})" + ); + return first; + } + } + let all: String = cells.iter().map(|(symbol, _)| symbol.as_str()).collect(); + panic!("{needle:?} was not rendered, so its style could not be read. Got: {all:?}"); + } + + /// A shell with the pickers failed, which is the state most of the roles are visible in. + fn shell_with_failed_pickers<'a>( + server: &'a FakeServer, + host: &'a FakeHost, + ) -> Renderer<'a, FakeServer, FakeHost> { + let mut shell = Renderer::new(server, host, 100, 40); + assert!(shell.focus_command(CommandId::Launch)); + assert!(shell.on_key(KeyCode::Enter)); + shell + } + + /// **FR-4.2 / OQ-4 — the `Vec` → `Vec` migration changed no text.** + /// + /// # This test answers an open question, and the answer was measured + /// + /// The old draw path handed `Paragraph` one string with embedded newlines; the new one hands it + /// a `Vec`. Whether those wrap identically under `Wrap { trim: false }` was OQ-4 in the + /// design — flagged as **unverified**, because a wrapping regression here is an NFR-6 violation + /// and NFR-6 is "wrap, never truncate". + /// + /// It was settled by capturing a full buffer dump at 100x40, 70x20 and 40x12 across three shell + /// states **before** the type change and diffing after: 23,237 bytes, identical, including + /// wrapped continuation rows at 40 columns. **The answer to OQ-4 is yes, they wrap the same.** + /// + /// This is the standing form of that check. It re-derives the plain text from the frame and + /// compares it to the same text read out of the rendered buffer, at three sizes — so a future + /// change that makes styling alter the *text* fails here rather than in a screenshot. + #[test] + fn the_frames_plain_text_survived_the_line_migration() { + let server = FakeServer::unreachable(); + let host = FakeHost::outside_tmux(); + let shell = shell_with_failed_pickers(&server, &host); + + // A styled line's plain text must equal the string the producer built. `split_styled` + // guarantees this by construction (its three spans concatenate to the input) and this is + // where that guarantee is checked rather than assumed. + let frame = shell.render(); + for (region_name, region) in [ + ("required_fields", &frame.required_fields), + ("optional_section", &frame.optional_section), + ("pickers", &frame.pickers), + ("footer", &frame.footer), + ("command_list", &frame.command_list), + ] { + for line in region { + let from_spans: String = line + .spans + .iter() + .map(|span| span.content.as_ref()) + .collect(); + assert_eq!( + line.to_string(), + from_spans, + "in {region_name}, a styled line's text must be exactly its spans concatenated \ + — if styling can change the text, every plain-text guard in this crate is \ + asserting on something the operator does not see" + ); + } + } + + // And nothing is empty, or the loop above proved nothing. + assert!( + frame + .plain_lines() + .iter() + .filter(|line| !line.trim().is_empty()) + .count() + > 10, + "the fixture must produce real content. Got: {:?}", + frame.plain_lines() + ); + } + + /// **FR-4.4 / FR-4.5 — `focus` is on the marker's LINE, and there is no border to put it on.** + /// + /// The issue's palette table assigns `focus` to the "focused region border, selected row". This + /// crate has **neither**: one borderless `Block::new()`, no `List`/`Table`/`highlight_style` + /// (design C-2, P-22). So the role goes on the structural `>` marker's row, and this test reads + /// it off the rendered cells to prove the translation actually happened. + /// + /// The viewport is 120x90 rather than 100x40 deliberately: at 40 rows the 30-row command list + /// fills the form region and the focused field row is **scrolled off**, so the assertion would + /// panic on absence rather than on the wrong colour. Measured, not guessed. + #[test] + fn the_focus_marker_row_carries_the_focus_role() { + let server = FakeServer::unreachable(); + let host = FakeHost::outside_tmux(); + let mut shell = shell_with_failed_pickers(&server, &host); + shell.set_theme(Theme::colour()); + + let expected = Theme::colour().focus.fg.expect("`focus` sets a foreground"); + assert_eq!( + drawn_foreground(&shell, 120, 90, "> --agents"), + expected, + "the focused row must carry `focus`. There is no border and no selected-row widget in \ + this crate, so the `>` marker's line IS where FR-4.5 puts it" + ); + } + + /// **FR-4.4 — an unset required field's `(required)` carries `required`, and a set one does not.** + /// + /// The second half is the point. Colouring a *satisfied* required field keeps a warning on + /// screen after the warning has been answered, which trains the operator to ignore the colour — + /// so "the style is present" is only half a requirement, and a test that checked only presence + /// would pass on an implementation that never turns it off. + /// + /// Read from the frame rather than the cells because the focused row takes `focus` for its whole + /// line, which would mask the `required` span: the field must be unset AND unfocused to see it, + /// and that combination is easier to construct honestly at the frame level. + #[test] + fn a_required_suffix_is_styled_only_while_the_field_is_unset() { + let server = FakeServer::healthy(); + let host = FakeHost::outside_tmux(); + let mut shell = Renderer::new(&server, &host, 100, 40); + shell.set_theme(Theme::colour()); + assert!(shell.focus_command(CommandId::Launch)); + assert!(shell.on_key(KeyCode::Enter)); + + let required_fg = Theme::colour() + .required + .fg + .expect("`required` sets a foreground"); + + // Move focus off the fields so `focus` does not take the whole row. + let styled_suffix = |shell: &Renderer<'_, FakeServer, FakeHost>| -> Vec { + shell + .render() + .required_fields + .iter() + .filter(|line| !line.to_string().starts_with('>')) + .flat_map(|line| line.spans.clone()) + .filter(|span| span.content.contains("(required)")) + .filter_map(|span| span.style.fg) + .collect() + }; + + // Unfocus the form: `[←]` returns to the command list. + assert!(shell.on_key(KeyCode::Left)); + let unset = styled_suffix(&shell); + assert_eq!( + unset, + vec![required_fg], + "an UNSET required field's `(required)` must carry the `required` role. Frame: {:?}", + shell + .render() + .required_fields + .iter() + .map(Line::to_string) + .collect::>() + ); + + // Now satisfy it — by TYPING, through `on_key`, which is how an operator does it. A + // test-only setter would have let the assertion pass against a state the keyboard cannot + // reach. + assert!( + shell.on_key(KeyCode::Tab), + "[tab] moves focus into the form" + ); + for character in "researcher".chars() { + assert!( + shell.on_key(KeyCode::Char(character)), + "typing {character:?} into the focused field must be consumed" + ); + } + assert!(shell.on_key(KeyCode::Left)); + + let plain = shell.render(); + let text = joined(&plain.required_fields, "\n"); + assert!( + text.contains("(required)"), + "the WORDING must survive — a satisfied required field is still required, and NFR-3 \ + says the text carries the meaning. Got: {text:?}" + ); + assert_eq!( + styled_suffix(&shell), + Vec::::new(), + "a SATISFIED required field must NOT be coloured. Leaving the warning up after it is \ + answered is how an operator learns to ignore the colour. Got: {text:?}" + ); + } + + /// **FR-4.4 — a failed picker's line carries `error`; a listed-but-unusable row carries `dim`.** + /// + /// # The one place FR-4.4 needed a ruling, recorded + /// + /// FR-4.4 asks for both "a picker's failure line (`theme.error`)" and "an `unavailable` line + /// (`theme.dim`)", and in this crate **those are the same string** — a failed picker renders + /// `agents: unavailable — {error}. Press [ctrl+r] to retry`. One line cannot carry two roles. + /// + /// Resolved against the design's own role docs, which gloss `dim` as `[unavailable]` — a + /// bracketed *row* marker, not a region failure. A failed picker is a failure with a remedy to + /// press, so it is `error`; an unloadable profile row is informational and FR-1.5 requires it be + /// shown rather than filtered, so only its marker dims. + #[test] + fn a_failed_picker_line_carries_error() { + let server = FakeServer::unreachable(); + let host = FakeHost::outside_tmux(); + let mut shell = shell_with_failed_pickers(&server, &host); + shell.set_theme(Theme::colour()); + + let expected = Theme::colour().error.fg.expect("`error` sets a foreground"); + for picker in ["agents: unavailable", "providers: unavailable"] { + assert_eq!( + drawn_foreground(&shell, 120, 90, picker), + expected, + "a failed picker must carry `error` — the read is unavailable and there is a \ + [ctrl+r] remedy. {picker:?}" + ); + } + } + + /// **FR-4.4 — the footer's blocked reason carries `required`, not `error`.** + /// + /// A blocked form is not a failure, it is an unfinished one: the reason names the field the + /// operator still has to fill. `error` here would report a problem where there is only an + /// incomplete step, and the palette already has a role meaning "you need to supply this". + /// Asserted as an inequality too, so the distinction is load-bearing rather than incidental. + #[test] + fn the_footers_blocked_reason_carries_required_not_error() { + let server = FakeServer::healthy(); + let host = FakeHost::outside_tmux(); + let mut shell = Renderer::new(&server, &host, 100, 40); + shell.set_theme(Theme::colour()); + assert!(shell.focus_command(CommandId::Launch)); + assert!(shell.on_key(KeyCode::Enter)); + + let theme = Theme::colour(); + let required_fg = theme.required.fg.expect("`required` sets a foreground"); + let error_fg = theme.error.fg.expect("`error` sets a foreground"); + assert_ne!( + required_fg, error_fg, + "precondition: if `required` and `error` were the same colour this test could not fail" + ); + + let actual = drawn_foreground(&shell, 100, 40, "blocked:"); + assert_eq!( + actual, required_fg, + "the blocked reason must carry `required`: the form is unfinished, not broken" + ); + assert_ne!( + actual, error_fg, + "and specifically NOT `error` — a gate the operator can clear by typing a value is \ + not a failure to report" + ); + } + + /// **FR-5.2 / FR-5.4 — THE RENDERER'S STRIP-STYLING GUARD. Read before changing wording.** + /// + /// Four states, each identifiable from **plain text alone**: nothing selected, a required field + /// unset, a picker failed, and a run finished with a non-zero exit. This is NFR-3's "no state by + /// colour alone" in executable form for the renderer, and the property it defends is not about + /// colour — it is about whether the words are sufficient. + /// + /// Reads [`Frame::plain_lines`], which discards every style. That is what a screen reader, a + /// pipe, a `TERM=dumb` terminal and a monochrome operator receive. + /// + /// **Mutation-proven (FR-5.4):** removing the footer's `blocked:` reason turns this red. + #[test] + fn every_renderer_state_is_identifiable_from_plain_text() { + let healthy = FakeServer::healthy(); + let unreachable = FakeServer::unreachable(); + let failing_server = FakeServer::healthy().with_run(RunAnswer::Chunks(vec!["boom\n"], 500)); + let host = FakeHost::outside_tmux(); + + // (label, shell, the marker the state must be identifiable BY) + let mut cases: Vec<(&str, Renderer<'_, FakeServer, FakeHost>, &str)> = Vec::new(); + + // 1. Nothing selected. The footer says so rather than showing an inert control (FR-6.2). + let mut nothing = Renderer::new(&healthy, &host, 100, 40); + nothing.set_theme(Theme::colour()); + cases.push(("nothing selected", nothing, "no command selected")); + + // 2. A command selected with its required field unset. + let mut unset = Renderer::new(&healthy, &host, 100, 40); + unset.set_theme(Theme::colour()); + assert!(unset.focus_command(CommandId::Launch)); + assert!(unset.on_key(KeyCode::Enter)); + cases.push(("required field unset", unset, "blocked: --agents required")); + + // 3. A picker failed. Cause AND remedy, both textual (FR-6.1). + let mut failed = shell_with_failed_pickers(&unreachable, &host); + failed.set_theme(Theme::colour()); + cases.push(("picker failed", failed, "unavailable")); + + // 4. A run complete with a NON-ZERO exit, reached through the REAL path: `run_in_app` + // maps an HTTP status >= 400 to `complete(1)`, so a fake 500 produces `exit 1` the same way + // production does. A test-only setter would have asserted about a state the API cannot make. + let mut failing = Renderer::new(&failing_server, &host, 100, 40); + failing.set_theme(Theme::colour()); + failing.run_in_app(CommandId::SessionList); + assert_eq!( + failing.pane().state(), + PaneState::Complete, + "precondition: the fixture must have reached a terminal state with a code" + ); + cases.push(("non-zero exit", failing, "exit 1")); + + assert_eq!( + cases.len(), + 4, + "FR-5.2 names four states; all four must be covered" + ); + + for (label, shell, marker) in cases { + let plain = shell.render().plain_lines().join("\n"); + assert!( + plain.contains(marker), + "the {label:?} state is NOT identifiable from plain text.\n\ + \n\ + Expected {marker:?} somewhere in the frame's plain lines, got:\n{plain}\n\ + \n\ + This is FR-5.2 and NFR-3: no state may be conveyed by colour alone. If you got \ + here by moving a distinction into a hue, move it back into the words." + ); + } + } + + /// The renderer's palette **actually reaches the cells** — the anti-vacuity floor for T-5. + /// + /// [`every_renderer_state_is_identifiable_from_plain_text`] passes *by design* when nothing is + /// styled, and the monochrome test below passes trivially. Without this, the whole styling half + /// of T-5 could be a no-op with a green suite. + #[test] + fn the_renderers_palette_reaches_the_rendered_cells() { + let server = FakeServer::unreachable(); + let host = FakeHost::outside_tmux(); + let mut shell = shell_with_failed_pickers(&server, &host); + shell.set_theme(Theme::colour()); + + let palette: Vec = Theme::colour() + .roles() + .iter() + .filter_map(|(_, style)| style.fg) + .collect(); + + let coloured: Vec<(String, Color)> = drawn_cells(&shell, 120, 90) + .into_iter() + .filter(|(_, fg)| *fg != Color::Reset) + .collect(); + + assert!( + coloured.len() > 20, + "only {} cells carried any colour. The renderer's styling is a no-op, and every other \ + styling assertion in this module is satisfied by that", + coloured.len() + ); + for (symbol, fg) in coloured { + assert!( + palette.contains(&fg), + "a cell {symbol:?} was drawn in {fg:?}, which is not one of the theme's roles \ + ({palette:?}). A colour that is not a role is one `NO_COLOR` cannot switch off \ + (FR-1.4)" + ); + } + } + + /// **FR-3.3 end-to-end for the renderer — under `NO_COLOR`, every cell is `Color::Reset`.** + /// + /// The half that catches a colour applied *outside* the theme: an inline `Color::Green` at a + /// call site satisfies every assertion in `theme.rs` and fails here. + #[test] + fn under_no_color_the_whole_renderer_draws_in_reset() { + let server = FakeServer::unreachable(); + let host = FakeHost::outside_tmux(); + let mut shell = shell_with_failed_pickers(&server, &host); + shell.set_theme(Theme::monochrome()); + + for (symbol, fg) in drawn_cells(&shell, 120, 90) { + assert_eq!( + fg, + Color::Reset, + "cell {symbol:?} was drawn in {fg:?} under NO_COLOR. Every monochrome role is \ + Color::Reset, so a coloured cell means a style bypassed the theme — an inline \ + colour `from_env` cannot switch off (FR-3.3, FR-1.4)" + ); + } + } + + /// A styled [`Line`] `Display`s **without escape codes**, which the piped path in `main` relies on. + /// + /// `main`'s non-interactive branch writes `frame.header`/`footer` with `{line}` straight to a + /// pipe. That is only safe because `Span`'s `Display` writes content and no SGR sequences — true + /// in ratatui-core 0.1.2, and now asserted, because a dependency on an upstream `Display` impl + /// with no test behind it is what silently breaks on a minor-version bump (SR-1). + #[test] + fn a_styled_line_displays_without_escape_codes() { + let server = FakeServer::unreachable(); + let host = FakeHost::outside_tmux(); + let mut shell = shell_with_failed_pickers(&server, &host); + shell.set_theme(Theme::colour()); + let frame = shell.render(); + + // Anti-vacuity: the fixture must actually BE styled, or this proves nothing about styling. + assert!( + frame.pickers.iter().any(|line| line.style.fg.is_some() + || line.spans.iter().any(|span| span.style.fg.is_some())), + "the fixture must contain a styled line for this test to mean anything" + ); + + for line in frame + .header + .iter() + .chain(&frame.footer) + .chain(&frame.pickers) + { + let displayed = line.to_string(); + assert!( + !displayed.bytes().any(|byte| byte == 0x1b), + "a styled Line must Display without an ESC byte — main pipes these straight to \ + stdout when stdout is not a terminal. Got: {displayed:?}" + ); + assert!( + !displayed.contains("[3") && !displayed.contains("[0m"), + "no SGR residue either. Got: {displayed:?}" + ); + } + } + /// **FR-4.5 — the command list's own marked row carries `focus`.** + /// + /// # Why this exists as a separate test from the form's marked row + /// + /// Two different functions put `focus` on a `>` row: [`Renderer::style_focus_marker`] serves + /// `command_list`, and [`Renderer::style_form`] serves the two form regions. They are separate + /// because the form's mapping also has to reach `(required)` and `[not sent — …]` spans, which + /// the command list has no equivalent of. + /// + /// A mutation proved the split matters: reducing `style_focus_marker` to a bare `Line::raw` + /// left the whole suite green, because every other `focus` assertion in this module targets a + /// *form* row (`> --agents`) and so exercises `style_form` instead. This test is the one that + /// fails when `style_focus_marker` stops styling. + /// + /// The needle is a prefix, not the whole 86-character row, so it cannot be pushed onto a + /// second visual line by wrapping — `drawn_foreground` reads a needle out of one rendered row. + #[test] + fn the_command_lists_marked_row_carries_the_focus_role() { + let server = FakeServer::healthy(); + let host = FakeHost::outside_tmux(); + let mut shell = Renderer::new(&server, &host, 100, 40); + shell.set_theme(Theme::colour()); + assert_eq!( + shell.focus(), + Focus::CommandList, + "the command list is focused on open, which is what puts a `>` in it at all" + ); + + let expected = Theme::colour().focus.fg.expect("`focus` sets a foreground"); + assert_eq!( + drawn_foreground(&shell, 120, 90, "> cao install"), + expected, + "the command list's marked row must carry `focus`; without this the region's whole \ + styling function can be deleted with the suite still green" + ); + + // And the unmarked rows must not: a list where every row is cyan says nothing about focus. + let unmarked = drawn_foreground(&shell, 120, 90, " cao launch"); + assert_ne!( + unmarked, expected, + "an unmarked command row must not carry `focus`, or the colour stops distinguishing \ + the cursor's row from the other 41" + ); + } + + /// **FR-4.4 — every `dim` marker: `[not sent — …]`, `[unloadable — …]`, `(not installed)`.** + /// + /// # Why all three in one test, and why each has a negative half + /// + /// These are the three strings the design's `dim` gloss covers, and they come from two + /// different functions ([`Renderer::style_form`] and [`style_pickers`]). Asserting one and + /// trusting the other two is how the [`Renderer::style_focus_marker`] gap happened: a role can + /// be "covered" while a whole call site of it is unstyled. + /// + /// `dim` goes on the **marker only**, never the row, and that is the point of the negative + /// half. FR-1.5 and FR-1.7 require an unloadable profile and an uninstalled provider to be + /// *shown* rather than filtered; dimming the whole row would work against the requirement it + /// is meant to serve, by making the row that most needs reading the hardest to read. + /// + /// Needles are ASCII prefixes of each marker — [`drawn_foreground`] indexes a row by byte + /// offset, and all three markers contain an em dash or run past one screen row. + #[test] + fn every_dim_marker_dims_its_marker_and_not_its_row() { + let server = FakeServer::healthy(); + // An unloadable profile and an uninstalled provider, alongside usable ones — the usable + // rows are what the negative half needs. + *server.profiles.borrow_mut() = + Ok(vec![profile("planner", true), profile("broken", false)]); + *server.providers.borrow_mut() = Ok(vec![provider("kiro_cli", false)]); + let host = FakeHost::outside_tmux(); + let mut shell = Renderer::new(&server, &host, 100, 40); + shell.set_theme(Theme::colour()); + assert!(shell.focus_command(CommandId::Launch)); + assert!(shell.on_key(KeyCode::Enter)); + + let dim = Theme::colour().dim.fg.expect("`dim` sets a foreground"); + + // The pickers are collapsed by default, so their ROWS are off-screen until expanded. Driven + // through the real key handler rather than by setting the flags, so a fold that cannot be + // opened by keyboard fails here rather than being asserted around. + for region in [Focus::AgentPicker, Focus::ProviderPicker] { + while shell.focus() != region { + assert!( + shell.on_key(KeyCode::Tab), + "tab must reach {region:?} — an unreachable fold cannot be opened at all" + ); + } + assert!( + shell.on_key(KeyCode::Enter), + "[enter] must expand {region:?}" + ); + } + + // Two picker row markers. + assert_eq!( + drawn_foreground(&shell, 160, 90, "[unloadable"), + dim, + "an unloadable profile's marker must be `dim`: the picker worked, this row is \ + informational" + ); + assert_eq!( + drawn_foreground(&shell, 160, 90, "(not installed)"), + dim, + "an uninstalled provider's marker must be `dim` — `installed` is display information, \ + never a filter (FR-1.7)" + ); + // ...and neither row is dimmed as a whole, or FR-1.5/FR-1.7's "shown, not hidden" is + // undone by the styling. + for row in ["broken", "kiro_cli"] { + assert_ne!( + drawn_foreground(&shell, 160, 90, row), + dim, + "{row:?} is the name the operator has to read; only its marker dims" + ); + } + + // The `[not sent — …]` marker, in the collapsed optional section. + while shell.focus() != Focus::OptionalSection { + assert!( + shell.on_key(KeyCode::Tab), + "tab must reach the optional section from the form" + ); + } + assert!(shell.on_key(KeyCode::Enter), "[enter] expands the section"); + + assert_eq!( + drawn_foreground(&shell, 160, 90, "[not sent"), + dim, + "the `[not sent — …]` marker must be `dim`: the field is present and explained, and \ + the explanation is secondary to the field's own name" + ); + assert_ne!( + drawn_foreground(&shell, 160, 90, "--yolo"), + dim, + "the flag's own name must not dim — an operator hunting for `--yolo` has to be able \ + to find it (FR-1.5's posture, applied to the form)" + ); + } + + // ── The picker folds ────────────────────────────────────────────────────────────────────── + + /// A server answering with `count` agents, named `agent-00`.. so each row is findable. + fn server_with_many_agents(count: usize) -> FakeServer { + let server = FakeServer::healthy(); + *server.profiles.borrow_mut() = Ok((0..count) + .map(|index| profile(&format!("agent-{index:02}"), true)) + .collect()); + server + } + + /// Selects `cao launch` and returns the shell, pickers populated. + fn shell_on_the_launch_form<'a, S: ServerApi, H: Host>( + server: &'a S, + host: &'a H, + cols: u16, + rows: u16, + ) -> Renderer<'a, S, H> { + let mut shell = Renderer::new(server, host, cols, rows); + assert!(shell.focus_command(CommandId::Launch)); + assert!(shell.on_key(KeyCode::Enter)); + shell + } + + /// **25 agents must not push the rest of the column off the screen.** + /// + /// The reported defect: the left column is one `Paragraph` and does not scroll, so with 25 + /// profiles the agent list ran past the last row and took the *banner* with it — a failure the + /// operator needed to read became invisible because a list above it was long. + /// + /// Asserted as a bound on the region's own height rather than by eyeballing a screenshot: the + /// fold's whole purpose is that picker height stops tracking profile count, and `<= 3` is the + /// two collapsed headers plus slack. A regression to the unfolded list makes this 28. + /// + /// The negative half is what stops the fold from being a delete: the count and the `[enter]` + /// that opens it must both still be on screen, or the operator has no way to learn 25 agents + /// exist. That is the FR-1.5/FR-1.7 posture applied to the fold. + #[test] + fn a_long_agent_list_stays_bounded_and_still_states_its_count() { + let server = server_with_many_agents(25); + let host = FakeHost::outside_tmux(); + let shell = shell_on_the_launch_form(&server, &host, 100, 40); + + let frame = shell.render(); + assert!( + frame.pickers.len() <= 3, + "25 agents must not occupy 25 rows — the column does not scroll, so a long list \ + clips the banner off the bottom. Got {} lines: {:?}", + frame.pickers.len(), + frame.plain_lines() + ); + + let pickers = joined(&frame.pickers, "\n"); + assert!( + pickers.contains("agents (25)"), + "the collapsed header must state the COUNT — a fold that hides the number leaves the \ + operator unable to tell 25 agents from none. Got: {pickers:?}" + ); + assert!( + pickers.contains("[enter]"), + "the collapsed header must name the key that opens it; an unadvertised fold is a \ + hidden list (NFR-3). Got: {pickers:?}" + ); + } + + /// **Every one of 25 agents is reachable by keyboard once expanded.** + /// + /// This is the half a `… N more` cap could not satisfy, and the reason scrolling was chosen + /// over one: the operator with 25 agents has to be able to *see agent 25*. Scrolls to the + /// bottom through the real key handler and asserts the last row arrived. + /// + /// The `assert_ne!` on the first row is the anti-vacuity floor. Without it a build whose window + /// never moved would still pass the "last row is visible" check if the window happened to be + /// tall enough to hold everything — the test would then be asserting the window size, not the + /// scrolling. At 40 rows the window is 10 and the list is 25, so the two views must differ. + #[test] + fn every_agent_is_reachable_by_scrolling_the_expanded_fold() { + let server = server_with_many_agents(25); + let host = FakeHost::outside_tmux(); + let mut shell = shell_on_the_launch_form(&server, &host, 100, 40); + + while shell.focus() != Focus::AgentPicker { + assert!(shell.on_key(KeyCode::Tab), "tab must reach the agent fold"); + } + assert!(shell.on_key(KeyCode::Enter), "[enter] must expand the fold"); + + let expanded = joined(&shell.render().pickers, "\n"); + assert!( + expanded.contains("agent-00"), + "an expanded fold must show the top of the list. Got: {expanded:?}" + ); + assert!( + !expanded.contains("agent-24"), + "the window must be SMALLER than the list at 40 rows, or this test cannot tell \ + scrolling from a window that already fits everything. Got: {expanded:?}" + ); + + // 25 presses is more than the clamp needs, which is the point: over-scrolling must stop at + // the last window rather than running off the end. + for _ in 0..25 { + shell.on_key(KeyCode::Down); + } + let bottom = joined(&shell.render().pickers, "\n"); + assert!( + bottom.contains("agent-24"), + "the LAST agent must be reachable by scrolling — an unreachable tail is the defect \ + the fold was meant to fix, one level in. Got: {bottom:?}" + ); + assert_ne!( + bottom, expanded, + "the viewport must actually have MOVED. Identical views would mean `Down` was \ + swallowed and the assertion above was passing on window size alone" + ); + + // And it clamps rather than scrolling into empty space below the last row. + assert!( + bottom.contains("above"), + "a scrolled window must state what is off-screen ABOVE it, or a partial view reads as \ + a complete list. Got: {bottom:?}" + ); + } + + /// **The collapsed header carries the unavailable count** (FR-1.5, FR-1.7). + /// + /// FR-1.5 and FR-1.7 require an unloadable profile and an uninstalled provider to be *listed* + /// so the operator learns the thing exists and why it is unavailable. Folding the rows away is a + /// layout decision; folding away the *fact* would be a regression against both, and a silent + /// one — the screen would simply read `agents (25)` with nothing missing-looking about it. + /// + /// Written after mutation testing found this unguarded: deleting the whole `diagnosis` clause + /// from `fold_lines` left all 183 tests green. `a_long_agent_list_stays_bounded_…` asserts + /// `agents (25)`, which is a *prefix* of `agents (25, 2 unloadable)` and so cannot tell the two + /// apart. This asserts the count itself, in both pickers, plus a zero case so the clause is not + /// simply always-on. + #[test] + fn a_collapsed_fold_still_states_how_many_rows_are_unavailable() { + let server = FakeServer::healthy(); + *server.profiles.borrow_mut() = Ok(vec![ + profile("planner", true), + profile("broken", false), + profile("also-broken", false), + ]); + *server.providers.borrow_mut() = + Ok(vec![provider("kiro_cli", true), provider("codex", false)]); + let host = FakeHost::outside_tmux(); + let shell = shell_on_the_launch_form(&server, &host, 100, 40); + + let pickers = joined(&shell.render().pickers, "\n"); + assert!( + pickers.contains("agents (3, 2 unloadable)"), + "the COLLAPSED header must state how many profiles are unloadable — FR-1.5 requires \ + the operator learn an unavailable profile exists, and a fold that drops the count \ + hides exactly that. Got: {pickers:?}" + ); + assert!( + pickers.contains("providers (2, 1 not installed)"), + "and the same for uninstalled providers (FR-1.7): `installed` is display information, \ + so the count must survive the fold. Got: {pickers:?}" + ); + + // The zero case: with everything usable there is no diagnosis to report, and appending + // `, 0 unloadable` to a healthy list would be noise the operator learns to ignore. + let healthy = server_with_many_agents(4); + let clean = shell_on_the_launch_form(&healthy, &host, 100, 40); + let clean_pickers = joined(&clean.render().pickers, "\n"); + assert!( + clean_pickers.contains("agents (4)") && !clean_pickers.contains("unloadable"), + "with nothing unavailable the header must be a bare count — otherwise the clause is \ + always-on and says nothing. Got: {clean_pickers:?}" + ); + } + + /// **A failed picker is never folded** — a failure must not need a keystroke to be read. + /// + /// The fold exists to bound a long *list*. A failure line is one row and carries the `[ctrl+r]` + /// remedy, so folding it would hide the one picker state the operator has to act on. Asserted + /// with the fold flags at their default, which is exactly the state a failure appears in. + #[test] + fn a_failed_picker_is_never_folded_away() { + let server = FakeServer::unreachable(); + let host = FakeHost::outside_tmux(); + let mut shell = shell_with_failed_pickers(&server, &host); + + let pickers = joined(&shell.render().pickers, "\n"); + for expected in ["agents: unavailable", "providers: unavailable", "[ctrl+r]"] { + assert!( + pickers.contains(expected), + "{expected:?} must be visible with no keystroke — a folded failure is a failure \ + the operator cannot see. Got: {pickers:?}" + ); + } + assert!( + !pickers.contains("[enter] expand"), + "a failure line must not be presented as a fold: there is nothing to expand, so the \ + key would do nothing. Got: {pickers:?}" + ); + + // And `[enter]` on the region does not silently start a run either. + while shell.focus() != Focus::AgentPicker { + assert!(shell.on_key(KeyCode::Tab)); + } + assert!( + !shell.on_key(KeyCode::Enter), + "[enter] on a failed picker must report UNHANDLED rather than toggling a fold that \ + has no rows" + ); + } + + // ── Reveal-then-run ─────────────────────────────────────────────────────────────────────── + + /// **The first `[enter]` reveals the options; only the second runs.** + /// + /// The reported defect: with the optional section and both pickers collapsed by default, the + /// first `[enter]` after choosing a command *executed* it, so the nine optional parameters + /// behind the fold were unreachable in the one flow every operator takes. The operator + /// described running the CLI by accident while trying to open the options. + /// + /// Asserts on the SERVER, not on a flag: `create_session_calls` is the irreversible side + /// effect, and a test that only checked `pending_action` would pass against a build that queued + /// the launch and fired it one tick later — the same defect, one frame further on. Both are + /// checked here for that reason. + #[test] + fn the_first_enter_reveals_the_options_and_the_second_one_runs() { + let server = + server_with_many_agents(25).with_session(SessionAnswer::Created(terminal("t"))); + let host = FakeHost::outside_tmux(); + let mut shell = shell_on_the_launch_form(&server, &host, 100, 40); + shell + .flow_mut() + .set("--agents", "agent-00") + .expect("a loadable profile in the fake's answer"); + + // Everything is folded, so this press must REVEAL. + assert!( + shell.on_key(KeyCode::Enter), + "the reveal press must be handled" + ); + assert_eq!( + server.create_session_calls.get(), + 0, + "the first [enter] must not run the command — this is the reported defect" + ); + assert!( + shell.pending_action.is_none(), + "nor may it QUEUE a run: a queued launch fires on the next tick, which is the same \ + accidental execution one frame later" + ); + + let revealed = joined(&shell.render().pickers, "\n"); + assert!( + revealed.contains("agent-00"), + "the reveal press must actually open the folds, or it is a wasted keystroke that \ + teaches the operator the key is broken. Got: {revealed:?}" + ); + + // Nothing is folded now, so this press must RUN. + assert!( + shell.on_key(KeyCode::Enter), + "the run press must be handled" + ); + assert!( + shell.pending_action.is_some(), + "the second [enter] must queue the run" + ); + assert!(shell.run_pending_action()); + assert_eq!( + server.create_session_calls.get(), + 1, + "the second [enter] must reach `launch()` — reveal-then-run must not become \ + reveal-then-nothing, which would make the command unrunnable" + ); + } + + /// **The footer states which of the two things `[enter]` will do right now.** + /// + /// A footer reading `ready — [enter] run` while the first press actually expands is the same + /// broken promise as a documented key that does nothing: the operator presses it expecting a + /// run, gets an expansion, and stops trusting the footer. Both phases are asserted, so a build + /// that hard-codes either wording fails. + #[test] + fn the_footer_promises_reveal_before_run_and_run_after() { + let server = server_with_many_agents(25); + let host = FakeHost::outside_tmux(); + let mut shell = shell_on_the_launch_form(&server, &host, 100, 40); + shell + .flow_mut() + .set("--agents", "agent-00") + .expect("a loadable profile in the fake's answer"); + + let folded = joined(&shell.render().footer, " "); + assert!( + folded.contains("show all options"), + "while options are folded the footer must say the press REVEALS. Got: {folded:?}" + ); + + assert!(shell.on_key(KeyCode::Enter)); + let revealed = joined(&shell.render().footer, " "); + assert!( + revealed.contains("[enter] run") && !revealed.contains("show all options"), + "once everything is revealed the footer must promise a RUN — a stale reveal hint \ + leaves the operator unsure whether the command ever starts. Got: {revealed:?}" + ); + } + + /// **A form with nothing to reveal runs on the first `[enter]`.** + /// + /// The reveal step is a consequence of something being *hidden*. A command with no optional + /// fields and both pickers empty has nothing to show, so demanding a first press there would + /// be a keystroke that visibly does nothing — and the operator would press `[enter]` forever + /// with the server down. This is the case `has_folded_options` exists to exclude, and without + /// it the two-press rule would be a hang rather than a safeguard. + #[test] + fn a_form_with_nothing_folded_runs_on_the_first_enter() { + let server = FakeServer::healthy().with_run(RunAnswer::Chunks(vec!["out\n"], 200)); + // Both pickers empty: `Loaded(vec![])` is a valid answer, and it is NOT foldable. + *server.profiles.borrow_mut() = Ok(vec![]); + *server.providers.borrow_mut() = Ok(vec![]); + let host = FakeHost::outside_tmux(); + + let mut shell = Renderer::new(&server, &host, 100, 40); + // `profile list` declares NO parameters at all — the only shape with genuinely nothing to + // reveal once both pickers are empty. `session list` was the obvious choice and is wrong: + // it has a `--json` flag, so it is foldable and this test's own floor caught it. + assert!(shell.focus_command(CommandId::ProfileList)); + assert!(shell.on_key(KeyCode::Enter)); + + assert!( + !shell.has_folded_options(), + "this test is only meaningful if nothing is folded; otherwise it asserts the reveal \ + path and its name lies" + ); + + assert!(shell.on_key(KeyCode::Enter)); + assert!( + shell.run_pending_action(), + "with nothing to reveal the FIRST [enter] must run — an unconditional reveal press \ + would be a keystroke that does nothing visible, and the run would never start" + ); + assert_eq!( + server.run_calls.borrow().as_slice(), + &[CommandId::ProfileList] + ); + } + + /// **Re-selecting a command re-collapses the folds.** + /// + /// Otherwise reveal-then-run depends on history: an operator who expanded the pickers for one + /// command would find the next command running on its *first* `[enter]`, which is the original + /// accidental-execution defect returning for everyone who had used the TUI for more than one + /// command. The two-press shape has to be a property of the screen, not of the session. + #[test] + fn selecting_another_command_re_collapses_the_folds() { + let server = server_with_many_agents(25); + let host = FakeHost::outside_tmux(); + let mut shell = shell_on_the_launch_form(&server, &host, 100, 40); + + assert!(shell.on_key(KeyCode::Enter), "reveal everything"); + assert!( + !shell.has_folded_options(), + "the reveal press must have opened the folds for this test to mean anything" + ); + + // Back to the list and pick a different command. + assert!(shell.on_key(KeyCode::Left)); + assert!(shell.focus_command(CommandId::MemoryList)); + assert!(shell.on_key(KeyCode::Enter)); + + let pickers = joined(&shell.render().pickers, "\n"); + assert!( + pickers.contains("[enter] expand"), + "a newly selected command must start COLLAPSED — inheriting the previous command's \ + expansion makes the first [enter] run, which is the reported defect. Got: {pickers:?}" + ); + assert!( + !pickers.contains("agent-00"), + "and the rows themselves must be folded away again. Got: {pickers:?}" + ); + } + + /// **Every region fits on screen at once — the whole point of the fold and the window.** + /// + /// This is the operator's actual complaint, asserted end to end: with 25 agents, is the picker + /// visible *at the same time as* the form and the banner? Folding the pickers alone did not + /// achieve that, and the fold's own unit test could not tell — it asserted on `frame.pickers`, + /// a region that exists whether or not it survives layout. + /// + /// Measured in a real pty at 40 and 70 rows, the pickers never appeared at all: the **42-command + /// list** is rendered first into a non-scrolling column and consumed the whole height. So the + /// assertion here is on the DRAWN BUFFER, which is the only thing that can catch a region that + /// renders correctly and is then clipped away. + #[test] + fn at_a_realistic_size_every_region_survives_the_layout() { + let server = server_with_many_agents(25); + let host = FakeHost::outside_tmux(); + let shell = shell_on_the_launch_form(&server, &host, 120, 40); + + let drawn: String = drawn_cells(&shell, 120, 40) + .chunks(120) + .map(|row| { + let text: String = row.iter().map(|(symbol, _)| symbol.as_str()).collect(); + format!("{}\n", text.trim_end()) + }) + .collect(); + + // One needle per region, each of which was invisible before the window was added. + for (region, needle) in [ + ("the command list", "cao install"), + ("the required fields", "--agents"), + ("the optional fold", "optional ("), + ("the agent fold", "agents (25)"), + ("the provider fold", "providers ("), + ("the footer", "[tab]"), + ] { + assert!( + drawn.contains(needle), + "{region} must be VISIBLE at 120x40 — {needle:?} was not drawn. A region that \ + renders into the frame and is then clipped by the layout is invisible to the \ + operator, which is the reported defect. Screen:\n{drawn}" + ); + } + + // And the list states that it is showing a window, so 42 commands do not read as 13. + assert!( + drawn.contains("of 42 commands"), + "the windowed list must say how many commands there are in total, or the operator \ + cannot tell a window from the whole catalog. Screen:\n{drawn}" + ); + } + + /// **The command-list window follows the cursor, so focus is never off-screen.** + /// + /// A fixed slice would bound the height just as well and would let `Down` walk the cursor past + /// the last visible row — the invisible-focus defect (NFR-3 item 7) one region over, and + /// indistinguishable from a frozen UI. Drives the cursor to the last command and asserts the + /// marked row is on screen. + #[test] + fn the_command_window_follows_the_cursor_to_the_end_of_the_list() { + let server = FakeServer::healthy(); + let host = FakeHost::outside_tmux(); + let mut shell = Renderer::new(&server, &host, 120, 40); + + let total = shell.command_rows().len(); + assert!( + total > shell.command_list_window(), + "this test needs a list LONGER than the window ({total} rows) or it proves nothing" + ); + + for _ in 0..total { + shell.on_key(KeyCode::Down); + } + + let rendered = joined(&shell.render().command_list, "\n"); + let marked = rendered + .lines() + .find(|line| line.starts_with('>')) + .unwrap_or_else(|| { + panic!( + "the focus marker must be ON SCREEN after scrolling to the end — a cursor \ + outside the window is invisible focus, and the TUI looks frozen. \ + Got:\n{rendered}" + ) + }); + assert!( + marked.contains("cao workflow validate"), + "the window must have followed the cursor to the LAST command. Marked row: {marked:?}" + ); + assert!( + rendered.contains("above"), + "and it must state what is off-screen above it. Got:\n{rendered}" + ); + } + + /// **A focused fold header carries the `focus` role, and an unfocused one does not.** (#556) + /// + /// The two new regions are focusable, so they need the same visible-focus guarantee as every + /// other region (NFR-3 item 7): a region the operator can Tab into with no cue looks like a + /// keystroke that did nothing. The negative half is what makes this a test of *focus* rather + /// than of "fold headers are cyan". + #[test] + fn the_focused_fold_header_carries_the_focus_role() { + let server = server_with_many_agents(3); + let host = FakeHost::outside_tmux(); + let mut shell = shell_on_the_launch_form(&server, &host, 120, 90); + shell.set_theme(Theme::colour()); + + while shell.focus() != Focus::AgentPicker { + assert!(shell.on_key(KeyCode::Tab)); + } + + let focus_fg = Theme::colour().focus.fg.expect("`focus` sets a foreground"); + assert_eq!( + drawn_foreground(&shell, 120, 90, "agents (3)"), + focus_fg, + "the focused fold header must carry `focus`; a region with no visible cue is one the \ + operator cannot tell they are in (NFR-3 item 7)" + ); + assert_ne!( + drawn_foreground(&shell, 120, 90, "providers ("), + focus_fg, + "the UNFOCUSED fold must not carry `focus` — if both are styled the same, the colour \ + says nothing about where the keyboard is" + ); + } } diff --git a/tui/src/results_pane.rs b/tui/src/results_pane.rs index 2d4b8f98c..9d31404c6 100644 --- a/tui/src/results_pane.rs +++ b/tui/src/results_pane.rs @@ -92,6 +92,7 @@ use ratatui::widgets::{Block, Paragraph, Widget, Wrap}; use thiserror::Error; use crate::catalog::Policy; +use crate::theme::Theme; /// Retained-line cap (Q2 at 3.1, SR-3, PR-2). Oldest lines are discarded past this. /// @@ -445,6 +446,16 @@ pub struct ResultsPane { scroll_offset: usize, /// The state [`Self::collapse`] left, so [`Self::expand`] can restore it. collapsed_from: Option, + /// The semantic palette (#556). Owned rather than passed to `render`, because + /// `Widget::render(self, area, buf)` has a fixed three-argument signature — see + /// [`Self::set_theme`]. + /// + /// **Presentation-only, and it is the one field here that is.** That is a real cost to the + /// model/view separation this module otherwise keeps, and it is accepted for a specific + /// reason: the alternative is a `ThemedPane<'a>` wrapper, which pushes a lifetime into the two + /// `(&self.pane).render(..)` sites in `renderer.rs` to save a `Copy` field. Nothing in this + /// module reads it outside `render`, `body`, and `footer_line`. (#556) + theme: Theme, } /// Hand-written rather than derived, because **`vte::Parser` does not implement `Debug`**. @@ -469,6 +480,7 @@ impl std::fmt::Debug for ResultsPane { .field("manual_command", &self.manual_command) .field("scroll_offset", &self.scroll_offset) .field("collapsed_from", &self.collapsed_from) + .field("theme", &self.theme) .finish() } } @@ -494,9 +506,35 @@ impl ResultsPane { manual_command: None, scroll_offset: 0, collapsed_from: None, + // `Theme::default()` is the COLOUR theme, deliberately: a pane whose theme was never + // set renders in colour rather than silently monochrome, so a forgotten `set_theme` + // looks like working code that has colour — not like working code that lost it, which + // nobody would notice. `renderer` sets it from `Theme::from_env()` at startup. + theme: Theme::default(), } } + /// Replaces the palette (#556). + /// + /// # Why this is a setter and not a `render` parameter + /// + /// `Widget::render(self, area: Rect, buf: &mut Buffer)` is a trait method with a fixed + /// three-argument signature; a theme parameter is not expressible without abandoning the trait + /// or wrapping the pane in a `ThemedPane<'a>` newtype. The wrapper is defensible — it keeps + /// presentation out of the model — but it puts a lifetime on the two `(&self.pane).render(..)` + /// call paths in `renderer.rs`, which today have none. [`Theme`] is `Copy` and six small + /// `Style` values, so owning one costs nothing at all. + /// + /// Called **once**, at construction, from `renderer`. Not per frame: `Theme::from_env` reads + /// the environment, and a value that cannot change mid-process has no business being re-read in + /// a render loop. (#556) + /// `pub(crate)` and not `pub`, matching [`Theme`]'s own visibility. `pub` here is a + /// `private_interfaces` error under `-D warnings`, which is the compiler making the same point: + /// a method cannot be more reachable than the type in its signature. + pub(crate) fn set_theme(&mut self, theme: Theme) { + self.theme = theme; + } + /// Begins a run: `collapsed` → `running`, buffer cleared, `policy` retained. /// /// **Infallible.** A UI that raises has nowhere to raise to, so every outcome is a rendered @@ -740,7 +778,12 @@ impl ResultsPane { true } - /// The footer for the current state (NFR-3: every state is textually distinguishable). + /// The footer text for the current state (NFR-3: every state is textually distinguishable). + /// + /// Kept separate from [`Self::footer_line`] so the **text** and the **style** are two + /// decisions in two places. That is what lets the strip-styling guard assert the wording is + /// self-sufficient without the guard being able to see a colour at all — which is the whole + /// property NFR-5 claims. (#556) fn footer(&self) -> String { match self.state { PaneState::Collapsed => String::new(), @@ -757,6 +800,41 @@ impl ResultsPane { } } + /// The footer's semantic role. **Three-way on the exit code, not two-way** (#556). + /// + /// # `exit ?` is `warn`, and collapsing it into `error` would be a lie + /// + /// The obvious branch is "zero is good, anything else is bad", which puts a missing exit code + /// in the same bucket as a failure. It is not one: `None` means the pane reached a terminal + /// state *without* an exit code, which says nothing about whether the command succeeded. The + /// comment on [`Self::footer`] already draws that distinction in text — "a missing exit code is + /// worth showing" — and styling it `error` would assert a failure this pane has no evidence + /// for. Three arms, and [`the_exit_footer_distinguishes_success_failure_and_absence`] holds + /// them apart. + /// + /// `Cancelled` is `warn` for the same family of reason: `[k]` stopped this pane following the + /// stream and the command is still running (see [`CANCELLED_NOTICE`]), so it is neither a + /// success nor a failure. + fn footer_style(&self) -> Style { + match self.state { + PaneState::Collapsed => self.theme.dim, + PaneState::Running => self.theme.dim, + PaneState::Complete | PaneState::Empty => match self.exit_code { + Some(0) => self.theme.ok, + Some(_) => self.theme.error, + // NOT `error` — see the note above. A missing code is not a failure. + None => self.theme.warn, + }, + PaneState::Cancelled => self.theme.warn, + PaneState::Refused => self.theme.error, + } + } + + /// The footer as a styled line: [`Self::footer`]'s text carrying [`Self::footer_style`]'s role. + fn footer_line(&self) -> Line<'static> { + Line::styled(self.footer(), self.footer_style()) + } + /// The viewport body for the current state, as display lines. /// /// # Why this takes the viewport height (PR-3, and the defect it fixes) @@ -790,14 +868,19 @@ impl ResultsPane { .refusal_reason .as_deref() .unwrap_or("the hand-off mechanism is unavailable and no reason was supplied"); - body.push(Line::from(reason)); + body.push(Line::styled(reason, self.theme.error)); // The exact argv, on its own line so a terminal's line-select copies it whole. if let Some(command) = self.manual_command.as_deref() { body.push(Line::from("")); + // The argv is deliberately NOT `error`, even though the refusal above is: it + // is the remedy, not the fault. Styling the thing the operator is meant to copy + // and run the same as the failure that produced it tells them to be alarmed by + // their own next step. Left unstyled rather than given a role — no role means + // "ordinary text", which is exactly what a command to run is. (#556) body.push(Line::from(command)); } } - PaneState::Empty => body.push(Line::from(EMPTY_NOTICE)), + PaneState::Empty => body.push(Line::styled(EMPTY_NOTICE, self.theme.dim)), PaneState::Complete if self.policy == Some(Policy::Handoff) => { // The command's output went to the new window; an empty pane would read as a // failed run, so the structured outcome line stands in for it. @@ -807,7 +890,11 @@ impl ResultsPane { } PaneState::Running | PaneState::Complete | PaneState::Cancelled => { if self.output.truncated { - body.push(Line::from(TRUNCATION_MARKER)); + // `dim`, not `warn`. Dropping the oldest lines is disclosure of a bounded + // buffer working as designed, not a problem — and SR-3 calls the marker "the + // security-relevant half" of the ring buffer precisely because it must be + // *present*, which is a text property no style can supply or remove. + body.push(Line::styled(TRUNCATION_MARKER, self.theme.dim)); } let lines = self.lines(); // The scroll offset counts up from the newest, so the window ENDS @@ -863,7 +950,9 @@ impl Widget for &ResultsPane { // FR-3.3: the strip is a rendered state, and it carries the count so the operator // can see there is something to expand into. let strip = format!("{COLLAPSED_PREFIX} ({})", self.lines().len()); - buf.set_string(area.x, area.y, strip, Style::default()); + // `dim`: a collapsed pane is present but not where the operator is working. The `▸` + // and the count are what say "there is something here", and both are text. (#556) + buf.set_string(area.x, area.y, strip, self.theme.dim); return; } @@ -875,22 +964,22 @@ impl Widget for &ResultsPane { .block(Block::new()) .render(body_area, buf); - buf.set_string( - footer_area.x, - footer_area.y, - self.footer(), - Style::default(), - ); + // Rendered through the `Line`, so the style travels with the text rather than being applied + // by position here. `set_string` with a separate `Style` argument would work identically + // today and would put the style decision in the render site instead of next to the text it + // describes — which is how a footer's wording and its meaning drift apart. (#556) + self.footer_line().render(footer_area, buf); } } #[cfg(test)] mod tests { use super::{ - NotRunning, PaneState, Policy, ResultsPane, BUFFER_CAPACITY, CANCELLED_NOTICE, - EMPTY_NOTICE, RUNNING_INDICATOR, TRUNCATION_MARKER, + NotRunning, PaneState, Policy, ResultsPane, Theme, BUFFER_CAPACITY, CANCELLED_NOTICE, + COLLAPSED_PREFIX, EMPTY_NOTICE, REFUSED_NOTICE, RUNNING_INDICATOR, TRUNCATION_MARKER, }; use ratatui::backend::TestBackend; + use ratatui::style::Color; use ratatui::Terminal; use std::io::Write; use std::sync::mpsc; @@ -1881,6 +1970,439 @@ mod tests { ); } + // ── #556: the semantic colour layer ────────────────────────────────────────────────────── + + /// One pane per [`PaneState`], each in its real state via the public API. + /// + /// Shared by the styling tests below and built by driving the pane rather than by setting + /// fields: a fixture that assigned `state` directly could be in a state the API cannot + /// actually produce, and then the tests would be about a pane that does not exist. + fn one_pane_per_state() -> Vec<(PaneState, ResultsPane)> { + let mut cases: Vec<(PaneState, ResultsPane)> = Vec::new(); + + cases.push((PaneState::Collapsed, ResultsPane::new())); + + let mut running = ResultsPane::new(); + running.attach(Policy::InApp); + running.push_bytes(b"working\n"); + cases.push((PaneState::Running, running)); + + let mut complete = ResultsPane::new(); + complete.attach(Policy::InApp); + complete.push_bytes(b"rows\n"); + complete.complete(0, None); + cases.push((PaneState::Complete, complete)); + + let mut empty = ResultsPane::new(); + empty.attach(Policy::InApp); + empty.complete(2, None); + cases.push((PaneState::Empty, empty)); + + let mut cancelled = ResultsPane::new(); + cancelled.attach(Policy::InApp); + cancelled.push_bytes(b"partial\n"); + cancelled.cancel().expect("running panes may be cancelled"); + cases.push((PaneState::Cancelled, cancelled)); + + let mut refused = ResultsPane::new(); + refused.attach(Policy::Handoff); + refused.refuse("no client".to_string(), Some("tmux ls".to_string())); + cases.push((PaneState::Refused, refused)); + + assert_eq!( + cases.len(), + 6, + "all SIX states must be covered — the interaction spec's five plus `cancelled` from \ + the Q1 ruling. A seventh state added without a fixture here would leave the styling \ + guards silently covering six of seven" + ); + for (expected, pane) in &cases { + assert_eq!(pane.state(), *expected, "fixture must be in {expected:?}"); + } + cases + } + + /// Renders `pane` and returns every cell as `(symbol, fg, bg)`. + /// + /// The style half is read from the same `Cell` the symbol comes from, so a test cannot assert + /// a style the terminal would not actually produce. + fn rendered_style_cells( + pane: &ResultsPane, + width: u16, + height: u16, + ) -> Vec<(String, Color, Color)> { + let mut terminal = Terminal::new(TestBackend::new(width, height)) + .expect("a TestBackend terminal must construct"); + terminal + .draw(|frame| frame.render_widget(pane, frame.area())) + .expect("rendering into a TestBackend must not fail"); + terminal + .backend() + .buffer() + .content() + .iter() + .map(|cell| (cell.symbol().to_string(), cell.fg, cell.bg)) + .collect() + } + + /// **FR-5.1 / FR-5.4 / NFR-5 — THE STRIP-STYLING GUARD. Read this before changing wording.** + /// + /// Every state stays identifiable when **all styling is discarded**. This is the executable + /// form of NFR-3's "no state conveyed by colour alone", and it is the one test in this feature + /// that must not be allowed to become a formality — because the property it defends is not + /// about colour at all. It is about whether the *text* is sufficient. + /// + /// # Why the expectations come from the constants + /// + /// [`CANCELLED_NOTICE`], [`EMPTY_NOTICE`], [`RUNNING_INDICATOR`], [`TRUNCATION_MARKER`] and + /// [`COLLAPSED_PREFIX`] are read from the module, not retyped here. That is the *opposite* of + /// the usual "never source a fixture from the value under test" rule, and the distinction is + /// which value is under test: this test asserts the **styling** is unnecessary, so sourcing + /// the **wording** from its constant cannot make it vacuous. It also stops the test reddening + /// on a copy edit, which is what would eventually get it deleted. The wording itself is pinned + /// by [`the_cancelled_wording_does_not_claim_the_command_stopped`] and by SR-4. + /// + /// `Complete` is the exception and is checked against a literal `exit 0`, because there is no + /// constant to read — the string is built by `format!` — and a derived expectation there would + /// agree with the formatter through any typo in it. + /// + /// # What "styling discarded" means here + /// + /// The cells' symbols are read and their `fg`/`bg` thrown away, which is what a screen reader, + /// a pipe, a `TERM=dumb` terminal, and a monochrome operator all see. The test does not render + /// under `monochrome()` to achieve this — that would prove something weaker, since + /// `monochrome()` keeps `Modifier::BOLD`. Discarding the style entirely is the stronger claim. + /// + /// **Mutation-proven (FR-5.4).** Dropping [`CANCELLED_NOTICE`] from the footer turns this red. + #[test] + fn every_state_is_identifiable_with_all_styling_discarded() { + for (state, pane) in one_pane_per_state() { + let text: String = rendered_style_cells(&pane, 70, 6) + .into_iter() + .map(|(symbol, _fg, _bg)| symbol) + .collect(); + + // The marker each state must be identifiable BY, in text alone. + let expected: &str = match state { + PaneState::Collapsed => COLLAPSED_PREFIX, + PaneState::Running => RUNNING_INDICATOR, + // No constant: built by `format!`, so a derived expectation would agree with the + // formatter through any typo in it. + PaneState::Complete => "exit 0", + PaneState::Empty => EMPTY_NOTICE, + PaneState::Cancelled => CANCELLED_NOTICE, + PaneState::Refused => REFUSED_NOTICE, + }; + + assert!( + text.contains(expected), + "{state:?} is NOT identifiable from text alone.\n\ + \n\ + Expected the rendered cells to contain {expected:?}, but got:\n {text:?}\n\ + \n\ + This is FR-5.1 and NFR-3: no state may be conveyed by colour alone. An operator \ + on a monochrome terminal, a screen-reader user, and anyone piping this output all \ + see exactly the text above and no styling whatsoever. If you reached this by \ + moving a distinction into a colour, move it back into the words." + ); + } + } + + /// The foreground the given text is actually drawn in, located by scanning rendered rows. + /// + /// Reads the style from the same cells the text occupies rather than from a helper on the pane, + /// so a test using this cannot pass by agreeing with a mapping function that is itself wrong. + /// Panics if the text is absent, which keeps "the style is right" from silently degrading into + /// "the text was not there, so nothing was checked". + fn foreground_of(pane: &ResultsPane, width: u16, height: u16, needle: &str) -> Color { + let cells = rendered_style_cells(pane, width, height); + let rows: Vec<&[(String, Color, Color)]> = cells.chunks(width as usize).collect(); + + for row in &rows { + let text: String = row.iter().map(|(symbol, _, _)| symbol.as_str()).collect(); + if let Some(start) = text.find(needle) { + // `find` returns a BYTE offset; the cells are graphemes. Every needle these tests + // pass is ASCII, so the two coincide — asserted rather than assumed, because a + // future non-ASCII needle would silently read the style of the wrong cell. + assert!( + needle.is_ascii() && text.is_char_boundary(start), + "foreground_of indexes cells by byte offset and so requires an ASCII needle; \ + got {needle:?}" + ); + let foregrounds: Vec = row[start..start + needle.len()] + .iter() + .map(|(_, fg, _)| *fg) + .collect(); + let first = foregrounds[0]; + assert!( + foregrounds.iter().all(|fg| *fg == first), + "{needle:?} is drawn in more than one colour ({foregrounds:?}), so there is no \ + single role to check" + ); + return first; + } + } + + let all: String = cells.iter().map(|(symbol, _, _)| symbol.as_str()).collect(); + panic!("{needle:?} was not rendered at all, so its style could not be read. Got: {all:?}"); + } + + /// **FR-4.1 — `exit 0` carries the `ok` role.** + /// + /// Asserted on the rendered cells via [`foreground_of`], not on [`ResultsPane::footer_style`]: + /// a test that called the mapping helper and compared it to the theme would agree with the + /// helper through any error in it. + #[test] + fn a_zero_exit_footer_carries_the_ok_role() { + let mut pane = ResultsPane::new(); + pane.attach(Policy::InApp); + pane.push_bytes(b"output\n"); + pane.complete(0, None); + pane.set_theme(Theme::colour()); + + let expected = Theme::colour().ok.fg.expect("`ok` sets a foreground"); + assert_eq!( + foreground_of(&pane, 40, 3, "exit 0"), + expected, + "a successful exit must be drawn in the `ok` role" + ); + } + + /// **FR-4.1 — a non-zero `exit` carries the `error` role.** + #[test] + fn a_non_zero_exit_footer_carries_the_error_role() { + let mut pane = ResultsPane::new(); + pane.attach(Policy::InApp); + pane.push_bytes(b"output\n"); + pane.complete(1, None); + pane.set_theme(Theme::colour()); + + let expected = Theme::colour().error.fg.expect("`error` sets a foreground"); + assert_eq!( + foreground_of(&pane, 40, 3, "exit 1"), + expected, + "a failing exit must be drawn in the `error` role" + ); + } + + /// **FR-4.1 / A-1 — a MISSING exit code carries `warn`, not `error`.** + /// + /// Three arms, not two, and this is the third. A missing exit code does not mean the command + /// failed; it means the pane reached a terminal state without one. Colouring that `error` would + /// have the pane assert a failure it has no evidence for — and it is the likeliest defect in + /// this feature, because the two-way `Some(0)` / `Some(_)` branch is the obvious one to write. + /// + /// # Why this one sets the field directly + /// + /// [`ResultsPane::complete`] takes `exit_code: i32` and always stores `Some`, so `exit ?` is + /// **unreachable through the public API today**. It is a defensive arm. The two honest options + /// were to delete the arm or to test it below the API; deleting it would put a `match` on + /// `Option` with no `None` case, i.e. a compile error, so the arm has to exist and therefore + /// has to be right. This test lives inside the module, drives the pane through the real API + /// first, then clears the one field the API cannot clear — and says so here so nobody reads it + /// as evidence that `exit ?` is reachable. + #[test] + fn a_missing_exit_code_footer_carries_warn_not_error() { + let mut pane = ResultsPane::new(); + pane.attach(Policy::InApp); + pane.push_bytes(b"output\n"); + pane.complete(0, None); + pane.set_theme(Theme::colour()); + + assert_eq!( + pane.exit_code, + Some(0), + "precondition: `complete` stores the code, which is why this state needs forcing" + ); + pane.exit_code = None; + assert_eq!( + pane.state(), + PaneState::Complete, + "clearing the code must not disturb the state — the footer arm under test is the \ + `Complete`/`Empty` one" + ); + + let theme = Theme::colour(); + let warn = theme.warn.fg.expect("`warn` sets a foreground"); + let error = theme.error.fg.expect("`error` sets a foreground"); + assert_ne!( + warn, error, + "precondition: if `warn` and `error` were the same colour this test could not fail" + ); + + assert_eq!( + foreground_of(&pane, 40, 3, "exit ?"), + warn, + "a missing exit code must be `warn`, NOT `error`: it does not mean the command failed, \ + only that no code arrived (A-1)" + ); + } + + /// The three exit roles are **actually different styles**. + /// + /// Without this, [`the_exit_footer_distinguishes_success_failure_and_absence`] would pass on a + /// theme whose `ok`, `error` and `warn` were all the same value — the three-way distinction + /// asserted against a palette that does not make it. This is the anti-vacuity half. + /// + /// Note `required` and `warn` ARE the same style by design (both `Yellow`), so this checks only + /// the three roles the exit footer uses. A blanket "all six roles differ" would be a stronger + /// claim than the palette makes and would fail on a deliberate decision. + #[test] + fn the_three_exit_roles_are_mutually_distinguishable() { + let theme = Theme::colour(); + assert_ne!(theme.ok, theme.error, "`exit 0` and `exit 1` must differ"); + assert_ne!( + theme.error, theme.warn, + "`exit 1` and `exit ?` must differ, or the three-way branch is decoration" + ); + assert_ne!(theme.ok, theme.warn, "`exit 0` and `exit ?` must differ"); + } + + /// **FR-3.3 end-to-end — under `NO_COLOR`, every rendered cell is `Color::Reset`.** + /// + /// The value-level version of this lives in `theme.rs`; this is the render-level one, and it is + /// the half that catches a style applied *outside* the theme. A `Color::Green` written inline + /// at a call site would satisfy every assertion in `theme.rs` and fail here. + /// + /// `bg` is checked as well as `fg`: `Cell::EMPTY` is `fg: Reset, bg: Reset`, so a monochrome + /// render is required to be indistinguishable from an unstyled one in both channels. + #[test] + fn under_no_color_every_rendered_cell_is_reset() { + for (state, mut pane) in one_pane_per_state() { + pane.set_theme(Theme::monochrome()); + + for (symbol, fg, bg) in rendered_style_cells(&pane, 70, 6) { + assert_eq!( + fg, + Color::Reset, + "{state:?} rendered cell {symbol:?} with foreground {fg:?} under \ + NO_COLOR. Every role in the monochrome theme is Color::Reset, so a coloured \ + cell here means a style was applied OUTSIDE the theme — an inline colour at a \ + call site that `from_env` cannot switch off (FR-3.3, FR-1.4)" + ); + assert_eq!( + bg, + Color::Reset, + "{state:?} rendered cell {symbol:?} with background {bg:?} under NO_COLOR. A \ + background colour is still colour" + ); + } + } + } + + /// The colour theme **does** reach the cells — the anti-vacuity floor for the render path. + /// + /// Every styling assertion above is satisfied by a pane that applies no style at all: + /// [`under_no_color_every_rendered_cell_is_reset`] passes trivially if nothing is ever styled, + /// and the strip guard passes *by design* when styling is absent. So without this test, wave 1 + /// of this feature could be a no-op with a green suite. + /// + /// Deliberately weak about *which* cells: it asserts only that at least one rendered cell + /// carries a non-`Reset` foreground drawn from the palette. Pinning positions would make it a + /// layout test that reddens on a wording change. + #[test] + fn the_colour_theme_actually_reaches_the_rendered_cells() { + let theme = Theme::colour(); + let palette: Vec = theme + .roles() + .iter() + .filter_map(|(_, style)| style.fg) + .collect(); + + let mut states_with_colour = 0; + for (state, mut pane) in one_pane_per_state() { + pane.set_theme(theme); + let coloured: Vec<_> = rendered_style_cells(&pane, 70, 6) + .into_iter() + .filter(|(_, fg, _)| *fg != Color::Reset) + .collect(); + + if !coloured.is_empty() { + states_with_colour += 1; + for (symbol, fg, _) in coloured { + assert!( + palette.contains(&fg), + "{state:?} rendered {symbol:?} with {fg:?}, which is not in the theme's \ + palette ({palette:?}). A colour that is not a role is a colour \ + `NO_COLOR` cannot switch off (FR-1.4)" + ); + } + } + } + + assert!( + states_with_colour >= 4, + "only {states_with_colour} of 6 states rendered any colour at all. This is the \ + anti-vacuity floor: every other styling assertion in this module is satisfied by a \ + pane that styles nothing, so if the palette stopped reaching the cells this is the \ + only test that would notice" + ); + } + + /// A forgotten [`ResultsPane::set_theme`] leaves the pane in **colour**, not monochrome. + /// + /// The failure this guards is asymmetric. A default of `monochrome()` would make a missing + /// `set_theme` call look like working code that simply has no colour — invisible in review and + /// invisible at run time, since nothing is broken. Defaulting to colour makes the same mistake + /// visible as "colour is on when `NO_COLOR` is set", which somebody reports. + #[test] + fn a_pane_with_no_theme_set_renders_in_colour() { + let mut fresh = ResultsPane::new(); + fresh.attach(Policy::InApp); + fresh.complete(1, None); + + let explicit = { + let mut pane = ResultsPane::new(); + pane.attach(Policy::InApp); + pane.complete(1, None); + pane.set_theme(Theme::colour()); + rendered_style_cells(&pane, 40, 3) + }; + + assert_eq!( + rendered_style_cells(&fresh, 40, 3), + explicit, + "a pane whose theme was never set must render exactly as one set to Theme::colour(). \ + Defaulting to monochrome would make a forgotten set_theme call indistinguishable from \ + working code" + ); + } + + /// Adding styling did **not** open the SR-1 escape path. + /// + /// `results_pane.rs`'s module docs record two measured facts: `Span::raw` does not put an ESC + /// byte into a `Cell` in ratatui 0.30.2 (the grapheme filter drops control characters), but + /// `Cell::set_symbol` is public and bypasses that filter entirely. So "we now attach styles" + /// is safe only as long as the styling goes through `Line`/`Span`, which is a property of the + /// implementation rather than of the API. + /// + /// This asserts it from the outside for every state, styled: no rendered cell contains an ESC + /// byte and no cell contains the `[2J` payload residue. The residue half is the half that can + /// fail — see the module docs on why the ESC half alone is vacuous. + #[test] + fn styling_did_not_open_a_path_for_an_escape_to_reach_a_cell() { + let mut pane = ResultsPane::new(); + pane.set_theme(Theme::colour()); + pane.attach(Policy::InApp); + pane.push_bytes(b"before\x1b[2Jafter\n"); + pane.complete(0, None); + + let cells = rendered_style_cells(&pane, 60, 4); + let text: String = cells.iter().map(|(symbol, _, _)| symbol.as_str()).collect(); + + assert!( + !text.bytes().any(|byte| byte == 0x1b), + "an ESC byte reached a rendered cell. Note this half of the assertion cannot fail via \ + Span/Paragraph in ratatui 0.30.2 — it is here to catch a switch to Cell::set_symbol, \ + which bypasses the grapheme filter (SR-1)" + ); + assert!( + !text.contains("2J"), + "the payload residue `[2J` reached a rendered cell, so the strip was bypassed. This is \ + the half that CAN fail: got {text:?}" + ); + } + /// **F-3 (§12a review) — HANDOFF with a NON-EMPTY buffer.** /// /// Every other HANDOFF test uses an empty buffer, so the `(Handoff, _)` arm's indifference to diff --git a/tui/src/theme.rs b/tui/src/theme.rs new file mode 100644 index 000000000..847b1b09e --- /dev/null +++ b/tui/src/theme.rs @@ -0,0 +1,608 @@ +//! The semantic colour layer: six roles, an ANSI-16 palette, and `NO_COLOR` (issue #556). +//! +//! **This is the only module in the crate permitted to name a [`Color`].** Call sites say +//! `theme.required`, never `Color::Yellow`, so the palette is one reviewable file — the same +//! "put the decision in one auditable place" discipline `catalog.rs` applies to the run policy. +//! `tests/no_colour_literal_outside_theme.rs` is what makes that a failing test rather than a +//! convention. +//! +//! # Colour here is DECORATION, never information (NFR-3 of #321) +//! +//! The affirmed rule is *"no state conveyed by colour alone"*, and every state marker in this +//! crate is already textual: `>` for focus, `*` for selection, `(required)`, the state word from +//! `renderer::pane_state_word`, `exit {code}`, `running…`. So this module decides what gets +//! **emphasized**, not what anything **means**. Nothing breaks if it is absent — which is +//! precisely why `NO_COLOR` support is cheap and why the strip-styling guard is the one test in +//! this feature that cannot be left as a convention. +//! +//! # ANSI-16 ONLY. No RGB, no 256-colour. +//! +//! The 16 base colours are resolved by the terminal against the palette **the operator already +//! chose**, so they respect a theme this crate cannot see and behave over `ssh`, in `tmux`, and +//! in `screen`. A hardcoded `Color::Rgb` fights that setup instead of joining it. +//! +//! `Color::Black` and `Color::White` are avoided **entirely** — they invert between light and +//! dark terminals, so a `White` foreground is invisible on a light background and vice versa. +//! [`Color::Reset`] is the default-foreground role instead, and it is exactly right for the +//! monochrome theme: `Cell::EMPTY` is `fg: Color::Reset`, so a `Reset` foreground is +//! indistinguishable from an unstyled cell. +//! +//! # There is no border and no selected-row highlight to colour +//! +//! Issue #556's palette table assigns `focus` to "focused region border, selected row". **This +//! crate has neither** — it renders exactly one `Block`, and it is `Block::new()` with no +//! borders; there is no `List`, no `Table`, no `highlight_style`. So [`Theme::focus`] applies to +//! the structural `>` marker `renderer` already writes and to the line carrying it. Adding +//! borders would be a layout change interacting with NFR-6's sub-80x24 stacked mode, which is a +//! different piece of work. (#556) + +use ratatui::style::{Color, Modifier, Style}; + +/// The number of semantic roles. A seventh role must update [`Theme::roles`], whose return type +/// is a fixed-size array — so the count and the accessor cannot drift apart silently. +/// +/// Used only by [`Theme::roles`] and by tests. Kept because it is what makes the "adding a seventh +/// role does not silently escape the guards" property hold — [`Theme::roles`]'s return type is +/// `[_; ROLE_COUNT]`, so a seventh field stops the file compiling rather than quietly leaving one +/// role unscanned. A hand-maintained count that only *reads* right is the failure mode this avoids. +/// +/// `allow` and not `expect`: under `--all-targets` the test cfg *does* use it, so an `expect` +/// would be unfulfilled there and `-D warnings` would fail the gate on the very attribute added +/// to satisfy it — the same trap `error.rs` documents on `TuiError::Http`. (#556) +#[allow(dead_code)] +pub(crate) const ROLE_COUNT: usize = 6; + +/// Six semantic roles. Call sites name a **role**; only this module names a **colour**. +/// +/// `Copy` because it is six [`Style`] values (each two `Option` plus two `Modifier` +/// bitflags) and it is read on every render path: threading it as a value costs no allocation +/// and, more usefully, no lifetime — which is what lets `renderer`'s shell hold one field and +/// `ResultsPane` own a copy without either growing a borrow. (#556) +/// +/// # Every role has a production consumer +/// +/// This module landed one task ahead of its call sites, under a struct-level +/// `#[allow(dead_code)]`, so that six colours could be argued with in one file before 40 call-site +/// edits were attached to them. **That attribute is gone**: `focus` and `required` are read by +/// `renderer::style_form`, `error` and `dim` by `renderer::style_pickers`, and `ok`/`warn`/`error` +/// by `results_pane::footer_style`. Only [`ROLE_COUNT`] and [`Theme::roles`] keep an `allow`, and +/// each documents why on itself. +/// +/// A role with no consumer is a role no operator ever sees, so the *absence* of that attribute is +/// load-bearing: it is what makes `cargo clippy -D warnings` the check that a seventh role added +/// here also gets used. (#556) +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) struct Theme { + /// The focus marker `>` and the line it marks. + /// + /// **Not a border** — see the module docs. `Cyan` is the conventional "you are here" hue and + /// is the least likely of the 16 to collide with the semantic roles below. + pub(crate) focus: Style, + + /// An unset required field's `(required)` suffix, and the footer's blocked reason. + /// + /// Shares `Yellow` with [`Theme::warn`] deliberately: both mean "attention, not failure", and + /// inventing a distinct hue for a distinction the operator does not need would spend one of + /// the 16. They are separate *roles* because a future palette may want to split them; the + /// call sites already do. + pub(crate) required: Style, + + /// `exit 0`. + pub(crate) ok: Style, + + /// Failures, a non-zero exit, a refused hand-off (FR-5.3 of #321), a failed picker. + pub(crate) error: Style, + + /// A cancelled follow, and a **missing** exit code. + /// + /// `exit ?` is `warn` and not `error`, and that is load-bearing: a missing exit code does not + /// mean the command failed, it means the pane reached a terminal state without one. Styling + /// it `error` would assert a failure the pane does not know occurred. See + /// `results_pane::ResultsPane::footer`, whose comment makes the same distinction in text. + pub(crate) warn: Style, + + /// `[unavailable]`, `[not sent — …]`, the collapsed strip, the truncation marker, key hints. + /// + /// `DarkGray` rather than `Modifier::DIM`: the DIM attribute is unimplemented or ignored by a + /// number of terminals, so it is not a reliable de-emphasis channel on its own. + pub(crate) dim: Style, +} + +impl Theme { + /// The ANSI-16 palette. + /// + /// `const` so it is a compile-time constant with no initialisation cost, and so a future + /// non-ANSI-16 colour is a *compile* error inside a `const fn` rather than a runtime value + /// only the guard would catch. (#556) + pub(crate) const fn colour() -> Self { + Self { + focus: Style::new().fg(Color::Cyan), + required: Style::new().fg(Color::Yellow), + ok: Style::new().fg(Color::Green), + error: Style::new().fg(Color::Red), + warn: Style::new().fg(Color::Yellow), + dim: Style::new().fg(Color::DarkGray), + } + } + + /// Every role at [`Color::Reset`] — the theme `NO_COLOR` selects. + /// + /// # Why `Reset` and not `Style::new()` + /// + /// `Style::new()` leaves `fg: None`, which means "do not change the cell's colour". That is + /// *usually* the same thing, but it inherits whatever a previously-rendered widget left in + /// the cell. `Color::Reset` states the intent — the terminal's default foreground — and + /// matches `Cell::EMPTY`'s own `fg`, which is what makes the monochrome render + /// indistinguishable from an unstyled one in a buffer assertion. + /// + /// # `Modifier`s ARE retained, deliberately + /// + /// `NO_COLOR` is about **colour**. Bold is not colour — and once colour is gone it is the + /// only emphasis channel left, so stripping it would make this theme strictly worse than it + /// needs to be for the operators who asked for it. `focus` and `required` keep `BOLD` so the + /// two roles that answer "where am I" and "what is blocking me" remain visually findable on + /// a monochrome terminal. The test asserts the modifier is *permitted*, not forbidden. (#556) + pub(crate) const fn monochrome() -> Self { + let plain = Style::new().fg(Color::Reset); + Self { + focus: plain.add_modifier(Modifier::BOLD), + required: plain.add_modifier(Modifier::BOLD), + ok: plain, + error: plain.add_modifier(Modifier::BOLD), + warn: plain, + dim: plain, + } + } + + /// [`Self::monochrome`] when `NO_COLOR` is set, [`Self::colour`] otherwise. + /// + /// # `NO_COLOR=` (empty) DISABLES colour — a deliberate deviation from + /// + /// That spec says colour should be disabled when the variable is "present and **not an empty + /// string**". This implementation treats *any* presence, empty included, as a request for + /// monochrome. The reason is which way the two readings fail: + /// + /// - An empty value is what a shell produces from `export NO_COLOR=`, and what a CI matrix + /// produces from an unset job variable. Reading that as "colour on" **ignores an + /// accessibility request on a technicality**. + /// - Reading it as "colour off" merely loses decoration, and every state stays legible + /// because no state is conveyed by colour alone (NFR-3). + /// + /// One direction is safe to be wrong in and the other is not. The divergence is recorded + /// here, next to the link, so a future reader sees a decision rather than a bug. (#556) + /// + /// # `var_os`, not `var` + /// + /// [`std::env::var`] returns `Err(NotUnicode)` for a non-UTF-8 value, which forces a caller + /// to classify it — and classifying "set to invalid UTF-8" as "not set" would re-enable + /// colour against an operator who asked for it off, the same failure as the empty-string case + /// one step removed. `var_os` collapses the question to present/absent, which is the only + /// distinction this function makes. + /// + /// # Read ONCE, at startup + /// + /// The caller stores the result; this is not called per frame. A per-frame `var_os` would be + /// a syscall in the render path for a value that cannot change mid-process, and it would make + /// the theme untestable without mutating process env — which `cargo test`'s threads make racy + /// and edition 2024 makes `unsafe` (and this crate is `#![forbid(unsafe_code)]`). The branch + /// itself is tested through [`Self::from_env_var`]. (#556) + pub(crate) fn from_env() -> Self { + Self::from_env_var(std::env::var_os("NO_COLOR")) + } + + /// The pure half of [`Self::from_env`]: the decision, with the environment read hoisted out. + /// + /// Separated **only** so the three `NO_COLOR` cases — a value, the empty string, and non-UTF-8 + /// bytes — are testable without touching process env. `from_env` is then a single expression + /// with nothing left in it to get wrong. (#556) + fn from_env_var(no_color: Option) -> Self { + match no_color { + // ANY presence, including "" — see the deviation note on `from_env`. + Some(_) => Self::monochrome(), + None => Self::colour(), + } + } + + /// Every role, named, for the exhaustive tests. + /// + /// # Why a fixed-size array and not a `Vec` + /// + /// The return type is `[_; ROLE_COUNT]`, so adding a seventh field to [`Theme`] without + /// listing it here **fails to compile**. A `Vec` — or a test that names the six fields itself + /// — would go stale on exactly the change it exists to catch: a seventh role added, and every + /// "all roles" test silently covering six of seven. A yardstick that is hand-maintained fails + /// when it is needed. (#556) + /// + /// `allow(dead_code)` for the same reason as [`ROLE_COUNT`]: its callers are all in test cfgs, + /// and `expect` would be unfulfilled under `--all-targets`. + #[allow(dead_code)] + pub(crate) const fn roles(&self) -> [(&'static str, Style); ROLE_COUNT] { + [ + ("focus", self.focus), + ("required", self.required), + ("ok", self.ok), + ("error", self.error), + ("warn", self.warn), + ("dim", self.dim), + ] + } +} + +impl Default for Theme { + fn default() -> Self { + Self::colour() + } +} + +#[cfg(test)] +mod tests { + use super::{Theme, ROLE_COUNT}; + use ratatui::style::{Color, Modifier, Style}; + use std::ffi::OsString; + + /// The sixteen ANSI colours plus `Reset`, **hard-coded**. + /// + /// Deliberately a literal list and NOT derived from [`Theme::colour`]. A fixture sourced from + /// the value under test cannot fail: an allow-set built by collecting the theme's own colours + /// would accept `Color::Rgb(1, 2, 3)` the moment somebody wrote it, which is the entire + /// property this list exists to check. (#556) + const ANSI_16_AND_RESET: [Color; 17] = [ + Color::Reset, + Color::Black, + Color::Red, + Color::Green, + Color::Yellow, + Color::Blue, + Color::Magenta, + Color::Cyan, + Color::Gray, + Color::DarkGray, + Color::LightRed, + Color::LightGreen, + Color::LightYellow, + Color::LightBlue, + Color::LightMagenta, + Color::LightCyan, + Color::White, + ]; + + // ── FR-2.1 / FR-2.2 — the palette ──────────────────────────────────────────────────────── + + /// **FR-2.2: every foreground in the colour theme is an ANSI-16 colour.** + /// + /// Iterates [`Theme::roles`], so it covers all six by construction rather than by a list this + /// test maintains. Checked against [`ANSI_16_AND_RESET`] — see that constant for why it is a + /// literal. + #[test] + fn every_colour_in_the_default_theme_is_ansi_16() { + for (name, style) in Theme::colour().roles() { + let fg = style + .fg + .unwrap_or_else(|| panic!("role `{name}` must set a foreground colour")); + assert!( + ANSI_16_AND_RESET.contains(&fg), + "role `{name}` uses {fg:?}, which is not in the ANSI-16 set. RGB and 256-colour \ + do not resolve against the operator's own terminal palette (FR-2.2, FR-2.3)" + ); + assert!( + style.bg.is_none(), + "role `{name}` sets a BACKGROUND ({:?}). A background colour is not decoration — \ + it repaints the cell and can make the foreground unreadable on a terminal theme \ + this crate cannot see", + style.bg + ); + } + } + + /// **FR-2.4: neither `Black` nor `White` appears in either theme.** + /// + /// The source-text guard in `tests/no_colour_literal_outside_theme.rs` checks the same + /// property over the whole crate. This one checks the *values*, which is a different net: a + /// colour reached through a `const` alias or a helper would satisfy the text scan and fail + /// here. + #[test] + fn neither_theme_uses_black_or_white() { + for (label, theme) in [("colour", Theme::colour()), ("mono", Theme::monochrome())] { + for (name, style) in theme.roles() { + for (channel, colour) in [("fg", style.fg), ("bg", style.bg)] { + assert!( + colour != Some(Color::Black) && colour != Some(Color::White), + "{label} theme role `{name}` uses {colour:?} for {channel}. Black and \ + White invert between light and dark terminals, so one of the two is \ + always unreadable (FR-2.4)" + ); + } + } + } + } + + /// **FR-2.1: there are exactly six roles, and each is distinguishable from unstyled.** + /// + /// The second half is the anti-vacuity check for the palette itself. Every assertion in this + /// module's other tests passes on a `Theme` whose six fields are all `Style::new()` — an + /// all-default theme has no non-ANSI colour, no `Black`, no `White`, and no background. So + /// without this, the palette could be empty and the suite green. (#556) + #[test] + fn the_colour_theme_has_six_distinguishable_roles() { + let roles = Theme::colour().roles(); + assert_eq!( + roles.len(), + ROLE_COUNT, + "the semantic palette is deliberately tiny; a seventh role is a design change" + ); + + for (name, style) in roles { + assert_ne!( + style, + Style::new(), + "role `{name}` is indistinguishable from unstyled, so every other assertion in \ + this module passes vacuously for it" + ); + } + } + + // ── FR-3 — NO_COLOR ────────────────────────────────────────────────────────────────────── + + /// **FR-3.1: `NO_COLOR` set to a value yields the monochrome theme; unset yields colour.** + #[test] + fn no_color_selects_the_monochrome_theme_and_its_absence_selects_colour() { + assert_eq!( + Theme::from_env_var(Some(OsString::from("1"))), + Theme::monochrome(), + "NO_COLOR=1 must disable colour (FR-3.1)" + ); + assert_eq!( + Theme::from_env_var(None), + Theme::colour(), + "an unset NO_COLOR is the ONLY case that enables colour (FR-3.1)" + ); + } + + /// **FR-3.2: `NO_COLOR=` — the empty string — disables colour.** + /// + /// This is the deliberate deviation from no-color.org, which says "present and not an empty + /// string". The reasoning is on [`Theme::from_env`]; this test is what makes the deviation a + /// pinned behaviour rather than an accident of implementation, so a future "fix" toward the + /// letter of the spec has to argue with a red test. (#556) + #[test] + fn an_empty_no_color_still_disables_colour() { + assert_eq!( + Theme::from_env_var(Some(OsString::new())), + Theme::monochrome(), + "NO_COLOR= is what `export NO_COLOR=` and an unset CI variable produce. Reading it \ + as `colour on` ignores an accessibility request on a technicality (FR-3.2)" + ); + } + + /// **FR-3.1: a non-UTF-8 `NO_COLOR` disables colour and does not panic.** + /// + /// The value is built from raw bytes that are not valid UTF-8, which is exactly the input + /// `std::env::var` would reject with `NotUnicode` — the case that makes `var_os` the right + /// call. Platform-gated because `OsStringExt` is a unix extension; the property it checks is + /// about `var_os` versus `var` and holds on every platform, but only unix can *construct* the + /// witness. (#556) + #[cfg(unix)] + #[test] + fn a_non_utf8_no_color_disables_colour_without_panicking() { + use std::os::unix::ffi::OsStringExt; + + // 0x80 is a UTF-8 continuation byte with no lead byte: invalid on its own. + let invalid = OsString::from_vec(vec![0x80]); + assert!( + invalid.to_str().is_none(), + "the witness must actually be invalid UTF-8, or this test proves nothing about \ + var_os vs var" + ); + assert_eq!( + Theme::from_env_var(Some(invalid)), + Theme::monochrome(), + "a non-UTF-8 NO_COLOR is SET. Treating it as unset would re-enable colour against an \ + operator who asked for it off" + ); + } + + /// **FR-3.3: every monochrome role is `Color::Reset` in the foreground and sets no + /// background.** + /// + /// Stated as a property of the *value* rather than of a render, because "renders without + /// colour" is only observable through a terminal. `Cell::EMPTY` is `fg: Color::Reset`, so a + /// `Reset` foreground is indistinguishable from an unstyled cell in a buffer — which is what + /// makes `results_pane`'s end-to-end monochrome assertion possible without one. (#556) + #[test] + fn every_monochrome_role_is_reset_with_no_background() { + for (name, style) in Theme::monochrome().roles() { + assert_eq!( + style.fg, + Some(Color::Reset), + "monochrome role `{name}` must be Color::Reset — the terminal's own default \ + foreground (FR-3.3)" + ); + assert!( + style.bg.is_none(), + "monochrome role `{name}` sets a background, which is still colour (FR-3.3)" + ); + } + } + + /// **FR-3.4: the monochrome theme MAY carry `Modifier`s, and this test asserts it does.** + /// + /// `NO_COLOR` is about colour; bold is not colour, and it is the only emphasis channel left + /// once colour is gone. A reviewer would reasonably propose asserting `Style::new()` exactly + /// here — this test is the counter-argument in executable form, so removing the modifiers is + /// a deliberate change rather than a tidy-up. (#556) + #[test] + fn the_monochrome_theme_keeps_bold_as_its_emphasis_channel() { + let mono = Theme::monochrome(); + assert!( + mono.focus.add_modifier.contains(Modifier::BOLD), + "with colour gone, BOLD is the only way `focus` can answer `where am I` (FR-3.4)" + ); + assert!( + mono.required.add_modifier.contains(Modifier::BOLD), + "with colour gone, BOLD is the only way `required` can answer `what is blocking me` \ + (FR-3.4)" + ); + assert!( + mono.warn.add_modifier.is_empty(), + "not every role takes a modifier; `warn` is plain, so this test cannot pass by \ + blanket-bolding the theme" + ); + } + + /// This module's production region with every comment removed. + /// + /// # Why the stripping is necessary, not tidiness + /// + /// The first version of [`from_env_reads_only_no_color`] scanned the raw region and asserted + /// one `var_os`. It found **four** — because the doc comments on [`Theme::from_env`] explain + /// at length why `var_os` and not `var`, and a text scan cannot tell an explanation from a + /// call. A prose-sensitive guard is worse than none: it goes red when somebody *documents* + /// the module and green when somebody *breaks* it. + /// + /// The cut is **literal-aware**, and that is not caution for its own sake. The first version + /// cut at the first `//` on the line, justified by "this region has no string literal + /// containing `//`" — a claim that happens to hold today and is exactly the reasoning + /// `no_backend_attach_call.rs` records as having already been **false** once, when + /// `format!("http://{host}:{port}")` silently truncated its scan mid-URL. The stale + /// justification was the defect there, because it read as a considered trade-off. Tracking the + /// literal costs eight lines and removes the claim entirely. (#556) + fn production_code() -> String { + let source = include_str!("theme.rs"); + let (production, _tests) = source + .split_once("#[cfg(test)]") + .expect("this module must carry a #[cfg(test)] marker for the region split to hold"); + + production + .lines() + .map(strip_line_comment) + .collect::>() + .join("\n") + } + + /// Returns `line` up to a `//` that is **not** inside a string literal. + /// + /// Deliberately the same shape as `tests/no_backend_attach_call.rs::strip_line_comment` rather + /// than a shared helper: an integration test is its own crate, so a tripwire cannot import + /// from another, and the crate's established answer is that each guard carries its own copy. + /// Not a full lexer — no raw strings, no block comments — and it does not need to be, because + /// [`the_stripper_is_literal_aware`] pins the one property this relies on. + fn strip_line_comment(line: &str) -> &str { + let bytes = line.as_bytes(); + let mut in_string = false; + let mut index = 0; + + while index < bytes.len() { + match bytes[index] { + // An escape inside a literal consumes the next byte, so `\"` does not end it. + b'\\' if in_string => index += 1, + b'"' => in_string = !in_string, + b'/' if !in_string && bytes.get(index + 1) == Some(&b'/') => return &line[..index], + _ => {} + } + index += 1; + } + line + } + + /// [`strip_line_comment`] keeps code after a `//`-bearing literal, and still cuts real + /// comments. + /// + /// Both directions, because the two plausible implementations each fail one of them and a test + /// asserting only one would license the other bug: cutting at the first `//` truncates inside + /// `http://` and hides whatever follows, while cutting only whole-line comments lets a + /// trailing `// Color::Red` comment fire a colour guard on prose — and this module carries + /// three `#[allow(dead_code)] // reason` comments, so that is not hypothetical. + /// + /// The URL fixture is **assembled** rather than written as a literal. `hermeticity_tripwire.rs` + /// treats a plaintext `http:` in code as its catch-all for an HTTP client it does not know + /// about, and it is right to: a guard that exempted "but this one is only a test fixture" would + /// exempt the real thing too. Writing the scheme out here fires that tripwire — verified, it + /// did — so it is built from pieces, which is the same dodge `no_backend_attach_call.rs` uses + /// for the identical reason. (#556) + #[test] + fn the_stripper_is_literal_aware() { + let scheme = format!("htt{}{}", "p:", "//"); + let url_line = format!(r#"let u = "{scheme}x"; let y = 1;"#); + assert_eq!( + strip_line_comment(&url_line), + url_line, + "the `//` inside a literal is not a comment; cutting there hides the code after it" + ); + assert_eq!( + strip_line_comment("let x = 1; // Color::Red"), + "let x = 1; ", + "a trailing comment must still be cut, or an absence guard fires on prose" + ); + assert_eq!( + strip_line_comment(r#"let q = "a\"//b"; let z = 2;"#), + r#"let q = "a\"//b"; let z = 2;"#, + "an escaped quote does not end the literal, so the `//` after it is still not a comment" + ); + } + + /// **FR-3.5: `from_env` reads `NO_COLOR` and nothing else.** + /// + /// A source-text assertion, because "reads no other variable" is not observable from the + /// outside — a second `var_os` call would simply change behaviour on a machine where that + /// variable happens to be set, which no value-level test can see. #556 floats a future + /// `CAO_TUI_NO_COLOR`; it is deliberately NOT implemented, because `CAO_TUI_*` is a namespace + /// this repository has never used and a decorative-styling change is the wrong place to + /// establish one. The `Theme` struct is the extension seam, not an env var. (#556) + #[test] + fn from_env_reads_only_no_color() { + let code = production_code(); + + // Anti-vacuity: every assertion below is an ABSENCE check, and all of them pass on an + // empty string. If the region split or the comment strip ever swallows the module, this + // is the line that notices. + assert!( + code.contains("fn from_env"), + "the scanned region does not contain `from_env`, so the absence checks below prove \ + nothing about it" + ); + + assert_eq!( + code.matches("env::").count(), + 1, + "exactly ONE environment read is permitted in production. Found: {:?}", + code.match_indices("env::").count() + ); + assert!( + code.contains("env::var_os(\"NO_COLOR\")"), + "the one permitted read must be `var_os` on NO_COLOR. `var` returns Err(NotUnicode) \ + for a non-UTF-8 value, and classifying that as `not set` re-enables colour against \ + an operator who asked for it off" + ); + assert!( + !code.contains("CAO_TUI"), + "CAO_TUI_* is unprecedented in this repository; #556's future hook is the Theme \ + struct, not an env var (FR-3.5)" + ); + assert!( + !code.contains("TERM"), + "terminal-capability detection is not in scope; NO_COLOR is the operator's own \ + explicit request and needs no corroboration (FR-3.5)" + ); + } + + // ── Housekeeping ───────────────────────────────────────────────────────────────────────── + + /// `Default` is the colour theme, so a `Theme::default()` written by mistake is not silently + /// monochrome — which would look like working code that never shows a colour. + #[test] + fn default_is_the_colour_theme() { + assert_eq!(Theme::default(), Theme::colour()); + } + + /// The two themes differ. Guards against a refactor that makes `colour()` delegate to + /// `monochrome()`, which would leave every FR-3 test above green and every FR-2 test + /// vacuous. + #[test] + fn the_two_themes_are_not_the_same_theme() { + assert_ne!( + Theme::colour(), + Theme::monochrome(), + "if these are equal, NO_COLOR is a no-op and the palette does not exist" + ); + } +} diff --git a/tui/tests/hermeticity_tripwire.rs b/tui/tests/hermeticity_tripwire.rs index 704c9feb9..c99f29a07 100644 --- a/tui/tests/hermeticity_tripwire.rs +++ b/tui/tests/hermeticity_tripwire.rs @@ -121,6 +121,16 @@ const SOURCES: &[(&str, &str)] = &[ // additionally where FR-1.4's forbidden CLI fallback would be written ("the picker failed, so // shell out to `cao profile list`") — the `cao`-spawn needles catch that. (#321) ("src/renderer.rs", include_str!("../src/renderer.rs")), + // Added by the semantic colour layer (#556). This entry was **not** forced by a failing test: + // when `mod theme;` landed, `no_backend_attach_call.rs` went red immediately and this tripwire + // stayed green, because its coverage check is a hand-maintained count and a count cannot + // notice an omission. The `catalog.rs` comment above named that hole exactly, and it stayed + // open — so `theme.rs` sat unscanned by the HTTP-ownership guard until somebody thought to + // look. It is closed now: [`the_scan_set_covers_every_source_in_the_crate`] cross-checks + // `src/main.rs`'s `mod` declarations, so the next module is forced in rather than remembered + // in. Nothing in a palette wants HTTP, which is the point — the file judged least interesting + // is the one an omission hides in. (#556) + ("src/theme.rs", include_str!("../src/theme.rs")), // Test targets and shared test infrastructure. ( "tests/binary_exits_zero.rs", @@ -143,6 +153,16 @@ const SOURCES: &[(&str, &str)] = &[ "tests/hermeticity_tripwire.rs", include_str!("hermeticity_tripwire.rs"), ), + // Added by the semantic colour layer (#556). A source-text guard needs no I/O at all, which is + // exactly why it is listed: the plausible regression is somebody deciding the scan would be + // tidier reading files from disk than embedding them with `include_str!`, and `fs` is one step + // from `minreq`. The `mod` cross-check above cannot force this entry — it derives from + // `src/main.rs`, and a test target is not a module — so a new file under `tests/` still has to + // be remembered. That gap is named in [`what_this_tripwire_cannot_detect`]. (#556) + ( + "tests/no_colour_literal_outside_theme.rs", + include_str!("no_colour_literal_outside_theme.rs"), + ), ]; /// The exemption set. **Exactly one member** (BR-9, INV-3, SR-5). @@ -847,28 +867,82 @@ fn every_needle_is_actually_findable_in_stripped_code() { /// /// The number is a literal for the usual reason — `SOURCES.len()` compared against itself proves /// nothing. +/// +/// # The count was never enough, and #556 demonstrated it +/// +/// A count reddens when a file is **added** to this list and stays green when one is **omitted**, +/// which is the wrong direction: the unlisted file is the unscanned one. That asymmetry was +/// documented on the `catalog.rs` entry in [`SOURCES`] and left open here, while +/// `no_backend_attach_call.rs` closed it by cross-checking `src/main.rs`'s `mod` declarations. The +/// consequence arrived on schedule: `mod theme;` landed, that tripwire went red and this one did +/// not, so `src/theme.rs` was outside the HTTP-ownership scan while this test reported full +/// coverage. The cross-check below is that repair — a `mod` declaration with no [`SOURCES`] entry +/// is now a failing test, so the next module cannot be forgotten the same way. (#556) #[test] fn the_scan_set_covers_every_source_in_the_crate() { assert_eq!( SOURCES.len(), - 16, - "expected 16 Rust sources: 10 under src/ (main, error, handoff, types, env_guard, catalog, \ - results_pane, server, guided_flow, renderer) and 6 under tests/ (binary_exits_zero, \ - endpoint_contract, no_backend_attach_call, pty, pty_harness/mod, hermeticity_tripwire). A \ - new file must be added to SOURCES or the tripwire silently stops covering it" + 18, + "expected 18 Rust sources: 11 under src/ (main, error, handoff, types, env_guard, catalog, \ + results_pane, server, guided_flow, renderer, theme) and 7 under tests/ \ + (binary_exits_zero, endpoint_contract, no_backend_attach_call, pty, pty_harness/mod, \ + hermeticity_tripwire, no_colour_literal_outside_theme). A new file must be added to \ + SOURCES or the tripwire silently stops covering it" ); let production = SOURCES .iter() .filter(|(path, _)| path.starts_with("src/")) .count(); - assert_eq!(production, 10, "10 production sources"); + assert_eq!(production, 11, "11 production sources"); let test_sources = SOURCES .iter() .filter(|(path, _)| path.starts_with("tests/")) .count(); - assert_eq!(test_sources, 6, "6 test sources"); + assert_eq!(test_sources, 7, "7 test sources"); + + // Every `mod` declared by the crate root must be listed. This is the half the count cannot + // do — see the note above on why `theme.rs` went unscanned. Derived from the declarations + // rather than from a second literal, so it closes the omission direction instead of + // restating the addition one. + let (_, crate_root) = SOURCES + .iter() + .find(|(path, _)| *path == "src/main.rs") + .expect("src/main.rs must be listed in SOURCES for the mod cross-check to run"); + + let mut declared = 0; + for line in crate_root.lines() { + let trimmed = line.trim(); + // `pub mod`/`pub(crate) mod` are handled so this does not quietly stop matching if a + // module's visibility changes. An inline `mod x { .. }` is not a separate file and is + // skipped; today every declaration in the root is a file. + let Some(rest) = trimmed + .strip_prefix("mod ") + .or_else(|| trimmed.strip_prefix("pub mod ")) + .or_else(|| trimmed.strip_prefix("pub(crate) mod ")) + else { + continue; + }; + let Some(module) = rest.strip_suffix(';') else { + continue; + }; + + declared += 1; + let expected = format!("src/{module}.rs"); + assert!( + SOURCES.iter().any(|(path, _)| *path == expected), + "`src/main.rs` declares `mod {module};` but {expected} is not in SOURCES, so this \ + tripwire does not scan it. An unlisted module is a silent hole: the count assertion \ + above only fires when a file is ADDED to the list, never when one is omitted" + ); + } + + assert_eq!( + declared, 10, + "expected 10 `mod` declarations in src/main.rs (every production source except main.rs \ + itself). If this is 0 the loop above matched nothing and its assertion is vacuous" + ); // No duplicate paths: a duplicated entry would inflate the count above and let a real file // go unlisted while the assertion still passed. diff --git a/tui/tests/no_backend_attach_call.rs b/tui/tests/no_backend_attach_call.rs index acdffc7df..0c8a63b75 100644 --- a/tui/tests/no_backend_attach_call.rs +++ b/tui/tests/no_backend_attach_call.rs @@ -88,6 +88,14 @@ const SOURCES: &[(&str, &str)] = &[ // verb: `renderer` receives the refusal argv as an opaque `Option` from `Refused` and // hands it straight to `ResultsPane::refuse`, so it never names, builds, or spawns it. (#321) ("src/renderer.rs", include_str!("../src/renderer.rs")), + // Added by the semantic colour layer (#556). Listed for coverage, not because a palette is a + // plausible place to spawn tmux — it is the least likely module in the crate. That is the + // reason to list it rather than an argument against: the `mod`-declaration cross-check below + // makes every `src/` file mandatory precisely so nobody has to be right about which files are + // "interesting", and the one judged uninteresting is where an unscanned hole would sit. Adding + // this entry was in fact FORCED by that cross-check, which reddened the moment `main.rs` + // declared `mod theme;` — the asymmetry the `env_guard` comment above describes, working. (#556) + ("src/theme.rs", include_str!("../src/theme.rs")), ]; /// Strips `//`-comments so the needles named in prose are not counted as code. @@ -340,10 +348,10 @@ fn no_rust_source_calls_either_backend_attach_session() { // (#321) assert_eq!( SOURCES.len(), - 10, - "expected exactly 10 Rust sources under src/ (main, error, handoff, types, env_guard, \ - catalog, results_pane, server, guided_flow, renderer); a new module must be added to \ - SOURCES or this tripwire silently stops covering it" + 11, + "expected exactly 11 Rust sources under src/ (main, error, handoff, types, env_guard, \ + catalog, results_pane, server, guided_flow, renderer, theme); a new module must be added \ + to SOURCES or this tripwire silently stops covering it" ); let crate_root = SOURCES diff --git a/tui/tests/no_colour_literal_outside_theme.rs b/tui/tests/no_colour_literal_outside_theme.rs new file mode 100644 index 000000000..d2b6e9547 --- /dev/null +++ b/tui/tests/no_colour_literal_outside_theme.rs @@ -0,0 +1,558 @@ +//! **`src/theme.rs` is the only module permitted to name a `Color`** (#556). +//! +//! # What this guard is for +//! +//! A semantic colour layer whose rule is a convention is a colour layer that decays. The first +//! call site that writes `Color::Yellow` inline because it is quicker than adding a role does not +//! break anything, so nothing objects; the tenth one has made the palette unfindable and +//! `NO_COLOR` a lie, because `from_env` cannot switch a literal. This test is what makes the rule +//! fail a build. +//! +//! It also enforces the palette's two shape rules, which are not about ownership at all: +//! +//! - **ANSI-16 only** (FR-2.3). `Color::Rgb` and `Color::Indexed` are scanned for over the +//! **whole** crate, test regions included, because a 24-bit colour hardcodes a value the +//! operator's own terminal palette cannot override — and a test asserting one is a fixture +//! pinning a behaviour the production palette must never have. +//! - **No `Black`, no `White`** (FR-2.4). They invert between light and dark terminals, so +//! whichever one is chosen is unreadable on half the terminals in use. +//! +//! # The guard is worthless without its anti-vacuity floor, and that is not a figure of speech +//! +//! Every ownership and shape check in this file is an **absence** check, and every one of them +//! passes on a crate with no colour anywhere — which is what this crate was before #556. Run this +//! file against the previous commit and it is green. That makes it indistinguishable, on its own, +//! from a test that does nothing. +//! +//! [`the_theme_module_actually_defines_a_palette`] is the repair: `src/theme.rs`'s production +//! region must name at least [`MINIMUM_THEME_COLOURS`] colours. With it, emptying `theme.rs`, +//! deleting the palette, or reducing `colour()` to `monochrome()` is a red test rather than a +//! green one. +//! +//! # What this cannot detect +//! +//! A static text scan, with the same limits the crate's other two tripwires document: +//! +//! - A colour reached through an alias — `use ratatui::style::Color as C;` then `C::Red` — is +//! invisible here. `theme.rs`'s own value-level tests cover the palette from the other side, and +//! between them the plausible routes are covered; neither alone is sufficient. +//! - A `Style` built from a colour computed at runtime. +//! - A colour written by a dependency. `ratatui`'s own widget defaults are outside this scan by +//! construction, which is one reason `results_pane.rs` renders a borderless `Block::new()` +//! rather than a themed one. +//! +//! These are stated rather than papered over: an overstated guard is worse than a narrow one, +//! because it discourages the guard that would actually have caught the thing. + +/// Every Rust source in the crate, embedded at compile time. +/// +/// `include_str!` rather than a directory walk, following the precedent set by +/// `no_backend_attach_call.rs` and `hermeticity_tripwire.rs`: a walk depends on the runner's +/// working directory and — worse — a walk that silently found no files would **pass**. Embedding +/// makes a missing file a *compile* error, so the scan cannot degrade into a vacuous pass. +/// +/// The cost is that a new file must be listed. That cost is paid by +/// [`the_scan_set_covers_every_module_the_crate_root_declares`], which derives the expected set +/// from `src/main.rs`'s `mod` declarations rather than from a hand-maintained count — because a +/// count only reddens when a file is **added** and stays green when one is **omitted**, and the +/// omitted file is the unscanned one. That exact asymmetry left `src/theme.rs` outside +/// `hermeticity_tripwire.rs`'s scan for the length of this feature's first task. (#556) +const SOURCES: &[(&str, &str)] = &[ + ("src/main.rs", include_str!("../src/main.rs")), + ("src/error.rs", include_str!("../src/error.rs")), + ("src/handoff.rs", include_str!("../src/handoff.rs")), + ("src/types.rs", include_str!("../src/types.rs")), + ("src/env_guard.rs", include_str!("../src/env_guard.rs")), + ("src/catalog.rs", include_str!("../src/catalog.rs")), + ( + "src/results_pane.rs", + include_str!("../src/results_pane.rs"), + ), + ("src/server.rs", include_str!("../src/server.rs")), + ("src/guided_flow.rs", include_str!("../src/guided_flow.rs")), + ("src/renderer.rs", include_str!("../src/renderer.rs")), + ("src/theme.rs", include_str!("../src/theme.rs")), +]; + +/// The one module allowed to name a colour. +const THEME_OWNER: &str = "src/theme.rs"; + +/// The lower bound on colours named in [`THEME_OWNER`]'s production region. +/// +/// Six, because the palette has six roles and each names one. A floor rather than an equality: +/// pinning the exact count would redden on a legitimate refactor — a `const` shared between the +/// two themes, say — which trains people to edit the number instead of reading the test. The +/// property that matters is "the palette is not empty", and a floor states exactly that. +/// +/// Deliberately **not** derived from `theme::ROLE_COUNT`. An integration test is its own crate and +/// cannot see a private item, but the more important reason is that a fixture sourced from the +/// value under test cannot fail: if `theme.rs` were emptied, a derived floor would fall to zero +/// alongside it and this test would stay green through the exact deletion it exists to catch. +const MINIMUM_THEME_COLOURS: usize = 6; + +/// How much of a file a forbidden-variant needle applies to. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum Scope { + /// Production **and** test regions. For a variant with no legitimate use anywhere. + WholeFile, + /// Production regions only. For a variant a test may legitimately *name* in order to forbid + /// it, or to enumerate a set it belongs to. + ProductionOnly, +} + +/// The forbidden `Color` variants and how far each needle reaches. +/// +/// # Why the scope is per-needle, and why it is not all `WholeFile` +/// +/// The design for this guard specified a whole-crate scan for all four, reasoning that "a +/// `Color::Rgb` in a test is a fixture asserting a behaviour the production palette must never +/// have". That reasoning is sound for `Rgb` and `Indexed` and **wrong** for `Black` and `White`, +/// which was found by writing the guard and watching it fail: it fired on three lines in +/// `src/theme.rs`'s own test module, and all three were correct code. +/// +/// - `theme.rs`'s ANSI-16 allow-set is a hard-coded list of all sixteen variants, and `Black` and +/// `White` *are* ANSI-16 colours. Omitting them to satisfy this scan would make the allow-set +/// wrong — it would then reject a legitimately-ANSI colour — and the list must stay a literal, +/// since deriving it from the theme would make it unable to fail. +/// - `theme.rs`'s FR-2.4 test asserts no role uses `Black` or `White`. It cannot assert that +/// without naming them. A guard that fires on the test enforcing the same rule one layer down is +/// a guard that gets deleted, and the deletion would be the reviewer's correct call. +/// +/// `Rgb` and `Indexed` have no such legitimate mention: nothing needs to enumerate them and +/// nothing needs to forbid them by name, so they stay `WholeFile`, where a test fixture pinning a +/// 24-bit colour is caught. +/// +/// The generalisation — "scan test regions unless a test has a reason to name the needle" — is the +/// one this crate's other tripwires already reached from the other direction, with per-region +/// budgets rather than blanket bans. (#556) +const FORBIDDEN_VARIANTS: &[(&str, Scope, &str)] = &[ + ( + "Color::Rgb", + Scope::WholeFile, + "a 24-bit colour hardcodes a value the operator's terminal palette cannot override, and \ + it degrades unpredictably over ssh, tmux and screen (FR-2.3)", + ), + ( + "Color::Indexed", + Scope::WholeFile, + "a 256-colour index is resolved against a palette this crate cannot see; indices 16-255 \ + are not the operator's chosen theme (FR-2.3)", + ), + ( + "Color::Black", + Scope::ProductionOnly, + "Black is invisible on a dark terminal. Color::Reset is the default-foreground role \ + (FR-2.4)", + ), + ( + "Color::White", + Scope::ProductionOnly, + "White is invisible on a light terminal. Color::Reset is the default-foreground role \ + (FR-2.4)", + ), +]; + +// ── The comment stripper ────────────────────────────────────────────────────────────────────── + +/// `source` with every line comment removed. `//` covers `///` and `//!` too. +/// +/// # Why this is not `line.find("//")` +/// +/// Because that was tried, in this crate, and it was wrong. `no_backend_attach_call.rs` records +/// the history: the justification "no string literal in this crate contains `//`" was **false**, +/// since `src/server.rs` builds a URL with `format!`, so cutting at the first `//` truncated real +/// code mid-literal and hid anything after it from the scan. The stale reasoning was the defect, +/// because it read as a considered trade-off. +/// +/// The obvious repair — strip only lines whose trimmed form *starts* with `//` — fails in the +/// other and worse direction: a trailing `let x = 1; // never write Color::Red` would survive into +/// the scanned text and fire this guard **on prose about itself**. This crate carries dozens of +/// `#[allow(..)] // reason` comments, and this feature's own modules discuss `Color::` at length, +/// so that is not hypothetical — it is guaranteed. +/// +/// So the scan tracks whether it is inside a string literal and treats `//` as a comment only +/// when it is not. Deliberately not a full lexer: raw strings and block comments are out of +/// scope, and [`the_stripper_survives_a_literal_containing_a_comment_marker`] pins the assumption +/// that makes that acceptable instead of merely asserting it in prose. +/// +/// Copied rather than shared, because an integration test is its own crate and cannot import from +/// a sibling test target. That is the crate's established answer — `hermeticity_tripwire.rs` +/// carries its own copy for the same reason. +fn code_only(source: &str) -> String { + source + .lines() + .map(strip_line_comment) + .collect::>() + .join("\n") +} + +/// Returns `line` up to a `//` that is not inside a string literal. +fn strip_line_comment(line: &str) -> &str { + let bytes = line.as_bytes(); + let mut in_string = false; + let mut index = 0; + + while index < bytes.len() { + match bytes[index] { + // An escape inside a literal consumes the next byte, so `\"` does not end it. + b'\\' if in_string => index += 1, + b'"' => in_string = !in_string, + b'/' if !in_string && bytes.get(index + 1) == Some(&b'/') => return &line[..index], + _ => {} + } + index += 1; + } + line +} + +/// Splits a source into its production and test regions at the `#[cfg(test)]` marker. +/// +/// # Why a missing marker PANICS rather than falling back to the whole file +/// +/// A whole-file fallback would look harmless and be the opposite. This module's ownership check +/// deliberately scans production only, because a test asserting `Color::Cyan` is legitimate and a +/// production line writing it is not. If the marker were missing and the split silently returned +/// the whole file, the scan would start reading test code as production and fire on a correct +/// test — and the fix a hurried reader would reach for is to delete the assertion. +/// +/// The mirror failure is worse: were the fallback the other way round (no marker → scan nothing), +/// a module with no test module would be scanned not at all. That is precisely where an unchecked +/// colour would sit, since a module nobody tested is a module nobody read. +/// +/// So neither fallback. `no_backend_attach_call.rs` panics at the same point for the same reason. +/// +/// # `split_once`, not `rsplit_once` +/// +/// `src/renderer.rs` contains five occurrences of the marker string: one real attribute and four +/// inside doc comments and string literals discussing the split. Cutting at the **first** is +/// correct — it is the real attribute at `src/renderer.rs:2348`, and everything after it is test +/// code by definition. Cutting at the last would put four test modules' worth of code into the +/// production region and fire this guard on their fixtures. (#556) +fn production_region<'a>(path: &str, code: &'a str) -> &'a str { + let marker = "#[cfg(test)]"; + let (production, _tests) = code.split_once(marker).unwrap_or_else(|| { + panic!( + "{path} contains no `{marker}` marker, so this guard cannot tell its production code \ + from its tests.\n\ + \n\ + This is a hard failure on purpose. Falling back to scanning the whole file would \ + fire on legitimate test fixtures, and falling back to scanning nothing would leave \ + the module with no tests — the likeliest home for an unreviewed colour — entirely \ + uncovered. Add a `#[cfg(test)] mod tests` to {path}, or add {path} to an explicit \ + exemption with a reason." + ) + }); + production +} + +// ── The tests ───────────────────────────────────────────────────────────────────────────────── + +/// **FR-1.4: no production module except `src/theme.rs` names a `Color`.** +/// +/// Steps 1-4 of design A-2. Two needles rather than one: `rustfmt` cannot emit `Color :: Red`, but +/// a hand-edit between two `cargo fmt` runs can, and the second needle costs one line. +#[test] +fn no_production_module_outside_the_theme_names_a_colour() { + let mut offenders = Vec::new(); + + for (path, source) in SOURCES { + // The owner is the point of the exercise, not an exception to it. + if *path == THEME_OWNER { + continue; + } + + let code = code_only(source); + let production = production_region(path, &code); + + for (number, line) in production.lines().enumerate() { + for needle in ["Color::", "Color ::"] { + if line.contains(needle) { + offenders.push(format!("{path}:{} — {}", number + 1, line.trim())); + } + } + } + } + + assert!( + offenders.is_empty(), + "these production lines name a colour outside `{THEME_OWNER}`:\n {}\n\ + \n\ + Colour belongs to the theme so that the palette is one reviewable file and `NO_COLOR` can \ + switch it. A literal at a call site cannot be switched, which makes it an accessibility \ + regression and not merely an untidy one. Add or reuse a semantic role — \ + `theme.focus`, `theme.required`, `theme.ok`, `theme.error`, `theme.warn`, `theme.dim` — \ + and write `theme.error` here instead (FR-1.4).", + offenders.join("\n ") + ); +} + +/// **FR-2.3 / FR-2.4: no file names `Rgb`, `Indexed`, `Black` or `White`.** +/// +/// Steps 5-8 of design A-2, with the per-needle scope [`FORBIDDEN_VARIANTS`] explains. `theme.rs` +/// is **not** exempt from this check — unlike the ownership check, this one is about which colours +/// may exist rather than about where they may be written, and the owner is the module most able to +/// introduce a bad one. +/// +/// Comments are stripped throughout: this file, `theme.rs`, and the spec all discuss these +/// variants by name, and a guard that fires on its own documentation is a guard people delete. +#[test] +fn no_file_uses_a_non_ansi_or_inverting_colour() { + let mut offenders = Vec::new(); + + for (path, source) in SOURCES { + let code = code_only(source); + let production = production_region(path, &code); + + for (needle, scope, reason) in FORBIDDEN_VARIANTS { + let scanned = match scope { + Scope::WholeFile => code.as_str(), + Scope::ProductionOnly => production, + }; + + for (number, line) in scanned.lines().enumerate() { + if line.contains(needle) { + offenders.push(format!( + "{path}:{} uses {needle} — {reason}\n {}", + number + 1, + line.trim() + )); + } + } + } + } + + assert!( + offenders.is_empty(), + "forbidden colour variants found:\n {}", + offenders.join("\n ") + ); +} + +/// The `ProductionOnly` scope is **load-bearing**, not a convenience. +/// +/// An exemption nobody checks is an exemption that outlives its reason. This pins both halves: +/// `Black` and `White` genuinely do appear in `theme.rs`'s test region — so narrowing their scope +/// was necessary and is not dead configuration — and they genuinely do **not** appear in any +/// production region, so the narrowing costs no coverage where it matters. +/// +/// If a refactor removes those test mentions, this test reddens and the scope should be widened +/// back to `WholeFile` deliberately. That is the opposite of how a stale exemption normally dies: +/// silently, still granted, protecting nothing. (#556) +#[test] +fn the_production_only_scope_is_load_bearing() { + let narrowed: Vec<&str> = FORBIDDEN_VARIANTS + .iter() + .filter(|(_, scope, _)| *scope == Scope::ProductionOnly) + .map(|(needle, _, _)| *needle) + .collect(); + + assert_eq!( + narrowed.len(), + 2, + "expected exactly 2 production-only needles (Black, White). Adding a third is a reviewable \ + widening of what tests may name, not a local decision. Found: {narrowed:?}" + ); + + let (_, theme_source) = SOURCES + .iter() + .find(|(path, _)| *path == THEME_OWNER) + .unwrap_or_else(|| panic!("{THEME_OWNER} must be listed in SOURCES")); + let code = code_only(theme_source); + let production = production_region(THEME_OWNER, &code); + let tests = &code[production.len()..]; + + for needle in &narrowed { + assert!( + tests.contains(needle), + "{needle} is scoped ProductionOnly but no longer appears in {THEME_OWNER}'s test \ + region, so the narrowing protects nothing. Widen it back to Scope::WholeFile" + ); + assert!( + !production.contains(needle), + "{needle} appears in {THEME_OWNER}'s PRODUCTION region. The narrowed scope exists so \ + tests may NAME these variants in order to forbid them — not so production may use one" + ); + } +} + +/// **The anti-vacuity floor. Read the module docs before touching this.** +/// +/// Every other assertion in this file passes on a crate containing no colour at all, so without +/// this one the guard is green before the feature exists and stays green if the feature is +/// deleted. This is the assertion that makes the file mean something. +/// +/// # It counts DISTINCT variants, and the first version did not +/// +/// This originally counted `Color::` *occurrences*. A T-3 mutation — rewriting every colour in +/// `theme.rs` to `Color::Reset` — left **12** occurrences in the production region, cleared a floor +/// of 6, and this test stayed **green** through the exact deletion the docstring above claims it +/// catches: `colour()` reduced to `monochrome()`. Seven tests elsewhere caught it, so the property +/// was never unguarded, but the file's own stated floor was not holding it. +/// +/// Counting distinct variants fixes that: six roles collapsed onto one hue is one variant, not six. +/// A monochrome `colour()` therefore reads as `1 < 6` and reddens here, where a reader looking for +/// the floor will find it. +#[test] +fn the_theme_module_actually_defines_a_palette() { + let (_, source) = SOURCES + .iter() + .find(|(path, _)| *path == THEME_OWNER) + .unwrap_or_else(|| panic!("{THEME_OWNER} must be listed in SOURCES")); + + let code = code_only(source); + let production = production_region(THEME_OWNER, &code); + + // The variant name is everything up to the first non-identifier character after `Color::`, so + // `Color::Cyan)` and `Color::Cyan,` are one variant and not two spellings of it. + let mut variants: Vec<&str> = production + .match_indices("Color::") + .map(|(at, needle)| { + let rest = &production[at + needle.len()..]; + let end = rest + .find(|c: char| !c.is_alphanumeric() && c != '_') + .unwrap_or(rest.len()); + &rest[..end] + }) + .collect(); + variants.sort_unstable(); + variants.dedup(); + let colours = variants.len(); + + assert!( + colours >= MINIMUM_THEME_COLOURS, + "{THEME_OWNER}'s production region names {colours} DISTINCT colours ({variants:?}); at \ + least {MINIMUM_THEME_COLOURS} are required, one per semantic role.\n\ + \n\ + This is the anti-vacuity floor. Every other assertion in this file is an ABSENCE check, \ + and all of them pass on a crate with no colour anywhere — so if the palette were emptied, \ + or `colour()` were reduced to `monochrome()`, this is the assertion that notices. It \ + counts DISTINCT variants for that reason: six roles rewritten to a single hue leaves \ + plenty of `Color::` occurrences and would clear an occurrence-based floor. If you are \ + here because a refactor legitimately shrank the palette, lower the floor deliberately and \ + say why; do not delete the check." + ); +} + +/// **Every `mod` the crate root declares is in [`SOURCES`].** +/// +/// A count assertion cannot do this job. It reddens when a file is **added** to the list and stays +/// green when one is **omitted**, and it is the omitted file that goes unscanned. That is not a +/// hypothetical: `hermeticity_tripwire.rs` shipped with a count and no cross-check, and when +/// `mod theme;` was added its coverage test stayed green while `src/theme.rs` sat outside its scan +/// entirely. This guard is born with the cross-check rather than acquiring one later. (#556) +#[test] +fn the_scan_set_covers_every_module_the_crate_root_declares() { + let (_, crate_root) = SOURCES + .iter() + .find(|(path, _)| *path == "src/main.rs") + .expect("src/main.rs must be listed in SOURCES for the cross-check to run"); + + let mut declared = 0; + for line in crate_root.lines() { + let trimmed = line.trim(); + // `pub mod` / `pub(crate) mod` are matched too, so a visibility change does not quietly + // stop this loop from seeing a module. + let Some(rest) = trimmed + .strip_prefix("mod ") + .or_else(|| trimmed.strip_prefix("pub mod ")) + .or_else(|| trimmed.strip_prefix("pub(crate) mod ")) + else { + continue; + }; + let Some(module) = rest.strip_suffix(';') else { + continue; + }; + + declared += 1; + let expected = format!("src/{module}.rs"); + assert!( + SOURCES.iter().any(|(path, _)| *path == expected), + "`src/main.rs` declares `mod {module};` but {expected} is not in SOURCES, so this \ + guard does not scan it — and an unscanned module is exactly where an inline \ + `Color::` would survive review" + ); + } + + assert_eq!( + declared, 10, + "expected 10 `mod` declarations in src/main.rs. If this is 0 the loop above matched \ + nothing and its assertion never ran" + ); + + // A duplicated entry would let a real file go unlisted while every count still agreed. + let mut seen = std::collections::BTreeSet::new(); + for (path, source) in SOURCES { + assert!(seen.insert(*path), "{path} is listed twice in SOURCES"); + assert!( + !source.is_empty(), + "{path} embedded as empty — an empty source scans clean and proves nothing" + ); + } +} + +/// The scan finds a colour it is supposed to find, and ignores one it is supposed to ignore. +/// +/// Without this, every assertion above could be passing because the needles never match anything. +/// Both directions, because each plausible stripper bug breaks exactly one of them. +#[test] +fn the_scan_detects_a_planted_colour_and_ignores_one_in_a_comment() { + // Assembled so this test file does not itself contain the needle it plants — otherwise this + // file could never be added to SOURCES, and the guard that scans every source but itself is + // the guard with a hole in the obvious place. + let needle = format!("Colo{}", "r::Cyan"); + + let planted = format!("let s = Style::new().fg({needle});"); + assert!( + code_only(&planted).contains("Color::"), + "the scan cannot see a colour on a plain code line, so every absence assertion above is \ + vacuous" + ); + + let commented = format!("// prefer theme.focus over {needle} at a call site"); + assert!( + !code_only(&commented).contains("Color::"), + "the scan fired on a COMMENT. Prose naming a colour must be safe, or this feature's own \ + documentation — including this file — becomes unwritable" + ); + + let spaced = format!("let s = Style::new().fg(Colo{});", "r ::Cyan"); + assert!( + code_only(&spaced).contains("Color ::"), + "the hand-edited spacing variant is not detectable, so the second needle is decorative" + ); +} + +/// [`strip_line_comment`] keeps code that follows a literal containing `//`, and still cuts real +/// trailing comments. +/// +/// Both directions, because the two plausible implementations each fail one and a test asserting +/// only one would license the other bug. The URL scheme is assembled from pieces rather than +/// written out: `hermeticity_tripwire.rs` treats a plaintext `http:` in code as its catch-all for +/// an unnamed HTTP client, and it is right to — a guard that exempted "but this one is only a +/// fixture" would exempt the real thing too. +#[test] +fn the_stripper_survives_a_literal_containing_a_comment_marker() { + let scheme = format!("htt{}{}", "p:", "//"); + let url_line = format!(r#" let u = format!("{scheme}{{host}}"); let y = 1;"#); + assert_eq!( + strip_line_comment(&url_line), + url_line, + "the `//` inside a literal is not a comment; cutting there hides the code after it, which \ + is a real defect this crate already shipped once" + ); + + let needle = format!("Colo{}", "r::Red"); + let trailing = format!(" let x = 1; // never write {needle} here"); + assert_eq!( + strip_line_comment(&trailing), + " let x = 1; ", + "a trailing comment must still be cut, or this guard fires on prose about itself" + ); + + let escaped = r#" let q = "a\"//b"; let z = 2;"#; + assert_eq!( + strip_line_comment(escaped), + escaped, + "an escaped quote does not end the literal, so the `//` after it is still not a comment" + ); +} From 75e85a7f1b53b3825119adf703f8a605b64e0638 Mon Sep 17 00:00:00 2001 From: Stan Fan Date: Fri, 7 Aug 2026 22:31:29 +1000 Subject: [PATCH 2/6] fix(tui): scroll the form column and mark focus in every picker state Addresses two P2 review findings on #564. Expanding all sections overflowed the non-scrolling left column, so revealed options stayed clipped at 80x24; it now scrolls to the focused region and says what is off-screen. Loading, empty and failed pickers stayed in the Tab ring without a focus marker, making Tab look dead. Co-Authored-By: Claude Opus 5 --- tui/Cargo.toml | 22 +- tui/src/renderer.rs | 502 ++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 505 insertions(+), 19 deletions(-) diff --git a/tui/Cargo.toml b/tui/Cargo.toml index 17ae66ac9..55c2d956c 100644 --- a/tui/Cargo.toml +++ b/tui/Cargo.toml @@ -92,7 +92,27 @@ minreq = "3" # these are `[dependencies]`, not `[dev-dependencies]`, because the pane is production code. # That is unavoidable for a TUI and is the reason the feature set is narrowed rather than # taken as it comes. (#321) -ratatui = { version = "0.30.2", default-features = false, features = ["std", "crossterm"] } +# +# `unstable-rendered-line-info` is added for `Paragraph::line_count`, which the left column's +# viewport needs to know how tall its content is ONCE WRAPPED. It adds **zero crates** +# (`Cargo.lock` is byte-identical with and without it — verified) and compiles on stable rustc: +# ratatui gates the API behind an attribute, not behind a nightly feature. +# +# The alternative was to compute wrapped height here, and that is the worse risk: it would be a +# second implementation of word wrapping that agrees with the one actually rendering only until a +# line holds a long word or a wide grapheme. Measured at the 80x24 floor, 32 lines occupy 44 rows, +# so the naive `Vec::len()` count understates height by a third and an overflow guard built on it +# passes while content runs off screen. +# +# The exposure is that the API is unstable upstream (ratatui#293) and could change or vanish in a +# minor release. `wrapped_heights_agrees_with_what_is_actually_drawn` pins the behaviour against a +# real rendered buffer rather than against the API's own claim, so an upstream change surfaces as a +# red test rather than as a silently mis-sized viewport. (PR #564 review) +ratatui = { version = "0.30.2", default-features = false, features = [ + "std", + "crossterm", + "unstable-rendered-line-info", +] } crossterm = "0.29.0" # --------------------------------------------------------------------------------------- diff --git a/tui/src/renderer.rs b/tui/src/renderer.rs index 94a8cd2e1..a6f838aac 100644 --- a/tui/src/renderer.rs +++ b/tui/src/renderer.rs @@ -1069,8 +1069,19 @@ impl<'a, S: ServerApi, H: Host> Renderer<'a, S, H> { /// Filtering it would hide the diagnosis, which is what /// `inception/practices-discovery/discovered-rules.md:29` would have had us do; FR-1.5 /// supersedes that rule per the operator's later decision. + /// + /// # Every state carries the focus marker, not just `Loaded` + /// + /// `Loading`, empty and `Failed` render one line each, and that line goes through + /// [`Self::focus_marked`] for the same reason the fold header does: these regions stay in the + /// `Tab` ring in every state, so without a marker `Tab` moves the keyboard somewhere the screen + /// does not acknowledge and reads as a dead keypress. Reported as a P2 on PR #564. The states + /// still differ in *what* they say — an empty list is a valid answer and a failure is not + /// (FR-6.1) — they no longer differ in whether focus is visible (NFR-3 item 7). fn picker_lines(&self) -> Vec { let mut lines = Vec::new(); + let agents_focused = self.focus == Focus::AgentPicker; + let providers_focused = self.focus == Focus::ProviderPicker; // ONE LINE PER CHOICE, not a comma-joined paragraph. // @@ -1078,9 +1089,14 @@ impl<'a, S: ServerApi, H: Host> Renderer<'a, S, H> { // pick a name out of it. The count stays on its own header line so it is still visible at a // glance, and each choice is indented beneath it. (#321) match self.flow.agent_choices() { - PickerState::Loading => lines.push("agents: loading…".to_string()), + PickerState::Loading => { + lines.push(focus_marked("agents: loading…", agents_focused)); + } PickerState::Loaded(profiles) if profiles.is_empty() => { - lines.push("agents: none found on this machine".to_string()); + lines.push(focus_marked( + "agents: none found on this machine", + agents_focused, + )); } PickerState::Loaded(profiles) => { // The unselectable marker travels WITH its row (FR-1.5): an unloadable profile @@ -1103,7 +1119,7 @@ impl<'a, S: ServerApi, H: Host> Renderer<'a, S, H> { "unloadable", self.agents_expanded, self.agent_scroll, - self.focus == Focus::AgentPicker, + agents_focused, )); } PickerState::Failed(error) => { @@ -1113,16 +1129,19 @@ impl<'a, S: ServerApi, H: Host> Renderer<'a, S, H> { // // NEVER FOLDED, in any state: a failure the operator has to act on must not need a // keystroke to become visible, and this line already fits in one row. - lines.push(format!( - "agents: unavailable — {error}. Press [ctrl+r] to retry" + lines.push(focus_marked( + &format!("agents: unavailable — {error}. Press [ctrl+r] to retry"), + agents_focused, )); } } match self.flow.provider_choices() { - PickerState::Loading => lines.push("providers: loading…".to_string()), + PickerState::Loading => { + lines.push(focus_marked("providers: loading…", providers_focused)); + } PickerState::Loaded(providers) if providers.is_empty() => { - lines.push("providers: none reported".to_string()); + lines.push(focus_marked("providers: none reported", providers_focused)); } PickerState::Loaded(providers) => { // `installed` is DISPLAY information, never a filter (FR-1.7): the endpoint @@ -1146,11 +1165,12 @@ impl<'a, S: ServerApi, H: Host> Renderer<'a, S, H> { "not installed", self.providers_expanded, self.provider_scroll, - self.focus == Focus::ProviderPicker, + providers_focused, )); } - PickerState::Failed(error) => lines.push(format!( - "providers: unavailable — {error}. Press [ctrl+r] to retry" + PickerState::Failed(error) => lines.push(focus_marked( + &format!("providers: unavailable — {error}. Press [ctrl+r] to retry"), + providers_focused, )), } @@ -1185,7 +1205,6 @@ impl<'a, S: ServerApi, H: Host> Renderer<'a, S, H> { scroll: usize, focused: bool, ) -> Vec { - let marker = if focused { ">" } else { " " }; let glyph = if expanded { "▾" } else { "▸" }; let diagnosis = if unavailable > 0 { format!(", {unavailable} {unavailable_word}") @@ -1193,9 +1212,12 @@ impl<'a, S: ServerApi, H: Host> Renderer<'a, S, H> { String::new() }; let action = if expanded { "collapse" } else { "expand" }; - let mut lines = vec![format!( - "{marker} {glyph} {label} ({count}{diagnosis}) — [enter] {action}", - count = rows.len() + let mut lines = vec![focus_marked( + &format!( + "{glyph} {label} ({count}{diagnosis}) — [enter] {action}", + count = rows.len() + ), + focused, )]; if !expanded { @@ -2419,12 +2441,26 @@ impl<'a, S: ServerApi, H: Host> Renderer<'a, S, H> { left.extend(frame.pickers.iter().cloned()); left.extend(frame.banner.iter().cloned()); + // Where the viewport anchors when the column is taller than its area. The banner is the + // fallback because it is the only region carrying an outcome the operator must read; with + // focus in the results pane no left-hand line is marked, and pinning to the top would hide + // a failure banner behind content the operator is not looking at. + let banner_start = frame.command_list.len() + + frame.required_fields.len() + + frame.optional_section.len() + + frame.pickers.len(); + let anchor = focused_line_index(&left).unwrap_or(if frame.banner.is_empty() { + 0 + } else { + banner_start + }); + match frame.layout { LayoutMode::TwoColumn => { let [form_area, results_area] = Layout::horizontal([Constraint::Percentage(60), Constraint::Percentage(40)]) .areas(main); - paragraph(&left).render(form_area, buf); + self.render_form(&left, anchor, form_area, buf); // The pane renders ITSELF (it is a `Widget`), so its buffer, scroll position and // state wording come from the pane rather than from a copy here. self.render_results(results_area, buf); @@ -2440,12 +2476,79 @@ impl<'a, S: ServerApi, H: Host> Renderer<'a, S, H> { ), ]) .areas(main); - paragraph(&left).render(form_area, buf); + self.render_form(&left, anchor, form_area, buf); self.render_results(results_area, buf); } } } + /// Renders the left column, **scrolled so the focused region is on screen**. + /// + /// # Why this region scrolls at all + /// + /// It is one non-scrolling `Paragraph`, so every row past the area's last one was silently + /// discarded. Folding the pickers and windowing the command list reduced the height but could + /// not bound it: the first `[enter]` expands the optional section *and* both pickers, and at the + /// 80x24 floor that content does not fit however it is sliced — measured at **12 rows over** with + /// 9 optional parameters, 25 agents and 9 providers, with the provider fold never drawn at all. + /// So `[enter]` promised to reveal options that stayed clipped (reported as a P2 on PR #564). + /// + /// The remaining options were to cap what `[enter]` reveals, or to let the column scroll. A cap + /// re-creates the original complaint one level down — content the operator was told is there and + /// cannot reach — so the column scrolls, anchored on focus, and says what is off-screen. + /// + /// # Why the anchor sits at the TOP of the viewport + /// + /// The anchored line is a *heading* — a fold header, or the marked form row — and what the + /// operator just asked to see is what follows it. Centring would spend half the viewport on rows + /// above the thing they opened, and bottom-anchoring would put an expanded fold's rows entirely + /// off-screen: `Tab` to agents, `[enter]`, and the 25 rows would render below the fold line at + /// the last row. The clamp keeps the final screen full rather than half-empty. + fn render_form(&self, lines: &[Line<'static>], anchor: usize, area: Rect, buf: &mut Buffer) { + if area.is_empty() { + return; + } + + let heights = wrapped_heights(lines, area.width); + let total: usize = heights.iter().sum(); + if total <= area.height as usize { + paragraph(lines).render(area, buf); + return; + } + + // One row is spent on the residue notice, so a scrolled column can never be mistaken for a + // complete one. It costs a row of content and buys the operator the knowledge that `[tab]` + // reaches more — the same trade the pickers' own residue line makes. + let [body, notice] = + Layout::vertical([Constraint::Min(1), Constraint::Length(1)]).areas(area); + let viewport = body.height as usize; + let above: usize = heights[..anchor.min(heights.len())].iter().sum(); + let scroll = above.min(total.saturating_sub(viewport)); + let below = total.saturating_sub(scroll + viewport); + + paragraph(lines) + .scroll((scroll as u16, 0)) + .render(body, buf); + + let mut parts = Vec::new(); + if scroll > 0 { + parts.push(format!("{scroll} above")); + } + if below > 0 { + parts.push(format!("{below} below")); + } + // `[tab]` and not `[↑↓]`: the arrows belong to whichever region holds focus, and this + // viewport follows focus rather than being steered directly. Promising `[↑↓]` here would + // name a key that scrolls the *fold*, not the column, which is the "documented key does + // nothing" defect the module already had once. + let text = if parts.is_empty() { + " … form scrolled".to_string() + } else { + format!(" … {} — [tab] moves focus", parts.join(", ")) + }; + paragraph(&[Line::styled(text, self.theme.dim)]).render(notice, buf); + } + /// Renders the pane, or its state word when the area is too small for the pane itself. /// /// The fallback matters for NFR-6: `ResultsPane::render` returns early on an empty area, so a @@ -2539,6 +2642,72 @@ const NOT_INSTALLED_MARKER: &str = "(not installed)"; /// marker always eventually reaches. const SCROLL_RESIDUE_MARKER: &str = "[↑↓] scroll"; +/// `content` prefixed with the focus marker, or an equal-width blank when unfocused. +/// +/// The **one** place the `"> "` / `" "` convention is written for the picker region, because both +/// `style_pickers` and `style_focus_marker` decide on `line.starts_with('>')` and every producer has +/// to agree with them. A picker state that formatted its own line — as `Loading`, empty and `Failed` +/// each did — silently opted out of the marker *and* of the `focus` colour, so `Tab` into it changed +/// nothing on screen (reported as a P2 on PR #564). +/// +/// The unfocused branch pads to the same two columns rather than returning `content` unchanged, so +/// the rows do not shift horizontally as focus moves — a jump that reads as the text changing. +fn focus_marked(content: &str, focused: bool) -> String { + let marker = if focused { ">" } else { " " }; + format!("{marker} {content}") +} + +/// The index of the focus-marked line, if any — the anchor [`Renderer::render_form`] scrolls to. +/// +/// Reads the **rendered lines** rather than asking `self.focus` which region is active, and that is +/// the point: the marker is what the operator can see, so anchoring on it cannot disagree with the +/// screen. A focus state whose region forgot to mark its line would scroll to the wrong place — and +/// that bug existed (three picker states rendered no marker at all), so this reads the one signal +/// that is also the operator's. +fn focused_line_index(lines: &[Line<'_>]) -> Option { + lines + .iter() + .position(|line| line_text(line).starts_with('>')) +} + +/// One [`Line`]'s plain text, spans concatenated. +fn line_text(line: &Line<'_>) -> String { + line.spans + .iter() + .map(|span| span.content.as_ref()) + .collect() +} + +/// Each line's height in screen rows once wrapped to `width`. +/// +/// # Why this measures rather than counts +/// +/// A line is not a row. At 80 columns the left column is 47 wide and most command summaries wrap to +/// two rows, so a budget in `Vec::len()` units understates the real height by roughly a third — +/// which would make an overflow guard pass while the content still ran off the screen. Measured at +/// the 80x24 floor: 32 lines occupied 44 rows. +/// +/// The measurement is [`Paragraph::line_count`], i.e. **ratatui's own wrapping code**, given the +/// same `Wrap { trim: false }` this crate renders with. Re-deriving the rule here (divide by width, +/// round up) would be a second implementation of word wrapping that agrees with the first only +/// until a line contains a long word or a wide grapheme. +/// +/// A zero width yields one row per line rather than zero: `line_count` returns 0 there, and a +/// zero total would make the caller believe everything fits. +fn wrapped_heights(lines: &[Line<'static>], width: u16) -> Vec { + lines + .iter() + .map(|line| { + if width == 0 { + return 1; + } + paragraph(std::slice::from_ref(line)) + .line_count(width) + .max(1) + }) + .collect() +} + /// A line split into three spans, the middle one styled: `before`, `needle`, `after`. /// /// The concatenation is **exactly** the input, so [`Frame::plain_lines`] is unchanged by styling — @@ -3004,8 +3173,8 @@ type Step3FailurePayload = TerminalStatus; #[cfg(test)] mod tests { use super::{ - in_app_readiness, Fatal, Focus, Frame, InAppReadiness, JsonSink, LayoutMode, PaneSink, - Renderer, Retryable, ServerApi, MIN_COLS, MIN_ROWS, + in_app_readiness, paragraph, wrapped_heights, Fatal, Focus, Frame, InAppReadiness, + JsonSink, LayoutMode, PaneSink, Renderer, Retryable, ServerApi, MIN_COLS, MIN_ROWS, }; use crate::catalog::{self, CommandId, Policy}; use crate::error::TuiError; @@ -7432,4 +7601,301 @@ mod tests { says nothing about where the keyboard is" ); } + + // ── PR #564 review: the two P2 layout defects ──────────────────────────────────────────── + + /// The whole screen as text, one line per row, trailing blanks trimmed. + /// + /// Every assertion about clipping has to read the DRAWN BUFFER: a region that renders into the + /// frame correctly and is then cut off by the layout is present in `Frame` and absent from the + /// operator's screen, which is the entire defect class here. + fn screen(shell: &Renderer<'_, S, H>, cols: u16, rows: u16) -> String { + drawn_cells(shell, cols, rows) + .chunks(cols as usize) + .map(|row| { + let text: String = row.iter().map(|(symbol, _)| symbol.as_str()).collect(); + format!("{}\n", text.trim_end()) + }) + .collect() + } + + /// **What the first `[enter]` reveals is REACHABLE at the 80x24 floor.** + /// + /// The reported P2: `[enter]` expands the optional section and both pickers, and at the minimum + /// supported size that content does not fit — measured at **12 rows over** with 9 optional + /// parameters, 25 agents and 9 providers, with the provider fold never drawn at all. So the + /// keypress promised options that stayed clipped. + /// + /// "Reachable" and not "all visible simultaneously", because at 80x24 the latter is impossible + /// and a test asserting it would be a test of arithmetic rather than of the product. The + /// property is that `[tab]` brings each region into view — which is what the residue notice + /// tells the operator to do. + #[test] + fn every_revealed_region_is_reachable_at_the_minimum_supported_size() { + let server = server_with_many_agents(25); + let host = FakeHost::outside_tmux(); + let mut shell = shell_on_the_launch_form(&server, &host, MIN_COLS, MIN_ROWS); + + // The reveal press, exactly as the operator makes it. + assert!( + shell.on_key(KeyCode::Enter), + "the first [enter] must be consumed as the reveal, or this test is exercising a run" + ); + + // ANTI-VACUITY: the content must genuinely overflow, or a non-scrolling column would pass + // this test and the guard would prove nothing. + let area = ratatui::layout::Rect::new(0, 0, MIN_COLS, MIN_ROWS); + let mut probe = ratatui::buffer::Buffer::empty(area); + shell.draw(area, &mut probe); + let frame = shell.render(); + let mut left: Vec> = Vec::new(); + for region in [ + &frame.command_list, + &frame.required_fields, + &frame.optional_section, + &frame.pickers, + ] { + left.extend(region.iter().cloned()); + } + let content: usize = wrapped_heights(&left, MIN_COLS * 6 / 10).iter().sum(); + assert!( + content > MIN_ROWS as usize, + "this test needs content TALLER than the screen ({content} rows at {MIN_ROWS}) or a \ + non-scrolling column would satisfy it and the regression would return unnoticed" + ); + + for region in [ + Focus::OptionalSection, + Focus::AgentPicker, + Focus::ProviderPicker, + ] { + while shell.focus() != region { + assert!(shell.on_key(KeyCode::Tab)); + } + let drawn = screen(&shell, MIN_COLS, MIN_ROWS); + let marked = drawn.lines().any(|line| line.starts_with('>')); + assert!( + marked, + "with focus on {region:?} at {MIN_COLS}x{MIN_ROWS} the focused row must be ON \ + SCREEN. A focused region scrolled out of the viewport is invisible focus, and \ + [tab] then appears to do nothing. Screen:\n{drawn}" + ); + } + } + + /// **A scrolled column says so, so a partial form never reads as the whole form.** + /// + /// Without this the operator sees a form that simply ends, with no cue that `[tab]` reaches + /// more — the same "a window reads as the complete list" failure the pickers' own residue line + /// exists to prevent, one region up. + /// + /// The negative half is what stops the notice from becoming permanent furniture: on a terminal + /// tall enough for everything there is nothing off-screen, and a notice claiming otherwise + /// would be a standing lie about the layout. + #[test] + fn a_scrolled_form_states_that_more_is_off_screen_and_a_short_one_does_not() { + let server = server_with_many_agents(25); + let host = FakeHost::outside_tmux(); + + let mut cramped = shell_on_the_launch_form(&server, &host, MIN_COLS, MIN_ROWS); + assert!(cramped.on_key(KeyCode::Enter)); + let drawn = screen(&cramped, MIN_COLS, MIN_ROWS); + assert!( + drawn.contains("below") && drawn.contains("[tab] moves focus"), + "a clipped column must state what is off-screen AND the key that reaches it. \ + Screen:\n{drawn}" + ); + + // Tall enough that the whole expanded form fits, so the notice must be absent. + let roomy_rows = 90; + let mut roomy = shell_on_the_launch_form(&server, &host, 200, roomy_rows); + assert!(roomy.on_key(KeyCode::Enter)); + let drawn = screen(&roomy, 200, roomy_rows); + assert!( + drawn.contains("providers ("), + "control: at 200x{roomy_rows} the whole form should fit, so the provider fold must be \ + drawn — if it is not, the negative assertion below proves nothing. Screen:\n{drawn}" + ); + assert!( + !drawn.contains("[tab] moves focus"), + "nothing is off-screen at 200x{roomy_rows}, so the residue notice must be ABSENT — a \ + notice that is always there tells the operator nothing. Screen:\n{drawn}" + ); + } + + /// **The wrap measurement agrees with what is actually drawn.** + /// + /// `wrapped_heights` is built on `Paragraph::line_count`, which is UNSTABLE upstream + /// (ratatui#293). This pins it against a real rendered buffer rather than against its own + /// claim, so an upstream change to wrapping surfaces as a red test here instead of as a + /// viewport that is quietly the wrong size. + /// + /// A line that wraps is required, not incidental: the count-vs-measure distinction is the whole + /// reason this function exists, and at the 80x24 floor a naive `Vec::len()` understates the + /// column's height by about a third. + #[test] + fn the_wrap_measurement_agrees_with_what_is_actually_drawn() { + let width = 24u16; + let lines: Vec> = vec![ + Line::raw("short"), + Line::raw("a line comfortably longer than twenty-four columns, so it wraps"), + ]; + + let heights = wrapped_heights(&lines, width); + assert!( + heights[1] > 1, + "the fixture must actually WRAP or this test cannot tell measuring from counting \ + (got {heights:?})" + ); + + // Render tall enough that nothing is clipped, then count the rows that received ink. + let total: usize = heights.iter().sum(); + let area = ratatui::layout::Rect::new(0, 0, width, total as u16 + 4); + let mut buffer = ratatui::buffer::Buffer::empty(area); + ratatui::widgets::Widget::render(paragraph(&lines), area, &mut buffer); + let inked = (0..area.height) + .filter(|row| (0..width).any(|column| buffer[(column, *row)].symbol().trim() != "")) + .count(); + assert_eq!( + total, inked, + "the measured height must equal the number of rows ratatui actually paints, or the \ + viewport is sized against a fiction" + ); + } + + /// One picker-state fixture: the agent answer to install, or `None` to hold it in `Loading`. + /// + /// `Loading` has no answer *by definition*, which is why it is `Option` rather than a third + /// `Result` variant — the absence is the state. + type AgentFixture = Option, String>>; + + /// A picker source whose fetches never return, so both pickers stay in [`PickerState::Loading`]. + /// + /// The blocking happens on the fetch threads `populate_pickers` spawns, so the test thread is + /// unaffected; the threads are left parked and the process ends with them. That is acceptable + /// in a unit test and is the only way to observe `Loading` — every other fake answers before + /// the first `render`. + struct NeverAnswers; + + impl crate::guided_flow::PickerSource for NeverAnswers { + fn profiles(&self) -> Result, TuiError> { + std::thread::sleep(std::time::Duration::from_secs(3600)); + unreachable!("the test finishes long before this returns") + } + + fn providers(&self) -> Result, TuiError> { + std::thread::sleep(std::time::Duration::from_secs(3600)); + unreachable!("the test finishes long before this returns") + } + } + + /// **Every picker state carries the focus marker — `Tab` is never a dead keypress.** + /// + /// The reported P2: `Loading`, empty and `Failed` formatted their own line and so opted out of + /// both the `>` marker and the `focus` colour, while remaining in the `Tab` ring. Tabbing into + /// them moved the keyboard somewhere the screen did not acknowledge. + /// + /// Every state is covered in one loop rather than one test for the interesting case: the defect + /// was three separate producers each formatting its own line, so a test naming one of them + /// would leave the other two free to regress. + #[test] + fn every_picker_state_shows_where_the_keyboard_is() { + // `None` means "hold the picker in `Loading`" — installed as a source that never answers, + // which is the only way to observe that state from a test: the single-threaded fallback in + // `populate_pickers` resolves both fetches before `render` is ever called. Without this + // case a `Loading` regression survives, and it did: mutation M7 was green until it was + // added. + let states: [(&str, AgentFixture); 4] = [ + ("loading", None), + ("empty", Some(Ok(Vec::new()))), + ("failed", Some(Err("boom".to_string()))), + ("loaded", Some(Ok(vec![profile("solo", true)]))), + ]; + + for (label, profiles) in states { + let server = FakeServer::healthy(); + let host = FakeHost::outside_tmux(); + let mut shell = match profiles { + Some(profiles) => { + *server.profiles.borrow_mut() = profiles; + shell_on_the_launch_form(&server, &host, 120, 60) + } + None => { + let mut shell = Renderer::new(&server, &host, 120, 60) + .with_concurrent_pickers(std::sync::Arc::new(NeverAnswers)); + assert!(shell.focus_command(CommandId::Launch)); + assert!(shell.on_key(KeyCode::Enter)); + assert!( + matches!(shell.flow.agent_choices(), PickerState::Loading), + "the {label} fixture must actually hold the picker in Loading, or this \ + case silently tests the loaded path instead" + ); + shell + } + }; + + let unfocused = joined(&shell.render().pickers, "\n"); + let agent_line = unfocused + .lines() + .find(|line| line.contains("agents")) + .unwrap_or_else(|| { + panic!("the {label} state must render an agents line; got:\n{unfocused}") + }) + .to_string(); + assert!( + !agent_line.starts_with('>'), + "control: the {label} agents line must NOT be marked before focus reaches it, or \ + the positive assertion below cannot fail. Got: {agent_line:?}" + ); + + while shell.focus() != Focus::AgentPicker { + assert!(shell.on_key(KeyCode::Tab)); + } + + let focused = joined(&shell.render().pickers, "\n"); + let marked = focused + .lines() + .find(|line| line.starts_with('>')) + .unwrap_or_else(|| { + panic!( + "with focus on the agent picker in the {label} state, SOME line must carry \ + the focus marker — otherwise [tab] moved the keyboard somewhere the screen \ + does not show, and the region looks unreachable. Got:\n{focused}" + ) + }); + assert!( + marked.contains("agents"), + "and the marked line must be the AGENTS line, not another region's. Got: {marked:?}" + ); + } + } + + /// **A focused degenerate picker is styled `focus`, exactly like a loaded one.** + /// + /// The marker and the colour are two channels on the same fact (FR-4.4/FR-4.5), and + /// `style_pickers` decides on `starts_with('>')` — so a state that produced no marker also lost + /// the colour. This asserts the colour reaches the cells, because the marker alone is what + /// NFR-3 item 7 calls insufficient. + /// + /// The `Failed` state is the one that matters most: its line already carries `error`, and + /// `style_pickers` checks the failure marker FIRST, so the focused-failed line is the one case + /// where the two roles genuinely compete for the same cells. + #[test] + fn a_focused_empty_picker_is_coloured_like_a_focused_loaded_one() { + let server = FakeServer::healthy(); + *server.profiles.borrow_mut() = Ok(Vec::new()); + let host = FakeHost::outside_tmux(); + let mut shell = shell_on_the_launch_form(&server, &host, 120, 60); + while shell.focus() != Focus::AgentPicker { + assert!(shell.on_key(KeyCode::Tab)); + } + + let focus_fg = Theme::colour().focus.fg.expect("`focus` sets a foreground"); + assert_eq!( + drawn_foreground(&shell, 120, 60, "none found"), + focus_fg, + "a focused EMPTY picker must carry `focus` in the drawn cells, like every other \ + focused row — the marker alone is the insufficient-focus case of NFR-3 item 7" + ); + } } From 37b55c65297223abdd724f4600fb1acfd0a81465 Mon Sep 17 00:00:00 2001 From: Stan Fan Date: Mon, 10 Aug 2026 11:42:17 +1000 Subject: [PATCH 3/6] fix(tui): label the optional fold's real key and clamp the fold to the last window MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses both must-fixes from the review on PR #564. 1. `[enter]` on the EXPANDED optional header launched the command while the header it sat on advertised `[enter] expand/collapse`. With nothing left folded, `on_key_form` routes `Enter` to `reveal_options_or_run`, which falls through to `run_selected()` — so an operator folding the section back up created a session instead. Reproduced through the real key handler: `pending_action` went to `Some(Launch)`, `running` to `true`, and a tick later `create_session_calls` was 1. The expanded header now advertises `[esc] collapse`, which is the key `on_key_form` actually honours. #556 is what made the old label reachable as a deliberate keystroke: the new picker folds advertise `[enter] collapse` and honour it, and the new footer teaches `[enter]` as the reveal key, so the same advertised affordance meant "collapse" on two of three folds and "launch a session" on the third. The alternative — giving the section the picker folds' Enter-toggles semantics — costs the "second [enter] runs" contract whenever focus rests there, so this takes the honest label. 2. `fold_lines` clamped the scroll offset with `rows.len().saturating_sub(1)` while `on_key_fold`'s `Down` arm used `rows.saturating_sub(window)` and its docstring stated the latter. Any path changing `rows` or `window` WITHOUT a keypress landed in the gap, and there are two: - A taller terminal. Scrolled to the bottom of 25 agents at 24 rows then resized to 200, the window becomes 50 and the whole list fits, but the stale offset pinned the view to six rows in a pane with room for 25. Pressing `Down` recomputed `last` as 0 and snapped to the top, so the key the residue line advertises scrolled upwards. - A shorter refetch. `[ctrl+r]` did not reset the offsets, so an offset of 19 against a two-row answer drew one row and left the other off-window, with `Up` doing nothing visible for 18 presses. Fixed at both ends: the render clamps to the last window, and `retry()` resets the two offsets so the operator's position is not defined by a list that no longer exists. The fold flags are deliberately left alone. Two regression tests, both confirmed to fail before the change and verified individually load-bearing — reverting either half of fix 2 reddens its own assertion. `the_expanded_optional_header_advertises_the_key_that_collapses_it` asserts on `create_session_calls` rather than the label alone, so a build that relabelled the header and left the launch wired still fails. cargo fmt --check and cargo clippy --locked --all-targets -- -D warnings are clean; 193 binary unit tests plus every other target pass. Also swept 1x1 through 200x4 after a reveal, a Tab walk and five `Down` presses with a stale offset resized both taller and shorter: no panic. Co-Authored-By: Claude Opus 5 --- tui/src/renderer.rs | 236 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 231 insertions(+), 5 deletions(-) diff --git a/tui/src/renderer.rs b/tui/src/renderer.rs index a6f838aac..a5c387bf6 100644 --- a/tui/src/renderer.rs +++ b/tui/src/renderer.rs @@ -1035,6 +1035,24 @@ impl<'a, S: ServerApi, H: Host> Renderer<'a, S, H> { /// Hidden-by-default is not the same as absent. The header states the count and the key that /// expands it, so every one of the nine remaining parameters is reachable without leaving the /// form — which is what makes NFR-3's keyboard-only requirement hold for them. + /// + /// # The advertised key changes with the state, because the wired one does + /// + /// This header used to read `[enter] expand/collapse` in both states, and only the first half + /// was true. Once expanded, `Enter` here reaches [`Self::reveal_options_or_run`] with nothing + /// left folded, so it falls through to `run_selected()` — the operator reads "collapse" on the + /// row carrying their own focus marker, presses it to fold the section back up, and **creates a + /// session instead**. That is the failure `reveal_options_or_run`'s docstring describes, + /// "running the CLI by accident while trying to open the options", arriving one keystroke later + /// than before. #556 is what made it reachable as a *deliberate* keystroke: the new picker folds + /// advertise `[enter] collapse` and honour it, and the new footer teaches `[enter]` as the reveal + /// key, so the same advertised affordance meant "collapse" on two of three folds and "launch a + /// session" on the third. (Review on PR #564.) + /// + /// So the expanded header advertises **`[esc]`**, which is what `on_key_form` actually collapses + /// on. The alternative — giving this section the picker folds' `Enter`-toggles semantics — is + /// more consistent, but it costs the "second `[enter]` runs" contract whenever focus happens to + /// rest here, so the honest label is what this takes. fn optional_section_lines(&self) -> Vec { let hidden = self.field_lines(false); if hidden.is_empty() { @@ -1047,8 +1065,13 @@ impl<'a, S: ServerApi, H: Host> Renderer<'a, S, H> { " " }; let glyph = if self.optional_expanded { "▾" } else { "▸" }; + let action = if self.optional_expanded { + "[esc] collapse" + } else { + "[enter] expand" + }; let mut lines = vec![format!( - "{marker} {glyph} optional ({count}) — [enter] expand/collapse", + "{marker} {glyph} optional ({count}) — {action}", count = hidden.len() )]; @@ -1225,10 +1248,17 @@ impl<'a, S: ServerApi, H: Host> Renderer<'a, S, H> { } let window = self.picker_window(); - // `min` against a saturating end, so a scroll offset left over from a longer list cannot - // index past a shorter one — the pickers are re-fetched by `[ctrl+r]` and the new answer - // may be shorter than the old. - let start = scroll.min(rows.len().saturating_sub(1)); + // Clamped to the last WINDOW, not the last row — the same bound `on_key_fold`'s `Down` arm + // uses, and the one its docstring states. `saturating_sub(1)` was the bug: it let a stale + // offset survive any change to `rows` or `window` that arrives WITHOUT a keypress, and + // there are two such paths. A taller terminal grows `window` until the whole list fits, and + // the stale offset still pinned the view to the last few rows in a pane with room for all + // of them; pressing `Down` then recomputed `last` as 0 and snapped to the top, so the key + // the residue line advertises scrolled *upwards*. And `[ctrl+r]` may answer with a shorter + // list, where `min(len - 1)` pins `start` one row from the end and leaves the rest + // unreachable by `Up` — a row the operator cannot reach by the advertised means, which is + // the defect the fold exists to fix. (Review on PR #564.) + let start = scroll.min(rows.len().saturating_sub(window)); let end = (start + window).min(rows.len()); lines.extend(rows[start..end].iter().cloned()); @@ -1988,6 +2018,14 @@ impl<'a, S: ServerApi, H: Host> Renderer<'a, S, H> { match retryable { Retryable::Pickers => { + // The viewports reset with the answer they were scrolled into. The refetch may + // return a SHORTER list, and an offset into the old one names nothing in the new + // one — the render clamp keeps that survivable, but leaving the offset would mean + // the operator's position is defined by a list that no longer exists. The fold + // flags are deliberately NOT reset: the operator opened them, and re-collapsing on + // a retry they asked for would hide the answer they were waiting to see. + self.agent_scroll = 0; + self.provider_scroll = 0; self.populate_pickers(); true } @@ -3175,6 +3213,7 @@ mod tests { use super::{ in_app_readiness, paragraph, wrapped_heights, Fatal, Focus, Frame, InAppReadiness, JsonSink, LayoutMode, PaneSink, Renderer, Retryable, ServerApi, MIN_COLS, MIN_ROWS, + SCROLL_RESIDUE_MARKER, }; use crate::catalog::{self, CommandId, Policy}; use crate::error::TuiError; @@ -7898,4 +7937,191 @@ mod tests { focused row — the marker alone is the insufficient-focus case of NFR-3 item 7" ); } + + // ── PR #564 review: the two must-fixes ─────────────────────────────────────────────────── + + /// **The optional header advertises a key that does what the header says it does.** + /// + /// The reported must-fix: with the section already expanded and focused, the header read + /// `[enter] expand/collapse` while `on_key_form` routed `Enter` through + /// [`Renderer::reveal_options_or_run`] — nothing was folded at that point, so it fell straight + /// through to `run_selected()` and **created a session**. The operator reads "collapse" on the + /// row their own focus marker is on, presses it to fold the section back up, and launches. + /// + /// That is `reveal_options_or_run`'s own docstring's failure — "running the CLI by accident + /// while trying to open the options" — arriving one keystroke later than before. It got worse + /// with #556, not better: the new picker folds advertise `[enter] collapse` and *honour* it, so + /// the same advertised key meant "collapse" on two of three folds and "launch" on the third. + /// + /// Asserted on `create_session_calls`, not on the label alone: a test that only read the string + /// would pass against a build that relabelled the header and left the launch wired. + #[test] + fn the_expanded_optional_header_advertises_the_key_that_collapses_it() { + let server = FakeServer::healthy().with_session(SessionAnswer::Created(terminal("t"))); + let host = FakeHost::outside_tmux(); + let mut shell = shell_on_the_launch_form(&server, &host, 100, 40); + shell + .flow_mut() + .set("--agents", "planner") + .expect("`planner` is loadable in the fake's answer"); + + // The reveal press, made where the operator makes it: on the required field, before they + // have gone looking for the options. This is what leaves the section EXPANDED with nothing + // else folded, which is the state the launch fired from. + assert_eq!( + shell.focus(), + Focus::RequiredFields, + "selecting a command lands on the required fields" + ); + assert!( + shell.on_key(KeyCode::Enter), + "the first [enter] must be consumed as the reveal, not as a run" + ); + assert!( + shell.optional_expanded && !shell.has_folded_options(), + "the reveal must open EVERYTHING — with nothing left folded, the next [enter] means \ + run, which is what makes the header's label load-bearing" + ); + + while shell.focus() != Focus::OptionalSection { + assert!( + shell.on_key(KeyCode::Tab), + "[tab] must reach the optional section" + ); + } + + // What the operator is reading, on the row carrying their focus marker. + let header = joined(&shell.render().optional_section, "\n") + .lines() + .find(|line| line.contains("optional (")) + .expect("the optional header is always rendered (FR-2.3)") + .to_string(); + assert!( + !header.contains("[enter]"), + "the EXPANDED header must not advertise `[enter]`: that key runs the command from \ + here, so naming it invites the launch-by-accident this flow exists to prevent. \ + Got: {header:?}" + ); + assert!( + header.contains("[esc] collapse"), + "and it must name the key that IS wired to collapse — an unadvertised affordance is a \ + hidden one (NFR-3), and `Esc` is what `on_key_form` honours. Got: {header:?}" + ); + + // The advertised key does what it says, with no side effect on the server. + assert!( + shell.on_key(KeyCode::Esc), + "[esc] must be consumed by the expanded section" + ); + assert!( + !shell.optional_expanded, + "[esc] must actually COLLAPSE the section — an advertised key that does nothing is the \ + `[c] clear` defect again" + ); + assert_eq!( + server.create_session_calls.get(), + 0, + "collapsing the section must never reach `create_session`: the irreversible side \ + effect is the whole reason this is a must-fix and not a wording nit" + ); + assert!( + shell.pending_action.is_none() && !shell.running, + "and no launch may be queued for a later tick either — that would be the same defect \ + one frame further on" + ); + } + + /// **The render-side scroll clamp stops at the last WINDOW, not the last row.** + /// + /// `fold_lines` clamped with `rows.len().saturating_sub(1)` while `on_key_fold`'s `Down` arm + /// clamped with `rows.saturating_sub(window)` and its docstring stated the latter. Any path that + /// changes `rows` or `window` **without a keypress** landed in the gap, and there are two: + /// + /// 1. **A taller terminal.** Scrolled to the bottom of 25 agents at 24 rows, then resized to + /// 200: the window becomes 50 and the whole list fits, but the stale offset pinned the view + /// to the last six rows in a pane with room for all 25. Pressing `Down` — the key the residue + /// line advertises — then recomputed `last = 0` and snapped to the top, so `Down` scrolled + /// *up*. + /// 2. **A shorter refetch.** `[ctrl+r]` does not reset the offsets, so a stale offset of 19 + /// against a 2-row answer drew one row and left the other off-window. `Up` decremented the + /// offset while `min(len - 1)` stayed pinned, so the key advertised to reveal what is above + /// did nothing visible for 18 presses — a row unreachable by the advertised means, which is + /// the exact defect the fold was built to fix. + /// + /// Both are asserted here because the fix has two halves: the clamp closes them at the render, + /// and `retry()` resetting the offsets closes the second at its source. + #[test] + fn a_stale_scroll_offset_never_hides_rows_that_now_fit() { + let server = server_with_many_agents(25); + let host = FakeHost::outside_tmux(); + let mut shell = shell_on_the_launch_form(&server, &host, 100, 24); + + while shell.focus() != Focus::AgentPicker { + assert!( + shell.on_key(KeyCode::Tab), + "[tab] must reach the agent fold" + ); + } + assert!(shell.on_key(KeyCode::Enter), "[enter] must expand the fold"); + for _ in 0..30 { + shell.on_key(KeyCode::Down); + } + assert!( + shell.agent_scroll > 0, + "this test needs a NON-ZERO offset to make stale, or both halves below are vacuous" + ); + + // (1) A taller terminal: the window now holds the whole list. + shell.resize(100, 200); + let pickers = joined(&shell.render().pickers, "\n"); + assert!( + pickers.contains("agent-00") && pickers.contains("agent-24"), + "after a resize that fits all 25 rows, ALL of them must be drawn — a stale offset that \ + shows six in a pane with room for 25 is a list the operator cannot see. Got: \ + {pickers:?}" + ); + assert!( + !pickers.contains(SCROLL_RESIDUE_MARKER), + "and with nothing off-window there must be no residue notice: a standing claim that \ + rows are hidden when none are is the inverse lie. Got: {pickers:?}" + ); + + // And `Down` must not scroll UP. The key clamps to a `last` of 0 here, so the offset stays + // put rather than snapping the view somewhere the operator did not ask for. + let before = joined(&shell.render().pickers, "\n"); + shell.on_key(KeyCode::Down); + assert_eq!( + joined(&shell.render().pickers, "\n"), + before, + "[↓] on a list that entirely fits must be inert — it must never move the view, least \ + of all upwards, which is what a disagreeing render clamp produced" + ); + + // (2) A shorter refetch, at the original size: `[ctrl+r]` answers with two profiles. + let mut shell = shell_on_the_launch_form(&server, &host, 100, 24); + while shell.focus() != Focus::AgentPicker { + assert!(shell.on_key(KeyCode::Tab)); + } + assert!(shell.on_key(KeyCode::Enter)); + for _ in 0..30 { + shell.on_key(KeyCode::Down); + } + *server.profiles.borrow_mut() = + Ok(vec![profile("only-one", true), profile("only-two", true)]); + assert!(shell.retry(), "[ctrl+r] must re-issue the picker fetch"); + + assert_eq!( + shell.agent_scroll, 0, + "a refetch must reset the viewport at the SOURCE: the answer being scrolled is gone, \ + so an offset into it is meaningless and only happens to be survivable because the \ + render clamps" + ); + let refetched = joined(&shell.render().pickers, "\n"); + assert!( + refetched.contains("only-one") && refetched.contains("only-two"), + "both rows of the new answer must be on screen — the header saying 2 while one row is \ + drawn and unreachable by [↑] is the defect the fold was meant to fix. Got: \ + {refetched:?}" + ); + } } From 86f3e084742537a97d26ab1fb281c09c0c71767c Mon Sep 17 00:00:00 2001 From: Stan Fan Date: Mon, 10 Aug 2026 13:44:29 +1000 Subject: [PATCH 4/6] docs(tui): cite a test that exists for the unstable ratatui feature The comment justifying `unstable-rendered-line-info` named `wrapped_heights_agrees_with_what_is_actually_drawn` as the guard pinning the API's behaviour, and no such test existed. The real one is `the_wrap_measurement_agrees_with_what_is_actually_drawn` in `src/renderer.rs`, which does exactly what the comment claims. A reviewer is asked to accept an unstable upstream dependency on the strength of that citation, so it has to resolve. Co-Authored-By: Claude Opus 5 --- tui/Cargo.toml | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/tui/Cargo.toml b/tui/Cargo.toml index 55c2d956c..3efe6f00d 100644 --- a/tui/Cargo.toml +++ b/tui/Cargo.toml @@ -105,9 +105,12 @@ minreq = "3" # passes while content runs off screen. # # The exposure is that the API is unstable upstream (ratatui#293) and could change or vanish in a -# minor release. `wrapped_heights_agrees_with_what_is_actually_drawn` pins the behaviour against a -# real rendered buffer rather than against the API's own claim, so an upstream change surfaces as a -# red test rather than as a silently mis-sized viewport. (PR #564 review) +# minor release. `the_wrap_measurement_agrees_with_what_is_actually_drawn` (in `src/renderer.rs`) +# pins the behaviour against a real rendered buffer rather than against the API's own claim, so an +# upstream change surfaces as a red test rather than as a silently mis-sized viewport. The test name +# is cited here on purpose — a reviewer is being asked to accept an unstable upstream dependency on +# the strength of that guard, so the citation has to resolve; the name it carried before named no +# test at all. (PR #564 review) ratatui = { version = "0.30.2", default-features = false, features = [ "std", "crossterm", From fbdfc0d913205fe101f6178a1742b53f766b59f5 Mon Sep 17 00:00:00 2001 From: Stan Fan Date: Mon, 10 Aug 2026 13:47:39 +1000 Subject: [PATCH 5/6] fix(tui): prove the reachability guard, drop dead residue wording, reword the list residue MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three review follow-ups in the left column and its notices. `every_revealed_region_is_reachable_at_the_minimum_supported_size` asserted only that some line began with `>`. The column always holds three marker-capable headers, so the check passed once any of them was on screen — including when [tab] had moved focus to a region that scrolled away, the very invisible-focus defect the test is named for. It now matches the marker against the focused region's own label. `render_form`'s "form scrolled" fallback was dead code: emptiness needs scroll == 0 && below == 0, and the early return guarantees total > viewport, which forces scroll > 0 whenever below == 0. An assert in its place survived the whole suite, an exhaustive sweep of the function's arguments, and an integration sweep over every size, reveal state and focus stop. Replaced with a debug_assert so a change to the early return reddens a test instead. The command-list residue read `34 below of 42 commands`; the total now attaches to the noun once, giving `34 of 42 commands below`. Co-Authored-By: Claude Opus 5 --- tui/src/renderer.rs | 83 ++++++++++++++++++++++++++++++++++----------- 1 file changed, 63 insertions(+), 20 deletions(-) diff --git a/tui/src/renderer.rs b/tui/src/renderer.rs index a5c387bf6..57d0aebd9 100644 --- a/tui/src/renderer.rs +++ b/tui/src/renderer.rs @@ -943,20 +943,30 @@ impl<'a, S: ServerApi, H: Host> Renderer<'a, S, H> { lines.extend(rows[start..end].iter().cloned()); // The residue names what is off-screen in each direction, so a partial list never reads as // the whole catalog. + // + // The total is folded into the FIRST direction rather than trailing the whole join, because + // trailing it produced `34 below of 42 commands` — every number correct and the word order + // fighting the reader, who parses "below of" as a broken phrase and has to re-read to learn + // that 42 is the catalog size and not a second offset. Attaching the total to the noun once, + // where the noun first appears, makes each shape read as a sentence: `34 of 42 commands + // below`, and `18 of 42 commands above, 11 below`. let (above, below) = (start, rows.len() - end); if above > 0 || below > 0 { + let total = rows.len(); let mut parts = Vec::new(); if above > 0 { - parts.push(format!("{above} above")); + parts.push(format!("{above} of {total} commands above")); } if below > 0 { - parts.push(format!("{below} below")); + // Only the leading part carries the total; repeating `of 42 commands` on both would + // read as two different populations rather than two ends of one list. + parts.push(if above > 0 { + format!("{below} below") + } else { + format!("{below} of {total} commands below") + }); } - lines.push(format!( - " … {} of {} commands, {SCROLL_RESIDUE_MARKER}", - parts.join(", "), - rows.len() - )); + lines.push(format!(" … {}, {SCROLL_RESIDUE_MARKER}", parts.join(", "))); } lines } @@ -2575,15 +2585,32 @@ impl<'a, S: ServerApi, H: Host> Renderer<'a, S, H> { if below > 0 { parts.push(format!("{below} below")); } + // `parts` cannot be empty here, so there is no "scrolled, but nothing is off-screen" + // wording to fall back to — this notice always has a direction and a count to name. + // + // The proof, because the previous fallback branch looked like defensive coding and was in + // fact dead code: emptiness needs `scroll == 0 && below == 0`. `scroll` is clamped to at + // most `total - viewport`, so `below == 0` forces `scroll == total - viewport` exactly — + // and that quantity is strictly positive, because this function returned early unless + // `total > area.height`, and `viewport == body.height <= area.height < total`. So + // `below == 0` implies `scroll > 0`, and the two conditions can never hold together. + // + // Verified rather than merely argued: an `assert!(!parts.is_empty())` left in this position + // survived the whole 193-test suite, an exhaustive sweep of `render_form`'s own arguments + // (1..40 lines x every anchor x 9 widths x 1..40 heights) and an integration sweep over + // 7 terminal widths x 15 heights x every reveal state, focus stop and scroll depth. Zero + // hits. The `debug_assert!` stays so that a future change to the early-return guard above + // breaks a test rather than silently resurrecting an unreachable message. + debug_assert!( + !parts.is_empty(), + "a scrolled column must have something above or below it: total={total} \ + viewport={viewport} scroll={scroll} below={below}" + ); // `[tab]` and not `[↑↓]`: the arrows belong to whichever region holds focus, and this // viewport follows focus rather than being steered directly. Promising `[↑↓]` here would // name a key that scrolls the *fold*, not the column, which is the "documented key does // nothing" defect the module already had once. - let text = if parts.is_empty() { - " … form scrolled".to_string() - } else { - format!(" … {} — [tab] moves focus", parts.join(", ")) - }; + let text = format!(" … {} — [tab] moves focus", parts.join(", ")); paragraph(&[Line::styled(text, self.theme.dim)]).render(notice, buf); } @@ -7703,21 +7730,37 @@ mod tests { non-scrolling column would satisfy it and the regression would return unnoticed" ); - for region in [ - Focus::OptionalSection, - Focus::AgentPicker, - Focus::ProviderPicker, + // The label each region's own header carries, so the assertion below proves the marker is + // on the row belonging to THE FOCUSED REGION rather than merely somewhere on screen. + // + // Asserting only "some line starts with `>`" was the weakness: the left column always + // holds three marker-capable headers, and once one of them is on screen the bare-marker + // check passes even if `[tab]` moved focus to a region that scrolled away. That is exactly + // the invisible-focus defect this test is named for, so the old form could have gone green + // through the wrong region's marker. + // + // Matched as a SUBSTRING, not a whole line: at 80x24 the layout puts the results strip on + // the same screen row as the optional header (`> ▾ optional (9) — [esc] collapse ▸ results + // (0)`), so an equality check would fail on the layout rather than on the property. + for (region, label) in [ + (Focus::OptionalSection, "optional ("), + (Focus::AgentPicker, "agents (25)"), + (Focus::ProviderPicker, "providers ("), ] { while shell.focus() != region { assert!(shell.on_key(KeyCode::Tab)); } let drawn = screen(&shell, MIN_COLS, MIN_ROWS); - let marked = drawn.lines().any(|line| line.starts_with('>')); + let marked = drawn + .lines() + .any(|line| line.starts_with('>') && line.contains(label)); assert!( marked, - "with focus on {region:?} at {MIN_COLS}x{MIN_ROWS} the focused row must be ON \ - SCREEN. A focused region scrolled out of the viewport is invisible focus, and \ - [tab] then appears to do nothing. Screen:\n{drawn}" + "with focus on {region:?} at {MIN_COLS}x{MIN_ROWS} the marked row must be ON \ + SCREEN *and* must be {region:?}'s own row — the one carrying {label:?}. A focused \ + region scrolled out of the viewport is invisible focus, and [tab] then appears to \ + do nothing; a marker on some OTHER region's header is the same defect wearing a \ + disguise this assertion is written to strip off. Screen:\n{drawn}" ); } } From cf69b5b341d075cbf6d7f1ccf5a59a55e605b90a Mon Sep 17 00:00:00 2001 From: Stan Fan Date: Mon, 10 Aug 2026 13:49:07 +1000 Subject: [PATCH 6/6] fix(tui): size the footer by wrapped height and stop advertising an unwired key The footer was truncated at the 80-column floor and lost [q] quit. Header and footer were sized with Constraint::Length(frame.footer.len()), and len() counts LOGICAL lines: in Focus::Results the 89-character hint wraps to two rows at 80 columns while len() reported 2 lines and reserved 2 rows. Measured at 80x24, neither "[q] quit" nor "stop following" reached the drawn buffer. NFR-6 says wrap, never truncate, and the key silently dropped was the documented way out. Both regions are now sized with wrapped_heights, the same measurement the left column's viewport already uses. The hint also promised [k] stop following in every non-text-entry focus, but on_key routes Char('k') to the pane from the Focus::Results arm alone. Per #547's precedent on unwired keys, it is now advertised only there. The new tests assert on the DRAWN BUFFER, not the frame: the frame always held the right text, so a frame-level assertion cannot see this defect class. Co-Authored-By: Claude Opus 5 --- tui/src/renderer.rs | 180 ++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 175 insertions(+), 5 deletions(-) diff --git a/tui/src/renderer.rs b/tui/src/renderer.rs index 57d0aebd9..4366dd767 100644 --- a/tui/src/renderer.rs +++ b/tui/src/renderer.rs @@ -1375,10 +1375,23 @@ impl<'a, S: ServerApi, H: Host> Renderer<'a, S, H> { // where a documented key did nothing. Ctrl+R and Ctrl+C work in EVERY focus, so they are // what a text-entry focus advertises. (#321) lines.push(if self.focus_is_text_entry() { - "[tab] focus · [←] commands · [enter] select/run · [ctrl+r] retry · [ctrl+c] quit".to_string() - } else { - "[tab] focus · [←] commands · [enter] run · [k] stop following · [ctrl+r] retry · [q] quit" + "[tab] focus · [←] commands · [enter] select/run · [ctrl+r] retry · [ctrl+c] quit" .to_string() + } else { + // `[k]` is advertised ONLY in `Focus::Results`, because that is the only focus that + // routes it anywhere: `on_key`'s `Focus::Results` arm hands the key to the pane, and + // every other arm never sees a `Char('k')` as a command. Advertising it from the command + // list or a picker fold was #547's unwired-key defect verbatim — the operator presses + // the documented key, nothing happens, and the footer stops being evidence of anything. + // + // `[q]`, by contrast, genuinely works in every non-text-entry focus (its `on_key` arm is + // guarded on `!focus_is_text_entry()`, not on a focus), so it stays in both branches. + let stop = if self.focus == Focus::Results { + " · [k] stop following" + } else { + "" + }; + format!("[tab] focus · [←] commands · [enter] run{stop} · [ctrl+r] retry · [q] quit") }); lines } @@ -2472,10 +2485,33 @@ impl<'a, S: ServerApi, H: Host> Renderer<'a, S, H> { let frame = self.render(); + // Sized by WRAPPED height, not by `Vec::len()`, and that distinction was a live truncation + // bug rather than a tidiness point. + // + // `len()` counts LOGICAL lines. The footer's non-text-entry hint is 89 characters, so at the + // 80-column floor it occupies two screen rows while `len()` reported the footer as 2 lines + // (gating reason + hint) and therefore reserved 2 rows. The hint's second row had nowhere to + // go: measured at 80x24, `[q] quit` and `stop following` were BOTH absent from the drawn + // buffer. NFR-6's rule is "wrap, never truncate", and losing the quit key off the right-hand + // edge is a truncation — the worst one available, since it is the documented way out. + // + // `wrapped_heights` is the same measurement the left column's viewport already uses, so the + // header and footer are now sized against what ratatui will actually paint rather than + // against a count that silently disagrees with it once a line is wider than the terminal. + // `.max(1)` survives for the empty-region case, where a zero-length constraint would give + // the region no row at all. + let header_rows = wrapped_heights(&frame.header, area.width) + .iter() + .sum::() + .max(1) as u16; + let footer_rows = wrapped_heights(&frame.footer, area.width) + .iter() + .sum::() + .max(1) as u16; let [header, main, footer] = Layout::vertical([ - Constraint::Length(frame.header.len().max(1) as u16), + Constraint::Length(header_rows), Constraint::Min(1), - Constraint::Length(frame.footer.len().max(1) as u16), + Constraint::Length(footer_rows), ]) .areas(area); @@ -8167,4 +8203,138 @@ mod tests { {refetched:?}" ); } + + /// **The footer's keys survive the 80-column floor — `[q] quit` is DRAWN, not wrapped away.** + /// + /// The reported defect, measured: `draw` sized the footer with `frame.footer.len()`, which counts + /// LOGICAL lines. In `Focus::Results` the footer is 2 logical lines whose hint is **89 + /// characters** — wider than the 80-column floor, so it occupies 3 screen rows once wrapped. The + /// layout reserved 2. The hint's overflow row had nowhere to go and `[q] quit` was simply absent + /// from the drawn buffer: at 80x24, `drawn.contains("[q] quit")` was `false`, and so was + /// `stop following`. NFR-6's rule is "wrap, never truncate", and silently dropping the documented + /// way out of the program is the worst truncation on offer. + /// + /// Asserted on the DRAWN BUFFER and not on `frame.footer`, which is the whole point: the frame + /// always contained the right text. The defect lived entirely in the gap between the text and + /// the rows allotted to it, so a frame-level assertion cannot see this class of bug at all — it + /// would have been green throughout. + /// + /// `Focus::Results` specifically, because it is the ONLY focus whose hint exceeds 80 columns + /// (measured: 89 there, 68 in the other non-text-entry focuses, 80 in text entry). A test parked + /// on the launch form's default focus would not reproduce it. + #[test] + fn the_footers_quit_key_is_drawn_and_not_truncated_at_the_minimum_supported_size() { + let server = server_with_many_agents(25); + let host = FakeHost::outside_tmux(); + let mut shell = shell_on_the_launch_form(&server, &host, MIN_COLS, MIN_ROWS); + while shell.focus() != Focus::Results { + assert!( + shell.on_key(KeyCode::Tab), + "[tab] must reach the results pane, or this test cannot reach the long hint" + ); + } + + // ANTI-VACUITY: the hint must genuinely be wider than the screen. If a future edit shortens + // it below the floor the truncation becomes unreproducible, and this test would go on + // passing while guarding nothing — so it fails loudly instead and asks to be re-pointed. + let footer = shell.render().footer; + let hint: String = footer + .last() + .expect("the footer always carries a hint line") + .spans + .iter() + .map(|span| span.content.as_ref()) + .collect::(); + assert!( + hint.chars().count() > MIN_COLS as usize, + "this test needs a hint WIDER than {MIN_COLS} columns to exercise wrapping (got {} \ + chars: {hint:?}). Re-point it at a focus whose hint still overflows, or the \ + truncation it guards can no longer occur here", + hint.chars().count() + ); + assert!( + footer.len() < wrapped_heights(&footer, MIN_COLS).iter().sum::(), + "the logical line count must UNDERSTATE the wrapped height, or `len()`-based sizing \ + would have been correct and this test would prove nothing" + ); + + let drawn = screen(&shell, MIN_COLS, MIN_ROWS); + for needle in ["[q] quit", "[k] stop following", "[ctrl+r] retry"] { + assert!( + drawn.contains(needle), + "{needle:?} must be ON SCREEN at {MIN_COLS}x{MIN_ROWS}. A key the footer promises \ + and the layout then clips is a key the operator cannot discover — and for [q] it \ + is the documented way to quit. Screen:\n{drawn}" + ); + } + + // The footer must not have eaten the screen to achieve that: the regions above it are still + // there. Growing the footer at the expense of the form would trade one invisible region for + // another, which is not a fix. + for (region, needle) in [ + ("the command list", "cao launch"), + ("the required fields", "--agents"), + ("the results pane", "results ("), + ] { + assert!( + drawn.contains(needle), + "{region} must survive the taller footer at {MIN_COLS}x{MIN_ROWS} — {needle:?} was \ + not drawn. Reclaiming rows for the footer by pushing the form off screen is the \ + same clipping defect one region over. Screen:\n{drawn}" + ); + } + } + + /// **`[k]` is advertised only where it does something.** + /// + /// The hint named `[k] stop following` in every non-text-entry focus, but `on_key` routes + /// `Char('k')` to the pane from the `Focus::Results` arm ALONE — from the command list or a + /// picker fold the press does nothing at all. That is #547's unwired-key defect verbatim: the + /// operator presses a documented key, gets silence, and the footer stops being evidence. + /// + /// Both halves are asserted. Without the positive half, deleting `[k]` from the hint entirely + /// would leave this test green while removing the operator's only cue that the key exists. + #[test] + fn the_stop_following_hint_appears_only_in_the_focus_that_handles_the_key() { + let server = server_with_many_agents(25); + let host = FakeHost::outside_tmux(); + + for focus in [ + Focus::CommandList, + Focus::OptionalSection, + Focus::AgentPicker, + Focus::ProviderPicker, + ] { + let mut shell = shell_on_the_launch_form(&server, &host, MIN_COLS, MIN_ROWS); + let mut guard = 0; + while shell.focus() != focus { + assert!(shell.on_key(KeyCode::Tab)); + guard += 1; + assert!(guard < 20, "[tab] must reach {focus:?}"); + } + let hint = joined(&shell.render().footer, " "); + assert!( + !hint.contains("stop following"), + "in {focus:?} the footer must NOT advertise [k]: `on_key` only routes it from \ + Focus::Results, so here it is a documented key that does nothing. Got: {hint:?}" + ); + // ...and the focus must still be told how to quit, so the trim above is a correction + // rather than a hint that quietly lost its contents. + assert!( + hint.contains("[q] quit"), + "in {focus:?} the footer must still name the working quit key. Got: {hint:?}" + ); + } + + let mut shell = shell_on_the_launch_form(&server, &host, MIN_COLS, MIN_ROWS); + while shell.focus() != Focus::Results { + assert!(shell.on_key(KeyCode::Tab)); + } + let hint = joined(&shell.render().footer, " "); + assert!( + hint.contains("[k] stop following"), + "in Focus::Results the footer MUST advertise [k] — that is the focus where the key \ + works, and an unadvertised working key is the mirror-image defect. Got: {hint:?}" + ); + } }