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.
This commit is contained in:
mateo-berri 2026-09-06 00:04:31 -07:00
parent 9f379b36b9
commit 5bbc83e3de
2 changed files with 319 additions and 51 deletions

View file

@ -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.<key>`, `format()`,
`==` and `!=`, and the `<cond> && <a> || <b>` idiom, which is how the shards
reach their real `<shard> / Run tests` names rather than staying opaque. A job
calling a local reusable workflow publishes one check run per job of the callee,
named `<caller> / <callee>`, 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.<key>`, `format()`, `==` and
`!=`, and the `<cond> && <a> || <b>` idiom, which is how the shards reach their
real `<shard> / 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 `<caller> / <callee>` 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<text>[^']*)'$")
FORMAT_CALL: Final = re.compile(r"^format\((?P<args>.*)\)$", re.DOTALL)
COMPARISON: Final = re.compile(r"^(?P<left>.+?)\s*(?P<operator>==|!=)\s*(?P<right>.+)$", 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())

View file

@ -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