Jinemi/thermograph

Fork 0

shell: add shellcheck CI guard and drive the tree to zero findings #19

Merged

admin_emi merged 2 commits from shell-lint into main

2026-07-23 22:26:10 +00:00

admin_emi commented

2026-07-23 22:00:25 +00:00

Owner

Adds .forgejo/workflows/shell-lint.yml and fixes everything it reports.

~2k lines of shell deploy, provision secrets, and bootstrap hosts as root over
SSH, with no static analysis in front of them. This adds that guard and drives
the tree to zero findings so it starts green.

The guard

Pinned shellcheck v0.11.0 + sha256 from the official static release, not
apt-get install shellcheck. The distro version drifts with the job image,
and a drifted shellcheck grows new warnings that fail CI on an unrelated
push. Pin verified to match the koalaman/shellcheck:stable image the
zero-findings pass was run against.
Not path-filtered, same call as secrets-guard: a backstop that only
runs when you expect it to isn't a backstop, and 26 scripts is seconds.
Scripts discovered with find, not listed, so a new script is covered the
moment it lands.
-x (follow sources), default severity, no excludes. The tree is at zero, so
anything reported is a regression, not noise. Findings become ::error
annotations on the diff.

The defect shellcheck cannot see

render-secrets.sh — plaintext secrets could survive in /tmp. The
mktemp receiving decrypted vault contents was only removed on the success
path and one failure branch. If the sops -d failed, the caller's set -e
aborted the function and neither cleanup ran, leaving decrypted
POSTGRES_PASSWORD in /tmp indefinitely on a live host.

Fixed with explicit rm -f "$tmp" on every exit path, plus || return 1 on
both sops calls so a decrypt failure can never write a partial
/etc/thermograph.env regardless of the caller's shell options.

Worth recording why it is not a trap ... RETURN, which is the tidier-looking
fix and was the first attempt here (56be0dd reverts it): 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 — so deploy.sh sourcing /etc/thermograph.env six lines
later re-fired it at top level, where tmp (function-local) is unset. Fatal
under deploy.sh's set -u, and silent, because that line already sends
stderr to /dev/null. Every SOPS host would have rendered secrets and then died
with no diagnostic before pulling or rolling anything. The comment in the file
now records this so it doesn't get "tidied" back in.

The write section also now captures its status in rc and cleans up once. The
old trailing rm was the function's last command, so it masked a failed
in-place cat write to status 0 — a half-written /etc/thermograph.env would
have deployed as if it succeeded.

Shellcheck findings

autoscale.sh [manual] — set -eu without pipefail while piping
docker stats into awk, so a failure on the left of the pipe was swallowed
and the loop autoscaled on empty input. Promoted to pipefail, with avg_cpu
failure treated as a missed sample so a daemon hiccup can't kill the
autoscaler. Verified the busybox ash in docker:27-cli (the image that
actually runs this) supports pipefail and the script still parses there.
capture-fixtures.sh [SC2015] — jq . || cat ran cat after jq had
already consumed stdin, silently writing a truncated fixture. Now a real
if/else that fails loudly.
capture-fixtures.sh [SC2012] — ls | wc -l → find.
deploy.sh / deploy-stack.sh [SC1090/SC1091] — # shellcheck source=
paths corrected for the monorepo layout (they still pointed at the split-repo
paths); /etc/thermograph.env marked unfollowable, since it is rendered at
deploy time from the SOPS vault and cannot exist at lint time.
dry-run-render.sh [SC2024] — not a bug: sudo is needed only to read the
root-owned live env, and the redirect target is a caller-owned mktemp, so
the redirect running as the invoking user is correct (sudo tee would be
wrong). Suppressed with a directive and the reasoning.
smoke.sh [SC2034] — unused loop counter → _.

Base branch

Targets main rather than dev deliberately. dev is currently behind main
and its thermograph-stack.yml still carries the pre-cutover
/opt/thermograph/deploy/... bind-mount paths; promoting through it would
revert main's monorepo path fix (0ed7e89).

Verification

shellcheck -x over all 26 scripts: 0 findings.
render-secrets.sh exercised in a container against the real call pattern
(strict-mode caller → source lib → call → 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.
autoscale.sh syntax-checked under docker:27-cli's busybox ash.

Adds `.forgejo/workflows/shell-lint.yml` and fixes everything it reports. ~2k lines of shell deploy, provision secrets, and bootstrap hosts as root over SSH, with no static analysis in front of them. This adds that guard and drives the tree to zero findings so it starts green. ## The guard - Pinned shellcheck **v0.11.0 + sha256** from the official static release, not `apt-get install shellcheck`. The distro version drifts with the job image, and a drifted shellcheck grows new warnings that fail CI on an unrelated push. Pin verified to match the `koalaman/shellcheck:stable` image the zero-findings pass was run against. - **Not path-filtered**, same call as `secrets-guard`: a backstop that only runs when you expect it to isn't a backstop, and 26 scripts is seconds. - Scripts discovered with `find`, not listed, so a new script is covered the moment it lands. - `-x` (follow sources), default severity, no excludes. The tree is at zero, so anything reported is a regression, not noise. Findings become `::error` annotations on the diff. ## The defect shellcheck cannot see **`render-secrets.sh` — plaintext secrets could survive in `/tmp`.** The `mktemp` receiving *decrypted* vault contents was only removed on the success path and one failure branch. If the `sops -d` failed, the caller's `set -e` aborted the function and neither cleanup ran, leaving decrypted `POSTGRES_PASSWORD` in `/tmp` indefinitely on a live host. Fixed with explicit `rm -f "$tmp"` on every exit path, plus `|| return 1` on both `sops` calls so a decrypt failure can never write a partial `/etc/thermograph.env` regardless of the caller's shell options. Worth recording why it is *not* a `trap ... RETURN`, which is the tidier-looking fix and was the first attempt here (56be0dd reverts it): 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 — so `deploy.sh` sourcing `/etc/thermograph.env` six lines later re-fired it at top level, where `tmp` (function-local) is unset. Fatal under `deploy.sh`'s `set -u`, and **silent**, because that line already sends stderr to `/dev/null`. Every SOPS host would have rendered secrets and then died with no diagnostic before pulling or rolling anything. The comment in the file now records this so it doesn't get "tidied" back in. The write section also now captures its status in `rc` and cleans up once. The old trailing `rm` was the function's last command, so it masked a failed in-place `cat` write to status 0 — a half-written `/etc/thermograph.env` would have deployed as if it succeeded. ## Shellcheck findings - `autoscale.sh` [manual] — `set -eu` without `pipefail` while piping `docker stats` into `awk`, so a failure on the left of the pipe was swallowed and the loop autoscaled on empty input. Promoted to `pipefail`, with `avg_cpu` failure treated as a missed sample so a daemon hiccup can't kill the autoscaler. Verified the busybox ash in `docker:27-cli` (the image that actually runs this) supports `pipefail` and the script still parses there. - `capture-fixtures.sh` [SC2015] — `jq . || cat` ran `cat` after `jq` had already consumed stdin, silently writing a truncated fixture. Now a real if/else that fails loudly. - `capture-fixtures.sh` [SC2012] — `ls | wc -l` → `find`. - `deploy.sh` / `deploy-stack.sh` [SC1090/SC1091] — `# shellcheck source=` paths corrected for the monorepo layout (they still pointed at the split-repo paths); `/etc/thermograph.env` marked unfollowable, since it is rendered at deploy time from the SOPS vault and cannot exist at lint time. - `dry-run-render.sh` [SC2024] — not a bug: `sudo` is needed only to *read* the root-owned live env, and the redirect target is a caller-owned `mktemp`, so the redirect running as the invoking user is correct (`sudo tee` would be wrong). Suppressed with a directive and the reasoning. - `smoke.sh` [SC2034] — unused loop counter → `_`. ## Base branch Targets `main` rather than `dev` deliberately. `dev` is currently behind `main` and its `thermograph-stack.yml` still carries the pre-cutover `/opt/thermograph/deploy/...` bind-mount paths; promoting through it would revert `main`'s monorepo path fix (0ed7e89). ## Verification - `shellcheck -x` over all 26 scripts: **0 findings**. - `render-secrets.sh` exercised in a container against the real call pattern (strict-mode caller → source lib → call → 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. - `autoscale.sh` syntax-checked under `docker:27-cli`'s busybox ash.

admin_emi added 1 commit 2026-07-23 22:00:28 +00:00

shell: add shellcheck CI guard and drive the tree to zero findings

secrets-guard / encrypted (pull_request) Successful in 9s

Details

PR build (required check) / changes (pull_request) Successful in 17s

Details

shell-lint / shellcheck (pull_request) Successful in 13s

Details

PR build (required check) / validate-observability (pull_request) Has been skipped

Details

PR build (required check) / build-frontend (pull_request) Successful in 1m27s

Details

PR build (required check) / build-backend (pull_request) Successful in 1m36s

Details

PR build (required check) / gate (pull_request) Successful in 3s

Details

40a3ce21d9

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).

admin_emi added 1 commit 2026-07-23 22:03:24 +00:00

render-secrets: drop the RETURN trap, it broke deploy silently

PR build (required check) / changes (pull_request) Successful in 9s

Details

shell-lint / shellcheck (pull_request) Successful in 8s

Details

secrets-guard / encrypted (pull_request) Successful in 9s

Details

PR build (required check) / build-frontend (pull_request) Has been skipped

Details

PR build (required check) / validate-observability (pull_request) Successful in 16s

Details

PR build (required check) / build-backend (pull_request) Successful in 53s

Details

PR build (required check) / gate (pull_request) Successful in 3s

Details

56be0dd9b7

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.