ci: gate workflows on actionlint+shellcheck, plus self-test (BLO-29596)

[allyblockcast]

Summary

Wires actionlint + shellcheck into CI as a gating job, and clears the 18 pre-existing findings so it can land green.

BLO-29523 was a bash -c 'cmd '$list in github-ci.yml: the serial rerun handed pytest only the first failing file and turned the rest into positional parameters of the -c script, so three real failures were certified by one green check. shellcheck sees that shape. Nothing in CI was running shellcheck — and .github/actionlint.yaml has been tracked since 7a91d45c with no job ever consuming it. That unconsumed config was the actual gap.

The 18 findings: 10 fixed, 8 suppressed with a reason, 0 baselined

Measured at the parent commit with actionlint 1.7.7 + shellcheck 0.10.0 — all 18 are rule shellcheck, highest severity warning:

file	fixed	suppressed
github-ci.yml	8	1
behind-base.yml	1	—
gtm-image-build.yml	1	1
nbg6817-openwrt-ipk.yml	—	6

The two substantive fixes:

• ADD_DOCKER_ENV becomes an array (3× SC2086). It was deliberately left unquoted so "-e X=1" would word-split into two docker arguments — the same scalar-vs-array construct BLO-29523 died of, one layer down. An array delivers those two arguments without re-splitting or globbing, and an empty array still expands to no arguments, which a quoted scalar could not do ("" would hand docker a stray empty argument). I checked the argv at all three call sites is byte-identical, in both the empty and the set case, before and after.
• The two apt retry loops become if cmd; then break; fi (2× SC2015, SC2034, 2× SC2086). The old cmd && break || sleep 10 needed its trailing || sleep 10 to stop set -e killing the step on the first failed attempt — so the retry and the set -e suppression were the same token. Both the retry-then-succeed and all-attempts-fail paths were replayed.

The 8 suppressions are each a genuine false positive, co-located with its reason: the bash -lc body whose $ must be expanded by the shell inside the container; IMAGE, a workflow-level env: shellcheck cannot see from one run: block; and six literal-markdown echoes, one carrying a \n that is an argument to a remote tr. Each disable was confirmed load-bearing by deleting it and re-running — 1+1+5+1 = 8, so none is dead config.

No severity floor is needed — measured, not assumed

The issue anticipated needing a -shellcheck severity floor because SC2086 is only info. It isn't needed: actionlint reports every shellcheck finding as its own error and exits 1 regardless of shellcheck's severity. The 18 findings above contained zero error-severity findings and still exited 1; so does the BLO-29523 shape, whose SC2128 is warning and SC2086 info. Adding a floor would be machinery for a gap that does not exist.

The self-test is the load-bearing part

actionlint with no shellcheck on PATH silently skips every SC* rule and exits 0. Measured on this tree: with shellcheck removed, the bug shape lints clean and the lint step goes green. So a green lint step is ambiguous on its own.

The Self-test step resolves it on every run, with two arms:

1. the BLO-29523 shape must be rejected, with SC2128 — not merely non-zero, since a crash or a bad fixture is also non-zero and would prove nothing about shellcheck;
2. the fixed shape must be accepted — without this, a checker that rejected everything would pass arm 1 and be reported healthy.

All four guards were mutation-tested one at a time:

mutation	result
baseline, guards intact	exit 0 ✅
shellcheck off PATH	exit 1 — arm-1 rc guard fires
arm-1 rc guard deleted, shellcheck off PATH	exit 1 — SC2128 grep fires independently
arm-2 fixture swapped for the bug shape	exit 1 — positive-control guard fires

Isolation

Own workflow on arc-light, not a step in github-ci.yml — the reason rerun-guard-selftest.yml already records: a check wired into the job it protects can abort that job before it reports anything, recreating the silent hole it exists to close. It also keeps a ~30 s linter off the ~2.5 h topotest critical path and out of that run's cancel-in-progress group.

Tools are pinned by version and sha256. actionlint's digest is the one published in actionlint_1.7.7_checksums.txt; shellcheck publishes no checksums file for v0.10.0, so its digest was observed at download and pinned — that stops a later substitution, it does not independently attest the bytes.

Also verified locally

check-runner-labels.py pass · its 4 fixtures OK · test_topotest_*.py 172 tests OK · test_verify_rerun_coverage.py 40 tests OK (the last two cover the github-ci.yml wiring this PR edits).

Mutation evidence — the gate is not vacuous (BLO-29596 verifying signal)

A gate that is green is not thereby a gate. Proven on this branch by pushing the
BLO-29523 bug shape at the real rerun call site in github-ci.yml (not in the
job's synthetic self-test fixture), then reverting it:

head	tree	actionlint run	result
c06276f2	fixed	38059664938	success
b007b0e1	bash -c 'cd ~/frr/tests/topotests ; sudo -E pytest '$rerun_files	38070394475	failure, exit 1
dadbffbd	revert — tree byte-identical to c06276f2	38083629329	success

The red run failed for exactly the right reason, not incidentally:

.github/workflows/github-ci.yml:1021:9: shellcheck reported issue in this script:
  SC2128:warning:340:55: Expanding an array without an index only gives the first element
  SC2086:info:340:55:    Double quote to prevent globbing and word splitting
##[error]Process completed with exit code 1.

SC2128 is the near-exact diagnosis of the BLO-29523 defect — "only gives the first
element" is the bug, restated. And it is a warning, SC2086 only an info: both
still exit 1, which is the measurement behind "no severity floor is needed" above.

Two corroborating reds at the mutation head, both expected: rerun-guard-selftest
(38070394496) and
review-gate-selftest
(38070394516) — the
pre-existing guard from #68 catches the same shape independently, so the two layers
agree rather than one covering for the other.

Reproduce in ~2 min

# from a frr checkout, with actionlint 1.7.7 + shellcheck 0.10.0 on PATH
actionlint -oneline | wc -l    # master => 18 ; this branch => 0
actionlint; echo $?            # this branch => 0

Related Issue

BLO-29596 — follow-up from Ally's review of #68. Closes the regression guard for BLO-29523.

Components

build (CI only — no daemon code touched)

[allyblockcast]

🔗 Paperclip issue: BLO-29596
🔗 Paperclip issue: BLO-29523

[allyblockcast]

@ally please review at head dadbffbd3990defc3dceee4e858346b2307ea8c5 — review/ally-complete is pending ("Waiting for Ally review of head dadbffb") and no review exists on either surface, so this is the first request on this PR, not a re-request.

Review focus, in priority order:

1. ADD_DOCKER_ENV scalar → array (github-ci.yml). This is the one behavioural change in the diff. The scalar was deliberately unquoted so -e X=1 word-split into two docker argv entries; the array must deliver the same argv in both the set and the empty case. I claim argv is byte-identical at all three call sites — please check that claim rather than the diff's shape, since an empty quoted scalar would have passed docker a stray empty argument and the array must not.
2. The two apt retry loops cmd && break || sleep 10 → if cmd; then break; fi. The trailing || sleep 10 was doing double duty as the set -e suppressor. Confirm the rewrite still survives a first-attempt failure under set -e, and still fails the step when all attempts fail.
3. The 8 # shellcheck disable= suppressions — each should be a genuine false positive with a co-located reason. I confirmed each is load-bearing by deleting it and re-running (1+1+5+1), but the justifications are the part worth a second opinion, particularly the bash -lc body whose $ must expand inside the container.
4. The self-test's two arms (actionlint.yml). Arm 1 requires the BLO-29523 shape be rejected with SC2128 specifically (not merely non-zero); arm 2 is the positive control that stops a reject-everything checker reading as healthy. Please sanity-check that pair actually closes the "actionlint without shellcheck on PATH exits 0 and skips every SC* rule" hole, which is the whole reason the job exists.

Not asking you to re-derive the gate's non-vacuity: that is already proven end-to-end on this branch by green c06276f2 → red b007b0e1 (SC2128 at the real rerun call site, exit 1) → green dadbffbd with the tree byte-identical to the first.

[allyblockcast]

@ally please review at head dadbffbd3990defc3dceee4e858346b2307ea8c5.

Re-request: the first request (2026-10-11T01:16Z) is 13h old with zero reviews on either surface — pulls/138/reviews is empty and no ## Ally comment-shaped review exists. Per policy that is past the ~10h measured request→response band, so this is a single re-request, not a timer.

Review focus:

• .github/workflows/actionlint.yml (new, +196) — the gating job. Specifically whether -shellcheck severity promotion actually makes the BLO-29523 bug shape (bash -c 'cmd '$list) fail rather than warn, and whether the ${{ / self-hosted-label carve-outs in .github/actionlint.yaml are narrow enough not to mute real findings.
• .github/workflows/github-ci.yml — the only semantic change is ADD_DOCKER_ENV scalar → array plus ${ADD_DOCKER_ENV[@]+"${ADD_DOCKER_ENV[@]}"} at three docker run sites. Measured empty on both 22.04 and 24.04 runners in this PR's run and in a same-day control run of another branch, so it expands to zero arguments either way — please sanity-check that reasoning rather than taking it from me.
• The job is deliberately isolated from the ~2.5h topotest job so a linter cannot abort what it protects.