fix(review): remember a rejected finding for the head it was rejected on - #874
Conversation
The second-pass audit reached opposite verdicts on one claim and one unchanged commit inside a single dogfood round. It rejected a SQL-injection candidate on the ground that a catalog allow-list gates the route value before it reaches the concatenated query, and forty minutes later, on the same commit, published that claim as CRITICAL on a public route. A second audit of the same claim on the same code is not a second opinion. It is the same question put again to a sampled model, and the answer that reaches the pull request is whichever round happened to post. The inputs cannot be made identical either: the previous-findings section grows with every posted round, the pull request context is bounded against a budget that moves with the diff, and the conversation the review reads changes as people comment. Verification also fails open, so a round may reach no verdict at all. An empty response body, a timeout, a refusal or the review's spend ceiling keeps the candidate exactly as the reviewer raised it, and a claim the audit had already rejected then posts with nothing in front of it. Rejections are now held per pull request and head commit, and a later round on that head drops the claim before the verification call rather than putting it to the model again. Only rejections are held, never confirmations: re-auditing a confirmed finding costs a call, while pinning one would repeat a false positive for the life of the head, so the store can only ever publish fewer findings than before. A push replaces the entry, because a rejection is an answer about code and the code changed. The store is capped in findings per pull request and in pull requests, and lives in memory per replica like the CI hold registry and the superseded-findings carryover. A remembered rejection is recognized through FollowUpAnalyzer's isSameFinding, the rule the follow-up passes already use for a re-raise, so a claim reworded between rounds is still the same claim.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
🤖 ThrillhouseBot PR SummaryWhat this PR doesThe change adds an in-memory store keyed by pull request and head commit that remembers which candidate findings the second-pass verification audit rejected. Later review rounds on the same head drop those claims before the verification call, so a rejected finding is neither re-litigated nor re-published on a round where verification fails open, and a push to a new head clears the remembered rejections. Description vs. ImplementationNo mismatch found between the PR description and the change. Control-Flow Diagram🔀 Show diagramflowchart TD
A["validated reviewer findings"] --> B{"remembered rejection on this head?"}
B -->|"match"| C["drop finding and log via LogSafe"]
B -->|"no match"| D["kept candidates"]
C --> D
D --> E["FindingVerificationService.verify"]
E --> F["remember candidates minus published"]
F --> G["rejections stored keyed by pr and head"]
Changes Overview
Changed Files
Risk Assessment
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):
- MEDIUM: Javadoc says a fail-open round remembers nothing; the self-retraction screen's removals are remembered (
src/main/java/dev/thiagogonzaga/thrillhousebot/review/VerifierRejectionMemory.java:140)
The remember() javadoc states: "A round whose verification failed open removed nothing and so remembers nothing, leaving the earlier rounds' answers standing." That is contradicted by the very same javadoc's first paragraph, which says the recorded difference is "exactly the audit's own removals — a 'rejected' verdict, and the deterministic self-retraction screen in front of the call". The pipeline callsrejectionMemory.remember(session, candidates.findings(), aiResponse.findings())after the verify call, comparing the pre-call candidates against whatever verify returned. Per the PR description, FindingVerificationService.audit's fail-open paths (empty body, IOException/RuntimeException, spend-ceiling refusal) allreturn screened— the output of the self-retraction screen that runs in front of the model call. So on a round where the screen retracted a candidate AND the model call then failed open, the published list isscreened(not the full candidate list), the difference taken byremoved(candidates, published)is non-empty, and the run DOES record those removals into the store — the opposite of what the sentence asserts. A maintainer reasoning from this javadoc that fail-open rounds never mutate the store (for example when reasoning about cap/eviction effects or writing a control test) would get the behavior wrong in that combination. The screen's removals are only recorded on a successful round when the model's returned findings also drop them. Confidence is low because the audit body itself is not in the provided material — thereturn screenedclaim rests on the PR description; verify FindingVerificationService.audit's fail-open returns before rewording.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
… diff The published side of the remembered-rejection difference is read by the same anchor as the candidate side, and the branch that skips a finding citing no file there had no test, so patch coverage reported a partial line.
There was a problem hiding this comment.
ThrillhouseBot found no issues in this PR, but some checks are still pending or failed:
- Check format is pending
- 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
Bot
added
bug
|
/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.
…n round records The remember() javadoc said a round whose verification failed open remembers nothing, which its own first paragraph contradicts. The self-retraction screen runs in front of the model call and the fail-open paths return its output, so such a round does record what the screen retracted; only the verdict half contributes nothing.
|
@thrillhousebot resolved src/main/java/dev/thiagogonzaga/thrillhousebot/review/VerifierRejectionMemory.java:140 — Javadoc says a fail-open round remembers nothing; the self-retraction screen's removals are remembered Correct, and fixed in 6bf9856. The sentence contradicted the paragraph above it. The javadoc now separates the two halves: the screen sits in front of the call and runs on every round, so its removals are recorded even when the call fails, which is a deterministic drop the next round would make again anyway; the verdict half contributes nothing on such a round, leaving a candidate the model never ruled on for a later round to rule on. Behaviour is unchanged — only the description was wrong. |
|
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
- Check format is pending
ThrillhouseBot closed 1 previous finding(s) this round:
src/main/java/dev/thiagogonzaga/thrillhousebot/review/VerifierRejectionMemory.java:140— Javadoc says a fail-open round remembers nothing; the self-retraction screen's removals are remembered
|
There was a problem hiding this comment.
Required CI is now green for 6bf9856, 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 |
1 participant
What type of PR is this?
Description
The second-pass audit reached opposite verdicts on one claim and one unchanged commit inside a
single dogfood round. On
ThrillhouseBot-test#70at2659f683it rejected a SQL-injectioncandidate with correct reasoning — the route value is gated by a catalog allow-list before it
reaches the concatenated query — and forty minutes later, on the same commit, published that same
claim as 🔴 CRITICAL on a public route.
What #650 and #871 already fixed, and what is left
Both landed before this change, and both remove reasons the verifier was wrong. Neither touches
the fact that it answers the same question twice.
CitedLocationResolver) resolves a finding's citedpath:lineagainst the real file atthe head commit and attaches the result to the candidate. It closes the rejection ground "the
evidence sits a few lines outside the nearest hunk, so nobody showed it to me".
ContextEvidenceResolver) carries a context-grounded finding's ownevidence — the patch-coverage measurement, the path-scoped rules — to the verifier. That is the
structural half of the issue's instance 2: a description-versus-code claim was unverifiable by
construction because the description was simply absent from the verifier's slots, and feat(review): carry a context-grounded finding's own evidence to the verifier #871 gives
the verifier the PR title and description (
FindingVerificationService.verify'sprContextslot). The issue itself says supplying the context "may fix the rejections without addressing the
variance", and that is exactly where things stand.
What is left is instance 1, and it is not a missing-material problem. The round that got the
SQL-injection claim right and the round that got it wrong were given the same code. Making the
inputs identical is not available as a fix either, for reasons the code settles rather than
guesses at:
FindingPipeline.refine/refineBatchOutcomepasspromptInputs.previousFindings()to theverifier, and that section grows with every posted round.
PrContextBudget.bound(prContext, budgetPlanner.perCallInputBudget(), …),so what is truncated moves with the diff and the model's window.
ReviewContextLoaderreads the pull request conversation, which changes as people comment.So two rounds on one head never see the same prompt, whatever the sampling settings are. This PR
makes the verdict stable by remembering it instead.
The fix
VerifierRejectionMemoryholds, per pull request and head commit, the candidates the audit threwout. A later round on that head drops those claims in
FindingPipelinebefore the verificationcall, so they are neither re-litigated nor paid for.
call; pinning one would repeat a false positive for the life of the head. The asymmetry is the
safe direction — this store can only ever publish fewer findings than before.
second test below is the control for exactly that.
FollowUpAnalyzer.isSameFinding, the rule the follow-up passes already use to recognize are-raise, rather than a second recognizer that could disagree with them. That method was the only
change to an existing class other than the pipeline wiring:
private→ package-private, with ajavadoc saying why.
forgotten first), 256 pull requests (least recently written evicted), in memory per replica, so a
restart falls back to today's re-litigation — the same bound
CiHoldRegistryandSupersededFindingsCarryoveraccept.The fail-open path
Yes, a fail-open path can publish a finding a successful verification would have dropped. The code
is explicit about it: in
FindingVerificationService.audit, an empty body, anyIOException/RuntimeException, and the spend-ceiling refusal allreturn screened— the candidate postsexactly as the reviewer raised it. That is the path behind the 16:00:29 UTC log line on our own
#871:
The right disclosure already exists and is not changed here: #623's
VerificationCoveragerecords(candidates, 0)on every one of those paths andVerdictBuilderrenders the clause — its ownjavadoc records that production measured roughly one review in three publishing findings no second
stage had screened. What was missing is that the disclosure does not drop anything, so a claim the
audit had already rejected still posted. Applying the memory before the call fixes that: the drop
holds on a round that never gets a verdict at all.
aRejectedFindingIsNotRePublishedByALaterRoundOnTheSameHeadstubs round two as the pass-through that every fail-open path produces, so it covers both the
"confirmed on the re-roll" and the "no verdict at all" shapes.
Sampling — investigated, deliberately unchanged
Verified in code rather than assumed:
FindingVerifieris bound to theconcisenamed model, whose generation parameters are appliedby
ChatModelCustomizers.ConciseChatModelCustomizerthroughBLOCKING_TUNING.temperature,topP,frequencyPenalty,presencePenaltyandseedfrom
ActiveModelSettings, each onlyifPresent. All five are unset inapplication.properties(thethrillhousebot.ai.models.*.temperature/.seedlines arecommented-out examples), so the verifier call today runs at the provider's defaults with no seed.
So a deterministic setting is already available on the configured OpenAI-compatible provider
without any code change —
thrillhousebot.ai.models.<model>.temperature=0and.seed=<n>reach theverifier's lane. This PR does not flip that default, for two reasons. The effect cannot be proven
from a deterministic test — a test can show the builder received
temperature(0.0), which is wiring,not behaviour — and the
concisebean is shared with the summary, changelog and reply calls, wherepinning temperature is a prose-quality change nobody has measured. More to the point, greedy
decoding would not have fixed this issue: the two rounds did not send the same prompt, for the three
reasons listed above, so identical sampling would still have permitted different verdicts.
Ordering, batch composition, truncation
Checked and found deterministic given the finding list, so nothing is changed there: candidate ids
are 1-based list positions in
renderCandidates,applyreads them back by the same positions, andbyCandidateIdcollapses duplicate ids first-wins for bothapplyandcandidatesCovered(#735).Batch composition comes from
DiffBudgetPlanner, which is a pure function of the file list and thebudget. The list order itself is model output and varies between rounds, which is one more reason
the stability has to come from memory rather than from input canonicalization.
Related Issues
Fixes #711
How Has This Been Tested?
Red first.
FindingPipelineTest#aRejectedFindingIsNotRePublishedByALaterRoundOnTheSameHeadruns two rounds against one
owner/repo#70at2659f683; round one's verifier rejects the claim,round two's returns its input unchanged (the shape both a
confirmedverdict and every fail-openpath take). Against the unfixed code:
Its control,
aRejectionOnAnOlderHeadDoesNotSuppressTheFindingAfterAPush, passes before and after:it pins the boundary the memory must never cross.
VerifierRejectionMemoryTestcovers the store itself — recall on the same head, recall of areworded re-raise, a finding the audit never rejected, the head moving, case-insensitive head
comparison, a push replacing rather than extending the entry, accumulation across rounds on one
head, a downgrade not read as a rejection, a finding citing no file, a session with no repository or
head, both caps.
ModelSuppliedTextInLogLinesTest#aCraftedPathAndTitleCannotForgeARecordFromTheRememberedRejectionDropholds the new INFO line to the same log-forgery guarantee as its neighbours (#742/#755).
Gates, from the worktree root:
Patch coverage against
origin/main, every addedsrc/mainline and branch:Checklist
Screenshots / Logs
The new drop is readable in the log, so a quiet round is never mistaken for a clean one:
Additional Notes
The memory is consulted and written in both review lanes — the single-call path in
FindingPipeline.refineand the per-batch path inrefineBatchOutcome— so a large pull requestgets the same stability as a small one. Batches partition by file and recall requires the same file,
so one batch's rejection can never drop another batch's finding within a round.
What this does not do is make a wrong rejection right. If the audit rejects a true finding, this
change makes that rejection stick for the head instead of the finding reappearing on a later round.
That is the trade the issue asks for — "a finding verified and rejected on a head could be
remembered for that head rather than re-litigated from scratch" — and the reason #871 had to land
first: it removes the material gap that was producing the wrong rejections in the first place.
No new dependency, no configuration knob, no extra model call.