From d1acd45c020a9c843b40a16b73ba90d67d52bbb9 Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Mon, 7 Sep 2026 09:49:54 -0300 Subject: [PATCH 1/3] fix(review): count only .xml entries toward the coverage artifact cap 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. --- CHANGELOG.md | 4 + README.md | 6 + .../review/JacocoCoverageReport.java | 234 ++++++++++++------ .../review/PatchCoverageResolver.java | 58 ++++- .../review/ReviewContextLoader.java | 71 +++++- .../thrillhousebot/review/VerdictBuilder.java | 12 +- .../review/JacocoCoverageReportTest.java | 203 +++++++++++++-- .../review/PatchCoverageResolverTest.java | 64 ++++- .../review/ReviewContextLoaderTest.java | 34 ++- .../review/ReviewOrchestratorTest.java | 13 +- .../review/VerdictBuilderTest.java | 76 +++++- 11 files changed, 651 insertions(+), 124 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 98a78176..cd375806 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,10 @@ All notable changes to ThrillhouseBot. ## [Unreleased] +### Fixed + +- **A coverage artifact that uploads the HTML report beside `jacoco.xml` is read, and one that is refused says so** (#813): the archive walk charged every file entry to the 512-entry cap, not only the `.xml` entries that can carry a report, so uploading `target/site/jacoco/` (one `.html` per class plus the stylesheet, images and script) crossed the cap on a project of a few hundred classes and the whole artifact was refused, with a DEBUG line as the only trace. Only `.xml` entries count toward the cap now; every other entry is still drained in full against the 128 MB aggregate budget from #789, so the zip-bomb bound is unchanged. A refusal is logged at WARN with the counts, and the review summary's scope note says the configured coverage artifact was not read and why. Latent until `thrillhousebot.review.patch-coverage.enabled` is turned on + ## [0.6.7] — 2026-09-07 Two production reviews drove this one: a pull request that was approved after most of the model's answer was thrown away, and one that was pushed to while under review and lost every finding to the push. The rest is hardening found by auditing the merged pull requests and by dogfooding the repository configuration. No configuration changes; upgrading is a redeploy. The one behaviour a deployment may notice is that `ignored-files` globs now match the way the documentation always said they did, so a pattern that was silently doing nothing starts excluding files. diff --git a/README.md b/README.md index 95bd3783..7d4fa871 100644 --- a/README.md +++ b/README.md @@ -751,6 +751,12 @@ Nothing about coverage is ever inferred from the diff, and a line's *absence* fr list is explicitly not evidence that a test covers it. Files the ignore list already excluded are never reported as under-tested. +Uploading the whole `target/site/jacoco/` tree, with the HTML report beside `jacoco.xml`, +is fine: only `.xml` entries count toward the archive's 512-entry cap. An artifact the +bot found but refused to read — more `.xml` entries than that, or one that inflates past +128 MB — is named in the review summary's scope note the way an ignore glob that matched +nothing is, so a configured artifact never goes quiet without saying why. + The file is read from the repository's default branch on each review and cached for five minutes. YAML anchors, aliases and merge keys are resolved; a document that is oversized, nested absurdly deep, or built on a runaway alias expansion is refused whole. diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/JacocoCoverageReport.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/JacocoCoverageReport.java index a50b3a40..7ed2062d 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/JacocoCoverageReport.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/JacocoCoverageReport.java @@ -54,14 +54,23 @@ * says or reports nothing. * *

The bytes come from a workflow artifact uploaded by an arbitrary repository, so parsing is - * defensive throughout — external entities and DTD loading are off, the archive's entry count and - * its aggregate inflated size are both capped against a zip bomb (see {@link - * #MAX_TOTAL_INFLATED_BYTES}), and every failure yields an {@link #EMPTY} report rather than an - * exception. + * defensive throughout — external entities and DTD loading are off, the archive's report-entry + * count and its aggregate inflated size are both capped against a zip bomb (see {@link + * #MAX_TOTAL_INFLATED_BYTES}), and every failure yields an empty report rather than an exception. + * An archive the walk refused says why on that empty report ({@link #refusal}), so the review can + * tell a maintainer who configured the artifact that it was not read (#813). */ final class JacocoCoverageReport { - /** Entries walked in the artifact archive before the rest are ignored. */ + /** + * Ceiling on the entries that can carry a report — names ending in {@code .xml} — before the + * archive is refused whole. Nothing else counts: the usual upload is the whole {@code + * target/site/jacoco/} tree, one {@code .html} per class beside the one {@code jacoco.xml}, and a + * cap that charged every file refused that shape on a project of a few hundred classes with a + * single legitimate report inside (#813). The entries left uncounted are still drained against + * {@link #MAX_TOTAL_INFLATED_BYTES}, and {@code ArtifactZipFetcher.MAX_BYTES} bounds how many of + * them a download can hold at all. + */ static final int MAX_ZIP_ENTRIES = 512; /** @@ -100,14 +109,55 @@ final class JacocoCoverageReport { private static final String BINARY_PACKAGE_SEPARATOR = "/"; /** No coverage data — the value every failure path degrades to. */ - static final JacocoCoverageReport EMPTY = new JacocoCoverageReport(Map.of()); + static final JacocoCoverageReport EMPTY = new JacocoCoverageReport(Map.of(), null); + + /** + * Why {@link #fromArtifactZip} gave up on an archive, carried on the empty report it returns so + * the review can say so (#813). Each reason is phrased for the summary's review-scope note, where + * it follows "the configured coverage artifact was not read:". 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. + */ + enum Refusal { + /** + * More {@code .xml} entries than {@link JacocoCoverageReport#MAX_ZIP_ENTRIES}. Merging the + * prefix that fit would let whoever built the archive choose which reports the review saw. + */ + ENTRY_CAP("it holds more than " + MAX_ZIP_ENTRIES + " `.xml` entries"), + /** + * Inflation past {@link JacocoCoverageReport#MAX_TOTAL_INFLATED_BYTES}: a zip bomb, or an + * upload far larger than any coverage report. + */ + INFLATION_BUDGET( + "it inflates past the " + (MAX_TOTAL_INFLATED_BYTES >> 20) + " MB decompression limit"), + /** The bytes broke mid-walk — a truncated download, or a body that is not a zip at all. */ + UNREADABLE("it could not be read as a zip archive"); + + private final String reason; + + Refusal(String reason) { + this.reason = reason; + } + + /** The reason in the summary's words, without the lead-in. */ + String reason() { + return reason; + } + } /** Uncovered lines per report source path, indexed by that path's file name for lookup. */ private final Map> byFileName; + /** + * Why the archive walk gave up, or {@code null} for a report read to the end — {@link #EMPTY} + * included, since an archive that held nothing usable was not refused. + */ + private final Refusal refusal; + private record SourceFile(String path, NavigableSet uncoveredLines) {} - private JacocoCoverageReport(Map> uncoveredLinesByPath) { + private JacocoCoverageReport( + Map> uncoveredLinesByPath, Refusal refusal) { var index = new HashMap>(); uncoveredLinesByPath.forEach( (path, lines) -> @@ -115,12 +165,27 @@ private JacocoCoverageReport(Map> uncoveredLinesBy .computeIfAbsent(fileName(path), unused -> new ArrayList<>()) .add(new SourceFile(path, lines))); this.byFileName = index; + this.refusal = refusal; + } + + /** An empty report that also says why the archive it came from was not read. */ + private static JacocoCoverageReport refused(Refusal refusal) { + return new JacocoCoverageReport(Map.of(), refusal); } boolean isEmpty() { return byFileName.isEmpty(); } + /** + * Why {@link #fromArtifactZip} refused the archive, or {@code null} when nothing was refused. An + * empty report with no refusal is the common "nothing usable was published" case and is not + * disclosed; a refused one is, because the artifact was there and this reader would not read it. + */ + Refusal refusal() { + return refusal; + } + /** * The uncovered lines the report holds for a repository-relative path, or an empty set when the * report says nothing about that file. @@ -259,21 +324,21 @@ private static String fileName(String path) { * agree on nothing about is dropped outright rather than reaching callers as a file with an empty * line set. * - *

{@link #EMPTY} when the bytes are not a readable zip, hold no such entry, hold nothing this - * parser understands — or when the walk gave up part-way, per the paragraph below. At most {@link - * #MAX_ZIP_ENTRIES} file entries are read. Directory entries are not counted against that cap, - * but they are not trusted either: a name ending in a slash may still carry a payload, so each - * one is drained against the same aggregate budget as a file and refused the same way when it - * blows it. An archive with more file entries than the cap is refused whole rather than merged as - * far as the cap allowed. + *

{@link #EMPTY} when the bytes hold no {@code .xml} entry or nothing this parser understands; + * an empty report carrying a {@link Refusal} when the walk gave up part-way, per the paragraph + * below. At most {@link #MAX_ZIP_ENTRIES} {@code .xml} entries are read, and an archive with more + * is refused whole rather than merged as far as the cap allowed. No other entry counts toward + * that cap — not the HTML report beside the XML, not a directory name — but none is trusted + * either: a name ending in a slash may still carry a payload, so every entry is drained against + * the same aggregate budget and refused the same way when it blows it. * *

Every entry — not only the {@code .xml} we want — is inflated through a counting copy * bounded by {@link #MAX_TOTAL_INFLATED_BYTES}. Reading only the entries we care about is not * enough: the next {@link ZipInputStream#getNextEntry()} implicitly inflates the whole of an * unread entry to reach the following header, which is exactly the path a maximally-compressed * archive takes to gigabytes. The moment that aggregate budget is blown — or the stream breaks - * mid-walk — the archive is abandoned and everything already merged is discarded for - * {@link #EMPTY}: the surviving prefix is chosen by whoever built the archive rather than by the + * mid-walk — the archive is abandoned and everything already merged is discarded for an + * empty report: the surviving prefix is chosen by whoever built the archive rather than by the * build, so returning it would report as uncovered whatever the unread remainder covers. No * partial answer leaves this method; the abort itself carries the full reasoning. */ @@ -282,49 +347,54 @@ static JacocoCoverageReport fromArtifactZip(byte[] zipBytes) { return EMPTY; } var merged = new HashMap>(); - var aborted = false; + Refusal refusal; try (var zip = new ZipInputStream(new ByteArrayInputStream(zipBytes))) { - aborted = walkRefused(zip, merged); + refusal = walkRefused(zip, merged); } catch (IOException | RuntimeException e) { - Log.debugf(e, "Could not read the coverage artifact archive"); - aborted = true; + Log.warn("Could not read the coverage artifact archive; refusing it", e); + refusal = Refusal.UNREADABLE; } // 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: an attacker who appends a bomb entry after a // benign report would otherwise decide which reports the merge sees, and the truncated result // reads as complete coverage — lines the rest of the artifact covers come back "uncovered". // The same holds for an IOException mid-walk, where the prefix is chosen by where the stream - // broke. This is what the class javadoc means by every failure yielding EMPTY. - if (aborted) { - return EMPTY; + // broke. This is what the class javadoc means by every failure yielding an empty report; the + // refusal rides along so the review can say the artifact was there and was not read. + if (refusal != null) { + return refused(refusal); } // An intersection that emptied out says every report disagreed about that file, which is not // coverage data; the rest of the class may assume a present path has at least one line. merged.values().removeIf(NavigableSet::isEmpty); - return merged.isEmpty() ? EMPTY : new JacocoCoverageReport(merged); + return merged.isEmpty() ? EMPTY : new JacocoCoverageReport(merged, null); } /** - * Inflates one archive entry within {@code budgetLeft} and merges it into {@code merged} when it - * turns out to be a JaCoCo report, answering how many bytes it cost — or {@code -1} when the - * aggregate budget is gone and the archive must be abandoned. - * - *

Every entry is drained through the counting copy, a report to collect and anything else to - * discard, because leaving an entry partly read hands the implicit inflation back to the next - * {@code getNextEntry()}. Only the aggregate budget stops the drain. + * Whether an entry can carry a report and so counts toward {@link #MAX_ZIP_ENTRIES}: a name + * ending in {@code .xml}. A directory name ends in a slash, so it can never pass; a non-report + * XML (a surefire report, say) does pass and costs a slot, because the cap bounds how many + * documents the merge will parse, not how many of them turn out to be JaCoCo. + */ + private static boolean isReport(ZipEntry entry) { + return entry.getName().toLowerCase(Locale.ROOT).endsWith(".xml"); + } + + /** + * Inflates one {@code .xml} entry within {@code budgetLeft} and merges it into {@code merged} + * when it turns out to be a JaCoCo report, answering how many bytes it cost — or {@code -1} when + * the aggregate budget is gone and the archive must be abandoned. A report larger than {@link + * #MAX_ENTRY_BYTES} is drained in full but contributes nothing, per {@link #inflateEntry}. */ - private static long readEntryInto( + private static long readReportInto( ZipInputStream zip, ZipEntry entry, long budgetLeft, Map> merged) throws IOException { - // Directories never reach here: walkRefused steps over them before charging the cap, so the - // only question left is whether this file entry is a report. - var isReport = entry.getName().toLowerCase(Locale.ROOT).endsWith(".xml"); - var sink = isReport ? new ByteArrayOutputStream() : null; + var sink = new ByteArrayOutputStream(); var read = inflateEntry(zip, budgetLeft, MAX_ENTRY_BYTES, sink); - if (read < 0 || sink == null || sink.size() == 0) { + if (read < 0 || sink.size() == 0) { return read; } var one = parseToMap(new ByteArrayInputStream(sink.toByteArray())); @@ -408,7 +478,7 @@ static long inflateEntry( /** Parses one JaCoCo XML document, or {@link #EMPTY} when it is not one / cannot be read. */ static JacocoCoverageReport parse(InputStream xml) { var map = parseToMap(xml); - return map.isEmpty() ? EMPTY : new JacocoCoverageReport(map); + return map.isEmpty() ? EMPTY : new JacocoCoverageReport(map, null); } /** @@ -530,7 +600,8 @@ static void closeQuietly(XMLStreamReader reader) { } /** - * Walks the archive's entries into {@code merged}, and reports whether it gave up. + * Walks the archive's entries into {@code merged}, and reports why it gave up — or {@code null} + * when it read the archive to the end. * *

Each refusal leaves by returning rather than by breaking, which keeps the two limits reading * as the two answers they are. It also matters mechanically: a {@code continue} here would run @@ -538,58 +609,59 @@ static void closeQuietly(XMLStreamReader reader) { * is leaving — so skipping past a bomb entry would pay exactly the cost the aggregate budget * exists to refuse. * - *

The cap counts file entries, report or not. Directories are drained against the aggregate - * budget like everything else but not charged to the cap, so a tree-shaped artifact is judged by - * how much content it holds rather than by how deeply it is nested. + *

The cap counts only entries that can carry a report ({@link #isReport}), because it exists + * to bound how many documents the merge parses, not how many files the archive holds (#813): the + * usual upload is the whole {@code target/site/jacoco/} tree, one {@code .html} per class beside + * the one {@code jacoco.xml}, and a cap that charged every file refused that shape on a project + * of a few hundred classes with a single legitimate report inside. Every other entry is drained + * in full against the aggregate budget and refused the same way when it blows it, so nothing left + * uncounted can smuggle inflation. A directory name in particular is not a promise of zero data: + * nothing in the local-header format stops a crafted entry called {@code bomb/} carrying + * megabytes of deflate, and leaving it to the loop's {@code getNextEntry()} would inflate that + * payload uncharged, since the stream must dispose of the current entry before it can reach the + * next header. A real directory costs one immediate EOF read. * - * @return {@code true} when a limit stopped the walk, so whatever merged is a prefix the archive - * chose and no report may be built from it + *

Both refusals are logged at WARN — a coverage section that goes quiet with only a DEBUG line + * behind it is a reason nobody ever reads (#813) — and with counts only, never an entry name, + * since the names are whoever built the archive's to choose. + * + * @return why a limit stopped the walk, so whatever merged is a prefix the archive chose and no + * report may be built from it; {@code null} when nothing stopped it */ - private static boolean walkRefused(ZipInputStream zip, Map> merged) + private static Refusal walkRefused(ZipInputStream zip, Map> merged) throws IOException { - var seen = 0; + var walked = 0; + var reports = 0; var inflatedTotal = 0L; for (var entry = zip.getNextEntry(); entry != null; entry = zip.getNextEntry()) { - if (entry.isDirectory()) { - // A name ending in '/' is not a promise of zero data: nothing in the local-header format - // stops a crafted entry called bomb/ carrying megabytes of deflate. Leaving it to the - // loop's - // getNextEntry() would inflate that payload uncharged, since the stream must dispose of the - // current entry before it can reach the next header. So the entry is drained here, against - // the same aggregate budget as everything else, and refused the same way when it blows it. - // A real directory costs one immediate EOF read. It is still not charged to the entry cap: - // a coverage artifact is usually a whole target/ tree, and its directories alone can push - // a handful of jacoco.xml files past a cap that bounds how much content is read. - var drained = inflateEntry(zip, MAX_TOTAL_INFLATED_BYTES - inflatedTotal, 0, null); - if (drained < 0) { - Log.debugf( - "Coverage artifact inflates past the %d-byte aggregate cap inside a directory entry;" - + " refusing it as a zip bomb", - MAX_TOTAL_INFLATED_BYTES); - return true; + walked++; + var budgetLeft = MAX_TOTAL_INFLATED_BYTES - inflatedTotal; + long read; + if (isReport(entry)) { + if (reports++ >= MAX_ZIP_ENTRIES) { + // Padding an archive past the cap would otherwise let whoever built it decide which + // reports the merge saw, while the result still reads as this build's coverage: lines + // the unread reports cover come back uncovered, against a diff the model is told to + // treat as fact. + Log.warnf( + "Coverage artifact holds more than %d .xml entries (%d entries walked, %d bytes" + + " inflated); refusing it rather than merging the prefix that fit", + MAX_ZIP_ENTRIES, walked, inflatedTotal); + return Refusal.ENTRY_CAP; } - inflatedTotal += drained; - continue; + read = readReportInto(zip, entry, budgetLeft, merged); + } else { + read = inflateEntry(zip, budgetLeft, 0, null); } - if (seen++ >= MAX_ZIP_ENTRIES) { - // Padding an archive past the cap would otherwise let whoever built it decide which reports - // the merge saw, while the result still reads as this build's coverage: lines the unread - // reports cover come back uncovered, against a diff the model is told to treat as fact. - Log.debugf( - "Coverage artifact carries more than %d entries; refusing it rather than merging the" - + " prefix that fit", - MAX_ZIP_ENTRIES); - return true; - } - var read = readEntryInto(zip, entry, MAX_TOTAL_INFLATED_BYTES - inflatedTotal, merged); if (read < 0) { - Log.debugf( - "Coverage artifact inflates past the %d-byte aggregate cap; refusing it as a zip bomb", - MAX_TOTAL_INFLATED_BYTES); - return true; + Log.warnf( + "Coverage artifact inflates past the %d-byte aggregate cap (%d entries walked, %d of" + + " them .xml); refusing it as a zip bomb", + MAX_TOTAL_INFLATED_BYTES, walked, reports); + return Refusal.INFLATION_BUDGET; } inflatedTotal += read; } - return false; + return null; } } diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/PatchCoverageResolver.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/PatchCoverageResolver.java index 90259be2..88e30d9c 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/PatchCoverageResolver.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/PatchCoverageResolver.java @@ -53,6 +53,13 @@ * coverage signal would be worse than none, because the reviewer is told to raise its confidence on * the strength of it. * + *

The one exception to that silence is an artifact that was found and then refused by the reader + * — more {@code .xml} entries than it will merge, or one that inflates past its decompression + * budget (#813). That contributes no section either, but it is not the same situation: the + * maintainer configured an artifact, the run uploaded it, and this review chose not to read it. So + * the reason travels out in {@link Resolution#artifactRefusal} and the summary's review-scope note + * says the artifact was not read, the way an ignore glob that matched nothing is disclosed (#481). + * *

Every fetch is best-effort. Nothing here can fail a review. */ @ApplicationScoped @@ -88,6 +95,18 @@ public class PatchCoverageResolver { static final String SECTION_HEADING = "### Patch coverage for this diff (from the repository's own CI coverage report)"; + /** + * What one review took from the configured coverage artifact: the prompt section, or {@code ""} + * when there is nothing truthful to say, and the reason the artifact was refused when it was — + * {@code ""} otherwise — in the words {@link #formatScopeNote} puts in the summary. A refused + * artifact never yields a section and a section never comes with a refusal; both empty is the + * common case. + */ + record Resolution(String section, String artifactRefusal) { + /** Nothing to add to the prompt and nothing to disclose. */ + static final Resolution NONE = new Resolution("", ""); + } + private final GitHubActionsClient actionsClient; private final ArtifactZipFetcher zipFetcher; private final boolean enabled; @@ -109,48 +128,67 @@ public PatchCoverageResolver( } /** - * The prompt-ready uncovered-changed-lines section, or {@code ""} when there is nothing truthful - * to say — the feature is off, the repository named no artifact, the head SHA is unknown, no - * report could be read, or every added line the report knows about is covered. + * The prompt-ready uncovered-changed-lines section, or an empty one when there is nothing + * truthful to say — the feature is off, the repository named no artifact, the head SHA is + * unknown, no report could be read, or every added line the report knows about is covered — and, + * separately, the reason the artifact was refused when it was. * * @param reviewableFiles the post-ignore-filter file list the rest of the review already uses, so * a file the ignore set removed from review scope is never reported as under-tested */ - String resolve( + Resolution resolve( String auth, ReviewOrchestrator.ReviewRequest req, RepoSettings repoSettings, List reviewableFiles) { if (!enabled) { - return ""; + return Resolution.NONE; } // Never null: RepoSettings normalizes an absent name to "" in its compact constructor. var artifactName = repoSettings.coverageArtifact(); if (artifactName.isBlank()) { - return ""; + return Resolution.NONE; } var headSha = req.commitSha(); if (headSha == null || headSha.isBlank() || reviewableFiles.isEmpty()) { - return ""; + return Resolution.NONE; } try { var report = loadReport(auth, req.owner(), req.repo(), headSha, artifactName.strip()); + var refusal = report.refusal(); + if (refusal != null) { + // The reader already logged the refusal at WARN with its counts; this is the one path + // where "no section" has to reach the maintainer as well as the operator. + return new Resolution("", refusal.reason()); + } if (report.isEmpty()) { - return ""; + return Resolution.NONE; } var uncovered = intersectWithAddedLines(report, reviewableFiles); if (uncovered.isEmpty()) { Log.debugf("Coverage report for %s covers every added line", headSha); - return ""; + return Resolution.NONE; } Log.infof( "Patch coverage: %d changed file(s) have added lines with no covering test", uncovered.size()); - return render(uncovered); + return new Resolution(render(uncovered), ""); } catch (RuntimeException e) { Log.warn("Patch-coverage resolution failed, continuing without it", e); + return Resolution.NONE; + } + } + + /** + * The summary's review-scope note for a refused artifact, in the voice of the other scope notes + * (#481's unmatched-glob note, #806's carry-over note): what this review did not look at, and + * why. Empty when nothing was refused. + */ + static String formatScopeNote(String artifactRefusal) { + if (artifactRefusal == null || artifactRefusal.isBlank()) { return ""; } + return "the configured coverage artifact was not read: " + artifactRefusal; } // ---------------------------------------------------------------- sourcing diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewContextLoader.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewContextLoader.java index a852b814..5bdf3b78 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewContextLoader.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewContextLoader.java @@ -140,7 +140,8 @@ public record ReviewContext( PrTotals prTotals, List conversationComments, List unmatchedIgnoreGlobs, - SupersededFindingsCarryover.Carried carried) { + SupersededFindingsCarryover.Carried carried, + String coverageArtifactRefusal) { public ReviewContext { files = List.copyOf(files); priorReviews = List.copyOf(priorReviews); @@ -153,6 +154,67 @@ public record ReviewContext( unmatchedIgnoreGlobs = List.copyOf(unmatchedIgnoreGlobs); } + /** + * Back-compat constructor for callers that carry no coverage-artifact refusal. Defaults it to + * empty, which reads as "the configured artifact was read, or none was configured" — the quiet + * direction, since a refusal nobody observed must never be disclosed (#813). + */ + @SuppressWarnings("java:S107") + public ReviewContext( + List files, + String diff, + String baseComparison, + int omittedFiles, + List priorReviews, + List priorAiResponseJsons, + List priorAiResponses, + boolean isFirstVisibleReview, + boolean hasContext, + String previousAiResponseJson, + List inlineComments, + String previousFindings, + InstructionsResolver.ResolvedInstructions instructions, + PathScopedInstructions pathInstructions, + List repoLabels, + String projectStack, + String linkedIssuesContext, + String configKeyContext, + String patchCoverage, + List reviewableFiles, + Supplier lineResolverSupplier, + PrTotals prTotals, + List conversationComments, + List unmatchedIgnoreGlobs, + SupersededFindingsCarryover.Carried carried) { + this( + files, + diff, + baseComparison, + omittedFiles, + priorReviews, + priorAiResponseJsons, + priorAiResponses, + isFirstVisibleReview, + hasContext, + previousAiResponseJson, + inlineComments, + previousFindings, + instructions, + pathInstructions, + repoLabels, + projectStack, + linkedIssuesContext, + configKeyContext, + patchCoverage, + reviewableFiles, + lineResolverSupplier, + prTotals, + conversationComments, + unmatchedIgnoreGlobs, + carried, + ""); + } + /** * Back-compat constructor for callers that carry nothing from a superseded run. Defaults it to * {@link SupersededFindingsCarryover.Carried#NONE}, which reads as "no run of this pull request @@ -483,7 +545,7 @@ ReviewContext load( var configKeyContext = resolveConfigKeyContext(auth, req, reviewableFiles, ignoreGlobs); // Reuses the repo settings and the post-ignore file list already computed above: the coverage // artifact name comes from the same single read, and an ignored file is never reported as - // under-tested. + // under-tested. The section goes to the prompt; a refusal goes to the summary's scope note. var patchCoverage = patchCoverageResolver.resolve(auth, req, repoSettings, reviewableFiles); return new ReviewContext( @@ -505,13 +567,14 @@ ReviewContext load( projectStack, linkedIssuesContext, configKeyContext, - patchCoverage, + patchCoverage.section(), reviewableFiles, lineResolverSupplier, prTotals, conversationComments, unmatchedIgnoreGlobs, - carried); + carried, + patchCoverage.artifactRefusal()); } /** Thread-safe memoizing supplier — the resolver is built at most once per review context. */ diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/VerdictBuilder.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/VerdictBuilder.java index 1f735dc4..a6eac6d7 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/VerdictBuilder.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/VerdictBuilder.java @@ -250,6 +250,7 @@ ReviewResult build( ReviewDiffFormatter.formatPureRenameRollup( ReviewDiffFormatter.pureRenameFiles(ctx.files())), ReviewDiffFormatter.formatUnmatchedIgnoreGlobs(ctx.unmatchedIgnoreGlobs()), + PatchCoverageResolver.formatScopeNote(ctx.coverageArtifactRefusal()), SupersededFindingsCarryover.formatScopeNote(ctx.carried())), unresolvedPrevious, ciEvaluation, @@ -503,13 +504,15 @@ static List toChangedFiles( /** * Inputs that only shape the summary walkthrough: the file rows, the pure-rename rollup, the - * unmatched-ignore-glob note, and the superseded-run carry-over note. The notes share the - * review-scope blockquote — each answers "what did this review look at, or not, and why". + * unmatched-ignore-glob note, the refused-coverage-artifact note, and the superseded-run + * carry-over note. The notes share the review-scope blockquote — each answers "what did this + * review look at, or not, and why". */ private record SummaryInputs( List changedFiles, String pureRenameRollup, String unmatchedIgnoreGlobs, + String coverageArtifactNotRead, String carriedFromSupersededRun) {} ReviewResult buildResult( @@ -524,7 +527,7 @@ ReviewResult buildResult( aiResponse, isFirstReview, diffStats, - new SummaryInputs(changedFiles, "", "", ""), + new SummaryInputs(changedFiles, "", "", "", ""), unresolvedPrevious, ciEvaluation, backstopUnresolved); @@ -665,9 +668,10 @@ private ReviewResult buildResult( * blockquote, so a reader meets one scope caveat rather than competing banners. */ private static String reviewScopeNote(SummaryInputs summaryInputs) { - var notes = new ArrayList(3); + var notes = new ArrayList(4); addScopeNote(notes, summaryInputs.pureRenameRollup()); addScopeNote(notes, summaryInputs.unmatchedIgnoreGlobs()); + addScopeNote(notes, summaryInputs.coverageArtifactNotRead()); addScopeNote(notes, summaryInputs.carriedFromSupersededRun()); return String.join("\n>\n> ", notes); } diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/JacocoCoverageReportTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/JacocoCoverageReportTest.java index e232fd65..623e610f 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/JacocoCoverageReportTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/JacocoCoverageReportTest.java @@ -28,6 +28,11 @@ import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.function.Supplier; +import java.util.logging.Handler; +import java.util.logging.Level; +import java.util.logging.LogRecord; import java.util.zip.ZipEntry; import java.util.zip.ZipInputStream; import java.util.zip.ZipOutputStream; @@ -82,6 +87,57 @@ private static byte[] zipOf(Map entries) throws IOException { return bytes.toByteArray(); } + /** What {@code action} answered, and every line the reader logged while answering it. */ + private record Captured(T value, List lines) {} + + /** + * Runs {@code action} with the reader's log lines captured, formatted as the log manager would. + */ + private static Captured capturing(Supplier action) { + var records = new CopyOnWriteArrayList(); + var handler = + new Handler() { + @Override + public void publish(LogRecord logRecord) { + records.add(logRecord); + } + + @Override + public void flush() { + // Nothing buffered. + } + + @Override + public void close() { + // Nothing to release. + } + }; + var logger = + org.jboss.logmanager.LogContext.getLogContext() + .getLogger(JacocoCoverageReport.class.getName()); + var previousLevel = logger.getLevel(); + logger.addHandler(handler); + logger.setLevel(Level.ALL); + T value; + try { + value = action.get(); + } finally { + logger.removeHandler(handler); + logger.setLevel(previousLevel); + } + var lines = + records.stream() + .map( + r -> + r.getLevel() + + " " + + (r.getParameters() != null && r.getParameters().length > 0 + ? String.format(r.getMessage(), r.getParameters()) + : r.getMessage())) + .toList(); + return new Captured<>(value, lines); + } + @Nested class Parsing { @@ -321,6 +377,7 @@ void readsTheReportOutOfTheUploadedZip() throws IOException { "src/main/java/dev/thiagogonzaga/thrillhousebot/review/CiStatusEvaluator.java") .isEmpty(), "the report entry must be found regardless of its path inside the archive"); + assertNull(report.refusal(), "an archive read to the end has nothing to disclose"); } @Test @@ -346,12 +403,81 @@ void ignoresDirectoryEntriesAndStopsAtTheEntryCap() throws IOException { var padded = new LinkedHashMap(); for (var i = 0; i < JacocoCoverageReport.MAX_ZIP_ENTRIES + 5; i++) { - padded.put("pad" + i + ".txt", "x"); + padded.put("pad" + i + ".xml", ""); } padded.put("jacoco.xml", REPORT); assertTrue( JacocoCoverageReport.fromArtifactZip(zipOf(padded)).isEmpty(), - "the walk stops at the entry cap instead of reading an unbounded archive"); + "the walk stops at the entry cap instead of parsing an unbounded number of documents"); + } + + /** + * #813 — the usual coverage upload is the whole {@code target/site/jacoco/} tree: one {@code + * .html} per class plus the stylesheet, images and script, with {@code jacoco.xml} beside them. + * Charging every file to the entry cap refused that shape whole on a project of a few hundred + * classes, so the one report inside was never read. Only an entry that can carry a report + * counts; the rest is still drained against the aggregate budget. + */ + @Test + void readsTheReportOutOfAnHtmlSiteTreeLargerThanTheEntryCap() throws IOException { + var bytes = new ByteArrayOutputStream(); + try (var zip = new ZipOutputStream(bytes)) { + for (var i = 0; i < 600; i++) { + zip.putNextEntry(new ZipEntry("site/jacoco/dev.example/Class" + i + ".html")); + zip.write("".getBytes(StandardCharsets.UTF_8)); + zip.closeEntry(); + } + zip.putNextEntry(new ZipEntry("site/jacoco/jacoco.xml")); + zip.write(REPORT.getBytes(StandardCharsets.UTF_8)); + zip.closeEntry(); + } + + var report = JacocoCoverageReport.fromArtifactZip(bytes.toByteArray()); + + assertFalse( + report.isEmpty(), + "600 .html entries beside one jacoco.xml is the ordinary target/site/jacoco upload and" + + " must be read"); + assertNull(report.refusal(), "nothing was refused, so nothing is disclosed"); + } + + @Test + void refusesAnArchiveWithMoreXmlEntriesThanTheCapAndSaysSoAtWarn() throws IOException { + // One .xml past the cap: 512 surefire reports ahead of the one jacoco.xml, the shape a whole + // target/ upload takes. Every .xml costs a slot whether or not it is JaCoCo, because the cap + // bounds how many documents the merge parses — and the archive is refused whole, never + // merged as far as the cap allowed (#789). + var bytes = new ByteArrayOutputStream(); + try (var zip = new ZipOutputStream(bytes)) { + for (var i = 0; i < JacocoCoverageReport.MAX_ZIP_ENTRIES; i++) { + zip.putNextEntry(new ZipEntry("surefire-reports/TEST-Case" + i + ".xml")); + zip.write("".getBytes(StandardCharsets.UTF_8)); + zip.closeEntry(); + } + zip.putNextEntry(new ZipEntry("site/jacoco/jacoco.xml")); + zip.write(REPORT.getBytes(StandardCharsets.UTF_8)); + zip.closeEntry(); + } + + var captured = capturing(() -> JacocoCoverageReport.fromArtifactZip(bytes.toByteArray())); + + assertTrue( + captured.value().isEmpty(), + "513 .xml entries is past the cap whatever else the archive holds"); + assertEquals( + JacocoCoverageReport.Refusal.ENTRY_CAP, + captured.value().refusal(), + "the empty report says why, so the review can disclose it"); + assertTrue( + captured.lines().stream() + .anyMatch( + line -> + line.startsWith("WARN ") + && line.contains( + "more than " + JacocoCoverageReport.MAX_ZIP_ENTRIES + " .xml")), + "a refusal nobody runs production at DEBUG to see is a coverage section that went quiet" + + " for no written reason: " + + captured.lines()); } @Test @@ -393,9 +519,14 @@ void degradesToEmptyWhenTheArchiveIsTruncatedMidEntry() throws IOException { var whole = zipOf(Map.of("jacoco.xml", REPORT)); var truncated = java.util.Arrays.copyOf(whole, whole.length / 2); + var report = JacocoCoverageReport.fromArtifactZip(truncated); + assertTrue( - JacocoCoverageReport.fromArtifactZip(truncated).isEmpty(), - "a half-downloaded archive must degrade, not throw out of the review"); + report.isEmpty(), "a half-downloaded archive must degrade, not throw out of the review"); + assertEquals( + JacocoCoverageReport.Refusal.UNREADABLE, + report.refusal(), + "the artifact was there and could not be read, which the maintainer should hear"); } @Test @@ -428,10 +559,47 @@ void refusesAnArchiveThatInflatesPastTheAggregateCap() throws IOException { zip.closeEntry(); } + var captured = capturing(() -> JacocoCoverageReport.fromArtifactZip(bytes.toByteArray())); + assertTrue( - JacocoCoverageReport.fromArtifactZip(bytes.toByteArray()).isEmpty(), + captured.value().isEmpty(), "an archive inflating past the aggregate cap must be refused, not walked to the report" + " hidden behind the bomb"); + assertEquals( + JacocoCoverageReport.Refusal.INFLATION_BUDGET, + captured.value().refusal(), + "the bomb refusal is disclosed like the entry-cap refusal"); + assertTrue( + captured.lines().stream() + .anyMatch(line -> line.startsWith("WARN ") && line.contains("aggregate cap")), + "the bomb refusal is logged at WARN with counts only: " + captured.lines()); + } + + @Test + void refusesABombWearingAReportName() throws IOException { + // The same padding as above, named .xml: each entry costs a report slot and is collected for + // parsing, and the aggregate budget still has to refuse the archive from inside a report + // entry — the cap on report entries bounds how many documents are parsed, never how much any + // of them inflates. + var perEntry = JacocoCoverageReport.MAX_ENTRY_BYTES / 2; + var bytes = new ByteArrayOutputStream(); + try (var zip = new ZipOutputStream(bytes)) { + var zeros = new byte[64 * 1024]; + var inflated = 0L; + for (var i = 0; inflated <= JacocoCoverageReport.MAX_TOTAL_INFLATED_BYTES; i++) { + zip.putNextEntry(new ZipEntry("module" + i + "/jacoco.xml")); + for (var written = 0; written < perEntry; written += zeros.length) { + zip.write(zeros); + } + zip.closeEntry(); + inflated += perEntry; + } + } + + var report = JacocoCoverageReport.fromArtifactZip(bytes.toByteArray()); + + assertTrue(report.isEmpty(), "a bomb is a bomb whatever its entries are called"); + assertEquals(JacocoCoverageReport.Refusal.INFLATION_BUDGET, report.refusal()); } @Test @@ -494,15 +662,18 @@ void refusesAnArchiveWithMoreEntriesThanItWalks() throws IOException { zip.write(REPORT.getBytes(StandardCharsets.UTF_8)); zip.closeEntry(); for (var i = 0; i <= JacocoCoverageReport.MAX_ZIP_ENTRIES; i++) { - zip.putNextEntry(new ZipEntry("pad" + i + ".txt")); - zip.write(new byte[] {'x'}); + zip.putNextEntry(new ZipEntry("pad" + i + ".xml")); + zip.write("".getBytes(StandardCharsets.UTF_8)); zip.closeEntry(); } } + var report = JacocoCoverageReport.fromArtifactZip(bytes.toByteArray()); + assertTrue( - JacocoCoverageReport.fromArtifactZip(bytes.toByteArray()).isEmpty(), - "an archive longer than the entry cap yields EMPTY, not the prefix that fit"); + report.isEmpty(), + "an archive with more .xml entries than the cap yields EMPTY, not the prefix that fit"); + assertEquals(JacocoCoverageReport.Refusal.ENTRY_CAP, report.refusal()); } @Test @@ -515,15 +686,15 @@ void readsAnArchiveThatExactlyFillsTheEntryCap() throws IOException { zip.write(REPORT.getBytes(StandardCharsets.UTF_8)); zip.closeEntry(); for (var i = 0; i < JacocoCoverageReport.MAX_ZIP_ENTRIES - 1; i++) { - zip.putNextEntry(new ZipEntry("pad" + i + ".txt")); - zip.write(new byte[] {'x'}); + zip.putNextEntry(new ZipEntry("pad" + i + ".xml")); + zip.write("".getBytes(StandardCharsets.UTF_8)); zip.closeEntry(); } } assertFalse( JacocoCoverageReport.fromArtifactZip(bytes.toByteArray()).isEmpty(), - "an archive that fits inside the cap is still read"); + "an archive whose .xml entries exactly fill the cap is still read"); } @Test @@ -692,9 +863,11 @@ void degradesToEmptyWhenTheArchiveIsUnusable() throws IOException { JacocoCoverageReport.fromArtifactZip("this is not a zip".getBytes(StandardCharsets.UTF_8)) .isEmpty(), "a body that is not a zip must degrade, not throw"); - assertTrue( - JacocoCoverageReport.fromArtifactZip(zipOf(Map.of("README.md", "hi"))).isEmpty(), - "an archive with no XML entry carries no coverage"); + var noReport = JacocoCoverageReport.fromArtifactZip(zipOf(Map.of("README.md", "hi"))); + assertTrue(noReport.isEmpty(), "an archive with no XML entry carries no coverage"); + assertNull( + noReport.refusal(), + "nothing usable is the designed quiet path, not a refusal to disclose"); } } } diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/PatchCoverageResolverTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/PatchCoverageResolverTest.java index c0a7bf6f..6828cfa3 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/PatchCoverageResolverTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/PatchCoverageResolverTest.java @@ -137,8 +137,11 @@ private void givenDownloadRedirectsTo(URI location) { .thenReturn(response); } + /** The prompt section alone; the tests that care about a refusal read the whole resolution. */ private String resolve(boolean enabled, String artifact) { - return resolver(enabled).resolve("token", request(), settingsNaming(artifact), changedFile()); + return resolver(enabled) + .resolve("token", request(), settingsNaming(artifact), changedFile()) + .section(); } @Nested @@ -154,7 +157,7 @@ void contributesNothingAndCallsNoApiWhenTheDeploymentSwitchIsOff() { @Test void contributesNothingAndCallsNoApiWhenTheRepositoryNamesNoArtifact() { assertEquals( - "", + PatchCoverageResolver.Resolution.NONE, resolver(true).resolve("token", request(), RepoSettings.EMPTY, changedFile()), "the common case: a repository that declares nothing gets no coverage context"); @@ -164,7 +167,8 @@ void contributesNothingAndCallsNoApiWhenTheRepositoryNamesNoArtifact() { @Test void contributesNothingWhenThereAreNoReviewableFiles() { assertEquals( - "", resolver(true).resolve("token", request(), settingsNaming(ARTIFACT), List.of())); + PatchCoverageResolver.Resolution.NONE, + resolver(true).resolve("token", request(), settingsNaming(ARTIFACT), List.of())); verifyNoInteractions(actionsClient, zipFetcher); } @@ -180,11 +184,12 @@ void contributesNothingForABlankArtifactNameOrAnUnknownHeadSha() { new ReviewOrchestrator.ReviewRequest( "o", "r", 7, " ", "title", "body", "basesha", "main", 1L, false, "main", false); assertEquals( - "", + PatchCoverageResolver.Resolution.NONE, resolver(true).resolve("token", noSha, settingsNaming(ARTIFACT), changedFile()), "without a head SHA there is no revision to attribute coverage to"); assertEquals( - "", resolver(true).resolve("token", blankSha, settingsNaming(ARTIFACT), changedFile())); + PatchCoverageResolver.Resolution.NONE, + resolver(true).resolve("token", blankSha, settingsNaming(ARTIFACT), changedFile())); verifyNoInteractions(actionsClient, zipFetcher); } @@ -197,7 +202,7 @@ void theInjectionConstructorReadsTheDeploymentKillSwitch() { var injected = new PatchCoverageResolver(actionsClient, zipFetcher, config); assertEquals( - "", + PatchCoverageResolver.Resolution.NONE, injected.resolve("token", request(), settingsNaming(ARTIFACT), changedFile()), "the CDI constructor must wire thrillhousebot.review.patch-coverage.enabled"); verifyNoInteractions(actionsClient, zipFetcher); @@ -291,10 +296,45 @@ void degradesToNoContextWhenTheDownloadCannotBeFollowed() { givenRunWithArtifact(ARTIFACT); givenDownloadRedirectsTo(null); - assertEquals("", resolve(true, ARTIFACT)); + assertEquals( + PatchCoverageResolver.Resolution.NONE, + resolver(true).resolve("token", request(), settingsNaming(ARTIFACT), changedFile()), + "an artifact that was never downloaded was not refused, so there is nothing to" + + " disclose"); verifyNoInteractions(zipFetcher); } + /** + * #813 — an artifact that was found, downloaded and then refused by the reader is the one "no + * section" the maintainer must hear about: they configured it, the run uploaded it, and the + * review chose not to read it. The reason travels out beside the (empty) section. + */ + @Test + void disclosesAnArtifactTheReaderRefusedInsteadOfGoingQuiet() { + givenRunWithArtifact(ARTIFACT); + givenDownloadRedirectsTo(BLOB); + var bytes = new ByteArrayOutputStream(); + try (var zip = new ZipOutputStream(bytes)) { + for (var i = 0; i <= JacocoCoverageReport.MAX_ZIP_ENTRIES; i++) { + zip.putNextEntry(new ZipEntry("module" + i + "/jacoco.xml")); + zip.write(REPORT.getBytes(StandardCharsets.UTF_8)); + zip.closeEntry(); + } + } catch (IOException e) { + throw new UncheckedIOException(e); + } + when(zipFetcher.fetch(BLOB)).thenReturn(bytes.toByteArray()); + + var resolution = + resolver(true).resolve("token", request(), settingsNaming(ARTIFACT), changedFile()); + + assertEquals( + new PatchCoverageResolver.Resolution( + "", "it holds more than " + JacocoCoverageReport.MAX_ZIP_ENTRIES + " `.xml` entries"), + resolution, + "a refused artifact yields no section and says why"); + } + @Test void degradesToNoContextWhenTheApiFails() { when(actionsClient.listWorkflowRuns(any(), any(), any(), any(), any(), any(), anyInt())) @@ -431,6 +471,16 @@ void ignoresAnAtAtLineThatIsNotAHunkHeaderAndEmptyLines() { @Nested class Rendering { + @Test + void formatsARefusalAsAReviewScopeNoteAndNothingOtherwise() { + assertEquals("", PatchCoverageResolver.formatScopeNote(""), "nothing refused, no note"); + assertEquals("", PatchCoverageResolver.formatScopeNote(null), "a null reason is no note"); + assertEquals( + "the configured coverage artifact was not read: it holds more than 512 `.xml` entries", + PatchCoverageResolver.formatScopeNote("it holds more than 512 `.xml` entries"), + "the note names the artifact the repository configured and carries the reader's reason"); + } + @Test void collapsesConsecutiveLinesAndCapsTheRanges() { var lines = new TreeSet(List.of(1, 2, 3, 9, 20, 21)); diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewContextLoaderTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewContextLoaderTest.java index 0fbf9a3b..c5d4a824 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewContextLoaderTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewContextLoaderTest.java @@ -70,6 +70,10 @@ void setUp() { // Default: budgeting off so existing formatter-driven assertions keep the line-capped path. when(activeModel.maxInputTokens()).thenReturn(0); lenient().when(followUpAnalyzer.parsePreviousResponses(any())).thenReturn(List.of()); + // A bare mock would answer null for the resolution record; the quiet default is a real NONE. + lenient() + .when(patchCoverageResolver.resolve(any(), any(), any(), anyList())) + .thenReturn(PatchCoverageResolver.Resolution.NONE); loader = new ReviewContextLoader( prClient, @@ -639,15 +643,43 @@ void patchCoverageIsResolvedFromTheOneSettingsReadAndReachesTheContext() { var session = ReviewSession.create("owner/repo", 1, "Title", "headsha1"); session.id = 1L; when(patchCoverageResolver.resolve(eq("auth"), any(), eq(settings), anyList())) - .thenReturn("### uncovered"); + .thenReturn(new PatchCoverageResolver.Resolution("### uncovered", "")); var ctx = loader.load("auth", request(), session, "owner/repo"); assertEquals("### uncovered", ctx.patchCoverage()); + assertEquals("", ctx.coverageArtifactRefusal(), "a report that was read discloses nothing"); verify(patchCoverageResolver).resolve("auth", request(), settings, ctx.reviewableFiles()); assertFalse(ctx.reviewableFiles().contains(ignored)); verify(repoSettingsResolver, times(1)).resolve("owner", "repo", "main", 99L); } + + /** + * #813 — a coverage artifact the reader refused contributes no prompt section, and the reason + * rides the context to the summary's review-scope note, so the maintainer who configured the + * artifact learns it was not read instead of wondering why the coverage section went quiet. + */ + @Test + void aRefusedCoverageArtifactReachesTheContextForTheSummaryToDisclose() { + var files = + List.of( + new GitHubPullRequestClient.FileDiff( + "payments/Charge.java", "modified", 1, 0, 1, "@@ -1 +1 @@\n+a")); + stubCommonLoadDeps(files); + var settings = + new RepoSettings(List.of(), List.of(), "coverage-report", ".github/thrillhousebot.yml"); + when(repoSettingsResolver.resolve("owner", "repo", "main", 99L)).thenReturn(settings); + var session = ReviewSession.create("owner/repo", 1, "Title", "headsha1"); + session.id = 1L; + when(patchCoverageResolver.resolve(eq("auth"), any(), eq(settings), anyList())) + .thenReturn( + new PatchCoverageResolver.Resolution("", "it holds more than 512 `.xml` entries")); + + var ctx = loader.load("auth", request(), session, "owner/repo"); + + assertEquals("", ctx.patchCoverage(), "a refused artifact adds nothing to the prompt"); + assertEquals("it holds more than 512 `.xml` entries", ctx.coverageArtifactRefusal()); + } } @Nested diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestratorTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestratorTest.java index ef2b4f31..81659066 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestratorTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestratorTest.java @@ -262,7 +262,7 @@ private ReviewOrchestrator newOrchestrator() { followUpAnalyzer, new BugFixContextResolver(commentClient), new ConfigKeyContextResolver(prClient), - mock(PatchCoverageResolver.class), + quietPatchCoverage(), sessionPersistence, BOT_ID, new ActiveModelSettings(config, "m"), @@ -279,6 +279,17 @@ diffFormatter, new TokenCounter(), config, new ActiveModelSettings(config, "m")) reviewExecutor); } + /** + * A coverage resolver that finds nothing. Stubbed rather than bare: a bare mock answers null for + * the resolution record, and the loader reads its section and refusal on every review. + */ + private static PatchCoverageResolver quietPatchCoverage() { + var resolver = mock(PatchCoverageResolver.class); + when(resolver.resolve(any(), any(), any(), any())) + .thenReturn(PatchCoverageResolver.Resolution.NONE); + return resolver; + } + private DiffLineResolver resolverFor(GitHubPullRequestClient.FileDiff... files) { return new DiffLineResolver(diffFormatter.patchesByFile(List.of(files))); } diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/VerdictBuilderTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/VerdictBuilderTest.java index eeb64687..813c20a3 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/VerdictBuilderTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/VerdictBuilderTest.java @@ -1476,6 +1476,66 @@ void findingsCarriedFromASupersededRunAreDisclosedInTheReviewScopeNote() { result.summaryMarkdown()); } + /** + * #813 — a coverage artifact the reader refused is disclosed in the same review-scope blockquote + * as an ignore glob that matched nothing: the maintainer configured it, and the review did not + * read it. Without this the only trace was a log line. + */ + @Test + void aRefusedCoverageArtifactIsDisclosedInTheReviewScopeNote() { + var ctx = contextWithCoverageRefusal("it holds more than 512 `.xml` entries"); + var realSummaryBuilder = + new VerdictBuilder( + new PrSummaryGenerator(false), + followUpAnalyzer, + BotIdentity.from(List.of("thrillhousebot[bot]")), + BlockingStrictness.BALANCED); + + var result = realSummaryBuilder.build(ctx, CLEAN_RESPONSE, CI_CLEAR, FULL_COVERAGE); + + assertTrue( + result + .summaryMarkdown() + .startsWith( + PrSummaryGenerator.SUMMARY_HEADING + + "\n\n> **AI review scope:** the configured coverage artifact was not read:" + + " it holds more than 512 `.xml` entries\n\n"), + result.summaryMarkdown()); + } + + /** The three config-shaped notes are separate paragraphs of one blockquote, in a fixed order. */ + @Test + void everyScopeNoteSharesOneBlockquoteInAFixedOrder() { + var carried = + new SupersededFindingsCarryover.Carried( + "23e277100000000", + "c957198", + List.of(new ReviewResponse.Finding("high", "high", "a.java", 1, "t", "d", null, null))); + var ctx = scopedContext(List.of("payments/"), carried, "it could not be read as a zip archive"); + var realSummaryBuilder = + new VerdictBuilder( + new PrSummaryGenerator(false), + followUpAnalyzer, + BotIdentity.from(List.of("thrillhousebot[bot]")), + BlockingStrictness.BALANCED); + + var result = realSummaryBuilder.build(ctx, CLEAN_RESPONSE, CI_CLEAR, FULL_COVERAGE); + + assertTrue( + result + .summaryMarkdown() + .startsWith( + PrSummaryGenerator.SUMMARY_HEADING + + "\n\n> **AI review scope:** 1 ignore glob declared in this repository's" + + " ThrillhouseBot config matched no file in this pull request (`payments/`)" + + "\n>\n> the configured coverage artifact was not read: it could not be read" + + " as a zip archive" + + "\n>\n> 1 finding from the review of superseded head `23e2771`, abandoned" + + " when the pull request head moved, was carried into this review as" + + " previous findings and re-checked against the current head\n\n"), + result.summaryMarkdown()); + } + @Test void aReviewWhoseDeclaredGlobsAllMatchedCarriesNoScopeNote() { var realSummaryBuilder = @@ -1501,6 +1561,19 @@ private static ReviewContextLoader.ReviewContext contextWithUnmatchedGlobs( /** The same one-file context, also carrying a superseded run's findings (#806). */ private static ReviewContextLoader.ReviewContext contextCarrying( List unmatched, SupersededFindingsCarryover.Carried carried) { + return scopedContext(unmatched, carried, ""); + } + + /** The same one-file context, carrying only a refused coverage artifact's reason (#813). */ + private static ReviewContextLoader.ReviewContext contextWithCoverageRefusal(String refusal) { + return scopedContext(List.of(), SupersededFindingsCarryover.Carried.NONE, refusal); + } + + /** A one-file context carrying every input the review-scope blockquote is built from. */ + private static ReviewContextLoader.ReviewContext scopedContext( + List unmatched, + SupersededFindingsCarryover.Carried carried, + String coverageArtifactRefusal) { var changed = new FileDiff("src/Main.java", "modified", 1, 0, 1, "@@ -1 +1 @@\n+x"); return new ReviewContextLoader.ReviewContext( List.of(changed), @@ -1527,7 +1600,8 @@ private static ReviewContextLoader.ReviewContext contextCarrying( null, List.of(), unmatched, - carried); + carried, + coverageArtifactRefusal); } @Test From f769bbdb7accaf9c486f99f03daf8d76bbea334f Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Mon, 7 Sep 2026 10:12:12 -0300 Subject: [PATCH 2/3] docs(review): say what EMPTY is now that a refused archive carries its 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". --- .../thrillhousebot/review/JacocoCoverageReport.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/JacocoCoverageReport.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/JacocoCoverageReport.java index 7ed2062d..df293fbe 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/JacocoCoverageReport.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/JacocoCoverageReport.java @@ -108,7 +108,11 @@ final class JacocoCoverageReport { */ private static final String BINARY_PACKAGE_SEPARATOR = "/"; - /** No coverage data — the value every failure path degrades to. */ + /** + * No coverage data and nothing refused — what a read that found nothing usable degrades to. A + * refused archive degrades to its own empty report instead, carrying the {@link Refusal}, so + * {@code isEmpty()} rather than identity with this constant is the test for "no coverage". + */ static final JacocoCoverageReport EMPTY = new JacocoCoverageReport(Map.of(), null); /** From 4dfdbd4cabf8e742746ae677009aff254136001e Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Mon, 7 Sep 2026 10:42:46 -0300 Subject: [PATCH 3/3] docs(readme): keep the quiet path quiet in the coverage-artifact paragraph 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. --- README.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 7d4fa871..9fafb24a 100644 --- a/README.md +++ b/README.md @@ -755,7 +755,8 @@ Uploading the whole `target/site/jacoco/` tree, with the HTML report beside `jac is fine: only `.xml` entries count toward the archive's 512-entry cap. An artifact the bot found but refused to read — more `.xml` entries than that, or one that inflates past 128 MB — is named in the review summary's scope note the way an ignore glob that matched -nothing is, so a configured artifact never goes quiet without saying why. +nothing is. 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. The file is read from the repository's default branch on each review and cached for five minutes. YAML anchors, aliases and merge keys are resolved; a document that is