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..b3283342 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,119 @@ 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 || !hasDecline(statuses)) { + return statuses; + } + // 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()); + 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; + } + + /** 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, + * 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..96649e9e --- /dev/null +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/RebuttalContradiction.java @@ -0,0 +1,265 @@ +/* + * 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.ArrayList; +import java.util.List; +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_CLAIMS}) — "single-threaded", "runs serially", "only ever called from …"; + *
  3. 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()}. + *
+ * + *

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 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 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, 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 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. 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("```.{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() {} + + /** + * 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 (rebuttal.length() > MAX_REBUTTAL_CHARS) { + return Optional.empty(); + } + if (!CONCURRENCY_FINDING.matcher(findingText(finding)).find()) { + return Optional.empty(); + } + var asserted = assertedText(rebuttal); + Matcher claim = earliestMatch(NO_CONCURRENCY_CLAIMS, asserted); + if (claim == null) { + return Optional.empty(); + } + Matcher evidence = earliestMatch(CONCURRENT_DISPATCHES, reviewedCode); + if (evidence == null) { + return Optional.empty(); + } + return Optional.of( + new Contradiction( + 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. */ + 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 kept = new ArrayList(); + for (var line : withoutFences.split("\n", -1)) { + if (!line.stripLeading().startsWith(">")) { + kept.add(line); + } + } + // 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. */ + 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(); + // 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) { + 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..ad6ed232 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/FollowUpAnalyzerTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/FollowUpAnalyzerTest.java @@ -17,13 +17,17 @@ 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; +import dev.thiagogonzaga.thrillhousebot.config.ThrillhouseConfig; import dev.thiagogonzaga.thrillhousebot.github.GitHubReviewClient; 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; @@ -1940,4 +1944,296 @@ 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()); + } + + /** + * 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 Supplier supplier(String value) { + return () -> value; + } + + @ParameterizedTest(name = "{0}") + @MethodSource("recheckNoOpInputs") + void recheckShouldReturnStatusesUntouchedWhenThereIsNothingToVerify( + String name, + List previous, + List statuses, + List comments, + 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 = 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" + + " 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 new file mode 100644 index 00000000..5ccd3c89 --- /dev/null +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/RebuttalContradictionTest.java @@ -0,0 +1,342 @@ +/* + * 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"); + } + + /** 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."; + + 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()); + } + + @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 58672b2d..c22f3e79 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,162 @@ 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); + + /** 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 plan = + new DiffBudgetPlanner.BudgetPlan( + List.of(new DiffBudgetPlanner.DiffBatch(DISPATCHING_DIFF, List.of(), 10)), + List.of(), + List.of(), + true); + + var result = + builderWithRealAnalyzer(summaryGenerator) + .build(declinedRaceContext(""), DECLINED_PRIOR_RESPONSE, 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 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);