From 9d697aa83dfbcc646760f18e7242def710fb2361 Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Sun, 16 Aug 2026 00:43:16 +0000 Subject: [PATCH] fix(review): collapse duplicate verdict ids the same way in both readers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #719 routed apply() and the coverage count through one decision normalizer so what the review does with a verdict and what it claims it verified cannot drift. The normalizer was shared; the duplicate-id resolution was not. apply() collapsed duplicates first — first element for an id wins — and read the label from the survivor, while candidatesCovered filtered by label first and de-duplicated the ids that were left. An id carrying an undecidable element followed by a decidable one therefore landed in apply()'s fail-open default, where the finding posts exactly as the reviewer raised it, while the count called that candidate screened. Every other candidate decided, that is FULL coverage on an unscreened finding: no banner, no coverage clause, no check-run brief, and no "N kept, M downgraded, K rejected" line either when nothing else was rejected or downgraded. That is the harm #623 exists to prevent and #710 was filed on, reached by a narrower route — on a complete, well-formed body, not only on a cut one. The drift is one-directional: the reverse ordering already agreed, so it only ever over-counted, exclusively toward the dangerous side. Both readers now resolve duplicates through one collapse, so the verdict the count reads for an id is exactly the one the audit acted on. Duplicates are not a hypothetical input shape: the collapse predates this fix precisely because the model emits them. decisionOf now strips, so the strictness genuinely matches the strictRisk and strictConfidence it documents itself against: "rejected " was unreadable as a decision while "high " was a readable rating. Both readers take that same value, so it never drove a drift — it cost an over-cautious keep and an under-count — but the asymmetry was unintended. Fixes #735 --- .../review/ai/FindingVerificationService.java | 53 ++++++++++--- .../ai/FindingVerificationServiceTest.java | 78 +++++++++++++++++++ 2 files changed, 120 insertions(+), 11 deletions(-) diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationService.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationService.java index 0aaef6cb..fbf68f5d 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationService.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationService.java @@ -32,6 +32,7 @@ import java.util.HashMap; import java.util.List; import java.util.Locale; +import java.util.Map; import java.util.Set; import java.util.function.Consumer; import java.util.regex.Matcher; @@ -728,6 +729,9 @@ private ReviewResponse salvageTruncatedVerdicts( * an id outside the 1-based candidate range; counting only the ids in range keeps the log honest * about what stayed unverified. * + *

Duplicate ids are collapsed through {@link #byCandidateId} before any label is read, so the + * verdict counted for an id is exactly the one {@link #apply} acted on (#735). + * *

An id alone is not coverage. A verdict whose decision label is absent, blank or not one of * {@link #ACTED_ON_DECISIONS} falls into {@link #apply}'s fail-open default, where the candidate * posts exactly as the reviewer raised it — the same state a candidate with no verdict at all @@ -753,24 +757,55 @@ private ReviewResponse salvageTruncatedVerdicts( */ private static long candidatesCovered( List salvaged, int candidates) { - return salvaged.stream() - .filter(verdict -> ACTED_ON_DECISIONS.contains(decisionOf(verdict))) - .mapToInt(VerificationResponse.Verdict::id) - .filter(id -> id >= 1 && id <= candidates) - .distinct() + return byCandidateId(salvaged).entrySet().stream() + .filter(entry -> entry.getKey() >= 1 && entry.getKey() <= candidates) + .filter(entry -> ACTED_ON_DECISIONS.contains(decisionOf(entry.getValue()))) .count(); } + /** + * The one verdict per candidate id, first element wins — the single collapse both {@link #apply} + * and {@link #candidatesCovered} resolve duplicates through, so neither can read a verdict the + * other ignored (#735). + * + *

Sharing {@link #decisionOf} was only half of what #710 needs. The two readers also have to + * agree on which verdict an id's decision is read from, and they did not: {@code apply} + * collapsed duplicates first and read the label from the survivor, while the count filtered by + * label first and de-duplicated the ids that were left. An id carrying an undecidable element + * followed by a decidable one therefore landed in {@code apply}'s fail-open default — the finding + * posts exactly as the reviewer raised it — while the count called that candidate screened, which + * is the published-set-reads-as-fully-screened harm #623 exists to prevent, reached by a narrower + * route. The drift is one-directional (the reverse order already agreed), so it only ever + * over-counted, i.e. exclusively toward the dangerous side. + * + *

Duplicates are model output, not a hypothetical: the collapse predates this fix, first-wins + * is the behaviour the audit already applies, and the count now inherits it rather than inventing + * a second resolution. + */ + private static Map byCandidateId( + List verdicts) { + var byId = new HashMap(); + for (VerificationResponse.Verdict verdict : verdicts) { + byId.putIfAbsent(verdict.id(), verdict); + } + return byId; + } + /** * The verdict's decision, normalized the one way both {@link #apply} and {@link * #candidatesCovered} read it; the empty string for a candidate with no verdict and for a verdict * carrying no label. One reader so the two cannot disagree about what counts as a decision — the * drift #710 was filed on. + * + *

Stripped, so the strictness genuinely matches {@link #strictRisk}/{@link #strictConfidence} + * as documented: {@code "rejected "} was unreadable as a decision while {@code "high "} was a + * readable rating, an asymmetry nothing intended (#735). Both readers take the same value, so + * this never drove a drift — it only cost an over-cautious keep and an under-count. */ private static String decisionOf(VerificationResponse.Verdict verdict) { return verdict == null || verdict.verdict() == null ? "" - : verdict.verdict().toLowerCase(Locale.ROOT); + : verdict.verdict().strip().toLowerCase(Locale.ROOT); } /** @@ -1181,11 +1216,7 @@ String renderCandidates(List findings) throws IOExceptio } ReviewResponse apply(ReviewResponse response, VerificationResponse verification) { - var byId = new HashMap(); - for (VerificationResponse.Verdict verdict : verification.verdicts()) { - byId.putIfAbsent(verdict.id(), verdict); - } - + var byId = byCandidateId(verification.verdicts()); var kept = new ArrayList(); var rejected = 0; var downgraded = 0; diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationServiceTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationServiceTest.java index 40c86d62..cd804eb9 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationServiceTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationServiceTest.java @@ -776,6 +776,84 @@ void doesNotCountAVerdictWhoseDecisionTheAuditCannotRead() { assertEquals(VerificationCoverage.Outcome.PARTIAL, reported.get(0).outcome()); } + @Test + void doesNotCountADuplicateIdWhoseFirstVerdictTheAuditCannotRead() { + // #735: a complete, well-formed body carrying two verdicts for the same id, the first of them + // undecidable. apply() collapses duplicates first-wins and lands in its fail-open default, so + // the finding posts exactly as the reviewer raised it; the count has to read that same first + // verdict rather than the decidable duplicate behind it, or the published set claims a second + // stage ruled on a finding it never ruled on — #710's harm on an uncut body. + ReviewResponse original = + response(finding("critical", "high", "Undecided"), finding("high", "high", "Ruled on")); + when(verifier.verify(anyString(), anyString(), anyString(), anyString(), anyString())) + .thenReturn( + aiOk( + """ + {"verdicts": [ + {"id": 1, "reason": "still weighing this one"}, + {"id": 1, "verdict": "rejected", "reason": "framework idiom"}, + {"id": 2, "verdict": "confirmed", "reason": "real"}]}""")); + var reported = new ArrayList(); + + var result = service.verify(SESSION, original, "diff", "stack", "", reported::add); + + assertEquals(2, result.findings().size()); + assertEquals("Undecided", result.findings().get(0).title()); + assertEquals(List.of(new VerificationCoverage(2, 1)), reported); + assertEquals(VerificationCoverage.Outcome.PARTIAL, reported.get(0).outcome()); + assertTrue(reported.get(0).disclosed()); + } + + @Test + void countsADuplicateIdWhoseFirstVerdictTheAuditActedOn() { + // The control for the case above: the same duplicate in the other order. apply() rejects on the + // first element, so the count must include the id — the two readers agreed here already, and + // collapsing the duplicate in the count must not turn that agreement into an under-count. + ReviewResponse original = + response(finding("critical", "high", "Ruled on"), finding("high", "high", "Also")); + when(verifier.verify(anyString(), anyString(), anyString(), anyString(), anyString())) + .thenReturn( + aiOk( + """ + {"verdicts": [ + {"id": 1, "verdict": "rejected", "reason": "framework idiom"}, + {"id": 1, "reason": "still weighing this one"}, + {"id": 2, "verdict": "confirmed", "reason": "real"}]}""")); + var reported = new ArrayList(); + + var result = service.verify(SESSION, original, "diff", "stack", "", reported::add); + + assertEquals(1, result.findings().size()); + assertEquals("Also", result.findings().get(0).title()); + assertEquals(List.of(new VerificationCoverage(2, 2)), reported); + assertFalse(reported.get(0).disclosed()); + } + + @Test + void readsADecisionLabelPaddedWithWhitespace() { + // #735: decisionOf did not strip while strictRisk/strictConfidence do, so "rejected " was + // unreadable as a decision even though "high " is a readable rating — an asymmetry against a + // javadoc claiming the strictness matches. Both readers take the same value, so the finding is + // acted on and counted together. + ReviewResponse original = + response(finding("critical", "high", "Padded"), finding("high", "high", "Kept")); + when(verifier.verify(anyString(), anyString(), anyString(), anyString(), anyString())) + .thenReturn( + aiOk( + """ + {"verdicts": [ + {"id": 1, "verdict": "rejected\\n", "reason": "framework idiom"}, + {"id": 2, "verdict": " CONFIRMED ", "reason": "real"}]}""")); + var reported = new ArrayList(); + + var result = service.verify(SESSION, original, "diff", "stack", "", reported::add); + + assertEquals(1, result.findings().size()); + assertEquals("Kept", result.findings().get(0).title()); + assertEquals(List.of(new VerificationCoverage(2, 2)), reported); + assertFalse(reported.get(0).disclosed()); + } + @Test void reportsZeroCoverageWhenTheCutLeavesNoCompleteVerdict() { ReviewResponse original = response(finding("critical", "high", "Bug"));