Skip to content

Add shellcheck to CI (refs #1) - #6

Merged
dolph merged 2 commits into
mainfrom
claude/review-repository-7F3Om
Apr 26, 2026
Merged

Add shellcheck to CI (refs #1)#6
dolph merged 2 commits into
mainfrom
claude/review-repository-7F3Om

Conversation

@dolph

@dolph dolph commented Apr 26, 2026

Copy link
Copy Markdown
Owner

Summary

  • Adds a shellcheck job to .github/workflows/go.yml that runs on every PR (and push to main).
  • The job discovers shell scripts dynamically rather than hard-coding paths — newly added or renamed scripts get linted automatically without further workflow changes, regardless of which directory they live in.
  • Fixes the one finding shellcheck reports today: SC2046 in build.sh, an unquoted command substitution nested inside git rev-parse --short. Quoting the inner $(git rev-list -1 HEAD) resolves the warning so the new job is green from the start.

Refs #1.

Discovery approach

shellcheck has no built-in recursive / directory-scan option (shellcheck --help confirms FILES... is a hard requirement), so discovery is done externally. The step uses git ls-files, which has two nice properties:

  1. It only enumerates tracked files — no scanning into .git/, vendored deps, or local untracked artifacts.
  2. New or renamed scripts appear in git ls-files output on the PR that introduces them, so they get linted on that same PR with no manual update to the workflow.

Two predicates feed into the script list, then deduped via sort -u:

  • Any file matching *.sh / *.bash / *.ksh / *.zsh.
  • Any file whose first line matches ^#!.*sh([[:space:]]|$) — covers #!/bin/sh, #!/bin/bash, #!/usr/bin/env bash, #!/bin/dash -x, #!/bin/zsh, etc., catching scripts without a conventional extension.

Tested locally on this branch: discovers build.sh and install.sh, shellcheck exits 0.

Scope note

This is the belt-and-suspenders half of the test plan posted on #1, not the load-bearing test. shellcheck doesn't model curl's flag semantics, so it does not catch the specific curl -L -o <URL> bug from that issue. A behavioral test — running the download step against a local HTTP fixture in CI and asserting that a ussher binary file actually lands in cwd — is still needed to cover #1 directly. That belongs in a separate PR.

What this PR does buy us: cheap, automatic coverage for the broader class of shell-script bugs (unquoted expansions, [[ vs [, dead code, missing -- separators, etc.) for every shell script in the repo, now and in the future.

Test plan

  • Discovery snippet picks up install.sh and build.sh locally.
  • shellcheck exits 0 against the discovered set.
  • ./build.sh still runs end-to-end and GIT_COMMIT is populated correctly after the SC2046 fix (tests pass, --version prints expected commit SHA).
  • CI run on the PR shows the new shellcheck job passing alongside the existing build job.
  • Future regression test: a follow-up PR that adds a script anywhere under the repo (with or without a .sh extension, as long as it has a shell shebang) is automatically linted with no further workflow changes.

https://claude.ai/code/session_013HnepY8MhhxrJJjE5ysW47

claude added 2 commits April 26, 2026 21:08
Introduces a shellcheck job to the existing Go workflow that runs on
every PR and lints install.sh and build.sh. Also fixes the one warning
shellcheck flagged in build.sh (SC2046, unquoted command substitution
inside `git rev-parse --short`) so the new job passes from day one.

shellcheck won't catch the specific `curl -L -o <URL>` bug from #1
(curl flag semantics aren't in shellcheck's domain), so this is the
"belt-and-suspenders" half of the test plan posted on that issue, not
the load-bearing test. A behavioral test that runs the download step
against a local fixture server is still needed to cover #1 directly.

Refs #1
Replaces the hard-coded `install.sh build.sh` argument list with a
discovery step that finds every tracked shell script in the repo,
regardless of directory. shellcheck has no built-in recursive mode, so
discovery is done externally via `git ls-files`:

  - any tracked file with a known shell extension (.sh/.bash/.ksh/.zsh)
  - any tracked file whose first line is a shell shebang

Scoping to `git ls-files` means newly added or renamed scripts are
picked up automatically on the PR that introduces them, and we don't
scan vendored/build artifacts that aren't under version control.

Refs #1
Comment thread .github/workflows/go.yml
# picks up newly added or renamed scripts on the PR that introduces them.
mapfile -t scripts < <(
{
git ls-files -- '*.sh' '*.bash' '*.ksh' '*.zsh'

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can safely assume all shell files in this repo are .sh

@dolph
dolph merged commit 08c89a0 into main Apr 26, 2026
2 checks passed
dolph added a commit that referenced this pull request Apr 26, 2026
Simplify shellcheck discovery to .sh only (review feedback from #6)
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.

2 participants