fix(review): degrade a ceiling-refused summary on the degenerate lane - #530
Merged
Merged
Conversation
summarizeWithoutReview — the degenerate budgeted lane whose only AI call is the summary — caught only AiResponseTruncatedException, so a TokenSpendCeilingExceededException thrown by a mid-retry gate crossing escaped the pipeline and failed the whole review, and the skip was never recorded on the plan for disclosure. Catch the typed refusal at the same seam and degrade to countsOnlySummary, exactly like the multi-call lane: the review still posts with its omission disclosures, and summarySkippedAtCeiling is recorded so the posted review names the ceiling.
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Contributor
🤖 ThrillhouseBot PR SummaryWhat this PR doesOn the degenerate budgeted lane (every file over budget, zero batches), the summary is the review's only AI call, and a TokenSpendCeilingExceededException raised at that seam previously escaped run() and failed the entire review. This change adds a catch for that typed exception in summarizeWithoutReview that degrades to countsOnlySummary (recording the ceiling skip on the plan), so the review still posts counts-only with its omission disclosures; a new unit test verifies the degradation, persistence, and plan recording. Control-Flow Diagram🔀 Show diagramflowchart TD
A["run on the degenerate all-omitted plan"] --> B["summarizeWithoutReview calls summarize"]
B --> C{"exception thrown by summarize?"}
C -->|"TokenSpendCeilingExceededException"| D["new catch degrades to countsOnlySummary"]
C -->|"AiResponseTruncatedException"| E["existing truncation degrade"]
C -->|"success"| F["normal summary path"]
D --> G["ceiling skip recorded on plan"]
G --> H["review still posts counts-only with disclosures"]
Changes Overview
Changed Files
Risk Assessment
No new issues found in this PR, but the review cannot be approved until CI is confirmed green.
|
| Check | Type | Status | Detail |
|---|---|---|---|
| frontend | check-run | ⏳ Pending | - |
| trivy | check-run | ⏳ Pending | - |
| test | check-run | ⏳ Pending | - |
| format | check-run | ⏳ Pending | - |
| actionlint | check-run | ⏳ Pending | - |
| changes | check-run | ⏳ Pending | - |
| dependency-review | check-run | ⏳ Pending | - |
Automated review by ThrillhouseBot. Reply with /review to re-run.
thrillhousebot
Bot
added
bug
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
This was referenced Aug 11, 2026
devops-thiago
added a commit
that referenced
this pull request
Aug 13, 2026
## What type of PR is this? - [x] 📝 Documentation - [x] 🏗️ CI/CD ## Description Prepares the 0.6.0 release: bumps the project version from `0.5.1-SNAPSHOT` to `0.6.0` and turns the `[Unreleased]` changelog section into `[0.6.0]`. The changelog is rewritten so it describes what changes for someone upgrading from 0.5.0. Entries recording fixes to work done inside this cycle are dropped, because the defects they name never shipped — a reader upgrading from 0.5.0 never saw them. Twenty-two issue references go: the truncation-salvager follow-ups (#582, #592, #617), the spend-ceiling disclosure fixes (#521, #524, #530), the write-pacing follow-ups (#541, #542, #577, #598), the verifier hedging refinements (#594, #610), and others of the same kind. What stays is the features 0.6.0 adds, the behaviour it changes, and the defects that were present in 0.5.0. Membership in that last group was decided against the `v0.5.0` tag rather than from when an issue was filed, since every issue in this cycle was filed after 0.5.0 shipped: | check against `v0.5.0` | result | consequence | |---|---|---| | `Dockerfile.runtime` present | yes | image fixes (#561, #564) shipped, kept | | streaming in `AiReviewService` | yes | empty SSE frame (#555) shipped, kept | | `List.copyOf` in `DiffBudgetPlanner` | 4 uses | null-filename crash (#472, #551) shipped, kept | | retry loop in `AiReviewService` | yes | length-cap retry (#495, #504) shipped, kept | | `FindingVerifierPrompts` present | yes | verifier precision (#611–#614) shipped, kept | | `TruncatedResponseSalvager` | absent | its follow-ups are cycle-internal, dropped | | `RebuttalContradiction`, `ConfigKeyContextResolver`, `PatchCoverage`, `GitHubWriteRetry` | absent | same, dropped | The prose follows the guidance on [signs of AI writing](https://en.wikipedia.org/wiki/Wikipedia:Signs_of_AI_writing): plain sentences, no promotional framing, and the em-dash and "X instead of Y" habits cut back to where they carry meaning. Also corrects the `max-diff-lines` comment in `application.properties`, which still listed `/describe`, `/changelog` and `/add-docs` as capped by it after this cycle moved those commands onto token-budgeted batches. The README table already described the new behaviour, so the two disagreed and the properties file was the wrong one. Reported by the bot on #532. ## Also in this PR Three gaps found after the first commit, each a convention the 0.5.0 cut followed and this one had missed. **Dependency bumps.** 0.4.0 and 0.5.0 both carry a `### Dependencies` section and 0.6.0 had none. Added: the Quarkus platform 3.37.4 to 3.38.0 and `quarkus-langchain4j` 1.12.0 to 1.12.2; `jackson-dataformat-yaml` declared explicitly now that reading a repository's own `.github/thrillhousebot.yml` uses it directly; frontend `next` to 16.2.12 and dev-only `jsdom` to 30.0.1; `github/codeql-action` to v4.37.6, `actions/setup-java` to v5.7.0, `docker/login-action` to v4.6.0; Spotless to 3.9.0. A fresh empty `[Unreleased]` is left above the release section, as the 0.5.0 cut did. **Architecture documentation.** `docs/ARCHITECTURE.md` was last touched on 2026-08-08 and eight of this cycle's capabilities appeared nowhere in it — verified at zero occurrences each before the change: the per-review spend ceiling, the `concise` model binding, patch coverage as review context, repository-supplied ignore globs, path-scoped instructions, the decline re-check, write pacing, and the coverage-honesty rules that decide when a file withholds APPROVE. The Packages table also predated the classes carrying them. Both updated, at the subsystem level the document is written at. README needed nothing: all 52 environment variables and all 12 commands are already documented there, and the website's Configuration and Commands pages are include-wrappers over README sections, so they track it automatically. **Docs freeze.** `versions.json` listed `current: v0.5.0` with archives stopping at 0.4.0, so 0.5.0 shipped without being frozen — the same lapse the 0.5.0 cut found for 0.4.0. Because `archive-docs-version.mjs` expands the include markers at archive time, archiving the working tree would have produced a directory labelled v0.5.0 documenting `/improve` and the spend ceiling. The doc sources were restored to the `v0.5.0` tag, archived, then reverted. Verified both directions: | check | archived 0.5.0 pages | live tree | |---|---|---| | `REVIEW_MAX_TOKENS_PER_REVIEW` | 0 | 1 | | `/improve` | 0 | present | | `REVIEW_CI_GATING` | 1 | — | Nine pages with matching assets and `src/content/versions/0.5.0.json`; `current.label` moves to v0.6.0. Unlike the 0.4.0 archive, the tag restore resurrected no deleted files. The site builds: 75 pages, all internal links valid. ## Related Issues N/A — release mechanics. ## How Has This Been Tested? - [x] Unit tests `./mvnw -B clean test` → `Tests run: 3067, Failures: 0, Errors: 0, Skipped: 0`, building `thrillhousebot 0.6.0`. `./mvnw -B clean compile spotbugs:check spotless:check` → `BugInstance size is 0`. No production code changes, so there is nothing to cover: the diff is the version string, the changelog, the architecture document, one properties comment and the frozen 0.5.0 docs. The docs site was built as well (`npm ci && npm run build` in `website/`): 75 pages, all internal links valid. ## Checklist - [x] My code follows the project's coding standards - [x] I have performed a self-review of my own code
10 tasks
devops-thiago
added a commit
that referenced
this pull request
Aug 13, 2026
…ngside summary degradations (#691) ## What type of PR is this? - [x] 🐛 Bug fix ## Description Two defects in `ReviewResult.coverageGapClause`, both about the coverage disclosure telling the reader the wrong thing (or nothing) about unreviewed files: - **#655 — wrong reason for failed review calls.** Files a failed batch left unreviewed flowed `recordUncoveredFiles → runtimeUncoveredFiles → effectiveOmittedFiles → TruncationDetail.omittedFileNames`, so the banner said they were withheld "because the diff exceeded the review budget" while the summary overview said "the review call for it did not complete" — contradictory reasons, and the banner's implied remedy (raise the input budget) cannot help. Runtime call failures are now their own `TruncationDetail` class (`callFailedFileNames`), carried explicitly the same way the spend-ceiling (#524/#530) and response-cut (#523) classes are, and disclosed as "not reviewed because the review call for them did not complete" in the coverage clause and the check-run brief. `VerdictBuilder` subtracts ceiling skips before classifying, so a deliberate ceiling stop is never relabelled a failure, and approval gating / omitted counts are unchanged. - **#659 — legacy count dropped next to a summary degradation.** On the legacy line-cap path the numeric fallback was an early return guarded by `detail.isEmpty()`; a detail carrying only a summary degradation skipped it, and nothing below read the `omittedFiles` int, so the count vanished (issue probes B/C). The numeric clause now joins the per-class clauses when no names are known and `omittedFiles > 0`, so it renders alongside the summary-degradation clause. Probe A (empty detail) is unchanged. The pre-existing `TruncationDetail` shape is kept via a convenience constructor, leaving existing call sites (including #628's future patchless-reason work) untouched. #628's patchless route is deliberately out of scope here. ## Related Issues Closes #655 Closes #659 ## How Has This Been Tested? - [x] Unit tests - [ ] Integration tests - [ ] Manual testing New tests cover the failed-call class end-to-end from `BudgetPlan.recordUncoveredFiles` through `VerdictBuilder` to the rendered banner and check-run brief (including no-double-listing with ceiling skips and clipped files), plus the issue's A/B/C probes for the legacy count. `./mvnw verify` passes locally (3133 tests) and every changed production line is covered against the merge-base diff. ## 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 - [x] I have updated the documentation accordingly - [x] My changes generate no new warnings or errors ## Screenshots / Logs N/A ## Additional Notes CHANGELOG `[Unreleased]` carries Fixed entries for both issues.
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
On the degenerate budgeted lane (every reviewable file over budget, zero batches),
FindingPipeline.summarizeWithoutReviewmakes the review's only AI call — the summary.AiReviewService.runWithRetriesruns the token-spend gate before every attempt and deliberately lets the typedTokenSpendCeilingExceededExceptionpropagate; the multi-call lane catches exactly that at its summarize seam and degrades tocountsOnlySummary, butsummarizeWithoutReviewcaught onlyAiResponseTruncatedException. A mid-retry ceiling refusal (attempt 1 bills usage that reachesREVIEW_MAX_TOKENS_PER_REVIEWand then fails transiently; attempt 2's gate throws) therefore escapedrun, landed inReviewOrchestrator.handleReviewFailure, and the whole review was lost instead of posting counts-only with its omission disclosures — defeating this lane's own stated contract.plan.summarySkippedAtCeilingwas never recorded either, so no disclosure would have been rendered even if the review had survived.Fix: catch
TokenSpendCeilingExceededExceptionat the same seam and degrade tocountsOnlySummary(...)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 namesREVIEW_MAX_TOKENS_PER_REVIEW.Related Issues
Fixes #526
How Has This Been Tested?
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, recordssummaryWasSkippedAtCeiling()on the plan, and leavessummaryWasCut()false (disjoint classes).Red on unfixed code (the refusal escapes
pipeline.runand the skip is never recorded):Green with the fix. Gates:
spotless:applyclean;clean compile spotbugs:check spotless:check— BugInstance size is 0; fullclean test— 2595 tests, 0 failures. Jacoco on the changed lines: 0 missed instructions, 0 missed branches.Checklist
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.