fix: Report a composite action that declares no steps #30

Merged

ahmad merged 1 commit from feature/28-composite-without-steps into main 2026-09-04 04:33:05 +00:00

Conversation 3 Commits 1 Files changed 1 +33 -10

ahmad commented 2026-09-04 03:35:16 +00:00

Owner

Copy link

Issue

Closes #28.

Problem

runs: {using: composite} with no steps: printed ok (0 run step(s) checked) — the exact shape this checker exists to make impossible. runs.get("steps") returns None both when the key is missing and when it is written with no value, and neither branch fired on None, so the file passed as though it had been checked.

Solution

Branch on the key's presence rather than its value, which closes both the absent key and the empty-value case.

On the second criterion — steps: [] — I am reporting it, with its own message. An action that declares no steps does nothing and the runner rejects it either way, so this is the same defect written differently rather than a separate shape deserving a separate decision. Distinct wording so the output says which was found; same exit code. If an empty list were ever a deliberate no-op, the right place to say so is a comment in the action, not silence in the checker.

Scoped to composite actions. A docker or node20 action has no steps: by design, and telling it to declare some would be wrong advice in a message someone acts on. Nothing is lost: a using that is not composite is already reported.

Review notes

Ran the same seven fixtures against origin/main and against this branch, each in an isolated tree so nothing lands where rglob would find it:

fixture	before	after
no steps: key	ok (0 run step(s) checked)	runs.steps is missing; a composite action must declare at least one step
steps: with no value	ok (0 run step(s) checked)	runs.steps is NoneType, expected a list
steps: []	ok (0 run step(s) checked)	runs.steps is empty; a composite action must declare at least one step
steps: nope	reported	reported, unchanged
a step that is not a mapping	reported	reported, unchanged
valid composite	ok, 1 step checked	ok, 1 step checked
using: node20, no steps	runs.using is 'node20'	runs.using is 'node20' — and no steps complaint, which is the point of the gating

The three previously-silent shapes are the whole of the change. The six real actions in this repository still pass with the same step counts (5, 1, 1, 1, 3, 4), and README lint is clean.

A gap I am not closing here. There is no test for check-actions.py itself, so the table above is evidence about this commit rather than a guard against the next one. That is outside what #28 asks for and I have not widened the pull request to add one; I will open it as its own issue.

Risks and trade-offs

• Reporting steps: [] is a judgement, not a certainty. If someone has an intentional no-op composite action somewhere, this now fails their pipeline. Nothing in this repository is that shape, and I would rather be told and reverse it than have the checker stay quiet about an action that does nothing.
• The checker still cannot execute an action, so a valid-but-wrong command reaches consumers when the v3 tag moves. Unchanged by this, and already noted in the knowledge file.

### Issue Closes #28. ### Problem `runs: {using: composite}` with no `steps:` printed `ok (0 run step(s) checked)` — the exact shape this checker exists to make impossible. `runs.get("steps")` returns `None` both when the key is missing and when it is written with no value, and neither branch fired on `None`, so the file passed as though it had been checked. ### Solution Branch on the key's presence rather than its value, which closes both the absent key and the empty-value case. **On the second criterion — `steps: []` — I am reporting it**, with its own message. An action that declares no steps does nothing and the runner rejects it either way, so this is the same defect written differently rather than a separate shape deserving a separate decision. Distinct wording so the output says which was found; same exit code. If an empty list were ever a deliberate no-op, the right place to say so is a comment in the action, not silence in the checker. **Scoped to composite actions.** A `docker` or `node20` action has no `steps:` by design, and telling it to declare some would be wrong advice in a message someone acts on. Nothing is lost: a `using` that is not composite is already reported. ### Review notes Ran the same seven fixtures against `origin/main` and against this branch, each in an isolated tree so nothing lands where `rglob` would find it: | fixture | before | after | |---|---|---| | no `steps:` key | **ok (0 run step(s) checked)** | `runs.steps is missing; a composite action must declare at least one step` | | `steps:` with no value | **ok (0 run step(s) checked)** | `runs.steps is NoneType, expected a list` | | `steps: []` | **ok (0 run step(s) checked)** | `runs.steps is empty; a composite action must declare at least one step` | | `steps: nope` | reported | reported, unchanged | | a step that is not a mapping | reported | reported, unchanged | | valid composite | ok, 1 step checked | ok, 1 step checked | | `using: node20`, no steps | `runs.using is 'node20'` | `runs.using is 'node20'` — **and no steps complaint**, which is the point of the gating | The three previously-silent shapes are the whole of the change. The six real actions in this repository still pass with the same step counts (5, 1, 1, 1, 3, 4), and README lint is clean. **A gap I am not closing here.** There is no test for `check-actions.py` itself, so the table above is evidence about this commit rather than a guard against the next one. That is outside what #28 asks for and I have not widened the pull request to add one; I will open it as its own issue. ### Risks and trade-offs - Reporting `steps: []` is a judgement, not a certainty. If someone has an intentional no-op composite action somewhere, this now fails their pipeline. Nothing in this repository is that shape, and I would rather be told and reverse it than have the checker stay quiet about an action that does nothing. - The checker still cannot execute an action, so a valid-but-wrong command reaches consumers when the `v3` tag moves. Unchanged by this, and already noted in the knowledge file.

ahmad added 1 commit 2026-09-04 03:35:16 +00:00

fix: Report a composite action that declares no steps ... d829fa922d

runs.get("steps") returns None both when the key is missing and when
it is written with no value, and neither branch fired on None, so
runs: {using: composite} with no steps printed ok (0 run step(s)
checked) — the shape this checker exists to make impossible.

Branch on the key's presence instead. An empty list is reported too,
with its own message: a composite action that declares no steps does
nothing and the runner rejects it either way, so the two are the same
defect written differently.

Scoped to composite actions, because a docker or node20 action has no
steps by design and telling it to declare some would be wrong advice.

ahmad commented 2026-09-04 04:02:48 +00:00

Author

Owner

Copy link

Verification against #28's acceptance criteria

PR / validate green on d829fa9 — worth noting in itself, since every ahmad/expiro build has failed for four hours on the runner's Docker Hub egress. This pipeline runs validate alone and pulls no base layers, which is why the change could be finished at all tonight.

Given an action.yml with runs.using: composite and no steps: key, when the checker runs, then it reports the action and exits non-zero.

before:  fixture/action.yml: ok (0 run step(s) checked)                     exit 0
after:   runs.steps is missing; a composite action must declare at least
         one step                                                           exit 1

Given an action with steps: [], when the checker runs, then the behaviour is whatever this issue decides — state the choice and say why.

Reported, with its own message:

before:  fixture/action.yml: ok (0 run step(s) checked)                     exit 0
after:   runs.steps is empty; a composite action must declare at least
         one step                                                           exit 1

The reasoning, since the criterion asks for it: an action that declares no steps does nothing, and the runner rejects it whether the key is absent or the list is empty. Those are the same defect written two ways, so they get the same exit code and different wording. The alternative — treating an empty list as a deliberate no-op — would mean the checker stays silent about an action that cannot work, which is the failure mode this whole script exists to prevent. If someone does want a no-op composite action, a comment in the file is the honest way to say so.

A third shape I found while testing, not in the criteria. steps: written with no value at all parses as None and was equally silent; it is now runs.steps is NoneType, expected a list. Same root cause — runs.get("steps") cannot distinguish a missing key from an empty value — so fixing one without the other would have left half the bug.

What I checked beyond the criteria

• The two shapes that already worked are unchanged: steps: nope and a step that is not a mapping.
• A valid composite action still passes with its step counted.
• A using: node20 action is not told to declare steps — only that its using is wrong. This is why the new check is gated on composite: a docker or node20 action has no steps by design, and a message telling someone to add them is worse than no message.
• The six real actions in this repository pass with unchanged step counts (5, 1, 1, 1, 3, 4).
• README lint clean.

Each fixture ran in its own temporary tree, because rglob would otherwise find a deliberately-broken action.yml left under the repository and fail the real run.

Still true after this

There is no test for check-actions.py itself. The before-and-after table is evidence about this commit, not a guard against the next one — a later refactor could reintroduce exactly this and nothing would notice. Outside #28's scope, so it is a follow-up rather than something I widened this pull request to fix.

### Verification against #28's acceptance criteria `PR / validate` green on `d829fa9` — worth noting in itself, since every `ahmad/expiro` build has failed for four hours on the runner's Docker Hub egress. This pipeline runs `validate` alone and pulls no base layers, which is why the change could be finished at all tonight. **Given an `action.yml` with `runs.using: composite` and no `steps:` key, when the checker runs, then it reports the action and exits non-zero.** ``` before: fixture/action.yml: ok (0 run step(s) checked) exit 0 after: runs.steps is missing; a composite action must declare at least one step exit 1 ``` **Given an action with `steps: []`, when the checker runs, then the behaviour is whatever this issue decides — state the choice and say why.** Reported, with its own message: ``` before: fixture/action.yml: ok (0 run step(s) checked) exit 0 after: runs.steps is empty; a composite action must declare at least one step exit 1 ``` The reasoning, since the criterion asks for it: an action that declares no steps does nothing, and the runner rejects it whether the key is absent or the list is empty. Those are the same defect written two ways, so they get the same exit code and different wording. The alternative — treating an empty list as a deliberate no-op — would mean the checker stays silent about an action that cannot work, which is the failure mode this whole script exists to prevent. If someone does want a no-op composite action, a comment in the file is the honest way to say so. **A third shape I found while testing, not in the criteria.** `steps:` written with no value at all parses as `None` and was equally silent; it is now `runs.steps is NoneType, expected a list`. Same root cause — `runs.get("steps")` cannot distinguish a missing key from an empty value — so fixing one without the other would have left half the bug. ### What I checked beyond the criteria - The two shapes that already worked are unchanged: `steps: nope` and a step that is not a mapping. - A valid composite action still passes with its step counted. - **A `using: node20` action is not told to declare steps** — only that its `using` is wrong. This is why the new check is gated on `composite`: a `docker` or `node20` action has no steps by design, and a message telling someone to add them is worse than no message. - The six real actions in this repository pass with unchanged step counts (5, 1, 1, 1, 3, 4). - README lint clean. Each fixture ran in its own temporary tree, because `rglob` would otherwise find a deliberately-broken `action.yml` left under the repository and fail the real run. ### Still true after this There is no test for `check-actions.py` itself. The before-and-after table is evidence about this commit, not a guard against the next one — a later refactor could reintroduce exactly this and nothing would notice. Outside #28's scope, so it is a follow-up rather than something I widened this pull request to fix.

ahmad changed title from WIP: fix: Report a composite action that declares no steps to fix: Report a composite action that declares no steps 2026-09-04 04:02:49 +00:00

ahmad_bot approved these changes 2026-09-04 04:32:08 +00:00

ahmad_bot left a comment

Copy link

Approving. I reproduced your table independently against origin/main and this branch, in isolated trees, and added two fixtures you did not list:

fixture	before	after
no steps: key	ok (0 run step(s) checked)	runs.steps is missing; a composite action must declare at least one step
steps: with no value	ok (0 run step(s) checked)	runs.steps is NoneType, expected a list
steps: []	ok (0 run step(s) checked)	runs.steps is empty; a composite action must declare at least one step
steps: nope	reported	reported, unchanged
a step that is not a mapping	reported	reported, unchanged
valid composite	ok, 1 step	ok, 1 step
using: node20, no steps	runs.using is 'node20'	same, and no steps complaint
composite whose steps are all uses:	ok, 0 run steps	ok, 0 run steps
using: node20 with steps: nope	reported (steps)	reported (using), exit 1

The last two are the ones I wanted to see. A composite action whose steps are all uses: is legitimate and declares steps, and it is exactly what an "empty means broken" rule could have caught wrongly — it does not, because the check is on the list being empty rather than on the run-body count. And gating on using does not let anything escape: a non-composite action with a malformed steps no longer gets that message, but the file still fails on using in the same run, so the exit code is unchanged and nothing passes that did not before.

Against #28. The first criterion holds. The third is proved by validate being green, since that job runs the checker over this repository. The second asked for a decision rather than a behaviour, and you gave one with its reasoning — that an action declaring no steps does nothing and the runner rejects it either way, so steps: [] is the same defect written differently. I agree: a distinct message and the same exit code is the right shape, and the alternative — staying quiet about an action that does nothing — is the failure this checker exists to prevent. Your note that an intentional no-op belongs in a comment rather than in the checker's silence is the right way round.

On the gap you named and did not close: check-actions.py now has five validation branches and no test of its own, in the repository whose whole problem is that a change to a shared action reaches the fleet unexecuted. The checker is the gate, and a gate with no test is the same shape as the thing it guards against. Leaving it out of this PR is right — #28 does not ask for it and widening a change to cover an adjacent gap is how a reviewable PR stops being one — and the table above is evidence about this commit rather than a guard against the next. Worth making sure that issue actually gets opened, because this is the second time the argument for it has been written down.

Approval does not merge — and merging moves v3.

Approving. I reproduced your table independently against `origin/main` and this branch, in isolated trees, and added two fixtures you did not list: | fixture | before | after | |---|---|---| | no `steps:` key | ok (0 run step(s) checked) | `runs.steps is missing; a composite action must declare at least one step` | | `steps:` with no value | ok (0 run step(s) checked) | `runs.steps is NoneType, expected a list` | | `steps: []` | ok (0 run step(s) checked) | `runs.steps is empty; a composite action must declare at least one step` | | `steps: nope` | reported | reported, unchanged | | a step that is not a mapping | reported | reported, unchanged | | valid composite | ok, 1 step | ok, 1 step | | `using: node20`, no steps | `runs.using is 'node20'` | same, and no steps complaint | | **composite whose steps are all `uses:`** | ok, 0 run steps | **ok, 0 run steps** | | **`using: node20` *with* `steps: nope`** | reported (steps) | **reported (`using`), exit 1** | The last two are the ones I wanted to see. A composite action whose steps are all `uses:` is legitimate and declares steps, and it is exactly what an "empty means broken" rule could have caught wrongly — it does not, because the check is on the list being empty rather than on the run-body count. And gating on `using` does not let anything escape: a non-composite action with a malformed `steps` no longer gets that message, but the file still fails on `using` in the same run, so the exit code is unchanged and nothing passes that did not before. **Against #28.** The first criterion holds. The third is proved by `validate` being green, since that job runs the checker over this repository. The second asked for a decision rather than a behaviour, and you gave one with its reasoning — that an action declaring no steps does nothing and the runner rejects it either way, so `steps: []` is the same defect written differently. I agree: a distinct message and the same exit code is the right shape, and the alternative — staying quiet about an action that does nothing — is the failure this checker exists to prevent. Your note that an intentional no-op belongs in a comment rather than in the checker's silence is the right way round. **On the gap you named and did not close:** `check-actions.py` now has five validation branches and no test of its own, in the repository whose whole problem is that a change to a shared action reaches the fleet unexecuted. The checker is the gate, and a gate with no test is the same shape as the thing it guards against. Leaving it out of this PR is right — #28 does not ask for it and widening a change to cover an adjacent gap is how a reviewable PR stops being one — and the table above is evidence about this commit rather than a guard against the next. Worth making sure that issue actually gets opened, because this is the second time the argument for it has been written down. Approval does not merge — and merging moves `v3`.

ahmad commented 2026-09-04 04:33:01 +00:00

Author

Owner

Copy link

Merging. Two things worth answering first.

Your two extra fixtures are better than the seven I ran, and the first one is the one I should have thought of. A composite action whose steps are all uses: declares steps and checks zero run bodies — it is precisely what an "empty means broken" rule catches wrongly, and I did not test it. It passes because the check is on the list being empty rather than on the run-body count, which is correct by construction rather than by my having considered it. Worth saying plainly: that distinction is load-bearing and I got it right without noticing the alternative.

The second — using: node20 with a malformed steps — closes the gap my gating opened. It loses the steps message but still fails on using in the same run, so nothing passes that did not before. That is the check I would have wanted and did not run.

The issue is opened: #31. You were right to press on it — it is the second time the argument has been written down, and an argument that keeps being made and never filed is one that quietly becomes a decision not to do it. Its criteria include the two traps I hit while testing this one: fixtures have to live where ROOT.rglob("action.yml") cannot reach them, and each case must be proved by deleting the branch it covers rather than by watching it pass.

On v3 moving. Worth being explicit for the record, since a tag that every repository consumes is production infrastructure: this pull request touches .forgejo/scripts/check-actions.py and nothing else. No action.yml changes, so the release moves v3 to a commit whose actions are byte-identical to the current one. Consumers get a new tag and the same behaviour.

Merging. Two things worth answering first. **Your two extra fixtures are better than the seven I ran**, and the first one is the one I should have thought of. A composite action whose steps are all `uses:` declares steps and checks zero run bodies — it is precisely what an "empty means broken" rule catches wrongly, and I did not test it. It passes because the check is on the list being empty rather than on the run-body count, which is correct by construction rather than by my having considered it. Worth saying plainly: that distinction is load-bearing and I got it right without noticing the alternative. The second — `using: node20` with a malformed `steps` — closes the gap my gating opened. It loses the `steps` message but still fails on `using` in the same run, so nothing passes that did not before. That is the check I would have wanted and did not run. **The issue is opened: `#31`.** You were right to press on it — it is the second time the argument has been written down, and an argument that keeps being made and never filed is one that quietly becomes a decision not to do it. Its criteria include the two traps I hit while testing this one: fixtures have to live where `ROOT.rglob("action.yml")` cannot reach them, and each case must be proved by *deleting* the branch it covers rather than by watching it pass. **On `v3` moving.** Worth being explicit for the record, since a tag that every repository consumes is production infrastructure: this pull request touches `.forgejo/scripts/check-actions.py` and nothing else. No `action.yml` changes, so the release moves `v3` to a commit whose actions are byte-identical to the current one. Consumers get a new tag and the same behaviour.

ahmad merged commit 1543ab023d into main 2026-09-04 04:33:05 +00:00

ahmad referenced this pull request from a commit 2026-09-04 04:33:06 +00:00

fix: Report a composite action that declares no steps (#30)

ahmad referenced this pull request 2026-09-04 07:02:51 +00:00

test: Keep the fixtures that proved each checker fix #34

ahmad referenced this pull request 2026-09-05 23:35:06 +00:00

chore: Propagate the Docker Hub login to the eleven app repositories #42

Sign in to join this conversation.

Reviewers

No reviewers

ahmad_bot

Labels

Clear labels   

priority/P0

Drop everything

  

priority/P1

High

  

priority/P2

Normal

  

priority/P3

Low

  

size/L

Several days

  

size/M

A day or two

  

size/S

A few hours

  

size/XL

  

size/XS

Under an hour

  

state/done

Merged and verified

  

state/in-progress

Being worked on

  

state/in-review

PR open

  

state/needs-refinement

Not ready to pick up

  

state/ready

Refined and ready to start

  

type/bug

Something is broken

  

type/chore

Maintenance, tooling, setup

  

type/feature

New functionality

  

type/refactor

Restructuring without behaviour change

No labels

priority/P0

priority/P1

priority/P2

priority/P3

size/L

size/M

size/S

size/XL

size/XS

state/done

state/in-progress

state/in-review

state/needs-refinement

state/ready

type/bug

type/chore

type/feature

type/refactor

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

amtronics/platform-actions!30

No description provided.