feat(review): feed diff patch coverage into the review context - #467
Merged
Conversation
The reviewer reasoned over the diff statically and took a green CI run at face value: nothing told it which of the CHANGED lines the test suite actually executes. PR #99 is the dogfood case — the new CI-gating path was under-covered and its one approve-path test passed only because a collaborator was left unmocked, so the suite stayed green while the headline bug was present. The bot never builds the pull request, so it cannot measure coverage itself and must be handed a report. It now reads one its CI already produced: a workflow artifact a repository names in .github/thrillhousebot.yml under review.coverage-artifact, taken from a completed workflow run for exactly the head commit under review. GitHub's head_sha filter is what makes the line numbers in the report and the line numbers in the diff describe the same revision; a report from a nearby commit would point at the wrong lines. The artifact's never-executed lines are intersected with the lines the diff adds — post ignore-filter, from the same single repo-settings read the rest of the review already does — and rendered as a bounded "uncovered changed lines" list. The prompt then uses it in both directions: changed logic nothing exercises is reportable in its own right, and the existing "a test in this diff exercises this path" self-check is explicitly inapplicable to a line measured as uncovered, so a correctness claim about it is not softened by a test that never runs it. Absence from the list is explicitly not evidence that a line IS covered. Off by default (REVIEW_PATCH_COVERAGE_ENABLED) and opt-in per repository. A repository that names no artifact — the common case — contributes nothing and its review is byte-for-byte what it was before; so does an expired artifact, a failed download, a report in another format, or an unreadable archive. Nothing is inferred from the diff. Refs #115
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
🤖 ThrillhouseBot PR SummaryWhat this PR doesThis change implements patch coverage integration: it obtains JaCoCo coverage report from a repository's CI artifact for the exact commit, intersects uncovered lines with diff-added lines, and injects a guidance section into the review prompt, enabling the reviewer to flag untested changed logic and preventing the in-diff-test self-check from suppressing findings on uncovered lines. Control-Flow Diagram🔀 Show diagramflowchart TD
A["ReviewContextLoader.load()"] --> B{"config enabled?"}
B -->|no| C["patchCoverage = ''"]
B -->|yes| D["PatchCoverageResolver.resolve()"]
D --> E{"artifact name present?"}
E -->|no| C
E -->|yes| F["loadReport()"]
F --> G["findArtifactId()"]
G --> H["listWorkflowRuns (single page)"]
G --> I["listRunArtifacts (single page)"]
H -->|found| J["download() -> ArtifactZipFetcher.fetch()"]
I -->|found| J
J --> K["JacocoCoverageReport.fromArtifactZip()"]
K --> L["parse() -> uncoveredLines map"]
L --> M["intersectWithAddedLines()"]
M --> N["render() -> prompt section"]
N --> O["ReviewPromptAssembler.patchCoverageSection()"]
O --> P["PATCH_COVERAGE_REQUEST + escaped list"]
Changes Overview
Changed Files
…and 8 more file(s). Risk Assessment
Things to double-check1 lower-confidence finding
|
| Check | Type | Status | Detail |
|---|---|---|---|
| trivy | check-run | ⏳ Pending | - |
| frontend | check-run | ⏳ Pending | - |
| format | check-run | ⏳ Pending | - |
| test | check-run | ⏳ Pending | - |
| build | check-run | ⏳ Pending | - |
| actionlint | check-run | ⏳ Pending | - |
| changes | check-run | ⏳ Pending | - |
| dependency-review | check-run | ⏳ Pending | - |
Automated review by ThrillhouseBot. Reply with /review to re-run.
![thrillhousebot[bot]](https://avatars.githubusercontent.com/in/3996985?s=60&v=4){kind=small}
There was a problem hiding this comment.
ThrillhouseBot noted 1 lower-confidence item(s) under Things to double-check in the PR summary (not posted as inline threads):
- LOW: Missing pagination for workflow runs list (
src/main/java/dev/thiagogonzaga/thrillhousebot/review/PatchCoverageResolver.java:131)
The call toactionsClient.listWorkflowRuns(...)infindArtifactIdfetches only the first page (30 runs) with no page walk. If the head commit has more than 30 completed workflow runs, runs beyond the first page are never seen, and the artifact search may miss a matching run even if it exists. The API defaults to 30 per page and sorts most-recent first, so the risk is low, but the design should either paginate until a short page or document why one page suffices. Additionally, the subsequentlistRunArtifactscall inside the loop also fetches a single page (100 items) without pagination, though a run seldom exceeds that limit. Both calls contribute to the same sourcing path.
thrillhousebot
Bot
added
enhancement
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Every bound and degrade path the feature's safety rests on is now exercised: the render/range/entry/line/source-file caps, the malformed and truncated archive branches, the empty-intersection and unmeasured-file cases, the transfer's status handling and size bound, and the interrupt and absent-proxy-selector edges. New-code line coverage is 100%. One test passed for the wrong reason and was rewritten: the expired-artifact case left the download unstubbed, so ignoring 'expired' still produced no section — the same unmocked-collaborator failure this feature exists to surface. It now wires a readable report behind the download, so only the expiry check can produce an empty result. The transfer is split from fetch's https-only policy so the bytes-on-the-wire behaviour can be driven by a loopback server without a TLS endpoint; the policy itself is still tested through fetch, the only entry point production uses. The unreachable null check on the artifact name is deleted rather than left uncovered: RepoSettings normalizes it. Sonar: unnamed catch patterns, and the three break/continue loops rewritten into the shapes that read better (a declarative run filter, an entry-driven zip walk, an if/else-if patch walk). The hard-coded '/' in the JaCoCo path join is kept and documented — a package name is a JVM binary name, which is '/'-separated on every platform, and it is matched against git paths, which are too; File.separator there would break every report built on Windows. Refs #115
…tch-coverage-context
thrillhousebot
Bot
added
the
documentation
They were one behaviour over three fixtures — identical download wiring,
only the report differing — so Sonar's S5976 is right that they read better
as one parameterized test. The label is the case and the reason travels with
it into the failure message, which is more informative on failure than the
old method names were. Matches the house idiom: the repo's existing
@ParameterizedTest(name = "{0}") sources report the same way.
Discrimination is unchanged, verified by re-running the mutations these
tests are responsible for: making the intersection ignore the diff's added
lines is still killed by the outside-the-diff case, and removing the
empty-intersection early return is still killed by the unmeasured-file and
outside-the-diff cases — the same two kills, from the same two fixtures, as
before the refactor.
Refs #115
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: listWorkflowRuns does not paginate, may miss artifact-hosting runs (
src/main/java/dev/thiagogonzaga/thrillhousebot/review/PatchCoverageResolver.java:198)
The call tolistWorkflowRunsinfindArtifactIduses a fixedper_page(30) but does not iterate through additional pages. GitHub Actions API lists workflow runs sorted by creation date descending, so the most recent 30 runs are returned. If there are more than 30 completed runs for the head SHA, runs beyond the first page are never examined, potentially missing the artifact that holds the coverage report. The feature is designed to be best-effort, and missing the artifact in this edge case is acceptable, but the code should document why a single page is sufficient (the artifact is expected to be among the most recent runs for the commit). The same concern applies tolistRunArtifactswithper_page100, though the likelihood is lower. Without pagination or a justification comment, the truncation is a silent gap.
A review of this branch flagged the unpaginated listWorkflowRuns call, and it was right to: the repository's own guidance singles out single-page list calls, and a reader could not tell from the code whether one page here was considered or overlooked. It is considered. GitHub applies head_sha as a query parameter, so the page holds only the runs for the commit under review rather than recent repository activity, and exceeding the page size would take that many completed workflows or re-runs on one SHA. When it happens the cost is bounded and already the designed degrade path — the artifact is not found, no coverage section is produced, and the review proceeds as it does for every repository that publishes no report. The artifact lookup has the same justification with more headroom: it is scoped to one run and asks for GitHub's maximum page size. No behaviour change; the reasoning now lives at both call sites, with the page-size constant pointing at it. Refs #115
There was a problem hiding this comment.
ThrillhouseBot found no issues in this PR, but some checks are still pending or failed:
- Check test is pending
- Check format is pending
- Check trivy is pending
- Check actionlint is pending
- Check frontend is pending
- Check changes is pending
- Check build is pending
- Check dependency-review is pending
Additionally, No new issues in this revision, but 1 previous finding(s) remain unresolved — fix them, or reply on their review thread (where one exists) with why they are deferred.
|
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
The reviewer never knew whether the changed code is actually exercised by a test. It
reasoned over the diff statically and took a green CI run at face value. PR #99 is the
dogfood case: the new CI-gating path in
evaluateCiCheckswas under-covered, and its oneapprove-path test passed only because a collaborator (
prClient.getPullRequest) was leftunmocked, returned
null, and skipped the new gate entirely — so the suite was green whilethe headline bug ("a PR with green CI is never approved") was present.
Where the coverage data comes from. The bot never builds the pull request, so it cannot
measure coverage itself and has to be handed a report. It reads one the repository's CI
already produced: a workflow artifact the repository names in
.github/thrillhousebot.ymlunder
review.coverage-artifact, attached to a completed workflow run for exactly the headcommit under review. That constraint is the whole point — GitHub filters runs by
head_sha,so the line numbers in the report and the line numbers in the diff describe the same revision.
The alternatives were considered and rejected: a report committed in the tree is stale by
construction (CI commits it after the commit it measured, so a review of the newer head
reads the older revision's numbers), and a coverage service's PR comment couples the bot to a
third party's rendering. The Actions route needs no new GitHub App permission —
Actions: Readis already declared.
What happens when a repository provides nothing — the common case. Nothing. No fallback,
no guess, no heuristic. A repository that declares no artifact name, whose run uploaded nothing
by that name, whose artifact expired, whose download failed, or whose report is not JaCoCo XML
contributes no section at all, and its review is byte-for-byte what it is today. This feature
is inert for most repositories by design; an invented coverage signal would be worse than none,
because the prompt tells the reviewer to stop softening claims on the strength of it.
How the prompt uses it. A new
## Patch Coverage Checktrailing-guidance block ships onlyalongside the data, and says both halves:
by what the untested code decides, one finding per untested block, not per line);
existing
SYSTEMself-check ("if a test in this same diff exercises the code path you claim isbroken…") is now explicitly inapplicable to a measured-uncovered line: no test runs it, so
none can be asked to explain the claim away.
The inverse inference is refused just as explicitly: a line's absence from the list is never
evidence that a test covers it (a file the report does not mention may simply not be measured).
Bounds. 12 files rendered, 12 line ranges per file, 2000 characters total, 10 workflow runs
probed per review, 16 MB artifact cap, 512 archive entries, 5000 source files / 5000 lines per
file from the report. The list is escaped through
PromptTemplateEscaperand framed as data likeevery other untrusted slot. Files the ignore list already excluded are never reported as
under-tested — patch coverage rides the same single repo-settings read and the same post-ignore
file list the rest of the review already computed, adding no second settings fetch.
Off by default (
REVIEW_PATCH_COVERAGE_ENABLED=false) and opt-in per repository, so it costsnothing until an operator and a repository both ask for it.
Files
github/GitHubActionsClient.javahead_sha, run artifacts, artifact-zip download (returns the rawResponseso the redirect can be handled)github/ArtifactZipFetcher.javaapi.github.com— https-only, bounded, fail-softreview/JacocoCoverageReport.javareview/PatchCoverageResolver.javagithub/RepoSettings(.Parser).javareview.coverage-artifactcomponent + parsing; every unusable shape leaves coverage offconfig/ThrillhouseConfig.javathrillhousebot.review.patch-coverage.enabled, defaultfalsereview/ReviewContextLoader.javapatchCoveragecontext componentreview/ReviewPromptAssembler.javareview/ai/PrReviewPrompts.javaPATCH_COVERAGE_REQUEST; the in-diff-test self-check made inapplicable to an uncovered lineREADME.md,.env.example,application.propertiescoverage-artifactsection under Repository configuration, including theupload-artifactsnippet a repo needsevalcorpus/pr99-uncovered-ci-gate-must-find/patchCoverageslot onEvalCase.Specand its injection inPromptEvalTestRelated Issues
Fixes #115
How Has This Been Tested?
Full suite on the merged tree (this branch merged with
release/v0.6.0atd53d438, whichcarries #463 and #465): 2296 tests, 0 failures, 0 SpotBugs, Spotless clean.
Coverage of the new code
Measured from
target/site/jacoco/jacoco.xmlon the merged tree, not from a dashboard:PatchCoverageResolver.javaJacocoCoverageReport.javaArtifactZipFetcher.javaRepoSettings.java/RepoSettingsParser.java/GitHubActionsClient.javaThe tests that closed the gap are on the paths that make the feature safe, not on whatever
raised the number: every cap (render/ranges/total chars, per-file lines, source files, zip
entries, runs probed, download size), and every degrade path (truncated archive, directory
entry, non-JaCoCo XML entry, unparseable/absent/non-positive line counters, blank sourcefile
name, nameless
packageelement, default package, unopenable document, close failure,unsupported hardening property, null runs/artifacts payloads, a download with no redirect, a
report measuring none of the changed files, uncovered lines entirely outside the diff).
One line was deleted rather than covered:
artifactName == null ||inresolvewasunreachable, because
RepoSettings' compact constructor normalizes a null artifact name to"".The single remaining partial branch is
ArtifactZipFetcher:96— javac's synthetictry-with-resources close path on
try (var body = response.body()). It is compiler-generated,not written here, and unreachable because
response.body()is never null.A test that passed for the wrong reason
Mutation testing caught one, and it is the same failure mode this feature exists to surface.
ignoresAnArtifactWithAnotherNameOrAnExpiredOneasserted an empty result, but with!artifact.expired()deleted it still passed: the expired artifact was accepted, thedownload ran,
zipFetcherwas unstubbed, Mockito returnednull, and the report came backempty. Green for lack of a mock — exactly PR #99, reproduced inside this feature's own suite.
It is now split into two tests that each wire a readable report behind the download, so an
empty result can only come from the check under test. The mutant now fails:
GitHub has already deleted an expired artifact ==> expected: <> but was: <### Patch coverage for this diff …>Mutation testing — 15 mutations, 15 killed
Each production behavior was neutralized one at a time, the covering test confirmed red, then
restored and confirmed green. Verbatim red-phase failures:
recordLineno longer requiresci == 0JacocoCoverageReportTest.reportsOnlyExecutableLinesWithZeroHitsonly ci=0: a fully covered line, a partially covered one (mi>0 but ci>0), and a non-executable line (ci=mi=0) are all excluded ==> expected: <[13, 14]> but was: <[13, 14, 16]>PatchCoverageResolverTest.ignoresAnAtAtLineThatIsNotAHunkHeaderAndEmptyLinesan unparseable @@ line must not reset the counter, and a bare empty line is skipped ==> expected: <[5, 7]> but was: …head_shaprovenance filter removedPatchCoverageResolverTest.ignoresRunsForAnotherCommitcoverage from a different revision would point at meaningless line numbers ==> expected: <> but was: <### Patch coverage …>PatchCoverageResolverTest.probesOnlyABoundedNumberOfRunsForTheSameCommitlistRunArtifacts, got 14PatchCoverageResolverTest.ignoresAnExpiredArtifactEvenThoughItsBytesWouldStillParseGitHub has already deleted an expired artifact ==> expected: <> but was: <### Patch coverage …>aReadableReportWithNoUncoveredAddedLinesProducesNoSection[2]pre-existing untested code is not this pull request's businessaReadableReportWithNoUncoveredAddedLinesProducesNoSection[1]and[2]a readable report that measures none of the changed files says nothing about themPatchCoverageResolverTest.truncatesASectionThatWouldRivalTheDiffJacocoCoverageReportTest.capsHowManyLinesOneSourceFileMayContributea pathological report cannot make one file's line list unbounded ==> expected: <5000> but was: <5050>JacocoCoverageReportTest.capsHowManySourceFilesOneReportMayContributefiles past the cap are dropped rather than growing the map without bound ==> expected: <true> but was: <false>JacocoCoverageReportTest.ignoresDirectoryEntriesAndStopsAtTheEntryCapthe walk stops at the entry cap instead of reading an unbounded archive ==> expected: <true> but was: <false>JacocoCoverageReportTest.degradesToEmptyWhenTheArchiveIsTruncatedMidEntry» IllegalState java.io.EOFException: Unexpected end of ZLIB input streamJacocoCoverageReportTest.ignoresLineElementsThatCarryNoUsableNumbersa non-positive, unparseable, or missing-counter line is dropped, not guessed at ==> expected: <[6]> but was: <[]>ArtifactZipFetcherTest.rejectsABodyLargerThanTheCapInsteadOfTruncatingIthalf a zip is not a coverage report; an over-long body is dropped, not truncated ==> expected: <0> but was: <16777217>ArtifactZipFetcherTest.degradesToNothingOnAnErrorStatusan expired or revoked signed URL yields no coverage ==> expected: <0> but was: <4>ArtifactZipFetcherTest.restoresTheInterruptFlagAndDegradesWhenTheDownloadIsInterruptedswallowing the InterruptedException without restoring the flag would hide the shutdown signal from the review threadArtifactZipFetcherTest.worksWhenTheJvmHasNoDefaultProxySelector» NullPointerGitHubActionsClientTest.anAbsentRunListReadsAsNoRuns» NullPointer Cannot invoke "java.util.Collection.isEmpty()" because "coll" is nullReviewPromptAssemblerTest.guidanceAndDataReachTheModelTogetherexpected: <true> but was: <false>(2 failures)PrReviewPromptsContentTest.inDiffTestSelfCheckDoesNotSuppressAClaimAboutAnUncoveredLinemissing marker: "INAPPLICABLE to a line a provided patch-coverage section lists as"readCoverageArtifactstops rejecting a YAML nullRepoSettingsResolverTest.leavesCoverageOffForEveryUnusableShapean explicit YAML null, whose asText() is the literal "null" ==> expected: <> but was: <null>""instead of the resolver resultReviewContextLoaderTest.patchCoverageIsResolvedFromTheOneSettingsReadAndReachesTheContextexpected: <### uncovered> but was: <>JacocoCoverageReportTest(4 tests)the report entry must be found regardless of its path inside the archive ==> expected: <false> but was: <true>— with DTD support on, every real JaCoCo report fails to parse, because the parser goes looking for thereport.dtdits DOCTYPE namesChecklist
Additional Notes
Review-findings ledger
Every finding raised against this branch, by any reviewer, and what became of it. Recorded here
so a later reader can see the reasoning for each rather than re-deriving it — particularly for
the two that were deliberately not "fixed".
S7467× 3 — unnamed catch patternRepoSettingsResolver.S135× 3 — too manybreak/continuefindArtifactIdnow states its policy declaratively (.filter(sameHeadSha).limit(MAX_RUNS_PROBED)) with the artifact lookup extracted; the zip walk is entry-driven;addedLinesis anif/else ifchain. No flag variables introduced.S5976— parameterize 3 repeated tests@ParameterizedTest(name = "{0}")with(label, report, why). Nothing lost: the per-case reason now reaches the failure message, and surefire numbers the cases[1] [2] [3]exactly as the repo's existingname = "{0}"sources already do. Discrimination re-verified by mutation after the refactor — same kills, same fixtures, as before.S1075— hard-coded path delimiter,JacocoCoverageReportpackage nameis the JVM internal binary name, which the class-file format defines as/-separated on every platform including Windows; it is joined here to build a path matched against git diff paths, which are also always/. Both sides are/-separated protocol strings, not filesystem paths.File.separatorthere would be the actual bug — it would break every report produced by a Windows runner. Sonar will re-report this on every analysis; that is the expected cost of the correct decision.listWorkflowRunsdoes not paginate, may miss artifact-hosting runshead_shais a server-side query parameter, so the page holds only the runs for the one commit under review, never recent repository activity — exceedingRUNS_PER_PAGEtakes that many completed workflows or re-runs on a single SHA. But the finding is right that the reasoning was invisible, and this repository's own guidance singles out unpaginated list calls precisely so they get justified rather than assumed. A comment at the call site now records thehead_shascoping and what the cap costs in the pathological case: the artifact is simply not found, no section is produced, and the review proceeds exactly as it does for every repository that publishes no report — the feature's designed degrade path.listRunArtifactscarries the same note with more headroom (scoped to one run, at GitHub's maximum page size).One mutant in this area is equivalent and deliberately not chased: deleting the
reportedUncovered.isEmpty()skip inintersectWithAddedLineschanges no observable behaviour,because a file with no reported-uncovered lines yields an empty intersection either way. It is a
short-circuit, not a gate.
Remaining limits, stated plainly
transfer(URI)is split out offetch(URI)so the bytes-on-the-wire behaviour (status handling, size bound, redirect following, failure
degradation, interrupt, absent proxy selector) runs against a loopback
HttpServerwithoutneeding a live endpoint.
fetchkeeps the https-only policy and remains the only entry pointproduction uses, so nothing is weakened — non-https is still refused.
anUploadedReportCannotSmuggleFileContentIntoASourcePathpins the observable property, but theJDK's StAX also refuses an external entity inside an attribute value on its own, so the
assertion survives flipping the hardening flags. The test carries a comment saying so. The
hardening is still load-bearing for a different reason — see the DTD row above.
-Pevalneeds a live provider key. The fixture isstructurally validated by
EvalCorpusTest(which runs in every build) and the runner injectionmirrors
ReviewPromptAssembler, but whether the model acts on the guidance has not beenmeasured.
Only JaCoCo XML is understood today; another format in the named artifact yields no section
rather than a guess.
RepoSettingsgained a fourth component — a 3-arg convenience constructorkeeps the existing call sites compiling, and the merged tree with #463's call sites builds green.
Two observations for the maintainer — not blockers on this PR
1. The codebase now demonstrates two patterns for one shape, with no precedent for the next
person. This PR and #465 both add "a rule that applies only when an optional context section is
present", and they solve it differently:
PrReviewPrompts.SYSTEMas review dimension 10, self-gated byprose (
… AND a "Config key definitions from the repository" section supplies that key's definition). Consequence: ~18 lines of prompt are spent on every review, including theoverwhelming majority that carry no config-key section.
data is present — the pattern feat(review): pull implementation behind config keys named in doc/.env diffs into review context #108 established, which is also the feature that supplies feat(review): report config-key documentation that is correct but incomplete (#109) #465's
data. Consequence: zero tokens when there is no coverage report, which is the common case.
I am explicitly not proposing to change #465 — it is out of scope here and would only create
a needless conflict. The point is that the next person adding a context-gated rule has two
in-tree precedents that disagree, and picking between them should be a deliberate decision rather
than a coin flip.
2.
SYSTEM's dimension list is drifting toward a list of special cases — and this PR is notwhat is doing it. Dimensions 1–5 are short and general (one to ten lines each, ~15 lines in
total). Dimensions 6–10 are long, conditional and dogfood-specific (thirteen to twenty lines
each), with the two most recent being the longest. That is a trend worth watching rather than a
defect in any single dimension, each of which was well justified on its own.
This PR adds no new dimension: its only
SYSTEMedit is a five-line clause that narrows anexisting self-check (making it inapplicable to a measured-uncovered line), and its actual guidance
lives outside
SYSTEMentirely. Reported as a trend, with the note that the next dimensionproposed for that list is where I would push back and ask whether it belongs in the
trailing-guidance slot instead.