Add CI gate for easy branch protection setup - #13
Conversation
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.
|
Build complete. ~Written by Claude, run via the agentic engineering loop |
|
Builder committed 1 of the 5 tasks its own plan ( ~Written by Claude, run via the agentic engineering loop |
mariushelf
left a comment
There was a problem hiding this comment.
Review — PR #13 (issue #7): aggregate ci-gate for branch protection
Verdict: approve. All ten acceptance criteria are met. No blocking or major findings — everything below is recorded for the human reviewer, not a request for another round.
The loop's "1 of 5 tasks" note
Checked and dismissed: the single commit 189e201 covers every acceptance criterion. Both workflows, the trigger change, both tests, pyproject.toml/uv.lock, and the README note are all in it. Short commit count, complete build.
What I verified, and how
| Criterion | Result |
|---|---|
ci-gate in template/.github/workflows/cicd.yaml, if: always(), runs-on: ubuntu-latest, no uses: step |
met (cicd.yaml:71-92) |
Same job in .github/workflows/ci.yaml with needs: [test] |
met (ci.yaml:29-50) |
push: restricted to main, pull_request: kept, template workflow_call: kept |
met (ci.yaml:3-6, cicd.yaml:3-7) |
Parameterized drift test: gate exists, if: always(), needs == other jobs |
met (tests/test_template.py:256-276) |
Test fails when a job is added without updating needs |
met — verified empirically, not by reasoning: appending a dummy: job to each file failed test_ci_gate_covers_every_job[ci.yaml] and [cicd.yaml]; both files restored, tree clean |
pyyaml explicit in dev group |
met (pyproject.toml:11); uv lock --check is consistent |
template/README.md.jinja names ci-gate in ≤4 added lines under Development |
met — exactly 4 lines, ## Branch protection sits under the # Development H1 |
cicd.yaml keeps its non-.jinja name, renders verbatim |
met — rendered-project tests (test_pre_commit_passes, test_make_test, test_make_docs_strict) still pass for both flat and hexagonal |
uv run pytest tests green |
met — 24 passed, 0 skipped |
ci: prefix, no AI attribution trailer |
met (189e201, trailer list empty) |
On the needs: [lint, test, docs] deviation
The author's call is right, and the PR body flags it honestly. The spec's snippet named [lint, test] but template/.github/workflows/cicd.yaml has had a third job, docs, since before this branch (cicd.yaml:57-70). Acceptance criterion 4 (needs equals every other job) and the literal [lint, test] in criterion 1 cannot both hold; taking the criterion-4 reading is the one that actually closes the failure mode this ticket exists to close. Reverting to [lint, test] would leave docs unguarded and break the mandated drift test. Flagging it only so a human ratifies the spec-text deviation rather than the loop doing so silently.
Correctness notes I checked and cleared
for r in ${results}is deliberately unquoted for word splitting; the${{ join(...) }}interpolation can only ever yield the fixedsuccess|failure|cancelled|skippedenum, so there is no expression-injection or glob surface.skippedtreated as pass is currently unreachable — no job in either file carries anif:— so it is forward-looking, as the comment says. Cancellation yieldscancelled, which the gate correctly fails on.template/.github/workflows/release.yaml:28callscicd.yamlviaworkflow_call; the gate rides along there asci / ci-gateand still blocksreleasethroughneeds: ci. No regression.- Generated projects do use
main(copier.yml:13runsgit checkout -b main), sobranches: [main]is correct in the shipped template, not just in this repo. - A matrix leg added to an existing job needs no
needs:edit — job-level dependency covers all legs. Correct by construction.
Findings (all non-blocking)
Three inline, plus one here:
- Nit — this PR adds a top-level
spec/directory (spec/issue-7.md, 173 lines) that does not exist onmain, via the separate commitca885cb. Worth a conscious yes/no on whether signed-off specs are meant to ship in the repo, since this is the first one.
What cannot be verified here
As the spec itself says, the gate's runtime behaviour is only provable on GitHub: that a ci-gate check run actually appears on a PR, and that a ruleset can require it by that literal name. Nothing in the diff can establish that.
~Written by Claude, run via the agentic engineering loop
| """ | ||
| triggers = _triggers(_load_workflow(workflow_path)) | ||
| assert triggers["push"] == {"branches": ["main"]} | ||
| assert "pull_request" in triggers |
There was a problem hiding this comment.
Minor (test coverage gap for a spec'd invariant). This test guards push: and pull_request:, but not workflow_call: — which acceptance criterion 3 names alongside them, and which template/.github/workflows/release.yaml:28 depends on (uses: ./.github/workflows/cicd.yaml).
Dropping workflow_call: from cicd.yaml would pass the whole suite and then break every generated project's release workflow at runtime, with an error that points at release.yaml rather than at the trigger block that actually moved. That is the same class of silent drift the rest of this PR exists to prevent, so it is worth one more line:
if workflow_path.name == "cicd.yaml":
assert "workflow_call" in triggersNot gating — the file is correct today.
~Written by Claude, run via the agentic engineering loop
| # 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 = [ |
There was a problem hiding this comment.
Nit. CI_WORKFLOWS is a hardcoded pair, so the drift test protects job-level drift but not workflow-file-level drift: a third CI workflow added to either .github/workflows/ directory gets no gate and no test, silently.
A glob is not the fix — release.yaml and publish.yaml legitimately have no gate, and would start failing. The cheap version is a comment saying so, e.g. "release.yaml/publish.yaml are deliberately absent: they are not PR checks, so nothing requires them in a ruleset." That way the next person adding a workflow has to decide rather than not notice.
~Written by Claude, run via the agentic engineering loop
|
|
||
| # 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()" |
There was a problem hiding this comment.
Nit. Exact-string match. if: ${{ always() }} is semantically identical to if: always() in Actions, but would fail this assertion — so a future editor could be told the gate is broken when it is fine.
assert "always()" in str(gate.get("if", "")) keeps the intent and tolerates the equivalent spelling. Genuinely optional; the strict form is what the spec asked for.
~Written by Claude, run via the agentic engineering loop
|
|
||
| ## 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. |
There was a problem hiding this comment.
Nit. This is a single ~185-character line, while the surrounding prose in this file wraps around 90 (see the Documentation section just below). Content and length are exactly right per the spec — only the wrapping is inconsistent.
~Written by Claude, run via the agentic engineering loop
|
Review iteration 1: approve — The change adds an aggregate ~Written by Claude, run via the agentic engineering loop |
|
Review verdict: approved (tier 2) ~Written by Claude, run via the agentic engineering loop |
Done — committed as
ad6c62e(amended), working tree clean,uv run pytest testsgreen (24 passed).PR-ready summary
ci: add aggregate ci-gate job for branch protectionWhat changed
.github/workflows/ci.yamlci-gatejob,needs: [test];push:restricted tomaintemplate/.github/workflows/cicd.yamlci-gatejob,needs: [lint, test, docs];push:restricted tomain;pull_request:andworkflow_call:retainedtests/test_template.pyif: always()/needscovers every other job, and trigger shapepyproject.toml,uv.lockpyyamlpromoted from transitive (viacopier) to explicitdevdependencytemplate/README.md.jinjaci-gateas the check to requireNew behavior
ci-gate, is all a branch ruleset has to require — on this repo and on every generated project. Adding a job or a matrix leg needs no ruleset edit; the "everything must be green" list lives inneeds:next to the jobs it guards.if: always()and fails on any upstreamfailure/cancelled, treatingskippedas acceptable. Withoutalways()a failing dependency would skip the gate, and a skipped required check leaves a PR waiting forever instead of failing.uses:step (not evenactions/checkout), so it adds nothing to the action-pinning surface of pin github actions to hashes #4.pull_request:stays so fork PRs still produce the required check.One deviation from the spec
The spec's snippet says
needs: [lint, test]for the template workflow, but that workflow has a third job,docs. Since the spec's own mandated drift test assertsneeds == all other jobs, the two can't both hold — I usedneeds: [lint, test, docs], which satisfies the test and the stated intent. Without it the gate would have gone green whiledocsburned, i.e. exactly the failure mode this ticket exists to close.Impact / risk
git revert-able. No auth, secrets, or permission changes — enabling the ruleset itself remains a repo-admin action (recipe is inspec/issue-7.md).ci.yamlfailedtest_ci_gate_covers_every_job[ci.yaml]; restoring the file made it pass again.cicd.yamlkeeps its non-.jinjaname and is still copied verbatim (${{ }}would collide with Jinja otherwise) — the rendered-project tests, includingpre-commit/yamllint on the generated workflow, still pass.ci-gate, this is inert; the gate's runtime behavior can only be fully confirmed on GitHub once a PR produces the check run.Fixes #7
~Written by Claude, run via the agentic engineering loop