shellcheck is not run anywhere in CI. Verified against origin/main: all seven workflow files (android-ci.yml, ci.yml, pages.yml, relay-cert-renew.yml, relay-ci.yml, relay-deploy.yml, release.yml) contain zero references to it.
Meanwhile scripts/ holds seven shell scripts, five of which are CI-enforced gates:
scripts/check-apk-distribution.sh <- FOSS purity (#548)
scripts/check-no-manual-json.sh
scripts/check-no-production-runblocking.sh
scripts/check-no-sensitive-logging.sh <- secret-in-logs gate
scripts/check-zombie-screens.sh
scripts/production-source-dirs.sh <- shared helper, all gates depend on it
scripts/backfill-relay-secrets.sh
Every agent that has touched these ran shellcheck locally and reported it clean — but nothing enforces that, so the next change is one forgotten local run away from landing unlinted. These scripts decide whether a build ships, so a silent breakage in one is a gate that stops gating.
Honest scoping — this would not have caught the recent bugs
shellcheck would not have flagged #577 or #590. Those are logic defects (an unanchored grep -v filtering file:line:content instead of just the content), and the shell is entirely valid. So this is not a fix for that class and should not be sold as one.
What it does catch is the class these scripts are actually exposed to: unquoted expansions, [ ] vs [[ ]] pitfalls, masked exit codes in pipelines, read without -r, subshell variable loss. Given the gates use grep pipelines whose exit status is load-bearing (|| true appears deliberately in at least one), masked-exit-status warnings are directly relevant.
Suggested shape
Add a shellcheck step to the Lint job covering scripts/*.sh and scripts/tests/*.sh. #588 established the precedent for CI-run script checks by wiring scripts/tests/check-no-sensitive-logging-test.sh into Lint, so there is now a natural home for it.
Make it fail the job rather than warn — an advisory linter in CI is noise that gets ignored. If the existing scripts produce findings, fix them or add targeted # shellcheck disable= with a reason, rather than lowering severity globally.
Found while working #577; the local-only shellcheck caveat was flagged by that PR's author and independently confirmed here.
shellcheckis not run anywhere in CI. Verified againstorigin/main: all seven workflow files (android-ci.yml,ci.yml,pages.yml,relay-cert-renew.yml,relay-ci.yml,relay-deploy.yml,release.yml) contain zero references to it.Meanwhile
scripts/holds seven shell scripts, five of which are CI-enforced gates:Every agent that has touched these ran
shellchecklocally and reported it clean — but nothing enforces that, so the next change is one forgotten local run away from landing unlinted. These scripts decide whether a build ships, so a silent breakage in one is a gate that stops gating.Honest scoping — this would not have caught the recent bugs
shellcheckwould not have flagged #577 or #590. Those are logic defects (an unanchoredgrep -vfilteringfile:line:contentinstead of just the content), and the shell is entirely valid. So this is not a fix for that class and should not be sold as one.What it does catch is the class these scripts are actually exposed to: unquoted expansions,
[ ]vs[[ ]]pitfalls, masked exit codes in pipelines,readwithout-r, subshell variable loss. Given the gates usegreppipelines whose exit status is load-bearing (|| trueappears deliberately in at least one), masked-exit-status warnings are directly relevant.Suggested shape
Add a
shellcheckstep to the Lint job coveringscripts/*.shandscripts/tests/*.sh. #588 established the precedent for CI-run script checks by wiringscripts/tests/check-no-sensitive-logging-test.shinto Lint, so there is now a natural home for it.Make it fail the job rather than warn — an advisory linter in CI is noise that gets ignored. If the existing scripts produce findings, fix them or add targeted
# shellcheck disable=with a reason, rather than lowering severity globally.Found while working #577; the local-only
shellcheckcaveat was flagged by that PR's author and independently confirmed here.