From 6f9c9e0d838f0b0c31d989c4e331fe8543c3b3cd Mon Sep 17 00:00:00 2001 From: Kartikey1306 Date: Tue, 1 Sep 2026 21:06:30 +0530 Subject: [PATCH 1/2] 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/2] 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"]