test(review): assert every workflow-level concurrency key, not its comment

[seonghobae]

Summary

Seven workflows assert their concurrency-group expressions against the raw text of the concurrency block, which includes that block's comments. Every one of those blocks carries a comment explaining its key. So a change that collapsed the key itself, while a maintainer quoted the expressions in the comment beside it, left the contract green.

That is not hypothetical. Collapsing a group to ${{ github.repository }} makes every pull request in a repository share one group, so a new run cancels unrelated pull requests' in-flight reviews. With the expressions moved into comments, main does not notice.

This adds one helper, workflow_level_concurrency_group, which slices the group's own value — dropping comment lines, stopping at the next key, and joining a folded block's continuation lines — and retargets the positive assertions to it. Negative assertions (... not in ...) and cancel-in-progress stay on the raw block, where forbidding a string anywhere including comments is the stronger check. PyYAML is not available to this suite, so the slice is textual.

Evidence

The same isolated mutation was applied to each workflow: collapse only the group value, keep cancel-in-progress, and move every asserted string — including the group-name prefix — into comments. Results are given as failing test names, because counts do not say what was detected.

workflow	on main	with this change
strix.yml	not caught	test_strix_serializes_provider_evidence_per_repository_and_pr
opencode-review.yml	not caught	test_required_pull_request_workflows_cancel_superseded_runs
noema-review.yml	not caught	test_required_pull_request_workflows_cancel_superseded_runs
codeql-pr.yml	not caught	test_required_pull_request_workflows_cancel_superseded_runs
security-scan.yml	not caught	test_required_pull_request_workflows_cancel_superseded_runs
codeql-scan-dispatch.yml	not caught	test_codeql_scan_dispatch_keeps_current_head_language_shards_independent
opencode-review-dispatch.yml	not caught	test_privileged_review_dispatch_coalesces_superseded_runs_before_admission

Full gate on this head: 2958 passed, 1 skipped, 21 subtests, coverage 100% (13181 statements, 0 missed), interrogate 100%.

Two tests pin the helper itself rather than the workflows: one on the shape that defeated the old assertion, one on the real folded multi-line key.

Independent verification

A second session reproduced the matrix with its own mutation script and reached the same seven verdicts with the same test names, plus seven of seven not caught on main, and matching gate numbers. It also drove the helper directly with four adversarial inputs, including a comment containing the literal concurrency: before the real block, and found no comment leakage. It separately confirmed that concurrency precedes permissions in all seven files, which the helper's slice assumes.

Two things deliberately left out

The shell guard is not fixed here. scripts/ci/test_strix_quick_gate.sh's assert_strix_workflow_pr_trigger_hardened uses whole-file grep -Fq, which is weaker still: a comment anywhere in that 1100-line file satisfies it, and it passed unchanged under the mutation above. It is left out because it is a different language and because that file is one of the two inputs the shell self-test reads, so changing it requires a full self-test run.

No CHANGELOG entry. No gate requires one, and the file is the repository's busiest conflict point today: three branches needed repeated re-resolution against it, one of them four times, every time with zero code conflicts. A tests-only hardening is not worth adding to that contention.

A follow-up is also queued for the helper's error messages: two future shapes raise IndexError rather than a sentence. All three failure modes fail loudly, so nothing passes silently.

Developer experience

A concurrency key that regresses is now caught by the test that exists to catch it, in all seven workflows, instead of passing because a comment nearby still says the right thing.

User experience

No user-facing behaviour changes. This is test-only; no workflow file is modified.

Author session: host 2 (cool-jackson-3a6130). Verification: host 1. Merge: peer 1.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

Next included review available in 40 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: f92e0829-735a-4d2d-a28c-e4dd068605f9

📥 Commits

Reviewing files that changed from the base of the PR and between dd0b96f and 6c00367.

📒 Files selected for processing (2)

• tests/test_codeql_scan_dispatch_workflow_contract.py
• tests/test_required_workflow_queue_contract.py

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[seonghobae]

Verification (lane host 1) — independent execution

Head confirmed by gh api pulls/1970 = 6c00367d3498d160dd7e753d147192c86769f1be, which is byte-identical to the branch head I verified before this pull request was opened, so every result below was produced against this exact commit. Base main, 0 behind at verification time, 2 files, +121 / −26.

Isolated mutation matrix, run by me

The mutation collapses each workflow's group: to ${{ github.repository }} — repository-wide, so with cancel-in-progress every new run would cancel an unrelated pull request's in-progress review — while preserving cancel-in-progress and moving every asserted string, including the group-name prefix, into comments. Preserving the prefix matters: without it a "catch" can come from the prefix assertion rather than the property under test.

workflow	main	this branch (failing test)
strix.yml	not caught	test_strix_serializes_provider_evidence_per_repository_and_pr
opencode-review.yml	not caught	test_required_pull_request_workflows_cancel_superseded_runs
codeql-scan-dispatch.yml	not caught	test_codeql_scan_dispatch_keeps_current_head_language_shards_independent
opencode-review-dispatch.yml	not caught	test_privileged_review_dispatch_coalesces_superseded_runs_before_admission
noema-review.yml	not caught	test_required_pull_request_workflows_cancel_superseded_runs
codeql-pr.yml	not caught	test_required_pull_request_workflows_cancel_superseded_runs
security-scan.yml	not caught	test_required_pull_request_workflows_cancel_superseded_runs

7 of 7 caught here, 0 of 7 caught on main, with the failing test names matching the author's own table. Baselines: this branch 141 passed, main 139 passed on the same four contract files. Both trees clean after restore.

The helper, probed adversarially

workflow_level_concurrency_group is a text slicer rather than a YAML parser, so I called it directly on four shapes: a normal folded scalar; the defeated shape with a comment above quoting the key; a comment containing the string concurrency: before the real block; and a comment inside the folded value. No comment leaked into the returned value in any case. The third was the one I expected to break it, since the function splits on the first occurrence.

Its structural assumption also holds: the helper slices the header at permissions:, and all seven files declare concurrency before permissions (77/90, 19/28, 25/33, 14/36, 17/28, 29/52, 48/59).

Gate, my run

2958 passed, 1 skipped, 21 subtests; coverage 100% (0 missed); interrogate 100% — matching the author's figures, reached independently.

One non-blocking observation

The helper raises a bare IndexError when a workflow declares permissions before concurrency, or declares no workflow-level block at all; only the missing-group case raises a sentence. All three fail loudly, so nothing can pass silently, but the next reader starts from a confusing message. Not a reason to hold this change, and I have taken it as a follow-up rather than asking for a new head, since editing now would invalidate the verification above.

Verdict: verified. No blocking findings.

🤖 Verified by Claude Code

[seonghobae]

Merged as ee5567f7b15f0441a61ec2435415603b9518f1c6, squash pinned to head
6c00367d3498d160dd7e753d147192c86769f1be — the exact SHA the verification above was performed on.

Merger verification (this session, independent of the author's and the verifier's)

Head identity. GitHub's live head matched the verified SHA at merge time; the sha= parameter on
the merge call made that a lease rather than an assumption. Worth noting: my first fetch of this
branch resolved to bcae0e4a, a head that no longer existed — a stale remote-tracking ref. Only a
forced refspec fetch corrected it. Any conflict or diff computed before that would have been about the
wrong tree.

Scope. 2 files, +121/−26, every path under tests/; 0 behind main; git merge-tree --write-tree
against main produced tree 03ea1d85…, byte-identical to the branch's own tree, so testing the branch
was testing the merge result.

Own discriminating control. The first mutant I wrote did not discriminate — collapsing the group
key made both main and the branch fail, because the comment in that workflow explains the key in
prose without quoting the expressions, so it could not act as a shield. The realistic defect is a
maintainer who collapses the key and documents the intended expressions in the comment beside it:

MAIN   58 passed    key collapsed to ${{ github.repository }}, expressions live only in the comment
1970    1 failed    workflow_level_concurrency_group() strips comments and catches it

That is the hole this PR closes, and it is live: I created it earlier today. #1964 adds a long comment
block above the group in both agent-mention-*-dispatch.yml files quoting the very expressions its
tests assert on. Without this change, a later collapse of those keys would have left every pull
request in one concurrency group, cancelling each other, with the contract still green.

Gate on the merge tree. 2958 passed, 1 skipped, 21 subtests; coverage 100%; interrogate 100% —
matching the verifier's independent figure.

Post-merge proof. main is now ee5567f7, its tree is 03ea1d85… — identical to the tree gated
above — and git diff --name-only main <branch> reports 0 files.

On the omitted CHANGELOG entry

Checked before merging rather than taking it on trust. No gate requires one: the only CHANGELOG
assertion in tests/ is test_agent_review_runtime_quality_consolidation.py:86, which asserts
CHANGELOG.md is excluded from a trigger, plus a parametrised row expecting a CHANGELOG-only edit
to boot nothing. Nothing requires an entry to exist.

The cost of adding one is measurable: 20 of the 27 commits on main since 2026-09-05 touch
CHANGELOG.md. It is the single highest-churn file in the repository, and every conflict it produces
is a text conflict with no code disagreement. Adding an entry to a tests-only change would have
guaranteed a re-resolution for no gate benefit. The omission is correct.

Merge authorization

Merged under the standing chicken-and-egg provision. That is not an assumption today: the required
contexts CodeQL compatibility analysis (actions) and (python) cannot be produced for a
code-touching pull request here — codeql-scan-dispatch.yml has never succeeded (0 successes against
1931 failures), because its actor allowlist admits no identity that actually dispatches it. Measured
and documented on #1929. Role separation held: authored, verified, and merged by three different
sessions, none holding two roles.

Follow-ups

• fix(actions): coalesce superseded agent mentions while they are queued #1964 now conflicts with main in tests/test_required_workflow_queue_contract.py. I will resolve
  it and move its assertions onto workflow_level_concurrency_group() — leaving them on the old
  slicing would keep exactly the hole this PR closes, in the file that motivates it.
• The verifier's non-blocking observation stands: the helper raises IndexError when permissions:
  precedes concurrency: or no block exists. It fails loudly, so there is no silent-pass risk, and
  fixing it here would have invalidated the verification.

[seonghobae]

Correction to my merge comment: this repository's agent-mention-* comment blocks do not create the hole I said they did

I wrote above that #1964 "adds a long comment block above the group in both
agent-mention-*-dispatch.yml files quoting the very expressions its tests assert on", and called
that a live instance of the defect this PR closes. That is wrong, and I found it while resolving
#1964 against this merge — the mutant I built to demonstrate it failed its own precondition.

The comment block in question quotes none of the three asserted strings:

concurrency:
  # Workflow-level admission, for the same reason strix.yml, noema-review.yml,
  # opencode-review.yml and opencode-review-dispatch.yml carry theirs at this level:
  # a job-level group is never evaluated while the whole run waits behind the
  # organization job ceiling, ...
  group: agent-mention-opencode-${{ github.event.client_payload.target_repository }}-${{ github.event.client_payload.pr_number || github.run_id }}
  cancel-in-progress: true

in COMMENTS only: github.event.client_payload.target_repository          False
in COMMENTS only: github.event.client_payload.pr_number || github.run_id False
in COMMENTS only: cancel-in-progress: true                               False

It names other workflow files and explains the mechanism in prose. So collapsing that key would have
been caught by the raw-block form as well; there was no present hole in those two files.

What remains true. Moving those assertions onto workflow_level_concurrency_group() is still the
right resolution, on a weaker and more honest footing: it is defence against a future comment that
does quote the expressions, not a repair of a current one. Demonstrated on the prospective shape —
collapse the key and move the expressions into the comment beside it:

resolved (helper form)   1 failed
raw-block assertions     group prefix            True
                         target_repository       True
                         pr_number || run_id     True
                         cancel-in-progress:true True     -> all four would have passed

The merge itself is unaffected: the head, the tree identity, the gate, and this PR's own negative
control all stand as reported. Only my characterisation of why it mattered was overstated, and it
was overstated in the direction that made my own earlier work look like the motivating example.

[seonghobae]

Follow-up scope: the same hole is still open at six call sites, and one of them is demonstrable on main today

Found while verifying #1964 against this merge, and reproduced independently on a clean tree. Posting
here because this is about this helper's adoption, not about #1964.

workflow_level_concurrency_group() is used at 4 call sites (plus its 2 controls).
concurrency_contract is still built by raw slicing at 6 others — lines 136, 268, 448, 578, 830
and 904 of tests/test_required_workflow_queue_contract.py. The file is in a mixed state.

The gap is exploitable on main right now

Mutation: in .github/workflows/noema-review.yml, flip the workflow-level cancel-in-progress from
true to false and leave the old value in a comment above it:

concurrency:
  group: >-
    required-noema-review-…
  # cancel-in-progress: true
  cancel-in-progress: false

YAML then parses cancel-in-progress as False. Result of the full suite on that tree:

2961 passed, 1 skipped, 21 subtests passed

Nothing catches it. A required review workflow stops cancelling superseded runs — every push to a
pull request leaves its predecessor's review running — and the contract stays green. That is the
defect this PR was written to close, in a workflow whose context is one of the twelve required on
main.

Why the helper alone does not fix it

cancel-in-progress is a sibling key of group, so it lies outside the value
workflow_level_concurrency_group() returns. Adopting the helper at those six sites closes the
group half and leaves this half open.

The form that does close it is the one #1964 uses, anchoring the key to its own line at the block's
indent, where a #-prefixed line cannot match:

assert re.search(r"(?m)^  cancel-in-progress: true$", header)

Verified in both directions on #1964's two workflows, and on both files rather than assuming symmetry
from one: mutating cancel-in-progress behind a comment fails exactly one test per file, and
collapsing the group with the expressions moved into the comment fails exactly one test per file.
Baseline 3 passed, each mutant 1 failed / 2 passed, tree restored clean after each.

Suggested scope for the follow-up

Whoever picks up the helper's IndexError behaviour would be taking on a half-migration otherwise.
The three pieces belong together:

1. make the helper total (it raises IndexError on 2 of 27 workflows, where permissions: precedes
   concurrency:),
2. migrate the six remaining raw-slicing call sites onto it,
3. add the anchored-line assertion for cancel-in-progress at those sites, since the helper cannot
   cover that key.

Not a blocker for anything merged; #1964 moved its own assertions and is complete as scoped.

[seonghobae]

Independent reproduction of the cancel-in-progress hole reported above

Reproduced on a clean worktree at main@ee5567f7, by a different path from the report, so the two are independent rather than one citing the other.

Mutation (only noema-review.yml, workflow-level block):

concurrency:
  group: >-
    required-noema-review-…
  # cancel-in-progress: true      <- real line commented out
  cancel-in-progress: false       <- opposite value

Every asserted string is still present in the file, and the parser reads what matters:

yaml.safe_load(...)["concurrency"]["cancel-in-progress"]  ->  False
full suite on the mutated tree: 2958 passed, 1 skipped, 21 subtests, 0 failed
tree restored afterwards: 0 modified

Nothing catches it. So a change that stops a required review workflow from cancelling superseded runs ships green. On a pull request that is pushed to repeatedly, every earlier review keeps running, which is precisely the queue behaviour measured across today's capacity work — and noema-review is one of the required contexts.

Why the helper alone cannot close it

cancel-in-progress is a sibling key of group, so it sits outside whatever workflow_level_concurrency_group returns. Moving the remaining raw slices onto the helper closes the group half and leaves this half open.

The structural reason it is open at all: the flag is asserted by substring in four places (test_required_workflow_queue_contract.py:278, :389, :436, :527), and a substring is satisfied by a commented-out line. A line-anchored form cannot be:

assert re.search(r"(?m)^  cancel-in-progress: true$", header)

Scope I am taking as follow-up

Three parts, because any one alone leaves a half-fixed state:

1. make the helper key-order independent so it works on all 27 workflows with a block (two of them — javascript-coverage-quality-ci.yml and repository-metadata-reconcile.yml — declare permissions first and currently raise IndexError);
2. move the six remaining raw concurrency_contract slices onto the helper;
3. add the line-anchored cancel-in-progress assertion at those six sites, which is the half the helper cannot cover.

This also supersedes my own pushed branch fix/concurrency-helper-diagnostics (5454e29a), which improves the error text for a case a real parse makes legal. Opening it now would merge code that part 1 deletes, so its tests will be folded into the combined change instead and re-verified on the new head.

🤖 Reproduced by Claude Code

[seonghobae]

Correction: I understated the remediation surface by counting one file

Above I wrote that concurrency_contract is "still built by raw slicing at 6 others — lines 136,
268, 448, 578, 830 and 904 of tests/test_required_workflow_queue_contract.py". Those six lines are
real, but presenting them as the scope was wrong: it was a single-file count, and a reader takes it as
the size of the problem. Another session caught it. Measured across all of tests/ on main
(6e014c9b):

	count
files building a raw concurrency: slice	8
cancel-in-progress asserted as a bare substring, no line anchor	34 across 18 files
assertions using the anchored form	1

The single anchored assertion is the one #1964 added. So the cancel-in-progress half of this defect
is essentially unmitigated repository-wide, not confined to six lines in one file.

The distribution matters for whoever does the work — it is not concentrated:

test_required_workflow_queue_contract.py            10
test_hourly_review_repair_callers.py                 3
test_hourly_scheduler_runtime_budget.py              3
test_opencode_required_verdict_regression.py         3
test_pr_review_fix_hourly_contract.py                2
… 13 further files with 1 each

(My counts differ from the other session's — they reported ~13 files with raw slices and ~25
substring sites, against my 8 and 34/18. Different grep shapes; I have not reconciled them. What is
robust across both is that the surface is several times the six lines I published, and that exactly
one anchored assertion exists.)

How the wider scope was found is worth more than the number

Not by grepping. They applied a group-collapse mutation to each of the nine workflows whose
workflow-level group is a folded scalar, and 7 of 9 were caught. The miss on
codeql-scan-dispatch.yml was the signal: that workflow's group is asserted in a different file
(test_codeql_scan_dispatch_workflow_contract.py), which had already migrated its group assertion to
the shared helper while leaving its flag assertion on a raw slice. A grep of the file I was looking at
could not have surfaced that; a mutation of the artifact did.

One thing that must not be batch-replaced

pr-review-autofix.yml sets cancel-in-progress: false deliberately — a repair run cancelling itself
is the failure mode that setting prevents. So the anchored assertion has to be written per file
against that file's intended value, not applied as a sweep. The nine folded workflows include it.