From 205b1a04eb558b9b22f43bc47ce8333ae590ec06 Mon Sep 17 00:00:00 2001 From: James Wiesebron Date: Tue, 7 Jul 2026 14:51:18 -0700 Subject: [PATCH] review: never post to a merged or closed PR 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/review-merged-pr-guard.md | 14 ++++++ workflows/review/README.md | 7 +++ workflows/review/review.md | 72 +++++++++++++++++++++++++++- 3 files changed, 92 insertions(+), 1 deletion(-) create mode 100644 .changeset/review-merged-pr-guard.md diff --git a/.changeset/review-merged-pr-guard.md b/.changeset/review-merged-pr-guard.md new file mode 100644 index 00000000..f246dc5d --- /dev/null +++ b/.changeset/review-merged-pr-guard.md @@ -0,0 +1,14 @@ +--- +"review": minor +--- + +Never post a review to a merged or closed PR. + +A review run takes ~12-17 minutes while PRs often merge minutes after their final push, so an in-flight run could complete after the merge and post a stale verdict (observed in production: CHANGES_REQUESTED with 11 blocking inline comments landing 16 minutes after the PR merged). Nothing in the pipeline checked PR state: gh-aw's safe-output handlers post regardless of it, and the compiled per-PR concurrency group only cancels a run when a newer run of the same workflow starts. + +Two layers close the race: + +- The workflow now subscribes to `pull_request: closed` (which also fires on merge) solely so the event enters the existing per-PR concurrency group and cancels an in-flight review within seconds. The job-level `if:` excludes `closed` runs from doing any work, so they skip every job and cost nothing. +- A guard step injected via `safe-outputs.steps` 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 job summary and cancels the run, so nothing posts; this also covers manual reruns of stale runs after a merge. The guard fails open on API errors and checks only PR state, never recency: a run whose PR is still open posts normally even if a newer push exists. + +Consumers pick this up by updating the pin and recompiling (`gh aw update` and committing the regenerated `review.md` + `review.lock.yml`). Requires gh-aw >= v0.81.6 at compile time for `safe-outputs.steps`; no config.md changes and no new secrets (`KHAN_ACTIONS_BOT_TOKEN` is already required). diff --git a/workflows/review/README.md b/workflows/review/README.md index f9d462da..0d9dbd1e 100644 --- a/workflows/review/README.md +++ b/workflows/review/README.md @@ -31,6 +31,13 @@ The workflow then posts the per-line Conventional Comments that survived validat submits an approve / request-changes review, and on approval posts the risk/patterns summary and requests the owning teams. The config files below feed these sub-agents. +A review is only ever posted while its PR is still open. Merging or closing the PR +cancels an in-flight run within seconds (the `closed` trigger enters the same per-PR +concurrency group), and a deterministic pre-check in the safe-outputs job skips all +posting if the PR is somehow no longer open at submit time (e.g. a manual rerun of a +stale run), recording why in the run's job summary. A run whose PR is still open posts +normally even when a newer push exists; the newer push's run supersedes it. + ## Install ```sh diff --git a/workflows/review/review.md b/workflows/review/review.md index c2790010..beee619b 100644 --- a/workflows/review/review.md +++ b/workflows/review/review.md @@ -7,13 +7,23 @@ description: > on: pull_request: - types: [opened, synchronize, reopened, ready_for_review] + types: [opened, synchronize, reopened, ready_for_review, closed] # Run automatically on every code push to a PR (`synchronize`) and when a PR # leaves draft (`ready_for_review`), not via a slash command. Reviewer requests # are gated on draft status in the prompt (Step 8). Do NOT post a "review # started / completed" status comment — only the review itself, the # risks/patterns comment (Step 7), and reviewer requests should appear on the # PR. + # + # `closed` (which also fires on merge) is subscribed ONLY to cancel an in-flight + # review of the same PR: gh-aw compiles a workflow-level concurrency group keyed + # on the PR number with `cancel-in-progress: true`, and cancellation happens at + # run level as soon as the closed-event run enters the group, before any job + # condition is evaluated. The `if:` below excludes `closed` runs from doing any + # work themselves, so every job skips and the run costs nothing. Without this, a + # review still running when its PR merges (median run is ~12 minutes) posts a + # stale verdict on the merged PR. The safe-outputs guard step below is the + # deterministic backstop for the same race. status-comment: false # Disable gh-aw's pre-activation permission + confused-deputy gate so a same-repo # collaborator pushing to a PR they didn't open still triggers the review (the @@ -32,7 +42,11 @@ on: # (zero AI credits) and posts nothing. The label is evaluated on each trigger event # (open/synchronize/reopen/ready), so adding it prevents the *next* run — it does not # retroactively dismiss a review already left on an earlier push. +# +# `closed` events are excluded entirely: those runs exist only to enter the per-PR +# concurrency group and cancel an in-flight review (see the trigger comment above). if: >- + github.event.action != 'closed' && !startsWith(github.event.pull_request.head.ref, 'deploy/') && github.event.pull_request.head.ref != 'changeset-release/main' && !contains(github.event.pull_request.labels.*.name, 'skip-ai-review') @@ -108,6 +122,62 @@ safe-outputs: # .github/aw/review/config.md (see the `imports:` note above), because its # `allowed-team-reviewers` allowlist is repo-specific. Defining it here would override # the import and drop the consumer's allowlist. + # + # Deterministic guard against the merge race: a run that started before its PR + # merged or closed would otherwise post its verdict after the fact (observed on + # a production PR: CHANGES_REQUESTED with 11 blocking comments landed 16 minutes + # after the merge). gh-aw injects these steps into the safe-outputs job after + # checkout/setup and before any safe-output handler executes; if the PR is no + # longer open at that moment, the step records why in the job summary and + # cancels the run, so nothing posts. The `closed` trigger above normally cancels + # such runs within seconds of the merge; this guard covers what that cannot: + # manual reruns of stale runs, and any cancellation miss. The guard fails OPEN + # on API errors (a transient GitHub error must never suppress a legitimate + # review of an open PR) and 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 if a newer push exists, since that push's own + # run supersedes it. The bot token is used because this job's GITHUB_TOKEN + # lacks the `actions: write` scope that cancelling the run requires. + steps: + - name: Skip posting when the PR is no longer open + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 + with: + github-token: ${{ secrets.KHAN_ACTIONS_BOT_TOKEN }} + script: | + const prNumber = context.payload.pull_request?.number; + if (!prNumber) return; + let pr; + try { + ({data: pr} = await github.rest.pulls.get({ + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: prNumber, + })); + } catch (error) { + core.warning( + `Could not check PR #${prNumber} state (${error.message}); posting normally.`); + return; + } + if (pr.state === 'open') return; + const why = pr.merged ? `merged at ${pr.merged_at}` : 'closed'; + await core.summary + .addRaw(`Review not posted: PR #${prNumber} was ${why} before this run ` + + 'finished, so its verdict could no longer affect the merge decision.') + .write(); + 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.`); network: allowed: