From 70c8dc27497a20f6890c9f39aab5a05d3795bccc Mon Sep 17 00:00:00 2001 From: Dennis Paler Date: Wed, 12 Aug 2026 13:42:20 +0800 Subject: [PATCH 1/3] fix(graph): refresh the code graph before every review --- CHANGELOG.md | 4 ++- apps/tui-cli/src/cli.rs | 5 ++++ apps/tui-cli/src/config.rs | 3 +++ apps/tui-cli/src/graph/store.rs | 44 +++++++++++++++++++++++++++++++++ apps/tui-cli/src/main.rs | 40 ++++++++++++++++++++++++++++++ apps/tui-cli/src/settings.rs | 3 +++ 6 files changed, 98 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f65d762..35a0c07 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,7 +30,9 @@ else's branch. Both surfaces share one engine; neither replaces the other. 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. Adding a language is one entry in + 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 diff --git a/apps/tui-cli/src/cli.rs b/apps/tui-cli/src/cli.rs index 10847ae..67f451e 100644 --- a/apps/tui-cli/src/cli.rs +++ b/apps/tui-cli/src/cli.rs @@ -119,6 +119,11 @@ pub struct Cli { #[arg(long)] pub no_cache: bool, + /// Do not refresh the code graph before reviewing. Faster, but findings are + /// judged against whatever the graph last saw. + #[arg(long)] + pub no_index: bool, + /// Ignore `.diffmind/baseline.json` for this run #[arg(long)] pub no_baseline: bool, diff --git a/apps/tui-cli/src/config.rs b/apps/tui-cli/src/config.rs index 4b05245..2b909b3 100644 --- a/apps/tui-cli/src/config.rs +++ b/apps/tui-cli/src/config.rs @@ -39,6 +39,9 @@ pub struct ReviewConfig { /// Same syntax as a rule's `files`: `*.ts`, `**/legacy/**`, or an exact path. #[serde(default)] pub ignore: Option>, + /// Refresh the code graph before each review. On by default — a stale graph + /// reports wrong line ranges, not merely missing ones. + pub auto_index: Option, } #[derive(Debug, Deserialize, Default, Clone)] diff --git a/apps/tui-cli/src/graph/store.rs b/apps/tui-cli/src/graph/store.rs index 7eeedce..ae15c1c 100644 --- a/apps/tui-cli/src/graph/store.rs +++ b/apps/tui-cli/src/graph/store.rs @@ -488,6 +488,50 @@ pub fn refresh(token: &str) -> bool { let _ = std::fs::remove_dir_all(&root); } + /// Why the graph is refreshed before every review rather than on demand. + /// + /// A stale graph does not merely miss new code — its line ranges point at + /// whatever now occupies those lines, and `source` reads the working tree, + /// so the model is handed unrelated code labelled as the enclosing symbol. + /// Confidently wrong context is worse than none. + #[test] + fn a_stale_graph_reports_the_wrong_lines_until_reindexed() { + let root = project("stale"); + write( + &root, + "src/a.rs", + "pub fn target() {\n let secret = 1;\n}\n", + ); + let mut g = Graph::open(&root).unwrap(); + g.index(&root, &|_| {}).unwrap(); + + let before = g.definitions_of("target", None, 1).remove(0); + assert!(before.source(&root, 100).unwrap().contains("let secret")); + + // Someone adds imports at the top — utterly ordinary. + std::thread::sleep(std::time::Duration::from_millis(20)); + write( + &root, + "src/a.rs", + "// added\n// added\n// added\n// added\n// added\npub fn target() {\n let secret = 1;\n}\n", + ); + + let stale = g.definitions_of("target", None, 1).remove(0); + assert!( + !stale.source(&root, 100).unwrap().contains("let secret"), + "this is the failure mode being guarded against" + ); + + // Re-indexing is what makes it right again, and is cheap enough to do + // before every review. + g.index(&root, &|_| {}).unwrap(); + let fresh = g.definitions_of("target", None, 1).remove(0); + assert_eq!(fresh.start_line, 6); + assert!(fresh.source(&root, 100).unwrap().contains("let secret")); + + let _ = std::fs::remove_dir_all(&root); + } + #[test] fn a_deleted_file_drops_out_of_the_graph() { let root = project("deleted"); diff --git a/apps/tui-cli/src/main.rs b/apps/tui-cli/src/main.rs index 3826960..7d07ac6 100644 --- a/apps/tui-cli/src/main.rs +++ b/apps/tui-cli/src/main.rs @@ -103,6 +103,11 @@ fn run() -> Result { // surfaces review exactly the same set of hunks. let (diff, prefilter) = apply_prefilter(&diff, &settings, &project_root)?; + // Refresh the graph before either surface reads it. + if settings.auto_index { + sync_graph(&project_root, settings.format.is_machine_readable()); + } + if prefilter.dropped_everything() { // Distinct from "no changes": there *were* changes, and none of them // were worth a reviewer's attention. Reporting zero findings without @@ -725,6 +730,41 @@ fn read_head(path: &Path) -> std::io::Result { /// a symbol lookup rather than a re-read of `symbols.json`. Returning a closure /// (instead of one context string for the whole diff) is what keeps each /// chunk's cache key independent of the other files in the diff. +/// Bring the code graph up to date before reviewing. +/// +/// Incremental and mtime-keyed: re-checking an unchanged 647-file repository +/// costs about a tenth of a second, which is nothing beside one inference pass. +/// +/// Doing it automatically matters more than the cost. A stale graph does not +/// merely miss new code — it reports **wrong line ranges**, and `Def::source` +/// then reads those lines out of the working tree and hands the model unrelated +/// code labelled as the enclosing function. Confidently wrong context is worse +/// than none, so the default is to never let the graph fall behind. +/// +/// Never fatal: the graph is an optimisation, and a review must still run +/// without it. +fn sync_graph(project_root: &Path, quiet: bool) { + let Ok(mut graph) = Graph::open(project_root) else { + return; + }; + // Only the first build is slow enough to be worth a spinner. + let spinner = (graph.is_empty() && !quiet) + .then(|| make_spinner("Building code graph (first run)...", false)); + + let progress = |n: usize| { + if let Some(s) = &spinner { + s.set_message(format!("Building code graph... {n} files")); + } + }; + if let Err(e) = graph.index(project_root, &progress) { + eprintln!(" ! code graph not refreshed: {e}"); + } + if let Some(s) = spinner { + s.finish_and_clear(); + } + runs::ensure_gitignore(project_root); +} + /// Merge units the code graph says are two halves of one change. Without a /// graph this is the identity function, so behaviour is unchanged. pub fn unit_grouper(project_root: &Path) -> core_engine::analyzer::UnitGrouper { diff --git a/apps/tui-cli/src/settings.rs b/apps/tui-cli/src/settings.rs index b43fe87..ea98533 100644 --- a/apps/tui-cli/src/settings.rs +++ b/apps/tui-cli/src/settings.rs @@ -71,6 +71,8 @@ pub struct Settings { pub debug: bool, /// Extra globs dropped by the pre-filter, from `.diffmind/config.toml`. pub ignore_globs: Vec, + /// Refresh the code graph before reviewing. + pub auto_index: bool, } pub fn resolve_settings(cli: &Cli, file: &FileConfig) -> Result { @@ -135,6 +137,7 @@ pub fn resolve_settings(cli: &Cli, file: &FileConfig) -> Result { // *supposed* to differ, and replaying one would be a lie. use_cache: !cli.no_cache && resolve(None, r.cache, true) && temperature == 0.0, ignore_globs: r.ignore.clone().unwrap_or_default(), + auto_index: !cli.no_index && resolve(None, r.auto_index, true), use_baseline: !cli.no_baseline, use_daemon: !cli.no_daemon, debug: cli.debug, From 888a12291a33f726eb5ef01fd631d3f1bf644eb1 Mon Sep 17 00:00:00 2001 From: Dennis Paler Date: Wed, 12 Aug 2026 16:49:51 +0800 Subject: [PATCH 2/3] fix: correctness, security and performance pass over the review pipeline --- CHANGELOG.md | 152 +++++++- README.md | 16 +- apps/tui-cli/src/daemon.rs | 86 ++++- apps/tui-cli/src/download.rs | 485 ++++++++++++++++-------- apps/tui-cli/src/graph/link.rs | 20 +- apps/tui-cli/src/graph/store.rs | 290 +++++++++++--- apps/tui-cli/src/main.rs | 66 +++- apps/tui-cli/src/rag.rs | 128 ++++++- apps/tui-cli/src/tui.rs | 53 +-- apps/tui-cli/tests/stdin_pipeline.rs | 485 ++++++++++++++++++++++++ packages/core-engine/src/analyzer.rs | 149 +++++++- packages/core-engine/src/detectors.rs | 111 +++++- packages/core-engine/src/diff.rs | 216 +++++++++-- packages/core-engine/src/json_guard.rs | 57 ++- packages/core-engine/src/prefilter.rs | 108 +++++- packages/core-engine/src/suppression.rs | 93 ++++- packages/core-engine/src/unit.rs | 51 ++- 17 files changed, 2227 insertions(+), 339 deletions(-) create mode 100644 apps/tui-cli/tests/stdin_pipeline.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 35a0c07..0a0b5e2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,156 @@ # Changelog -## 0.9.0 — the reviewer's cockpit +## [0.9.0] — 08-12-2026 + +### Security + +- **Model weights are now pinned and verified.** Downloads used HuggingFace's + `resolve/main/…`, a moving ref, and the only checksum on record was whatever + digest the bytes that arrived happened to produce — so `--verify` could prove a + file had not rotted on disk and never that it was the right file. Each entry + now carries an immutable commit sha plus the expected SHA-256 and byte count, + checked before the download is moved into place; a mismatch refuses to install + rather than warning. The tokenizer is pinned the same way, since one that + disagrees with the model shifts every token id without failing loudly. + A model left behind by a build pinning a different revision is treated as + absent and re-fetched instead of loaded. + + This closes a real gap in the reproducibility claim: the same model id could + previously mean different bytes on two machines, or on one machine a month + apart. The per-download `.receipt.json` sidecars are gone — the expectation + ships in the binary, so the "no checksum on record" state no longer exists. + Existing downloads are verified against the new pins on next use; the current + files match, so no re-download is expected. + +### Fixed + +- **A non-ASCII path in a diff header could abort the process.** The + `diff --git a/… b/…` parser sliced the line at a byte midpoint without a + character-boundary check, so a rename involving a non-ASCII filename — routine + with `core.quotePath=false`, or in any diff piped to `--stdin` — panicked + instead of reviewing. The same arithmetic was one byte off, so the branch had + never matched anything and every correct answer came from the fallback below + it. Now fixed rather than removed: a `diff --git` line resolves on its own, + which matters for binary files and mode changes, where no `+++ b/…` follows to + correct it. + +- **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 + `char::is_alphanumeric` is Unicode-aware — landed mid-codepoint and panicked + the process on the next search. The same byte arithmetic also read a UTF-8 + continuation byte as a word boundary, so `élan` looked like a standalone + reference inside `béélan`. Both now work in characters. + +- **A diff with no `diff --git` lines was parsed as one merged file.** Piping + `diff -u`, `git format-patch` or a review tool's output to `--stdin` gives no + `diff --git` line, and nothing else closed the previous file — so the second + file's `+++` merely *renamed* the first, every hunk in the diff collapsed under + the last path seen, and the pre-filter dropped the paths entirely (which also + meant lockfiles and generated files in a piped diff were never filtered). The + `--- old` / `+++ new` / `@@` sequence is now recognised as a file boundary in + the parser, the pre-filter and unit splitting alike. + + Relatedly, `--- ` and `+++ ` were treated as path lines even *inside* a hunk, + where they are content: a removed `-- note` in SQL, Lua or Haskell renders as + `--- note`, so it was dropped from the hunk and its `+++` twin renamed the file + to the comment's own text. Path lines are now only read outside a hunk. + +- **A suppression comment with a written reason silently stopped suppressing.** + `// diffmind-ignore false positive, validated upstream` parsed every word of + the reason as a rule ID, matched nothing, and let the finding straight + through. Rule IDs are now read up to the first word that is not shaped like + one, and the rest is treated as prose, so a bare reason means "all rules" and + `// diffmind-ignore DM002 -- the caller restores it` still scopes to `DM002`. + A `:` after the marker is punctuation. If you use single-lowercase-word IDs in + `rules.toml` (`id = "todo"`), prefix or hyphenate them — such a word now reads + as prose, which over-suppresses on that line rather than under-suppressing. + +- **The TUI's evidence pane was empty for cross-file findings.** It planned the + review units itself to show the hunk behind a finding, but that plan ran + before unit grouping and triage, both of which change which units exist and + therefore what IDs findings carry — so a merged unit, exactly the cross-file + change the code graph exists to link, resolved to nothing. Units now come from + the analyzer as it reviews them, so the pane cannot disagree with the run. + +- **The blast radius collapsed to one caller.** `callers_of` limited reference + rows and deduplicated afterwards, so a function calling a changed symbol + several times consumed the entire budget: three callers of six calls each, + asking for three, returned one. Review context now sees the callers it was + meant to. Results are also ordered — tightest enclosing scope first — where an + unordered `LIMIT` previously returned whatever the query planner chose, so the + same repository could produce different context from run to run. + +- **A brace in the model's preamble threw away the whole unit.** JSON extraction + tried the first `{` or `[` in the output and gave up if it was not valid, + rather than looking further along — so `Looking at the diff {specifically + auth.rs}, here is the review: {…}` yielded nothing and the unit was counted + unparseable. A rejected opener is now skipped; only a genuinely unterminated + one stops the scan, since anything after it is nested inside it. Affects the + remote backends (Ollama, OpenAI-compatible), whose decoding cannot be + constrained and which are therefore the likeliest to add a preamble. + +- **`--seed` and `--temperature` were ignored whenever a daemon was running.** + Neither was part of the daemon protocol, so a served review used whatever + sampling `diffmind serve` had been started with, and the same diff could + produce different findings depending on whether a daemon happened to be up. + 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. + +### Performance + +All three of these ran before a single token was generated, so they were paid on +every review — including one that turned out to be fully cached. + +- **The code graph is no longer queried per line.** Deciding whether two units + were related asked the graph for the enclosing symbol of every line in a + unit's span, which for a 1500-line unit was 1500 queries. One query for the + span, resolved in memory: **17x faster** on that unit (122ms → 7ms), with a + byte-identical result. Still the innermost declaration per line — taking every + overlapping one would have been cheaper again, but it would make any two units + inside the same module look related. + +- **Referenced symbols resolve in one batch instead of one query per word.** + Context assembly asked the graph about every distinct word in the diff as it + encountered it, then looked the survivors up a second time. On a diff with + ~1600 candidate words that was ~1600 queries; it is now a handful of batched + ones, **9.7x faster**, resolving the same definitions. + +- **Context is assembled once per unit, not twice.** The TUI asks for a unit's + context when the unit starts, and the analyzer asks again when it builds that + unit's prompt. The builder now memoises on the chunk text, so the second ask + costs nothing (3.4ms → 9µs). Keyed by the text itself rather than a hash: a + 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 diff --git a/README.md b/README.md index 17c4b29..d64d96a 100644 --- a/README.md +++ b/README.md @@ -360,15 +360,23 @@ is opt-in. Models download to `~/.diffmind/models/` — all Qwen2.5-Coder, Q4_K_M quantised. Inference runs on the Apple Silicon GPU via Metal where available, and on CPU -(with Accelerate/BLAS) everywhere else. Downloads are atomic and checksummed, so -an interrupted download cannot leave a truncated file that looks valid. +(with Accelerate/BLAS) everywhere else. + +Every download is **pinned to an immutable commit and verified against a SHA-256 +that ships in the binary**, before the file is moved into place. Writes are +atomic, so an interrupted download cannot leave a truncated file that looks +valid — and weights that are not byte-for-byte what this build expects are +refused rather than loaded. `diffmind download --verify` re-checks an existing +download against the same pins. This is what makes "the same diff produces the +same review" true across machines rather than merely likely: a model id names +exact bytes, not whatever a branch points at today. | Model | Size | Min RAM | Use for | | -------------------- | ------- | ------- | ----------------------------------- | -| Qwen2.5-Coder-0.5B | 0.4 GB | 2 GB | CI on small runners, lint-grade | +| Qwen2.5-Coder-0.5B | 0.5 GB | 2 GB | CI on small runners, lint-grade | | Qwen2.5-Coder-1.5B ★ | 1.1 GB | 4 GB | Default — balanced | | Qwen2.5-Coder-7B | 4.7 GB | 10 GB | Noticeably better security analysis | -| Qwen2.5-Coder-32B | 20.0 GB | 40 GB | Maximum, if you have the machine | +| Qwen2.5-Coder-32B | 19.9 GB | 40 GB | Maximum, if you have the machine | `3b` and `14b` also exist. `diffmind download` gives an interactive picker with a hardware check; `--model 1.5b --verify` checks an existing download. diff --git a/apps/tui-cli/src/daemon.rs b/apps/tui-cli/src/daemon.rs index eaaed80..dbe0ab0 100644 --- a/apps/tui-cli/src/daemon.rs +++ b/apps/tui-cli/src/daemon.rs @@ -81,14 +81,30 @@ pub enum Request { Review(Box), } +/// Everything the client can vary per invocation. +/// +/// A field missing here is not a missing feature — it is a flag that silently +/// does nothing the moment a daemon is running, because the daemon falls back +/// to whatever `diffmind serve` was started with. Nothing about the field's +/// absence looks wrong from either side. +/// +/// Note the deliberate lack of `#[serde(default)]` on the scalars: a default +/// would let a renamed or dropped field arrive as `0`, which for `seed` is a +/// perfectly plausible value and would quietly change every result. `read_info` +/// already refuses a daemon from another build, so a hard parse error is the +/// better failure. #[derive(Debug, Serialize, Deserialize)] pub struct ReviewRequest { pub diff: String, - pub languages: Vec, pub requirements: Option, pub max_tokens: u32, pub min_confidence: f32, pub triage: String, + /// Sampling, which decides whether two runs of the same diff agree. The + /// daemon must not substitute its own: reproducibility is the property the + /// whole gate rests on, and `--seed` silently ignored is worse than absent. + pub temperature: f64, + pub seed: u64, pub rules: Vec, /// Prose rule sets, sent rather than re-read: the daemon may be serving a /// different repository than the one it was started in. @@ -571,11 +587,12 @@ mod tests { let response = client .review(ReviewRequest { diff: "d".into(), - languages: vec![], requirements: None, max_tokens: 128, min_confidence: 0.0, triage: "off".into(), + temperature: 0.0, + seed: core_engine::DEFAULT_SEED, rules: vec![], rulebooks: vec![], baseline: None, @@ -637,11 +654,12 @@ mod tests { let response = client .review(ReviewRequest { diff: "d".into(), - languages: vec![], requirements: None, max_tokens: 128, min_confidence: 0.0, triage: "off".into(), + temperature: 0.0, + seed: core_engine::DEFAULT_SEED, rules: vec![], rulebooks: vec![core_engine::Rulebook { id: "api".into(), @@ -671,6 +689,65 @@ mod tests { handle.join().unwrap(); } + /// `--seed` and `--temperature` used to be absent from the protocol + /// entirely, so a daemon reviewed with whatever sampling `diffmind serve` + /// was started with. Two runs of the same diff could then disagree for no + /// visible reason — and reproducibility is the property the gate rests on. + #[test] + fn sampling_survives_the_wire() { + let server = Server::bind(0, Duration::from_secs(30)).unwrap(); + let port = server.port().unwrap(); + let token = server.token().to_string(); + + let handle = std::thread::spawn(move || { + server + .run(|req| { + // Echo what the handler actually received, so the assertion + // sees the deserialized request rather than the sent one. + Response::Ok(Box::new(ReviewResponse { + summary: ReviewSummary { + positives: vec![format!("{}|{}", req.temperature, req.seed)], + ..Default::default() + }, + stats: SerializableStats::default(), + backend: "test".into(), + })) + }) + .unwrap(); + }); + + let client = Client { + info: info(port, &token), + }; + // Deliberately not the defaults — a dropped field would read as 0.0/0 + // or as the daemon's own values, and both would pass a laxer assertion. + let response = client + .review(ReviewRequest { + diff: "d".into(), + requirements: None, + max_tokens: 128, + min_confidence: 0.0, + triage: "off".into(), + temperature: 0.7, + seed: 424_242, + rules: vec![], + rulebooks: vec![], + baseline: None, + use_cache: false, + project_root: ".".into(), + }) + .expect("review should round-trip"); + + assert_eq!( + response.summary.positives, + vec!["0.7|424242"], + "sampling must reach the daemon, or --seed silently does nothing" + ); + + client.shutdown().unwrap(); + handle.join().unwrap(); + } + #[test] fn a_bad_token_is_refused_and_never_reaches_the_handler() { // A short idle timeout so the server winds itself down promptly. @@ -698,11 +775,12 @@ mod tests { let err = attacker .review(ReviewRequest { diff: "secret code".into(), - languages: vec![], requirements: None, max_tokens: 16, min_confidence: 0.0, triage: "off".into(), + temperature: 0.0, + seed: core_engine::DEFAULT_SEED, rules: vec![], rulebooks: vec![], baseline: None, diff --git a/apps/tui-cli/src/download.rs b/apps/tui-cli/src/download.rs index e6d7e64..2f1c065 100644 --- a/apps/tui-cli/src/download.rs +++ b/apps/tui-cli/src/download.rs @@ -4,7 +4,7 @@ use sha2::{Digest, Sha256}; use std::cmp::Reverse; use std::fs; use std::io::{self, Read, Write}; -use std::path::{Path, PathBuf}; +use std::path::Path; // ─── Model catalog ──────────────────────────────────────────────────────────── @@ -13,16 +13,62 @@ pub struct ModelInfo { pub name: &'static str, /// One-line description shown in the picker pub description: &'static str, + /// HuggingFace repository, and the exact commit within it. Never a branch: + /// see [`ModelInfo::gguf_url`]. + pub repo: &'static str, + pub revision: &'static str, pub gguf_filename: &'static str, - pub gguf_url: &'static str, - /// Approximate compressed download size in GB - pub size_gb: f64, + /// SHA-256 of the file at `revision`, taken from the repository's Git LFS + /// object id. Checked before the download is moved into place, so weights + /// that are not byte-for-byte what this build expects never get loaded. + pub sha256: &'static str, + /// Exact size in bytes, so a truncated file is caught without hashing. + pub bytes: u64, /// Minimum total system RAM in GB (soft requirement — warns, does not block) pub min_ram_gb: u64, /// Minimum free disk space in GB (warns before download) pub min_disk_gb: u64, } +impl ModelInfo { + /// Download URL, pinned to an immutable commit. + /// + /// This used to be `resolve/main/…`. A branch is a moving target: the same + /// `diffmind download` a month apart could fetch different weights, and the + /// same model id could mean different things on two developers' machines — + /// which quietly undoes the reproducibility the whole gate is built on. A + /// commit sha cannot move, and `sha256` proves what arrived. + pub fn gguf_url(&self) -> String { + format!( + "https://huggingface.co/{}/resolve/{}/{}", + self.repo, self.revision, self.gguf_filename + ) + } + + /// Download size in GB, derived from `bytes` rather than stated separately — + /// a hand-maintained second copy is a number that drifts. + /// + /// Decimal GB, not GiB: this is a download size, and it should agree with + /// what HuggingFace shows next to the file the user is about to fetch. + pub fn size_gb(&self) -> f64 { + self.bytes as f64 / 1_000_000_000.0 + } + + fn expected(&self) -> Expected<'_> { + Expected { + sha256: self.sha256, + bytes: self.bytes, + } + } +} + +/// What a downloaded file must turn out to be. +#[derive(Clone, Copy)] +struct Expected<'a> { + sha256: &'a str, + bytes: u64, +} + /// All supported Qwen2.5-Coder models (Q4_K_M quantisation). /// Coding-optimised only — no generic Qwen chat models. /// @@ -30,14 +76,29 @@ pub struct ModelInfo { /// overhead. It used to assume the file was the whole cost, which was /// optimistic by roughly a factor of two because the CLI read the entire GGUF /// onto the heap before candle copied out of it; the engine now memory-maps it. +/// +/// ## Updating an entry, or adding a model +/// +/// `revision`, `sha256` and `bytes` must be taken together from one commit, or +/// the download will refuse the file it fetched. All three come from the +/// HuggingFace API without downloading anything — the LFS object id *is* the +/// content SHA-256: +/// +/// ```text +/// curl -s https://huggingface.co/api/models//revision/main | jq -r .sha +/// curl -s 'https://huggingface.co/api/models//tree/main?recursive=true' \ +/// | jq -r '.[] | select(.path=="") | "\(.lfs.oid) \(.lfs.size)"' +/// ``` pub const MODELS: &[ModelInfo] = &[ ModelInfo { id: "0.5b", name: "Qwen2.5-Coder-0.5B", description: "Fastest — lint-style checks, great for CI or low-end hardware", + repo: "Qwen/Qwen2.5-Coder-0.5B-Instruct-GGUF", + revision: "ebb2015119c907b064c512bf053e945850b5875f", gguf_filename: "qwen2.5-coder-0.5b-instruct-q4_k_m.gguf", - gguf_url: "https://huggingface.co/Qwen/Qwen2.5-Coder-0.5B-Instruct-GGUF/resolve/main/qwen2.5-coder-0.5b-instruct-q4_k_m.gguf", - size_gb: 0.4, + sha256: "1d9614638d18024d0fbb36575a15f1302a3adf044df10345688ec4f6e1c4ff32", + bytes: 491_400_064, min_ram_gb: 2, min_disk_gb: 1, }, @@ -45,9 +106,11 @@ pub const MODELS: &[ModelInfo] = &[ id: "1.5b", name: "Qwen2.5-Coder-1.5B", description: "Recommended — balanced quality and speed for most developers", + repo: "Qwen/Qwen2.5-Coder-1.5B-Instruct-GGUF", + revision: "f86cb2c1fa58255f8052cc32aeede1b7482d4361", gguf_filename: "qwen2.5-coder-1.5b-instruct-q4_k_m.gguf", - gguf_url: "https://huggingface.co/Qwen/Qwen2.5-Coder-1.5B-Instruct-GGUF/resolve/main/qwen2.5-coder-1.5b-instruct-q4_k_m.gguf", - size_gb: 1.1, + sha256: "cc324af070c2ecbfd324a30884d2f951a7ff756aba85cb811a6ec436933bb046", + bytes: 1_117_320_768, min_ram_gb: 4, min_disk_gb: 2, }, @@ -55,9 +118,11 @@ pub const MODELS: &[ModelInfo] = &[ id: "3b", name: "Qwen2.5-Coder-3B", description: "Better — deeper reasoning, handles complex codebases well", + repo: "Qwen/Qwen2.5-Coder-3B-Instruct-GGUF", + revision: "f74adce6aa16316c625447af059dbebe4983757c", gguf_filename: "qwen2.5-coder-3b-instruct-q4_k_m.gguf", - gguf_url: "https://huggingface.co/Qwen/Qwen2.5-Coder-3B-Instruct-GGUF/resolve/main/qwen2.5-coder-3b-instruct-q4_k_m.gguf", - size_gb: 2.1, + sha256: "724fb256bec1ff062b2f65e4569e871ad2e95ab2a3989723d1769c54294730b7", + bytes: 2_104_932_800, min_ram_gb: 6, min_disk_gb: 3, }, @@ -65,9 +130,11 @@ pub const MODELS: &[ModelInfo] = &[ id: "7b", name: "Qwen2.5-Coder-7B", description: "High quality — strong security analysis", + repo: "Qwen/Qwen2.5-Coder-7B-Instruct-GGUF", + revision: "13fb94bfda8c8cf22497dc57b78f391a9acb426a", gguf_filename: "qwen2.5-coder-7b-instruct-q4_k_m.gguf", - gguf_url: "https://huggingface.co/Qwen/Qwen2.5-Coder-7B-Instruct-GGUF/resolve/main/qwen2.5-coder-7b-instruct-q4_k_m.gguf", - size_gb: 4.7, + sha256: "509287f78cb4d4cf6b3843734733b914b2c158e43e22a7f4bf5e963800894d3c", + bytes: 4_683_073_536, min_ram_gb: 10, min_disk_gb: 6, }, @@ -75,9 +142,11 @@ pub const MODELS: &[ModelInfo] = &[ id: "14b", name: "Qwen2.5-Coder-14B", description: "Expert — deep code understanding, workstation recommended", + repo: "Qwen/Qwen2.5-Coder-14B-Instruct-GGUF", + revision: "d0a692ef765eefbf2fabb130b3cb2e8917e3d225", gguf_filename: "qwen2.5-coder-14b-instruct-q4_k_m.gguf", - gguf_url: "https://huggingface.co/Qwen/Qwen2.5-Coder-14B-Instruct-GGUF/resolve/main/qwen2.5-coder-14b-instruct-q4_k_m.gguf", - size_gb: 9.0, + sha256: "c1e659736d89ac1065fb495330fb824d94001974a4bfa78e7270e43476a8d940", + bytes: 8_988_110_272, min_ram_gb: 18, min_disk_gb: 11, }, @@ -85,17 +154,35 @@ pub const MODELS: &[ModelInfo] = &[ id: "32b", name: "Qwen2.5-Coder-32B", description: "Maximum — near human-level review quality, server-grade hardware", + repo: "Qwen/Qwen2.5-Coder-32B-Instruct-GGUF", + revision: "9d3053fce650fe1cdbdb75998c2a87add9d178ef", gguf_filename: "qwen2.5-coder-32b-instruct-q4_k_m.gguf", - gguf_url: "https://huggingface.co/Qwen/Qwen2.5-Coder-32B-Instruct-GGUF/resolve/main/qwen2.5-coder-32b-instruct-q4_k_m.gguf", - size_gb: 20.0, + sha256: "4d64b316b5e6319d9613e0d97935d9ebd631fc7e334da400d00085eca749d085", + bytes: 19_851_335_872, min_ram_gb: 40, min_disk_gb: 22, }, ]; -// Shared tokenizer for all Qwen2.5-Coder variants -const TOKENIZER_URL: &str = - "https://huggingface.co/Qwen/Qwen2.5-Coder-1.5B-Instruct/resolve/main/tokenizer.json"; +// Shared tokenizer for all Qwen2.5-Coder variants. Pinned and verified for the +// same reason the weights are: a tokenizer that disagrees with the model shifts +// every token id, which does not fail loudly — it just reviews badly. +// +// Note for whoever updates this: the `.lfs.oid` recipe above does **not** apply. +// At 7 MB this file is below HuggingFace's LFS threshold, so it is a plain Git +// blob — the API reports a sha1 `oid`, and `X-Linked-ETag` on the download is +// that same sha1. The sha256 below was obtained the only way available, by +// fetching the file and hashing it: +// +// ```text +// curl -sL | shasum -a 256 +// ``` +pub const TOKENIZER_FILENAME: &str = "tokenizer.json"; +const TOKENIZER_URL: &str = "https://huggingface.co/Qwen/Qwen2.5-Coder-1.5B-Instruct/resolve/2e1fd397ee46e1388853d2af2c993145b0f1098a/tokenizer.json"; +const TOKENIZER_EXPECTED: Expected<'static> = Expected { + sha256: "c0382117ea329cdf097041132f6d735924b697924d6f6fc3945713e96ce87539", + bytes: 7_031_645, +}; /// Look up a model by its short ID (e.g. "1.5b", "7b"). pub fn find_model(id: &str) -> Option<&'static ModelInfo> { @@ -106,20 +193,6 @@ pub fn model_ids() -> Vec<&'static str> { MODELS.iter().map(|m| m.id).collect() } -/// Sidecar recording what was downloaded, so a later run can tell a complete -/// file from a truncated one without re-hashing gigabytes every startup. -#[derive(serde::Serialize, serde::Deserialize)] -struct ModelReceipt { - filename: String, - bytes: u64, - sha256: String, - url: String, -} - -fn receipt_path(model_dir: &Path, filename: &str) -> PathBuf { - model_dir.join(format!("{filename}.receipt.json")) -} - // ─── Interactive model picker ───────────────────────────────────────────────── fn prompt_model_selection() -> Result<&'static ModelInfo> { @@ -137,7 +210,7 @@ fn prompt_model_selection() -> Result<&'static ModelInfo> { i + 1, marker, m.name, - m.size_gb, + m.size_gb(), m.min_ram_gb, m.description ); @@ -272,7 +345,12 @@ fn http_client() -> Result { /// an inscrutable GGUF parse error. Now: read errors propagate, bytes are /// counted against `Content-Length`, the file is hashed, and only a complete /// download is renamed into place. -pub fn download_file(url: &str, dest: &Path) -> Result<(u64, String)> { +/// +/// `expect` closes the remaining gap. Counting bytes proves the transfer +/// finished; only the digest proves the *right file* arrived. Without it the +/// only recorded hash was whatever the server happened to send, which could +/// confirm the file had not rotted on disk and nothing more. +fn download_file(url: &str, dest: &Path, expect: Expected<'_>) -> Result<(u64, String)> { let client = http_client()?; let mut response = client .get(url) @@ -353,7 +431,31 @@ pub fn download_file(url: &str, dest: &Path) -> Result<(u64, String)> { )); } - Ok((downloaded, format!("{:x}", hasher.finalize()))) + // Against the catalog, not against the server. `Content-Length` only + // says the transfer matched what this response promised. + if downloaded != expect.bytes { + return Err(anyhow::anyhow!( + "unexpected file size: got {downloaded} bytes, expected {}. \ + This is not the file diffmind pinned — check your network for a \ + proxy that rewrites downloads.", + expect.bytes + )); + } + + let digest = format!("{:x}", hasher.finalize()); + if !digest.eq_ignore_ascii_case(expect.sha256) { + return Err(anyhow::anyhow!( + "checksum mismatch — refusing to install these weights.\n\ + \x20 expected {}\n\ + \x20 got {digest}\n\ + The download was corrupted in transit or the file is not what \ + diffmind pinned. Re-run `diffmind download --force`; if it fails \ + again, report it rather than working around it.", + expect.sha256 + )); + } + + Ok((downloaded, digest)) })(); pb.finish_and_clear(); @@ -388,52 +490,40 @@ fn hash_file(path: &Path) -> Result<(u64, String)> { Ok((total, format!("{:x}", hasher.finalize()))) } -fn write_receipt(model_dir: &Path, filename: &str, bytes: u64, sha256: &str, url: &str) { - let receipt = ModelReceipt { - filename: filename.to_string(), - bytes, - sha256: sha256.to_string(), - url: url.to_string(), - }; - if let Ok(json) = serde_json::to_string_pretty(&receipt) { - let _ = fs::write(receipt_path(model_dir, filename), json); - } -} - -/// Verify a downloaded file against its receipt. -/// `Ok(None)` means there is no receipt to check against. -pub fn verify_file(model_dir: &Path, filename: &str) -> Result> { - let path = model_dir.join(filename); +/// Verify a file on disk against what the catalog pins. +/// +/// This used to compare against a sidecar receipt written at download time, +/// which made it a tautology: the receipt held whatever digest the server's +/// bytes produced, so the check could prove the file had not rotted on disk and +/// never that it was the right file. It also had a third state — "no checksum on +/// record" — which is gone, because the expectation now ships in the binary. +fn verify_file(path: &Path, expect: Expected<'_>) -> Result { if !path.exists() { - return Ok(Some(false)); - } - let receipt_file = receipt_path(model_dir, filename); - if !receipt_file.exists() { - return Ok(None); + return Ok(false); } - let raw = fs::read_to_string(&receipt_file)?; - let receipt: ModelReceipt = serde_json::from_str(&raw)?; - // Size is the cheap check and catches every truncation; only hash when it // passes, because hashing 20 GB on every startup is not acceptable. - let actual_len = fs::metadata(&path)?.len(); - if actual_len != receipt.bytes { - return Ok(Some(false)); + if fs::metadata(path)?.len() != expect.bytes { + return Ok(false); } - let (_, digest) = hash_file(&path)?; - Ok(Some(digest == receipt.sha256)) + let (_, digest) = hash_file(path)?; + Ok(digest.eq_ignore_ascii_case(expect.sha256)) } /// Quick integrity check run before loading: size only, so it costs nothing. -pub fn looks_truncated(model_dir: &Path, filename: &str) -> bool { - let path = model_dir.join(filename); - let receipt_file = receipt_path(model_dir, filename); - let (Ok(meta), Ok(raw)) = (fs::metadata(&path), fs::read_to_string(&receipt_file)) else { - return false; - }; - serde_json::from_str::(&raw) - .map(|r| meta.len() != r.bytes) - .unwrap_or(false) +/// +/// Catches both a truncated download and a file left behind by a build that +/// pinned a different revision — the latter would otherwise load happily and +/// review with weights nobody asked for. +fn wrong_size(path: &Path, expect: Expected<'_>) -> bool { + fs::metadata(path).is_ok_and(|m| m.len() != expect.bytes) +} + +/// Is the model on disk the one this build pins? Size only — cheap enough to +/// run before every load. +pub fn looks_truncated(model_dir: &Path, model: &ModelInfo) -> bool { + wrong_size(&model_dir.join(model.gguf_filename), model.expected()) + || wrong_size(&model_dir.join(TOKENIZER_FILENAME), TOKENIZER_EXPECTED) } // ─── Public entry point ─────────────────────────────────────────────────────── @@ -461,56 +551,48 @@ pub fn ensure_model_files(model_id: Option<&str>, model_dir: &Path, force: bool) check_requirements(model, model_dir, force)?; let model_path = model_dir.join(model.gguf_filename); - let tokenizer_path = model_dir.join("tokenizer.json"); + let tokenizer_path = model_dir.join(TOKENIZER_FILENAME); if force { for p in [&model_path, &tokenizer_path] { let _ = fs::remove_file(p); } - let _ = fs::remove_file(receipt_path(model_dir, model.gguf_filename)); - let _ = fs::remove_file(receipt_path(model_dir, "tokenizer.json")); println!("Existing files removed. Re-downloading...\n"); } - // Tokenizer (shared across all models) - if tokenizer_path.exists() && !looks_truncated(model_dir, "tokenizer.json") { - println!("tokenizer.json already present (use --force to re-download)."); - } else { - println!("Downloading tokenizer.json..."); - let (bytes, digest) = download_file(TOKENIZER_URL, &tokenizer_path)?; - write_receipt(model_dir, "tokenizer.json", bytes, &digest, TOKENIZER_URL); - } - - // Model weights - if model_path.exists() && !looks_truncated(model_dir, model.gguf_filename) { - println!( - "{} already present (use --force to re-download).", - model.gguf_filename - ); - } else { - if model_path.exists() { - println!( - "Existing {} is incomplete — re-downloading.", - model.gguf_filename - ); - let _ = fs::remove_file(&model_path); + // Each file is fetched only if it is absent or is not what this build pins. + // A file left by a build pinning a different revision counts as absent: + // loading it would review with weights nobody chose. + let fetch = |what: &str, url: &str, path: &Path, expect: Expected<'_>| -> Result<()> { + if path.exists() && !wrong_size(path, expect) { + println!("{what} already present (use --force to re-download)."); + return Ok(()); } - println!( - "Downloading {} ({:.1} GB)...", - model.gguf_filename, model.size_gb - ); - let (bytes, digest) = download_file(model.gguf_url, &model_path)?; - write_receipt( - model_dir, - model.gguf_filename, - bytes, - &digest, - model.gguf_url, - ); - println!("\nModel ready: {}", model_path.display()); - println!(" sha256 {digest}"); - } + if path.exists() { + println!("Existing {what} is not the pinned file — re-downloading."); + let _ = fs::remove_file(path); + } + println!("Downloading {what}..."); + let (_, digest) = download_file(url, path, expect)?; + println!(" sha256 {digest} (verified)"); + Ok(()) + }; + // Tokenizer first: it is small, and a failure there is cheap to discover. + fetch( + TOKENIZER_FILENAME, + TOKENIZER_URL, + &tokenizer_path, + TOKENIZER_EXPECTED, + )?; + fetch( + &format!("{} ({:.1} GB)", model.gguf_filename, model.size_gb()), + &model.gguf_url(), + &model_path, + model.expected(), + )?; + + println!("\nModel ready: {}", model_path.display()); Ok(()) } @@ -524,21 +606,22 @@ pub fn verify_model_files(model_id: &str, model_dir: &Path) -> Result { })?; let mut all_ok = true; - for filename in [model.gguf_filename, "tokenizer.json"] { + for (filename, expect) in [ + (model.gguf_filename, model.expected()), + (TOKENIZER_FILENAME, TOKENIZER_EXPECTED), + ] { print!(" {filename} ... "); io::stdout().flush()?; - match verify_file(model_dir, filename)? { - Some(true) => println!("ok"), - Some(false) => { - println!("CORRUPT or MISSING"); - all_ok = false; - } - None => println!("no checksum on record (downloaded by an older version)"), + if verify_file(&model_dir.join(filename), expect)? { + println!("ok"); + } else { + println!("MISSING, CORRUPT, or not the pinned revision"); + all_ok = false; } } if all_ok { - println!("\n All files verified."); + println!("\n All files match the checksums pinned in this build."); } else { println!("\n Re-download with: diffmind download --model {model_id} --force"); } @@ -553,24 +636,79 @@ mod tests { fn every_catalog_entry_is_self_consistent() { for m in MODELS { assert!( - m.gguf_url.ends_with(m.gguf_filename), - "{}: url and filename disagree, so the receipt would be written \ - against a file that was never fetched", + m.gguf_url().ends_with(m.gguf_filename), + "{}: url and filename disagree, so the download would verify a \ + file that was never fetched", m.id ); assert!( - m.min_disk_gb as f64 >= m.size_gb, + m.min_disk_gb as f64 >= m.size_gb(), "{}: disk floor below download size", m.id ); assert!( - m.min_ram_gb as f64 > m.size_gb, + m.min_ram_gb as f64 > m.size_gb(), "{}: RAM floor below weights", m.id ); } } + /// Every download is refused unless it hashes to the pinned value, so a + /// malformed entry here does not fail safe — it makes the model unusable. + /// Cheaper to catch in CI than in a bug report. + #[test] + fn every_pinned_digest_is_wellformed() { + let sha_ok = |s: &str| s.len() == 64 && s.chars().all(|c| c.is_ascii_hexdigit()); + let rev_ok = |s: &str| s.len() == 40 && s.chars().all(|c| c.is_ascii_hexdigit()); + + for m in MODELS { + assert!(sha_ok(m.sha256), "{}: sha256 is not 64 hex chars", m.id); + assert!( + rev_ok(m.revision), + "{}: revision must be a full commit sha, not `{}` — a branch or \ + tag can move, and then the pinned digest is simply wrong", + m.id, + m.revision + ); + assert!(m.bytes > 0, "{}: bytes must be the real file size", m.id); + } + assert!(sha_ok(TOKENIZER_EXPECTED.sha256)); + assert!(TOKENIZER_URL.contains("/resolve/")); + } + + /// A moving ref is the whole defect: `resolve/main/…` meant the same + /// `diffmind download` a month apart could fetch different weights, which no + /// checksum can protect against because there is nothing to compare to. + #[test] + fn no_download_url_points_at_a_branch() { + let urls: Vec = MODELS + .iter() + .map(|m| m.gguf_url()) + .chain([TOKENIZER_URL.to_string()]) + .collect(); + for url in urls { + assert!( + !url.contains("/resolve/main/") && !url.contains("/resolve/master/"), + "unpinned URL: {url}" + ); + assert!(url.starts_with("https://"), "insecure URL: {url}"); + } + } + + /// The displayed size comes from the pinned byte count rather than a + /// separately maintained number that drifts away from it. + #[test] + fn sizes_are_derived_from_the_pinned_byte_counts() { + let m = find_model("1.5b").unwrap(); + assert_eq!(m.bytes, 1_117_320_768); + assert!( + (m.size_gb() - 1.117).abs() < 0.001, + "decimal GB, to agree with what HuggingFace displays; got {}", + m.size_gb() + ); + } + #[test] fn model_ids_are_unique() { let mut ids = model_ids(); @@ -587,44 +725,83 @@ mod tests { assert!(find_model("99b").is_none()); } + fn tmpdir(name: &str) -> std::path::PathBuf { + let d = std::env::temp_dir().join(format!("diffmind-dl-{name}-{}", std::process::id())); + let _ = fs::remove_dir_all(&d); + fs::create_dir_all(&d).unwrap(); + d + } + + /// `b"12345"`, so a test can pin a real expectation. + fn known() -> Expected<'static> { + Expected { + sha256: "5994471abb01112afcc18159f6cc74b4f511b99806da59b3caf5a9c173cacfc5", + bytes: 5, + } + } + #[test] fn verify_reports_missing_files_as_failures() { - let dir = std::env::temp_dir().join(format!("diffmind-verify-{}", std::process::id())); - std::fs::create_dir_all(&dir).unwrap(); - assert_eq!(verify_file(&dir, "nope.gguf").unwrap(), Some(false)); - let _ = std::fs::remove_dir_all(&dir); + let dir = tmpdir("missing"); + assert!(!verify_file(&dir.join("nope.gguf"), known()).unwrap()); + let _ = fs::remove_dir_all(&dir); } #[test] fn a_size_mismatch_is_detected_without_hashing() { - let dir = std::env::temp_dir().join(format!("diffmind-trunc-{}", std::process::id())); - std::fs::create_dir_all(&dir).unwrap(); - std::fs::write(dir.join("m.gguf"), b"12345").unwrap(); - write_receipt(&dir, "m.gguf", 999, "deadbeef", "http://x"); + let dir = tmpdir("trunc"); + let path = dir.join("m.gguf"); + fs::write(&path, b"12345").unwrap(); - assert!(looks_truncated(&dir, "m.gguf")); - assert_eq!(verify_file(&dir, "m.gguf").unwrap(), Some(false)); + let wrong_len = Expected { + sha256: known().sha256, + bytes: 999, + }; + assert!(wrong_size(&path, wrong_len)); + assert!(!verify_file(&path, wrong_len).unwrap()); - // A correct receipt passes both checks. - let (bytes, digest) = hash_file(&dir.join("m.gguf")).unwrap(); - write_receipt(&dir, "m.gguf", bytes, &digest, "http://x"); - assert!(!looks_truncated(&dir, "m.gguf")); - assert_eq!(verify_file(&dir, "m.gguf").unwrap(), Some(true)); + assert!(!wrong_size(&path, known())); + assert!(verify_file(&path, known()).unwrap()); - let _ = std::fs::remove_dir_all(&dir); + let _ = fs::remove_dir_all(&dir); + } + + /// The check that the receipt could never make: right length, wrong bytes. + /// + /// Comparing against a digest recorded at download time was a tautology — it + /// held whatever the server sent. Only a digest that ships in the binary can + /// tell "the file I have" from "the file I asked for". + #[test] + fn a_file_of_the_right_length_but_the_wrong_content_is_rejected() { + let dir = tmpdir("swapped"); + let path = dir.join("m.gguf"); + fs::write(&path, b"54321").unwrap(); + + assert!( + !wrong_size(&path, known()), + "same length, so the cheap check cannot see it" + ); + assert!( + !verify_file(&path, known()).unwrap(), + "substituted content must be caught by the digest" + ); + let _ = fs::remove_dir_all(&dir); } + /// A file left behind by a build that pinned a different revision is not + /// "already present" — loading it would review with weights nobody chose. #[test] - fn missing_receipt_is_unknown_not_corrupt() { - let dir = std::env::temp_dir().join(format!("diffmind-noreceipt-{}", std::process::id())); - std::fs::create_dir_all(&dir).unwrap(); - std::fs::write(dir.join("m.gguf"), b"x").unwrap(); - assert_eq!( - verify_file(&dir, "m.gguf").unwrap(), - None, - "a model from an older version must not be reported as corrupt" + fn a_model_from_another_revision_is_not_accepted_as_present() { + let dir = tmpdir("stale-revision"); + let model = find_model("1.5b").unwrap(); + // Whatever an older build downloaded, at a plausible-looking size. + fs::write(dir.join(model.gguf_filename), vec![0u8; 1024]).unwrap(); + fs::write(dir.join(TOKENIZER_FILENAME), b"{}").unwrap(); + + assert!( + looks_truncated(&dir, model), + "the pre-load check must refuse a file that is not the pinned one" ); - assert!(!looks_truncated(&dir, "m.gguf")); - let _ = std::fs::remove_dir_all(&dir); + let _ = fs::remove_dir_all(&dir); } } diff --git a/apps/tui-cli/src/graph/link.rs b/apps/tui-cli/src/graph/link.rs index cf6c766..938c6a0 100644 --- a/apps/tui-cli/src/graph/link.rs +++ b/apps/tui-cli/src/graph/link.rs @@ -98,9 +98,25 @@ fn profile(unit: &ReviewUnit, graph: &Graph) -> Profile { let mut mentions = HashSet::new(); // Anything the graph knows is declared over the unit's changed span. + // + // One query for the span, then each line resolved against the result. This + // used to ask the graph per line, so a unit spanning 400 lines cost 400 + // queries — and this runs for every unit of every review, before a single + // token is generated. + // + // Still the *innermost* declaration per line, deliberately. Taking every + // overlapping declaration instead would be cheaper still, but it would add + // enclosing modules and impl blocks to `declares`, and then any two units + // inside `mod utils` would look related to each other. + let candidates = graph.declarations_overlapping(unit.file(), unit.new_start, unit.new_end); for line in unit.new_start..=unit.new_end { - if let Some(def) = graph.enclosing(unit.file(), line) { - declares.insert(def.name); + // `candidates` is ordered innermost-first, so the first hit is the one + // `enclosing` would have returned. + if let Some(def) = candidates + .iter() + .find(|d| d.start_line <= line && d.end_line >= line) + { + declares.insert(def.name.clone()); } } diff --git a/apps/tui-cli/src/graph/store.rs b/apps/tui-cli/src/graph/store.rs index ae15c1c..228655f 100644 --- a/apps/tui-cli/src/graph/store.rs +++ b/apps/tui-cli/src/graph/store.rs @@ -64,10 +64,6 @@ impl Def { let end = end.min(start + max_lines); Some(lines[start..end].join("\n")) } - - fn span(&self) -> u32 { - self.end_line.saturating_sub(self.start_line) - } } #[derive(Debug, Default, Clone, Copy)] @@ -243,19 +239,6 @@ impl Graph { Ok(stats) } - /// Definitions of `name`, preferring one in `near` when the name is - /// ambiguous — a local declaration is far likelier to be the referent. - pub fn definitions_of(&self, name: &str, near: Option<&str>, limit: usize) -> Vec { - let mut defs = self.query_defs( - "SELECT path, name, kind, start_line, end_line FROM defs WHERE name = ?1 LIMIT ?2", - params![name, limit as i64], - ); - if let Some(near) = near { - defs.sort_by_key(|d| d.path != near); - } - defs - } - /// The innermost definition whose body contains `line`. pub fn enclosing(&self, path: &str, line: u32) -> Option { self.query_defs( @@ -268,31 +251,86 @@ impl Graph { .next() } + /// Every definition in `path` overlapping `start..=end`, innermost first. + /// + /// For a caller that needs the enclosing symbol of *many* lines. Asking + /// [`Self::enclosing`] per line is one query per line, and a unit spanning + /// several hundred lines then costs several hundred queries before any + /// inference starts. One query, resolved in memory, answers the same + /// question — see `graph::link`. + pub fn declarations_overlapping(&self, path: &str, start: u32, end: u32) -> Vec { + self.query_defs( + "SELECT path, name, kind, start_line, end_line FROM defs \ + WHERE path = ?1 AND start_line <= ?3 AND end_line >= ?2 \ + ORDER BY (end_line - start_line), start_line, name", + params![path, start, end], + ) + } + + /// Every definition whose name is one of `names`. + /// + /// Ordered by `(name, path, start_line)`, so a caller picking one definition + /// per name gets the same answer on every run. Batched rather than one query + /// per name: `rag` derives candidate names from every word in the diff, which + /// is thousands of lookups on a large branch — and it does it per review + /// unit. + /// + /// Chunked so a diff with more distinct words than SQLite's bound-parameter + /// limit still works; in practice this is a single query. + pub fn definitions_of_names(&self, names: &[String]) -> Vec { + /// Comfortably below SQLite's `SQLITE_MAX_VARIABLE_NUMBER`, whose + /// historical value was 999. + const CHUNK: usize = 400; + + let mut out = Vec::new(); + for batch in names.chunks(CHUNK) { + let placeholders = vec!["?"; batch.len()].join(","); + out.extend(self.query_defs( + &format!( + "SELECT path, name, kind, start_line, end_line FROM defs \ + WHERE name IN ({placeholders}) ORDER BY name, path, start_line" + ), + rusqlite::params_from_iter(batch.iter()), + )); + } + out + } + /// The definitions that mention `name` — the reverse edge, and the reason /// this file exists. /// - /// A reference sitting inside nested declarations belongs to the innermost - /// one; SQLite's bare-column rule hands back the row matching `MIN`. + /// `limit` bounds **callers**, not references. Deduplicating in Rust after a + /// SQL `LIMIT` looked equivalent and was not: the limit fell on reference + /// rows, so one function calling a symbol three times consumed the whole + /// budget and every other caller — the ones a blast radius exists to + /// surface — was silently dropped. `rag.rs` asks for three. + /// + /// Two levels, therefore. The inner query resolves each reference to the + /// innermost declaration containing it (nested declarations both match the + /// join; SQLite's bare-column rule hands back the row matching `MIN`). + /// The outer collapses those to distinct callers before the limit applies. + /// + /// Ordered smallest-span first, then by location, so the most specific + /// caller leads and the result does not vary between identical runs — an + /// unordered `LIMIT` is whatever the query planner felt like returning. /// Self-references are excluded, or every symbol would appear to call itself. pub fn callers_of(&self, name: &str, limit: usize) -> Vec { - let mut callers = self.query_defs( - "SELECT d.path, d.name, d.kind, d.start_line, d.end_line, \ - MIN(d.end_line - d.start_line) \ - FROM refs r \ - JOIN defs d ON d.path = r.path AND r.line >= d.start_line AND r.line <= d.end_line \ - WHERE r.name = ?1 AND d.name != ?1 \ - GROUP BY r.rowid \ + self.query_defs( + "SELECT path, name, kind, start_line, end_line FROM ( \ + SELECT d.path AS path, d.name AS name, d.kind AS kind, \ + d.start_line AS start_line, d.end_line AS end_line, \ + MIN(d.end_line - d.start_line) \ + FROM refs r \ + JOIN defs d ON d.path = r.path \ + AND r.line >= d.start_line AND r.line <= d.end_line \ + WHERE r.name = ?1 AND d.name != ?1 \ + GROUP BY r.rowid \ + ) \ + GROUP BY path, name, kind, start_line, end_line \ + ORDER BY (end_line - start_line), path, start_line, name \ LIMIT ?2", params![name, limit as i64], - ); - // One caller referencing a symbol five times is still one caller. - callers.sort_by(|a, b| { - (&a.path, a.start_line, &a.name).cmp(&(&b.path, b.start_line, &b.name)) - }); - callers - .dedup_by(|a, b| a.path == b.path && a.start_line == b.start_line && a.name == b.name); - callers.sort_by_key(|d| d.span()); - callers + ) } pub fn counts(&self) -> (usize, usize, usize) { @@ -357,6 +395,13 @@ mod tests { g } + /// Definitions of a single name, for tests that only care whether the graph + /// knows a symbol. Production resolves names in batches — a query per name + /// is what made context assembly scale with the size of the diff. + fn defs_of(g: &Graph, name: &str) -> Vec { + g.definitions_of_names(&[name.to_string()]) + } + const LIB: &str = "\ pub fn validate_token(t: &str) -> bool { !t.is_empty() @@ -422,6 +467,77 @@ pub fn refresh(token: &str) -> bool { let _ = std::fs::remove_dir_all(&root); } + /// The limit counts callers, not references. + /// + /// It used to bound the reference *rows* and deduplicate afterwards, so a + /// function calling the symbol several times spent the whole budget on its + /// own. Three callers of six calls each, asking for three, returned one — + /// and `rag.rs` asks for three. The blast radius quietly collapsed to the + /// first caller the query planner happened to reach. + #[test] + fn a_chatty_caller_does_not_crowd_out_the_others() { + let root = project("crowding"); + write(&root, "src/lib.rs", "fn target() {}\n"); + for name in ["a", "m", "z"] { + let calls = " target();\n".repeat(6); + write( + &root, + &format!("src/{name}.rs"), + &format!("fn caller_{name}() {{\n{calls}}}\n"), + ); + } + let g = indexed(&root); + + let distinct = |k: usize| -> Vec { + g.callers_of("target", k) + .into_iter() + .map(|d| d.name) + .collect() + }; + assert_eq!( + distinct(3), + ["caller_a", "caller_m", "caller_z"], + "three callers exist and three were asked for" + ); + assert_eq!(distinct(2).len(), 2, "a smaller limit still yields callers"); + assert_eq!(distinct(1).len(), 1); + let _ = std::fs::remove_dir_all(&root); + } + + /// An unordered `LIMIT` returns whatever the planner chose, so the same + /// repository could yield different review context from run to run — and a + /// review that is not reproducible cannot be a gate. + #[test] + fn callers_come_back_in_a_stable_most_specific_first_order() { + let root = project("ordering"); + write(&root, "src/lib.rs", "fn target() {}\n"); + write( + &root, + "src/big.rs", + &format!( + "fn big() {{\n target();\n{}}}\n", + " let x = 1;\n".repeat(20) + ), + ); + write(&root, "src/small.rs", "fn small() { target(); }\n"); + let g = indexed(&root); + + let names = |n: usize| -> Vec { + g.callers_of("target", n) + .into_iter() + .map(|d| d.name) + .collect() + }; + assert_eq!(names(10), ["small", "big"], "tightest scope leads"); + assert_eq!(names(10), names(10), "repeated queries must agree"); + assert_eq!( + names(1), + ["small"], + "a limit must keep the most specific caller, not an arbitrary one" + ); + let _ = std::fs::remove_dir_all(&root); + } + #[test] fn the_innermost_definition_owns_a_reference() { let root = project("innermost"); @@ -480,11 +596,8 @@ pub fn refresh(token: &str) -> bool { write(&root, "src/a.rs", "pub fn after() {}\n"); g.index(&root, &|_| {}).unwrap(); - assert!( - g.definitions_of("before", None, 10).is_empty(), - "stale symbol survived" - ); - assert_eq!(g.definitions_of("after", None, 10).len(), 1); + assert!(defs_of(&g, "before").is_empty(), "stale symbol survived"); + assert_eq!(defs_of(&g, "after").len(), 1); let _ = std::fs::remove_dir_all(&root); } @@ -505,7 +618,7 @@ pub fn refresh(token: &str) -> bool { let mut g = Graph::open(&root).unwrap(); g.index(&root, &|_| {}).unwrap(); - let before = g.definitions_of("target", None, 1).remove(0); + let before = defs_of(&g, "target").remove(0); assert!(before.source(&root, 100).unwrap().contains("let secret")); // Someone adds imports at the top — utterly ordinary. @@ -516,7 +629,7 @@ pub fn refresh(token: &str) -> bool { "// added\n// added\n// added\n// added\n// added\npub fn target() {\n let secret = 1;\n}\n", ); - let stale = g.definitions_of("target", None, 1).remove(0); + let stale = defs_of(&g, "target").remove(0); assert!( !stale.source(&root, 100).unwrap().contains("let secret"), "this is the failure mode being guarded against" @@ -525,7 +638,7 @@ pub fn refresh(token: &str) -> bool { // Re-indexing is what makes it right again, and is cheap enough to do // before every review. g.index(&root, &|_| {}).unwrap(); - let fresh = g.definitions_of("target", None, 1).remove(0); + let fresh = defs_of(&g, "target").remove(0); assert_eq!(fresh.start_line, 6); assert!(fresh.source(&root, 100).unwrap().contains("let secret")); @@ -538,29 +651,98 @@ pub fn refresh(token: &str) -> bool { write(&root, "src/gone.rs", "pub fn vanishing() {}\n"); let mut g = Graph::open(&root).unwrap(); g.index(&root, &|_| {}).unwrap(); - assert_eq!(g.definitions_of("vanishing", None, 10).len(), 1); + assert_eq!(defs_of(&g, "vanishing").len(), 1); std::fs::remove_file(root.join("src/gone.rs")).unwrap(); let stats = g.index(&root, &|_| {}).unwrap(); assert_eq!(stats.files_removed, 1); assert!( - g.definitions_of("vanishing", None, 10).is_empty(), + defs_of(&g, "vanishing").is_empty(), "a deleted file must stop answering queries" ); let _ = std::fs::remove_dir_all(&root); } + /// An ambiguous name keeps every candidate, in a fixed order, so the caller + /// choosing between them (`rag`, which prefers a definition in the file the + /// reference was seen in) gets the same answer on every run. #[test] - fn a_local_definition_is_preferred_when_a_name_is_ambiguous() { + fn an_ambiguous_name_returns_every_candidate_in_a_stable_order() { let root = project("ambiguous"); - write(&root, "src/a.rs", "pub struct Config { a: u32 }\n"); write(&root, "src/b.rs", "pub struct Config { b: u32 }\n"); + write(&root, "src/a.rs", "pub struct Config { a: u32 }\n"); let g = indexed(&root); - let defs = g.definitions_of("Config", Some("src/b.rs"), 10); + let defs = defs_of(&g, "Config"); assert_eq!(defs.len(), 2, "both are real and both are kept"); - assert_eq!(defs[0].path, "src/b.rs", "the local one comes first"); + let paths: Vec<&str> = defs.iter().map(|d| d.path.as_str()).collect(); + assert_eq!( + paths, + ["src/a.rs", "src/b.rs"], + "ordered by path regardless of which file was indexed first" + ); + let _ = std::fs::remove_dir_all(&root); + } + + /// Several names in one round trip, which is how `rag` resolves the words it + /// harvests from a diff. Names the graph does not know simply do not appear. + #[test] + fn many_names_resolve_in_one_query() { + let root = project("batch"); + write( + &root, + "src/a.rs", + "pub fn alpha() {}\npub fn beta() {}\npub fn gamma() {}\n", + ); + let g = indexed(&root); + + let names: Vec = ["gamma", "alpha", "nonexistent"] + .iter() + .map(|s| s.to_string()) + .collect(); + let resolved = g.definitions_of_names(&names); + let found: Vec<&str> = resolved.iter().map(|d| d.name.as_str()).collect(); + assert_eq!( + found, + ["alpha", "gamma"], + "ordered by name, unknowns absent" + ); + assert!(g.definitions_of_names(&[]).is_empty()); + let _ = std::fs::remove_dir_all(&root); + } + + /// The query the per-line `enclosing` loop was replaced with. + #[test] + fn declarations_overlapping_a_span_come_back_innermost_first() { + let root = project("overlapping"); + write( + &root, + "src/a.rs", + "mod outer {\n pub fn inner() {\n let x = 1;\n }\n}\nfn after() {}\n", + ); + let g = indexed(&root); + + let overlapping = g.declarations_overlapping("src/a.rs", 2, 3); + let names: Vec<&str> = overlapping.iter().map(|d| d.name.as_str()).collect(); + assert_eq!( + names, + ["inner", "outer"], + "both contain the span; the tighter one leads" + ); + + // The same answer `enclosing` gives, for every line in the span. + for line in 2..=3 { + assert_eq!( + g.enclosing("src/a.rs", line).map(|d| d.name), + Some("inner".into()), + "line {line}" + ); + } + assert!( + g.declarations_overlapping("src/a.rs", 90, 99).is_empty(), + "a span past the end of the file declares nothing" + ); let _ = std::fs::remove_dir_all(&root); } @@ -569,7 +751,7 @@ pub fn refresh(token: &str) -> bool { let root = project("source"); write(&root, "src/a.rs", "pub fn f() {\n let x = 1;\n}\n"); let g = indexed(&root); - let def = g.definitions_of("f", None, 1).remove(0); + let def = defs_of(&g, "f").remove(0); assert!(def.source(&root, 100).unwrap().contains("let x = 1;")); @@ -586,7 +768,7 @@ pub fn refresh(token: &str) -> bool { let body = format!("pub fn big() {{\n{}}}\n", " let x = 1;\n".repeat(500)); write(&root, "src/a.rs", &body); let g = indexed(&root); - let def = g.definitions_of("big", None, 1).remove(0); + let def = defs_of(&g, "big").remove(0); assert_eq!(def.source(&root, 10).unwrap().lines().count(), 10); let _ = std::fs::remove_dir_all(&root); } @@ -599,9 +781,9 @@ pub fn refresh(token: &str) -> bool { write(&root, "target/debug/gen.rs", "pub fn generated() {}\n"); let g = indexed(&root); - assert_eq!(g.definitions_of("real", None, 10).len(), 1); - assert!(g.definitions_of("fake", None, 10).is_empty()); - assert!(g.definitions_of("generated", None, 10).is_empty()); + assert_eq!(defs_of(&g, "real").len(), 1); + assert!(defs_of(&g, "fake").is_empty()); + assert!(defs_of(&g, "generated").is_empty()); let _ = std::fs::remove_dir_all(&root); } diff --git a/apps/tui-cli/src/main.rs b/apps/tui-cli/src/main.rs index 7d07ac6..b6b392d 100644 --- a/apps/tui-cli/src/main.rs +++ b/apps/tui-cli/src/main.rs @@ -501,20 +501,23 @@ pub fn build_backend(choice: &BackendChoice, model_dir: &Path) -> Result core_engine::analyzer::UnitGrouper { pub fn context_builder(project_root: &Path, budget: usize) -> impl Fn(&str) -> String { let graph = Graph::open(project_root).ok().filter(|g| !g.is_empty()); let root = project_root.to_path_buf(); + // Memoised on the chunk text, because the same chunk is asked about twice: + // the TUI wants a unit's context when the unit starts, so it can show the + // evidence behind a finding, and the analyzer wants it again when it builds + // that unit's prompt. Assembling it walks the graph and reads source files, + // which is far too much to pay twice for one answer. + // + // Keyed by the text itself rather than a hash of it: a collision would serve + // one unit the context of another, and confidently wrong context is the + // failure this whole module exists to avoid. The keys are the unit texts, + // which the caller is already holding. + let memo: std::sync::Mutex> = Default::default(); move |chunk: &str| { - graph + if let Ok(cache) = memo.lock() + && let Some(hit) = cache.get(chunk) + { + return hit.clone(); + } + let built = graph .as_ref() .and_then(|g| rag::build_context(chunk, g, &root, budget)) - .unwrap_or_default() + .unwrap_or_default(); + if let Ok(mut cache) = memo.lock() { + cache.insert(chunk.to_string(), built.clone()); + } + built } } @@ -864,11 +887,12 @@ fn review( let response = client .review(daemon::ReviewRequest { diff: diff.to_string(), - languages: detect_languages(diff), requirements: ticket.clone(), max_tokens: settings.max_tokens, min_confidence: settings.min_confidence, triage: format!("{:?}", settings.triage).to_lowercase(), + temperature: settings.temperature, + seed: settings.seed, rules: project.custom.clone(), rulebooks: project.books.clone(), baseline: project @@ -985,7 +1009,7 @@ fn run_with_progress( diff, context_for, settings.max_tokens, - move |chunk, total| { + move |chunk, total, _unit| { if let Ok(mut l) = progress_label.lock() { *l = format!("unit {chunk}/{total}"); } @@ -1031,7 +1055,7 @@ fn print_header( let model_label = match &settings.backend { BackendChoice::Local { model, .. } => download::find_model(model) - .map(|m| format!("{} · Q4_K_M · {:.1} GB", m.name, m.size_gb)) + .map(|m| format!("{} · Q4_K_M · {:.1} GB", m.name, m.size_gb())) .unwrap_or_else(|| model.clone()), BackendChoice::Remote { protocol, @@ -1486,11 +1510,21 @@ fn run_serve( // request, not from whatever the daemon happened to be started with — // otherwise `--no-cache` and friends are silently ignored, and a daemon // started in one repo would write another repo's cache. + // + // `settings` here is only the daemon's own startup configuration. The + // fields left untouched below are the ones that genuinely belong to the + // resident process (`backend`, and `debug`, whose output goes to the + // daemon's console rather than the client's terminal). let mut per_request = settings.clone(); per_request.max_tokens = req.max_tokens; per_request.min_confidence = req.min_confidence; per_request.triage = core_engine::TriageMode::parse(&req.triage); - per_request.use_cache = req.use_cache; + per_request.temperature = req.temperature; + per_request.seed = req.seed; + // Re-assert the invariant `resolve_settings` establishes rather than + // trusting the client to have applied it: a cached answer replayed for + // a sampled run would be a lie about what the model produced. + per_request.use_cache = req.use_cache && req.temperature == 0.0; let request_root = if req.project_root.is_empty() { project_root.clone() @@ -1514,7 +1548,13 @@ fn run_serve( // Built here rather than sent by the client: the daemon owns chunking, // so only the daemon knows what each chunk contains. let context_for = context_builder(&request_root, 8000); - let outcome = analyzer.analyze(&req.diff, &context_for, req.max_tokens, |_, _| {}, |_| {}); + let outcome = analyzer.analyze( + &req.diff, + &context_for, + req.max_tokens, + |_, _, _| {}, + |_| {}, + ); let backend_label = analyzer.backend_description(); // Reclaim the loaded weights for the next request — the whole point of diff --git a/apps/tui-cli/src/rag.rs b/apps/tui-cli/src/rag.rs index 3b3c778..6754b4b 100644 --- a/apps/tui-cli/src/rag.rs +++ b/apps/tui-cli/src/rag.rs @@ -19,7 +19,7 @@ use crate::graph::{Def, Graph}; use core_engine::diff::{FileDiff, parse_diff}; -use std::collections::HashSet; +use std::collections::{HashMap, HashSet}; use std::path::Path; /// Enclosing bodies to include before the budget is better spent elsewhere. @@ -117,14 +117,38 @@ pub fn build_context( } // 3. Symbols the added lines mention but do not define here. - for (name, near) in referenced_symbols(&files, graph) - .into_iter() - .take(MAX_REFERENCED) - { - let Some(def) = graph - .definitions_of(&name, near.as_deref(), 1) - .into_iter() - .next() + // + // Candidate names are gathered from the text first and resolved in one + // batch. Asking the graph about each word as it was encountered meant a + // query per distinct word in the diff — thousands on a large branch, times + // every review unit — and then the six survivors were looked up a second + // time to get the definition that had just been discarded. + let candidates = referenced_names(&files); + let known = graph.definitions_of_names( + &candidates + .iter() + .map(|(n, _)| n.clone()) + .collect::>(), + ); + let mut by_name: HashMap<&str, Vec<&Def>> = HashMap::new(); + for def in &known { + by_name.entry(def.name.as_str()).or_default().push(def); + } + + let mut referenced_included = 0; + for (name, seen_in) in &candidates { + if referenced_included >= MAX_REFERENCED { + break; + } + let Some(defs) = by_name.get(name.as_str()) else { + continue; + }; + // A definition in the file the name was seen in is far likelier to be + // the referent than a same-named symbol elsewhere. + let Some(def) = defs + .iter() + .find(|d| &d.path == seen_in) + .or_else(|| defs.first()) else { continue; }; @@ -142,6 +166,7 @@ pub fn build_context( break; } out.push_str(&entry); + referenced_included += 1; } // 4. The test file, which says what the code is *supposed* to do. @@ -202,9 +227,15 @@ fn test_file_for(path: &str, project_root: &Path) -> Option { .find(|c| project_root.join(c).is_file()) } -/// Names the added lines mention that the graph knows about, paired with the -/// file they were seen in so lookup can prefer a local definition. -fn referenced_symbols(files: &[FileDiff], graph: &Graph) -> Vec<(String, Option)> { +/// Candidate symbol names from the added lines, in order of first appearance, +/// each paired with the file it was seen in so lookup can prefer a local +/// definition. +/// +/// Purely textual — whether the graph knows a name is decided in one batch by +/// the caller. This used to consult the graph per word, which is what made +/// context assembly scale with the size of the diff rather than with the number +/// of symbols actually reported. +fn referenced_names(files: &[FileDiff]) -> Vec<(String, String)> { let mut seen = HashSet::new(); let mut out = Vec::new(); @@ -218,11 +249,7 @@ fn referenced_symbols(files: &[FileDiff], graph: &Graph) -> Vec<(String, Option< if word.len() < 3 || !seen.insert(word.to_string()) { continue; } - // A name defined in the diff's own file is already visible. - if graph.definitions_of(word, None, 1).is_empty() { - continue; - } - out.push((word.to_string(), Some(file.path.clone()))); + out.push((word.to_string(), file.path.clone())); } } } @@ -392,6 +419,73 @@ diff --git a/src/math.ts b/src/math.ts let _ = std::fs::remove_dir_all(&root); } + /// When two files declare the same name, the one in the file being reviewed + /// is the referent. This preference used to live in `Graph::definitions_of`; + /// it moved here when name resolution became a single batched query, and it + /// is the kind of thing that goes missing in such a move. + #[test] + fn a_local_definition_wins_over_a_same_named_one_elsewhere() { + let root = project("ambiguous-ref"); + write( + &root, + "src/a.rs", + "pub fn shared_helper() {\n let from_a = 1;\n}\npub fn uses_it() {}\n", + ); + write( + &root, + "src/z.rs", + "pub fn shared_helper() {\n let from_z = 2;\n}\n", + ); + let graph = indexed(&root); + + // The diff touches z.rs and mentions the ambiguous name. + let diff = "\ +diff --git a/src/z.rs b/src/z.rs ++++ b/src/z.rs +@@ -1,3 +1,3 @@ ++pub fn caller() { shared_helper(); } +"; + let ctx = build_context(diff, &graph, &root, 8000).expect("context"); + assert!( + ctx.contains("let from_z"), + "z.rs's own definition should be the one shown:\n{ctx}" + ); + assert!( + !ctx.contains("let from_a"), + "a same-named definition in another file is not the referent:\n{ctx}" + ); + let _ = std::fs::remove_dir_all(&root); + } + + /// Names the graph has never heard of must not consume the referenced-symbol + /// budget. Resolution is batched now, so a diff full of ordinary words has to + /// leave room for the few that are real symbols. + #[test] + fn unknown_words_do_not_crowd_out_real_symbols() { + let root = project("crowded-words"); + write( + &root, + "src/lib.rs", + "pub fn real_target() {\n let m = 1;\n}\n", + ); + let graph = indexed(&root); + + // Plenty of noise words before the one symbol that matters. + let noise: String = (0..40) + .map(|i| format!("+ let unknown_word_{i} = {i};\n")) + .collect(); + let diff = format!( + "diff --git a/src/use.rs b/src/use.rs\n+++ b/src/use.rs\n@@ -1,50 +1,50 @@\n{noise}+ real_target();\n" + ); + + let ctx = build_context(&diff, &graph, &root, 8000).expect("context"); + assert!( + ctx.contains("Definition of `real_target`"), + "the one real symbol must survive a diff full of unknown words:\n{ctx}" + ); + let _ = std::fs::remove_dir_all(&root); + } + #[test] fn a_symbol_is_never_included_twice() { // A function that is both the enclosing definition and a referenced diff --git a/apps/tui-cli/src/tui.rs b/apps/tui-cli/src/tui.rs index 2266290..a1d4fb9 100644 --- a/apps/tui-cli/src/tui.rs +++ b/apps/tui-cli/src/tui.rs @@ -55,8 +55,14 @@ struct UnitView { enum Msg { Progress(String), - /// Sent once, as soon as the backend is up and the units are known. - Units(HashMap), + /// One unit, sent as it starts — always before any finding that names it. + /// + /// Sent from the analyzer's own progress callback rather than planned up + /// front. Predicting the unit list means reimplementing triage and unit + /// grouping outside the engine, and getting either subtly wrong leaves a + /// finding pointing at an id nothing can resolve — a silently empty + /// evidence pane on exactly the cross-file changes that most need one. + Unit(String, Box), Findings(Vec), Done(Box), Error(String), @@ -264,6 +270,10 @@ where app.analyzing = true; app.findings.clear(); app.verdicts.clear(); + // A re-run re-derives its units; keeping the old ones would leave the + // pane showing a hunk from the previous run for an id that no longer + // exists in it. + app.units.clear(); app.stats = None; app.status = "Loading model…".into(); @@ -307,7 +317,9 @@ where loop { match rx.try_recv() { Ok(Msg::Progress(s)) => app.status = s, - Ok(Msg::Units(units)) => app.units = units, + 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(); @@ -396,34 +408,29 @@ fn analyze( let mut analyzer = crate::build_analyzer(backend, &settings, &project_root, &diff, ticket, project); - // Capture the evidence before reviewing, so a finding can show its hunk the - // moment it streams in rather than after the run completes. - let units: HashMap = analyzer - .plan_units(&diff, settings.max_tokens) - .into_iter() - .map(|u| { - let context = context_for(&u.text); - ( - u.id, - UnitView { - text: u.text, - context, - }, - ) - }) - .collect(); - let _ = tx.send(Msg::Units(units)); let _ = tx.send(Msg::Progress("Analyzing…".into())); - let progress_tx = tx.clone(); let findings_tx = tx.clone(); + // Borrowed, not moved: `context_for` is handed to `analyze` at the same + // time, and both uses are read-only. + let context_ref = &context_for; let (summary, stats) = analyzer .analyze( &diff, &context_for, settings.max_tokens, - move |done, total| { - let _ = progress_tx.send(Msg::Progress(format!("Analyzing unit {done}/{total}…"))); + |done, total, unit| { + // Capture the evidence as the unit starts. The analyzer + // guarantees this lands before any finding naming it, so the + // detail pane is never asked for a hunk it has not been given. + let _ = tx.send(Msg::Unit( + unit.id.clone(), + Box::new(UnitView { + context: context_ref(&unit.text), + text: unit.text.clone(), + }), + )); + let _ = tx.send(Msg::Progress(format!("Analyzing unit {done}/{total}…"))); }, move |batch| { let _ = findings_tx.send(Msg::Findings(batch.to_vec())); diff --git a/apps/tui-cli/tests/stdin_pipeline.rs b/apps/tui-cli/tests/stdin_pipeline.rs new file mode 100644 index 0000000..ebf9054 --- /dev/null +++ b/apps/tui-cli/tests/stdin_pipeline.rs @@ -0,0 +1,485 @@ +//! End-to-end over the real binary: a diff arrives on stdin, findings come out +//! of stdout, and the exit code reflects the gate. +//! +//! # Why this exists +//! +//! The `--stdin` path had no coverage at all, and it hid four separate copies of +//! the same defect — "where does a file start in a diff" was answered +//! independently in `parse_diff`, `prefilter`, `unit::split_files` and again for +//! path lines inside a hunk. Each had unit tests; none of them tested the four +//! together, so a diff piped from `diff -u` was parsed as one merged file with no +//! path, every finding was attributed to the wrong file, and the pre-filter +//! silently stopped filtering. Four unit tests written after the fact would not +//! have caught it either — only running the whole pipeline does. +//! +//! # Why the binary, and why a stub server +//! +//! Spawning the real executable is the only way to cover `cli::parse`, stdin +//! capture, project-root resolution and the exit code — none of which a library +//! test can reach. +//! +//! Inference is supplied by a stub HTTP endpoint rather than the bundled GGUF: +//! a 1.1 GB download is not a test dependency, and `--backend +//! openai-compatible` is a shipped, supported path, so the stub exercises real +//! code rather than a test-only seam. The stub answers each request with a +//! finding naming whichever file it can see in the prompt, which is precisely +//! the property that was broken. + +use std::io::{BufRead, BufReader, Read, Write}; +use std::net::{TcpListener, TcpStream}; +use std::path::{Path, PathBuf}; +use std::process::{Command, Stdio}; +use std::sync::{Arc, Mutex}; + +/// A multi-file diff with no `diff --git` lines — what `diff -u`, +/// `git format-patch` and most review tools produce. +const PLAIN_DIFF: &str = "\ +--- a/src/one.rs ++++ b/src/one.rs +@@ -1,2 +1,2 @@ +-let a = 1; ++let a = 2; +--- a/src/two.rs ++++ b/src/two.rs +@@ -10,2 +10,2 @@ +-let b = 1; ++let b = 2; +"; + +// ─── Stub inference endpoint ───────────────────────────────────────────────── + +struct Stub { + port: u16, + /// The prompt bodies the binary sent, so a test can assert on what the model + /// was actually shown rather than only on what came back. + prompts: Arc>>, +} + +impl Stub { + /// Serves at most `calls` requests, then stops. A bounded accept loop means + /// the thread ends on its own and a hung test fails on the assertion rather + /// than by timing out. + fn spawn(calls: usize) -> Stub { + let listener = TcpListener::bind("127.0.0.1:0").expect("bind stub"); + let port = listener.local_addr().expect("stub addr").port(); + let prompts = Arc::new(Mutex::new(Vec::new())); + + let seen = Arc::clone(&prompts); + std::thread::spawn(move || { + for _ in 0..calls { + match listener.accept() { + Ok((stream, _)) => serve_one(stream, &seen), + Err(_) => return, + } + } + }); + + Stub { port, prompts } + } + + fn url(&self) -> String { + format!("http://127.0.0.1:{}", self.port) + } + + fn prompts(&self) -> Vec { + self.prompts.lock().expect("stub prompts").clone() + } +} + +fn serve_one(mut stream: TcpStream, seen: &Mutex>) { + let body = match read_http_body(&mut stream) { + Some(b) => b, + None => return, + }; + seen.lock().expect("stub prompts").push(body.clone()); + + // Answer about whichever file this request is actually about. A backend that + // ignored the prompt and always named the same file would let the bug this + // test exists for pass unnoticed. + let path = ["src/one.rs", "src/two.rs"] + .into_iter() + .find(|p| body.contains(p)) + .unwrap_or("unknown"); + + // Line 1 deliberately: it is a real changed line in `one.rs` but not in + // `two.rs`, so anchoring has to snap the second finding to line 10. That + // only works if the parser kept the two files' hunks apart. + let review = serde_json::json!({ + "findings": [{ + "file": path, + "line": 1, + "severity": "high", + "category": "quality", + "issue": format!("something is wrong in {path}"), + "suggested_fix": "fix it", + }], + "positives": [], + "suggestions": [], + }) + .to_string(); + + let payload = serde_json::json!({ + "choices": [{ "message": { "content": review } }] + }) + .to_string(); + + let _ = write!( + stream, + "HTTP/1.1 200 OK\r\n\ + Content-Type: application/json\r\n\ + Content-Length: {}\r\n\ + Connection: close\r\n\r\n{payload}", + payload.len() + ); + let _ = stream.flush(); +} + +/// Enough HTTP to read one request: headers to the blank line, then +/// `Content-Length` bytes of body. +fn read_http_body(stream: &mut TcpStream) -> Option { + let mut reader = BufReader::new(stream.try_clone().ok()?); + let mut length = 0usize; + + loop { + let mut line = String::new(); + if reader.read_line(&mut line).ok()? == 0 { + return None; + } + if line == "\r\n" || line == "\n" { + break; + } + if let Some(value) = line.to_ascii_lowercase().strip_prefix("content-length:") { + length = value.trim().parse().ok()?; + } + } + + let mut body = vec![0u8; length]; + reader.read_exact(&mut body).ok()?; + String::from_utf8(body).ok() +} + +// ─── Harness ───────────────────────────────────────────────────────────────── + +struct Run { + stdout: String, + stderr: String, + code: i32, +} + +/// Run the real binary with `diff` on stdin, inside an isolated directory. +/// +/// The working directory matters: `run()` anchors `.diffmind/` at the repository +/// root, so without this the test would write its cache and run history into +/// diffmind's own checkout. `DIFFMIND_HOME` is redirected for the same reason. +fn review_stdin(dir: &Path, diff: &str, extra: &[&str]) -> Run { + let stub_args: Vec<&str> = extra.to_vec(); + let mut child = Command::new(env!("CARGO_BIN_EXE_diffmind")) + .current_dir(dir) + .env("DIFFMIND_HOME", dir) + // Colour codes in the middle of a path would defeat every assertion. + .env("NO_COLOR", "1") + .args(["--stdin", "--no-daemon", "--no-index", "--no-cache"]) + .args(stub_args) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .expect("spawn diffmind"); + + child + .stdin + .as_mut() + .expect("stdin") + .write_all(diff.as_bytes()) + .expect("write diff to stdin"); + + let out = child.wait_with_output().expect("wait for diffmind"); + Run { + stdout: String::from_utf8_lossy(&out.stdout).into_owned(), + stderr: String::from_utf8_lossy(&out.stderr).into_owned(), + code: out.status.code().unwrap_or(-1), + } +} + +fn tmpdir(name: &str) -> PathBuf { + let d = std::env::temp_dir().join(format!("diffmind-e2e-{name}-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&d); + std::fs::create_dir_all(&d).expect("create temp dir"); + d +} + +fn findings(stdout: &str) -> Vec { + let parsed: serde_json::Value = serde_json::from_str(stdout) + .unwrap_or_else(|e| panic!("stdout was not JSON: {e}\n{stdout}")); + parsed["findings"].as_array().cloned().unwrap_or_default() +} + +// ─── Tests ─────────────────────────────────────────────────────────────────── + +/// The whole point. Two files in, two files out, each finding on its own file. +#[test] +fn a_piped_multi_file_diff_reports_each_file_separately() { + let dir = tmpdir("multi-file"); + let stub = Stub::spawn(4); + + let run = review_stdin( + &dir, + PLAIN_DIFF, + &[ + "--format", + "json", + "--backend", + "openai-compatible", + "--backend-model", + "stub", + "--backend-url", + &stub.url(), + ], + ); + + let found = findings(&run.stdout); + let mut located: Vec<(String, u64)> = found + .iter() + .map(|f| { + ( + f["file"].as_str().unwrap_or_default().to_string(), + f["line"].as_u64().unwrap_or_default(), + ) + }) + .collect(); + located.sort(); + + assert_eq!( + located, + vec![ + ("src/one.rs".to_string(), 1), + ("src/two.rs".to_string(), 10) + ], + "each file must get its own finding, anchored to its own changed line.\n\ + stdout: {}\nstderr: {}", + run.stdout, + run.stderr + ); + + // And the model was genuinely shown one file per request, not one merged + // blob — the failure mode that made every finding land on the last path. + let prompts = stub.prompts(); + assert_eq!(prompts.len(), 2, "one request per file"); + assert_eq!( + prompts.iter().filter(|p| p.contains("src/one.rs")).count(), + 1, + "src/one.rs should appear in exactly one prompt" + ); + assert_eq!( + prompts.iter().filter(|p| p.contains("src/two.rs")).count(), + 1 + ); + + // Findings at or above the fail threshold mean exit 1, distinct from the + // exit 2 that means diffmind could not run. + assert_eq!(run.code, 1, "high findings should trip the gate"); + + let _ = std::fs::remove_dir_all(&dir); +} + +/// Noise filtering is path-based, so it only works if the piped diff's paths +/// survive parsing. They did not: a lockfile arriving on stdin was reviewed. +#[test] +fn a_piped_lockfile_is_filtered_and_the_code_beside_it_is_not() { + let dir = tmpdir("lockfile"); + let stub = Stub::spawn(2); + + let diff = "\ +--- a/pnpm-lock.yaml ++++ b/pnpm-lock.yaml +@@ -1,2 +1,2 @@ +- integrity: sha512-aaa ++ integrity: sha512-bbb +--- a/src/two.rs ++++ b/src/two.rs +@@ -10,2 +10,2 @@ +-let b = 1; ++let b = 2; +"; + + let run = review_stdin( + &dir, + diff, + &[ + "--format", + "json", + "--backend", + "openai-compatible", + "--backend-model", + "stub", + "--backend-url", + &stub.url(), + ], + ); + + let prompts = stub.prompts(); + assert_eq!( + prompts.len(), + 1, + "the lockfile must not cost an inference pass.\nstderr: {}", + run.stderr + ); + // Assert on the lockfile's *content*, not its path. When the header lines + // were being swallowed as `-`/`+` content, the path string still turned up + // in the merged prompt and a path-only assertion passed while the lockfile's + // body was in fact being reviewed. + assert!( + !prompts[0].contains("sha512-"), + "the lockfile's content reached the model:\n{}", + prompts[0] + ); + assert!( + !prompts[0].contains("pnpm-lock.yaml"), + "the lockfile reached the model:\n{}", + prompts[0] + ); + assert!(prompts[0].contains("src/two.rs")); + + let found = findings(&run.stdout); + let files: Vec<&str> = found.iter().filter_map(|f| f["file"].as_str()).collect(); + assert_eq!(files, ["src/two.rs"]); + + let _ = std::fs::remove_dir_all(&dir); +} + +/// A diff that is entirely noise must not be reported as a clean review, and +/// must not reach the backend at all — this path returns before a model is even +/// loaded, which is why it needs no stub. +#[test] +fn a_piped_diff_that_is_entirely_noise_says_so_and_costs_nothing() { + let dir = tmpdir("all-noise"); + let diff = "\ +--- a/pnpm-lock.yaml ++++ b/pnpm-lock.yaml +@@ -1,2 +1,2 @@ +- integrity: sha512-aaa ++ integrity: sha512-bbb +"; + + // Pointed at a dead port on purpose. Reaching a backend at all is the + // failure this asserts against, and a refused connection fails fast and + // loudly — where omitting the backend entirely would fall through to the + // local model and, on a machine that has one downloaded, quietly run a real + // inference pass instead of failing. + let run = review_stdin( + &dir, + diff, + &[ + "--format", + "json", + "--backend", + "openai-compatible", + "--backend-model", + "stub", + "--backend-url", + "http://127.0.0.1:1", + "--backend-timeout", + "5", + ], + ); + + assert_eq!(run.code, 0, "nothing reviewable is not a failure"); + assert!(findings(&run.stdout).is_empty(), "stdout: {}", run.stdout); + + let _ = std::fs::remove_dir_all(&dir); +} + +/// An empty diff on stdin is a passing review, not an error — a clean branch +/// piped into CI must exit 0. +#[test] +fn an_empty_diff_on_stdin_exits_zero() { + let dir = tmpdir("empty"); + let run = review_stdin(&dir, "", &["--format", "json"]); + + assert_eq!(run.code, 0, "stderr: {}", run.stderr); + assert!(findings(&run.stdout).is_empty()); + + let _ = std::fs::remove_dir_all(&dir); +} + +/// `--stdin` bypasses git entirely, so it has to work outside a repository — +/// which is the whole reason the flag exists. +#[test] +fn stdin_works_outside_a_git_repository() { + let dir = tmpdir("no-repo"); + assert!(!dir.join(".git").exists()); + + let stub = Stub::spawn(2); + let run = review_stdin( + &dir, + "\ +--- a/src/one.rs ++++ b/src/one.rs +@@ -1,2 +1,2 @@ +-let a = 1; ++let a = 2; +", + &[ + "--format", + "json", + "--backend", + "openai-compatible", + "--backend-model", + "stub", + "--backend-url", + &stub.url(), + ], + ); + + assert!( + !run.stderr.contains("not inside a git repository"), + "stderr: {}", + run.stderr + ); + assert_ne!(run.code, 2, "exit 2 means it could not run: {}", run.stderr); + let found = findings(&run.stdout); + let files: Vec<&str> = found.iter().filter_map(|f| f["file"].as_str()).collect(); + assert_eq!(files, ["src/one.rs"]); + + let _ = std::fs::remove_dir_all(&dir); +} + +/// The gate is what CI reads. Reporting everything while failing only on `high` +/// must still exit 0 when nothing reaches the threshold. +#[test] +fn the_exit_code_follows_the_fail_threshold_not_the_finding_count() { + let dir = tmpdir("gate"); + let stub = Stub::spawn(2); + + let run = review_stdin( + &dir, + "\ +--- a/src/one.rs ++++ b/src/one.rs +@@ -1,2 +1,2 @@ +-let a = 1; ++let a = 2; +", + &[ + "--format", + "json", + "--min-severity", + "low", + // The stub reports `high`, so raising the gate above it is the only + // way to be sure the exit code follows the gate and not the count. + "--fail-on", + "high", + "--backend", + "openai-compatible", + "--backend-model", + "stub", + "--backend-url", + &stub.url(), + ], + ); + + assert_eq!(findings(&run.stdout).len(), 1, "the finding is reported"); + assert_eq!(run.code, 1, "and a high finding trips a high gate"); + + let _ = std::fs::remove_dir_all(&dir); +} diff --git a/packages/core-engine/src/analyzer.rs b/packages/core-engine/src/analyzer.rs index a1d8265..0725c60 100644 --- a/packages/core-engine/src/analyzer.rs +++ b/packages/core-engine/src/analyzer.rs @@ -269,11 +269,22 @@ impl ReviewAnalyzer { /// The units this analyzer would review, for a caller that needs to show a /// reader the hunk a finding actually came from. /// - /// Ids match those stamped onto findings, because the sizing comes from the - /// same backend and units are grouped per file — so triage dropping a whole - /// file cannot shift the ids of the files that survive. + /// Ids match those stamped onto findings: the sizing comes from the same + /// backend, unit ids are content-derived rather than positional (so triage + /// dropping a whole file cannot shift the ids of the files that survive), + /// and the same grouper runs here as in [`Self::analyze`]. + /// + /// That last part is easy to lose. Merging two units mints a **new** id, so + /// planning without the grouper returned only the unmerged halves — and + /// every finding from a merged unit named an id no caller could resolve. + /// The TUI's evidence pane went blank for precisely the cross-file changes + /// the code graph exists to link. pub fn plan_units(&self, diff: &str, max_tokens: u32) -> Vec { - build_units(diff, self.max_chunk_lines(max_tokens as usize)) + let units = build_units(diff, self.max_chunk_lines(max_tokens as usize)); + match &self.unit_grouper { + Some(group) => group(units), + None => units, + } } fn gen_options(&self, max_new_tokens: usize) -> GenOptions { @@ -342,7 +353,12 @@ impl ReviewAnalyzer { /// - `context_for(chunk)` supplies the symbol context for one chunk. It is /// a callback rather than a string because context must be assembled per /// chunk: see [`Self::analyze_chunk`]. - /// - `on_progress(done, total)` fires when a chunk starts. + /// - `on_progress(done, total, unit)` fires when a unit starts, and is + /// handed the unit itself. A caller that wants to show a reader the hunk + /// behind a finding should record it here rather than predict the unit + /// list up front: triage and the grouper both run inside this method, and + /// either can change which units exist and therefore what ids findings + /// carry. It always fires before that unit's `on_chunk_result`. /// - `on_chunk_result(findings)` fires with each batch as it completes, so /// the CLI can print before the whole diff is processed. /// @@ -357,7 +373,7 @@ impl ReviewAnalyzer { on_chunk_result: G, ) -> Result<(ReviewSummary, AnalysisStats), EngineError> where - F: Fn(usize, usize), + F: Fn(usize, usize, &crate::unit::ReviewUnit), G: Fn(&[ReviewFinding]), { let files = parse_diff(diff); @@ -410,7 +426,7 @@ impl ReviewAnalyzer { units.sort_by_key(|u| rulebook::group_key(&applicable_to_unit(&rulebooks, u))); for (i, unit) in units.iter().enumerate() { - on_progress(i + 1, units.len()); + on_progress(i + 1, units.len(), unit); let books = applicable_to_unit(&rulebooks, unit); match self.analyze_chunk(&unit.text, context_for, &books, max_tokens_per_chunk, 0) { @@ -1060,7 +1076,7 @@ diff --git a/b.rs b/b.rs }; analyzer - .analyze(TWO_FILES, &context_for, 512, |_, _| {}, |_| {}) + .analyze(TWO_FILES, &context_for, 512, |_, _, _| {}, |_| {}) .expect("analysis should succeed"); let prompts = seen.lock().expect("recording backend mutex"); @@ -1088,6 +1104,117 @@ diff --git a/b.rs b/b.rs ); } + const TWO_SRC_FILES: &str = "\ +diff --git a/src/a.rs b/src/a.rs ++++ b/src/a.rs +@@ -1,2 +1,2 @@ +-let old_a = 1; ++let new_a = 2; +diff --git a/src/b.rs b/src/b.rs ++++ b/src/b.rs +@@ -1,2 +1,2 @@ +-let old_b = 1; ++let new_b = 2; +"; + + /// What the TUI's evidence pane actually relies on. + /// + /// It records each unit from `on_progress` and looks it up by a finding's + /// `unit_id`. That only works if a unit is announced before any finding + /// naming it arrives — otherwise the pane is asked for a hunk it has not + /// been given, and silently shows nothing. + #[test] + fn a_unit_is_announced_before_any_finding_that_names_it() { + let seen = Arc::new(Mutex::new(Vec::new())); + let mut analyzer = + ReviewAnalyzer::new(Box::new(FindingBackend { seen })).with_triage(TriageMode::Off); + + // One log, both callbacks, so the interleaving is what is asserted. + let announced: Mutex> = Mutex::new(HashSet::new()); + let violations: Mutex> = Mutex::new(Vec::new()); + + analyzer + .analyze( + TWO_SRC_FILES, + &|_| String::new(), + 512, + |_, _, unit| { + announced.lock().expect("announced").insert(unit.id.clone()); + }, + |findings| { + let known = announced.lock().expect("announced"); + for f in findings { + if let Some(id) = f.unit_id.as_deref() + && !known.contains(id) + { + violations.lock().expect("violations").push(id.to_string()); + } + } + }, + ) + .expect("analysis should succeed"); + + assert!( + !announced.lock().expect("announced").is_empty(), + "no unit was announced at all" + ); + assert!( + violations.lock().expect("violations").is_empty(), + "findings arrived naming units that had not been announced: {:?}", + violations.lock().expect("violations") + ); + } + + /// The regression that blanked the TUI's evidence pane. + /// + /// A caller planning units up front looks a hunk up by the finding's + /// `unit_id`. Merging two units mints a **new** id, so a plan built without + /// the grouper resolved nothing for a merged unit — and a merged unit is + /// precisely the cross-file change the code graph exists to link, i.e. the + /// finding whose evidence a reviewer most needs to see. + /// + /// The TUI now takes its units from `analyze` itself rather than predicting + /// them, but `plan_units` is still public API and must not lie. + #[test] + fn planned_unit_ids_cover_every_unit_a_finding_can_name() { + let seen = Arc::new(Mutex::new(Vec::new())); + let mut analyzer = ReviewAnalyzer::new(Box::new(FindingBackend { seen })) + .with_triage(TriageMode::Off) + // Stands in for the code graph linking a symbol to its caller. + .with_unit_grouper(Box::new(|units: Vec| match units + .split_first() + { + Some((first, rest)) => { + vec![rest.iter().fold(first.clone(), |a, b| a.merged_with(b))] + } + None => units, + })); + + let planned: HashSet = analyzer + .plan_units(TWO_SRC_FILES, 512) + .into_iter() + .map(|u| u.id) + .collect(); + + let (summary, stats) = analyzer + .analyze(TWO_SRC_FILES, &|_| String::new(), 512, |_, _, _| {}, |_| {}) + .expect("analysis should succeed"); + + assert_eq!(stats.units_total, 1, "the grouper should have merged both"); + let used: HashSet<&str> = summary + .findings + .iter() + .filter_map(|f| f.unit_id.as_deref()) + .collect(); + assert!(!used.is_empty(), "a model finding must record its unit"); + for id in &used { + assert!( + planned.contains(*id), + "a finding names unit {id}, which the plan cannot resolve: {planned:?}" + ); + } + } + /// Two regions of one file are two units, and each finding records which /// unit produced it — that is what lets the reader be shown the exact hunk /// the model was looking at. @@ -1108,7 +1235,7 @@ diff --git a/src/a.rs b/src/a.rs let mut analyzer = ReviewAnalyzer::new(backend).with_triage(TriageMode::Off); let (summary, stats) = analyzer - .analyze(diff, &|_| String::new(), 512, |_, _| {}, |_| {}) + .analyze(diff, &|_| String::new(), 512, |_, _, _| {}, |_| {}) .expect("analysis should succeed"); assert_eq!(stats.units_total, 2, "two regions, two units"); @@ -1159,7 +1286,7 @@ diff --git a/src/a.rs b/src/a.rs .with_triage(TriageMode::Off) .with_rulebooks(books); analyzer - .analyze(ONE_FILE, &|_| String::new(), 512, |_, _| {}, |_| {}) + .analyze(ONE_FILE, &|_| String::new(), 512, |_, _, _| {}, |_| {}) .expect("analysis should succeed") .0 .findings @@ -1253,7 +1380,7 @@ diff --git a/src/auth.js b/src/auth.js let mut analyzer = ReviewAnalyzer::new(backend).with_triage(TriageMode::Off); let (summary, _) = analyzer - .analyze(diff, &|_| String::new(), 512, |_, _| {}, |_| {}) + .analyze(diff, &|_| String::new(), 512, |_, _, _| {}, |_| {}) .expect("analysis should succeed"); let detector_findings: Vec<_> = summary diff --git a/packages/core-engine/src/detectors.rs b/packages/core-engine/src/detectors.rs index b1e35a8..edd5115 100644 --- a/packages/core-engine/src/detectors.rs +++ b/packages/core-engine/src/detectors.rs @@ -341,33 +341,42 @@ pub fn extract_declared_name(line: &str) -> Option { /// /// Member accesses (`self.name`, `opts.name`) are excluded: they read a field, /// not the removed local, and were a large source of false positives. +/// +/// Everything here works in characters rather than bytes, because `name` can +/// legitimately be non-ASCII — [`extract_declared_name`] collects it with +/// `char::is_alphanumeric`, which is Unicode-aware, so `const élan = 1` yields +/// `élan`. Byte arithmetic got that wrong twice over: advancing by one byte past +/// a multi-byte first character landed mid-codepoint and panicked the next +/// slice, and comparing the neighbouring *byte* against ASCII treated a UTF-8 +/// continuation byte as a word boundary — so `élan` looked like a standalone +/// reference inside `béélan`. pub fn references_identifier(text: &str, name: &str) -> bool { - let bytes = text.as_bytes(); - let name_len = name.len(); - let mut start = 0; - - let is_ident_byte = |b: u8| b.is_ascii_alphanumeric() || b == b'_' || b == b'$'; + if name.is_empty() { + return false; + } + let is_ident = |c: char| c.is_alphanumeric() || c == '_' || c == '$'; - while let Some(pos) = text[start..].find(name) { - let abs = start + pos; - let end = abs + name_len; + let mut search_from = 0; + while let Some(pos) = text[search_from..].find(name) { + let abs = search_from + pos; + let end = abs + name.len(); - // Byte indexing is safe here: `find` returns a char boundary, and the - // neighbours are only compared against ASCII. The previous version - // mixed this byte offset with `chars().nth()`, which read the wrong - // character on any line containing non-ASCII text. - let before = if abs == 0 { None } else { Some(bytes[abs - 1]) }; - let after = bytes.get(end).copied(); + // `find` reports a char boundary and `name` matched whole, so both + // slices split cleanly. + let before = text[..abs].chars().next_back(); + let after = text[end..].chars().next(); - let boundary_before = before.is_none_or(|b| !is_ident_byte(b)); - let boundary_after = after.is_none_or(|b| !is_ident_byte(b)); - let is_member_access = before == Some(b'.'); + let standalone = before.is_none_or(|c| !is_ident(c)) && after.is_none_or(|c| !is_ident(c)); + let is_member_access = before == Some('.'); - if boundary_before && boundary_after && !is_member_access { + if standalone && !is_member_access { return true; } - start = abs + 1; - if start >= text.len() { + + // Step past this occurrence's first character, not its first byte. + let step = text[abs..].chars().next().map_or(1, char::len_utf8); + search_from = abs + step; + if search_from >= text.len() { break; } } @@ -679,6 +688,68 @@ diff --git a/src/a.ts b/src/a.ts assert!(!references_identifier("self.timeout", "timeout")); } + /// A non-ASCII *identifier*, not merely a non-ASCII line. `is_alphanumeric` + /// is Unicode-aware, so these names are real and reachable. + #[test] + fn a_non_ascii_identifier_is_matched_by_character_not_by_byte() { + assert!(references_identifier("return élan;", "élan")); + assert!(references_identifier("f(élan)", "élan")); + + // Part of a longer identifier is not a reference to it. The old + // byte-wise boundary check saw a UTF-8 continuation byte, decided that + // was a word boundary, and said yes. + assert!( + !references_identifier("béélan = 1;", "élan"), + "`élan` inside `béélan` is not a standalone reference" + ); + assert!(!references_identifier("élanor()", "élan")); + + // A member access is a field read, as in the ASCII case — and reaching + // this branch is what used to advance by one byte into the middle of + // `é` and abort the process on the next search. + assert!(!references_identifier("return this.élan;", "élan")); + assert!(!references_identifier("a.élan + b.élan", "élan")); + } + + /// The panic, end to end. `--stdin` or `core.quotePath=false` is not needed + /// here: an identifier is enough, and a review that aborts mid-run reports + /// nothing at all. + #[test] + fn a_non_ascii_declaration_does_not_abort_the_detector() { + let diff = "\ +diff --git a/src/a.ts b/src/a.ts +--- a/src/a.ts ++++ b/src/a.ts +@@ -1,4 +1,3 @@ + function f(opts) { +- const élan = 30; + return opts.élan; + } +"; + let findings = detect_removed_used_variables(&parse_diff(diff)); + assert!( + findings.is_empty(), + "opts.élan is a field read, exactly as opts.timeout is: {findings:#?}" + ); + } + + #[test] + fn a_removed_non_ascii_declaration_is_still_flagged() { + let diff = "\ +diff --git a/src/a.ts b/src/a.ts +--- a/src/a.ts ++++ b/src/a.ts +@@ -1,4 +1,3 @@ + function f() { +- const élan = 30; + return élan; + } +"; + let findings = detect_removed_used_variables(&parse_diff(diff)); + assert_eq!(findings.len(), 1, "got: {findings:#?}"); + assert!(findings[0].issue.contains("élan")); + } + #[test] fn commented_out_block_is_flagged_with_a_real_line_number() { let diff = "\ diff --git a/packages/core-engine/src/diff.rs b/packages/core-engine/src/diff.rs index d3bd7a1..9a47fad 100644 --- a/packages/core-engine/src/diff.rs +++ b/packages/core-engine/src/diff.rs @@ -69,6 +69,24 @@ impl FileDiff { } } +/// Is `lines[i]` the `--- old` of a `--- old` / `+++ new` / `@@ …` file header? +/// +/// The only file boundary a plain `diff -u` provides. `git diff` announces each +/// file with a `diff --git` line, but a diff reaching `--stdin` from `diff -u`, +/// `format-patch` or a review tool has nothing else to separate one file from +/// the next. +/// +/// Deliberately strict about all three lines. Inside a hunk, a removed line of +/// content beginning `-- ` renders as `--- `, and SQL comments do exactly that — +/// requiring the `@@` too is what stops `-- note` / `++ x` being read as a file +/// boundary. Unified diff always puts the hunk header immediately after the path +/// pair, so nothing legitimate is lost. +pub(crate) fn starts_file_header_pair(lines: &[&str], i: usize) -> bool { + lines[i].starts_with("--- ") + && lines.get(i + 1).is_some_and(|l| l.starts_with("+++ ")) + && lines.get(i + 2).is_some_and(|l| l.starts_with("@@")) +} + /// Parse a unified diff into per-file hunks. /// /// Tolerant by design: `git diff` output is interleaved with `index`, `old @@ -90,7 +108,8 @@ pub fn parse_diff(diff: &str) -> Vec { } } - for line in diff.lines() { + let lines: Vec<&str> = diff.lines().collect(); + for (i, line) in lines.iter().copied().enumerate() { if let Some(rest) = line.strip_prefix("diff --git ") { flush_hunk(&mut current, &mut hunk); if let Some(f) = current.take() { @@ -111,29 +130,55 @@ pub fn parse_diff(diff: &str) -> Vec { continue; } - // `+++ b/path` is authoritative when present — it survives paths with - // spaces, which the `diff --git` line cannot express unambiguously. - if let Some(rest) = line.strip_prefix("+++ ") { - if let Some(f) = current.as_mut() { - let path = rest.trim(); - if path != "/dev/null" { - f.path = path.strip_prefix("b/").unwrap_or(path).to_string(); - } - } else { + // In a plain multi-file diff there is no `diff --git` line to close the + // previous file, so the next `--- old` / `+++ new` pair has to. Without + // this the second file's `+++` merely *renamed* the first file — every + // hunk in the diff ended up under the last path seen, and findings in + // the earlier files were attributed to the wrong one. + if starts_file_header_pair(&lines, i) + && current + .as_ref() + .is_some_and(|f| !f.hunks.is_empty() || hunk.is_some()) + { + flush_hunk(&mut current, &mut hunk); + if let Some(f) = current.take() { + files.push(f); + } + } + + // The `--- old` / `+++ new` path pair, which only ever appears *outside* + // a hunk. Inside one, a line beginning `--- ` or `+++ ` is content whose + // diff marker happens to be followed by two more of the same character: + // a removed `-- note` in SQL, Lua or Haskell renders as `--- note`. + // Handling those unconditionally meant such a line was dropped from the + // hunk, and its `+++` twin silently renamed the file to the comment's + // own text — so every finding in that file pointed at a path that does + // not exist. + if hunk.is_none() { + // `+++ b/path` is authoritative when present — it survives paths + // with spaces, which the `diff --git` line cannot express + // unambiguously. + if let Some(rest) = line.strip_prefix("+++ ") { let path = rest.trim(); if path != "/dev/null" { - current = Some(FileDiff { - path: path.strip_prefix("b/").unwrap_or(path).to_string(), - hunks: Vec::new(), - is_deletion: false, - }); + let path = path.strip_prefix("b/").unwrap_or(path).to_string(); + match current.as_mut() { + Some(f) => f.path = path, + None => { + current = Some(FileDiff { + path, + hunks: Vec::new(), + is_deletion: false, + }); + } + } } + continue; } - continue; - } - if line.starts_with("--- ") { - continue; + if line.starts_with("--- ") { + continue; + } } if line.starts_with("@@") { @@ -231,14 +276,8 @@ fn parse_git_header_path(rest: &str) -> String { return b_side.strip_prefix("b/").unwrap_or(b_side).to_string(); } - // Even split: `a/ b/` where both paths are identical. - if let Some(a_body) = rest.strip_prefix("a/") { - let midpoint = a_body.len().saturating_sub(1) / 2; - let candidate = &a_body[..midpoint.min(a_body.len())]; - let expected = format!("a/{candidate} b/{candidate}"); - if rest == expected { - return candidate.to_string(); - } + if let Some(candidate) = even_split(rest) { + return candidate.to_string(); } // Fall back to the last " b/" occurrence — right more often than the first @@ -249,6 +288,30 @@ fn parse_git_header_path(rest: &str) -> String { rest.to_string() } +/// `a/ b/` where both sides name the same file — the overwhelmingly +/// common case, and the only one that stays right when the path itself contains +/// " b/". Returns `None` when the two halves are not identical, leaving the +/// caller's `rfind` fallback to handle renames. +/// +/// The arithmetic used to be `(len - 1) / 2`, which is one byte long: the +/// remainder after `a/` is ` b/`, so for a path of `n` bytes it is +/// `2n + 3` bytes, not `2n + 1`. The comparison therefore never matched and +/// this branch never returned — every correct answer came from the fallback +/// below it. Worse, the slice was taken without a boundary check, so a rename +/// like `a/x.rs b/ääää.rs` landed mid-codepoint and panicked the process. +fn even_split(rest: &str) -> Option<&str> { + let a_body = rest.strip_prefix("a/")?; + // ` b/` is `2n + 3` bytes, so an odd remainder cannot be one. + let n = a_body.len().checked_sub(3).filter(|r| r % 2 == 0)? / 2; + // A rename whose two sides merely happen to be the same byte length can put + // `n` inside a multi-byte character. Refuse rather than slice. + if !a_body.is_char_boundary(n) { + return None; + } + let candidate = &a_body[..n]; + (rest.len() == 2 + n + 3 + n && a_body[n..] == format!(" b/{candidate}")).then_some(candidate) +} + /// Parse `@@ -old_start,old_count +new_start,new_count @@ optional context`. /// Returns `(old_start, new_start)`, defaulting to 1 on malformed input. pub(crate) fn parse_hunk_header(line: &str) -> (u32, u32) { @@ -416,6 +479,105 @@ index 111..222 100644 assert_eq!(files[0].path, "my b/dir.rs"); } + /// The `diff --git` line has to stand on its own: a binary file or a mode + /// change carries no `+++ b/…` to correct it, and the pre-filter classifies + /// on the header before any `+++` has been read. + #[test] + fn the_git_header_alone_resolves_a_path_containing_b_slash() { + assert_eq!( + parse_git_header_path("a/my b/dir.rs b/my b/dir.rs"), + "my b/dir.rs", + "the last ' b/' is the wrong split point when the path contains one" + ); + assert_eq!(parse_git_header_path("a/src/x.rs b/src/x.rs"), "src/x.rs"); + } + + /// The even split is only valid when both sides are the same file; a rename + /// must fall through to the `b/` side rather than report the `a/` side. + #[test] + fn a_rename_reports_the_post_image_path() { + assert_eq!(parse_git_header_path("a/old.rs b/new.rs"), "new.rs"); + // Two different names that happen to be the same byte length — the case + // an even split would silently get wrong if it did not compare halves. + assert_eq!(parse_git_header_path("a/aa.rs b/bb.rs"), "bb.rs"); + } + + /// `core.quotePath=false` is a common setting, and a piped diff can carry + /// raw UTF-8 regardless. Slicing the header at a byte midpoint used to land + /// inside a codepoint and abort the process. + #[test] + fn a_non_ascii_path_does_not_panic_the_parser() { + assert_eq!(parse_git_header_path("a/x.rs b/ääää.rs"), "ääää.rs"); + assert_eq!(parse_git_header_path("a/ä.rs b/ä.rs"), "ä.rs"); + assert_eq!(parse_git_header_path("a/éé b/x"), "x"); + + let files = parse_diff("diff --git a/x.rs b/ääää.rs\n@@ -1 +1 @@\n+x\n"); + assert_eq!(files[0].path, "ääää.rs"); + } + + /// A diff with no `diff --git` lines — `diff -u`, `format-patch`, or a + /// review tool piped to `--stdin`. + /// + /// Nothing closed the previous file, so the second `+++` merely *renamed* + /// the first one: every hunk in the diff collapsed under the last path seen, + /// and findings in the earlier files were reported against the wrong file. + #[test] + fn a_plain_multi_file_diff_keeps_its_files_apart() { + let files = parse_diff( + "\ +--- a/src/one.rs ++++ b/src/one.rs +@@ -1,2 +1,2 @@ +-let a = 1; ++let a = 2; +--- a/src/two.rs ++++ b/src/two.rs +@@ -10,2 +10,2 @@ +-let b = 1; ++let b = 2; +", + ); + + let paths: Vec<&str> = files.iter().map(|f| f.path.as_str()).collect(); + assert_eq!(paths, ["src/one.rs", "src/two.rs"]); + assert_eq!( + files[0].hunks.len(), + 1, + "one hunk each, not both in one file" + ); + assert_eq!(files[1].hunks.len(), 1); + assert_eq!(files[0].hunks[0].new_start, 1); + assert_eq!(files[1].hunks[0].new_start, 10); + assert_eq!( + files[1].hunks[0].added().next().map(|l| l.text.as_str()), + Some("let b = 2;") + ); + } + + /// The pair detection must not fire on content. A removed SQL comment + /// (`-- note`) renders as `--- note`, which is why the `@@` is required too. + #[test] + fn a_removed_sql_comment_is_not_a_file_boundary() { + let files = parse_diff( + "\ +diff --git a/q.sql b/q.sql ++++ b/q.sql +@@ -1,4 +1,4 @@ + select 1; +--- old note ++++ new note + select 2; +", + ); + assert_eq!(files.len(), 1, "one file, not two"); + assert_eq!(files[0].path, "q.sql"); + let removed: Vec<&str> = files[0].hunks[0] + .removed() + .map(|l| l.text.as_str()) + .collect(); + assert_eq!(removed, ["-- old note"], "still a removed content line"); + } + fn f(file: &str, line: u32) -> ReviewFinding { ReviewFinding { file: file.into(), diff --git a/packages/core-engine/src/json_guard.rs b/packages/core-engine/src/json_guard.rs index a10f321..3a2d609 100644 --- a/packages/core-engine/src/json_guard.rs +++ b/packages/core-engine/src/json_guard.rs @@ -492,17 +492,30 @@ pub fn extract_json(text: &str) -> Option<&str> { continue; } let mut state = JsonPrefix::new(); + let mut rejected = false; for (offset, c) in text[start..].char_indices() { if state.push(c).is_err() { + rejected = true; break; } if state.is_complete() { return Some(&text[start..start + offset + c.len_utf8()]); } } - // This opener never closed; a later one will not either, since it is - // nested inside the same unterminated value. - break; + // Why the two endings are not the same thing. A *rejected* opener was + // never JSON — a brace in prose, `Note: {see below}` — and says nothing + // about what follows, so keep looking. An opener that merely ran out of + // text is genuinely unterminated, and every later opener is nested + // inside it, so none of them can close either: stop rather than rescan + // the tail once per brace. + // + // Conflating the two cost a whole unit whenever a model wrote a brace + // before its answer, which is exactly the habit the constrained decoder + // exists to correct — and the backends that cannot be constrained, the + // remote ones, are the only callers that reach here. + if !rejected { + break; + } } None } @@ -707,5 +720,43 @@ mod tests { #[test] fn extract_json_returns_none_when_unterminated() { assert!(extract_json(r#"{"a": [1, 2"#).is_none()); + // Nothing after an unterminated opener can close it either — the inner + // brackets are inside it. + assert!(extract_json(r#"{"a": {"b": 1"#).is_none()); + } + + /// A brace in the prose before the answer used to lose the answer. + /// + /// The scan tried the first `{`, found it was not JSON, and gave up instead + /// of looking further along — so the unit was counted unparseable and its + /// findings thrown away. Only reachable on backends whose decoding cannot be + /// constrained, which is precisely where a stray preamble is likely. + #[test] + fn extract_json_skips_a_brace_that_is_only_prose() { + let text = "Note: {see below}\n{\"findings\": [], \"positives\": [\"ok\"]}"; + assert_eq!( + extract_json(text), + Some(r#"{"findings": [], "positives": ["ok"]}"#) + ); + + // Several false starts, and a bracket rather than a brace. + assert_eq!( + extract_json("[not json] {also not} finally: [1, 2]"), + Some("[1, 2]") + ); + } + + /// The whole pipeline, not just the extractor: a preamble containing a brace + /// must still yield the findings the model actually reported. + #[test] + fn a_braced_preamble_does_not_lose_the_findings() { + let response = "Looking at the diff {specifically auth.rs}, here is the review:\n\ + {\"findings\":[{\"file\":\"a.rs\",\"line\":1,\"severity\":\"high\",\ + \"category\":\"security\",\"issue\":\"boom\",\"suggested_fix\":\"f\"}],\ + \"positives\":[],\"suggestions\":[]}"; + let summary = crate::analyzer::parse_review_response(response) + .expect("a brace in the preamble must not cost the whole unit"); + assert_eq!(summary.findings.len(), 1); + assert_eq!(summary.findings[0].issue, "boom"); } } diff --git a/packages/core-engine/src/prefilter.rs b/packages/core-engine/src/prefilter.rs index db58e5a..45380ef 100644 --- a/packages/core-engine/src/prefilter.rs +++ b/packages/core-engine/src/prefilter.rs @@ -265,7 +265,8 @@ pub fn prefilter(diff: &str, opts: &PrefilterOptions) -> (String, PrefilterRepor let mut file = FileState::default(); let mut hunk = HunkState::default(); - for line in diff.lines() { + let lines: Vec<&str> = diff.lines().collect(); + for (i, line) in lines.iter().copied().enumerate() { if line.starts_with("diff --git ") { flush_file(&mut out, &mut report, &mut file, &mut hunk); file.classify(&header_path(line), opts); @@ -273,9 +274,28 @@ pub fn prefilter(diff: &str, opts: &PrefilterOptions) -> (String, PrefilterRepor continue; } + // A `--- old` / `+++ new` pair opens a file when nothing else has. + // + // `git diff` announces each file with a `diff --git` line, so the pair + // that follows belongs to a file already open. A plain `diff -u` — or + // anything piped to `--stdin` — has no such line, and then this pair is + // the only thing separating one file from the next. Without recognising + // it, the first file's header was dropped (leaving hunks that could not + // be attributed to any path) and, in a multi-file diff, the second + // file's header arrived while a hunk was open and was counted as `-` and + // `+` content lines — silently corrupting both files. + let opens_a_file = crate::diff::starts_file_header_pair(&lines, i) + && (file.header.is_empty() || !hunk.lines.is_empty()); + if opens_a_file { + flush_file(&mut out, &mut report, &mut file, &mut hunk); + } + // Header lines belong to the file, not to any hunk, and are replayed // only if some hunk of that file survives. - if hunk.lines.is_empty() && !file.header.is_empty() && is_file_header_line(line) { + if hunk.lines.is_empty() + && (opens_a_file || !file.header.is_empty()) + && is_file_header_line(line) + { // `+++ b/path` is the authoritative path; re-classify against it. if let Some(rest) = line.strip_prefix("+++ ") { let p = rest.trim(); @@ -727,6 +747,90 @@ index 0000000..1234567 ); } + /// `git diff main...HEAD | diffmind --stdin` is documented, but a diff can + /// reach stdin from `diff -u`, `git format-patch`, or a code-review tool, + /// and then there is no `diff --git` line at all. + /// + /// Both halves of this used to be broken: the first file's header was + /// dropped entirely, and the second file's header arrived while a hunk was + /// open and was counted as removed/added *content* — so two files became one + /// corrupted hunk with no path attached to either. + #[test] + fn a_plain_diff_with_no_git_headers_keeps_its_files_apart() { + let diff = "\ +--- a/src/one.rs ++++ b/src/one.rs +@@ -1,2 +1,2 @@ +-let a = 1; ++let a = 2; +--- a/src/two.rs ++++ b/src/two.rs +@@ -1,2 +1,2 @@ +-let b = 1; ++let b = 2; +"; + let (out, report) = count(diff, &opts()); + + assert_eq!(report.files_total, 2, "two files, not one"); + assert_eq!(report.hunks_total, 2); + assert_eq!(report.hunks_kept, 2); + assert_eq!(out, diff, "a clean plain diff must survive untouched"); + + // The paths have to reach the model, or no finding can be anchored. + assert!(out.contains("+++ b/src/one.rs")); + assert!(out.contains("+++ b/src/two.rs")); + + // And the second file's header must not have been eaten as content. + let files = crate::diff::parse_diff(&out); + let paths: Vec<&str> = files.iter().map(|f| f.path.as_str()).collect(); + assert_eq!(paths, ["src/one.rs", "src/two.rs"]); + } + + /// Classification works off the `+++` path, so the noise rules apply to a + /// piped diff exactly as they do to one diffmind ran itself. + #[test] + fn a_plain_diff_is_still_filtered_by_path() { + let diff = "\ +--- a/pnpm-lock.yaml ++++ b/pnpm-lock.yaml +@@ -1,2 +1,2 @@ +- integrity: sha512-aaa ++ integrity: sha512-bbb +--- a/src/auth.rs ++++ b/src/auth.rs +@@ -1,2 +1,2 @@ +-let token = None; ++let token = Some(secret); +"; + let (out, report) = count(diff, &opts()); + assert_eq!(report.dropped.get(&DropReason::Lockfile), Some(&1)); + assert!(!out.contains("pnpm-lock.yaml")); + assert!(out.contains("+let token = Some(secret);")); + assert!( + out.contains("+++ b/src/auth.rs"), + "the surviving file still needs its header" + ); + } + + /// The pair detection must not fire on ordinary content. A removed SQL + /// comment renders as `--- note`, which is why the `@@` is required too. + #[test] + fn a_removed_sql_comment_is_not_mistaken_for_a_file_header() { + let diff = "\ +diff --git a/q.sql b/q.sql ++++ b/q.sql +@@ -1,4 +1,4 @@ + select 1; +--- old note ++++ new note + select 2; +"; + let (out, report) = count(diff, &opts()); + assert_eq!(report.files_total, 1, "one file, not two"); + assert_eq!(report.hunks_total, 1); + assert_eq!(out, diff, "content must pass through untouched"); + } + #[test] fn empty_input_is_not_an_error() { let (out, report) = count("", &opts()); diff --git a/packages/core-engine/src/suppression.rs b/packages/core-engine/src/suppression.rs index 125f051..8759b22 100644 --- a/packages/core-engine/src/suppression.rs +++ b/packages/core-engine/src/suppression.rs @@ -147,16 +147,49 @@ fn parse_directive(text: &str) -> Option { // Everything up to a closing comment token is the rule list. let rest = rest.split("*/").next().unwrap_or(rest); + // `// diffmind-ignore: DM001` reads naturally; the colon is punctuation. + let rest = rest.trim_start().strip_prefix(':').unwrap_or(rest); + + // Rule ids first, then prose. Everything from the first word that is not + // shaped like a rule id is the author's reason for the suppression, not a + // rule — see `looks_like_rule_id`. let rules: Vec = rest .split([',', ' ', '\t']) - .map(|s| s.trim()) - .filter(|s| !s.is_empty() && s.chars().next().is_some_and(|c| c.is_alphanumeric())) - .map(|s| s.to_string()) + .map(str::trim) + .filter(|s| !s.is_empty()) + .take_while(|s| looks_like_rule_id(s)) + .map(str::to_string) .collect(); Some(Directive { scope, rules }) } +/// Could this word be a rule id, or is it part of a written explanation? +/// +/// Writing *why* a finding is being suppressed is the most natural thing to put +/// after the marker, and it used to silently break the directive: every word of +/// `// diffmind-ignore false positive, validated upstream` was read as a rule +/// id, none of them matched anything, and the suppression quietly did nothing. +/// +/// Every id diffmind issues carries a separator, a digit, or a capital — +/// `DM001`, `DM900.quality`, `rulebook.api`, `custom.no-console` — and the +/// kebab-case convention covers a hand-written `id` in `rules.toml`. A bare +/// lowercase word is prose. Guessing wrong here only ever over-suppresses on a +/// line the author already marked, which is the safe direction: the alternative +/// is a directive that looks right and does nothing. +fn looks_like_rule_id(token: &str) -> bool { + token + .chars() + .next() + .is_some_and(|c| c.is_ascii_alphanumeric()) + && token + .chars() + .all(|c| c.is_ascii_alphanumeric() || matches!(c, '.' | '-' | '_')) + && token + .chars() + .any(|c| c.is_ascii_digit() || c.is_ascii_uppercase() || matches!(c, '.' | '-' | '_')) +} + // ─── Baseline ──────────────────────────────────────────────────────────────── /// Accepted pre-existing findings, so a team can adopt diffmind on a codebase @@ -353,6 +386,60 @@ diff --git a/a.ts b/a.ts assert!(!round_tripped.contains(&other)); } + /// Writing down *why* something is suppressed is the most natural thing to + /// do, and it used to silently disable the directive: every word of the + /// reason was parsed as a rule id, matched nothing, and the finding came + /// straight back with no indication anything was wrong. + #[test] + fn a_written_reason_does_not_break_the_directive() { + let with = |comment: &str| { + let diff = format!( + "diff --git a/a.ts b/a.ts\n--- a/a.ts\n+++ b/a.ts\n@@ -1 +1,2 @@\n+const x = 1; {comment}\n" + ); + InlineSuppressions::from_diff(&parse_diff(&diff)) + }; + + // A bare reason means "all rules", exactly as a bare marker does. + assert!( + with("// diffmind-ignore false positive, x is validated upstream") + .is_suppressed(&finding("a.ts", 1, "DM900.quality")) + ); + + // A rule id followed by a reason still scopes to that rule. + let scoped = with("// diffmind-ignore DM002 -- the caller restores it"); + assert!(scoped.is_suppressed(&finding("a.ts", 1, "DM002"))); + assert!( + !scoped.is_suppressed(&finding("a.ts", 1, "DM001")), + "the reason must not widen the directive to every rule" + ); + } + + #[test] + fn rule_ids_of_every_shape_are_recognised() { + for id in [ + "DM001", + "DM900.quality", + "rulebook.api", + "custom.no-console", + ] { + assert!(looks_like_rule_id(id), "{id} should read as a rule id"); + } + // A hand-written id from rules.toml, by the documented convention. + assert!(looks_like_rule_id("no-console")); + + for prose in ["false", "positive", "upstream", "--", "(intentional)"] { + assert!(!looks_like_rule_id(prose), "{prose} should read as prose"); + } + } + + #[test] + fn a_colon_after_the_marker_is_punctuation_not_a_rule() { + let diff = "diff --git a/a.ts b/a.ts\n--- a/a.ts\n+++ b/a.ts\n@@ -1 +1,2 @@\n+const x = 1; // diffmind-ignore: DM001\n"; + let s = InlineSuppressions::from_diff(&parse_diff(diff)); + assert!(s.is_suppressed(&finding("a.ts", 1, "DM001"))); + assert!(!s.is_suppressed(&finding("a.ts", 1, "DM002"))); + } + #[test] fn apply_reports_how_many_it_hid() { let diff = "diff --git a/a.ts b/a.ts\n--- a/a.ts\n+++ b/a.ts\n@@ -1 +1,2 @@\n+const x = 1; // diffmind-ignore\n"; diff --git a/packages/core-engine/src/unit.rs b/packages/core-engine/src/unit.rs index 4712864..2fe0749 100644 --- a/packages/core-engine/src/unit.rs +++ b/packages/core-engine/src/unit.rs @@ -245,7 +245,8 @@ fn split_files(diff: &str) -> Vec { } } - for line in diff.lines() { + let lines: Vec<&str> = diff.lines().collect(); + for (i, line) in lines.iter().copied().enumerate() { if line.starts_with("diff --git ") { flush(&mut files, &mut hunk); files.push(RawFile { @@ -256,6 +257,24 @@ fn split_files(diff: &str) -> Vec { continue; } + // A `--- old` / `+++ new` / `@@ …` sequence with no `diff --git` above + // it is the only file boundary a plain `diff -u` provides. Without this, + // a piped multi-file diff produced one anonymous unit spanning every + // file, and a finding in it could not be anchored to a path at all. + // See `prefilter::starts_file_header_pair`. + if crate::diff::starts_file_header_pair(&lines, i) + && (files.is_empty() + || hunk.is_some() + || files.last().is_some_and(|f| !f.hunks.is_empty())) + { + flush(&mut files, &mut hunk); + files.push(RawFile { + path: String::new(), + header: Vec::new(), + hunks: Vec::new(), + }); + } + if line.starts_with("@@") { flush(&mut files, &mut hunk); // A hunk with no preceding file header still deserves reviewing; @@ -493,6 +512,36 @@ rename to new.rs ); } + /// A piped diff has no `diff --git` lines, and every unit still has to name + /// a file — anchoring drops a finding whose path it cannot resolve, so an + /// anonymous unit is a unit whose findings are thrown away. + #[test] + fn a_plain_diff_yields_one_named_unit_per_file() { + let diff = "\ +--- a/src/one.rs ++++ b/src/one.rs +@@ -1,2 +1,2 @@ +-let a = 1; ++let a = 2; +--- a/src/two.rs ++++ b/src/two.rs +@@ -1,2 +1,2 @@ +-let b = 1; ++let b = 2; +"; + let units = build_units(diff, 1000); + assert_eq!(units.len(), 2, "two files, two units"); + + let files: Vec<&str> = units.iter().map(|u| u.file()).collect(); + assert_eq!(files, ["src/one.rs", "src/two.rs"]); + + // Each unit carries only its own content, and its own header. + assert!(units[0].text.contains("+let a = 2;")); + assert!(!units[0].text.contains("+let b = 2;")); + assert!(units[1].text.contains("+++ b/src/two.rs")); + assert_ne!(units[0].id, units[1].id); + } + #[test] fn empty_input_yields_no_units() { assert!(build_units("", 1000).is_empty()); From 34738d2dcb72015c807109feafce4267ee9520a0 Mon Sep 17 00:00:00 2001 From: Dennis Paler Date: Wed, 12 Aug 2026 18:03:40 +0800 Subject: [PATCH 3/3] docs: update --- README.md | 490 +++++++++++++++++---------- apps/tui-cli/src/daemon.rs | 4 + apps/tui-cli/src/output.rs | 9 + apps/tui-cli/src/tui.rs | 2 +- packages/core-engine/src/analyzer.rs | 83 ++++- packages/core-engine/src/error.rs | 9 + 6 files changed, 396 insertions(+), 201 deletions(-) diff --git a/README.md b/README.md index d64d96a..c052742 100644 --- a/README.md +++ b/README.md @@ -5,42 +5,59 @@ [![License: MIT](https://img.shields.io/badge/license-MIT-blue.svg)](LICENSE) [![Rust](https://img.shields.io/badge/built%20with-Rust-orange.svg)](https://www.rust-lang.org) -**Diffmind reviews a `git diff` and reports security issues, bugs and quality -problems** — from a single binary, on your machine, with no API key and no -network. It runs as a CI gate, a git hook, or an interactive terminal cockpit -for whoever has to review the branch. +### *Local AI code review, in your terminal. Run it as a CI gate, a git hook, or an interactive review. One Rust binary, no API key.* -The hard part of an automated reviewer is not producing findings. It is -producing them *the same way twice*, keeping false positives from accumulating -until someone deletes the job, and doing it cheaply enough to run on every push. -That is what this is built around. +**Diffmind reviews a `git diff` and reports security issues, bugs and quality +problems** — on your machine, with no API key and no network. All three ways of +running it share one engine, so what fails your build is the same thing you read +in the terminal. + +It does not look at the diff alone. Diffmind builds **its own code graph** of +your repository — tree-sitter, 13 languages, a SQLite file inside your repo — so +a changed function is checked against the code that calls it, not just the +twenty lines around it. Other tools sell this index as a separate product. Here +it is one command, `diffmind index`, and it is free. + +The model is a setting, not a fixed part. **A local model is the default and +stays the default** — bundled, offline, free every run. Point diffmind at your +own Ollama or vLLM server if you want a bigger one, and frontier models are +coming as an opt-in, for the diffs worth paying for. + +The hard part of an automated reviewer is not finding problems. It is finding +them *the same way twice*, keeping false alarms low enough that nobody deletes +the job, and staying cheap enough to run on every push. That is what this is +built around. --- ## When diffmind is the right tool -Reach for it when you need review that is: - -- **Reproducible** — greedy decoding at a fixed seed with a constrained JSON - decoder. The same diff reviews the same way every time. A gate that flags a - line on Tuesday and not on Wednesday gets deleted within a month. -- **Free per run** — no per-token bill, so it can run on every push, on every - fork's PR, on a repo with no budget. -- **Offline and secretless** — nothing leaves the machine. No API key in CI, no - bot account, no third-party data processor to get approved. -- **Stateful** — a baseline, inline suppressions, stable rule IDs and a - recorded accept/wrong ratio. Review debt that a team carries for two years - needs somewhere to live. - -### When to use something else - -Diffmind runs local models by default. **It is not smarter than a frontier -model, and does not try to be.** If you want the deepest possible read of one -tricky diff and you have an interactive agent and a budget, use those — they -will find things diffmind will not. - -The intended shape is both: an agent when you are thinking hard about a single -change, and diffmind on the other several hundred, unattended, for free. +Use it when you need review that is: + +- **Repeatable** — greedy decoding at a fixed seed, with a constrained JSON + decoder. The same diff gets the same review every time. A gate that flags a + line on Tuesday but not on Wednesday gets deleted within a month. +- **Free to run** — no per-token bill, so it can run on every push, on every + fork's PR, and on a repo with no budget. +- **Offline, no secrets** — nothing leaves the machine. No API key in CI, no bot + account, no outside company to get approved first. +- **Persistent** — a baseline, inline suppressions, fixed rule IDs and a + recorded accept/wrong ratio. A team carries review debt for years, so it needs + somewhere to live. + +### About model size + +Diffmind is not a model. It is everything around one — the diff parser, the code +graph, the context budget, the suppression system, the run history, the gate. +**The default 1.5B model is not as smart as a frontier model, and does not +pretend to be.** But the model is the easy part to swap. The rest is the part you +did not want to build. + +So model size is a setting, not a limit. Run the bundled model on all several +hundred diffs for free. Point at a 14B on your own machine for the ones that +matter. When hosted models arrive, pay only for the diff that is worth it. +What never changes is where findings are stored, how you silence them, and how +the gate behaves. --- @@ -48,21 +65,21 @@ change, and diffmind on the other several hundred, unattended, for free. Findings are not all worth the same, and the output says which is which. -**Deterministic findings** — no model involved. Commented-out code (`DM001`), -a declaration removed while still referenced (`DM002`), your own regex rules -(`custom.`). These are pattern-true: they cost nothing, never vary, and -you can gate on them without thinking about it. +**Deterministic findings** — no model involved. Commented-out code (`DM001`), a +declaration deleted while something still uses it (`DM002`), and your own regex +rules (`custom.`). These match a pattern or they don't: they cost nothing, +never change between runs, and you can block on them without worrying. -**Model findings** (`DM900.`) — a model's judgement on a hunk, given -the enclosing function, the callers of what changed, referenced definitions and -the file's tests. Depth scales with the model you run: the default 1.5B is a -lint-grade reviewer that catches obvious mistakes; a 14B behind Ollama reads -much more like a colleague. Both are worth reading. Neither is worth trusting -blindly, which is why the cockpit records when they are wrong. +**Model findings** (`DM900.`) — the model's opinion on a hunk, after +being shown the function it sits in, the callers of what changed, the +definitions it refers to, and the file's tests. How good they are depends on the +model: the default 1.5B catches obvious mistakes, like a linter; a 14B behind +Ollama reads more like a teammate. Both are worth reading. Neither is worth +trusting blindly, which is why interactive review records when they are wrong. -The split is deliberate. **Set `--fail-on` for the tier you trust** — many teams -gate on high-severity deterministic findings and let model findings report -without blocking. +The split is on purpose. **Set `--fail-on` for the tier you trust** — many teams +block on high-severity deterministic findings and let model findings show up +without failing the build. --- @@ -95,6 +112,7 @@ Windows: download `diffmind-x86_64-pc-windows-msvc.zip` from diffmind download # one-time model download (~1.1 GB) diffmind index # build the code graph — recommended, big context win diffmind # review this branch against the repo's default branch +diffmind --tui # same review, stepped through in the terminal ``` ```bash @@ -103,7 +121,7 @@ diffmind --staged # just what's staged diffmind v1.2.0..HEAD # an explicit revision range diffmind src/auth/ # just these paths diffmind v1.2.0..HEAD src/api/ # a range, narrowed to paths -diffmind --tui # the reviewer's cockpit +diffmind --tui # interactive review ``` The base branch is read from `origin/HEAD`, falling back to whichever of @@ -112,19 +130,58 @@ A bare `a..b` argument is recognised as a revision range without `--range`; the detection is strict, so `diffmind ../lib` and a file genuinely named `a..b` are still treated as paths. +**There is really only one command.** `diffmind` on its own does the review. +Everything else is either setup you run once (`download`, `index`, +`install-hooks`, `rules init`) or upkeep you run now and then (`baseline`, +`stats`, `cache`, `serve`). You do not have to configure anything before the +first review works. + --- -## Language support +## The code graph + +Most diff reviewers only see a hunk and a filename. Diffmind indexes the +repository first, so when it reviews a changed function it also sees the code +that calls it. That is the difference between "this line looks wrong" and "this +signature change breaks three callers". + +`diffmind index` scans the repo with tree-sitter and saves definitions and +references into `.diffmind/graph.db`, a normal SQLite file. Each review asks it +four questions: + +| Question | What you get | +| --- | --- | +| Which function contains this line? | The whole function, so a hunk is never read alone | +| What does this hunk define? | The symbols that changed | +| **What calls this symbol?** | Who breaks if it changes — a regex index cannot answer this at all | +| Where is this name defined? | The helpers and types the hunk uses | -Two things here are language-specific, and only one of them limits you. +Three things keep it reliable: -**Review itself works on any text diff** — any language, plus config files, SQL -migrations, shell scripts. The model reads the hunk; the deterministic detectors -and your regex rules run on added lines regardless of extension. +- **It does not store code.** A definition only stores a line range; the source + is read from your files when needed. So the database can never show a snippet + that no longer matches the file being reviewed. +- **It only re-reads what changed.** Re-indexing checks file times and re-parses + just those files. `node_modules`, `target`, `dist`, `.venv` and similar are + skipped. `diffmind index --rebuild` starts fresh, and if the schema changes it + rebuilds itself instead of reading a format it does not understand. +- **It stays local.** No server, no hosted index, no account, nothing to log in + to. It is gitignored by default, and you can rebuild it any time. -**The code graph is the part that needs a grammar.** It supplies the enclosing -definition, the callers of a changed symbol, and referenced definitions — -tree-sitter parsers, 13 languages: +Reference patterns stay narrow on purpose — function calls, constructors, type +names. If it recorded every identifier, "what calls this" would return half the +repo, and an answer that includes everything is as useless as no answer. + +### Languages + +Two things depend on the language, and only one of them limits you. + +**The review works on any text diff** — any language, plus config files, SQL +migrations and shell scripts. The model reads the hunk, and the fixed rules and +your regex rules run on added lines no matter the file type. + +**Only the code graph needs a parser.** Its four questions are answered by +tree-sitter, and diffmind ships 13 of them: | Language | Extensions | | ---------- | -------------------------------- | @@ -142,14 +199,14 @@ tree-sitter parsers, 13 languages: | C++ | `.cpp` `.cc` `.cxx` `.hpp` `.hh` | | Scala | `.scala` `.sc` | -Each gets definitions, references and callers. Files outside the table are still -reviewed — with their hunk, their test file when it is conventionally named, and -your rule sets — just without caller context, so a changed signature is judged on -its own rather than against the code that depends on it. +Each of these gets definitions, references and callers. Files not in the table +are still reviewed — you get the hunk, the test file if it follows the usual +naming, and your rule sets — but without caller information, so a changed +signature is judged on its own instead of against the code that uses it. Adding a language is one entry in the `LANGS` table in -[`apps/tui-cli/src/graph/extract.rs`](apps/tui-cli/src/graph/extract.rs) — -contributions welcome. +[`apps/tui-cli/src/graph/extract.rs`](apps/tui-cli/src/graph/extract.rs). Pull +requests welcome. --- @@ -183,7 +240,8 @@ git diff origin/main...HEAD | diffmind --stdin --format sarif --output diffmind. | `1` | Findings at or above `--fail-on` | | `2` | diffmind itself failed (bad flag, missing model) | -`1` and `2` are distinct, so a crashed binary never looks like a failed review. +`1` and `2` are different on purpose, so a crash never looks like a failed +review. ### Git hooks @@ -192,21 +250,61 @@ diffmind install-hooks --hook pre-push --min-severity high diffmind install-hooks --hook pre-commit ``` -The generated hook exits 0 when diffmind isn't installed, so it never blocks a -teammate who hasn't set it up, and refuses to overwrite a hook it didn't write -unless you pass `--force`. Also available through +The hook it writes exits 0 if diffmind is not installed, so it never blocks a +teammate who has not set it up, and it will not overwrite a hook it did not +write unless you pass `--force`. Also available through [pre-commit](https://pre-commit.com) — `repo: https://github.com/thinkgrid-labs/diffmind`, `id: diffmind`. --- -## The cockpit — `diffmind --tui` - -The gate is public and must be conservative. The cockpit is private, so it can -afford to show you more. +## Interactive review — `diffmind --tui` + +The CI gate is public, so it has to be careful about what it reports. This runs +on your own machine, so it can show you everything. + +It starts reviewing as soon as it opens. Each finding shows the **exact hunk the +model read** and the **context it was given** — because if you cannot check a +finding, sooner or later you stop reading them. + +```text +┌Status────────────────────────────────────────────────────────────────────────┐ +│ diffmind 9 findings — a accept · d dismiss · w wrong │ +└──────────────────────────────────────────────────────────────────────────────┘ +┌Findings (9)────────────────┐┌Detail──────────────────────────────────────────┐ +│> HIGH src/auth/token.rs:47 ││ Severity HIGH │ +│ HIGH …pi/handlers.rs:214 ││ Category Security │ +│✓ MED src/db/pool.rs:112 ││ Location src/auth/token.rs:47 │ +│✗ MED src/api/users.rs:88 ││ Rule DM900.security │ +│· LOW src/util/fmt.rs:12 ││ Confidence 78% │ +│ LOW src/db/pool.rs:130 ││ │ +│ ││ Issue │ +│ ││ validate_token() now returns true for an │ +│ ││ empty string. login() and refresh() both │ +│ ││ gate on it. │ +│ ││ │ +│ ││ Suggested fix │ +│ ││ Reject empty input before the length check. │ +│ ││ │ +│ ││ The diff it reviewed │ +│ ││ -pub fn validate_token(t: &str) -> bool { │ +│ ││ +pub fn validate_token(t: &str, strict: bool) │ +│ ││ - !t.is_empty() │ +│ ││ + t.len() >= 0 │ +│ ││ │ +│ ││ Context it was given │ +│ ││ callers: login() src/auth/session.rs:22 │ +│ ││ refresh() src/auth/session.rs:40 │ +│ ││ │ +│ ││ Suppress with: // diffmind-ignore-next-line │ +│ ││ DM900.security │ +└────────────────────────────┘└────────────────────────────────────────────────┘ + [j/k] Move · [a] Accept+copy · [d] Dismiss · [w] Wrong · [r] Re-run · [q] Quit +``` -Analysis starts on launch. Each finding shows the **actual hunk the model -reviewed** and the **context it was given** — a finding you cannot check is one -you will eventually stop reading. +The left panel lists the findings, marked with the answer you gave (`✓` accept, +`✗` wrong, `·` dismiss). The right panel shows the proof. This is not a summary +of a review that ran somewhere else — it is the review, and the keys below save +straight to `.diffmind/`. | Key | Action | | --------------- | -------------------------------------------------------- | @@ -218,19 +316,19 @@ you will eventually stop reading. | `r` | Re-run | | `q` | Quit | -Verdicts are written through immediately, so closing the terminal mid-triage -loses nothing. Accept copies via **OSC 52**, the terminal's own clipboard -escape — no dependency, and it works over SSH. tmux and screen need clipboard -passthrough; if the copy fails you are told, so you never believe you have -copied something you have not. +Your answers are saved the moment you press the key, so closing the terminal +halfway through loses nothing. Accept copies the comment using **OSC 52**, the +terminal's own clipboard feature — no extra tool needed, and it works over SSH. +tmux and screen need clipboard passthrough turned on; if the copy fails you are +told, so you never think you copied something when you did not. --- ## Keeping the gate alive -Noise is this category's failure mode. One false positive that cannot be -silenced is how a review job gets deleted. Every finding therefore carries a -stable rule ID, shown in the output, so you can silence exactly one thing. +Noise is what kills tools like this. One false alarm that nobody can turn off +is how a review job ends up deleted. So every finding has a fixed rule ID, shown +in the output, and you can turn off exactly that one thing. ### Inline @@ -243,12 +341,12 @@ const x = 1; // diffmind-ignore /* diffmind-ignore-file DM900.maintainability */ ``` -Listing no rule IDs suppresses everything at that location. `DM900` suppresses -every model category without listing each one. +If you list no rule IDs, everything at that spot is silenced. `DM900` silences +every model category at once, so you do not have to list them one by one. ### Baseline -Adopting a reviewer on an existing codebase shouldn't mean fixing everything +Adding a reviewer to an existing codebase should not mean fixing everything first. ```bash @@ -257,13 +355,13 @@ diffmind baseline show diffmind baseline clear ``` -Commit `.diffmind/baseline.json`; future runs report only new issues. The -baseline keys on a content fingerprint rather than a line number, so it survives -unrelated edits above the finding. +Commit `.diffmind/baseline.json`, and later runs report only new issues. The +baseline matches on the content of the finding, not the line number, so editing +unrelated lines above it does not break it. -### Measuring whether it earns its keep +### Checking whether it is worth keeping -Every review is filed to `.diffmind/runs//`. `diffmind stats` reads them +Every review is saved to `.diffmind/runs//`. `diffmind stats` reads them back: ``` @@ -280,29 +378,30 @@ back: 3 rulebook.house-style ``` -The **accept-to-wrong ratio** is the number that decides whether to keep running -it — a reviewer who cannot measure noise will just quietly stop running the -reviewer. Verdicts come from the cockpit. Dismissals are excluded: choosing not -to raise a correct observation is not the tool being wrong. +The **accept-to-wrong ratio** is the number that tells you whether to keep using +it. A team that cannot measure the noise will just quietly stop running the +tool. The answers come from interactive review. Dismissals do not count against +it: deciding a correct point is not worth raising is not the tool being wrong. -Run snapshots are overwritten when a sha is reviewed again; verdicts are -append-only and survive `diffmind stats --clear`, because the ratio is only -meaningful over months. +Saved runs are overwritten when the same commit is reviewed again. Your answers +are only ever added to, and survive `diffmind stats --clear`, because the ratio +only means something over months. -Diffmind writes `.diffmind/.gitignore` covering `runs/`, `cache/`, `models/`, -`graph.db` and `daemon.json` — your review notes stay private, while `rules/`, -`rules.toml`, `config.toml` and `baseline.json` remain committable. Your -repository's own `.gitignore` is never touched. +Diffmind writes its own `.diffmind/.gitignore` covering `runs/`, `cache/`, +`models/`, `graph.db` and `daemon.json`, so your review notes stay private, while +`rules/`, `rules.toml`, `config.toml` and `baseline.json` can still be committed. +Your repository's own `.gitignore` is never touched. --- ## Your team's standards -### Prose rules — `.diffmind/rules/*.md` +### Written rules — `.diffmind/rules/*.md` -Rules that need judgement rather than a pattern. Committed to the repo and read -by the model on every review, so review culture becomes a versioned artifact -instead of tacit knowledge. +For rules that need judgement instead of a pattern. They live in the repo and +the model reads them on every review, so the things your team keeps repeating in +PR comments become a file anyone can read, instead of knowledge only the senior +people carry. ```markdown --- @@ -317,25 +416,26 @@ severity: high - Reject changes that widen a response struct without a version bump. ``` -`scope` globs the files a rule set governs (omit for the whole repo); `severity` -is a **ceiling** for findings attributed to it, never a promotion; `id` defaults -to the file stem. Scaffold with `diffmind rules init`, check what loads with +`scope` is a glob for the files the rule set applies to (leave it out for the +whole repo). `severity` is a **maximum** for findings from that rule set — it can +lower a finding's severity but never raise it. `id` defaults to the filename. +Create a starter file with `diffmind rules init`, and see what loads with `diffmind rules list`. -A finding attributed 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 — a small model will invent a plausible name, and an invented one -could never be suppressed. +A finding from a rule set gets the ID `rulebook.` and can be silenced like +any other. If the model credits a rule set that does not apply to that file, the +credit is dropped — a small model will make up a name that sounds right, and a +made-up name could never be silenced. -Rule bodies go in the *stable* half of the prompt and units are grouped by which -rule sets govern them, so every unit in a group sends a byte-identical prefix. -That is what keeps prompt-prefix caching possible. A rule set that fails to -parse is reported and skipped, never silently ignored. +Rule text goes in the *unchanging* part of the prompt, and reviews are grouped by +which rule sets apply, so every review in a group starts with an identical +prefix. That is what makes prompt-prefix caching possible later. A rule set that +fails to parse is reported and skipped, never ignored quietly. ### Pattern rules — `.diffmind/rules.toml` -Regex, matched against added lines before the model runs: instant, -deterministic, zero inference cost. +Regex, checked against added lines before the model runs: instant, always the +same, and free. ```toml [[rule]] @@ -355,21 +455,32 @@ message. ## Models and backends -**The local model is the default and stays the default.** Everything else here -is opt-in. +**The local model is the default and stays the default.** Everything else is +opt-in. There are three levels, in this order on purpose: + +| Level | What runs | Same result every time | Cost per run | Status | +| --- | --- | --- | --- | --- | +| **Built-in local** | Qwen2.5-Coder GGUF inside diffmind, Metal or CPU | ✅ pinned weights, greedy + constrained decoding | free | **default** | +| **Your own server** | Ollama, vLLM, LM Studio, anything OpenAI-compatible | ✗ | free — your own hardware | shipped | +| **Frontier, hosted** | your key, your provider | ✗ | per token | planned, opt-in | + +Making a hosted model the default would break the whole idea. No key, no +network and no per-token bill is exactly why this can run on every push, on +every fork's PR, and in a repo with no budget. Hosted models are for the diff +you *choose* to spend money on — never the normal case. Models download to `~/.diffmind/models/` — all Qwen2.5-Coder, Q4_K_M quantised. Inference runs on the Apple Silicon GPU via Metal where available, and on CPU (with Accelerate/BLAS) everywhere else. -Every download is **pinned to an immutable commit and verified against a SHA-256 -that ships in the binary**, before the file is moved into place. Writes are -atomic, so an interrupted download cannot leave a truncated file that looks -valid — and weights that are not byte-for-byte what this build expects are -refused rather than loaded. `diffmind download --verify` re-checks an existing -download against the same pins. This is what makes "the same diff produces the -same review" true across machines rather than merely likely: a model id names -exact bytes, not whatever a branch points at today. +Every download is **locked to a fixed commit and checked against a SHA-256 that +ships inside the binary** before the file is put in place. The write is atomic, +so an interrupted download cannot leave a half-written file that looks fine. If +the weights are not byte-for-byte what this build expects, they are rejected +instead of loaded. `diffmind download --verify` re-checks a model you already +have. This is why "the same diff gives the same review" holds on other machines +too: a model name points at exact bytes, not at whatever a branch happens to +contain today. | Model | Size | Min RAM | Use for | | -------------------- | ------- | ------- | ----------------------------------- | @@ -383,10 +494,10 @@ a hardware check; `--model 1.5b --verify` checks an existing download. ### Your own model server -"Local" doesn't have to mean "in this process". If you already run Ollama, vLLM -or LM Studio, point diffmind at it — your code still never leaves your machine -or your network, and you get a model class diffmind will never ship as a 20 GB -download. +"Local" does not have to mean "inside diffmind". If you already run Ollama, vLLM +or LM Studio, just point diffmind at it. Your code still never leaves your +machine or your network, and you get a bigger model than diffmind would ever +ship as a 20 GB download. ```bash diffmind --backend ollama --backend-model qwen2.5-coder:14b @@ -398,18 +509,30 @@ diffmind --backend openai-compatible --backend-url http://localhost:8000/v1 --ba API keys are read from the environment only (`DIFFMIND_API_KEY`, override with `--backend-api-key-env`) — never from the config file, which gets committed. -> **What you trade for a bigger model.** The constrained JSON decoder hooks the -> bundled sampler directly, so remote backends cannot use it: you get better -> judgement and lose the same-diff-same-result guarantee. Token counts reported -> by remote endpoints are estimates, marked `~`. Reproducibility is a property -> of the local path. +> **What a bigger model costs you.** The constrained JSON decoder plugs directly +> into the built-in sampler, so a remote server cannot use it. You get better +> answers, but you lose the guarantee that the same diff gives the same review. +> Token counts from remote servers are estimates, marked `~`. Same-result-every-time +> only applies to the built-in local model. + +### Frontier backends — planned, opt-in + +The plan is small and specific: a built-in Anthropic backend, an opt-in +`claude-cli` backend that reuses a subscription you already pay for, and real +token counts from remote servers instead of estimates. Same rules as everything +else here — added on top, off by default, and clear that you give up the +same-result guarantee. -Hosted backends are planned on the same terms — additive, opt-in, never the -default, and subject to the same trade-off above. +The part that makes it worth paying for is **letting the cheap model decide what +the expensive model reads**. The local model sorts several hundred hunks for +free, then the frontier model looks closely at the few that actually carry risk. +That is very different from sending every diff to an API, and it is the only +version anyone keeps paying for after the first bill. ### Daemon mode -Every invocation otherwise pays the model-load cost — seconds, every time. +Without this, every run pays the model loading cost again — a few seconds, every +time. ```bash diffmind serve # loads the model, unloads after 10 idle minutes @@ -426,49 +549,45 @@ work to it. ## How it works -1. **Parse** — the diff becomes typed per-file hunks with real pre/post-image - line numbers. -2. **Pre-filter** — lockfiles, `linguist-generated` paths, `@generated` banners, +1. **Parse** — the diff is turned into per-file hunks with correct before and + after line numbers. +2. **Filter** — lockfiles, `linguist-generated` paths, `@generated` banners, minified bundles, assets, snapshots, your `ignore` globs and whitespace-only - hunks are dropped. Costs nothing, typically removes most of a real branch, - and the counts are reported rather than silently applied: + hunks are dropped. This is free, usually removes most of a real branch, and + the counts are shown instead of hidden: `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. -3. **Deterministic detectors** — `DM001`, `DM002` and your regex rules. No model - involved. -4. **Context** — assembled per unit from `.diffmind/graph.db`: the enclosing - definition, the callers of every changed symbol, definitions of referenced - symbols, and the corresponding test file. Bounded by a byte budget, so - context does not grow with the repository. Bodies are read from the working - tree, so a snippet can never disagree with the file being reviewed. -5. **Triage** — on large diffs, a cheap first pass decides which files carry real - risk. -6. **Review units** — hunks are grouped into regions of a file rather than cut - wherever a line budget ran out, so related hunks are read together and an - edit in one function only re-reviews that function. When a changed symbol and - a changed caller both appear, the two are merged into one unit and reviewed - together. Units are sized to the backend's *actual* context window, read from - the GGUF metadata. -7. **Constrained decoding** — the sampler consults a JSON state machine before - committing each token, so the model cannot emit a preamble, an unbalanced - brace or a truncated string. Output that hits the token cap is repaired - rather than discarded. -8. **Anchoring** — findings pointing at a file not in the diff are dropped; - off-by-N line numbers snap to the nearest changed line. -9. **Suppression** — inline directives, the baseline and `--min-confidence` are - applied, then results are deduplicated and sorted. - -The **code graph** behind step 4 is the tree-sitter symbol index from -[Language support](#language-support), kept in `.diffmind/graph.db` and updated -incrementally by mtime. Build it with `diffmind index`. + Whitespace inside a string still counts as a real change, and indentation is + never dropped in Python or YAML. +3. **Fixed rules** — `DM001`, `DM002` and your regex rules. No model involved. +4. **Context** — built for each review from `.diffmind/graph.db`: the function + the hunk is in, the callers of every changed symbol, the definitions it + refers to, and the matching test file. There is a size limit, so context does + not grow as the repo grows. Code is read from your files, so a snippet can + never disagree with the file being reviewed. +5. **Sort by risk** — on large diffs, a quick first pass decides which files + actually carry risk. +6. **Group** — hunks are grouped by area of the file rather than cut wherever a + line limit ran out, so related hunks are read together, and editing one + function only re-reviews that function. If a changed symbol and one of its + changed callers both appear, they are reviewed together as one. Group size + matches the model's *real* context window, read from the GGUF file. +7. **Constrained decoding** — before each token, the sampler checks a JSON state + machine, so the model cannot write an intro sentence, an unbalanced brace or a + cut-off string. Output that hits the token limit is repaired, not thrown away. +8. **Line matching** — findings that point at a file not in the diff are dropped, + and line numbers that are slightly off snap to the nearest changed line. +9. **Silencing** — inline comments, the baseline and `--min-confidence` are + applied, then duplicates are removed and the results sorted. + +Step 4 is [the code graph](#the-code-graph). It is the step that separates this +from a reviewer that only ever sees the diff. --- ## Configuration -`.diffmind/config.toml` — precedence is CLI flag > config file > default. -Unknown keys are reported rather than silently ignored. +`.diffmind/config.toml`. A CLI flag beats the config file, and the config file +beats the default. Keys diffmind does not recognise are reported, not ignored. ```toml [review] @@ -490,14 +609,14 @@ model = "qwen2.5-coder:14b" api_key_env = "DIFFMIND_API_KEY" ``` -### Options that matter +### The options you will actually use -| Flag | Effect | +| Flag | What it does | | ----------------------- | -------------------------------------------------- | | `-b, --branch` | Base branch [default: the repo's default branch] | | `-m, --model` | `0.5b` … `32b` [default: `1.5b`] | | `-l, --last` / `--staged` / `--range` / `--stdin` | What to review | -| `-t, --tui` | Launch the cockpit | +| `-t, --tui` | Start an interactive review | | `--min-severity` | Minimum severity to report [default: `low`] | | `--fail-on` | Severity causing exit 1 [default: `--min-severity`] | | `--min-confidence` | Minimum confidence to report, 0.0–1.0 | @@ -510,21 +629,24 @@ api_key_env = "DIFFMIND_API_KEY" `--triage`, `--temperature`, `--seed`, `--max-tokens`, `--device` and `--debug` are in `diffmind --help`. -The binary also carries two conveniences that are not the point of the tool: -`diffmind describe` (a PR title and summary) and `diffmind commit` (a -conventional commit message for staged changes). Use them if they're handy; they -are not what diffmind is for. +Two extras come along because the model is already loaded: `diffmind describe` +(a PR title and summary) and `diffmind commit` (a conventional commit message +for staged changes). Useful, but review is the product — nothing else here is +built around them. --- ## Roadmap -- Hosted backends — opt-in only, with the local model still the default -- Auto-fix patches (`diffmind fix`) — gated on those backends, since a 1.5B's - patches are not trustworthy enough to apply -- Cheap-model triage feeding a strong-model deep pass, and prompt-prefix caching -- Learning from verdicts: marking a finding wrong should suppress its kind next - time, not merely count it +- **Frontier models, opt-in** — a built-in Anthropic backend and a `claude-cli` + backend, with the local model still the default +- **Cheap model picks what the strong model reads**, plus prompt-prefix caching — + this is what makes a paid model affordable +- **Auto-fix patches** (`diffmind fix`) — waiting on those backends, because a + 1.5B's patches are not safe enough to apply +- **Learn from your answers** — marking a finding wrong should stop that kind of + finding next time, not just count it +- More languages in the code graph, one `LANGS` entry at a time - VS Code / JetBrains extensions, talking to the daemon - Homebrew tap and scoop manifest diff --git a/apps/tui-cli/src/daemon.rs b/apps/tui-cli/src/daemon.rs index dbe0ab0..67a7bee 100644 --- a/apps/tui-cli/src/daemon.rs +++ b/apps/tui-cli/src/daemon.rs @@ -148,6 +148,8 @@ pub struct SerializableStats { pub tokens_estimated: bool, pub units_cached: usize, pub units_unparseable: usize, + #[serde(default)] + pub units_too_large: usize, pub files_skipped_by_triage: usize, pub suppressed: usize, pub below_confidence: usize, @@ -164,6 +166,7 @@ impl From<&AnalysisStats> for SerializableStats { tokens_estimated: s.tokens_estimated, units_cached: s.units_cached, units_unparseable: s.units_unparseable, + units_too_large: s.units_too_large, files_skipped_by_triage: s.files_skipped_by_triage, suppressed: s.suppressed, below_confidence: s.below_confidence, @@ -182,6 +185,7 @@ impl From for AnalysisStats { tokens_estimated: s.tokens_estimated, units_cached: s.units_cached, units_unparseable: s.units_unparseable, + units_too_large: s.units_too_large, files_skipped_by_triage: s.files_skipped_by_triage, suppressed: s.suppressed, below_confidence: s.below_confidence, diff --git a/apps/tui-cli/src/output.rs b/apps/tui-cli/src/output.rs index d0656e0..bea1257 100644 --- a/apps/tui-cli/src/output.rs +++ b/apps/tui-cli/src/output.rs @@ -161,6 +161,14 @@ pub fn print_footer(shown: usize, gated: usize, stats: &AnalysisStats) { plural(stats.units_unparseable) ); } + if stats.units_too_large > 0 { + eprintln!( + " {} {} unit{} too large for the context window — skipped, not reviewed", + "!".yellow(), + stats.units_too_large, + plural(stats.units_too_large) + ); + } if stats.suppressed > 0 { eprintln!( " {} {} finding{} suppressed (inline comments or baseline)", @@ -255,6 +263,7 @@ pub fn render( "units": stats.units_total, "units_cached": stats.units_cached, "units_unparseable": stats.units_unparseable, + "units_too_large": stats.units_too_large, "inference_ms": stats.inference_ms, "prompt_tokens": stats.prompt_tokens, "completion_tokens": stats.completion_tokens, diff --git a/apps/tui-cli/src/tui.rs b/apps/tui-cli/src/tui.rs index a1d4fb9..0a2d8f3 100644 --- a/apps/tui-cli/src/tui.rs +++ b/apps/tui-cli/src/tui.rs @@ -1,4 +1,4 @@ -//! The reviewer's cockpit. +//! Interactive review — the terminal UI behind `diffmind --tui`. //! //! This is the surface the whole tool is for: not a report, but a place to sit //! while deciding what to say about someone else's branch. Three things follow diff --git a/packages/core-engine/src/analyzer.rs b/packages/core-engine/src/analyzer.rs index 0725c60..8a1da68 100644 --- a/packages/core-engine/src/analyzer.rs +++ b/packages/core-engine/src/analyzer.rs @@ -32,6 +32,14 @@ const MAX_CHUNK_LINES: usize = 1200; /// How many times a chunk may be halved when its prompt overruns the window. const MAX_SPLIT_DEPTH: u32 = 4; +/// Ceiling on either optional prompt section. Past this the model's attention is +/// the binding constraint rather than the window. +const MAX_SECTION_BYTES: usize = 12_000; +/// Floor for an optional section, below which it is not worth carrying. +const MIN_SECTION_BYTES: usize = 400; +/// Ticket text is a fixed brief, not something that grows with the diff. +const MAX_REQUIREMENTS_BYTES: usize = 2000; + /// Triage only pays for itself once a diff spans enough files that skipping /// some saves more than the extra inference pass costs. const TRIAGE_MIN_FILES: usize = 6; @@ -63,6 +71,9 @@ pub struct AnalysisStats { pub units_cached: usize, /// Units whose output could not be parsed even after repair. pub units_unparseable: usize, + /// Units skipped because no amount of splitting fit them in the window. + /// Distinct from `units_unparseable`: the model was never asked. + pub units_too_large: usize, pub files_skipped_by_triage: usize, pub suppressed: usize, pub below_confidence: usize, @@ -312,25 +323,55 @@ impl ReviewAnalyzer { (available / TOKENS_PER_DIFF_LINE).clamp(MIN_CHUNK_LINES, MAX_CHUNK_LINES) } - fn build_prompt(&self, diff: &str, context: &str, rulebooks: &[&Rulebook]) -> Prompt { + fn build_prompt( + &self, + diff: &str, + context: &str, + rulebooks: &[&Rulebook], + max_new_tokens: usize, + depth: u32, + ) -> Prompt { + let budget = self.section_budget_bytes(max_new_tokens, depth); review_prompt(&ReviewPromptInput { diff, context, languages: self.languages.as_deref(), requirements: self.requirements.as_deref(), rulebooks, - max_context_bytes: self.context_budget_bytes(), - max_requirements_bytes: 2000, - max_rules_bytes: self.context_budget_bytes(), + max_context_bytes: budget, + max_requirements_bytes: budget.min(MAX_REQUIREMENTS_BYTES), + max_rules_bytes: budget, }) } - /// Byte budget for the RAG/context section, scaled to the window rather - /// than pinned at the 2 KB a 4K window demanded. - fn context_budget_bytes(&self) -> usize { - let ctx = self.backend.context_tokens(); - // Roughly a sixth of the window, three bytes per token. - ((ctx / 6) * 3).clamp(1500, 12_000) + /// Byte budget for each of the prompt's optional sections — the symbol + /// context and the project rules — at recursion `depth`. + /// + /// Two properties, both learned the hard way. + /// + /// **The diff keeps at least half the window.** These sections used to be + /// sized from the window alone, so a large `.diffmind/rules/` could claim + /// 12 KB and the context another 12 KB regardless of what was left for the + /// thing actually under review. + /// + /// **The budget shrinks with depth.** When a prompt overruns, the analyzer + /// halves the *diff* and retries — which cannot help when the fixed sections + /// are what overran. A big enough rule set therefore failed identically at + /// every recursion level and gave up at the depth cap, having never had a + /// chance. Shrinking here is what makes the retry mean something. + /// [`crate::prompt`] drops whole rule sets that no longer fit rather than + /// truncating one mid-sentence, so this degrades to "fewer rules", then to + /// "no rules" — never to half a rule that reads like a complete one. + fn section_budget_bytes(&self, max_new_tokens: usize, depth: u32) -> usize { + let available = self + .backend + .context_tokens() + .saturating_sub(max_new_tokens + SYSTEM_PROMPT_TOKENS); + // Half of what is left, shared by the two sections, at ~3 bytes a token. + let share = ((available / 4) * 3).min(MAX_SECTION_BYTES); + // The floor never exceeds the share, or a window too small to afford it + // would be pushed further over by the very thing meant to protect it. + (share >> depth.min(16)).max(MIN_SECTION_BYTES.min(share)) } /// True when the prompt fits the window with room for the response. @@ -458,6 +499,16 @@ impl ReviewAnalyzer { } stats.units_unparseable += 1; } + // Counted and skipped, not fatal. A hunk nobody can fit in the + // window is a fact about that hunk; failing the run over it + // would throw away every other unit's findings — including the + // deterministic ones, which never needed a model at all. + Err(EngineError::UnitTooLarge(why)) => { + if self.debug { + eprintln!("[debug] unit {} ({}) skipped: {why}", i + 1, unit.file()); + } + stats.units_too_large += 1; + } Err(e) => return Err(e), } } @@ -573,15 +624,15 @@ impl ReviewAnalyzer { } let context = context_for(chunk); - let prompt = self.build_prompt(chunk, &context, rulebooks); + let prompt = self.build_prompt(chunk, &context, rulebooks, max_tokens as usize, depth); if !self.prompt_fits(&prompt, max_tokens as usize) { if depth >= MAX_SPLIT_DEPTH { - return Err(EngineError::ForwardError( - "a single diff hunk is too large for the model's context window even after \ - splitting; review that file on its own" - .into(), - )); + return Err(EngineError::UnitTooLarge(format!( + "still {} bytes of diff after {MAX_SPLIT_DEPTH} splits, with every \ + optional section already at its minimum", + chunk.len() + ))); } // Halve and recurse rather than truncate: silently dropping half a // hunk means silently not reviewing it. Each half re-derives its own diff --git a/packages/core-engine/src/error.rs b/packages/core-engine/src/error.rs index 9d769c4..5bec169 100644 --- a/packages/core-engine/src/error.rs +++ b/packages/core-engine/src/error.rs @@ -18,6 +18,15 @@ pub enum EngineError { SamplingError(String), #[error("serialization error: {0}")] SerializationError(String), + /// A review unit could not be made to fit the model's context window, even + /// after splitting and after shrinking everything optional in the prompt. + /// + /// Reported as an error so the analyzer can tell it apart from a chunk that + /// merely came back unparseable, but it is *counted and skipped* rather than + /// failing the run: one unreviewable hunk must not discard the findings + /// every other hunk produced. + #[error("review unit does not fit the context window: {0}")] + UnitTooLarge(String), #[error("io error: {0}")] Io(String), /// A remote backend (Ollama, OpenAI-compatible) failed to answer.