diff --git a/tui/Cargo.toml b/tui/Cargo.toml index 17ae66ac9..3efe6f00d 100644 --- a/tui/Cargo.toml +++ b/tui/Cargo.toml @@ -92,7 +92,30 @@ 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. `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", + "unstable-rendered-line-info", +] } crossterm = "0.29.0" # --------------------------------------------------------------------------------------- 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..4366dd767 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,87 @@ 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. + // + // 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} of {total} commands above")); + } + if below > 0 { + // 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!(" … {}, {SCROLL_RESIDUE_MARKER}", parts.join(", "))); + } + 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 +1030,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()) }) @@ -785,6 +1045,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() { @@ -797,8 +1075,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() )]; @@ -819,8 +1102,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. // @@ -828,58 +1122,186 @@ 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) => { - 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, + agents_focused, + )); } 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) - lines.push(format!( - "agents: unavailable — {error}. Press [ctrl+r] to retry" + // + // 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(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) => { - 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, + 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, )), } 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 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![focus_marked( + &format!( + "{glyph} {label} ({count}{diagnosis}) — [enter] {action}", + count = rows.len() + ), + focused, + )]; + + if !expanded { + return lines; + } + + let window = self.picker_window(); + // 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()); + + // 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 +1359,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()), } @@ -946,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 } @@ -1036,6 +1478,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 +1574,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 +1632,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 +1659,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 +1675,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 +1691,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 { @@ -1435,6 +2041,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 } @@ -1871,29 +2485,66 @@ 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); 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()); 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); @@ -1909,12 +2560,96 @@ 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")); + } + // `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 = 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 @@ -1937,9 +2672,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 +2707,179 @@ 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"; + +/// `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 — +/// 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 { @@ -2348,21 +3274,67 @@ 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, + SCROLL_RESIDUE_MARKER, }; use crate::catalog::{self, CommandId, Policy}; use crate::error::TuiError; 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 +3890,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 +3913,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 +4118,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 +4161,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 +4607,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 +4740,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,11 +4773,26 @@ 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 keypress must queue launch so the event loop can draw pending before network I/O" + "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(), + 0, + "the keypress must queue launch so the event loop can draw pending before network I/O" ); assert_eq!( shell.banner().map(|banner| banner.severity), @@ -3818,7 +4812,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 +4850,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 +4919,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 +5160,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 +5291,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 +5466,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 +5783,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 +5813,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 +5914,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 +5980,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 +6033,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 +6590,1751 @@ 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" + ); + } + + // ── 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" + ); + + // 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('>') && line.contains(label)); + assert!( + marked, + "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}" + ); + } + } + + /// **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" + ); + } + + // ── 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:?}" + ); + } + + /// **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:?}" + ); + } } 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" + ); +}