diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ec25391..639d6a45 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 - **An unclosed quote no longer truncates dispatch evidence inside a later closed literal** (#656): when a quote opener never closes (a Rust lifetime, an apostrophe in prose), the comment scan now resumes past the opener quote-aware instead of cutting at the first bare `//` it swallowed, so a line like `let f = &'a ctx; var s = "//cdn.example.com"; executor.submit(...)` keeps the dispatch after the closed string and a "runs serially" decline is still challenged - **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 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 —