From edea403bc00e6761ecc8fee10c8051f3a2ceda0f Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 7 Sep 2026 00:41:15 +0900 Subject: [PATCH 1/2] test(concurrency): read the real key and the real cancel flag 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 --- .../test_required_workflow_queue_contract.py | 154 ++++++++++++++++-- 1 file changed, 142 insertions(+), 12 deletions(-) diff --git a/tests/test_required_workflow_queue_contract.py b/tests/test_required_workflow_queue_contract.py index 4055bb5b9d..512cb6bef5 100644 --- a/tests/test_required_workflow_queue_contract.py +++ b/tests/test_required_workflow_queue_contract.py @@ -21,6 +21,18 @@ def workflow_text(name: str) -> str: return (REPO_ROOT / ".github" / "workflows" / name).read_text(encoding="utf-8") +# The workflow-level block is the one whose key starts at column zero; job-level +# blocks are indented under ``jobs:``. Anchoring there instead of slicing the text +# before ``permissions:`` makes the search independent of key order, which two +# workflows already need: javascript-coverage-quality-ci.yml and +# repository-metadata-reconcile.yml declare ``permissions:`` above ``concurrency:``, +# and the older slice returned nothing for them and raised IndexError rather than +# reading the block that is plainly there. +WORKFLOW_LEVEL_CONCURRENCY_BLOCK = re.compile( + r"(?m)^concurrency:[ \t]*\n(?P(?:[ \t]+[^\n]*\n)+)" +) + + def workflow_level_concurrency_group(workflow: str) -> str: """Return only the workflow-level ``concurrency.group`` value, comments removed. @@ -33,11 +45,12 @@ def workflow_level_concurrency_group(workflow: str) -> str: cancelling each other, with the contract still green. Slice to the group's own value so the assertion tests the key rather than the documentation beside it. """ - header = workflow.split("permissions:", 1)[0] - block = header.split("concurrency:", 1)[1] + block_match = WORKFLOW_LEVEL_CONCURRENCY_BLOCK.search(workflow) + if block_match is None: + raise AssertionError("workflow declares no workflow-level concurrency block") value: list[str] = [] collecting = False - for line in block.splitlines(): + for line in block_match.group("body").splitlines(): if line.strip().startswith("#"): continue if not collecting: @@ -50,7 +63,46 @@ def workflow_level_concurrency_group(workflow: str) -> str: value.append(line) if not collecting: raise AssertionError("workflow-level concurrency block declares no group") - return "\n".join(value) + head = value[0].strip() + if head.startswith("|"): + # Not represented here, and on 2026-09-07 no workflow uses one: a literal + # block keeps its newlines, so folding it would return a value YAML never + # produces. Refusing is better than returning a plausible wrong string. + raise AssertionError("literal block scalars are not supported for the group key") + if head.startswith(">"): + # Nine of the twenty-nine workflow-level keys are folded, including every + # required review workflow, so this is the majority shape rather than an + # edge case. YAML joins a folded scalar's lines with single spaces, so + # returning the indicator and the raw newlines would make the helper + # disagree with the file's own meaning. Blank lines and more-deeply + # indented lines inside a fold keep their newlines in YAML and are not + # handled here; neither shape occurs in this tree. + return " ".join(part.strip() for part in value[1:] if part.strip()) + return "\n".join(value).strip() + + +def workflow_level_cancels_in_progress(workflow: str) -> bool: + """Return whether the workflow-level block really sets ``cancel-in-progress: true``. + + Anchored to the start of a block line, so a commented-out setting cannot + satisfy it. Substring assertions could: commenting the real line out and + adding ``cancel-in-progress: false`` beside it leaves the searched text in + the file while YAML reads the opposite, and on 2026-09-06 that mutation + passed the whole suite (2958 passed, 0 failed) against ``noema-review.yml``. + A required review workflow that stops cancelling superseded runs keeps every + earlier review alive on each push, which is the queue behaviour this + repository has been trying to remove. + + Kept separate from the group helper on purpose: ``cancel-in-progress`` is a + sibling of ``group``, so it lies outside the value that helper returns and + cannot be covered by moving assertions onto it. + """ + block_match = WORKFLOW_LEVEL_CONCURRENCY_BLOCK.search(workflow) + if block_match is None: + raise AssertionError("workflow declares no workflow-level concurrency block") + return bool( + re.search(r"(?m)^[ \t]+cancel-in-progress:[ \t]+true[ \t]*$", block_match.group("body")) + ) def workflow_step(workflow: str, name: str) -> str: @@ -144,7 +196,10 @@ def test_merge_scheduler_uses_native_auto_merge_after_required_checks() -> None: assert "github.event_name == 'repository_dispatch' && github.run_id" not in ( concurrency_contract ) - assert "cancel-in-progress: ${{" in concurrency_contract + # Anchored, not a substring: this workflow's value is an expression rather + # than a constant, so it cannot use the boolean helper, but a commented-out + # setting must not satisfy it either. + assert re.search(r"(?m)^[ \t]+cancel-in-progress:[ \t]+\$\{\{", concurrency_contract) assert "github.event_name == 'repository_dispatch'" in concurrency_contract @@ -275,7 +330,7 @@ def test_privileged_review_dispatch_coalesces_superseded_runs_before_admission() in group_value ) assert "github.event.client_payload.pr_number || github.run_id" in group_value - assert "cancel-in-progress: true" in concurrency_contract + assert workflow_level_cancels_in_progress(workflow) assert "github.event.client_payload.pr_head_sha" not in concurrency_contract assert re.search(r"(?m)^ concurrency:", workflow) @@ -406,6 +461,74 @@ def test_concurrency_group_slice_reads_a_folded_multi_line_key() -> None: assert "cancel-in-progress" not in group_value +def test_concurrency_helpers_read_the_block_when_permissions_comes_first() -> None: + """Key order must not decide whether the contract can see the block. + + The earlier helper sliced the text before ``permissions:`` and then split on + ``concurrency:``. That works only when ``concurrency:`` is declared first. Two + workflows in this repository declare ``permissions:`` above it -- + javascript-coverage-quality-ci.yml and repository-metadata-reconcile.yml -- + and for those the slice was empty, so the helper raised ``IndexError`` instead + of reading the block that is plainly there. Anchoring at column zero makes the + order irrelevant. + """ + permissions_first = textwrap.dedent( + """\ + name: Example + permissions: + contents: read + concurrency: + group: example-${{ github.repository }}-${{ github.event.pull_request.number }} + cancel-in-progress: true + jobs: + build: + runs-on: ubuntu-latest + """ + ) + + assert ( + workflow_level_concurrency_group(permissions_first) + == "example-${{ github.repository }}-${{ github.event.pull_request.number }}" + ) + assert workflow_level_cancels_in_progress(permissions_first) + + +def test_concurrency_helpers_name_a_missing_block_instead_of_index_error() -> None: + """A workflow with no top-level block must fail with a sentence, not ``IndexError``. + + ``IndexError: list index out of range`` names neither the workflow nor the + contract it broke, so a reader has to reconstruct both from the traceback. + """ + no_block = "name: Example\njobs:\n build:\n runs-on: ubuntu-latest\n" + + for helper in (workflow_level_concurrency_group, workflow_level_cancels_in_progress): + with pytest.raises(AssertionError, match="no workflow-level concurrency block"): + helper(no_block) + + +def test_cancel_in_progress_assertion_rejects_a_commented_out_setting() -> None: + """The negative control for ``workflow_level_cancels_in_progress``. + + A substring test for ``cancel-in-progress: true`` is satisfied by a comment + that quotes it. On 2026-09-06 that exact mutation -- comment out the real line + in noema-review.yml, add ``cancel-in-progress: false`` beneath it -- passed the + whole suite (2958 passed, 0 failed) while every push to a pull request stopped + cancelling its own superseded run. Anchoring to the start of a block line is + what closes it. + """ + quoted_but_disabled = textwrap.dedent( + """\ + concurrency: + group: example-${{ github.repository }}-${{ github.event.pull_request.number }} + # cancel-in-progress: true + cancel-in-progress: false + """ + ) + + assert "cancel-in-progress: true" in quoted_but_disabled + assert not workflow_level_cancels_in_progress(quoted_but_disabled) + + def test_required_opencode_dispatch_does_not_wait_on_merge_scheduler() -> None: """Dispatch review execution directly so polling cannot starve its producer.""" workflow = workflow_text("opencode-review.yml") @@ -455,7 +578,7 @@ def test_required_pull_request_workflows_cancel_superseded_runs() -> None: assert "github.repository" in group_value assert "github.event.pull_request.number" in workflow assert re.search(r"(?m)^concurrency:", workflow) - assert "cancel-in-progress: true" in concurrency_contract + assert workflow_level_cancels_in_progress(workflow) if filename == "security-scan.yml": assert ( "github.event_name == 'pull_request_target'" in group_value @@ -497,12 +620,17 @@ def test_pr_quality_workflows_isolate_concurrency_by_repository_and_pr() -> None "${{ github.event.pull_request.number || github.ref }}" ) in concurrency if filename == "cloudflare-dns.yml": - assert ( - "cancel-in-progress: ${{ github.event_name == 'pull_request' }}" - in concurrency + # Anchored like the ``true`` contracts below: a commented-out setting + # must not satisfy this either, and this workflow deliberately cancels + # only for pull requests, so its value is an expression rather than a + # constant. + assert re.search( + r"(?m)^[ \t]+cancel-in-progress:[ \t]+\$\{\{ github\.event_name ==" + r" 'pull_request' \}\}[ \t]*$", + concurrency, ) else: - assert "cancel-in-progress: true" in concurrency + assert workflow_level_cancels_in_progress(workflow) def test_central_semgrep_logs_every_finding_and_distinguishes_engine_failure() -> None: @@ -832,7 +960,9 @@ def test_pull_request_close_events_cancel_superseded_runs_without_heavy_jobs() - )[0] assert "github.event.pull_request.number" in concurrency_contract assert "github.event.pull_request.head.sha" not in concurrency_contract - assert "cancel-in-progress:" in concurrency_contract + assert re.search( + r"(?m)^[ \t]+cancel-in-progress:[ \t]+\S", concurrency_contract + ) else: raise AssertionError(f"unclassified close-event workflow: {filename}") assert "github.event.action != 'closed'" in workflow From 8c6b052e59698da5427dd6ffef05ebfd8eabb8d2 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 7 Sep 2026 00:44:56 +0900 Subject: [PATCH 2/2] test(concurrency): anchor the CodeQL dispatch cancel assertion too 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 --- tests/test_codeql_scan_dispatch_workflow_contract.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/test_codeql_scan_dispatch_workflow_contract.py b/tests/test_codeql_scan_dispatch_workflow_contract.py index 2439d2936e..71fa43541f 100644 --- a/tests/test_codeql_scan_dispatch_workflow_contract.py +++ b/tests/test_codeql_scan_dispatch_workflow_contract.py @@ -20,6 +20,7 @@ from scripts.ci import audit_central_required_workflows as ruleset_audit from tests.test_opencode_workflow_shell_syntax import _extract_run_block from tests.test_required_workflow_queue_contract import ( + workflow_level_cancels_in_progress, workflow_level_concurrency_group, ) @@ -96,7 +97,6 @@ def test_codeql_scan_dispatch_workflow_structure(): def test_codeql_scan_dispatch_keeps_current_head_language_shards_independent(): """A current-head language scan cannot cancel its sibling language scans.""" workflow = WORKFLOW_PATH.read_text(encoding="utf-8") - concurrency = workflow.split("concurrency:\n", 1)[1].split("\n\npermissions:", 1)[0] group_value = workflow_level_concurrency_group(workflow) # The language segment is what keeps sibling language shards in separate groups, so it is @@ -105,7 +105,9 @@ def test_codeql_scan_dispatch_keeps_current_head_language_shards_independent(): assert "github.event.client_payload.target_repository" in group_value assert "github.event.client_payload.pr_number" in group_value assert "github.event.client_payload.required_language" in group_value - assert "cancel-in-progress: true" in concurrency + # Same reasoning as the group above, applied to the flag: the substring form + # is satisfied by a comment quoting it while the key beside it reads false. + assert workflow_level_cancels_in_progress(workflow) def _run_validate_step(tmp_path: Path, env_overrides: dict[str, str], pull_request: dict) -> subprocess.CompletedProcess[str]: