Jinemi/thermograph

Fork 0

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

Merged

admin_emi merged 3 commits from shell-lint into dev

2026-07-24 19:39:41 +00:00

admin_emi commented

2026-07-24 19:32:51 +00:00

Owner

A RETURN trap added in an earlier commit leaked out of render_thermograph_secrets and killed every deploy on a SOPS-configured host — a RETURN trap set inside a sourced function isn't function-scoped, and also fires when the ./source itself completes. deploy.sh sources /etc/thermograph.env a few lines after calling the function, re-firing the trap at top level where tmp (function-local) is unset — fatal under set -u, and silent, since that line's stderr already goes to /dev/null.

Replaces the trap with explicit rm -f "$tmp" on each exit path, and captures the write section's exit status in rc instead of ending on rm (which was masking a failed in-place write as success).

No PR existed for this branch yet — opening one now since it's a real, already-tested bugfix with no dependencies on other in-flight work.

A RETURN trap added in an earlier commit leaked out of `render_thermograph_secrets` and killed every deploy on a SOPS-configured host — a RETURN trap set inside a sourced function isn't function-scoped, and also fires when the `.`/source itself completes. `deploy.sh` sources `/etc/thermograph.env` a few lines after calling the function, re-firing the trap at top level where `tmp` (function-local) is unset — fatal under `set -u`, and silent, since that line's stderr already goes to `/dev/null`. Replaces the trap with explicit `rm -f "$tmp"` on each exit path, and captures the write section's exit status in `rc` instead of ending on `rm` (which was masking a failed in-place write as success). No PR existed for this branch yet — opening one now since it's a real, already-tested bugfix with no dependencies on other in-flight work.

admin_emi added 2 commits 2026-07-24 19:32:52 +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).

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.

admin_emi added 1 commit 2026-07-24 19:38:44 +00:00

Merge remote-tracking branch 'origin/dev' into shell-lint-merged

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

Details

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

Details

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

Details

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

Details

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

Details