Keel-Linux/.github

Fork 0

Name the appliance gate for what it proves, and stop passing a boot that did not happen #12

Merged

marcos-mendez merged 4 commits from ci/name-the-gate-for-what-it-proves into main

2026-09-29 00:20:35 -03:00

marcos-mendez commented

2026-09-28 00:50:28 -03:00

(Migrated from github.com)

Closes #11. Org-wide context: Keel-Linux/tracker#14 (left open; the design
question it records is wider than this repository).

The problem

appliance / build-and-boot reported success in two states where it says
nothing about the branch it is attached to.

The layer has never been published. Steps 6 to 10 were guarded by
if: steps.layer.outputs.published == 'true', were skipped, and the job
still concluded success: keel-redis#5, run 36369705307, green in 11 seconds
having pulled, verified and booted nothing. The step summary said "Nothing
was built, assembled or booted", which is honest prose attached to a
dishonest conclusion, because branch protection and gh pr checks read the
conclusion.
The layer is published. The job pulls the published layer from the
mirror and boots that, which on a pull request that changes the recipe is
the code before the change. The branch is never exercised.

Audit of the mirror, re-run before starting: eleven layers answer 200 at
https://mirror.keellinux.org/layers/<name>.manifest (core, apache-php,
nodejs-nginx, mariadb, postgresql, nodebb, wordpress, lamp, lamp-client, lapp,
lapp-client) and redis answers 404. So one repository is in the
unearned-green state and eleven checks are real, which is what makes the
second face the bigger one.

What this changes

The job id is boot-published-layer, not build-and-boot. The check is
now appliance / boot-published-layer. It keeps the value it has, catching a
published layer that does not boot and catching the parent mismatch it was
written for, and its name no longer reads as a claim about a branch. The job
summary repeats it in prose and prints the product_commit and pool date the
booted layer was built from, so a reviewer can see how old the thing that
booted is. Building from the branch instead is not attempted: that needs the
build host, which is serialised behind a lock, so it does not fit a
per-pull-request check.

An unpublished layer fails the job, unless the caller declares
allow_unpublished: true.

The decision is a tested script, bin/appliance-gate, which the job
checks out and runs (decision 0004). The job holds the curl; everything
concluded from the mirror's answer is in the script, where
tests/appliance-gate.bats asks it every question.

A decision that cannot be written down is fatal, exit 5. The append of
published= to $GITHUB_OUTPUT was unchecked in the first revision, so a
failed write was swallowed and the job concluded success having skipped all
six boot steps: this same defect by another road. The calling step now also
reads the file back and fails when neither true nor false was recorded.

tooling_ref, so a change to this workflow can be checked before it
merges instead of running against main's copy of the script. Default main;
no ordinary caller sets it, and a run that does not use main carries a
::warning:: naming the ref, on every path, for the same reason
allow_unpublished does.

A job summary that cannot be written is fatal too, same exit 5. It was the
same unchecked write as publish_decision, five lines away, and it drops the
caveats that are the only record that a passing job booted nothing.

Why this mechanism and not one of the other two

The issue offered three. Two of them do not survive contact with GitHub.

A neutral conclusion cannot be produced by a workflow job. Measured
rather than assumed: a probe on this repository, run 36372853847, gave a job
running exit 78, the old neutral exit code, the conclusion failure, and a
job skipped by if: the conclusion skipped. The Actions documentation on
using conditions to control job execution says of the latter, "A job that is
skipped will report its status as 'Success'. It will not prevent a pull
request from merging, even if it is a required check." So failure is the
only conclusion a workflow job can produce that withholds a merge. Anything
built on neutral would have to be a separate check run posted through the
Checks API under a name colliding with the job's own check, which I did not
test and am not proposing.

"Fail when the layer has ever been published before" is not knowable here.
The mirror serves the current manifest and no history, so a 404 today is
indistinguishable from a layer that was published and then deleted. There is
nowhere to read "ever" from without inventing state to carry it.

So: fail by default, with an exemption the caller declares. The remaining
choice is where the bootstrap exemption lives, and the answer is the caller's
own workflow file rather than the shared gate, because that is where the
person reviewing that repository will see it. Once the manifest answers 200 it
becomes an error telling you to delete the line. Its two limits are real and
are printed in the summary of every run that uses it: the error only stops
somebody where this check is a required status, and it keys on the name in
appliance:, so a typo answers 404 for ever and never expires. Neither can be
fixed from inside the gate, which cannot see a protection rule and cannot know
which name you meant.

keel-redis does not get the exemption. It is not bootstrapping. It has a
repository, a recipe, a boot test and a layer whose publication is in
progress, so the honest state of its gate is red with "publish the layer",
which is exactly the signal missing when keel-redis#5 looked ready to merge.
Its branch protection does not require this check, so nothing is blocked by
that red, and it is owned and dated in keel-redis#10. No caller sets
allow_unpublished today, which is the intended population: it exists for a
repository whose first layer does not exist yet.

How each behaviour was verified

Not by reasoning. Temporary callers were pointed at this branch and the runs
read back. All of them are closed and their branches deleted.

State	Where	Run	Result
404, no exemption	keel-redis	36373971968	failure in 12s at the gate step; steps 7 to 12 skipped
404, `allow_unpublished: true`	keel-redis	36374304506	success, nothing pulled or booted
200, no exemption	keel-core	36373993854	success in 43s, full pull, verify, assemble, boot, boot test
200, `allow_unpublished: true`	keel-core	36374306997	failure, exit 2, "allow_unpublished no longer has anything to excuse"
200, two nodes	keel-mariadb, `roles: primary replica`	36377248376	success in 92s, roles checked, both containers booted, topology in the summary
`exit 78` in a job	this repository	36372853847	failure, not neutral
200, after the review fixes	keel-core, via `tooling_ref`	36381555313	success in 43s, full boot, log reads `the gate recorded published=true`
404 + exemption, after the review fixes	keel-redis, via `tooling_ref`	36381562472	success, nothing booted, the `::warning::` is a real annotation on the check run, `the gate recorded published=false`
200, with a non-default `tooling_ref`	keel-core	36385954751	success in 43s, full boot, and the tooling warning is a real annotation naming the ref: the visibility costs no false failure on a good run

The check name appliance / boot-published-layer was read off the job lists
of those runs, not predicted from the YAML. Runs 36373993854, 36377248376 and
36381555313 are the ones that matter most for regression: the ordinary
published path still boots the core layer end to end with the tooling checkout
reorganised and the gate script in the way, and the two node path still gets
its node library, now through actions/checkout of this repository rather
than a hand written git clone, checks the declared roles and renders the
topology in the summary.

Locally: bats tests/ is 77 tests green, tests/coverage.sh reports 100.00
percent total (bin/require-changelog 57/57, lib/boot-test-nodes.sh
113/113, bin/appliance-gate 115/115), and the pinned actionlint 1.7.7,
sha256 023070a2..., reports no findings on the five workflows. The tests were
written before the script and failed for the right reason first.

The one thing covered by tests rather than by a run is the unwritable
$GITHUB_OUTPUT: inducing ENOSPC on the shared runner is not something to do
in order to verify a message. Two bats cases cover it, one per path that
records a decision.

Callers, verified by grep and not by the list I was given

grep -rln test-appliance over every repository checked out locally gives ten
callers and twelve caller jobs: keel-core, keel-apache-php, keel-nodejs-nginx,
keel-nodebb, keel-mariadb, keel-postgresql, keel-redis, keel-wordpress,
keel-lamp (appliance and appliance-client) and keel-lapp (appliance and
appliance-client). Each gets its own pull request updating the check name
and the sentence that claimed an unpublished layer passes. They are
documentation in the caller: the rename takes effect for all of them the
moment this merges, because every one of them calls @main.

Branch protection, which is not changed here

Renaming the reusable job renames the check for every caller at once, and
GitHub fails closed: a required context that is never reported does not
stop applying, it leaves the pull request blocked, waiting for a status that
will never arrive. So merging this without renaming the rules locks the
eight repositories below rather than leaving them unguarded, and
enforce_admins is true on keel-core and keel-mariadb, so not even an owner
could merge past it.

Measured, because the first revision of this description asserted the
opposite: two pull requests in keel-mariadb under one rule, every reported
check green on both. Number 16, reporting appliance / build-and-boot:
clean. Number 17, reporting appliance / boot-published-layer and identical
otherwise: blocked. keel-core#10 blocked the same way; keel-redis#6, in
the one repository that does not require the context, clean.

These eight require it today and need appliance / build-and-boot replaced
with appliance / boot-published-layer, in the same change as this merge:

Repository	Branch	Contexts to rename
keel-core	master	`appliance / build-and-boot`
keel-apache-php	main	`appliance / build-and-boot`
keel-nodebb	main	`appliance / build-and-boot`
keel-mariadb	main	`appliance / build-and-boot`
keel-postgresql	main	`appliance / build-and-boot`
keel-wordpress	master	`appliance / build-and-boot`
keel-lamp	master	`appliance / build-and-boot`, `appliance-client / build-and-boot`
keel-lapp	master	`appliance / build-and-boot`, `appliance-client / build-and-boot`

keel-redis (master) and keel-nodejs-nginx (no protection) require no rename;
keel-redis protects tests / coverage and package / changelog only. The
rename is a minute of work, but it has to happen before anything else merges
in those eight.

Worth deciding at the same time, and not decided here: whether this check
should be required at all. It is real evidence about the mirror and no
evidence about the diff, so requiring it gates merges on something the pull
request did not change. The rename makes that legible instead of hidden, which
is as far as this pull request goes.

What the review changed

Two HIGH findings, both fixed in the third commit.

publish_decision could still hand back an unearned success. Under
set -uo pipefail with no -e, the append to $GITHUB_OUTPUT was unchecked.
A failed write was swallowed, boot() returned 0, the step passed, and every
boot step, all guarded by published == 'true', skipped: the original defect
reached through an I/O error rather than by design, and with the more
misleading log line of the two. It is now die 5, with a test for the
published path and a test for the exemption path, which is the one that
passes. The calling step reads the file back and fails when the recorded value
is neither true nor false, so the guard those steps depend on is checked
where it is used rather than assumed.

The branch protection claim was inverted, here and in keel-core#12's
workflow comment. Corrected in both, with the keel-mariadb measurement above.
The action it implied was right; the reason was wrong, and a wrong reason
would make someone rush this merge to close a hole that does not exist.

Three MEDIUM findings, closed as far as they can be:

The exemption self-destructs only where the check is required, and only
for the name the caller declares. Both limits are now printed in the summary
of every run that uses it, tested, and in profile/WORKFLOWS.md, and the
notice became a ::warning:: so the run carries an annotation without
anyone opening the summary (measured: run 36381562472).
Nothing checked a change to this workflow before it merged. That is why
verifying the first revision needed throwaway branches pointed at
themselves, which the next person would not think to do. tooling_ref makes
it one input and profile/WORKFLOWS.md gives the procedure; it should be
github.job_workflow_sha and no input at all, and cannot be while actionlint
1.7.7 rejects that property.
Two standing red appliance checks now have dated, owned issues:
keel-nodejs-nginx#3, red since 2026-09-26 because tests/boot-test.sh does
not exist there and nobody noticed for two days, and keel-redis#10, red by
design until the layer is published. Honest red is honest only while it is
short-lived and has a name against it.

Both LOW points are fixed: the 200 line reports what the step found instead of
promising a boot that step does not perform, and the redirect test exercises a
3xx.

Second review pass

tooling_ref now has the visibility its sibling gained. It carries a
::warning:: and a job summary line naming the ref whenever it is not main,
on every path including the exemption path that passes without booting, and
the checkout step records the ref and commit in the summary unconditionally
rather than only when a layer was published. Five tests. Measured on run
36385954751: the annotation is on the check run, and that run still boots the
core layer in 43 seconds, so the visibility costs no false failure.

summary()'s cat >> is fatal, exit 5, reproduced with
GITHUB_STEP_SUMMARY=/dev/full. It has to return rather than die, because
summary is always the right hand side of a pipeline and an exit there exits
a subshell and the script carries on: every call site now ends
| summary || summary_failed, and pipefail carries the 5 to the caller,
which is what the /dev/full test proves.

Taken as offered: die emits ::error:: instead of writing to stderr, so
exits 4 and 5 annotate the run rather than leaving "Process completed with
exit code N" as all a reader gets.

Re-measured with GITHUB_OUTPUT=/dev/full: 200 and 404 + exemption, the
two paths that record a decision, exit 5. 404 without the exemption exits 1,
because it records no decision and refuses anyway. No path reaches a pass.

Left out of this pull request, on purpose

The handbook still names the old check in docs/ci-cd.md and
docs/releases-host.md, including in the procedure a maintainer follows to
apply protection, and docs/releases-host.md still says an unpublished layer
"skips with a notice and passes". That is a different repository and it is
recorded as Keel-Linux/handbook#9 rather than changed here. unit-mariadb,
unit-postgresql and unit-redis mention the old name once each in a header
comment; those are prose about a neighbouring repository's gate, not gates,
and are listed in the same issue.

Closes #11. Org-wide context: Keel-Linux/tracker#14 (left open; the design question it records is wider than this repository). ## The problem `appliance / build-and-boot` reported `success` in two states where it says nothing about the branch it is attached to. 1. **The layer has never been published.** Steps 6 to 10 were guarded by `if: steps.layer.outputs.published == 'true'`, were skipped, and the job still concluded success: keel-redis#5, run 36369705307, green in 11 seconds having pulled, verified and booted nothing. The step summary said "Nothing was built, assembled or booted", which is honest prose attached to a dishonest conclusion, because branch protection and `gh pr checks` read the conclusion. 2. **The layer is published.** The job pulls the *published* layer from the mirror and boots that, which on a pull request that changes the recipe is the code before the change. The branch is never exercised. Audit of the mirror, re-run before starting: eleven layers answer 200 at `https://mirror.keellinux.org/layers/<name>.manifest` (core, apache-php, nodejs-nginx, mariadb, postgresql, nodebb, wordpress, lamp, lamp-client, lapp, lapp-client) and `redis` answers 404. So one repository is in the unearned-green state and eleven checks are real, which is what makes the second face the bigger one. ## What this changes **The job id is `boot-published-layer`, not `build-and-boot`.** The check is now `appliance / boot-published-layer`. It keeps the value it has, catching a published layer that does not boot and catching the parent mismatch it was written for, and its name no longer reads as a claim about a branch. The job summary repeats it in prose and prints the `product_commit` and pool date the booted layer was built from, so a reviewer can see how old the thing that booted is. Building from the branch instead is not attempted: that needs the build host, which is serialised behind a lock, so it does not fit a per-pull-request check. **An unpublished layer fails the job**, unless the caller declares `allow_unpublished: true`. **The decision is a tested script**, `bin/appliance-gate`, which the job checks out and runs (decision 0004). The job holds the curl; everything concluded from the mirror's answer is in the script, where `tests/appliance-gate.bats` asks it every question. **A decision that cannot be written down is fatal**, exit 5. The append of `published=` to `$GITHUB_OUTPUT` was unchecked in the first revision, so a failed write was swallowed and the job concluded success having skipped all six boot steps: this same defect by another road. The calling step now also reads the file back and fails when neither `true` nor `false` was recorded. **`tooling_ref`**, so a change to this workflow can be checked before it merges instead of running against `main`'s copy of the script. Default `main`; no ordinary caller sets it, and a run that does not use `main` carries a `::warning::` naming the ref, on every path, for the same reason `allow_unpublished` does. **A job summary that cannot be written is fatal too**, same exit 5. It was the same unchecked write as `publish_decision`, five lines away, and it drops the caveats that are the only record that a passing job booted nothing. ## Why this mechanism and not one of the other two The issue offered three. Two of them do not survive contact with GitHub. **A neutral conclusion cannot be produced by a workflow job.** Measured rather than assumed: a probe on this repository, run 36372853847, gave a job running `exit 78`, the old neutral exit code, the conclusion `failure`, and a job skipped by `if:` the conclusion `skipped`. The Actions documentation on using conditions to control job execution says of the latter, "A job that is skipped will report its status as 'Success'. It will not prevent a pull request from merging, even if it is a required check." So `failure` is the only conclusion a workflow job can produce that withholds a merge. Anything built on `neutral` would have to be a separate check run posted through the Checks API under a name colliding with the job's own check, which I did not test and am not proposing. **"Fail when the layer has ever been published before" is not knowable here.** The mirror serves the current manifest and no history, so a 404 today is indistinguishable from a layer that was published and then deleted. There is nowhere to read "ever" from without inventing state to carry it. **So: fail by default, with an exemption the caller declares.** The remaining choice is where the bootstrap exemption lives, and the answer is the caller's own workflow file rather than the shared gate, because that is where the person reviewing that repository will see it. Once the manifest answers 200 it becomes an error telling you to delete the line. Its two limits are real and are printed in the summary of every run that uses it: the error only stops somebody where this check is a required status, and it keys on the name in `appliance:`, so a typo answers 404 for ever and never expires. Neither can be fixed from inside the gate, which cannot see a protection rule and cannot know which name you meant. **keel-redis does not get the exemption.** It is not bootstrapping. It has a repository, a recipe, a boot test and a layer whose publication is in progress, so the honest state of its gate is red with "publish the layer", which is exactly the signal missing when keel-redis#5 looked ready to merge. Its branch protection does not require this check, so nothing is blocked by that red, and it is owned and dated in keel-redis#10. No caller sets `allow_unpublished` today, which is the intended population: it exists for a repository whose first layer does not exist yet. ## How each behaviour was verified Not by reasoning. Temporary callers were pointed at this branch and the runs read back. All of them are closed and their branches deleted. | State | Where | Run | Result | | --- | --- | --- | --- | | 404, no exemption | keel-redis | 36373971968 | **failure** in 12s at the gate step; steps 7 to 12 skipped | | 404, `allow_unpublished: true` | keel-redis | 36374304506 | success, nothing pulled or booted | | 200, no exemption | keel-core | 36373993854 | success in 43s, full pull, verify, assemble, boot, boot test | | 200, `allow_unpublished: true` | keel-core | 36374306997 | **failure**, exit 2, "allow_unpublished no longer has anything to excuse" | | 200, two nodes | keel-mariadb, `roles: primary replica` | 36377248376 | success in 92s, roles checked, both containers booted, topology in the summary | | `exit 78` in a job | this repository | 36372853847 | failure, not neutral | | 200, after the review fixes | keel-core, via `tooling_ref` | 36381555313 | success in 43s, full boot, log reads `the gate recorded published=true` | | 404 + exemption, after the review fixes | keel-redis, via `tooling_ref` | 36381562472 | success, nothing booted, the `::warning::` is a real annotation on the check run, `the gate recorded published=false` | | 200, with a non-default `tooling_ref` | keel-core | 36385954751 | success in 43s, full boot, and the tooling warning is a real annotation naming the ref: the visibility costs no false failure on a good run | The check name `appliance / boot-published-layer` was read off the job lists of those runs, not predicted from the YAML. Runs 36373993854, 36377248376 and 36381555313 are the ones that matter most for regression: the ordinary published path still boots the core layer end to end with the tooling checkout reorganised and the gate script in the way, and the two node path still gets its node library, now through `actions/checkout` of this repository rather than a hand written `git clone`, checks the declared roles and renders the topology in the summary. Locally: `bats tests/` is 77 tests green, `tests/coverage.sh` reports **100.00 percent** total (`bin/require-changelog` 57/57, `lib/boot-test-nodes.sh` 113/113, `bin/appliance-gate` 115/115), and the pinned actionlint 1.7.7, sha256 `023070a2...`, reports no findings on the five workflows. The tests were written before the script and failed for the right reason first. The one thing covered by tests rather than by a run is the unwritable `$GITHUB_OUTPUT`: inducing ENOSPC on the shared runner is not something to do in order to verify a message. Two bats cases cover it, one per path that records a decision. ## Callers, verified by grep and not by the list I was given `grep -rln test-appliance` over every repository checked out locally gives ten callers and twelve caller jobs: keel-core, keel-apache-php, keel-nodejs-nginx, keel-nodebb, keel-mariadb, keel-postgresql, keel-redis, keel-wordpress, keel-lamp (`appliance` and `appliance-client`) and keel-lapp (`appliance` and `appliance-client`). Each gets its own pull request updating the check name and the sentence that claimed an unpublished layer passes. They are documentation in the caller: the rename takes effect for all of them the moment this merges, because every one of them calls `@main`. ## Branch protection, which is not changed here Renaming the reusable job renames the check for every caller at once, and GitHub **fails closed**: a required context that is never reported does not stop applying, it leaves the pull request blocked, waiting for a status that will never arrive. So merging this without renaming the rules **locks** the eight repositories below rather than leaving them unguarded, and `enforce_admins` is `true` on keel-core and keel-mariadb, so not even an owner could merge past it. Measured, because the first revision of this description asserted the opposite: two pull requests in keel-mariadb under one rule, every reported check green on both. Number 16, reporting `appliance / build-and-boot`: `clean`. Number 17, reporting `appliance / boot-published-layer` and identical otherwise: `blocked`. keel-core#10 `blocked` the same way; keel-redis#6, in the one repository that does not require the context, `clean`. These eight require it today and need `appliance / build-and-boot` replaced with `appliance / boot-published-layer`, in the same change as this merge: | Repository | Branch | Contexts to rename | | --- | --- | --- | | keel-core | master | `appliance / build-and-boot` | | keel-apache-php | main | `appliance / build-and-boot` | | keel-nodebb | main | `appliance / build-and-boot` | | keel-mariadb | main | `appliance / build-and-boot` | | keel-postgresql | main | `appliance / build-and-boot` | | keel-wordpress | master | `appliance / build-and-boot` | | keel-lamp | master | `appliance / build-and-boot`, `appliance-client / build-and-boot` | | keel-lapp | master | `appliance / build-and-boot`, `appliance-client / build-and-boot` | keel-redis (master) and keel-nodejs-nginx (no protection) require no rename; keel-redis protects `tests / coverage` and `package / changelog` only. The rename is a minute of work, but it has to happen before anything else merges in those eight. Worth deciding at the same time, and not decided here: whether this check should be required at all. It is real evidence about the mirror and no evidence about the diff, so requiring it gates merges on something the pull request did not change. The rename makes that legible instead of hidden, which is as far as this pull request goes. ## What the review changed Two HIGH findings, both fixed in the third commit. **`publish_decision` could still hand back an unearned success.** Under `set -uo pipefail` with no `-e`, the append to `$GITHUB_OUTPUT` was unchecked. A failed write was swallowed, `boot()` returned 0, the step passed, and every boot step, all guarded by `published == 'true'`, skipped: the original defect reached through an I/O error rather than by design, and with the more misleading log line of the two. It is now `die 5`, with a test for the published path and a test for the exemption path, which is the one that passes. The calling step reads the file back and fails when the recorded value is neither `true` nor `false`, so the guard those steps depend on is checked where it is used rather than assumed. **The branch protection claim was inverted**, here and in keel-core#12's workflow comment. Corrected in both, with the keel-mariadb measurement above. The action it implied was right; the reason was wrong, and a wrong reason would make someone rush this merge to close a hole that does not exist. Three MEDIUM findings, closed as far as they can be: - **The exemption self-destructs only where the check is required**, and only for the name the caller declares. Both limits are now printed in the summary of every run that uses it, tested, and in `profile/WORKFLOWS.md`, and the notice became a `::warning::` so the run carries an annotation without anyone opening the summary (measured: run 36381562472). - **Nothing checked a change to this workflow before it merged.** That is why verifying the first revision needed throwaway branches pointed at themselves, which the next person would not think to do. `tooling_ref` makes it one input and `profile/WORKFLOWS.md` gives the procedure; it should be `github.job_workflow_sha` and no input at all, and cannot be while actionlint 1.7.7 rejects that property. - **Two standing red appliance checks now have dated, owned issues**: keel-nodejs-nginx#3, red since 2026-09-26 because `tests/boot-test.sh` does not exist there and nobody noticed for two days, and keel-redis#10, red by design until the layer is published. Honest red is honest only while it is short-lived and has a name against it. Both LOW points are fixed: the 200 line reports what the step found instead of promising a boot that step does not perform, and the redirect test exercises a 3xx. ### Second review pass **`tooling_ref` now has the visibility its sibling gained.** It carries a `::warning::` and a job summary line naming the ref whenever it is not `main`, on every path including the exemption path that passes without booting, and the checkout step records the ref and commit in the summary unconditionally rather than only when a layer was published. Five tests. Measured on run 36385954751: the annotation is on the check run, and that run still boots the `core` layer in 43 seconds, so the visibility costs no false failure. **`summary()`'s `cat >>` is fatal**, exit 5, reproduced with `GITHUB_STEP_SUMMARY=/dev/full`. It has to `return` rather than `die`, because `summary` is always the right hand side of a pipeline and an exit there exits a subshell and the script carries on: every call site now ends `| summary || summary_failed`, and `pipefail` carries the 5 to the caller, which is what the `/dev/full` test proves. **Taken as offered:** `die` emits `::error::` instead of writing to stderr, so exits 4 and 5 annotate the run rather than leaving "Process completed with exit code N" as all a reader gets. Re-measured with `GITHUB_OUTPUT=/dev/full`: `200` and `404 + exemption`, the two paths that record a decision, exit 5. `404` without the exemption exits 1, because it records no decision and refuses anyway. No path reaches a pass. ## Left out of this pull request, on purpose The handbook still names the old check in `docs/ci-cd.md` and `docs/releases-host.md`, including in the procedure a maintainer follows to apply protection, and `docs/releases-host.md` still says an unpublished layer "skips with a notice and passes". That is a different repository and it is recorded as Keel-Linux/handbook#9 rather than changed here. `unit-mariadb`, `unit-postgresql` and `unit-redis` mention the old name once each in a header comment; those are prose about a neighbouring repository's gate, not gates, and are listed in the same issue.

marcos-mendez commented

2026-09-28 02:00:52 -03:00

(Migrated from github.com)

Review

The defect is real, the diagnosis is right, and the verification is unusually honest: I re-ran every claim rather than reading it, and all six runs concluded what the table says. Two things should change before this merges, and one of them is a statement in the description that is the opposite of what GitHub actually does.

What I verified rather than accepted

Claim	Result
keel-redis 36373971968, 404 no exemption	failure, 12s (03:30:20→03:30:32), step 6 `failure`, steps 7–12 `skipped`, `ALLOW_UNPUBLISHED: false`, exit code 1
keel-redis 36374304506, 404 + exemption	success, step 6 `success` with the `::notice::`, steps 7–12 `skipped` — nothing pulled or booted
keel-core 36373993854, 200 no exemption	success, 43s, steps 7–12 all `success`: keel checkout, parent check, pull, verify, boot test, summary
keel-core 36374306997, 200 + exemption	failure, `Process completed with exit code 2`, "allow_unpublished no longer has anything to excuse"
keel-mariadb 36377248376, two nodes	success, 92s; roles step green, both containers assembled, started, addressed and first-booted, topology written
.github 36372853847, `exit 78`	job `exit-78` → failure; job `plain-failure` → failure; job `skipped-by-if` → `skipped`

The tooling commits those runs used (1d642bd for redis/core, cda32e11 for mariadb) differ from this branch's head by exactly one line — ref: verify/gate-behaviour instead of ref: main at .github/workflows/test-appliance.yml:153. Nothing else. So the evidence is about the code being proposed.

actions/checkout genuinely replaced the hand-written clone and the two-node run genuinely exercised it: the mariadb log shows Syncing repository: keel-linux/.github, path: .keel-ci, then btn_node_count reading the library out of that checkout and both nodes booting from it.

Also confirmed: the mirror audit is exact (eleven layers 200, redis 404, re-checked just now); the motivating run keel-redis 36369705307 is appliance / build-and-boot success in 11 seconds with steps 6–10 skipped; bats tests/ is 68 green and tests/coverage.sh reports 100.00 total, bin/appliance-gate 89/89, locally; shellcheck bin/appliance-gate is clean and the file is committed 100755; .github has no debian/ and its protection requires only actionlint and tests / coverage, so no changelog entry is owed.

HIGH — `bin/appliance-gate:37-39`: the gate can still conclude success having booted nothing

publish_decision appends to $GITHUB_OUTPUT under set -uo pipefail with no -e and no check on the redirection:

printf 'published=%s\n' "$1" >> "$GITHUB_OUTPUT"

If that write fails, the failure is swallowed, boot() still return 0, and the step passes. steps.layer.outputs.published is then empty, so every one of test-appliance.yml:204, 222, 234, 241, 256, 289 evaluates false, all six boot steps are skipped, and the job concludes success — the original defect, bit for bit, and with the more misleading summary of the two, because the log line that did print says the layer "will be pulled, verified, assembled and booted".

Reproduced against this branch with GITHUB_OUTPUT pointing at an unwritable file:

$ bash bin/appliance-gate 200 core https://mirror.keellinux.org/layers false
bin/appliance-gate: line 38: .../out: Permission denied
appliance-gate: ... answers 200; the published core layer will be pulled, verified, assembled and booted
EXIT=0

The scenario is not exotic. $GITHUB_OUTPUT lives on the runner's own disk, and this is the runner that assembles multi-gigabyte rootfs trees under /var/tmp/keel-ci; the cleanup step at line 362 already warns that a scratch tree can survive. One ENOSPC and the gate reports a boot it did not do. The whole point of the change is that a skipped boot must not be reportable as a pass, and here it still is — reached by an I/O failure instead of by design, which is the harder one to notice.

Make the write fatal, e.g. || die 5 "cannot record the decision", with a test for it. Cheap belt-and-braces on top: have the step fail when published comes back neither true nor false.

HIGH — the branch-protection premise is inverted, and it is being written into eight repositories

The description says a protection rule naming the old check "stops matching anything and stops applying, silently", and keel-core#12 puts the same sentence into that repository's workflow comment ("a rule naming the old check would have matched nothing and stopped applying without saying so").

GitHub does the opposite. It fails closed: a required status check that is never reported leaves the pull request blocked, permanently, waiting for a status that will never arrive.

The controlled experiment is already in this organization, produced by the verification runs themselves. Two pull requests in keel-mariadb, same branch, same protection rule (strict, enforce_admins: true, required_conversation_resolution, required_approving_review_count: 0), neither with a single review thread:

	reported checks	merge state
keel-mariadb#16	`tests / coverage` ✓, `package / changelog` ✓, `appliance / build-and-boot` ✓	`CLEAN`
keel-mariadb#17	`tests / coverage` ✓, `package / changelog` ✓, `appliance / boot-published-layer` ✓	`BLOCKED`

Every check that reported on #17 is green. The only difference is that the required context appliance / build-and-boot was never reported, and that alone is enough to block. keel-core#10 is the same shape and also blocked. The control is keel-redis#6, which carries the identical new check name but in the one repository that does not require it: clean. Closing a pull request does not force BLOCKED — #6 is closed and clean.

So the risk of merging this without renaming protection in the same breath is not an open gate. It is a lockout of all eight repositories, and on keel-core and keel-mariadb enforce_admins is true, so nobody can merge past it — protection has to be edited before anything else moves. Recoverable in minutes, but worth knowing which way it falls before you start, and worth not teaching the next person the wrong model. The contexts to rename are exactly as listed; I re-read all ten rules and the table is correct, including the two contexts each on keel-lamp and keel-lapp and the two repositories that need nothing.

Please fix the sentence here and in keel-core#12. The instruction that follows from it — rename protection at merge time — is right either way.

MEDIUM — `allow_unpublished` only self-destructs where the check is required

test-appliance.yml:97-108 and bin/appliance-gate:118-132. The exemption "cannot quietly outlive the bootstrap" because it turns into an error once the manifest answers 200. That holds only where somebody is stopped by the error. In keel-nodejs-nginx there is no branch protection at all, and in keel-redis the gate is not required; a stale exemption there is a red check that blocks nothing, on a repository whose gate is already expected to be red. It is exactly the population most likely to hold one, since the exemption exists for repositories that have not published yet.

Second, narrower hole in the same mechanism: the self-destruct keys on the manifest of the name the caller declares. appliance: wordpres answers 404 forever, so an exemption sitting behind a typo never expires and the job is green for the life of the repository. never_published catches the typo today only because the default is to fail; with the exemption set, nothing does.

MEDIUM — `test-appliance.yml:149-154`: nothing checks a change to the gate before it is merged

The tooling checkout is ref: main, so a pull request against this repository that changes bin/appliance-gate or the workflow's wiring runs against main's copy. tests / coverage exercises the script in isolation and nothing exercises it in the job. That is why verifying this change needed two throwaway branches and two temporary pull requests hand-pointed at themselves. The diligence was real; it is also unrepeatable by anyone who does not think to do it. Worth an issue for github.job_workflow_sha once actionlint knows the property, and meanwhile worth saying in the comment that the manual procedure is the only check there is.

MEDIUM — two permanently red appliance checks, and docs/traps.md has already written this one down

keel-redis is deliberately red until its layer is published, and I agree with the reasoning: it is not bootstrapping, and the red is the honest state. But keel-nodejs-nginx has been red since 2026-09-26 with tests/boot-test.sh is missing or not executable (runs 36278621010 and 36376230342, same error both times) and nobody noticed in two days. That is two of the ten callers red by default, which is the trap the project already paid for:

A daily check reports drift every morning. Nobody reads it any more.

The honest red is right only if it is short-lived and owned. Give each a dated issue, or the next real red goes unread. keel-nodejs-nginx is the more urgent of the two — its gate has never proved anything.

LOW

bin/appliance-gate:63 promises rather than reports: "the published core layer will be pulled, verified, assembled and booted" is printed before any of that happens, and in the failure mode above it is printed when none of it will. A check may only be named for what it proves; the same goes for the line it logs.
tests/appliance-gate.bats:111 is named "a redirect or a forbidden answer is not a skip either" and exercises only 403. A 3xx value is never passed to the script. gate 301 core ... false costs one line and matches the name.
The error paths asked about are covered where the script can see them — 503 and 403 both reach unreachable, a non-numeric status reaches die 4, and the workflow catches curl's own exit before the script is called. A malformed 200 is caught one step later by the parent check (test-appliance.yml:225): the live core.manifest says parent none, so an empty or garbage body yields an empty parent and fails against the declared none. That is load-bearing and has no test, because it is inline shell rather than part of the script the tests can reach.
The tests assert behaviour, not the code: exit codes, the presence of published= in the output file, the absence of it on every refusing path, and the remediation text in the summary. "Only the exact word true is the exemption" is the right kind of test. The one thing not asserted is the one that broke above.
The supporting claim that a job skipped by if: counts as success for a required check is the only one not measured. Run 36372853847 measured exit 78 → failure, which is solid, and measured the skipped job's conclusion as skipped — on a push, with no protection in play. No pull request in this organization has ever had a required context reported with conclusion skipped, so it rests on the documentation. It points the conservative way, so the design does not change either way; it just is not in the same class of evidence as the rest.

Coverage claim verified, the script is genuinely tested for behaviour, the callers are comments and documentation only with no functional YAML change in any of the ten, and the two red checks are pre-existing — though keel-wordpress#9's is the intermittent TLS failure to archive.keellinux.org (certificate verify failed, IP 127.0.1.1) rather than a steady one: the same repository went green on another branch 34 minutes later, so #9 needs a re-run to unblock, not a fix.

Warning — no blocking defect, but publish_decision can still hand back an unearned success, and the branch-protection sentence is wrong in a way that will mislead whoever applies the rename. Fix both, then merge and rename the eight rules back to back.

## Review The defect is real, the diagnosis is right, and the verification is unusually honest: I re-ran every claim rather than reading it, and all six runs concluded what the table says. Two things should change before this merges, and one of them is a statement in the description that is the opposite of what GitHub actually does. ### What I verified rather than accepted | Claim | Result | | --- | --- | | keel-redis 36373971968, 404 no exemption | **failure**, 12s (03:30:20→03:30:32), step 6 `failure`, steps 7–12 `skipped`, `ALLOW_UNPUBLISHED: false`, exit code 1 | | keel-redis 36374304506, 404 + exemption | **success**, step 6 `success` with the `::notice::`, steps 7–12 `skipped` — nothing pulled or booted | | keel-core 36373993854, 200 no exemption | **success**, 43s, steps 7–12 all `success`: keel checkout, parent check, pull, verify, boot test, summary | | keel-core 36374306997, 200 + exemption | **failure**, `Process completed with exit code 2`, "allow_unpublished no longer has anything to excuse" | | keel-mariadb 36377248376, two nodes | **success**, 92s; roles step green, both containers assembled, started, addressed and first-booted, topology written | | .github 36372853847, `exit 78` | job `exit-78` → **failure**; job `plain-failure` → failure; job `skipped-by-if` → `skipped` | The tooling commits those runs used (`1d642bd` for redis/core, `cda32e11` for mariadb) differ from this branch's head by exactly one line — `ref: verify/gate-behaviour` instead of `ref: main` at `.github/workflows/test-appliance.yml:153`. Nothing else. So the evidence is about the code being proposed. `actions/checkout` genuinely replaced the hand-written clone and the two-node run genuinely exercised it: the mariadb log shows `Syncing repository: keel-linux/.github`, `path: .keel-ci`, then `btn_node_count` reading the library out of that checkout and both nodes booting from it. Also confirmed: the mirror audit is exact (eleven layers 200, `redis` 404, re-checked just now); the motivating run keel-redis 36369705307 is `appliance / build-and-boot` **success in 11 seconds** with steps 6–10 skipped; `bats tests/` is 68 green and `tests/coverage.sh` reports **100.00 total**, `bin/appliance-gate` 89/89, locally; `shellcheck bin/appliance-gate` is clean and the file is committed `100755`; `.github` has no `debian/` and its protection requires only `actionlint` and `tests / coverage`, so no changelog entry is owed. --- ### HIGH — `bin/appliance-gate:37-39`: the gate can still conclude success having booted nothing `publish_decision` appends to `$GITHUB_OUTPUT` under `set -uo pipefail` with no `-e` and no check on the redirection: ```bash printf 'published=%s\n' "$1" >> "$GITHUB_OUTPUT" ``` If that write fails, the failure is swallowed, `boot()` still `return 0`, and the step passes. `steps.layer.outputs.published` is then empty, so every one of `test-appliance.yml:204, 222, 234, 241, 256, 289` evaluates false, all six boot steps are skipped, and the job concludes **success** — the original defect, bit for bit, and with the more misleading summary of the two, because the log line that did print says the layer "will be pulled, verified, assembled and booted". Reproduced against this branch with `GITHUB_OUTPUT` pointing at an unwritable file: ``` $ bash bin/appliance-gate 200 core https://mirror.keellinux.org/layers false bin/appliance-gate: line 38: .../out: Permission denied appliance-gate: ... answers 200; the published core layer will be pulled, verified, assembled and booted EXIT=0 ``` The scenario is not exotic. `$GITHUB_OUTPUT` lives on the runner's own disk, and this is the runner that assembles multi-gigabyte rootfs trees under `/var/tmp/keel-ci`; the cleanup step at line 362 already warns that a scratch tree can survive. One ENOSPC and the gate reports a boot it did not do. The whole point of the change is that a skipped boot must not be reportable as a pass, and here it still is — reached by an I/O failure instead of by design, which is the harder one to notice. Make the write fatal, e.g. `|| die 5 "cannot record the decision"`, with a test for it. Cheap belt-and-braces on top: have the step fail when `published` comes back neither `true` nor `false`. ### HIGH — the branch-protection premise is inverted, and it is being written into eight repositories The description says a protection rule naming the old check "stops matching anything and stops applying, silently", and keel-core#12 puts the same sentence into that repository's workflow comment ("a rule naming the old check would have matched nothing and stopped applying without saying so"). GitHub does the opposite. It **fails closed**: a required status check that is never reported leaves the pull request blocked, permanently, waiting for a status that will never arrive. The controlled experiment is already in this organization, produced by the verification runs themselves. Two pull requests in **keel-mariadb**, same branch, same protection rule (`strict`, `enforce_admins: true`, `required_conversation_resolution`, `required_approving_review_count: 0`), neither with a single review thread: | | reported checks | merge state | | --- | --- | --- | | keel-mariadb#16 | `tests / coverage` ✓, `package / changelog` ✓, `appliance / build-and-boot` ✓ | `CLEAN` | | keel-mariadb#17 | `tests / coverage` ✓, `package / changelog` ✓, `appliance / boot-published-layer` ✓ | `BLOCKED` | Every check that reported on #17 is green. The only difference is that the required context `appliance / build-and-boot` was never reported, and that alone is enough to block. keel-core#10 is the same shape and also `blocked`. The control is keel-redis#6, which carries the identical new check name but in the one repository that does not require it: `clean`. Closing a pull request does not force `BLOCKED` — #6 is closed and clean. So the risk of merging this without renaming protection in the same breath is not an open gate. It is a **lockout of all eight repositories**, and on keel-core and keel-mariadb `enforce_admins` is `true`, so nobody can merge past it — protection has to be edited before anything else moves. Recoverable in minutes, but worth knowing which way it falls before you start, and worth not teaching the next person the wrong model. The contexts to rename are exactly as listed; I re-read all ten rules and the table is correct, including the two contexts each on keel-lamp and keel-lapp and the two repositories that need nothing. Please fix the sentence here and in keel-core#12. The instruction that follows from it — rename protection at merge time — is right either way. ### MEDIUM — `allow_unpublished` only self-destructs where the check is required `test-appliance.yml:97-108` and `bin/appliance-gate:118-132`. The exemption "cannot quietly outlive the bootstrap" because it turns into an error once the manifest answers 200. That holds only where somebody is stopped by the error. In keel-nodejs-nginx there is no branch protection at all, and in keel-redis the gate is not required; a stale exemption there is a red check that blocks nothing, on a repository whose gate is already expected to be red. It is exactly the population most likely to hold one, since the exemption exists for repositories that have not published yet. Second, narrower hole in the same mechanism: the self-destruct keys on the manifest of the name the caller declares. `appliance: wordpres` answers 404 forever, so an exemption sitting behind a typo never expires and the job is green for the life of the repository. `never_published` catches the typo today only because the default is to fail; with the exemption set, nothing does. ### MEDIUM — `test-appliance.yml:149-154`: nothing checks a change to the gate before it is merged The tooling checkout is `ref: main`, so a pull request against this repository that changes `bin/appliance-gate` or the workflow's wiring runs against **main's** copy. `tests / coverage` exercises the script in isolation and nothing exercises it in the job. That is why verifying this change needed two throwaway branches and two temporary pull requests hand-pointed at themselves. The diligence was real; it is also unrepeatable by anyone who does not think to do it. Worth an issue for `github.job_workflow_sha` once actionlint knows the property, and meanwhile worth saying in the comment that the manual procedure is the only check there is. ### MEDIUM — two permanently red appliance checks, and docs/traps.md has already written this one down keel-redis is deliberately red until its layer is published, and I agree with the reasoning: it is not bootstrapping, and the red is the honest state. But keel-nodejs-nginx has been red since 2026-09-26 with `tests/boot-test.sh is missing or not executable` (runs 36278621010 and 36376230342, same error both times) and nobody noticed in two days. That is two of the ten callers red by default, which is the trap the project already paid for: > A daily check reports drift every morning. Nobody reads it any more. The honest red is right only if it is short-lived and owned. Give each a dated issue, or the next real red goes unread. keel-nodejs-nginx is the more urgent of the two — its gate has never proved anything. ### LOW - `bin/appliance-gate:63` promises rather than reports: "the published core layer **will be** pulled, verified, assembled and booted" is printed before any of that happens, and in the failure mode above it is printed when none of it will. A check may only be named for what it proves; the same goes for the line it logs. - `tests/appliance-gate.bats:111` is named "a redirect or a forbidden answer is not a skip either" and exercises only `403`. A `3xx` value is never passed to the script. `gate 301 core ... false` costs one line and matches the name. - The error paths asked about are covered where the script can see them — `503` and `403` both reach `unreachable`, a non-numeric status reaches `die 4`, and the workflow catches curl's own exit before the script is called. A malformed 200 is caught one step later by the parent check (`test-appliance.yml:225`): the live `core.manifest` says `parent none`, so an empty or garbage body yields an empty parent and fails against the declared `none`. That is load-bearing and has no test, because it is inline shell rather than part of the script the tests can reach. - The tests assert behaviour, not the code: exit codes, the presence of `published=` in the output file, the absence of it on every refusing path, and the remediation text in the summary. "Only the exact word true is the exemption" is the right kind of test. The one thing not asserted is the one that broke above. - The supporting claim that a job skipped by `if:` counts as success for a required check is the only one not measured. Run 36372853847 measured `exit 78` → `failure`, which is solid, and measured the skipped job's conclusion as `skipped` — on a push, with no protection in play. No pull request in this organization has ever had a required context reported with conclusion `skipped`, so it rests on the documentation. It points the conservative way, so the design does not change either way; it just is not in the same class of evidence as the rest. --- Coverage claim verified, the script is genuinely tested for behaviour, the callers are comments and documentation only with no functional YAML change in any of the ten, and the two red checks are pre-existing — though keel-wordpress#9's is the intermittent TLS failure to `archive.keellinux.org` (`certificate verify failed`, IP 127.0.1.1) rather than a steady one: the same repository went green on another branch 34 minutes later, so #9 needs a re-run to unblock, not a fix. **Warning** — no blocking defect, but `publish_decision` can still hand back an unearned success, and the branch-protection sentence is wrong in a way that will mislead whoever applies the rename. Fix both, then merge and rename the eight rules back to back.

marcos-mendez commented

2026-09-28 02:33:41 -03:00

(Migrated from github.com)

Both HIGH findings are fixed in 51aa16d, and the description is rewritten
around the corrected protection model.

The unchecked write. publish_decision now dies with exit 5 when the
append to $GITHUB_OUTPUT fails, and the calling step reads the file back and
fails when the recorded value is neither true nor false, so the guard the
six boot steps depend on is checked where it is used. Two bats cases cover it,
one per path that records a decision, including the exemption path, which is
the one that passes. That one stays a test rather than a run: inducing ENOSPC
on the shared runner to verify a message is not a trade worth making.

The inverted premise. Corrected here and in keel-core#12, in the workflow
comment as well as the body, and in Keel-Linux/handbook#9 which had inherited
it. I reproduced your experiment before rewriting: keel-mariadb#16 clean,
keel-mariadb#17 blocked, keel-core#10 blocked, keel-redis#6 clean, and
enforce_admins true on keel-core and keel-mariadb and false on the other
six. The description now says a lockout, not a hole.

The two ways allow_unpublished fails to expire are printed in the
summary of every run that uses it and documented; the notice became a
::warning::, so the annotation is on the check run without anyone opening
the summary. Neither limit can be closed from inside the gate, which cannot
read a protection rule and cannot know which name you meant, so they are
stated instead of pretended away.

tooling_ref replaces the throwaway-branch procedure, with the recipe in
profile/WORKFLOWS.md. It should be github.job_workflow_sha; actionlint
1.7.7 still rejects that property.

The two standing reds are owned: keel-nodejs-nginx#3 and keel-redis#10,
both dated.

Re-verified with the new input rather than by hand-pointed branches, both
closed: keel-core run 36381555313, full boot in 43 seconds, log reads the gate recorded published=true; keel-redis run 36381562472, exemption path,
warning annotation present on the check run, the gate recorded published=false, nothing booted. 71 tests green, coverage 100.00 total with
bin/appliance-gate at 100/100, actionlint clean.

keel-wordpress#9 was the intermittent TLS failure as you said. It failed a
second time on re-run and passed on the third; that pull request is clean
now.

Both HIGH findings are fixed in 51aa16d, and the description is rewritten around the corrected protection model. **The unchecked write.** `publish_decision` now dies with exit 5 when the append to `$GITHUB_OUTPUT` fails, and the calling step reads the file back and fails when the recorded value is neither `true` nor `false`, so the guard the six boot steps depend on is checked where it is used. Two bats cases cover it, one per path that records a decision, including the exemption path, which is the one that passes. That one stays a test rather than a run: inducing ENOSPC on the shared runner to verify a message is not a trade worth making. **The inverted premise.** Corrected here and in keel-core#12, in the workflow comment as well as the body, and in Keel-Linux/handbook#9 which had inherited it. I reproduced your experiment before rewriting: keel-mariadb#16 `clean`, keel-mariadb#17 `blocked`, keel-core#10 `blocked`, keel-redis#6 `clean`, and `enforce_admins` true on keel-core and keel-mariadb and false on the other six. The description now says a lockout, not a hole. **The two ways `allow_unpublished` fails to expire** are printed in the summary of every run that uses it and documented; the notice became a `::warning::`, so the annotation is on the check run without anyone opening the summary. Neither limit can be closed from inside the gate, which cannot read a protection rule and cannot know which name you meant, so they are stated instead of pretended away. **`tooling_ref`** replaces the throwaway-branch procedure, with the recipe in `profile/WORKFLOWS.md`. It should be `github.job_workflow_sha`; actionlint 1.7.7 still rejects that property. **The two standing reds are owned**: keel-nodejs-nginx#3 and keel-redis#10, both dated. Re-verified with the new input rather than by hand-pointed branches, both closed: keel-core run 36381555313, full boot in 43 seconds, log reads `the gate recorded published=true`; keel-redis run 36381562472, exemption path, warning annotation present on the check run, `the gate recorded published=false`, nothing booted. 71 tests green, coverage 100.00 total with `bin/appliance-gate` at 100/100, actionlint clean. keel-wordpress#9 was the intermittent TLS failure as you said. It failed a second time on re-run and passed on the third; that pull request is `clean` now.

marcos-mendez commented

2026-09-28 03:04:49 -03:00

(Migrated from github.com)

Re-review of `51aa16d`

Both HIGH findings are closed, and closed with evidence rather than with an argument. I re-ran my own reproduction, tried four further ways to break the new code, and checked the new code where it matters most — in a real job, which it has now had.

The HIGH that could still report an unearned success — closed

bin/appliance-gate:45-51. My original break now fails, and so does the failure mode my repro did not cover. The distinction matters: I broke it with an unwritable file, which fails at open. ENOSPC is different — the open succeeds and the write fails — so I tested that separately with GITHUB_OUTPUT=/dev/full, which is exactly that shape:

probe	before	at `51aa16d`
unwritable file, 200, no exemption	`EXIT=0`, nothing recorded	`EXIT=5`, "cannot record the decision …"
`/dev/full` (open succeeds, write returns ENOSPC), 200	—	`EXIT=5`
`/dev/full`, 404 + exemption (the path that passes)	—	`EXIT=5`

The ordering is also right now: boot() calls publish_decision before it echoes, so the failure path no longer prints a sentence about a layer that will be booted. Nothing misleading survives it.

The read-back — I gave it the scepticism it deserves, and it holds

This is new code on the path that decides whether anything boots, added in response to a review, so I treated it as a suspect rather than as a fix. I transcribed the step body (test-appliance.yml:205-244) verbatim and drove it.

The tail -1 ordering guess holds, in both directions. With a stale published=false already in the file and a real 200, the read-back returns true. With a stale published=true and the exemption taken, it returns false. The gate's write is always the last append, and last-wins is also how the runner itself resolves steps.layer.outputs.*, so the guess agrees with the thing it is guessing about. The manifest= line the step writes first is correctly ignored, because awk -F= reads its $1 as manifest.
No reachable false pass. A stale value could only be mistaken for the gate's own if the gate exited 0 without writing. It cannot: the write is fatal when GITHUB_OUTPUT is non-empty, and when it is empty or unset the step dies four lines earlier — GITHUB_OUTPUT empty gives : No such file or directory, unset gives unbound variable under set -u, and a full filesystem makes the step's own echo "manifest=…" fail first. All three exit 1.
No false failure on a good run, which is the way new code on this path would most likely bite. Confirmed in place, not reasoned: run 36381555313 printed the gate recorded published=true and went on through pull, verify, assemble, boot and boot test; run 36381562472 printed the gate recorded published=false and skipped them.
A partial write cannot leave a half-written value to be read, because the write failing is fatal and the step aborts before the read-back runs.

One residual, and it is the gate's CLI contract rather than a hole: with GITHUB_OUTPUT empty the script still reports a publishable layer and records nothing, by the [[ -n … ]] guard that the "outside Actions" test covers. It is unreachable from the workflow, per the above.

The new code was exercised before you asked me to look at it. Runs 36381555313 (keel-core, 200, no exemption) and 36381562472 (keel-redis, 404 with the exemption) both ran with tooling_ref: ci/name-the-gate-for-what-it-proves and KEEL_CI_COMMIT: 51aa16d0a85a57f72ce3e159bdc895c2484cb605 — this head, not main's copy. That is the new input paying for itself on its first day. The ::warning:: annotation is real: the Checks API returns it on the redis run, so it is visible without opening the summary.

The two refusing paths (404 → exit 1, 200 + exemption → exit 2) were not re-run on this head. never_published and stale_exemption are untouched by this commit and neither calls publish_decision, and a failing gate aborts the step before the read-back, so I accept that gap rather than asking for two more runs.

bats tests/ is 71 green, tests/coverage.sh reports 100.00 total with bin/appliance-gate at 100/100 — I reproduced both locally — and shellcheck is clean.

The HIGH on branch protection — accurate, not merely changed

I checked the new wording against what I measured, clause by clause. "GitHub fails closed"; "leaves the pull request blocked, waiting for a status that will never arrive"; "locks the eight repositories below rather than leaving them unguarded"; "enforce_admins is true on keel-core and keel-mariadb, so not even an owner could merge past it" — all four are what the keel-mariadb#16/#17 pair shows, and both enforce_admins values are true. The keel-core#10 and keel-redis#6 corroboration is reported correctly, including why #6 is the control. keel-core#12's comment now says the same and adds "enforce_admins is true here", which is right for that repository.

The precision on "merge past it" is the correct one: an owner can still edit the rule, which is what unblocks it. Nothing to fix.

The three MEDIUMs

The exemption expires only where the check is required, and only for the declared name — closed as far as the mechanism allows, and I accept it. Both limits are now printed in the summary of every run that uses the exemption (bin/appliance-gate:128-139), asserted by a test that greps for them, documented, and the notice became a ::warning::. The underlying property is inherent — an error cannot stop somebody where nothing is required — so writing the limit down loudly is the proportionate answer, and it is the pattern this project already uses for a trap it cannot remove.
ref: main — partially closed, deliberately, with the reason stated: the tooling_ref input, the manual procedure written out step by step, and a comment (test-appliance.yml:168-177) that says plainly nothing checks the change automatically. github.job_workflow_sha is deferred on the actionlint 1.7.7 constraint. Deferral accepted — the constraint is real and the workaround was immediately load-bearing. It does carry one new cost, below.
Dated issues — closed. keel-redis#10 and keel-nodejs-nginx#3, both opened 2026-09-28, both naming the exact error and the run ids, and keel-redis#10 explicitly exists "to own a red check and to give it an end". That is the right shape.

New, from the fix — MEDIUM, and not a blocker

test-appliance.yml:113-127 and :183. tooling_ref is a caller-declared input that substitutes the gate's own decision logic, and it is the one such input with none of the visibility its sibling just gained. allow_unpublished now shouts: a ::warning:: on the run, caveats in the summary, an error when it expires. tooling_ref has no annotation when it is not main, no expiry, and no error. KEEL_CI_COMMIT is recorded only in the Summary step, which is guarded by published == 'true' (:332), so on the exemption path — the one path that passes without booting — a non-default tooling_ref leaves no trace at all outside the raw step environment.

The scenario is a copy-paste: the documented procedure's YAML includes tooling_ref: my-branch, and somebody lifts that block into a real caller, or a throwaway caller gets merged. That repository's gate then runs whatever that branch says, for as long as the branch exists, silently. One line fixes it — ::warning:: when inputs.tooling_ref != 'main' — and by this pull request's own standard for allow_unpublished it should have one. Follow-up, not a condition.

LOW

bin/appliance-gate:56-62: summary()'s cat >> is still unchecked, five lines from the redirection just made fatal. With GITHUB_STEP_SUMMARY on a full filesystem the exemption path prints cat: write error: No space left on device and still returns 0, so the two caveats just added are dropped while the job passes. The ::warning:: annotation still fires, which is the part a reader actually sees, so this is cosmetic — but it is the same class, in the same family of functions.
die writes appliance-gate: … to stderr rather than an ::error::, so exits 4 and 5 annotate the run only as "Process completed with exit code N" while every other refusal gets a workflow annotation. Pre-existing pattern; the message is in the log either way.

Callers

All ten still comments and documentation only — zero added or changed YAML keys in any of them — and the inverted claim is gone from every one. Nine are CLEAN. keel-wordpress#9 has gone from BLOCKED to CLEAN on a re-run, which confirms its red was the intermittent TLS failure and not a fault. keel-nodejs-nginx#2 is UNSTABLE on the pre-existing missing boot test, now owned by keel-nodejs-nginx#3, and that repository has no protection so nothing is blocked by it.

Approve.

No CRITICAL and no HIGH. Both HIGH findings are fixed and I verified the fixes by breaking them again rather than by reading them; the read-back is sound and has run green in a real job on this head; the protection statement now matches the measurement; the three MEDIUMs are closed or deferred with reasons I accept. The one new MEDIUM and the two LOWs are follow-ups and none of them gates this.

Merge it, and rename the eight rules in the same window — the list in the description is correct, and until it is done every pull request in those eight repositories is blocked rather than unguarded.

## Re-review of `51aa16d` Both HIGH findings are closed, and closed with evidence rather than with an argument. I re-ran my own reproduction, tried four further ways to break the new code, and checked the new code where it matters most — in a real job, which it has now had. ### The HIGH that could still report an unearned success — closed `bin/appliance-gate:45-51`. My original break now fails, and so does the failure mode my repro did **not** cover. The distinction matters: I broke it with an unwritable file, which fails at `open`. ENOSPC is different — the open succeeds and the *write* fails — so I tested that separately with `GITHUB_OUTPUT=/dev/full`, which is exactly that shape: | probe | before | at `51aa16d` | | --- | --- | --- | | unwritable file, 200, no exemption | `EXIT=0`, nothing recorded | `EXIT=5`, "cannot record the decision …" | | `/dev/full` (open succeeds, write returns ENOSPC), 200 | — | `EXIT=5` | | `/dev/full`, 404 + exemption (the path that passes) | — | `EXIT=5` | The ordering is also right now: `boot()` calls `publish_decision` before it echoes, so the failure path no longer prints a sentence about a layer that will be booted. Nothing misleading survives it. ### The read-back — I gave it the scepticism it deserves, and it holds This is new code on the path that decides whether anything boots, added in response to a review, so I treated it as a suspect rather than as a fix. I transcribed the step body (`test-appliance.yml:205-244`) verbatim and drove it. - **The `tail -1` ordering guess holds, in both directions.** With a stale `published=false` already in the file and a real 200, the read-back returns `true`. With a stale `published=true` and the exemption taken, it returns `false`. The gate's write is always the last append, and last-wins is also how the runner itself resolves `steps.layer.outputs.*`, so the guess agrees with the thing it is guessing about. The `manifest=` line the step writes first is correctly ignored, because `awk -F=` reads its `$1` as `manifest`. - **No reachable false pass.** A stale value could only be mistaken for the gate's own if the gate exited 0 without writing. It cannot: the write is fatal when `GITHUB_OUTPUT` is non-empty, and when it is empty or unset the step dies four lines earlier — `GITHUB_OUTPUT` empty gives `: No such file or directory`, unset gives `unbound variable` under `set -u`, and a full filesystem makes the step's own `echo "manifest=…"` fail first. All three exit 1. - **No false failure on a good run**, which is the way new code on this path would most likely bite. Confirmed in place, not reasoned: run 36381555313 printed `the gate recorded published=true` and went on through pull, verify, assemble, boot and boot test; run 36381562472 printed `the gate recorded published=false` and skipped them. - A partial write cannot leave a half-written value to be read, because the write failing is fatal and the step aborts before the read-back runs. One residual, and it is the gate's CLI contract rather than a hole: with `GITHUB_OUTPUT` empty the script still reports a publishable layer and records nothing, by the `[[ -n … ]]` guard that the "outside Actions" test covers. It is unreachable from the workflow, per the above. **The new code was exercised before you asked me to look at it.** Runs 36381555313 (keel-core, 200, no exemption) and 36381562472 (keel-redis, 404 with the exemption) both ran with `tooling_ref: ci/name-the-gate-for-what-it-proves` and `KEEL_CI_COMMIT: 51aa16d0a85a57f72ce3e159bdc895c2484cb605` — this head, not main's copy. That is the new input paying for itself on its first day. The `::warning::` annotation is real: the Checks API returns it on the redis run, so it is visible without opening the summary. The two refusing paths (404 → exit 1, 200 + exemption → exit 2) were not re-run on this head. `never_published` and `stale_exemption` are untouched by this commit and neither calls `publish_decision`, and a failing gate aborts the step before the read-back, so I accept that gap rather than asking for two more runs. `bats tests/` is 71 green, `tests/coverage.sh` reports **100.00 total** with `bin/appliance-gate` at **100/100** — I reproduced both locally — and shellcheck is clean. ### The HIGH on branch protection — accurate, not merely changed I checked the new wording against what I measured, clause by clause. "GitHub fails closed"; "leaves the pull request blocked, waiting for a status that will never arrive"; "locks the eight repositories below rather than leaving them unguarded"; "`enforce_admins` is `true` on keel-core and keel-mariadb, so not even an owner could merge past it" — all four are what the keel-mariadb#16/#17 pair shows, and both `enforce_admins` values are `true`. The keel-core#10 and keel-redis#6 corroboration is reported correctly, including why #6 is the control. keel-core#12's comment now says the same and adds "`enforce_admins` is true here", which is right for that repository. The precision on "merge past it" is the correct one: an owner can still edit the rule, which is what unblocks it. Nothing to fix. ### The three MEDIUMs 1. **The exemption expires only where the check is required, and only for the declared name** — closed as far as the mechanism allows, and I accept it. Both limits are now printed in the summary of every run that uses the exemption (`bin/appliance-gate:128-139`), asserted by a test that greps for them, documented, and the notice became a `::warning::`. The underlying property is inherent — an error cannot stop somebody where nothing is required — so writing the limit down loudly is the proportionate answer, and it is the pattern this project already uses for a trap it cannot remove. 2. **`ref: main`** — partially closed, deliberately, with the reason stated: the `tooling_ref` input, the manual procedure written out step by step, and a comment (`test-appliance.yml:168-177`) that says plainly nothing checks the change automatically. `github.job_workflow_sha` is deferred on the actionlint 1.7.7 constraint. **Deferral accepted** — the constraint is real and the workaround was immediately load-bearing. It does carry one new cost, below. 3. **Dated issues** — closed. keel-redis#10 and keel-nodejs-nginx#3, both opened 2026-09-28, both naming the exact error and the run ids, and keel-redis#10 explicitly exists "to own a red check and to give it an end". That is the right shape. ### New, from the fix — MEDIUM, and not a blocker `test-appliance.yml:113-127` and `:183`. `tooling_ref` is a caller-declared input that substitutes the gate's own decision logic, and it is the one such input with none of the visibility its sibling just gained. `allow_unpublished` now shouts: a `::warning::` on the run, caveats in the summary, an error when it expires. `tooling_ref` has no annotation when it is not `main`, no expiry, and no error. `KEEL_CI_COMMIT` is recorded only in the Summary step, which is guarded by `published == 'true'` (`:332`), so on the exemption path — the one path that passes without booting — a non-default `tooling_ref` leaves no trace at all outside the raw step environment. The scenario is a copy-paste: the documented procedure's YAML includes `tooling_ref: my-branch`, and somebody lifts that block into a real caller, or a throwaway caller gets merged. That repository's gate then runs whatever that branch says, for as long as the branch exists, silently. One line fixes it — `::warning::` when `inputs.tooling_ref != 'main'` — and by this pull request's own standard for `allow_unpublished` it should have one. Follow-up, not a condition. ### LOW - `bin/appliance-gate:56-62`: `summary()`'s `cat >>` is still unchecked, five lines from the redirection just made fatal. With `GITHUB_STEP_SUMMARY` on a full filesystem the exemption path prints `cat: write error: No space left on device` and still returns 0, so the two caveats just added are dropped while the job passes. The `::warning::` annotation still fires, which is the part a reader actually sees, so this is cosmetic — but it is the same class, in the same family of functions. - `die` writes `appliance-gate: …` to stderr rather than an `::error::`, so exits 4 and 5 annotate the run only as "Process completed with exit code N" while every other refusal gets a workflow annotation. Pre-existing pattern; the message is in the log either way. ### Callers All ten still comments and documentation only — zero added or changed YAML keys in any of them — and the inverted claim is gone from every one. Nine are `CLEAN`. keel-wordpress#9 has gone from `BLOCKED` to `CLEAN` on a re-run, which confirms its red was the intermittent TLS failure and not a fault. keel-nodejs-nginx#2 is `UNSTABLE` on the pre-existing missing boot test, now owned by keel-nodejs-nginx#3, and that repository has no protection so nothing is blocked by it. --- **Approve.** No CRITICAL and no HIGH. Both HIGH findings are fixed and I verified the fixes by breaking them again rather than by reading them; the read-back is sound and has run green in a real job on this head; the protection statement now matches the measurement; the three MEDIUMs are closed or deferred with reasons I accept. The one new MEDIUM and the two LOWs are follow-ups and none of them gates this. Merge it, and rename the eight rules in the same window — the list in the description is correct, and until it is done every pull request in those eight repositories is blocked rather than unguarded.

marcos-mendez commented

2026-09-28 03:26:04 -03:00

(Migrated from github.com)

Both closed in 62c2563, and the third point taken as offered.

tooling_ref visibility. It now emits a ::warning:: naming the ref
whenever it is not main, from bin/appliance-gate so the case is testable,
and writes a summary line saying the logic did not come from main. It runs
before the case, so every path carries it, including the exemption path that
passes without booting. Separately, the checkout step now writes the ref and
the resolved commit into the summary unconditionally instead of leaving
KEEL_CI_COMMIT to the published == 'true' Summary step, which was the gap
you named. Five tests: non-default announced on the boot, exemption and
refusal paths, and silent on main and on unset.

Measured on run 36385954751: the annotation is on the check run with the ref
in it, and the same run pulled, verified, assembled and booted core in 43
seconds. The visibility buys no false failure.

summary()'s unchecked cat. Fatal now, exit 5, reproduced with
GITHUB_STEP_SUMMARY=/dev/full. Worth noting why it is not a one-liner:
summary is always the right hand side of a pipeline, so a die inside it
would exit a subshell and the script would carry on, which is this bug class
again. It returns 5, every call site ends | summary || summary_failed, and
pipefail carries it to the caller. The /dev/full test is what proves the 5
arrives rather than being swallowed by the subshell.

die emits ::error:: rather than writing to stderr, so exits 4 and 5
annotate the run.

Re-ran your /dev/full reproduction on GITHUB_OUTPUT after the change: the
two paths that record a decision, 200 and 404 with the exemption, both
exit 5. 404 without the exemption exits 1, since it records no decision and
refuses regardless. No path reaches a pass.

77 tests green, coverage 100.00 total with bin/appliance-gate at 115/115,
actionlint 1.7.7 and shellcheck clean. Branch protection untouched: I re-read
all ten rules and they are exactly the table in the description.

Both closed in 62c2563, and the third point taken as offered. **`tooling_ref` visibility.** It now emits a `::warning::` naming the ref whenever it is not `main`, from `bin/appliance-gate` so the case is testable, and writes a summary line saying the logic did not come from `main`. It runs before the `case`, so every path carries it, including the exemption path that passes without booting. Separately, the checkout step now writes the ref and the resolved commit into the summary unconditionally instead of leaving `KEEL_CI_COMMIT` to the `published == 'true'` Summary step, which was the gap you named. Five tests: non-default announced on the boot, exemption and refusal paths, and silent on `main` and on unset. Measured on run 36385954751: the annotation is on the check run with the ref in it, and the same run pulled, verified, assembled and booted `core` in 43 seconds. The visibility buys no false failure. **`summary()`'s unchecked `cat`.** Fatal now, exit 5, reproduced with `GITHUB_STEP_SUMMARY=/dev/full`. Worth noting why it is not a one-liner: `summary` is always the right hand side of a pipeline, so a `die` inside it would exit a subshell and the script would carry on, which is this bug class again. It returns 5, every call site ends `| summary || summary_failed`, and `pipefail` carries it to the caller. The `/dev/full` test is what proves the 5 arrives rather than being swallowed by the subshell. **`die` emits `::error::`** rather than writing to stderr, so exits 4 and 5 annotate the run. Re-ran your `/dev/full` reproduction on `GITHUB_OUTPUT` after the change: the two paths that record a decision, `200` and `404` with the exemption, both exit 5. `404` without the exemption exits 1, since it records no decision and refuses regardless. No path reaches a pass. 77 tests green, coverage 100.00 total with `bin/appliance-gate` at 115/115, actionlint 1.7.7 and shellcheck clean. Branch protection untouched: I re-read all ten rules and they are exactly the table in the description.