feat(review): trace a change's data structure from producer to consumer (#117) - #458
Merged
Merged
Conversation
The reviewer judges hunks locally, so a producer/consumer contract mismatch that spans two individually-valid hunks slips through. PR #99 is the dogfood case: evaluateCiChecks appended every check it walked, including the ones it had just classified "success"; the caller bound the result to offendingCiChecks; and buildResult downgraded APPROVE on !offendingCiChecks.isEmpty(). Each hunk read fine on its own, and the result was that no PR with green CI could ever be approved — the inverse of what the PR said it did. Adds review dimension 9 to the generator system prompt: name where the change's primary data structure is PRODUCED and where it is CONSUMED, then check the consumer's assumption against what the producer puts in it. Three concrete checks — a predicate-named value (offending/invalid/failed/missing) must hold only items satisfying that predicate; a collection gated on isEmpty()/size()/anyMatch must hold only gate-worthy entries; and the end-to-end behavior must match the PR title/description, which routes an inverted trace into summary.description_gaps as well as a finding. Precision is guarded from both sides: the dimension is scoped to the change's primary structure and stays silent when producer and consumer agree, and a new self-check bullet invalidates a contract claim that cannot quote both ends and name the case on which they disagree. Two generator cases are added to the eval corpus from PR #99 — the buggy revision (must-find) and the shipped fix with the contract intact (must-not-find) — so the rule is measured for recall and for false positives on the same code. Refs #117
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
🤖 ThrillhouseBot PR SummaryWhat this PR doesAdds review dimension 9 (PRODUCER → CONSUMER CONTRACT) to the AI generator prompt, instructing the model to trace a change's primary data structure from its producer to its consumer and flag mismatches. Integrates inverted-trace detection into the summary's description_gaps field. Adds eval corpus fixtures and content-anchor tests to guard the new prompt guidance. 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 |
|---|---|---|---|
| format | check-run | ⏳ Pending | - |
| test | check-run | ⏳ Pending | - |
| frontend | check-run | ⏳ Pending | - |
| actionlint | check-run | ⏳ Pending | - |
| trivy | check-run | ⏳ Pending | - |
| changes | check-run | ⏳ Pending | - |
| dependency-review | check-run | ⏳ Pending | - |
Automated review by ThrillhouseBot. Reply with /review to re-run.
thrillhousebot
Bot
added
enhancement
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
18 tasks
devops-thiago
added a commit
that referenced
this pull request
Aug 9, 2026
…463) ## What type of PR is this? - [ ] 🐛 Bug fix - [x] ✨ Feature - [x] 📝 Documentation - [x] 🔧 Refactor - [ ] 🚀 Performance - [ ] ✅ Test - [ ] 🔒 Security - [ ] 📦 Dependency update - [ ] 🏗️ CI/CD ## Description `max-diff-lines` predates token budgeting and had become a second, cruder ceiling sitting in front of it. The review path stopped using it when map-reduce (#53) landed, and `/improve` was moved onto that in #316 — but `/describe` and `/changelog` still shrank a large PR to the first `max-diff-lines` of its rendered diff. A description was therefore written from a partial diff, and a CHANGELOG entry drafted from one, with whole files never reaching the model at all. **The seam.** The three methods `/improve` proved — batch planning, the shared prompt overhead, and the per-repo ignore re-filter — are lifted into `AbstractPrSuggestionGenerator`, parameterised by each command's own prompt constants. `/improve` is refactored onto the lifted versions rather than keeping a private copy. Every on-request suggestion command now plans batches over the reviewable **file list** under the per-call token budget and makes one call per batch. One call: ```java var plan = planBatches(reviewable, inputs, ownSystemPrompt, ownUserPrompt, reservedCalls); ``` **The reduce step is per-command, because the reductions genuinely differ.** Batching is only the map step: | Command | Reduce | Extra AI call? | |---|---|---| | `/describe` | Per-batch partial descriptions **synthesized** into one coherent title + description | Yes — reserved, spent only when >1 batch | | `/changelog` | Per-batch candidate entries **merged** into one entry | Yes — reserved, spent only when >1 candidate | | `/improve` | Local union of per-batch suggestions, deduped by `file:line` | No | | `/generate-tests` | Local union of per-batch test files, deduped by path | No | Nothing is concatenated. Stapling `/describe`'s partials together repeats the overview once per part and reads as several pull requests; for `/changelog`, a deterministic merge could unify headings and drop identical bullets, but the duplicates that actually arise are *not* identical — two batches that saw different files of one feature describe that change in two different sentences, which only a reader that understands them can collapse. Both reduce calls are reserved out of `max-ai-calls` up front, the same way the review path reserves one for its summary, so a run never exceeds the ceiling of one review; a single-batch PR still costs exactly one call. **Also in this change** - The shared overhead is assembled from each command's **own** prompts. Sizing a batch against another command's prompts would let an "in-budget" batch overshoot the real input limit. - Coverage disclosure now comes from `BudgetPlan.omittedFiles()` / `clippedFiles()` — files **named**, not counted — keeping #296's wording. `Inputs.omittedFiles` (the line-cap count) is gone, so nothing can reach for the wrong number. - Coverage on a huge PR is bounded by `max-ai-calls`, not the file list. Files that never got a batch are named. When *no* file fit any batch at all, every command says so and names the files rather than going silent — a misconfigured budget must not look like a bot that ignored the command. An empty plan that omitted *nothing*, because the repository ignores every changed file, is the opposite case and posts nothing. - Per-repo ignore patterns (#449) are applied on top of the global set for every command, and the filtered list stays authoritative for everything downstream — batches *and* the line map alike. That is the bug #452's audit found; the lifted method carries the invariant in its javadoc. - `max-input-tokens <= 0` keeps budgeting off as a single uncapped batch rather than regressing to the line-capped string. - A batch whose model call fails is skipped rather than failing the run, and the shortfall is disclosed. - Sizing callers reach the prompts through `systemPrompt()` / `userPrompt()` accessors: a reference to a `static final String` is inlined into the caller's class file at compile time, and a third copy of a multi-kilobyte prompt trips SpotBugs' `HSC_HUGE_SHARED_STRING_CONSTANT`. The accessors are deliberately *not* named `system()` / `user()` — differing from the constant only by capitalization reads as a typo at the call site. **`/add-docs` is deliberately out of scope.** It does not extend `AbstractPrSuggestionGenerator`; it orders its loading around a hard head-SHA precondition (every output is an inline suggestion, so no head SHA means nothing postable and a distinct user-facing message), and it feeds its assistant a different input set — project stack, a combined `PromptSections.prContext(...)` block, and a pre-rendered instructions section built from `ResolvedInstructions` rather than the content string the shared `Inputs` carries. Folding it in therefore means changing the shared `Inputs` contract at the same time as first lifting the seam, on the command that posts committable edits. It is worth doing and should be tracked separately; `/add-docs` remains line-capped and the README now says so precisely. Note it also still has **no per-repo ignore filter at all**, which is worth carrying into that follow-up. **Path-scoped instructions (#460) are not part of what a command batch carries.** `PathScopedInstructions` is resolved only by `ReviewContextLoader` and rendered only into `ReviewPromptAssembler`'s trailing-guidance slot, so it reaches the review prompt and nothing else. The `repoInstructions` slot of the batched commands is fed solely by `InstructionsResolver.resolve(...).content()` — the global instructions file — which `sharedPromptOverhead(...)` already counts in full. No batch is mis-sized by the scoped rules. ## `/generate-tests` is migrated onto the seam in this PR #461 merged before this one, so `/generate-tests` landed on `release/v0.6.0` still line-capped and still reading `Inputs.omittedFiles`, which this PR removes. The migration #461's agent was going to perform *after* this merged is therefore done **here** — there is no "later", because without it base does not compile. `UnitTestGenerator` now: 1. resolves its effective file list once via `respectPerRepoIgnores(...)` and plans from it, 2. plans token-budgeted batches and sends `batch.text()` per batch, 3. discloses coverage from `disclosure(plan)`, 4. reserves **0** calls — its reduce is a local union, so the whole `max-ai-calls` allowance buys batches. **Two things this surfaced that are worth reading closely.** **The shared overhead was not sufficient for this command, and using it unchanged would have been a real bug.** `sharedPromptOverhead(...)` counts system + user + fence + title + body + instructions. `/generate-tests` also sends the resolved **project stack** on every call — dependency manifests, kilobytes, not a rounding error — so the estimate would have undercounted every batch by the size of the stack and let "in-budget" batches overshoot the model's real input limit. That is precisely the failure the overhead exists to prevent. `planBatches(...)` therefore gains a six-argument form taking the command's own extra per-call sections, and `/generate-tests` declares the stack there. A future adopter with its own extra section must do the same rather than reach for the five-argument form. **It uses its own prompt templates, not the shared ones.** `UnitTestAssistant` is annotated with `UnitTestAssistantPrompts.SYSTEM` / `UnitTestAssistantPrompts.USER` — it does **not** share `PrSuggestionPrompts.USER`, because its user template carries the project-stack section. Sizing its batches against the shared user template would measure the wrong prompt. `UnitTestAssistantPrompts` had no accessors (it is new from #461), so `systemPrompt()` / `userPrompt()` are added to it — SpotBugs failed the build without them, exactly as the accessor javadoc predicts. **Why dedupe by path rather than merge.** Batches partition the file list, so two batches usually propose disjoint test paths. When they do collide, each proposal's `code` is a *complete* compilable file — package, imports and fixtures included, posted verbatim to paste at that path — so two of them at one path are alternatives, not additions. Rendering both would invite pasting the second over the first and silently losing the first's cases, and merging them properly would need a model call for a rare collision. The first wins and the rest are counted in a disclosure line, so the maintainer can re-run for the others. ## Related Issues Fixes #457 ## How Has This Been Tested? - [x] Unit tests - [ ] Integration tests - [ ] Manual testing Every new behavior was validated red/green: the test was written, the production change was mutated to neutralize exactly that behavior, the test was confirmed to **fail**, and the mutation was reverted to confirm it passes. **One mutation initially stayed green and the test was rewritten.** `proposesTestsForFilesThatTheLineCapWouldHaveDroppedEntirely` first asserted only that each batch *contained* its file — which is also true when every call is handed the whole-PR diff, the very behavior being replaced. Strengthened to assert the partition (batch 1 contains `Foo` and **not** `Other`, batch 2 the reverse), it goes red properly: ``` [ERROR] UnitTestGeneratorTest.proposesTestsForFilesThatTheLineCapWouldHaveDroppedEntirely:439 [[THRILLHOUSEBOT-UNTRUSTED-DATA-3d24804df8d0a8a72a18bb0d9f6a121f]] ``` **The project stack is counted in the budget.** Mutation: use the five-argument `planBatches(...)`, leaving the stack out of the overhead. With a 20k-character stack no file can honestly fit, so the correct run makes no call at all; the mutant ships batches that overshoot: ``` [ERROR] UnitTestGeneratorTest.countsTheProjectStackInTheBudgetSoBatchesAreNotOversized:496 No interactions wanted here: ``` **Per-repo ignores stay authoritative.** Mutation: plan from `inputs.reviewableFiles()` instead of the filtered list. ``` [ERROR] UnitTestGeneratorTest.leavesFilesTheRepositoryAskedTheBotToIgnoreOutOfScope:520 [[THRILLHOUSEBOT-UNTRUSTED-DATA-5c207a4fc3e69a526faf47a8f7df5769]] [ERROR] UnitTestGeneratorTest.staysSilentWhenEveryChangedFileIsOutOfScope:574 ``` **Same-path proposals are deduped.** Mutation: drop the `seenPaths` guard. ``` [ERROR] UnitTestGeneratorTest.keepsOneProposalPerPathAndSaysHowManyWereLeftOut:479 ## 🤖 ThrillhouseBot — suggested unit tests ``` **Disclosure comes from the budget plan.** Mutation: `disclosure(plan)` returns `""` and the empty-plan branch returns `null`. ``` [ERROR] UnitTestGeneratorTest.disclosesPartialCoverageEvenWhenNoTestsWereProposed:321 🧪 ThrillhouseBot found nothing in this PR's changes that warrants a new unit test. ==> expected: <true> but was: <false> [ERROR] UnitTestGeneratorTest.namesTheFilesLeftUncoveredWhenTheBatchBudgetRunsOut:303 ## 🤖 ThrillhouseBot — suggested unit tests [ERROR] UnitTestGeneratorTest.namesTheFilesWhenTheBudgetCouldNotCoverASingleOne:555 expected: not <null> ``` Earlier rounds for `/describe`, `/changelog` and `/improve` (batch text vs. line-capped render, synthesis vs. concatenation, reserved reduce call, per-repo ignores, disclosure, budgeting-disabled, nothing-covered, merge declines) all went red as recorded before; the `<= 0` guard mutation remains the one that does not, because `max-input-tokens=0` reaches `Integer.MAX_VALUE` down the fall-through path and yields the same single batch. Build results, on the merge of `release/v0.6.0` at `ad36d22` (#458, #460, #464, #459, #461): ``` ./mvnw -B spotless:apply # clean ./mvnw -B clean compile spotbugs:check spotless:check # BUILD SUCCESS ./mvnw -B clean test # Tests run: 2226, Failures: 0, Errors: 0, Skipped: 0 cd website && npm run build # "All internal links are valid." ``` ## Checklist - [x] My code follows the project's coding standards - [x] I have performed a self-review of my own code - [x] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [x] I have updated the documentation accordingly - [x] My changes generate no new warnings or errors ## Notes for anything still in flight `Inputs.omittedFiles` is gone. It carried how many files the `max-diff-lines` render dropped; once a command plans its own token-budgeted batches that number describes a render nothing sends to a model, so it is not merely redundant but wrong. Nothing on `release/v0.6.0` or in this tree still reads it. Any command extending `AbstractPrSuggestionGenerator` that is still in flight needs the same four steps `/generate-tests` just took: 1. `disclosure(plan)` rather than a line-cap count. 2. `planBatches(reviewable, inputs, <its own system prompt>, <its own user prompt>, reservedCalls)` — its **own** prompt constants, and the six-argument form if it repeats a section the shared overhead does not know about. Check the merged tree for whether the class exposes `systemPrompt()` / `userPrompt()` accessors or only constants; adding them is required if a sizing reference would inline a third copy. 3. `batch.text()` per batch, never `inputs.diff()`. 4. `respectPerRepoIgnores(target, COMMAND, inputs.reviewableFiles())`, with that same list used for anything that anchors onto the diff. ## Additional Notes **Operator-visible cost change.** `/describe` and `/changelog` on a PR that needs more than one batch now cost one more model call than the batches alone, reserved out of `REVIEW_MAX_AI_CALLS`, so the ceiling per run is unchanged. `/improve` and `/generate-tests` reserve nothing. Documented in the config table, the "AI call budget" section, the command prose, and the Known limitations bullet. **The ignore filter is authoritative for the line resolver, not just the planner.** `/improve` threads the resolved list into both `planBatches(...)` and `post(...)`, where the resolver is built as `new DiffLineResolver(diffFormatter().patchesByReviewableFiles(reviewable))` — never from `inputs.reviewableFiles()`. `PrImprovementServiceTest.neverCommitsASuggestionToAFileTheRepositoryAskedTheBotToIgnore` pins it. `/describe`, `/changelog` and `/generate-tests` build no line map (a proposed test file is a new file with no diff line to anchor to), so they cannot exercise it, but the rule is stated in `respectPerRepoIgnores(...)`'s javadoc for future adopters. **New prompts.** `PrDescribeAssistantPrompts.SYNTHESIS_SYSTEM`/`SYNTHESIS_USER` and `ChangelogAssistantPrompts.MERGE_SYSTEM`/`MERGE_USER`, with matching `synthesize(...)` / `merge(...)` methods on the assistants. Both user templates mirror `PrSuggestionPrompts.USER` — same context sections, same random-fence untrusted-data block — with the partials/candidates in place of the diff. `AiServicePromptRenderingTest` drives both through the real rendering pipeline and asserts every `@V` reaches the message. **No new config keys.**
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 judges hunks locally. Nothing asks whether a change actually does what its title claims by following its central data structure from where it is produced to where it is consumed, so a producer/consumer contract mismatch spanning two individually-valid hunks slips through.
The dogfood case is PR #99 (Fixes #95):
evaluateCiChecksappended every check it walked — including the ones it had just classified"success"— the caller bound the result tooffendingCiChecks, andbuildResultdowngradedAPPROVEon!offendingCiChecks.isEmpty(). Both hunks read fine in isolation; the net effect was that a PR with fully green CI could never be approved, the inverse of the PR's stated intent ("downgrades a would-be APPROVE to COMMENT when any offending check exists").PrReviewPrompts.SYSTEMgains review dimension 9,PRODUCER → CONSUMER CONTRACT:offending,invalid,failed,missing,stale,duplicate— must be populated only with items satisfying it; (b) a collection consumed as a gate throughisEmpty()/size()/anyMatchmust hold only gate-worthy entries; (c) the resulting end-to-end behavior must match the PR title and description.summary.description_gapsentry — thedescription_gapsfield description now names the inverted trace as one of its inputs.Precision is guarded from both sides so ordinary single-hunk diffs do not turn into data-flow essays: the dimension is scoped to the change's primary structure, is explicitly not a finding when producer and consumer agree or when the consumer is not in the provided material, and a new self-check bullet invalidates a contract claim that cannot quote both ends and name the concrete case on which they disagree.
Two generator cases are added to the eval corpus, both derived from PR #99 and sharing the same producer/consumer shape:
pr99-offending-list-holds-passing-checks-must-find— the buggy revision (commit7f38487),must-find.pr99-offending-list-contract-holds-must-not-find— the shipped fix (commit1e9ee8f) with the contract intact,must-not-find.Having both means the rule can be measured for recall and for false positives on the same code rather than only for recall.
Note: this is the contract rule. It is most reliable once both ends of the data flow are in context (#55, not implemented here) but is deliberately written to be useful on multi-hunk diffs alone.
SUMMARY_SYSTEM/SUMMARY_USERare untouched.Related Issues
Fixes #117
How Has This Been Tested?
Three new coarse content-anchor cases in
PrReviewPromptsContentTest, following the convention the file already uses for prompt guidance:generatorPromptTracesTheChangesStructureFromProducerToConsumergeneratorPromptRoutesAnInvertedTraceIntoDescriptionGapsgeneratorPromptKeepsTheDataFlowDimensionFromFiringOnOrdinaryDiffsRed/green validation. With the tests in place and the prompt change stashed (
git stash push -- src/main/java), the three cases fail:With the prompt change restored:
Tests run: 25, Failures: 0(PrReviewPromptsContentTest) andTests run: 4, Failures: 0(EvalCorpusTest, which parses and well-formedness-checks the two new fixtures on every build).Full local run:
./mvnw -B spotless:apply,./mvnw -B clean compile spotbugs:check spotless:check, and./mvnw -B clean test(2047 tests) all green.What is not verified
The content anchors prove the prompt text is present and guard against a future edit silently reverting it. They prove nothing about model behaviour.
The two eval-corpus cases are the real evidence for this change, and they were not executed. No AI provider key exists in the environment this was built in.
must-findandmust-not-findare therefore expected outcomes recorded as corpus labels, not measured results.A reviewer should run the eval before merging:
That run also re-checks the six pre-existing corpus cases. Those six are the actual regression guard against dimension 9 adding noise elsewhere in the review.
The
must-not-findcontrol trips only on the keywords["producer", "non-offending", "including passing", "contract mismatch"]. A false positive worded differently would slip past it, so a cleanmust-not-findresult is weaker evidence of precision than it looks.Checklist
Screenshots / Logs
N/A — prompt-text change; the deterministic evidence is the red/green output above.
Additional Notes
.env.exampleentry is required.PromptInputs, and the assembler are unchanged.