From ccba27ca35bb1d8ab3c76ca67e7153df74588016 Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Thu, 13 Aug 2026 18:41:08 -0300 Subject: [PATCH 1/4] fix(review): log the 422 body, abandon superseded posts, and preserve refused reviews as comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A review on #701 was lost to a GitHub 422 the logs could not explain: the response body was discarded, the run posted 9.5 minutes after the dispatcher knew it was superseded, and the no-comments rejection path threw the whole generation away. - A rejected review post now logs GitHub's own response body through the GitHubApiError seam (credential-shaped text redacted, length capped), so the next 422 names its cause. - ReviewOrchestrator re-reads the PR head just before the first write (a fresh read through the #693-healed seam) and abandons the post when the head moved: counted as a structured HEAD_MOVED skip, the stale check run concluded as skipped, nothing user-facing — the coalesced run for the new head posts in its place. Fail-open: a failed head read never abandons a finished review. - createReviewWithFallback keeps the retry-without-comments path, and when a summary-only review (or the retry) is refused, posts the same body as an issue comment through the capped, paced write path with a note that GitHub refused the review post, instead of surfacing "review could not be completed". ReviewPostException now fires only when the comment fallback fails too. Closes #704 --- CHANGELOG.md | 2 + .../review/ReviewContextLoader.java | 27 ++ .../review/ReviewOrchestrator.java | 77 ++++++ .../review/ReviewPublisher.java | 99 +++++++- .../review/ReviewSkipReason.java | 7 +- .../review/ReviewContextLoaderTest.java | 34 +++ .../review/ReviewOrchestratorTest.java | 237 +++++++++++++++++- 7 files changed, 473 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f224f1ca..137d4dd8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,8 @@ All notable changes to ThrillhouseBot. ### Fixed +- **A review post GitHub refuses is diagnosed, abandoned when stale, and never lost** (#704): a rejected review post now logs GitHub's own response body (redacted and length-capped) instead of only the status code; a run whose PR head moved while the model call ran abandons its post — counted as a structured `HEAD_MOVED` skip, its check run concluded as skipped — because the coalesced run for the new head re-reviews and posts in its place; and a summary-only review GitHub refuses falls back to posting the same body as an issue comment through the capped, paced write path, instead of discarding the generation behind a "review could not be completed" notice + - **Inline code spans in a decline are stripped delimiter-aware** (#697): the decline re-check now scans backtick runs the CommonMark way — an opening run of N backticks closes at the next run of exactly N — so a span whose body carries a longer backtick run (`` `a``b` ``) or one line ending is stripped whole instead of leaving quoted claim text to reopen a correct decline. An unclosed run stays literal, and a length bound still keeps a stray backtick from swallowing the reply - **Mention-form commands follow the configured bot login** (#698): `TriggerDetector` builds the `@ ` trigger patterns from `BotIdentity.mentionNames()` instead of a hardcoded slug, so `@my-review-bot review` works on a custom-login install; the mention's `@` must open the comment or follow a non-word character, so an email local part never triggers a command. Slash forms and default-config behavior are unchanged diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewContextLoader.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewContextLoader.java index 4bb9dc69..ff23a0c5 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewContextLoader.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewContextLoader.java @@ -32,6 +32,7 @@ import jakarta.enterprise.context.ApplicationScoped; import jakarta.inject.Inject; import java.util.List; +import java.util.Optional; import java.util.function.Supplier; import org.eclipse.microprofile.rest.client.inject.RestClient; @@ -480,6 +481,32 @@ List fetchPrFiles( * {@code null} so the summary falls back to the diff-derived counts rather than failing the * review. */ + /** + * The PR's head SHA as GitHub reports it right now — a fresh read, taken just before the run + * posts, so a head that moved during the minutes-long model call is caught (#704). Goes through + * {@link GitHubPullRequestClient#getPullRequest}, so a credential that expired mid-run is healed + * the same way every other late read is (#626/#693). Fail-open: a failed or headless read returns + * empty, and the caller posts rather than losing a finished review to this guard. + */ + Optional currentHeadSha(String auth, ReviewOrchestrator.ReviewRequest req) { + try { + var pr = prClient.getPullRequest(auth, ACCEPT, req.owner(), req.repo(), req.prNumber()); + return Optional.ofNullable(pr) + .map(GitHubPullRequestClient.PullRequestDetails::head) + .map(GitHubPullRequestClient.Ref::sha) + .filter(sha -> !sha.isBlank()); + } catch (RuntimeException e) { + Log.warnf( + e, + "Could not re-read the head of %s/%s #%d before posting — posting against the reviewed" + + " sha", + req.owner(), + req.repo(), + req.prNumber()); + return Optional.empty(); + } + } + PrTotals fetchPrTotals(String auth, String owner, String repo, int prNumber) { try { var pr = prClient.getPullRequest(auth, ACCEPT, owner, repo, prNumber); diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestrator.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestrator.java index c4c3590a..9d1578cf 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestrator.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestrator.java @@ -28,6 +28,7 @@ import jakarta.enterprise.context.control.ActivateRequestContext; import jakarta.inject.Inject; import java.util.List; +import java.util.Optional; import java.util.concurrent.CompletableFuture; import java.util.concurrent.ExecutorService; import java.util.function.Consumer; @@ -100,6 +101,8 @@ static String contextWindowCheckSummary() { private final FindingFeedbackCaptureService findingFeedbackCapture; + private final ReviewSkipEmitter skipEmitter; + private final ExecutorService reviewExecutor; /** @@ -215,6 +218,7 @@ public ReviewOrchestrator( VerdictBuilder verdictBuilder, FindingPipeline findingPipeline, FindingFeedbackCaptureService findingFeedbackCapture, + ReviewSkipEmitter skipEmitter, @ReviewExecutor ExecutorService reviewExecutor) { this.config = config; this.authClient = authClient; @@ -229,6 +233,7 @@ public ReviewOrchestrator( this.verdictBuilder = verdictBuilder; this.findingPipeline = findingPipeline; this.findingFeedbackCapture = findingFeedbackCapture; + this.skipEmitter = skipEmitter; this.reviewExecutor = reviewExecutor; } @@ -296,6 +301,16 @@ public boolean review(ReviewRequest request) { String conclusion = VerdictBuilder.conclusionForResult(result); String checkTitle = VerdictBuilder.checkTitleForResult(result); String checkSummary = VerdictBuilder.checkSummaryForResult(result); + // #704: the model call can run for minutes; a push landing in that window supersedes this + // run (the dispatcher already queued a coalesced run for the new head). Re-read the head + // just before the first write and abandon the post when it moved, instead of posting a + // review — and resolving inline comments — against a diff that changed underneath it. + var freshHead = contextLoader.currentHeadSha(auth, req); + if (headMoved(req, freshHead)) { + abandonSupersededRun(auth, req, session, checkRunId, freshHead.get()); + return false; + } + boolean summaryPosted = publishSummaryBestEffort(auth, req, result); // Opt-in follow-up delta comment. Runs only when no summary was posted this round, and its // outcome is intentionally discarded — it must not feed summaryPosted below. @@ -377,6 +392,68 @@ public boolean review(ReviewRequest request) { return resultSurfaced; } + /** SKIPPED check-run title when the finished run's post was abandoned (#704). */ + static final String SUPERSEDED_CHECK_TITLE = "Review superseded by a newer commit"; + + /** + * Whether the freshly read head names a different commit than the one this run reviewed. False + * when the fresh read failed (fail-open — a finished review is never lost to its own guard) or + * when the run has no reviewed sha to compare against. Visible for tests. + */ + static boolean headMoved(ReviewRequest req, Optional freshHead) { + return req.commitSha() != null + && !req.commitSha().isBlank() + && freshHead.filter(sha -> !sha.equalsIgnoreCase(req.commitSha())).isPresent(); + } + + /** + * Retires a run whose head moved while it reviewed: counted as a structured skip, the check run + * on the reviewed (old) sha concluded as skipped, and the session closed out — nothing is posted + * to the PR and no user-facing error is raised, because the dispatcher's coalesced run for the + * new head re-reviews and posts in this run's place (#704). Visible for tests. + */ + void abandonSupersededRun( + String auth, ReviewRequest req, ReviewSession session, long checkRunId, String freshHead) { + skipEmitter.recordSkip( + ReviewSkipReason.HEAD_MOVED, + req.owner(), + req.repo(), + req.prNumber(), + "head moved from " + + req.commitSha() + + " to " + + freshHead + + " while the review ran — abandoning the post; the queued run for the new head" + + " replaces it"); + if (checkRunId > 0) { + try { + checkRunManager.updateCheckRun( + new CheckRunManager.CheckRunUpdate( + auth, + req.owner(), + req.repo(), + checkRunId, + CHECK_STATUS_COMPLETED, + "skipped", + SUPERSEDED_CHECK_TITLE, + "The pull request head moved to " + + freshHead + + " while this review ran, so its" + + " result was not posted. The review of the new head replaces it.", + sessionUrl(session))); + } catch (RuntimeException checkRunError) { + Log.warnf(checkRunError, "Failed to mark superseded check run %d as skipped", checkRunId); + } + } + try { + applyReviewFailure( + session, "Superseded: the PR head moved to " + freshHead + " during the review"); + } catch (RuntimeException persistenceError) { + Log.warnf(persistenceError, "Failed to persist superseded review session %d", session.id); + } + broadcaster.broadcast(SessionEventBroadcaster.SessionEvent.failed(session)); + } + /** * Posts the PR summary comment, swallowing any failure: it is first-review enrichment, not the * review itself, so a transient failure here must not abort before {@code postReview} and surface diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java index d429ce39..f6abfc31 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java @@ -17,6 +17,7 @@ import dev.thiagogonzaga.thrillhousebot.config.BotIdentity; import dev.thiagogonzaga.thrillhousebot.config.ThrillhouseConfig; +import dev.thiagogonzaga.thrillhousebot.github.GitHubApiError; import dev.thiagogonzaga.thrillhousebot.github.GitHubCommentClient; import dev.thiagogonzaga.thrillhousebot.github.GitHubReviewClient; import dev.thiagogonzaga.thrillhousebot.github.ReviewThreadService; @@ -24,6 +25,7 @@ import io.quarkus.logging.Log; import jakarta.enterprise.context.ApplicationScoped; import jakarta.inject.Inject; +import jakarta.ws.rs.WebApplicationException; import java.util.ArrayList; import java.util.List; import java.util.Optional; @@ -742,9 +744,17 @@ void dismissPendingBotReviews( } } + /** Lead-in for a review body preserved as an issue comment after GitHub refused it (#704). */ + static final String REVIEW_REFUSED_NOTE = + "⚠️ GitHub refused the review post, so ThrillhouseBot is posting the review as a regular" + + " comment instead."; + /** * Submits a PR review, falling back to a summary-only review when inline comments are rejected - * (e.g. stale line numbers after a force-push). + * (e.g. stale line numbers after a force-push), and to an issue comment carrying the same body + * when the review post itself is refused (#704) — a rejected summary-only review used to discard + * the whole generation behind a "review could not be completed" notice. Throws {@link + * ReviewPostException} only when the comment fallback fails too. */ void createReviewWithFallback( String auth, @@ -752,16 +762,18 @@ void createReviewWithFallback( String repo, int prNumber, GitHubReviewClient.CreateReviewRequest req) { + RuntimeException rejection; try { reviewClient.createReview(auth, ACCEPT, owner, repo, prNumber, req); + return; } catch (RuntimeException e) { - // CreateReviewRequest's compact constructor normalizes a null comments list to List.of(). - if (req.comments().isEmpty()) { - throw new ReviewPostException( - "GitHub review rejected for " + owner + "/" + repo + " #" + prNumber, e); - } + logReviewRejection(e, owner, repo, prNumber); + rejection = e; + } + // CreateReviewRequest's compact constructor normalizes a null comments list to List.of(). + if (!req.comments().isEmpty()) { Log.warnf( - e, + rejection, "PR review with inline comments rejected for %s/%s #%d — retrying without comments", owner, repo, @@ -769,8 +781,79 @@ void createReviewWithFallback( var fallback = new GitHubReviewClient.CreateReviewRequest( req.commitId(), req.body(), req.event(), List.of()); - reviewClient.createReview(auth, ACCEPT, owner, repo, prNumber, fallback); + try { + reviewClient.createReview(auth, ACCEPT, owner, repo, prNumber, fallback); + return; + } catch (RuntimeException retryFailure) { + logReviewRejection(retryFailure, owner, repo, prNumber); + rejection = retryFailure; + } + } + postReviewBodyAsComment(auth, owner, repo, prNumber, req.body(), rejection); + } + + /** + * Preserves a refused review as an issue comment: the same body, prefixed with a note that GitHub + * refused the review post. Goes through {@link GitHubCommentClient#createComment}, so the comment + * gets the same body cap (#487) and paced/backed-off write path (#597/#568) as every other + * conversation comment. A blank body (a bare first-review APPROVE) is replaced with the + * clean-review message so the comment still states an outcome. + */ + private void postReviewBodyAsComment( + String auth, String owner, String repo, int prNumber, String body, RuntimeException cause) { + var outcome = body == null || body.isBlank() ? PrSummaryGenerator.ZERO_ISSUES_MESSAGE : body; + try { + commentClient.createComment( + auth, + ACCEPT, + owner, + repo, + prNumber, + new GitHubCommentClient.CreateCommentRequest(REVIEW_REFUSED_NOTE + "\n\n" + outcome)); + } catch (RuntimeException commentFailure) { + var failure = + new ReviewPostException( + "GitHub review rejected for " + + owner + + "/" + + repo + + " #" + + prNumber + + " and the comment fallback failed too", + cause); + failure.addSuppressed(commentFailure); + throw failure; + } + Log.warnf( + "GitHub review rejected for %s/%s #%d — the review body was preserved as an issue comment", + owner, repo, prNumber); + } + + /** + * Logs what GitHub actually said when it rejected a review post (#704). The runtime's default + * exception mapper surfaces the status and nothing else, so without this line a 422 here is + * undiagnosable after the fact. The body is read through {@link GitHubApiError}, which redacts + * anything credential-shaped and caps the length before it reaches the log. + */ + private static void logReviewRejection( + RuntimeException rejection, String owner, String repo, int prNumber) { + var diagnostics = + webApplicationFailure(rejection) + .flatMap(GitHubApiError::of) + .map(GitHubApiError::diagnostics) + .orElse("no HTTP response to read (" + rejection + ")"); + Log.warnf( + "GitHub rejected the review post for %s/%s #%d: %s", owner, repo, prNumber, diagnostics); + } + + /** The HTTP-carrying failure in the cause chain, if any — the one whose response can be read. */ + private static Optional webApplicationFailure(Throwable rejection) { + for (Throwable t = rejection; t != null; t = t.getCause()) { + if (t instanceof WebApplicationException web) { + return Optional.of(web); + } } + return Optional.empty(); } /** diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewSkipReason.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewSkipReason.java index 93591c85..87990d81 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewSkipReason.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewSkipReason.java @@ -38,5 +38,10 @@ public enum ReviewSkipReason { /** An automatic review completed within {@code review.auto-review-min-interval}. */ RATE_LIMITED, /** The review executor rejected the task (saturated or shutting down). */ - DISPATCH_REJECTED + DISPATCH_REJECTED, + /** + * The PR head moved while the review ran, so the finished run abandoned its post — the coalesced + * run for the new head covers it (#704). + */ + HEAD_MOVED } diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewContextLoaderTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewContextLoaderTest.java index d5509b1e..fec73aab 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewContextLoaderTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewContextLoaderTest.java @@ -606,6 +606,40 @@ private ReviewSession session() { return session; } + @Test + void currentHeadShaReturnsTheFreshHead() { + when(prClient.getPullRequest(any(), any(), eq("owner"), eq("repo"), eq(42))) + .thenReturn( + new GitHubPullRequestClient.PullRequestDetails( + "Title", + "", + new GitHubPullRequestClient.Ref("fresh-sha"), + new GitHubPullRequestClient.Ref("base-sha"))); + + assertEquals(java.util.Optional.of("fresh-sha"), loader.currentHeadSha("auth", request())); + } + + @Test + void currentHeadShaIsEmptyWhenTheReadFailsOrTheHeadIsMissing() { + // Fail-open: a guard read that fails must not lose a finished review. + when(prClient.getPullRequest(any(), any(), eq("owner"), eq("repo"), eq(42))) + .thenThrow(new RuntimeException("GitHub unavailable")) + .thenReturn(null) + .thenReturn( + new GitHubPullRequestClient.PullRequestDetails( + "Title", "", null, new GitHubPullRequestClient.Ref("base-sha"))) + .thenReturn( + new GitHubPullRequestClient.PullRequestDetails( + "Title", + "", + new GitHubPullRequestClient.Ref(""), + new GitHubPullRequestClient.Ref("base-sha"))); + + for (var ignored = 0; ignored < 4; ignored++) { + assertTrue(loader.currentHeadSha("auth", request()).isEmpty()); + } + } + @Test void shouldRejectWhenHeadChangesAfterFilesAreFetched() { when(prClient.getPullRequestFiles(any(), any(), eq("owner"), eq("repo"), eq(42))) diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestratorTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestratorTest.java index b45632b8..68224687 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestratorTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestratorTest.java @@ -114,6 +114,9 @@ class ReviewOrchestratorTest { private FindingPipeline findingPipeline; + private final ReviewSkipEmitter skipEmitter = + new ReviewSkipEmitter(io.opentelemetry.api.OpenTelemetry.noop()); + private final ExecutorService reviewExecutor = Executors.newVirtualThreadPerTaskExecutor(); private ReviewOrchestrator orchestrator; @@ -257,6 +260,7 @@ diffFormatter, new TokenCounter(), config, new ActiveModelSettings(config, "m")) verdictBuilder, findingPipeline, mock(FindingFeedbackCaptureService.class), + skipEmitter, reviewExecutor); } @@ -1583,6 +1587,12 @@ void postReviewFailureMarksReviewFailedInsteadOfLeavingAConcludedCheckRun() { when(reviewClient.createReview( anyString(), anyString(), anyString(), anyString(), anyInt(), any())) .thenThrow(new RuntimeException("502 Bad Gateway")); + // The issue-comment fallback (#704) fails too — only then is the post truly lost; the + // later failure-notice comment goes through. + when(commentClient.createComment( + anyString(), anyString(), anyString(), anyString(), anyInt(), any())) + .thenThrow(new RuntimeException("comment 502")) + .thenReturn(null); orchestrator.review( new ReviewOrchestrator.ReviewRequest( @@ -2873,6 +2883,134 @@ void shouldSubmitRequestChangesReviewWhenCriticalFindingsPostedInline() { } } + @Test + void headMovedComparesTheReviewedShaAgainstTheFreshHead() { + var req = + new ReviewOrchestrator.ReviewRequest( + "owner", "repo", 42, "abcdefgh", "T", "", "base", "main", 1L, false); + assertTrue(ReviewOrchestrator.headMoved(req, java.util.Optional.of("other-sha"))); + assertFalse(ReviewOrchestrator.headMoved(req, java.util.Optional.of("ABCDEFGH"))); + // Fail-open: an unreadable fresh head never abandons a finished review. + assertFalse(ReviewOrchestrator.headMoved(req, java.util.Optional.empty())); + // No reviewed sha to compare against: never treated as moved. + var noSha = + new ReviewOrchestrator.ReviewRequest( + "owner", "repo", 42, "", "T", "", "base", "main", 1L, false); + assertFalse(ReviewOrchestrator.headMoved(noSha, java.util.Optional.of("other-sha"))); + var nullSha = + new ReviewOrchestrator.ReviewRequest( + "owner", "repo", 42, null, "T", "", "base", "main", 1L, false); + assertFalse(ReviewOrchestrator.headMoved(nullSha, java.util.Optional.of("other-sha"))); + } + + @Test + void abandonSupersededRunSurvivesCheckRunAndPersistenceFailures() { + var session = ReviewSession.create("owner/repo", 42, "T", "abcdefgh"); + session.id = 7L; + session.setPublicId("test-public-id"); + var req = + new ReviewOrchestrator.ReviewRequest( + "owner", "repo", 42, "abcdefgh", "T", "", "base", "main", 1L, false); + doThrow(new RuntimeException("check run API down")) + .when(checkRunClient) + .updateCheckRun(anyString(), anyString(), anyString(), anyString(), anyLong(), any()); + doThrow(new RuntimeException("db down")).when(sessionPersistence).update(anyLong(), any()); + + assertDoesNotThrow( + () -> orchestrator.abandonSupersededRun("Bearer tok", req, session, 1L, "movedsha")); + // And with no check run to conclude, the abandon still counts and broadcasts. + assertDoesNotThrow( + () -> orchestrator.abandonSupersededRun("Bearer tok", req, session, -1L, "movedsha")); + + assertEquals(2L, skipEmitter.countsByReason().get("HEAD_MOVED")); + verify(broadcaster, times(2)).broadcast(any(SessionEventBroadcaster.SessionEvent.class)); + } + + @Test + void shouldAbandonPostWhenHeadMovedDuringReview() { + try (var mockedStatic = mockStatic(ReviewSession.class)) { + var session = mock(ReviewSession.class); + session.id = 1L; + when(session.getRepository()).thenReturn("owner/repo"); + when(session.getPrNumber()).thenReturn(42); + when(session.getPrTitle()).thenReturn("Test PR"); + when(session.getCommitSha()).thenReturn("abcdefgh"); + when(session.getTimestamp()).thenReturn(java.time.Instant.parse("2025-06-01T12:00:00Z")); + mockedStatic + .when(() -> ReviewSession.create(anyString(), anyInt(), anyString(), anyString())) + .thenReturn(session); + + when(authClient.getAuthHeader(123L)).thenReturn("Bearer test"); + when(checkRunClient.createCheckRun( + anyString(), anyString(), anyString(), anyString(), any())) + .thenReturn(new GitHubCheckRunClient.CheckRunResponse(1L, "http://check")); + when(prClient.getPullRequestFiles( + anyString(), anyString(), anyString(), anyString(), anyInt())) + .thenReturn(List.of(fileDiffWithLine("src/Main.java", 10))); + when(prClient.compareCommits( + anyString(), anyString(), anyString(), anyString(), anyString(), anyString())) + .thenReturn(new GitHubPullRequestClient.CompareResponse(0, List.of())); + when(reviewClient.listReviews(anyString(), anyString(), anyString(), anyString(), anyInt())) + .thenReturn(List.of()); + when(instructionsResolver.resolve(anyString(), anyString(), anyString(), anyLong())) + .thenReturn(InstructionsResolver.ResolvedInstructions.EMPTY); + when(aiReviewService.review(any(ReviewSession.class), any())) + .thenReturn(new ReviewResponse(List.of(), List.of(), null)); + // The head is the reviewed sha while the context loads, then moves before the post — the + // #701 shape: a push landing during the minutes-long model call. + when(prClient.getPullRequest(anyString(), anyString(), anyString(), anyString(), anyInt())) + .thenReturn( + new GitHubPullRequestClient.PullRequestDetails( + "Test PR", + "", + new GitHubPullRequestClient.Ref("abcdefgh"), + new GitHubPullRequestClient.Ref("base-sha"), + 1, + 1, + 1)) + .thenReturn( + new GitHubPullRequestClient.PullRequestDetails( + "Test PR", + "", + new GitHubPullRequestClient.Ref("d4389d2aa"), + new GitHubPullRequestClient.Ref("base-sha"), + 1, + 1, + 1)); + + var surfaced = + orchestrator.review( + new ReviewOrchestrator.ReviewRequest( + "owner", + "repo", + 42, + "abcdefgh", + "Test PR", + "", + "base1234567", + "main", + 123L, + false)); + + assertFalse(surfaced); + // Nothing lands on the PR: no review, no summary comment, no failure notice. + verify(reviewClient, never()) + .createReview(anyString(), anyString(), anyString(), anyString(), anyInt(), any()); + verify(commentClient, never()) + .createComment(anyString(), anyString(), anyString(), anyString(), anyInt(), any()); + // Counted as a structured skip, and the stale check run is concluded as skipped. + assertEquals(1L, skipEmitter.countsByReason().get("HEAD_MOVED")); + var updateCaptor = + ArgumentCaptor.forClass(GitHubCheckRunClient.UpdateCheckRunRequest.class); + verify(checkRunClient) + .updateCheckRun( + anyString(), anyString(), anyString(), anyString(), eq(1L), updateCaptor.capture()); + assertEquals("skipped", updateCaptor.getValue().conclusion()); + verify(session).setStatus(ReviewSession.STATUS_FAILED); + verify(broadcaster, times(2)).broadcast(any(SessionEventBroadcaster.SessionEvent.class)); + } + } + @Test void shouldFallbackWhenInlineCommentsRejected() { var comment = @@ -2898,13 +3036,109 @@ void shouldFallbackWhenInlineCommentsRejected() { } @Test - void shouldThrowReviewPostExceptionWhenFallbackImpossible() { + void shouldPostBodyAsIssueCommentWhenNoCommentsReviewRejected() { + var req = new GitHubReviewClient.CreateReviewRequest("sha", "body", "COMMENT", List.of()); + + when(reviewClient.createReview( + anyString(), anyString(), anyString(), anyString(), anyInt(), any())) + .thenThrow(new RuntimeException("422")); + + assertDoesNotThrow( + () -> reviewPublisher.createReviewWithFallback("Bearer tok", "owner", "repo", 7, req)); + + verify(reviewClient, times(1)) + .createReview(anyString(), anyString(), anyString(), anyString(), anyInt(), any()); + var captor = ArgumentCaptor.forClass(GitHubCommentClient.CreateCommentRequest.class); + verify(commentClient) + .createComment( + eq("Bearer tok"), anyString(), eq("owner"), eq("repo"), eq(7), captor.capture()); + assertTrue(captor.getValue().body().startsWith(ReviewPublisher.REVIEW_REFUSED_NOTE)); + assertTrue(captor.getValue().body().contains("body")); + } + + @Test + void shouldLogGitHubResponseBodyWhenReviewRejected() { + // A rejection whose cause chain carries the HTTP response: the 422 body must be readable + // through the GitHubApiError seam (redacted + capped) instead of being discarded. + var response = + jakarta.ws.rs.core.Response.status(422) + .entity("{\"message\":\"Unprocessable Entity\",\"errors\":[\"commit_id stale\"]}") + .build(); + var rejection = new RuntimeException(new jakarta.ws.rs.WebApplicationException(response)); + var req = new GitHubReviewClient.CreateReviewRequest("sha", "body", "COMMENT", List.of()); + when(reviewClient.createReview( + anyString(), anyString(), anyString(), anyString(), anyInt(), any())) + .thenThrow(rejection); + + assertDoesNotThrow( + () -> reviewPublisher.createReviewWithFallback("Bearer tok", "owner", "repo", 7, req)); + + // The generation is preserved as a comment; the diagnostics path ran without consuming the + // response in a way that breaks the fallback. + verify(commentClient) + .createComment(eq("Bearer tok"), anyString(), eq("owner"), eq("repo"), eq(7), any()); + } + + @Test + void shouldPostBodyAsIssueCommentWhenRetryWithoutCommentsAlsoRejected() { + var comment = + new GitHubReviewClient.ReviewComment( + "src/Main.java", 10, null, null, "RIGHT", "Fix this"); + var req = + new GitHubReviewClient.CreateReviewRequest("sha", "body", "COMMENT", List.of(comment)); + when(reviewClient.createReview( + anyString(), anyString(), anyString(), anyString(), anyInt(), any())) + .thenThrow(new RuntimeException("422")); + + assertDoesNotThrow( + () -> reviewPublisher.createReviewWithFallback("Bearer tok", "owner", "repo", 7, req)); + + verify(reviewClient, times(2)) + .createReview(anyString(), anyString(), anyString(), anyString(), anyInt(), any()); + verify(commentClient) + .createComment(eq("Bearer tok"), anyString(), eq("owner"), eq("repo"), eq(7), any()); + } + + @Test + void shouldUseZeroIssuesMessageWhenRejectedReviewBodyIsBlank() { + when(reviewClient.createReview( + anyString(), anyString(), anyString(), anyString(), anyInt(), any())) + .thenThrow(new RuntimeException("422")); + + reviewPublisher.createReviewWithFallback( + "Bearer tok", + "owner", + "repo", + 7, + new GitHubReviewClient.CreateReviewRequest("sha", "", "APPROVE", List.of())); + reviewPublisher.createReviewWithFallback( + "Bearer tok", + "owner", + "repo", + 7, + new GitHubReviewClient.CreateReviewRequest("sha", null, "APPROVE", List.of())); + + var captor = ArgumentCaptor.forClass(GitHubCommentClient.CreateCommentRequest.class); + verify(commentClient, times(2)) + .createComment( + anyString(), anyString(), anyString(), anyString(), anyInt(), captor.capture()); + for (var comment : captor.getAllValues()) { + assertTrue(comment.body().contains(PrSummaryGenerator.ZERO_ISSUES_MESSAGE)); + } + } + + @Test + void shouldThrowReviewPostExceptionWhenCommentFallbackFailsToo() { var req = new GitHubReviewClient.CreateReviewRequest("sha", "body", "COMMENT", List.of()); var rejection = new RuntimeException("422"); when(reviewClient.createReview( anyString(), anyString(), anyString(), anyString(), anyInt(), any())) .thenThrow(rejection); + var commentFailure = new RuntimeException("comment 403"); + when(commentClient.createComment( + anyString(), anyString(), anyString(), anyString(), anyInt(), any())) + .thenThrow(commentFailure); ReviewPostException ex = assertThrows( @@ -2914,6 +3148,7 @@ void shouldThrowReviewPostExceptionWhenFallbackImpossible() { assertTrue(ex.getMessage().contains("owner/repo #7")); assertSame(rejection, ex.getCause()); + assertSame(commentFailure, ex.getSuppressed()[0]); verify(reviewClient, times(1)) .createReview(anyString(), anyString(), anyString(), anyString(), anyInt(), any()); } From 3fe0174e3c154d899c09c98ef6965c4803ae5150 Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Thu, 13 Aug 2026 18:59:22 -0300 Subject: [PATCH 2/4] fix(review): prove the rejection log on the production exception shape and fix Sonar findings - Replace the fabricated WebApplicationException in the 422-logging test with the REST client's own ClientWebApplicationException (which extends jakarta.ws.rs.WebApplicationException and carries the response), and assert the logged line carries status=422 and GitHub's message. - Guard the Optional access Sonar flagged (S3655) by filtering the fresh head through headMoved before isPresent/get. - Reattach fetchPrTotals' javadoc, moving currentHeadSha below it (S8491). --- .../review/ReviewContextLoader.java | 20 ++--- .../review/ReviewOrchestrator.java | 17 ++-- .../review/ReviewOrchestratorTest.java | 90 ++++++++++++++----- 3 files changed, 86 insertions(+), 41 deletions(-) diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewContextLoader.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewContextLoader.java index ff23a0c5..6ec14787 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewContextLoader.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewContextLoader.java @@ -481,6 +481,16 @@ List fetchPrFiles( * {@code null} so the summary falls back to the diff-derived counts rather than failing the * review. */ + PrTotals fetchPrTotals(String auth, String owner, String repo, int prNumber) { + try { + var pr = prClient.getPullRequest(auth, ACCEPT, owner, repo, prNumber); + return new PrTotals(pr.changedFiles(), pr.additions(), pr.deletions()); + } catch (RuntimeException e) { + Log.warn("Failed to fetch PR totals; summary will fall back to diff-derived counts", e); + return null; + } + } + /** * The PR's head SHA as GitHub reports it right now — a fresh read, taken just before the run * posts, so a head that moved during the minutes-long model call is caught (#704). Goes through @@ -507,16 +517,6 @@ Optional currentHeadSha(String auth, ReviewOrchestrator.ReviewRequest re } } - PrTotals fetchPrTotals(String auth, String owner, String repo, int prNumber) { - try { - var pr = prClient.getPullRequest(auth, ACCEPT, owner, repo, prNumber); - return new PrTotals(pr.changedFiles(), pr.additions(), pr.deletions()); - } catch (RuntimeException e) { - Log.warn("Failed to fetch PR totals; summary will fall back to diff-derived counts", e); - return null; - } - } - /** * Reads totals after the file list and rejects a review whose webhook SHA is no longer current. * The files endpoint is keyed only by PR number, so without this check a force-push can pair the diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestrator.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestrator.java index 9d1578cf..a87f1aa4 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestrator.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestrator.java @@ -28,7 +28,6 @@ import jakarta.enterprise.context.control.ActivateRequestContext; import jakarta.inject.Inject; import java.util.List; -import java.util.Optional; import java.util.concurrent.CompletableFuture; import java.util.concurrent.ExecutorService; import java.util.function.Consumer; @@ -305,9 +304,11 @@ public boolean review(ReviewRequest request) { // run (the dispatcher already queued a coalesced run for the new head). Re-read the head // just before the first write and abandon the post when it moved, instead of posting a // review — and resolving inline comments — against a diff that changed underneath it. - var freshHead = contextLoader.currentHeadSha(auth, req); - if (headMoved(req, freshHead)) { - abandonSupersededRun(auth, req, session, checkRunId, freshHead.get()); + final var headReq = req; + var movedHead = + contextLoader.currentHeadSha(auth, req).filter(fresh -> headMoved(headReq, fresh)); + if (movedHead.isPresent()) { + abandonSupersededRun(auth, req, session, checkRunId, movedHead.get()); return false; } @@ -397,13 +398,13 @@ public boolean review(ReviewRequest request) { /** * Whether the freshly read head names a different commit than the one this run reviewed. False - * when the fresh read failed (fail-open — a finished review is never lost to its own guard) or - * when the run has no reviewed sha to compare against. Visible for tests. + * when the run has no reviewed sha to compare against; a fresh read that failed never reaches + * here (fail-open — a finished review is never lost to its own guard). Visible for tests. */ - static boolean headMoved(ReviewRequest req, Optional freshHead) { + static boolean headMoved(ReviewRequest req, String freshHead) { return req.commitSha() != null && !req.commitSha().isBlank() - && freshHead.filter(sha -> !sha.equalsIgnoreCase(req.commitSha())).isPresent(); + && !freshHead.equalsIgnoreCase(req.commitSha()); } /** diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestratorTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestratorTest.java index 68224687..d6badf70 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestratorTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestratorTest.java @@ -2888,19 +2888,17 @@ void headMovedComparesTheReviewedShaAgainstTheFreshHead() { var req = new ReviewOrchestrator.ReviewRequest( "owner", "repo", 42, "abcdefgh", "T", "", "base", "main", 1L, false); - assertTrue(ReviewOrchestrator.headMoved(req, java.util.Optional.of("other-sha"))); - assertFalse(ReviewOrchestrator.headMoved(req, java.util.Optional.of("ABCDEFGH"))); - // Fail-open: an unreadable fresh head never abandons a finished review. - assertFalse(ReviewOrchestrator.headMoved(req, java.util.Optional.empty())); + assertTrue(ReviewOrchestrator.headMoved(req, "other-sha")); + assertFalse(ReviewOrchestrator.headMoved(req, "ABCDEFGH")); // No reviewed sha to compare against: never treated as moved. var noSha = new ReviewOrchestrator.ReviewRequest( "owner", "repo", 42, "", "T", "", "base", "main", 1L, false); - assertFalse(ReviewOrchestrator.headMoved(noSha, java.util.Optional.of("other-sha"))); + assertFalse(ReviewOrchestrator.headMoved(noSha, "other-sha")); var nullSha = new ReviewOrchestrator.ReviewRequest( "owner", "repo", 42, null, "T", "", "base", "main", 1L, false); - assertFalse(ReviewOrchestrator.headMoved(nullSha, java.util.Optional.of("other-sha"))); + assertFalse(ReviewOrchestrator.headMoved(nullSha, "other-sha")); } @Test @@ -3058,25 +3056,71 @@ void shouldPostBodyAsIssueCommentWhenNoCommentsReviewRejected() { @Test void shouldLogGitHubResponseBodyWhenReviewRejected() { - // A rejection whose cause chain carries the HTTP response: the 422 body must be readable - // through the GitHubApiError seam (redacted + capped) instead of being discarded. - var response = - jakarta.ws.rs.core.Response.status(422) - .entity("{\"message\":\"Unprocessable Entity\",\"errors\":[\"commit_id stale\"]}") - .build(); - var rejection = new RuntimeException(new jakarta.ws.rs.WebApplicationException(response)); - var req = new GitHubReviewClient.CreateReviewRequest("sha", "body", "COMMENT", List.of()); - when(reviewClient.createReview( - anyString(), anyString(), anyString(), anyString(), anyInt(), any())) - .thenThrow(rejection); + // The production shape end-to-end: the REST client throws ClientWebApplicationException + // (a jakarta.ws.rs.WebApplicationException carrying the response), and the 422 body must + // land in the log through the GitHubApiError seam (redacted + capped). + var records = new java.util.concurrent.CopyOnWriteArrayList(); + var handler = + new java.util.logging.Handler() { + @Override + public void publish(java.util.logging.LogRecord logRecord) { + records.add(logRecord); + } + + @Override + public void flush() { + // Nothing buffered. + } + + @Override + public void close() { + // Nothing to release. + } + }; + var logger = + org.jboss.logmanager.LogContext.getLogContext() + .getLogger(ReviewPublisher.class.getName()); + var previousLevel = logger.getLevel(); + logger.addHandler(handler); + logger.setLevel(java.util.logging.Level.ALL); + try { + var response = + jakarta.ws.rs.core.Response.status(422) + .entity( + "{\"message\":\"Unprocessable Entity\",\"errors\":[\"commit_id is stale\"]}") + .build(); + var rejection = new org.jboss.resteasy.reactive.ClientWebApplicationException(response); + var req = new GitHubReviewClient.CreateReviewRequest("sha", "body", "COMMENT", List.of()); + when(reviewClient.createReview( + anyString(), anyString(), anyString(), anyString(), anyInt(), any())) + .thenThrow(rejection); - assertDoesNotThrow( - () -> reviewPublisher.createReviewWithFallback("Bearer tok", "owner", "repo", 7, req)); + assertDoesNotThrow( + () -> reviewPublisher.createReviewWithFallback("Bearer tok", "owner", "repo", 7, req)); - // The generation is preserved as a comment; the diagnostics path ran without consuming the - // response in a way that breaks the fallback. - verify(commentClient) - .createComment(eq("Bearer tok"), anyString(), eq("owner"), eq("repo"), eq(7), any()); + // The generation is preserved as a comment, and the log carries GitHub's own words. + verify(commentClient) + .createComment(eq("Bearer tok"), anyString(), eq("owner"), eq("repo"), eq(7), any()); + var logged = + records.stream() + .map( + r -> + r.getParameters() != null && r.getParameters().length > 0 + ? String.format(r.getMessage(), r.getParameters()) + : r.getMessage()) + .toList(); + assertTrue( + logged.stream() + .anyMatch( + line -> + line.contains("GitHub rejected the review post for owner/repo #7") + && line.contains("status=422") + && line.contains("commit_id is stale")), + () -> "422 diagnostics not logged; saw: " + logged); + } finally { + logger.removeHandler(handler); + logger.setLevel(previousLevel); + } } @Test From 6e2064e128d579b9c097d0cd6fa4d35fbaa23a41 Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Thu, 13 Aug 2026 19:17:07 -0300 Subject: [PATCH 3/4] fix(review): gate the issue-comment fallback on a confirmed 4xx refusal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An ambiguous failure — a client-side timeout, a connection reset, a 5xx — carries no proof GitHub refused the review post; the POST may have been applied, so posting the body again as a comment would duplicate the review while asserting a refusal the code cannot support. The fallback now fires only when the failure carries a response with a 4xx status (the same distinction logReviewRejection already draws); everything else propagates as ReviewPostException and keeps today's fail-and-mark-failed behavior. --- CHANGELOG.md | 2 +- .../review/ReviewPublisher.java | 30 +++++++- .../review/ReviewOrchestratorTest.java | 69 ++++++++++++++++--- 3 files changed, 87 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 137d4dd8..6a058d1b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,7 @@ All notable changes to ThrillhouseBot. ### Fixed -- **A review post GitHub refuses is diagnosed, abandoned when stale, and never lost** (#704): a rejected review post now logs GitHub's own response body (redacted and length-capped) instead of only the status code; a run whose PR head moved while the model call ran abandons its post — counted as a structured `HEAD_MOVED` skip, its check run concluded as skipped — because the coalesced run for the new head re-reviews and posts in its place; and a summary-only review GitHub refuses falls back to posting the same body as an issue comment through the capped, paced write path, instead of discarding the generation behind a "review could not be completed" notice +- **A review post GitHub refuses is diagnosed, abandoned when stale, and never lost** (#704): a rejected review post now logs GitHub's own response body (redacted and length-capped) instead of only the status code; a run whose PR head moved while the model call ran abandons its post — counted as a structured `HEAD_MOVED` skip, its check run concluded as skipped — because the coalesced run for the new head re-reviews and posts in its place; and a summary-only review GitHub definitely refused (a response-carrying 4xx — an ambiguous timeout/5xx still fails, since the review may have landed) falls back to posting the same body as an issue comment through the capped, paced write path, instead of discarding the generation behind a "review could not be completed" notice - **Inline code spans in a decline are stripped delimiter-aware** (#697): the decline re-check now scans backtick runs the CommonMark way — an opening run of N backticks closes at the next run of exactly N — so a span whose body carries a longer backtick run (`` `a``b` ``) or one line ending is stripped whole instead of leaving quoted claim text to reopen a correct decline. An unclosed run stays literal, and a length bound still keeps a stray backtick from swallowing the reply - **Mention-form commands follow the configured bot login** (#698): `TriggerDetector` builds the `@ ` trigger patterns from `BotIdentity.mentionNames()` instead of a hardcoded slug, so `@my-review-bot review` works on a custom-login install; the mention's `@` must open the comment or follow a non-word character, so an email local part never triggers a command. Slash forms and default-config behavior are unchanged diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java index f6abfc31..1e3a89b6 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java @@ -752,9 +752,11 @@ void dismissPendingBotReviews( /** * Submits a PR review, falling back to a summary-only review when inline comments are rejected * (e.g. stale line numbers after a force-push), and to an issue comment carrying the same body - * when the review post itself is refused (#704) — a rejected summary-only review used to discard - * the whole generation behind a "review could not be completed" notice. Throws {@link - * ReviewPostException} only when the comment fallback fails too. + * when GitHub definitely refused the review post (#704) — a rejected summary-only review used to + * discard the whole generation behind a "review could not be completed" notice. Only a + * response-carrying 4xx counts as a refusal; an ambiguous failure (timeout, connection reset, + * 5xx) may have landed the review, so it throws {@link ReviewPostException} instead of risking a + * duplicate — as does a refusal whose comment fallback fails too. */ void createReviewWithFallback( String auth, @@ -789,9 +791,31 @@ void createReviewWithFallback( rejection = retryFailure; } } + // The comment fallback fires only on a definite refusal — a response-carrying 4xx, where + // GitHub rejected the request and the review provably does not exist. An ambiguous failure (a + // timeout, a connection reset, a 5xx) is one where the review may well have landed, so posting + // the body again would duplicate it while asserting a refusal the code cannot support; those + // propagate as before. + if (!isRefusal(rejection)) { + throw new ReviewPostException( + "GitHub review rejected for " + owner + "/" + repo + " #" + prNumber, rejection); + } postReviewBodyAsComment(auth, owner, repo, prNumber, req.body(), rejection); } + /** + * Whether this failure is a definite refusal: it carries GitHub's response and that response is a + * 4xx, so the request was rejected and the review was not created. False for anything ambiguous — + * no response at all, or a 5xx — where the write may have landed. + */ + private static boolean isRefusal(RuntimeException rejection) { + return webApplicationFailure(rejection) + .map(WebApplicationException::getResponse) + .map(jakarta.ws.rs.core.Response::getStatus) + .filter(status -> status >= 400 && status < 500) + .isPresent(); + } + /** * Preserves a refused review as an issue comment: the same body, prefixed with a note that GitHub * refused the review post. Goes through {@link GitHubCommentClient#createComment}, so the comment diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestratorTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestratorTest.java index d6badf70..a60e5d21 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestratorTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestratorTest.java @@ -1587,12 +1587,6 @@ void postReviewFailureMarksReviewFailedInsteadOfLeavingAConcludedCheckRun() { when(reviewClient.createReview( anyString(), anyString(), anyString(), anyString(), anyInt(), any())) .thenThrow(new RuntimeException("502 Bad Gateway")); - // The issue-comment fallback (#704) fails too — only then is the post truly lost; the - // later failure-notice comment goes through. - when(commentClient.createComment( - anyString(), anyString(), anyString(), anyString(), anyInt(), any())) - .thenThrow(new RuntimeException("comment 502")) - .thenReturn(null); orchestrator.review( new ReviewOrchestrator.ReviewRequest( @@ -3033,13 +3027,21 @@ void shouldFallbackWhenInlineCommentsRejected() { assertTrue(captor.getAllValues().get(1).comments().isEmpty()); } + /** A response-carrying 422 in the REST client's own exception type — a definite refusal. */ + private static RuntimeException refusal422() { + return new org.jboss.resteasy.reactive.ClientWebApplicationException( + jakarta.ws.rs.core.Response.status(422) + .entity("{\"message\":\"Unprocessable\"}") + .build()); + } + @Test void shouldPostBodyAsIssueCommentWhenNoCommentsReviewRejected() { var req = new GitHubReviewClient.CreateReviewRequest("sha", "body", "COMMENT", List.of()); when(reviewClient.createReview( anyString(), anyString(), anyString(), anyString(), anyInt(), any())) - .thenThrow(new RuntimeException("422")); + .thenThrow(refusal422()); assertDoesNotThrow( () -> reviewPublisher.createReviewWithFallback("Bearer tok", "owner", "repo", 7, req)); @@ -3132,7 +3134,7 @@ void shouldPostBodyAsIssueCommentWhenRetryWithoutCommentsAlsoRejected() { new GitHubReviewClient.CreateReviewRequest("sha", "body", "COMMENT", List.of(comment)); when(reviewClient.createReview( anyString(), anyString(), anyString(), anyString(), anyInt(), any())) - .thenThrow(new RuntimeException("422")); + .thenThrow(refusal422()); assertDoesNotThrow( () -> reviewPublisher.createReviewWithFallback("Bearer tok", "owner", "repo", 7, req)); @@ -3147,7 +3149,7 @@ void shouldPostBodyAsIssueCommentWhenRetryWithoutCommentsAlsoRejected() { void shouldUseZeroIssuesMessageWhenRejectedReviewBodyIsBlank() { when(reviewClient.createReview( anyString(), anyString(), anyString(), anyString(), anyInt(), any())) - .thenThrow(new RuntimeException("422")); + .thenThrow(refusal422()); reviewPublisher.createReviewWithFallback( "Bearer tok", @@ -3171,11 +3173,58 @@ void shouldUseZeroIssuesMessageWhenRejectedReviewBodyIsBlank() { } } + @Test + void shouldNotPostFallbackCommentOnAmbiguousTransportFailure() { + // A timeout/reset/5xx carries no proof GitHub refused — the review may have landed, so the + // fallback must not risk duplicating it under a "GitHub refused" note. + var req = new GitHubReviewClient.CreateReviewRequest("sha", "body", "COMMENT", List.of()); + var transportFailure = new RuntimeException("502 Bad Gateway"); + when(reviewClient.createReview( + anyString(), anyString(), anyString(), anyString(), anyInt(), any())) + .thenThrow(transportFailure); + + ReviewPostException ex = + assertThrows( + ReviewPostException.class, + () -> + reviewPublisher.createReviewWithFallback("Bearer tok", "owner", "repo", 7, req)); + + assertSame(transportFailure, ex.getCause()); + verify(commentClient, never()) + .createComment(anyString(), anyString(), anyString(), anyString(), anyInt(), any()); + + // A response-carrying 5xx is equally ambiguous: GitHub may have applied the write. + var serverError = + new org.jboss.resteasy.reactive.ClientWebApplicationException( + jakarta.ws.rs.core.Response.status(502).build()); + doThrow(serverError) + .when(reviewClient) + .createReview(anyString(), anyString(), anyString(), anyString(), anyInt(), any()); + + assertThrows( + ReviewPostException.class, + () -> reviewPublisher.createReviewWithFallback("Bearer tok", "owner", "repo", 7, req)); + + // A non-4xx response (however unlikely on this path) is not a refusal either. + var redirect = + new jakarta.ws.rs.WebApplicationException( + jakarta.ws.rs.core.Response.status(302).build()); + doThrow(redirect) + .when(reviewClient) + .createReview(anyString(), anyString(), anyString(), anyString(), anyInt(), any()); + + assertThrows( + ReviewPostException.class, + () -> reviewPublisher.createReviewWithFallback("Bearer tok", "owner", "repo", 7, req)); + verify(commentClient, never()) + .createComment(anyString(), anyString(), anyString(), anyString(), anyInt(), any()); + } + @Test void shouldThrowReviewPostExceptionWhenCommentFallbackFailsToo() { var req = new GitHubReviewClient.CreateReviewRequest("sha", "body", "COMMENT", List.of()); - var rejection = new RuntimeException("422"); + var rejection = refusal422(); when(reviewClient.createReview( anyString(), anyString(), anyString(), anyString(), anyInt(), any())) .thenThrow(rejection); From 95ed05c731b2439d0fce2fdaad817a32f4353a54 Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Thu, 13 Aug 2026 19:30:37 -0300 Subject: [PATCH 4/4] fix(review): require every post attempt to be a confirmed refusal before the comment fallback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit rejection = retryFailure overwrote the first attempt's failure, so an ambiguous first attempt (which may have landed its review) followed by a refused retry classified the run by its last failure alone and fell into the comment fallback — risking a duplicate of the landed review. The run now tracks whether any attempt was ambiguous and throws ReviewPostException when one was; the fallback fires only when every attempt drew a response-carrying 4xx. Both mixed orders are tested. --- .../review/ReviewPublisher.java | 22 ++++++----- .../review/ReviewOrchestratorTest.java | 38 +++++++++++++++++++ 2 files changed, 51 insertions(+), 9 deletions(-) diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java index 1e3a89b6..09937847 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java @@ -754,9 +754,10 @@ void dismissPendingBotReviews( * (e.g. stale line numbers after a force-push), and to an issue comment carrying the same body * when GitHub definitely refused the review post (#704) — a rejected summary-only review used to * discard the whole generation behind a "review could not be completed" notice. Only a - * response-carrying 4xx counts as a refusal; an ambiguous failure (timeout, connection reset, - * 5xx) may have landed the review, so it throws {@link ReviewPostException} instead of risking a - * duplicate — as does a refusal whose comment fallback fails too. + * response-carrying 4xx counts as a refusal, and every attempt made here must have been one; an + * ambiguous failure (timeout, connection reset, 5xx) on any attempt may have landed that + * attempt's review, so it throws {@link ReviewPostException} instead of risking a duplicate — as + * does a refusal whose comment fallback fails too. */ void createReviewWithFallback( String auth, @@ -765,12 +766,14 @@ void createReviewWithFallback( int prNumber, GitHubReviewClient.CreateReviewRequest req) { RuntimeException rejection; + boolean anyAmbiguous; try { reviewClient.createReview(auth, ACCEPT, owner, repo, prNumber, req); return; } catch (RuntimeException e) { logReviewRejection(e, owner, repo, prNumber); rejection = e; + anyAmbiguous = !isRefusal(e); } // CreateReviewRequest's compact constructor normalizes a null comments list to List.of(). if (!req.comments().isEmpty()) { @@ -789,14 +792,15 @@ void createReviewWithFallback( } catch (RuntimeException retryFailure) { logReviewRejection(retryFailure, owner, repo, prNumber); rejection = retryFailure; + anyAmbiguous |= !isRefusal(retryFailure); } } - // The comment fallback fires only on a definite refusal — a response-carrying 4xx, where - // GitHub rejected the request and the review provably does not exist. An ambiguous failure (a - // timeout, a connection reset, a 5xx) is one where the review may well have landed, so posting - // the body again would duplicate it while asserting a refusal the code cannot support; those - // propagate as before. - if (!isRefusal(rejection)) { + // The comment fallback fires only when EVERY attempt was a definite refusal — a + // response-carrying 4xx, where GitHub rejected the request and the review provably does not + // exist. An ambiguous failure (a timeout, a connection reset, a 5xx) on any attempt is one + // where that attempt's review may well have landed, so posting the body again would duplicate + // it while asserting a refusal the code cannot support; those propagate as before. + if (anyAmbiguous) { throw new ReviewPostException( "GitHub review rejected for " + owner + "/" + repo + " #" + prNumber, rejection); } diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestratorTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestratorTest.java index a60e5d21..25a523c6 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestratorTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestratorTest.java @@ -3220,6 +3220,44 @@ void shouldNotPostFallbackCommentOnAmbiguousTransportFailure() { .createComment(anyString(), anyString(), anyString(), anyString(), anyInt(), any()); } + @Test + void shouldNotPostFallbackCommentWhenFirstAttemptWasAmbiguousAndRetryWasRefused() { + // Mixed sequence: the comment-carrying attempt fails ambiguously (that review may have + // landed), then the retry without comments draws a confirmed 422. The last failure alone + // looks like a refusal, but the run is not — falling back would risk duplicating the first + // attempt's landed review. + var comment = + new GitHubReviewClient.ReviewComment( + "src/Main.java", 10, null, null, "RIGHT", "Fix this"); + var req = + new GitHubReviewClient.CreateReviewRequest("sha", "body", "COMMENT", List.of(comment)); + doThrow(new RuntimeException("read timeout")) + .doThrow(refusal422()) + .when(reviewClient) + .createReview(anyString(), anyString(), anyString(), anyString(), anyInt(), any()); + + assertThrows( + ReviewPostException.class, + () -> reviewPublisher.createReviewWithFallback("Bearer tok", "owner", "repo", 7, req)); + + verify(reviewClient, times(2)) + .createReview(anyString(), anyString(), anyString(), anyString(), anyInt(), any()); + verify(commentClient, never()) + .createComment(anyString(), anyString(), anyString(), anyString(), anyInt(), any()); + + // The reverse order — refused first, ambiguous retry — must equally withhold the fallback. + doThrow(refusal422()) + .doThrow(new RuntimeException("connection reset")) + .when(reviewClient) + .createReview(anyString(), anyString(), anyString(), anyString(), anyInt(), any()); + + assertThrows( + ReviewPostException.class, + () -> reviewPublisher.createReviewWithFallback("Bearer tok", "owner", "repo", 7, req)); + verify(commentClient, never()) + .createComment(anyString(), anyString(), anyString(), anyString(), anyInt(), any()); + } + @Test void shouldThrowReviewPostExceptionWhenCommentFallbackFailsToo() { var req = new GitHubReviewClient.CreateReviewRequest("sha", "body", "COMMENT", List.of());