fix(review): failed-call coverage clause and legacy omitted count alongside summary degradations

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

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<String> clippedFileNames,
       List<String> spendCeilingSkippedFileNames,
       List<String> responseCutFileNames,
+      List<String> 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<String> omittedFileNames,
+        List<String> clippedFileNames,
+        List<String> spendCeilingSkippedFileNames,
+        List<String> 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)");

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()

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(),