From 6220edcd29beb59db63d7effaa7d6e6db273bffa Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Fri, 18 Sep 2026 00:31:24 +0000 Subject: [PATCH 1/2] fix(cursor-review): own a per-PR concurrency group so skip-cursor-review cancels an in-flight panel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The documented caller group keys on `github.event.label.name`, which puts `labeled: skip-cursor-review` in a different group from `labeled: cursor-review`. A veto applied mid-panel therefore cancelled nothing: it started a run that no-opped in the Gate while the panel it was meant to stop ran on and posted its review. The Gate's own comment claimed the opposite. Give the reusable its own workflow-level `concurrency:` group, the shape pr-size.yml and pr-area-label.yml already use here, so it reaches every caller through the normal pin bump with no caller edit and no permission change. Slot rule: the trigger label and the `skip-cursor-review` veto label share one `trigger` slot, so applying the veto mid-flight cancels the running panel. Every other label gets its own `format('label-{0}', …)` slot, so an unrelated label add never kills a running review, and `pull_request_review_thread` events stay out of `trigger` so resolving a finding thread cannot cancel a panel. Under `run_without_label: true` the four plain PR actions the Gate accepts join `trigger` as well — otherwise a synchronize-triggered panel would sit in a slot the veto cannot reach, which is the case this block exists for. Gate comments corrected, the three doc sites re-stated (caller groups are now optional belt-and-braces, and a caller must never name its group `cursor-review-reusable-*` or it deadlocks its own run), and a new test_workflow_concurrency.py pins the slot rule, cancel-in-progress, the namespace prefix and the no-other-workflow-shares-the-group deadlock guard. --- .github/cursor-review/README.md | 11 +- .../tests/test_workflow_concurrency.py | 189 ++++++++++++++++++ .github/workflows/cursor-review.yml | 58 +++++- docs/callers/cursor-review.md | 23 ++- 4 files changed, 269 insertions(+), 12 deletions(-) create mode 100644 .github/cursor-review/tests/test_workflow_concurrency.py diff --git a/.github/cursor-review/README.md b/.github/cursor-review/README.md index c57c4e45..68af1568 100644 --- a/.github/cursor-review/README.md +++ b/.github/cursor-review/README.md @@ -153,6 +153,12 @@ permissions: contents: read pull-requests: write concurrency: + # OPTIONAL once pinned past the commit that gave the reusable its own + # workflow-level `cursor-review-reusable--` group — that one already + # collapses per-PR runs and is what lets `skip-cursor-review` cancel a panel + # that is already running. Redundant but harmless to keep; just never name a + # caller group `cursor-review-reusable-*` (same group as the reusable = the + # caller deadlocks its own run). # Re-labeling cancels an in-flight run for the same PR + label. group: cursor-review-pr-${{ github.event.pull_request.number }}-${{ github.event.label.name }} cancel-in-progress: true @@ -295,7 +301,10 @@ and upsert are both `continue-on-error`: the size verdict lives in the ### Escape hatches - **Skip a PR**: add the `skip-cursor-review` label. It wins even if the trigger - label is present. Removing it (while the trigger label is on) starts a run. + label is present, and — on a caller pinned past the reusable's own + `cursor-review-reusable-*` concurrency group — it also **cancels a panel that + is already running**, since the veto label and the trigger label share one + concurrency slot. Removing it (while the trigger label is on) starts a run. - **Re-review after changes**: push commits. The new HEAD SHA bypasses the idempotency check and a re-applied label runs a fresh panel. - **Re-review unchanged content**: dismiss the existing review, then re-apply diff --git a/.github/cursor-review/tests/test_workflow_concurrency.py b/.github/cursor-review/tests/test_workflow_concurrency.py new file mode 100644 index 00000000..e5ba7215 --- /dev/null +++ b/.github/cursor-review/tests/test_workflow_concurrency.py @@ -0,0 +1,189 @@ +#!/usr/bin/env python3 +"""Structural regression tests for cursor-review.yml's own concurrency group. + +The reusable owns a workflow-level `concurrency:` group so that every caller +picks it up at its next pin bump, with no caller edit. Its whole point is the +SLOT rule: the trigger label and the `skip-cursor-review` veto label resolve to +the SAME `trigger` slot, which is what makes applying `skip-cursor-review` +mid-flight cancel the running panel — the Gate's documented +"skip-cursor-review wins" precedence, made true for in-flight runs and not only +for runs that have yet to start. Every other label gets its own +`format('label-{0}', …)` slot so an unrelated label add never kills a running +review. + +None of that is visible in a diff: splitting the veto label into its own slot, +dropping `cancel-in-progress`, or renaming the group to something a caller +would plausibly also pick (which deadlocks that caller's run — it holds the +group while its `uses:` job waits on it) all leave a workflow that parses, +lints and runs. So the shape is pinned here. + +Deliberately parsed WITHOUT PyYAML, for the same reason +`test_workflow_job_isolation.py` is: this repo is stdlib-only and CI installs +no requirements for this suite, so a `yaml` import would simply not run. + +Run: python3 .github/cursor-review/tests/test_workflow_concurrency.py +""" + +import glob +import os +import re +import unittest + +WORKFLOWS_DIR = os.path.normpath( + os.path.join(os.path.dirname(__file__), "..", "..", "workflows") +) +WORKFLOW = os.path.join(WORKFLOWS_DIR, "cursor-review.yml") + +# The group name prefix. Deliberately NOT something a caller would pick: a +# caller declaring the reusable's own group deadlocks its own run. +GROUP_PREFIX = "cursor-review-reusable-" + +COMMENT = re.compile(r"^\s*#") +TOP_LEVEL_KEY = re.compile(r"^([A-Za-z0-9_-]+):") + + +def read_workflow(path=WORKFLOW): + with open(path, encoding="utf-8") as f: + return f.read().split("\n") + + +def top_level_block(lines, key): + """The lines of the top-level `:` mapping, comments dropped. + + Returns None when the key is absent. Comments are dropped so a comment that + merely MENTIONS `cancel-in-progress` or the group name can never satisfy an + assertion below — the same trap `code_lines` guards in the sibling suite. + """ + body, inside = [], False + for line in lines: + if inside: + if line.strip() and not line.startswith(" "): + break # dedented back to another top-level key + if not COMMENT.match(line): + body.append(line) + continue + match = TOP_LEVEL_KEY.match(line) + if match and match.group(1) == key: + inside = True + return body if inside else None + + +def mapping_value(block, key): + """The scalar value of ` :` in a top-level block, or None.""" + prefix = " %s:" % key + for line in block: + if line.startswith(prefix): + return line[len(prefix):].strip() + return None + + +def trigger_disjunction(group): + """The parenthesised condition immediately left of `&& 'trigger'`, or None. + + Balanced-paren walk rather than a regex: the condition nests (the + run_without_label arm carries its own parenthesised action list), and a + fixed-depth regex silently stops matching the moment someone adds another + level — reporting "the slot rule is gone" when it is merely nested deeper. + """ + marker = re.search(r"\)\s*&&\s*'trigger'", group) + if not marker: + return None + close = group.index(")", marker.start()) + depth = 0 + for i in range(close, -1, -1): + if group[i] == ")": + depth += 1 + elif group[i] == "(": + depth -= 1 + if depth == 0: + return group[i + 1:close] + return None + + +class WorkflowConcurrencyTest(unittest.TestCase): + def setUp(self): + self.block = top_level_block(read_workflow(), "concurrency") + self.assertIsNotNone( + self.block, + "cursor-review.yml has no workflow-level `concurrency:` key — " + "without it skip-cursor-review cannot cancel an in-flight panel", + ) + self.group = mapping_value(self.block, "group") + self.assertIsNotNone(self.group, "`concurrency:` declares no `group:`") + + def test_cancel_in_progress_is_true(self): + # A group with cancel-in-progress false QUEUES the veto behind the panel + # it is meant to kill, which is worse than no group at all. + self.assertEqual( + mapping_value(self.block, "cancel-in-progress"), + "true", + "`concurrency.cancel-in-progress` must be true — the veto cancels " + "the in-flight panel, it does not queue behind it", + ) + + def test_group_is_namespaced_away_from_caller_groups(self): + self.assertTrue( + self.group.startswith(GROUP_PREFIX), + "the group must start with %r so no caller picks the same name; " + "got %r" % (GROUP_PREFIX, self.group), + ) + + def test_group_is_keyed_per_pr(self): + # A group that is not per-PR serializes/cancels ACROSS PRs. + self.assertIn("github.event.pull_request.number", self.group) + + def test_trigger_label_and_veto_label_share_one_slot(self): + # The assertion this suite exists for. Both labels must sit in the SAME + # `&& 'trigger'` disjunction — one of them moved into its own slot (the + # shape draft PR #58 used) silently restores the bug. + disjunction = trigger_disjunction(self.group) + self.assertIsNotNone( + disjunction, + "no `(...) && 'trigger'` disjunction in the group expression: %r" + % self.group, + ) + self.assertIn( + "inputs.review_label", + disjunction, + "the trigger label is not in the `trigger` slot's disjunction", + ) + self.assertIn( + "'skip-cursor-review'", + disjunction, + "the skip-cursor-review veto label is not in the SAME disjunction " + "as the trigger label — applying it mid-flight would land in a " + "different slot and cancel nothing", + ) + + def test_every_other_label_gets_its_own_namespaced_slot(self): + # Without the format() namespacing, a label literally named `trigger` + # (or `Trigger` — group names are case-insensitive) would reach the + # shared slot and cancel a running panel. + self.assertIn( + "format('label-{0}'", + self.group, + "the non-trigger branch must namespace the label name via " + "format('label-{0}', …)", + ) + + def test_no_other_workflow_shares_the_reusable_group(self): + # The deadlock guard. Any other workflow here — above all this repo's + # own ci-cursor-review.yml caller — declaring the same group would hold + # it while its `uses:` job waits to acquire it, hanging until timeout. + offenders = [] + for path in sorted(glob.glob(os.path.join(WORKFLOWS_DIR, "*.yml"))): + if os.path.abspath(path) == os.path.abspath(WORKFLOW): + continue + with open(path, encoding="utf-8") as f: + if GROUP_PREFIX in f.read(): + offenders.append(os.path.basename(path)) + self.assertEqual( + offenders, + [], + "these workflows reference %r, which deadlocks a caller against " + "the reusable's own group: %s" % (GROUP_PREFIX, ", ".join(offenders)), + ) + + +if __name__ == "__main__": + unittest.main(verbosity=2) diff --git a/.github/workflows/cursor-review.yml b/.github/workflows/cursor-review.yml index 7efd2645..57e76e00 100644 --- a/.github/workflows/cursor-review.yml +++ b/.github/workflows/cursor-review.yml @@ -46,9 +46,14 @@ name: Cursor Review (reusable) # contents: read # pull-requests: write # concurrency: -# # PR number ONLY. `github.event.label.name` is empty on every widened -# # event, so keying on it splits one PR across groups that cannot cancel -# # each other. +# # OPTIONAL belt-and-braces once you are pinned past the commit that gave +# # this reusable its OWN workflow-level `concurrency:` block — that group +# # already collapses per-PR runs, and it is what makes skip-cursor-review +# # cancel a panel that is already running. Keep or drop this; just never +# # name a caller group `cursor-review-reusable-*` (a caller sharing the +# # reusable's group deadlocks its own run). PR number ONLY: +# # `github.event.label.name` is empty on every widened event, so keying on +# # it splits one PR across groups that cannot cancel each other. # group: cursor-review-pr-${{ github.event.pull_request.number }} # cancel-in-progress: true # jobs: @@ -307,6 +312,38 @@ on: description: PEM private key matching the bot_app_id input. Required only when bot_app_id is set. required: false +concurrency: + # Owned by the REUSABLE so every caller gets it at its next pin bump — the + # same shape pr-size.yml and pr-area-label.yml use here. The `-reusable-` + # infix keeps the name off what a caller would naturally pick: a caller that + # declares the SAME group DEADLOCKS its own run, holding the group while its + # `uses:` job waits to acquire it (see the note in ci-groom.yml and + # docs/callers/pr-size.md). + # + # Slot rule: the trigger label AND the `skip-cursor-review` veto label share + # one `trigger` slot, so applying skip-cursor-review mid-flight CANCELS the + # running panel — the Gate's "skip-cursor-review wins" precedence, made true + # for in-flight runs too — and removing it cancels nothing that matters + # before starting a fresh one. Every OTHER label gets its own namespaced + # `label-` slot, so an unrelated label add never kills a running + # review. `format('label-{0}', …)` is what stops a label literally named + # `trigger` (or `Trigger`; group names are case-insensitive) from reaching + # the shared slot. + # + # Non-label events carry an EMPTY `github.event.label.name`, so they fall to + # `label-` and share one slot among themselves. That is deliberate for + # `pull_request_review_thread` on a blocking caller: resolving a thread must + # never cancel a panel. But a `run_without_label` caller STARTS panels from + # plain PR events, so those four actions — and only those four, mirroring the + # Gate's own case list — join `trigger` as well; otherwise a + # synchronize-triggered panel would sit in a slot the veto label cannot + # reach, which is the case this block exists for. + # + # `inputs` is an allowed context in a called workflow's workflow-level + # `concurrency:`, same as the `env:` block below. + group: cursor-review-reusable-${{ github.event.pull_request.number || github.ref }}-${{ (github.event.label.name == inputs.review_label || github.event.label.name == 'skip-cursor-review' || (inputs.run_without_label && github.event_name == 'pull_request' && (github.event.action == 'opened' || github.event.action == 'reopened' || github.event.action == 'ready_for_review' || github.event.action == 'synchronize'))) && 'trigger' || format('label-{0}', github.event.label.name) }} + cancel-in-progress: true + # DIFF_SIZE_CAP / REVIEW_LABEL / JUDGE_MODEL / DIFF_EXCLUDES are mapped from # `inputs` here so the run steps below read them verbatim from the original # (private-repo) workflow without per-step plumbing. `inputs` is available to @@ -407,8 +444,16 @@ jobs: esac fi - # Only adding the trigger label fires the matrix. Removing it cancels - # any in-progress run via the concurrency group and then no-ops here. + # Only adding the trigger label fires the matrix. Adding or removing + # the trigger label, and ADDING skip-cursor-review, all resolve to the + # reusable's shared `trigger` concurrency slot (see the workflow-level + # `concurrency:` block), so any in-flight run for this PR is already + # cancelled before this step gets to decide — which is what makes the + # skip-cursor-review veto above true for a panel that is still + # running, not just for one that has yet to start. A caller pinned + # BELOW that commit has only its own label-scoped group, which covers + # the trigger label alone: there, applying skip-cursor-review does not + # stop a running panel. if [ "$LABEL_NAME" = "$REVIEW_LABEL" ] && [ "$GH_EVENT_ACTION" = "labeled" ]; then echo "$REVIEW_LABEL label added — running." echo "should_run=true" >> "$GITHUB_OUTPUT" @@ -417,6 +462,9 @@ jobs: # Removing skip-cursor-review while the trigger label is present # unblocks a previously skipped review — treat it as a fresh trigger. + # This event shares the `trigger` concurrency slot too, so anything + # in flight for this PR has already been cancelled; the dup check + # below still covers the case where a review COMPLETED at this SHA. if [ "$LABEL_NAME" = "skip-cursor-review" ] && [ "$GH_EVENT_ACTION" = "unlabeled" ]; then if echo "$PR_LABELS" | jq -e --arg l "$REVIEW_LABEL" 'index($l)' > /dev/null; then echo "skip-cursor-review removed while $REVIEW_LABEL is present — running." diff --git a/docs/callers/cursor-review.md b/docs/callers/cursor-review.md index 6ebf7184..4ee22785 100644 --- a/docs/callers/cursor-review.md +++ b/docs/callers/cursor-review.md @@ -65,8 +65,10 @@ on: types: [labeled, unlabeled] concurrency: - # cursor-review declares no group of its own, so a caller-level group is safe - # and worth having — it stops label-toggling from stacking panels. + # OPTIONAL once you are pinned past the commit that gave the reusable its own + # workflow-level group (see "The reusable owns a group of its own" below) — + # redundant but harmless, and it is what an older pin relies on. Never name a + # caller group `cursor-review-reusable-*`. # NOTE: label.name is part of the key only because this caller is label-only. # Drop it if you widen `types:` — see the run_without_label gotcha. group: cursor-review-pr-${{ github.event.pull_request.number }}-${{ github.event.label.name }} @@ -175,7 +177,9 @@ on: types: [opened, reopened, ready_for_review, synchronize, labeled, unlabeled] concurrency: - # Drop `label.name` from the key — see below. + # Optional once pinned past the reusable's own group — see below. If you keep + # it, drop `label.name` from the key, and never name it + # `cursor-review-reusable-*`. group: cursor-review-pr-${{ github.event.pull_request.number }} cancel-in-progress: true # ... @@ -195,6 +199,10 @@ Keep `labeled`/`unlabeled` in the list even in label-free mode: the label path stays live alongside it, which is how you force a re-review on an unchanged commit (dismiss the existing review, then apply the label — see the dedupe gotcha below). +**The reusable owns a group of its own, so your caller group is optional.** `cursor-review.yml` declares a workflow-level `concurrency: cursor-review-reusable--` with `cancel-in-progress: true`, which reaches you at your next pin bump with no caller edit and no permission change. Its slot rule: the trigger label and `skip-cursor-review` share one `trigger` slot, so **applying `skip-cursor-review` mid-panel cancels the running panel**; every other label gets its own `label-` slot, so an unrelated label add never kills a running review; and `pull_request_review_thread` events stay out of `trigger`, so resolving a finding thread on a blocking caller cannot cancel a panel. Under `run_without_label: true` the four plain PR actions the gate accepts (`opened` / `reopened` / `ready_for_review` / `synchronize`) join `trigger` as well, so a push supersedes a running panel and the veto label can still reach it. Keeping your own caller-level group alongside it is redundant but harmless — it cancels the same-label cases the reusable's `trigger` slot also cancels. **One hard rule, the same one [`pr-size`](pr-size.md) carries: never name a caller group `cursor-review-reusable-*`.** A caller that declares the reusable's own group deadlocks its own run — the caller holds the group while its `uses:` job waits to acquire it. + +**Veto mid-flight: on a pin BELOW that change, `skip-cursor-review` does not stop a running panel.** With only a caller-level group, `labeled: skip-cursor-review` and `labeled: cursor-review` land in *different* groups (the group key carries `label.name`), so the veto starts a run that no-ops in the gate while the panel it was meant to stop keeps going — and still posts its review. Do not try to fix it caller-side by collapsing to a PR-number-only group: that does cancel on the veto, but it also puts *every* label event in one group, so adding an unrelated label kills a running review. Bump your pin past the reusable's own group instead — there is nothing to change in the caller. + **`run_without_label: true` reviews every PR.** On a busy repo that is a large step up in spend. Start label-gated. @@ -216,8 +224,11 @@ on: types: [resolved, unresolved] concurrency: - # PR number only — label.name is empty on the widened events, and split - # groups can't cancel each other (see the run_without_label gotcha). + # OPTIONAL belt-and-braces once pinned past the reusable's own group (see + # "The reusable owns a group of its own"); never name it + # `cursor-review-reusable-*`. PR number only — label.name is empty on the + # widened events, and split groups can't cancel each other (see the + # run_without_label gotcha). group: cursor-review-pr-${{ github.event.pull_request.number }} cancel-in-progress: true ``` @@ -238,7 +249,7 @@ are the same story: they can't run the panel (see above), so they never gate red. **Neither the skip label nor removing the trigger label waives the gate.** -`skip-cursor-review` stops new panels from running; it does not resolve the +`skip-cursor-review` stops new panels from running and cancels a running one; it does not resolve the threads an earlier panel already posted, and neither does taking the trigger label off. Once findings exist, the ways out are resolving each thread, pushing a fix that outdates them, or a ruleset bypass. Dismissing the review does not From 4f09aabbb5f63464536053c5cb826026f800cf5c Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Fri, 18 Sep 2026 01:06:52 +0000 Subject: [PATCH 2/2] fix(cursor-review): guard the trigger slot against an empty review_label, stop paying for a cancelled run, and correct the caller-group docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on the reusable's own concurrency group. Workflow: - Guard the trigger-label disjunct with `inputs.review_label != ''`. The input is `required: false`, so a caller can pass ''. GitHub coerces a missing `github.event.label` and '' alike in a mixed comparison, so without the guard every non-label event — `pull_request_review_thread` included — compared EQUAL and joined `trigger`, making a thread resolve cancel a running panel: the case the block's own comment called impossible. - `consolidate` and `notify-complete` move from `always()` to `!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 what was 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` keeps `always()` — a cancelled run that skipped it would mint a green required check, the fail-open BE-4691 added it to close. - Comments: the gate no longer claims an in-flight run "has already been cancelled" by the time it decides. Cancellation is requested, asynchronous and slot-scoped; a `label-` occupant is not cancelled at all. Records the case gap too — expression `==` is case-insensitive while the gate's own test is a case-sensitive shell `=`, so a case-mismatched `review_label` cancels a panel and then no-ops. Test: - `test_trigger_label_and_veto_label_share_one_slot` asserted substring containment over the whole disjunction, which stayed green when the veto comparison was demoted into the nested `run_without_label` arm or its `||` swapped for `&&` — the regressions it exists to catch. It now splits the disjunction on its TOP-LEVEL `||` and requires the veto to be its own unconditional operand. All four mutations fail it; the empty-label guard gets its own test. - The deadlock guard globs `*.yaml` as well as `*.yml`. Docs (three sites): - "your caller group is optional / redundant but harmless" was wrong for the caller shapes this 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 a PR-number-only caller group is coarser than the reusable's, so it cancels first and leaves the per-slot isolation nothing to refine. Both sides are now stated; every example says KEEP THIS. - Ships the second hard rule `pr-size` carries and this had dropped: call it from a dedicated workflow file, because cancellation is run-scoped — and unlike the deadlock rule, this one arrives silently at a pin bump. - New blocking-gate gotcha for the mid-panel veto: the cancelled run and the veto run both publish `Blocking gate` on the same head SHA, red and green respectively, and nothing orders them. Co-Authored-By: Claude Opus 5 --- .github/cursor-review/README.md | 20 ++- .../tests/test_workflow_concurrency.py | 123 ++++++++++++++++-- .github/workflows/cursor-review.yml | 88 ++++++++++--- docs/callers/cursor-review.md | 46 +++++-- 4 files changed, 228 insertions(+), 49 deletions(-) diff --git a/.github/cursor-review/README.md b/.github/cursor-review/README.md index 68af1568..d30461d2 100644 --- a/.github/cursor-review/README.md +++ b/.github/cursor-review/README.md @@ -153,12 +153,15 @@ permissions: contents: read pull-requests: write concurrency: - # OPTIONAL once pinned past the commit that gave the reusable its own - # workflow-level `cursor-review-reusable--` group — that one already - # collapses per-PR runs and is what lets `skip-cursor-review` cancel a panel - # that is already running. Redundant but harmless to keep; just never name a - # caller group `cursor-review-reusable-*` (same group as the reusable = the - # caller deadlocks its own run). + # KEEP THIS. The reusable owns a `cursor-review-reusable--` group + # of its own — that is what lets `skip-cursor-review` cancel a panel already + # running — but it REFINES this one rather than replacing it: under the + # default `run_without_label: false` it does not cancel on push. Two rules: + # never name a caller group `cursor-review-reusable-*` (same group as the + # reusable = the caller deadlocks its own run), and call the reusable from a + # dedicated workflow file — its cancellation is run-scoped, so a label event + # would take the rest of a shared `ci.yml` down with it. Details in + # docs/callers/cursor-review.md. # Re-labeling cancels an in-flight run for the same PR + label. group: cursor-review-pr-${{ github.event.pull_request.number }}-${{ github.event.label.name }} cancel-in-progress: true @@ -304,7 +307,10 @@ and upsert are both `continue-on-error`: the size verdict lives in the label is present, and — on a caller pinned past the reusable's own `cursor-review-reusable-*` concurrency group — it also **cancels a panel that is already running**, since the veto label and the trigger label share one - concurrency slot. Removing it (while the trigger label is on) starts a run. + concurrency slot. Cancellation is asynchronous, so the vetoed run keeps + unwinding for a moment; under `blocking: true` that leaves the gate's verdict + racy for a beat (see the blocking-gate gotchas in the caller guide). Removing + it (while the trigger label is on) starts a run. - **Re-review after changes**: push commits. The new HEAD SHA bypasses the idempotency check and a re-applied label runs a fresh panel. - **Re-review unchanged content**: dismiss the existing review, then re-apply diff --git a/.github/cursor-review/tests/test_workflow_concurrency.py b/.github/cursor-review/tests/test_workflow_concurrency.py index e5ba7215..0b7622d1 100644 --- a/.github/cursor-review/tests/test_workflow_concurrency.py +++ b/.github/cursor-review/tests/test_workflow_concurrency.py @@ -100,6 +100,52 @@ def trigger_disjunction(group): return None +def top_level_or_operands(disjunction): + """`disjunction` split on its TOP-LEVEL `||`, each operand unwrapped once. + + Substring containment over the whole disjunction is not enough to pin the + slot rule: moving the veto comparison INTO the nested + `(inputs.run_without_label && …)` arm, or swapping the `||` between the two + label comparisons for an `&&`, keeps every substring present while the veto + label stops reaching `trigger` on a label-gated caller. Both mutations + change the top-level operand LIST, so that is what the tests below assert + on. Depth tracking is what makes it a top-level split — the + run_without_label arm carries `||`s of its own, nested one level deeper. + """ + operands, depth, start = [], 0, 0 + i = 0 + while i < len(disjunction): + char = disjunction[i] + if char == "(": + depth += 1 + elif char == ")": + depth -= 1 + elif char == "|" and depth == 0 and disjunction[i:i + 2] == "||": + operands.append(disjunction[start:i]) + i += 2 + start = i + continue + i += 1 + operands.append(disjunction[start:]) + return [unwrap(o) for o in operands] + + +def unwrap(operand): + """`operand` stripped, with ONE redundant enclosing paren pair removed.""" + operand = operand.strip() + if operand.startswith("(") and operand.endswith(")"): + depth = 0 + for i, char in enumerate(operand): + if char == "(": + depth += 1 + elif char == ")": + depth -= 1 + if depth == 0 and i != len(operand) - 1: + return operand # the parens are not a single outer pair + return operand[1:-1].strip() + return operand + + class WorkflowConcurrencyTest(unittest.TestCase): def setUp(self): self.block = top_level_block(read_workflow(), "concurrency") @@ -133,26 +179,72 @@ def test_group_is_keyed_per_pr(self): self.assertIn("github.event.pull_request.number", self.group) def test_trigger_label_and_veto_label_share_one_slot(self): - # The assertion this suite exists for. Both labels must sit in the SAME - # `&& 'trigger'` disjunction — one of them moved into its own slot (the - # shape draft PR #58 used) silently restores the bug. + # The assertion this suite exists for. Both labels must be TOP-LEVEL + # alternatives of the same `&& 'trigger'` disjunction — one of them + # moved into its own slot (the shape draft PR #58 used), or demoted + # into the nested run_without_label arm, or `||`-to-`&&`'d, silently + # restores the bug while leaving both substrings in the expression. disjunction = trigger_disjunction(self.group) self.assertIsNotNone( disjunction, "no `(...) && 'trigger'` disjunction in the group expression: %r" % self.group, ) - self.assertIn( - "inputs.review_label", - disjunction, - "the trigger label is not in the `trigger` slot's disjunction", + operands = top_level_or_operands(disjunction) + + trigger_arms = [o for o in operands if "inputs.review_label" in o] + self.assertEqual( + len(trigger_arms), + 1, + "expected exactly ONE top-level `||` operand comparing against " + "inputs.review_label; got %r from %r" % (operands, disjunction), ) self.assertIn( - "'skip-cursor-review'", - disjunction, - "the skip-cursor-review veto label is not in the SAME disjunction " - "as the trigger label — applying it mid-flight would land in a " - "different slot and cancel nothing", + "github.event.label.name == inputs.review_label", + trigger_arms[0], + "the trigger label's operand does not compare label.name to " + "inputs.review_label: %r" % trigger_arms[0], + ) + + veto_arms = [o for o in operands if "'skip-cursor-review'" in o] + self.assertEqual( + len(veto_arms), + 1, + "the skip-cursor-review veto label must be its OWN top-level `||` " + "operand of the `trigger` disjunction — nested inside another arm " + "(or joined with `&&`) it no longer reaches `trigger` on a " + "label-gated caller, and applying it mid-flight cancels nothing; " + "got %r from %r" % (operands, disjunction), + ) + self.assertEqual( + veto_arms[0], + "github.event.label.name == 'skip-cursor-review'", + "the veto operand must be an unconditional label comparison, not " + "%r — any extra conjunct is a condition under which the veto " + "silently stops cancelling the panel" % veto_arms[0], + ) + self.assertIsNot( + veto_arms[0], + trigger_arms[0], + "the trigger and veto comparisons collapsed into one operand", + ) + + def test_trigger_label_disjunct_is_guarded_against_an_empty_review_label(self): + # `review_label` is `required: false`, so a caller can pass ''. GitHub + # coerces a MISSING `github.event.label` and '' alike in a mixed + # comparison, so an unguarded `label.name == inputs.review_label` is + # TRUE on every non-label event — `pull_request_review_thread` included + # — dragging them into `trigger`, where resolving a thread cancels a + # running panel. + disjunction = trigger_disjunction(self.group) + trigger_arms = [ + o for o in top_level_or_operands(disjunction) + if "inputs.review_label" in o + ] + self.assertTrue( + trigger_arms and "inputs.review_label != \'\'" in trigger_arms[0], + "the trigger-label operand must be guarded by " + "`inputs.review_label != \'\'`; got %r" % (trigger_arms or None,), ) def test_every_other_label_gets_its_own_namespaced_slot(self): @@ -171,7 +263,12 @@ def test_no_other_workflow_shares_the_reusable_group(self): # own ci-cursor-review.yml caller — declaring the same group would hold # it while its `uses:` job waits to acquire it, hanging until timeout. offenders = [] - for path in sorted(glob.glob(os.path.join(WORKFLOWS_DIR, "*.yml"))): + # BOTH extensions: GitHub loads `.yaml` workflows too, so a caller + # added here as `.yaml` would otherwise pass this guard and still + # deadlock its own run. + paths = glob.glob(os.path.join(WORKFLOWS_DIR, "*.yml")) + paths += glob.glob(os.path.join(WORKFLOWS_DIR, "*.yaml")) + for path in sorted(paths): if os.path.abspath(path) == os.path.abspath(WORKFLOW): continue with open(path, encoding="utf-8") as f: diff --git a/.github/workflows/cursor-review.yml b/.github/workflows/cursor-review.yml index 57e76e00..70748032 100644 --- a/.github/workflows/cursor-review.yml +++ b/.github/workflows/cursor-review.yml @@ -46,14 +46,16 @@ name: Cursor Review (reusable) # contents: read # pull-requests: write # concurrency: -# # OPTIONAL belt-and-braces once you are pinned past the commit that gave -# # this reusable its OWN workflow-level `concurrency:` block — that group -# # already collapses per-PR runs, and it is what makes skip-cursor-review -# # cancel a panel that is already running. Keep or drop this; just never +# # KEEP THIS. The reusable owns a workflow-level `concurrency:` block of +# # its own — that is what makes skip-cursor-review cancel a panel already +# # running — but it REFINES this group rather than replacing it: under the +# # default `run_without_label: false` it does not cancel on push. Never # # name a caller group `cursor-review-reusable-*` (a caller sharing the -# # reusable's group deadlocks its own run). PR number ONLY: -# # `github.event.label.name` is empty on every widened event, so keying on -# # it splits one PR across groups that cannot cancel each other. +# # reusable's group deadlocks its own run), and call this from a dedicated +# # workflow file, not one job of a larger ci.yml — cancellation is +# # run-scoped. PR number ONLY: `github.event.label.name` is empty on every +# # widened event, so keying on it splits one PR across groups that cannot +# # cancel each other. # group: cursor-review-pr-${{ github.event.pull_request.number }} # cancel-in-progress: true # jobs: @@ -339,9 +341,31 @@ concurrency: # synchronize-triggered panel would sit in a slot the veto label cannot # reach, which is the case this block exists for. # + # `inputs.review_label != ''` guards the first disjunct because the input is + # `required: false` and a caller CAN pass an empty string. GitHub coerces a + # missing `github.event.label` and `''` alike in a mixed comparison, so + # without the guard every non-label event — `pull_request_review_thread` + # included — would compare EQUAL and join `trigger`, making a thread resolve + # cancel a running panel: the exact case the paragraph above calls impossible. + # + # KNOWN GAP, case. `==` here is case-INSENSITIVE (GitHub expression + # semantics) while the Gate's own test is a case-SENSITIVE shell `=` and its + # veto check a case-sensitive `jq index(...)`. A label that differs from + # `review_label` only in case therefore reaches `trigger` and cancels the + # panel, then no-ops in the Gate — a review destroyed with nothing replacing + # it. Unfixable in an expression (there is no `lower()`, and `contains()` is + # case-insensitive too); it needs a case-matched `review_label`, which is the + # caller's to get right. Reachable only via that misconfiguration. + # + # Cancellation is ASYNCHRONOUS and SLOT-SCOPED, not ordered and total: a + # superseded run keeps unwinding after the new one starts, and only same-slot + # runs are cancelled at all. Anything reasoning about the state a cancelled + # run leaves behind has to allow for both — see the blocking-gate gotchas in + # docs/callers/cursor-review.md for the one place it is observable. + # # `inputs` is an allowed context in a called workflow's workflow-level # `concurrency:`, same as the `env:` block below. - group: cursor-review-reusable-${{ github.event.pull_request.number || github.ref }}-${{ (github.event.label.name == inputs.review_label || github.event.label.name == 'skip-cursor-review' || (inputs.run_without_label && github.event_name == 'pull_request' && (github.event.action == 'opened' || github.event.action == 'reopened' || github.event.action == 'ready_for_review' || github.event.action == 'synchronize'))) && 'trigger' || format('label-{0}', github.event.label.name) }} + group: cursor-review-reusable-${{ github.event.pull_request.number || github.ref }}-${{ ((inputs.review_label != '' && github.event.label.name == inputs.review_label) || github.event.label.name == 'skip-cursor-review' || (inputs.run_without_label && github.event_name == 'pull_request' && (github.event.action == 'opened' || github.event.action == 'reopened' || github.event.action == 'ready_for_review' || github.event.action == 'synchronize'))) && 'trigger' || format('label-{0}', github.event.label.name) }} cancel-in-progress: true # DIFF_SIZE_CAP / REVIEW_LABEL / JUDGE_MODEL / DIFF_EXCLUDES are mapped from @@ -447,13 +471,18 @@ jobs: # Only adding the trigger label fires the matrix. Adding or removing # the trigger label, and ADDING skip-cursor-review, all resolve to the # reusable's shared `trigger` concurrency slot (see the workflow-level - # `concurrency:` block), so any in-flight run for this PR is already - # cancelled before this step gets to decide — which is what makes the - # skip-cursor-review veto above true for a panel that is still - # running, not just for one that has yet to start. A caller pinned - # BELOW that commit has only its own label-scoped group, which covers - # the trigger label alone: there, applying skip-cursor-review does not - # stop a running panel. + # `concurrency:` block), so GitHub has REQUESTED cancellation of any + # in-flight same-slot run for this PR by the time this step decides — + # which is what makes the skip-cursor-review veto above true for a + # panel that is still running, not just for one that has yet to + # start. Requested, not completed: cancellation is asynchronous, so + # the superseded run is still unwinding alongside this one, and it is + # slot-scoped, so a run sitting in a `label-` slot is not + # cancelled at all. Do not add a panel starter outside `trigger` on + # the assumption that this step runs after the field is clear. A + # caller pinned BELOW that commit has only its own label-scoped group, + # which covers the trigger label alone: there, applying + # skip-cursor-review does not stop a running panel. if [ "$LABEL_NAME" = "$REVIEW_LABEL" ] && [ "$GH_EVENT_ACTION" = "labeled" ]; then echo "$REVIEW_LABEL label added — running." echo "should_run=true" >> "$GITHUB_OUTPUT" @@ -462,9 +491,10 @@ jobs: # Removing skip-cursor-review while the trigger label is present # unblocks a previously skipped review — treat it as a fresh trigger. - # This event shares the `trigger` concurrency slot too, so anything - # in flight for this PR has already been cancelled; the dup check - # below still covers the case where a review COMPLETED at this SHA. + # This event shares the `trigger` concurrency slot too, so a same-slot + # run in flight for this PR has been asked to cancel (asynchronously + # — it may still be unwinding here); the dup check below still covers + # the case where a review COMPLETED at this SHA. if [ "$LABEL_NAME" = "skip-cursor-review" ] && [ "$GH_EVENT_ACTION" = "unlabeled" ]; then if echo "$PR_LABELS" | jq -e --arg l "$REVIEW_LABEL" 'index($l)' > /dev/null; then echo "skip-cursor-review removed while $REVIEW_LABEL is present — running." @@ -1714,7 +1744,16 @@ jobs: # a write-scoped token. name: Consolidate panel needs: [gate, diff-size, preflight, review, ledger] - if: always() && needs.gate.outputs.should_run == 'true' && needs.gate.outputs.already_reviewed != 'true' && needs.diff-size.outputs.within_cap == 'true' && needs.review.result != 'skipped' + # `!cancelled()`, not `always()`: the judge is the most expensive call in + # the workflow, and the only job that does anything with its payload, + # `post-review`, is itself `!cancelled()`. On a cancelled run — now an + # advertised escape hatch, since applying `skip-cursor-review` mid-panel + # cancels the run — `always()` spent up to 30 minutes adjudicating partial + # cell output that nothing could ever post. Individual cell FAILURES are a + # different thing and are still judged: `cancelled()` asks whether the RUN + # was cancelled, so a red or cancelled matrix cell still reaches the judge, + # which is what `always()` was here for. + if: ${{ !cancelled() && needs.gate.outputs.should_run == 'true' && needs.gate.outputs.already_reviewed != 'true' && needs.diff-size.outputs.within_cap == 'true' && needs.review.result != 'skipped' }} runs-on: ubuntu-latest # ABOVE the `review` cap, never under it. The judge reads every cell's # artifact with the same top reasoning tier one cell uses, so it is the @@ -2810,7 +2849,16 @@ jobs: notify-complete: name: Notify complete needs: [gate, diff-size, review, consolidate, post-review] - if: always() && needs.gate.outputs.should_run == 'true' && needs.gate.outputs.already_reviewed != 'true' && needs.diff-size.outputs.within_cap == 'true' && needs.review.result != 'skipped' + # `!cancelled()` for the same reason `consolidate` above uses it, plus one + # of its own: this job reads a skipped/cancelled `post-review` as a posting + # failure and DMs the triggerer "Cursor review failed to post". On a + # cancelled run that message is simply false — the veto label, or a newer + # push, stopped the panel on purpose — so a run someone deliberately + # stopped should not page anyone. Genuine failures are untouched: a failed + # cell, judge or post is not a cancelled RUN and still notifies. (It also + # keeps this job's `needs: consolidate` honest now that `consolidate` is + # itself `!cancelled()` and no longer runs on a cancelled run.) + if: ${{ !cancelled() && needs.gate.outputs.should_run == 'true' && needs.gate.outputs.already_reviewed != 'true' && needs.diff-size.outputs.within_cap == 'true' && needs.review.result != 'skipped' }} runs-on: ubuntu-latest permissions: contents: read diff --git a/docs/callers/cursor-review.md b/docs/callers/cursor-review.md index 4ee22785..7033bf58 100644 --- a/docs/callers/cursor-review.md +++ b/docs/callers/cursor-review.md @@ -65,10 +65,9 @@ on: types: [labeled, unlabeled] concurrency: - # OPTIONAL once you are pinned past the commit that gave the reusable its own - # workflow-level group (see "The reusable owns a group of its own" below) — - # redundant but harmless, and it is what an older pin relies on. Never name a - # caller group `cursor-review-reusable-*`. + # KEEP THIS. The reusable owns a group of its own (see "The reusable owns a + # group of its own" below), but it refines yours rather than replacing it. + # Never name a caller group `cursor-review-reusable-*`. # NOTE: label.name is part of the key only because this caller is label-only. # Drop it if you widen `types:` — see the run_without_label gotcha. group: cursor-review-pr-${{ github.event.pull_request.number }}-${{ github.event.label.name }} @@ -177,9 +176,9 @@ on: types: [opened, reopened, ready_for_review, synchronize, labeled, unlabeled] concurrency: - # Optional once pinned past the reusable's own group — see below. If you keep - # it, drop `label.name` from the key, and never name it - # `cursor-review-reusable-*`. + # KEEP THIS — it is what supersedes a running panel on push; the reusable's + # own group only does that under `run_without_label: true` (see below). Drop + # `label.name` from the key, and never name it `cursor-review-reusable-*`. group: cursor-review-pr-${{ github.event.pull_request.number }} cancel-in-progress: true # ... @@ -199,7 +198,19 @@ Keep `labeled`/`unlabeled` in the list even in label-free mode: the label path stays live alongside it, which is how you force a re-review on an unchanged commit (dismiss the existing review, then apply the label — see the dedupe gotcha below). -**The reusable owns a group of its own, so your caller group is optional.** `cursor-review.yml` declares a workflow-level `concurrency: cursor-review-reusable--` with `cancel-in-progress: true`, which reaches you at your next pin bump with no caller edit and no permission change. Its slot rule: the trigger label and `skip-cursor-review` share one `trigger` slot, so **applying `skip-cursor-review` mid-panel cancels the running panel**; every other label gets its own `label-` slot, so an unrelated label add never kills a running review; and `pull_request_review_thread` events stay out of `trigger`, so resolving a finding thread on a blocking caller cannot cancel a panel. Under `run_without_label: true` the four plain PR actions the gate accepts (`opened` / `reopened` / `ready_for_review` / `synchronize`) join `trigger` as well, so a push supersedes a running panel and the veto label can still reach it. Keeping your own caller-level group alongside it is redundant but harmless — it cancels the same-label cases the reusable's `trigger` slot also cancels. **One hard rule, the same one [`pr-size`](pr-size.md) carries: never name a caller group `cursor-review-reusable-*`.** A caller that declares the reusable's own group deadlocks its own run — the caller holds the group while its `uses:` job waits to acquire it. +**The reusable owns a group of its own — it ADDS to your caller group, it does not replace it.** `cursor-review.yml` declares a workflow-level `concurrency: cursor-review-reusable--` with `cancel-in-progress: true`, which reaches you at your next pin bump with no caller edit and no permission change. Its slot rule: the trigger label and `skip-cursor-review` share one `trigger` slot, so **applying `skip-cursor-review` mid-panel cancels the running panel**; every other label gets its own `label-` slot, so an unrelated label add never kills a running review; `pull_request_review_thread` events stay out of `trigger`, so resolving a finding thread on a blocking caller cannot cancel a panel; and under `run_without_label: true` the four plain PR actions the gate accepts (`opened` / `reopened` / `ready_for_review` / `synchronize`) join `trigger` as well, so a push supersedes a running panel and the veto label can still reach it. + +What it does **not** do is make your own group redundant. Keep the caller group in every shape above, and know what each side costs: + +- **Under the default `run_without_label: false`, the reusable's group does not supersede on push.** That arm of the `trigger` slot is gated on the input, so a `synchronize` event lands in `label-` while the label-triggered panel sits in `trigger`, and the push cancels nothing. `post-review`'s `!cancelled()` guard exists to stop a review pinned to a superseded head SHA and depends on that cancellation — so a widened or blocking caller that drops its PR-number-only group posts reviews against stale diffs, and under `blocking: true` gates red on threads for code that no longer exists. +- **A PR-number-only caller group is coarser than the reusable's, and that is the price of the line above.** It puts *every* event for the PR in one slot, so an unrelated label add — or, on the blocking caller, a `pull_request_review_thread: resolved` — cancels the caller run, and with it the `uses:` panel job, before the reusable's per-slot group can isolate anything. The reusable's slots only refine what your own group has not already cancelled. + +**Two hard rules, both of them the ones [`pr-size`](pr-size.md) carries:** + +- **Never name a caller group `cursor-review-reusable-*`.** A caller that declares the reusable's own group deadlocks its own run — the caller holds the group while its `uses:` job waits to acquire it. +- **Call cursor-review from a dedicated workflow file, not as one job of a larger `ci.yml`.** Cancellation is run-scoped, so the reusable's group cancels the whole caller *run* — including builds, tests and deploys that have nothing to do with the review. Unlike the deadlock rule this one arrives silently at your next pin bump, with no caller edit to warn you, so check it before you bump. + +**`review_label` must match your label's case exactly.** The slot expression compares it with a GitHub expression `==`, which is case-**in**sensitive, while the gate's own decision is a case-**sensitive** shell comparison. A label differing from `review_label` only in case therefore reaches the shared `trigger` slot and cancels a running panel, then no-ops in the gate — a review destroyed with nothing replacing it. GitHub expressions have no case-sensitive string compare, so the caller has to get this right. **Veto mid-flight: on a pin BELOW that change, `skip-cursor-review` does not stop a running panel.** With only a caller-level group, `labeled: skip-cursor-review` and `labeled: cursor-review` land in *different* groups (the group key carries `label.name`), so the veto starts a run that no-ops in the gate while the panel it was meant to stop keeps going — and still posts its review. Do not try to fix it caller-side by collapsing to a PR-number-only group: that does cancel on the veto, but it also puts *every* label event in one group, so adding an unrelated label kills a running review. Bump your pin past the reusable's own group instead — there is nothing to change in the caller. @@ -210,6 +221,22 @@ step up in spend. Start label-gated. Everything in this section applies only once you pass `blocking: true`. +**A mid-panel veto leaves the check's verdict racy.** Applying `skip-cursor-review` +while a panel is running cancels that run and starts a second one, both on the +same head SHA, and both publish a `Blocking gate` check. The cancelled run trips +the "a fresh review was triggered but did not land" guard (`post-review` is +`cancelled`) and reports **red**; the veto run has `should_run=false`, skips that +guard, falls through to the live thread query and reports **green** unless an +earlier round left unresolved threads. Which one sticks is whichever job finishes +last, and nothing orders them. Treat a red gate straight after a veto as "re-run +it", not as a finding: re-applying the trigger label, resolving the threads, or +re-running the gate job settles it. The `always()` on that job is deliberate and +is not the bug — a cancelled run that *skipped* the gate would mint a green +required check, which is the exact fail-open BE-4691 added the job to close. Which +verdict a vetoed PR *should* get is a policy question and is tracked separately; +until it is settled, do not require the check on a repo where mid-panel vetoes are +routine. + **Widen your triggers before you require the check, or pushes brick the PR.** A required check that never *reports* on the head SHA blocks merge as "Expected", and the label-only caller above delivers no event on push — so @@ -224,7 +251,8 @@ on: types: [resolved, unresolved] concurrency: - # OPTIONAL belt-and-braces once pinned past the reusable's own group (see + # KEEP THIS — with `run_without_label: false` the reusable's own group does + # not cancel a panel on push, and this gate reports on the head SHA (see # "The reusable owns a group of its own"); never name it # `cursor-review-reusable-*`. PR number only — label.name is empty on the # widened events, and split groups can't cancel each other (see the