From 7c5096d7a446b2323002818496082ae6e0924cdd Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Thu, 13 Aug 2026 11:59:13 -0300 Subject: [PATCH] fix(review): give failed review calls their own coverage clause and keep the legacy omitted count alongside summary degradations (#655, #659) Files a failed batch left unreviewed were folded into the budget-omission class, so the banner blamed the diff budget while the summary overview said the call did not complete. They are now their own TruncationDetail class, disclosed as 'not reviewed because the review call for them did not complete' in the clause and the check-run brief. On the legacy line-cap path, a detail carrying only a summary degradation skipped the numeric fallback and nothing below it read the omitted count, so the count vanished; it now renders as a clause alongside the degradation. --- CHANGELOG.md | 2 + .../thrillhousebot/review/ReviewResult.java | 56 +++++- .../thrillhousebot/review/VerdictBuilder.java | 9 +- .../review/ReviewResultTest.java | 163 +++++++++++++++++- .../review/VerdictBuilderTest.java | 74 +++++++- 5 files changed, 290 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cf5e1de2..f245f740 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,8 @@ All notable changes to ThrillhouseBot. ### Fixed +- **Coverage banner no longer blames the diff budget for files whose review call failed** (#655): files a failed batch left unreviewed are now their own disclosure class — "not reviewed because the review call for them did not complete" — in the banner, the coverage clause and the check-run brief, matching the summary overview's per-file note instead of contradicting it and pointing the operator at a budget knob that cannot help +- **Coverage clause keeps the legacy omitted-file count alongside summary degradations** (#659): on the legacy line-cap path, a review that also had its summary shortened or skipped dropped the "N file(s) were omitted" count entirely; the numeric clause now renders next to the summary-degradation clause instead of vanishing - **A provider context-length rejection is no longer retried at full price** (#622): a rejection for exceeding the model's context window is deterministic, so the review call now fails fast on the first attempt instead of re-billing up to `max-ai-retries` identical requests. In a multi-call review the rejected batch's files are disclosed as not reviewed while the other batches keep their findings; a single-call review fails with a notice and check run that name the cause and the `REVIEW_MAX_INPUT_TOKENS` knob to lower, instead of generic retry advice - **Dashboard token test no longer assumes an en-US locale** (#661): the test now asserts the same `toLocaleString()` output the component renders, so it passes on machines with any runtime locale - **Injection-sink floor no longer under-fires on three defeater-pattern gaps** (#676): "Nothing sanitizes ..." now registers as an absence claim, "does not prevent SQL injection" is no longer read as a denial of the sink, and a parenthetical coordinator inside a conditional ("If, however, ...") no longer truncates the hypothetical span and leaves its mitigation verb read as asserted diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewResult.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewResult.java index 9d00e538..7936ddf1 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewResult.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewResult.java @@ -171,6 +171,8 @@ public ReviewResult( * was reached — a different reason with a different fix, so the rendered copy names it separately * — and {@code responseCutFileNames} were only partially reviewed because the model's response * was cut at its length cap and the findings up to the cut were kept (#500). {@code + * callFailedFileNames} were sent but their review call failed all its retries (#655) — a + * different reason again, so the rendered copy must not blame the diff budget for them. {@code * summaryDegradation} marks the same degradations on the summary call: the findings are complete, * but the prose summary either was salvaged from a length-cap-cut response or replaced by the * counts-only fallback ({@link SummaryDegradation#RESPONSE_CUT}), or the call was skipped (or @@ -185,9 +187,31 @@ public record TruncationDetail( List clippedFileNames, List spendCeilingSkippedFileNames, List responseCutFileNames, + List callFailedFileNames, SummaryDegradation summaryDegradation) { public static final TruncationDetail EMPTY = - new TruncationDetail(List.of(), List.of(), List.of(), List.of(), SummaryDegradation.NONE); + new TruncationDetail( + List.of(), List.of(), List.of(), List.of(), List.of(), SummaryDegradation.NONE); + + /** + * Convenience constructor for details built before the call-failed class existed (and tests): + * no review call failed, so no such clause is rendered. The production path ({@code + * VerdictBuilder}) passes the real list through the canonical constructor. + */ + public TruncationDetail( + List omittedFileNames, + List clippedFileNames, + List spendCeilingSkippedFileNames, + List responseCutFileNames, + SummaryDegradation summaryDegradation) { + this( + omittedFileNames, + clippedFileNames, + spendCeilingSkippedFileNames, + responseCutFileNames, + List.of(), + summaryDegradation); + } public TruncationDetail { omittedFileNames = omittedFileNames == null ? List.of() : List.copyOf(omittedFileNames); @@ -198,6 +222,8 @@ public record TruncationDetail( : List.copyOf(spendCeilingSkippedFileNames); responseCutFileNames = responseCutFileNames == null ? List.of() : List.copyOf(responseCutFileNames); + callFailedFileNames = + callFailedFileNames == null ? List.of() : List.copyOf(callFailedFileNames); summaryDegradation = summaryDegradation == null ? SummaryDegradation.NONE : summaryDegradation; } @@ -207,7 +233,7 @@ public boolean isEmpty() { } /** - * Whether any per-file coverage gap exists — a name in any of the four file classes. False for + * Whether any per-file coverage gap exists — a name in any of the five file classes. False for * a detail whose only content is a summary degradation: the findings then cover the whole diff, * so surfaces whose framing is per-file partial coverage (the on-demand disclosure, the delta * comment) treat such a detail as empty (#516) while the summary-aware surfaces (banner, @@ -217,7 +243,8 @@ public boolean hasFileGaps() { return !omittedFileNames.isEmpty() || !clippedFileNames.isEmpty() || !spendCeilingSkippedFileNames.isEmpty() - || !responseCutFileNames.isEmpty(); + || !responseCutFileNames.isEmpty() + || !callFailedFileNames.isEmpty(); } } @@ -513,6 +540,17 @@ static String coverageGapClause(int omittedFiles, TruncationDetail detail) { detail.spendCeilingSkippedFileNames().size(), nameList(detail.spendCeilingSkippedFileNames()))); } + // Runtime call failure carries its own reason too (#655): these files fit the diff budget and + // were sent — the review call for them failed all its retries — so the budget wording would + // misdirect the operator toward a knob that cannot help, and the summary overview already + // says the call did not complete; the two surfaces must agree. + if (!detail.callFailedFileNames().isEmpty()) { + clauses.add( + String.format( + "%d file(s) were not reviewed because the review call for them did not complete" + + " (%s)", + detail.callFailedFileNames().size(), nameList(detail.callFailedFileNames()))); + } // The response-cut class is partial in a third way: the files were sent and reviewed, but the // model's answer was cut at its length cap — the findings produced before the cut were kept, // so "not reviewed" would understate the coverage and silence the honest caveat. @@ -523,6 +561,12 @@ static String coverageGapClause(int omittedFiles, TruncationDetail detail) { + " its length cap (max-output-tokens) — findings up to the cut were kept (%s)", detail.responseCutFileNames().size(), nameList(detail.responseCutFileNames()))); } + // A detail carrying only a summary degradation still owes the reader the legacy omitted-file + // count (#659): nothing below the fallback reads the int, so the count vanished whenever the + // summary also degraded — the coverage disclosure this class exists to guarantee. + if (!detail.hasFileGaps() && omittedFiles > 0) { + clauses.add(omittedFilesClause(omittedFiles)); + } // A summary degradation affects prose, not findings: the findings are complete, but the // summary call either had its response cut at the length cap and was salvaged (or replaced by // the counts-only fallback), or was skipped outright at the token spend ceiling (#518) — the @@ -580,6 +624,12 @@ public String coverageGapBrief() { "%d file(s) partially reviewed (response cut at the length cap)", truncation.responseCutFileNames().size())); } + if (!truncation.callFailedFileNames().isEmpty()) { + parts.add( + String.format( + "%d file(s) not reviewed (review call did not complete)", + truncation.callFailedFileNames().size())); + } switch (truncation.summaryDegradation()) { case RESPONSE_CUT -> parts.add("summary shortened (response cut at the length cap)"); case SKIPPED_AT_CEILING -> parts.add("summary skipped (token spend ceiling reached)"); diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/VerdictBuilder.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/VerdictBuilder.java index 3d7ad2a0..04ee64c4 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/VerdictBuilder.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/VerdictBuilder.java @@ -124,16 +124,22 @@ ReviewResult build( // whose batch response was cut but salvaged (#500) are a further class: partially reviewed, // holding approval like the others, disclosed with the response cut as the reason — and a file // both clipped and response-cut is disclosed once, under the stronger (output-side) statement. + // Files a failed review call left uncovered are pulled out the same way (#655): they gate + // approval through the omitted set, but the disclosure says the call did not complete rather + // than blaming the diff budget. var ceilingSkipped = plan.spendCeilingSkippedFiles(); + var callFailed = withoutNames(plan.runtimeUncoveredFiles(), ceilingSkipped); var responseCut = plan.responseCutFiles(); var clipped = withoutNames(plan.effectiveClippedFiles(), responseCut); var truncation = plan.budgeted() ? new ReviewResult.TruncationDetail( - withoutNames(plan.effectiveOmittedFiles(), ceilingSkipped), + withoutNames( + withoutNames(plan.effectiveOmittedFiles(), ceilingSkipped), callFailed), clipped, ceilingSkipped, responseCut, + callFailed, plan.summaryDegradation()) : ReviewResult.TruncationDetail.EMPTY; var omitted = @@ -152,6 +158,7 @@ ReviewResult build( // clipped and response-cut files keep theirs: those were partially reviewed. var omittedNames = new HashSet<>(truncation.omittedFileNames()); omittedNames.addAll(truncation.spendCeilingSkippedFileNames()); + omittedNames.addAll(truncation.callFailedFileNames()); var changedFiles = toChangedFiles( overviewFiles.stream() diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewResultTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewResultTest.java index c30dff92..84948df8 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewResultTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewResultTest.java @@ -331,12 +331,173 @@ void coverageGapBriefCountsSpendCeilingSkipsAsTheirOwnClass() { assertTrue(brief.contains("1 file(s) skipped at the token spend ceiling"), brief); } + @Test + void coverageGapClauseNamesTheCallFailureSeparatelyFromTheBudgetOmissions() { + // #655: files whose review call failed all its retries fit the diff budget fine, so the + // budget wording — and its implied remedy, raising the input budget — is wrong for them. The + // clause must say the call did not complete, matching the summary overview's per-file note. + var detail = + new ReviewResult.TruncationDetail( + List.of("a.java"), + List.of(), + List.of(), + List.of(), + List.of("failed.java"), + SummaryDegradation.NONE); + + var clause = ReviewResult.coverageGapClause(2, detail); + + assertTrue(clause.contains("1 file(s) were omitted entirely (a.java)"), clause); + assertTrue( + clause.contains( + "1 file(s) were not reviewed because the review call for them did not complete" + + " (failed.java)"), + clause); + } + + @Test + void coverageGapClauseWithOnlyCallFailuresDropsTheBudgetWording() { + var detail = + new ReviewResult.TruncationDetail( + List.of(), + List.of(), + List.of(), + List.of(), + List.of("failed.java"), + SummaryDegradation.NONE); + + var clause = ReviewResult.coverageGapClause(1, detail); + + assertFalse(clause.contains("review budget"), clause); + assertTrue(clause.contains("the review call for them did not complete"), clause); + } + + @Test + void coverageGapBriefCountsCallFailuresAsTheirOwnClass() { + var result = + new ReviewResult( + List.of(), + 0, + 0, + 0, + 0, + null, + ReviewState.COMMENT, + true, + "", + List.of(), + List.of(), + 1, + false, + true, + new ReviewResult.TruncationDetail( + List.of(), + List.of(), + List.of(), + List.of(), + List.of("failed.java"), + SummaryDegradation.NONE)); + + var brief = result.coverageGapBrief(); + + assertTrue(brief.contains("1 file(s) not reviewed (review call did not complete)"), brief); + } + + @Test + void truncationDetailWithOnlyCallFailuresIsNotEmpty() { + var detail = + new ReviewResult.TruncationDetail( + List.of(), + List.of(), + List.of(), + List.of(), + List.of("failed.java"), + SummaryDegradation.NONE); + assertFalse(detail.isEmpty()); + assertTrue(detail.hasFileGaps()); + // The pre-#655 convenience constructor carries no call failures. + assertEquals( + List.of(), + new ReviewResult.TruncationDetail( + List.of("a.java"), List.of(), List.of(), List.of(), SummaryDegradation.NONE) + .callFailedFileNames()); + } + + @Test + void coverageGapClauseKeepsTheLegacyCountAlongsideASummaryCut() { + // #659 probe B: on the legacy path (count known, no names) a detail carrying only a summary + // degradation skipped the numeric fallback, and nothing below it read the int — the reader + // was told the summary was shortened and never that files went unreviewed. + var detail = + new ReviewResult.TruncationDetail( + List.of(), List.of(), List.of(), List.of(), SummaryDegradation.RESPONSE_CUT); + + var clause = ReviewResult.coverageGapClause(3, detail); + + assertTrue( + clause.contains("3 file(s) were omitted because the diff exceeded the size budget"), + clause); + assertTrue(clause.contains("the summary was shortened"), clause); + } + + @Test + void coverageGapClauseKeepsTheLegacyCountAlongsideACeilingSkippedSummary() { + // #659 probe C: same drop with the ceiling flavor of the summary degradation. + var detail = + new ReviewResult.TruncationDetail( + List.of(), List.of(), List.of(), List.of(), SummaryDegradation.SKIPPED_AT_CEILING); + + var clause = ReviewResult.coverageGapClause(3, detail); + + assertTrue( + clause.contains("3 file(s) were omitted because the diff exceeded the size budget"), + clause); + assertTrue(clause.contains("the summary was skipped"), clause); + } + + @Test + void coverageGapClauseWithAZeroCountAndOnlyASummaryCutSkipsTheLegacyClause() { + // Summary-only degradation with nothing omitted: no count to disclose, so the clause is the + // degradation alone. + var detail = + new ReviewResult.TruncationDetail( + List.of(), List.of(), List.of(), List.of(), SummaryDegradation.RESPONSE_CUT); + + var clause = ReviewResult.coverageGapClause(0, detail); + + assertFalse(clause.contains("size budget"), clause); + assertTrue(clause.contains("the summary was shortened"), clause); + } + + @Test + void coverageGapClauseWithAnEmptyDetailStillRendersTheLegacyCount() { + // #659 probe A: the empty-detail fallback is unchanged. + var clause = ReviewResult.coverageGapClause(3, ReviewResult.TruncationDetail.EMPTY); + + assertEquals("3 file(s) were omitted because the diff exceeded the size budget", clause); + } + + @Test + void coverageGapClauseDoesNotAddTheLegacyCountWhenFileGapsAreNamed() { + // With names known the count is already accounted for per class — adding the numeric clause + // would double-report the same files. + var detail = + new ReviewResult.TruncationDetail( + List.of("a.java"), List.of(), List.of(), List.of(), SummaryDegradation.RESPONSE_CUT); + + var clause = ReviewResult.coverageGapClause(1, detail); + + assertTrue(clause.contains("omitted entirely (a.java)"), clause); + assertFalse(clause.contains("size budget"), clause); + } + @Test void truncationDetailNormalizesNullListsToEmpty() { - var detail = new ReviewResult.TruncationDetail(null, null, null, null, null); + var detail = new ReviewResult.TruncationDetail(null, null, null, null, null, null); assertTrue(detail.isEmpty()); assertEquals(List.of(), detail.spendCeilingSkippedFileNames()); assertEquals(List.of(), detail.responseCutFileNames()); + assertEquals(List.of(), detail.callFailedFileNames()); assertEquals( SummaryDegradation.NONE, detail.summaryDegradation(), diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/VerdictBuilderTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/VerdictBuilderTest.java index 77c426a3..a8b0657a 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/VerdictBuilderTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/VerdictBuilderTest.java @@ -166,10 +166,11 @@ void budgetedReviewWithFullCoverageIsNotTruncated() { } @Test - void aFailedBatchsRuntimeUncoveredFilesHoldApprovalAndAreDisclosedAsOmitted() { - // Lead#3: a batch that failed all its retries records its files on the shared plan. The verdict - // reads that same instance and must fold them into the omitted set — holding APPROVE and naming - // them — exactly like a planned omission, so a partial review never claims full coverage. + void aFailedBatchsRuntimeUncoveredFilesHoldApprovalAndAreDisclosedAsCallFailures() { + // Lead#3 + #655: a batch that failed all its retries records its files on the shared plan. The + // verdict reads that same instance and must gate approval on them exactly like a planned + // omission — but the disclosure must say the review call did not complete, not blame the diff + // budget, so the banner agrees with the summary overview's per-file note. var ctx = contextWithLineCapOmissions(0); var plan = new DiffBudgetPlanner.BudgetPlan( @@ -181,15 +182,26 @@ void aFailedBatchsRuntimeUncoveredFilesHoldApprovalAndAreDisclosedAsOmitted() { assertEquals(1, result.omittedFiles()); assertTrue(result.truncated()); assertEquals(ReviewState.COMMENT, result.reviewState()); + assertEquals(List.of(), result.truncation().omittedFileNames()); + assertEquals(List.of("failed.java"), result.truncation().callFailedFileNames()); assertTrue( - result.summaryMarkdown().contains("omitted entirely (failed.java)"), + result + .summaryMarkdown() + .contains( + "1 file(s) were not reviewed because the review call for them did not complete" + + " (failed.java)"), result.summaryMarkdown()); + assertFalse(result.summaryMarkdown().contains("review budget"), result.summaryMarkdown()); + var checkSummary = VerdictBuilder.checkSummaryForResult(result); + assertTrue( + checkSummary.contains("1 file(s) not reviewed (review call did not complete)"), + checkSummary); } @Test - void aRuntimeUncoveredFileThatWasAlsoClippedIsCountedOnceAsOmitted() { - // No double-count: a clipped file whose batch then failed is reported as omitted, not also as - // partially analyzed. + void aRuntimeUncoveredFileThatWasAlsoClippedIsCountedOnceAsACallFailure() { + // No double-count: a clipped file whose batch then failed is reported as not reviewed, not + // also as partially analyzed. var ctx = contextWithLineCapOmissions(0); var plan = new DiffBudgetPlanner.BudgetPlan( @@ -200,10 +212,54 @@ void aRuntimeUncoveredFileThatWasAlsoClippedIsCountedOnceAsOmitted() { assertEquals(1, result.omittedFiles()); assertTrue( - result.summaryMarkdown().contains("omitted entirely (f.java)"), result.summaryMarkdown()); + result + .summaryMarkdown() + .contains("not reviewed because the review call for them did not complete (f.java)"), + result.summaryMarkdown()); assertFalse(result.summaryMarkdown().contains("partially analyzed"), result.summaryMarkdown()); } + @Test + void aPlannedOmissionAndACallFailureAreDisclosedUnderTheirOwnReasons() { + // #655: the two classes coexist in one review — the planned omission keeps the budget wording, + // the failed call gets its own clause, and neither file is listed twice. + var ctx = contextWithLineCapOmissions(0); + var plan = + new DiffBudgetPlanner.BudgetPlan( + List.of(), List.of("big.java"), List.of(), true, null, null, null, null); + plan.recordUncoveredFiles(List.of("failed.java")); + + var result = builder.build(ctx, CLEAN_RESPONSE, CI_CLEAR, plan); + + assertEquals(2, result.omittedFiles()); + assertEquals(List.of("big.java"), result.truncation().omittedFileNames()); + assertEquals(List.of("failed.java"), result.truncation().callFailedFileNames()); + var summary = result.summaryMarkdown(); + assertTrue(summary.contains("omitted entirely (big.java)"), summary); + assertTrue( + summary.contains( + "1 file(s) were not reviewed because the review call for them did not complete" + + " (failed.java)"), + summary); + } + + @Test + void aSpendCeilingSkipIsNotAlsoDisclosedAsACallFailure() { + // Ceiling skips flow through recordUncoveredFiles too, but their cause is a deliberate stop — + // the call-failed clause must not claim them. + var ctx = contextWithLineCapOmissions(0); + var plan = + new DiffBudgetPlanner.BudgetPlan( + List.of(), List.of(), List.of(), true, null, null, null, null); + plan.recordSpendCeilingSkippedFiles(List.of("skipped.java")); + + var result = builder.build(ctx, CLEAN_RESPONSE, CI_CLEAR, plan); + + assertEquals(List.of("skipped.java"), result.truncation().spendCeilingSkippedFileNames()); + assertEquals(List.of(), result.truncation().callFailedFileNames()); + assertFalse(result.summaryMarkdown().contains("did not complete"), result.summaryMarkdown()); + } + @Test void spendCeilingSkippedFilesAreDisclosedWithTheCeilingAsTheReason() { // #499: a batch skipped at the token spend ceiling withholds coverage like any runtime gap —