[PR #303] [MERGED] fix(cursor-review): own a per-PR concurrency group so skip-cursor-review cancels an in-flight panel #304

New issue

Closed

opened 2026-10-26 22:38:15 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:38:15 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/303
Author: @mattmillerai
Created: 9/18/2026
Status: ✅ Merged
Merged: 9/18/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-15263-cursor-review-concurrency

📝 Commits (2)

6220edc fix(cursor-review): own a per-PR concurrency group so skip-cursor-review cancels an in-flight panel
4f09aab fix(cursor-review): guard the trigger slot against an empty review_label, stop paying for a cancelled run, and correct the caller-group docs

📊 Changes

4 files changed (+450 additions, -14 deletions)

View changed files

📝 .github/cursor-review/README.md (+16 -1)
➕ .github/cursor-review/tests/test_workflow_concurrency.py (+286 -0)
📝 .github/workflows/cursor-review.yml (+103 -7)
📝 docs/callers/cursor-review.md (+45 -6)

📄 Description

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

.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-`

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.
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.
.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 #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".

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/github-workflows/pull/303 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/18/2026 **Status:** ✅ Merged **Merged:** 9/18/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-15263-cursor-review-concurrency` --- ### 📝 Commits (2) - [`6220edc`](https://github.com/Comfy-Org/github-workflows/commit/6220edcd29beb59db63d7effaa7d6e6db273bffa) fix(cursor-review): own a per-PR concurrency group so skip-cursor-review cancels an in-flight panel - [`4f09aab`](https://github.com/Comfy-Org/github-workflows/commit/4f09aabbb5f63464536053c5cb826026f800cf5c) fix(cursor-review): guard the trigger slot against an empty review_label, stop paying for a cancelled run, and correct the caller-group docs ### 📊 Changes **4 files changed** (+450 additions, -14 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-review/README.md` (+16 -1) ➕ `.github/cursor-review/tests/test_workflow_concurrency.py` (+286 -0) 📝 `.github/workflows/cursor-review.yml` (+103 -7) 📝 `docs/callers/cursor-review.md` (+45 -6) </details> ### 📄 Description ## 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-**in**sensitive 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 #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". --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:38:15 +00:00