fix(test): restore release/v0.6.0 to a compiling state - #464
Merged
devops-thiago merged 1 commit intoAug 8, 2026
Conversation
release/v0.6.0 does not compile. #452 added PrImprovementServiceTest with two-argument RepoSettings construction, and #460 added the pathInstructions component to that record. Each PR was green on its own head — #460's run predated #452 landing, so neither ever saw the other's code — and the merge of both is textually clean, so nothing failed until the branch itself was built. Both call sites are ignore-filter tests that declare no path scope, so the new component is empty and their assertions are unchanged. Refs #33, #316
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
🤖 ThrillhouseBot PR SummaryWhat this PR doesFixes compile error in PrImprovementServiceTest caused by a new RepoSettings record component (pathInstructions) added in a separate PR, by passing an empty list at two call sites. 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 |
|---|---|---|---|
| changes | check-run | ⏳ Pending | - |
| frontend | check-run | ⏳ Pending | - |
| test | check-run | ⏳ Pending | - |
| trivy | check-run | ⏳ Pending | - |
| format | check-run | ⏳ Pending | - |
| actionlint | 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! |
|
devops-thiago
added a commit
that referenced
this pull request
Aug 8, 2026
…ared-command-batching Resolves the config table in README.md, where this branch's rewrite of the REVIEW_MAX_INPUT_TOKENS row (it now covers /describe and /changelog too) and the REVIEW_FOLLOW_UP_SUMMARY_ENABLED row #459 added landed on the same lines. Both rows are kept. No source conflict: #464 had already restored the three-argument RepoSettings construction on base, and this branch carried the identical change, so the ignore-filter tests of /describe, /changelog and /improve merged as-is. No batching behaviour changes.
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
release/v0.6.0currently does not compile../mvnw clean test-compileoneaceafafails:Two lines, both in one test file.
How it happened
A semantic conflict between two PRs that were each green on their own head:
00cde9f,/improve) addedPrImprovementServiceTestwith two-argumentRepoSettingsconstruction.eaceafa, path-scoped instructions) added thepathInstructionscomponent to theRepoSettingsrecord.#460's CI run predated #452 landing, so it never saw the new test file, and #452 never saw the new record component. Neither PR was wrong, and the merge of both is textually clean — git reports no conflict. Nothing failed until the branch itself was built, at which point every branch merging from it inherits the breakage.
Worth noting for future sequencing: a clean merge is not evidence that two feature branches compose. Only a build of the merged tree is.
The fix
Both call sites are ignore-filter tests —
leavesFilesTheRepositoryAskedTheBotToIgnoreOutOfScopeandneverCommitsASuggestionToAFileTheRepositoryAskedTheBotToIgnore— and neither declares a path scope, so the new component is empty and their assertions are unchanged. No production code is touched.Why this is on base rather than in each PR
Every open PR against
release/v0.6.0inherits the failure the moment it merges base. Fixing it per-branch means several independent copies of the same two-line patch, which then conflict with each other. One fix on base clears all of them at once.Related Issues
N/A — not an issue; a regression introduced by the interaction of #452 and #460.
Refs #316, #33.
How Has This Been Tested?
Reproduced first, on a clean checkout of
origin/release/v0.6.0ateaceafa:With this change applied, on the same tree:
So the failure is reproducible on base without the change and absent with it — the compile error is itself the red/green evidence here, and no new test is warranted for a two-line constructor-arity fix in existing tests.
Checklist
Additional Notes
Open PRs currently red for this reason and expected to go green once this lands: #459 (which also carries its own copy of the same fix — harmless, it will resolve as identical content) and #463. #461 is separately conflicted for unrelated reasons and needs its own merge.