fix(review): count only .xml entries toward the coverage artifact cap and disclose a refusal - #824
Merged
Merged
Conversation
… and disclose a refusal JacocoCoverageReport.walkRefused charged every file entry to MAX_ZIP_ENTRIES (512), not only the .xml entries that can carry a report, and an archive past the cap was refused whole. The usual coverage upload is the whole target/site/jacoco/ tree, one .html per class beside jacoco.xml, so a project of a few hundred classes crossed the cap with a single legitimate report inside and got no coverage section, with a DEBUG line as the only trace (#813). Only .xml entries count toward the cap now. Every other entry is still drained in full against the 128 MB aggregate budget from #789 and the archive is refused the moment it is blown, so the zip-bomb bound is unchanged; the entries left uncounted are bounded by the download ceiling. Both refusals are logged at WARN with counts only, and the reason rides the empty report out as a Refusal, through the resolver's Resolution and the review context, to the summary's review-scope note, which says the configured coverage artifact was not read and why, the way an ignore glob that matched nothing is disclosed (#481). walkRefused's javadoc describes what the code does.
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Contributor
🤖 ThrillhouseBot PR SummaryWhat this PR doesThe coverage artifact walk now charges only .xml-named entries toward the MAX_ZIP_ENTRIES (512) refusal cap, draining all other entries (HTML, CSS, JS, directories) solely against the aggregate inflated-byte budget, so a whole target/site/jacoco/ upload with hundreds of HTML files and one jacoco.xml is read. Refusal reasons (entry cap, inflation budget, unreadable archive) travel as a Refusal on the empty report, are logged at WARN with counts only, and are disclosed to maintainers through PatchCoverageResolver.Resolution, ReviewContext.coverageArtifactRefusal, and the VerdictBuilder review-scope blockquote. Description vs. ImplementationNo mismatch found between the PR description and the change. Control-Flow Diagram🔀 Show diagramflowchart TD
A["fromArtifactZip: walkRefused loops over entries"] --> B{"entry name ends with .xml?"}
B -- "no" --> C["drain entry vs 128 MB aggregate budget"]
C --> D{"aggregate budget blown?"}
D -- "no" --> B
D -- "yes" --> E["log WARN with counts; Refusal.INFLATION_BUDGET"]
B -- "yes (.xml)" --> F{"512 .xml already counted?"}
F -- "no" --> G["readReportInto: parse and merge report"]
G --> B
F -- "yes" --> H["log WARN with counts; Refusal.ENTRY_CAP"]
B -- "end of archive" --> I["merged report or EMPTY, refusal null"]
E --> J["refused(): empty report carrying Refusal"]
H --> J
I --> K["PatchCoverageResolver: section, no note"]
J --> L["PatchCoverageResolver: empty section + refusal reason"]
K --> M["ReviewContext / VerdictBuilder summary & prompt"]
L --> M
Changes Overview
Changed Files
Risk Assessment
No new issues found in this PR, but the review cannot be approved until required CI is confirmed green.
|
| Check | Type | Status | Detail |
|---|---|---|---|
| frontend | check-run | ⏳ Pending | - |
| test | check-run | ⏳ Pending | - |
| trivy | check-run | ⏳ Pending | - |
| format | 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! |
Owner
Author
|
/review |
Contributor
🤖 ThrillhouseBot — changes since the last review
|
![thrillhousebot[bot]](https://avatars.githubusercontent.com/in/3996985?s=60&v=4){kind=small}
…s own empty report The constant's javadoc still called it the value every failure path degrades to, which stopped being true when fromArtifactZip began answering a refusal with a separate empty report carrying the reason. It now says which reads degrade to EMPTY and that isEmpty(), not identity with the constant, is the test for "no coverage".
Contributor
🤖 ThrillhouseBot — changes since the last review
|
Contributor
There was a problem hiding this comment.
ThrillhouseBot found no issues in this PR, but some checks are still pending or failed:
- Check test is pending
- Check format is pending
- Check trivy is pending
- Check frontend is pending
- Check dependency-review is pending
ThrillhouseBot closed 1 previous finding(s) this round:
src/main/java/dev/thiagogonzaga/thrillhousebot/review/JacocoCoverageReport.java:111— EMPTY javadoc says every failure path degrades to EMPTY, but refusal paths now return refused(Refusal)
Owner
Author
|
/review |
Contributor
🤖 ThrillhouseBot — changes since the last review
|
Contributor
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):
- LOW: README claim that a configured artifact never goes quiet contradicts the designed quiet path (
README.md:758)
The README paragraph added by this PR ends: "nothing is, so a configured artifact never goes quiet without saying why." This overstates the actual behavior, which the same PR documents in JacocoCoverageReport.Refusal's javadoc: "An archive that was merely unhelpful — no {@code .xml} entry, nothing that parsed as JaCoCo — is not refused and carries no reason: that is the designed quiet path for a repository publishing nothing usable." Likewise, PatchCoverageResolver.resolve() returns Resolution.NONE (silent) when a configured artifact is never found (download cannot be followed) or parses to nothing usable. The scope-note disclosure fires only for the reader's refusals (ENTRY_CAP / INFLATION_BUDGET / UNREADABLE). A maintainer who configured an artifact whose job never uploads it — or that only ever publishes non-coverage files — would, per the README, expect a scope note that by design never comes. The PR's own description confirms the intended behavior: "An artifact that was never found, or held nothing that parsed, is not a refusal and is not disclosed. That stays the designed quiet path."
…graph The sentence added for the refusal disclosure ended by promising that a configured artifact never goes quiet without saying why. Only an archive the reader refused is disclosed; one that was never uploaded, has expired or holds no JaCoCo XML is still the designed quiet case the paragraph above it describes. The paragraph now says so.
Owner
Author
|
@thrillhousebot resolved README.md:758 — README claim that a configured artifact never goes quiet contradicts the designed quiet path Agreed: only a refused archive is disclosed, and an artifact that was never uploaded, has expired, or holds no JaCoCo XML stays the designed quiet path. Fixed in 4dfdbd4, where the paragraph now ends: "An artifact that was never uploaded, has expired, or holds no JaCoCo XML is still the quiet case above: nothing was refused, so nothing is disclosed." |
Contributor
|
The next review will close every previous finding this comment names by its |
Contributor
🤖 ThrillhouseBot — changes since the last review
|
Contributor
There was a problem hiding this comment.
ThrillhouseBot found no issues in this PR, but some checks are still pending or failed:
- Check trivy is pending
- Check frontend is pending
- Check format is pending
- Check test is pending
- Check dependency-review is pending
ThrillhouseBot closed 1 previous finding(s) this round:
README.md:758— README claim that a configured artifact never goes quiet contradicts the designed quiet path
Owner
Author
|
/review |
10 tasks
…y-cap # Conflicts: # CHANGELOG.md
Contributor
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 trivy is pending
- Check frontend is pending
- Check dependency-review is pending
⚠️ Large PR — partial review. 11 file(s) were only partially reviewed because the model's response was cut at its length cap (max-output-tokens) — findings up to the cut were kept (src/main/java/dev/thiagogonzaga/thrillhousebot/review/JacocoCoverageReport.java, src/test/java/dev/thiagogonzaga/thrillhousebot/review/JacocoCoverageReportTest.java, src/test/java/dev/thiagogonzaga/thrillhousebot/review/VerdictBuilderTest.java, src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewContextLoader.java, src/test/java/dev/thiagogonzaga/thrillhousebot/review/PatchCoverageResolverTest.java, src/main/java/dev/thiagogonzaga/thrillhousebot/review/PatchCoverageResolver.java, src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewContextLoaderTest.java, src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestratorTest.java, src/main/java/dev/thiagogonzaga/thrillhousebot/review/VerdictBuilder.java, README.md, +1 more); the findings and verdict below cover only the reviewed portion.
Owner
Author
|
/review |
devops-thiago
added a commit
that referenced
this pull request
Sep 12, 2026
## What type of PR is this? - [x] 🚀 Performance - [x] 🔧 Refactor - [x] ✅ Test ## Description `ArtifactZipFetcher.transfer` built an `HttpClient` for every coverage-artifact download and closed it in the same try-with-resources: a selector thread created and torn down, a connection pool that never held anything, and a fresh TLS handshake per call, for a client with no per-call state. Verified against `main` at 9ae348d: the builder, the `ProxySelector.getDefault()` read and the `try (var client = builder.build())` were all inside `transfer`, and the only per-request setting, the 30 s timeout, was already on the `HttpRequest`. The issue is right on every point, including that this was not a leak. What changed: - **One client per bean.** The client is a `final` field built in the constructor of the `@ApplicationScoped` bean; `transfer` only builds the request. `HttpClient` is thread-safe and pools connections, so the second download of a deployment reuses the first one's connection. - **No credential, restated on the field.** The field's javadoc says no default header, authenticator or cookie handler may ever be attached, and why: on a long-lived shared client a default `Authorization` header would carry the installation token to every blob host the client is ever pointed at. That is also why following redirects stays safe. - **`ProxySelector.getDefault()` is read once, at construction.** Stated on the field rather than assumed: a proxy configured through JVM system properties at startup is unaffected; a selector swapped at runtime is no longer followed. The null guard stays, since `Builder.proxy(null)` throws. - **`@PreDestroy shutdown()`** releases the selector thread and pooled connections when the application stops, through `shutdownNow` rather than `close` so a download still in flight cannot hold the shutdown open. A download that reaches a shut-down client degrades to no bytes like every other failure. Behaviour otherwise unchanged: https-only through `fetch`, the `MAX_BYTES` reject-not-truncate bound, connect and request timeouts, redirect following, and the fail-soft empty array on every error path. The existing `transfer` tests pass unchanged against the loopback server. Nothing leaks a real client into unit tests: `PatchCoverageResolverTest` and `ReviewOrchestratorTest` use `mock(ArtifactZipFetcher.class)`, and Mockito's inline mock maker never runs the constructor. `ArtifactZipFetcherTest` builds real fetchers and now shuts each one down in `@AfterEach`. ## Related Issues Fixes #478 ## How Has This Been Tested? - [x] Unit tests Red first. The reuse is observed on the wire rather than by object identity: a client that outlives the call keeps its HTTP/1.1 connection pooled, so a second download to the loopback server arrives on the same TCP connection, which a per-call client cannot do. | Test | Verbatim failure before the fix | | --- | --- | | `twoDownloadsShareOneClientAndItsPooledConnection` | `a second download on the same bean must reuse the first one's pooled connection, which only a client that outlives the call can hold ==> expected: <57541> but was: <57542>` (the server saw two different remote ports) | `aDownloadAfterShutdownDegradesInsteadOfPropagating` covers the `@PreDestroy` path (no bytes, no exception, a second shutdown is harmless); it has no red form because there was nothing to shut down before. `worksWhenTheJvmHasNoDefaultProxySelector` now builds the fetcher inside the null-selector window, since the read moved to construction; against a fetcher built at field-init time it would have stopped exercising the guard. Gates on JDK 25: ``` ./mvnw -B clean compile spotbugs:check spotless:check → BugInstance size is 0, BUILD SUCCESS ./mvnw -B clean test → Tests run: 3633, Failures: 0, Errors: 0, Skipped: 0 ``` Every added line and branch of the `src/main` diff is covered, checked against `target/site/jacoco/jacoco.xml`. ## 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 Independent of #824 (branched from `main`). Both add a `CHANGELOG.md` entry under Unreleased, so whichever merges second needs a trivial conflict resolution there.
…y-cap # Conflicts: # CHANGELOG.md # src/main/java/dev/thiagogonzaga/thrillhousebot/review/VerdictBuilder.java # src/test/java/dev/thiagogonzaga/thrillhousebot/review/VerdictBuilderTest.java
Owner
Author
|
/pause |
Contributor
|
⏸️ ThrillhouseBot is now paused on this PR — automatic and manual reviews are silenced. Comment |
|
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
JacocoCoverageReport.walkRefusedcharged every file entry toMAX_ZIP_ENTRIES(512), not only the.xmlentries that can carry a report, and an archive past the cap was refused whole. The usual coverage upload is the wholetarget/site/jacoco/tree — one.htmlper class plus the stylesheet, images and script, withjacoco.xmlbeside them — so a project of a few hundred classes crossed the cap with one legitimate report inside. The result wasEMPTY, no coverage section, and aLog.debugfline nobody runs production at.Verified against
mainat 9ae348d before changing anything:walkRefused:if (seen++ >= MAX_ZIP_ENTRIES)ran for every non-directory entry; the.xmltest sat inreadEntryInto, after the count. Confirmed.fromArtifactZipreturnsEMPTYon refusal by design (fix(review): harden the patch-coverage report reader (zip-bomb bound, cross-module attribution, failed-run, multi-module merge) #789), and that stays: the cap still refuses whole, it just counts the right thing.What changed:
Only
.xmlentries count toward the cap. Every other entry — the HTML report, a stylesheet, a directory name — is still drained in full through the counting copy againstMAX_TOTAL_INFLATED_BYTES, and the archive is refused the moment the budget is blown, so the fix(review): harden the patch-coverage report reader (zip-bomb bound, cross-module attribution, failed-run, multi-module merge) #789 bound is exactly as strong. The directory branch and the file branch collapsed into one "not a report" branch, since a directory name ends in/and can never pass the.xmltest; the reasoning about abomb/entry moved into the method javadoc. The entries left uncounted are bounded byArtifactZipFetcher.MAX_BYTES.Both refusals log at WARN with counts only (cap, entries walked, bytes inflated or
.xmlentries seen), never an entry name. An archive that breaks mid-walk logs at WARN too.A refusal is disclosed in the review-scope note. The reader returns an empty report carrying a
Refusal(ENTRY_CAP,INFLATION_BUDGET,UNREADABLE);PatchCoverageResolver.resolveanswers aResolution(section, artifactRefusal);ReviewContextcarriescoverageArtifactRefusal(new back-compat constructor, the same pattern asunmatchedIgnoreGlobsandcarried);VerdictBuilderrendersPatchCoverageResolver.formatScopeNotein the same blockquote as the unmatched-glob note, between it and the carry-over note:An artifact that was never found, or held nothing that parsed, is not a refusal and is not disclosed. That stays the designed quiet path for a repository that publishes nothing usable.
Still refused, on purpose: a
target/upload with more than 512 surefire XML reports besidejacoco.xml, since every.xmlcosts a slot (the cap bounds how many documents are parsed, not how many turn out to be JaCoCo). The disclosure now says so instead of going quiet.Related Issues
Fixes #813
How Has This Been Tested?
Red first, against the unfixed reader:
readsTheReportOutOfAnHtmlSiteTreeLargerThanTheEntryCap(600.html+ onejacoco.xml)600 .html entries beside one jacoco.xml is the ordinary target/site/jacoco upload and must be read ==> expected: <false> but was: <true>refusesAnArchiveWithMoreXmlEntriesThanTheCapAndSaysSoAtWarn(513.xml)a refusal nobody runs production at DEBUG to see is a coverage section that went quiet for no written reason: [INFO Read patch coverage from artifact entry module0/jacoco.xml, …followed by the 512 INFO/DEBUG lines that were captured and no WARN lineAlso added:
refusesABombWearingAReportName(the aggregate budget refuses from inside an.xmlentry, which was the one uncovered branch of the new walk), theRefusalasserted on every refused shape (entry cap, bomb, truncated download) and its absence on a read archive and on an archive with nothing usable,disclosesAnArtifactTheReaderRefusedInsteadOfGoingQuietandformatsARefusalAsAReviewScopeNoteAndNothingOtherwiseinPatchCoverageResolverTest,aRefusedCoverageArtifactReachesTheContextForTheSummaryToDiscloseinReviewContextLoaderTest, andaRefusedCoverageArtifactIsDisclosedInTheReviewScopeNotepluseveryScopeNoteSharesOneBlockquoteInAFixedOrderinVerdictBuilderTest.The existing bomb tests (
refusesAnArchiveThatInflatesPastTheAggregateCap,refusesABombCarriedUnderADirectoryName,carriesNoPartialAnswerOutOfAnArchiveItRefused) pass with their assertions unchanged; the first additionally asserts the WARN line and the refusal. The three entry-cap tests that padded with.txtnow pad with.xml, because.txtpadding is exactly what stopped being counted.Gates on JDK 25:
Every added line and branch of the
src/maindiff is covered, checked againsttarget/site/jacoco/jacoco.xml.Checklist
Screenshots / Logs
The two WARN lines, with the counts substituted at runtime:
Additional Notes
README.md's patch-coverage section now says thetarget/site/jacoco/upload is fine and that a refused artifact is named in the summary;CHANGELOG.mdhas the entry under Unreleased. Latent in production untilthrillhousebot.review.patch-coverage.enabledis turned on.