chore: Check the templates, not only the actions #49

ahmad · 2026-09-05T09:35:29Z

ahmad commented

2026-09-05 09:35:29 +00:00

Issue

Closes #43

Problem

check-actions.py walks rglob("action.yml") and checks nothing else, so templates/release.yml — the file every new application copies its pipeline from — had no validation at all: not its YAML, not its shell.

That is the wrong file to leave unchecked. An action is consumed through a moving v3 tag, so a mistake surfaces the first time any pipeline runs it. A template is copied: broken shell is duplicated into an application repository before anything executes it, and each copy must then be fixed separately.

Found while adding a docker login step to that template (#37, PR #41). Its shell was correct, but only because I checked it by hand with the checker's own technique.

Solution

check_workflow() parses each templates/*.yml, walks jobs.<id>.steps[], and runs the same ${{ }} substitution and bash -n the actions get. Four cases added to the committed suite.

Review notes

Every new case fails against the unfixed checker. That is the whole point of adding them here rather than verifying by hand as the last three changes in this repository did:

against the current checker      24/28  — all four template cases FAIL
against this change              28/28

Specifically: the syntax-error, not-YAML and jobs-not-a-mapping cases all exited 0 before, because a file that is never read cannot fail — which looks exactly like a clean run.

A workflow is not an action, so the traversal is new even though the checks are not. Bodies live at jobs.<id>.steps[] rather than runs.steps[]; the substitution and bash -n are reused. Only the YAML and the shell are checked — the fields a workflow needs are the runner's business, and a template is deliberately incomplete anyway.

The message names the job. A template has several, and step 1 alone would not say which. The syntax-error case asserts on job 'publish' step 1 for that reason.

The placeholder question, answered by measuring rather than assuming. I raised it on the issue: would a bare <OWNER>/<APP> inside a run: body need substituting or exempting? There are none — templates/release.yml has 7 run bodies and every placeholder in the file sits in uses:, with: or env: values, which are not shell. So no exemption today, and if one ever appears bash -n reports a syntax error naming the line, which is the right way to find out.

One detail the fixtures force, and it would have made a case pass while proving nothing. main() exits non-zero with "No action.yml found" when a tree has none, so a template-only fixture would fail for the wrong reason. Each template fixture gets a valid action.yml alongside it; the docstring on run_template_case says why, so nobody removes it as clutter.

Risks and trade-offs

templates/*.yml is a non-recursive glob, matching the directory's flat shape today. A template placed in a subdirectory would be silently skipped — the same class of defect as the one being fixed. rglob would be more defensive but would also pick up any stray YAML someone parks under templates/, and the checker cannot tell a workflow from an arbitrary document. Worth revisiting if that directory ever grows structure.

This checks shell syntax, not shell correctness: rm -rf / parses cleanly. It catches the class of mistake that reached four repositories unnoticed, not the class that needs a runtime — which is #48.

### Issue Closes #43 ### Problem `check-actions.py` walks `rglob("action.yml")` and checks nothing else, so `templates/release.yml` — the file every new application copies its pipeline from — had no validation at all: not its YAML, not its shell. That is the wrong file to leave unchecked. An action is consumed through a moving `v3` tag, so a mistake surfaces the first time any pipeline runs it. A template is **copied**: broken shell is duplicated into an application repository before anything executes it, and each copy must then be fixed separately. Found while adding a `docker login` step to that template (#37, PR #41). Its shell was correct, but only because I checked it by hand with the checker's own technique. ### Solution `check_workflow()` parses each `templates/*.yml`, walks `jobs.<id>.steps[]`, and runs the same `${{ }}` substitution and `bash -n` the actions get. Four cases added to the committed suite. ### Review notes **Every new case fails against the unfixed checker.** That is the whole point of adding them here rather than verifying by hand as the last three changes in this repository did: ``` against the current checker 24/28 — all four template cases FAIL against this change 28/28 ``` Specifically: the syntax-error, not-YAML and jobs-not-a-mapping cases all exited **0** before, because a file that is never read cannot fail — which looks exactly like a clean run. **A workflow is not an action, so the traversal is new even though the checks are not.** Bodies live at `jobs.<id>.steps[]` rather than `runs.steps[]`; the substitution and `bash -n` are reused. Only the YAML and the shell are checked — the fields a workflow needs are the runner's business, and a template is deliberately incomplete anyway. **The message names the job.** A template has several, and `step 1` alone would not say which. The syntax-error case asserts on `job 'publish' step 1` for that reason. **The placeholder question, answered by measuring rather than assuming.** I raised it on the issue: would a bare `<OWNER>/<APP>` inside a `run:` body need substituting or exempting? There are none — `templates/release.yml` has 7 run bodies and every placeholder in the file sits in `uses:`, `with:` or `env:` values, which are not shell. So no exemption today, and if one ever appears `bash -n` reports a syntax error naming the line, which is the right way to find out. **One detail the fixtures force, and it would have made a case pass while proving nothing.** `main()` exits non-zero with "No action.yml found" when a tree has none, so a template-only fixture would fail for the wrong reason. Each template fixture gets a valid `action.yml` alongside it; the docstring on `run_template_case` says why, so nobody removes it as clutter. ### Risks and trade-offs `templates/*.yml` is a non-recursive glob, matching the directory's flat shape today. A template placed in a subdirectory would be silently skipped — the same class of defect as the one being fixed. `rglob` would be more defensive but would also pick up any stray YAML someone parks under `templates/`, and the checker cannot tell a workflow from an arbitrary document. Worth revisiting if that directory ever grows structure. This checks shell syntax, not shell correctness: `rm -rf /` parses cleanly. It catches the class of mistake that reached four repositories unnoticed, not the class that needs a runtime — which is `#48`.

ahmad added 1 commit

2026-09-05 09:35:30 +00:00

chore: Check the templates, not only the actions

PR / validate (pull_request) Successful in 1m7s

Details

01104e18ec

An action is consumed through a moving tag, so a mistake surfaces the
first time any pipeline runs it. A template is copied — broken shell
reaches an application repository before anything executes it, and
each copy must then be fixed separately.

The templates' YAML and run bodies are now checked with the same
substitution and bash -n, reporting the job as well as the step since
a template has several.

Closes #43

ahmad commented

2026-09-05 09:37:23 +00:00

validate green on 01104e1. Verification against each acceptance criterion — and unlike the recent changes here, these are committed cases that will run again rather than a table in a comment.

Given a template with a syntactically invalid run: body, when the checker runs, then it fails and names the file, the job and the step.

templates/fixture.yml: job 'publish' step 1 has a shell syntax error: line 3: syntax error: unexpected end of file
exit 1

Given a template that is not valid YAML, when the checker runs, then it fails and names the file.

templates/fixture.yml: does not parse as YAML: …
exit 1

Given the templates as they stand today, when the checker runs, then it passes.

templates/release.yml: ok (7 run step(s) checked)

Seven, not zero — the count is the part worth reading, since "0 run step(s) checked" is what a file that is silently skipped looks like.

Given the change, when the checker's own test suite runs, then it covers a failing template and a passing one.

Four cases in TEMPLATE_CASES: valid, syntax error, not YAML, and jobs not a mapping. Each fails against the unfixed checker:

against the current checker   24/28   all four template cases FAIL
against this change           28/28

The three failing-input cases exited 0 before this change, because a file that is never read cannot fail — which is indistinguishable from a clean run, and is exactly the defect.

A fourth case beyond the criteria: jobs: as a list rather than a mapping. It is the shape that yields no bodies while looking parseable, and without it the checker would print 0 run step(s) checked and pass — the failure this repository's own history is made of.

Not covered: shell correctness. rm -rf / parses cleanly. This catches the class that reached four repositories unnoticed, not the class that needs a runtime — #48.

`validate` green on `01104e1`. Verification against each acceptance criterion — and unlike the recent changes here, these are committed cases that will run again rather than a table in a comment. > Given a template with a syntactically invalid `run:` body, when the checker runs, then it fails and names the file, the job and the step. ``` templates/fixture.yml: job 'publish' step 1 has a shell syntax error: line 3: syntax error: unexpected end of file exit 1 ``` > Given a template that is not valid YAML, when the checker runs, then it fails and names the file. ``` templates/fixture.yml: does not parse as YAML: … exit 1 ``` > Given the templates as they stand today, when the checker runs, then it passes. ``` templates/release.yml: ok (7 run step(s) checked) ``` Seven, not zero — the count is the part worth reading, since "0 run step(s) checked" is what a file that is silently skipped looks like. > Given the change, when the checker's own test suite runs, then it covers a failing template and a passing one. Four cases in `TEMPLATE_CASES`: valid, syntax error, not YAML, and `jobs` not a mapping. **Each fails against the unfixed checker:** ``` against the current checker 24/28 all four template cases FAIL against this change 28/28 ``` The three failing-input cases exited **0** before this change, because a file that is never read cannot fail — which is indistinguishable from a clean run, and is exactly the defect. **A fourth case beyond the criteria:** `jobs:` as a list rather than a mapping. It is the shape that yields no bodies while looking parseable, and without it the checker would print `0 run step(s) checked` and pass — the failure this repository's own history is made of. **Not covered:** shell *correctness*. `rm -rf /` parses cleanly. This catches the class that reached four repositories unnoticed, not the class that needs a runtime — `#48`.

ahmad changed title from ~~WIP: chore: Check the templates, not only the actions~~ to chore: Check the templates, not only the actions

2026-09-05 09:37:28 +00:00

ahmad_bot reviewed

2026-09-05 10:02:41 +00:00

ahmad_bot left a comment

Two findings, both non-blocking and both prospective — neither is live against the templates in the tree today. CI green.

I reproduced the before/after rather than taking the numbers, because that claim is the whole justification for the change:

new suite against the current checker   24/28
new suite against this change           28/28

And the four that fail do so in the right way. Three of them exited 0 with fixture/action.yml: ok (1 run step(s) checked) — a broken template producing a clean-looking pass, which is precisely the defect being fixed rather than merely a missing feature:

FAIL a shell syntax error in a template is reported with its job: exit 0, expected 1
FAIL a template that is not YAML is reported, not skipped:        exit 0, expected 1
FAIL a template whose jobs is not a mapping is reported:          exit 0, expected 1

This is the first change in this repository to commit the fixtures rather than verify by hand and discard them, and the difference is not cosmetic: the last three changes here — including two of mine to review — rest on evidence about a commit rather than on something that runs again. That the suite fails against the unfixed checker is what makes it a test rather than a description.

The placeholder question was settled by counting, which was the right instinct. Seven run bodies in templates/release.yml, every placeholder in uses:/with:/env: values, so no exemption is needed — and if one ever appears, bash -n names the line. Better than either assuming it is fine or adding an exemption nothing needs.

On the fixture detail: giving each template fixture a valid action.yml because main() fails a tree with none, and documenting why in the docstring, is the kind of thing that gets deleted as clutter a year later and then quietly makes a case pass for the wrong reason. Worth the two lines.

The findings are inline: a malformed steps skipped silently, and *.yaml not matched.

Two findings, both non-blocking and both prospective — neither is live against the templates in the tree today. CI green. **I reproduced the before/after rather than taking the numbers**, because that claim is the whole justification for the change: ``` new suite against the current checker 24/28 new suite against this change 28/28 ``` And the four that fail do so in the right way. Three of them exited **0** with `fixture/action.yml: ok (1 run step(s) checked)` — a broken template producing a clean-looking pass, which is precisely the defect being fixed rather than merely a missing feature: ``` FAIL a shell syntax error in a template is reported with its job: exit 0, expected 1 FAIL a template that is not YAML is reported, not skipped: exit 0, expected 1 FAIL a template whose jobs is not a mapping is reported: exit 0, expected 1 ``` **This is the first change in this repository to commit the fixtures rather than verify by hand and discard them**, and the difference is not cosmetic: the last three changes here — including two of mine to review — rest on evidence about a commit rather than on something that runs again. That the suite fails against the unfixed checker is what makes it a test rather than a description. **The placeholder question was settled by counting, which was the right instinct.** Seven run bodies in `templates/release.yml`, every placeholder in `uses:`/`with:`/`env:` values, so no exemption is needed — and if one ever appears, `bash -n` names the line. Better than either assuming it is fine or adding an exemption nothing needs. **On the fixture detail:** giving each template fixture a valid `action.yml` because `main()` fails a tree with none, and documenting *why* in the docstring, is the kind of thing that gets deleted as clutter a year later and then quietly makes a case pass for the wrong reason. Worth the two lines. The findings are inline: a malformed `steps` skipped silently, and `*.yaml` not matched.

.forgejo/scripts/check-actions.py

					
				@ -140,0 +173,4 @@

				        if not isinstance(job, dict):

				            problems.append(f"{path}: job {job_name!r} is {type(job).__name__}, expected a mapping")

				            continue

				        steps = job.get("steps")

A job whose steps is present but malformed is skipped silently, and if it is the only job the template reports a clean pass:

jobs:
  build:
    steps: "oops not a list"

templates/fixture.yml: ok (0 run step(s) checked)     exit 0

That is the exact shape this file's own test docstring names — "a validator that stops validating reports success, which is the failure it exists to prevent" — and you applied that reasoning one level up, for jobs not being a mapping, in the comment three lines above this one.

The reason it is continue rather than a report is sound and must be preserved: a job that calls a reusable workflow has no steps, and that is legitimate. So the fix is to separate absent from present-but-wrong-type rather than to report everything:

steps = job.get("steps")
if steps is None:
    # A job that calls a reusable workflow has no steps of its own.
    continue
if not isinstance(steps, list):
    problems.append(f"{path}: job {job_name!r} steps is {type(steps).__name__}, expected a list")
    continue

Verified against the current head: steps: "oops", absent steps, and steps: (null) all three produce ok (0 run step(s) checked) today, where only the middle one should.

Not live — templates/release.yml has four jobs and all four have list steps — so this is about the next template, which is the same argument the pull request makes for checking templates at all.

A job whose `steps` is present but malformed is skipped silently, and if it is the only job the template reports a clean pass: ``` jobs: build: steps: "oops not a list" ``` ``` templates/fixture.yml: ok (0 run step(s) checked) exit 0 ``` That is the exact shape this file's own test docstring names — *"a validator that stops validating reports success, which is the failure it exists to prevent"* — and you applied that reasoning one level up, for `jobs` not being a mapping, in the comment three lines above this one. The reason it is `continue` rather than a report is sound and must be preserved: a job that calls a reusable workflow has **no** `steps`, and that is legitimate. So the fix is to separate absent from present-but-wrong-type rather than to report everything: ```python steps = job.get("steps") if steps is None: # A job that calls a reusable workflow has no steps of its own. continue if not isinstance(steps, list): problems.append(f"{path}: job {job_name!r} steps is {type(steps).__name__}, expected a list") continue ``` Verified against the current head: `steps: "oops"`, absent `steps`, and `steps:` (null) all three produce `ok (0 run step(s) checked)` today, where only the middle one should. Not live — `templates/release.yml` has four jobs and all four have list `steps` — so this is about the next template, which is the same argument the pull request makes for checking templates at all.

.forgejo/scripts/check-actions.py Outdated

					
				@ -156,0 +215,4 @@

				    # wrong here is duplicated into application repositories before it runs.

				    # Absent templates are not an error: not every repository using this script

				    # has any.

				    for path in sorted((ROOT / "templates").glob("*.yml")):

glob("*.yml") also misses *.yaml. You flagged the subdirectory case in the risks section; this is the same class and cheaper to close:

templates/broken.yaml   (a real shell syntax error)  ->  no output at all, exit 0

Not a line saying it was skipped — nothing, which is indistinguishable from a directory with no templates. glob("*.yml") plus glob("*.yaml"), or a tuple check on the suffix, closes it without inviting the arbitrary-YAML problem that made you reject rglob.

Only templates/release.yml exists today, so this is prospective too.

`glob("*.yml")` also misses `*.yaml`. You flagged the subdirectory case in the risks section; this is the same class and cheaper to close: ``` templates/broken.yaml (a real shell syntax error) -> no output at all, exit 0 ``` Not a line saying it was skipped — nothing, which is indistinguishable from a directory with no templates. `glob("*.yml")` plus `glob("*.yaml")`, or a tuple check on the suffix, closes it without inviting the arbitrary-YAML problem that made you reject `rglob`. Only `templates/release.yml` exists today, so this is prospective too.

ahmad changed title from ~~chore: Check the templates, not only the actions~~ to WIP: chore: Check the templates, not only the actions

2026-09-05 10:04:17 +00:00

ahmad added 1 commit

2026-09-05 10:05:54 +00:00

fix: Report a malformed steps, and match .yaml too

PR / validate (pull_request) Successful in 1m9s

Details

ac28157bf6

A job whose steps was present but not a list was skipped silently, so
a template with one such job reported ok (0 run step(s) checked) — a
validator that stops validating reporting success. Absent steps stays
legitimate: a job calling a reusable workflow has none.

A .yaml template matched neither glob and produced no output at all,
indistinguishable from a directory with no templates.

Refs #43

ahmad commented

2026-09-05 10:06:14 +00:00

Both taken, fixed in ac28157, and both are now committed cases rather than a table here — which is this pull request's own argument, so it would have been poor to fix them any other way.

The malformed steps is the better find, and I had already made the argument one level up. Three lines above it I explained why jobs not being a mapping must be reported rather than skipped — and then skipped a malformed steps on the next line. Your framing of the fix is the one I took: absent and present-but-wrong-type are different, because a job calling a reusable workflow legitimately has no steps.

steps: "oops not a list"   ->  job 'build' steps is str, expected a list   exit 1
steps absent (uses:)       ->  ok (0 run step(s) checked)                  exit 0

The .yaml case, likewise, and I parameterised the fixture filename so it is a real case rather than an assertion about a glob:

templates/fixture.yaml with a shell syntax error  ->  job 'build' step 0 has a shell syntax error   exit 1

Three cases added, and I want to be precise about what each proves, because two kill and one does not:

against this PR's previous head (01104e1)   29/31
  FAIL  a job whose steps is malformed is reported, not silently skipped
  FAIL  a .yaml template is checked too, not silently skipped
against this change                          31/31

The third — "a job with no steps at all is legitimate and does not fail" — passes against both heads. That is correct rather than a weak test: it asserts behaviour that was already right, and exists to stop my own fix turning a legitimate reusable-workflow job into a failure. A regression guard, not a demonstration, and worth saying so rather than letting three cases look like three proofs.

On committing the fixtures: agreed, and your point about the docstring is why I wrote it. The action.yml alongside each template fixture looks like clutter until the day someone removes it and three cases start passing for the wrong reason.

Both taken, fixed in `ac28157`, and both are now committed cases rather than a table here — which is this pull request's own argument, so it would have been poor to fix them any other way. **The malformed `steps` is the better find, and I had already made the argument one level up.** Three lines above it I explained why `jobs` not being a mapping must be *reported* rather than skipped — and then skipped a malformed `steps` on the next line. Your framing of the fix is the one I took: absent and present-but-wrong-type are different, because a job calling a reusable workflow legitimately has no steps. ``` steps: "oops not a list" -> job 'build' steps is str, expected a list exit 1 steps absent (uses:) -> ok (0 run step(s) checked) exit 0 ``` **The `.yaml` case, likewise**, and I parameterised the fixture filename so it is a real case rather than an assertion about a glob: ``` templates/fixture.yaml with a shell syntax error -> job 'build' step 0 has a shell syntax error exit 1 ``` **Three cases added, and I want to be precise about what each proves**, because two kill and one does not: ``` against this PR's previous head (01104e1) 29/31 FAIL a job whose steps is malformed is reported, not silently skipped FAIL a .yaml template is checked too, not silently skipped against this change 31/31 ``` The third — *"a job with no steps at all is legitimate and does not fail"* — passes against **both** heads. That is correct rather than a weak test: it asserts behaviour that was already right, and exists to stop my own fix turning a legitimate reusable-workflow job into a failure. A regression guard, not a demonstration, and worth saying so rather than letting three cases look like three proofs. **On committing the fixtures:** agreed, and your point about the docstring is why I wrote it. The `action.yml` alongside each template fixture looks like clutter until the day someone removes it and three cases start passing for the wrong reason.

ahmad commented

2026-09-05 10:07:48 +00:00

validate green on ac28157; back to ready.

The delta since the reviewed head is the two findings and three cases for them, verified above — two failing against the previous head, the third a regression guard that passes both ways and is labelled as such.

Everything else stands: templates/release.yml: ok (7 run step(s) checked), and the original four cases still failing 24/28 against the checker before this branch.

`validate` green on `ac28157`; back to ready. The delta since the reviewed head is the two findings and three cases for them, verified above — two failing against the previous head, the third a regression guard that passes both ways and is labelled as such. Everything else stands: `templates/release.yml: ok (7 run step(s) checked)`, and the original four cases still failing 24/28 against the checker before this branch.

ahmad changed title from ~~WIP: chore: Check the templates, not only the actions~~ to chore: Check the templates, not only the actions

2026-09-05 10:07:51 +00:00

ahmad_bot approved these changes

2026-09-05 10:31:19 +00:00

ahmad_bot left a comment

Approved. Both findings are closed, and closed with cases that fail against the head I reviewed. Re-reviewed only the delta (01104e18ec..ac28157bf6); CI green.

Ran the new suite against both heads, which is the check that matters for a change to a checker:

against this head        31/31
against the previous     29/31
    FAIL a job whose steps is malformed is reported, not silently skipped: exit 0, expected 1
    FAIL a .yaml template is checked too, not silently skipped:            exit 0, expected 1

Both fail with exit 0 — the silent-pass shape rather than a missing feature — so they are tests of the defects rather than descriptions of the fixes.

The third new case is a guard, not a kill, and that is worth saying explicitly since a mutation table can otherwise read as three-for-three. a job with no steps at all is legitimate and does not fail passes on both heads, because the legitimate path was never broken. It earns its place for a different reason: it pins the distinction between absent and malformed, so the next person tightening this loop cannot start failing jobs that call a reusable workflow without a red test. Adding it alongside the two kills is the right instinct — the risk in my suggested fix was always over-reporting, not under-reporting.

On set(glob("*.yml")) | set(glob("*.yaml")) rather than rglob: this keeps the reason you rejected rglob intact — arbitrary YAML parked under templates/ that the script cannot distinguish from a workflow — while closing the case where a file produces no output at all. That distinction, between "reported as skipped" and "silently absent", is the one this whole pull request is about, and the comment now says so at the call site.

Nothing further from me.

Approved. Both findings are closed, and closed with cases that fail against the head I reviewed. Re-reviewed only the delta (`01104e18ec..ac28157bf6`); CI green. **Ran the new suite against both heads, which is the check that matters for a change to a checker:** ``` against this head 31/31 against the previous 29/31 FAIL a job whose steps is malformed is reported, not silently skipped: exit 0, expected 1 FAIL a .yaml template is checked too, not silently skipped: exit 0, expected 1 ``` Both fail with `exit 0` — the silent-pass shape rather than a missing feature — so they are tests of the defects rather than descriptions of the fixes. **The third new case is a guard, not a kill, and that is worth saying explicitly** since a mutation table can otherwise read as three-for-three. `a job with no steps at all is legitimate and does not fail` passes on both heads, because the legitimate path was never broken. It earns its place for a different reason: it pins the distinction between absent and malformed, so the next person tightening this loop cannot start failing jobs that call a reusable workflow without a red test. Adding it alongside the two kills is the right instinct — the risk in my suggested fix was always over-reporting, not under-reporting. **On `set(glob("*.yml")) | set(glob("*.yaml"))` rather than `rglob`:** this keeps the reason you rejected `rglob` intact — arbitrary YAML parked under `templates/` that the script cannot distinguish from a workflow — while closing the case where a file produces *no output at all*. That distinction, between "reported as skipped" and "silently absent", is the one this whole pull request is about, and the comment now says so at the call site. Nothing further from me.

ahmad merged commit 032b9f88d6 into main

2026-09-05 10:32:47 +00:00

ahmad referenced this pull request from a commit

2026-09-05 10:32:48 +00:00

chore: Check the templates, not only the actions (#49)

ahmad deleted branch feature/43-check-templates

2026-09-05 10:32:48 +00:00

Sign in to join this conversation.

No reviewers