shell: add shellcheck CI guard and drive the tree to zero findings #19
Merged
admin_emi
merged 2 commits from 2026-07-23 22:26:10 +00:00
shell-lint into main
2 commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
|
|
56be0dd9b7 |
render-secrets: drop the RETURN trap, it broke deploy silently
All checks were successful
PR build (required check) / changes (pull_request) Successful in 9s
shell-lint / shellcheck (pull_request) Successful in 8s
secrets-guard / encrypted (pull_request) Successful in 9s
PR build (required check) / build-frontend (pull_request) Has been skipped
PR build (required check) / validate-observability (pull_request) Successful in 16s
PR build (required check) / build-backend (pull_request) Successful in 53s
PR build (required check) / gate (pull_request) Successful in 3s
The RETURN trap added in the previous commit leaked out of the function and killed every deploy on a SOPS-configured host. A RETURN trap set inside a SOURCED function is not function-scoped: it persists in the caller's shell after the function returns, and a RETURN trap also fires when a `.`/source completes. deploy.sh sources /etc/thermograph.env six lines after calling render_thermograph_secrets, which re-fired the trap at top level where `tmp` -- function-local -- is unset. Under deploy.sh's `set -u` that is fatal, and silent: that line already sends stderr to /dev/null, so the deploy rendered secrets and then died with no diagnostic before pulling or rolling anything. Replaced with explicit `rm -f "$tmp"` on each exit path, plus a comment recording why the tidier-looking trap is wrong here so it doesn't come back. The original defect the trap was meant to fix stays fixed: the decrypt-failure path removes the plaintext temp file before returning 1. The write section now captures its status in `rc` and cleans up once, rather than ending on `rm` -- as the last command it was masking a failed in-place `cat` write to status 0, so a half-written /etc/thermograph.env would have deployed as if it succeeded. Verified in a container against the real call pattern (strict-mode caller, source lib, call, then source the rendered env): success path returns 0 and the caller survives the subsequent source; decrypt-failure path aborts the caller with no /etc/thermograph.env written; both leave zero temp files. |
||
|
|
40a3ce21d9 |
shell: add shellcheck CI guard and drive the tree to zero findings
All checks were successful
secrets-guard / encrypted (pull_request) Successful in 9s
PR build (required check) / changes (pull_request) Successful in 17s
shell-lint / shellcheck (pull_request) Successful in 13s
PR build (required check) / validate-observability (pull_request) Has been skipped
PR build (required check) / build-frontend (pull_request) Successful in 1m27s
PR build (required check) / build-backend (pull_request) Successful in 1m36s
PR build (required check) / gate (pull_request) Successful in 3s
No static analysis has ever run over the ~2k lines of shell that deploy, provision secrets, and bootstrap hosts as root over SSH. Add shell-lint.yml (pinned shellcheck v0.11.0 + sha256, -x, default severity, fail on any finding) and fix everything it reports, plus two defects it structurally cannot see. Not path-filtered, matching secrets-guard's call: a backstop that only runs when you expect it to isn't a backstop. Scripts are discovered with find, so new ones are covered on landing. The version is pinned to a static release rather than apt's, so a drifted shellcheck can't fail CI on an unrelated push. render-secrets.sh: the mktemp holding DECRYPTED vault contents was only removed on the success path and one failure branch, so a sops decrypt failure left plaintext POSTGRES_PASSWORD in /tmp indefinitely on a live host. A RETURN trap makes removal unconditional, and the two sops calls now `|| return 1` explicitly instead of relying on the caller's set -e (a bare set -e abort skips the trap). The function stays free of `set -e` itself -- it is sourced, and shell options would leak into the caller. autoscale.sh: ran `set -eu` without pipefail while piping docker stats into awk, so a failed left side was swallowed and the loop scaled on empty input. Promoted to pipefail with avg_cpu's failure treated as a missed sample, so a daemon hiccup can't kill the autoscaler. Verified busybox ash in docker:27-cli supports pipefail and the script still parses there. capture-fixtures.sh: `jq . || cat` ran cat after jq had already consumed stdin, silently writing a truncated fixture; now a real if/else that fails loudly. deploy.sh/deploy-stack.sh: `# shellcheck source=` paths corrected for the monorepo layout, and /etc/thermograph.env marked unfollowable (it is rendered at deploy time and cannot exist at lint time). |