fix(ci): turn shellcheck on in workflow-lint and clear the 44 findings (#1130) - #1148
Conversation
#1130) #1122 shipped `workflow-lint` with `-shellcheck=`, checking only whether a workflow compiles, because shellcheck reported 44 pre-existing findings — almost all in deploy.yml, where quoting changes cannot be verified from a PR. The issue guessed the SC1072/SC1073 parse errors were a `${{ }}` false positive. They are not, and the real defect is worse than a style nit. deploy.yml's ssh blocks use an UNQUOTED heredoc delimiter, so the runner's shell expands the body — including lines that begin with `#`, which are heredoc data, not shell comments. Two prose comments contained backticks: # `pm2 restart <name>` silently drops newly-added .env vars # Delete by name (NOT `pm2 delete all`) to spare unrelated apps Both were executed as command substitutions on the GitHub runner, with the step's secrets in the environment. Reproduced standalone: $ cat << ENDSSH # `echo EXECUTED_ON_CLIENT` in a comment ENDSSH # EXECUTED_ON_CLIENT in a comment `pm2` is absent from the runner so nothing happened, and `<name>` failed as a redirection — which is exactly the SC1073 shellcheck was reporting. The comment also reached the server with its text silently rewritten. Triage of the rest: - SC2087 / SC2029 (4) — client-side expansion is deliberate: NEXT_PUBLIC_*, BACKEND_NAME/FRONTEND_NAME, ENV_BASE64 and REMOTE_PATH exist only on the runner, and the server-side forms are already escaped as \${...}. Quoting the delimiter would break the deploy. Disabled inline with that reason. - SC2086 (29) — quoted. Mostly `>> $GITHUB_STEP_SUMMARY`. - SC2129 (3) — the three summary blocks became grouped `{ ... } >> "$..."`. - SC2044 (1) — the hardcoded-URL check looped over `$(find ...)`, so any path containing a space split into two non-existent files and every grep below it silently missed. Now `while IFS= read -r` with process substitution, which also keeps ISSUES accumulating in the current shell. - SC2002 (1) — useless cat. - SC2006 (3) — the backticks above; gone with them. The job now runs `./actionlint` with shellcheck enabled, and first asserts the binary is on PATH: actionlint SKIPS every shell finding when it is missing, without warning, so the gate would otherwise go hollow the day the runner image drops it. That is not theoretical — it is why #1122's local verification was invalid.
|
Claude finished @frankbria's task in 5m 33s —— View job ✅ Precision bug review: no defects foundScoped to concrete defects only (logic errors, security, data loss, races, contract breaks, resource leaks). Style/coverage is covered by a separate review. Verdict: clean. No inline findings to post. What I verified against the checked-out files (not just the diff):
One non-blocking note (not a defect — out of my scope, mentioned only because it touches correctness confidence): the two new wiring tests assert only that the strings
|
Demo — the AC that CI cannot check, checked#1130's last acceptance criterion: "Verify a staging deploy still succeeds after any Ran a real staging deploy from this branch — run 31457587132, Post-deploy state: $ curl -o /dev/null -w "%{http_code}" https://dev.codeframe.sh/health → 200
$ curl -o /dev/null -w "%{http_code}" https://dev.codeframe.sh/ → 200
$ curl -o /dev/null -w "%{http_code}" https://dev.codeframe.sh/api/v2/settings/keys → 401 (auth enforcement intact)
$ ssh staging 'pm2 list'
codeframe-staging-backend online 2m 0 restarts
codeframe-staging-frontend online 2m 0 restartsZero restarts — the PM2 cold-start path (the part these quoting changes touch most) worked on the first attempt. The heredoc bug, caught in a production logI said in the PR body that the backticked comments were being executed on the runner. Here is the previous deploy of That is the runner attempting to run Everything else
Third-party review
Known limitation
|
1 participant
Closes #1130.
The headline finding is not a style nit
The issue guessed the
SC1072/SC1073parse errors atdeploy.yml:570were a${{ }}false positive. They are not, and what they were pointing at is a real defect.deploy.yml's ssh blocks use an unquoted heredoc delimiter (<< ENDSSH), so the runner's shell expands the body before sending it — including lines that start with#. Inside a heredoc those are data, not shell comments. Two prose comments contained backticks:Both were executed as command substitutions on the GitHub runner, with the step's secrets in the environment. Reproduced standalone before changing anything:
pm2is absent from the runner so nothing actually ran, and<name>failed as a redirection — which is precisely theSC1073shellcheck was reporting. The comment also reached the server with its text silently rewritten. Only shellcheck sees this class;actionlintalone never would.Triage of the remaining 43
SC2086>> $GITHUB_STEP_SUMMARYSC2087/SC2029NEXT_PUBLIC_*,BACKEND_NAME/FRONTEND_NAME,ENV_BASE64andREMOTE_PATHexist only on the runner and must be baked in; the forms that resolve server-side are already escaped\${...}. Quoting the delimiter would break the deploySC2129{ ... } >> "$GITHUB_STEP_SUMMARY"SC2006SC2044$(find ...), so any path with a space split into two non-existent files and everygrepbelow it silently missed. Nowwhile IFS= read -rwith process substitution, which also keepsISSUESaccumulating in the current shell rather than a subshellSC2002The gate now fails rather than going hollow
./actionlint -shellcheck=→./actionlint, plus a guard: actionlint skips every shell finding when shellcheck is absent from PATH, without warning. That is not theoretical — it is exactly why #1122's local verification was invalid on my machine. The step now asserts the binary exists and emits::error::if not.tests/test_workflow_lint_wiring_1122.pyflips accordingly: the suppression must be gone, and the missing-binary guard must be present.Verification
actionlint(with shellcheck on PATH): 0 findings, was 44deploy.ymlchange cannot be validated by CI aloneruff— reported in the commentsNote on scope
Every
deploy.ymlchange here is either quoting, a grouped redirect, or a comment. No control flow, no command, and no expansion site changed — the escaped/unescaped split that decides client-vs-server resolution is exactly as it was. That is the property the staging deploy re-run checks.