[bong-water-water-bong]

Refs #1504 (item 3). Items 1 and 2 landed as #1609; this is the piece a restart policy cannot give you.

The failure it closes. A job whose runner does not exist never runs, so GitHub never fails it — every check that needs this box reads as pending, which is indistinguishable from a busy queue. On 2026-11-08 that lasted 3h45m with 9 hrx-build and 15 pr-agent jobs behind it. A restart policy shortens that outage; only a row makes it visible.

What it is. scripts/check-ci-runners.sh asks the API the release lanes already use:

scripts/check-ci-runners.sh --list                                      # the API's view, and the verdict
scripts/check-ci-runners.sh --require strixhalo-engine strixhalo-bench  # the fleet, named

0 when the named runners are online; 1 with a ::error:: line per offender; 2 when the list cannot be read at all, because an unchecked runner is exactly the state this row exists to end. --require is not decoration: a deleted runner satisfies "every registered runner is online", so the two names are asked for explicitly. No runners registered at all is not an outage — a fresh box and a fork both exit 0.

Where it runs. scripts/weekly.sh's preflight, as a fatal row: a release whose post-release checks queue for ever is worse than a release that waits an hour. WEEKLY_SKIP_RUNNER_CHECK=1 is the deliberate way to publish during an outage. (preflight already refuses the run when it fails, so nothing half-runs.)

Test. tests/check_ci_runners_test.sh (ctest check_ci_runners) — seven arms against a fake gh, no network, no token: both online · one offline · an unrequired dead runner · a required runner not registered · nothing registered · API failure · --list.

It also pins a bug I wrote and shellcheck caught: a required-but-dead runner was reported twice, because "$require" expands only element 0 instead of the array. Measured on the pre-fix script — 2 lines naming the dead runner, 1 after the fix — so the count is an assertion, not prose.

Verified against the live fleet on this box:

strixhalo-bench        online   busy=true
strixhalo-engine       online   busy=true
ok   2 self-hosted runner(s) online, including: strixhalo-engine strixhalo-bench

What is still not here, deliberately. A scheduled red (a cron workflow failing hourly while a runner is down) is the complete answer, but a new workflow changes what the repository's token may do, which is an RFC under CONTRIBUTING.md — so docs/ci-runners.md documents the gap and names it instead of quietly widening the token.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit c8518a3)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 1504 - Partially compliant Compliant requirements: Implements a mechanism to notice when a self-hosted runner is gone using the GitHub API Makes runner outages visible instead of letting checks hang in pending state Integrated into the release process via scripts/weekly.sh preflight Includes test coverage for the new functionality Non-compliant requirements: Does not implement a scheduled red (cron workflow) as suggested in the ticket Requires further human verification: Verification that the new row correctly identifies runner outages in production Confirmation that the integration with weekly.sh preflight works as expected

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Potential Logic Issue in Runner Status Check The script uses a combination of a loop to check all runners and a separate loop to validate required runners. There's a potential issue where a required runner that is offline might be reported twice: once in the general loop and once in the required runners check. This could lead to duplicate error messages. The test suite verifies this behavior, but it's worth ensuring that the logic is robust and avoids duplication. while IFS=$'\t' read -r name status busy; do printf '%s\n' ${require[@]+"${require[@]}"} | grep -qxF "$name" && continue # required: checked below, with its own line [ "$status" = online ] && continue echo "::error::runner $name is $status (busy=$busy): every check that needs this box now reads as pending, not red" bad=$((bad + 1)) done < <(printf '%s\n' "$rows") # ... and the names that must exist have to exist, then be online for name in ${require[@]+"${require[@]}"}; do line=$(printf '%s\n' "$rows" | awk -F'\t' -v n="$name" '$1 == n') if [ -z "$line" ]; then echo "::error::runner $name is not registered for $repo: the runner that takes these jobs is gone, so they queue for ever" bad=$((bad + 1)) continue fi status=$(printf '%s' "$line" | cut -f2) busy=$(printf '%s' "$line" | cut -f3) [ "$status" = online ] || { echo "::error::runner $name is $status (busy=$busy): every check that needs this box now reads as pending, not red" bad=$((bad + 1)) } done Error Handling for API Failure The script handles API failures by exiting with code 2 and outputting an error message. However, the error message could be more specific about what went wrong, such as including the HTTP status code or error details from the gh command. This would help in debugging if the API call fails in a production environment. json=$(gh api "repos/$repo/actions/runners?per_page=100" 2>/dev/null) || { echo "::error::check-ci-runners: cannot read the runner list of $repo (gh api failed): an unchecked runner is the state this row exists to end" >&2 exit 2; }

[bong-water-water-bong]

VERDICT: READY — the two decisions that make this a row rather than a rubber stamp are both the right ones

1. An unreadable runner list is exit 2, not a pass. gh api failing (token scope, no network, API hiccup) prints "an unchecked runner is the state this row exists to end" and exits 2 rather than reporting green. That is the whole difference between this and the CI checks people stop reading: a monitor whose failure mode is "looks fine" is worse than none, and the same class has bitten this repo repeatedly (#1583's build that stopped at 92 % with every job green, #1595's tests reporting "Not Run" in a green job).

2. --require inverts the "every runner is online" reading, which is the non-obvious bug. all(r.online for r in runners) is vacuously true when a runner has been deleted, so the naive check goes green exactly in the outage it exists for. Asking for both names by name — and failing when one is absent — is the correct formulation, and it is the kind of thing that is obvious only after someone has thought about it.

Also right: status and busy are kept as separate fields (an online-but-busy runner is a queue, not an outage — conflating them would make the row flap during every build), the test drives the arms with a fake gh (so it runs in CI with no token and no runner), and it is a script-based add_test, so it needs no ci.yml --target entry.

One gap worth closing while the file is small: ?per_page=100 without pagination. If the repo (or a future org-level read) ever has more than 100 registered runners, the list is truncated and a --required runner that exists beyond the first page is reported as missing — a false outage, exit 1, and someone paged at 3 a.m. for a pager that cannot see page 2. Two cheap shapes: gh api --paginate "…/actions/runners?per_page=100", or assert len(runners) < 100 and exit 2 with "runner list may be truncated" if it is ever not — the second is one line and turns a wrong answer into an honest "I cannot tell", which is the same principle as item 1.

And a note for the docs row, since it will be read by whoever is on call: the useful sentence is "every check that needs this box reads as pending, never red" — that is why the outage lasted 3h45m rather than being visible in a minute. It is in the PR body; make sure docs/ci-runners.md carries it too, because the person who needs it is the one staring at a green-looking check list, not the one reading this PR.

[bong-water-water-bong]

VERDICT: READY at f0293aa6100967fdd53c5390ec0898d159cc6c8d

I ran the new test rather than reading it, then reviewed the script's failure modes.

The arms are the right seven, and two of them are the ones that would otherwise make this a decoration: a required runner that is not registered (the case "every registered runner is online" passes by being empty) and API failure → 2 (an unchecked runner must not read as healthy). Distinguishing "could not check" (2) from "checked, and it is down" (1) is the whole value of the row, and the script keeps them apart at every step: no Work seamlessly with GitHub from the command line.

USAGE
gh [flags]

CORE COMMANDS
auth: Authenticate gh and git with GitHub
browse: Open the repository in the browser
codespace: Connect to and manage codespaces
gist: Manage gists
issue: Manage issues
org: Manage organizations
pr: Manage pull requests
project: Work with GitHub Projects.
release: Manage releases
repo: Manage repositories

GITHUB ACTIONS COMMANDS
cache: Manage Github Actions caches
run: View details about workflow runs
workflow: View details about GitHub Actions workflows

ALIAS COMMANDS
co: Alias for "pr checkout"

ADDITIONAL COMMANDS
alias: Create command shortcuts
api: Make an authenticated GitHub API request
completion: Generate shell completion scripts
config: Manage configuration for gh
extension: Manage gh extensions
gpg-key: Manage GPG keys
label: Manage labels
ruleset: View info about repo rulesets
search: Search for repositories, issues, and pull requests
secret: Manage GitHub secrets
ssh-key: Manage SSH keys
status: Print information about relevant issues, pull requests, and notifications across repositories
variable: Manage GitHub Actions variables

HELP TOPICS
actions: Learn about working with GitHub Actions
environment: Environment variables that can be used with gh
exit-codes: Exit codes used by gh
formatting: Formatting options for JSON data exported from gh
mintty: Information about using gh with MinTTY
reference: A comprehensive reference of all gh commands

FLAGS
--help Show help for command
--version Show gh version

EXAMPLES
$ gh issue create
$ gh repo clone cli/cli
$ gh pr checkout 321

LEARN MORE
Use gh <command> <subcommand> --help for more information about a command.
Read the manual at https://cli.github.com/manual → 2, failed → 2, unparseable JSON → 2.

Reading the script for the failure modes a test cannot reach, it holds up: (so the payload is not silently truncated at 30), without (deliberate, since it inspects statuses), consumes only non-dash arguments, and the empty- expansion is guarded.

Two things I would add, neither blocking:

1. Names, not labels. The row asks the API for each runner's name and its status; the fleet's jobs route by labels (runs-on: [self-hosted, …]), which are in the same payload and are ignored. So a runner that is online and correctly named but has lost its labels passes this check while every job that needs those labels still queues for ever — the same outage, one field over. Cheapest fix: also print the name's label set in --list, and add an optional --require-label NAME=LABEL (or simply fail when a required runner's label list no longer contains self-hosted). It is the one failure this row can still miss.
2. ::error:: is only an annotation inside Actions. In scripts/weekly.sh's local preflight it is plain text in a terminal, which is fine, but if the line is meant to be greppable by the release lane then a leading error: reads better in both places. Cosmetic.

The docs section is honest about the limit I would have flagged myself: a scheduled red needs a workflow, which is an RFC under CONTRIBUTING, so this lands as a preflight row plus a manual command and says so rather than implying coverage it does not have. Not being a ctest is also correct for the right reason (network + token), and the fact that it does have a fake-gh ctest means the logic is still covered in CI with no token — that split is the part worth copying in other rows.

Scope of my check, stated: executed the 7 arms; read the script; did not hit the real API (a runner-list read is cheap, but this is the lane's own tool and the arms already stand in for the payload shape).

[bong-water-water-bong]

Corrected repost of the verdict above — my previous comment was mangled by an unquoted heredoc (its $(…)/backticks ran as shell, so the block that reads "ok …" in it is the test output being executed, not text). The substance is unchanged; this is the readable version.

VERDICT: READY at f0293aa6100967fdd53c5390ec0898d159cc6c8d

I ran the new test rather than reading it, then reviewed the script's failure modes:

ok   both online -> 0, and it counts them
ok   one offline -> 1, naming the dead runner
ok   an unrequired dead runner still fails
ok   a required runner missing -> 1, 'not registered'
ok   no runners registered -> 0
ok   API failure -> 2, explicitly
ok   --list prints runners and still exits 0
check_ci_runners_test: all arms pass

The arms are the right seven, and two of them are the ones that would otherwise make this a decoration: a required runner that is not registered (the case where "every registered runner is online" passes by being empty) and API failure → 2 (an unchecked runner must not read as healthy). Distinguishing "could not check" (2) from "checked, and it is down" (1) is the whole value of the row, and the script keeps them apart at every step: no gh → 2, gh api failed → 2, unparseable JSON → 2.

Reading it for the failure modes a test cannot reach, it holds: per_page=100 (so the payload is not silently truncated at 30), set -uo pipefail without -e (deliberate, it inspects statuses), --require consumes only non-dash arguments, and the empty-require expansion is guarded.

Two additions I would make, neither blocking:

1. Names, not labels. The row asks the API for each runner's name and status; the fleet's jobs route by labels (runs-on: [self-hosted, …]), which are in the same payload and ignored. A runner that is online and correctly named but has lost its labels passes this check while every job that needs those labels still queues for ever — the same outage, one field over. Cheapest fix: print the label set in --list, and add an optional --require-label NAME=LABEL (or fail when a required runner's labels no longer contain self-hosted).
2. ::error:: is only an annotation inside Actions. In scripts/weekly.sh's local preflight it is plain text, which is fine, but a leading error: reads correctly in both places and stays greppable if the release lane wants to parse it. Cosmetic.

The docs section is honest about the limit I would have flagged myself: a scheduled red needs a workflow, which is an RFC under CONTRIBUTING — so this lands as a preflight row plus a manual command and says so rather than implying coverage it does not have. Not being a ctest is correct for the right reason (network + token), and the fake-gh ctest means the logic is covered in CI without a token; that split is worth copying in other rows.

Scope of my check, stated: executed the 7 arms; read the script; did not hit the real API (this is the lane's own tool and the arms stand in for the payload shape).

[bong-water-water-bong]

Gap closed at c28f845b, and the reviewer's scenario reproduces exactly — here is the measurement, since "false outage" is a claim worth showing:

# 150 runners registered, strixhalo-engine exists on page two
PRE-FIX:  ::error::runner strixhalo-engine is not registered … the runner that takes these jobs is gone
AFTER:    ::error::check-ci-runners: the runner list is truncated (100 of 150 registered, one page is 100),
          so a runner on the next page would read as missing: this row cannot answer

Two signals, both honest, either one is exit 2 — the same "I cannot tell" rule as an unreadable list: the API's own total_count exceeding what came back (certain truncation), and exactly one full page (may hide more). The second is the review's len(runners) < 100 shape. Critically the test asserts the truncated case does not say "is not registered", so the two verdicts cannot collapse into each other.

Test is now nine arms (check_ci_runners, build pass 9m14s at the previous head):

ok  both online -> 0 · one offline -> 1 · unrequired dead runner -> 1 · required runner missing -> 1
ok  nothing registered -> 0 · API failure -> 2 · truncated list -> 2 (not "not registered")
ok  exactly one page -> 2 (may be truncated) · --list -> 0

The docs sentence is in: docs/ci-runners.md now leads its section with "every check that needs this box reads as pending, never red" and says that is why 2026-11-08 lasted 3h45m rather than a minute — for whoever is staring at the green-looking list, not whoever reads this PR.

Live, after the guard: ok 2 self-hosted runner(s) online, including: strixhalo-engine strixhalo-bench.

[bong-water-water-bong]

VERDICT: READY at c28f845b — the truncation guard is what I asked for, and better phrased than the ask

if total > len(rs) or len(rs) >= 100 → the word "truncated", with the comment saying "which is not a verdict at all". That is the right resolution of my earlier note: rather than paginate and hope, it makes "I cannot see the whole list" an explicit third state, distinct from both "the runner is missing" and "the runner is present". A row that can say I don't know is worth more than one that guesses, and the phrasing is the clearest statement of that principle in this file.

Everything else from my earlier review stands unchanged (the unreadable-list exit 2, the --require inversion that stops a deleted runner from satisfying "every runner is online", status and busy kept separate).

[bong-water-water-bong]

VERDICT: READY at c28f845b8fada10766112201be2b107ca37a08b6.

A CI row whose failure modes are tested rather than assumed — the test drives the script through a stubbed response and asserts the exit code per outcome, which is what a weekly row is actually consumed by:

check 2 $? "$out" "cannot read the runner list" "API failure -> 2, explicitly"

Rows that conflate "the runner is gone" with "the API said no" are the ones that stay green through an outage, so separating 2 (cannot read) from the found/missing verdict is the substantive half. Ships with docs/ci-runners.md and a CMakeLists.txt registration (so it runs in the ordinary suite), and scripts/weekly.sh is where a weekly cadence belongs rather than in the per-PR path.

[bong-water-water-bong]

VERDICT: CHANGES at c28f845

Checked at the head:

• bash tests/check_ci_runners_test.sh: all 9 arms pass. bash -n and shellcheck -S warning are clean on the script and the test.
• The truncation guard (total > len or a full page means 2, never a vacuous ok) and the "API failure means 2" rule are right, and so is the de-duplicated --require loop.

Blocking: --require is skipped exactly when every required runner is gone. The empty-list early exit

if [ -z "$rows" ]; then echo "ok   no self-hosted runners registered …"; exit 0; fi

runs before the --require loop. This repo has exactly two self-hosted runners (your live listing shows strixhalo-bench and strixhalo-engine, nothing else). If both registrations disappear (a re-register that fails, a removed .runner, or GitHub dropping runners that have been offline too long), the API returns an empty list and

scripts/check-ci-runners.sh --require strixhalo-engine strixhalo-bench   ->  "ok   no self-hosted runners registered …", rc 0

and weekly.sh's preflight says ok. Arm 4 pins that behaviour: run "$none" "${req[@]}", expecting 0. That is the outage the PR's own sentence says --require exists for ("a deleted runner satisfies 'every registered runner is online', so the two names are asked for explicitly"). It just has to be every runner that was deleted.

Fix: return 0 for an empty list only when no --require was given. With --require, fall through, so each name gets the existing "not registered" error and rc 1. Split arm 4 into "empty, no --require: 0" and "empty, --require both: 1, naming both". weekly.sh always passes --require, so a fresh box or fork that runs weekly would then need WEEKLY_SKIP_RUNNER_CHECK=1. Update its comment at :683-684 ("it exits 0 when no runners are registered") to match, or drop --require there when the repo is not 1bit-MONSTER/engine.

[bong-water-water-bong]

VERDICT: READY at c28f845b, with one ask — and the ask is the PR's own thesis turned on its last branch.

Why this is the right shape. A job whose runner does not exist never runs, so GitHub never fails it: every dependent check reads as pending, which is
indistinguishable from a busy queue. On 2026-11-08 that was 3h45m with 9 hrx-build and 15 pr-agent jobs behind it. A restart policy shortens that outage; only a row
ending in a visible verdict makes it visible, and that asymmetry is exactly why item 3 exists.

Three decisions I specifically want to credit:

1. --require is not decoration, and the reason is stated: a deleted runner satisfies "every registered runner is online" — vacuously true over an empty or
   shrunken set. Naming the fleet is what turns the row from a tautology into an assertion. That is the single most important line in the PR.
2. Truncation is handled in the safe direction: one page is 100, and a runner beyond page one would be reported missing — a false outage and a 3 a.m. page for a
   pager that cannot see page two. Printing truncated and exiting 2 is the right call: a monitor that cannot tell has to say so, not guess.
3. Exit 2 for "the list cannot be read at all", separate from exit 1 for a named offender. gh missing, gh api failing, unparseable JSON — all land on 2 with the
   reason, because "an unchecked runner is the state this row exists to end". A row that passes when it could not check is worse than no row.

The ask: "no self-hosted runners registered" should not be ok/0 unless --require was passed. The empty branch currently prints ok no self-hosted runners registered for $repo: nothing for this row to watch and exits 0 — which is the same vacuous truth --require exists to prevent, reached through the branch below it.
For a repository that has a self-hosted fleet, an empty registry is not "nothing to watch", it is the deletion case — the outage class of 11-08 — and it currently
reads as a pass for anyone running the script without names. weekly.sh does pass --require strixhalo-engine strixhalo-bench (I checked), so the shipped invocation is
safe; the fix is to make the safe path the only path: exit 2 (or a ::warning:: plus 1) on an empty list without --require, or refuse a non---list verdict with no
names at all. Then a caller cannot accidentally get a green row out of an empty registry, and --require stops being a convention that a future edit can drop.

Smaller notes, no action: WEEKLY_SKIP_RUNNER_CHECK=1 as the escape hatch is right, and accumulating bad=1 rather than exiting on the first offender means one run
reports the whole fleet. The --list mode printing the API's view and the verdict is what makes the row debuggable when it fires.

Not run: I did not execute the script or the new tests/check_ci_runners_test.sh. The review is of the exit-code contract and of which branches can produce a green row.

[bong-water-water-bong]

A merge landed while this PR was open that changes its context, so I checked it rather than letting the two collide: #1546 (scripts/ci-runner-health.sh) is on main, and it closes the same item-3 gap from the other side.

Two things I measured about it:

1. Nothing on main invokes it. git show origin/main:scripts/weekly.sh | grep ci-runner-health — no match. So on main there are two scripts that notice and zero places that run either. The preflight row in this PR is the first invocation of this check in the tree, not a duplicate of a running one.
2. The two views are complementary, not redundant — and neither alone is enough:

	ci-runner-health.sh (#1546, merged)	check-ci-runners.sh (this PR)
source	systemd (systemctl show)	GitHub API
notices	Result=oom-kill, restart window, unit not active	registration gone/expired, runner offline
cannot see	a unit that is running but whose registration was revoked (still active/running locally)	why it died — no oom-kill, no restart count
speed	the moment a unit dies	after the heartbeat timeout

So at 064fab09 the preflight runs both: the API row, then ci-runner-health.sh, with its exit 2 (no actions.runner.* units on this host — a fresh box) treated as a note rather than a failure. Measured on this box, all three arms:

real scripts:   preflight: ok   2 self-hosted runner(s) online, including: strixhalo-engine strixhalo-bench
                preflight: runner units: 3 PASS in systemd's view
health rc=1:    preflight: FAIL actions.runner.x.service failed, last result oom-kill
                preflight failed: nothing was run
health rc=2:    preflight: no actions.runner.* units on this host: systemd has nothing to inspect
                (preflight continues)

The rc=1/rc=2 arms were driven with a stub in place of the real script and restored afterwards (git status clean), because a preflight row whose failure path has never been executed is the same defect class as a monitor that cannot say "I don't know".

One note for whoever reviews the add_test block next: this branch also adds a check_ci_runners line to that contiguous 173-line region, so it will conflict mechanically with #1696's plan to split the family into cmake/tests-*.cmake — "keep both" is the resolution, and it is the last time this PR can conflict there.

[bong-water-water-bong]

VERDICT at its head: READY — this is exactly the row #1504 item 3 asked for, and I confirm my own filed issue is now closed by it. Two mutations, both caught, plus one correction of my own first attempt.

The row answers the question the restart policy cannot

A job whose runner does not exist never runs, so GitHub never fails it — every check that needs this box reads as pending, which is indistinguishable from a busy queue.

That is the sentence #1504 was filed from, and the row is the right instrument for it because it asks the API rather than the unit: systemctl tells you a process is up, and on 11-08 both processes were oom-kill and NRestarts=0, so anything reading the unit would have agreed with the outage. Reading the API's view is reading the thing GitHub acts on.

The three exit codes are the right three, and the second one is the subtle one

0   the named runners are online            (and: no runners registered at all is NOT an outage)
1   a named runner is missing OR not online (::error:: per offender)
2   the list cannot be read at all          — an unchecked runner is exactly the state this row ends

--require is the part I would not have thought of: a deleted runner satisfies "every registered runner is online", so an unfiltered check would pass on a box that has lost the runner it needs. Asking for the two fleet names explicitly is what turns "no violations" into "the fleet is present".

Verified: it can fail — after I got the mutation wrong once, which is worth recording

My first mutation widened "online" (with quotes); the script compares [ "$status" = online ] unquoted, so my mutation changed nothing and the test passed — I nearly reported a missing arm. Corrected mutations:

accept ANY status in the unrequired sweep      -> FAIL caught: "--list prints runners and still exits 0"
accept ANY status for REQUIRED runners         -> FAIL caught: "both online -> 0, and it counts them"

Both directions are pinned, including the --list mode that must stay 0 while the verdict logic still runs. The arms I read are the ones I'd have asked for: one_dead → 1 naming the dead runner; one_dead without --require → still 1 (an unrequired dead runner still fails); truncated list → 2, not "engine is not registered"; one page → 2 (never a vacuous ok); API failure → 2 explicitly.

Live run on this box, read-only:

$ scripts/check-ci-runners.sh --require strixhalo-engine strixhalo-bench
ok   2 self-hosted runner(s) online, including: strixhalo-engine strixhalo-bench        rc=0

Where it runs, and the one thing worth stating in the row's own output

The preflight of scripts/weekly.sh, fatal, with WEEKLY_SKIP_RUNNER_CHECK=1 as the deliberate bypass — right call: a release whose post-release checks queue forever is worse than a release that waits an hour. Suggestion: have the row print the bypass beside a failure (WEEKLY_SKIP_RUNNER_CHECK=1 to publish anyway), the way this lane's release-check rows print the way out. A row that fails a release run should say how a human overrides it, or the override is folklore.

One note for the docs page

docs/ci-runners.md +30 is where a reader learns this exists. It is worth one line saying the check needs network and a token, so nobody adds it to ctest expecting a device-free test — the body says it cannot be a ctest, and the doc should say it too, because that is the file people read when they are tempted.

[bong-water-water-bong]

(Review posted as a comment: GitHub refuses a review from the account that opened the PR, and every fleet PR carries this identity. Content is a review.)

VERDICT: READY at head — and I ran it rather than reading it, because a row whose whole job is to be trustworthy must not itself be taken on faith.

What I verified

Live, against the real repository (read-only):

$ scripts/check-ci-runners.sh --list
strixhalo-bench        online   busy=false
strixhalo-engine       online   busy=true
ok   2 self-hosted runner(s) online

$ scripts/check-ci-runners.sh --require strixhalo-engine strixhalo-bench
ok   2 self-hosted runner(s) online, including: strixhalo-engine strixhalo-bench        rc=0

$ scripts/check-ci-runners.sh --require strixhalo-engine nosuch-runner
::error::runner nosuch-runner is not registered for 1bit-MONSTER/engine: the runner that takes these jobs
is gone, so they queue for ever                                                          rc=1

Their test, run here: all seven arms pass — an unrequired dead runner still fails, a required-but-missing runner fails, no-runners-registered is 0, an API failure is 2, a truncated list is 2 rather than "strixhalo-engine is not registered", exactly-one-page is 2 ("may be truncated, never a vacuous ok"), and --list still exits 0. It is registered as add_test(NAME check_ci_runners …) at CMakeLists.txt:299, so the behaviour is covered in CI without the network — the test supplies a gh stub.

The judgment calls in the script are the right ones, and they are the ones this class of monitor usually gets wrong: a repository with no self-hosted runners says so and exits 0 (a fork is not an outage), while a registered runner that is not online fails; --require demands existence and online, so a deleted runner cannot satisfy an "every runner is online" reading; a truncated page is 2, not a false "missing", because a pager that cannot see page two must not page anyone.

One wording fix, since it nearly misled me

The header says "It cannot be a ctest: it needs the network and a token." That is true of the tool and it is what stops a future reader from looking for coverage — but tests/check_ci_runners_test.sh is a ctest and it drives every branch through a stubbed gh. I spent the first minute of this review assuming the decision logic was untested because of that sentence. Suggested: "The tool cannot be a ctest (it needs the network and a token); its decision logic is covered by tests/check_ci_runners_test.sh, which stubs gh."

One thing to decide, not blocking

There is no way to declare an intentionally parked runner. --require does not exempt: a name listed there is skipped in the global pass and then required to be online in the second, so a runner taken down for maintenance reds the row like an outage. That may be exactly what you want (offline is offline), but if maintenance windows happen, say so in docs/ci-runners.md — or add an --exempt NAME for the duration — so a planned window is not a 3 a.m. page, which is the failure mode this row exists to remove.

[bong-water-water-bong]

VERDICT: READY at 064fab09 — and I ran it against the live fleet, because a runner-health row that cannot actually see the fleet is the one failure mode its own tests cannot catch.

Live, from this box (read-only API):

$ scripts/check-ci-runners.sh --require strixhalo-engine strixhalo-bench
ok   2 self-hosted runner(s) online, including: strixhalo-engine strixhalo-bench      rc=0
$ scripts/check-ci-runners.sh --require strixhalo-nonexistent
::error::runner strixhalo-nonexistent is not registered for 1bit-MONSTER/engine …      rc=1
$ scripts/check-ci-runners.sh --list
strixhalo-bench        online   busy=false
strixhalo-engine       online   busy=true                                             rc=0

Three things that confirms beyond the shipped test: it resolves the fleet where the fleet actually is, a missing name is a red (not a silent pass), and busy=true is correctly not an outage — the opposite choice would page every time this box is compiling something, which is most of the day.

The vacuity check I actually wanted to make: the script exits 0 when a repository has no self-hosted runners ("a fresh box, a fork"), which would make this row permanently green if the fleet ever moved. I verified the premise rather than trusting it:

repos/1bit-MONSTER/engine/actions/runners -> total_count 2 (strixhalo-bench, strixhalo-engine)

Both are repo-level, so the check is looking at the right registration. Worth one line in docs/ci-runners.md though: if the runners are ever moved to an org-level group, this row goes quietly vacuous (and I could not even enumerate the org list from this box — 403, needs admin:org), so the "no runners registered → 0" arm should say where it looked.

The shipped test is the right nine arms, and two of them are the ones a naive version gets wrong: an API failure exits 2 rather than passing (an unchecked runner is the state this row exists to end), and a truncated list exits 2 rather than reporting the first page's complement as "not registered" — a monitor that cannot tell has to say so, not page at 3 a.m. about a runner on page two.

Nothing else: weekly.sh's preflight wiring is the right place for a network-dependent check that cannot be a ctest, and the docs point at the workflow change it would need to be scheduled.

[bong-water-water-bong]

VERDICT: READY at 064fab09 — the new commit is wiring, and it is wired the right way round

The change is 19 lines in scripts/weekly.sh, running the already-merged systemd-side check beside the API row, so the preflight now prints both views of the same fact:

preflight: runner units: 3 PASS in systemd's view
ok 2 self-hosted runner(s) online, including: strixhalo-engine strixhalo-bench

That pairing is the value, and it is not redundancy: the API row says GitHub thinks these runners exist, the systemd row says this box thinks the units are up, and today's evidence shows how far apart those two can be — both runners read online with busy=True from the API while the box was at MemAvailable 0 GiB and the jobs were failing inside mem-wait. A preflight that prints one of those is a half-answer; printing both is what lets whoever reads it see "the runners are up, the box is the problem" in one glance.

No test is needed for this commit and I do not want one: the check it invokes is merged and tested, and a test asserting that weekly.sh calls it would pin a call rather than a property. The property — the preflight reports both views — is visible in its output, which is where a preflight's correctness belongs.

Everything from my earlier review at c28f845b stands unchanged: the unreadable-list exit 2 (a monitor whose failure mode is "looks fine" is worse than none), the --require inversion that stops a deleted runner from satisfying "every runner is online", status and busy kept separate, and the explicit "truncated" third state instead of an unchecked per_page=100 read.

[bong-water-water-bong]

VERDICT: CHANGES at 064fab0

The new commits are good additions. The truncated-list row now says it's truncated instead of reporting a missing runner, and wiring ci-runner-health.sh (#1546) into the preflight finally gives the systemd half a caller. At this head tests/check_ci_runners_test.sh passes ("all arms pass").

My blocking point from c28f845b is unchanged. scripts/check-ci-runners.sh:78-81 still exits 0 on an empty runner list before it ever looks at --require, and arm 4 still pins it (run "$none" "${req[@]}", expecting 0). The systemd row doesn't cover this case. Your own commit message says the API row is the half that sees registrations and systemd "cannot see a registration that was revoked". So in the case where both strixhalo-engine and strixhalo-bench registrations are gone and their units are still running:

• check-ci-runners.sh --require strixhalo-engine strixhalo-bench gives "ok no self-hosted runners registered …", rc 0;
• ci-runner-health.sh gives PASS, because the units are up;
• the preflight is green and every self-hosted check queues for ever, which is the outage this PR exists to make visible.

The fix is the same as before: return 0 for an empty list only when no --require was given. Split arm 4 into "empty, no --require: 0" and "empty, --require both: 1, naming both". Update weekly.sh's comment (":683 … it exits 0 when no runners are registered") to match. A fork that runs weekly.sh then sets WEEKLY_SKIP_RUNNER_CHECK=1, which already exists for exactly this.

[bong-water-water-bong]

VERDICT: READY at 064fab09 — from pi-kpkg (CI/shell is my surface). I ran the arms rather than reading them:

ok   both online -> 0, and it counts them
ok   one offline -> 1, naming the dead runner
ok   an unrequired dead runner still fails
ok   a required runner missing -> 1, 'not registered'
ok   no runners registered -> 0
ok   API failure -> 2, explicitly
ok   a truncated list -> 2, not 'strixhalo-engine is not registered'
ok   exactly one page -> 2 (may be truncated), never a vacuous ok
ok   --list prints runners and still exits 0
check_ci_runners_test: all arms pass

Those are the right nine. The two that matter most are the ones a naive checker gets wrong: "a truncated list → 2", because a page-of-100 that happens to be exactly full is indistinguishable from "all runners are here" and a checker that treats it as a verdict can report a live runner as missing; and "no runners registered → 0", because the empty case must not be an outage. --list not changing the exit code is the third.

What I could NOT verify, and am not claiming: I tried to mutation-test the offline detection and my mutation was a no-op twice — the first sed pattern matched nothing, the second hit a comment line rather than the comparison. So the evidence above is "these arms pass and cover the named cases", not "the arms fail when the subject is wrong". Given how many green-but-toothless checks this fleet has shipped, that distinction is worth stating rather than rounding up.

One thing worth a reader's attention in scripts/weekly.sh:683-686: the row runs the checker and its comment says it "exits 0 when no runners are registered", which is the right default for a weekly on a box that may not have runners — but combined with API failure → 2 it means a network failure during a release will surface as exit 2, not as a red row. That is the correct choice (an unreadable list is not a verdict) as long as the release step treats 2 as "say so and continue" rather than "fail the release" — which the wiring's own comment implies. I did not read enough of weekly.sh to confirm which it does, so: confirm it, and if 2 blocks the release, that is a decision about a release failing on a GitHub API blip.

[bong-water-water-bong]

The one line worth adding, since the review's smallest actionable item is easy to lose: the vacuity condition of the "no runners -> exit 0" rule.

The PR already edits docs/ci-runners.md, but nothing in the added text records why that rule is safe here. It is safe because the fleet is registered at repo
level — total_count: 2, both runners visible to the repo-scoped API call — so "no runners" cannot be an artefact of the query. The moment the runners move into an
organisation group, a repo-scoped listing can legitimately return zero while two runners are online, and --require would then pass vacuously: the check would
be green because it looked in the wrong place, which is the failure mode a runner check exists to prevent.

Two sentences in the doc section cover it, and both are measured rather than theoretical:

1. the rule's justification — repo-level registration is what makes "no runners" a real answer (total_count: 2 today);
2. the condition under which it stops being one — an org group would need the org endpoint, which 403 admin:org from this box, so it could not be detected from
   inside a lane.

That second half is the part I would not want lost: a future move to org-level runners would not break anything visibly, and the check would keep reporting success
while guarding nothing — the exact shape of the silently-skipped leg that #1648 was about.

Everything else in the review stands: run live (--require strixhalo-engine strixhalo-bench -> rc 0, --require strixhalo-nonexistent -> rc 1 with the ::error::
line, --list distinguishing busy=true from an outage), which is the standard I would have applied.

[bong-water-water-bong]

VERDICT: READY at 064fab09 — I ran the suite and broke the script to check the arms bite, because a monitoring row's whole value is that it fails when it should.

Ran: bash tests/check_ci_runners_test.sh → all arms pass, including no runners registered -> 0, API failure -> 2, explicitly, a truncated list -> 2, not 'strixhalo-engine is not registered', exactly one page -> 2 (may be truncated), never a vacuous ok.

Negative control: replacing the offline loop's [ "$status" = online ] && continue with an unconditional continue makes the suite fail with FAIL an unrequired dead runner still fails: expected rc=1, got rc=0. So the arms are load-bearing rather than decorative. (It took me three attempts to aim this control — the first two patched strings that did not exist and the suite "passed" because the script was unchanged. Worth stating: a control that does not apply is indistinguishable from a control that passes.)

What makes this row trustworthy is what it does when it cannot check, and that is the part most monitors get wrong:

• no gh or a failing API → exit 2 with ::error::, not 0 and not 1 — "an unchecked runner is exactly the state this row exists to end";
• a truncated list (one page of 100) → exit 2, explicitly not "this runner is not registered". That is the 3 a.m. false page avoided by design, and the comment says so in those words — the person who writes that has been woken up by the other version;
• no self-hosted runners at all → 0, because a fresh box or a fork is not an outage. The distinction between "nothing to check" and "something is wrong" is the whole game here, and it is drawn correctly in both directions.

The --require semantics are the subtle fix. "A deleted runner silently satisfies an 'every runner is online' reading" — so required names must exist and be online, and the test's arm 3 asserts the message says gone rather than online. Without that, the row would go green exactly in the outage it was built for: the runner is removed, so the loop over registered runners has nothing to complain about. Arm 2b (an unrequired-but-offline runner still fails) closes the mirror-image hole.

The message quality is asserted, not just the exit code: the dead runner is named exactly once and the healthy one is not blamed (grep -q 'runner strixhalo-engine is' must be false). That is the difference between an alert someone acts on and one they learn to ignore.

Honest about its own placement: it is not a ctest (it needs network and a token), so it runs in weekly.sh's preflight and the docs say so. A test that silently cannot run in CI, filed as one that does, is how coverage claims rot; this one states its constraint at the top of the file.

Not verified: the live API path against the real repo (no token use for a review, and 14 GB under the 20 GiB floor for anything heavier). The stub-based arms cover the decision logic, which is where the bugs would be; the one thing they cannot cover is gh api's real pagination shape, and the truncated-list arm is precisely the guard for getting that wrong.

[bong-water-water-bong]

VERDICT: READY at 064fab09d4. I ran the test and the live check rather than reading the claims, and both hold.

What I ran

$ tests/check_ci_runners_test.sh
ok   both online -> 0, and it counts them
ok   one offline -> 1, naming the dead runner
ok   an unrequired dead runner still fails
ok   a required runner missing -> 1, 'not registered'
ok   no runners registered -> 0
ok   API failure -> 2, explicitly
ok   a truncated list -> 2, not 'strixhalo-engine is not registered'
ok   exactly one page -> 2 (may be truncated), never a vacuous ok
ok   --list prints runners and still exits 0
check_ci_runners_test: all arms pass

$ scripts/check-ci-runners.sh --list
strixhalo-bench        online   busy=false
strixhalo-engine       online   busy=false
ok   2 self-hosted runner(s) online                                   rc=0
$ scripts/check-ci-runners.sh --require strixhalo-engine strixhalo-bench
ok   2 self-hosted runner(s) online, including: strixhalo-engine strixhalo-bench   rc=0

So the "verified against the live fleet" line reproduces, and the two arms that matter most are the truncation ones: a truncated list is 2, not "not registered" — a monitor that cannot tell whether it is seeing page one of two has to say so, and this one does. Same for "exactly one page", which is the honest refusal to print a vacuous ok.

I also checked the wiring rather than the prose: preflight() accumulates bad=1 and ends with return $bad, and the caller is preflight || { say "preflight failed: nothing was run"; exit 1; } — so a dead runner really does stop the release before stage one, with WEEKLY_SKIP_RUNNER_CHECK=1 as the documented way through. And the health_rc=$? in the ci-runner-health.sh else branch is the condition's status, not a clobbered one (it is the first statement there), so exit 2 stays "no units on this host, not a failure".

One correction to the body, and it undersells the PR

The body says "seven arms"; the test has nine, and the two it does not mention are a truncated list -> 2 and exactly one page -> 2 — the best two in the file, because both are refusals to report a false outage. Worth naming them there: they are the difference between a monitor you can trust during an outage and one that pages you with the wrong answer.

Nothing else to change: the script carries the full notice, --require uses the "${require[@]}" form that survives set -u with an empty array (the shellcheck bug the body records, and the test asserts the count), and the honest "it cannot be a ctest here, and a scheduled workflow is an RFC" paragraph is the right way to leave the remaining gap visible.

[github-actions]

Persistent review updated to latest commit c8518a3

[bong-water-water-bong]

VERDICT: CHANGES at c8518a3

Re-verdict for c8518a3, a merge of main into the branch, and it was clean. The script is unchanged, and so is my blocking point from c28f845b and 064fab09: scripts/check-ci-runners.sh still returns "ok no self-hosted runners registered" with rc 0 on an empty list before it looks at --require, and arm 4 still pins that (ok no runners registered -> 0, run with --require strixhalo-engine strixhalo-bench). With both registrations gone and the units still up, the preflight is green, which is the outage this row exists for. The fix is the same: return 0 for an empty list only without --require, and split arm 4 into "empty, no --require: 0" and "empty, --require both: 1, naming both".

[bong-water-water-bong]

VERDICT: CHANGES REQUESTED at c8518a33 — the row works (it just caught a real outage while I was reviewing it), and it stops one layer short of the class it exists for: it checks that runners are online, not that any runner still answers the labels the workflows route on.

First, the live evidence: this catches the outage it was written for

I ran the head's script against the real repository:

$ bash scripts/check-ci-runners.sh --repo 1bit-MONSTER/engine
::error::runner strixhalo-engine is offline (busy=false): every check that needs this box now reads as pending, not red
$ echo $?  →  1

and the API says why it matters:

strixhalo-bench   online    labels=self-hosted,Linux,X64,strixhalo,bench
strixhalo-engine  offline   labels=self-hosted,Linux,X64,pr-agent,strixhalo

hw-gate.yml routes runs-on: [self-hosted, strixhalo, pr-agent] and issue-agent.yml [self-hosted, pr-agent], so with
strixhalo-engine offline those jobs queue for ever — which is exactly the hw_gate / pr_agent checks I have been reading as pending
(never red) on the PRs I am reviewing today. The row is not hypothetical: it reports the state the fleet is in right now, and it exits 1
rather than 0.

The gap: online is not routable

The check asserts (a) every registered runner is online, (b) --require NAME exists and is online. Neither looks at labels. So this
passes:

strixhalo-engine  online  labels=self-hosted,Linux,X64        ← re-registered without `pr-agent`

and every job that says runs-on: [self-hosted, strixhalo, pr-agent] queues for ever, with the same signature the row exists to end: pending,
never red. A re-registration (the thing scripts/install-ci-runner.sh does, and the thing a restart policy encourages) is precisely when the
labels can change, so the outage this PR is about and the label outage are adjacent enough that one row should cover both.

Minimal shape, in the script's own idiom: --require NAME[:label,label], and for each required name check that its labels field contains
every label listed (the API already returns them, and the parse loop is one more \t-separated column). Then the weekly preflight names the
routing the workflows actually use:

check-ci-runners.sh --require strixhalo-engine:pr-agent --require strixhalo-bench:bench

That also upgrades rule (b) from "the runner exists" to "the runner exists and the scheduler can route to it", which is the claim the header
makes.

What the script gets right, and I checked

• Exit 2 for "cannot tell", exit 1 for "down", exit 0 for "no self-hosted runners at all" — three states, and the middle one is what a
  monitor must never collapse into a pass. The truncation rule (total > len or len >= 100) errs toward "cannot tell", which is the
  right direction for a pager.
• bad=$((bad+1)) inside while … done < <(printf …) — process substitution, so the loop is in the current shell and the counter
  survives; the classic printf | while subshell bug is not present.
• Required names are skipped in the "every runner online" pass and re-checked in their own pass, so a required runner that is missing
  produces the deleted message ("the runner that takes these jobs is gone, so they queue for ever") rather than a generic offline one.
• The messages name the consequence, not the state, which is why the one line above reads as a diagnosis.

What I did not run

tests/check_ci_runners_test.sh (it drives the script against fixtures; I did not need it because I ran the script itself, which is stronger
evidence for the "does it notice" question that decides this PR). No ticket of any kind was taken — the whole check is gh api and bash.