feat(review): /improve — whole-PR improvement pass (PR-Agent parity)

[devops-thiago]

What type of PR is this?

• 🐛 Bug fix
• ✨ Feature
• 📝 Documentation
• 🔧 Refactor
• 🚀 Performance
• ✅ Test
• 🔒 Security
• 📦 Dependency update
• 🏗️ CI/CD

Description

PR-Agent exposes /improve, a dedicated pass that proposes broad, committable improvements across an entire PR. ThrillhouseBot only attached inline suggestion blocks to findings produced by the review pipeline, so there was no way to ask for an "improve this change set" pass.

This adds /improve (and the @thrillhousebot improve mention form) as an on-demand, write-gated command:

• Whole-PR pass, token-budgeted. PrImprovementService plans batches over the reviewable file list with DiffBudgetPlanner, the way the review path has worked since spike(review): define a better large-diff handling strategy #53, and makes one model call per batch. It extends AbstractPrSuggestionGenerator for the PR title/body and the resolved repository instructions (.github/thrillhousebot.md and the fallback chain), with the same fail-soft degradation the other on-request commands use.
• Committable suggestions. Every improvement whose quoted suggestion_old anchors cleanly onto the diff is posted as an inline ```suggestion block on the lines it replaces. A single-line replacement must land on the exact reported line and reproduce that line's text including its leading indentation; a multi-line replacement is anchored by its verbatim range so it overwrites the whole span rather than only its first line.
• Copy-paste fallback. Improvements that cannot be pinned to the diff (or that GitHub rejects) are surfaced as copy-paste blocks in the run's summary comment instead of being dropped.
• Coverage disclosure. The summary discloses the plan's omitted and clipped files — genuinely over-budget ones — and names them, via a new ReviewResult.truncationDisclosure(int, TruncationDetail) overload mirroring the existing truncationNotice(int, TruncationDetail). The on-demand surface therefore upholds the same "reported by name, never silently dropped" contract as the review banner (same standard as fix(review): /describe, /changelog, /add-docs run on a truncated diff with no partial-coverage disclosure #296).
• Gating. Write access is enforced by ManualReviewAuthorizer, the command is refused with the paused notice while a PR is paused, both suggestion kinds count against max-review-comments, and the whole command sits behind a new kill switch.

AbstractPrSuggestionGenerator.Inputs now also carries the head SHA and the ignore-filtered file list — both already fetched by loadInputs and simply discarded before — so a command can anchor its output back onto the diff without a second round of API calls.

How a run is planned

loadInputs(...)                       diff + title + body + instructions + reviewable files
  └─ respectPerRepoIgnores(...)       #449 globs applied on top of the global set  ──┐
       └─ planBatches(reviewable)                                                    │
            ├─ max-input-tokens <= 0  → one uncapped batch                           │
            └─ otherwise              → DiffBudgetPlanner.plan(                      │
                                            reviewable,                              │
                                            sharedPromptOverhead(inputs),            │
                                            perCallInputBudget(),                    │
                                            maxBatches())                            │
  └─ generate(...)  one assistant call per batch, merged and deduped by file:line    │
  └─ post(...)      DiffLineResolver over the SAME effective file list ──────────────┘

• Batches come from the file list, not the diff string. DiffBudgetPlanner orders files highest-impact-first and packs them First-Fit-Decreasing into bins that each fit the per-call input budget. A file too large for one bin is hunk-clipped; one that still does not fit is reported by name.
• What bounds coverage now is max-ai-calls, not max-diff-lines. The line cap no longer gates what the model sees. Unlike a review, /improve makes no final summary call — its summary comment is assembled locally — so the whole max-ai-calls allowance goes to batches (a review reserves one). Files that never get a batch are named in the summary.
• Shared prompt overhead is this command's own. sharedPromptOverhead(...) is built from PrImproveAssistantPrompts.SYSTEM + PrSuggestionPrompts.USER (the actual @UserMessage on PrImproveAssistant.improve) + the fence scaffolding + the escaped title/body/instructions — exactly the non-diff text generateOne(...) sends, and nothing from PrReviewPrompts. Sizing batches against the review path's prompts would let every "in-budget" batch overshoot the real input limit.
• Anchoring stays whole-PR. The DiffLineResolver is built once from the effective reviewable file list — never from a batch — so an improvement produced by batch 3 still anchors to its correct absolute line.
• Merging and dedupe. Results are merged across batches and deduped by file and line, so two batches can never propose the same line twice.
• Partial failure is survivable. A batch whose call or parse fails is skipped and disclosed rather than failing the run; only an all-batches failure posts the failure notice.

Why batching, not max-diff-lines

The first version of this command took the pre-#53 route: a single call over the diff string that ReviewDiffFormatter caps at max-diff-lines. For a command whose entire value proposition is covering the whole change set, silently shrinking to the first N lines is the wrong failure mode — and it is not hypothetical. On a change set with the cap set low, the model received literally this and nothing else:

## Overview: 2 files (+7 -0)

(diff truncated at 4 lines — 2 files omitted)

Batching sizes by tokens over the file list, so a long diff only loses coverage once it exceeds the whole budget. Truncation becomes the rare genuinely-over-budget fallback rather than the design.

Cost. One model call per batch, capped at max-ai-calls (default 6) — so an /improve costs at most what one review costs, and typically far less, since most PRs fit in a single batch. It is not free, though: a large PR that used to cost one call can now cost up to six. Lowering max-ai-calls lowers the ceiling for both commands.

The seam #457 builds on

#457 moves /describe, /changelog and /add-docs onto the same batching. The seam is the pair of methods in PrImprovementService:

// budget → batches, over an already-ignore-filtered file list
private DiffBudgetPlanner.BudgetPlan planBatches(List<FileDiff> reviewable, Inputs inputs)
// the non-diff text this command's own calls repeat, so the planner can subtract it
private String sharedPromptOverhead(Inputs inputs)

Everything else those commands need is already shared: AbstractPrSuggestionGenerator.Inputs carries the ignore-filtered reviewableFiles, DiffBudgetPlanner.plan(reviewable, overhead, perCallInputBudget(), maxBatches) is package-visible, and ReviewResult.truncationDisclosure(int, TruncationDetail) renders the disclosure. So each command needs only its own overhead string. The natural refactor is to lift planBatches and sharedPromptOverhead into AbstractPrSuggestionGenerator, parameterised by the caller's prompt constants; respectPerRepoIgnores(...) should move up with them (it is wired only into /improve here to avoid changing the other three commands' behaviour in this PR).

Respecting per-repo ignore patterns

Now that every file is in scope, the per-repo ignore patterns from #449 are applied on top of the deployment-wide set. While the pass stopped at max-diff-lines, a repo-ignored file beyond the cap was excluded by accident; without this it would newly receive committable suggestions against generated or vendored code a repository explicitly asked the bot to leave alone. Per-repo patterns are strictly additive, so the already-filtered list is narrowed again — no extra diff fetch — and it fails soft to the global set exactly like the review path.

The same effective list also backs the line map, so an ignored file cannot be reached by a hallucinated path either: a suggestion naming one no longer resolves, and degrades to a copy-paste note instead of a one-click commit.

Why the anchoring is stricter than /add-docs

/add-docs only ever inserts a doc comment above a declaration, so a quote that matches loosely is harmless. /improve rewrites the line, and committing a suggestion replaces the anchored range with suggestion_new verbatim. A model that re-indented the code it quoted has almost certainly re-indented its replacement too, so a loose match would silently reflow the line — and in an indentation-sensitive language, change what the code means. /improve therefore requires an exact match (trailing whitespace excepted) and fails closed when the line text cannot be read: an unverifiable line must not be rewritten on the author's behalf. Anything that does not anchor becomes a copy-paste block a human applies deliberately. The prompt asks for character-exact indentation in suggestion_old so this path stays the common one rather than the fallback.

Configuration

New key thrillhousebot.review.improve-enabled / REVIEW_IMPROVE_ENABLED, documented in README.md and .env.example.

It defaults to true, matching the add-docs-enabled precedent rather than the usual "new flag defaults to current behaviour" rule: the command never runs automatically, requires write access, and only spends AI budget when a maintainer explicitly asks for it. Flagging that here explicitly in case you would rather ship it opt-in — it is a one-line change to @WithDefault.

No new key was needed for batching: it reuses REVIEW_MAX_INPUT_TOKENS, REVIEW_TOKEN_SAFETY_MARGIN, REVIEW_OUTPUT_BUFFER_TOKENS and REVIEW_MAX_AI_CALLS. Their README rows now say so, and REVIEW_MAX_DIFF_LINES no longer lists /improve among the single-call line-capped renders.

Documentation

The docs site's current-version pages include: the README sections, so the README edits flow into them automatically; the 0.1.0–0.4.0 trees are frozen archives and are untouched. website/src/content/docs/index.md keeps its own hand-maintained command teaser, which listed every on-demand generation command except the new one, so it is updated too. npm ci && npm run build in website/ passes with "All internal links are valid."

docs/COMPARISON.md was left untouched — it compares licensing, hosting, model and cost dimensions and carries no command matrix, so there is nothing there to update.

Related Issues

Fixes #316

How Has This Been Tested?

• Unit tests
• Integration tests
• Manual testing

PrImprovementServiceTest (35 tests), ImprovementParserTest (6), ImprovementResponseTest (12), plus additions to SuggestionFormatterTest, TriggerDetectorTest, CommentCommandServiceTest and WebhookControllerTest. Full suite: 2112 tests, 0 failures, 0 errors.

Every behaviour was validated by neutralizing only the production change — never the test — and confirming a real assertion failure.

Round 1 — the command itself

1. Command detection — removed the IMPROVE entry from TriggerDetector.buildPatterns():

TriggerDetectorTest.shouldDetectEachSlashCommand:62 expected: IMPROVE but was: NONE
TriggerDetectorTest.shouldDetectEachMentionCommand:76 expected: IMPROVE but was: NONE
TriggerDetectorTest.shouldDetectImproveOutsideAQuotedMention:168 expected: IMPROVE but was: NONE

2. Command routing — removed the IMPROVE switch arm and the help-table row from CommentCommandService:

CommentCommandServiceTest.improveDelegatesToImprovementServiceWhenAuthorized
Wanted but not invoked:
improvementService.handle(
    ImproveTask[owner=owner, repo=repo, prNumber=7, defaultBranch=main, installationId=12345],
    "token"
);
Actually, there were zero interactions with this mock.

3. Generation / committable suggestions — made PrImprovementService.postInline return false unconditionally:

PrImprovementServiceTest.postsCommittableSuggestionForAnImprovement
Wanted but not invoked:
reviewClient.createPullRequestComment(any, any, "owner", "repo", 7,
    Capturing argument: CreatePullRequestCommentRequest);
Actually, there were zero interactions with this mock.

(also red: anchorsAMultiLineImprovementAcrossItsWholeRange, capsThePerRunCommentCount)

Round 2 — the indentation fix

Reverting the comparison to the original .strip() form, keeping the test:

PrImprovementServiceTest.doesNotRewriteALineWhoseQuoteDropsTheLeadingIndentation
org.mockito.exceptions.verification.NeverWantedButInvoked:

reviewClient.createPullRequestComment(
    any,
    any,
    any,
    any,
    any integer,
    any
);
Never wanted here:

That is the defect itself: with .strip() the mis-indented rewrite was posted as a one-click commit. Green once the fix is restored. anchorsAQuoteThatReproducesTheLeadingIndentation and anchorsDespiteInsignificantTrailingWhitespaceInTheDiff cover the other side, so the stricter rule cannot silently swallow correct suggestions.

Round 3 — coverage

codecov/patch was red at 85.33% with 33 uncovered lines, all of them fail-soft and degradation paths — the ones that matter most for work running off the webhook ACK thread. Now covered: malformed and empty model responses, isPostable() for every missing field, a PR with a blank or absent head ref, a formatter yielding no reviewable file list, a blank or absent rendered diff, a multi-line replacement whose range cannot be resolved, a reported line that only snaps to a neighbour, the per-run cap swallowing every improvement, and a summary post that throws. Each new test was proven red first, e.g. with isPostable() stubbed to return true:

ImprovementResponseTest.anImprovementMissingRequiredDataIsNotPostable:76 null file ==> expected: false but was: true
ImprovementResponseTest.anImprovementMissingRequiredDataIsNotPostable:76 blank file ==> expected: false but was: true
ImprovementResponseTest.anImprovementMissingRequiredDataIsNotPostable:76 zero line ==> expected: false but was: true

Round 4 — token-budgeted batching, mutation-tested per test

Each batching test was mutation-tested individually: one minimal change to production code, ./mvnw -B test -Dtest=PrImprovementServiceTest, then restore. Verbatim failures:

coversFilesThatTheLineCapWouldHaveDroppedEntirely — sent inputs.diff() (the line-capped string) instead of the batch text:

org.opentest4j.AssertionFailedError:
[[THRILLHOUSEBOT-UNTRUSTED-DATA-3cc17bcc8c94fe51adb1bbe6ec3fcee4]]
## Overview: 2 files (+7 -0)


(diff truncated at 4 lines — 2 files omitted)

[[THRILLHOUSEBOT-UNTRUSTED-DATA-3cc17bcc8c94fe51adb1bbe6ec3fcee4]] ==> expected: <true> but was: <false>

That is the whole case for this change: under the old design the model was handed a truncation notice and zero file content. The same mutation also reddened sendsOneUncappedBatchWhenTokenBudgetingIsDisabled and leavesFilesTheRepositoryAskedTheBotToIgnoreOutOfScope.

splitsAnOversizedChangeSetAcrossBatchesAndDedupesTheResults — dropped the seen.add(dedupeKey(improvement)) guard:

org.mockito.exceptions.verification.TooManyActualInvocations:
reviewClient.createPullRequestComment(<any>, <any>, <any>, <any>, <any integer>, <any>);
Wanted 1 time:
But was 2 times:
-> at PrImprovementService.postInline(PrImprovementService.java:397)
-> at PrImprovementService.postInline(PrImprovementService.java:397)

neverSpendsMoreModelCallsThanMaxAiCalls — maxBatches() returning Integer.MAX_VALUE:

org.mockito.exceptions.verification.TooManyActualInvocations:
improveAssistant.improve(<any>, <any>, <any>, <any>);
Wanted 1 time:
But was 2 times:

The same mutation also reddened namesTheFilesLeftUncoveredWhenTheBatchBudgetRunsOut — with unbounded batches the summary says "partially analyzed" instead of naming the files no batch ever reached.

disclosesFilesTheTokenBudgetCouldNotCoverByName — disclosure passed only a count, dropping the TruncationDetail:

org.opentest4j.AssertionFailedError:
✨ ThrillhouseBot has no improvements to suggest for the changes in this PR.

> ⚠️ **Large PR — partial coverage.** 2 file(s) were omitted because the diff exceeded the size budget, so this covers only part of the diff. ==> expected: <true> but was: <false>

sendsOneUncappedBatchWhenTokenBudgetingIsDisabled — the budgeting-off path planning with a 1-token diff budget instead of an uncapped one:

Wanted but not invoked:
improveAssistant.improve(<Capturing argument: String>, <any>, <any>, <any>);
Actually, there were zero interactions with this mock.

keepsImprovementsFromTheBatchesThatSucceededWhenOneBatchFails — if (failed == plan.batches().size()) weakened to if (failed > 0):

Wanted but not invoked:
reviewClient.createPullRequestComment(<any>, <any>, <any>, <any>, <any integer>, <any>);
Actually, there were zero interactions with this mock.

leavesFilesTheRepositoryAskedTheBotToIgnoreOutOfScope — respectPerRepoIgnores short-circuited to the global list:

org.opentest4j.AssertionFailedError:
[[THRILLHOUSEBOT-UNTRUSTED-DATA-293487720f3452d4b6d88f0c5795f471]]
## Overview: 2 files (+7 -0)

### src/Foo.java (modified, +4 -0)
...
### src/Other.java (modified, +3 -0)
...
==> expected: <false> but was: <true>

doesNotDiscloseTruncationForTheLineCapWhenTheBudgetCoveredEverything — re-appended the old ReviewResult.truncationDisclosure(inputs.omittedFiles()):

org.opentest4j.AssertionFailedError:
## ✨ ThrillhouseBot — suggested improvements

Proposed **1** committable improvement(s) inline on the changed lines.

---
*Nothing was committed — review each suggestion and commit the ones you want. Re-run with `/improve`.*

> ⚠️ **Large PR — partial coverage.** 48 file(s) were omitted because the diff exceeded the size budget, so this covers only part of the diff. ==> expected: <false> but was: <true>

That is the false partial-coverage claim the test exists to forbid: 48 files the line cap dropped, against a plan that batched every file within budget.

toleratesAFormatterThatYieldsNoReviewableFileList — removed the empty-plan early return:

org.opentest4j.AssertionFailedError: expected: <✨ ThrillhouseBot found no reviewable changes to improve in this PR.> but was: <✨ ThrillhouseBot could not generate improvements for this PR. Please try `/improve` again.>

One mutation survived, and it should have. Narrowing the activeModel.maxInputTokens() <= 0 guard in planBatches to < 0 leaves the whole class green. That is correct rather than a hole: with budgeting off, perCallInputBudget() already returns Integer.MAX_VALUE, so the general path also produces one uncapped batch. The branch exists to skip the BPE pass entirely and to mark the plan budgeted=false, mirroring the identical guard in DiffBudgetPlanner.plan(reviewable, PromptInputs); it is kept for parity with the review path, not as behaviour the tests need to pin. The behaviour it protects is pinned — the same test dies under both the plan(reviewable, 1, 1) mutation above and the line-capped-string mutation.

Batch budgets in these tests are derived from the real prompt overhead via TokenCounter rather than hardcoded, so editing a prompt cannot silently turn them into no-ops by making every file overflow.

Two earlier tests changed meaning rather than being deleted, since the design change invalidated their premise: the old "disclosure comes from the line cap" test is now doesNotDiscloseTruncationForTheLineCapWhenTheBudgetCoveredEverything — a formatter reporting 48 line-omitted files against a plan that covered every file must produce no warning, since a false partial-coverage claim is its own defect. And a formatter yielding no reviewable files now reports "no reviewable changes" instead of falling through to the model, because there is nothing to plan batches over.

Round 5 — cross-batch anchoring and ignore scope

Two gaps the batching commit left untested, closed here.

anchorsAnImprovementFromALaterBatchToItsAbsoluteLine — a three-file PR, one file per batch, where the only improvement comes out of the last batch and names a file whose hunk starts at line 120. Committing a rewrite to the wrong line is the exact failure the stricter anchoring exists to prevent, so it needs a multi-batch fixture rather than an argument. The per-call budget is computed from the real rendered sections (oneFilePerBatchBudget), so the split is deterministic and nothing is clipped. Building the line map from the first batch instead of the whole PR:

PrImprovementServiceTest.anchorsAnImprovementFromALaterBatchToItsAbsoluteLine
Wanted but not invoked:
reviewClient.createPullRequestComment(<any>, <any>, "owner", "repo", 7,
    <Capturing argument: CreatePullRequestCommentRequest>);
Actually, there were zero interactions with this mock.

(The same mutation also reddens coversFilesThatTheLineCapWouldHaveDroppedEntirely, which anchors onto the second file of a single batch.)

neverCommitsASuggestionToAFileTheRepositoryAskedTheBotToIgnore — this one found a real, if narrow, hole. The batches were planned over the ignore-filtered list, but DiffLineResolver was still built from inputs.reviewableFiles(), the pre-per-repo-ignore list. A model naming an ignored file therefore still resolved against it and got a committable suggestion posted onto code the repository asked the bot to leave alone. Fixed by computing the effective list once in handle(...) and threading it into both the planner and the resolver — the compute-once discipline ReviewContextLoader already uses. With the resolver reverted to inputs.reviewableFiles():

PrImprovementServiceTest.neverCommitsASuggestionToAFileTheRepositoryAskedTheBotToIgnore
org.mockito.exceptions.verification.NeverWantedButInvoked:

reviewClient.createPullRequestComment(<any>, <any>, <any>, <any>, <any integer>, <any>);
Never wanted here:
But invoked here:
-> at PrImprovementService.postInline(PrImprovementService.java:405) with arguments:
   [..., CreatePullRequestCommentRequest[commitId=headsha1234567, body=**✨ Improvement — Bound the retry loop** `error-handling`
   ...
   ```suggestion
   while (retries++ < 3) { call(); }
   ```
   , path=src/Other.java, line=2, side=RIGHT, startLine=null, startSide=null]]

src/Other.java is the file the repository asked the bot to ignore, and that is a one-click commit against it.

Coverage

Local JaCoCo on PrImprovementService: every line covered, and exactly one partial branch out of 78 — the current != null arm of quotesCurrentLine. It is the same guard Codecov has been flagging since Round 3; it only moved down the file as batching was added (line 309 → 445), so it is not a new gap. Nothing in the batching or anchoring code is uncovered. It is unreachable by construction: postInline only calls it after resolveRightSideLine returned the exact line, and DiffLineResolver.appendRightSide does lineText.put(...) unconditionally for every line it adds to lines. It is kept as fail-closed defence against a future divergence rather than deleted to buy a percentage point. DiffBudgetPlanner is 100% line and 100% branch.

ThrillhouseBot's own findings on this PR

Dogfooding, so all three are recorded with a verdict.

🟡 MEDIUM — "Lenient whitespace comparison may allow un-anchorable suggestions" (quotesCurrentLine): conclusion right, reasoning wrong. Fixed, and hardened beyond the proposal.

The stated mechanism is not correct: GitHub does not validate suggestion_old at all. Committing a suggestion replaces the anchored line range with the block body verbatim, so it cannot "fail to apply" — suggestion_old is purely this codebase's own anchor.

The conclusion is right for a different reason, and it is a real defect: DiffLineResolver stores right-side text as rawLine.substring(1), so the diff marker is gone but the indentation is preserved. Comparing with .strip() therefore matched an un-indented quote, and the committable suggestion we posted carried the model's un-indented replacement — committing it reflowed the line.

The suggested patch kept current == null ||, i.e. fail open. Under a verbatim-commit contract "cannot verify this line" must mean "do not rewrite it", so the fix inverts that too:

return current != null
    && current.stripTrailing().equals(improvement.suggestionOld().stripTrailing());

🔵 LOW — "Missing newline after opening code fence in formatImprovementBlock": wrong. Patch not applied; it would have introduced the defect it describes.

CODE_FENCE_CLOSE was "\n```\n" — it already carries a newline on both sides, and is used as the opening delimiter as well as the closing one. Applying the suggested patch verbatim and running the byte-exact rendering test (newlines shown escaped, since the values contain fences themselves):

SuggestionFormatterTest.shouldRenderImprovementBlockFencesByteExactly
expected: **T**\n\n```\nline one\nline two\n```\n
but was:  **T**\n\n```\n\nline one\nline two\n\n```\n

— a blank line as the first and last line inside every code block, in /add-docs notes as well as /improve blocks.

The finding reasoned from the constant's name rather than its value, and the name was genuinely misleading, so the underlying issue is fixed instead: CODE_FENCE_CLOSE is renamed to CODE_FENCE with the value and rendered output unchanged, and both renderings are now locked by byte-exact assertions.

🔵 LOW — "Verify that file list retrieval paginates to avoid silent truncation" (SoftLoaders.files): false positive — but a well-formed one.

The concern was that SoftLoaders.files might take GitHub's 30-per-page default and silently drop everything beyond the first page, which under batching would matter more than before. It does not: SoftLoaders.files calls GitHubPullRequestClient.getPullRequestFiles, which walks pages of FILES_PER_PAGE = 100 up to MAX_FILE_PAGES = 30 — 3000 files, which is GitHub's own cap on the files endpoint. No change made.

Worth recording how this one was raised, because it is the shape a low-confidence finding should have: the code it flagged is outside this diff, so it hedged to LOW, said explicitly that the file was "not shown in this diff", asked for verification rather than asserting a bug, and was routed to "Things to double-check" in the summary instead of an inline thread. That is the correct handling for a hypothesis about unseen code — the check cost a minute and the alternative (staying silent about a plausible whole-PR-coverage bug) would have been worse.

Other

Quoted-input safety per .github/thrillhousebot.md: /improve inside fenced code blocks, blockquotes and inline code is asserted not to trigger the command, while a genuine /improve alongside a quoted one still fires.

All four SonarCloud issues raised against this PR are resolved (S5976 — three structurally identical tests merged into one parameterized test; two S6126 — text blocks, each verified byte-identical by comparing the resulting String values, not by the tests still passing; S135 — the loop's continue removed by inverting the guard).

Local gates: ./mvnw -B spotless:apply clean, ./mvnw -B clean compile spotbugs:check spotless:check reports BugInstance size is 0, ./mvnw -B clean test reports Tests run: 2112, Failures: 0, Errors: 0, Skipped: 0.

Checklist

• My code follows the project's coding standards
• I have performed a self-review of my own code
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• I have updated the documentation accordingly
• My changes generate no new warnings or errors

Screenshots / Logs

The prompt the model actually received under the old single-call design, on a two-file change set with the line cap set to 4 — captured verbatim from the assertion failure when the batching change is reverted:

[[THRILLHOUSEBOT-UNTRUSTED-DATA-3cc17bcc8c94fe51adb1bbe6ec3fcee4]]
## Overview: 2 files (+7 -0)


(diff truncated at 4 lines — 2 files omitted)

[[THRILLHOUSEBOT-UNTRUSTED-DATA-3cc17bcc8c94fe51adb1bbe6ec3fcee4]]

Additional Notes

PrDescriptionGeneratorTest and ChangelogEntryGeneratorTest each needed a two-line stub update: loadInputs now calls the two-argument buildDiffStringWithStats(files, reviewableFiles) overload (the one-argument form computed the reviewable list internally and threw it away), so the mocked formatter stubs had to match the new arity. No assertions changed.

Known follow-ups, deliberately out of scope:

• feat(review): move /describe, /changelog and /add-docs off the line cap onto token-budgeted batching #457 — see The seam #457 builds on above for the two methods to lift and what is already shared.
• The multi-line anchoring path still matches on stripped lines via DiffLineResolver.resolveSuggestionRange, which is shared with the review/finding path. Applying the same indentation strictness there would change behaviour for other commands and belongs in its own change.
• An improvement naming a repo-ignored file still renders as a copy-paste block in the summary (it just cannot be committed with one click). The block contains only model-authored text and the path it invented, never content from the ignored file, so this is the same treatment any unanchorable suggestion gets.

[github-actions]

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

[thrillhousebot]

🤖 ThrillhouseBot PR Summary

What this PR does

This change adds an on-demand /improve command that runs a whole-PR improvement pass over the diff and posts committable suggestions or copy-paste blocks inline on the PR, reusing the existing suggestion infrastructure and input loading.

Control-Flow Diagram

🔀 Show diagram

flowchart TD
  A["User comments /improve"] --> B["Detect command (TriggerDetector)"]
  B --> C["CommentCommandService.handleImprove"]
  C --> D{"improveEnabled?"}
  D -->|No| E["Log & ignore"]
  D -->|Yes| F{"Authorized?"}
  F -->|No| G["Log & ignore"]
  F -->|Yes| H{"PR paused?"}
  H -->|Yes| I["Post paused notice"]
  H -->|No| J["PrImprovementService.handle"]
  J --> K["loadInputs: fetch diff, title, body, instructions"]
  K --> L{"Diff available?"}
  L -->|No| M["Post NO_CHANGES"]
  L -->|Yes| N["improveAssistant.improve"]
  N --> O{"Parse response"}
  O -->|Failure| P["Post GENERATION_FAILED"]
  O -->|Success| Q["Post inline suggestions / copy-paste blocks"]
  Q --> R["Post summary comment with cap disclosure"]
  R --> S["Done"]

Loading

Changes Overview

• Files changed: 21
• Lines added: +1205
• Lines removed: -24

Changed Files

File	Change	Summary
.env.example	Modified	Add commented example for REVIEW_IMPROVE_ENABLED.
README.md	Modified	Document new /improve command and config key; update on-demand command list.
docs/ARCHITECTURE.md	Modified	-
src/main/java/dev/thiagogonzaga/thrillhousebot/config/ThrillhouseConfig.java	Modified	New improve-enabled config key defaulting to true, providing kill switch for the /improve command.
src/main/java/dev/thiagogonzaga/thrillhousebot/review/AbstractPrSuggestionGenerator.java	Modified	Extend Inputs record with headSha and reviewableFiles; adjust loadInputs to collect and pass them.
src/main/java/dev/thiagogonzaga/thrillhousebot/review/PrImprovementService.java	Added	Handle /improve command: load diff with truncation, call AI, post inline suggestions or copy-paste blocks with per-run comment cap.
src/main/java/dev/thiagogonzaga/thrillhousebot/review/SuggestionFormatter.java	Modified	Add formatting methods for improvement inline comments and copy-paste blocks.
src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/ImprovementParser.java	Added	-
src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/ImprovementResponse.java	Added	Record to hold parsed improvements with a postable check that filters out incomplete entries.
src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/PrImproveAssistant.java	Added	LangChain4j AI service interface for the /improve pass, grounded in the same user prompt as other suggestion commands.
src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/PrImproveAssistantPrompts.java	Added	System prompt instructing the model to propose concrete, committable improvements across the diff.
src/main/java/dev/thiagogonzaga/thrillhousebot/webhook/CommentCommand.java	Modified	Add IMPROVE enum value.
src/main/java/dev/thiagogonzaga/thrillhousebot/webhook/CommentCommandService.java	Modified	Route /improve command with authorization, pause check, and enabled guard; delegate to PrImprovementService.
src/main/java/dev/thiagogonzaga/thrillhousebot/webhook/TriggerDetector.java	Modified	Add patterns for /improve and @mention form; tests confirm quotation contexts are excluded.
src/main/resources/application.properties	Modified	Bind REVIEW_IMPROVE_ENABLED to improve-enabled config with default true.
src/test/java/dev/thiagogonzaga/thrillhousebot/review/ChangelogEntryGeneratorTest.java	Modified	-
src/test/java/dev/thiagogonzaga/thrillhousebot/review/PrDescriptionGeneratorTest.java	Modified	-
src/test/java/dev/thiagogonzaga/thrillhousebot/review/PrImprovementServiceTest.java	Added	12 tests covering inline anchoring, copy-paste fallback, truncation disclosure, comment cap, no-diff, and failure cases.
src/test/java/dev/thiagogonzaga/thrillhousebot/webhook/CommentCommandServiceTest.java	Modified	-
src/test/java/dev/thiagogonzaga/thrillhousebot/webhook/TriggerDetectorTest.java	Modified	-

…and 1 more file(s).

Risk Assessment

Risk	Count
🔴 Critical	0
🟠 High	0
🟡 Medium	0
🔵 Low	0

No new issues found in this PR, but the review cannot be approved until CI is confirmed green.

⚠️ CI Checks Status

Some checks are still pending or have failed:

Check	Type	Status	Detail
changes	check-run	⏳ Pending	-
build	check-run	⏳ Pending	-
test	check-run	⏳ Pending	-
frontend	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.

[codecov]

Codecov Report

❌ Patch coverage is 99.64413% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...ga/thrillhousebot/review/PrImprovementService.java	99.51%	0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[thrillhousebot]

🟡 MEDIUM — Lenient whitespace comparison may allow un-anchorable suggestions (medium confidence — verify before acting)

quotesCurrentLine uses .strip() on both the diff line text and the improvement's suggestion_old, stripping all whitespace including leading indentation. If the assistant omits leading whitespace in suggestion_old, the comparison will still match, causing the bot to post an inline suggestion whose old code does not match the actual source line exactly. GitHub may then fail to apply the suggestion, breaking the committable-suggestion flow. Compare after trimming only trailing whitespace (.stripTrailing()) to preserve leading indentation while tolerating insignificant trailing spaces.

Suggested change

	return sb.append(FOOTER).append(ReviewResult.truncationDisclosure(omittedFiles)).toString();
	return current == null || current.stripTrailing().equals(improvement.suggestionOld().stripTrailing());

[thrillhousebot]

Everything's coming up Thrillhouse! 🎉

No issues found in this PR.

[thrillhousebot]

Everything's coming up Thrillhouse! 🎉

No issues found in this PR.

[thrillhousebot]

ThrillhouseBot noted 1 lower-confidence item(s) under Things to double-check in the PR summary (not posted as inline threads):

• LOW: Verify that file list retrieval paginates to avoid silent truncation (src/main/java/dev/thiagogonzaga/thrillhousebot/review/AbstractPrSuggestionGenerator.java:84)
  The new /improve command uses SoftLoaders.files to retrieve the PR file list, which is then used for diff rendering and line anchoring. GitHub's REST API defaults to 30 files per page; if the existing SoftLoaders.files does not implement pagination, a PR with more than 30 files will have all improvements for files beyond the first page silently dropped because their diffs and line mappings will be missing. Verify that SoftLoaders.files (not shown in this diff) walks all pages or that the bot explicitly limits the scope to a single page with justification.

[thrillhousebot]

ThrillhouseBot found no issues in this PR, but some checks are still pending or failed:

• Check build is pending
• Check changes is pending
• Check test is pending
• Check format is pending
• Check trivy is pending
• Check actionlint is pending
• Check frontend 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.

[thrillhousebot]

ThrillhouseBot found no issues in this PR, but some checks are still pending or failed:

• Check trivy is pending
• Check format is pending
• Check changes is pending
• Check frontend is pending
• Check actionlint is pending
• Check test 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.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
99.3% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud