From 5752078b5f8a2834ba2d449d2b9821a05ddfe965 Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Sat, 8 Aug 2026 00:54:13 +0000 Subject: [PATCH 1/3] feat(review): re-check a maintainer's decline against the code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .env.example | 7 + README.md | 29 +++ .../config/ThrillhouseConfig.java | 11 + .../review/FollowUpAnalyzer.java | 130 ++++++++++- .../review/RebuttalContradiction.java | 206 ++++++++++++++++++ .../thrillhousebot/review/VerdictBuilder.java | 28 +++ .../review/ai/PrReviewPrompts.java | 16 ++ src/main/resources/application.properties | 4 + .../review/FollowUpAnalyzerTest.java | 159 ++++++++++++++ .../review/RebuttalContradictionTest.java | 141 ++++++++++++ .../review/VerdictBuilderTest.java | 111 ++++++++++ 11 files changed, 841 insertions(+), 1 deletion(-) create mode 100644 src/main/java/dev/thiagogonzaga/thrillhousebot/review/RebuttalContradiction.java create mode 100644 src/test/java/dev/thiagogonzaga/thrillhousebot/review/RebuttalContradictionTest.java diff --git a/.env.example b/.env.example index 7bef07fe..18c8f6cb 100644 --- a/.env.example +++ b/.env.example @@ -136,6 +136,13 @@ AI_TIMEOUT=300s # hedging guard still runs, and verifier errors always fail open. #REVIEW_VERIFIER_ENABLED=true +# Re-check a maintainer's decline against the reviewed code before recording a +# prior finding "justified" (default true). The finding stays open for one more +# round only when the reviewed diff plainly contradicts the stated reason; style, +# intent and accepted-risk rebuttals are respected, and a second reply on the +# thread always ends the re-check. false = a reply closes the finding outright. +#REVIEW_DECLINE_RECHECK_ENABLED=true + # When findings block the merge (REQUEST_CHANGES). Default balanced matches v0.x: # only CRITICAL/HIGH risk with HIGH confidence. Use strict for security-heavy # repos (any CRITICAL/HIGH blocks, even after the verifier demotes confidence). diff --git a/README.md b/README.md index d6f6eb71..02e839cd 100644 --- a/README.md +++ b/README.md @@ -243,6 +243,7 @@ will change per provider: | `WEBHOOK_BASE_BRANCHES` | Comma-separated globs; only auto-review PRs whose base branch matches one (e.g. `main,release/*`). Globs are gitignore-style: `*` does **not** cross `/`, so use `**` to span slashes (`**` alone matches every branch) | _(empty — all branches)_ | | `WEBHOOK_IGNORED_BASE_BRANCHES` | Comma-separated globs; skip auto-review of PRs whose base branch matches one (wins over allowlist; same `*`/`**` rule — match nested branches with `**`, e.g. `dependabot/**`) | _(empty)_ | | `REVIEW_VERIFIER_ENABLED` | Second, skeptical AI pass that re-checks each finding against the diff before posting, dropping or downgrading what it can't confirm (see [AI call budget](#ai-call-budget)); fails open — a verifier error keeps the original findings | `true` | +| `REVIEW_DECLINE_RECHECK_ENABLED` | Re-check a maintainer's decline against the reviewed code before a prior finding is recorded "justified" (see [Re-checking declines](#re-checking-declines)); the finding stays open for one more round only when the reviewed diff plainly contradicts the stated reason. `false` makes a maintainer reply close the finding unconditionally | `true` | | `REVIEW_BLOCKING_STRICTNESS` | When findings escalate to `REQUEST_CHANGES`: `balanced` (CRITICAL/HIGH + HIGH confidence), `strict` (any CRITICAL/HIGH), or `lenient` (CRITICAL + HIGH confidence only). See [Blocking strictness](#blocking-strictness) | `balanced` | | `REVIEW_CONVERSATIONAL_REPLIES_ENABLED` | Answer `@thrillhousebot` mentions in PR threads (including finding replies) with an AI reply | `true` | | `REVIEW_ADD_DOCS_ENABLED` | Allow the on-demand `/add-docs` command to generate docstrings as committable suggestions | `true` | @@ -281,6 +282,34 @@ cost of more false positives; a deterministic hedging guard still runs, and a verifier failure never blocks the review (it fails open, keeping the original findings). +### Re-checking declines + +When a maintainer replies to a finding to decline it, the follow-up analysis +records that finding as **justified** and the bot moves on. A dismissal is a +claim, though, not ground truth — a correct finding can 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's no race" — +on an executor that starts a thread per event). + +`REVIEW_DECLINE_RECHECK_ENABLED=true` (the default) therefore traces a decline's +stated reason against the code the review actually saw. When the reviewed diff +**plainly contradicts** that reason, the finding is kept **open for one more +round** with a note quoting both the claim and the contradicting line, instead +of being recorded justified. It is deliberately conservative: + +- Trusting the maintainer is the default. A rebuttal about house style, intent, + accepted risk, or priority — anything not refutable from the code — is + respected, as is any premise whose supporting code is not in the diff. +- **One push-back, then defer.** The re-check only fires while the thread carries + a single maintainer reply; replying again always ends it, so the bot can never + keep re-opening the same finding round after round. +- The re-opened finding is never re-posted as a new comment — it stays tracked in + *Previous Findings Status*, so nobody is asked to answer the same comment twice. +- The override holds approval (`APPROVE` → `COMMENT`) exactly like any other + unresolved previous finding; it never invents a new blocking finding. + +Set it to `false` to make a maintainer's reply final, unconditionally. + ### Blocking strictness By default (`REVIEW_BLOCKING_STRICTNESS=balanced`), only **CRITICAL** or **HIGH** diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/config/ThrillhouseConfig.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/config/ThrillhouseConfig.java index 41b986d4..994660d0 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/config/ThrillhouseConfig.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/config/ThrillhouseConfig.java @@ -204,6 +204,17 @@ interface ReviewConfig { @WithName("verifier-enabled") boolean verifierEnabled(); + /** + * Whether a maintainer's decline is re-checked against the reviewed code before a prior finding + * is recorded "justified". When the reviewed code plainly contradicts the rebuttal's premise + * the finding stays open for one more round; every other decline is respected, and a second + * reply on the thread always ends the re-check. Turn it off to make a maintainer's reply final, + * unconditionally. + */ + @WithDefault("true") + @WithName("decline-recheck-enabled") + boolean declineRecheckEnabled(); + /** * How severely a finding must score before the review escalates to {@code REQUEST_CHANGES}. One * of {@code balanced} (default — CRITICAL/HIGH + HIGH confidence), {@code strict} (any diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FollowUpAnalyzer.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FollowUpAnalyzer.java index 79957279..475fd3ae 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FollowUpAnalyzer.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FollowUpAnalyzer.java @@ -18,6 +18,7 @@ import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.ObjectMapper; import dev.thiagogonzaga.thrillhousebot.config.BotIdentity; +import dev.thiagogonzaga.thrillhousebot.config.ThrillhouseConfig; import dev.thiagogonzaga.thrillhousebot.github.GitHubReviewClient; import dev.thiagogonzaga.thrillhousebot.review.ai.FindingVerificationService; import dev.thiagogonzaga.thrillhousebot.review.ai.ReviewResponse; @@ -33,6 +34,7 @@ import java.util.Map; import java.util.Set; import java.util.function.Predicate; +import java.util.function.Supplier; import java.util.stream.Stream; /** Analyzes follow-up reviews by comparing new findings against prior reviews. */ @@ -71,9 +73,23 @@ public class FollowUpAnalyzer { private final ObjectMapper mapper; + /** Whether {@link #recheckDeclines} may override a maintainer decline; see the config key. */ + private final boolean declineRecheckEnabled; + @Inject - public FollowUpAnalyzer(ObjectMapper mapper) { + public FollowUpAnalyzer(ObjectMapper mapper, ThrillhouseConfig config) { + this(mapper, config.review().declineRecheckEnabled()); + } + + /** Visible for tests; the decline re-check is on, matching the shipped default. */ + FollowUpAnalyzer(ObjectMapper mapper) { + this(mapper, true); + } + + /** Visible for tests: pins the decline re-check flag. */ + FollowUpAnalyzer(ObjectMapper mapper, boolean declineRecheckEnabled) { this.mapper = mapper; + this.declineRecheckEnabled = declineRecheckEnabled; } /** @@ -705,6 +721,118 @@ private static boolean hasVanished( return !lineResolver.isFindingPresent(currentPath, finding.suggestionOld()); } + /** + * Re-checks a maintainer's decline against the code before it is recorded {@code justified}. The + * model reports a decline as an outcome; this step treats it as a claim. When the code + * the review actually saw plainly contradicts the rebuttal's premise ({@link + * RebuttalContradiction}), the status is rewritten back to {@code unresolved} with a one-line + * note quoting both the claim and the contradicting line — so a correct finding is not closed by + * an incorrect rebuttal, and only declines that survive the re-check are safe to remember. + * + *

Trusting the maintainer stays the default; every leg below must hold before an override + * fires, and any one of them missing leaves the {@code justified} status untouched: + * + *

+ * + *

Overridden findings re-enter the ordinary {@code unresolved} path — they hold APPROVE + * exactly like a model-reported unresolved status and are never re-posted as new findings, so the + * maintainer is not asked to answer the same comment twice. + * + * @param reviewedCode supplies the diff text the review call saw; resolved lazily because most + * rounds have no declined finding at all + */ + public List recheckDeclines( + List previous, + List statuses, + List inlineComments, + BotIdentity botIdentity, + Supplier reviewedCode) { + if (statuses == null || statuses.isEmpty()) { + return statuses == null ? List.of() : statuses; + } + if (!declineRecheckEnabled + || previous == null + || previous.isEmpty() + || inlineComments == null + || inlineComments.isEmpty() + || reviewedCode == null + || statuses.stream().noneMatch(s -> STATUS_JUSTIFIED.equalsIgnoreCase(s.status()))) { + return statuses; + } + String code = reviewedCode.get(); + if (code == null || code.isBlank()) { + return statuses; + } + var rewritten = new ArrayList(statuses.size()); + for (var status : statuses) { + var contradiction = declineContradiction(status, previous, inlineComments, botIdentity, code); + if (contradiction == null) { + rewritten.add(status); + continue; + } + Log.infof( + "Re-opening previous finding #%d: the maintainer's decline claims '%s' but the reviewed" + + " code shows '%s'", + status.id(), contradiction.claim(), contradiction.evidence()); + rewritten.add( + new ReviewResponse.PreviousFindingStatus( + status.id(), STATUS_UNRESOLVED, contradiction.note())); + } + return rewritten; + } + + /** + * The contradiction that disqualifies a {@code justified} status, or {@code null} when the + * decline stands. Returning {@code null} is the conservative outcome and is what every unmatched, + * absent, or ambiguous input produces. + */ + private static RebuttalContradiction.Contradiction declineContradiction( + ReviewResponse.PreviousFindingStatus status, + List previous, + List inlineComments, + BotIdentity botIdentity, + String reviewedCode) { + if (!STATUS_JUSTIFIED.equalsIgnoreCase(status.status())) { + return null; + } + var id = status.id(); + if (id < 1 || id > previous.size()) { + return null; + } + var finding = previous.get(id - 1); + Long rootId = rootCommentId(finding, id, inlineComments, botIdentity); + if (rootId == null) { + return null; + } + var humanReplies = humanReplies(rootId, inlineComments, botIdentity); + if (humanReplies.size() != 1) { + return null; + } + return RebuttalContradiction.find(finding, humanReplies.get(0), reviewedCode).orElse(null); + } + + /** Bodies of the maintainer replies on a thread, oldest first; bot replies are not rebuttals. */ + private static List humanReplies( + Long rootId, + List inlineComments, + BotIdentity botIdentity) { + return inlineComments.stream() + .filter(c -> rootId.equals(c.inReplyToId())) + .filter(c -> c.user() != null && !botIdentity.matches(c.user().login())) + .map(GitHubReviewClient.PullRequestComment::body) + .filter(body -> body != null && !body.isBlank()) + .toList(); + } + /** * Deterministic approve backstop. The bot's own prior findings the model silently dropped — still * present in the current diff, carrying no maintainer reply, and not closed by any round — diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/RebuttalContradiction.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/RebuttalContradiction.java new file mode 100644 index 00000000..1b9f2720 --- /dev/null +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/RebuttalContradiction.java @@ -0,0 +1,206 @@ +/* + * Copyright 2026 Thiago Gonzaga + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package dev.thiagogonzaga.thrillhousebot.review; + +import dev.thiagogonzaga.thrillhousebot.review.ai.ReviewResponse; +import java.util.Optional; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +/** + * Deterministic re-check of a maintainer's decline against the code the review actually saw. + * + *

A decline is a claim, not ground truth: a correct finding can be closed by an + * incorrect rebuttal, and the rebuttal often names the very mechanism that makes the bug real. This + * class answers one narrow question with high precision — does the reviewed code plainly contradict + * the rebuttal's premise? — and says nothing at all otherwise, so the default stays "trust the + * maintainer". + * + *

Exactly one contradiction family is detected, the one that is demonstrable from code text + * alone: a concurrency finding declined on a "this cannot run concurrently" premise + * while the reviewed code shows the path being dispatched onto a shared executor / new + * thread. All three legs must hold: + * + *

    + *
  1. the prior finding is about concurrency ({@link #CONCURRENCY_FINDING}) — a race, a + * check-then-act, thread-safety, atomicity; + *
  2. the maintainer's reply asserts that concurrency is impossible ({@link + * #NO_CONCURRENCY_CLAIM}) — "single-threaded", "runs serially", "only ever called from …"; + *
  3. the reviewed code contains a concurrent-dispatch construct ({@link #CONCURRENT_DISPATCH}) — + * an unbounded/pooled executor, {@code executor.submit/execute}, {@code CompletableFuture + * .runAsync}, {@code new Thread(...)}, {@code @Async}, {@code parallelStream()}. + *
+ * + *

Leg 2 is deliberately evaluated on the reply with fenced code blocks and blockquotes removed: + * a reply that merely quotes the bot's own finding, or pastes code, is not the maintainer asserting + * anything (the same "never act on quoted markdown" rule the comment-command parsers follow). + * + *

Everything else — house style, intent, accepted risk, priority, "we'll do it later", or any + * premise that is not refutable from code text — matches nothing here and keeps the decline. + * + *

Known limitation. The evidence must be inside the material the review call saw (the + * reviewed diff). When the contradicting mechanism lives in an unchanged file — the executor + * producer of the dogfood PR, say — this check cannot see it and stays silent; only the + * prompt-level rule in {@code PrReviewPrompts} can catch that case, and only when the model has + * that context. + */ +final class RebuttalContradiction { + + /** Max characters of the reply sentence / code line quoted back in the status note. */ + private static final int QUOTE_LIMIT = 120; + + /** The prior finding must itself be about concurrency for this family to apply. */ + private static final Pattern CONCURRENCY_FINDING = + Pattern.compile( + "race condition|data race|\\brace\\b|concurrent|concurrency|thread[- ]saf" + + "|time[- ]of[- ]check|toctou|check[- ]then[- ](?:act|insert|update|write)" + + "|atomicity|not atomic|interleav", + Pattern.CASE_INSENSITIVE); + + /** + * The maintainer asserting that the flagged path cannot run concurrently. "Only ever called from + * X" belongs here: a single call site is the usual way a decline argues away a race, and it is + * exactly the premise an asynchronous dispatch at that single call site refutes. + */ + private static final Pattern NO_CONCURRENCY_CLAIM = + Pattern.compile( + "single[- ]threaded|\\bsingle thread\\b|runs? serially|executed? serially|serialized" + + "|sequentially|one at a time|never (?:runs?|executes?|happens) concurrently" + + "|(?:cannot|can't|can not|never) (?:run|be|happen|occur)s? concurrent(?:ly)?" + + "|no concurrency|not concurrent|there is no race|no race|isn'?t a race" + + "|not a race|only ever (?:called|invoked|triggered|reached) from" + + "|only (?:called|invoked|triggered|reached) from|the only caller", + Pattern.CASE_INSENSITIVE); + + /** + * Code that dispatches work concurrently. A single call site that hands the work to any of these + * does not serialize it: two events each dispatch, and both run. + */ + private static final Pattern CONCURRENT_DISPATCH = + Pattern.compile( + "newVirtualThreadPerTaskExecutor|newCachedThreadPool|newWorkStealingPool" + + "|newFixedThreadPool\\s*\\(\\s*(?!1\\s*\\))|newScheduledThreadPool" + + "|\\.submit\\s*\\(|\\.execute\\s*\\(|CompletableFuture\\s*\\.\\s*(?:runAsync|supplyAsync)" + + "|new\\s+Thread\\s*\\(|@Async\\b|\\.parallelStream\\s*\\("); + + /** Fenced code blocks in a markdown reply — quoted material, never the maintainer's assertion. */ + private static final Pattern FENCED_BLOCK = + Pattern.compile("```.*?```", Pattern.DOTALL | Pattern.MULTILINE); + + private RebuttalContradiction() {} + + /** + * The contradiction between {@code rebuttal} and {@code reviewedCode} for {@code finding}, or + * empty when there is none — which is the overwhelmingly common case and means the decline + * stands. + */ + static Optional find( + ReviewResponse.Finding finding, String rebuttal, String reviewedCode) { + if (finding == null || rebuttal == null || reviewedCode == null || reviewedCode.isBlank()) { + return Optional.empty(); + } + if (!CONCURRENCY_FINDING.matcher(findingText(finding)).find()) { + return Optional.empty(); + } + Matcher claim = NO_CONCURRENCY_CLAIM.matcher(assertedText(rebuttal)); + if (!claim.find()) { + return Optional.empty(); + } + Matcher evidence = CONCURRENT_DISPATCH.matcher(reviewedCode); + if (!evidence.find()) { + return Optional.empty(); + } + return Optional.of( + new Contradiction( + sentenceAround(assertedText(rebuttal), claim.start()), + lineAround(reviewedCode, evidence.start()))); + } + + /** The quoted claim and the quoted code line that refutes it, both already trimmed for a note. */ + record Contradiction(String claim, String evidence) { + + /** One-line status note naming the contradiction, for {@code previous_findings_status}. */ + String note() { + return "Decline re-checked against the code and not accepted: the reply argues \"" + + claim + + "\", but the reviewed code dispatches this path concurrently — \"" + + evidence + + "\". A single call site does not serialize work handed to an executor or a new" + + " thread, so the premise does not refute the finding. Reply again to keep the" + + " decline."; + } + } + + private static String findingText(ReviewResponse.Finding finding) { + return (finding.title() == null ? "" : finding.title()) + + "\n" + + (finding.description() == null ? "" : finding.description()); + } + + /** + * The reply with fenced code blocks and blockquoted lines removed, lower-cased boundaries intact + * — what the maintainer actually asserts, as opposed to what they quote. + */ + private static String assertedText(String rebuttal) { + var withoutFences = FENCED_BLOCK.matcher(rebuttal).replaceAll(" "); + var sb = new StringBuilder(withoutFences.length()); + for (var line : withoutFences.split("\n", -1)) { + if (line.stripLeading().startsWith(">")) { + continue; + } + sb.append(line).append('\n'); + } + return sb.toString(); + } + + /** The sentence containing {@code index}, collapsed to one line and clipped for a note. */ + private static String sentenceAround(String text, int index) { + var start = index; + while (start > 0 && !isSentenceEnd(text.charAt(start - 1))) { + start--; + } + var end = index; + while (end < text.length() && !isSentenceEnd(text.charAt(end))) { + end++; + } + return clip(text.substring(start, Math.min(end + 1, text.length()))); + } + + private static boolean isSentenceEnd(char c) { + return c == '.' || c == '\n' || c == '!' || c == '?' || c == ';'; + } + + /** The source line containing {@code index}, clipped for a note. */ + private static String lineAround(String text, int index) { + var start = text.lastIndexOf('\n', index) + 1; + var end = text.indexOf('\n', index); + return clip(text.substring(start, end < 0 ? text.length() : end)); + } + + /** + * Collapses whitespace, drops a leading unified-diff marker, and clips to {@link #QUOTE_LIMIT}. + */ + private static String clip(String raw) { + var collapsed = raw.replaceAll("\\s+", " ").strip(); + if (!collapsed.isEmpty() && (collapsed.charAt(0) == '+' || collapsed.charAt(0) == '-')) { + collapsed = collapsed.substring(1).strip(); + } + if (collapsed.length() <= QUOTE_LIMIT) { + return collapsed; + } + return collapsed.substring(0, QUOTE_LIMIT).stripTrailing() + "…"; + } +} diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/VerdictBuilder.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/VerdictBuilder.java index f91989a4..d5c8daab 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/VerdictBuilder.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/VerdictBuilder.java @@ -155,6 +155,15 @@ ReviewResult build( ctx.lineResolver(), currentRenameTargets); } + // A maintainer's decline is a claim, not ground truth: a "justified" whose stated reason the + // reviewed code plainly contradicts goes back to "unresolved" for one more round (#169). + effectiveStatuses = + followUpAnalyzer.recheckDeclines( + ctx.previousFindingsList(), + effectiveStatuses, + ctx.inlineComments(), + botIdentity, + () -> reviewedCode(ctx, plan)); var effectiveResponse = new ReviewResponse(aiResponse.findings(), effectiveStatuses, aiResponse.summary()); var unresolvedPrevious = @@ -330,6 +339,25 @@ static List overviewFiles( return merged; } + /** + * The diff text the review call(s) actually saw — the only material a decline may be re-checked + * against. With token budgeting on, {@code ctx.diff()} is empty and the planned batches are + * authoritative for what the model received, so they are concatenated; with budgeting disabled + * the legacy single diff is it. Resolved lazily by {@link FollowUpAnalyzer#recheckDeclines}, so a + * round with no declined finding never pays for the concatenation. + */ + private static String reviewedCode( + ReviewContextLoader.ReviewContext ctx, DiffBudgetPlanner.BudgetPlan plan) { + if (!plan.budgeted() || plan.batches().isEmpty()) { + return ctx.diff(); + } + var sb = new StringBuilder(); + for (var batch : plan.batches()) { + sb.append(batch.text()).append('\n'); + } + return sb.toString(); + } + /** * Maps a prior path to its current rename target; blank means a content-identical pure rename. */ diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/PrReviewPrompts.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/PrReviewPrompts.java index bc3de5c0..21711db1 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/PrReviewPrompts.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/PrReviewPrompts.java @@ -237,6 +237,22 @@ same enclosing unit (the same function, block, or scope). When the two places ar - "justified" means the issue is intentionally not fixed and a thread reply gives a concrete reason (intentional behavior, disputed with evidence, explicitly deferred); a reply that only acknowledges the finding leaves it "unresolved" + - A reply that declines a finding is a CLAIM TO VERIFY, not ground truth. Before + marking one "justified", trace its stated reason against the code in the provided + material. When that material PLAINLY CONTRADICTS the premise, mark the finding + "unresolved" instead and quote the contradicting line in the note — for example a + reply saying the path "runs single-threaded / serially / only from one caller, so + there is no race" while the code hands that path to a shared or unbounded executor, + a new thread, or an async dispatch (running after the ack is not running serially); + or a reply saying "the caller already guards X" while the caller shown here does + not. Do NOT re-raise such a finding as a new finding — it stays tracked through + previous_findings_status + - Override a decline ONLY at high confidence, and only on evidence you can quote from + the provided material. Trusting the maintainer is the default: a reply about house + style, intent, accepted risk, priority, or anything else not refutable from the code + is a valid justification, and so is any premise whose supporting code is not in the + provided material. When the evidence is absent, partial, or ambiguous, mark the + finding "justified" and move on - Never emit a new finding that duplicates ANY prior finding, whatever its status — prior findings are tracked exclusively through previous_findings_status, and re-stating one as a new finding double-posts it. If you disagree with a thread diff --git a/src/main/resources/application.properties b/src/main/resources/application.properties index 1ef456e0..eeab3f91 100644 --- a/src/main/resources/application.properties +++ b/src/main/resources/application.properties @@ -133,6 +133,10 @@ thrillhousebot.review.max-diff-lines=${REVIEW_MAX_DIFF_LINES:5000} thrillhousebot.review.instructions-file=.github/thrillhousebot.md # Second-pass AI audit that drops/downgrades unverifiable findings before they are posted thrillhousebot.review.verifier-enabled=${REVIEW_VERIFIER_ENABLED:true} +# Re-check a maintainer's decline against the reviewed code before recording a prior finding +# "justified": the finding stays open one more round only when the reviewed code plainly +# contradicts the stated reason. false = a maintainer reply closes the finding unconditionally. +thrillhousebot.review.decline-recheck-enabled=${REVIEW_DECLINE_RECHECK_ENABLED:true} # When a finding blocks the merge (REQUEST_CHANGES): balanced (default) = CRITICAL/HIGH + HIGH # confidence; strict = any CRITICAL/HIGH; lenient = CRITICAL + HIGH confidence only. thrillhousebot.review.blocking-strictness=${REVIEW_BLOCKING_STRICTNESS:balanced} diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/FollowUpAnalyzerTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/FollowUpAnalyzerTest.java index 9d9a00d2..06e7861f 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/FollowUpAnalyzerTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/FollowUpAnalyzerTest.java @@ -1940,4 +1940,163 @@ void unreportedUnresolvedShouldExcludeDriftedReRaisedFindingIfAnyMemberHasReply( assertTrue(held.isEmpty()); } + + // --- recheckDeclines: a maintainer's decline is a claim to verify, not ground truth (#169) --- + + private static final String PAUSE_FILE = + "src/main/java/dev/thiagogonzaga/thrillhousebot/webhook/PrPauseService.java"; + + private static final String RACE_TITLE = + "Race condition in pause() can cause a UniqueConstraint violation under concurrent webhooks"; + + /** The dogfood prior finding from PR #160 — correct, low confidence, "verify before acting". */ + private static final List RACE_PREVIOUS = + List.of( + new ReviewResponse.Finding( + "medium", + "low", + PAUSE_FILE, + 60, + RACE_TITLE, + "pause() checks for an existing PausedPr and then inserts one; two deliveries can" + + " both pass the check before either inserts.", + null, + null)); + + /** + * The same PR's command path: every command is handed to the shared review executor, so the + * "single call site, runs after the ack" premise does not serialize anything. + */ + private static final String DISPATCHING_DIFF = + """ + diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/webhook/CommentCommandService.java + @@ -130,7 +130,9 @@ public class CommentCommandService { + + private void dispatch(CommandContext ctx) { + + executor.execute(() -> execute(ctx)); + + } + """; + + private static List raceThread(String... humanReplies) { + var comments = new java.util.ArrayList(); + comments.add(comment(700L, null, PAUSE_FILE, "**MEDIUM — " + RACE_TITLE + "**", BOT)); + for (var i = 0; i < humanReplies.length; i++) { + comments.add(comment(701L + i, 700L, PAUSE_FILE, humanReplies[i], "maintainer")); + } + return List.copyOf(comments); + } + + private static List justified() { + return List.of( + new ReviewResponse.PreviousFindingStatus( + 1, "justified", "maintainer says the path cannot run concurrently")); + } + + @Test + void recheckShouldReopenDeclineWhoseAsyncAfterAckPremiseTheReviewedCodeContradicts() { + var comments = + raceThread( + "Not changed — pause() is only ever called from the /pause command path, which runs" + + " asynchronously on the review executor after the webhook has returned 200."); + + var rechecked = + analyzer.recheckDeclines( + RACE_PREVIOUS, justified(), comments, BOT_ID, () -> DISPATCHING_DIFF); + + assertEquals(1, rechecked.size()); + assertEquals( + "unresolved", + rechecked.get(0).status(), + "a decline the reviewed code contradicts must not be recorded justified"); + assertTrue( + rechecked.get(0).note().contains("only ever called from"), + "the note must quote the maintainer's claim, was: " + rechecked.get(0).note()); + assertTrue( + rechecked.get(0).note().contains("executor.execute(() -> execute(ctx));"), + "the note must quote the contradicting line, was: " + rechecked.get(0).note()); + } + + @Test + void recheckShouldKeepDeclineThatRestsOnStyleOrIntent() { + var comments = + raceThread("Intentional — this is the house style for command handlers. Not changing it."); + + var rechecked = + analyzer.recheckDeclines( + RACE_PREVIOUS, justified(), comments, BOT_ID, () -> DISPATCHING_DIFF); + + assertEquals( + "justified", + rechecked.get(0).status(), + "a rebuttal that is not refutable from the code must be respected"); + } + + @Test + void recheckShouldDeferOnceTheMaintainerHasAnsweredTwice() { + var comments = + raceThread( + "Not changed — pause() is only ever called from the /pause command path, which runs" + + " asynchronously on the review executor after the webhook has returned 200.", + "Still no — I looked, the executor never runs two of these for one PR."); + + var rechecked = + analyzer.recheckDeclines( + RACE_PREVIOUS, justified(), comments, BOT_ID, () -> DISPATCHING_DIFF); + + assertEquals( + "justified", + rechecked.get(0).status(), + "a second maintainer reply answers the push-back and always wins"); + } + + @Test + void recheckShouldBeDisabledByConfig() { + var disabled = new FollowUpAnalyzer(new ObjectMapper(), false); + var comments = + raceThread( + "Not changed — pause() is only ever called from the /pause command path, which runs" + + " asynchronously on the review executor after the webhook has returned 200."); + + var rechecked = + disabled.recheckDeclines( + RACE_PREVIOUS, justified(), comments, BOT_ID, () -> DISPATCHING_DIFF); + + assertEquals("justified", rechecked.get(0).status()); + } + + @Test + void recheckShouldLeaveNonJustifiedStatusesAndUnmatchableInputsAlone() { + var unresolved = + List.of(new ReviewResponse.PreviousFindingStatus(1, "unresolved", "still there")); + var comments = + raceThread( + "Not changed — pause() is only ever called from the /pause command path, which runs" + + " asynchronously on the review executor after the webhook has returned 200."); + + assertEquals( + unresolved, + analyzer.recheckDeclines( + RACE_PREVIOUS, unresolved, comments, BOT_ID, () -> DISPATCHING_DIFF)); + // No thread to read the rebuttal from. + assertEquals( + "justified", + analyzer + .recheckDeclines(RACE_PREVIOUS, justified(), List.of(), BOT_ID, () -> DISPATCHING_DIFF) + .get(0) + .status()); + // No reviewed code to check the rebuttal against. + assertEquals( + "justified", + analyzer + .recheckDeclines(RACE_PREVIOUS, justified(), comments, BOT_ID, () -> "") + .get(0) + .status()); + // Status id outside the prior round. + var outOfRange = + List.of(new ReviewResponse.PreviousFindingStatus(9, "justified", "no such finding")); + assertEquals( + outOfRange, + analyzer.recheckDeclines( + RACE_PREVIOUS, outOfRange, comments, BOT_ID, () -> DISPATCHING_DIFF)); + assertTrue(analyzer.recheckDeclines(RACE_PREVIOUS, null, comments, BOT_ID, null).isEmpty()); + } } diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/RebuttalContradictionTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/RebuttalContradictionTest.java new file mode 100644 index 00000000..0e907e11 --- /dev/null +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/RebuttalContradictionTest.java @@ -0,0 +1,141 @@ +/* + * Copyright 2026 Thiago Gonzaga + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package dev.thiagogonzaga.thrillhousebot.review; + +import static org.junit.jupiter.api.Assertions.*; + +import dev.thiagogonzaga.thrillhousebot.review.ai.ReviewResponse; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + +class RebuttalContradictionTest { + + /** The dogfood finding: PrPauseService.pause() check-then-insert under concurrent webhooks. */ + private static final ReviewResponse.Finding RACE_FINDING = + new ReviewResponse.Finding( + "medium", + "low", + "src/main/java/dev/thiagogonzaga/thrillhousebot/webhook/PrPauseService.java", + 60, + "Race condition in pause() can cause a UniqueConstraint violation under concurrent" + + " webhooks", + "pause() checks for an existing PausedPr and then inserts one. Two deliveries can both" + + " pass the check before either inserts — low confidence, verify before acting.", + null, + null); + + /** The command path from the same PR: each command is handed to the shared review executor. */ + private static final String DISPATCHING_CODE = + """ + diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/webhook/CommentCommandService.java + @@ -130,7 +130,9 @@ public class CommentCommandService { + + private void dispatch(CommandContext ctx) { + + executor.execute(() -> execute(ctx)); + + } + """; + + private static final String SERIAL_CODE = + """ + diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/webhook/CommentCommandService.java + @@ -130,7 +130,9 @@ public class CommentCommandService { + + private void dispatch(CommandContext ctx) { + + execute(ctx); + + } + """; + + @Test + void shouldContradictAsyncAfterAckRebuttalWhenTheCodeDispatchesConcurrently() { + var rebuttal = + "Not changed — pause() is only ever called from the /pause command path, which runs" + + " asynchronously on the review executor after the webhook has returned 200."; + + var contradiction = RebuttalContradiction.find(RACE_FINDING, rebuttal, DISPATCHING_CODE); + + assertTrue( + contradiction.isPresent(), + "the async-after-ack rebuttal is refuted by executor.execute(...) in the reviewed code"); + assertTrue( + contradiction.get().claim().contains("only ever called from"), + "the note must quote the maintainer's claim, was: " + contradiction.get().claim()); + assertEquals("executor.execute(() -> execute(ctx));", contradiction.get().evidence()); + assertTrue(contradiction.get().note().contains("executor.execute(() -> execute(ctx));")); + } + + @ParameterizedTest + @ValueSource( + strings = { + "Intentional — this is the house style for command handlers and we're keeping it.", + "We accept this risk; the window is tiny and a retry fixes it.", + "Not worth it right now, deferring to the v0.7 cleanup.", + "Won't fix — that's what the product wants here.", + }) + void shouldRespectRebuttalsThatAreNotRefutableFromCode(String rebuttal) { + assertTrue( + RebuttalContradiction.find(RACE_FINDING, rebuttal, DISPATCHING_CODE).isEmpty(), + "style / intent / accepted-risk rebuttals must keep the decline"); + } + + @Test + void shouldRespectDeclineWhenTheCodeShowsNoConcurrentDispatch() { + var rebuttal = "pause() is only ever called from the /pause command path — single-threaded."; + + assertTrue( + RebuttalContradiction.find(RACE_FINDING, rebuttal, SERIAL_CODE).isEmpty(), + "without concurrent-dispatch evidence in the reviewed code the maintainer is trusted"); + } + + @Test + void shouldRespectDeclineWhenTheFindingIsNotAboutConcurrency() { + var styleFinding = + new ReviewResponse.Finding( + "low", "high", "src/A.java", 3, "Method name is misleading", "rename it", null, null); + + assertTrue( + RebuttalContradiction.find( + styleFinding, "It only ever runs single-threaded anyway.", DISPATCHING_CODE) + .isEmpty(), + "the concurrency family must not fire on a finding that is not about concurrency"); + } + + @Test + void shouldIgnoreClaimsThatAppearOnlyInQuotedMarkdown() { + var rebuttal = + """ + > Race condition — two deliveries can both pass the check. It is not single-threaded. + + ```java + // only ever called from the command path + ``` + + Yes, agreed in principle, but we are not changing it in this PR. + """; + + assertTrue( + RebuttalContradiction.find(RACE_FINDING, rebuttal, DISPATCHING_CODE).isEmpty(), + "a blockquote or fenced block is quoted material, not the maintainer's own assertion"); + } + + @Test + void shouldRespectDeclineWhenThereIsNoCodeToCheckAgainst() { + var rebuttal = "It is single-threaded, so there is no race."; + + assertTrue(RebuttalContradiction.find(RACE_FINDING, rebuttal, "").isEmpty()); + assertTrue(RebuttalContradiction.find(RACE_FINDING, rebuttal, null).isEmpty()); + assertTrue(RebuttalContradiction.find(RACE_FINDING, null, DISPATCHING_CODE).isEmpty()); + assertTrue(RebuttalContradiction.find(null, rebuttal, DISPATCHING_CODE).isEmpty()); + } +} diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/VerdictBuilderTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/VerdictBuilderTest.java index 58672b2d..789e43f4 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/VerdictBuilderTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/VerdictBuilderTest.java @@ -79,6 +79,13 @@ class VerdictBuilderTest { List statuses = inv.getArgument(1); return statuses == null ? List.of() : statuses; }); + lenient() + .when(followUpAnalyzer.recheckDeclines(any(), any(), any(), any(), any())) + .thenAnswer( + inv -> { + List statuses = inv.getArgument(1); + return statuses == null ? List.of() : statuses; + }); lenient() .when(summaryGenerator.generate(anyInt(), anyInt(), anyInt(), any(), any(), any())) .thenReturn(""); @@ -289,6 +296,110 @@ void unresolvedPriorFindingStillInTheDiffKeepsHoldingApprove() { assertFalse(result.hasSupersededPrevious()); } + /** Prior round from the dogfood PR: the pause() race the maintainer went on to decline. */ + private static final String RACE_FILE = "src/main/java/.../webhook/PrPauseService.java"; + + private static final String RACE_TITLE = + "Race condition in pause() can cause a UniqueConstraint violation under concurrent webhooks"; + + private static final ReviewResponse RACE_PRIOR_RESPONSE = + new ReviewResponse( + List.of( + new ReviewResponse.Finding( + "medium", + "low", + RACE_FILE, + 60, + RACE_TITLE, + "pause() checks for an existing PausedPr and then inserts one; two deliveries" + + " can both pass the check before either inserts.", + "if (repository.find(pr) == null) {", + null)), + List.of(), + null); + + @Test + void declinedPriorFindingTheReviewedCodeContradictsStaysOpenAndHoldsApprove() { + var realBuilder = + new VerdictBuilder( + summaryGenerator, + new FollowUpAnalyzer(new com.fasterxml.jackson.databind.ObjectMapper()), + BotIdentity.from(List.of("thrillhousebot[bot]")), + BlockingStrictness.BALANCED); + var batchText = + """ + diff --git a/src/main/java/.../webhook/CommentCommandService.java + @@ -130,7 +130,9 @@ public class CommentCommandService { + + private void dispatch(CommandContext ctx) { + + executor.execute(() -> execute(ctx)); + + } + """; + var plan = + new DiffBudgetPlanner.BudgetPlan( + List.of(new DiffBudgetPlanner.DiffBatch(batchText, List.of(), 10)), + List.of(), + List.of(), + true); + var comments = + List.of( + new dev.thiagogonzaga.thrillhousebot.github.GitHubReviewClient.PullRequestComment( + 700L, + null, + RACE_FILE, + "**MEDIUM — " + RACE_TITLE + "**", + new dev.thiagogonzaga.thrillhousebot.github.GitHubReviewClient.ReviewResponse.User( + "thrillhousebot[bot]")), + new dev.thiagogonzaga.thrillhousebot.github.GitHubReviewClient.PullRequestComment( + 701L, + 700L, + RACE_FILE, + "Not changed — pause() is only ever called from the /pause command path, which" + + " runs asynchronously on the review executor after the webhook has returned" + + " 200.", + new dev.thiagogonzaga.thrillhousebot.github.GitHubReviewClient.ReviewResponse.User( + "maintainer"))); + var ctx = + new ReviewContextLoader.ReviewContext( + List.of(), + "", + "", + 0, + List.of(), + List.of("{}"), + List.of(RACE_PRIOR_RESPONSE), + false, + true, + "{}", + comments, + "", + new InstructionsResolver.ResolvedInstructions("", ""), + List.of(), + "", + "", + List.of(new FileDiff(RACE_FILE, "modified", 1, 0, 1, "")), + () -> + new DiffLineResolver( + Map.of( + RACE_FILE, "@@ -60,1 +60,1 @@\n-old\n+if (repository.find(pr) == null) {")), + null); + var declined = + new ReviewResponse( + List.of(), + List.of( + new ReviewResponse.PreviousFindingStatus( + 1, "justified", "maintainer says the path cannot run concurrently")), + null); + + var result = realBuilder.build(ctx, declined, CI_CLEAR, plan); + + assertEquals(1, result.unresolvedPreviousCount()); + assertEquals(ReviewState.COMMENT, result.reviewState()); + assertTrue( + result.previousStatuses().get(0).note().contains("executor.execute(() -> execute(ctx));"), + "the re-opened status must name the contradiction, was: " + + result.previousStatuses().get(0).note()); + } + @Test void disabledBudgetingDisclosesTheLegacyLineCapCount() { var ctx = contextWithLineCapOmissions(2); From 5c7b84861a7fddf863ccba36ee2fa8e2d805569e Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Sat, 8 Aug 2026 01:46:02 +0000 Subject: [PATCH 2/3] test(review): cover every decline re-check branch 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 --- .../review/FollowUpAnalyzer.java | 19 +- .../review/RebuttalContradiction.java | 15 +- .../review/FollowUpAnalyzerTest.java | 134 +++++++++++++ .../review/RebuttalContradictionTest.java | 102 ++++++++++ .../review/VerdictBuilderTest.java | 182 +++++++++++------- 5 files changed, 372 insertions(+), 80 deletions(-) diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FollowUpAnalyzer.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FollowUpAnalyzer.java index 475fd3ae..b3283342 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FollowUpAnalyzer.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FollowUpAnalyzer.java @@ -759,17 +759,13 @@ public List recheckDeclines( if (statuses == null || statuses.isEmpty()) { return statuses == null ? List.of() : statuses; } - if (!declineRecheckEnabled - || previous == null - || previous.isEmpty() - || inlineComments == null - || inlineComments.isEmpty() - || reviewedCode == null - || statuses.stream().noneMatch(s -> STATUS_JUSTIFIED.equalsIgnoreCase(s.status()))) { + if (!declineRecheckEnabled || !hasDecline(statuses)) { return statuses; } - String code = reviewedCode.get(); - if (code == null || code.isBlank()) { + // An empty prior round and an empty comment list need no fast path of their own: the + // id-range check and the thread lookup below already yield "no contradiction" for both. + String code = reviewedCode == null ? null : reviewedCode.get(); + if (previous == null || inlineComments == null || code == null || code.isBlank()) { return statuses; } var rewritten = new ArrayList(statuses.size()); @@ -790,6 +786,11 @@ public List recheckDeclines( return rewritten; } + /** Whether any status is a maintainer decline — the only kind this re-check looks at. */ + private static boolean hasDecline(List statuses) { + return statuses.stream().anyMatch(s -> STATUS_JUSTIFIED.equalsIgnoreCase(s.status())); + } + /** * The contradiction that disqualifies a {@code justified} status, or {@code null} when the * decline stands. Returning {@code null} is the conservative outcome and is what every unmatched, diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/RebuttalContradiction.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/RebuttalContradiction.java index 1b9f2720..5a9c326e 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/RebuttalContradiction.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/RebuttalContradiction.java @@ -16,6 +16,7 @@ package dev.thiagogonzaga.thrillhousebot.review; import dev.thiagogonzaga.thrillhousebot.review.ai.ReviewResponse; +import java.util.ArrayList; import java.util.Optional; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -156,14 +157,15 @@ private static String findingText(ReviewResponse.Finding finding) { */ private static String assertedText(String rebuttal) { var withoutFences = FENCED_BLOCK.matcher(rebuttal).replaceAll(" "); - var sb = new StringBuilder(withoutFences.length()); + var kept = new ArrayList(); for (var line : withoutFences.split("\n", -1)) { - if (line.stripLeading().startsWith(">")) { - continue; + if (!line.stripLeading().startsWith(">")) { + kept.add(line); } - sb.append(line).append('\n'); } - return sb.toString(); + // Joined, not terminated: a reply that ends mid-sentence must stay unterminated, so + // sentenceAround's end-of-text bound is a live case rather than an unreachable guard. + return String.join("\n", kept); } /** The sentence containing {@code index}, collapsed to one line and clipped for a note. */ @@ -195,7 +197,8 @@ private static String lineAround(String text, int index) { */ private static String clip(String raw) { var collapsed = raw.replaceAll("\\s+", " ").strip(); - if (!collapsed.isEmpty() && (collapsed.charAt(0) == '+' || collapsed.charAt(0) == '-')) { + // startsWith, not charAt, so no emptiness guard is needed for a blank quoted line. + if (collapsed.startsWith("+") || collapsed.startsWith("-")) { collapsed = collapsed.substring(1).strip(); } if (collapsed.length() <= QUOTE_LIMIT) { diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/FollowUpAnalyzerTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/FollowUpAnalyzerTest.java index 06e7861f..8ca3bdc1 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/FollowUpAnalyzerTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/FollowUpAnalyzerTest.java @@ -20,6 +20,7 @@ import com.fasterxml.jackson.databind.ObjectMapper; import dev.thiagogonzaga.thrillhousebot.config.BotIdentity; +import dev.thiagogonzaga.thrillhousebot.config.ThrillhouseConfig; import dev.thiagogonzaga.thrillhousebot.github.GitHubReviewClient; import dev.thiagogonzaga.thrillhousebot.review.ai.ReviewResponse; import java.util.List; @@ -2099,4 +2100,137 @@ void recheckShouldLeaveNonJustifiedStatusesAndUnmatchableInputsAlone() { RACE_PREVIOUS, outOfRange, comments, BOT_ID, () -> DISPATCHING_DIFF)); assertTrue(analyzer.recheckDeclines(RACE_PREVIOUS, null, comments, BOT_ID, null).isEmpty()); } + + /** + * Each way the re-check can find nothing to work with. Every one must hand the statuses back + * exactly as the model reported them — the conservative outcome — and none may reach the matcher. + */ + static Stream recheckNoOpInputs() { + var thread = + raceThread( + "Not changed — pause() is only ever called from the /pause command path, which runs" + + " asynchronously on the review executor after the webhook has returned 200."); + return Stream.of( + arguments( + "no statuses at all", RACE_PREVIOUS, List.of(), thread, supplier(DISPATCHING_DIFF)), + arguments("no prior round", null, justified(), thread, supplier(DISPATCHING_DIFF)), + arguments("empty prior round", List.of(), justified(), thread, supplier(DISPATCHING_DIFF)), + arguments( + "no inline comments", RACE_PREVIOUS, justified(), null, supplier(DISPATCHING_DIFF)), + arguments("no code supplier", RACE_PREVIOUS, justified(), thread, null), + arguments("supplier yields null", RACE_PREVIOUS, justified(), thread, supplier(null)), + arguments("supplier yields blank", RACE_PREVIOUS, justified(), thread, supplier(" \n")), + arguments( + "id below the prior round", + RACE_PREVIOUS, + List.of(new ReviewResponse.PreviousFindingStatus(0, "justified", "bad id")), + thread, + supplier(DISPATCHING_DIFF)), + arguments( + "thread cannot be located", + RACE_PREVIOUS, + justified(), + List.of(comment(900L, null, "src/Unrelated.java", "**LOW — something else**", BOT)), + supplier(DISPATCHING_DIFF))); + } + + private static java.util.function.Supplier supplier(String value) { + return () -> value; + } + + @ParameterizedTest(name = "{0}") + @MethodSource("recheckNoOpInputs") + void recheckShouldReturnStatusesUntouchedWhenThereIsNothingToVerify( + String name, + List previous, + List statuses, + List comments, + java.util.function.Supplier code) { + assertEquals( + statuses, + analyzer.recheckDeclines(previous, statuses, comments, BOT_ID, code), + "the decline must survive untouched when the re-check has nothing to verify: " + name); + } + + @Test + void recheckShouldOnlyRewriteTheDeclinedEntryOfAMixedStatusList() { + var twoFindings = + List.of( + RACE_PREVIOUS.get(0), + new ReviewResponse.Finding( + "low", "high", "src/B.java", 5, "Missing null check", "may NPE", null, null)); + var mixed = + List.of( + new ReviewResponse.PreviousFindingStatus(1, "justified", "cannot run concurrently"), + new ReviewResponse.PreviousFindingStatus(2, "resolved", "fixed in abc123")); + var comments = + raceThread( + "Not changed — pause() is only ever called from the /pause command path, which runs" + + " asynchronously on the review executor after the webhook has returned 200."); + + var rechecked = + analyzer.recheckDeclines(twoFindings, mixed, comments, BOT_ID, () -> DISPATCHING_DIFF); + + assertEquals("unresolved", rechecked.get(0).status()); + assertEquals( + mixed.get(1), + rechecked.get(1), + "a non-declined status must pass through the re-check byte for byte"); + } + + @Test + void recheckShouldIgnoreBotAnonymousAndEmptyRepliesWhenCountingTheRebuttal() { + // Only ONE real maintainer reply is present; the bot's own follow-up, an author-less reply and + // the body-less ones (null, blank) must not count as a second human answer that would end the + // re-check. + var comments = + List.of( + comment(700L, null, PAUSE_FILE, "**MEDIUM — " + RACE_TITLE + "**", BOT), + comment( + 701L, + 700L, + PAUSE_FILE, + "Not changed — pause() is only ever called from the /pause command path, which" + + " runs asynchronously on the review executor after the webhook has returned" + + " 200.", + "maintainer"), + comment(702L, 700L, PAUSE_FILE, "Thanks, noted.", BOT), + new GitHubReviewClient.PullRequestComment( + 703L, 700L, PAUSE_FILE, "anonymous reply", null), + comment(704L, 700L, PAUSE_FILE, " ", "maintainer"), + comment(705L, 700L, PAUSE_FILE, null, "maintainer"), + comment(706L, 800L, PAUSE_FILE, "reply on a different thread", "maintainer")); + + var rechecked = + analyzer.recheckDeclines( + RACE_PREVIOUS, justified(), comments, BOT_ID, () -> DISPATCHING_DIFF); + + assertEquals( + "unresolved", + rechecked.get(0).status(), + "bot, author-less, body-less and other-thread replies are not the maintainer answering" + + " the push-back"); + } + + @Test + void injectedAnalyzerShouldTakeTheRecheckFlagFromConfig() { + var review = org.mockito.Mockito.mock(ThrillhouseConfig.ReviewConfig.class); + org.mockito.Mockito.when(review.declineRecheckEnabled()).thenReturn(false); + var config = org.mockito.Mockito.mock(ThrillhouseConfig.class); + org.mockito.Mockito.when(config.review()).thenReturn(review); + var comments = + raceThread( + "Not changed — pause() is only ever called from the /pause command path, which runs" + + " asynchronously on the review executor after the webhook has returned 200."); + + var configured = new FollowUpAnalyzer(new ObjectMapper(), config); + + assertEquals( + "justified", + configured + .recheckDeclines(RACE_PREVIOUS, justified(), comments, BOT_ID, () -> DISPATCHING_DIFF) + .get(0) + .status(), + "the injected constructor must honour thrillhousebot.review.decline-recheck-enabled"); + } } diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/RebuttalContradictionTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/RebuttalContradictionTest.java index 0e907e11..644075cf 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/RebuttalContradictionTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/RebuttalContradictionTest.java @@ -138,4 +138,106 @@ void shouldRespectDeclineWhenThereIsNoCodeToCheckAgainst() { assertTrue(RebuttalContradiction.find(RACE_FINDING, null, DISPATCHING_CODE).isEmpty()); assertTrue(RebuttalContradiction.find(null, rebuttal, DISPATCHING_CODE).isEmpty()); } + + @Test + void shouldReadTheConcurrencySignalFromTheDescriptionWhenTheFindingHasNoTitle() { + var untitled = + new ReviewResponse.Finding( + "medium", + "low", + "src/A.java", + 7, + null, + "Two deliveries can interleave between the check and the insert — a data race.", + null, + null); + + assertTrue( + RebuttalContradiction.find(untitled, "It runs serially.", DISPATCHING_CODE).isPresent(), + "a null title must not hide the concurrency signal carried by the description"); + } + + @Test + void shouldReadTheConcurrencySignalFromTheTitleWhenTheFindingHasNoDescription() { + var undescribed = + new ReviewResponse.Finding( + "medium", + "low", + "src/A.java", + 7, + "Race condition on the paused-PR insert", + null, + null, + null); + + assertTrue( + RebuttalContradiction.find(undescribed, "It runs serially.", DISPATCHING_CODE).isPresent(), + "a null description must not hide the concurrency signal carried by the title"); + } + + @Test + void shouldQuoteOnlyTheClaimSentenceOfALongerReply() { + var rebuttal = + "Thanks for the flag, I dug into this one. The command path is single-threaded." + + " Closing it out."; + + var contradiction = RebuttalContradiction.find(RACE_FINDING, rebuttal, DISPATCHING_CODE); + + assertTrue(contradiction.isPresent()); + assertEquals( + "The command path is single-threaded.", + contradiction.get().claim(), + "the quote must start after the preceding sentence, not at the top of the reply"); + } + + @Test + void shouldQuoteAWholeUnterminatedReplyAsTheClaim() { + // The claim opens the reply and the reply never ends a sentence, so the quote runs from the + // first character to the last with no terminator on either side. + var contradiction = + RebuttalContradiction.find(RACE_FINDING, "single-threaded here", DISPATCHING_CODE); + + assertTrue(contradiction.isPresent()); + assertEquals("single-threaded here", contradiction.get().claim()); + } + + @ParameterizedTest + @ValueSource(strings = {"!", "?", ";", ".", "\n"}) + void shouldStopTheQuotedClaimAtEverySentenceTerminator(String terminator) { + var rebuttal = "Nope, it is single-threaded" + terminator + " Moving on to the next thing"; + + var contradiction = RebuttalContradiction.find(RACE_FINDING, rebuttal, DISPATCHING_CODE); + + assertTrue(contradiction.isPresent()); + assertFalse( + contradiction.get().claim().contains("Moving on"), + "the quote must stop at the terminator, was: " + contradiction.get().claim()); + } + + @Test + void shouldQuoteEvidenceOnTheLastLineWhenTheCodeHasNoTrailingNewline() { + var codeEndingOnTheDispatch = + "diff --git a/Worker.java\n@@ -1,2 +1,3 @@\n+ pool.submit(task);"; + + var contradiction = + RebuttalContradiction.find(RACE_FINDING, "It runs serially.", codeEndingOnTheDispatch); + + assertTrue(contradiction.isPresent()); + assertEquals("pool.submit(task);", contradiction.get().evidence()); + } + + @Test + void shouldStripTheDiffMarkerFromAnEvidenceLineOnEitherSide() { + var removedDispatch = + "diff --git a/Worker.java\n@@ -1,3 +1,2 @@\n- pool.submit(task);\n+ run(task);\n"; + + var contradiction = + RebuttalContradiction.find(RACE_FINDING, "It runs serially.", removedDispatch); + + assertTrue(contradiction.isPresent()); + assertEquals( + "pool.submit(task);", + contradiction.get().evidence(), + "a leading -/+ diff marker is noise in the quote"); + } } diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/VerdictBuilderTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/VerdictBuilderTest.java index 789e43f4..c22f3e79 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/VerdictBuilderTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/VerdictBuilderTest.java @@ -318,79 +318,94 @@ void unresolvedPriorFindingStillInTheDiffKeepsHoldingApprove() { List.of(), null); + /** The dogfood PR's command path: each command is handed to the shared review executor. */ + private static final String DISPATCHING_DIFF = + """ + diff --git a/src/main/java/.../webhook/CommentCommandService.java + @@ -130,7 +130,9 @@ public class CommentCommandService { + + private void dispatch(CommandContext ctx) { + + executor.execute(() -> execute(ctx)); + + } + """; + + /** The bot's finding thread plus the maintainer's single decline on it. */ + private static final List< + dev.thiagogonzaga.thrillhousebot.github.GitHubReviewClient.PullRequestComment> + DECLINE_THREAD = + List.of( + new dev.thiagogonzaga.thrillhousebot.github.GitHubReviewClient.PullRequestComment( + 700L, + null, + RACE_FILE, + "**MEDIUM — " + RACE_TITLE + "**", + new dev.thiagogonzaga.thrillhousebot.github.GitHubReviewClient.ReviewResponse + .User("thrillhousebot[bot]")), + new dev.thiagogonzaga.thrillhousebot.github.GitHubReviewClient.PullRequestComment( + 701L, + 700L, + RACE_FILE, + "Not changed — pause() is only ever called from the /pause command path, which" + + " runs asynchronously on the review executor after the webhook has" + + " returned 200.", + new dev.thiagogonzaga.thrillhousebot.github.GitHubReviewClient.ReviewResponse + .User("maintainer"))); + + private static final ReviewResponse DECLINED_PRIOR_RESPONSE = + new ReviewResponse( + List.of(), + List.of( + new ReviewResponse.PreviousFindingStatus( + 1, "justified", "maintainer says the path cannot run concurrently")), + null); + + private static VerdictBuilder builderWithRealAnalyzer(PrSummaryGenerator summaryGenerator) { + return new VerdictBuilder( + summaryGenerator, + new FollowUpAnalyzer(new com.fasterxml.jackson.databind.ObjectMapper()), + BotIdentity.from(List.of("thrillhousebot[bot]")), + BlockingStrictness.BALANCED); + } + + /** + * A follow-up context carrying the declined race finding, with {@code diff} as the legacy diff. + */ + private static ReviewContextLoader.ReviewContext declinedRaceContext(String diff) { + return new ReviewContextLoader.ReviewContext( + List.of(), + diff, + "", + 0, + List.of(), + List.of("{}"), + List.of(RACE_PRIOR_RESPONSE), + false, + true, + "{}", + DECLINE_THREAD, + "", + new InstructionsResolver.ResolvedInstructions("", ""), + List.of(), + "", + "", + List.of(new FileDiff(RACE_FILE, "modified", 1, 0, 1, "")), + () -> + new DiffLineResolver( + Map.of(RACE_FILE, "@@ -60,1 +60,1 @@\n-old\n+if (repository.find(pr) == null) {")), + null); + } + @Test void declinedPriorFindingTheReviewedCodeContradictsStaysOpenAndHoldsApprove() { - var realBuilder = - new VerdictBuilder( - summaryGenerator, - new FollowUpAnalyzer(new com.fasterxml.jackson.databind.ObjectMapper()), - BotIdentity.from(List.of("thrillhousebot[bot]")), - BlockingStrictness.BALANCED); - var batchText = - """ - diff --git a/src/main/java/.../webhook/CommentCommandService.java - @@ -130,7 +130,9 @@ public class CommentCommandService { - + private void dispatch(CommandContext ctx) { - + executor.execute(() -> execute(ctx)); - + } - """; var plan = new DiffBudgetPlanner.BudgetPlan( - List.of(new DiffBudgetPlanner.DiffBatch(batchText, List.of(), 10)), + List.of(new DiffBudgetPlanner.DiffBatch(DISPATCHING_DIFF, List.of(), 10)), List.of(), List.of(), true); - var comments = - List.of( - new dev.thiagogonzaga.thrillhousebot.github.GitHubReviewClient.PullRequestComment( - 700L, - null, - RACE_FILE, - "**MEDIUM — " + RACE_TITLE + "**", - new dev.thiagogonzaga.thrillhousebot.github.GitHubReviewClient.ReviewResponse.User( - "thrillhousebot[bot]")), - new dev.thiagogonzaga.thrillhousebot.github.GitHubReviewClient.PullRequestComment( - 701L, - 700L, - RACE_FILE, - "Not changed — pause() is only ever called from the /pause command path, which" - + " runs asynchronously on the review executor after the webhook has returned" - + " 200.", - new dev.thiagogonzaga.thrillhousebot.github.GitHubReviewClient.ReviewResponse.User( - "maintainer"))); - var ctx = - new ReviewContextLoader.ReviewContext( - List.of(), - "", - "", - 0, - List.of(), - List.of("{}"), - List.of(RACE_PRIOR_RESPONSE), - false, - true, - "{}", - comments, - "", - new InstructionsResolver.ResolvedInstructions("", ""), - List.of(), - "", - "", - List.of(new FileDiff(RACE_FILE, "modified", 1, 0, 1, "")), - () -> - new DiffLineResolver( - Map.of( - RACE_FILE, "@@ -60,1 +60,1 @@\n-old\n+if (repository.find(pr) == null) {")), - null); - var declined = - new ReviewResponse( - List.of(), - List.of( - new ReviewResponse.PreviousFindingStatus( - 1, "justified", "maintainer says the path cannot run concurrently")), - null); - var result = realBuilder.build(ctx, declined, CI_CLEAR, plan); + var result = + builderWithRealAnalyzer(summaryGenerator) + .build(declinedRaceContext(""), DECLINED_PRIOR_RESPONSE, CI_CLEAR, plan); assertEquals(1, result.unresolvedPreviousCount()); assertEquals(ReviewState.COMMENT, result.reviewState()); @@ -400,6 +415,43 @@ void declinedPriorFindingTheReviewedCodeContradictsStaysOpenAndHoldsApprove() { + result.previousStatuses().get(0).note()); } + @Test + void declineRecheckReadsTheLegacyDiffWhenBudgetingIsDisabled() { + // budgeted=false: the planner holds no batches, so the legacy uncapped ctx.diff() is the only + // record of what the model saw and must still be the material the decline is checked against. + var legacyPlan = new DiffBudgetPlanner.BudgetPlan(List.of(), List.of(), List.of(), false); + + var result = + builderWithRealAnalyzer(summaryGenerator) + .build( + declinedRaceContext(DISPATCHING_DIFF), + DECLINED_PRIOR_RESPONSE, + CI_CLEAR, + legacyPlan); + + assertEquals(1, result.unresolvedPreviousCount()); + assertTrue( + result.previousStatuses().get(0).note().contains("executor.execute(() -> execute(ctx));")); + } + + @Test + void declineRecheckFallsBackToTheLegacyDiffWhenABudgetedPlanHasNoBatches() { + // budgeted=true but every file overflowed the budget, so batches is empty and the concatenation + // would yield nothing; ctx.diff() is the fallback the re-check must use. + var emptyBudgetedPlan = + new DiffBudgetPlanner.BudgetPlan(List.of(), List.of("big.java"), List.of(), true); + + var result = + builderWithRealAnalyzer(summaryGenerator) + .build( + declinedRaceContext(DISPATCHING_DIFF), + DECLINED_PRIOR_RESPONSE, + CI_CLEAR, + emptyBudgetedPlan); + + assertEquals(1, result.unresolvedPreviousCount()); + } + @Test void disabledBudgetingDisclosesTheLegacyLineCapCount() { var ctx = contextWithLineCapOmissions(2); From 47823a832b7f30becdc365dccfbf67246c74bb12 Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Sat, 8 Aug 2026 02:13:12 +0000 Subject: [PATCH 3/3] refactor(review): split the decline-recheck matchers into named patterns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../review/RebuttalContradiction.java | 118 +++++++++++++----- .../review/FollowUpAnalyzerTest.java | 15 ++- .../review/RebuttalContradictionTest.java | 99 +++++++++++++++ 3 files changed, 195 insertions(+), 37 deletions(-) diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/RebuttalContradiction.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/RebuttalContradiction.java index 5a9c326e..96649e9e 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/RebuttalContradiction.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/RebuttalContradiction.java @@ -17,6 +17,7 @@ import dev.thiagogonzaga.thrillhousebot.review.ai.ReviewResponse; import java.util.ArrayList; +import java.util.List; import java.util.Optional; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -39,9 +40,9 @@ *

  • the prior finding is about concurrency ({@link #CONCURRENCY_FINDING}) — a race, a * check-then-act, thread-safety, atomicity; *
  • the maintainer's reply asserts that concurrency is impossible ({@link - * #NO_CONCURRENCY_CLAIM}) — "single-threaded", "runs serially", "only ever called from …"; - *
  • the reviewed code contains a concurrent-dispatch construct ({@link #CONCURRENT_DISPATCH}) — - * an unbounded/pooled executor, {@code executor.submit/execute}, {@code CompletableFuture + * #NO_CONCURRENCY_CLAIMS}) — "single-threaded", "runs serially", "only ever called from …"; + *
  • the reviewed code contains a concurrent-dispatch construct ({@link #CONCURRENT_DISPATCHES}) + * — an unbounded/pooled executor, {@code executor.submit/execute}, {@code CompletableFuture * .runAsync}, {@code new Thread(...)}, {@code @Async}, {@code parallelStream()}. * * @@ -72,34 +73,69 @@ final class RebuttalContradiction { Pattern.CASE_INSENSITIVE); /** - * The maintainer asserting that the flagged path cannot run concurrently. "Only ever called from - * X" belongs here: a single call site is the usual way a decline argues away a race, and it is - * exactly the premise an asynchronous dispatch at that single call site refutes. + * The ways a maintainer asserts the flagged path cannot run concurrently, one small pattern per + * argument rather than a single mega-alternation. Kept separate on purpose: this matcher decides + * when the bot is allowed to overrule a human, so each argument has to be readable and testable + * on its own, and every quantifier is bounded because the input is untrusted reply prose. + * + *

    "Only ever called from X" is one of them: a single call site is the usual way a decline + * argues a race away, and it is exactly the premise an asynchronous dispatch at that call site + * refutes. */ - private static final Pattern NO_CONCURRENCY_CLAIM = - Pattern.compile( - "single[- ]threaded|\\bsingle thread\\b|runs? serially|executed? serially|serialized" - + "|sequentially|one at a time|never (?:runs?|executes?|happens) concurrently" - + "|(?:cannot|can't|can not|never) (?:run|be|happen|occur)s? concurrent(?:ly)?" - + "|no concurrency|not concurrent|there is no race|no race|isn'?t a race" - + "|not a race|only ever (?:called|invoked|triggered|reached) from" - + "|only (?:called|invoked|triggered|reached) from|the only caller", - Pattern.CASE_INSENSITIVE); + private static final List NO_CONCURRENCY_CLAIMS = + List.of( + // "the handler is single-threaded" + ci("single[- ]threaded|\\bsingle thread\\b"), + // "they run serially / one at a time" + ci("(?:runs?|executed?) serially|serialized|sequentially|one at a time"), + // "it cannot run concurrently" + ci("(?:can ?not|can'?t) (?:run|be|happen|occur|execute)s? concurrent(?:ly)?"), + // "it never runs concurrently" + ci("never (?:run|be|happen|occur|execute)s? concurrent(?:ly)?"), + // "there is no race here" + ci("no concurrency|not concurrent|no race|(?:isn'?t|not) a race"), + // "it is only ever called from one place" + ci("only (?:ever )?(?:called|invoked|triggered|reached) from|the only caller")); /** - * Code that dispatches work concurrently. A single call site that hands the work to any of these - * does not serialize it: two events each dispatch, and both run. + * Code that dispatches work concurrently, again one small pattern per construct. A single call + * site that hands the work to any of these does not serialize it: two events each dispatch, and + * both run. */ - private static final Pattern CONCURRENT_DISPATCH = - Pattern.compile( - "newVirtualThreadPerTaskExecutor|newCachedThreadPool|newWorkStealingPool" - + "|newFixedThreadPool\\s*\\(\\s*(?!1\\s*\\))|newScheduledThreadPool" - + "|\\.submit\\s*\\(|\\.execute\\s*\\(|CompletableFuture\\s*\\.\\s*(?:runAsync|supplyAsync)" - + "|new\\s+Thread\\s*\\(|@Async\\b|\\.parallelStream\\s*\\("); + private static final List CONCURRENT_DISPATCHES = + List.of( + // an unbounded or pooled executor + Pattern.compile( + "newVirtualThreadPerTaskExecutor|newCachedThreadPool|newScheduledThreadPool" + + "|newWorkStealingPool"), + // a fixed pool of more than one thread + Pattern.compile("newFixedThreadPool\\s{0,16}\\(\\s{0,16}(?!1\\s{0,16}\\))"), + // handing the work to an executor + Pattern.compile("\\.(?:submit|execute)\\s{0,16}\\("), + // an asynchronous future + Pattern.compile("CompletableFuture\\s{0,16}\\.\\s{0,16}(?:runAsync|supplyAsync)"), + // a raw thread, an async annotation, or a parallel stream + Pattern.compile( + "new\\s{1,16}Thread\\s{0,16}\\(|@Async\\b|\\.parallelStream\\s{0,16}\\(")); - /** Fenced code blocks in a markdown reply — quoted material, never the maintainer's assertion. */ + /** + * Fenced code blocks in a markdown reply — quoted material, never the maintainer's assertion. The + * body is bounded: an unbounded lazy match would rescan from every opening fence in a reply that + * never closes one. + */ private static final Pattern FENCED_BLOCK = - Pattern.compile("```.*?```", Pattern.DOTALL | Pattern.MULTILINE); + Pattern.compile("```.{0,10000}?```", Pattern.DOTALL | Pattern.MULTILINE); + + /** + * Replies longer than this are not analyzed at all. It keeps the markdown-stripping scan over + * untrusted prose bounded, and skipping is the conservative outcome: an unread reply keeps its + * decline. + */ + private static final int MAX_REBUTTAL_CHARS = 20_000; + + private static Pattern ci(String regex) { + return Pattern.compile(regex, Pattern.CASE_INSENSITIVE); + } private RebuttalContradiction() {} @@ -113,21 +149,41 @@ static Optional find( if (finding == null || rebuttal == null || reviewedCode == null || reviewedCode.isBlank()) { return Optional.empty(); } + if (rebuttal.length() > MAX_REBUTTAL_CHARS) { + return Optional.empty(); + } if (!CONCURRENCY_FINDING.matcher(findingText(finding)).find()) { return Optional.empty(); } - Matcher claim = NO_CONCURRENCY_CLAIM.matcher(assertedText(rebuttal)); - if (!claim.find()) { + var asserted = assertedText(rebuttal); + Matcher claim = earliestMatch(NO_CONCURRENCY_CLAIMS, asserted); + if (claim == null) { return Optional.empty(); } - Matcher evidence = CONCURRENT_DISPATCH.matcher(reviewedCode); - if (!evidence.find()) { + Matcher evidence = earliestMatch(CONCURRENT_DISPATCHES, reviewedCode); + if (evidence == null) { return Optional.empty(); } return Optional.of( new Contradiction( - sentenceAround(assertedText(rebuttal), claim.start()), - lineAround(reviewedCode, evidence.start()))); + sentenceAround(asserted, claim.start()), lineAround(reviewedCode, evidence.start()))); + } + + /** + * The leftmost match of any pattern in {@code patterns}, or {@code null} when none matches. Every + * pattern is tried and the earliest wins, so splitting one alternation into several keeps the + * leftmost-match semantics the single pattern had and leaves the result independent of list + * order. + */ + private static Matcher earliestMatch(List patterns, String text) { + Matcher earliest = null; + for (var pattern : patterns) { + var candidate = pattern.matcher(text); + if (candidate.find() && (earliest == null || candidate.start() < earliest.start())) { + earliest = candidate; + } + } + return earliest; } /** The quoted claim and the quoted code line that refutes it, both already trimmed for a note. */ diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/FollowUpAnalyzerTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/FollowUpAnalyzerTest.java index 8ca3bdc1..ad6ed232 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/FollowUpAnalyzerTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/FollowUpAnalyzerTest.java @@ -17,6 +17,8 @@ import static org.junit.jupiter.api.Assertions.*; import static org.junit.jupiter.params.provider.Arguments.arguments; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; import com.fasterxml.jackson.databind.ObjectMapper; import dev.thiagogonzaga.thrillhousebot.config.BotIdentity; @@ -25,6 +27,7 @@ import dev.thiagogonzaga.thrillhousebot.review.ai.ReviewResponse; import java.util.List; import java.util.Map; +import java.util.function.Supplier; import java.util.stream.Stream; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; @@ -2134,7 +2137,7 @@ static Stream recheckNoOpInputs() { supplier(DISPATCHING_DIFF))); } - private static java.util.function.Supplier supplier(String value) { + private static Supplier supplier(String value) { return () -> value; } @@ -2145,7 +2148,7 @@ void recheckShouldReturnStatusesUntouchedWhenThereIsNothingToVerify( List previous, List statuses, List comments, - java.util.function.Supplier code) { + Supplier code) { assertEquals( statuses, analyzer.recheckDeclines(previous, statuses, comments, BOT_ID, code), @@ -2214,10 +2217,10 @@ void recheckShouldIgnoreBotAnonymousAndEmptyRepliesWhenCountingTheRebuttal() { @Test void injectedAnalyzerShouldTakeTheRecheckFlagFromConfig() { - var review = org.mockito.Mockito.mock(ThrillhouseConfig.ReviewConfig.class); - org.mockito.Mockito.when(review.declineRecheckEnabled()).thenReturn(false); - var config = org.mockito.Mockito.mock(ThrillhouseConfig.class); - org.mockito.Mockito.when(config.review()).thenReturn(review); + var review = mock(ThrillhouseConfig.ReviewConfig.class); + when(review.declineRecheckEnabled()).thenReturn(false); + var config = mock(ThrillhouseConfig.class); + when(config.review()).thenReturn(review); var comments = raceThread( "Not changed — pause() is only ever called from the /pause command path, which runs" diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/RebuttalContradictionTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/RebuttalContradictionTest.java index 644075cf..5ccd3c89 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/RebuttalContradictionTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/RebuttalContradictionTest.java @@ -89,6 +89,105 @@ void shouldRespectRebuttalsThatAreNotRefutableFromCode(String rebuttal) { "style / intent / accepted-risk rebuttals must keep the decline"); } + /** One phrasing per "concurrency is impossible" argument the matcher recognises. */ + @ParameterizedTest + @ValueSource( + strings = { + "The handler is single-threaded.", + "There is a single thread doing this.", + "They run serially.", + "The commands are executed serially.", + "Access is serialized by the caller.", + "These are processed sequentially.", + "They happen one at a time.", + "It cannot run concurrently.", + "Two of them can't be concurrent.", + "This never executes concurrently.", + "There is no concurrency on this path.", + "The path is not concurrent.", + "There is no race here.", + "That isn't a race.", + "That is not a race, really.", + "pause() is only ever called from the command path.", + "It is only invoked from the webhook path.", + "The command path is the only caller.", + }) + void shouldRecogniseEverySerializationArgument(String rebuttal) { + assertTrue( + RebuttalContradiction.find(RACE_FINDING, rebuttal, DISPATCHING_CODE).isPresent(), + "this is a 'concurrency is impossible' claim and the code refutes it: " + rebuttal); + } + + /** One snippet per concurrent-dispatch construct the matcher accepts as refuting evidence. */ + @ParameterizedTest + @ValueSource( + strings = { + "+ var pool = Executors.newVirtualThreadPerTaskExecutor();", + "+ var pool = Executors.newCachedThreadPool();", + "+ var pool = Executors.newScheduledThreadPool(2);", + "+ var pool = Executors.newWorkStealingPool();", + "+ var pool = Executors.newFixedThreadPool(8);", + "+ executor.submit(() -> run(ctx));", + "+ executor.execute(() -> run(ctx));", + "+ CompletableFuture.runAsync(() -> run(ctx));", + "+ CompletableFuture.supplyAsync(() -> load(ctx));", + "+ new Thread(() -> run(ctx)).start();", + "+ @Async", + "+ items.parallelStream().forEach(this::handle);", + }) + void shouldRecogniseEveryConcurrentDispatchConstruct(String codeLine) { + assertTrue( + RebuttalContradiction.find(RACE_FINDING, "It runs serially.", codeLine).isPresent(), + "this construct dispatches concurrently: " + codeLine); + } + + @Test + void shouldNotTreatASingleThreadedFixedPoolAsConcurrentDispatch() { + var oneThread = "+ var pool = Executors.newFixedThreadPool(1);\n"; + + assertTrue( + RebuttalContradiction.find(RACE_FINDING, "It runs serially.", oneThread).isEmpty(), + "a one-thread pool genuinely serializes, so it does not refute the decline"); + } + + @Test + void shouldSkipARebuttalTooLargeToAnalyze() { + var huge = "It is single-threaded. ".repeat(2000); + + assertTrue(huge.length() > 20_000, "the fixture must exceed the analysis cap"); + assertTrue( + RebuttalContradiction.find(RACE_FINDING, huge, DISPATCHING_CODE).isEmpty(), + "an unread reply keeps its decline — skipping is the conservative outcome"); + } + + @Test + void shouldQuoteTheEarliestClaimWhicheverArgumentItBelongsTo() { + // Two claims in two different sentences. "only ever called from" is declared LAST but appears + // FIRST; "no race" is declared earlier but appears later. The quote must follow position in the + // reply, not the order the patterns happen to be declared in — otherwise splitting the original + // single alternation would have silently changed which sentence gets quoted back at the + // maintainer. + var rebuttal = "It is only ever called from the command path. Anyway there is no race."; + + var contradiction = RebuttalContradiction.find(RACE_FINDING, rebuttal, DISPATCHING_CODE); + + assertTrue(contradiction.isPresent()); + assertEquals( + "It is only ever called from the command path.", + contradiction.get().claim(), + "the earliest claim in the reply wins, regardless of which pattern matched it"); + + // The mirror image: now the FIRST-declared argument is also the one appearing first, so the + // later match must be rejected rather than overwrite it. Both directions together pin the + // quote to position alone. + var mirrored = + RebuttalContradiction.find( + RACE_FINDING, "It is single-threaded. Anyway there is no race.", DISPATCHING_CODE); + + assertTrue(mirrored.isPresent()); + assertEquals("It is single-threaded.", mirrored.get().claim()); + } + @Test void shouldRespectDeclineWhenTheCodeShowsNoConcurrentDispatch() { var rebuttal = "pause() is only ever called from the /pause command path — single-threaded.";