Skip to content

Stop no-op auto-fire reviews from cancelling an in-flight review - #22182

Merged
CamSoper merged 1 commit into
masterfrom
fix/review-supersede-race
Oct 8, 2026
Merged

CamSoper merged 1 commit into
masterfrom
fix/review-supersede-race

Conversation

@CamSoper

@CamSoper CamSoper commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

Proposed changes

Fixes a race in claude-code-review.yml where a push plus a @claude #new-review lands a PR on review:error with no review.

How it happened (#21172 / #21210, 2026-10-08):

  1. A push fires claude-triage.yml, which runs on synchronize, and it chains a workflow_run into claude-review.
  2. The #new-review dispatch starts first and claims the claude-review-<PR> concurrency group (Add blog post: AI Agent Guardrails for Infrastructure #21172: 01:08:11).
  3. Triage finishes and the chained run queues in the same group. When it gets a runner it cancels the in-flight dispatch through cancel-in-progress (01:09:37). That chained run was always going to skip: it's the workflow_run path on a PR that already has a review-state label.
  4. The cancel lands in a pre-step (Layer B claim extraction), so the Opus step's outcome is skipped rather than cancelled. The classify step only treats cancelled as superseded, so superseded=false. The error trap then sets review:error.
  5. The chained run's pr-context reads review:error and skips with "already-reviewed" (01:10:27). There's no review, and the label says error.

The other five PRs in that batch got lucky on ordering: their triage-chained run queued first and the dispatch cancelled it.

Fix (two layers):

  • autofire-gate job. A cheap job that only runs on the workflow_run path. It reads draft status and the review-state labels, which are the same signals pr-context's workflow_run skip uses. When the run is certain to skip, claude-review moves into a per-run concurrency group (claude-review-<PR>-noop-<run_id>), so it can't cancel anything. claude-review still runs and publishes its skipped check-run as before. The gate fails open: a lookup error puts the run in the shared group, same as today. claude-review gets needs: autofire-gate with a !cancelled() wrapper on its if, so the gate being skipped on workflow_dispatch doesn't skip the review.
  • Classify review outcome. If the Opus step outcome is skipped but job.status is cancelled, the run counts as superseded (or as timed out if past the job budget). The error trap then leaves the labels to the newer run. On its own this would just leave review:in-progress behind instead of review:error, which is why the first layer is the real fix.

Validation:

  • make test-review-pipeline: passes.
  • test_workflow_sanity.py: 137 passed.
  • actionlint 1.7.7: clean, before and after.
  • The jq filter was checked against label/draft fixtures.

Two things to watch after merge:

  • job.status reading cancelled inside an always() step after a concurrency cancel is documented GitHub behavior, but I couldn't exercise it here.
  • needs in a job-level concurrency expression is an allowed context, but this is its first use in the repo.

A cheap live check is to push and immediately comment #new-review on a reviewed PR, then confirm the triage-chained run logs autofire-gate: … noop=true and the dispatch survives.

Related issues (optional)

Seen on #21172 and #21210 after the v3-scripts merge batch. Same family as the guard comment in the error trap (a superseded run's review:error blocking its replacement).

🤖 Generated with Claude Code

https://claude.ai/code/session_015EbMbem5iFw1cErRVNJXzH


Generated by Claude Code

Triage runs on every push, so every push chains a workflow_run into
claude-review. On an already-reviewed PR that run skips, but it claimed
the per-PR concurrency group first and cancel-in-progress killed the
review in flight -- e.g. a #new-review dispatch posted right after the
push. The cancel landed in a pre-step, so the outcome read "skipped",
the supersession guard missed it, the error trap set review:error, and
the no-op run then skipped on that label (#21172, #21210).

- autofire-gate: a cheap job that reads draft + review-state labels and,
  when the triage-chained run is certain to skip, moves claude-review
  into a per-run concurrency group so it can't cancel anything. Fails
  open.
- Classify review outcome: treat a cancelled job whose Opus step never
  ran as superseded (via job.status), so the error trap leaves labels to
  the newer run.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Claude-Session: https://claude.ai/code/session_015EbMbem5iFw1cErRVNJXzH
@github-actions

github-actions Bot commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Sentinel — merge gate status (preview)

Warning

⚠️ Preview mode — this check is NOT blocking your merge

The Sentinel is in preview (report-only) mode: its check-run concludes neutral whatever the gates below say, so nothing here gates this PR. The rows are informational and safe to ignore for now. It would have concluded failure.

This will be enforced in the near future. Once it is, every row below has to be green before this PR can merge. The rows are the sign-offs a merge needs — some yours, some a reviewer's or a deploy's. Tell us in #docs if one looks wrong.

1 of 5 gates would block this merge once the Sentinel is enforced.

Gate What it needs
✅ G1 review-ran Nothing to do: review current at 1df0012c1.
✅ G2 findings-answered Nothing to do: every finding answered.
🔴 G3 right-approver Needs an approval from anyone with write access to this repo — no qualifying human approval at this head (bot approvals count only on the automated lanes bot_approvers lists).
➖ G4 infra-evidence Doesn't apply to this PR: no changed path affects the deploy.
➖ G5 oversized-ack Doesn't apply to this PR: not oversized.

Break glass: a member of a routing team (.github/review-routing.yml teams:) can apply review:waived, which is logged.

Evaluated at head 1df0012c1.

@pulumi-bot

Copy link
Copy Markdown
Collaborator

@CamSoper
CamSoper marked this pull request as ready for review October 8, 2026 14:54
@github-actions github-actions Bot added review:triaging Claude Triage is currently classifying the PR domain:infra PR touches workflows, scripts, infra, Makefile, or build config labels Oct 8, 2026
@pulumi-bot
pulumi-bot requested a review from a team October 8, 2026 14:58
@github-actions github-actions Bot added review:in-progress Claude review is currently running and removed review:triaging Claude Triage is currently classifying the PR labels Oct 8, 2026
@github-actions

github-actions Bot commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

Reviewer's guide v1 — not for the author

Tip

Check the ⚠️ items, then approve. Your approval covers only those; links, shortcodes, metadata, and verified claims were checked by machine (receipts on the evidence page). Code samples are read, not compiled.

PR author: nothing here is yours. Your to-do list is the "Author action guide" comment.

Review requested from: @pulumi/docs-tools — an approval from anyone with write access satisfies the merge gate.

Note

What this PR changes:

  • New autofire-gate job: on workflow_run, reads the PR's draft state and review-state labels and outputs noop.
  • claude-review now needs: autofire-gate, adds a !cancelled() guard, and moves a noop run into its own per-run concurrency group.
  • Outcome step: a cancel that lands in a pre-step (Opus step skipped, job.status cancelled) now counts as superseded or timed out instead of review:error.

The changes would be wrong if the gate drifted from pr-context's skip guard, or if the needs wiring skipped claude-review on workflow_dispatch. The review compared the gate's label list with the already-reviewed guard (they match, and the gate fails open) and traced the !cancelled() condition through the dispatch path.

Review confidence:

Dimension Level Notes
mechanics HIGH
facts HIGH

⚠️ Check these before approving

Nothing needs a human eye beyond the rubber-stamp list below.

✅ What you can rubber-stamp

  • Facts: no factual claims found in the changed lines.
  • Mechanics: no internal links added.
  • Style: 0 advisory suggestions left with the author; never blocking.

Pre-existing issues in touched files: 0 — details on the evidence page.

Full evidence: verification trail, investigation log, review history.

Review v1 · updated 2026-10-08T14:59:23Z · head commit 1df0012


For the reviewer: something look wrong? Comment @claude <your point> #update-review and the review re-checks with your input.

@github-actions

github-actions Bot commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

Author action guide v1 — nothing blocks merge

Note

Nothing here needs an answer from you. A human reviewer still approves the merge.

This PR adds an autofire-gate job so an auto-fire run that is certain to skip can't cancel an in-flight review, and treats a pre-step cancel as superseded rather than an error; the review checked the gate's skip signals against pr-context's already-reviewed guard and the needs/if wiring on workflow_dispatch.

Full evidence: verification trail, investigation log, review history.

Review v1 · updated 2026-10-08T14:59:23Z · head commit 1df0012


Please don't edit or delete this comment; it's the review's record.

@github-actions github-actions Bot added review:no-blockers Claude review completed cleanly; outstanding is empty and removed review:in-progress Claude review is currently running labels Oct 8, 2026
@CamSoper
CamSoper merged commit 6aacded into master Oct 8, 2026
17 checks passed
@CamSoper
CamSoper deleted the fix/review-supersede-race branch October 8, 2026 16:14
CamSoper added a commit that referenced this pull request Oct 8, 2026
#22182 put an autofire-gate job in front of claude-review. That job is
skipped on workflow_dispatch, and a skipped ancestor fails the implicit
success() check on every job below it, so publish (and redispatch) were
skipped on every dispatched run: #new-review, the CLI dispatcher, and the
freshness redispatch all reviewed the PR and published nothing, leaving
review:in-progress on the PR. claude-review already used !cancelled() for
this; publish and redispatch now do too, with an explicit result check so
they still only run after a successful upstream job. A test now asserts
every job downstream of autofire-gate carries a status function.

Co-authored-by: Claude Opus 5.5 (1M context) <[email protected]>

This branch was successfully deployed

1 active deployment
testing — 1df0012c Deployed Oct 8, 2026 by CamSoper via Do cleanup #14672
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

domain:infra PR touches workflows, scripts, infra, Makefile, or build config review:no-blockers Claude review completed cleanly; outstanding is empty

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants