From bc3cb9d7d7d1a48f2aad887fb72f38948259baab Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Wed, 16 Sep 2026 14:43:33 -0300 Subject: [PATCH 1/3] fix(review): remember a rejected finding for the head it was rejected on 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. --- CHANGELOG.md | 1 + .../review/FindingPipeline.java | 16 +- .../review/FollowUpAnalyzer.java | 12 +- .../review/VerifierRejectionMemory.java | 254 ++++++++++++++++++ .../review/FindingPipelineTest.java | 94 +++++++ .../ModelSuppliedTextInLogLinesTest.java | 19 ++ .../review/ReviewOrchestratorTest.java | 2 + .../review/VerifierRejectionMemoryTest.java | 223 +++++++++++++++ 8 files changed, 617 insertions(+), 4 deletions(-) create mode 100644 src/main/java/dev/thiagogonzaga/thrillhousebot/review/VerifierRejectionMemory.java create mode 100644 src/test/java/dev/thiagogonzaga/thrillhousebot/review/VerifierRejectionMemoryTest.java diff --git a/CHANGELOG.md b/CHANGELOG.md index bfa0d9c2..eab11665 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ All notable changes to ThrillhouseBot. ### Fixed +- **A claim the verifier rejected is not published by a later round on the same commit** (#711): the second-pass audit reached opposite verdicts on one claim and one unchanged commit within a single dogfood round. It rejected a SQL-injection candidate with correct reasoning — the route value is gated by a catalog allow-list before it reaches the 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 nothing about being later makes the second answer the right one. Nor is a second audit guaranteed to happen: verification fails open, so an empty response body, a timeout or the review's spend ceiling posts the candidate exactly as the reviewer raised it, and production has logged rounds that published a finding no second stage had screened. Rejections are now remembered for the head commit they were reached on, and a later round on that head drops the claim before the verification call instead of putting it to the model again — so it also stays dropped on the rounds where no verdict comes back at all. Only rejections are remembered, never confirmations, which means the store can only ever publish fewer findings than before. A push clears the entry: a rejection is an answer about code, and the code changed. - **The three recurring container defects are graded the same way in every pull request** (#773): a round of dogfood scoring read 65 findings over twelve pull requests against the code — 62 of them true — and found the grading, not the findings, to be the problem. The same unpinned base image drew medium on four of those pull requests and low on three, with nothing in the changes to tell them apart. A root-owned volume under a non-root `USER` drew low on the pull request where the container dies on its first write and medium on another of the same shape. The same omission drew a confident inline finding in one half of a paired-language change and a low-confidence collapsed item in the other, on evidence that was equally provable from the diff in both. A severity that moves with the review rather than with the defect teaches a reader that the field carries no information. Three classes now carry a stated grade, decided by what the defect costs and who it reaches rather than by the review's impression of it: a mutable external reference (a base image, action or chart named by a tag instead of a digest) is medium, a path the running user cannot write is high, and a container that never drops privilege is medium, each at medium confidence. The review prompt states them, and a calibration stage applies them after the finding verifier, so a downgrade there cannot re-spread the class. It regrades only a finding anchored in a Dockerfile, manifest, compose file, workflow or Terraform file that states the class in its own words, and it leaves alone any finding that also asserts something the class does not cover — a privileged container, a host mount or namespace, an added capability, a credential, a named CVE. Nothing is dropped, added or reworded, so the finding set is exactly the one the review produced. Medium confidence puts every anchored finding on the diff instead of in the collapsed "Things to double-check" block, and leaves the merge verdict to the rest of the review: under the default `REVIEW_BLOCKING_STRICTNESS=balanced`, which needs high confidence, no anchored finding requests changes on its own, and one the review over-graded at critical with high confidence stops doing so, which is the calibration working rather than a side effect of it. The prompt also states the mirror of the severity rule for confidence, which is the half no deterministic stage can decide: equally provable defects get equal confidence, and an unequal one must name the fact it could not check - **Repeated timeouts on one AI call stop spending the whole retry budget** (#862): a streaming attempt waits `THRILLHOUSEBOT_REVIEW_AI_TIMEOUT_SECONDS`, 900 seconds in production, and a timeout was then retried like any other transient failure up to `THRILLHOUSEBOT_REVIEW_MAX_AI_RETRIES`, so one call could spend 75 minutes of wall clock while its review held the pull request's dispatcher slot. Production saw 20 timed-out attempts in 24 hours, all on one 503-file pull request, and the failed reviews of that day cost more than the completed ones. At most two attempts of one call may now end at the deadline: the second one fails the call instead of spending the attempts that are left, and the decision is logged at WARN with the session id, the attempt and the wait. Every other transient failure keeps the whole budget, a timeout followed by a successful attempt still succeeds, and the reasoning step-down's repeat (#839) shares the bound rather than getting a second pair of waits. The repeat keeps the full deadline, since it is there for the attempt whose first token never arrived because the provider queued the request, and the bound already brings the ceiling down from 75 minutes to 30. The setting still means one attempt's wait. The final summary call shares the loop and behaves the same way, and a review whose batches time out still discloses the files it did not read - **A review interrupted by a restart no longer stays `in_progress` for good** (#863): a session row is written `in_progress` when the review starts and updated when it ends, so a review killed between those two writes — a deploy restart, a crash, a `docker kill` — never got the terminal one and stayed `in_progress` for the life of the database. Production had 10 such rows, the oldest from 2026-06-09 and the newest from the 2026-09-09 restart, each counting as a running review and hiding the genuinely in-flight ones among them. Startup now moves every `in_progress` row to `failed` with "Review interrupted before it finished (bot restart or crash)" as the reason, which is what tells it apart on the dashboard from a review that failed on its own. Nothing carries a review across a restart, so a row still in progress at boot belongs to a review that is over: the sweep needs no age threshold and reconciles the rows stranded before it existed, with no manual SQL. Only the status and the reason are written, so the tokens and the cost the review had already paid for stay on the row. Sweeping every row at boot is safe because the bot is a single process; running more than one replica would need the rows to carry an owner first diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipeline.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipeline.java index 07bc64ad..75849d82 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipeline.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipeline.java @@ -194,6 +194,7 @@ private static AiReviewService.PromptInputs withDiff( private final FrameworkFalsePositiveFilter frameworkFilter; private final FindingDeduplicator deduplicator; private final FindingVerificationService findingVerificationService; + private final VerifierRejectionMemory rejectionMemory; private final FollowUpAnalyzer followUpAnalyzer; private final ObjectMapper mapper; private final BotIdentity botIdentity; @@ -209,6 +210,7 @@ public FindingPipeline( FrameworkFalsePositiveFilter frameworkFilter, FindingDeduplicator deduplicator, FindingVerificationService findingVerificationService, + VerifierRejectionMemory rejectionMemory, FollowUpAnalyzer followUpAnalyzer, ObjectMapper mapper, BotIdentity botIdentity, @@ -221,6 +223,7 @@ public FindingPipeline( this.frameworkFilter = frameworkFilter; this.deduplicator = deduplicator; this.findingVerificationService = findingVerificationService; + this.rejectionMemory = rejectionMemory; this.followUpAnalyzer = followUpAnalyzer; this.mapper = mapper; this.botIdentity = botIdentity; @@ -767,12 +770,15 @@ private BatchOutcome refineBatchOutcome( var attached = run.evidence().forFindings(batchResponse.findings()); var validated = quoteValidator.validate(batchResponse, batch.text()); validated = frameworkFilter.filter(validated, batch.text()); + // #711: as in the single-call lane — a claim the audit already rejected on this head is not + // put to the verifier a second time, and stays dropped on a round that never gets a verdict. + var candidates = rejectionMemory.withoutRejectionsOnThisHead(run.session(), validated); // #736: the verification call is the one review-path call that does no budget arithmetic of // its own, so the section the author alone sizes is bounded here before it is sent. var verified = findingVerificationService.verify( ledgerSessionId(run.session()), - validated, + candidates, PrContextBudget.bound( batchInputs.prContext(), budgetPlanner.perCallInputBudget(), tokenCounter), batchInputs.diff(), @@ -780,6 +786,7 @@ private BatchOutcome refineBatchOutcome( batchInputs.previousFindings(), attached, run.plan()::recordVerificationCoverage); + rejectionMemory.remember(run.session(), candidates.findings(), verified.findings()); return new BatchOutcome( index, verified.findings(), @@ -1435,12 +1442,16 @@ private ReviewResponse refine( aiResponse = quoteValidator.validate(aiResponse, diff); aiResponse = frameworkFilter.filter(aiResponse, diff); aiResponse = deduplicator.dedupe(aiResponse); + // #711: a claim the audit already rejected on this head is dropped before the call rather + // than put to it again. A second verdict on unchanged code is a re-roll of the same question, + // and on the fail-open paths there is no second verdict at all. + var candidates = rejectionMemory.withoutRejectionsOnThisHead(session, aiResponse); // #736: the verification call is the one review-path call that does no budget arithmetic of // its own, so the section the author alone sizes is bounded here before it is sent. aiResponse = findingVerificationService.verify( ledgerSessionId(session), - aiResponse, + candidates, PrContextBudget.bound( promptInputs.prContext(), budgetPlanner.perCallInputBudget(), tokenCounter), promptInputs.diff(), @@ -1448,6 +1459,7 @@ private ReviewResponse refine( promptInputs.previousFindings(), attached, plan::recordVerificationCoverage); + rejectionMemory.remember(session, candidates.findings(), aiResponse.findings()); // #773: the last word on the two graded fields, so the anchored infrastructure classes cannot // be re-spread by the verifier's own lowering, and the grade the publisher routes on is the // one persisted below for the next round to compare against. diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FollowUpAnalyzer.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FollowUpAnalyzer.java index 241f8450..fde7aa1e 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FollowUpAnalyzer.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FollowUpAnalyzer.java @@ -838,8 +838,16 @@ private static boolean sameAnchor(ReviewResponse.Finding finding, ReviewResponse && Math.abs(finding.line() - prior.line()) <= DUPLICATE_LINE_TOLERANCE; } - private static boolean isSameFinding( - ReviewResponse.Finding finding, ReviewResponse.Finding prior) { + /** + * Whether two findings raised in different rounds argue the same defect: the same file, and + * either the same anchor with a similar title or enough shared content to be one claim reworded. + * + *

Package-private so {@link VerifierRejectionMemory} recalls a rejection by the rule the + * follow-up passes already recognize a re-raise by (#711). A claim that comes back only because + * the model worded it differently is the same claim, and a second recognizer would let the two + * passes disagree about that. + */ + static boolean isSameFinding(ReviewResponse.Finding finding, ReviewResponse.Finding prior) { if (finding.file() == null || !FilePaths.same(finding.file(), prior.file())) { return false; } diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/VerifierRejectionMemory.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/VerifierRejectionMemory.java new file mode 100644 index 00000000..9aac19a0 --- /dev/null +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/VerifierRejectionMemory.java @@ -0,0 +1,254 @@ +/* + * Copyright 2026 Thiago Gonzaga + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package dev.thiagogonzaga.thrillhousebot.review; + +import dev.thiagogonzaga.thrillhousebot.LogSafe; +import dev.thiagogonzaga.thrillhousebot.dashboard.ReviewSession; +import dev.thiagogonzaga.thrillhousebot.review.ai.FindingVerificationService; +import dev.thiagogonzaga.thrillhousebot.review.ai.ReviewResponse; +import io.quarkus.logging.Log; +import jakarta.enterprise.context.ApplicationScoped; +import java.util.ArrayList; +import java.util.HashSet; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; + +/** + * Remembers, per pull request and head commit, the candidate findings the second-pass audit threw + * out, so a later round on that same head drops them instead of putting them to the model again + * (#711). + * + *

The verifier reached opposite verdicts on one claim and one commit: a CRITICAL SQL-injection + * candidate it rejected with correct reasoning was published forty minutes later, on an unchanged + * head, at the highest severity 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 asked again of a sampled model, and the + * answer that reaches the pull request is whichever round happened to post. Only one of the two can + * be right, and nothing about being later makes the second one it. + * + *

Nor is a second audit guaranteed to happen at all. Verification fails open by design: an empty + * response body, a timeout, a refusal, or the review's spend ceiling all keep the candidate exactly + * as the reviewer raised it, and {@link VerificationCoverage} was added because production measured + * roughly one review in three publishing findings no second stage had screened. On such a round a + * claim the audit had already rejected posts with nothing standing between it and the pull request. + * That is the shape production showed on 2026-09-16, where the review body oscillated 0, 1, 0, 2, + * 1, 0 findings across rounds on near-identical code, and one round logged "Finding verification + * returned no response body — keeping the 1 unverified finding(s)". + * + *

Determinism cannot come from the inputs here. Two rounds on one head do not see the same + * material: 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. The verdict is made stable by remembering it instead. + * + *

Only rejections are remembered, never confirmations. Re-raising a claim the audit rejected is + * the failure #711 reports; re-auditing one it confirmed costs nothing but a call, and pinning a + * "confirmed" would turn a single false positive into one the bot repeats for the life of the head. + * The asymmetry is the safe direction: this store can only ever publish fewer findings than today. + * + *

An entry is bound to the head it was gathered on. A push replaces it, because a rejection is + * an answer about code and the code changed — the memory must not outlive the lines that earned it. + * The store holds one entry per pull request, capped in findings, and is capped in pull requests, + * evicting the one least recently written. It lives in memory per replica, so a restart falls back + * to today's re-litigation, the same bound {@link CiHoldRegistry} and {@link + * SupersededFindingsCarryover} accept. + */ +@ApplicationScoped +public class VerifierRejectionMemory { + + /** + * Rejections held per pull request. Past this the oldest are forgotten: a claim raised many + * rounds ago and never raised since is the one whose suppression is worth the least. + */ + static final int MAX_REJECTIONS = 50; + + /** Pull requests remembered at once; the entry least recently written is evicted past this. */ + static final int MAX_PULL_REQUESTS = 256; + + private record PrKey(String repository, int prNumber) {} + + /** A pull request's rejections and the head they were reached on. */ + private record Entry(String headSha, List rejected) {} + + private final Object lock = new Object(); + + /** Insertion-ordered so eviction past the cap drops the least recently written pull request. */ + private final Map entries = new LinkedHashMap<>(); + + /** + * The candidates to hand the verifier: {@code response} without the findings a previous round + * already rejected on this head. Applied before the call rather than after it, so the claim is + * neither re-litigated nor paid for, and so the drop holds on the rounds where no verdict comes + * back at all. + * + *

A remembered rejection is matched the way every other cross-round comparison in the review + * matches one — {@link FollowUpAnalyzer#isSameFinding} — rather than on exact text: the model + * rewords a title between rounds, and a claim that survives only by being phrased differently is + * the same claim. + */ + public ReviewResponse withoutRejectionsOnThisHead( + ReviewSession session, ReviewResponse response) { + if (response.findings().isEmpty()) { + return response; + } + var rejected = rejectedAt(session); + if (rejected.isEmpty()) { + return response; + } + var kept = new ArrayList(response.findings().size()); + for (ReviewResponse.Finding finding : response.findings()) { + if (wasRejected(finding, rejected)) { + Log.infof( + "Dropping finding '%s' (%s:%d): the second-pass audit rejected the same claim on head" + + " %s in an earlier round of this pull request", + LogSafe.oneLine(finding.title()), + LogSafe.oneLine(finding.file()), + finding.line(), + session.getCommitSha()); + } else { + kept.add(finding); + } + } + if (kept.size() == response.findings().size()) { + return response; + } + return new ReviewResponse( + kept, + response.previousFindingsStatus(), + FindingVerificationService.recount(response.summary(), kept)); + } + + /** + * Records what this round's audit threw out: the candidates it was handed that its result no + * longer carries. That difference is exactly the audit's own removals — a "rejected" verdict, and + * the deterministic self-retraction screen in front of the call — because every other stage of + * verification changes a finding's risk or confidence and keeps its location and title, which is + * what the difference is taken on. + * + *

A round whose verification failed open removed nothing and so remembers nothing, leaving the + * earlier rounds' answers standing. + */ + public void remember( + ReviewSession session, + List candidates, + List published) { + if (!identifies(session)) { + return; + } + var rejected = removed(candidates, published); + if (rejected.isEmpty()) { + return; + } + var key = new PrKey(session.getRepository(), session.getPrNumber()); + int held; + synchronized (lock) { + var previous = entries.remove(key); + var kept = + previous == null || !previous.headSha().equalsIgnoreCase(session.getCommitSha()) + ? new ArrayList() + : new ArrayList<>(previous.rejected()); + kept.addAll(rejected); + var overflow = kept.size() - MAX_REJECTIONS; + if (overflow > 0) { + kept.subList(0, overflow).clear(); + } + // Re-inserted so the entry counts as the newest for eviction purposes. + entries.put(key, new Entry(session.getCommitSha(), List.copyOf(kept))); + while (entries.size() > MAX_PULL_REQUESTS) { + entries.remove(entries.keySet().iterator().next()); + } + held = kept.size(); + } + Log.infof( + "Remembering %d finding(s) the audit rejected for %s#%d at %s (%d held); a later round on" + + " the same head will not put them to the verifier again", + rejected.size(), + session.getRepository(), + session.getPrNumber(), + session.getCommitSha(), + held); + } + + /** The rejections remembered for this pull request, only when they were reached on its head. */ + private List rejectedAt(ReviewSession session) { + if (!identifies(session)) { + return List.of(); + } + synchronized (lock) { + var entry = entries.get(new PrKey(session.getRepository(), session.getPrNumber())); + return entry == null || !entry.headSha().equalsIgnoreCase(session.getCommitSha()) + ? List.of() + : entry.rejected(); + } + } + + private static boolean wasRejected( + ReviewResponse.Finding finding, List rejected) { + for (ReviewResponse.Finding prior : rejected) { + if (FollowUpAnalyzer.isSameFinding(finding, prior)) { + return true; + } + } + return false; + } + + /** + * The candidates absent from the published set, compared on {@link FindingKey} — the same + * within-one-round identity the evidence resolvers read a rebuilt finding back by. Exact rather + * than tolerant here on purpose: both lists come from one call, so a near-match is a second + * finding, not the same one reworded. + * + *

A finding citing no file is skipped. There is no anchor to recognize it by on the next + * round, so remembering it could only ever match something else. + */ + private static List removed( + List candidates, List published) { + var survived = new HashSet(); + for (ReviewResponse.Finding finding : published) { + var key = FindingKey.of(finding); + if (key != null) { + survived.add(key); + } + } + var rejected = new ArrayList(); + for (ReviewResponse.Finding finding : candidates) { + var key = FindingKey.of(finding); + if (key != null && !survived.contains(key)) { + rejected.add(finding); + } + } + return rejected; + } + + /** + * Whether the session names a pull request and a head to key on. A blank head is not a head: a + * rejection stored under one would be recalled by every later round whose head is equally + * unknown, which is the cross-push suppression the head key exists to prevent. + */ + private static boolean identifies(ReviewSession session) { + return !isBlank(session.getRepository()) && !isBlank(session.getCommitSha()); + } + + private static boolean isBlank(String value) { + return value == null || value.isBlank(); + } + + /** Visible for tests that assert the caps. */ + int size() { + synchronized (lock) { + return entries.size(); + } + } +} diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipelineTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipelineTest.java index 72b36c36..e6aa855c 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipelineTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipelineTest.java @@ -88,6 +88,7 @@ void setUp() { frameworkFilter, deduplicator, findingVerificationService, + new VerifierRejectionMemory(), followUpAnalyzer, new ObjectMapper(), BotIdentity.from(List.of("thrillhousebot[bot]")), @@ -2451,6 +2452,7 @@ void verifierAiCallsAreSkippedOnceTheSpendCeilingIsReached() { frameworkFilter, deduplicator, realVerificationService, + new VerifierRejectionMemory(), followUpAnalyzer, new ObjectMapper(), BotIdentity.from(List.of("thrillhousebot[bot]")), @@ -2507,6 +2509,7 @@ void aDisabledCeilingLeavesTheMultiCallPathUntouched() { frameworkFilter, deduplicator, findingVerificationService, + new VerifierRejectionMemory(), followUpAnalyzer, new ObjectMapper(), BotIdentity.from(List.of("thrillhousebot[bot]")), @@ -2649,6 +2652,7 @@ void findingsSerializationFailureFallsBackToEmptyArray() throws Exception { frameworkFilter, deduplicator, findingVerificationService, + new VerifierRejectionMemory(), followUpAnalyzer, throwingMapper, BotIdentity.from(List.of("thrillhousebot[bot]")), @@ -3105,4 +3109,94 @@ private static ReviewResponse withoutSuggestions(ReviewResponse response) { } return new ReviewResponse(stripped, response.previousFindingsStatus(), response.summary()); } + + /** + * #711 — the verifier reached opposite verdicts on the same claim and the same commit across + * rounds: a CRITICAL SQL-injection candidate it rejected in one round was published in the next, + * forty minutes later, on an unchanged head. The second round's audit is no more authoritative + * than the first's, and on the fail-open paths (an empty response body, a timeout, the spend + * ceiling) there is no second audit at all — the candidate posts exactly as the reviewer raised + * it. A rejection is therefore remembered for the head it was reached on, and a later round on + * that same head drops the claim instead of re-rolling it. + */ + @Test + void aRejectedFindingIsNotRePublishedByALaterRoundOnTheSameHead() { + var ctx = reviewContext(); + var template = new AiReviewService.PromptInputs("d", "ctx", "base", "stack", "tests", "", ""); + var raised = + new ReviewResponse( + List.of(finding("Repositories/ReservationRepository.cs", "Concatenated SQL")), + List.of(), + null); + when(aiReviewService.review(any(), any())).thenReturn(raised); + when(findingVerificationService.verify( + anyLong(), any(), any(), any(), any(), any(), any(), any())) + // Round one rejects the candidate. + .thenReturn(new ReviewResponse(List.of(), List.of(), null)) + // Round two keeps it, the shape both a "confirmed" verdict and every fail-open path take. + .thenAnswer(inv -> inv.getArgument(1)); + + var first = + pipeline.run( + ReviewSession.create("owner/repo", 70, "PR", "2659f683"), + template, + ctx, + singleBatchPlan(batch("Repositories/ReservationRepository.cs"), List.of()), + new DiffLineResolver(Map.of()), + ReviewEvidence.NONE); + assertEquals(0, first.findings().size(), "round one's audit rejected the claim"); + + var second = + pipeline.run( + ReviewSession.create("owner/repo", 70, "PR", "2659f683"), + template, + ctx, + singleBatchPlan(batch("Repositories/ReservationRepository.cs"), List.of()), + new DiffLineResolver(Map.of()), + ReviewEvidence.NONE); + + assertEquals( + List.of(), + second.findings().stream().map(ReviewResponse.Finding::title).toList(), + "a claim rejected on this head must not be published by a later round on the same head"); + } + + /** + * The bound on the memory above: a rejection describes the code it was reached on, so a push + * clears it. Without this the audit's answer on an old head would outlive the code that earned it + * and suppress a finding on lines that have since changed. + */ + @Test + void aRejectionOnAnOlderHeadDoesNotSuppressTheFindingAfterAPush() { + var ctx = reviewContext(); + var template = new AiReviewService.PromptInputs("d", "ctx", "base", "stack", "tests", "", ""); + var raised = + new ReviewResponse(List.of(finding("a.java", "Concatenated SQL")), List.of(), null); + when(aiReviewService.review(any(), any())).thenReturn(raised); + when(findingVerificationService.verify( + anyLong(), any(), any(), any(), any(), any(), any(), any())) + .thenReturn(new ReviewResponse(List.of(), List.of(), null)) + .thenAnswer(inv -> inv.getArgument(1)); + + pipeline.run( + ReviewSession.create("owner/repo", 70, "PR", "2659f683"), + template, + ctx, + singleBatchPlan(batch("a.java"), List.of()), + new DiffLineResolver(Map.of()), + ReviewEvidence.NONE); + var afterPush = + pipeline.run( + ReviewSession.create("owner/repo", 70, "PR", "9c1d4e07"), + template, + ctx, + singleBatchPlan(batch("a.java"), List.of()), + new DiffLineResolver(Map.of()), + ReviewEvidence.NONE); + + assertEquals( + List.of("Concatenated SQL"), + afterPush.findings().stream().map(ReviewResponse.Finding::title).toList(), + "the new head's finding must be judged on its own"); + } } diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ModelSuppliedTextInLogLinesTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ModelSuppliedTextInLogLinesTest.java index a65e67a2..c55dfd08 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ModelSuppliedTextInLogLinesTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ModelSuppliedTextInLogLinesTest.java @@ -358,6 +358,7 @@ void aCraftedPathAndTitleCannotForgeARecordFromTheAnchorBackfill() { mock(FrameworkFalsePositiveFilter.class), mock(FindingDeduplicator.class), mock(FindingVerificationService.class), + new VerifierRejectionMemory(), mock(FollowUpAnalyzer.class), new ObjectMapper(), BotIdentity.of("thrillhousebot"), @@ -503,4 +504,22 @@ void aCraftedTitleCannotForgeARecordFromTheVerifierRejection() { assertRecordCannotBeForged(captured, "Missing null check"); } + + /** {@link VerifierRejectionMemory} dropping a claim the audit rejected on this head (#711). */ + @Test + void aCraftedPathAndTitleCannotForgeARecordFromTheRememberedRejectionDrop() { + var memory = new VerifierRejectionMemory(); + var session = + dev.thiagogonzaga.thrillhousebot.dashboard.ReviewSession.create( + "owner/repo", 70, "PR", "2659f683"); + var claim = finding(FORGED_PATH, 1, FORGED_TITLE, "old"); + memory.remember(session, List.of(claim), List.of()); + + var captured = + logsOf( + VerifierRejectionMemory.class, + () -> memory.withoutRejectionsOnThisHead(session, response(claim))); + + assertRecordCannotBeForged(captured, "Missing null check"); + } } diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestratorTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestratorTest.java index 48af2dfd..e65995af 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestratorTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestratorTest.java @@ -164,6 +164,7 @@ void setUp() { frameworkFilter, deduplicator, findingVerificationService, + new VerifierRejectionMemory(), followUpAnalyzer, mapper, BOT_ID, @@ -5536,6 +5537,7 @@ void shouldHandleSerializationFailureGracefully() throws Exception { frameworkFilter, deduplicator, findingVerificationService, + new VerifierRejectionMemory(), followUpAnalyzer, badMapper, BOT_ID, diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/VerifierRejectionMemoryTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/VerifierRejectionMemoryTest.java new file mode 100644 index 00000000..1bcc6e78 --- /dev/null +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/VerifierRejectionMemoryTest.java @@ -0,0 +1,223 @@ +/* + * Copyright 2026 Thiago Gonzaga + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package dev.thiagogonzaga.thrillhousebot.review; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertSame; + +import dev.thiagogonzaga.thrillhousebot.dashboard.ReviewSession; +import dev.thiagogonzaga.thrillhousebot.review.ai.ReviewResponse; +import java.util.ArrayList; +import java.util.List; +import org.junit.jupiter.api.Test; + +/** Unit tests for {@link VerifierRejectionMemory} (#711). */ +class VerifierRejectionMemoryTest { + + private final VerifierRejectionMemory memory = new VerifierRejectionMemory(); + + private static ReviewSession session(String headSha) { + return ReviewSession.create("owner/repo", 70, "PR", headSha); + } + + private static ReviewResponse.Finding finding(String file, int line, String title) { + return new ReviewResponse.Finding( + "critical", "high", file, line, title, "description of the claim", "old", "new"); + } + + private static ReviewResponse response(ReviewResponse.Finding... findings) { + return new ReviewResponse( + List.of(findings), + List.of(), + new ReviewResponse.Summary( + findings.length, findings.length, 0, 0, 0, "assessment", "purpose", List.of())); + } + + private static List titlesOf(ReviewResponse response) { + return response.findings().stream().map(ReviewResponse.Finding::title).toList(); + } + + @Test + void recallsARejectionReachedOnTheSameHead() { + var head = session("2659f683"); + var claim = finding("Repositories/ReservationRepository.cs", 31, "Concatenated SQL"); + memory.remember(head, List.of(claim), List.of()); + + var next = memory.withoutRejectionsOnThisHead(session("2659f683"), response(claim)); + + assertEquals(List.of(), titlesOf(next)); + assertEquals(0, next.summary().totalFindings(), "the summary counts are recomputed"); + assertEquals("assessment", next.summary().overallAssessment(), "the prose fields survive"); + } + + /** The model rewords a title between rounds; the claim underneath is the same one. */ + @Test + void recallsARejectionRaisedAgainUnderADifferentWording() { + memory.remember( + session("2659f683"), + List.of(finding("src/Repo.cs", 31, "Concatenated SQL reaches the query")), + List.of()); + + var reworded = finding("src/Repo.cs", 32, "Concatenated SQL reaches the query unparameterized"); + var next = memory.withoutRejectionsOnThisHead(session("2659f683"), response(reworded)); + + assertEquals(List.of(), titlesOf(next)); + } + + @Test + void keepsAFindingTheAuditNeverRejected() { + memory.remember(session("2659f683"), List.of(finding("src/A.cs", 3, "Rejected")), List.of()); + + var other = response(finding("src/B.cs", 9, "A different defect entirely")); + var next = memory.withoutRejectionsOnThisHead(session("2659f683"), other); + + assertSame(other, next, "a response with nothing to drop is returned unchanged"); + } + + @Test + void forgetsARejectionOnceTheHeadMoves() { + var claim = finding("src/A.cs", 3, "Concatenated SQL"); + memory.remember(session("2659f683"), List.of(claim), List.of()); + + var afterPush = memory.withoutRejectionsOnThisHead(session("9c1d4e07"), response(claim)); + + assertEquals(List.of("Concatenated SQL"), titlesOf(afterPush)); + } + + /** The head is compared case-insensitively, as GitHub's own SHAs are spelled either way. */ + @Test + void recallsARejectionWhenTheHeadIsSpelledInAnotherCase() { + var claim = finding("src/A.cs", 3, "Concatenated SQL"); + memory.remember(session("2659F683"), List.of(claim), List.of()); + + assertEquals( + List.of(), + titlesOf(memory.withoutRejectionsOnThisHead(session("2659f683"), response(claim)))); + } + + @Test + void aPushReplacesTheEntryRatherThanAddingToIt() { + var first = finding("src/A.cs", 3, "Rejected before the push"); + var second = finding("src/B.cs", 9, "Rejected after the push"); + memory.remember(session("2659f683"), List.of(first), List.of()); + memory.remember(session("9c1d4e07"), List.of(second), List.of()); + + var next = memory.withoutRejectionsOnThisHead(session("9c1d4e07"), response(first, second)); + + assertEquals( + List.of("Rejected before the push"), + titlesOf(next), + "only the rejection reached on the current head is recalled"); + assertEquals(1, memory.size(), "the pull request holds one entry"); + } + + @Test + void accumulatesRejectionsAcrossRoundsOnOneHead() { + var first = finding("src/A.cs", 3, "Rejected in round one"); + var second = finding("src/B.cs", 9, "Rejected in round two"); + memory.remember(session("2659f683"), List.of(first), List.of()); + memory.remember(session("2659f683"), List.of(second), List.of()); + + var next = memory.withoutRejectionsOnThisHead(session("2659f683"), response(first, second)); + + assertEquals(List.of(), titlesOf(next)); + } + + @Test + void rememberNothingWhenTheAuditKeptEveryCandidate() { + var kept = finding("src/A.cs", 3, "Confirmed"); + // Verification lowers risk and confidence and keeps the location and title, so the difference + // the memory is taken on must not read a downgrade as a rejection. + var downgraded = + new ReviewResponse.Finding( + "medium", "low", "src/A.cs", 3, "Confirmed", "description of the claim", "old", "new"); + memory.remember(session("2659f683"), List.of(kept), List.of(downgraded)); + + assertEquals(0, memory.size(), "a round that rejected nothing remembers nothing"); + assertEquals( + List.of("Confirmed"), + titlesOf(memory.withoutRejectionsOnThisHead(session("2659f683"), response(kept)))); + } + + @Test + void rememberNothingForAFindingThatCitesNoFile() { + var anchorless = finding("", 0, "Nowhere in particular"); + memory.remember(session("2659f683"), List.of(anchorless), List.of()); + + assertEquals(0, memory.size()); + } + + @Test + void rememberNothingWithoutARepositoryOrAHead() { + var claim = finding("src/A.cs", 3, "Concatenated SQL"); + memory.remember(ReviewSession.create("", 70, "PR", "2659f683"), List.of(claim), List.of()); + memory.remember(ReviewSession.create("owner/repo", 70, "PR", null), List.of(claim), List.of()); + + assertEquals(0, memory.size()); + assertEquals( + List.of("Concatenated SQL"), + titlesOf( + memory.withoutRejectionsOnThisHead( + ReviewSession.create("owner/repo", 70, "PR", " "), response(claim))), + "a session with no head recalls nothing either"); + } + + @Test + void anEmptyFindingListIsReturnedUnchanged() { + var empty = response(); + assertSame(empty, memory.withoutRejectionsOnThisHead(session("2659f683"), empty)); + } + + @Test + void aPullRequestWithNothingRememberedIsReturnedUnchanged() { + var raised = response(finding("src/A.cs", 3, "Concatenated SQL")); + assertSame(raised, memory.withoutRejectionsOnThisHead(session("2659f683"), raised)); + } + + @Test + void forgetsTheOldestRejectionsPastTheCap() { + var head = session("2659f683"); + var rejected = new ArrayList(); + for (var i = 0; i < VerifierRejectionMemory.MAX_REJECTIONS + 1; i++) { + rejected.add(finding("src/F" + i + ".cs", 3, "Rejected claim number " + i)); + } + memory.remember(head, rejected, List.of()); + + var oldest = rejected.get(0); + var newest = rejected.get(rejected.size() - 1); + assertEquals( + List.of(oldest.title()), + titlesOf(memory.withoutRejectionsOnThisHead(session("2659f683"), response(oldest, newest))), + "the oldest rejection is forgotten past the cap and the newest is still recalled"); + } + + @Test + void evictsTheLeastRecentlyWrittenPullRequestPastTheCap() { + var claim = finding("src/A.cs", 3, "Concatenated SQL"); + for (var pr = 1; pr <= VerifierRejectionMemory.MAX_PULL_REQUESTS + 1; pr++) { + memory.remember( + ReviewSession.create("owner/repo", pr, "PR", "2659f683"), List.of(claim), List.of()); + } + + assertEquals(VerifierRejectionMemory.MAX_PULL_REQUESTS, memory.size()); + assertEquals( + List.of("Concatenated SQL"), + titlesOf( + memory.withoutRejectionsOnThisHead( + ReviewSession.create("owner/repo", 1, "PR", "2659f683"), response(claim))), + "the first pull request written was evicted"); + } +} From fdee8de52468f4a6eb37a43217c773f54121ec5a Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Wed, 16 Sep 2026 14:56:55 -0300 Subject: [PATCH 2/3] test(review): cover the anchorless published finding in the rejection 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. --- .../review/VerifierRejectionMemoryTest.java | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/VerifierRejectionMemoryTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/VerifierRejectionMemoryTest.java index 1bcc6e78..27d2c739 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/VerifierRejectionMemoryTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/VerifierRejectionMemoryTest.java @@ -152,6 +152,22 @@ void rememberNothingWhenTheAuditKeptEveryCandidate() { titlesOf(memory.withoutRejectionsOnThisHead(session("2659f683"), response(kept)))); } + /** + * The published side is read by the same anchor. A finding that cites no file is skipped there + * too, so it can neither stand in for a rejected candidate nor be mistaken for one. + */ + @Test + void readsThePublishedSetByTheSameAnchor() { + var rejected = finding("src/A.cs", 3, "Rejected"); + var anchorless = finding("", 0, "Nowhere in particular"); + memory.remember(session("2659f683"), List.of(rejected, anchorless), List.of(anchorless)); + + assertEquals( + List.of(), + titlesOf(memory.withoutRejectionsOnThisHead(session("2659f683"), response(rejected))), + "the anchored candidate the audit dropped is remembered"); + } + @Test void rememberNothingForAFindingThatCitesNoFile() { var anchorless = finding("", 0, "Nowhere in particular"); From 6bf9856f32170a51265c3cbafdb8a6085c71ba31 Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Wed, 16 Sep 2026 15:09:42 -0300 Subject: [PATCH 3/3] docs(review): say which half of the rejection difference a failed-open 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/review/VerifierRejectionMemory.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/VerifierRejectionMemory.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/VerifierRejectionMemory.java index 9aac19a0..e0647faa 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/VerifierRejectionMemory.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/VerifierRejectionMemory.java @@ -137,8 +137,12 @@ public ReviewResponse withoutRejectionsOnThisHead( * verification changes a finding's risk or confidence and keeps its location and title, which is * what the difference is taken on. * - *

A round whose verification failed open removed nothing and so remembers nothing, leaving the - * earlier rounds' answers standing. + *

The two halves of that difference behave differently when the model call fails open. The + * screen sits in front of the call and runs on every round, and the fail-open paths in {@link + * FindingVerificationService} all return its output, so a round that lost its call still records + * what the screen retracted — a deterministic drop the next round would make again anyway. The + * verdict half contributes nothing on such a round, so a candidate the model never ruled on is + * left for a later round to rule on, with the earlier rounds' answers standing. */ public void remember( ReviewSession session,