fix(review): revisit a verdict held on pending CI when CI completes - #833
Conversation
With REVIEW_CI_GATING=strict a review that finds nothing while a required check is still running ends as a neutral COMMENT, "cannot be approved until required CI is confirmed green", and nothing came back to it: the bot handled pull_request and comment events only, so the moment CI finished was invisible to it and every green pull request needed a manual /review that re-ran the model to reach the verdict it already had. The app now subscribes to check_suite and status. A review records the head it reviews in CiHoldRegistry before it reads the CI gate, and the verdict when that gate was all that kept it from APPROVE. A completion on that head is routed to its pull request(s) through the registry, since the payload's pull_requests list is empty for a fork, and dispatched as a CI recheck on the pull request's own worker: it runs after any review in flight, is dropped when a review is already queued, and collapses repeated completions into one. CiHoldRevisit then re-reads the head, re-evaluates the gate through the same CiStatusEvaluator and VerdictBuilder code that placed the hold, and posts the held APPROVE and concludes the check run success when it is green, or refreshes the check-run summary and keeps the hold when it is not. No model call is made. The bot's own suite completing is skipped, a pending status is ignored, and the check run is re-concluded by id. check_suite rather than workflow_run because it fires for every app that reports check runs, not only GitHub Actions, and reads from the source the gate does; status as well, because a required context reported through the legacy Commit Status API completes no check suite. Held verdicts are in memory per replica, capped and documented under the single-process limitation; an app registered before this change must subscribe to the two events by hand, and the README says where.
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 doesSubscribes the bot to check_suite and status events, keeps the head and held verdict of a strict-gate CI hold in a bounded in-memory registry, and adds a per-PR dispatchCiRecheck worker path that re-evaluates only the CI gate on completion: green posts the held APPROVE with no model call and concludes the check run success; red refreshes the neutral summary; a moved head drops the hold. Webhook payload, manifest, install pages and README are updated for the two new events. Description vs. ImplementationNo mismatch found between the PR description and the change. Control-Flow Diagram🔀 Show diagramflowchart TD
A["check_suite completed / status settled event"] --> B{"event fields + repo + installation present?"}
B -- "no" --> Z["200 / ignore"]
B -- "yes" --> C["CiHoldRegistry.pullRequestsAt(owner, repo, head)"]
C -- "no held PRs" --> Z
C -- "held PRs" --> D["ReviewDispatcher.dispatchCiRecheck"]
D -- "executor rejected" --> E["dispatch false; controller rolls back dedup"]
D -- "queued or started" --> G["per-PR worker drain: CiHoldRevisit.revisit"]
G --> H{"verdict still held on that head?"}
H -- "no" --> Z
H -- "yes" --> I{"currentHeadSha moved?"}
I -- "yes" --> J["registry.release; no GitHub write"]
I -- "no" --> K["CiStatusEvaluator.evaluate + VerdictBuilder.ciHoldsApproval"]
K -- "red or unreadable" --> L["refresh check-run summary; hold stays"]
K -- "green" --> M["post APPROVE; release hold; conclude check run success"]
Changes Overview
Changed Files
…and 2 more file(s). Risk Assessment
Key Findings
|
| Check | Type | Status | Detail |
|---|---|---|---|
| format | check-run | ⏳ Pending | - |
| test | check-run | ⏳ Pending | - |
| frontend | 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}
thrillhousebot
Bot
added
bug
A re-evaluation was dropped as soon as a review was queued for the pull request, on the premise that the review reads the CI gate itself. A queued review is not guaranteed to reach its gate read: the rate-limit re-check at drain time can skip it, and a review can fail before the read. In that interleaving the green completion and the hold it should have lifted were both lost, and the pull request was back to needing a manual /review. The worker now runs every queued review first and the re-evaluation after them; one that finds the hold released or replaced is a no-op. Also guards the recheck log line on the info level, drops an in-block README anchor that does not resolve on the website page the block is included in, and covers the owner-mismatch branch of the registry lookup.
🤖 ThrillhouseBot — changes since the last review
|
There was a problem hiding this comment.
ThrillhouseBot found no issues in this PR, but some checks are still pending or failed:
- Check test is pending
- Check trivy is pending
- Check format is pending
- Check frontend is pending
- Check dependency-review is pending
ThrillhouseBot closed 1 previous finding(s) this round:
src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewDispatcher.java:353— CI recheck lost when the queued review that superseded it never reads the CI gate
|
/review |
🤖 ThrillhouseBot — changes since the last review
|
The revisit mirrored the orchestrator's moved-head guard, which fails open so a finished review is never lost to its own check. That trade-off does not carry over: a re-evaluation is free to retry on the next CI event, while an approval posted on a head the bot could not verify is the wrong direction. An unreadable head now keeps the hold and logs a warning instead of proceeding to the gate read and the post.
🤖 ThrillhouseBot — changes since the last review
|
There was a problem hiding this comment.
ThrillhouseBot found no issues in this PR, but some checks are still pending or failed:
- Check test is pending
- Check format is pending
- Check frontend is pending
- Check trivy is pending
- Check dependency-review is pending
ThrillhouseBot closed 1 previous finding(s) this round:
src/main/java/dev/thiagogonzaga/thrillhousebot/review/CiHoldRevisit.java:122— CI revisit posts the held APPROVE when the current head cannot be read (fail-open)
|
/review |
# Conflicts: # CHANGELOG.md
…ad swallowed The entry for #825 was written over the '## [0.6.7] — 2026-09-07' heading, so the released section's intro and its fifteen entries sat under Unreleased on this branch and the merge of main brought them along. Unreleased now holds main's entries and this branch's own; the released section is main's.
There was a problem hiding this comment.
ThrillhouseBot found no issues in this PR, but some checks are still pending or failed:
- Check trivy is pending
- Check format is pending
- Check test is pending
- Check frontend is pending
- Check dependency-review is pending
⚠️ Large PR — partial review. 22 file(s) were only partially reviewed because the model's response was cut at its length cap (max-output-tokens) — findings up to the cut were kept (src/test/java/dev/thiagogonzaga/thrillhousebot/review/CiHoldRevisitTest.java, src/main/java/dev/thiagogonzaga/thrillhousebot/review/CiHoldRevisit.java, src/test/java/dev/thiagogonzaga/thrillhousebot/webhook/WebhookControllerTest.java, src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestratorTest.java, src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewDispatcherTest.java, src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewDispatcher.java, src/main/java/dev/thiagogonzaga/thrillhousebot/review/CiHoldRegistry.java, src/test/java/dev/thiagogonzaga/thrillhousebot/review/CiHoldRegistryTest.java, src/main/java/dev/thiagogonzaga/thrillhousebot/webhook/WebhookController.java, src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewResultTest.java, +12 more); the findings and verdict below cover only the reviewed portion.
|
/review |
🤖 ThrillhouseBot — changes since the last review
|
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: Approval posted before check run concluded; failure leaves hold released (
src/main/java/dev/thiagogonzaga/thrillhousebot/review/CiHoldRevisit.java:137)
In reevaluate(), the APPROVE review is posted via createReviewWithFallback and then registry.release(...) is called BEFORE the check run is concluded. If updateCheckRun throws, the catch logs and swallows, but the hold has already been released. The test failedCheckRunUpdateAfterApprovalIsSwallowed asserts the hold is empty after a failed update, confirming this behavior. The PR description says 'A failure while posting keeps the hold for the next event' but a failure while concluding the check run after a successful post does not keep the hold. This is a minor inconsistency: the approval is already posted, so the hold is no longer needed, but the check run remains neutral. The comment on line 137 says 'concluding the check run is best-effort after it' which is accurate, but the release-before-conclusion means a failed conclusion leaves the check run neutral while the PR is approved. Consider concluding the check run before releasing the hold, or documenting that a failed conclusion is acceptable.
⚠️ Large PR — partial review. 22 file(s) were only partially reviewed because the model's response was cut at its length cap (max-output-tokens) — findings up to the cut were kept (src/test/java/dev/thiagogonzaga/thrillhousebot/review/CiHoldRevisitTest.java, src/main/java/dev/thiagogonzaga/thrillhousebot/review/CiHoldRevisit.java, src/test/java/dev/thiagogonzaga/thrillhousebot/webhook/WebhookControllerTest.java, src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestratorTest.java, src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewDispatcherTest.java, src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewDispatcher.java, src/main/java/dev/thiagogonzaga/thrillhousebot/review/CiHoldRegistry.java, src/test/java/dev/thiagogonzaga/thrillhousebot/review/CiHoldRegistryTest.java, src/main/java/dev/thiagogonzaga/thrillhousebot/webhook/WebhookController.java, src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewResultTest.java, +12 more); the findings and verdict below cover only the reviewed portion.
|
/review |
|
The model's response was cut at its response-length cap ( |
# Conflicts: # CHANGELOG.md
|
@thrillhousebot resolved src/main/java/dev/thiagogonzaga/thrillhousebot/webhook/WebhookController.java:520 — recheckHeldVerdicts returns false if any dispatch fails, but the dedup slot is rolled back for all Deferred on purpose, as the reply on its thread explains: |
|
The next review will close every previous finding this comment names by its |
|
/review |
There was a problem hiding this comment.
ThrillhouseBot found no issues in this PR, but some checks are still pending or failed:
- Check test is pending
- Check trivy is pending
- Check format is pending
- Check frontend is pending
- Check dependency-review is pending
Additionally, No new issues in this revision, but 2 previous finding(s) remain unresolved — fix them, or reply on their review thread with why they are deferred. A finding listed only under "Things to double-check" has no thread: clear it by commenting @thrillhousebot resolved path/to/File.java:42 — <the finding's title> on this PR.
There was a problem hiding this comment.
ThrillhouseBot found no issues in this PR, but some checks are still pending or failed:
- Check test is pending
Additionally, No new issues in this revision, but 2 previous finding(s) remain unresolved — fix them, or reply on their review thread with why they are deferred. A finding listed only under "Things to double-check" has no thread: clear it by commenting @thrillhousebot resolved path/to/File.java:42 — <the finding's title> on this PR.
# Conflicts: # src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestratorTest.java
|
/review |
There was a problem hiding this comment.
ThrillhouseBot found no issues in this PR, but some checks are still pending or failed:
- Check format is pending
- Check trivy is pending
- Check test is pending
- Check frontend is pending
- Check dependency-review is pending
Additionally, No new issues in this revision, but 2 previous finding(s) remain unresolved — fix them, or reply on their review thread with why they are deferred. A finding listed only under "Things to double-check" has no thread: clear it by commenting @thrillhousebot resolved path/to/File.java:42 — <the finding's title> on this PR.
There was a problem hiding this comment.
ThrillhouseBot found no issues in this PR, but some checks are still pending or failed:
- Check test is pending
Additionally, No new issues in this revision, but 2 previous finding(s) remain unresolved — fix them, or reply on their review thread with why they are deferred. A finding listed only under "Things to double-check" has no thread: clear it by commenting @thrillhousebot resolved path/to/File.java:42 — <the finding's title> on this PR.
# Conflicts: # CHANGELOG.md
|
@thrillhousebot resolved src/main/java/dev/thiagogonzaga/thrillhousebot/webhook/WebhookController.java:528 — Status handler dispatches rechecks for the bot's own check-run status events Checked against GitHub's webhook documentation rather than assumed, and the premise does not hold. The |
|
The next review will close every previous finding this comment names by its |
|
/review |
🤖 ThrillhouseBot — changes since the last review
|
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: heldOnCiOnly trusts ciHoldsApproval, which cannot tell a CI hold from other non-approve causes (
src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewResult.java:382)
Producer: ReviewResult.heldOnCiOnly() — 'return reviewState == ReviewState.COMMENT && ciHoldsApproval() && !hasIssues() && unresolvedPreviousCount() == 0 && !truncated();', where the pre-existing ciHoldsApproval() (line ~372) is 'return reviewState != ReviewState.APPROVE && (!offendingCiChecks.isEmpty() || ciUnreadable);'. Consumer: ReviewOrchestrator.holdVerdictOnCi — 'if (!result.heldOnCiOnly()) { return false; }' followed by registry.hold, and later CiHoldRevisit reevaluates only the gate and posts an APPROVE whose body asserts 'that review found no issues, and only the CI gate held its approval'. The gap: the predicate establishes that the verdict is not APPROVE and that CI is offending, not that CI was the reason it is not APPROVE. If VerdictBuilder can also produce a findings-free COMMENT for an independent reason — the confidence hold whose banner exists per the pre-existing VerdictBuilderTest case 'theCoverageBannerStaysAboveTheConfidenceHold', or any other non-CI downgrade of an otherwise-clean verdict — and CI happens to be pending, that verdict is reported as held on CI, the revisit posts an APPROVE when CI green lights, and the confidence hold (the actual blocker) is silently lifted without human review — inverting the safety the hold exists for. This is a verification request, not a settled claim: the confidence-hold downgrade code is not in the provided material, so confirm whether a REVIEW state COMMENT with zero findings, zero unresolved-previous, and zero truncation can be produced by a cause other than the CI gate (and note the same conflation would apply in warn mode, where the gate itself never downgrades). The existing ReviewResultTest cases cover truncation, findings and unresolved-previous as non-CI causes but do not cover the confidence-hold case.
🤖 ThrillhouseBot — changes since the last review
|
# Conflicts: # CHANGELOG.md
…d say what the recheck catch guards
|
@thrillhousebot resolved src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewResult.java:382 — heldOnCiOnly trusts ciHoldsApproval, which cannot tell a CI hold from other non-approve causes Verified against |
|
The next review will close every previous finding this comment names by its |
There was a problem hiding this comment.
ThrillhouseBot found no issues in this PR, but some checks are still pending or failed:
- Check test is pending
- Check frontend is pending
- Check trivy is pending
- Check format is pending
- Check dependency-review is pending
Additionally, No new issues in this revision, but 1 previous finding(s) remain unresolved — fix them, or reply on their review thread with why they are deferred. A finding listed only under "Things to double-check" has no thread: clear it by commenting @thrillhousebot resolved path/to/File.java:42 — <the finding's title> on this PR.
|
|
/review |
There was a problem hiding this comment.
No new issues in this revision, but 1 previous finding(s) remain unresolved — fix them, or reply on their review thread with why they are deferred. A finding listed only under "Things to double-check" has no thread: clear it by commenting @thrillhousebot resolved path/to/File.java:42 — <the finding's title> on this PR.
…eld approval (#853) (#856) ## What type of PR is this? - [x] 🐛 Bug fix - [ ] ✨ Feature - [ ] 📝 Documentation - [ ] 🔧 Refactor - [ ] 🚀 Performance - [x] ✅ Test - [ ] 🔒 Security - [ ] 📦 Dependency update - [ ] 🏗️ CI/CD ## Description `ReviewOrchestrator.review` starts `resolveCiEvaluation` with `CompletableFuture.supplyAsync(...)` before `findingPipeline.run(...)` and only `join()`s it afterwards. The verdict was therefore built from a CI reading taken before a model call that routinely runs for minutes. On `kart_backend#446` CI was read at 14:26:39, the last required check completed `success` at 14:27, and at 14:33 the review posted "Check **Backend (Django tests)** is pending" with a `neutral` check run. What changes: - The early read still runs concurrently with the model call, so the fast path is unchanged. - After the call, the new `rereadCiIfHeld` checks the early reading with `VerdictBuilder.ciHoldsApproval(CiEvaluation)`. That is the same predicate the verdict applies (and `CiHoldRevisit` too), so it follows the configured gating mode: a pending, failing or missing required check, or an unreadable source, holds only under a fail-closed mode. - When the early reading holds approval, CI is read once more through `resolveCiEvaluation` and the verdict is built from the fresh reading. - When it does not, the early reading is used and no second call is made. - When the re-read changes the outcome (held → not held), one INFO line names the checks that held the early reading, each passed through `LogSafe.oneLine`, or says the hold was an unreadable CI source. No statuses, conclusions or bodies are logged. - CHANGELOG entry under [Unreleased] → Fixed. ### Interaction with the CI hold from #825 / #833 Nothing in `CiHoldRegistry` or `CiHoldRevisit` changes. `track` still runs before the early read. The hold is still decided by `holdVerdictOnCi` from `ReviewResult.heldOnCiOnly()`, and that result is now built from the fresh reading whenever there was a re-read. - **Re-read is green:** the verdict is `APPROVE` and the check run concludes `success`. `heldOnCiOnly()` is false, so `hold` is never called, and the `finally` block releases the head `track` recorded. A `check_suite`/`status` recheck queued behind the run while the model was working finds no held verdict (`heldAt` is empty) and does nothing, so no second approval is posted. - **Re-read still holds:** the verdict is the same neutral `COMMENT` as today, and `hold` registers the `HeldVerdict` with the head, base ref, check-run id and details URL, exactly as before. A later CI completion lifts it through `CiHoldRevisit`, which re-reads the gate with the same predicate. - **Early reading does not hold:** there is one CI call and behaviour is identical to today. A check that completes after the re-read but before the post is still caught only by the #825 revisit, as before. ## Related Issues Fixes #853 ## How Has This Been Tested? - [x] Unit tests - [ ] Integration tests - [ ] Manual testing New tests in `ReviewOrchestratorTest.CiGatingThroughReview` use the existing wiring: the real `CiStatusEvaluator` over the `checkRunClient` mock, `stubCommonReviewMocks`, `buildCheck`, and the `ciHoldRegistry`. Evaluator calls are counted through `checkRunClient.getAllCheckRuns` / `getAllCombinedStatus`, which each run exactly once per evaluation. - `reviewShouldApproveFromAReReadWhenCiTurnsGreenDuringTheModelCall`: early read `in_progress`, re-read `completed/success`. Expects two evaluations, `APPROVE`, a `success` check run, and no hold or tracked head left. - `reviewShouldApproveFromAReReadWhenAnUnreadableCiSourceReadsCleanly`: early combined-status read throws (unreadable), re-read is clean. Expects two evaluations, `APPROVE`, and no hold. - `reviewShouldStillHoldWhenTheReReadIsPendingToo`: both readings `in_progress`. Expects two evaluations, no `APPROVE`, a `neutral` check run, and a hold registered with check run 1 and base `main`. - `reviewShouldNotReReadCiWhenTheEarlyReadingDoesNotHoldApproval`: early read green. Expects exactly one evaluation. Red first, on the unchanged `main` code (verbatim, both re-read tests): ``` [ERROR] dev.thiagogonzaga.thrillhousebot.review.ReviewOrchestratorTest.reviewShouldApproveFromAReReadWhenCiTurnsGreenDuringTheModelCall -- Time elapsed: 0.155 s <<< FAILURE! org.mockito.exceptions.verification.TooFewActualInvocations: checkRunClient.getAllCheckRuns( <any>, <any>, <any>, <any>, <any> ); Wanted 2 times: -> at dev.thiagogonzaga.thrillhousebot.github.GitHubCheckRunClient.getAllCheckRuns(GitHubCheckRunClient.java:184) But was 1 time: ``` `reviewShouldStillHoldWhenTheReReadIsPendingToo` failed the same way ("Wanted 2 times: … But was 1 time"). The no-re-read test passed on `main` as expected, since it guards the fast path rather than the bug. After the fix: - `./mvnw verify`: 3836 tests, 0 failures, 0 errors; SpotBugs `BugInstance size is 0`; Spotless clean. - Patch coverage (JaCoCo, diff against the merge-base with `main`): 12 trackable lines, 0 gaps. ## Checklist - [x] My code follows the project's coding standards - [x] I have performed a self-review of my own code - [x] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [x] I have updated the documentation accordingly - [x] My changes generate no new warnings or errors ## Screenshots / Logs N/A ## Additional Notes The re-read costs one required-contexts lookup plus one check listing for the head, and only on a review whose early reading held approval. No configuration changes.
1 participant
What type of PR is this?
Description
With
REVIEW_CI_GATING=stricta review that finds nothing while a required check is still running ends as a neutralCOMMENT("cannot be approved until required CI is confirmed green") and nothing came back to it:WebhookControllerhandledpull_request,issue_commentandpull_request_review_commentonly, so the moment CI finished was invisible to the bot and every green PR needed a manual/reviewthat re-ran the model to reach the verdict it already had. Every claim in #825 checks out against the code: the downgrade isVerdictBuilder.buildResult(APPROVE→COMMENTwhenciGating.holdsApproval()and a check is offending or a source unreadable), the check run concludesneutralfromReviewState.COMMENT, and no CI event reached the router. One detail differs from the issue text: the manifest already requestschecks: write(which coverschecks:read) andactions: read, so no new permission is needed, only the event subscriptions.What changes:
WebhookPayload/WebhookControllerhandlecheck_suitecompletedandstatus.check_suiterather thanworkflow_runbecause it fires for every app that reports check runs (Actions, SonarCloud, any other CI), not only GitHub Actions workflows, and it is the source the gate reads (Check Runs API).statusas well, because a required context reported through the legacy Commit Status API (codecov, older CI) completes no check suite, and a hold whose last outstanding check is one of those would otherwise stay stuck. The head is matched through the registry rather than the payload'spull_requestslist, which is empty for a fork. The bot's own suite completing (what every held review ends with) is skipped, and apendingstatus is ignored.CiHoldRegistry(new): per-replica, bounded (256 PRs, LRU by write) map of PR → tracked head or held verdict. The orchestratortracks the head before it reads the CI gate, because the gate is read concurrently with the model call: CI that completes during the call completed after the read, and its event must still find the PR so a recheck can be queued behind the running review. On a normal end the verdict isholded whenReviewResult.heldOnCiOnly()(no finding, nothing unresolved, no truncation, fail-closed CI hold) andreleased on every other outcome (findings, failure, superseded run).ReviewDispatcher.dispatchCiRecheck: the recheck rides the same per-PR worker as reviews. It runs after the review in flight and after any queued review, is never dropped for a queued review (a queued review can be skipped by the drain-time rate-limit re-check or fail before its gate read, and a recheck that finds the hold released or replaced is a cheap no-op), and repeated completions collapse into one pending recheck. Same retire/rejected-executor handling asdispatch.CiHoldRevisit(new): on the worker, re-reads the head (ReviewContextLoader.currentHeadSha+ReviewOrchestrator.headMoved, the A review lost to a GitHub 422: the response body is not logged, and a superseded run still posts #704 guard), drops the hold if it moved and keeps it if the head could not be read; re-evaluates the gate through the same code that placed it (CiStatusEvaluator.evaluate, extracted from the orchestrator'sresolveCiEvaluation, andVerdictBuilder.ciHoldsApproval, extracted frombuildResult). Green: posts anAPPROVEreview on the held sha throughReviewPublisher.createReviewWithFallbackand re-concludes the original check runsuccesswith the usual approve title/summary. Red or unreadable: keeps the hold and refreshes the check-run summary ("No new issues found, but 1 required CI check(s) are still pending or failing. Approval is re-evaluated as the remaining checks complete."). No model call on either path. A failure while posting keeps the hold for the next event.ReviewSession. The session row carries no installation id, check-run id or base ref, so extending it is three columns plus a query and a restart path; the in-memory bound is the same oneSupersededFindingsCarryover(HEAD_MOVED discards a superseded run's verified findings, so a PR pushed to during review ratchets toward approval #806) accepts, and a hold lost to a restart falls back to exactly today's behaviour (manual/review). Documented under the "Single process" limitation and a new "CI gating" README section, which also gives the exact settings path existing installations must use to subscribe the app to the two events. The manifest (install.htmlboth copies,manifest.json), the manual-registration table and the website's getting-started page list the events.Classes near the other open PRs (
ReviewPublisher,GitHubWriteRetry,FollowUpAnalyzer,MaintainerReplyService) are untouched; the revisit usesReviewPublisher's existing package-privatecreateReviewWithFallback.Related Issues
Fixes #825
How Has This Been Tested?
Red first. The tests were written against the unfixed tree; the API they exercise did not exist, so the red is the compile failure:
Tests added:
ReviewOrchestratorTest.CiGatingThroughReview: a review withbuildin progress holds the verdict (registry carries check-run id 1, basemain, the session URL) and concludesneutral; passing checks hold nothing; a failed review releases the tracked head; held + green completion →APPROVEon the held sha, check run re-concludedsuccess,aiReviewService.reviewverified called exactly once (the original review); held + failed check → no approve, hold stays.CiHoldRevisitTest: green posts approve and concludes success; unknown required set drops "required" from the body; red keeps the hold and refreshes the summary with the revisit note; unreadable keeps the hold; moved head drops the hold without any GitHub write; an unreadable head keeps the hold without posting (fails closed, unlike the orchestrator's own guard: a recheck is free to retry on the next CI event, an approval on an unverified head is not); PR without a hold is ignored with no GitHub call at all; event for another sha than the held one is ignored; failed approve post keeps the hold; failed check-run update after the approve is swallowed.CiHoldRegistryTest: track/hold/release, lookup by head (case-insensitive), replacement on a new head, two PRs at one head, cap eviction and re-hold ordering.ReviewDispatcherTest: recheck alone runs; recheck queued behind an in-flight review runs after it (in order); it also runs after a review that was queued before or after it, and after a queued review the rate limiter skips at drain time (a queued review is not guaranteed to read the gate, so the recheck is never dropped for one); two rechecks collapse; a failing recheck does not break the worker; rejected executor returns false and clears state; retired state is not revived.WebhookControllerTest:check_suitecompletedparses and dispatches a recheck per PR at the head;requestedaction, the bot's own suite, missing fields (parameterized) and no held PR are ignored; rejected dispatch rolls back the dedup slot;statussuccessdispatches,pending/missing state/missing sha are ignored.ReviewResultTest.heldOnCiOnly*andVerdictBuilderTeststrict/warnciHoldsApproval.Gates, from the worktree root:
Checklist
Screenshots / Logs
N/A
Additional Notes
statusevent fires for every commit status on any branch; the handler does one in-memory lookup and returns for anything not under a held review.