fix(review): harden the patch-coverage report reader (zip-bomb bound, cross-module attribution, failed-run, multi-module merge) - #789
Open
devops-thiago wants to merge 16 commits into
Conversation
fromArtifactZip stopped one entry short with readNBytes(MAX_ENTRY_BYTES), but the next getNextEntry() implicitly inflates the whole remainder of an unread entry to reach the following header. A maximally-compressed 16 MB artifact (the ArtifactZipFetcher.MAX_BYTES download ceiling) could drive ~16 GB of decompression per review, and a fork PR can make the base repo's CI emit such an artifact under the configured name for the head SHA under review. The class javadoc falsely claimed the archive's uncompressed size was capped. Every entry is now inflated through a counting copy that charges a running aggregate budget (MAX_TOTAL_INFLATED_BYTES), and the walk aborts the moment the budget is blown. Each entry is fully consumed short of an abort so the next getNextEntry() never has to inflate a remainder. Report entries are collected up to the per-entry ceiling for parsing; anything else is drained and discarded. The javadoc claim is now true. Refs audit ZIP-BOMB
uncoveredLines guarded the N-entries->1-path ambiguity but not its mirror: a single report entry that suffix-matches several repository paths. In a multi-module or multi-variant build the same com/example/Foo.java report path is a whole-segment suffix of every module's copy, and a <package name=""> default-package entry suffix-matches every file of that name, so module-a's uncovered lines were reported against module-b's same-named class. intersectWithAddedLines now resolves the whole reviewableFiles list through a new uncoveredLinesByPath, which drops any attribution ambiguous from either side — a path matched by two entries (the existing guard) or an entry matching two paths (this fix). Only a unique 1:1 report<->path match contributes lines. This must be in place before the F6 merge, which increases the chance of same-named collisions. Refs audit F1
fromArtifactZip returned on the first .xml entry that parsed, so a multi-module artifact carrying one report per module lost every report but the first. It now walks every .xml entry within the aggregate zip-bomb bound and unions the per-report-path maps, bounded by MAX_SOURCE_FILES source files and MAX_LINES_PER_FILE lines each. parse() is refactored onto a shared parseToMap() so one document and the merged walk build the same way. Sequenced after F1 deliberately: merging raises the chance that two modules' same-named classes share a report path, which F1's uncoveredLinesByPath dedup guard now drops rather than misattributes. Refs audit F6
findArtifactId filtered runs on status == "completed" but never read conclusion, which WorkflowRun parses yet nothing in src/main consumed. With the common `if: always()` upload, a run that aborted still attaches the coverage artifact, and its large ci=0 regions were handed to the model as measured fact via PATCH_COVERAGE_REQUEST. Runs are now also filtered on conclusion — accept success and neutral, skip failure/cancelled/timed_out/skipped/action_required/stale — before their artifacts are listed. This is necessary but not sufficient (a run reported successful can still emit a partial report, which this cannot detect), as noted in the code. The probesOnlyABoundedNumberOfRunsForTheSameCommit test seeded runs with a placeholder conclusion "ok" that is not a real GitHub value and would now be filtered out; it is updated to the real "success" so it still exercises the bounded-probe walk. No test pinned the defect itself. Refs audit F2
Landing the four hardening commits on current main left uncoveredLines and uncoveredLinesByPath carrying two copies of the same suffix-matching walk, with only the by-path copy reachable from src/main. Two copies of a rule whose whole job is to refuse an ambiguous attribution is one copy too many, so the single-path form now delegates to the by-path form — its null and blank paths included, which is why it hands over a singletonList rather than a null-hostile List.of — and its javadoc says what it can and cannot see. Resolving a whole file list also has to be duplicate-safe: the same path listed twice would count as two repository files matching one report entry and drop coverage that is in fact unambiguous, so the walk runs over distinct paths. The null check in intersectWithAddedLines no longer also tests isEmpty(): the by-path map never holds an empty set — readSourceFiles drops the empty ones and the merge only ever adds lines — so that arm was unreachable, and an unreachable arm reads as a case someone has thought about. inflateEntry becomes package-private so a test can drive its per-entry collect ceiling and its aggregate budget directly; reaching either through fromArtifactZip would mean building a 64 MB XML entry.
…eader The zip-bomb test padded the archive with a single entry far larger than MAX_ENTRY_BYTES, so it could not tell the aggregate budget apart from the per-entry ceiling that was already there — a reader that refused any oversized entry would have passed it. The padding is now several entries each half the per-entry ceiling, whose sum alone passes the aggregate budget, with the real report still hidden behind them so a reader that reaches it is a reader that paid the full inflation cost. Three bounds the merge introduced went unexercised: an entry drained without being collected because it outgrew the per-entry ceiling, the source-file cap on the merged map, and the per-file line cap across two reports. Each now has a test, the first driving inflateEntry directly rather than building a 64 MB entry to reach it. Also covers a completed run with no conclusion at all, which is no more a run that said it succeeded than an outright failure is, and an empty .xml entry, which must not end the walk.
The multi-module merge unioned the uncovered lines of every report entry naming the same report path. Two entries name one path in exactly the situations the `**/jacoco.xml` upload this merge exists for produces: a same-named class in two modules, and one class measured twice when a per-module report is collected alongside an aggregate report of the same build. Unioning is wrong in both. It charges module-b's misses to module-a's file, and it reports a line as never executed that the aggregate run did execute — the model is then told, as measured fact it must not lower its confidence on, that tested code is untested. The F1 by-path guard was expected to catch the first of those and cannot: by the time it runs the merge has collapsed the two entries into one, which exactly one repository file suffix-matches, so nothing looks ambiguous. It is only visible while the walk still knows two reports claimed the same path. The merge now intersects instead, and drops a path the reports agree on nothing about. That is sound whichever repository file the entry later matches: a line every report recorded as missed is missed in that file's own report too. It is also the reason the per-file line cap left the merge — with no union there is nothing to grow, and the parse already caps each file. Refs audit F1, F6
The walk had grown to hold two jobs at once: charging the aggregate inflation budget, and deciding what a given entry contributes. Reading them apart meant carrying four levels of nesting in one method that is already the class's most security-sensitive one. The loop now reads as the budget accounting it is — inflate an entry, abandon the archive if that blew the budget, charge what it cost — and one helper answers what the entry was worth. No behaviour change.
…at matters The padding size was asserted to be under the per-entry ceiling, which it is by construction — half of it — so the assertion could never fail and said nothing. What has to hold for the test to mean anything is that no single entry blows the aggregate budget on its own, because otherwise a per-entry bound would refuse this archive and the aggregate one would never be reached.
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Contributor
🤖 ThrillhouseBot PR SummaryWhat this PR doesHardens the latent patch-coverage report reader: caps aggregate zip decompression at 128 MB to close a fork-reachable zip bomb, drains every archive entry through the budget so getNextEntry never implicitly inflates a remainder, merges every XML report entry (intersecting, not unioning, same-path misses), resolves the whole reviewable-file list at once to drop attribution ambiguous from either side, and ignores coverage from runs whose conclusion is not success or neutral. Description vs. ImplementationNo mismatch found between the PR description and the change. Control-Flow Diagram🔀 Show diagramflowchart TD
R["findArtifactId: keep runs with success or neutral conclusion, probe their artifacts"] --> Z["download artifact bytes"]
Z --> W["fromArtifactZip: drain every entry against the 128 MB aggregate budget"]
W --> B{"budget blown?"}
B -- yes --> K["abort the walk, keep reports already merged"]
B -- no --> X{".xml entry?"}
X -- yes --> M["parse entry; mergeInto intersects same-path misses"]
X -- no --> W
M --> W
K --> E{"any path left with lines?"}
E -- yes --> P["build JacocoCoverageReport"]
E -- no --> P2["EMPTY report"]
P --> I["uncoveredLinesByPath: drop ambiguous entry to path attribution"]
I --> A["intersect uncovered lines with added lines"]
A --> O["UncoveredFile findings"]
Changes Overview
Changed Files
Risk Assessment
Key Findings
|
| Check | Type | Status | Detail |
|---|---|---|---|
| trivy | check-run | ⏳ Pending | - |
| test | check-run | ⏳ Pending | - |
| frontend | check-run | ⏳ Pending | - |
| format | check-run | ⏳ Pending | - |
| dependency-review | check-run | ⏳ 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}
thrillhousebot
Bot
added
bug
…ader refused The class javadoc says every failure yields EMPTY. The walk did not: reports merged before a zip-bomb abort, or before an IOException broke the stream, were returned as the artifact's coverage. The prefix that survives is chosen by the archive, not by the build. An attacker who appends a bomb entry after a benign report decides which reports the merge sees, and the truncated result reads as complete coverage — every line the rest of the artifact covers comes back uncovered, against a diff the model is told to treat as fact. The IOException case has the same shape with the cut chosen by where the stream broke. The existing aggregate-cap test could not catch this: it hides the report behind the bomb, so nothing has merged by the time the walk gives up. The new test puts a real report first and asserts EMPTY, which fails against the unguarded walk. Reported by the reviewer on this PR.
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):
- MEDIUM: fromArtifactZip javadoc says aborted walk keeps merged reports; code returns EMPTY (
src/main/java/dev/thiagogonzaga/thrillhousebot/review/JacocoCoverageReport.java:228)
The method javadoc added in this diff states: "The walk aborts the moment the aggregate budget is blown, keeping whatever it had already merged." The code directly below it does the opposite:if (aborted) { return EMPTY; }, with the inline comment "A walk that gave up carries no partial answer out. Whatever merged before the abort is a prefix chosen by the archive, not by the build..." and the new testcarriesNoPartialAnswerOutOfAnArchiveItRefusedassertingJacocoCoverageReport.fromArtifactZip(bytes.toByteArray()).isEmpty()on exactly this path (report merged before the bomb, then aborted). A maintainer trusting this javadoc would believe a refused archive yields partial coverage for the reports merged before the bomb — the "wrong signal instead of no signal" failure this PR exists to remove — and would trust or even "fix" a return path that never happens. This sentence is the stale residue of the pre-EMPTY contract (the previous review quoted it as the then-accurate contract); the class javadoc ("every failure yields an {@link #EMPTY} report rather than an exception") and the method's own inline comment both state the real behavior, so the method javadoc also contradicts the class javadoc.
ThrillhouseBot closed 1 previous finding(s) this round:
src/main/java/dev/thiagogonzaga/thrillhousebot/review/JacocoCoverageReport.java:58— Class javadoc claims every failure yields EMPTY, but the abort path keeps partially merged reports
fromArtifactZip's javadoc still described the contract it had before the abort guard landed: "the walk aborts the moment the aggregate budget is blown, keeping whatever it had already merged". The code below it does the opposite — it returns EMPTY — and carriesNoPartialAnswerOutOfAnArchiveItRefused pins exactly that on the path where a real report was merged before the bomb. A stale sentence on a security path is worse than no sentence: a maintainer reading it would believe a refused archive still yields the coverage that happened to precede the bomb, which is the wrong-signal-instead-of-no-signal failure this branch exists to remove, and would trust — or "repair" — a return path that never happens. It also contradicted the class javadoc, which already says every failure degrades to EMPTY. The rest of the paragraph was checked against the current code while there. Three facts it never stated are now stated: the walk gives up on a broken stream as well as a blown budget, an intersection that empties out drops the path instead of surfacing a file with no uncovered lines, and the walk sees at most MAX_ZIP_ENTRIES entries.
uncoveredLinesByPath had grown to hold three separate jobs in one body — finding a path's suffix matches, counting how many paths reached each report entry, and judging whether the resulting attribution is unambiguous — which Sonar reported as cognitive complexity 20 against a limit of 15, plus three loops carrying more than one jump each (S3776, S135 x3). Both decisions now have a name and a javadoc that says what they refuse and why: suffixMatches answers what a path matches and deliberately says nothing about ambiguity, since that answer depends on the other paths in the same request; unambiguousEntry answers whether the one match may lend its lines, refusing both directions of collision. The mergeInto loop drops its two continues for an else-if, and gains a note that the source-file cap only ever bounds NEW paths, so reaching it can never turn an intersection into a union. No behaviour moves: the same guards run in the same order, the ambiguity rules and their debug logs are unchanged, and the merge still intersects. The existing tests are the proof, all 3468 of them, unchanged.
The javadoc written a commit ago said suffixMatches returns its matches "in the order the reports listed them". It does not: the index it reads is a HashMap keyed by file name, so the order is whatever hashing produced. Nothing depends on it — a second match is an ambiguity whichever one came first — so the fix is to say that, not to impose an order no caller wants.
The constant predates this branch and its doc still read as though it bounded one entry's decompression — which it did, back when the walk truncated the read at readNBytes(MAX_ENTRY_BYTES). It no longer does: the entry is drained in full and this only bounds how much is KEPT for parsing, because stopping early hands the remainder to the next getNextEntry() to inflate regardless. Only the aggregate budget bounds inflation, and the doc now says so.
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
ThrillhouseBot closed 1 previous finding(s) this round:
src/main/java/dev/thiagogonzaga/thrillhousebot/review/JacocoCoverageReport.java:228— fromArtifactZip javadoc says aborted walk keeps merged reports; code returns EMPTY
…erging the prefix The bomb abort stopped the reader carrying a partial answer out; the entry cap still did. A walk that stops at entry 512 returns whatever merged, and the archive decides what that is: pad past the cap and whoever built it chooses which reports the merge saw, while the result still reads as this build's coverage. Every line the unread reports cover comes back uncovered, against a diff the prompt tells the model to treat as fact. That is the same prefix-choosing power the aggregate budget refuses, reached by padding rather than by compression, so it gets the same answer. #483's framing is that these defects degrade to wrong signal rather than to no signal, and a truncated merge is wrong signal; EMPTY is merely no signal. The trade is deliberate and costs a legitimately huge artifact its coverage. A JaCoCo upload names one report per module, so 512 is already far past what a real multi-module build produces, and an artifact past it is anomalous rather than large. Two tests: an archive one entry past the cap yields EMPTY, and one that exactly fills the cap is still read, so the refusal covers only what the walk cannot see.
Contributor
🤖 ThrillhouseBot — changes since the last review
|
…t, not to directories The refusal added for archives past the entry cap counted every entry the walk saw, directories included. A coverage artifact is usually a whole target/ tree rather than a flat list of reports, so its directories alone can outnumber its modules and push a handful of jacoco.xml files past a cap meant to bound how much content is read. The multi-module merge this branch exists for is exactly the shape that loses, with a readable report sitting near the front. The reasoning in the previous commit — that one report per module leaves 512 far out of reach — held only for an archive of reports. It does not hold for the archive shape the feature actually meets. Directories are walked past without charge: they carry no data, cost nothing to skip, and can never hide a report. The bound still limits how much content is read, which is what it was for. The refusals also move into walkRefused and leave by returning. The exits read as the two answers they are, and a continue could not be used for either: the loop's update expression calls getNextEntry, which inflates the remainder of the entry it leaves, so skipping past a bomb entry would pay the exact cost the aggregate budget refuses. Reported by the reviewer on this PR.
|
Contributor
🤖 ThrillhouseBot — changes since the last review
|
Comment on lines
+262
to
+264
| * parser understands — or when the walk gave up part-way, per the paragraph below. At most {@link | ||
| * #MAX_ZIP_ENTRIES} entries are walked, so a report sitting behind that many others is never | ||
| * reached. |
Contributor
There was a problem hiding this comment.
🔵 LOW — Javadoc says at most MAX_ZIP_ENTRIES entries are walked, but directory entries bypass the cap
The rewritten class-level javadoc states: "At most {@link #MAX_ZIP_ENTRIES} entries are walked, so a report sitting behind that many others is never reached." The code it documents does the opposite: walkRefused() executes if (entry.isDirectory()) { continue; } before the seen++ >= MAX_ZIP_ENTRIES check, so every directory entry is walked without counting toward the cap and a report sitting behind thousands of directories IS reached. The new test walksPastDirectoriesWithoutSpendingTheEntryCap itself builds an archive with MAX_ZIP_ENTRIES + 2 entries to demonstrate that. walkRefused's own javadoc states the true mechanism ("Directories are walked past without charge"), so the two comments contradict each other about the same bound; a maintainer trusting only the class javadoc gets the walk bound wrong.
Suggested change
| * parser understands — or when the walk gave up part-way, per the paragraph below. At most {@link | |
| * #MAX_ZIP_ENTRIES} entries are walked, so a report sitting behind that many others is never | |
| * reached. | |
| * parser understands — or when the walk gave up part-way, per the paragraph below. At most {@link | |
| * #MAX_ZIP_ENTRIES} non-directory entries are walked; directories are walked past without charge, | |
| * so a report behind that many content entries is never reached. |
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: Consumer relies on a 'present entry is non-empty' invariant only enforced by fromArtifactZip (
src/main/java/dev/thiagogonzaga/thrillhousebot/review/PatchCoverageResolver.java:276)
The consumer now skips only onreportedUncovered == nulland the new comment claims "a present entry always carries at least one uncovered line." That invariant is enforced on exactly one producer path: fromArtifactZip ends withmerged.values().removeIf(NavigableSet::isEmpty)before constructing the report. The other producer,parse(InputStream), buildsnew JacocoCoverageReport(map)directly with no such filtering step, and the guard this diff removes — the oldif (reportedUncovered.isEmpty()) { continue; }— is what covered that path before. Verification request: checkreadSourceFiles(body not in the provided material) — if it can record a<sourcefile>element all of whose lines are covered (mi=0, a normal shape for jacoco.xml), a parse()-built report yields present-but-empty entries and the consumer proceeds where it previously skipped. The in-diff tests all feed reports whose source files carry at least one missed line, so none of them can distinguish the two behaviors.
ThrillhouseBot closed 1 previous finding(s) this round:
src/main/java/dev/thiagogonzaga/thrillhousebot/review/JacocoCoverageReport.java:284— Archives over the entry cap now yield EMPTY, regressing legitimate large artifacts
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
Four defects in the patch-coverage report reader (
#115, new in v0.6.0), written onfix/coverage-report-hardeningoffrelease/v0.6.0and brought onto currentmainhere, plus afifth found while re-validating them against today's code.
All of it is latent:
thrillhousebot.review.patch-coverage.enableddefaultsfalse. It stillmatters, because none of these degrade to no coverage signal — they degrade to a wrong one, and
PATCH_COVERAGE_REQUESTtells the model the section is "measurement, not inference — treat it asfact about those lines".
1. Aggregate decompression was unbounded — a zip bomb, reachable from a fork.
fromArtifactZipbounded each entry withreadNBytes(MAX_ENTRY_BYTES)but nothing bounded thetotal: the next
getNextEntry()implicitly inflates the whole remainder of an unread entry toreach the following header, so an entry the walk skipped was inflated in full anyway. A
maximally-compressed artifact at the 16 MB
ArtifactZipFetcher.MAX_BYTESdownload ceiling reachesroughly 16 GB of decompression per review, and a
pull_request-triggered workflow that builds afork's code lets a fork PR publish such an artifact under the configured name for the head SHA
under review. The class javadoc claimed the archive's "entry count and uncompressed size are
capped"; only the first half was true. Every entry — report or not — is now inflated through a
counting copy charging a running
MAX_TOTAL_INFLATED_BYTES(128 MB) budget, each one drained infull so the next
getNextEntry()never inflates a remainder, and the walk abandons the archive themoment the budget is blown. The javadoc now says what the code does.
2. One report entry attributed to every same-named repository file.
uncoveredLinesrefusedthe N-entries→1-path ambiguity but not its mirror.
com/example/Foo.javais a whole-segment suffixof every module's copy of that class, and a
<package name="">default-package entrysuffix-matches every file of that name anywhere, so module-a's misses were reported against
module-b's same-named class — the exact failure the method's own comment says it exists to avoid.
The intersection now resolves the whole reviewable-file list at once and drops any attribution
ambiguous from either side; only a unique 1:1 report↔path match contributes lines.
3. Coverage from a run that did not succeed was used as fact.
findArtifactIdfiltered runs onstatus == "completed"and never readconclusion, whichWorkflowRunparsed and nothingconsumed. With the common
if: always()coverage upload, a run that aborted partway still attachesits artifact, and its large
ci=0regions became "measured fact" about untested code. Runs are nowalso filtered on
conclusion(success/neutralaccepted, and an absent conclusion is not a runthat said it succeeded) before their artifacts are listed. Necessary, not sufficient — a green run
can still emit a partial report under
-Dmaven.test.failure.ignore=trueor a skipped module — andthe code says so.
4. A multi-module artifact lost every report but the first.
fromArtifactZipreturned on thefirst
.xmlthat parsed non-empty, so a build uploading**/jacoco.xmlkept one module andsilently discarded the rest. Every
.xmlentry now contributes, within the same aggregateinflation budget and the existing
MAX_SOURCE_FILES/MAX_LINES_PER_FILEcaps.5. Found while re-validating: merging two reports of the same path unioned their misses. Fix 4
merged by union, and fix 2's guard was expected to catch the collisions that creates. It cannot:
by the time it runs, the merge has already collapsed the two entries into one that exactly one
repository file suffix-matches, so nothing looks ambiguous. Two entries claim one path in precisely
the situations a
**/jacoco.xmlupload produces — a same-named class in two modules, and one classmeasured twice when a per-module report is collected next to an aggregate report of the same build
— and the union charges module-b's misses to module-a's file, or reports a line as never executed
that the aggregate run did execute. The merge now intersects, which is sound whichever repository
file the entry later matches: a line every report recorded as missed is missed in that file's own
report too. A path the reports agree on nothing about is dropped rather than left as an empty
entry.
Re-validation against current
mainThe branch was four commits off a base from two weeks and dozens of PRs ago. All four defects
reproduce verbatim on today's
main— none had been fixed or made redundant in the meantime, andthe only change to
JacocoCoverageReportsince that base was extracting a/separator into aconstant. Beyond finding 5, rebasing surfaced three things worth calling out:
uncoveredLinesand the newuncoveredLinesByPathended up carrying two copies of the samesuffix-matching walk, with only the by-path copy reachable from
src/main. The single-path formnow delegates, so there is one matching policy to reason about.
two repository files matching one entry and drop coverage that is in fact unambiguous.
ceiling, so it could not tell the new aggregate bound apart from the per-entry bound already
there — a reader that simply refused any oversized entry would have passed it. The padding is now
several entries each half the per-entry ceiling whose sum alone passes the aggregate budget, with
the real report still hidden behind them.
Related Issues
Part of #483 — deliberately not
Closes.The issue carries six acceptance criteria; this PR satisfies four. F7 (config-key candidate files
under an ignored path are still fetched and rendered) and F9 (the dead
defaultBranchfallback inReviewContextLoader.resolveConfigKeyContext) are outside this branch's file scope, both needReviewContextLoaderchanges, and both were verified still open onmain. The issue's ownEnvironment section says as much: the branch "carries the first four fixes; F7/F9 remain to be
written." Auto-closing here would leave two boxes unticked and the work forgotten
Not in this PR: the issue's last two acceptance criteria (F7, config-key candidate files bypassing
the review's ignore globs; F9, the dead
defaultBranchfallback inReviewContextLoader). Both areoutside this branch's file scope, both are still open on
main, and both want their own change toReviewContextLoader— worth a follow-up issue rather than folding into this one.How Has This Been Tested?
Every behavioural change was run against the unfixed code first:
refusesAnArchiveThatInflatesPastTheAggregateCapan archive inflating past the aggregate cap must be refused, not walked to the report hidden behind the bomb ==> expected: <true> but was: <false>dropsAReportEntryThatMatchesMoreThanOneRepositoryFileone report entry matches both modules' same-named class; attributing it to either is a guess: [module-a/src/main/java/com/example/Foo.java, module-b/src/main/java/com/example/Foo.java] ==> expected: <true> but was: <false>ignoresCoverageFromARunThatDidNotSucceedcoverage from a failed or cancelled run is not something the model may treat as fact ==> expected: <> but was: <### Patch coverage for this diff …>mergesEveryXmlReportNotJustTheFirsta second module's report must not be lost to a first-match-wins walk ==> expected: <[2]> but was: <[]>keepsOnlyWhatTwoReportsOfTheSamePathAgreeOnonly a line every report recorded as missed may be reported; a line one of them saw executed was executed ==> expected: <[2]> but was: <[1, 2, 3]>Gates on JDK 25:
Patch coverage on the changed lines, from
target/site/jacoco/jacoco.xmlintersected withgit diff -U0 origin/main --: 0 uncovered lines and 0 uncovered branches across both changedsource files.
Checklist
Additional Notes
The feature stays off by default; nothing here changes behaviour for a repository that configures no
coverage artifact. The source branch
fix/coverage-report-hardeningis the only copy of theoriginal work and has been left untouched.