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"));