Skip to content

ci(shell): gate every tracked shell script on bash -n + ShellCheck - #285

Merged
prakashrj merged 1 commit into
mainfrom
upstream-fix/shellcheck-gate
Sep 3, 2026
Merged

ci(shell): gate every tracked shell script on bash -n + ShellCheck#285
prakashrj merged 1 commit into
mainfrom
upstream-fix/shellcheck-gate

Conversation

@prakashrj

Copy link
Copy Markdown
Contributor

The hole

This template's release path is shell. ci/local-release-check.sh alone is ~470 lines of signing, packaging and re-signing — and nothing in CI ever executed any of it. The app (...) cells build the app; they never source bin/rename.sh or run the ship path.

So a PR that broke a shell script outright merged green, and failed later on somebody's release. There was no shellcheck and no bash -n anywhere in this repo.

The gate

ci/check-shell.sh, run on every PR:

  1. bash -n over every tracked *.sh — a script that cannot be parsed must never reach main.
  2. ShellCheck at warning — the tier above that: constructs that run but do the wrong thing.

Seven findings on the first run, all fixed in this PR:

code where why it matters
SC2164 ×2 check-app-icon.sh, check-identity.sh cd "$(dirname "$0")/.." with no || exit. In a guard this is the worst case — cd fails, the script keeps going in the wrong directory, and then passes files that are not the ones it exists to check
SC1087 ×3 rename.sh, test-rename.sh ×2 $var[[:space:]] parses as array indexing, not expand-then-literal-bracket
SC2046 ×1 local-release-check.sh unquoted command substitution. Rewritten to the ${ARR[@]+"${ARR[@]}"} idiom the two lines directly above it already use, so the "pass nothing" branch is zero arguments rather than one empty one
SC2088 ×1 refork-smoketest.sh false positive — a tilde in an error message. Suppressed inline with the reason rather than reworded

Two deliberate calls

Severity capped at warning. The 19 remaining info/style findings are printed every run for visibility but never fail. They are dominated by SC2012 (ls \| grep) and SC1091 (cannot follow source), neither a defect here — and a gate that cries wolf gets switched off.

ci/lib/ is covered, not exempted — via shellcheck -s bash. Those are sourced libraries with no shebang, and they are pinned byte-for-byte by ci/lib/SHA256SUMS across this template and every consumer. Adding a # shellcheck shell=bash directive would force a re-pin in every downstream repo for the sake of a comment. The command-line flag gets identical coverage and touches nothing.

Verification

Mutation-verified, because a check that cannot fail is not evidence:

mutation result
deliberate syntax break in a script fails
reintroduce the SC2164 cd bug fails, naming the file and code
remove shellcheck from PATH fails — never a silent skip

It also caught a defect in its own header on the first run: prose wrapped so a line began # shellcheck, which ShellCheck parses as a malformed directive (SC1073/SC1072). That is noted in the header now, since the next person writing a comment about shellcheck will hit it too.

Portable to bash 3.2 (mapfile avoided) so it behaves the same if it ever runs on a macOS runner. Sibling suites unaffected: parser 9/9, Sh.stream 10/10, build number 11/11, xcconfig 51 checks / 0 failures, demo-account 14/14.

Wired as pr.yml's shell scripts job (ubuntu, seconds, no paths filter — a shell script is reachable from almost any change here) and into ci/local-check.sh on the pre-push path. shellcheck added to the Brewfile.

This template's release path is shell — ci/local-release-check.sh alone
is ~470 lines of signing, packaging and re-signing — and nothing in CI
ever executed any of it. The app(...) cells build the app; they never
source bin/rename.sh or run the ship path. So a PR that broke a shell
script outright merged green and failed later, on someone's release.

Adds ci/check-shell.sh: bash -n over every tracked *.sh (a script that
cannot be parsed must never reach main), then ShellCheck at warning
severity for the tier above that — constructs that run but do the wrong
thing. Seven found on the first run, all fixed here:

- SC2164 x2 (check-app-icon.sh, check-identity.sh): `cd` with no
  `|| exit`. In a guard this is the worst case — cd fails, the script
  continues in the wrong directory, and passes files that are not the
  ones it exists to check.
- SC1087 x3 (rename.sh, test-rename.sh x2): `$var[[:space:]]` parses as
  array indexing rather than expand-then-literal-bracket.
- SC2046 x1 (local-release-check.sh): unquoted command substitution,
  rewritten to the ${ARR[@]+"${ARR[@]}"} idiom the two lines above it
  already use, so the "pass nothing" branch is zero arguments instead of
  one empty one.
- SC2088 x1 (refork-smoketest.sh): false positive, a tilde in an error
  message. Suppressed inline with the reason rather than reworded.

Severity capped at warning deliberately. info/style (19) are printed
every run for visibility but never fail — they are dominated by SC2012
and SC1091, neither a defect here, and a gate that cries wolf gets
switched off.

ci/lib/ is covered with `shellcheck -s bash` rather than exempted. Those
are sourced libraries with no shebang, pinned byte-for-byte by
ci/lib/SHA256SUMS across this template and every consumer, so adding a
shell directive to them would force a re-pin in every downstream repo
for a comment. The command-line flag gets the same coverage and touches
nothing.

Mutation-verified: a syntax break fails it, reintroducing the cd bug
fails it, and removing shellcheck from PATH fails it rather than
skipping. It also caught a defect in its own header on the first run —
prose wrapped so a line began `# shellcheck`, which ShellCheck reads as
a malformed directive.

Wired as pr.yml's `shell scripts` job (ubuntu, seconds, no paths filter)
and into ci/local-check.sh on the pre-push path. shellcheck added to the
Brewfile.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@prakashrj
prakashrj merged commit 0a47db1 into main Sep 3, 2026
12 checks passed
@prakashrj
prakashrj deleted the upstream-fix/shellcheck-gate branch September 3, 2026 07:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant