From the review of #216. Filed rather than fixed there — see the bottom.
#215 was the third bug of this shape in the fleet:
#217 proposes writing the lesson down. This issue proposes enforcing it, which is strictly stronger — a docs page does not fail CI, and #215 got in past a careful review by someone who had already fixed the same class of bug twice.
The check
scripts/pr_sanity.py:check_workflows already parses every workflow, resolves the YAML-1.1 on:/True key quirk, and inspects triggers. The guard drops straight in:
If a workflow's on: contains both pull_request and an event that fires independently of a push — issue_comment, schedule, issue_comment-like pull_request_review_comment — then any concurrency.group it declares (workflow-level or job-level) must discriminate by run or event: the expression has to reference github.run_id or github.event_name.
Flag as something like CONCURRENCY_GROUP_SHARED_ACROSS_TRIGGERS.
It is a string test on a template expression, so it is a heuristic — it proves the key can vary, not that it varies correctly. That is still enough to have caught #215, where the group was claude-review-${{ ...pr number... }} with no discriminator at all. Cheap and high-yield beats precise and unwritten.
Worth pairing with
The paths:-filter invariant already in check_workflows (NO_UNFILTERED_CI, from #200) is the same species of guard: a structural property of workflow YAML that no test would otherwise assert. This would sit next to it.
Why not in #216
#216 is a one-line concurrency fix, already green and approved, and until it merges no PR in this repo gets a Claude review — including the PR that would add this check. Bundling a new lint plus its tests into it would invalidate the review and delay unblocking every other PR, to fix a hypothetical recurrence rather than the live outage. Better as its own PR immediately after, which also gets to be the first change reviewed by the now-working workflow.
From the review of #216. Filed rather than fixed there — see the bottom.
#215 was the third bug of this shape in the fleet:
vendored-sync'scancel-in-progresscould skip verification of an intermediatemaincommit.claude-code-reviewcancelled itself: its own progress comment firedissue_commentinto the group itspull_requestrun was holding.#217 proposes writing the lesson down. This issue proposes enforcing it, which is strictly stronger — a docs page does not fail CI, and #215 got in past a careful review by someone who had already fixed the same class of bug twice.
The check
scripts/pr_sanity.py:check_workflowsalready parses every workflow, resolves the YAML-1.1on:/Truekey quirk, and inspects triggers. The guard drops straight in:Flag as something like
CONCURRENCY_GROUP_SHARED_ACROSS_TRIGGERS.It is a string test on a template expression, so it is a heuristic — it proves the key can vary, not that it varies correctly. That is still enough to have caught #215, where the group was
claude-review-${{ ...pr number... }}with no discriminator at all. Cheap and high-yield beats precise and unwritten.Worth pairing with
The
paths:-filter invariant already incheck_workflows(NO_UNFILTERED_CI, from #200) is the same species of guard: a structural property of workflow YAML that no test would otherwise assert. This would sit next to it.Why not in #216
#216 is a one-line concurrency fix, already green and approved, and until it merges no PR in this repo gets a Claude review — including the PR that would add this check. Bundling a new lint plus its tests into it would invalidate the review and delay unblocking every other PR, to fix a hypothetical recurrence rather than the live outage. Better as its own PR immediately after, which also gets to be the first change reviewed by the now-working workflow.