fix(review): disclose a ceiling-skipped summary in the posted review - #524
Merged
Merged
Conversation
🤖 ThrillhouseBot PR SummaryWhat this PR doesWhen the token spend ceiling trips after all batches succeed — either at the pre-summary gate or when the summary call is refused mid-call — FindingPipeline.countsOnlySummary now records a summarySkippedAtCeiling flag on the BudgetPlan. VerdictBuilder threads it into TruncationDetail, and the rendered review gains a dedicated SUMMARY_SKIPPED_NOTICE banner (when no file gaps exist), a coverageGapClause-clause and coverageGapBrief-part naming REVIEW_MAX_TOKENS_PER_REVIEW, and a check-run suffix — without holding approval because the findings are complete. The change also corrects the two over-claiming comments that previously asserted the spend-ceiling degradation already disclosed itself. Control-Flow Diagram🔀 Show diagramflowchart TD
A["Batch calls succeed"] --> B{"Ceiling reached at summary gate?"}
B -- "yes" --> C["countsOnlySummary records summarySkippedAtCeiling"]
B -- "no" --> D["summarize() call"]
D -- "TokenSpendCeilingExceededException" --> C
D -- "ok" --> E["Normal summary path"]
C --> F["VerdictBuilder threads flag into TruncationDetail"]
F --> G{"File-coverage gaps present?"}
G -- "no" --> H["SUMMARY_SKIPPED_NOTICE banner + check suffix; no approval hold"]
G -- "yes" --> I["Skip folds into coverageGapClause; partial-review banner"]
Changes Overview
Changed Files
Risk Assessment
Everything's coming up Thrillhouse! 🎉 No issues found in this PR. Automated review by ThrillhouseBot. Reply with |
![thrillhousebot[bot]](https://avatars.githubusercontent.com/in/3996985?s=60&v=4){kind=small}
thrillhousebot
Bot
added
bug
devops-thiago
force-pushed
the
fix/518-ceiling-summary-disclosure
branch
from
August 11, 2026 04:55
414715c to
5d56cf4
Compare
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
8 tasks
devops-thiago
added a commit
that referenced
this pull request
Aug 11, 2026
…#530) ## What type of PR is this? - [x] 🐛 Bug fix ## Description On the degenerate budgeted lane (every reviewable file over budget, zero batches), `FindingPipeline.summarizeWithoutReview` makes the review's only AI call — the summary. `AiReviewService.runWithRetries` runs the token-spend gate before every attempt and deliberately lets the typed `TokenSpendCeilingExceededException` propagate; the multi-call lane catches exactly that at its summarize seam and degrades to `countsOnlySummary`, but `summarizeWithoutReview` caught only `AiResponseTruncatedException`. A mid-retry ceiling refusal (attempt 1 bills usage that reaches `REVIEW_MAX_TOKENS_PER_REVIEW` and then fails transiently; attempt 2's gate throws) therefore escaped `run`, landed in `ReviewOrchestrator.handleReviewFailure`, and the whole review was lost instead of posting counts-only with its omission disclosures — defeating this lane's own stated contract. `plan.summarySkippedAtCeiling` was never recorded either, so no disclosure would have been rendered even if the review had survived. Fix: catch `TokenSpendCeilingExceededException` at the same seam and degrade to `countsOnlySummary(...)` exactly like the multi-call lane — the review still posts with its omission disclosures, and the ceiling skip is recorded on the plan so the posted review names `REVIEW_MAX_TOKENS_PER_REVIEW`. ## Related Issues Fixes #526 ## How Has This Been Tested? - [x] Unit tests New test `FindingPipelineTest.summarizeWithoutReviewDegradesToCountsOnlyWhenTheCeilingRefusesTheSummary`: degenerate all-omitted plan, summarize stub throwing the typed refusal; asserts the review no longer throws, returns the counts-only shape (empty findings, null summary), persists the response, records `summaryWasSkippedAtCeiling()` on the plan, and leaves `summaryWasCut()` false (disjoint classes). Red on unfixed code (the refusal escapes `pipeline.run` and the skip is never recorded): ``` [ERROR] dev.thiagogonzaga.thrillhousebot.review.FindingPipelineTest.summarizeWithoutReviewDegradesToCountsOnlyWhenTheCeilingRefusesTheSummary -- Time elapsed: 2.596 s <<< ERROR! dev.thiagogonzaga.thrillhousebot.review.ai.TokenSpendCeilingExceededException: AI call skipped: this review has consumed 120000 tokens, at or above its 100000-token spend ceiling (REVIEW_MAX_TOKENS_PER_REVIEW / thrillhousebot.review.max-tokens-per-review) at dev.thiagogonzaga.thrillhousebot.review.ai.AiReviewService.summarize(AiReviewService.java:103) at dev.thiagogonzaga.thrillhousebot.review.FindingPipeline.summarizeWithoutReview(FindingPipeline.java:625) at dev.thiagogonzaga.thrillhousebot.review.FindingPipeline.runWithLedger(FindingPipeline.java:144) at dev.thiagogonzaga.thrillhousebot.review.FindingPipeline.run(FindingPipeline.java:128) ``` Green with the fix. Gates: `spotless:apply` clean; `clean compile spotbugs:check spotless:check` — BugInstance size is 0; full `clean test` — 2595 tests, 0 failures. Jacoco on the changed lines: 0 missed instructions, 0 missed branches. ## Checklist - [x] My code follows the project's coding standards - [x] I have performed a self-review of my own code - [x] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [ ] I have updated the documentation accordingly - [x] My changes generate no new warnings or errors ## Screenshots / Logs N/A ## Additional Notes The catch mirrors the multi-call lane's `"summary call refused"` degradation verbatim; no rendering changes — the disclosure plumbing shipped in #524 renders the recorded skip.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
Description
When the token spend ceiling trips after every batch succeeded — before the summary call, or refusing the summary call itself — the review degrades to the counts-only summary with a log-only warning:
TruncationDetailstayed empty, so the posted review carried a bare counts-only summary with no stated reason (and could even APPROVE). #513 fixed exactly this asymmetry for the truncation flavor of the same summary lane; this PR gives the ceiling flavor the same seams:BudgetPlangains asummarySkippedAtCeilingflag (recorder + snapshot accessor, mirroringsummaryResponseCut), recorded inFindingPipeline.countsOnlySummary— covering both of its call sites (pre-summary gate skip, and the mid-callTokenSpendCeilingExceededExceptionrefusal).VerdictBuilder.buildthreads it into a matchingTruncationDetail.summarySkippedAtCeilingcomponent.SUMMARY_SKIPPED_NOTICEbanner ("Summary skipped. The review's token spend ceiling (REVIEW_MAX_TOKENS_PER_REVIEW) was reached … the findings themselves are complete."), acoverageGapClauseclause andcoverageGapBriefpart naming the ceiling, and a check-run suffix — each folding into the partial-review clause instead of stacking when real file gaps exist.truncated()is untouched by the flag.summaryResponseCut:hasFileGaps()ignores it, so the delta comment / on-demand disclosure never wrap it in partial-coverage framing.FindingPipeline.salvagedOrCountsOnlySummaryjavadoc and theReviewResult.coverageGapClausecomment now describe both flavors accurately.Related Issues
Fixes #518
How Has This Been Tested?
Red/green proven via two integration tests through the real pipeline→plan→verdict seam (see Logs):
FindingPipelineTest.aCeilingSkippedSummaryIsDisclosedInThePostedReviewNotJustTheLogand…aSummaryCallRefusedAtTheCeilingIsDisclosedInThePostedReviewNotJustTheLog— one percountsOnlySummarycall site. Plus mirrors of #513's unit tests for the new flag: plan recorder/snapshot (DiffBudgetPlannerTest), verdict banner/no-approval-hold and fold-into-clause (VerdictBuilderTest), clause/brief/isEmpty/hasFileGaps and the #516 guard (ReviewResultTest), delta-comment guard (FollowUpDeltaSummaryTest), and disjointness assertions on the pipeline paths. Full suite: 2581 tests, 0 failures.Gates:
spotless:apply→clean compile spotbugs:check spotless:check(BugInstance size 0) →clean testgreen. JaCoCo ∩git diff -U0against the stack parent: zero uncovered lines/branches in changed main code.Checklist
Screenshots / Logs
Red run of both integration tests on unfixed code (stack parent, before this commit):
Both green with the fix applied.
Additional Notes
Third PR of the #515 → #516 → #518 stack: based on
fix/516-delta-disclosure; re-target torelease/v0.6.0after #523 (#516) merges — the diff scoped to this issue is the last commit. Design per audit finding AUDIT3-A F2, following #513's implementation as the pattern.