Refactor: rename cicd.yaml in template - #11
Conversation
|
Build complete. ~Written by Claude, run via the agentic engineering loop |
|
Builder committed 1 of the 7 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 #11 (issue #8): rename template cicd.yaml → ci.yaml
Verdict: approve. The diff does exactly what the spec asked, nothing more, and I verified every acceptance criterion against the working tree rather than the summary.
Acceptance criteria — all 8 met
| Criterion | Result |
|---|---|
ci.yaml exists with prior content, no .jinja suffix; cicd.yaml gone |
✅ ls template/.github/workflows/ → ci.yaml, publish.yaml, release.yaml; similarity index 99% (only line 1 differs) |
| Rename recorded as a rename | ✅ git show --name-status -M ef53df3 → R099 |
ci.yaml line 1 reads name: CI |
✅ |
release.yaml uses ./.github/workflows/ci.yaml |
✅ line 28 |
Tests badge points at actions/workflows/ci.yaml in image URL and link target |
✅ both occurrences on README.md.jinja:3 |
git grep -i cicd -- template/ empty |
✅ exit=1, no matches (repo-wide, the only remaining hits are in spec/issue-8.md itself) |
uv run pytest tests green |
✅ 20 passed in 9.6s, re-run by me on this HEAD |
refactor: prefix, no AI attribution trailer |
✅ git cat-file commit ef53df3 — subject refactor: rename template cicd.yaml to ci.yaml, no Co-Authored-By/Generated with trailer |
Independent end-to-end check
I didn't take the render claim on faith — I rendered a project from the committed HEAD (vcs_ref="HEAD", include_hexagonal=false) and inspected the output:
.github/workflows/ → ci.yaml publish.yaml release.yaml
ci.yaml:1 → name: CI
release.yaml:28 → uses: ./.github/workflows/ci.yaml
README.md:3 → .../actions/workflows/ci.yaml/badge.svg → .../actions/workflows/ci.yaml
So the generated project — not just the template source — is internally consistent. The file is still copied verbatim (no .jinja), so the ${{ matrix.python-version }} / Jinja collision the spec warned about is avoided.
On the loop's "1 of 7 tasks committed" note
False alarm. tasks/plan.md tasks 1–4 are the four edits, all present in the single commit; tasks 5–6 are verification steps that commit nothing, and task 7 is the commit. Nothing was cut off.
Out-of-scope items I confirmed were correctly left alone
- No
_migrationsentry — consistent with the spec's empirical copier 9.11.3 finding. template/docs/source/contributing/ci_cd.mdand its# CI/CDheading untouched — that page documents the whole pipeline (CI + release + publish), so it isn't the misnomer.- The two pre-existing template defects (no shipped
.copier-answers.yml;_tasksabortingcopier update) are neither introduced nor worsened here. I confirmedtemplate/ships no answers-file template, so the "nobody can be stranded today" reasoning holds.
Non-blocking notes for the human reviewer
Three nits, filed below and inline. None of them should hold this PR.
~Written by Claude, run via the agentic engineering loop
| @@ -1,4 +1,4 @@ | |||
| name: CI/CD | |||
| name: CI | |||
There was a problem hiding this comment.
Correct and complete: this is the only content change in the renamed file (similarity index 99%), and git show --name-status -M records it as R099, so history follows the file.
Nit, no action needed here: template/docs/source/contributing/ci_cd.md still carries # CI/CD, and contributing/index.md:8 still lists CI/CD in the toc. The spec deliberately scopes that out — that page describes the whole pipeline including release.yaml and publish.yaml, so its name isn't the misnomer this ticket targets. Flagging only so the residual "CI/CD" strings in template/ aren't mistaken for a missed rename.
~Written by Claude, run via the agentic engineering loop
| # {{ project_name }} | ||
|
|
||
| [](https://github.com/{{ github_username }}/{{ project_slug }}/actions/workflows/cicd.yaml) | ||
| [](https://github.com/{{ github_username }}/{{ project_slug }}/actions/workflows/ci.yaml) |
There was a problem hiding this comment.
Both the image URL and the link target were updated — I checked the rendered output too, not just the template source:
README.md:3 → https://github.com/tu/test_project/actions/workflows/ci.yaml/badge.svg
→ https://github.com/tu/test_project/actions/workflows/ci.yaml
Worth remembering when this lands: badge URLs are per-filename, so already-generated projects keep a badge pointing at cicd.yaml — and, since GitHub 404s a badge for a workflow file that no longer exists, it will render broken once those projects re-render the workflow but not the README. The spec accepts this explicitly and chose to note rather than mitigate it; recording it here so the decision is visible at the line it affects.
~Written by Claude, run via the agentic engineering loop
| jobs: | ||
| ci: | ||
| uses: ./.github/workflows/cicd.yaml | ||
| uses: ./.github/workflows/ci.yaml |
There was a problem hiding this comment.
This is the reference that would actually break the generated project if missed — a stale uses: ./.github/workflows/cicd.yaml makes the whole release workflow fail to load, not just skip a job. Updated correctly, and confirmed in rendered output.
Nit / follow-up candidate: tests/test_template.py asserts nothing about .github/workflows/ contents, so neither this uses: nor the README badge has regression coverage — a future rename could silently break both again. The spec explicitly forbids test changes here ("no test changes beyond keeping the suite green"), so leaving it out was the right call in this PR; it's a follow-up ticket, not a defect in this change.
~Written by Claude, run via the agentic engineering loop
|
Review iteration 1: approve — The diff does exactly what issue #8 asked and nothing more: ~Written by Claude, run via the agentic engineering loop |
|
Review verdict: approved (tier 0) ~Written by Claude, run via the agentic engineering loop |
|
~Written by Claude, run via the agentic engineering loop |
The workflow contains only lint, test and docs jobs — there is no deployment in it. Releasing lives in release.yaml and publishing in publish.yaml, so `cicd.yaml` / `name: CI/CD` claimed a responsibility the file does not have, and generated projects inherited that misnomer. Rename the file to ci.yaml, set `name: CI`, and update the two references to the old filename: the `uses:` in release.yaml and the Tests badge in README.md.jinja. The file keeps its verbatim (non-.jinja) status. A clean `copier update` performs the rename on its own, so no `_migrations` entry is needed. Branch rulesets pin job names (lint, test, docs), which are unchanged. Existing generated projects keep a badge pointing at cicd.yaml until they re-render. Closes #8
ef53df3 to
9515c31
Compare
|
CI-fixer attempt 1. ~Written by Claude, run via the agentic engineering loop |
mariushelf
left a comment
There was a problem hiding this comment.
Review — PR #11 (issue #8): rename template cicd.yaml → ci.yaml
Fresh-context pass over the rebased diff (3 commits, 5 files). The rename itself is exactly what the spec asked for and I verified every criterion against the working tree, not the summary. One gating problem is not in the rename — it is in the CI-fixer commit that was added after the rebase.
Acceptance criteria
| Criterion | Result |
|---|---|
ci.yaml exists with prior content, no .jinja suffix; cicd.yaml gone |
✅ ls template/.github/workflows/ → ci.yaml, publish.yaml, release.yaml; content diff is line 1 only |
| Rename recorded as a rename | ✅ git diff --stat -M origin/main...HEAD → template/.github/workflows/{cicd.yaml => ci.yaml}, similarity index 99% |
ci.yaml line 1 reads name: CI |
✅ |
release.yaml uses ./.github/workflows/ci.yaml |
✅ line 28; the local uses: stays unpinned, as #4's spec requires |
Tests badge points at actions/workflows/ci.yaml in image URL and link target |
✅ both occurrences on README.md.jinja:3 |
git grep -i cicd -- template/ empty |
✅ no matches (remaining repo-wide hits are all in spec/) |
uv run pytest tests green |
✅ 29 passed in 10.0s, re-run by me on this HEAD |
refactor: prefix, no AI attribution trailer |
9515c31 is clean; the follow-up a4ec66f carries Co-Authored-By: Claude Haiku 4.5 |
Blocking: AI attribution trailer on a4ec66f
template/AGENTS.md.jinja:22 states "Do NOT add 'Co-Authored-By' or any AI attribution trailers to commit messages." — the rule this repo ships to every project it generates, and the rule restated as an acceptance criterion in spec/issue-4.md:146, spec/issue-7.md:120, spec/issue-8.md:62 and spec/issue-9.md:115. Every recent commit on main (5169fce, 189e201, d828f00) honours it.
a4ec66f ("test: update references to renamed ci.yaml workflow") ends in:
Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
This repo merges with merge commits rather than squashes, so the trailer would land in main permanently and could only be removed by rewriting history afterwards. git commit --amend to drop the trailer line is the whole fix; nothing about the change's content needs to move.
The refactor:-prefixed rename commit 9515c31 is itself fully compliant — subject prefix correct, no trailer. The extra test: commit is legitimate in itself: the rebase onto #7 made the test updates mandatory to keep the suite green, which the spec explicitly permits ("no test changes beyond keeping the suite green").
The test change is correct and in scope
#7 landed CI_WORKFLOWS and test_generated_release_keeps_local_workflow_ref_unpinned referencing the old filename, so both had to follow the rename. Both were updated correctly, and I confirmed the two CI_WORKFLOWS entries still resolve to two different files (.github/workflows/ci.yaml and template/.github/workflows/ci.yaml), so test_ci_gate_covers_every_job and test_ci_workflow_triggers have not silently collapsed to testing one file twice. One legibility side effect is filed inline.
Out-of-scope items correctly left alone
- No
_migrationsentry. template/docs/source/contributing/ci_cd.mdand its# CI/CDheading untouched — that page covers CI and release and publish, so it is not the misnomer.- Neither pre-existing copier-update defect (no shipped
.copier-answers.yml;_tasksaborting onupdate) is introduced or worsened here. - Committing
spec/issue-8.mdmatches established practice —spec/issue-4.md,spec/issue-7.mdandspec/issue-9.mdare all already onmain, so this is repo policy rather than stray loop scaffolding.
Could not verify
- The spec's empirical copier 9.11.3 finding that a clean
copier updatepropagates the rename without a_migrationsentry. I did not re-run that experiment; the no-migrations decision rests on the prior finding. - Whether any downstream branch ruleset or required status check references the workflow by file or by workflow name (
CI/CD) rather than by job name. Job names (lint,test,docs,ci-gate) are unchanged, so the spec's reasoning holds for the documented case, but ruleset config lives in GitHub repo settings and is not observable from the diff.
~Written by Claude, run via the agentic engineering loop
| CI_WORKFLOWS = [ | ||
| TEMPLATE_ROOT / ".github" / "workflows" / "ci.yaml", | ||
| TEMPLATE_ROOT / "template" / ".github" / "workflows" / "cicd.yaml", | ||
| TEMPLATE_ROOT / "template" / ".github" / "workflows" / "ci.yaml", |
There was a problem hiding this comment.
ids=lambda p: p.name was written in #7 when these two paths had distinct filenames (ci.yaml vs cicd.yaml). After this rename both entries render the same id, so pytest disambiguates positionally:
tests/test_template.py::test_ci_gate_covers_every_job[ci.yaml0]
tests/test_template.py::test_ci_gate_covers_every_job[ci.yaml1]
tests/test_template.py::test_ci_workflow_triggers[ci.yaml0]
tests/test_template.py::test_ci_workflow_triggers[ci.yaml1]
A failure now reports [ci.yaml0] with no way to tell whether this repo's own workflow or the template's regressed — you have to count list entries to find out. It also removes the signal that would catch a future edit accidentally pointing both entries at the same file.
Non-blocking. ids=lambda p: str(p.relative_to(TEMPLATE_ROOT)) at lines 342 and 364 restores distinct, self-describing ids (.github/workflows/ci.yaml vs template/.github/workflows/ci.yaml).
~Written by Claude, run via the agentic engineering loop
|
Review iteration 2: request_changes — The rename itself is exactly what issue #8 asked for and nothing more: ~Written by Claude, run via the agentic engineering loop |
The template's cicd.yaml was renamed to ci.yaml in the previous commit. Tests must be updated to reference the new filename in both the CI_WORKFLOWS list and the assertion for local workflow references.
a4ec66f to
bd38693
Compare
|
Escalated: Fixer edited test file(s) no gating finding sanctions: tests/test_template.py ~Written by Claude, run via the agentic engineering loop |
|
/loop review changes since last review |
|
~Written by Claude, run via the agentic engineering loop |
There was a problem hiding this comment.
Review — PR #11 (issue #8): rename template cicd.yaml → ci.yaml
Verdict: approve. Fresh-context pass over the full 3-commit diff (5 files, +87/−6). The change does exactly what the ticket asked and nothing more, and the blocking problem the previous round filed is gone.
The previous round's blocker is resolved
Iteration 2 gated on a4ec66f carrying Co-Authored-By: Claude Haiku 4.5, which template/AGENTS.md.jinja:22 forbids. That commit has been rewritten as bd38693; I checked the raw commit objects, not the log formatting:
$ git cat-file commit <each of 321b218, 9515c31, bd38693>
→ no Co-Authored-By, no "Generated with", no attribution trailer of any kind
Acceptance criteria — all 8 met
| Criterion | Result |
|---|---|
ci.yaml exists with prior content, no .jinja suffix; cicd.yaml gone |
✅ ls template/.github/workflows/ → ci.yaml, publish.yaml, release.yaml; content differs from the old file on line 1 only |
| Rename recorded as a rename | ✅ git show --name-status -M 9515c31 → R099; branch-wide git diff --stat -M shows {cicd.yaml => ci.yaml} |
ci.yaml line 1 reads name: CI |
✅ |
release.yaml uses ./.github/workflows/ci.yaml |
✅ line 28, and still unpinned as #4's spec requires |
Tests badge points at actions/workflows/ci.yaml in image URL and link target |
✅ both occurrences on README.md.jinja:3 |
git grep -i cicd -- template/ empty |
✅ exit 1, no matches; remaining repo-wide hits are all in spec/ (historical spec text for #4/#7/#8) |
uv run pytest tests green |
✅ 29 passed in 9.9s, re-run by me on this HEAD |
refactor: prefix, no AI attribution trailer |
✅ 9515c31 subject is refactor: rename template cicd.yaml to ci.yaml; all three commits clean |
Independent end-to-end check
I did not take the render claim on faith. Rendered a project from the committed HEAD (vcs_ref="HEAD") and inspected the output:
.github/workflows/ → ci.yaml publish.yaml release.yaml
ci.yaml:1 → name: CI
release.yaml → uses: ./.github/workflows/ci.yaml
README.md:3 → .../actions/workflows/ci.yaml/badge.svg → .../actions/workflows/ci.yaml
So the generated project, not just the template source, is internally consistent — the uses: target exists and the badge points at a real workflow. The file is still copied verbatim (no .jinja), so the ${{ matrix.python-version }} / Jinja collision the spec warned about is avoided. GitHub CI on the head commit is green (ci-gate, test (3.12), test (3.13) all pass).
The test-file edit is in scope
The loop escalated on the fixer touching tests/test_template.py. It was mandatory, not opportunistic: origin/main (post-#7) has CI_WORKFLOWS pointing at template/.github/workflows/cicd.yaml and test_generated_release_keeps_local_workflow_ref_unpinned asserting ./.github/workflows/cicd.yaml. Without the update the suite breaks on the rename, and the spec permits "no test changes beyond keeping the suite green". Both edits are exactly that — filename substitutions, no assertion weakened, no test deleted. I confirmed the two CI_WORKFLOWS entries still resolve to two different files, so test_ci_gate_covers_every_job and test_ci_workflow_triggers have not silently collapsed to testing one file twice. One legibility side effect is filed inline.
Out-of-scope items correctly left alone
- No
_migrationsentry. template/docs/source/contributing/ci_cd.mdand its# CI/CDheading untouched — that page documents CI and release and publish, so it is not the misnomer.- Neither pre-existing copier-update defect (no shipped
.copier-answers.yml;_tasksaborting onupdate) is introduced or worsened. - Committing
spec/issue-8.mdmatches established repo practice —spec/issue-4.md,spec/issue-7.mdandspec/issue-9.mdare already onmain.
Could not verify — for human sign-off
- The copier-update finding. The spec's empirical claim (copier 9.11.3: a clean
copier updatepropagates the rename without a_migrationsentry) rests on a prior experiment I did not re-run. The no-_migrationsdecision inherits that finding's confidence. - Downstream ruleset references. Job names (
lint,test,docs,ci-gate) are unchanged, so the spec's reasoning holds for the documented case — but whether any downstream project's branch ruleset or required check pins the workflow file or the workflow name (CI/CD) lives in GitHub repo settings and is not observable from this diff.
Non-blocking notes
Three findings, all inline: one minor (parametrize IDs collide to ci.yaml0/ci.yaml1), two nits. None should hold this PR.
~Written by Claude, run via the agentic engineering loop
| CI_WORKFLOWS = [ | ||
| TEMPLATE_ROOT / ".github" / "workflows" / "ci.yaml", | ||
| TEMPLATE_ROOT / "template" / ".github" / "workflows" / "cicd.yaml", | ||
| TEMPLATE_ROOT / "template" / ".github" / "workflows" / "ci.yaml", |
There was a problem hiding this comment.
Minor (legibility, introduced by this diff): both CI_WORKFLOWS entries are now called ci.yaml, so the two parametrized tests that use ids=lambda p: p.name (lines 342 and 364) no longer produce distinguishable test IDs. Confirmed by collection on this HEAD:
test_ci_gate_covers_every_job[ci.yaml0]
test_ci_gate_covers_every_job[ci.yaml1]
test_ci_workflow_triggers[ci.yaml0]
test_ci_workflow_triggers[ci.yaml1]
Before the rename these read [ci.yaml] and [cicd.yaml], so a failure named the offending workflow. Now the reader has to count list order to know whether the repo's own workflow or the template's broke. Correctness is unaffected — I verified the two paths still resolve to two different files, so the ci-gate tests have not silently collapsed to asserting one file twice.
Suggested fix (one line, no behaviour change):
ids=lambda p: str(p.relative_to(TEMPLATE_ROOT))Not blocking, and the spec's "no test changes beyond keeping the suite green" arguably scopes it out of this PR.
~Written by Claude, run via the agentic engineering loop
| # 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 | ||
| # rendered, so the template's ci.yaml is copied verbatim (which is also why |
There was a problem hiding this comment.
Nit: now that both files are named ci.yaml, "the template's ci.yaml" is doing all the disambiguating work in this comment. It is still correct — and the possessive does distinguish it from the repo's own workflow — but template/.github/workflows/ci.yaml would leave nothing to infer.
~Written by Claude, run via the agentic engineering loop
| if ref.startswith("./") | ||
| ] | ||
| assert local == [("release.yaml", "./.github/workflows/cicd.yaml")] | ||
| assert local == [("release.yaml", "./.github/workflows/ci.yaml")] |
There was a problem hiding this comment.
Nit (pre-existing gap, not introduced here): this asserts the text of the local uses: ref but never that the referenced file exists in the generated project. A future rename that updated release.yaml and this assertion in lockstep, but missed the file itself, would still pass. Same for the README badge — nothing asserts it points at a workflow that exists.
The PR description already flags this as a follow-up candidate, and the spec explicitly scopes test additions out ("no test changes beyond keeping the suite green"), so this is a note for the human reviewer rather than a request. I closed the gap manually for this PR by rendering a project from the committed HEAD — see the summary.
~Written by Claude, run via the agentic engineering loop
|
Review iteration 3: approve — Fresh-context review of the full 3-commit diff (5 files, +87/−6). The change does exactly what issue #8 asked and nothing more: ~Written by Claude, run via the agentic engineering loop |
|
Review verdict: approved (tier 1) ~Written by Claude, run via the agentic engineering loop |
All acceptance criteria met, tree clean, committed on
feat/8-loop-refactor-rename-cicd-yaml-in-template.PR-ready summary
refactor: rename template
cicd.yamltoci.yamlWhat changed (commit
ef53df3, 3 files, +3/−3):template/.github/workflows/cicd.yaml→ci.yamlgit mv(recorded asR099);name: CI/CD→name: CItemplate/.github/workflows/release.yaml:28uses: ./.github/workflows/cicd.yaml→ci.yamltemplate/README.md.jinja:3actions/workflows/ci.yamlin both the image URL and the link targetWhy: the workflow holds only
lint,testanddocsjobs — releasing lives inrelease.yaml, publishing inpublish.yaml.cicd.yaml/name: CI/CDclaimed a responsibility the file doesn't have, and every generated project inherited the misnomer.ci.yamlalso matches this repo's own workflow name.Behavior for generated projects: a rendered project now gets
.github/workflows/{ci,publish,release}.yaml, withname: CIand a badge pointing atci.yaml. Verified by rendering from the committed HEAD, not just by reading the diff.Verification
uv run pytest tests— 20 passed. Note: the suite renders withvcs_ref="HEAD", so the pre-commit run did not exercise the change; the green result above is from a re-run after committing.git grep -i cicd -- template/— no matches (exit 1).git show --name-status -M—R099, so the rename is recorded as a rename.Impact / risk
git revert. No code, no_migrations, no test changes.lint,test,docs) — unchanged, so no required-check breaks.copier updatepropagates the rename on its own — hence no_migrationsentry. Two pre-existing template defects gate that in practice (no shipped.copier-answers.yml;_tasksaborting on update) — both explicitly out of scope, each with its own ticket, and neither introduced here.cicd.yamluntil they re-render. Accepted by the spec, noted rather than mitigated.Deliberately not done: the spec scopes out test changes ("no test changes beyond keeping the suite green"), so I added no regression test — the suite still has no coverage asserting workflow filenames in rendered output, and a future rename could silently break the badge or
release.yaml'suses:again. Flagging as a candidate follow-up, not a defect in this change.Fixes #8
~Written by Claude, run via the agentic engineering loop