From ca885cbb13a1f2e72874d20daa07e251f8416fa1 Mon Sep 17 00:00:00 2001 From: Marius Helf Date: Fri, 14 Aug 2026 18:06:59 +0000 Subject: [PATCH 1/2] Materialize signed-off spec for #7 --- spec/issue-7.md | 173 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 173 insertions(+) create mode 100644 spec/issue-7.md diff --git a/spec/issue-7.md b/spec/issue-7.md new file mode 100644 index 0000000..a25eb47 --- /dev/null +++ b/spec/issue-7.md @@ -0,0 +1,173 @@ +Goal: allow easy activation of branch protection in github via rule set. + +Normally, all targets that must be green must be hand-configured. This is flaky. + +Instead, implement via CI gate so that rules don't need to list all possible targets. + +See https://github.com/rodekruis/qualitative-feedback-analysis/blob/main/.github/workflows/ci.yaml for reference. + +--- + +## Spec + +**What:** Add one aggregate `ci-gate` job to **both** CI workflows — the shipped +`template/.github/workflows/cicd.yaml` (so every generated project has it) and this repo's own +`.github/workflows/ci.yaml` — restrict `push:` to `main` in both, add a drift test that the gate's +`needs` covers every other job, and note the required check name in the template README. + +**Why:** A branch ruleset must name required status checks literally, so today it has to list every +matrix leg (`test (3.12)`, `test (3.13)`, …). That list silently rots the moment a matrix dimension +or job is added or renamed — the ruleset keeps passing while the new leg goes unchecked, which is +the flakiness this ticket is about. One unmatrixed aggregate job whose name never changes is the +only thing the ruleset has to require, and `needs:` moves the "everything must be green" list into +the workflow, next to the jobs it guards. + +### The gate job + +Identical in both files except its `needs:` list. Append to `jobs:` in each: + +```yaml + # Single aggregate check to require in the branch ruleset. It depends on + # every other CI job and fails if any of them failed or was cancelled. + # Requiring this one check (instead of each job or matrix leg) keeps the + # ruleset stable when matrix dimensions change, and means a conditionally + # skipped dependency never leaves a required check stuck "waiting" — + # `skipped` is treated as acceptable here. + ci-gate: + if: always() + needs: [lint, test] # own ci.yaml has only `test`, so: needs: [test] + runs-on: ubuntu-latest + steps: + - name: Verify no required job failed + run: | + results="${{ join(needs.*.result, ' ') }}" + echo "Upstream job results: ${results}" + for r in ${results}; do + if [ "${r}" = "failure" ] || [ "${r}" = "cancelled" ]; then + echo "::error::A required CI job did not pass (result: ${r})." + exit 1 + fi + done + echo "All required CI jobs passed (or were acceptably skipped)." +``` + +`if: always()` is load-bearing: without it the job is skipped when a dependency fails, and a +skipped required check leaves the PR waiting forever instead of failing. + +The job has **no `uses:` step** — not even `actions/checkout` — so it adds nothing to the +action-pinning surface of #4. + +### Trigger change (both files) + +Both workflows currently fire on `push:` (every branch) *and* `pull_request:`, which produces two +identical `ci-gate` check runs on any same-repo PR branch. Restrict push to `main`: + +```yaml +on: + push: + branches: [main] + pull_request: + workflow_call: # template/cicd.yaml only; the repo's own ci.yaml has none +``` + +Keep `pull_request:` — dropping it (as the qfa reference does) means fork PRs never run CI, so the +required check never appears and those PRs become unmergeable. Accepted cost: pushing a branch with +no PR open no longer runs CI. + +### Drift test + +The gate is only as good as its `needs:` list — add a job, forget the list, and the gate goes green +while the new job burns. Add to `tests/test_template.py`, parameterized over both workflow files on +disk (no rendering needed, see below), asserting: a `ci-gate` job exists, it carries `if: always()`, +and `set(needs) == set(jobs) - {"ci-gate"}`. + +Two traps for the implementer: + +- **PyYAML parses a bare `on:` key as boolean `True`** (YAML 1.1). Any assertion that inspects + triggers must read `workflow[True] if True in workflow else workflow["on"]` — the same dance + `tests/test_docker_packaging.py` uses in the aeloop repo. +- **`pyyaml` is currently only a transitive dependency** (via `copier`). Add it explicitly to the + `dev` group in the root `pyproject.toml` rather than relying on the transitive. + +The template workflow can be parsed straight from disk because `_templates_suffix: .jinja` means +only `.jinja` files are rendered — `cicd.yaml` is copied verbatim, which is why its existing +`${{ matrix.python-version }}` survives generation today. Do **not** rename it to `.jinja`; `${{ }}` +would then collide with Jinja. + +### README note + +Under *Development* in `template/README.md.jinja`, at most four lines naming `ci-gate` as the single +check to require in a branch ruleset. Per `AGENTS.md`, add the fact and nothing more — no +walkthrough of the GitHub settings UI. + +## Acceptance criteria + +- [ ] `template/.github/workflows/cicd.yaml` has a `ci-gate` job exactly as specified above, with + `needs: [lint, test]`, `if: always()`, `runs-on: ubuntu-latest`, and no `uses:` step. +- [ ] `.github/workflows/ci.yaml` has the same job with `needs: [test]`. +- [ ] In both files `push:` is restricted to `branches: [main]`, `pull_request:` is retained, and the + template's `workflow_call:` is retained. +- [ ] `tests/test_template.py` gains a test, parameterized over both workflow files, asserting the + gate exists, carries `if: always()`, and that its `needs` equals every other job in that file. +- [ ] That test fails if a job is added to either workflow without being added to `needs` (verify by + temporarily adding a dummy job locally, or by reasoning stated in the PR body). +- [ ] `pyyaml` is an explicit entry in the `dev` dependency group of the root `pyproject.toml`. +- [ ] `template/README.md.jinja` names `ci-gate` as the required check in ≤4 added lines under + *Development*. +- [ ] `template/.github/workflows/cicd.yaml` still has no `.jinja` suffix and renders verbatim — the + existing rendered-project tests still pass. +- [ ] `uv run pytest tests` is green. +- [ ] Commit uses the `ci:` prefix (as in 9b55106) and carries no AI attribution trailer. + +## Activating the protection (repo admin, after merge) + +The one check to require is `ci-gate`. Applies identically to this repo and to any generated project. + +UI: *Settings → Rules → Rulesets → New branch ruleset* → target **Default branch** → tick **Require +status checks to pass** → add `ci-gate` → enforcement **Active**. + +Or: + +```bash +gh api -X POST repos///rulesets --input - <<'JSON' +{ + "name": "main", + "target": "branch", + "enforcement": "active", + "conditions": { "ref_name": { "include": ["~DEFAULT_BRANCH"], "exclude": [] } }, + "rules": [ + { "type": "required_status_checks", + "parameters": { + "strict_required_status_checks_policy": false, + "required_status_checks": [ { "context": "ci-gate" } ] + } } + ] +} +JSON +``` + +- Leave **Require branches to be up to date** (`strict_required_status_checks_policy`) off unless you + want every merge to force a rebase and a full CI re-run on the next PR in the queue. +- Add the parameterless `deletion` and `non_fast_forward` rule types to also block branch deletion + and force-pushes. +- Adding a CI job later needs no ruleset edit — that is the point of the gate. + +## Out of scope + +- Creating or enabling the branch ruleset itself — that is a repo-admin action in GitHub settings, + not code. The section above is the recipe; this ticket only makes one stable check name available + to require. +- Automating ruleset creation via `gh api` (GitHub does not import rulesets from a repo file). +- Renaming `cicd.yaml` → `ci.yaml` in the template (#8) and pinning actions to hashes (#4). Both are + independent of this change; whichever lands first, the other rebases. + +## Verification + +```bash +uv run pytest tests # needs network; renders a project and runs its lint/test/pre-commit +``` + +The gate's runtime behaviour can only be fully proven on GitHub: after merge, confirm a `ci-gate` +check run appears on a PR and that a ruleset can require it by that name. + +No `Depends-on:` — nothing this spec builds on is unmerged. From 189e201f977fc594a86277856d50c95fc1049eb2 Mon Sep 17 00:00:00 2001 From: Marius Helf Date: Fri, 14 Aug 2026 20:09:28 +0200 Subject: [PATCH 2/2] ci: add aggregate ci-gate job for branch protection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A branch ruleset names required status checks literally, so protecting main meant listing every matrix leg (`test (3.12)`, `test (3.13)`, ...). That list rots silently the moment a job or matrix dimension is added or renamed: the ruleset keeps passing while the new leg goes unchecked. Add one unmatrixed `ci-gate` job to both CI workflows — the shipped template/.github/workflows/cicd.yaml and this repo's own ci.yaml. It depends on every other job and fails if any failed or was cancelled, so the ruleset only ever has to require that one stable name and the "everything must be green" list lives in the workflow next to the jobs it guards. `if: always()` is load-bearing: without it the gate is skipped when a dependency fails, and a skipped required check leaves the PR waiting forever instead of failing. Restrict `push:` to main in both workflows — firing on every push and on pull_request produced two identical ci-gate check runs per PR branch. `pull_request:` is kept so fork PRs still get the required check. Guard the gate's `needs:` list with a drift test parameterized over both workflow files, asserting it covers every other job. pyyaml becomes an explicit dev dependency rather than a transitive of copier. --- .github/workflows/ci.yaml | 23 +++++++++++ pyproject.toml | 1 + template/.github/workflows/cicd.yaml | 23 +++++++++++ template/README.md.jinja | 4 ++ tests/test_template.py | 59 ++++++++++++++++++++++++++++ uv.lock | 2 + 6 files changed, 112 insertions(+) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 183c27f..b56d2f6 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -2,6 +2,7 @@ name: CI on: push: + branches: [main] pull_request: jobs: @@ -25,3 +26,25 @@ jobs: run: uv sync - name: Run pytest run: uv run pytest + # Single aggregate check to require in the branch ruleset. It depends on + # every other CI job and fails if any of them failed or was cancelled. + # Requiring this one check (instead of each job or matrix leg) keeps the + # ruleset stable when matrix dimensions change, and means a conditionally + # skipped dependency never leaves a required check stuck "waiting" — + # `skipped` is treated as acceptable here. + ci-gate: + if: always() + needs: [test] + runs-on: ubuntu-latest + steps: + - name: Verify no required job failed + run: | + results="${{ join(needs.*.result, ' ') }}" + echo "Upstream job results: ${results}" + for r in ${results}; do + if [ "${r}" = "failure" ] || [ "${r}" = "cancelled" ]; then + echo "::error::A required CI job did not pass (result: ${r})." + exit 1 + fi + done + echo "All required CI jobs passed (or were acceptably skipped)." diff --git a/pyproject.toml b/pyproject.toml index 4ad5ca3..bf2c539 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -8,6 +8,7 @@ requires-python = ">=3.12" dev = [ "pytest", "copier", + "pyyaml", ] [tool.pytest.ini_options] diff --git a/template/.github/workflows/cicd.yaml b/template/.github/workflows/cicd.yaml index 3981646..9994f9b 100644 --- a/template/.github/workflows/cicd.yaml +++ b/template/.github/workflows/cicd.yaml @@ -2,6 +2,7 @@ name: CI/CD on: push: + branches: [main] pull_request: workflow_call: @@ -67,3 +68,25 @@ jobs: run: make docs-strict - name: Run docs drift tests run: make test-docs + # Single aggregate check to require in the branch ruleset. It depends on + # every other CI job and fails if any of them failed or was cancelled. + # Requiring this one check (instead of each job or matrix leg) keeps the + # ruleset stable when matrix dimensions change, and means a conditionally + # skipped dependency never leaves a required check stuck "waiting" — + # `skipped` is treated as acceptable here. + ci-gate: + if: always() + needs: [lint, test, docs] + runs-on: ubuntu-latest + steps: + - name: Verify no required job failed + run: | + results="${{ join(needs.*.result, ' ') }}" + echo "Upstream job results: ${results}" + for r in ${results}; do + if [ "${r}" = "failure" ] || [ "${r}" = "cancelled" ]; then + echo "::error::A required CI job did not pass (result: ${r})." + exit 1 + fi + done + echo "All required CI jobs passed (or were acceptably skipped)." diff --git a/template/README.md.jinja b/template/README.md.jinja index 7ed14cd..a88dd0d 100644 --- a/template/README.md.jinja +++ b/template/README.md.jinja @@ -47,6 +47,10 @@ make test make lint ``` +## Branch protection + +`ci-gate` is the single status check to require in the branch ruleset: it depends on every other CI job, so the ruleset stays correct as jobs and matrix legs change. + ## Documentation Documentation is a [Sphinx](https://www.sphinx-doc.org/) + [MyST](https://myst-parser.readthedocs.io/) diff --git a/tests/test_template.py b/tests/test_template.py index 2bdf570..3451a21 100644 --- a/tests/test_template.py +++ b/tests/test_template.py @@ -6,6 +6,7 @@ from types import SimpleNamespace import pytest +import yaml import copier @@ -15,6 +16,28 @@ TEMPLATE_ROOT = Path(__file__).resolve().parent.parent +# Both CI workflows carry the aggregate `ci-gate` job. They can be parsed +# straight from disk: `_templates_suffix: .jinja` means only `.jinja` files are +# rendered, so the template's cicd.yaml is copied verbatim (which is also why +# its `${{ matrix.python-version }}` survives generation). +CI_WORKFLOWS = [ + TEMPLATE_ROOT / ".github" / "workflows" / "ci.yaml", + TEMPLATE_ROOT / "template" / ".github" / "workflows" / "cicd.yaml", +] + + +def _load_workflow(path: Path) -> dict: + return yaml.safe_load(path.read_text(encoding="utf-8")) + + +def _triggers(workflow: dict) -> dict: + """Return a workflow's `on:` block. + + PyYAML follows YAML 1.1, where a bare ``on`` key is the boolean ``True``, + so the block cannot simply be looked up by its name. + """ + return workflow[True] if True in workflow else workflow["on"] + COPIER_DATA = { "author_name": "Test Author", "author_email": "test@example.com", @@ -230,6 +253,42 @@ def test_default_slug_is_valid_package_name(tmp_path): assert slug.isidentifier(), f"slug is not a valid identifier: {slug!r}" +@pytest.mark.parametrize("workflow_path", CI_WORKFLOWS, ids=lambda p: p.name) +def test_ci_gate_covers_every_job(workflow_path): + """The `ci-gate` job must depend on every other job in its workflow. + + A branch ruleset names required checks literally, so it requires only + `ci-gate` and the workflow itself carries the "everything must be green" + list. That indirection is only safe while `needs:` stays complete — add a + job (or a whole matrix) and forget the list, and the gate goes green while + the new job burns. This test is the tripwire for exactly that drift. + """ + workflow = _load_workflow(workflow_path) + jobs = workflow["jobs"] + + assert "ci-gate" in jobs, f"{workflow_path.name} has no ci-gate job" + gate = jobs["ci-gate"] + + # Without `if: always()` the gate is skipped when a dependency fails, and a + # skipped required check leaves the PR waiting forever instead of failing. + assert gate.get("if") == "always()" + assert set(gate["needs"]) == set(jobs) - {"ci-gate"} + + +@pytest.mark.parametrize("workflow_path", CI_WORKFLOWS, ids=lambda p: p.name) +def test_ci_workflow_triggers(workflow_path): + """Push runs only on main; pull_request stays so fork PRs still get a gate. + + Firing on every push *and* on pull_request produces two identical `ci-gate` + check runs for a same-repo PR branch. Dropping `pull_request:` instead + would mean fork PRs never run CI, so the required check never appears and + those PRs cannot be merged. + """ + triggers = _triggers(_load_workflow(workflow_path)) + assert triggers["push"] == {"branches": ["main"]} + assert "pull_request" in triggers + + def test_invalid_project_slug_is_rejected(tmp_path): """An explicitly supplied invalid project_slug must fail validation. diff --git a/uv.lock b/uv.lock index 70a8939..a6cab34 100644 --- a/uv.lock +++ b/uv.lock @@ -53,6 +53,7 @@ source = { virtual = "." } dev = [ { name = "copier" }, { name = "pytest" }, + { name = "pyyaml" }, ] [package.metadata] @@ -61,6 +62,7 @@ dev = [ dev = [ { name = "copier" }, { name = "pytest" }, + { name = "pyyaml" }, ] [[package]]