feat(review): re-check a maintainer's decline against the code - #453
Merged
Conversation
A reply that declines a finding was treated as ground truth: the follow-up
analysis recorded it "justified" and the bot moved on, so a correct finding
could be closed by an incorrect rebuttal — and the rebuttal often names the
very mechanism that makes the bug real ("it only runs after the webhook is
acked, so there is no race", on an executor that starts a thread per event).
A decline is now a claim to verify. Two layers, both conservative:
- A prompt rule in the previous_findings_status contract tells the model to
trace a decline's stated reason against the code in the provided material and
keep the finding "unresolved", quoting the contradiction, when that material
plainly refutes the premise — overriding only at high confidence, and
respecting style, intent and accepted-risk rebuttals.
- A deterministic post-processing step re-checks the model's own verdict, in
the same shape as supersedeVanished/addUnreportedVanished. It fires on one
high-precision family: a concurrency finding, declined on a "this cannot run
concurrently" premise, while the reviewed diff shows the path handed to a
shared executor, a new thread, or an async dispatch. The status goes back to
"unresolved" with a note quoting both the claim and the contradicting line;
the finding is never re-posted as a new comment.
The re-check only fires while the thread carries a single maintainer reply, so
replying again always ends it and the bot cannot re-open the same finding round
after round. Everything not refutable from code text keeps the decline, and
thrillhousebot.review.decline-recheck-enabled=false disables the step outright.
This is also the gate that makes durable maintainer-feedback memory safe: only
declines that survive the re-check are sound enough to remember.
Refs #169
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
🤖 ThrillhouseBot PR SummaryWhat this PR doesAdds a step after the AI review to re-check a maintainer's decline against the reviewed code, reopening the finding if the code plainly contradicts the rebuttal; the deterministic check currently covers only concurrency-related premises. Control-Flow Diagram🔀 Show diagramflowchart TD
A["VerdictBuilder.build()"] --> B["Compute effective statuses\n(supersede/unreported vanished)"]
B --> C["Call followUpAnalyzer.recheckDeclines()"]
C --> D{"declineRecheckEnabled\n&& justified status exists?"}
D -- No --> E["Return statuses unchanged"]
D -- Yes --> F["Fetch reviewed code text\n(budget batches or ctx.diff())"]
F --> G["For each prior status"]
G --> H{"Status is justified?"}
H -- No --> I["Keep status"]
H -- Yes --> J["Find thread root comment\nand human replies"]
J --> K{"Exactly one human reply?"}
K -- No --> I
K -- Yes --> L["RebuttalContradiction.find()"]
L --> M{"Contradiction found?"}
M -- No --> I
M -- Yes --> N["Rewrite status to 'unresolved'\nwith contradiction note"]
N --> O["Add to rewritten list"]
O --> G
E --> P["Merge into final effectiveStatuses"]
I --> O
P --> Q["Build effective response and verdict"]
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 |
|---|---|---|---|
| frontend | check-run | ⏳ Pending | - |
| changes | check-run | ⏳ Pending | - |
| trivy | check-run | ⏳ Pending | - |
| test | check-run | ⏳ Pending | - |
| actionlint | check-run | ⏳ Pending | - |
| format | check-run | ⏳ Pending | - |
| dependency-review | check-run | ⏳ Pending | - |
| build | check-run | ⏳ Pending | - |
Automated review by ThrillhouseBot. Reply with /review to re-run.
thrillhousebot
Bot
added
documentation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
codecov/patch flagged 24 uncovered lines and partial branches across the decline re-check. The gaps were real, and two of them were dead code rather than missing tests. Production simplifications that remove unreachable branches: - clip() used charAt(0) behind an isEmpty() guard that no caller could ever trigger; startsWith() needs no guard at all. - assertedText() terminated every kept line with a newline, so the forward sentence walk could never reach end-of-text and its bound was unreachable. Joining the lines instead leaves an unterminated reply unterminated, which is both truer to the reply and a live case. - recheckDeclines() dropped the previous.isEmpty() and inlineComments.isEmpty() fast paths: the id-range check and the thread lookup already return "no contradiction" for both, so they were branches with no behavior behind them. The decline test moved into a named hasDecline() helper. New tests, all covering real behavior: - every no-op input the re-check can be handed, as one parameterized case per guard, each asserting the statuses come back untouched; - a mixed status list, so only the declined entry is rewritten; - bot, author-less, body-less and other-thread replies do not count as the maintainer answering the push-back; - the injected constructor honouring the config key; - title-only and description-only findings, sentence-boundary quoting on every terminator, evidence on an unterminated last line, and a -/+ diff marker; - the legacy ctx.diff() path, for a non-budgeted plan and for a budgeted plan whose batches are all empty. All three changed files are now free of uncovered lines and partial branches. Refs #169
![thrillhousebot[bot]](https://avatars.githubusercontent.com/in/3996985?s=60&v=4){kind=small}
Sonar flagged both matchers under java:S5843 — regex complexity 53 and 35 against an allowance of 20. Worth taking seriously rather than waving through: these patterns run over untrusted maintainer reply prose, and this is the code that decides when the bot may overrule a human. A single mega-alternation that intricate is hard to convince yourself fires only on genuine "concurrency is impossible" claims, and the override is only sound if it is high-confidence. Each alternation is now several small patterns, one per argument or construct, matched through a shared earliestMatch helper: - serialization claims: single-threaded / runs serially / cannot run concurrently / never runs concurrently / no race / only ever called from. - concurrent dispatch: pooled executor factory / a fixed pool wider than one thread / handing work to an executor / an async future / a raw thread, @async or a parallel stream. The matched set is unchanged except for two deliberate merges of alternatives that already subsumed one another ("only ever called from" now covers the bare "only called from"; "no race" covers "there is no race"), so nothing new matches. earliestMatch takes the leftmost match across every pattern, which is exactly what one alternation did, so splitting cannot change which sentence is quoted back at the maintainer — pinned by a test that fails under a first-pattern-wins implementation. Hardening the same untrusted-input surface while here: - every quantifier is bounded (\s{0,16} / \s{1,16}), and the fenced-block body is capped, so an unclosed fence cannot make the strip rescan from every opener. - replies over 20k characters are not analyzed at all. Skipping is the conservative outcome: an unread reply keeps its decline. Also switches the four Mockito calls to static imports, matching every neighbouring test in the package (java:S8924). Refs #169
|
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.
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
When a maintainer replies to decline a finding, the follow-up analysis recorded it
justifiedand the bot moved on — it never checked whether the stated rebuttalactually holds. A dismissal was treated as ground truth rather than a claim, so a
correct finding could be closed by an incorrect rebuttal, and the rebuttal
often names the very mechanism that makes the bug real.
This PR makes a decline a claim to verify. Two layers, both deliberately conservative:
1. Prompt rule (
PrReviewPrompts.SYSTEM,previous_findings_statuscontract).Before marking a prior finding
justified, trace the reply's stated reason againstthe code in the provided material. When that material plainly contradicts the
premise, keep the finding
unresolvedand quote the contradicting line in the note —never re-raise it as a new finding. The rule also spells out the conservative side:
override only at high confidence, on evidence quotable from the provided material;
style, intent, accepted risk, priority, or any premise whose supporting code is not in
context keep the decline.
2. Deterministic post-processing (
FollowUpAnalyzer.recheckDeclines+RebuttalContradiction), in the same shape as the existingsupersedeVanished/addUnreportedVanishedstatus rewriters, so the guarantee does not rest on the modelobeying prose. It detects exactly one high-precision family — the dogfood one — and
all three legs must hold:
serially", "only ever called from …"), judged on the reply with fenced blocks and
blockquotes stripped, so quoted material is never read as the maintainer's assertion;
newVirtualThreadPerTaskExecutor,newCachedThreadPool,executor.submit/execute,CompletableFuture.runAsync,new Thread(...),@Async,parallelStream()).The status is then rewritten
justified→unresolvedwith a one-line note quoting boththe claim and the contradicting line. Anything else — every rebuttal about style, intent,
accepted risk, or priority, and every premise not refutable from code text — matches nothing
and keeps the decline.
Safety properties:
maintainer reply. A second reply is the maintainer answering the push-back and always wins,
so the bot can never keep re-opening the same finding round after round.
unresolvedpath: it holds approval(
APPROVE→COMMENT) exactly like any other unresolved prior finding and is neverre-posted as a new inline comment, so nobody answers the same comment twice.
REVIEW_DECLINE_RECHECK_ENABLED=falsedisables the step outright, making a maintainerreply final.
This is also the gate that makes durable maintainer-feedback memory safe: only declines that
survive this re-check are sound enough to persist as learnings.
Known limitation (stated honestly). The deterministic step can only refute a rebuttal whose
contradicting code is inside the material the review call saw. In the dogfood PR the executor
producer itself was an unchanged file and would not have been visible — but the same PR's
CommentCommandServicechange was in the diff and containsexecutor.execute(() -> execute(ctx)),which is the evidence the regression test uses. When the mechanism lives entirely outside the
diff, only the prompt rule can catch it, and only when the model has that context.
Files
review/RebuttalContradiction.javareview/FollowUpAnalyzer.javarecheckDeclines(...)status rewriter, plus the thread/reply lookup and the enabled flag.review/VerdictBuilder.javabuild(...)afteraddUnreportedVanished; lazily supplies the reviewed diff (budget batches, elsectx.diff()).review/ai/PrReviewPrompts.javaprevious_findings_statuscontract.config/ThrillhouseConfig.java,application.propertiesthrillhousebot.review.decline-recheck-enabled(defaulttrue).README.md,.env.example.env.exampleentry, and a "Re-checking declines" section.Related Issues
Fixes #169
How Has This Been Tested?
New tests:
RebuttalContradictionTest(9 cases), 5 new cases inFollowUpAnalyzerTest, and anend-to-end wiring case in
VerdictBuilderTest. Both directions are covered, and each wasvalidated red/green by neutralizing only the production code.
(a) A code-contradicted rebuttal must not be recorded justified. Neutralized by early-returning
the statuses unchanged from
recheckDeclinesand returningOptional.empty()fromRebuttalContradiction.find:(b) A style/intent rebuttal must still be recorded justified. Neutralized in the other
direction, by widening the claim pattern so the re-check over-fires:
(c) The escape hatch and the config flag. Neutralized by relaxing the single-reply guard to
humanReplies.isEmpty()and by dropping the flag check:(d) The wiring. Removing the
recheckDeclinescall fromVerdictBuilder.build:(The assertion values above are quoted for markdown's sake; the runner prints them in angle
brackets.) Every one passes again with the production code restored. The regression case is
derived from the dogfood scenario: the
PrPauseService.pause()race finding, declined with"only ever called from the /pause command path, which runs asynchronously on the review executor
after the webhook has returned 200", against a diff containing
executor.execute(() -> execute(ctx));.Format / lint / suite:
./mvnw -B spotless:apply— clean./mvnw -B clean compile spotbugs:check spotless:check— BUILD SUCCESS,BugInstance size is 0./mvnw -B clean test— Tests run: 1892, Failures: 0, Errors: 0, Skipped: 0Unit tests
Integration tests
Manual testing
Checklist
Additional Notes
any premise the model can trace, including ones no regex will ever encode) but is unverifiable
and unenforceable. The deterministic step covers one narrow family with genuinely load-bearing
tests and, crucially, runs after the model — so it also catches the case where the model itself
accepted the bad rebuttal, which is exactly what happened in the dogfood PR. Neither alone is
sufficient.
the prompt rule but not the deterministic step on purpose: proving a guard is absent from a
partial diff is inference from missing evidence, which is the unsafe direction here.
PreviousFindingStatus.note, which is persisted and shown in thedashboard but not yet rendered in the summary markdown — the same as the existing superseded note.
Surfacing notes in the summary table felt like separate scope.