fix(cursor-review): own a per-PR concurrency group so skip-cursor-review cancels an in-flight panel

[mattmillerai]

ELI-5

Applying skip-cursor-review to a PR while a review panel was already running did nothing to that panel. It kept going and posted its review anyway. This gives the reusable workflow its own "one run per PR" group, and deliberately puts the trigger label and the veto label in the same slot — so applying the veto now cancels the panel that is in flight. Every other label gets its own slot, so adding an unrelated label still never kills a running review.

What was wrong

The documented caller concurrency group is cursor-review-pr-<n>-${{ github.event.label.name }}. labeled: skip-cursor-review and labeled: cursor-review therefore resolve to different groups and cannot cancel each other. The veto started a run that no-opped in the Gate while the panel it was meant to stop ran to completion and posted. The Gate's own comment claimed the opposite: "Removing it cancels any in-progress run via the concurrency group and then no-ops here."

What changed

1. .github/workflows/cursor-review.yml — a workflow-level concurrency: block, above the existing workflow-level env:. Same shape pr-size.yml and pr-area-label.yml already use here, so it reaches every caller at its next pin bump with no caller edit and no permission change.

   Slot rule, as rendered by the group expression:

   Event	Slot
   labeled / unlabeled of the trigger label	trigger
   labeled / unlabeled of skip-cursor-review	trigger
   opened / reopened / ready_for_review / synchronize, only under run_without_label: true	trigger
   any other label	label-<name>
   everything else, incl. pull_request_review_thread	label-

2. Gate comments corrected — the stale "removing it cancels any in-progress run" claim now states which events share the trigger slot, and that a caller pinned below this commit still relies on its own label-scoped group, which covers the trigger label alone. The unblock comment notes the slot has already cancelled anything in flight and that the dup-check still covers an unblock after a completed review at this SHA. The veto and unblock logic is unchanged.

3. Docs, four sites — docs/callers/cursor-review.md (a new "The reusable owns a group of its own" section, a new "Veto mid-flight" gotcha for older pins, a new blocking-gate gotcha, and the blocking-gate line amended to "stops new panels from running and cancels a running one"), .github/cursor-review/README.md (the caller example's group line and the "Escape hatches" skip bullet), and the caller pattern in the cursor-review.yml header. Every caller example says KEEP THIS: the reusable's group refines a caller group, it does not replace one (see "Review round 1" below). All of them carry both hard rules pr-size carries — never name a caller group cursor-review-reusable-* (that deadlocks the caller's own run), and call the reusable from a dedicated workflow file, because cancellation is run-scoped.

4. .github/cursor-review/tests/test_workflow_concurrency.py — pins the workflow-level concurrency key, cancel-in-progress: true, the cursor-review-reusable- prefix, the per-PR key, both labels inside the same && 'trigger' disjunction, the format('label-{0}' namespacing, and a deadlock guard asserting no other file under .github/workflows/ mentions the prefix.

Review round 1 (commit 2)

The panel raised 10 findings; 8 are addressed in code, 2 in docs + a follow-up.

• Empty review_label dragged every non-label event into trigger. The input is required: false, so a caller can pass '', and GitHub coerces a missing github.event.label and '' alike in a mixed comparison — so pull_request_review_thread: resolved compared EQUAL and joined trigger, cancelling a running panel. Exactly the case the block's own comment called impossible. Guarded with inputs.review_label != '', with its own test.
• The docs' "your caller group is optional / redundant but harmless" was wrong, and wrong for the caller shapes this very guide recommends. Under the default run_without_label: false the reusable's group does not supersede on push, so a widened or blocking caller that drops its PR-number-only group posts reviews against superseded head SHAs — and under blocking: true gates red on threads for code that no longer exists. In the other direction, a PR-number-only caller group is coarser than the reusable's: it cancels the whole caller run on any label or thread event, before the per-slot isolation can refine anything. Both sides are now stated, and every example says KEEP THIS.
• The second pr-size hard rule was dropped. Cancellation is run-scoped, so the reusable's group takes down the whole caller run — and unlike the deadlock rule, this one arrives silently at a pin bump with no caller edit to warn anyone. Now shipped in all three doc sites.
• always() on consolidate and notify-complete → !cancelled(). Cancelling is now an advertised escape hatch, and on a cancelled run always() spent up to 30 minutes on a judge whose payload nothing could post, then DMed the triggerer "Cursor review failed to post" for a deliberate veto. Cell-level failures are unaffected — cancelled() asks whether the RUN was cancelled, so a red or cancelled matrix cell still reaches the judge and still notifies.
• blocking-gate deliberately keeps always(). Three reviewers proposed the opposite. Its comment argues the case at length: a cancelled run that skipped the gate would mint a green required check, which is the fail-open BE-4691 added the job to close. Not changed.
• The central test assertion was vacuous against its own stated regressions. It did substring containment over the whole disjunction, so demoting the veto comparison into the nested run_without_label arm — or swapping its || for && — kept it green. It now splits on the TOP-LEVEL || and requires the veto to be its own unconditional operand. Mutation-checked: all four mutations fail it (see Verification).
• Gate comments no longer claim cancellation is ordered or total. It is requested, asynchronous and slot-scoped; a label-<name> occupant is not cancelled at all.
• Deadlock guard globs *.yaml too.

Two findings are documented rather than fixed, both stated plainly in the docs:

• Case. Expression == is case-insensitive while the gate's own test is a case-sensitive shell =, so a review_label that differs from the real label only in case reaches trigger, cancels the panel and then no-ops. GitHub expressions have no case-sensitive string compare and no lower(), so this is not fixable in the expression — it is a caller-config requirement, and now a documented one. Reachable only via that misconfiguration.
• The mid-panel veto makes the Blocking gate verdict racy. The cancelled run trips the "review did not land" guard and reports red; the veto run has should_run=false, falls through to the live thread query and reports green — same head SHA, nothing orders them. Which verdict a vetoed PR should get is a policy question about a deliberately fail-closed job, not a concurrency bug, so it is written up in the blocking-gate gotchas and proposed as a follow-up rather than decided here.

Judgment calls

• The run_without_label disjunct is an explicit action allowlist, not an empty-label.name test. The plan sketched inputs.run_without_label && github.event.label.name == '' && github.event_name == 'pull_request'. That also admits edited, closed, assigned, converted_to_draft and any other pull_request action a caller widens its types: to — each of which would then cancel a running panel and immediately no-op in the Gate. The allowlist mirrors the Gate's own case list exactly (opened|reopened|ready_for_review|synchronize), so the concurrency slot and the "should this run" decision cannot drift apart. It also sidesteps GitHub's loose null == '' coercion on a missing github.event.label.
• The new test is parsed without PyYAML. The plan said to reuse the sibling suite's "YAML load", but test_workflow_job_isolation.py deliberately parses with regex — this repo is stdlib-only and test-cursor-review-scripts.yml installs no requirements, so a yaml import simply would not run in CI. It reuses that suite's WORKFLOW path and comment-stripping discipline instead.
• The trigger disjunction is located with a balanced-paren walk, not a regex. A fixed-depth regex stops matching the moment the condition nests one level deeper and reports "the slot rule is gone" when it is merely nested.
• This repo's own ci-cursor-review.yml caller is left alone. It pins by SHA, and moving that pin is bump-cursor-review-callers.yml's job, not this PR's. Its group (cursor-review-pr-…) does not collide with the new one.

Verification

• python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py' — 599 passed, 0 failed (6 new).
• python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py' — 341 passed, 0 failed.
• python3 .github/workflow-pins/check_workflow_pins.py — OK, 11 workflows declare workflows_ref, none with a default, every ref checkout guarded.
• python3 .github/agents-md-integrity/check_agents_md.py --root . — passed (2 pre-existing warnings, unchanged by this PR).
• actionlint .github/workflows/cursor-review.yml (v1.7.7) — the same two findings as on main, byte-identical, none in the new block: the pre-existing SC2153 HEAD_REF false positive and the known job.workflow_sha property (the BE-4169/BE-8077 carve-out AGENTS.md documents). Baseline captured by stashing this diff and re-running.
• The inputs-in-concurrency question the plan flagged is answered empirically: injecting ${{ secrets.NOPE }} into the group made actionlint print "context secrets is not allowed here. available contexts are github, inputs, vars" against this exact key — so the context list is validated, inputs is on it, and the check is non-vacuous. Reverted immediately.
• The new test was mutation-checked, six ways, each reverted: move the veto label into its own slot → fails; cancel-in-progress: false → fails; drop the format('label-{0}' namespacing → fails; rename the group to cursor-review-pr- → fails; give ci-cursor-review.yml the cursor-review-reusable- prefix → the deadlock guard fails; delete the whole concurrency: block → all six fail.

Residual

The live end-to-end verification named in the plan was not run, and cannot be run from this PR. The plan asks for: apply cursor-review, wait for panel cells to start, apply skip-cursor-review, confirm the run shows cancelled and no review is posted; remove skip-cursor-review and confirm exactly one fresh panel; and apply an unrelated label mid-panel and confirm nothing is cancelled. This repo self-enrols via ci-cursor-review.yml, which pins cursor-review.yml by merged-main SHA precisely so a PR cannot change the reviewer judging it — so the panel that runs on this PR is the old one, and the new block is not exercised by it. Repointing that caller at this branch would defeat the tamper-proofing AGENTS.md requires, so it was not done. Someone should run that sequence on a throwaway PR once this is merged and the pin has moved, and in particular confirm the two things static analysis cannot: that a called workflow's workflow-level concurrency cancels the panel matrix jobs (not merely the caller's uses: job), and that the cancelled run posts no review. The pr-size.yml / pr-area-label.yml precedent in this repo is the evidence this shape works; it is precedent, not a run of this expression.

The 4108-run / 29-caller history behind the plan was not re-measured. Those callers are private and their roster lives in the CURSOR_REVIEW_CALLERS secret, which is write-only and unreadable from here, so the "no incident found" premise is carried forward from the spike rather than re-verified. Nothing in this PR depends on it beyond priority.

The portion not fixed, swept here: 16 reusable workflows in this repo declare on: workflow_call; 6 now own a workflow-level concurrency: group (cursor-review, pr-size, pr-area-label, groom, linear-ticket, and — pre-existing — none other), leaving 10 that do not. That is not a latent copy of this bug: none of the 10 pairs a minutes-long job with a mid-flight veto label, which is the combination that makes a split group observable. Checked by shape (a skip-/bypass grep over each), not by run history.

Explicitly out of scope, per the plan: the thread-resolve-cancels-panel wart for widened-types: callers, the over-cap comment race, and the workflows_ref guard. Draft PR #58 was not rebased on or merged into this; if it is ever revived it must adopt the slot expression added here rather than routing skip-cursor-review to a slot of its own.

Provenance

• Authored by: agent-work loop
• Verified: at commit 2 — python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py': 600 passed, 0 failed (7 in the new concurrency suite); python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py': 341 passed, 0 failed; check_workflow_pins.py: OK (11 workflows declare workflows_ref, none with a default, every ref checkout guarded); check_agents_md.py --root .: passed (2 pre-existing warnings, unchanged); cursor-review.yml re-parsed as YAML and the group expression paren-balanced after the edit. The hardened test was mutation-checked four ways, each reverted: veto demoted into the nested run_without_label arm → fails; || between the two label comparisons swapped for && → fails; the inputs.review_label != '' guard removed → fails; veto given its own slot (the PR Add native cursor-review XL mode #58 shape) → fails. Commit 1's actionlint v1.7.7 baseline (2 findings, byte-identical to main, none in the new block) was not re-run — no actionlint binary on this host — so the guard against a new lint finding in the edited expression rests on the YAML parse and the paren check.
• Deviations: the run_without_label disjunct uses an explicit action allowlist rather than the plan's empty-label.name test, and the new test parses the workflow with the sibling suite's stdlib regex approach rather than PyYAML (this suite's CI installs no requirements) — both under "Judgment calls" above. The plan's live end-to-end verification step is unmet and is written up under "Residual". Commit 2 departs from the panel's recommendation on one finding — blocking-gate keeps always() rather than moving to !cancelled() — for the reason its own comment gives; see "Review round 1".

[coderabbitai]

Warning

Review paused — included plan limit reached

Keep your review moving with free on-demand reviews.

• Run this review for free

On-demand reviews are free for the next 22 days.

• Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Promotion and pricing details

On-demand reviews are free for the next 22 days. After that, they cost $0.25 per reviewed file.

Review limit details

Or wait 16 minutes for your next included review.

Check out review usage here.

Limit details: You’ve used the included review currently available. Your 131 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Team

Run ID: 3b0eb98e-cc23-40f6-95b3-bf03211aef44

📥 Commits

Reviewing files that changed from the base of the PR and between cf7b8d9 and 4f09aab.

📒 Files selected for processing (4)

• .github/cursor-review/README.md
• .github/cursor-review/tests/test_workflow_concurrency.py
• .github/workflows/cursor-review.yml
• docs/callers/cursor-review.md

---

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

Found 10 finding(s).

Severity	Count
🟠 High	2
🟡 Medium	4
🟢 Low	4

Panel: 6/6 reviewers contributed findings.

[mattmillerai]

🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:

• BE-15295 — Decide and implement the Blocking gate's verdict for a mid-panel cursor-review veto — filed as agent-spike (premise unverified)

The following carry agent-spike instead of agent-ok because their reachability claim was not backed by evidence (BE-5378) — the claim is investigated before any code is written, and "the premise does not hold" is a valid, successful outcome:

• Decide and implement the Blocking gate's verdict for a mid-panel cursor-review veto — no reachability block in the proposal