fix(review): ground the PR summary in the whole change set (#335) - #451
Merged
Conversation
The summary call of a large multi-call review never sees the diff: it gets the computed findings, the PR title/description, and a changed-file list the input budget may clamp. Nothing in SUMMARY_SYSTEM told the model to describe the whole change set, and nothing told it how big that set is — so pr_purpose could latch onto a single file and a full multi-collaborator decompose was summarized as "extracts CI-status evaluation". Prepend a deterministic PR-scope block to the changed-files section: the authoritative file/line totals (GitHub's, the same numbers the rendered Changes Overview reports) plus a per-directory breakdown, rendered ahead of the per-file rows so clamping can only drop the tail. SUMMARY_SYSTEM now grounds pr_purpose in the PR title/description together with those totals, forbids presenting one extracted class as the whole pull request, and treats a description narrower than the change as a description gap. The single-call path is untouched: that call sees the entire diff, so small single-purpose PRs keep their existing summary behavior. Refs #335
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
🤖 ThrillhouseBot PR SummaryWhat this PR doesThis change adds a deterministic PR scope block (file counts, line totals, per-directory breakdown) to the summary input, so the AI model can ground the 'What this PR does' description in the whole change set rather than just one file. It also updates the summary system/user prompts to instruct the model to cover the entire PR and flag when the description is narrower than the actual changes. Control-Flow Diagram🔀 Show diagramflowchart TD
A["changedFilesOverview()"] --> B{"Pure renames present?"}
B -->|yes| C["Append rename rollup"]
C --> D["changeScopeSummary(ctx)"]
B -->|no| D
D --> E{"ctx.prTotals() != null?"}
E -->|yes| F["Use GitHub totals for header"]
E -->|no| G["Sum additions/deletions from diff"]
F --> H["Build header: files changed, +A -D"]
G --> H
H --> I["appendDirectoryBreakdown"]
I --> J["Group by directory, sort"]
J --> K["List top 10, rollup rest"]
K --> L["Return scope block text"]
L --> M["Append 'Directories touched:' line"]
M --> N["Append per-file rows"]
Changes Overview
Changed Files
Risk Assessment
No new issues found in this PR, but the review cannot be approved until CI is confirmed green.
|
| Check | Type | Status | Detail |
|---|---|---|---|
| test | check-run | ⏳ Pending | - |
| format | check-run | ⏳ Pending | - |
| actionlint | check-run | ⏳ Pending | - |
| changes | check-run | ⏳ Pending | - |
| frontend | check-run | ⏳ Pending | - |
| trivy | check-run | ⏳ Pending | - |
| dependency-review | check-run | ⏳ Pending | - |
Automated review by ThrillhouseBot. Reply with /review to re-run.
thrillhousebot
Bot
added
bug
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
codecov/patch flagged six lines of the new scope block with no covering test: the empty-change-set guards and the path-with-no-directory branch. Cover them, and fix what the gap exposed. Totals reporting zero files are not usable — treat them like a failed fetch and fall back to the diff-derived counts, instead of announcing a zero-file PR above a non-empty file list. Drop the null check in directoryOf: the per-file loop above already probes the same name against an immutable set, so a null name throws there first and the guard could never run. Refs #335
![thrillhousebot[bot]](https://avatars.githubusercontent.com/in/3996985?s=60&v=4){kind=small}
|
devops-thiago
added a commit
that referenced
this pull request
Aug 8, 2026
Absorbs #449 (per-repo ignore patterns), #451 (whole-change-set PR summary), #453 (decline re-check) and four dependency bumps. Two textual conflicts, both from independent additions at the same insertion point rather than any disagreement: - ReviewContextLoader: #449's resolveIgnoreGlobs and this branch's resolveConfigKeyContext are separate private helpers that git could not place. Kept both. - FindingPipelineTest: #451 parameterized the reviewContext helper with an explicit reviewable-file list while this branch added the configKeyContext record component. Kept both — the helper's parameter, with "" in the new component's position. One silent breakage git merged cleanly: #453's new declinedRaceContext helper constructs a ReviewContext without configKeyContext. Filled in. The interaction between the two features is the one worth noting. #449 made load() compute reviewableFiles from the global globs unioned with the repo's own, and config-key resolution already read that post-filter list, so a key documented only in an ignored file is now correctly never resolved — and it inherits per-repo ignore rules for free. Pinned with a test that fails if the raw file list is ever passed instead.
18 tasks
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
Dogfooding turned up a summary that described a full multi-collaborator decompose (~1.6k LOC, 7 new collaborators) as "extracts CI-status evaluation" — the prose named one class while the diff was the whole refactor.
Diagnosis. On a large PR the summary comes from a separate model call (
AiReviewService.summarize→PrReviewer.summarizeStream, driven byPrReviewPrompts.SUMMARY_SYSTEM/SUMMARY_USER), and that call never sees the diff. The PR title and description were already reaching it through theprContextslot, so the gap was not the metadata — it was that:SUMMARY_SYSTEMtold the model to derivepr_purpose"from the changed files and findings" and never mentioned the author's stated intent or the size of the change; andclampOverviewmay truncate on exactly the large PRs where this matters.Fix.
FindingPipelinenow renders a deterministic PR-scope block at the head of the changed-files section:PR scope (whole pull request): N files changed, +A -D, then a per-directory breakdown (- src/main/java/...: 3 files (+460 -900)), capped at 10 directories with a rollup for the rest. The totals are GitHub's authoritative PR totals (ctx.prTotals(), the same numbers the rendered Changes Overview reports since fix(review): PR summary "Changes Overview" counts diverge from GitHub's file/line totals #298), falling back to the diff-derived counts when the fetch failed. It sits ahead of the per-file rows so budget clamping can only drop the tail, the same reason the pure-rename rollup leads.SUMMARY_SYSTEMnow requirespr_purposeto describe the WHOLE change set, grounded in the PR title/description and the scope totals, and explicitly forbids presenting one extracted class, one file, or the one component carrying findings as if it were the whole PR. It also states that a summary narrower than the stated PR scope is wrong and that few/no findings never means the change was small.description_gapsnow covers the qualitative case this issue is about: a description whose scope is narrower than the change itself.SUMMARY_USERrenames the file-list section to## PR scope and changed files (computed from the diff — authoritative)and states the purpose must account for all of it.Everything new is data derived from the diff and file list; the untrusted prose (title/body) keeps going through the existing escaped
prContextslot.Deliberately not implemented (both optional in the issue):
pr_purposeis prose that the prompt explicitly asks to "describe behavior, not a file listing", so a well-written summary mentions no file paths at all. A mention-count threshold would append "(partial — see walkthrough)" to correct summaries, which erodes trust in the other direction. The deterministic effort went into making the scope authoritative and unclampable instead.FindingVerifierPrompts.SYSTEM. That verifier audits individual candidate findings and runs per batch, before the summary exists — it has no summary to check. The equivalent signal is instead encoded where the summary is produced (description_gaps, already rendered byPrSummaryGenerator.appendDescriptionGapsas "No new config keys, so no README/
.env.examplechanges.Related Issues
Fixes #335
How Has This Been Tested?
Red/green validated per test: with the production change stashed (
git stash push -- src/main/java) and the tests kept, all seven new assertions fail; with it restored they pass.Red phase (
./mvnw -B test -Dtest='FindingPipelineTest,PrReviewPromptsContentTest'→Tests run: 47, Failures: 7):(The assertion message on the pipeline tests is the captured
SummaryInputs.changedFiles()value, i.e. the file list with no scope header.)Green phase: same command,
Tests run: 47, Failures: 0.Coverage of the acceptance criteria:
summaryOverviewStatesTheWholePrScopeForAMultiFileRefactordrives the multi-file fixture (5 files over 3 directories, title/body announcing the full scope) through the map-reduce path and asserts the summary call receives the totals, the per-directory breakdown, and that they precede the per-file rows. The prompt-side half is pinned by thePrReviewPromptsContentTestcases.summaryOverviewScopeUsesGitHubsAuthoritativeTotalsWhenAvailable(GitHub totals win over diff-derived counts) andsummaryOverviewRollsUpDirectoriesBeyondTheCap(the breakdown stays bounded).summaryOverviewScopeStaysSingularForASingleFilePrasserts a one-file PR renders singular scope text with no multi-file or multi-directory language, and the single-call review path (which is what small PRs use) is untouched.Also run:
./mvnw -B spotless:apply,./mvnw -B clean compile spotbugs:check spotless:check(BugInstance size is 0, BUILD SUCCESS),./mvnw -B clean test(full suite:Tests run: 1884, Failures: 0, Errors: 0, Skipped: 0).Checklist
Screenshots / Logs
The block the summary call now receives ahead of the file list, for a decompose-shaped PR:
Additional Notes
summaryFindingsJsonIsClampedToThePerCallBudgetcalibrates a per-call budget against the exact fixed prompt sections, so it needed its expected overview updated for the new scope block and slightly longer finding descriptions to keep the findings share dominant; its assertions are unchanged.