From 6f9c9e0d838f0b0c31d989c4e331fe8543c3b3cd Mon Sep 17 00:00:00 2001 From: Kartikey1306 Date: Tue, 1 Sep 2026 21:06:30 +0530 Subject: [PATCH 1/6] ci: add one job branch protection can require #87 and eos#92 both stay open on the same point: `required_status_checks` is null on master, so nothing builds the merge result before it becomes master. Marking a check required is a settings change, but there is no name here worth pointing it at. `test` is a 3x3 matrix, so it reports as nine generated names that change whenever the matrix does, and a required name that stops appearing blocks every pull request until someone edits the protection rule. `release` is skipped on every pull request, and a skipped required check never reports at all. `ci-gate` is one job, one stable name, that succeeds only if every other job that runs on a pull request succeeded. It carries `if: always()` -- without it the gate is skipped when an earlier job fails, and the pull request would wait for a status that never arrives rather than showing a red X. Any non-success result fails it, `skipped` included: a job that did not run did not verify anything. tests/unit/test_ci_gate.py fails if a job is added to the workflow without being wired into the gate, which is the way this would rot. Verified by mutation -- dropping a job from `needs`, removing `if: always()`, renaming the gate, adding an unwired job, weakening the result check to `== "failure"`, and deleting the gate are each caught. 567 tests pass. --- .github/workflows/ci.yml | 40 ++++++++++++++ tests/unit/test_ci_gate.py | 108 +++++++++++++++++++++++++++++++++++++ 2 files changed, 148 insertions(+) create mode 100644 tests/unit/test_ci_gate.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 89bfb6c..13901d9 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -137,3 +137,43 @@ jobs: with: files: dist/* generate_release_notes: true + + # ── Required check ──────────────────────────────────────────────────────── + # One job that succeeds only if every other job in this workflow did, so + # branch protection has a single stable name to require. #87 asks for a + # required check on master; this is the name to point it at. + # + # Requiring the jobs themselves does not work here. `test` is a 3x3 matrix, + # so its checks arrive as nine generated names that change whenever the + # matrix does, and a required name that no longer appears blocks every PR + # until someone edits the branch protection. `release` is skipped on every + # pull request, and a required check that is skipped never reports. + # + # `if: always()` matters: without it the gate is skipped whenever an earlier + # job fails, and a skipped required check leaves the PR waiting for a status + # that will never arrive -- the failure would present as a hang, not a red X. + # + # A non-success result of any kind fails the gate, `skipped` included. A job + # that did not run did not verify anything, so treating it as a pass is the + # same fail-open shape this repository has been removing elsewhere. + ci-gate: + name: CI Gate + runs-on: ubuntu-22.04 + needs: [test, build] + if: always() + steps: + - name: Every job in this workflow must have succeeded + env: + RESULTS: ${{ toJSON(needs) }} + run: | + printf '%s\n' "$RESULTS" + bad=$(printf '%s' "$RESULTS" | jq -r ' + to_entries[] + | select(.value.result != "success") + | " \(.key): \(.value.result)"') + if [ -n "$bad" ]; then + echo "::error::CI Gate failed. These jobs did not succeed:" + printf '%s\n' "$bad" + exit 1 + fi + echo "All jobs succeeded." diff --git a/tests/unit/test_ci_gate.py b/tests/unit/test_ci_gate.py new file mode 100644 index 0000000..aeff6b3 --- /dev/null +++ b/tests/unit/test_ci_gate.py @@ -0,0 +1,108 @@ +"""The CI workflow must expose one job that summarises all the others. + +#87 asks for a required status check on `master`. Branch protection can only +require a check by name, and the names this workflow produces are not usable +for that directly: `test` is a matrix, so it arrives as nine generated names +that change with the matrix, and `release` is skipped on every pull request. + +So `ci-gate` exists to be the one name to require. These tests keep it honest +-- specifically, they fail if someone adds a job to the workflow and does not +wire it into the gate, which would otherwise silently create a job that the +required check does not cover. +""" + +import yaml +import pytest +from pathlib import Path + + +WORKFLOW = Path(__file__).resolve().parents[2] / ".github" / "workflows" / "ci.yml" + +# The name branch protection is pointed at. Changing it silently un-requires +# the check, so it is pinned here rather than merely read. +GATE_ID = "ci-gate" +GATE_NAME = "CI Gate" + + +@pytest.fixture(scope="module") +def workflow(): + assert WORKFLOW.is_file(), f"{WORKFLOW} does not exist" + return yaml.safe_load(WORKFLOW.read_text(encoding="utf-8")) + + +@pytest.fixture(scope="module") +def jobs(workflow): + return workflow["jobs"] + + +def _only_runs_on_tags(job): + """Is this job gated to tag builds, and therefore skipped on every PR?""" + condition = str(job.get("if", "")) + return "refs/tags" in condition + + +def test_gate_job_exists(jobs): + assert GATE_ID in jobs, ( + f"no {GATE_ID!r} job; branch protection has no single name to require" + ) + + +def test_gate_display_name_is_pinned(jobs): + assert jobs[GATE_ID]["name"] == GATE_NAME, ( + "the gate's display name is what branch protection matches on; " + "renaming it un-requires the check without failing anything" + ) + + +def test_gate_runs_even_when_an_earlier_job_fails(jobs): + condition = str(jobs[GATE_ID].get("if", "")).strip() + assert condition == "always()", ( + "the gate needs `if: always()`. Without it the gate is skipped when an " + "earlier job fails, and a skipped required check never reports -- the " + "pull request waits for a status that never arrives instead of showing " + "a failure" + ) + + +def test_gate_covers_every_job_that_runs_on_a_pull_request(jobs): + expected = { + name for name, job in jobs.items() + if name != GATE_ID and not _only_runs_on_tags(job) + } + declared = set(jobs[GATE_ID].get("needs", [])) + + missing = expected - declared + assert not missing, ( + f"these jobs run on pull requests but the gate does not wait for them: " + f"{sorted(missing)}. A job outside the gate is a job the required " + f"check does not cover." + ) + + unknown = declared - set(jobs) + assert not unknown, f"the gate needs jobs that do not exist: {sorted(unknown)}" + + +def test_jobs_left_out_of_the_gate_are_genuinely_tag_only(jobs): + """Excluding a job from the gate must be justified, not just convenient.""" + declared = set(jobs[GATE_ID].get("needs", [])) + for name, job in jobs.items(): + if name == GATE_ID or name in declared: + continue + assert _only_runs_on_tags(job), ( + f"job {name!r} is not in the gate and is not tag-only; either add " + f"it to `needs` or give it an `if:` that explains why it cannot run " + f"on a pull request" + ) + + +def test_gate_fails_on_any_non_success_result(jobs): + """A skipped or cancelled job must fail the gate, not pass it.""" + steps = jobs[GATE_ID]["steps"] + script = "\n".join(str(s.get("run", "")) for s in steps) + + assert '!= "success"' in script, ( + "the gate must require success specifically. Checking only for " + "'failure' lets a skipped or cancelled job through, which is the " + "fail-open shape this repository has been removing elsewhere" + ) + assert "exit 1" in script, "the gate must actually fail the job" From 06e5b9f07056394f8da644cae23b60180a4a8bff Mon Sep 17 00:00:00 2001 From: kartikey1306 Date: Thu, 3 Sep 2026 15:52:57 +0530 Subject: [PATCH 2/6] ci: make the gate's coverage checkable, not just its wiring Answers findings 1 and 2 from the review on #103. Finding 2 (Medium) -- `CI Gate` gates ci.yml and nothing else, while the PR body's closing instruction ("Settings -> Branches -> master -> Require status checks -> add `CI Gate`. One name") read as the complete fix for #87. It is the complete fix for ci.yml. Six other workflows produce pull-request checks here, and the rot-guard was hardcoded to ci.yml so it could not see any of them: a job added to codeql.yml or simulation-test.yml was uncovered and nothing failed. Adds two tests over every workflow with a `pull_request` trigger: - test_every_pull_request_workflow_is_accounted_for -- each must be in REQUIRED_CHECKS (gated, with the display name a maintainer requires) or in NO_GATE with a reason about the workflow itself. "It has no gate yet" is explicitly not a reason, since that is the gap being made visible. - test_gated_workflows_really_have_their_gate -- every name in REQUIRED_CHECKS resolves to a job that exists and displays under that name, so the required set cannot point at a status that never arrives. REQUIRED_CHECKS is the answer to "what does a maintainer actually require", which one name was never going to be. The six NO_GATE entries carry their reasons; simulation-test.yml's says plainly that its gate still fails open on part of its `needs`, which is why requiring it today would assert more than it checks. Finding 1 (High), the run log, is answered on the thread: the log in my 2026-09-01 comment is from eBoot, not from this repository -- those are eBoot's job names and `build-arm` is a key `jq` could never produce here. I posted the same comment on three gate PRs without adjusting it. Correction posted; re-running against this workflow for the real log. Verified: pytest tests/unit/test_ci_gate.py 8 passed pytest tests/ 1 failed, 565 passed, 3 skipped discrimination: dropping an ungated pull_request workflow into .github/workflows/ gives AssertionError: these workflows produce pull-request checks but are neither gated nor excused: ['zz-probe.yml'] so the guard catches the class it was written for. The one failure is PRE-EXISTING and not caused by this change -- confirmed by checking out origin/master and reproducing it there: test_build_dir_resolution.py::test_end_to_end_build_from_outside_produces_the_binary No module named ninja It is the `[sys.executable, "-m", "ninja"]` invocation @srpatcha raised on #66: the build ignores a perfectly good `ninja` on PATH and fails when the PyPI wheel is absent. CI installs that wheel, so CI does not see it. Their offer to open the `shutil.which("ninja")` follow-up is still open and still worth taking. Refs #103, #87, #66 --- tests/unit/test_ci_gate.py | 95 +++++++++++++++++++++++++++++++++++++- 1 file changed, 94 insertions(+), 1 deletion(-) diff --git a/tests/unit/test_ci_gate.py b/tests/unit/test_ci_gate.py index aeff6b3..7c09420 100644 --- a/tests/unit/test_ci_gate.py +++ b/tests/unit/test_ci_gate.py @@ -16,13 +16,71 @@ from pathlib import Path -WORKFLOW = Path(__file__).resolve().parents[2] / ".github" / "workflows" / "ci.yml" +WORKFLOWS_DIR = Path(__file__).resolve().parents[2] / ".github" / "workflows" +WORKFLOW = WORKFLOWS_DIR / "ci.yml" # The name branch protection is pointed at. Changing it silently un-requires # the check, so it is pinned here rather than merely read. GATE_ID = "ci-gate" GATE_NAME = "CI Gate" +#: Every workflow that produces a pull-request status, and the display name a +#: maintainer must require for it. `CI Gate` covers ci.yml and nothing else -- +#: cross-workflow `needs` is not something GitHub offers -- so the full +#: required set is this mapping, not one name. +#: +#: A workflow may sit in NO_GATE only with a reason about the workflow itself. +#: "It has no gate yet" is not one: that is the gap this table exists to make +#: visible. +REQUIRED_CHECKS = { + "ci.yml": GATE_NAME, +} + +NO_GATE = { + "auto-assign.yml": + "assigns a reviewer; it verifies nothing, so requiring it would block " + "merges on a housekeeping step", + "claude-code-review.yml": + "posts advisory review comments and never fails on content", + "codeql.yml": + "already reports a single stable name, `CodeQL`, which should be " + "required directly rather than wrapped in a gate", + "book-build.yml": + "builds documentation; a docs failure should not block a code merge, " + "and this is a deliberate policy choice rather than an oversight", + "simulation-test.yml": + "has a gate job whose body still fails open on part of its `needs` -- " + "tracked separately; requiring it today would assert more than it " + "checks", + "vendor-drift.yml": + "reports third-party drift for triage and is expected to fail while a " + "vendored dependency is behind", +} + + +def _load(path): + return yaml.safe_load(path.read_text(encoding="utf-8")) + + +def _runs_on_pull_request(doc): + # PyYAML parses a bare `on:` key as the boolean True. + triggers = doc.get("on", doc.get(True, {})) + if isinstance(triggers, dict): + return "pull_request" in triggers + if isinstance(triggers, list): + return "pull_request" in triggers + return triggers == "pull_request" + + +def _pr_workflows(): + found = {} + for path in sorted(WORKFLOWS_DIR.glob("*.yml")): + doc = _load(path) + if isinstance(doc, dict) and _runs_on_pull_request(doc): + found[path.name] = doc + assert found, f"no pull-request workflows found under {WORKFLOWS_DIR}" + return found + @pytest.fixture(scope="module") def workflow(): @@ -95,6 +153,41 @@ def test_jobs_left_out_of_the_gate_are_genuinely_tag_only(jobs): ) +def test_every_pull_request_workflow_is_accounted_for(): + """A new workflow must be gated or explicitly excused, not silently added. + + `CI Gate` summarises ci.yml only. Requiring that one name -- which is what + this PR's own description asked a maintainer to do -- leaves every other + workflow's checks unrequired, and the rot-guard above cannot see them + either. This is the test that notices. + """ + unaccounted = [ + name for name in _pr_workflows() + if name not in REQUIRED_CHECKS and name not in NO_GATE + ] + assert not unaccounted, ( + f"these workflows produce pull-request checks but are neither gated " + f"nor excused: {sorted(unaccounted)}. Add a gate job and list it in " + f"REQUIRED_CHECKS, or add it to NO_GATE with the reason." + ) + + +def test_gated_workflows_really_have_their_gate(): + """Each entry in REQUIRED_CHECKS names a job that exists and is a gate.""" + workflows = _pr_workflows() + for filename, display in REQUIRED_CHECKS.items(): + assert filename in workflows, ( + f"{filename} is in REQUIRED_CHECKS but produces no pull-request " + f"checks; the required set names a check that never reports" + ) + jobs = workflows[filename]["jobs"] + matching = [j for j in jobs.values() if j.get("name") == display] + assert matching, ( + f"{filename} has no job displaying as {display!r}, so branch " + f"protection would wait forever for a status that never arrives" + ) + + def test_gate_fails_on_any_non_success_result(jobs): """A skipped or cancelled job must fail the gate, not pass it.""" steps = jobs[GATE_ID]["steps"] From 715e7a9ba88494681d4f6bf9f3807561457cb25e Mon Sep 17 00:00:00 2001 From: Kartikey1306 Date: Tue, 1 Sep 2026 21:18:35 +0530 Subject: [PATCH 3/6] ci: give each matrix leg its own check name The test job's `name:` interpolated the Python version but not the OS, so all three legs of each version reported under one name. Measured on #103's head: $ gh api repos/embeddedos-org/ebuild/commits//check-runs \ --jq '.check_runs[].name' | sort | uniq -c 3 Test (Python 3.12) 3 Test (Python 3.11) 3 Test (Python 3.10) Nine check runs, three names. Two consequences, one for people and one for branch protection: A Windows-only failure is indistinguishable from the ubuntu and macOS legs in the checks list -- all three rows read "Test (Python 3.10)" and you have to open them to find which failed. That is not hypothetical here: the Windows leg did not run at all until `shell: bash` was added, and once it did it surfaced failures the other two legs never saw. And a required status check names a check. With three check runs behind one name, a rule naming it cannot say which leg it means. Adding `${{ matrix.os }}` makes all nine names distinct. Doing it now, before #103's `CI Gate` gives anyone a reason to start pinning check names: `required_status_checks` is null today, so nothing anywhere refers to these names -- `grep` finds the template itself and nothing else. Renaming nine checks is cheap exactly while no rule depends on them, and expensive afterwards. Two tests added to tests/unit/test_ci_gate.py: check names must be unique across the whole workflow, and a matrix job must name every dimension it varies. Verified by mutation -- reverting the template to its previous form, adding a third matrix dimension without naming it, and removing `os` from the name while leaving it in the matrix are each caught (2 failed of 8 in every case). 569 tests pass. --- .github/workflows/ci.yml | 8 ++++- tests/unit/test_ci_gate.py | 73 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 13901d9..cde5a10 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -19,7 +19,13 @@ concurrency: jobs: # ── Test Matrix ─────────────────────────────────────────────────────────── test: - name: Test (Python ${{ matrix.python-version }}) + # Both matrix dimensions belong in the name. With only the Python + # version here, the ubuntu, macOS and Windows legs all reported under + # one check name -- three check runs called "Test (Python 3.10)" -- + # so a Windows-only failure was indistinguishable from the other two + # until you opened it, and no branch protection rule could name one + # leg without naming all three. + name: Test (Python ${{ matrix.python-version }}, ${{ matrix.os }}) runs-on: ${{ matrix.os }} strategy: matrix: diff --git a/tests/unit/test_ci_gate.py b/tests/unit/test_ci_gate.py index 7c09420..c802414 100644 --- a/tests/unit/test_ci_gate.py +++ b/tests/unit/test_ci_gate.py @@ -199,3 +199,76 @@ def test_gate_fails_on_any_non_success_result(jobs): "fail-open shape this repository has been removing elsewhere" ) assert "exit 1" in script, "the gate must actually fail the job" + + +# ── Check names must be distinct ───────────────────────────────────────────── +# +# Branch protection addresses a check by name, and so does anyone reading the +# checks list on a pull request. A matrix job whose `name:` does not mention +# every dimension produces several check runs sharing one name: this workflow +# reported three runs called "Test (Python 3.10)" until the `os` dimension was +# added to the template. That is not cosmetic -- a required rule naming that +# check cannot say which of the three it means, and a Windows-only failure is +# indistinguishable from the other two legs without opening the run. + +import itertools +import re + +# `include` and `exclude` shape a matrix but are not dimensions of it, so they +# are not part of the cartesian product. +_NOT_A_DIMENSION = {"include", "exclude"} + + +def _expanded_names(job_name, matrix): + """Every check name this job produces, one per matrix combination.""" + dimensions = {k: v for k, v in matrix.items() + if k not in _NOT_A_DIMENSION and isinstance(v, list)} + if not dimensions: + return [job_name] + + keys = list(dimensions) + names = [] + for combo in itertools.product(*(dimensions[k] for k in keys)): + name = job_name + for key, value in zip(keys, combo): + name = name.replace("${{ matrix.%s }}" % key, str(value)) + name = name.replace("${{matrix.%s}}" % key, str(value)) + names.append(name) + return names + + +def _all_check_names(jobs): + names = [] + for job_id, job in jobs.items(): + job_name = job.get("name", job_id) + matrix = job.get("strategy", {}).get("matrix", {}) + names.extend(_expanded_names(job_name, matrix or {})) + return names + + +def test_every_check_name_is_unique(jobs): + names = _all_check_names(jobs) + duplicates = sorted({n for n in names if names.count(n) > 1}) + assert not duplicates, ( + f"these check names are produced more than once: {duplicates}. " + f"A name that maps to several check runs cannot be required, and cannot " + f"be read -- add the missing matrix dimension to the job's `name:`." + ) + + +def test_a_matrix_job_names_every_dimension_it_varies(jobs): + """The rule behind the test above, stated where it will be read.""" + for job_id, job in jobs.items(): + matrix = job.get("strategy", {}).get("matrix", {}) or {} + dimensions = [k for k, v in matrix.items() + if k not in _NOT_A_DIMENSION + and isinstance(v, list) and len(v) > 1] + if not dimensions: + continue + name = job.get("name", job_id) + missing = [k for k in dimensions + if not re.search(r"\$\{\{\s*matrix\.%s\s*\}\}" % re.escape(k), name)] + assert not missing, ( + f"job {job_id!r} varies {missing} but its `name:` does not mention " + f"them, so its legs share a check name" + ) From 9382536f73aad9cd9757c09aa118b17c91df0c89 Mon Sep 17 00:00:00 2001 From: kartikey1306 Date: Thu, 3 Sep 2026 15:55:25 +0530 Subject: [PATCH 4/6] ci: model include/exclude in the matrix guard, and name both dimensions in the flag too Answers the review on #104. Finding 1 (Medium) -- _expanded_names() built the cartesian product of list dimensions and skipped `include`/`exclude` entirely. The comment justifying that was right as far as it went, but the consequence was that a matrix expressed *entirely* through `include:` has no list dimensions, so the function returned one name for however many legs the job really has. The reviewer's probe -- a `lint` job with a two-entry `include:` and a static `name: Lint` -- passed the whole suite, which is the exact defect this PR exists to catch, in the form the guard could not see. Not hypothetical: release.yml:67 and eosim-sanity.yml:83 are both `matrix: include:` with no list dimensions today, so the blind spot goes live as soon as the guard is pointed at them -- which #103's REQUIRED_CHECKS table now makes a natural next step. `include` entries that refine an existing combination update it; entries that introduce new values add a leg. `exclude` removes combinations rather than being ignored, which also closes the mirror problem the finding notes -- a combination that never runs could otherwise be reported as a duplicate. Finding 2 (Low) -- the Codecov flag four lines below the name template had the identical defect: `ebuild-py${{ matrix.python-version }}` interpolates the Python version and not the OS, so after this PR nine legs reported under nine check names and three coverage flags. Fixed rather than commented, since the PR's own stated rule ("a matrix job names every dimension it varies") covers it and merging three OS legs into one flag was the leftover of the bug, not a decision. Finding 3 (Low) is a PR-body correction and is handled there: the "nothing refers to these names" grep covered three extensions in one repository and cannot support the word *nothing*. `required_status_checks` being null is the load-bearing half, and that half is solid. Verified: pytest tests/unit/test_ci_gate.py 10 passed pytest tests/ 1 failed, 567 passed, 3 skipped discrimination: with an include:-only `lint` job wired into the gate, AssertionError: these check names are produced more than once: ['Lint'] from test_every_check_name_is_unique. Before this commit the same probe passed. The one failure is PRE-EXISTING on origin/master and unrelated -- the `python -m ninja` invocation from #66's open follow-up; see the note on #103's commit. Refs #104 --- .github/workflows/ci.yml | 5 +++- tests/unit/test_ci_gate.py | 60 +++++++++++++++++++++++++++++++------- 2 files changed, 53 insertions(+), 12 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index cde5a10..d6e2f0b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -100,7 +100,10 @@ jobs: uses: codecov/codecov-action@v4 with: files: coverage.xml - flags: ebuild-py${{ matrix.python-version }} + # Names both dimensions, for the same reason the job above does: + # nine legs reporting under three flags loses the OS split in + # coverage exactly as it was lost in the checks list. + flags: ebuild-py${{ matrix.python-version }}-${{ matrix.os }} # ── Build Python Package ────────────────────────────────────────────────── build: diff --git a/tests/unit/test_ci_gate.py b/tests/unit/test_ci_gate.py index c802414..ed82503 100644 --- a/tests/unit/test_ci_gate.py +++ b/tests/unit/test_ci_gate.py @@ -219,22 +219,60 @@ def test_gate_fails_on_any_non_success_result(jobs): _NOT_A_DIMENSION = {"include", "exclude"} +def _substitute(name, values): + for key, value in values.items(): + name = name.replace("${{ matrix.%s }}" % key, str(value)) + name = name.replace("${{matrix.%s}}" % key, str(value)) + return name + + +def _matches(combo, spec): + """Does this combination match an `exclude:` entry?""" + return all(str(combo.get(k)) == str(v) for k, v in spec.items()) + + def _expanded_names(job_name, matrix): - """Every check name this job produces, one per matrix combination.""" + """Every check name this job produces, one per matrix combination. + + `include` and `exclude` are not dimensions of the product, but ignoring + them entirely left a blind spot: a matrix expressed *entirely* through + `include:` has no list dimensions at all, so this returned [job_name] -- + one name for however many legs the job really has. release.yml and + eosim-sanity.yml both use that form today, so the blind spot goes live + the moment this guard is pointed at them. + """ dimensions = {k: v for k, v in matrix.items() if k not in _NOT_A_DIMENSION and isinstance(v, list)} - if not dimensions: + + combos = [] + if dimensions: + keys = list(dimensions) + for values in itertools.product(*(dimensions[k] for k in keys)): + combos.append(dict(zip(keys, values))) + + # exclude removes combinations rather than being ignored: counting a + # combination that never runs could report a duplicate that cannot happen. + for spec in matrix.get("exclude", []) or []: + if isinstance(spec, dict): + combos = [c for c in combos if not _matches(c, spec)] + + # include adds legs. An entry that only refines an existing combination + # does not add a name; one that introduces new values does. + for spec in matrix.get("include", []) or []: + if not isinstance(spec, dict): + continue + refines = [c for c in combos if _matches(c, {k: v for k, v in spec.items() + if k in c})] + if refines: + for c in refines: + c.update(spec) + else: + combos.append(dict(spec)) + + if not combos: return [job_name] - keys = list(dimensions) - names = [] - for combo in itertools.product(*(dimensions[k] for k in keys)): - name = job_name - for key, value in zip(keys, combo): - name = name.replace("${{ matrix.%s }}" % key, str(value)) - name = name.replace("${{matrix.%s}}" % key, str(value)) - names.append(name) - return names + return [_substitute(job_name, c) for c in combos] def _all_check_names(jobs): From 60605054ed2c845fe95078747a2d88d2455ad0c3 Mon Sep 17 00:00:00 2001 From: kartikey1306 Date: Thu, 3 Sep 2026 23:00:38 +0530 Subject: [PATCH 5/6] test(ci): model GitHub's auto-generated matrix names, not a collision it prevents Answers finding 1 from the second review on #104. A matrix job with no `name:` was reported as a check-name collision that cannot happen. `_expanded_names()` fell back to `job.get("name", job_id)` and substituted nothing into it, so an unnamed two-leg matrix yielded `["smoke", "smoke"]`. GitHub generates distinct names for exactly this case -- `smoke (ubuntu-22.04)`, `smoke (macos-latest)` -- so the legs never share a name. Asserting a duplicate GitHub prevents sends the reader looking for a bug that is not there, which is worse than not checking. `_expanded_names()` now takes `has_explicit_name` and, when there is none, expands to the default form with the matrix values in it. `test_a_matrix_job_names_every_dimension_it_varies` skips an unnamed job for the same reason: the rule is about a `name:` that varies less than its matrix does, and no `name:` at all already satisfies it. Reproduced the reviewer's probe. An unnamed two-leg `os` matrix wired into the gate: before this commit 2 failed (test_every_check_name_is_unique and test_a_matrix_job_names_every_dimension) after 10 passed And the guard still catches the real thing -- reverting the `name:` template on `test` gives `2 failed` with `assert not ['os']`, unchanged. Finding 2 (Low) is scope rather than a code change: this rule is enforced in ebuild only, while eos and eBoot have the same matrix jobs. Carrying test_ci_gate.py across is worth doing and belongs with the gate PRs there (eos#121, eBoot#90) rather than here, where it would be a third copy landing before the first two have merged. Noted in the PR body. Verified: pytest tests/unit/test_ci_gate.py 10 passed pytest tests/ 1 failed, 567 passed, 3 skipped The one failure is PRE-EXISTING on origin/master and unrelated -- the `python -m ninja` invocation from #66's open follow-up. Refs #104 --- tests/unit/test_ci_gate.py | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/tests/unit/test_ci_gate.py b/tests/unit/test_ci_gate.py index ed82503..8a4a1e0 100644 --- a/tests/unit/test_ci_gate.py +++ b/tests/unit/test_ci_gate.py @@ -231,7 +231,7 @@ def _matches(combo, spec): return all(str(combo.get(k)) == str(v) for k, v in spec.items()) -def _expanded_names(job_name, matrix): +def _expanded_names(job_name, matrix, has_explicit_name=True): """Every check name this job produces, one per matrix combination. `include` and `exclude` are not dimensions of the product, but ignoring @@ -272,6 +272,16 @@ def _expanded_names(job_name, matrix): if not combos: return [job_name] + # A job with no `name:` gets GitHub's auto-generated one, which already + # carries the matrix values -- `smoke (ubuntu-22.04)`, `smoke (macos- + # latest)`. Modelling that matters: falling back to the bare job id made + # every leg of an unnamed matrix look like the same check name, so this + # reported a collision GitHub would never produce and sent the reader + # looking for a bug that is not there. + if not has_explicit_name: + return [f"{job_name} ({', '.join(str(c[k]) for k in sorted(c))})" + for c in combos] + return [_substitute(job_name, c) for c in combos] @@ -280,7 +290,8 @@ def _all_check_names(jobs): for job_id, job in jobs.items(): job_name = job.get("name", job_id) matrix = job.get("strategy", {}).get("matrix", {}) - names.extend(_expanded_names(job_name, matrix or {})) + names.extend(_expanded_names(job_name, matrix or {}, + has_explicit_name="name" in job)) return names @@ -303,7 +314,12 @@ def test_a_matrix_job_names_every_dimension_it_varies(jobs): and isinstance(v, list) and len(v) > 1] if not dimensions: continue - name = job.get("name", job_id) + if "name" not in job: + # No `name:` means GitHub generates one that already carries the + # matrix values, so the rule is satisfied by the default. The rule + # is about a `name:` that varies less than the matrix does. + continue + name = job["name"] missing = [k for k in dimensions if not re.search(r"\$\{\{\s*matrix\.%s\s*\}\}" % re.escape(k), name)] assert not missing, ( From b67d3211c85ee6f523b600bdcc831930de84f426 Mon Sep 17 00:00:00 2001 From: kartikey1306 Date: Fri, 4 Sep 2026 11:58:11 +0530 Subject: [PATCH 6/6] ci: rename the last static-named matrix, and check name uniqueness repo-wide Answers the overnight review on #104. Finding 1 (Medium) -- nightly.yml's full-test-suite was `name: Full Test Suite` over three Python legs: three check runs, one name, the identical defect this PR fixes in ci.yml. The reviewer expanded every matrix in all 15 workflows with this PR's own helper and found it to be the only remaining offender. Renamed now, while nothing refers to the old name -- the PR's own timing argument, taken. Finding 3 (Low) -- uniqueness was checked within one workflow file, but branch protection matches check-run names repo-wide. test_check_names_are_unique_across_every_workflow builds the name set over every pull_request-triggered workflow plus the schedule-only ones (their names land in the same namespace) and asserts uniqueness over the union. Discrimination, executed: reverting the nightly rename makes the new test fail on its own -- these check names are produced more than once across the repository's workflows: ['Full Test Suite'] -- and it passes again on the fixed tree. Finding 1's class now fails loudly wherever it recurs, which is what makes it a class fix rather than a rename. Finding 2 (Low) -- the docstring claimed release.yml and eosim-sanity.yml used the include:-only form and were the live blind spot. Expansion showed neither does; nightly.yml was the real one and the docstring did not mention it. It now records what was actually found: the repo is clean, and the cross-file test is what keeps it that way. Finding 4 (Low) -- the codecov flags change (ebuild-py -> ebuild-py-) was in the diff but not the body. Disclosed on the thread with its one real consequence: codecov.yml has no flag_management or carryforward and comment.layout includes flags, so PR comments will list 9 new flags with the previous 3 absent until history rolls over. Verified: pytest tests/unit/test_ci_gate.py 11 passed pytest tests/ 1 failed, 568 passed, 3 skipped (the one failure is the pre-existing python -m ninja environment gap from #66's open follow-up, reproduced on origin/master) Refs #104 --- .github/workflows/nightly.yml | 6 +++++- tests/unit/test_ci_gate.py | 36 ++++++++++++++++++++++++++++++++++- 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/.github/workflows/nightly.yml b/.github/workflows/nightly.yml index 6ce439c..d59b1b0 100644 --- a/.github/workflows/nightly.yml +++ b/.github/workflows/nightly.yml @@ -14,7 +14,11 @@ concurrency: jobs: full-test-suite: - name: Full Test Suite + # Both matrix legs belong in the name: with a static name the three + # Python legs all reported as one check run called "Full Test Suite". + # Last remaining instance of the defect ci.yml's rename fixed; renaming + # is cheap exactly while nothing refers to the old name. + name: Full Test Suite (Python ${{ matrix.python-version }}) runs-on: ubuntu-latest strategy: matrix: diff --git a/tests/unit/test_ci_gate.py b/tests/unit/test_ci_gate.py index 8a4a1e0..964ca54 100644 --- a/tests/unit/test_ci_gate.py +++ b/tests/unit/test_ci_gate.py @@ -305,8 +305,42 @@ def test_every_check_name_is_unique(jobs): ) +def test_check_names_are_unique_across_every_workflow(): + """Branch protection matches check-run names repo-wide, not per file. + + The per-file test above catches a matrix that collides with itself; this + one catches ci.yml colliding with book-build.yml, and a static-named + matrix in any workflow (nightly.yml's Full Test Suite was one: three + Python legs, one name, invisible to the per-file check because the + fixture reads ci.yml only). + """ + names = [] + for doc in _pr_workflows().values(): + names.extend(_all_check_names(doc["jobs"])) + # nightly.yml is schedule-only, so _pr_workflows() misses it -- but its + # names still land in the same namespace branch protection matches on. + for path in sorted(WORKFLOWS_DIR.glob("*.yml")): + doc = _load(path) + if isinstance(doc, dict) and not _runs_on_pull_request(doc): + names.extend(_all_check_names(doc.get("jobs", {}) or {})) + duplicates = sorted({n for n in names if names.count(n) > 1}) + assert not duplicates, ( + f"these check names are produced more than once across the " + f"repository's workflows: {duplicates}. A name backed by several " + f"check runs cannot be required, whichever files produce them." + ) + + def test_a_matrix_job_names_every_dimension_it_varies(jobs): - """The rule behind the test above, stated where it will be read.""" + """The rule behind the test above, stated where it will be read. + + An earlier version of this docstring claimed release.yml and + eosim-sanity.yml used the include:-only form and were the live blind + spot. Expansion showed neither does -- every matrix job in both names + every dimension it varies -- while nightly.yml's full-test-suite was the + one real offender in the repository, now fixed. The repo is clean; the + cross-file test below is what keeps it that way. + """ for job_id, job in jobs.items(): matrix = job.get("strategy", {}).get("matrix", {}) or {} dimensions = [k for k, v in matrix.items()