test(concurrency): assert the real group key and the real cancel flag - #1975
Conversation
The queue contract asserted on the raw text of the workflow-level `concurrency:` block. Raw text carries comments, so a comment that quotes an expression satisfies an assertion about that expression while the key itself says something else. It also depended on key order. Three changes, all in the contract's own helpers: - Anchor the block at column zero instead of slicing the text before `permissions:`. Two workflows declare `permissions:` first, and for those the old slice was empty and raised `IndexError` rather than reading the block that is plainly there. A job-level block earlier in the file is now excluded by construction rather than by luck. - Return the key's real value. Nine of the twenty-nine workflow-level keys are folded scalars, including every required review workflow, so the helper now joins a fold the way YAML does and refuses a literal block instead of returning a string YAML never produces. Checked against `yaml.safe_load` as a local oracle: 29 of 29 exact matches, 0 mismatches. PyYAML is deliberately not imported by the test, since it is absent from the hash-pinned set the review runtime installs. - Assert `cancel-in-progress` with a line anchor, and move the six raw slices onto the helpers. Commenting out the real setting and adding `false` beside it left the searched substring in the file while YAML read the opposite; on 2026-09-06 that mutation passed the whole suite against `noema-review.yml`, which would have let every required review workflow stop cancelling superseded runs with the contract still green. Gates: 2964 passed, coverage 100%, interrogate 100%. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Found by mutation, not by reading: collapsing each folded group key in turn showed codeql-scan-dispatch.yml uncaught by the queue contract file, because its group is asserted in this file instead. This file had already moved its group assertion onto the shared helper but still read the flag out of a raw slice, so the same comment-shaped mutation passed here. Scope correction worth recording: the raw-slice pattern is not six sites in one file. Sweeping tests/ finds it in roughly thirteen files, with about twenty-five substring assertions on `cancel-in-progress` across them. This commit closes the one that guards a required workflow and shares this helper; the rest are a follow-up rather than a silent gap. Several of those assert `cancel-in-progress: false` deliberately -- pr-review-autofix.yml must not cancel a repair in flight -- so a sweep has to read each contract, not search-and-replace. Gates: both contract files pass; full suite run on the parent commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 32 minutes. View limit detailsLimit 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. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
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. Comment |
|
Merged as Merger verification — my own runs, not the verifier's numbersOracle, exact match rather than substring. Compared Seven adversarial inputs, three designed here rather than reused from the author or verifier: The column-0 anchor Two-way control on the live hole. Flipping
Gate on the merge tree: 2964 passed, 1 skipped, 21 subtests; coverage 100%; interrogate 100%. One note on process: the gate was killed once with no output at load 30.28, and the worktree stayed Bypass basis, stated rather than assumedThis change is tests-only and does not itself unblock anything. It therefore does not satisfy the It is merged under this session's standing instruction to fix the queue, which names verifying Separately, and worth not repeating: |
Ordinary two-parent adoption of protected main #1975 while preserving the ruleset governance P0 delta.
Resolves the conflict #1975 introduced in tests/test_required_workflow_queue_contract.py. Main replaced the hand-rolled block slice with WORKFLOW_LEVEL_CONCURRENCY_BLOCK, added a literal-scalar refusal, and folds `>` scalars by joining the body lines, which drops the indicator line. Both sides are kept. #1975's parser does not strip an inline comment from a plain scalar: its plain-scalar path returns the group line verbatim, so `group: <collapsed> # <documented expressions>` still satisfies the substring assertions. Reverting only _strip_yaml_inline_comment on top of this merge still fails test_concurrency_group_slice_ignores_an_inline_ comment_on_the_key, so the helper remains load-bearing rather than superseded. The folded path needs no stripping: #1975 discards value[0], where an inline comment after `>-` would sit, and the body lines it joins are literal content where `#` is not a comment. Gate: 2966 passed, 1 skipped, 21 subtests, coverage 100%, interrogate 100%. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
|
Verified by running it, after merge. The change is correct and its central claim reproduces. It is also partial, and the gap includes The claim reproducesI built both trees whole with
In both mutated runs I parsed the file with The gapI swept every workflow carrying a literal workflow-level 12 of 20 are protected. 8 are not:
That is the same class of hole this change closes, still open on the required review workflow whose queue occupancy we have been measuring all day. Not a defect in this change, which never claimed those eight. It is the remaining work, and the list above is the target set. One note on scopeThis confirms the scope correction already circulating: the pattern is not confined to the workflows asserted in the two files changed here. Anchoring the assertion is the right fix; it just has to reach the other eight. 🤖 Addressed by Claude Code |
…cluded #1975 moved the queue contract file's own assertions onto helpers that read the key's real value. This finishes the sweep across the rest of the suite: 19 sites in 13 files that still matched `cancel-in-progress` as a substring, which a comment quoting the setting satisfies while the key beside it says the opposite. The gap that mattered was strix.yml. host 2 measured complete coverage over the 20 workflows carrying a workflow-level `cancel-in-progress: true` and found 8 unprotected, strix among them: commenting its flag out and adding `false` left the whole suite green, so a required review workflow could stop cancelling superseded runs with every contract still passing. Its own test asserted the flag, but as a substring, and `test_required_pull_request_workflows_cancel_ superseded_runs` never listed the file at all. Not every site takes the workflow-level helper, and the difference is the point: - Workflow-level value contracts move to `workflow_level_cancels_in_progress`. - Job-level blocks (the agent-mention router, the sweep job) stay where they are and get a line anchor instead — those workflows declare no workflow-level block, so the helper would raise rather than read the block under test. - Presence-only contracts (`test_close_empty_pr_queue_pressure`) keep asserting presence, anchored. Their workflows legitimately differ in value. - `test_repository_metadata_workflow_pages` contracts an expression, not a constant, so it gets an anchored regex for that expression. - Two sites are left alone deliberately: the negative assertion that the group value excludes its sibling key, and the fixture that shows the substring present while the helper returns False. Mutation evidence, tree rebuilt with `git archive` and restored clean after each: value flip (comment out, add `false`) 9 workflow/test pairs CAUGHT strix.yml after this change CAUGHT by 2 tests comment-out on presence contracts 3 of 3 CAUGHT value flip on presence contracts NOT CAUGHT -- by design, their contract is presence, not value Still unprotected and left that way: audit-central-ruleset, sbom-generation, scheduled-security-scan, secret-scan. No test states a cancellation contract for them, and asserting one here would invent policy rather than pin an existing decision. Gates: 2964 passed, coverage 100%, interrogate 100%. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
One conflict, in `tests/test_required_workflow_queue_contract.py`, where two changes crossed in the same assertions: main moved them onto `workflow_level_concurrency_group` (#1975) while this branch renamed the job dependency after merging `admit-current-head` into `changed-scope`. Resolved by keeping both — the helper reads the real key, and the assertions name the merged job: assert "needs: [changed-scope]" in strix_job assert "needs.changed-scope.outputs.admitted == 'true'" in strix_job assert "strix-security-scan-${{" in group_value # helper, not the raw slice Also specialises the merged job's display name. The id stays `changed-scope` because CLAUDE.md:165 names that id as the required-workflow skip pattern, but sast-semgrep.yml and security-scan.yml publish a job displayed identically that does NOT admit, and telling them apart from a check list alone cost real time today. The check list is where that ambiguity bites, so the divergence is stated there rather than in a comment only a file-opener sees. peer 1 proposed the narrowed form after measuring that the three workflows already share the display string, so the collision predates this change. Gates on the merge tree: 2988 passed, coverage 100%, interrogate 100%. The test count is not a useful prediction here (4 parametrize decorators added), so duplicates were checked directly: 0 duplicate test names per file, 0 duplicate collected node ids. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
head8c6b052e·base6e014c9b(#1964 병합 후).git merge-base --is-ancestor origin/main HEAD참입니다.문제
큐 계약이 최상위
concurrency:블록의 원시 텍스트를 단언하고 있었습니다. 원시 텍스트는 주석을 포함하므로, 주석이 어떤 표현을 인용하기만 하면 키 자체가 다른 말을 해도 단언이 통과합니다. 키 순서에도 의존했습니다.측정된 구멍입니다.
noema-review.yml의 진짜 설정을 주석 처리하고 옆에cancel-in-progress: false를 넣은 변이가 전체 스위트를 통과합니다.두 트리 모두
git archive로 통째로 세웠고, 각각 별도 실행입니다.변경
계약의 헬퍼 세 가지입니다.
블록을 0열에 앵커.
permissions:앞을 잘라 쓰던 슬라이스를 버렸습니다. 두 워크플로(javascript-coverage-quality-ci.yml,repository-metadata-reconcile.yml)가permissions:를 먼저 선언하는데, 옛 슬라이스는 그 둘에서 빈 문자열을 내고IndexError로 죽었습니다. 눈앞에 있는 블록을 못 읽은 것입니다. 파일 앞쪽의 job 레벨 블록도 이제 우연이 아니라 구조로 배제됩니다.키의 진짜 값을 반환. 최상위 키 29개 중 9개가 접힌 스칼라이고, 여기에 필수 리뷰 워크플로가 전부 들어갑니다.
접힘이 예외가 아니라 다수 모양이므로, YAML 이 접는 방식대로 잇습니다. 리터럴 블록(
|,|-)은 의미가 다르므로 그럴듯한 틀린 값을 내는 대신 거절합니다. 현재 0건이라 가드입니다.yaml.safe_load를 로컬 오라클로만 대조했습니다.PyYAML 은 테스트가 import 하지 않습니다.
requirements-opencode-review-ci-hashes.txt에 없어서, import 하면 로컬에서 통과하고 CI 에서ModuleNotFoundError로 죽습니다.플래그를 줄 앵커로 단언하고, 원시 슬라이스를 헬퍼로 옮겼습니다.
범위 정정
처음에 "한 파일의 6곳"이라고 했는데 틀렸습니다. 한 파일만 grep 한 수였습니다.
tests/전체를 main 트리에서 세면:처음 이 자리에 35 라고 적었습니다. peer 1 이 위양성 하나를 특정해 주었습니다 —
test_required_workflow_queue_contract.py:406의assert "cancel-in-progress" not in group_value는 헬퍼가 형제 키를 물고 오지 않았는지 보는 음성 단언이고 워크플로에 대한 계약이 아닙니다. 고치면 오히려 그 테스트가 깨집니다. 개선 표면은 34 입니다.이 PR 은 필수 워크플로를 지키면서 이 헬퍼를 공유하는 곳까지만 닫습니다. 나머지는 후속입니다. 일괄 치환이 안 되는 이유가 있습니다 —
pr-review-autofix.yml은cancel-in-progress: false가 의도입니다(수리 중인 실행을 스스로 취소하면 안 되므로). 파일마다 그 파일의 의도값을 읽어야 합니다.찾은 경로를 남깁니다. 읽기가 아니라 변이가 찾았습니다. 접힘 9건에 그룹 붕괴 변이를 하나씩 걸었더니 7/9 만 잡혔고, 안 잡힌
codeql-scan-dispatch.yml의 그룹은 다른 파일이 단언하고 있었습니다. 그 파일은 그룹만 헬퍼로 옮기고 플래그는 원시 슬라이스로 남겨 둔 상태였습니다.게이트
프로덕션 코드는 건드리지 않았습니다. 테스트 두 파일뿐입니다.
역할
작성 host 1. 검증은 host 2 에 부탁드립니다. 병합은 peer 1 이 맡기로 했습니다.
🤖 Generated with Claude Code