Run shellcheck in CI

[cfzimmerman]

Inspired by oxidecomputer/maghemite#746

Follow up issue: #368

[cfzimmerman]

🤖 CLAUDE REVIEW

Reviewed the two commits this PR adds on top of cory/ci-tests (#350) — Add shellcheck job to CI and Apply shellcheck lints, i.e. .github/buildomat/{common,illumos,linux,packet-test-common}.sh and jobs/{image,linux,test,shellcheck}.sh. Nothing in swadm/ is in scope here.

I re-ran the gate locally (shopt -s globstar; shellcheck -x -o all .github/buildomat/**/*.sh) on shellcheck 0.9.0, the version Ubuntu 24.04 ships — clean, exit 0.

Checked the mechanical rewrites for behavior change and found them equivalent:

• common.sh — [ \uname -s` == SunOS ] && SERIES=illumos/|| SERIES=linux→if/else`. Same result, and the old AND-list form was already errexit-safe, so nothing changes for the sourcing jobs.
• linux.sh — shasum -a 256 "$1" | awk -F ' ' '{print $1}' → out=$(shasum -a 256 "$1"); echo "${out%% *}". %% strips from the first space, so identical output. Slightly better under errexit: a failing shasum now aborts instead of quietly yielding an empty digest through the pipeline.
• illumos.sh — quoting [[ "${SDE_CALC}" != "${SDE_PKG_SHA256}" ]] fixes a latent case: with the old unquoted [, an empty SDE_CALC made the test itself error out rather than reporting "bad checksum".
• image.sh — NAME="$(echo $FEATURE | sed s/tofino_//)" → ${FEATURE//tofino_/}. // replaces all occurrences vs. sed's first, but no feature name (tofino_stub, tofino_asic, softnpu) contains it twice.
• packet-test-common.sh — the SC2024 suppressions on the two sudo -E ... &> /work/*.log & lines are right: /work is chowned to the invoking user two lines above, so the unprivileged redirect is exactly what's wanted.

Two things I'd change, both in the new job. No correctness bugs in the lint pass itself.

---

.github/buildomat/jobs/shellcheck.sh:14 — -o all + unpinned shellcheck will redden unrelated PRs

-o all enables every optional check, including ones added in future shellcheck releases — upstream documents it as unstable for exactly this reason. Combined with sudo apt install -y shellcheck (no version pin), the gate's strictness is whatever the Ubuntu archive happens to serve on the day.

Concretely: 24.04 currently ships 0.9.0 with 9 optional checks (shellcheck --list-optional). 0.10.0 exists upstream and adds more. The first archive refresh that bumps the package turns this job red on whatever PR is open at the time, with findings unrelated to that PR's diff.

Two of those optional checks — require-variable-braces and require-double-brackets — are what drove most of the churn in this PR, so they're clearly wanted. I'd rather see the set named explicitly than inherited:

shellcheck -x \
    -o require-variable-braces,require-double-brackets,quote-safe-variables,... \
    .github/buildomat/**/*.sh

That way adopting a new check is a deliberate commit rather than a surprise, and it documents which conventions the repo has actually signed up for. If you'd rather keep -o all, pinning the version (apt install -y shellcheck=0.9.0-*) gets most of the way there.

.github/buildomat/jobs/shellcheck.sh:14 — tools/*.sh is unlinted

The glob covers .github/buildomat/ only. That leaves out run_dpd.sh, run_tofino_model.sh, veth_setup.sh, veth_teardown.sh and check_copyrights.sh — scripts the CI jobs invoke directly (packet-test-common.sh runs four of them) and that developers run by hand far more often than they touch a buildomat job. Those are where a quoting bug actually costs someone an afternoon.

Fine as a follow-up if you want this PR small, but worth saying out loud in the commit message that the coverage is deliberately partial, since -x already makes shellcheck read nothing outside that tree.

.github/buildomat/jobs/shellcheck.sh:7 — no errexit/pipefail/xtrace (nit)

Every other job in .github/buildomat/jobs/ sets all three right after the metadata block. This one sets none. The job still fails correctly when lints fail — shellcheck is the last command, so its status becomes the script's — but a failed sudo apt install -y shellcheck surfaces as a bare command not found on line 14 instead of failing at line 9 where the problem is.

Also the only ubuntu-24.04 target in the repo (everything else Linux is ubuntu-22.04). Deliberate, I assume, to get shellcheck 0.9.0 rather than 22.04's 0.8.0 — just flagging that it's now a second Ubuntu image to keep in mind.

.github/buildomat/packet-test-common.sh:18 — TESTNAME no longer word-splits

testname=(${TESTNAME:+"${TESTNAME}"})

then "${testname[@]}" at the cargo test call. The empty case is handled correctly, which is the point of the change — but for a non-empty value this yields exactly one element (verified: TESTNAME="a b" → count=1), whereas the previous unquoted $TESTNAME split on whitespace.

The Local Usage block documents TESTNAME='$name' as a single test name, so this is fine as intended. It does mean TESTNAME='--test nat', or any two-token value that used to work, now reaches cargo as one argv element and errors. Either drop the inner quotes (testname=(${TESTNAME})) if splitting was load-bearing, or add half a line to Local Usage noting it's one name only.