review: never post to a merged or closed PR#213
Open
jwbron wants to merge 1 commit into
Open
Conversation
A review run takes 12-17 minutes while PRs often merge minutes after the final push, so an in-flight run could complete after the merge and post a stale verdict (seen in production: CHANGES_REQUESTED with 11 blocking comments landing 16 minutes after the merge). Nothing checked PR state: gh-aw's safe-output handlers post regardless of it, and the per-PR concurrency group only cancels when a newer run of the same workflow starts. Two layers close the race: - Subscribe to pull_request closed (fires on merge too) solely so the event enters the existing per-PR concurrency group and cancels an in-flight review within seconds; the activation if excludes closed runs from doing any work, so every job skips and the run costs nothing. - A safe-outputs.steps guard runs immediately before the posting handlers; if the PR is not open at that moment it records why in the job summary and cancels the run, covering manual reruns of stale runs and any cancellation miss. Fails open on API errors, fails closed once a non-open state is observed; checks only state, never recency, so open-PR reviews post normally even when a newer push exists. Verified by compiling with gh-aw v0.81.6 (the version consumers pin) against Khan/frontend's config: closed in the trigger types, the exclusion on the activation job, the guard step rendered before Process Safe Outputs, and the concurrency group unchanged.
🦋 Changeset detectedLatest commit: 205b1a0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
khan-actions-bot
requested review from
a team,
jaredly and
kevinb-khan
and removed request for
a team
jeresig
approved these changes
Jul 8, 2026
Comment on lines
+168
to
+180
| try { | ||
| await github.rest.actions.cancelWorkflowRun({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| run_id: context.runId, | ||
| }); | ||
| // Wait for the runner to receive the cancellation and kill this step. | ||
| await new Promise((resolve) => setTimeout(resolve, 120000)); | ||
| } catch (error) { | ||
| core.warning(`Could not cancel the run (${error.message}).`); | ||
| } | ||
| core.setFailed( | ||
| `PR #${prNumber} is ${pr.state}; refusing to post a review after the fact.`); |
Member
There was a problem hiding this comment.
It's not clear to me why this is needed - it seems like we could just intentionally fail the step, which would have the same effect as cancelling the rest of the workflow, I believe.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A review run takes ~12-17 minutes while PRs often merge minutes after their final push, so an in-flight run can complete after the merge and post a stale verdict. Real case on Khan/frontend#13088: the author pushed at 14:30, merged at 14:31, and the run posted CHANGES_REQUESTED with 11 blocking inline comments at 14:47 on the already-merged PR.
Nothing in the pipeline checks PR state: gh-aw's safe-output handlers post regardless of it (verified in
submit_pr_review.cjs/pr_review_buffer.cjsat gh-aw HEAD; posting to closed targets is documented behavior), and the compiled per-PR concurrency group only cancels a run when a newer run of the same workflow starts, which a merge never produces.The fix (two layers, frontmatter only)
pull_request: closed(fires on merge too) solely so the event enters the existing per-PR concurrency group (cancel-in-progress: true, workflow level) and cancels an in-flight review within seconds. Theif:excludesclosedruns from doing any work; the exclusion compiles onto theactivationjob, so every downstream job skips and the run costs nothing.safe-outputs.stepsstep runs in the safe-outputs job immediately before the posting handlers. If the PR is not open at that moment, it records why in the run's job summary and cancels the run, so nothing posts. This covers what layer 1 cannot: manual reruns of stale runs after a merge, and any cancellation miss.Guard semantics: fails open on API errors (a transient GitHub error must never suppress a legitimate review of an open PR); fails closed once a non-open state is positively observed (if the cancel call is rejected, it fails the job so the handlers still never run). It checks only
state, never recency: a run whose PR is still open posts normally even when a newer push exists, since that push's own run supersedes it.Post-merge findings are deliberately discarded rather than rerouted (nobody acts on them today; see below); rerouting them to an issue/comment, or making the reviewer a blocking check, are separate policy decisions this change does not foreclose.
Why discard rather than keep or reroute the post-merge output
Measured over the last 1,000 merged Khan/frontend PRs (Dec 2025 through Jul 2026), 598 of which received a bot review:
So post-merge posting produces output nobody acts on, at agent-run cost, with the occasional wrong blocking verdict on the record as the downside. If deliberate post-merge coverage is ever wanted, the right shape is a designed
merged-trigger flow that files findings somewhere triaged, which this change does not preclude.Consumers
Pick up by updating the pin and recompiling (
gh aw update, commit the regeneratedreview.md+review.lock.yml). Requires gh-aw >= v0.81.6 at compile time (safe-outputs.stepsis in its schema; consumers already compile with v0.81.6). No config.md changes, no new secrets: the guard reusesKHAN_ACTIONS_BOT_TOKEN(already required by resolve-thread/add-reviewer) because the safe-outputs job'sGITHUB_TOKENlacksactions: writefor the cancel call.No conflict with the open #196-#208 stack: it leaves the
on:,if:, andsafe-outputs:frontmatter regions untouched (its only frontmatter change is thepre-agent-stepslib checkout).Test plan
.github/aw/review/config.md: 0 errors; asserted in the lock thatclosedis in the trigger types, the exclusion landed on theactivationjobif:, the guard step renders before "Process Safe Outputs" with the bot token, and the concurrency group is unchanged.node --check.