chore: add shellcheck gate to CI (#132)

[garretpremo]

Closes #132

Summary

.github/workflows/ci.yml ran eslint (bun run lint, scoped to src/ tests/ bin/) and bun test. The 37 shell scripts under scripts/ and .claude/skills/*/scripts/ had no static checking at all. This adds a shellcheck job that gates them.

The configuration was chosen by measuring shellcheck v0.10.0 against the actual tree rather than by adopting the issue's suggested defaults — and that measurement contradicted the issue's stated rationale. See "On the issue's premise" below; it matters for how the workflow comment is worded, and for anyone who later wonders why --enable=all was declined.

What changes

New shellcheck job

Runs on ubuntu-latest only (shell linting is OS-independent — it is not added to the Windows matrix). Installs shellcheck pinned to v0.10.0 from the official koalaman release with sha256 verification, rather than using a third-party action or the runner image's preinstalled binary:

• the repo's CI uses only actions/checkout and oven-sh/setup-bun; this keeps that property
• pinning stops a runner-image bump from silently changing what the gate enforces

Each glob group is asserted non-empty independently, and the resolved counts are logged:

shopt -s nullglob globstar

script_files=(scripts/**/*.sh)
if [ "${#script_files[@]}" -eq 0 ]; then
  echo "::error::scripts/**/*.sh matched no files"
  exit 1
fi

skill_files=(.claude/skills/**/scripts/**/*.sh)
if [ "${#skill_files[@]}" -eq 0 ]; then
  echo "::error::.claude/skills/**/scripts/**/*.sh matched no files"
  exit 1
fi

echo "shellcheck: ${#script_files[@]} in scripts/, ${#skill_files[@]} in .claude/skills/"

shellcheck --severity=warning "${script_files[@]}" "${skill_files[@]}"

The per-group check is load-bearing. nullglob makes an unmatched glob expand to nothing, so a single merged non-empty check would keep the job green while silently checking only 11 of 37 files if .claude/skills were ever renamed or the skill scripts moved. Guarding each group separately, and naming the offending glob in the error, is what makes a path rename fail loudly.

Both globs are recursive, so a future scripts/lib/foo.sh or .claude/skills/foo/scripts/lib/helper.sh is picked up rather than silently skipped. The count echo exists because shellcheck is silent when clean: without it the log carries no record of what was checked, and a partial-coverage regression that still leaves both groups non-empty — one skill's scripts/ dir renamed while others keep theirs — would be invisible. The guards cannot catch that case; the logged count is the mitigation.

Extended pull_request paths filter

Added scripts/** and .claude/skills/**/scripts/**. Without this the new job would never run on a PR that only touches shell scripts — precisely the PRs it exists to gate. The filter is shared across jobs, so shell-only PRs now also run lint and unit-tests; that's cheaper than a second workflow file and those jobs are fast. (push: to main/dev carries no paths: filter, so branch heads are always gated regardless.)

scripts/ship.sh — two SC2034 fixes

Two unused loop counters (for attempt in $(seq 1 N), at lines 43 and 105) renamed to _. Behaviour-preserving: iteration counts (5 and 30) and sleep values are unchanged, and ship.sh never reads bash's automatic $_.

Worth noting for anyone re-running the numbers: shellcheck dedups SC2034 by variable name per file, so a first pass reports only one of these. The second surfaces only once the first is fixed. Fixed, not suppressed — no # shellcheck disable= was added anywhere.

On the issue's premise

The issue proposes --enable=all on the grounds that SC2310/SC2311 catch the #127 class of bug. Measured against this tree, they do not:

• SC2310 and SC2311 fire zero times across all 37 scripts, even at --enable=all --severity=style.
• Reconstructing the actual pre-fix label-merged-issues.sh pipeline from b6c48da — the grep … | grep … | sort -un under set -euo pipefail with the now-dead if [ -z "$issues" ] branch below it — shellcheck exits 0 at --enable=all --severity=info. The only output at style is SC2250 (brace preference) and SC2292 (prefer [[), unrelated cosmetics.
• The second fix: ignore closing refs inside code when labeling issues (#129, #127) #130 bug (blanket || true on a multi-stage pipeline) is likewise undetected.

So neither of the two bugs cited in the issue would have been caught by shellcheck at any severity or check set. The gate is still worth having — it covers real defect classes on future scripts, and scripts/ currently has zero static checking — but not on the stated justification. The workflow comment records the measured rationale rather than the SC2310/SC2311 one, so it doesn't hand a future reader a false claim.

Measurements

All figures from the identical 37-file invocation the CI job uses. Per-file arithmetic does not reconcile these — SC2034 dedups by variable name per file, and SC1091 resolution shifts with how many files are passed in one invocation — so both trees were measured whole.

setting	pre-change (e5673e5)	post-change
default checks, --severity=error	0	0
default checks, --severity=warning	1	0
default checks, --severity=style	6	5
--enable=all --severity=info	38	37
--enable=all --severity=style	615	614

error alone gates on nothing, which is why warning is the floor. The 5 remaining at style are 3×SC1091, 1×SC2016, 1×SC2317 — none of them real defects.

Acceptance criteria

• CI fails on a shellcheck violation in scripts/ or .claude/skills/*/scripts/
• The job actually runs on PRs that touch only shell scripts (paths: filter extended)
• A renamed/moved script directory fails the job loudly instead of silently checking fewer files
• Existing scripts pass at the chosen severity, fixed rather than suppressed
• The severity level and --enable set are recorded in the workflow with a comment explaining the choice, and that comment is accurate against the real measurements

Test plan

• bun test — 1018 pass / 0 fail
• bun run lint — 0 errors (118 pre-existing warnings)
• shellcheck --severity=warning over the real tree — 37 files (11 + 26), exit 0
• Canary in a scripts/*.sh file → exit 1; reverted → exit 0
• Canary in a .claude/skills/*/scripts/*.sh file → exit 1; reverted → exit 0
• Partial-glob simulation: each directory renamed in turn → exit 1 with the offending glob named (this case silently passed before the per-group check)
• globstar widening check: a symlinked directory under scripts/ containing a live SC2034 is not matched (bash ** does not follow symlinked dirs); no duplicate expansions, 37 unique paths
• Pinned sha256 verified against the real tarball; sha256sum -c - exits 1 on mismatch and set -euo pipefail propagates it
• .github/workflows/ci.yml parses as YAML

Follow-up

#144 tracks the two shell scripts outside the gate's globs (.claude-jobs/_internal-authors.sh, tests/e2e-tutorial/run.sh) — out of scope for #132, to be either folded in or documented as a deliberate exclusion. .claude-jobs/** is likewise absent from the paths: filter, which belongs in that issue's scope.

[garretpremo]

🤖 Automated review by claude — generated by the review-issue skill. Treat as advisory; a human still owns the merge decision.

All three acceptance criteria from #132 are met, plus the two the PR added for itself. The gate is real (verified in the job log: checksum OK, shellcheck ran, exit 0 over 11 + 26 = 37 files), the ship.sh fixes are behaviour-preserving, and the workflow comment records the measured rationale rather than the issue's unverified SC2310/SC2311 premise. No blocking issues.

Verification performed:

• Base is dev; all five checks pass (lint, shellcheck, unit-tests on both ubuntu and windows).
• Confirmed the glob coverage independently against the branch tree: scripts/ = 11, .claude/skills/*/scripts/ = 26, total 37. The two .sh files outside those globs are .claude-jobs/_internal-authors.sh and tests/e2e-tutorial/run.sh, matching the triage note.
• bash -n on the post-change ship.sh passes; grep confirms no read of $_ anywhere in the file, so shadowing it as a loop variable is inert.
• Reproduced the local x="$1" _ + for _ in $(seq 1 5) idiom in isolation: 5 iterations, positional arg intact, global $_ correctly restored after return. The 5 and 30 iteration counts are unchanged from the original.
• Read the job log to confirm the install step verified the pinned sha256 (/home/runner/work/_temp/shellcheck.tar.xz: OK) rather than falling through, and that the lint step was the last command in the run block, so its exit code gates the job.

The per-group emptiness guard is the right call and the reasoning in the PR body holds: with nullglob, a single merged check would stay green while checking a third of the files.

Non-blocking observations

• Glob asymmetry between the two groups. scripts/**/*.sh is recursive, but .claude/skills/**/scripts/*.sh is not — a future .claude/skills/foo/scripts/lib/helper.sh is silently skipped, and the non-empty guard won't catch it because the group still has 26 other files. This matches the glob the issue stated, so it isn't a criteria miss, but it's the same "silently checks fewer files" failure mode the per-group guard was added to prevent. .claude/skills/**/scripts/**/*.sh would close it at no cost.

• The job prints nothing on success. shellcheck is silent when clean, so the log gives no record of what was checked — I had to reconstruct the file count from the tree rather than read it off the run. An echo "shellcheck: ${#script_files[@]} in scripts/, ${#skill_files[@]} in .claude/skills/" before the invocation would make the log self-auditing and would surface a partial-coverage regression that still leaves both groups non-empty (the case the guards can't see).

• Two scripts stay permanently unchecked. .claude-jobs/_internal-authors.sh and tests/e2e-tutorial/run.sh are excluded deliberately and consistently with the issue's scope. Worth a tracking issue if the intent is to fold them in later, since nothing in the tree currently records that they're outside the gate on purpose.

• style (5) in the workflow comment is a pre-fix number. Severity is a floor, so the 5 style findings included the 2 SC2034 that this PR fixes; the post-merge figure is 3. Everything else in that comment is accurate against the current tree — this is the one number that drifted by the PR's own change.

Nitpicks

• local pr="$1" _ shadows bash's "last argument" special variable. Verified harmless here, but for ((i = 0; i < 5; i++)) silences SC2034 without overlapping a special variable at all, and doesn't need the reader to confirm $_ is unused before trusting the diff.

• The paths: filter now sends shell-only PRs through lint and unit-tests as well. The PR body calls this out and the trade is right; noting only that if CI time ever becomes a concern, splitting shellcheck into its own workflow file is the escape hatch.

[garretpremo]

Addressed the non-blocking observations in 0805c6d. Label reset to needs review for another pass.

Fixed (cheap and in scope):

1. Glob asymmetry — .claude/skills/**/scripts/**/*.sh is now recursive, matching scripts/**/*.sh. Agreed this was the same silent-partial-coverage failure mode the per-group guards were added to prevent, and the guards structurally can't catch it. Still resolves to the same 37 files (11 + 26), no duplicate expansions.

2. Silent success log — the job now echoes shellcheck: 11 in scripts/, 26 in .claude/skills/ before invoking shellcheck, so the run is self-auditing and a partial-coverage regression that still leaves both groups non-empty becomes visible.

3. style (5) accuracy — the number was right but ambiguous about which tree it described, so the comment now says so explicitly and names the codes.

One correction on that last point: the review says "the post-merge figure is 3." It's 5. Measuring both trees under the identical 37-file invocation CI uses:

tree	--severity=warning	--severity=style
pre-change	1	6
post-change	0	5

Two things make the arithmetic non-obvious. SC2034 is deduped by variable name per file, so the pre-change tree reports only one of the two attempt counters even though two were fixed — the second surfaces only after the first is gone. And SC1091 resolution depends on how many files are passed in one invocation, so per-file subtraction doesn't reconcile either; it has to be measured on the whole set. The remaining 5 are 3×SC1091, 1×SC2016, 1×SC2317 — none of them real defects, which is the point the comment is making.

Filed as follow-up, not fixed here: #144, for the two scripts outside the globs (.claude-jobs/_internal-authors.sh, tests/e2e-tutorial/run.sh) — out of scope for #132, but worth either folding in or documenting as a deliberate exclusion.

Not taken: the local pr="$1" _ nitpick. $_ shadowing is verified inert here (nothing in ship.sh reads it), and switching to a C-style loop would mean touching ship.sh again for a readability preference after its behaviour-preservation has already been checked. Happy to change it if you'd rather.

[garretpremo]

🤖 Automated review by claude — generated by the review-issue skill. Treat as advisory; a human still owns the merge decision.

Second pass over 0805c6d. All three non-blocking observations from the first pass are addressed, the fourth is tracked in #144, and every acceptance criterion still holds. I re-measured the whole thing independently against shellcheck v0.10.0 rather than taking the PR body's numbers on trust. No blocking issues.

On the disputed number: you're right, it's 5, not 3. My first pass said the post-merge style figure would be 3. I installed the pinned v0.10.0 and ran both trees under the exact 37-file invocation CI uses:

tree	--severity=warning	--severity=style
pre-change (e5673e5)	1	6
post-change (0805c6d)	0	5

The post-change 5 breaks down as 3×SC1091, 1×SC2016, 1×SC2317 — exactly what the workflow comment now claims. Both mechanisms you gave for why the arithmetic doesn't reconcile are real: the pre-change tree reports only one SC2034 (at ship.sh:105) even though two attempt counters were fixed, because shellcheck dedups by variable name per file; and SC1091 resolution shifts with how many files are in one invocation, so per-file subtraction doesn't work either. My earlier figure came from subtracting the fixes from the style total, which is precisely the invalid step. The comment as written is accurate.

Verification performed on this pass:

• Base is dev; all four checks green on 0805c6d (lint, shellcheck, unit-tests ubuntu + windows).
• Glob asymmetry closed. .claude/skills/**/scripts/**/*.sh is now recursive and still resolves to the same 26 files; expanded both groups under nullglob globstar and confirmed 11 + 26 = 37 with 37 unique paths — no duplicate expansion introduced by the second **.
• The count log is live. The job log for run 30883968541 shows shellcheck: 11 in scripts/, 26 in .claude/skills/ before the invocation, so the run is now self-auditing — that was the gap that forced me to reconstruct coverage from the tree last time.
• Independently reproduced the gate: the pre-change tree exits 1 at --severity=warning; the post-change tree exits 0. error is 0 on both, confirming it would be a no-op gate.
• The --enable=all premise holds. SC2310/SC2311 fire zero times across all 37 files even at the broadest setting (--enable=all --severity=style). Declining --enable=all is correct on the measured evidence.
• Downloaded the pinned tarball myself: sha256 6c881ab0…dedf87 verifies OK against the real koalaman v0.10.0 release, and the CI log shows the same OK rather than a fall-through.
• Scope is clean: 2 files, +54/−4. git diff confirms no # shellcheck disable= anywhere — fixed, not suppressed, as AC 2 requires.
• bash -n scripts/ship.sh passes; grep confirms nothing in the file reads $_. Iteration counts (5 and 30) and sleep values are unchanged.
• push: to main/dev carries no paths: filter, so the gate always runs on branch heads regardless of the PR-level filter.

On the declined nitpick — agreed, leave it. $_ shadowing is verified inert here and ship.sh's behaviour-preservation has already been checked; re-touching it for a readability preference is the worse trade. Not worth another round.

Non-blocking observations

• The two --enable=all figures in the workflow comment have drifted slightly. The comment says ~35 info + ~592 style; measured against the current tree they are 37 and 614 (confirmed with -f json | jq length, so it isn't a line-counting artifact — the pre-change tree gives 38 and 615). Both are tilde-hedged and the gap is ~4%, which doesn't touch the conclusion they support: enabling all buys tens of info findings and hundreds of style findings for zero signal on the motivating case. Noting it only because these are the one pair of numbers in that comment I couldn't reproduce exactly, and they'll keep drifting as scripts are added — the surrounding prose would carry the argument fine without the precise counts.

• The non-empty guards make deleting a whole script directory a CI failure. That's the intended design and the right default, but it does mean a legitimate future change that removes the last file from either group fails the job rather than skipping it. The error message names the offending glob, so the fix is obvious to whoever hits it; just worth knowing the guard is strict rather than advisory.

Nitpicks

• #144 is filed and open but sits at needs triage. Nothing blocks this PR on it — flagging only so the deliberate exclusion of .claude-jobs/_internal-authors.sh and tests/e2e-tutorial/run.sh doesn't sit untracked indefinitely now that the gate exists.

[garretpremo]

🤖 Automated review by claude — generated by the review-issue skill. Treat as advisory; a human still owns the merge decision.

Third pass, over 6859876. The single outstanding observation from the second pass — the drifted --enable=all figures in the workflow comment — is resolved, and I confirmed the new wording is exactly right rather than just plausible. Every acceptance criterion still holds and no new issues appeared. Nothing blocking.

The corrected figures reproduce exactly. I re-installed shellcheck v0.10.0 from the pinned URL and re-measured the full 37-file invocation against 6859876, without reference to the previous passes' numbers:

setting	measured
default, --severity=error	0
default, --severity=warning	0
default, --severity=style	5 — 3×SC1091, 1×SC2016, 1×SC2317
--enable=all --severity=info	37
--enable=all --severity=style	614
SC2310 + SC2311, at the broadest setting	0

Every one of those matches what the comment now claims. The fix is also the right shape: tens of info and hundreds of style findings (37 and 614 when this was written) puts the argument's weight on the order of magnitude — which is what actually justifies declining --enable=all — and timestamps the exact counts as a point-in-time measurement rather than a standing assertion that decays as scripts are added. That's the durable version of the correction, not just an arithmetic patch.

Re-verified independently on this pass rather than carried forward:

• Base is dev; all four checks green on 6859876 (lint, shellcheck, unit-tests ubuntu + windows).
• Downloaded the pinned tarball myself — sha256 6c881ab0…dedf87 verifies OK against the real koalaman v0.10.0 release asset.
• Ran the job's exact script against the tree: baseline exits 0 over 11 + 26 = 37 files.
• Re-ran the full gate simulation end to end, not just the parts this commit touched: injected SC2034 into a scripts/ file → exit 1; same into a .claude/skills/*/scripts/ file → exit 1; renamed scripts/ → exit 1 naming scripts/**/*.sh; renamed .claude/skills/ → exit 1 naming .claude/skills/**/scripts/**/*.sh. The guards fail loudly and name the offending glob in all four cases.
• git grep confirms zero # shellcheck disable= directives anywhere in the tree — fixed, not suppressed, per AC 2.
• bash -n scripts/ship.sh parses; nothing in the file reads $_, so the attempt → _ rename is inert. Iteration counts (5, 30) and sleep values unchanged.
• Scope is clean — this commit is 3 insertions / 2 deletions, comment prose only, no behavioural change to the gate.
• Branch protection is not enabled on dev or main, so the new shellcheck job doesn't need adding to a required-checks list.

Non-blocking observations

• The per-group guards still can't see a single-skill rename. They catch a wholesale directory rename in either group, but if one skill's scripts/ dir were renamed while the others kept theirs, skill_files stays non-empty and the job stays green over fewer files. The count echo added in 0805c6d is the mitigation and is the right tradeoff — a hard-coded expected count would fail on every legitimate script addition — but it only helps if someone reads the log. Noting the residual gap, not asking for a change.
• The two scripts outside the globs (.claude-jobs/_internal-authors.sh, tests/e2e-tutorial/run.sh) are tracked in #144 and both still pass at --severity=warning, so nothing regressed while that sits at needs triage. Related: .claude-jobs/** isn't in the paths: filter either, so a PR touching only those scripts won't trigger the workflow at all — worth folding into #144's scope when it gets triaged rather than raising separately.

Nitpicks

• The PR description's cost table still reads --enable=all --severity=info 35 and --severity=style 592 — the pre-0805c6d numbers. The workflow comment has been corrected to 37 and 614; the description hasn't. The committed artifact is what AC 5 is about and it's accurate, so this is cosmetic, but the body is the thing a future reader hits first from the issue.
• Same staleness one section up: the description quotes the skill glob as .claude/skills/**/scripts/*.sh, while the merged version is .claude/skills/**/scripts/**/*.sh.

[garretpremo]

🤖 Final review by claude — generated by the final-review skill. CI is green, no blockers, soak window elapsed. Marking approved and merging.

Approved.

Outstanding observations from first-pass review

• The per-group guards still can't see a single-skill rename. They catch a wholesale directory rename in either group, but if one skill's scripts/ dir were renamed while the others kept theirs, skill_files stays non-empty and the job stays green over fewer files. The count echo added in 0805c6d is the mitigation and is the right tradeoff — a hard-coded expected count would fail on every legitimate script addition — but it only helps if someone reads the log. Noting the residual gap, not asking for a change.
• The two scripts outside the globs (.claude-jobs/_internal-authors.sh, tests/e2e-tutorial/run.sh) are tracked in #144 and both still pass at --severity=warning, so nothing regressed while that sits at needs triage. Related: .claude-jobs/** isn't in the paths: filter either, so a PR touching only those scripts won't trigger the workflow at all — worth folding into #144's scope when it gets triaged rather than raising separately.