fix(review): log the 422 body, abandon superseded posts, and preserve refused reviews as comments - #705
Conversation
… refused reviews as comments 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
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
🤖 ThrillhouseBot PR SummaryWhat this PR doesShips the three #704 postmortem fixes: a rejected review post now logs GitHub's own response body through the existing GitHubApiError seam (redacted and capped); ReviewOrchestrator re-reads the PR head just before posting and abandons a superseded run (HEAD_MOVED skip, check run concluded skipped, session failed, nothing user-facing posted) when the head moved during the model call; and a refused summary-only review (or a refused retry) falls back to posting the same body as an issue comment through the paced conversation-write path, with ReviewPostException firing only when that comment fallback also fails. Description vs. ImplementationNo mismatch found between the PR description and the change. Control-Flow Diagram🔀 Show diagramflowchart TD
A["review() finishes the model call"] --> B["currentHeadSha: fresh GET of PR head"]
B --> C{"head moved from reviewed sha?"}
C -- "yes" --> D["abandonSupersededRun: HEAD_MOVED skip, check run skipped, session failed"]
D --> E["return false: nothing posted"]
C -- "no" --> F["publishSummaryBestEffort"]
F --> G["postReview"]
G --> H{"createReview throws?"}
H -- "yes" --> I["logReviewRejection: response body via GitHubApiError"]
I --> J{"comments empty or retry failed?"}
J -- "yes" --> K["postReviewBodyAsComment: REVIEW_REFUSED_NOTE plus body"]
J -- "no" --> L["retry without comments"]
L --> H
K --> M["ReviewPostException only when comment fallback fails too"]
H -- "no" --> N["review posted"]
Changes Overview
Changed Files
Risk Assessment
Key Findings
Things to double-check1 lower-confidence finding
|
| Check | Type | Status | Detail |
|---|---|---|---|
| format | check-run | ⏳ Pending | - |
| frontend | check-run | ⏳ Pending | - |
| test | check-run | ⏳ Pending | - |
| trivy | check-run | ⏳ Pending | - |
| dependency-review | check-run | ⏳ Pending | - |
Automated review by ThrillhouseBot. Reply with /review to re-run.
![thrillhousebot[bot]](https://avatars.githubusercontent.com/in/3996985?s=60&v=4){kind=small}
There was a problem hiding this comment.
ThrillhouseBot noted 1 lower-confidence item(s) under Things to double-check in the PR summary (not posted as inline threads):
- MEDIUM: 422-body logging relies on production exception being a WebApplicationException and its test never asserts the log (
src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java:850)
Fix 1 only reads the HTTP response if webApplicationFailure finds ajakarta.ws.rs.WebApplicationExceptionin the failure's cause chain (if (t instanceof WebApplicationException web)). The #701 log quoted in the issue shows the real production failure is aClientWebApplicationExceptionthrown by the MicroProfile REST client, while the only test covering this path fabricates the trigger asnew RuntimeException(new jakarta.ws.rs.WebApplicationException(response))— a shape chosen by the test author, not the production one. If the REST client exception is not assignable tojakarta.ws.rs.WebApplicationException(or does not carry one in its cause chain),logReviewRejectionfalls into theorElse("no HTTP response to read ...")branch and the PR's headline fix silently does nothing in production. Additionally,shouldLogGitHubResponseBodyWhenReviewRejectednever asserts that any log or diagnostics output was produced — it only verifies the fallback comment was created, so it would stay green even if the diagnostics computation were broken. Verify that the exception Quarkus/RESTEasy surfaces for a 422 (theClientWebApplicationExceptionseen in #701) is caught byinstanceof WebApplicationExceptionand thatGitHubApiError.of(...)/diagnostics()reads the response body; consider asserting the diagnostics log line in the test.
thrillhousebot
Bot
added
bug
…e 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).
|
@thrillhousebot resolved src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java:850 — 422-body logging relies on production exception being a WebApplicationException and its test never asserts the log Verified against resteasy-reactive-common 3.38.0: 🤖 Addressed by Claude Code |
|
A line number followed by a spaced separator and another number reads as a line range, and a range names no single finding. The one exception is the summary's own row: when an em dash (—) after the line number is followed by the finding's full title exactly as the summary prints it, the next review will still close that finding. Anything else — a real range, a shortened title, or a dotted spelling — clears nothing; name each finding by its |
|
@thrillhousebot resolved src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java:850 — 422-body logging relies on production exception being a WebApplicationException and its test never asserts the log |
|
A line number followed by a spaced separator and another number reads as a line range, and a range names no single finding. The one exception is the summary's own row: when an em dash (—) after the line number is followed by the finding's full title exactly as the summary prints it, the next review will still close that finding. Anything else — a real range, a shortened title, or a dotted spelling — clears nothing; name each finding by its |
🤖 ThrillhouseBot — changes since the last review
|
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.
🤖 ThrillhouseBot — changes since the last review
|
…ore the comment fallback 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.
|
🤖 ThrillhouseBot — changes since the last review
|
1 participant
What type of PR is this?
Description
A review on #701 was lost to a GitHub 422 that the logs could not explain. This PR ships the three fixes from the postmortem:
GitHubApiErrorseam and logs its diagnostics line — status, throttle headers, and the response body, with credential-shaped text redacted and the length capped. The next 422 will name its cause.ReviewOrchestratorre-reads the PR head (a fresh read through the token-healing read seam), and when it moved from the sha the run reviewed, the post is abandoned: counted as a structuredHEAD_MOVEDskip viaReviewSkipEmitter, the stale check run concluded asskipped, and nothing user-facing posted — the dispatcher's coalesced run for the new head re-reviews and posts in its place. This also stops inline comments resolving against a diff that moved underneath them. Fail-open: a failed head read never abandons a finished review.createReviewWithFallbackkeeps today's retry-without-comments path; when a no-comments review (or that retry) is refused, the same body is posted as an issue comment — through the capped, paced conversation-write path — with a note that GitHub refused the review post, instead of discarding the generation behind "review could not be completed".ReviewPostExceptionnow fires only when the comment fallback fails too.Per the issue, this does not attempt to fix the unknown 422 root cause (it cannot be named until fix 1 ships) and does not change review-event semantics beyond what the fallback needs.
Related Issues
Closes #704
How Has This Been Tested?
New tests cover: the rejection-diagnostics path with a response-carrying cause chain; the no-comments 422 falling back to an issue comment carrying the body (and the zero-issues message when the body is blank/null); the comment-carrying 422 keeping the retry-without-comments path and falling through to the comment fallback when the retry is refused too;
ReviewPostException(with the comment failure suppressed) when the fallback also fails; the full-path head-moved abandon (no review, no comment,HEAD_MOVEDcounted, check runskipped);headMovedcomparisons including fail-open; abandon surviving check-run and persistence failures; andcurrentHeadShareturning the fresh head and empty on every unreadable shape../mvnw verifypasses (3222 tests) and a local merge-base JaCoCo simulation of codecov/patch reports 100% on changed lines.Checklist
Screenshots / Logs
N/A
Additional Notes
CHANGELOG updated under
[Unreleased]→ Fixed.