rto_graph/reviewer.rs
1//! The reviewer's pure core: what to ask, how to read the answer, and what the
2//! answer is allowed to claim (Stage 35b).
3//!
4//! [`crate::review_score`] built the instrument; this is the thing it measures.
5//! Everything here is a pure function of bytes — prompt assembly, response
6//! parsing, budget arithmetic and the [`crate::compile_claim`] site derivation —
7//! so the whole of the reviewer's *judgement* is testable offline, with no model,
8//! no network and no git. What is left outside is a loop that calls an engine,
9//! and that lives in the binary.
10//!
11//! # Per file, and the budget is not the constraint
12//!
13//! 35a established that whole-diff review is not the shape: reconstructing the 15
14//! review diffs costs ~513k tokens, ~34k mean, and **9 of 15 exceed the ~30k
15//! single-call budget**. What that argument left open is how much room *per-file*
16//! review actually has, and the answer is: a great deal. Measured over all **190
17//! file-diffs** in the corpus:
18//!
19//! | | raw | annotated |
20//! |---|---|---|
21//! | mean | 2,704 | 3,275 |
22//! | median | 1,476 | 1,758 |
23//! | p90 | 5,621 | 6,711 |
24//!
25//! The second column is what a review actually sends, because [`annotate_diff`]
26//! adds a line-number column. That costs a measured **1.21×** over the corpus —
27//! and the shape of the cost is worth knowing: it is 9 characters per *line*, so
28//! it is ~1.2× on ordinary source and would be ~4× on a diff of two-character
29//! lines. It is charged against the budget rather than estimated around.
30//!
31//! Even so, exactly **one of the 190** exceeds the single-call budget annotated,
32//! and it is a generated JSON fixture; the next largest is `Cargo.lock`. **The
33//! largest reviewable *source* file-diff in the entire corpus is 14,034 tokens
34//! raw and 17,202 annotated** — still under two-thirds of the single-call budget.
35//! So the median file leaves ~28k of that budget unused and the worst source file
36//! leaves ~12k.
37//!
38//! That matters for one reason. The stage's central claim is that pre-assembled
39//! graph context lets a per-file reviewer see a doc in *another* file
40//! contradicting the code under review — the `contract-drift` class. Had the
41//! per-file budget been tight, that claim would have been untestable on this
42//! repository whatever the graph contained. It is not tight. [`GraphContext`] is
43//! the slot that headroom is for, and this module reserves it while shipping it
44//! empty: PR 1 measures the diff-only arm, and a filled slot is the comparison.
45//!
46//! # The prompt is derived from the standards, not from the corpus
47//!
48//! [`build_prompt`] states the house's review standards — contract accuracy, the
49//! defect vocabulary, the output shape — from `docs/REVIEW_CHECKLIST.md` and
50//! [`crate::review_corpus::DefectClass`], both of which predate it. It is
51//! deliberately **not** written against the corpus rows.
52//!
53//! This is a property of the experiment rather than a style preference. A prompt
54//! tuned until the known rows are found measures how well it was tuned, and the
55//! resulting recall would not survive the 23rd row. The rows are the test set and
56//! nothing here may read them, which is why this module depends on `DefectClass`
57//! and not on [`crate::review_corpus::BUILTIN`].
58//!
59//! # Nothing here decides what is true
60//!
61//! [`parse_findings`] converts what a model said into
62//! [`crate::review_score::CandidateFinding`]s and no further. It does not check a
63//! finding, rank it, or drop it for looking implausible. The one filter in this
64//! module is [`crate::compile_claim`]'s, and even that is applied by the caller
65//! against evidence the caller supplies — see [`claim_site`], which only computes
66//! *what configuration the code needs*, never whether a check ran.
67
68use std::fmt::Write as _;
69
70use crate::compile_claim::{ClaimSite, Features, TargetOs};
71use crate::review_corpus::{CLASSES, DefectClass};
72use crate::review_score::CandidateFinding;
73
74/// The measured single-call context budget on this repository, in tokens.
75///
76/// 35a's figure, and the one 189 of the corpus's 190 file-diffs fit inside. Used
77/// as the default per-file budget because a per-file reviewer that also fits the
78/// single-call budget needs no second number to explain.
79pub const SINGLE_CALL_BUDGET_TOKENS: usize = 30_000;
80
81/// Estimate a string's token count as `len / 4`.
82///
83/// The same basis every budget figure in this stage is quoted on — 35a's
84/// corpus-wide totals and this module's per-file distribution alike — so the
85/// numbers compare. It is an estimate of the right order, **not** a tokeniser's
86/// count, and is deliberately not swapped for one: a real count would need the
87/// model's vocabulary, which would make a pure function depend on which model is
88/// installed and make two runs on two machines incomparable.
89#[must_use]
90pub fn estimate_tokens(text: &str) -> usize {
91 text.len() / 4
92}
93
94/// One file to review, with the diff that changed it.
95#[derive(Debug, Clone, PartialEq, Eq)]
96pub struct FileUnderReview {
97 /// The commit being reviewed — a corpus `reviewed_sha` on a replay run.
98 pub reviewed_sha: String,
99 /// Repository-relative path.
100 pub path: String,
101 /// The unified diff for this file alone.
102 pub diff: String,
103}
104
105/// One piece of pre-assembled, provenance-tagged context for the file under
106/// review — **the slot that PR 1 ships empty**.
107///
108/// The graph's contribution to a review is not access: an agentic reviewer can
109/// read any file it likes via tool calls, and one has been observed doing so
110/// correctly on this very corpus. It is that the relevant context arrives
111/// *already selected and already labelled with where it came from*, so the model
112/// spends its budget reading rather than searching.
113///
114/// [`provenance`](Self::provenance) is carried into the prompt rather than
115/// flattened away because the three layers mean different things to a reviewer: a
116/// `derived` fact is a deterministic function of the bytes, an `authored` one is
117/// somebody's stated intent, and an `inferred` one is a guess with a confidence.
118/// A reviewer told an ADR *governs* a symbol is being told something different
119/// from a reviewer handed a similar-looking file.
120#[derive(Debug, Clone, PartialEq, Eq)]
121pub struct ContextItem {
122 /// What this is, for the prompt — `ADR-0019 §3`, or `callers of resolve`.
123 pub label: String,
124 /// `derived` | `authored` | `inferred` — the graph's own vocabulary.
125 pub provenance: String,
126 /// The text itself.
127 pub body: String,
128}
129
130/// The hard ceiling on a context block, in estimated tokens.
131///
132/// PR 1 measured ~28k of the single-call budget free on a median file, and that
133/// headroom is what made this arm testable at all. It is **permission, not an
134/// instruction**: a prompt in which the diff is 6% of the tokens is a
135/// needle-in-a-haystack task, and handing a model 28k of loosely-related text is
136/// a plausible way to make both recall *and* the finding rate worse.
137///
138/// So the cap is stated as a constant, before any number was seen, rather than
139/// discovered by watching a score move.
140pub const CONTEXT_CAP_TOKENS: usize = 4_000;
141
142/// How much context one file may carry, relative to its own diff.
143///
144/// The cap is `min(CONTEXT_CAP_TOKENS, RELATIVE * diff_tokens)`, so a two-line
145/// change does not arrive under a thousand lines of ADR. A reviewer should be
146/// reading the change, with context beside it — not the other way round.
147pub const CONTEXT_RELATIVE_TO_DIFF: usize = 2;
148
149/// The graph context handed to one file's review.
150///
151/// [`GraphContext::none`] is the diff-only arm, and [`build_prompt`] renders no
152/// context section for it, so the baseline prompt carries no vestigial heading
153/// promising something that is not there.
154///
155/// # What is in it, and why those and not the rest
156///
157/// The menu `roteiro review` already computes is governing ADRs, callers,
158/// callees, blast radius and authored drift. The arm takes **two** of those and
159/// records the omission of the others as a decision:
160///
161/// * **Governing ADR and blueprint sections** (`authored`). The one thing a
162/// per-file reviewer structurally cannot obtain: a decision written in another
163/// file that the code under review contradicts. This is `contract-drift`'s
164/// defining shape and the authored layer's whole purpose.
165/// * **The file's own doc surface outside the diff** (`derived`). [`build_prompt`]
166/// instructs the model to stay silent unless *both* halves of a conflict are
167/// visible, and a `-U3` diff shows at most three lines either side. A doc
168/// comment at the top of a file and the code that betrays it 700 lines down are
169/// never both in the hunks. The graph holds each symbol's doc comment, so this
170/// makes the promise-half visible without pasting the file.
171/// * **Callers, callees and blast radius are deliberately excluded.** Measured on
172/// this repository a single changed symbol carries dozens of caller keys, and
173/// their bodies would dominate the prompt — the failure mode above, bought
174/// knowingly. Recorded here so the absence reads as a decision rather than an
175/// oversight, and so a later arm that adds them knows it is changing a
176/// pre-registered variable.
177#[derive(Debug, Clone, Default, PartialEq, Eq)]
178pub struct GraphContext {
179 /// The items, in the order they are rendered.
180 pub items: Vec<ContextItem>,
181 /// Items dropped by [`GraphContext::fit`] to stay inside the cap.
182 ///
183 /// Counted rather than silently absorbed, for the same reason
184 /// [`Prompt::dropped_tokens`] is: a run that quietly shed the context it was
185 /// measuring would report the graph arm as having been tested when it was
186 /// partly the diff-only arm wearing its name.
187 pub dropped_items: usize,
188}
189
190impl GraphContext {
191 /// No context — the diff-only arm.
192 #[must_use]
193 pub fn none() -> Self {
194 Self::default()
195 }
196
197 /// Whether any context is present.
198 #[must_use]
199 pub fn is_empty(&self) -> bool {
200 self.items.is_empty()
201 }
202
203 /// Estimated tokens this context will cost, by [`estimate_tokens`] over the
204 /// text [`build_prompt`] actually renders — label, provenance tag and body.
205 #[must_use]
206 pub fn tokens(&self) -> usize {
207 self.items.iter().map(ContextItem::tokens).sum()
208 }
209
210 /// Drop whole items, lowest-priority last-first, until the context fits the
211 /// cap for a diff of `diff_tokens`.
212 ///
213 /// **Whole items, never a truncated one.** A half-quoted ADR is worse than no
214 /// ADR: it reads as a complete statement of a decision and is not one, so a
215 /// model can be handed a promise whose exception was cut off and report the
216 /// code as contradicting it. [`build_prompt`] makes the same choice for the
217 /// same reason and truncates only the diff.
218 ///
219 /// Callers pass items in priority order — most valuable first — because this
220 /// drops from the tail.
221 #[must_use]
222 pub fn fit(items: Vec<ContextItem>, diff_tokens: usize) -> Self {
223 let cap = CONTEXT_CAP_TOKENS.min(CONTEXT_RELATIVE_TO_DIFF.saturating_mul(diff_tokens));
224 let mut kept: Vec<ContextItem> = Vec::new();
225 let mut spent = 0usize;
226 let mut dropped = 0usize;
227 for item in items {
228 let cost = item.tokens();
229 // A later, smaller item is still allowed in after a large one was
230 // refused: the cap is on the block, and skipping an oversized ADR
231 // should not also discard the three short doc comments behind it.
232 if spent + cost > cap {
233 dropped += 1;
234 continue;
235 }
236 spent += cost;
237 kept.push(item);
238 }
239 Self {
240 items: kept,
241 dropped_items: dropped,
242 }
243 }
244}
245
246impl ContextItem {
247 /// What this item costs in the prompt, by [`estimate_tokens`].
248 ///
249 /// Counts the rendered form — the `--- label [provenance]` heading as well as
250 /// the body — because that is what the budget actually pays for.
251 #[must_use]
252 pub fn tokens(&self) -> usize {
253 estimate_tokens(&self.label)
254 + estimate_tokens(&self.provenance)
255 + estimate_tokens(&self.body)
256 + 4
257 }
258}
259
260/// The body of one `## ` section of a markdown document, **by its heading title**.
261///
262/// The graph stores an `adr_section` node per heading but **no body text** — the
263/// node carries a slug, a title and nothing else. So the graph selects *which*
264/// decision governs the code under review, and this renders it. That split is the
265/// point: retrieval is the graph's, quoting is a string operation, and nothing
266/// here decides relevance.
267///
268/// # Matched on the title, not on the slug, and that is deliberate
269///
270/// An `adr_section` key is `adr:0005#decision` — the slug is right there, so
271/// matching on it looks like the obvious read. It would mean reimplementing
272/// `rto_spec`'s slug rule here, because that rule is `pub(crate)` and `rto-graph`
273/// does not depend on `rto-spec` at all. A second copy of a rule this crate cannot
274/// see is a rule free to drift, and the drift would be silent: a heading whose
275/// punctuation the two versions collapsed differently would simply stop resolving,
276/// and the graph arm would quietly run with one fewer ADR than it reported.
277///
278/// The node's `name` is the heading text verbatim, so comparing titles needs no
279/// shared rule and cannot drift. Where two `## ` headings share a title the first
280/// wins; nothing downstream distinguishes them either.
281///
282/// Returns everything after the heading up to the next `## `, or `None` when no
283/// heading matches.
284#[must_use]
285pub fn section_body(markdown: &str, title: &str) -> Option<String> {
286 let mut out: Option<String> = None;
287 for line in markdown.lines() {
288 if let Some(heading) = line.strip_prefix("## ") {
289 if out.is_some() {
290 break;
291 }
292 if heading.trim() == title.trim() {
293 out = Some(String::new());
294 }
295 continue;
296 }
297 // A `# ` title or a deeper `### ` subheading is body, not a boundary: only
298 // `## ` delimits the sections the graph made nodes for.
299 if let Some(body) = out.as_mut() {
300 body.push_str(line);
301 body.push('\n');
302 }
303 }
304 out.map(|b| b.trim().to_owned())
305}
306
307/// How much of a doc comment must already be visible in the diff before quoting
308/// it again is redundant.
309///
310/// Compared on the first `PROBE` significant characters rather than the whole
311/// text: the two copies are never byte-identical, because the diff's is wrapped,
312/// numbered and comment-marked while the graph's is the extracted prose.
313const DOC_PROBE_CHARS: usize = 60;
314
315/// Reduce text to the characters two copies of the same doc comment share.
316///
317/// Whitespace, `annotate_diff`'s line-number column, and Rust's comment markers
318/// all differ between the diff's rendering of a doc and the graph's, and none of
319/// them carry meaning for this comparison. Dropping them is what lets a doc
320/// wrapped across three numbered `///` lines match the single paragraph the
321/// extractor stored.
322fn doc_signature(text: &str, strip_annotation_column: bool) -> String {
323 let mut out = String::with_capacity(text.len());
324 for line in text.lines() {
325 // `annotate_diff` renders `{n:>6} +|body`, ` - |body` and
326 // `{n:>6} |body`, so the content is whatever follows the first `|`. A
327 // source line containing its own `|` is unaffected: the column's comes
328 // first. Lines before the first hunk carry no column and are taken whole.
329 let body = if strip_annotation_column {
330 line.split_once('|').map_or(line, |(_, rest)| rest)
331 } else {
332 line
333 };
334 let body = body.trim_start();
335 let body = body
336 .strip_prefix("///")
337 .or_else(|| body.strip_prefix("//!"))
338 .or_else(|| body.strip_prefix("//"))
339 .unwrap_or(body);
340 out.extend(body.chars().filter(|c| !c.is_whitespace()));
341 }
342 out
343}
344
345/// Whether `doc` is already visible in `annotated_diff`, so quoting it again
346/// would spend budget on text the model can already read.
347///
348/// **The point of the graph arm is the doc that is *not* in the hunks.**
349/// [`build_prompt`] tells the model to stay silent unless both halves of a
350/// conflict are visible, and a `-U3` diff shows three lines either side — so a
351/// module doc at line 16 and the code that betrays it at line 700 are never both
352/// shown. Re-sending the halves that *are* shown would inflate the context block
353/// with duplicates and buy nothing; worse, on a cap that drops whole items it
354/// would evict the ones that matter.
355///
356/// `annotated_diff` is the diff **as the model sees it** — [`annotate_diff`]'s
357/// output, line-number column and all — because "already visible" is a claim about
358/// the prompt, not about the raw hunks.
359#[must_use]
360pub fn doc_already_shown(doc: &str, annotated_diff: &str) -> bool {
361 let probe: String = doc_signature(doc, false)
362 .chars()
363 .take(DOC_PROBE_CHARS)
364 .collect();
365 // Too short to identify anything, and too short to state a contract that could
366 // drift: treated as shown rather than padding the context with one-word docs.
367 if probe.chars().count() < DOC_PROBE_CHARS {
368 return true;
369 }
370 doc_signature(annotated_diff, true).contains(&probe)
371}
372
373/// An assembled prompt, with what it cost and what it had to leave out.
374#[derive(Debug, Clone, PartialEq, Eq)]
375pub struct Prompt {
376 /// The text to send.
377 pub text: String,
378 /// Its estimated size, by [`estimate_tokens`].
379 pub tokens: usize,
380 /// Diff tokens dropped to fit the budget, or `0`.
381 ///
382 /// Reported rather than silently absorbed: a review of a truncated file is a
383 /// review of part of it, and a run that does not say so reads as coverage it
384 /// did not have.
385 pub dropped_tokens: usize,
386}
387
388/// The output contract, stated once and used twice — [`build_prompt`] asks for
389/// this and [`parse_findings`] reads it.
390///
391/// A line format rather than JSON, because the failure modes are not symmetric.
392/// A model that mangles one line of a line format loses that finding; a model
393/// that mangles one brace of a JSON document loses the whole review, and the
394/// low-tier instruct model this resolver defaults to does the second more often
395/// than the first.
396const FINDING_PREFIX: &str = "FINDING";
397
398/// What a model emits when it has nothing to report — required, so that "no
399/// findings" and "the model ignored the format" are distinguishable in
400/// [`Parsed::unparsed`] rather than both arriving as silence.
401const NO_FINDINGS: &str = "NO FINDINGS";
402
403/// Build the review prompt for one file.
404///
405/// `budget` caps the whole prompt; the diff is truncated to fit and the amount
406/// dropped is reported on [`Prompt::dropped_tokens`]. Context is never truncated —
407/// a half-quoted ADR is worse than none, and on the measured distribution it never
408/// comes to that.
409#[must_use]
410pub fn build_prompt(file: &FileUnderReview, context: &GraphContext, budget: usize) -> Prompt {
411 let mut head = String::new();
412 head.push_str(
413 "You are reviewing ONE FILE of a change to a Rust codebase.\n\n\
414 The defects that matter here are **contract-accuracy** defects: code that \
415 runs correctly but does not mean what it says. They compile, they pass \
416 tests, and CI is green on them by definition — so they are found only by \
417 reading the words against the behaviour. Do not look for crashes or \
418 compile errors; look for places where a promise and its implementation \
419 have come apart.\n\n\
420 Work through the change against each of these, in order:\n\n",
421 );
422 for class in CLASSES {
423 let _ = writeln!(head, " {} — {}", class.as_str(), class_gloss(class));
424 }
425 head.push_str(
426 "\nFor each one, ask specifically:\n\
427 - Does a doc comment, `///` line, README sentence or ADR in this diff \
428 state something the code beside it does not do? Compare the two texts \
429 word by word — a doc that describes the old behaviour after the code \
430 moved on is the single most common defect in this codebase.\n\
431 - Does an error message name the rule it actually enforces, or a \
432 different one?\n\
433 - Does a test assert the behaviour its name claims, or would it pass with \
434 the feature removed?\n\
435 - Does a check permit the state it exists to forbid (off-by-one, wrong \
436 comparison, missing case)?\n\
437 - Is a key, hash or id built from something lossy, so two different \
438 inputs collide?\n\n\
439 Output format. One finding per line, nothing else on the line:\n\
440 \x20 FINDING | line=<n> | class=<class> | compile=<yes|no> | <one sentence>\n\n\
441 For example:\n\
442 \x20 FINDING | line=214 | class=contract-drift | compile=no | the doc says \
443 the cache is unbounded but `insert` evicts at 256 entries\n\
444 \x20 FINDING | line=87 | class=permissive-constraint | compile=no | uses \
445 `<=` so a zero-length span passes the guard that exists to reject it\n\n\
446 Rules:\n\
447 - Cite the NEW-SIDE line number from the left column. Every line is \
448 numbered for you; never compute one from the hunk header.\n\
449 - **Both halves must be visible below.** Report a conflict only when the \
450 promise AND the behaviour that breaks it are both in the lines shown. If \
451 you can see a doc comment but not the code it describes, or a call but \
452 not the signature it calls, you cannot tell whether they disagree — say \
453 nothing. Do not infer what code you have not been shown does.\n\
454 - Quote the specific words that conflict, so a reader can check you \
455 without opening the file.\n\
456 - Do not restate one point as several findings. Each finding must be a \
457 separate defect a separate commit would fix.\n\
458 - `compile=yes` ONLY if you are claiming the code will not build. \
459 Everything else is `compile=no`.\n",
460 );
461 let _ = writeln!(
462 head,
463 " - Reply {NO_FINDINGS} only if you have worked through every class \
464 above and found nothing. A file whose change is routine is a normal \
465 outcome, and reporting nothing is better than reporting a guess."
466 );
467
468 let mut context_block = String::new();
469 if !context.is_empty() {
470 context_block.push_str(
471 "\nContext from the repository's graph. This is not part of the \
472 change; it is what the graph knows about the code under review, and \
473 each item says which layer it came from.\n\n",
474 );
475 for item in &context.items {
476 let _ = writeln!(
477 context_block,
478 "--- {} [{}]\n{}",
479 item.label,
480 item.provenance,
481 item.body.trim_end()
482 );
483 }
484 }
485
486 let annotated = annotate_diff(&file.diff);
487 let tail_header = format!("\nFile under review: {}\n\n", file.path);
488 let fixed =
489 estimate_tokens(&head) + estimate_tokens(&context_block) + estimate_tokens(&tail_header);
490 let room = budget.saturating_sub(fixed);
491 let (body, dropped_tokens) = truncate_to_tokens(&annotated, room);
492
493 let text = format!("{head}{context_block}{tail_header}{body}");
494 Prompt {
495 tokens: estimate_tokens(&text),
496 text,
497 dropped_tokens,
498 }
499}
500
501/// A one-line gloss per defect class, for the prompt.
502///
503/// Written from the class's own meaning rather than from any corpus row, and kept
504/// beside [`CLASSES`] so a new class cannot be added without deciding how to
505/// describe it to a reviewer. `class_gloss_covers_every_class` holds that.
506fn class_gloss(class: DefectClass) -> &'static str {
507 match class {
508 DefectClass::CleanupGap => "a guard stops a cleanup path doing its job",
509 DefectClass::ContractDrift => {
510 "a doc comment, README or ADR states something the code does not do"
511 }
512 DefectClass::ErrorTextDrift => "an error message does not state the rule it enforces",
513 DefectClass::FalseCompileClaim => "the code will not compile (see the compile= rule)",
514 DefectClass::LintConvention => "a lint suppression carries no justification",
515 DefectClass::LossyIdentity => {
516 "a key built from a lossy conversion, so distinct inputs collide"
517 }
518 DefectClass::MissingEvent => "an early return skips a documented side effect",
519 DefectClass::OrderingBug => "an aggregate is computed after the mutation it must precede",
520 DefectClass::PerfContract => "the implementation defeats a field's stated design goal",
521 DefectClass::PermissiveConstraint => "a check permits the state it exists to forbid",
522 DefectClass::ProseClarity => "wording that misleads a reader",
523 DefectClass::SilentTruncation => "a read or copy drops a remainder without erroring",
524 DefectClass::UxDiagnostic => "a message tells the user to do the wrong thing",
525 DefectClass::VacuousTest => "a test passes whether or not the behaviour it names works",
526 }
527}
528
529/// Render a unified diff with **new-side line numbers in a left column**.
530///
531/// A reviewer's finding is scored by its line, within
532/// [`crate::review_score::LINE_WINDOW`]. Asking a model to derive a line number
533/// from `@@ -a,b +c,d @@` spends budget on arithmetic it is bad at and turns a
534/// correct finding into a miss — which would be measured as the reviewer failing
535/// to see the defect rather than failing to count. So the arithmetic is done here,
536/// where it is exact.
537///
538/// Removed lines carry no new-side number and are marked `-`, so the model can
539/// still see what was replaced without being able to cite a line that no longer
540/// exists.
541#[must_use]
542pub fn annotate_diff(diff: &str) -> String {
543 let mut out = String::with_capacity(diff.len() + diff.len() / 8);
544 let mut new_line: Option<u32> = None;
545 for raw in diff.lines() {
546 if raw.starts_with("@@") {
547 new_line = parse_hunk_new_start(raw);
548 out.push_str(raw);
549 out.push('\n');
550 continue;
551 }
552 // Everything before the first hunk (`diff --git`, `---`, `+++`, mode
553 // lines) is passed through unnumbered: it is not file content.
554 let Some(n) = new_line else {
555 out.push_str(raw);
556 out.push('\n');
557 continue;
558 };
559 match raw.as_bytes().first() {
560 Some(b'-') => {
561 let _ = writeln!(out, " - |{}", &raw[1..]);
562 }
563 Some(b'+') => {
564 let _ = writeln!(out, "{n:>6} +|{}", &raw[1..]);
565 new_line = Some(n + 1);
566 }
567 Some(b'\\') => {
568 let _ = writeln!(out, " |{raw}");
569 }
570 // A context line, including the empty string a bare `\n` produces.
571 _ => {
572 let body = raw.strip_prefix(' ').unwrap_or(raw);
573 let _ = writeln!(out, "{n:>6} |{body}");
574 new_line = Some(n + 1);
575 }
576 }
577 }
578 out
579}
580
581/// The new-side start line of a `@@ -a,b +c,d @@` header.
582fn parse_hunk_new_start(header: &str) -> Option<u32> {
583 let plus = header.split('+').nth(1)?;
584 let digits: String = plus.chars().take_while(char::is_ascii_digit).collect();
585 digits.parse().ok()
586}
587
588/// Truncate `text` to `budget` tokens at a line boundary, returning the kept text
589/// and the number of tokens dropped.
590///
591/// The head is kept rather than the tail: a diff's first hunks are the ones a
592/// reviewer can still anchor, and a review of the first half of a file is a
593/// partial review, while a review of the second half with no idea what preceded it
594/// is a confused one. The marker is left in the text so the *model* also knows it
595/// is seeing part of a file.
596fn truncate_to_tokens(text: &str, budget: usize) -> (String, usize) {
597 /// Charged against the budget before cutting, so adding it cannot push the
598 /// result back over the budget it was just cut to.
599 const MARKER: &str =
600 "\n[... truncated to fit the context budget: this is PART of the file ...]\n";
601
602 if estimate_tokens(text) <= budget {
603 return (text.to_owned(), 0);
604 }
605 let room = budget.saturating_sub(estimate_tokens(MARKER)) * 4;
606 let mut kept = 0usize;
607 for line in text.split_inclusive('\n') {
608 if kept + line.len() > room {
609 break;
610 }
611 kept += line.len();
612 }
613 let dropped = estimate_tokens(&text[kept..]);
614 (format!("{}{MARKER}", &text[..kept]), dropped)
615}
616
617/// What [`parse_findings`] made of a model's reply.
618#[derive(Debug, Clone, Default, PartialEq, Eq)]
619pub struct Parsed {
620 /// Findings in the corpus's coordinate system, ready to score.
621 pub findings: Vec<CandidateFinding>,
622 /// Lines that looked like an attempted finding but could not be read as one.
623 ///
624 /// Counted rather than discarded. A model that ignores the output format
625 /// scores exactly like a model that found nothing, and those are opposite
626 /// facts about a reviewer: the first needs a different prompt, the second a
627 /// different model. A run reports this so the two cannot be confused.
628 pub unparsed: Vec<String>,
629 /// Whether the reply declared the file clean in the required form.
630 pub declared_clean: bool,
631 /// The generation stopped inside a reasoning block, so the model never
632 /// reached its answer.
633 ///
634 /// **This is the stage's own silent zero, found by walking into it.** A
635 /// reasoning GGUF opens `<think>` and deliberates before answering; hit the
636 /// token cap first and the reply contains no findings and no `NO FINDINGS` —
637 /// indistinguishable, to anything counting findings, from a reviewer that read
638 /// the file and passed it. Measured on `qwen3.8-27b`, whose careful
639 /// doc-versus-code deliberation was **entirely** inside the block and scored
640 /// as silence.
641 ///
642 /// So it is a reported outcome rather than an absence. A run that cannot tell
643 /// "found nothing" from "never answered" is reporting a recall figure it did
644 /// not measure.
645 pub reasoning_truncated: bool,
646}
647
648/// Read a model's reply into findings.
649///
650/// Lenient about presentation and strict about content: a leading bullet, bold
651/// markers or a code fence are stripped, because a model wrapping the format in
652/// markdown has still followed it — but a finding with no readable positive line
653/// number goes to [`Parsed::unparsed`], since an unanchored finding cannot be
654/// scored, shown to a human, or acted on.
655///
656/// Leniency stops at the description. Decoration is stripped from the line's ends
657/// and from the *structure* of each `key=value` field, never from the free-form
658/// prose a human is going to read — a parser that quietly rewrites the text it
659/// reports is editing the evidence.
660#[must_use]
661pub fn parse_findings(reviewed_sha: &str, path: &str, reply: &str) -> Parsed {
662 let mut out = Parsed {
663 // Checked on the text as handed over, which a caller has already run its
664 // `</think>` strip across: an *opening* tag still present means the
665 // closing one never arrived, so generation stopped mid-deliberation.
666 reasoning_truncated: reply.contains("<think>"),
667 ..Parsed::default()
668 };
669 for raw in reply.lines() {
670 let line = raw.trim().trim_start_matches(['-', '*', '>', '#', ' ']);
671 let line = line.trim_start_matches('`').trim_end();
672 // Whole-line decoration only. The leading trim above has already taken any
673 // opening `**`, so this closes the pair — for `**NO FINDINGS**`, and for a
674 // finding line a model has bolded end to end. Bold *inside* the line is
675 // deliberately left alone here and dealt with per-field in `parse_one`;
676 // see the note there for why the difference matters.
677 let line = line.strip_suffix("**").unwrap_or(line).trim();
678 if line.eq_ignore_ascii_case(NO_FINDINGS) {
679 out.declared_clean = true;
680 continue;
681 }
682 if !line
683 .get(..FINDING_PREFIX.len())
684 .is_some_and(|p| p.eq_ignore_ascii_case(FINDING_PREFIX))
685 {
686 continue;
687 }
688 match parse_one(reviewed_sha, path, line) {
689 Some(finding) => out.findings.push(finding),
690 None => out.unparsed.push(line.to_owned()),
691 }
692 }
693 out
694}
695
696/// Parse one `FINDING | …` line, or `None` if it carries no usable line number.
697fn parse_one(reviewed_sha: &str, path: &str, line: &str) -> Option<CandidateFinding> {
698 let mut number: Option<u32> = None;
699 let mut class = None;
700 let mut claims_compile_failure = false;
701 let mut description = String::new();
702
703 for field in line.split('|').skip(1) {
704 let field = field.trim().trim_end_matches('`').trim();
705 // **Bold is stripped for the structural read only, and only where it
706 // resolves to a field this format defines.**
707 //
708 // The line-wide `replace("**", "")` this replaces was not there to handle
709 // *leading* bold — the trim in `parse_findings` already takes that. It was
710 // there for bold *inside* the line, i.e. a model emitting
711 // `class=**contract-drift**` or `**line**=42`, which has still followed the
712 // format and must still parse. But applying it to the whole line also
713 // rewrote the description, so emphasis a model put in prose vanished from
714 // the text a human is shown and a corpus is scored against.
715 //
716 // Splitting the two reads keeps the field tolerance and drops the
717 // rewriting: `structural` exists only to decide what this token *is*, and
718 // if the answer is "not a known field" the original bytes go through
719 // untouched.
720 let structural = field.replace("**", "");
721 let key_value = structural
722 .split_once('=')
723 .map(|(k, v)| (k.trim().to_ascii_lowercase(), v.trim()));
724
725 match key_value.as_ref().map(|(k, v)| (k.as_str(), *v)) {
726 Some(("line", value)) => number = value.parse().ok().filter(|n| *n > 0),
727 Some(("class", value)) => class = DefectClass::from_token(&value.to_ascii_lowercase()),
728 Some(("compile", value)) => {
729 claims_compile_failure = matches!(
730 value.to_ascii_lowercase().as_str(),
731 "yes" | "true" | "y" | "1"
732 );
733 }
734 // Everything else is description, kept verbatim. A field with no `=`
735 // is the description proper; later ones are its continuation, because
736 // a description may itself contain a pipe. An *unknown* `key=value` is
737 // kept as prose rather than dropped: it is more likely a description
738 // containing an `=` than an invented field, and losing it would leave
739 // a human a bare line number.
740 _ => {
741 if !description.is_empty() {
742 description.push_str(" | ");
743 }
744 description.push_str(field);
745 }
746 }
747 }
748
749 Some(CandidateFinding {
750 reviewed_sha: reviewed_sha.to_owned(),
751 path: path.to_owned(),
752 line: number?,
753 description: if description.trim().is_empty() {
754 "(no description given)".to_owned()
755 } else {
756 description.trim().to_owned()
757 },
758 claims_compile_failure,
759 defect_class: class,
760 })
761}
762
763/// Derive the [`ClaimSite`] for a compile claim, from the reviewed file's bytes.
764///
765/// `parent_source` is the module's parent (`lib.rs`/`mod.rs`) when the caller has
766/// it, which is the only place a file's feature gate is written.
767///
768/// # Conservative on every axis, deliberately
769///
770/// [`crate::compile_claim`] states the asymmetry this follows: a claim wrongly
771/// suppressed is a defect shipped silently — the #291 macOS teardown shape — while
772/// a claim wrongly kept costs a human one look at a CI page. So every derivation
773/// here errs toward *establishing a requirement*, which makes a site harder to
774/// refute, never easier:
775///
776/// * **Platform.** Any `cfg(target_os = "macos"/"windows")` anywhere in the file
777/// marks the whole file as needing that platform, so no job in this
778/// ubuntu-only CI can refute a claim about it. Coarse, and coarse in the safe
779/// direction: a file with one macOS-gated function keeps its compile claims.
780/// * **Features.** A `#[cfg(feature = …)]` on the module's declaration in
781/// `parent_source` marks the site as needing [`Features::All`], so only an
782/// `--all-features` job covers it. Without `parent_source` nothing is
783/// established, which is the one axis where "unknown" reads as unconditional —
784/// stated here rather than left for a reader to infer from the field docs.
785/// * **Targets.** A path under `tests/`, `benches/` or `examples/` is test code;
786/// so is a line *after* a `#[cfg(test)]` attribute in the file. Both need a job
787/// that passed `--all-targets`, which `msrv` does not.
788#[must_use]
789pub fn claim_site(
790 reviewed_sha: &str,
791 path: &str,
792 line: u32,
793 source: &str,
794 parent_source: Option<&str>,
795) -> ClaimSite {
796 ClaimSite {
797 platform: required_platform(source),
798 features: parent_source.and_then(|p| module_feature_gate(path, p)),
799 is_test_code: is_test_code(path, line, source),
800 toolchain: None,
801 ..ClaimSite::unknown(reviewed_sha, path)
802 }
803}
804
805/// The platform a file's `cfg` gates require, if it names one.
806fn required_platform(source: &str) -> Option<TargetOs> {
807 // macOS first: it is the platform this repository actually has uncompiled
808 // code for, and the one #291 shipped a defect behind.
809 for (needle, os) in [
810 ("target_os = \"macos\"", TargetOs::MacOs),
811 ("target_os = \"windows\"", TargetOs::Windows),
812 ] {
813 if source.contains(needle) {
814 return Some(os);
815 }
816 }
817 None
818}
819
820/// Whether `line` is test code: a test-only path, or a line after a
821/// `#[cfg(test)]` attribute.
822fn is_test_code(path: &str, line: u32, source: &str) -> bool {
823 if ["tests/", "benches/", "examples/"]
824 .iter()
825 .any(|d| path.starts_with(d) || path.contains(&format!("/{d}")))
826 {
827 return true;
828 }
829 // The line-relative form, so a `#[cfg(test)] mod tests` at the bottom of a
830 // source file does not mark the library code above it as test code — which
831 // would disable the filter on nearly every file in this repository.
832 source
833 .lines()
834 .position(|l| l.trim_start().starts_with("#[cfg(test)]"))
835 .is_some_and(|idx| line as usize > idx + 1)
836}
837
838/// The name a file is declared under by the `mod` item in its parent.
839///
840/// For `a/b/thing.rs` that is the file stem, `thing`. For `a/b/mod.rs` it is the
841/// *directory* name, `b`: a `mod.rs` is declared by `mod b;` in `a`'s source, and
842/// never by `mod mod;`.
843///
844/// # Why this is spelled out rather than left as `file_stem`
845///
846/// Taking the stem for a `mod.rs` searches the parent for `mod mod;`, which
847/// cannot match, so the lookup returns `None` — and a `None` on the features axis
848/// reads as *unconditional*, i.e. covered by any green job. That is the
849/// permissive direction: a feature-gated module would have its compile claims
850/// suppressed by a job that never compiled it. [`claim_site`]'s contract is that
851/// every derivation errs toward establishing a requirement, so this one case has
852/// to be got right rather than left to fall through.
853fn declaring_name(path: &str) -> Option<&str> {
854 let stem = path.rsplit('/').next()?.strip_suffix(".rs")?;
855 if stem == "mod" {
856 // `a/b/mod.rs` is declared as `b`; a bare `mod.rs` with no directory
857 // above it is declared by nothing, so there is no name to look for.
858 path.rsplit('/').nth(1)
859 } else {
860 Some(stem)
861 }
862}
863
864/// The feature gate on this file's `mod` declaration in its parent, if any.
865///
866/// Returns [`Features::All`] rather than naming the feature: the coverage model
867/// asks which *job* compiled the code, and this repository's jobs are
868/// `--all-features`, the default set, or nothing. Which named feature it is does
869/// not change the answer.
870///
871/// # Known gap: `#[path = "…"]`
872///
873/// A module declared `#[path = "elsewhere.rs"] mod name;` is not found by this
874/// lookup, because the declaring name cannot be recovered from the file path at
875/// all — the mapping lives in the attribute, in a parent this function is not
876/// given a way to search for. The result is `None`, which is the permissive
877/// direction, so this is a real (if narrow) hole rather than a tidy limitation.
878/// It is left open deliberately: closing it means scanning candidate parents for
879/// `#[path]` attributes and resolving them relative to the declaring file, which
880/// is a different shape of change from this one. This repository contains no
881/// `#[path]` attributes, so nothing here relies on it today.
882fn module_feature_gate(path: &str, parent_source: &str) -> Option<Features> {
883 let stem = declaring_name(path)?;
884 let lines: Vec<&str> = parent_source.lines().collect();
885 let decl = lines.iter().position(|l| {
886 let t = l.trim_start().trim_start_matches("pub ").trim_start();
887 t.starts_with(&format!("mod {stem};")) || t.starts_with(&format!("mod {stem} "))
888 })?;
889 // Walk back over the declaration's own attributes only, stopping at the first
890 // line that is not one — attributes bind to what follows them, so a `cfg` two
891 // items up governs a different item.
892 for above in lines[..decl].iter().rev() {
893 let t = above.trim();
894 if t.is_empty() || t.starts_with("//") {
895 continue;
896 }
897 if !t.starts_with("#[") {
898 break;
899 }
900 if t.contains("cfg(feature") || t.contains("cfg(all(feature") {
901 return Some(Features::All);
902 }
903 }
904 None
905}
906
907#[cfg(test)]
908mod tests {
909 use super::{
910 FileUnderReview, GraphContext, NO_FINDINGS, Prompt, SINGLE_CALL_BUDGET_TOKENS,
911 annotate_diff, build_prompt, claim_site, class_gloss, estimate_tokens, parse_findings,
912 };
913 use crate::compile_claim::{CheckRun, Conclusion, Features, TargetOs, Targets, suppression};
914 use crate::review_corpus::{CLASSES, DefectClass};
915
916 const SHA: &str = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa";
917
918 fn file(diff: &str) -> FileUnderReview {
919 FileUnderReview {
920 reviewed_sha: SHA.to_owned(),
921 path: "crates/rto-graph/src/lib.rs".to_owned(),
922 diff: diff.to_owned(),
923 }
924 }
925
926 /// **The line column is the reviewer's whole anchoring story**, so it must be
927 /// exact: a finding is credited only within `LINE_WINDOW` of a corpus row, and
928 /// a model asked to do hunk arithmetic instead would miss by more than that
929 /// and be scored as blind rather than as bad at counting.
930 #[test]
931 fn the_annotated_diff_numbers_the_new_side_exactly() {
932 let diff = "@@ -10,3 +20,4 @@ fn thing()\n unchanged\n-gone\n+added\n+also\n context\n";
933 let out = annotate_diff(diff);
934 let numbered: Vec<(u32, String)> = out
935 .lines()
936 .filter_map(|l| {
937 let (num, body) = l.split_once('|')?;
938 let n: u32 = num
939 .trim()
940 .trim_end_matches(['+', '-'])
941 .trim()
942 .parse()
943 .ok()?;
944 Some((n, body.to_owned()))
945 })
946 .collect();
947 assert_eq!(
948 numbered,
949 vec![
950 (20, "unchanged".to_owned()),
951 (21, "added".to_owned()),
952 (22, "also".to_owned()),
953 (23, "context".to_owned()),
954 ],
955 "new-side numbering starts at the hunk's + start and skips removals"
956 );
957 // A removed line is shown but carries no citable number.
958 assert!(out.contains(" - |gone"), "{out}");
959 }
960
961 /// A diff with several hunks restarts numbering at each header rather than
962 /// counting straight through — the failure that would put every finding after
963 /// the first hunk out of window.
964 #[test]
965 fn numbering_restarts_at_each_hunk() {
966 let diff = "@@ -1,2 +1,2 @@\n a\n b\n@@ -50,2 +90,2 @@\n c\n d\n";
967 let out = annotate_diff(diff);
968 assert!(out.contains(" 1 |a"), "{out}");
969 assert!(out.contains(" 90 |c"), "{out}");
970 assert!(out.contains(" 91 |d"), "{out}");
971 }
972
973 /// Every class the corpus can score is described to the model. A class with no
974 /// gloss is a class the reviewer was never told to look for, which would be
975 /// measured as a recall failure rather than as the omission it is.
976 #[test]
977 fn class_gloss_covers_every_class() {
978 for class in CLASSES {
979 let gloss = class_gloss(class);
980 assert!(!gloss.is_empty(), "{class} has no gloss");
981 }
982 let prompt = build_prompt(&file("@@ -1 +1 @@\n+x\n"), &GraphContext::none(), 30_000);
983 for class in CLASSES {
984 assert!(
985 prompt.text.contains(class.as_str()),
986 "{class} is not named in the prompt"
987 );
988 }
989 }
990
991 /// A helper for the budget tests: an item whose body is `chars` bytes long,
992 /// so its cost is predictable in `len / 4` terms.
993 fn item(label: &str, bytes: usize) -> super::ContextItem {
994 super::ContextItem {
995 label: label.to_owned(),
996 provenance: "authored".to_owned(),
997 body: "x".repeat(bytes),
998 }
999 }
1000
1001 /// **The cap is on the block, and it is relative to the diff.** A two-line
1002 /// change must not arrive under four thousand tokens of ADR: the whole risk
1003 /// this arm carries is that context drowns the change, and the guard against
1004 /// it is arithmetic rather than judgement.
1005 #[test]
1006 fn context_is_capped_relative_to_the_diff_it_accompanies() {
1007 // A 100-token diff admits at most 200 tokens of context, so the second
1008 // 150-token item cannot join the first.
1009 let fitted = GraphContext::fit(vec![item("a", 600), item("b", 600)], 100);
1010 assert_eq!(
1011 fitted.items.len(),
1012 1,
1013 "two 150-token items fit a 200-token cap"
1014 );
1015 assert_eq!(fitted.dropped_items, 1);
1016 assert!(
1017 fitted.tokens() <= 200,
1018 "cap breached: {} tokens",
1019 fitted.tokens()
1020 );
1021 }
1022
1023 /// The absolute ceiling binds even when the diff is enormous, so a huge file
1024 /// cannot pull in a proportionally huge context.
1025 #[test]
1026 fn the_absolute_cap_binds_on_a_large_diff() {
1027 // 20k diff tokens would allow 40k by the relative rule alone.
1028 let fitted = GraphContext::fit(
1029 (0..20).map(|i| item(&format!("adr-{i}"), 2_000)).collect(),
1030 20_000,
1031 );
1032 assert!(
1033 fitted.tokens() <= super::CONTEXT_CAP_TOKENS,
1034 "the absolute cap did not bind: {} tokens",
1035 fitted.tokens()
1036 );
1037 assert!(
1038 fitted.dropped_items > 0,
1039 "nothing was dropped, so nothing was capped"
1040 );
1041 }
1042
1043 /// **Whole items only.** A half-quoted ADR reads as a complete statement of a
1044 /// decision and is not one, so a model can be handed a promise whose exception
1045 /// was cut off. This asserts the bodies come through byte-identical.
1046 #[test]
1047 fn fitting_never_truncates_an_item_it_keeps() {
1048 let original = item("adr", 400);
1049 let fitted = GraphContext::fit(vec![original.clone(), item("big", 100_000)], 1_000);
1050 assert_eq!(fitted.items, vec![original], "a kept item was rewritten");
1051 assert_eq!(fitted.dropped_items, 1);
1052 }
1053
1054 /// A large item that does not fit must not also discard the small ones behind
1055 /// it — the cap is a budget, not a stopping point.
1056 #[test]
1057 fn an_oversized_item_does_not_evict_the_smaller_ones_after_it() {
1058 let fitted = GraphContext::fit(vec![item("huge", 100_000), item("small", 40)], 1_000);
1059 assert_eq!(fitted.dropped_items, 1);
1060 assert_eq!(
1061 fitted.items.len(),
1062 1,
1063 "the small item behind an oversized one was lost"
1064 );
1065 assert_eq!(fitted.items[0].label, "small");
1066 }
1067
1068 /// An empty diff admits no context at all, which is the conservative
1069 /// direction: `min(cap, 2 * 0) == 0`.
1070 #[test]
1071 fn an_empty_diff_admits_no_context() {
1072 let fitted = GraphContext::fit(vec![item("adr", 40)], 0);
1073 assert!(fitted.is_empty());
1074 assert_eq!(fitted.dropped_items, 1, "the drop must still be counted");
1075 }
1076
1077 /// [`ContextItem::tokens`] must charge for the heading, not just the body —
1078 /// otherwise a context of many tiny items is billed as nearly free while the
1079 /// prompt pays for every `--- label [provenance]` line.
1080 #[test]
1081 fn an_item_is_charged_for_its_heading_as_well_as_its_body() {
1082 let bare = super::ContextItem {
1083 label: String::new(),
1084 provenance: String::new(),
1085 body: "x".repeat(40),
1086 };
1087 let labelled = super::ContextItem {
1088 label: "ADR-0019 §3 governs `resolve`".to_owned(),
1089 provenance: "authored".to_owned(),
1090 body: "x".repeat(40),
1091 };
1092 assert!(
1093 labelled.tokens() > bare.tokens(),
1094 "the heading was not charged: {} vs {}",
1095 labelled.tokens(),
1096 bare.tokens()
1097 );
1098 }
1099
1100 /// A section runs to the next `## `, and a deeper heading inside it is body.
1101 #[test]
1102 fn a_section_body_stops_at_the_next_sibling_heading() {
1103 let md = "# ADR-0005\n\n## Context\nwhy\n\n## Decision\nthe rule\n\n### Detail\nmore\n\n## Consequences\nafter\n";
1104 assert_eq!(
1105 super::section_body(md, "Decision").as_deref(),
1106 Some("the rule\n\n### Detail\nmore"),
1107 "a `###` subheading must not end the section"
1108 );
1109 assert_eq!(super::section_body(md, "Context").as_deref(), Some("why"));
1110 assert_eq!(super::section_body(md, "Absent"), None);
1111 }
1112
1113 /// **The title is matched verbatim, because a slug rule would be a second
1114 /// copy of one this crate cannot see.** Punctuation that a slug would collapse
1115 /// must still resolve here.
1116 #[test]
1117 fn a_heading_with_punctuation_resolves_without_a_slug_rule() {
1118 let md = "## Options considered + consequences\nbody\n\n## Next\nx\n";
1119 assert_eq!(
1120 super::section_body(md, "Options considered + consequences").as_deref(),
1121 Some("body")
1122 );
1123 }
1124
1125 /// A doc comment already visible in the hunks is not worth re-sending: the
1126 /// context block's whole value is the half the diff does *not* show.
1127 #[test]
1128 fn a_doc_already_in_the_diff_is_not_re_quoted() {
1129 let doc = "Returns the cache entry for `key`, evicting the least recently used entry when the cache is full.";
1130 let shown = format!(" 12 |/// {doc}\n 13 |pub fn get(&self) {{}}\n");
1131 assert!(
1132 super::doc_already_shown(doc, &shown),
1133 "the doc is in the diff and was not recognised"
1134 );
1135 assert!(
1136 !super::doc_already_shown(doc, " 12 |pub fn unrelated() {}\n"),
1137 "a doc absent from the diff was treated as shown"
1138 );
1139 }
1140
1141 /// The diff carries a line-number column and `+`/` ` markers the graph's copy
1142 /// of the same doc does not, so the comparison must ignore whitespace — this
1143 /// is the case that a naive `contains` gets wrong.
1144 #[test]
1145 fn the_visibility_test_ignores_the_line_number_column() {
1146 let doc = "The slot lock is held only long enough to hand out an `Arc`, never across initialisation.";
1147 // Wrapped across lines and numbered, exactly as `annotate_diff` renders it.
1148 let shown = " 16 +|/// The slot lock is held only long enough to hand\n 17 +|/// out an `Arc`, never across initialisation.\n";
1149 assert!(
1150 super::doc_already_shown(doc, shown),
1151 "wrapping and numbering defeated the visibility test"
1152 );
1153 }
1154
1155 /// A doc too short to state a contract is treated as already shown, so the
1156 /// context block is not padded with one-word comments that cannot drift.
1157 #[test]
1158 fn a_doc_too_short_to_state_a_contract_is_never_carried() {
1159 assert!(super::doc_already_shown("The key.", "unrelated diff text"));
1160 }
1161
1162 /// **PR 1's arm is diff-only, and the prompt must say nothing else.** An empty
1163 /// [`GraphContext`] renders no context heading at all, so the baseline is not
1164 /// quietly a reviewer told it has context and given none — which is a
1165 /// different prompt, and would make the two arms differ by more than the
1166 /// context.
1167 #[test]
1168 fn an_empty_context_renders_no_context_section() {
1169 let bare = build_prompt(&file("@@ -1 +1 @@\n+x\n"), &GraphContext::none(), 30_000);
1170 assert!(!bare.text.contains("Context from"), "{}", bare.text);
1171 assert!(
1172 !bare.text.contains("[authored]") && !bare.text.contains("[derived]"),
1173 "no provenance labels without context: {}",
1174 bare.text
1175 );
1176
1177 let with = build_prompt(
1178 &file("@@ -1 +1 @@\n+x\n"),
1179 &GraphContext {
1180 items: vec![super::ContextItem {
1181 label: "ADR-0019 §3".to_owned(),
1182 provenance: "authored".to_owned(),
1183 body: "the user layer alone never suffices".to_owned(),
1184 }],
1185 dropped_items: 0,
1186 },
1187 30_000,
1188 );
1189 assert!(with.text.contains("Context from"), "{}", with.text);
1190 assert!(with.text.contains("ADR-0019 §3"), "{}", with.text);
1191 assert!(
1192 with.text.contains("[authored]"),
1193 "provenance travels with the item: {}",
1194 with.text
1195 );
1196 }
1197
1198 /// The prompt fits its budget, and says so when it could not fit the diff.
1199 #[test]
1200 fn a_prompt_respects_its_budget_and_reports_what_it_dropped() {
1201 let big = format!(
1202 "@@ -1,1 +1,{0} @@\n{}",
1203 "+a line of code here\n".repeat(20_000)
1204 );
1205 let f = file(&big);
1206 let Prompt {
1207 text,
1208 tokens,
1209 dropped_tokens,
1210 } = build_prompt(&f, &GraphContext::none(), 8_000);
1211 assert!(tokens <= 8_000, "over budget: {tokens}");
1212 assert!(dropped_tokens > 0, "a 20k-line diff cannot have fit");
1213 assert!(
1214 text.contains("truncated to fit"),
1215 "the model is told it is seeing part of a file"
1216 );
1217
1218 // And the common case: nothing dropped, nothing claimed.
1219 let small = build_prompt(&file("@@ -1 +1 @@\n+x\n"), &GraphContext::none(), 30_000);
1220 assert_eq!(small.dropped_tokens, 0);
1221 assert!(!small.text.contains("truncated"));
1222 }
1223
1224 /// **The headroom the graph arm depends on, asserted rather than assumed.**
1225 ///
1226 /// The largest reviewable source file-diff in the corpus is 14,034 raw tokens
1227 /// (`rto-graph/src/models.rs`), which [`annotate_diff`] takes to **17,202** —
1228 /// the numbering column costs a measured **1.21×** across all 190 file-diffs,
1229 /// because it adds a fixed 9 characters per *line* rather than a fraction of
1230 /// the bytes. Reconstructed here at that size and at this repository's line
1231 /// length, so a prompt change that eats the headroom fails here rather than
1232 /// showing up later as a worse score nobody can attribute.
1233 #[test]
1234 fn the_prompt_scaffolding_leaves_the_measured_headroom_intact() {
1235 // ~44 characters per line, this repository's rough average, so the
1236 // annotation overhead lands where it was measured rather than at the
1237 // 4× a two-character line would produce.
1238 let line = format!("+{}\n", "a".repeat(43));
1239 let worst = line.repeat(14_034 * 4 / 44);
1240 let f = file(&format!("@@ -1,1 +1,1 @@\n{worst}"));
1241 let p = build_prompt(&f, &GraphContext::none(), SINGLE_CALL_BUDGET_TOKENS);
1242 assert_eq!(
1243 p.dropped_tokens, 0,
1244 "the corpus's largest source file must not need truncating"
1245 );
1246 let headroom = SINGLE_CALL_BUDGET_TOKENS - p.tokens;
1247 assert!(
1248 headroom > 10_000,
1249 "only {headroom} tokens left for graph context on the worst source \
1250 file; the arm needs room to be testable at all"
1251 );
1252
1253 // And the median file, which is what the headroom claim is really about:
1254 // 1,758 annotated tokens leaves nearly the whole budget free.
1255 let median = file(&format!("@@ -1,1 +1,1 @@\n{}", line.repeat(1_476 * 4 / 44)));
1256 let p = build_prompt(&median, &GraphContext::none(), SINGLE_CALL_BUDGET_TOKENS);
1257 assert!(
1258 SINGLE_CALL_BUDGET_TOKENS - p.tokens > 25_000,
1259 "the median file should leave ~28k free, left {}",
1260 SINGLE_CALL_BUDGET_TOKENS - p.tokens
1261 );
1262 }
1263
1264 #[test]
1265 fn a_well_formed_finding_parses() {
1266 let reply = "FINDING | line=42 | class=contract-drift | compile=no | the doc says X";
1267 let parsed = parse_findings(SHA, "src/a.rs", reply);
1268 assert_eq!(parsed.findings.len(), 1);
1269 let f = &parsed.findings[0];
1270 assert_eq!(f.line, 42);
1271 assert_eq!(f.defect_class, Some(DefectClass::ContractDrift));
1272 assert!(!f.claims_compile_failure);
1273 assert_eq!(f.description, "the doc says X");
1274 assert!(parsed.unparsed.is_empty());
1275 }
1276
1277 /// A model that wraps the format in markdown has still followed it. Being
1278 /// strict here would measure formatting compliance and call it recall.
1279 #[test]
1280 fn presentation_is_tolerated_but_content_is_not() {
1281 let reply = "\
1282- **FINDING** | line=7 | class=vacuous-test | compile=no | asserts nothing
1283 > FINDING | line=9 | class=ordering-bug | compile=YES | will not build
1284FINDING | class=prose-clarity | compile=no | no line at all
1285FINDING | line=0 | class=prose-clarity | compile=no | line zero is not a line
1286here is some prose the model added";
1287 let parsed = parse_findings(SHA, "src/a.rs", reply);
1288 assert_eq!(parsed.findings.len(), 2, "{:?}", parsed.findings);
1289 assert_eq!(parsed.findings[0].line, 7);
1290 assert!(
1291 parsed.findings[1].claims_compile_failure,
1292 "compile= is case-insensitive"
1293 );
1294 // Both unanchored forms are counted, not silently dropped.
1295 assert_eq!(parsed.unparsed.len(), 2, "{:?}", parsed.unparsed);
1296 // Prose that is not an attempted finding is not counted as a failure.
1297 assert!(!parsed.unparsed.iter().any(|u| u.contains("here is some")));
1298 }
1299
1300 /// **Bold inside a field must still parse; bold inside a description must
1301 /// still be there afterwards.** These pull in opposite directions and the two
1302 /// halves of this test are the reason the strip is per-field rather than
1303 /// per-line.
1304 ///
1305 /// Stripping only the line's ends would be the tidy-looking fix and would
1306 /// regress the first half: `class=**contract-drift**` stops resolving and the
1307 /// finding lands unclassified. Stripping the whole line — what this replaces —
1308 /// buys the first half by silently editing the description a human reads.
1309 #[test]
1310 fn bold_is_stripped_from_fields_and_left_in_descriptions() {
1311 let reply = "\
1312FINDING | **line**=42 | class=**contract-drift** | **compile**=YES | the **remote** path is not gated
1313**NO FINDINGS**";
1314 let parsed = parse_findings(SHA, "src/a.rs", reply);
1315 assert_eq!(parsed.findings.len(), 1, "{:?}", parsed.unparsed);
1316
1317 let f = &parsed.findings[0];
1318 assert_eq!(f.line, 42, "a bolded key still names the line field");
1319 assert_eq!(
1320 f.defect_class,
1321 DefectClass::from_token("contract-drift"),
1322 "a bolded value still resolves to its class"
1323 );
1324 assert!(
1325 f.claims_compile_failure,
1326 "a bolded key still names `compile`"
1327 );
1328 assert_eq!(
1329 f.description, "the **remote** path is not gated",
1330 "the model's emphasis is the model's; the parser does not edit prose \
1331 it is about to report"
1332 );
1333
1334 // Whole-line bold is decoration and is closed at the ends, so the clean
1335 // declaration is still recognised rather than read as unparsed prose.
1336 assert!(parsed.declared_clean);
1337 }
1338
1339 /// **"Found nothing" and "ignored the format" are opposite facts.** A run that
1340 /// cannot tell them apart cannot tell a bad model from a bad prompt.
1341 #[test]
1342 fn a_clean_declaration_is_distinguishable_from_silence() {
1343 let clean = parse_findings(SHA, "src/a.rs", NO_FINDINGS);
1344 assert!(clean.declared_clean);
1345 assert!(clean.findings.is_empty() && clean.unparsed.is_empty());
1346 assert!(!clean.reasoning_truncated);
1347
1348 let waffle = parse_findings(SHA, "src/a.rs", "I reviewed the file and it looks fine.");
1349 assert!(
1350 !waffle.declared_clean,
1351 "prose is not the declaration the format requires"
1352 );
1353 }
1354
1355 /// **A reviewer cut off mid-deliberation must not read as a clean file.**
1356 /// Measured on `qwen3.8-27b`: a reasoning GGUF opens `<think>`, and if the
1357 /// token cap lands before `</think>` the reply carries no finding and no
1358 /// declaration — which counts identically to a careful pass unless something
1359 /// says otherwise. That is the same silent zero the corpus's `reviewed_sha`
1360 /// rule exists to prevent, arriving from a third direction.
1361 #[test]
1362 fn a_reply_cut_off_inside_a_reasoning_block_is_not_a_clean_file() {
1363 let cut = parse_findings(
1364 SHA,
1365 "src/a.rs",
1366 "<think>\nLet me check the doc against the code. Line 12 says the cache is\n\
1367 unbounded, and the insert path",
1368 );
1369 assert!(
1370 cut.reasoning_truncated,
1371 "an unterminated block is truncation"
1372 );
1373 assert!(!cut.declared_clean, "and it is emphatically not clean");
1374 assert!(cut.findings.is_empty());
1375
1376 // A block the model actually closed is a normal answer; the caller strips
1377 // it before parsing, so nothing here should fire.
1378 let finished = parse_findings(SHA, "src/a.rs", NO_FINDINGS);
1379 assert!(!finished.reasoning_truncated);
1380 }
1381
1382 /// An unknown class is dropped to `None` rather than failing the finding:
1383 /// recall is about the defect, and `review_score` already reports
1384 /// misclassification separately.
1385 #[test]
1386 fn an_unknown_class_does_not_cost_the_finding() {
1387 let parsed = parse_findings(
1388 SHA,
1389 "src/a.rs",
1390 "FINDING | line=3 | class=off-by-one | compile=no | oops",
1391 );
1392 assert_eq!(parsed.findings.len(), 1);
1393 assert_eq!(parsed.findings[0].defect_class, None);
1394 }
1395
1396 /// A description containing a pipe survives, because the alternative is a
1397 /// human handed a bare line number.
1398 #[test]
1399 fn a_description_may_contain_the_separator() {
1400 let parsed = parse_findings(
1401 SHA,
1402 "src/a.rs",
1403 "FINDING | line=3 | class=prose-clarity | compile=no | says a | b but means a",
1404 );
1405 assert_eq!(parsed.findings[0].description, "says a | b but means a");
1406 }
1407
1408 /// **The #291 shape, derived rather than assumed.** A file with macOS-gated
1409 /// code yields a site no job in this ubuntu-only CI covers, so a compile claim
1410 /// about it survives a wholly green build.
1411 #[test]
1412 fn a_macos_gated_file_yields_an_unrefutable_site() {
1413 let source = "#[cfg(target_os = \"macos\")]\nfn teardown() {}\n";
1414 let site = claim_site(SHA, "crates/rto-llama/src/backend.rs", 2, source, None);
1415 assert_eq!(site.platform, Some(TargetOs::MacOs));
1416 assert!(!suppression(&site, &ci()).is_refuted());
1417
1418 // The same file without the gate is ordinary library code, and a green
1419 // all-features job does refute it — so the filter is not simply off.
1420 let plain = claim_site(
1421 SHA,
1422 "crates/rto-llama/src/backend.rs",
1423 2,
1424 "fn t() {}\n",
1425 None,
1426 );
1427 assert_eq!(plain.platform, None);
1428 assert!(suppression(&plain, &ci()).is_refuted());
1429 }
1430
1431 /// A `#[cfg(test)] mod tests` at the foot of a source file must not mark the
1432 /// library code above it as test code — that would establish a requirement
1433 /// `msrv` cannot meet on nearly every file here and disable the filter
1434 /// wholesale.
1435 #[test]
1436 fn test_code_is_decided_per_line_not_per_file() {
1437 let source = "fn real() {}\n#[cfg(test)]\nmod tests {\n fn t() {}\n}\n";
1438 let above = claim_site(SHA, "crates/rto-graph/src/lib.rs", 1, source, None);
1439 assert!(!above.is_test_code);
1440 let below = claim_site(SHA, "crates/rto-graph/src/lib.rs", 4, source, None);
1441 assert!(below.is_test_code);
1442
1443 // An integration-test path is test code at any line.
1444 let integration = claim_site(SHA, "crates/rto-graph/tests/review_corpus.rs", 1, "", None);
1445 assert!(integration.is_test_code);
1446 }
1447
1448 /// **The `boxlite.rs` row of the corpus, derived.** Its module is
1449 /// `#[cfg(feature = "exec-boxlite")]` in the parent, so only an
1450 /// `--all-features` job compiles it — which `compile_claim`'s own licence test
1451 /// asserts by hand and this derives from bytes.
1452 #[test]
1453 fn a_feature_gated_module_needs_an_all_features_job() {
1454 let parent = "pub mod subprocess;\n#[cfg(feature = \"exec-boxlite\")]\npub mod boxlite;\n";
1455 let site = claim_site(
1456 SHA,
1457 "crates/rto-exec/src/boxlite.rs",
1458 10,
1459 "fn run() {}\n",
1460 Some(parent),
1461 );
1462 assert_eq!(site.features, Some(Features::All));
1463
1464 // Its ungated sibling establishes nothing, so it is not accidentally
1465 // narrowed to the all-features jobs.
1466 let sibling = claim_site(
1467 SHA,
1468 "crates/rto-exec/src/subprocess.rs",
1469 10,
1470 "fn run() {}\n",
1471 Some(parent),
1472 );
1473 assert_eq!(sibling.features, None);
1474 }
1475
1476 /// **A `mod.rs` is declared by its directory name, not by `mod mod;`.**
1477 ///
1478 /// Latent in this repository rather than live: it has exactly two `mod.rs`
1479 /// files, both under `tests/`, and no `src/**/mod.rs` at all — so no compile
1480 /// claim here has ever taken this path. It is fixed anyway because the failure
1481 /// is permissive. Taking the stem searches for `mod mod;`, never matches, and
1482 /// yields `features: None`, which reads as *unconditional* — a feature-gated
1483 /// module would have its compile claims suppressed by a job that never
1484 /// compiled it. That is the #291 shape: a build reporting coverage it did not
1485 /// have. The reviewer is also scored against a 190-path corpus and pointed at
1486 /// other repositories, where `src/**/mod.rs` is ordinary.
1487 #[test]
1488 fn a_mod_rs_is_gated_by_the_declaration_of_its_directory() {
1489 let parent = "#[cfg(feature = \"serve\")]\npub mod thing;\n";
1490 let site = claim_site(
1491 SHA,
1492 "crates/x/src/thing/mod.rs",
1493 10,
1494 "fn run() {}\n",
1495 Some(parent),
1496 );
1497 assert_eq!(
1498 site.features,
1499 Some(Features::All),
1500 "`thing/mod.rs` is declared by `mod thing;`, so the gate on it applies"
1501 );
1502
1503 // The stem-based lookup this replaces would find nothing and establish
1504 // nothing, which is the permissive answer rather than a missing one.
1505 let ungated = claim_site(
1506 SHA,
1507 "crates/x/src/other/mod.rs",
1508 10,
1509 "fn run() {}\n",
1510 Some(parent),
1511 );
1512 assert_eq!(
1513 ungated.features, None,
1514 "the gate governs `thing`, not every `mod.rs`"
1515 );
1516
1517 // A `mod.rs` with no directory above it is declared by nothing.
1518 assert_eq!(
1519 claim_site(SHA, "mod.rs", 1, "", Some(parent)).features,
1520 None
1521 );
1522 }
1523
1524 /// An attribute belonging to a different item must not be read as this
1525 /// module's gate — the walk stops at the first non-attribute line.
1526 #[test]
1527 fn a_gate_on_another_item_is_not_borrowed() {
1528 let parent = "#[cfg(feature = \"serve\")]\npub mod served;\n\npub mod plain;\n";
1529 let site = claim_site(SHA, "crates/x/src/plain.rs", 1, "", Some(parent));
1530 assert_eq!(
1531 site.features, None,
1532 "the gate governs `served`, not `plain`"
1533 );
1534 }
1535
1536 /// `len / 4` is the basis every budget number in this stage is quoted on.
1537 #[test]
1538 fn token_estimation_is_the_documented_basis() {
1539 assert_eq!(estimate_tokens(&"a".repeat(400)), 100);
1540 assert_eq!(estimate_tokens(""), 0);
1541 }
1542
1543 /// This repository's compiling jobs, as `compile_claim`'s own tests model
1544 /// them: all ubuntu, only `checks`/`default-features` with `--all-targets`.
1545 fn ci() -> Vec<CheckRun> {
1546 vec![
1547 CheckRun {
1548 job: "msrv".to_owned(),
1549 sha: SHA.to_owned(),
1550 conclusion: Conclusion::Success,
1551 toolchain: "1.94".to_owned(),
1552 platform: TargetOs::Linux,
1553 features: Features::All,
1554 targets: Targets::LibsAndBins,
1555 },
1556 CheckRun {
1557 job: "checks".to_owned(),
1558 sha: SHA.to_owned(),
1559 conclusion: Conclusion::Success,
1560 toolchain: "stable".to_owned(),
1561 platform: TargetOs::Linux,
1562 features: Features::All,
1563 targets: Targets::AllTargets,
1564 },
1565 ]
1566 }
1567}