fix(review): give the verifier the PR description it judges description gaps against - #724
Merged
Merged
Conversation
…on gaps against The verifier received findings, the diff, the project stack and the previous findings — never the PR title and description. A candidate weighing the stated intent against the code has half its claim in that description, so it was unverifiable by construction, and the verifier's own prompt instructs it to reject a claim whose material is not provided. Across one dogfood round five PRs planted the same description-versus-code mismatch on one build. Two were rejected on exactly that ground — 'depends on a PR description not in the provided material' — and three were kept. The code fact was equally checkable in all five, so the split is sampling rather than judgement, and it lands in both directions: correct findings deleted on one side, a 3/5 score that measures the coin flip rather than the capability on the other. The description now reaches the call, escaped and fenced as untrusted data the same way the review prompt receives it, with the section instructing that such a candidate is checkable here and must not be rejected for missing material. Supplying the material makes the rejection ground inapplicable instead of asking the model to remember not to apply it. The existing entry points keep their shape and send no context, which leaves the section out of the prompt rather than sending an empty heading; both production call sites already held the value. Scope: this closes the structural half of #711. The CRITICAL false positive in instance 1 is variance on material that WAS present, and the verifier shares the concise model with the summary and reply lanes, so a per-call sampling knob is not available without splitting that lane.
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Contributor
🤖 ThrillhouseBot PR SummaryWhat this PR doesThis PR fixes the finding verifier never seeing the PR title and description, so findings that weigh the author's stated intent against the code are no longer rejected for missing material. The PR context is escaped and fenced as untrusted data in the verifier prompt when a caller has it, empty callers send an empty string to suppress the section, and the production pipeline passes the batch's own prContext. All call sites, mocks, and prompt-rendering tests are updated to the new five-argument verify contract. Description vs. ImplementationNo mismatch found between the PR description and the change. Control-Flow Diagram🔀 Show diagramflowchart TD
A["ReviewPipeline / ReviewOrchestrator"] --> B["FindingVerificationService.verify(...)"]
B --> C{"caller has prContext?"}
C -- "yes" --> D["FindingVerifier.verify(findings, prContext, diff, stack, prevFindings)"]
C -- "no (null)" --> E["prContext = empty string"]
E --> D
D --> F["FindingVerifierPrompts renders PR Title & Description section"]
F --> G["PR title/description fenced as untrusted data"]
G --> H["LLM verifier can check description vs code"]
F --> I["No prContext: section omitted from prompt"]
I --> J["Findings judged against diff only"]
Changes Overview
Changed Files
Risk Assessment
No new issues found in this PR, but the review cannot be approved until required CI is confirmed green.
|
| Check | Type | Status | Detail |
|---|---|---|---|
| dependency-review | check-run | ⏳ Pending | - |
| test | check-run | ⏳ Pending | - |
| trivy | check-run | ⏳ Pending | - |
| format | check-run | ⏳ Pending | - |
| frontend | check-run | ⏳ Pending | - |
Automated review by ThrillhouseBot. Reply with /review to re-run.
thrillhousebot
Bot
added
bug
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This was referenced Aug 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
Description
The finding verifier receives
findings,diff,projectStackandpreviousFindings. It has neverreceived the PR title and description.
That makes one whole class of candidate unverifiable by construction: a finding that weighs the
author's stated intent against the code has half its claim in a description the verifier cannot see.
And the verifier's own prompt tells it to reject a claim whose material is not provided — so whether
such a finding survives depends on whether the model happens to notice the absence.
It noticed roughly half the time. One dogfood round, one build, five PRs each planting the same
description-versus-code mismatch:
Both rejections cite the same ground — "depends on a PR description not in the provided material" —
and the code fact was equally checkable in all five. The three that survived rested on the
description exactly as much as the two that did not. That is sampling, not judgement, and it lands in
both directions: correct findings deleted on one side, and a 3/5 dimension score that measures the
coin flip rather than the capability on the other.
The fix supplies the material rather than asking the model to remember an exception. The PR
context now reaches the verifier, escaped and fenced as untrusted data exactly as the review prompt
receives it, with the section stating that such a candidate is checkable there and must not be
rejected for missing material. The rejection ground becomes inapplicable instead of remaining
applicable-but-discouraged.
The existing entry points keep their shape and send no context, which leaves the section out of the
prompt entirely rather than sending an empty heading. Both production call sites already held the
value —
PromptInputs.prContext()— so nothing new is fetched or spent beyond the prompt text.Scope — what this does not fix
This closes the structural half of #711 (instance 2). Instance 1 — a CRITICAL SQL-injection false
positive correctly rejected in one round and published in the next, on unchanged code — is variance
on material that was present, so supplying more material cannot address it. The verifier also
shares the
concisemodel with the summary and reply lanes, so a per-call temperature or seed is notavailable without splitting that lane. I'd keep #711 open for that half rather than close it here.
Related Issues
Part of #711. Related to #475, which is the general form of "the verifier never sees the context that
produced the finding"; this is the one slice of it with measured evidence of non-determinism.
How Has This Been Tested?
Red proof, with the prompt section removed:
Three seams are pinned separately, because a pass-through can be wired to the wrong value and still
compile: the prompt renders the context, the service forwards it to the model call (and forwards
""when a caller has none), and the pipeline passes the batch's own
prContextrather than the diff../mvnw -B clean compile spotbugs:check spotless:check→BugInstance size is 0./mvnw -B clean test→Tests run: 3267, Failures: 0, Errors: 0, Skipped: 0Checklist