fix(review): failed-call coverage clause and legacy omitted count alongside summary degradations - #691
Conversation
…eep 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.
🤖 ThrillhouseBot PR SummaryWhat this PR doesAdds a dedicated 'review call did not complete' disclosure class for files a failed batch left unreviewed, so the coverage banner and check-run brief no longer blame the diff budget and now agree with the summary overview; it also restores the legacy numeric omitted-files count on the coverage clause when a detail carries only a summary degradation, fixing the #659 probes B/C while leaving probe A unchanged. Description vs. ImplementationNo mismatch found between the PR description and the change. Control-Flow Diagram🔀 Show diagramflowchart TD
A["Failed review batch: plan.recordUncoveredFiles(...)"] --> B["BudgetPlan merges runtime-uncovered into effectiveOmittedFiles"]
B --> C["VerdictBuilder: callFailed = withoutNames(runtimeUncoveredFiles, ceilingSkipped)"]
C --> D["TruncationDetail carries callFailedFileNames; omittedFileNames excludes them"]
D --> E["coverageGapClause builds per-class clauses"]
E --> F{"detail.hasFileGaps()?"}
F -- "no" --> G["legacy count clause when omittedFiles > 0"]
F -- "yes" --> H["per-class clauses render, no numeric clause"]
D --> I["summary-degradation clause renders"]
G --> J["banner and check-run brief disclose call-failed files, not budget"]
H --> J
I --> J
Changes Overview
Changed Files
Risk Assessment
Things to double-check1 lower-confidence finding
|
| Check | Type | Status | Detail |
|---|---|---|---|
| format | missing | ⏳ Pending | - |
| test | missing | ⏳ Pending | - |
| frontend | missing | ⏳ Pending | - |
| trivy | missing | ⏳ Pending | - |
| dependency-review | missing | ⏳ Pending | - |
Automated review by ThrillhouseBot. Reply with /review to re-run.
![thrillhousebot[bot]](https://avatars.githubusercontent.com/in/3996985?s=60&v=4){kind=small}
There was a problem hiding this comment.
ThrillhouseBot noted 1 lower-confidence item(s) under Things to double-check in the PR summary (not posted as inline threads):
- MEDIUM: Legacy omitted count still dropped when a per-file class coexists (e.g. call failure) (
src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewResult.java:564)
The new #659 clause is gated onif (!detail.hasFileGaps() && omittedFiles > 0), andhasFileGaps()is true whenever ANY of the five name classes is non-empty — including the newcallFailedFileNamesclass this same PR adds. On the legacy line-cap path the #659 issue's own reachability analysis states the precondition is "omittedFiles > 0 with no names": the count is known while the planned omissions carry no names. If such a review also has a failed batch — producer side is visible in VerdictBuilder:var callFailed = withoutNames(plan.runtimeUncoveredFiles(), ceilingSkipped);followed bycallFailedFileNames: callFailed— then the count clause is suppressed (hasFileGaps true) while the named clauses cover only the failed file, and the unnamed line-cap omissions are again disclosed to nobody. That is the exact loss #659 was filed for, now arising from the combination of the PR's two fixes. The PR's tests never exercise this combination:coverageGapClauseDoesNotAddTheLegacyCountWhenFileGapsAreNamedassumes count == sum of named classes, and the detail unit tests for #659 use a detail with no name classes at all. Verify whetheromittedFilescan exceed the sum of the named classes on the legacy path — check BudgetPlan.effectiveOmittedFiles and theomittedcount computation in VerdictBuilder.build (just below the truncation construction, not shown in the diff). If it can, gate the numeric clause on the count exceeding the named-class sum rather than on!detail.hasFileGaps(), and add a test combining the legacy count with a call failure plus a summary degradation.
thrillhousebot
Bot
added
bug
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
There was a problem hiding this comment.
ThrillhouseBot found no issues in this PR, but some checks are still pending or failed:
- Check format is pending
- Check test is pending
- Check frontend is pending
- Check trivy is pending
- Check dependency-review is pending
Additionally, No new issues in this revision, but 1 previous finding(s) remain unresolved — fix them, or reply on their review thread with why they are deferred. A finding listed only under "Things to double-check" has no thread: clear it by commenting @thrillhousebot resolved path/to/File.java:42 — <the finding's title> on this PR.
|
@thrillhousebot resolved src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewResult.java:564 — Legacy omitted count still dropped when a per-file class coexists (e.g. call failure) The combination cannot be produced:
Gating on "count exceeds the named total" would guard a state no producer can construct. 🤖 Addressed by Claude Code |
|
The next review will close every previous finding this comment names by its |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
1 participant
What type of PR is this?
Description
Two defects in
ReviewResult.coverageGapClause, both about the coverage disclosure telling the reader the wrong thing (or nothing) about unreviewed files: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 ownTruncationDetailclass (callFailedFileNames), carried explicitly the same way the spend-ceiling (fix(review): disclose a ceiling-skipped summary in the posted review #524/fix(review): degrade a ceiling-refused summary on the degenerate lane #530) and response-cut (fix(review): keep partial-coverage framing out of a summary-only-cut delta comment #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.VerdictBuildersubtracts ceiling skips before classifying, so a deliberate ceiling stop is never relabelled a failure, and approval gating / omitted counts are unchanged.detail.isEmpty(); a detail carrying only a summary degradation skipped it, and nothing below read theomittedFilesint, so the count vanished (issue probes B/C). The numeric clause now joins the per-class clauses when no names are known andomittedFiles > 0, so it renders alongside the summary-degradation clause. Probe A (empty detail) is unchanged.The pre-existing
TruncationDetailshape 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?
New tests cover the failed-call class end-to-end from
BudgetPlan.recordUncoveredFilesthroughVerdictBuilderto 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 verifypasses locally (3133 tests) and every changed production line is covered against the merge-base diff.Checklist
Screenshots / Logs
N/A
Additional Notes
CHANGELOG
[Unreleased]carries Fixed entries for both issues.