feat(review): carry a context-grounded finding's own evidence to the verifier - #871
Conversation
…verifier The review pass is handed context the verification pass is not: the patch coverage measurement for the head commit (#115) and the maintainers' path-scoped rules (#33). A finding raised from one of those reaches the verifier as a claim about material nobody showed it, which is the ground the verifier's own prompt tells it to reject on, so every context dimension added to the review pass makes the verifier more likely to demote findings the reviewer was right to raise. The finding now carries its grounding. The review prompt requires a finding raised from a context section to quote the measurement or the rule it rests on, and before the verification call each finding is matched against the sections the review actually supplied: a finding anchored to a line the coverage section lists carries that measurement, and a finding on a file a scope governs carries that scope's rules verbatim. The verifier is told to treat that material as established, so it stops demoting such a finding for resting on an artifact outside its own material. The other direction is attached too. A finding that credits a coverage report for a line the section does not list, or in a review that read no report at all, carries that fact instead, and the verifier is told the attributed measurement is not established and earns the finding nothing. A quoted measurement nobody took must not buy a finding a verdict it has not earned. No such note is needed for a scoped rule: the scope's real text travels beside the finding, so a rule the maintainers never wrote is visible as absent from it. The evidence rides on the candidate JSON beside the resolved cited location (#650) rather than in a verifier input of its own, so the next context dimension reaches the verifier without a new prompt slot. Both notes are charged to one character budget held by the review, extracted from the cited-location round for that reason, so what is attached can never rival the diff the verifier is reading. What is read is the rendered section the reviewer was shown, caps and all, so a file the section's own render dropped is one no finding can claim a measurement for. No model call is added, and when nothing matches, nothing is attached.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
🤖 ThrillhouseBot PR SummaryWhat this PR doesCandidates are now handed the review-context material they rest on: the bot matches each finding against the rendered patch-coverage section and the maintainers' path-scoped rules and attaches a context_evidence field to it, supporting or contradicting, before the verification call. The two evidence resolvers share one character budget with the #650 cited-location resolution, and the generator and verifier prompts are taught to quote and trust (or distrust) that grounding. Description vs. ImplementationNo mismatch found between the PR description and the change. Control-Flow Diagram🔀 Show diagramflowchart TD
O["ReviewOrchestrator.review"] --> R["ReviewEvidence.forReview"]
R --> B["EvidenceBudget shared"]
R --> CLR["CitedLocationResolver.Round"]
R --> CER["ContextEvidenceResolver.Round"]
CLR --> FP["FindingPipeline.run"]
CER --> FP
FP --> FE["forFindings(batch findings)"]
FE --> FVS["FindingVerificationService.verify"]
FVS --> RC["renderCandidates: context_evidence on candidate"]
RC --> V["Verification model call"]
Changes Overview
Changed Files
Risk Assessment
Key Findings
Things to double-check1 lower-confidence finding
|
| Check | Type | Status | Detail |
|---|---|---|---|
| test | check-run | ⏳ Pending | - |
Automated review by ThrillhouseBot. Reply with /review to re-run.
![thrillhousebot[bot]](https://avatars.githubusercontent.com/in/3996985?s=60&v=4){kind=small}
There was a problem hiding this comment.
ThrillhouseBot noted 1 lower-confidence item(s) under Things to double-check in the PR summary (not posted as inline threads):
- LOW: governs() trusts an abbreviated file list and never refuses a suffix-ambiguous citation (
src/main/java/dev/thiagogonzaga/thrillhousebot/review/ContextEvidenceResolver.java:274)
The scope gate is: 'return scope.files().stream().anyMatch(f -> f.equals(cited) || CitedLocationResolver.sharesPathSuffix(f, cited));' while the rule note it feeds asserts '' + cited + 'is one of the files in this pull request they govern'. Two fidelity gaps are visible against other material in this PR. (1) The scoped-rules guidance in ReviewPromptAssembler states 'that list may be abbreviated; the glob is what decides' — so a scope that genuinely governs the cited file can attach nothing if the cited file was dropped from the abbreviation, silently sending the finding to the verifier without its grounding, the exact failure this PR exists to fix. If AppliedScope.files() is the abbreviated list, matching the scope glob (as the prompt text says it decides) would be the correct test rather than membership in files(). (2) Unlike coveragePathFor, which explicitly refuses to guess when two listed files match one citation ('Two listed files match the citation, so which one it means is not settled; saying nothing beats naming another file's measurement as this finding's'), governs() uses anyMatch and would attach a scope's rules for, say, 'Renderer.java' when the scope lists src/a/Renderer.java but the finding actually concerns src/b/Renderer.java — asserting a governance fact that may be false, the direction the PR's own thesis ('a quoted measurement nobody took is not evidence') guards against for measurements. Verify what InstructionsResolver puts into AppliedScope.files() and the exact contract of sharesPathSuffix; an ambiguity-refusing variant mirroring coveragePathFor is a safe local fix for (2) regardless.
thrillhousebot
Bot
added
enhancement
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…ranches The new resolver's defensive paths went untested: a review that loaded no coverage section and no scopes, a finding keyed at an anchor another finding already claimed, a finding citing no file, two scopes governing one file, a citation that names a governed file without its leading directory, a scope whose rules are empty and a finding carrying no prose at all, and a range bound the section's roll-up leaves unparseable. Each is a shape the review pass can produce, and none of them may throw or attach material the sections do not hold.
🤖 ThrillhouseBot — changes since the last review
|
There was a problem hiding this comment.
ThrillhouseBot noted 2 lower-confidence item(s) under Things to double-check in the PR summary (not posted as inline threads):
- MEDIUM: Size-cap-cut section lines claim to fail parsing but can parse into a forged measurement (
src/main/java/dev/thiagogonzaga/thrillhousebot/review/ContextEvidenceResolver.java:324)
The parseUncovered javadoc asserts: "the heading, the prose, the roll-up count and a line the section's own size cap cut in half all fail to parse and are skipped, which is the honest outcome for material the reviewer could not read either." The code directly below contradicts that for a class of cuts: a "- " line containing ": " parses wherever the cut lands after the colon. Counterexample within the domain the comment itself establishes (the class doc says the section has a total size cap, and the comment anticipates mid-line cuts): the cap cutting "- src/main/java/app/App.java: 40-47, 61" after the colon leaves "- src/App.java: 40-47, 6"; lines 322-326 put the value in the map, and in rangesContain the bare token "6" parses through parseLine as line 6, so rangeContains returns true. A finding on line 6 then receives the supporting note "The patch-coverage section this review supplied lists line 6 ... among the added lines ... never executed" — a measurement created by truncation rather than by the report, the exact fabricated-evidence direction this class's own design ("A quoted measurement nobody took is not evidence") forbids. A cut landing immediately after "path: " stores an empty ranges string, which also renders into the contradiction message as "lists these added lines ofpathas never executed: .". A maintainer trusting the comment would believe cut lines are safely inert; they are not. The honest fix is on the renderer side — PatchCoverageResolver.render should end the section on a line boundary or emit a truncation marker the parser can decline — since a cut "61"→"6" is indistinguishable from a genuine line token at parse time. Confidence medium because PatchCoverageResolver.render is not in the diff; verify where its total-size cap truncates. - LOW: Attribution literal "never executed" also matches diff-grounded unreachable-code findings (
src/main/java/dev/thiagogonzaga/thrillhousebot/review/ContextEvidenceResolver.java:93)
COVERAGE_ATTRIBUTIONS (lines 87-95) includes the bare phrase "never executed" (line 93), described as "the wording the review prompt itself asks for". But the same phrase is the natural wording of a finding grounded purely in the diff with no coverage claim at all — synthesized input not in the diff: "This branch is never executed: the guard above returns early on every path", a static-unreachable claim, in a review that supplied a coverage section listing the cited file but not that line. For such a finding attributesToCoverage returns true, so coverageNote returns the contradiction "The patch-coverage section this review supplied lists these added lines ofpathas never executed: ... The cited line N is not among them.", and the verifier prompt instructs that "the attribution earns the finding nothing" and to "reject the finding when the attribution was all it had" — a verifier-side push against a sound finding that claimed no measurement. The class comment justifies the narrow list by saying a broader scan would "start contradicting findings that never claimed a measurement at all", yet this literal can hit exactly that case. The consequence is mitigated (the verifier is also instructed to judge the diff remainder on its merits), and the false-negative direction — a coverage wording the scan misses — is deliberately accepted, so risk stays low and the wording is held at low confidence; verify the behavior against a dead-code finding in a review with a supplied coverage section. A stricter gate would require a phrase naming the report ("coverage report" / "patch coverage") rather than the outcome phrase alone.
A caught exception the handler never reads is written as an unnamed pattern elsewhere in the codebase; the range parser's fallback was the one place that still bound a variable for it.
🤖 ThrillhouseBot — changes since the last review
|
There was a problem hiding this comment.
ThrillhouseBot noted 1 lower-confidence item(s) under Things to double-check in the PR summary (not posted as inline threads):
- LOW: disabled() round's 'attaches nothing' comment contradicts its contradiction notes (
src/main/java/dev/thiagogonzaga/thrillhousebot/review/ContextEvidenceResolver.java:121)
The javadoc on ContextEvidenceResolver.disabled() states: 'A round that attaches nothing, for a caller that loaded no review context.' The body it documents — 'return new Round("", PathScopedInstructions.NONE, new EvidenceBudget());' — contradicts that in a demonstrable case: a Round built from a blank section has coverageSupplied == false, and coverageNote then returns contradiction(...) for any finding whose title/description matches COVERAGE_ATTRIBUTIONS ('coverage report', 'never executed', …), i.e. it attaches a verifier-facing note. The PR's own test proves this exact state attaches something: attachesNothingWhenTheReviewLoadedNoSectionAndNoScopes builds the equivalent round via 'ContextEvidenceResolver.forReview(null, null, new EvidenceBudget())' and asserts the evidence 'contains("supplied no patch-coverage section")'. The same false statement appears a second time on ReviewEvidence.NONE ('Resolves nothing, for a caller with no repository access and no loaded context.'). Contrast with the established sibling CitedLocationResolver.disabled(), whose comment 'A round that resolves nothing' is true because locate() short-circuits on prClient == null — the new symmetric-looking API diverges. A maintainer who picks disabled()/ReviewEvidence.NONE for a no-context caller trusting the comments believes the verifier's material is unchanged, when in fact coverage-attributing findings gain a contradiction note. Fix the comments (the behavior itself is the honest design direction); if genuine inertness is intended for disabled(), gate the contradiction accordingly — but that would change ReviewEvidence.NONE callers, so the comment correction is the safe fix.
|
/review |
🤖 ThrillhouseBot — changes since the last review
|
There was a problem hiding this comment.
ThrillhouseBot noted 2 lower-confidence item(s) under Things to double-check in the PR summary (not posted as inline threads):
- MEDIUM: parseUncovered javadoc claims cut section lines fail to parse; parseable truncations are accepted instead (
src/main/java/dev/thiagogonzaga/thrillhousebot/review/ContextEvidenceResolver.java:311)
The javadoc on parseUncovered states: "the heading, the prose, the roll-up count and a line the section's own size cap cut in half all fail to parse and are skipped, which is the honest outcome for material the reviewer could not read either." The code it documents disagrees: byPath.put(entry.substring(0, colon), entry.substring(colon + 2).strip()); accepts any line that retains the "- " prefix, the ": " separator and a numeric range prefix — it has no way to detect that a line was truncated, so a parseable cut is trusted as if it were complete. The class's own javadoc elsewhere establishes that cut lines occur at all: "The coverage section is capped at a file count, a range count per file and a total size, so a file the render dropped is a file the reviewer never saw." Synthesized input (not in the diff): a total-size truncation of the rendered per-file line "- src/main/java/app/Renderer.java: 40-47, 61" that lands after ", 6" leaves "- src/main/java/app/Renderer.java: 40-47, 6", which parses to ranges "40-47, 6"; rangesContain then returns true for line 6 and coverageNote attaches "lists line 6 of…Renderer.javaamong the added lines … never executed" — fabricated supporting evidence produced by an artifact of truncation, exactly the false-evidence class this PR exists to prevent (a cut after "40-4" instead yields the never-matching range 40..4 and silently loses the supporting note the reviewer could partially see). Whether PatchCoverageResolver's total-size cap truncates mid-line, and at which character boundaries, is governed by that renderer's cap mechanics, which are not in the provided material — verify where its cap cuts before treating a mid-line cut as reachable; the contradiction between the comment's universal "all fail to parse" claim and the accepting parse code is demonstrable from the diff alone. Suggested fix direction: have the parser reject a trailing truncation marker or verify the section's declared integrity before trusting parsed ranges, or correct the comment to state what actually happens to parseable cuts. - LOW: "never executed" attribution phrase also matches static dead-code prose, attaching a contradiction note to findings that never claimed a measurement (
src/main/java/dev/thiagogonzaga/thrillhousebot/review/ContextEvidenceResolver.java:91)
COVERAGE_ATTRIBUTIONS includes the literal "never executed" (beside "coverage report", "patch coverage", …). The class javadoc defends the scan's width only against wider phrases: "a broader scan ('untested', 'no tests') would start contradicting findings that never claimed a measurement at all" — but the same false-attribute shape exists for this entry. Synthesized input (not in the diff): a finding whose description reads "The catch block on this line is unreachable — the guard two lines up returns first, so it is never executed." That is static dead-code reasoning, not an attribution to a coverage report, yet text::contains matches the literal, so with a supplied section listing the file without that line, coverageNote falls through to contradiction(...) — the candidate is told the cited line is "not among them" the section's list, and contradiction() logs at INFO: "attributes a claim to patch coverage that this review's section does not carry" for a finding that made no such claim. The verifier prompt then instructs: "Judge what is left on the diff alone, and reject the finding when the attribution was all it had", and "the attribution earns the finding nothing". The project's own test (contradictsACoverageClaimWhenThisReviewSuppliedNoSection) uses "This line is never executed, so the branch is untested." as its attribution fixture, so treating this phrasing as attribution-adjacent is documented intent, and the verifier is still told to judge the remaining substance on the diff alone — which is why impact is bounded and the risk is low. Verification request: run the eval corpus over static dead-branch phrasings that naturally contain "never executed" ("this branch is never executed when input is empty", "unreachable, so never executed") and confirm the contradiction note plus the mislabeled INFO log do not suppress valid findings; if they do, the literal needs a co-occurring coverage-report cue (e.g. match "never executed" only alongside "coverage"/"report" in the same text).
…idence Four defects in the evidence this branch attaches, all in the direction the feature exists to close: material a finding must not be able to claim. The coverage section's size cap cut mid-line, and half of "61" is "6". An entry the cap cut reads to everything downstream as a measurement the report never made, and nothing there can tell a cut token from a real one, so a finding anchored at that number would have carried supporting evidence for a line nobody measured. The cap now falls on a line boundary and the section never holds a partial entry. The scan that decides whether a finding credits the coverage report matched "never executed", which is also how a finding grounded in the diff alone describes unreachable code. Such a finding was told its measurement is not in the section, and the verifier is instructed to give an unestablished attribution nothing — against a claim it never made. The phrases now all name the report. A citation that lost a leading directory was matched against the scoped file lists with anyMatch, so a finding about `src/b/Renderer.java` citing `Renderer.java` could be handed the rules of a scope that governs `src/a/Renderer.java` alone, asserting a governance fact that is not true. The lookup now refuses an ambiguous citation and names the file it matched, the same discipline the coverage lookup already applied. A round built by disabled() said "this review supplied no patch-coverage section" to a coverage-attributing finding. A caller that loaded no context knows nothing about what the review measured, so that is a statement about the caller; the round is silent instead, which is also the contract its cited-location sibling has. Also drops the javadoc left over the pipeline test's field when the constant it documented was removed.
|
@thrillhousebot resolved src/main/java/dev/thiagogonzaga/thrillhousebot/review/ContextEvidenceResolver.java:324 — Size-cap-cut section lines claim to fail parsing but can parse into a forged measurement Correct, and it reproduced.
Fixed in 6846c20, on the renderer side as suggested, because nothing downstream can tell a cut token from a real one: the cap now falls on the last line boundary before it, so the section never holds a partial entry. One half of the report is refuted: a cut landing immediately after The |
|
The next review will close every previous finding this comment names by its |
|
@thrillhousebot resolved src/main/java/dev/thiagogonzaga/thrillhousebot/review/ContextEvidenceResolver.java:91 — "never executed" attribution phrase also matches static dead-code prose, attaching a contradiction note to findings that never claimed a measurement Correct. "The catch block is unreachable: the guard two lines up returns first, so it is never executed" is diff-grounded dead-code reasoning that names no report, and with a coverage section present listing that file but not that line it was told "the cited line is not among them" — a contradiction of a claim the finding never made, plus an INFO line saying it attributed something to patch coverage. Fixed in 6846c20, along the line suggested: every phrase in |
|
@thrillhousebot resolved src/main/java/dev/thiagogonzaga/thrillhousebot/review/ContextEvidenceResolver.java:274 — governs() trusts an abbreviated file list and never refuses a suffix-ambiguous citation Half right, and the half that is right is fixed. (2) is a real defect and matches the asymmetry you name with (1) is refuted. |
|
The next review will close every previous finding this comment names by its |
1 similar comment
|
The next review will close every previous finding this comment names by its |
|
@thrillhousebot resolved src/main/java/dev/thiagogonzaga/thrillhousebot/review/ContextEvidenceResolver.java:121 — disabled() round's 'attaches nothing' comment contradicts its contradiction notes Correct: a round built from a blank section had Fixed in 6846c20 by making the behaviour match the comment rather than the other way round, because the note was also wrong on its own terms: a caller that loaded no context has not established that no coverage report was read — it never looked — so the sentence is a statement about the caller, not about the review. |
|
The next review will close every previous finding this comment names by its |
The entry describes what the attached evidence reads; the cap that decides where that material ends is part of it.
|
/review |
There was a problem hiding this comment.
No new issues in this revision, but 1 previous finding(s) remain unresolved — fix them, or reply on their review thread with why they are deferred. A finding listed only under "Things to double-check" has no thread: clear it by commenting @thrillhousebot resolved path/to/File.java:42 — <the finding's title> on this PR.
The note for a line a listed file's ranges do not name asserted that the line is not among them. A file's rendered ranges are capped like the file list is, and the render discloses that with its own roll-up, so a line past the named ones may well have been measured and the assertion could cost a real coverage-grounded finding its verdict — the failure this evidence exists to prevent. When the ranges carry that roll-up the note says the section names only part of the file's uncovered lines and leaves the question open. The file-level hedge had a hole of its own: the size cap takes whole lines and the roll-up is the last of them, so a cut section can name fewer files than the report measured with no count left to say so. The parse now carries the cut, and the note says the section may not name every file.
🤖 ThrillhouseBot — changes since the last review
|
There was a problem hiding this comment.
ThrillhouseBot found no issues in this PR, but some checks are still pending or failed:
- Check test is pending
ThrillhouseBot closed 1 previous finding(s) this round:
src/main/java/dev/thiagogonzaga/thrillhousebot/review/ContextEvidenceResolver.java:244— Contradiction note denies a coverage measurement the section's own range roll-up may hide
There was a problem hiding this comment.
Required CI is now green for 5e6339a, so the approval the earlier review held back is posted. The code was not re-reviewed: that review found no issues, and only the CI gate held its approval.
|
/review |
🤖 ThrillhouseBot — changes since the last review
|
There was a problem hiding this comment.
ThrillhouseBot noted 1 lower-confidence item(s) under Things to double-check in the PR summary (not posted as inline threads):
- LOW: Single-stem coverage scan misses fabricated attributions worded without the word 'coverage' (
src/main/java/dev/thiagogonzaga/thrillhousebot/review/ContextEvidenceResolver.java:421)
Dimension 5/heuristic: namesCoverage gates the contradicting direction on one substring, 'return text.contains(COVERAGE_MENTION);' where COVERAGE_MENTION = "coverage". Any fabricated coverage attribution worded around the stem escapes it and reaches the verifier with neither the measurement nor a contradiction attached — and the new absence rule in FindingVerifierPrompts says absence 'implies nothing either way', so the verifier no longer has the old 'claim about unshown material' rejection ground to fall back on. Synthesized probe (input not in the diff): a finding whose description reads 'The CI instrumentation run for this commit confirms this branch has no test reaching it' — it credits a measurement nobody took, contains no 'coverage' stem, and travels uncontradicted. The class javadoc discloses this trade-off deliberately (scanning outcome words like 'never executed' would contradict diff-grounded dead-code findings), so this is a design-cost observation, not a confirmed defect: verify whether the scan should add a second stem of report-artifact nouns that cannot appear in a diff-grounded finding ('instrumentation report', 'the CI measurements'). A test in-diff exercises paraphrase handling (contradictsAParaphrasedCoverageClaimToo uses 'Per the coverage analysis', which still contains the stem), so a no-stem phrasing is beyond its reach.
The constant came out of the cited-location round with its comment unchanged, and the comment still described the per-round cap the extraction replaced. One review holds one budget and every resolver charges it; reading the cap as per-round again would double what a review can attach.
🤖 ThrillhouseBot — changes since the last review
|
|
/review |
There was a problem hiding this comment.
No new issues in this revision, but 2 previous finding(s) remain unresolved — fix them, or reply on their review thread with why they are deferred. A finding listed only under "Things to double-check" has no thread: clear it by commenting @thrillhousebot resolved path/to/File.java:42 — <the finding's title> on this PR.
…'s files Resolving a finding's scoped rules walked every file any scope governs, once per finding, and then re-walked each scope's file list to find the scopes holding the match. Both scans are sized by the pull request, so the cost grew with findings times changed files for a lookup the review context can answer directly. The round now builds two indexes once: the scopes governing each file, and the governed files by their last path segment. A suffix match asks one path to end in the other, so it always shares that segment, and a citation that lost a leading directory is a lookup in the name index rather than a walk. Behaviour is unchanged and pinned by the existing citation tests, with the file name that belongs to another directory added to them.
|
@thrillhousebot resolved src/main/java/dev/thiagogonzaga/thrillhousebot/review/ContextEvidenceResolver.java:421 — Single-stem coverage scan misses fabricated attributions worded without the word 'coverage' Accurate about the mechanics, and it is the trade the constant records rather than a defect to close. Keeping it, with reasons. The miss is not a regression. A phrasing the scan does not catch attaches nothing, which is exactly the material the verifier had before this branch: the finding arrives as it always did and is judged on the diff. Nothing this branch adds makes such a claim stronger. The verifier is not disarmed by the absence rule. "Absence of the field … implies nothing either way about the finding" says the absence establishes nothing — it does not establish the attribution either. A finding that credits "the CI instrumentation run for this commit" is still a claim about an artifact the verifier was not shown, and the prompt's standing grounds for that are untouched: unverifiable claims about artifacts not in the diff are capped at medium risk with low confidence, and a claim resting on an execution signal with no such signal in the provided material is rejected outright. A second stem list is the direction this branch already backed out of once. "Never executed" was in the list and had to go, because it is also how a diff-grounded finding describes unreachable code; "instrumentation report" and "the CI measurements" are the same shape one step further out, and the list can never be complete — a fabricator's wording is unbounded, so each addition buys a narrow catch at the price of contradicting findings that claimed nothing. The scan is one containment test precisely so its behaviour is something a test can pin, and its errors fall in the direction that costs a finding nothing. The generator side already narrows the gap. The patch-coverage block requires a finding raised from it to quote the list's own entry for that file, path and range as written, so a fabrication has no measurement to quote and reads as an unsupported claim about an unshown artifact even before this field is consulted. If dogfooding turns up no-stem fabrications in practice, the fix I would reach for is not a wider scan but the eval corpus deciding which phrasings are real, which is the probe named in the PR body as needing provider credentials this branch cannot run. |
|
The next review will close every previous finding this comment names by its |
🤖 ThrillhouseBot — changes since the last review
|
There was a problem hiding this comment.
ThrillhouseBot found no issues in this PR, but some checks are still pending or failed:
- Check test is pending
Additionally, No new issues in this revision, but 1 previous finding(s) remain unresolved — fix them, or reply on their review thread with why they are deferred. A finding listed only under "Things to double-check" has no thread: clear it by commenting @thrillhousebot resolved path/to/File.java:42 — <the finding's title> on this PR.
ThrillhouseBot closed 1 previous finding(s) this round:
src/main/java/dev/thiagogonzaga/thrillhousebot/review/ContextEvidenceResolver.java:209— Per-finding linear scans make context evidence resolution quadratic in PR size
|
|
/review |
1 participant
What type of PR is this?
Description
FindingVerifieris the second-opinion pass that can downgrade or drop a finding before the maintainer sees it, and its inputs are fixed: candidates, PR context, diff, project stack, previous findings. The review pass gets more than that.patchCoverage(#115) andpathInstructions(#33) reach the reviewer and never reach the verifier, so a finding raised from one of them arrives as a claim about material nobody showed it — the exact shape the verifier's generic rejection ground deletes.configKeyContext(#108/#109) is the counter-example the ticket points at: it reaches the verifier transitively, inside the finding itself, and the verifier has a rule for judging it.This generalizes that pattern, composed with #650 rather than duplicating it.
Which dimensions carry evidence. Patch coverage and path-scoped instructions. Linked-issue context is deliberately out (see the scope section below).
How the material is quoted.
ContextEvidenceResolvermatches each candidate against the sections the review actually supplied and attaches acontext_evidencefield to the candidate JSON, beside #650'scited_location:The supporting direction is attached whenever it exists, with no test on the finding's wording: the measurement belongs to the line, not to how the finding phrased itself, and it is what stops the verifier assuming some unshown test covers that line.
What is read. The rendered sections, not the resolvers that produced them. What the verifier must be told is what the reviewer was shown, caps and disclosures and all.
ContextEvidenceResolverTest.readsBackTheSectionTheRendererItselfEmitsparsesPatchCoverageResolver.render's own output so the two sides cannot drift. That cap also changed here: it fell at a character count and could cut a file's entry mid-range, and half of an uncovered line's61is6, a line number too — so a finding anchored there would have carried a measurement nobody took. It now falls on a line boundary, and the section this reads never holds a partial entry.How it is bounded. One budget over all attached evidence, as the ticket asks.
EvidenceBudgetis extracted out ofCitedLocationResolver.Round(sameMAX_NOTE_CHARS1,200 /MAX_TOTAL_CHARS6,000, same monitor and the same #866-review reserve-then-charge semantics) andReviewEvidence.forReviewhands the one instance to both rounds. A quoted scope's rules get their own 500-character cap so one verbose scope cannot crowd out every other note. Anything that no longer fits is simply not attached — the pre-#475 behaviour — and anything the per-note cap cuts ends in a marker, since a note carries material the prompt presents as the real thing. The extraction also fixes a latent detail: the cap now cuts on a surrogate boundary (ConfigKeyContextResolver.truncate) instead of mid-pair.What the verifier is told when the evidence is missing or unreadable. The field is absent, and the prompt says absence means only that nothing was matched and implies nothing either way — the same contract #650 gave
cited_location. Nothing is inferred from silence.How a fabricated quote is handled. This is the direction that matters, and it is not symmetric with the supporting one. A finding that names coverage material is checked against the section: when the section lists that file but not that line, when it lists the file not at all, or when this review read no report at all, the field carries that fact and the verifier is told the attributed fact is not established, the attribution earns the finding nothing, and the finding is rejected when the attribution was all it had. A quoted measurement nobody took is not evidence. The contradiction is gated on one stem — does the finding's own prose say
coverage— because a note saying "this review measured nothing" on a finding that never mentioned it would invite exactly the inverse inference #115 forbids; the stem stops short of outcome wording such as "never executed", which is also how a finding grounded in the diff alone describes unreachable code. The scan only ever adds a contradiction, so a phrasing it misses attaches nothing and the finding is judged as it is today. No such guard is needed for a scoped rule: the scope's real text is attached beside the finding, so a rule the maintainers never wrote is visible as absent from it.What the contradiction does not claim. The section is capped at a file count, a per-file range count and a total size, and it discloses each of those in its own words. A file it does not list may be one the cap dropped, so the note carries the render's roll-up count ("the section also says 2 further changed file(s) have uncovered added lines without naming them") or, when the cut took the roll-up line itself, the cut. A line beyond a file's named ranges may be one the range cap dropped, so when the ranges carry "and N more range(s)" the note says the section names only part of that file's uncovered lines rather than asserting the line was not measured. Absence in a capped section is not a refutation, and the note never reads as one. Two matching files settle nothing, for coverage and for scopes alike: an ambiguous citation attaches nothing rather than naming another file's material as this finding's.
Verifier slots are unchanged. No new
@V. The evidence rides on the candidate JSON, which is where #668 and the ticket both point, and is why the next context dimension needs no slot of its own. The generator prompt gets the general rule (SELF-CARRYING EVIDENCE) plus one sentence per dimension, and the verifier prompt gets one generalcontext_evidencerule that ends by saying it is the pattern every future context section follows — the "documented next to the verifier prompt" acceptance criterion.Two corrections to the ticket, verified against the code. The verifier has five template slots, not four:
prContextwas added by #711's fix and is inFindingVerifierPrompts.USERtoday. AndconfigKeyContextdoes have a rendered section (ReviewPromptAssembler.configKeyContextSection), which the ticket's table describes correctly; it is dimension 10's quoting requirement, not the section, that carries it to the verifier. Neither changes the design.Out of scope, drawn deliberately
linkedIssuesContext. It is review-level prose, not per-finding material: nothing deterministic maps a finding to the requirement in an issue body the way a line maps to a coverage range or a path maps to a glob. Attaching the whole issue text to every finding would flood the shared budget with material most findings do not rest on, and picking the "relevant" sentence is a model judgement, not a lookup. That belongs with feat(review): targeted per-finding verification context — the verifier reads only the material each finding needs [opt-in] #668's per-finding context scoping, which is the stage that decides what each finding needs; the generator-side half (a bug-fix efficacy finding must quote the requirement it rests on) is already covered by the generalSELF-CARRYING EVIDENCErule added here.repoInstructionsblock, which is not in the ticket's table. It is review-wide and unbounded, so it has the same shape as the linked-issue case and is left to the same place.Related Issues
Fixes #475
How Has This Been Tested?
Red first. The behaviour was neutered in place (context notes suppressed, the candidate field passed
null, the four prompt additions removed) and the new tests run against it:Green after the change, full suite:
Gates, from the worktree root:
Patch coverage of the
src/maindiff againstorigin/main, every added line and branch:GAPS: none.Also covered: the budget is shared across both resolvers (
ReviewEvidenceTest.spendsOneBudgetAcrossBothResolvers— cited-location notes that fill it leave nothing for context evidence), a scope that governs another file attaches nothing, a citation matching two listed files resolves to neither, the rendered section is parsed back fromPatchCoverageResolver.render's own output, and the candidate carries neither field when nothing resolved.The eval corpus (
-Peval) needs provider credentials and was not run here; the two prompt-content pin suites it guards alongside are green.Checklist
Screenshots / Logs
A finding whose attribution the section does not carry is logged at INFO, in the voice of #650's citation corrections:
Additional Notes
No configuration, no database change, no new dependency, no added model call. Behaviour is unchanged for a review with no coverage report and no path-scoped rules, which is the common case: nothing is attached and the verifier's material is exactly what it is today.
CHANGELOG.mdgets an### Addedentry under[Unreleased], beside #650's.