fix(review): log the 422 body, abandon superseded posts, and preserve refused reviews as comments

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 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 `@<bot> <command>` 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
 

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;
 
@@ -490,6 +491,32 @@ PrTotals fetchPrTotals(String auth, String owner, String repo, int prNumber) {
     }
   }
 
+  /**
+   * 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<String> 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();
+    }
+  }
+
   /**
    * 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

src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestrator.java

@@ -100,6 +100,8 @@ static String contextWindowCheckSummary() {
 
   private final FindingFeedbackCaptureService findingFeedbackCapture;
 
+  private final ReviewSkipEmitter skipEmitter;
+
   private final ExecutorService reviewExecutor;
 
   /**
@@ -215,6 +217,7 @@ public ReviewOrchestrator(
       VerdictBuilder verdictBuilder,
       FindingPipeline findingPipeline,
       FindingFeedbackCaptureService findingFeedbackCapture,
+      ReviewSkipEmitter skipEmitter,
       @ReviewExecutor ExecutorService reviewExecutor) {
     this.config = config;
     this.authClient = authClient;
@@ -229,6 +232,7 @@ public ReviewOrchestrator(
     this.verdictBuilder = verdictBuilder;
     this.findingPipeline = findingPipeline;
     this.findingFeedbackCapture = findingFeedbackCapture;
+    this.skipEmitter = skipEmitter;
     this.reviewExecutor = reviewExecutor;
   }
 
@@ -296,6 +300,18 @@ 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.
+      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;
+      }
+
       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 +393,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 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, String freshHead) {
+    return req.commitSha() != null
+        && !req.commitSha().isBlank()
+        && !freshHead.equalsIgnoreCase(req.commitSha());
+  }
+
+  /**
+   * 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

src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java

@@ -17,13 +17,15 @@
 
 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;
 import dev.thiagogonzaga.thrillhousebot.review.ai.ReviewResponse;
 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,35 +744,144 @@ 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 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, 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,
       String owner,
       String repo,
       int prNumber,
       GitHubReviewClient.CreateReviewRequest req) {
+    RuntimeException rejection;
+    boolean anyAmbiguous;
     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;
+      anyAmbiguous = !isRefusal(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,
           prNumber);
       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;
+        anyAmbiguous |= !isRefusal(retryFailure);
+      }
+    }
+    // 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);
+    }
+    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
+   * 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<WebApplicationException> webApplicationFailure(Throwable rejection) {
+    for (Throwable t = rejection; t != null; t = t.getCause()) {
+      if (t instanceof WebApplicationException web) {
+        return Optional.of(web);
+      }
     }
+    return Optional.empty();
   }
 
   /**

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
 }

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)))