[GH-ISSUE #251] cursor-review: label-scoped concurrency lets a veto race an in-flight panel; workflows_ref duplicates the uses: SHA by hand #5

New issue

Open

opened 2026-10-26 22:34:25 +00:00 by bsakaguchi · 1 comment

bsakaguchi commented

2026-10-26 22:34:25 +00:00

Owner

Originally created by @mattmillerai on GitHub (Aug 31, 2026).
Original GitHub issue: https://github.com/Comfy-Org/github-workflows/issues/251

Two findings from the Cursor panel reviewing a thin caller (comfy-typescript-sdk#111), both properties of the shared pattern rather than of any one caller, so filing here where the fix can land once.

1. The documented caller concurrency group can't cancel across labels

The recommended caller sets:

group: cursor-review-pr-${{ github.event.pull_request.number }}-${{ github.event.label.name }}

The label in the group is load-bearing (this workflow fires on every labeled/unlabeled event, and a shared per-PR group with cancel-in-progress: true would let ANY label add kill an in-flight panel). But it also means the two events that interact — labeled: cursor-review and the veto/unblock label — land in different groups and never cancel each other: applying skip-cursor-review while a panel is mid-flight runs a Gate that vetoes nothing already running, and toggling both within a panel's runtime can land two full reviews (the head-SHA dedupe is check-then-act, evaluated before either posts). Since the Gate owns the label semantics, the clean fix is probably Gate-side: on a veto event, cancel the PR's in-flight review runs via the API rather than relying on group collision.

2. `workflows_ref` is a hand-maintained duplicate of the `uses:` SHA

Every caller pins the reusable workflow by SHA and must copy the same SHA into workflows_ref so prompts/scripts load from the same commit — enforced only by a comment. A bump that updates one and not the other silently runs the workflow definition from one commit and its scripts from another. Options: default workflows_ref to the workflow's own ref where resolvable, or add a startup assertion that the two match and fail loudly.

Raised by: gpt-5.6-sol-max + claude-opus-5-thinking-max + kimi-k3-high panel legs on the caller PR.

🤖 Generated with Claude Code

Originally created by @mattmillerai on GitHub (Aug 31, 2026). Original GitHub issue: https://github.com/Comfy-Org/github-workflows/issues/251 Two findings from the Cursor panel reviewing a thin caller (comfy-typescript-sdk#111), both properties of the shared pattern rather than of any one caller, so filing here where the fix can land once. ## 1. The documented caller concurrency group can't cancel across labels The recommended caller sets: ```yaml group: cursor-review-pr-${{ github.event.pull_request.number }}-${{ github.event.label.name }} ``` The label in the group is load-bearing (this workflow fires on every `labeled`/`unlabeled` event, and a shared per-PR group with `cancel-in-progress: true` would let ANY label add kill an in-flight panel). But it also means the two events that interact — `labeled: cursor-review` and the veto/unblock label — land in different groups and never cancel each other: applying `skip-cursor-review` while a panel is mid-flight runs a Gate that vetoes nothing already running, and toggling both within a panel's runtime can land two full reviews (the head-SHA dedupe is check-then-act, evaluated before either posts). Since the Gate owns the label semantics, the clean fix is probably Gate-side: on a veto event, cancel the PR's in-flight review runs via the API rather than relying on group collision. ## 2. `workflows_ref` is a hand-maintained duplicate of the `uses:` SHA Every caller pins the reusable workflow by SHA and must copy the same SHA into `workflows_ref` so prompts/scripts load from the same commit — enforced only by a comment. A bump that updates one and not the other silently runs the workflow definition from one commit and its scripts from another. Options: default `workflows_ref` to the workflow's own ref where resolvable, or add a startup assertion that the two match and fail loudly. Raised by: gpt-5.6-sol-max + claude-opus-5-thinking-max + kimi-k3-high panel legs on the caller PR. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

bsakaguchi commented

2026-10-26 22:34:27 +00:00

Author

Owner

@mattmillerai commented on GitHub (Sep 18, 2026):

Triage: both findings still valid and unmitigated. Leaving this open, tracked internally as two separate work items — they share no fix, so splitting them keeps each reviewable.

Verified against origin/main @ cf7b8d9c2614ad9f8ee41acf245381ad76fcdc8b.

Finding 1 — the veto race: confirmed live, and deployed in this repo

The label-scoped group is still documented at docs/callers/cursor-review.md:72 and .github/cursor-review/README.md:157, and — worth noting — this repo's own caller uses it: ci-cursor-review.yml:28. So this is not hypothetical-in-docs.
The Gate does not cancel anything. The veto path at cursor-review.yml:371-376 sets should_run=false and exits; that is the entire response. The unblock path at :418-426 starts a second panel with no in-flight check. A repo-wide search for gh run cancel or an actions/runs/<id>/cancel call returns zero hits.
The reusable declares no top-level concurrency: of its own, so the caller's group is the only mechanism.
The dedupe is still check-then-act and still fails open (:431-484, with :450-451 stating the fail-open intent). The codebase corroborates your reading twice: post-review.py:928-932 ("two runs can both pass it before either posts") and cursor-review.yml:1113-1119.

One detail worth adding to your writeup: cursor-review.yml:410-411 carries the stale assumption in a comment — "Removing it cancels any in-progress run via the concurrency group and then no-ops here." That is true for the trigger label (add and remove share label.name, hence the same group) and false for skip-cursor-review. The comment being correct for one label is precisely what makes the gap invisible for the other.

Note the header example inside the reusable (:48-53) does use a PR-number-only group — but that is the widened-types: caller shape, and its stated reason is that label.name is empty on non-label events. Different race, not a fix for this one.

Finding 2 — the `workflows_ref` duplicate: real, but the framing can be upgraded

Your two options are no longer "options" — both are already built in this repo, just not on the workflows that need them:

Option B (startup assertion) is shipped in public-repo-hygiene.yml:142-209, which reads job.workflow_sha, requires non-empty and 40-hex, and hard-fails on inequality at :209 with "The uses: pin and workflows_ref MUST name the same commit". Landed in #206.
Option A (default to the workflow's own ref) is shipped in groom.yml via default: '' plus inputs.workflows_ref || job.workflow_sha.

And the repo has already reached your conclusion and written down why the rollout is parked — pr-risk.yml:593-597: "Asserting equality against it is deliberately NOT done here: it would fail red on every caller whose uses: and workflows_ref have drifted, so it is a caller-contract change tracked separately."

Two things that do not address this, in case they look like they might:

The docs cross-check that landed in #292 compares a caller guide's table row against the declaration, inside this repo. It never parses a uses: line and never compares two pin values.
The shared bumper does rewrite both pins in one pass and asserts afterwards that both moved (bump-callers.sh:834-840 and :909-969, failing the repo rather than half-bumping it) — but that runs only when a fleet processes that caller. Hand-drift between bumps runs drifted until the next bump silently heals it.

What cursor-review has today is weaker than you might expect: eight copies of a Require a pinned workflows_ref step that hard-fail on empty but only ::warning:: on a non-40-hex value (:705-712), with no comparison to job.workflow_sha at all.

So this is tracked as a rollout blocked on one unmeasured number — how many enrolled callers are actually drifted today — which is what decides whether turning the guard on is a one-line addition per workflow or a migration.

Also spotted while verifying: AGENTS.md:83-84 says "A groom, pr-risk or pr-derisk caller pins TWICE", which is under-inclusive — cursor-review, agents-md-integrity, public-repo-hygiene, linear-ticket and pr-size callers all pin twice too.

Both items stay open here until the fixes land. Reopen or comment if either verdict misses something.

Created by Claude Code

@mattmillerai commented on GitHub (Sep 18, 2026): **Triage: both findings still valid and unmitigated. Leaving this open, tracked internally as two separate work items** — they share no fix, so splitting them keeps each reviewable. Verified against `origin/main` @ `cf7b8d9c2614ad9f8ee41acf245381ad76fcdc8b`. ### Finding 1 — the veto race: confirmed live, and deployed in this repo - The label-scoped group is still documented at `docs/callers/cursor-review.md:72` and `.github/cursor-review/README.md:157`, and — worth noting — **this repo's own caller uses it**: `ci-cursor-review.yml:28`. So this is not hypothetical-in-docs. - The Gate does not cancel anything. The veto path at `cursor-review.yml:371-376` sets `should_run=false` and exits; that is the entire response. The unblock path at `:418-426` starts a *second* panel with no in-flight check. A repo-wide search for `gh run cancel` or an `actions/runs/<id>/cancel` call returns **zero hits**. - The reusable declares no top-level `concurrency:` of its own, so the caller's group is the only mechanism. - The dedupe is still check-then-act and still fails open (`:431-484`, with `:450-451` stating the fail-open intent). The codebase corroborates your reading twice: `post-review.py:928-932` ("two runs can both pass it before either posts") and `cursor-review.yml:1113-1119`. One detail worth adding to your writeup: `cursor-review.yml:410-411` carries the stale assumption in a comment — *"Removing it cancels any in-progress run via the concurrency group and then no-ops here."* That is **true for the trigger label** (add and remove share `label.name`, hence the same group) and **false for `skip-cursor-review`**. The comment being correct for one label is precisely what makes the gap invisible for the other. Note the header example inside the reusable (`:48-53`) does use a PR-number-only group — but that is the widened-`types:` caller shape, and its stated reason is that `label.name` is empty on non-label events. Different race, not a fix for this one. ### Finding 2 — the `workflows_ref` duplicate: real, but the framing can be upgraded Your two options are no longer "options" — **both are already built in this repo**, just not on the workflows that need them: - **Option B (startup assertion) is shipped** in `public-repo-hygiene.yml:142-209`, which reads `job.workflow_sha`, requires non-empty and 40-hex, and hard-fails on inequality at `:209` with "The uses: pin and workflows_ref MUST name the same commit". Landed in #206. - **Option A (default to the workflow's own ref)** is shipped in `groom.yml` via `default: ''` plus `inputs.workflows_ref || job.workflow_sha`. And the repo has already reached your conclusion and written down why the rollout is parked — `pr-risk.yml:593-597`: *"Asserting equality against it is deliberately NOT done here: it would fail red on every caller whose `uses:` and `workflows_ref` have drifted, so it is a caller-contract change tracked separately."* Two things that do **not** address this, in case they look like they might: 1. The docs cross-check that landed in #292 compares a caller *guide's* table row against the declaration, inside this repo. It never parses a `uses:` line and never compares two pin values. 2. The shared bumper does rewrite both pins in one pass and asserts afterwards that both moved (`bump-callers.sh:834-840` and `:909-969`, failing the repo rather than half-bumping it) — but that runs only when a fleet processes that caller. Hand-drift between bumps runs drifted until the next bump silently heals it. What cursor-review has today is weaker than you might expect: eight copies of a `Require a pinned workflows_ref` step that hard-fail on empty but only `::warning::` on a non-40-hex value (`:705-712`), with no comparison to `job.workflow_sha` at all. So this is tracked as a rollout blocked on one unmeasured number — how many enrolled callers are actually drifted today — which is what decides whether turning the guard on is a one-line addition per workflow or a migration. Also spotted while verifying: `AGENTS.md:83-84` says "A groom, pr-risk or pr-derisk caller pins TWICE", which is under-inclusive — cursor-review, agents-md-integrity, public-repo-hygiene, linear-ticket and pr-size callers all pin twice too. Both items stay open here until the fixes land. Reopen or comment if either verdict misses something. *Created by Claude Code*

bsakaguchi referenced this issue

2026-10-26 22:35:25 +00:00

[PR #5] [MERGED] feat(cursor-review): add reusable auto-label workflow #13