From 5bbc83e3defa48db50546aa81ba60a9cdb45ed58 Mon Sep 17 00:00:00 2001 From: mateo-berri <277851410+mateo-berri@users.noreply.github.com> Date: Sun, 6 Sep 2026 00:04:31 -0700 Subject: [PATCH] fix: model the check-run names GitHub really publishes The collision sweep read a job's name as its `name:` or bare job id, which is wrong for a matrix job that sets no name: GitHub publishes `build (3.12)`, one per combination. That missed real duplicates and invented ones that don't exist. It also crossed every matrix value while ignoring `exclude`, so it checked combinations no job ever runs. Four smaller gaps went with it. Boolean matrix values reached a name as `True` rather than `true`. A `format()` whose arguments cannot fill its placeholders raised straight out of the script instead of leaving the name unresolved. A job calling a reusable workflow only ever chained one level, and a call outside the repo fell back to the caller's own name, which GitHub never posts. A job whose `name:` was not a string failed validation and silently dropped every job in that file, so the sweep now renders any scalar and reports a file it cannot read instead of skipping it. --- .../check_workflow_job_name_collisions.py | 174 +++++++++++----- .../test_workflow_job_name_collisions.py | 196 +++++++++++++++++- 2 files changed, 319 insertions(+), 51 deletions(-) diff --git a/tests/code_coverage_tests/check_workflow_job_name_collisions.py b/tests/code_coverage_tests/check_workflow_job_name_collisions.py index 9dc13c2ea0c..4eef3df1c11 100644 --- a/tests/code_coverage_tests/check_workflow_job_name_collisions.py +++ b/tests/code_coverage_tests/check_workflow_job_name_collisions.py @@ -11,19 +11,27 @@ clash completely, so the context quietly stops meaning what the ruleset intended 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 name carrying `${{ ... }}` publishes one check run per -combination the matrix produces, with each `include` row's values held together -rather than crossed with the other rows', so two shard lists that overlap collide -even though their templates read differently. Each expression is evaluated per combination -over the pieces a job name can hold: string literals, `matrix.`, `format()`, -`==` and `!=`, and the ` && || ` idiom, which is how the shards -reach their real ` / Run tests` names rather than staying opaque. 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. An expression nothing resolves stays in the -string, so two jobs carrying the same unresolved template still compare equal and -their collision is still caught. +A job publishes its `name:` when it sets one, and otherwise its job id plus the +values of the combination it runs, the way GitHub writes `build (3.12)`. A name +carrying `${{ ... }}` publishes one check run per combination the matrix +produces: `exclude` rows drop combinations before `include` rows fold into the +survivors, and each `include` row's values stay together rather than crossing +with the other rows', so two shard lists that overlap collide even though their +templates read differently. Each expression is evaluated per combination over the +pieces a job name can hold: string literals, `matrix.`, `format()`, `==` and +`!=`, and the ` && || ` idiom, which is how the shards reach their +real ` / Run tests` names rather than staying opaque. An expression +nothing resolves stays in the string, so two jobs carrying the same unresolved +template still compare equal and their collision is still caught. + +A job calling a local reusable workflow publishes one check run per job of the +callee, named ` / ` and chained through however many levels of +local calls it takes, which is why a caller's name never collides with a plain +job that happens to match it. A call this sweep cannot follow, to a workflow +outside the repo or to one that is not there, publishes nothing rather than a +name GitHub never posts. A file under `.github/workflows/` that does not read as +a workflow at all is reported rather than skipped, since skipping it silently +would hide every job it holds. """ import itertools @@ -31,6 +39,7 @@ import operator import re import sys from collections.abc import Iterator, Mapping, Sequence +from dataclasses import dataclass from pathlib import Path from types import MappingProxyType from typing import Final @@ -46,13 +55,19 @@ LITERAL: Final = re.compile(r"^'(?P[^']*)'$") FORMAT_CALL: Final = re.compile(r"^format\((?P.*)\)$", re.DOTALL) COMPARISON: Final = re.compile(r"^(?P.+?)\s*(?P==|!=)\s*(?P.+)$", re.DOTALL) NO_MATRIX: Final[Mapping[str, str]] = MappingProxyType({}) +NO_CALLERS: Final[frozenset[str]] = frozenset() SCALAR: Final = (str, int, float) MATRIX_DIRECTIVES: Final = frozenset({"include", "exclude"}) LOCAL_CALL_PREFIX: Final = "./" +@dataclass(frozen=True, slots=True) +class Unreadable: + reason: str + + class Job(BaseModel): - name: str | None = None + name: object = None uses: str | None = None strategy: Mapping[str, object] = Field(default_factory=dict) @@ -61,15 +76,20 @@ 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) +def scalar_text(value: object) -> str: + """A YAML scalar the way GitHub renders it, so `true` never reaches a name as `True`.""" + return str(value).lower() if isinstance(value, bool) else str(value) + + +def parse(source: str) -> tuple[Workflow, object] | Unreadable: + """The workflow plus its raw `on:` value, or why the file does not read as one.""" + parsed: Final = yaml.safe_load(source) if not isinstance(parsed, dict): - return None + return Unreadable("its top level is not a mapping of workflow keys") try: return Workflow.model_validate(parsed), parsed.get(True, parsed.get("on")) - except ValidationError: - return None + except ValidationError as error: + return Unreadable(f"{error.error_count()} of its job definitions have a shape GitHub would reject") def events(raw_on: object) -> frozenset[str]: @@ -89,23 +109,28 @@ def publishes_check_runs(raw_on: object) -> bool: def listed_values(matrix: Mapping[str, object]) -> tuple[tuple[str, tuple[str, ...]], ...]: return tuple( - (str(key), tuple(str(value) for value in values if isinstance(value, SCALAR))) + (str(key), tuple(scalar_text(value) for value in values if isinstance(value, SCALAR))) for key, values in matrix.items() if str(key) not in MATRIX_DIRECTIVES and isinstance(values, Sequence) and not isinstance(values, str) ) -def include_rows(matrix: Mapping[str, object]) -> tuple[Mapping[str, str], ...]: - rows: Final = matrix.get("include") +def directive_rows(matrix: Mapping[str, object], directive: str) -> tuple[Mapping[str, str], ...]: + rows: Final = matrix.get(directive) if not isinstance(rows, Sequence) or isinstance(rows, str): return () return tuple( - MappingProxyType({str(key): str(value) for key, value in row.items() if isinstance(value, SCALAR)}) + MappingProxyType({str(key): scalar_text(value) for key, value in row.items() if isinstance(value, SCALAR)}) for row in rows if isinstance(row, Mapping) ) +def drops(row: Mapping[str, str], combination: Mapping[str, str]) -> bool: + """GitHub removes a combination that carries every value one `exclude` row names.""" + return all(combination.get(key) == value for key, value in row.items()) + + def extends(row: Mapping[str, str], combination: Mapping[str, str]) -> bool: """GitHub folds an `include` row into a combination only where it overwrites no listed value.""" return all(combination[key] == value for key, value in row.items() if key in combination) @@ -116,23 +141,29 @@ def extended(combination: Mapping[str, str], rows: Sequence[Mapping[str, str]]) return MappingProxyType({**combination, **additions}) +def crossed_values(listed: Sequence[tuple[str, tuple[str, ...]]]) -> tuple[Mapping[str, str], ...]: + if not listed: + return () + return tuple( + MappingProxyType(dict(zip((key for key, _ in listed), values))) + for values in itertools.product(*(values for _, values in listed)) + ) + + def matrix_combinations(job: Job) -> tuple[Mapping[str, str], ...]: - """One mapping per job the matrix produces, each `include` row's values staying together.""" + """One mapping per job the matrix produces, `exclude` applied before `include` as GitHub does.""" matrix: Final = job.strategy.get("matrix") if not isinstance(matrix, Mapping): return () - listed: Final = listed_values(matrix) - rows: Final = include_rows(matrix) - crossed: Final = ( - tuple( - MappingProxyType(dict(zip((key for key, _ in listed), values))) - for values in itertools.product(*(values for _, values in listed)) - ) - if listed - else () + rows: Final = directive_rows(matrix, "include") + dropped: Final = directive_rows(matrix, "exclude") + kept: Final = tuple( + combination + for combination in crossed_values(listed_values(matrix)) + if not any(drops(row, combination) for row in dropped) ) - standalone: Final = tuple(row for row in rows if not any(extends(row, combination) for combination in crossed)) - return (*(extended(combination, rows) for combination in crossed), *standalone) + standalone: Final = tuple(row for row in rows if not any(extends(row, combination) for combination in kept)) + return (*(extended(combination, rows) for combination in kept), *standalone) def scanned(state: tuple[int, bool], char: str) -> tuple[int, bool]: @@ -156,6 +187,14 @@ def split_outside(text: str, token: str) -> tuple[str, ...]: return tuple(text[start:end] for start, end in zip(starts, (*cuts, len(text)))) +def formatted(template: str, arguments: Sequence[str]) -> str | None: + """A `format()` whose placeholders the arguments cannot fill resolves to nothing, never a crash.""" + try: + return template.format(*arguments) + except (IndexError, KeyError, ValueError): + return None + + def value_of(text: str, values: Mapping[str, str]) -> str | None: expression: Final = text.strip() literal: Final = LITERAL.match(expression) @@ -171,7 +210,7 @@ def value_of(text: str, values: Mapping[str, str]) -> str | None: resolved: Final = tuple(argument for argument in arguments if argument is not None) if not resolved or len(resolved) != len(arguments): return None - return resolved[0].format(*resolved[1:]) + return formatted(resolved[0], resolved[1:]) def holds(condition: str, values: Mapping[str, str]) -> bool | None: @@ -211,27 +250,58 @@ def expand(template: str, job: Job) -> tuple[str, ...]: return tuple(dict.fromkeys(rendered(template, values) for values in combinations)) +def suffixed(job_id: str, combination: Mapping[str, str]) -> str: + """The name GitHub gives a job with no `name:`, its id plus the combination it runs.""" + return f"{job_id} ({', '.join(combination.values())})" if combination else job_id + + +def published_names(job_id: str, job: Job) -> tuple[str, ...]: + if job.name is not None: + return expand(scalar_text(job.name), job) + suffixes: Final = tuple(dict.fromkeys(suffixed(job_id, values) for values in matrix_combinations(job))) + return suffixes or (job_id,) + + 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: +def job_names( + job_id: str, job: Job, workflows: Mapping[str, Workflow], callers: frozenset[str] = NO_CALLERS +) -> tuple[str, ...]: + prefixes: Final = published_names(job_id, job) + if job.uses is None: return prefixes + path: Final = callee_path(job) + callee: Final = workflows.get(path or "") + if path is None or callee is None or path in callers: + return () 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) + for name in job_names(callee_id, callee_job, workflows, callers | {path}) ) return tuple(f"{prefix} / {suffix}" for prefix in prefixes for suffix in suffixes) +def readable(sources: Mapping[str, str]) -> Mapping[str, tuple[Workflow, object]]: + parsed: Final = {rel: parse(source) for rel, source in sources.items()} + return MappingProxyType({rel: entry for rel, entry in parsed.items() if not isinstance(entry, Unreadable)}) + + +def unreadable(sources: Mapping[str, str]) -> tuple[str, ...]: + parsed: Final = {rel: parse(source) for rel, source in sources.items()} + return tuple( + f"{rel} sits in the workflows directory but {entry.reason}, so none of its jobs were checked." + for rel, entry in sorted(parsed.items()) + if isinstance(entry, Unreadable) + ) + + 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} + parsed: Final = readable(sources) 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): @@ -260,16 +330,26 @@ def workflow_sources() -> Mapping[str, str]: return {path.relative_to(REPO_ROOT).as_posix(): path.read_text() for path in sorted(WORKFLOWS_DIR.glob("*.y*ml"))} -def main() -> int: - sources: Final = workflow_sources() +def report(header: str, problems: Sequence[str]) -> None: + if problems: + print(f"ERROR: {header}:\n - " + "\n - ".join(problems), file=sys.stderr) + + +def exit_code(sources: Mapping[str, str]) -> int: + unread: Final = unreadable(sources) found: Final = collisions(sources) - if found: - print("ERROR: Check-run names are not unique:\n - " + "\n - ".join(found), file=sys.stderr) + report("Some workflows could not be read", unread) + report("Check-run names are not unique", found) + if unread or found: return 1 print(f"Check-run names are unique across {len(sources)} workflows") return 0 +def main() -> int: + return exit_code(workflow_sources()) + + if __name__ == "__main__": sys.exit(main()) diff --git a/tests/code_coverage_tests/test_workflow_job_name_collisions.py b/tests/code_coverage_tests/test_workflow_job_name_collisions.py index 359072326bd..3dd8a3d4088 100644 --- a/tests/code_coverage_tests/test_workflow_job_name_collisions.py +++ b/tests/code_coverage_tests/test_workflow_job_name_collisions.py @@ -1,6 +1,15 @@ from typing import Final -from check_workflow_job_name_collisions import callee_path, collisions, parse, published, workflow_sources +from check_workflow_job_name_collisions import ( + Unreadable, + callee_path, + collisions, + exit_code, + parse, + published, + unreadable, + workflow_sources, +) REUSABLE_BASE: Final = """on: workflow_call: @@ -53,6 +62,62 @@ jobs: - label: fast """ +NAMELESS_MATRIX: Final = """on: pull_request +jobs: + build: + runs-on: ubuntu-latest + strategy: + matrix: + python-version: ["3.12", "3.13"] +""" + +EXCLUDED_PAIR: Final = """on: pull_request +jobs: + unit: + name: ${{ matrix.os }}-${{ matrix.python-version }} + runs-on: ubuntu-latest + strategy: + matrix: + os: [ubuntu, macos] + python-version: ["3.12", "3.13"] + exclude: + - os: macos + python-version: "3.13" +""" + +EXCLUDED_KEY: Final = """on: pull_request +jobs: + unit: + name: ${{ matrix.os }}-${{ matrix.python-version }} + runs-on: ubuntu-latest + strategy: + matrix: + os: [ubuntu, macos] + python-version: ["3.12", "3.13"] + exclude: + - os: macos +""" + +BOOLEAN_MATRIX: Final = """on: pull_request +jobs: + unit: + name: cache ${{ matrix.cached }} + runs-on: ubuntu-latest + strategy: + matrix: + cached: [true, false] +""" + +UNFILLABLE_FORMAT: Final = """on: pull_request +jobs: + unit: + name: ${{ format('{0} {1}', matrix.shard) }} + runs-on: ubuntu-latest + strategy: + matrix: + shard: [core] +""" + def test_every_workflow_in_the_repo_publishes_a_unique_check_run_name() -> None: assert collisions(workflow_sources()) == () @@ -60,7 +125,9 @@ def test_every_workflow_in_the_repo_publishes_a_unique_check_run_name() -> None: 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 + rel + for rel, source in workflow_sources().items() + if isinstance(entry := parse(source), Unreadable) or not entry[0].jobs ) assert unparsed == () @@ -68,7 +135,7 @@ def test_every_workflow_in_the_repo_parses_into_jobs() -> None: 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) + parsed: Final = tuple(entry for text in sources.values() if not isinstance(entry := parse(text), Unreadable)) unresolved: Final = tuple( job.uses for workflow, _ in parsed @@ -249,15 +316,28 @@ def test_two_jobs_sharing_one_unresolvable_template_still_collide() -> None: assert "is published by 2 jobs" in found[0] -def test_a_file_that_is_not_a_workflow_is_ignored() -> None: +def test_a_file_that_is_not_a_workflow_is_reported_rather_than_skipped() -> None: sources: Final = { "notes.yml": "just a string\n", "a.yml": "on: pull_request\njobs:\n test:\n runs-on: ubuntu-latest\n", } + found: Final = unreadable(sources) + + assert len(found) == 1 + assert "notes.yml" in found[0] assert collisions(sources) == () +def test_a_workflow_holding_a_job_shape_github_would_reject_is_reported() -> None: + sources: Final = {"a.yml": "on: pull_request\njobs:\n test:\n uses: [not, a, string]\n"} + + found: Final = unreadable(sources) + + assert len(found) == 1 + assert "a.yml" in found[0] + + def test_a_conditional_name_expands_to_the_branch_each_matrix_value_takes() -> None: names: Final = frozenset( name for name, _ in published({".github/workflows/base.yml": REUSABLE_BASE, "unit.yml": SHARD_CALLER}) @@ -310,3 +390,111 @@ def test_every_workflow_in_the_repo_resolves_every_expression_in_its_job_names() unresolved: Final = tuple(f"{owner}: {name}" for name, owner in published(workflow_sources()) if "${{" in name) assert unresolved == () + + +def test_a_matrix_job_with_no_name_publishes_the_id_and_values_github_appends() -> None: + names: Final = frozenset(name for name, _ in published({"a.yml": NAMELESS_MATRIX})) + + assert names == frozenset({"build (3.12)", "build (3.13)"}) + + +def test_a_matrix_job_with_no_name_does_not_collide_with_a_plain_job_carrying_its_id() -> None: + sources: Final = { + "a.yml": NAMELESS_MATRIX, + "b.yml": "on: pull_request\njobs:\n build:\n runs-on: ubuntu-latest\n", + } + + assert collisions(sources) == () + + +def test_a_matrix_job_with_no_name_collides_with_the_suffixed_name_github_writes() -> None: + sources: Final = { + "a.yml": NAMELESS_MATRIX, + "b.yml": "on: pull_request\njobs:\n legacy:\n name: build (3.13)\n runs-on: ubuntu-latest\n", + } + + found: Final = collisions(sources) + + assert len(found) == 1 + assert "`build (3.13)` is published by 2 jobs" in found[0] + + +def test_an_excluded_combination_publishes_no_check_run() -> None: + names: Final = frozenset(name for name, _ in published({"unit.yml": EXCLUDED_PAIR})) + + assert names == frozenset({"ubuntu-3.12", "ubuntu-3.13", "macos-3.12"}) + + +def test_an_exclude_row_naming_one_key_drops_every_combination_carrying_it() -> None: + names: Final = frozenset(name for name, _ in published({"unit.yml": EXCLUDED_KEY})) + + assert names == frozenset({"ubuntu-3.12", "ubuntu-3.13"}) + + +def test_a_boolean_matrix_value_renders_the_way_github_writes_it() -> None: + names: Final = frozenset(name for name, _ in published({"unit.yml": BOOLEAN_MATRIX})) + + assert names == frozenset({"cache true", "cache false"}) + + +def test_a_format_call_its_arguments_cannot_fill_leaves_the_name_unresolved() -> None: + names: Final = frozenset(name for name, _ in published({"unit.yml": UNFILLABLE_FORMAT})) + + assert names == frozenset({"${{ format('{0} {1}', matrix.shard) }}"}) + + +def test_a_call_to_a_workflow_outside_the_repo_publishes_no_name() -> None: + sources: Final = { + "a.yml": "on: pull_request\njobs:\n unit:\n uses: BerriAI/other/.github/workflows/base.yml@main\n", + "b.yml": "on: pull_request\njobs:\n unit:\n runs-on: ubuntu-latest\n", + } + + assert frozenset(name for name, _ in published(sources)) == frozenset({"unit"}) + assert collisions(sources) == () + + +def test_a_chain_of_local_reusable_calls_publishes_every_level_of_the_chain() -> None: + sources: Final = { + ".github/workflows/leaf.yml": ( + "on:\n workflow_call:\njobs:\n run:\n name: Leaf\n runs-on: ubuntu-latest\n" + ), + ".github/workflows/mid.yml": ( + "on:\n workflow_call:\njobs:\n call:\n name: Mid\n uses: ./.github/workflows/leaf.yml\n" + ), + "top.yml": "on: pull_request\njobs:\n top:\n name: Top\n uses: ./.github/workflows/mid.yml\n", + } + + names: Final = frozenset(name for name, _ in published(sources)) + + assert names == frozenset({"Top / Mid / Leaf"}) + + +def test_a_job_name_that_is_not_a_string_still_publishes_the_value_github_renders() -> None: + sources: Final = { + "a.yml": "on: pull_request\njobs:\n sweep:\n name: 2024\n runs-on: ubuntu-latest\n", + "b.yml": 'on: pull_request\njobs:\n other:\n name: "2024"\n runs-on: ubuntu-latest\n', + } + + found: Final = collisions(sources) + + assert len(found) == 1 + assert "`2024` is published by 2 jobs" in found[0] + + +def test_the_check_fails_when_a_file_in_the_workflows_directory_cannot_be_read() -> None: + assert exit_code({"notes.yml": "just a string\n"}) == 1 + + +def test_the_check_fails_when_two_jobs_publish_one_check_run_name() -> None: + plain: Final = "on: pull_request\njobs:\n test:\n runs-on: ubuntu-latest\n" + + assert exit_code({"a.yml": plain, "b.yml": plain}) == 1 + + +def test_the_check_passes_when_every_file_reads_and_every_name_is_unique() -> None: + sources: Final = { + "a.yml": "on: pull_request\njobs:\n test:\n runs-on: ubuntu-latest\n", + "b.yml": "on: pull_request\njobs:\n sweep:\n runs-on: ubuntu-latest\n", + } + + assert exit_code(sources) == 0