diff --git a/.github/workflows/test-code-quality.yml b/.github/workflows/test-code-quality.yml index 36fd656c231..e6d2264fbf0 100644 --- a/.github/workflows/test-code-quality.yml +++ b/.github/workflows/test-code-quality.yml @@ -74,6 +74,12 @@ jobs: - name: check_workflow_startup_safety run: uv run --no-sync python ./tests/code_coverage_tests/check_workflow_startup_safety.py + - name: check_workflow_job_name_collisions + run: uv run --no-sync python ./tests/code_coverage_tests/check_workflow_job_name_collisions.py + + - name: test_workflow_job_name_collisions + run: uv run --no-sync pytest -q --noconftest -p no:cacheprovider -c /dev/null tests/code_coverage_tests/test_workflow_job_name_collisions.py + - name: test_e2e_changed_gate run: uv run --no-sync pytest -q --noconftest -p no:cacheprovider -c /dev/null tests/code_coverage_tests/test_e2e_changed_gate.py diff --git a/tests/code_coverage_tests/check_workflow_job_name_collisions.py b/tests/code_coverage_tests/check_workflow_job_name_collisions.py new file mode 100644 index 00000000000..42e11a3e81d --- /dev/null +++ b/tests/code_coverage_tests/check_workflow_job_name_collisions.py @@ -0,0 +1,179 @@ +"""Catch two workflow jobs that publish check runs under the same name. + +A ruleset's required status check names a check run and GitHub matches it by that +name alone. When two jobs publish the same name the required context stops +mapping to the job that proves it: the commit carries two check runs under one +name and nothing says which one the ruleset required. Both being green hides the +clash completely, so the context quietly stops meaning what the ruleset intended. + +`.github/workflows/auto-close-duplicates.yml` shipped a job id `test` while +`.github/workflows/test-mcp.yml` already published the required `test` context, +and commit ed5761daef4ae17152446d182c860630c38b7268 carried both check runs. +This invariant has to be enforced here because CI cannot enforce it on itself. + +A job publishes its `name:` when it sets one and its job id otherwise. Two shapes +expand that further. A `${{ matrix.key }}` reference publishes one check run per +value the matrix supplies, so two shard lists that overlap collide even though +their templates read differently. A job calling a local reusable workflow +publishes one check run per job of the callee, named ` / `, which +is why a caller's name never collides with a plain job that happens to match it. +A reference nothing resolves stays in the string, so two jobs carrying the same +unresolved template still compare equal and their collision is still caught. +""" + +import itertools +import operator +import re +import sys +from collections.abc import Iterator, Mapping, Sequence +from pathlib import Path +from typing import Final + +import yaml +from pydantic import BaseModel, Field, ValidationError + +REPO_ROOT: Final = Path(__file__).resolve().parent.parent.parent +WORKFLOWS_DIR: Final = REPO_ROOT / ".github" / "workflows" +MATRIX_REF: Final = re.compile(r"\$\{\{\s*matrix\.(?P[\w-]+)\s*\}\}") +LOCAL_CALL_PREFIX: Final = "./" + + +class CheckRunNameCollision(Exception): + pass + + +class Job(BaseModel): + name: str | None = None + uses: str | None = None + strategy: Mapping[str, object] = Field(default_factory=dict) + + +class Workflow(BaseModel): + jobs: Mapping[str, Job] = Field(default_factory=dict) + + +def parse(text: str) -> tuple[Workflow, object] | None: + """The workflow plus its raw `on:` value, or None when the file is not a workflow.""" + parsed: Final = yaml.safe_load(text) + if not isinstance(parsed, dict): + return None + try: + return Workflow.model_validate(parsed), parsed.get(True, parsed.get("on")) + except ValidationError: + return None + + +def events(raw_on: object) -> frozenset[str]: + if isinstance(raw_on, Mapping): + return frozenset(str(key) for key in raw_on) + if isinstance(raw_on, str): + return frozenset({raw_on}) + if isinstance(raw_on, Sequence): + return frozenset(str(event) for event in raw_on) + return frozenset() + + +def publishes_check_runs(raw_on: object) -> bool: + """A `workflow_call`-only workflow posts its check runs through callers, never itself.""" + return events(raw_on) != frozenset({"workflow_call"}) + + +def matrix_values(job: Job, key: str) -> tuple[str, ...]: + matrix: Final = job.strategy.get("matrix") + if not isinstance(matrix, Mapping): + return () + listed: Final = matrix.get(key) + rows: Final = matrix.get("include") + from_list: Final = ( + tuple(str(value) for value in listed if isinstance(value, (str, int, float))) + if isinstance(listed, Sequence) and not isinstance(listed, str) + else () + ) + from_rows: Final = ( + tuple(str(row[key]) for row in rows if isinstance(row, Mapping) and isinstance(row.get(key), (str, int, float))) + if isinstance(rows, Sequence) and not isinstance(rows, str) + else () + ) + return tuple(dict.fromkeys(from_list + from_rows)) + + +def substituted(template: str, values: Mapping[str, str]) -> str: + return MATRIX_REF.sub(lambda ref: values.get(ref.group("key"), ref.group(0)), template) + + +def expand(template: str, job: Job) -> tuple[str, ...]: + keys: Final = tuple(dict.fromkeys(ref.group("key") for ref in MATRIX_REF.finditer(template))) + candidates: Final = tuple((key, matrix_values(job, key)) for key in keys) + resolvable: Final = tuple((key, values) for key, values in candidates if values) + if not resolvable: + return (template,) + return tuple( + substituted(template, dict(zip((key for key, _ in resolvable), combination))) + for combination in itertools.product(*(values for _, values in resolvable)) + ) + + +def callee_path(job: Job) -> str | None: + if job.uses is None or not job.uses.startswith(LOCAL_CALL_PREFIX): + return None + return job.uses[len(LOCAL_CALL_PREFIX) :].split("@")[0] + + +def job_names(job_id: str, job: Job, workflows: Mapping[str, Workflow]) -> tuple[str, ...]: + prefixes: Final = expand(job.name or job_id, job) + callee: Final = workflows.get(callee_path(job) or "") + if callee is None: + return prefixes + suffixes: Final = tuple( + name + for callee_id, callee_job in callee.jobs.items() + for name in expand(callee_job.name or callee_id, callee_job) + ) + return tuple(f"{prefix} / {suffix}" for prefix in prefixes for suffix in suffixes) + + +def published(sources: Mapping[str, str]) -> Iterator[tuple[str, str]]: + parsed: Final = {rel: entry for rel, text in sources.items() if (entry := parse(text)) is not None} + workflows: Final = {rel: workflow for rel, (workflow, _) in parsed.items()} + for rel, (workflow, raw_on) in parsed.items(): + if not publishes_check_runs(raw_on): + continue + for job_id, job in workflow.jobs.items(): + for name in job_names(job_id, job, workflows): + yield name, f"{rel} job `{job_id}`" + + +def owners_by_name(sources: Mapping[str, str]) -> Iterator[tuple[str, tuple[str, ...]]]: + for name, pairs in itertools.groupby(sorted(published(sources)), key=operator.itemgetter(0)): + yield name, tuple(owner for _, owner in pairs) + + +def collisions(sources: Mapping[str, str]) -> tuple[str, ...]: + return tuple( + f"`{name}` is published by {len(owners)} jobs: {', '.join(owners)}. A required status check matching " + f"that name cannot say which job proves it; give one of them a distinct `name:` or job id." + for name, owners in owners_by_name(sources) + if len(owners) > 1 + ) + + +def workflow_sources() -> Mapping[str, str]: + """Repo-relative posix paths to text, the keys `uses: ./...` resolves against.""" + return {path.relative_to(REPO_ROOT).as_posix(): path.read_text() for path in sorted(WORKFLOWS_DIR.glob("*.y*ml"))} + + +def main() -> None: + sources: Final = workflow_sources() + found: Final = collisions(sources) + if found: + raise CheckRunNameCollision("Check-run names are not unique:\n - " + "\n - ".join(found)) + + print(f"Check-run names are unique across {len(sources)} workflows") + + +if __name__ == "__main__": + try: + main() + except CheckRunNameCollision as exc: + print(f"ERROR: {exc}", file=sys.stderr) + sys.exit(1) diff --git a/tests/code_coverage_tests/test_workflow_job_name_collisions.py b/tests/code_coverage_tests/test_workflow_job_name_collisions.py new file mode 100644 index 00000000000..35c1480ecd8 --- /dev/null +++ b/tests/code_coverage_tests/test_workflow_job_name_collisions.py @@ -0,0 +1,211 @@ +import sys +from pathlib import Path +from typing import Final + +sys.path.insert(0, str(Path(__file__).resolve().parent)) + +from check_workflow_job_name_collisions import callee_path, collisions, parse, workflow_sources # noqa: E402 + + +def test_every_workflow_in_the_repo_publishes_a_unique_check_run_name() -> None: + assert collisions(workflow_sources()) == () + + +def test_every_workflow_in_the_repo_parses_into_jobs() -> None: + unparsed: Final = tuple( + rel for rel, source in workflow_sources().items() if (entry := parse(source)) is None or not entry[0].jobs + ) + + assert unparsed == () + + +def test_every_local_reusable_call_in_the_repo_resolves_to_a_workflow() -> None: + sources: Final = workflow_sources() + parsed: Final = tuple(entry for text in sources.values() if (entry := parse(text)) is not None) + unresolved: Final = tuple( + job.uses + for workflow, _ in parsed + for job in workflow.jobs.values() + if (callee := callee_path(job)) is not None and callee not in sources + ) + + assert unresolved == () + + +def test_two_jobs_falling_back_to_the_same_job_id_collide() -> None: + sources: Final = { + "a.yml": "on: pull_request\njobs:\n test:\n runs-on: ubuntu-latest\n", + "b.yml": "on: pull_request\njobs:\n test:\n runs-on: ubuntu-latest\n", + } + + found: Final = collisions(sources) + + assert len(found) == 1 + assert "`test` is published by 2 jobs" in found[0] + assert "a.yml job `test`" in found[0] and "b.yml job `test`" in found[0] + + +def test_an_explicit_name_overrides_the_job_id_and_clears_the_collision() -> None: + sources: Final = { + "a.yml": "on: pull_request\njobs:\n test:\n name: Sweep tests\n runs-on: ubuntu-latest\n", + "b.yml": "on: pull_request\njobs:\n test:\n runs-on: ubuntu-latest\n", + } + + assert collisions(sources) == () + + +def test_an_explicit_name_matching_another_job_id_collides() -> None: + sources: Final = { + "a.yml": "on: pull_request\njobs:\n sweep:\n name: test\n runs-on: ubuntu-latest\n", + "b.yml": "on: pull_request\njobs:\n test:\n runs-on: ubuntu-latest\n", + } + + found: Final = collisions(sources) + + assert len(found) == 1 + assert "`test` is published by 2 jobs" in found[0] + + +def test_two_callers_of_one_reusable_workflow_collide_on_a_shared_matrix_value() -> None: + base: Final = "on:\n workflow_call:\njobs:\n run:\n name: Run tests\n runs-on: ubuntu-latest\n" + caller: Final = ( + "on: pull_request\n" + "jobs:\n" + " {job}:\n" + " name: ${{{{ matrix.shard }}}}\n" + " uses: ./.github/workflows/base.yml\n" + " strategy:\n" + " matrix:\n" + " include:\n" + " - shard: {shard}\n" + ) + sources: Final = { + ".github/workflows/base.yml": base, + "unit.yml": caller.format(job="unit", shard="proxy-auth"), + "proxy-db.yml": caller.format(job="proxy-db", shard="proxy-auth"), + } + + found: Final = collisions(sources) + + assert len(found) == 1 + assert "`proxy-auth / Run tests` is published by 2 jobs" in found[0] + + +def test_distinct_matrix_values_through_one_reusable_workflow_do_not_collide() -> None: + base: Final = "on:\n workflow_call:\njobs:\n run:\n name: Run tests\n runs-on: ubuntu-latest\n" + caller: Final = ( + "on: pull_request\n" + "jobs:\n" + " {job}:\n" + " name: ${{{{ matrix.shard }}}}\n" + " uses: ./.github/workflows/base.yml\n" + " strategy:\n" + " matrix:\n" + " include:\n" + " - shard: {shard}\n" + ) + sources: Final = { + ".github/workflows/base.yml": base, + "unit.yml": caller.format(job="unit", shard="proxy-auth"), + "proxy-db.yml": caller.format(job="proxy-db", shard="budgets"), + } + + assert collisions(sources) == () + + +def test_a_reusable_caller_does_not_collide_with_a_plain_job_of_the_same_name() -> None: + sources: Final = { + ".github/workflows/base.yml": ( + "on:\n workflow_call:\njobs:\n run:\n name: Run tests\n runs-on: ubuntu-latest\n" + ), + "unit.yml": ( + "on: pull_request\n" + "jobs:\n" + " unit:\n" + " name: ${{ matrix.shard }}\n" + " uses: ./.github/workflows/base.yml\n" + " strategy:\n" + " matrix:\n" + " include:\n" + " - shard: proxy-behavior\n" + ), + "postgres.yml": ( + "on: pull_request\n" + "jobs:\n" + " postgres:\n" + " name: ${{ matrix.shard }}\n" + " runs-on: ubuntu-latest\n" + " strategy:\n" + " matrix:\n" + " include:\n" + " - shard: proxy-behavior\n" + ), + } + + assert collisions(sources) == () + + +def test_a_workflow_call_only_workflow_publishes_nothing_of_its_own() -> None: + sources: Final = { + "base.yml": "on:\n workflow_call:\njobs:\n run:\n runs-on: ubuntu-latest\n", + "other.yml": "on:\n workflow_call:\njobs:\n run:\n runs-on: ubuntu-latest\n", + } + + assert collisions(sources) == () + + +def test_a_workflow_call_workflow_that_also_runs_on_pull_request_still_publishes() -> None: + sources: Final = { + "base.yml": "on:\n workflow_call:\n pull_request:\njobs:\n run:\n runs-on: ubuntu-latest\n", + "other.yml": "on: pull_request\njobs:\n run:\n runs-on: ubuntu-latest\n", + } + + found: Final = collisions(sources) + + assert len(found) == 1 + assert "`run` is published by 2 jobs" in found[0] + + +def test_a_matrix_list_supplies_values_the_same_way_include_rows_do() -> None: + sources: Final = { + "a.yml": ( + "on: pull_request\n" + "jobs:\n" + " build:\n" + " name: Analyze (${{ matrix.language }})\n" + " runs-on: ubuntu-latest\n" + " strategy:\n" + " matrix:\n" + " language: [python, go]\n" + ), + "b.yml": "on: pull_request\njobs:\n go:\n name: Analyze (go)\n runs-on: ubuntu-latest\n", + } + + found: Final = collisions(sources) + + assert len(found) == 1 + assert "`Analyze (go)` is published by 2 jobs" in found[0] + + +def test_two_jobs_sharing_one_unresolvable_template_still_collide() -> None: + template: Final = ( + "on: pull_request\njobs:\n {job}:\n name: ${{{{ matrix.shard }}}}\n runs-on: ubuntu-latest\n" + ) + sources: Final = { + "a.yml": template.format(job="one"), + "b.yml": template.format(job="two"), + } + + found: Final = collisions(sources) + + assert len(found) == 1 + assert "is published by 2 jobs" in found[0] + + +def test_a_file_that_is_not_a_workflow_is_ignored() -> None: + sources: Final = { + "notes.yml": "just a string\n", + "a.yml": "on: pull_request\njobs:\n test:\n runs-on: ubuntu-latest\n", + } + + assert collisions(sources) == ()