From 8059d43c293d4742e80e2d58f8a4fcbaa6c4a0d4 Mon Sep 17 00:00:00 2001 From: Dennis Paler Date: Wed, 12 Aug 2026 18:25:48 +0800 Subject: [PATCH] chore: terminal banner --- CHANGELOG.md | 229 ++++++++-------- apps/tui-cli/src/tui.rs | 373 ++++++++++++++++++++++++--- packages/core-engine/src/analyzer.rs | 111 ++++++++ 3 files changed, 561 insertions(+), 152 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0a0b5e2..96bcc17 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,22 @@ ## [0.9.0] — 08-12-2026 +diffmind was built as a gate: run it, get a verdict, pass or fail. This release +adds the other half — a place to *sit* while deciding what to say about someone +else's branch. Both surfaces share one engine; neither replaces the other. + +### Breaking + +- **`--format json`: `stats.chunks` → `stats.units`**, plus `units_cached` and + `units_unparseable`. The unit of review is now a region of a file rather than + an arbitrary slice of lines, and the field names say so. Exit codes, SARIF and + Markdown output are unchanged, so CI gates are unaffected. +- **Daemon protocol changed.** A running 0.8 daemon is ignored rather than + misused — the version gate already handled this — but you will want + `diffmind serve --stop` after upgrading. +- **`chunk_diff` removed** from the `core-engine` public API, superseded by + `build_units`. + ### Security - **Model weights are now pinned and verified.** Downloads used HuggingFace's @@ -22,6 +38,64 @@ Existing downloads are verified against the new pins on next use; the current files match, so no re-download is expected. +### Added + +- **Code graph.** A tree-sitter symbol graph for 13 languages — Rust, + TypeScript, TSX, JavaScript, Python, Go, Java, C#, Ruby, PHP, C, C++ and + Scala — stored in `.diffmind/graph.db` and updated incrementally + by mtime. Replaces the regex symbol index, which could only see `pub`/`export` + declarations and had no concept of a reference at all. + Review context now includes **the callers of every changed symbol** — a + changed signature is judged against the code that depends on it — plus the + enclosing definition, referenced definitions, and the file's tests. Everything + stays inside a byte budget, so context does not grow with the repository. + Bodies are read from the working tree rather than stored, so a snippet can + never disagree with the file being reviewed. The graph refreshes itself + incrementally before every review (~0.1s on 647 unchanged files) and builds on + first use, so it can never fall behind the code; `--no-index` opts out. Adding a language is one entry in + a table; contributions welcome. + +- **Cross-file review units.** When a symbol and code that calls it both change + in one diff, they are reviewed together as a single unit instead of separately. + Reviewed apart, the model judges an interaction while seeing only one side of + it as background. One call replaces two. +- **Reviewer's cockpit** (`diffmind --tui`). Analyses on launch. Each finding + shows the actual hunk the model reviewed and the context it was given. + `a` accepts (and copies a review comment via OSC 52, which works over SSH), + `d` dismisses, `w` marks wrong. Verdicts are written through immediately. +- **Review standards as markdown** — `.diffmind/rules/*.md`, scoped by path + glob, committed to the repo. `diffmind rules init` / `rules list`. A finding + the model attributes to a rule set gets the ID `rulebook.` and suppresses + like any other; an attribution naming a rule set that does not govern that + file is discarded. +- **Reportable pre-filter.** Lockfiles, `linguist-generated` paths, `@generated` + banners, minified bundles, assets, snapshots and formatting-only hunks are + dropped before the model sees them — and the run says what it skipped: + `312 hunks → 74 reviewable (238 filtered: lockfiles, generated, formatting)`. + Whitespace inside a string literal counts as content; indentation is never + dismissed in Python or YAML. +- **`diffmind stats`** — findings, cost and the accept-to-wrong ratio over + recorded runs. Every review is filed to `.diffmind/runs//`. +- **Cost reporting** — wall-clock and token counts in the footer, in JSON, and + in the run record. Estimated counts are marked `~` rather than passed off as + exact. +- **Revision ranges** — `diffmind v1.2.0..HEAD`, or `--range`. Paths after a + range narrow it. +- **`review.ignore`** globs in `.diffmind/config.toml`. +- diffmind writes its own `.diffmind/.gitignore`, keeping generated state out of + git while leaving `rules/`, `rules.toml`, `config.toml` and `baseline.json` + committable. Your repository's `.gitignore` is not touched. + +### Changed + +- `Graph::definitions_of` is replaced by `definitions_of_names` (batched) and + `declarations_overlapping` (span-based). The "prefer a definition in this + file" rule moved to `rag`, where the file being reviewed is known. +- `ReviewAnalyzer::analyze`'s progress callback takes the unit being reviewed: + `Fn(usize, usize)` → `Fn(usize, usize, &ReviewUnit)`. Callers that need to + show a reader the hunk behind a finding should record it here rather than + predicting the unit list with `plan_units`. + ### Fixed - **A non-ASCII path in a diff header could abort the process.** The @@ -34,6 +108,21 @@ which matters for binary files and mode changes, where no `+++ b/…` follows to correct it. +- **A large rule set could fail the whole review.** Two faults compounding. + Splitting an oversized prompt halves the *diff*, but the context and rule + sections were sized from the window alone — identical at every recursion + level — so a prompt that overran because of its rules failed the same way four + times and then gave up. And giving up returned an error that propagated out of + `analyze`, exiting 2 and discarding the findings every *other* unit had + produced, including deterministic ones that never needed a model. + + Both sections now shrink with each split (dropping whole rule sets rather than + truncating one mid-sentence), and are held to at most half of what the window + has left, so the diff under review cannot be crowded out. A unit that still + will not fit is counted in the new `units_too_large` stat and skipped, and the + footer says so. Unchanged for an ordinary review: at depth 0 on the shipped + 32K window the budgets are exactly what they were. + - **A non-ASCII identifier could abort the detector.** `references_identifier` advanced by one *byte* past a match, so a name whose first character is multi-byte — `const élan = 1`, which `extract_declared_name` collects because @@ -97,20 +186,24 @@ Both are now per-request. The daemon also re-asserts that caching is off above temperature 0 rather than trusting the client to have done so. -### Internal - -- **End-to-end coverage for `--stdin`.** `apps/tui-cli/tests/stdin_pipeline.rs` - runs the real binary with a diff on stdin and asserts on stdout and the exit - code, with inference supplied by a stub `openai-compatible` endpoint rather - than the bundled model — so no 1.1 GB download is a test dependency, and the - path exercised is a shipped one rather than a test-only seam. - The stub answers each request about whichever file it can see in the prompt, - and records the prompts, so the tests can assert what the model was *shown* and - not merely what came back. - - This is the coverage whose absence let one defect live in four places at once: - each copy had unit tests, and none of them tested the four together. Reverting - all four fixes now fails four of the six new tests. +- **The result cache never worked across files.** Context was assembled once + from the whole diff and folded into every chunk's cache key, so editing one + file invalidated every other file's cached result — a re-review after a + force-push re-inferred the entire diff. Context is now assembled per unit. +- **Each chunk was given the wrong context.** The six enclosing functions were + taken from whichever files sorted first, so most chunks paid for context about + files they did not contain. +- **Editing one region of a file re-reviewed the whole file.** Units are grouped + by region, so a change in one function only invalidates that function. +- **Glob matching could not express a directory prefix.** `src/api/**` matched + nothing, and `**/gen/**` was a substring test that also matched + `src/gen-legacy/`. Affects `rules.toml` `files` patterns as well. +- **Conflicting input selectors were silently resolved by order.** Passing both + `--staged` and `--last` reviewed whichever the code checked first; it now + errors. +- A diff dominated by a lockfile is no longer refused for size before the + lockfile is filtered out. +- The TUI now honours the pre-filter and files a run record, which it did not. ### Performance @@ -138,101 +231,17 @@ every review — including one that turned out to be fully cached. collision would hand one unit another's context, and confidently wrong context is the failure the whole module exists to prevent. -### Changed - -- `Graph::definitions_of` is replaced by `definitions_of_names` (batched) and - `declarations_overlapping` (span-based). The "prefer a definition in this - file" rule moved to `rag`, where the file being reviewed is known. -- `ReviewAnalyzer::analyze`'s progress callback takes the unit being reviewed: - `Fn(usize, usize)` → `Fn(usize, usize, &ReviewUnit)`. Callers that need to - show a reader the hunk behind a finding should record it here rather than - predicting the unit list with `plan_units`. -- **Daemon protocol changed** — `ReviewRequest` gains `temperature` and `seed` - and drops the unused `languages` field. The version gate already refuses a - daemon from another build; run `diffmind serve --stop` after upgrading. - - -diffmind was built as a gate: run it, get a verdict, pass or fail. This release -adds the other half — a place to *sit* while deciding what to say about someone -else's branch. Both surfaces share one engine; neither replaces the other. - -### Breaking - -- **`--format json`: `stats.chunks` → `stats.units`**, plus `units_cached` and - `units_unparseable`. The unit of review is now a region of a file rather than - an arbitrary slice of lines, and the field names say so. Exit codes, SARIF and - Markdown output are unchanged, so CI gates are unaffected. -- **Daemon protocol changed.** A running 0.8 daemon is ignored rather than - misused — the version gate already handled this — but you will want - `diffmind serve --stop` after upgrading. -- **`chunk_diff` removed** from the `core-engine` public API, superseded by - `build_units`. - -### Added - -- **Code graph.** A tree-sitter symbol graph for 13 languages — Rust, - TypeScript, TSX, JavaScript, Python, Go, Java, C#, Ruby, PHP, C, C++ and - Scala — stored in `.diffmind/graph.db` and updated incrementally - by mtime. Replaces the regex symbol index, which could only see `pub`/`export` - declarations and had no concept of a reference at all. - Review context now includes **the callers of every changed symbol** — a - changed signature is judged against the code that depends on it — plus the - enclosing definition, referenced definitions, and the file's tests. Everything - stays inside a byte budget, so context does not grow with the repository. - Bodies are read from the working tree rather than stored, so a snippet can - never disagree with the file being reviewed. The graph refreshes itself - incrementally before every review (~0.1s on 647 unchanged files) and builds on - first use, so it can never fall behind the code; `--no-index` opts out. Adding a language is one entry in - a table; contributions welcome. - -- **Cross-file review units.** When a symbol and code that calls it both change - in one diff, they are reviewed together as a single unit instead of separately. - Reviewed apart, the model judges an interaction while seeing only one side of - it as background. One call replaces two. -- **Reviewer's cockpit** (`diffmind --tui`). Analyses on launch. Each finding - shows the actual hunk the model reviewed and the context it was given. - `a` accepts (and copies a review comment via OSC 52, which works over SSH), - `d` dismisses, `w` marks wrong. Verdicts are written through immediately. -- **Review standards as markdown** — `.diffmind/rules/*.md`, scoped by path - glob, committed to the repo. `diffmind rules init` / `rules list`. A finding - the model attributes to a rule set gets the ID `rulebook.` and suppresses - like any other; an attribution naming a rule set that does not govern that - file is discarded. -- **Reportable pre-filter.** Lockfiles, `linguist-generated` paths, `@generated` - banners, minified bundles, assets, snapshots and formatting-only hunks are - dropped before the model sees them — and the run says what it skipped: - `312 hunks → 74 reviewable (238 filtered: lockfiles, generated, formatting)`. - Whitespace inside a string literal counts as content; indentation is never - dismissed in Python or YAML. -- **`diffmind stats`** — findings, cost and the accept-to-wrong ratio over - recorded runs. Every review is filed to `.diffmind/runs//`. -- **Cost reporting** — wall-clock and token counts in the footer, in JSON, and - in the run record. Estimated counts are marked `~` rather than passed off as - exact. -- **Revision ranges** — `diffmind v1.2.0..HEAD`, or `--range`. Paths after a - range narrow it. -- **`review.ignore`** globs in `.diffmind/config.toml`. -- diffmind writes its own `.diffmind/.gitignore`, keeping generated state out of - git while leaving `rules/`, `rules.toml`, `config.toml` and `baseline.json` - committable. Your repository's `.gitignore` is not touched. +### Internal -### Fixed +- **End-to-end coverage for `--stdin`.** `apps/tui-cli/tests/stdin_pipeline.rs` + runs the real binary with a diff on stdin and asserts on stdout and the exit + code, with inference supplied by a stub `openai-compatible` endpoint rather + than the bundled model — so no 1.1 GB download is a test dependency, and the + path exercised is a shipped one rather than a test-only seam. + The stub answers each request about whichever file it can see in the prompt, + and records the prompts, so the tests can assert what the model was *shown* and + not merely what came back. -- **The result cache never worked across files.** Context was assembled once - from the whole diff and folded into every chunk's cache key, so editing one - file invalidated every other file's cached result — a re-review after a - force-push re-inferred the entire diff. Context is now assembled per unit. -- **Each chunk was given the wrong context.** The six enclosing functions were - taken from whichever files sorted first, so most chunks paid for context about - files they did not contain. -- **Editing one region of a file re-reviewed the whole file.** Units are grouped - by region, so a change in one function only invalidates that function. -- **Glob matching could not express a directory prefix.** `src/api/**` matched - nothing, and `**/gen/**` was a substring test that also matched - `src/gen-legacy/`. Affects `rules.toml` `files` patterns as well. -- **Conflicting input selectors were silently resolved by order.** Passing both - `--staged` and `--last` reviewed whichever the code checked first; it now - errors. -- A diff dominated by a lockfile is no longer refused for size before the - lockfile is filtered out. -- The TUI now honours the pre-filter and files a run record, which it did not. + This is the coverage whose absence let one defect live in four places at once: + each copy had unit tests, and none of them tested the four together. Reverting + all four fixes now fails four of the six new tests. diff --git a/apps/tui-cli/src/tui.rs b/apps/tui-cli/src/tui.rs index 0a2d8f3..1df49ca 100644 --- a/apps/tui-cli/src/tui.rs +++ b/apps/tui-cli/src/tui.rs @@ -29,7 +29,7 @@ use crossterm::{ use ratatui::{ Frame, Terminal, backend::{Backend, CrosstermBackend}, - layout::{Constraint, Direction, Layout, Rect}, + layout::{Alignment, Constraint, Direction, Layout, Rect}, style::{Color, Modifier, Style}, text::{Line, Span}, widgets::{Block, Borders, List, ListItem, ListState, Paragraph, Wrap}, @@ -42,6 +42,7 @@ use std::{ time::Duration, }; +use crate::output::VERSION; use crate::runs::{self, Verdict}; use crate::settings::Settings; @@ -104,6 +105,46 @@ impl App { self.units.get(finding.unit_id.as_deref()?) } + /// Fold one message from the analysis thread into the view. + /// + /// Lifted out of the event loop so it can be tested. The loop itself cannot + /// be: it polls real stdin and only returns on a keypress, which left the + /// step that actually populates the evidence pane — `Msg::Unit` — reachable + /// only by launching the terminal and looking at it. That is exactly the + /// wiring that was silently broken before, so it should not be the one part + /// nothing can assert on. + fn apply(&mut self, msg: Msg) { + match msg { + Msg::Progress(s) => self.status = s, + Msg::Unit(id, view) => { + self.units.insert(id, *view); + } + Msg::Findings(mut batch) => { + batch.retain(|f| f.severity >= self.min_severity); + let was_empty = self.findings.is_empty(); + self.findings.extend(batch); + // Select the first finding the moment one arrives, so the detail + // pane has something in it without the reviewer pressing a key. + if was_empty && !self.findings.is_empty() { + self.state.select(Some(0)); + } + } + Msg::Done(stats) => { + self.analyzing = false; + self.status = format!( + "{} finding{} — a accept · d dismiss · w wrong", + self.findings.len(), + if self.findings.len() == 1 { "" } else { "s" } + ); + self.stats = Some(*stats); + } + Msg::Error(e) => { + self.analyzing = false; + self.status = format!("Error: {e}"); + } + } + } + /// Record a verdict for the selected finding and advance. /// /// The verdict is written through to disk immediately rather than batched on @@ -316,31 +357,7 @@ where let mut disconnected = false; loop { match rx.try_recv() { - Ok(Msg::Progress(s)) => app.status = s, - Ok(Msg::Unit(id, view)) => { - app.units.insert(id, *view); - } - Ok(Msg::Findings(mut batch)) => { - batch.retain(|f| f.severity >= app.min_severity); - let was_empty = app.findings.is_empty(); - app.findings.extend(batch); - if was_empty && !app.findings.is_empty() { - app.state.select(Some(0)); - } - } - Ok(Msg::Done(stats)) => { - app.analyzing = false; - app.status = format!( - "{} finding{} — a accept · d dismiss · w wrong", - app.findings.len(), - if app.findings.len() == 1 { "" } else { "s" } - ); - app.stats = Some(*stats); - } - Ok(Msg::Error(e)) => { - app.analyzing = false; - app.status = format!("Error: {e}"); - } + Ok(msg) => app.apply(msg), Err(TryRecvError::Empty) => break, Err(TryRecvError::Disconnected) => { disconnected = true; @@ -494,13 +511,26 @@ fn draw(f: &mut Frame, app: &mut App) { ]) .split(f.area()); - let header = Paragraph::new(format!(" diffmind {}", app.status)) - .block(Block::default().borders(Borders::ALL).title("Status")) - .style(if app.analyzing { - Style::default().fg(Color::Yellow) - } else { - Style::default().fg(Color::Green) - }); + let status_style = if app.analyzing { + Style::default().fg(Color::Yellow) + } else { + Style::default().fg(Color::Green) + }; + let header = Paragraph::new(Line::from(vec![ + Span::styled( + format!(" diffmind v{VERSION}"), + Style::default() + .fg(Color::Cyan) + .add_modifier(Modifier::BOLD), + ), + Span::styled(format!(" {}", app.status), status_style), + ])) + .block( + Block::default() + .borders(Borders::ALL) + .border_style(status_style) + .title("Status"), + ); f.render_widget(header, rows[0]); if app.findings.is_empty() { @@ -523,20 +553,151 @@ fn draw(f: &mut Frame, app: &mut App) { ); } +/// The wordmark, in block letters, shown while there is nothing to triage yet. +/// +/// It lives in the empty pane rather than the header because that is the only +/// place it is free: the pane is already blank during analysis, and the banner +/// is gone the moment the first finding lands. A logo that persisted into +/// triage would be spending rows on itself that the findings list needs. +const BANNER: [&str; 6] = [ + "██████╗ ██╗███████╗███████╗███╗ ███╗██╗███╗ ██╗██████╗ ", + "██╔══██╗██║██╔════╝██╔════╝████╗ ████║██║████╗ ██║██╔══██╗", + "██║ ██║██║█████╗ █████╗ ██╔████╔██║██║██╔██╗ ██║██║ ██║", + "██║ ██║██║██╔══╝ ██╔══╝ ██║╚██╔╝██║██║██║╚██╗██║██║ ██║", + "██████╔╝██║██║ ██║ ██║ ╚═╝ ██║██║██║ ╚████║██████╔╝", + "╚═════╝ ╚═╝╚═╝ ╚═╝ ╚═╝ ╚═╝╚═╝╚═╝ ╚═══╝╚═════╝ ", +]; + +/// Half-height wordmark for panes too narrow for [`BANNER`]. +const BANNER_COMPACT: [&str; 2] = [ + "█▀▄ █ █▀▀ █▀▀ █▀▄▀█ █ █▄ █ █▀▄", + "█▄▀ █ █▀ █▀ █ ▀ █ █ █ ▀█ █▄▀", +]; + +const TAGLINE: &str = "local-first AI code review"; + +/// Columns each wordmark needs. Both are pure ASCII-width glyphs, so the +/// character count is the column count. +const BANNER_WIDTH: u16 = 59; +const BANNER_COMPACT_WIDTH: u16 = 30; + +/// The wordmark sized to the pane, or nothing if the pane cannot spare it. +/// +/// A banner that wraps is worse than no banner, so each size is used only when +/// it fits whole — in both axes. The height check reserves room for the version +/// line and status beneath it, which are the part a reviewer actually needs. +fn banner_lines(inner: Rect) -> Vec> { + let art: &[&str] = if inner.width >= BANNER_WIDTH { + &BANNER + } else if inner.width >= BANNER_COMPACT_WIDTH { + &BANNER_COMPACT + } else { + return Vec::new(); + }; + + // The art, plus a blank line, the version line, and two rows of status. + if inner.height < art.len() as u16 + 4 { + return Vec::new(); + } + + art.iter().map(|row| banner_row(row)).collect() +} + +/// One row of block letters, with the drop shadow dimmed behind the face. +/// +/// Runs of like-styled characters are coalesced rather than emitted per glyph: +/// this pane redraws on every tick of the event loop. +fn banner_row(row: &str) -> Line<'static> { + let mut spans: Vec> = Vec::new(); + let mut run = String::new(); + let mut run_is_face = None; + + for ch in row.chars() { + let is_face = ch == '█' || ch == '▀' || ch == '▄'; + if run_is_face != Some(is_face) && !run.is_empty() { + spans.push(Span::styled( + std::mem::take(&mut run), + banner_style(run_is_face == Some(true)), + )); + } + run_is_face = Some(is_face); + run.push(ch); + } + if !run.is_empty() { + spans.push(Span::styled(run, banner_style(run_is_face == Some(true)))); + } + Line::from(spans) +} + +fn banner_style(is_face: bool) -> Style { + if is_face { + Style::default() + .fg(Color::Cyan) + .add_modifier(Modifier::BOLD) + } else { + Style::default().fg(Color::Blue) + } +} + fn render_empty(f: &mut Frame, area: Rect, app: &App) { - let body = if app.analyzing { - "\n Analyzing…\n\n Findings appear here as each unit completes." + let block = Block::default().borders(Borders::ALL).title("Review"); + let inner = block.inner(area); + f.render_widget(block, area); + + let status: Vec = if app.analyzing { + vec![ + Line::styled("Analyzing…", Style::default().fg(Color::Cyan)), + Line::styled( + "Findings appear here as each unit completes.", + Style::default().fg(Color::DarkGray), + ), + ] } else if app.stats.is_some() { - "\n No issues found.\n\n Press 'r' to re-run." + vec![ + Line::styled("No issues found.", Style::default().fg(Color::Green)), + Line::styled("Press 'r' to re-run.", Style::default().fg(Color::DarkGray)), + ] } else { - "\n Starting…" + vec![Line::styled("Starting…", Style::default().fg(Color::Cyan))] }; - let widget = Paragraph::new(body) - .block(Block::default().borders(Borders::ALL).title("Review")) - .style(Style::default().fg(Color::Cyan)) - .wrap(Wrap { trim: false }); - f.render_widget(widget, area); + let banner = banner_lines(inner); + if banner.is_empty() { + // Too small for the wordmark — fall back to the plain left-aligned pane. + let mut lines = vec![Line::raw("")]; + lines.extend(status.into_iter().map(|l| { + let mut spans = vec![Span::raw(" ")]; + spans.extend(l.spans); + Line::from(spans) + })); + f.render_widget(Paragraph::new(lines).wrap(Wrap { trim: false }), inner); + return; + } + + let mut lines = banner; + lines.push(Line::raw("")); + // The tagline is the first thing to go: the pane does not wrap, so a + // subtitle wider than the pane would be sheared mid-word. + let subtitle = format!("v{VERSION} · {TAGLINE}"); + lines.push(Line::styled( + if subtitle.chars().count() as u16 <= inner.width { + subtitle + } else { + format!("v{VERSION}") + }, + Style::default().fg(Color::DarkGray), + )); + lines.push(Line::raw("")); + lines.extend(status); + + // Sit the block a third of the way down rather than dead centre: optically + // centred beats arithmetically centred, and it keeps the status text near + // where the findings list will replace it. + let pad = inner.height.saturating_sub(lines.len() as u16) / 3; + let mut padded = vec![Line::raw(""); pad as usize]; + padded.extend(lines); + + f.render_widget(Paragraph::new(padded).alignment(Alignment::Center), inner); } fn render_findings(f: &mut Frame, area: Rect, app: &mut App) { @@ -721,6 +882,59 @@ fn kv<'a>(key: &'a str, value: &str) -> Line<'a> { mod tests { use super::*; + /// A ragged row would wrap and shear the wordmark in half, and the declared + /// widths are what the fit check is decided on — so both must be true. + #[test] + fn banner_rows_all_match_their_declared_width() { + for row in BANNER { + assert_eq!( + row.chars().count(), + BANNER_WIDTH as usize, + "row {row:?} does not match BANNER_WIDTH" + ); + } + for row in BANNER_COMPACT { + assert_eq!( + row.chars().count(), + BANNER_COMPACT_WIDTH as usize, + "row {row:?} does not match BANNER_COMPACT_WIDTH" + ); + } + } + + #[test] + fn banner_steps_down_and_then_out_as_the_pane_narrows() { + let pane = |w, h| Rect::new(0, 0, w, h); + + assert_eq!(banner_lines(pane(BANNER_WIDTH, 20)).len(), BANNER.len()); + assert_eq!( + banner_lines(pane(BANNER_WIDTH - 1, 20)).len(), + BANNER_COMPACT.len(), + "one column short of the full wordmark must drop to the compact one" + ); + assert!( + banner_lines(pane(BANNER_COMPACT_WIDTH - 1, 20)).is_empty(), + "a pane too narrow for either wordmark gets none" + ); + } + + /// A short pane spends its rows on the status text, not on the logo. + #[test] + fn banner_is_dropped_when_the_pane_is_too_short() { + assert!(banner_lines(Rect::new(0, 0, BANNER_WIDTH, 8)).is_empty()); + assert!(!banner_lines(Rect::new(0, 0, BANNER_WIDTH, 10)).is_empty()); + } + + /// The face of the letters and their drop shadow must not collapse into one + /// flat colour — the depth is the whole point of the block font. + #[test] + fn banner_row_styles_the_shadow_apart_from_the_face() { + let line = banner_row(BANNER[0]); + let colours: Vec<_> = line.spans.iter().filter_map(|s| s.style.fg).collect(); + assert!(colours.contains(&Color::Cyan), "no lit face"); + assert!(colours.contains(&Color::Blue), "no shadow"); + } + fn finding(sev: Severity) -> ReviewFinding { ReviewFinding { file: "src/a.rs".into(), @@ -885,6 +1099,81 @@ mod tests { assert_eq!(base64("→".as_bytes()), "4oaS"); } + /// The wiring the evidence pane actually rests on, driven the way the event + /// loop drives it. + /// + /// The pane is fed by two messages arriving in order: the analyzer announces + /// a unit, then streams findings that name it. Previously the units were + /// *predicted* up front instead, which resolved to nothing for any unit the + /// grouper had merged — a blank pane on exactly the cross-file findings that + /// most need evidence. Nothing could assert on it, because the only code + /// path ran inside a loop that polls stdin. + #[test] + fn a_unit_announced_before_its_findings_gives_them_evidence() { + let mut a = app(0, "apply-order"); + + a.apply(Msg::Unit( + "unit-7".into(), + Box::new(UnitView { + text: "@@ -1 +1 @@\n+let x = 2;".into(), + context: "fn enclosing() {}".into(), + }), + )); + + let mut f = finding(Severity::High); + f.unit_id = Some("unit-7".into()); + a.apply(Msg::Findings(vec![f])); + + assert_eq!(a.findings.len(), 1); + assert_eq!( + a.state.selected(), + Some(0), + "the first finding selects itself, so the pane is never blank on arrival" + ); + + let shown = a + .evidence(&a.findings[0]) + .expect("the announced unit must resolve — this is the blank-pane bug"); + assert!(shown.text.contains("+let x = 2;")); + assert!(shown.context.contains("fn enclosing()")); + } + + /// A detector finding has no unit and must not borrow another's hunk, but it + /// must also not stop the pane working for the findings that do have one. + #[test] + fn a_detector_finding_coexists_with_unit_backed_ones() { + let mut a = app(0, "apply-mixed"); + a.apply(Msg::Unit( + "unit-1".into(), + Box::new(UnitView { + text: "hunk".into(), + context: String::new(), + }), + )); + + let detector = finding(Severity::High); // unit_id: None + let mut model = finding(Severity::High); + model.unit_id = Some("unit-1".into()); + a.apply(Msg::Findings(vec![detector, model])); + + assert!(a.evidence(&a.findings[0]).is_none()); + assert!(a.evidence(&a.findings[1]).is_some()); + } + + /// Findings below the reporting threshold never reach the queue, so the + /// verdict count and the gate agree with what the reviewer can see. + #[test] + fn applying_findings_respects_the_severity_threshold() { + let mut a = app(0, "apply-severity"); + a.min_severity = Severity::High; + a.apply(Msg::Findings(vec![ + finding(Severity::Low), + finding(Severity::High), + ])); + assert_eq!(a.findings.len(), 1); + assert_eq!(a.findings[0].severity, Severity::High); + } + #[test] fn evidence_is_found_by_unit_id() { let mut a = app(1, "evidence"); diff --git a/packages/core-engine/src/analyzer.rs b/packages/core-engine/src/analyzer.rs index 8a1da68..7ccafe5 100644 --- a/packages/core-engine/src/analyzer.rs +++ b/packages/core-engine/src/analyzer.rs @@ -1305,6 +1305,117 @@ diff --git a/src/a.rs b/src/a.rs ); } + /// A backend whose window is far too small for any prompt, so every unit + /// exhausts the split depth. Records nothing: it is never reached. + struct TinyWindowBackend; + + impl ReviewBackend for TinyWindowBackend { + fn generate(&mut self, _p: &Prompt, _o: &GenOptions) -> Result { + panic!("a unit that cannot fit must never reach the backend"); + } + fn describe(&self) -> String { + "tiny".into() + } + fn context_tokens(&self) -> usize { + 128 + } + } + + /// A unit nobody can fit must not take the run down with it. + /// + /// This used to return `Err` from `analyze_chunk`, and every error except a + /// parse failure propagated straight out of `analyze` — so one oversized + /// hunk exited 2 and discarded the findings every *other* unit had already + /// produced, including the deterministic ones that never needed a model. + #[test] + fn a_unit_that_cannot_fit_is_counted_and_skipped_not_fatal() { + // Commented-out auth, which DM001 catches with no model involved. + let diff = "\ +diff --git a/src/auth.js b/src/auth.js +--- a/src/auth.js ++++ b/src/auth.js +@@ -10,6 +10,6 @@ + function check() { +- const token = read(); +- if (!token) throw new Error('no token'); +- return verify(token); ++ // const token = read(); ++ // if (!token) throw new Error('no token'); ++ // return verify(token); + } +"; + let mut analyzer = + ReviewAnalyzer::new(Box::new(TinyWindowBackend)).with_triage(TriageMode::Off); + + let (summary, stats) = analyzer + .analyze(diff, &|_| String::new(), 512, |_, _, _| {}, |_| {}) + .expect("an unfittable unit must not fail the run"); + + assert_eq!(stats.units_too_large, 1, "the skip should be counted"); + assert_eq!( + stats.units_unparseable, 0, + "not a parse failure — the model was never asked" + ); + assert!( + summary + .findings + .iter() + .any(|f| f.rule_id.as_deref() == Some(crate::types::RULE_COMMENTED_OUT_CODE)), + "deterministic findings must survive a unit the model could not read: {:?}", + summary.findings + ); + } + + /// The rule sets are the same size no matter how the diff is split, so + /// halving the hunk cannot rescue a prompt they overran. The budget has to + /// shrink with depth or the retry is theatre. + #[test] + fn the_optional_sections_shrink_with_each_split() { + let analyzer = ReviewAnalyzer::new(Box::new(RecordingBackend { + seen: Arc::new(Mutex::new(Vec::new())), + })); + + let at = |depth| analyzer.section_budget_bytes(1024, depth); + assert!(at(0) > at(1), "depth 1 must be smaller than depth 0"); + assert!(at(1) > at(2)); + assert!(at(2) > at(3)); + assert!(at(4) >= MIN_SECTION_BYTES, "but never vanishes entirely"); + + // Unchanged at depth 0 for the shipped 32K window, so an ordinary review + // sees exactly the context and rules it saw before. + assert_eq!(at(0), MAX_SECTION_BYTES); + } + + /// The diff is the thing under review; the optional sections must not be + /// able to crowd it out of its own window. + #[test] + fn the_optional_sections_never_claim_more_than_half_the_window() { + struct Window(usize); + impl ReviewBackend for Window { + fn generate(&mut self, _p: &Prompt, _o: &GenOptions) -> Result { + unreachable!() + } + fn describe(&self) -> String { + "w".into() + } + fn context_tokens(&self) -> usize { + self.0 + } + } + + for window in [2048usize, 4096, 8192, 32_768] { + let analyzer = ReviewAnalyzer::new(Box::new(Window(window))); + let max_new = 1024; + let available = window.saturating_sub(max_new + SYSTEM_PROMPT_TOKENS); + // Two sections, three bytes a token, against what the window has left. + let claimed_tokens = (analyzer.section_budget_bytes(max_new, 0) * 2) / 3; + assert!( + claimed_tokens <= available.max(1), + "window {window}: sections claim {claimed_tokens} of {available} tokens" + ); + } + } + /// Emits one finding that claims to violate `rule`, at `high`. struct ClaimingBackend { rule: &'static str,