Skip to content

feat(shellcheck): support tracked extensionless inputs - #114

Merged
kyle-sexton merged 2 commits into
mainfrom
feat/shellcheck-extra-globs
Jul 16, 2026
Merged

kyle-sexton merged 2 commits into
mainfrom
feat/shellcheck-extra-globs

Conversation

@kyle-sexton

Copy link
Copy Markdown
Contributor

Summary

  • add newline-delimited extra-globs Git pathspecs for tracked extensionless shell inputs
  • add extra-exclude-codes, applied only to the extra lane through a separate ShellCheck invocation
  • move discovery into a directly tested runner and execute its focused regression suite in this repository's CI
  • document compatibility and the new consumer contract

Closes #64.

Why

The action previously discovered only tracked *.sh and *.bash files, so extensionless sourced Bash files required a second consumer-owned CI invocation. ShellCheck accepts explicit file arguments independently of filename extension, while Git's NUL-delimited tracked-file output supplies a safe discovery boundary for caller-provided pathspecs.

The design follows the official ShellCheck command contract, git ls-files pathspec and -z behavior, and GitHub composite-action input guidance.

Compatibility and scope

  • Default discovery remains tracked *.sh/*.bash.
  • Explicit paths retain their existing raw-filesystem behavior.
  • Extra matches are always tracked, absent sparse-checkout entries are skipped, and overlapping normal files stay in the strict lane.
  • Extra suppressions fail closed when malformed or configured without extra pathspecs.
  • No standards-managed Lefthook content changes here; local-hook migration belongs in the upstream standards/consumer sync path.

Validation

  • node --test .github/scripts/*.test.cjs — 234/234 passing
  • bash .github/actions/shellcheck/run.test.sh — 24/24 passing
  • ShellCheck and shfmt on both new scripts
  • actionlint plus GitHub action/workflow schema validation
  • markdownlint-cli2, typos, gitleaks, and git diff --check
  • real consumer integration against the dotfiles checkout: standard files plus dot_bash* extras with scoped SC1090,SC1091, exit 0
  • zizmor pedantic/offline: no low, medium, or high findings; informational anonymous-job findings match main

@claude

claude Bot commented Jul 16, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 0s —— View job


I'll analyze this and get back to you.

@claude

claude Bot commented Jul 16, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 0s —— View job


I'll analyze this and get back to you.

@kyle-sexton
kyle-sexton marked this pull request as ready for review July 16, 2026 13:21
@claude

claude Bot commented Jul 16, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 4m 32s —— View job


PR #114 Review: feat(shellcheck): support tracked extensionless inputs

Tasks:

  • Gather context from PR metadata
  • Read changed files (action.yml, run.sh, run.test.sh, ci.yml, README.md)
  • Review run.sh — shell correctness, injection safety, error handling
  • Review action.yml — input contracts, SHA pinning, security
  • Review run.test.sh — test coverage adequacy
  • Review ci.yml and README.md changes
  • Post comprehensive review findings

Security

Injection safety — clean. All five inputs arrive via env: in action.yml:69-74, never as ${{ inputs.xxx }} inside a run: shell expression. The multiline extra_globs value is parsed with while IFS= read -r pathspec in run.sh:21-24, so no shell metacharacter from a pathspec is ever evaluated. Pathspecs flow through a quoted array and behind -- at run.sh:39, which prevents a leading - from becoming a git option. No injection vector found.

extra-exclude-codes validation — correct and fail-closed. The two-step guard at run.sh:26-33 rejects orphaned codes before format, and the regex ^SC[0-9]{4}(,SC[0-9]{4})*$ is tight (no spaces, no trailing comma, no lowercase prefix). Matches test assertions at lines 162–170.


Correctness

Status aggregation — safe under set -e. The arithmetic at run.sh:140:

((extra_status <= status)) || status=$extra_status

When ((expr)) evaluates false, bash exits the subexpression with code 1. Without the ||, this would trigger set -e. With it, the || clause fires and assigns the higher status — the pattern is correct. The comment at lines 138–140 explains the ShellCheck exit-code ordering assumption, which matches the ShellCheck man page.

discover_tracked_files returns via an implicit global run.sh:44:

git_files=()
mapfile -d '' -t git_files <"$output"

Bash cannot return arrays from functions, so storing to a callee-visible global is the right approach. The failure path (return 2 at line 42) causes set -e to terminate the script before any caller reads git_files, so stale-value hazard is not reachable. Correct, if non-obvious at first glance.

filter_files — unquoted $exclude performs glob expansion run.sh:83:

for substring in $exclude; do

Word-splitting is intentional (the exclude input is space-separated), but bash also applies glob expansion to the unquoted variable. If $exclude contains * or ?, those characters expand to filenames in the working directory rather than being treated as literal substring patterns. This is pre-existing behavior brought in from the old action.yml inline script and not introduced by this PR, but now that the logic lives in a directly tested file it could be fixed cheaply with set -f / set +f around the loop, or by reading the value into an array with read -ra excludes <<<"$exclude". Fix this →

Overlap deduplication — correct. The normal_seen / extra_seen associative array approach at run.sh:98-108 correctly keeps a normal-lane file in the strict lane and removes it from the extra lane, even if the user's extra pathspec matches it.

Empty-array guards — idiomatic. The ${arr[@]+"${arr[@]}"} pattern is used consistently throughout (lines 80, 88, 100, 103, 108) to avoid set -u errors on empty arrays. ✓


Test Coverage

The 24-case suite in run.test.sh covers the key scenarios well: default discovery, two-lane invocation, extra-only discovery, empty result, overlap deduplication, gitignored exclusion, raw paths, exclude filtering, orphaned codes, malformed codes, invalid pathspec, and findings propagation. The wiring meta-test at run.test.sh:181-184 is a particularly good pattern.

Minor gap — split-lane failure: The test at line 177 uses a single FAKE_STATUS=1 that applies to both ShellCheck invocations. There is no test for "only the extra lane fails" (normal passes, extra returns 1). This scenario exercises the ((extra_status <= status)) branch where extra_status=1 and status=0, which is a distinct code path. The existing logic is correct for that case, but a targeted test would confirm it. Fix this →

Minor gap — CRLF stripping: run.sh:22 strips trailing \r from each pathspec line, which is a useful defensive measure for Windows consumers, but this path has no test coverage.


action.yml

  • No uses: references in the composite action body → CLAUDE.md SHA-pinning rule does not apply here. ✓
  • The env: block correctly forwards all five inputs without expression-injection. ✓
  • extra-exclude-codes description accurately states the extra-globs dependency. ✓

ci.yml

The new test step is cleanly appended to the existing shellcheck job (lines 316–317). No new jobs were added, so the ci-status aggregator requires no changes. ✓

README.md

The two-sentence addition at lines 49–51 is accurate and consistent with the action.yml input descriptions. ✓


Summary

This is a well-designed feature. The security model is sound: the git-tracked discovery boundary prevents untracked/ignored files from entering the extra lane even when paths uses a raw filesystem walk; inputs never reach shell evaluation; pathspec arguments are properly shielded with --. The two-lane ShellCheck architecture correctly isolates extra suppressions from the strict lane. The test suite is comprehensive and the in-test wiring check is a nice self-consistency mechanism.

Two actionable items above the fold: the pre-existing $exclude glob-expansion issue is worth fixing now that the code is directly tested, and a split-lane failure test would close the one untested branch in the status aggregation logic. Both are minor. Everything else looks correct and production-ready.

@kyle-sexton
kyle-sexton merged commit 30b9caf into main Jul 16, 2026
30 checks passed
@kyle-sexton
kyle-sexton deleted the feat/shellcheck-extra-globs branch July 16, 2026 13:25

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f6fb9fa516

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +59 to +62
read -r -a path_roots <<<"$paths"
mapfile -d '' -t normal_files < <(
find "${path_roots[@]}" -type f \( -name '*.sh' -o -name '*.bash' \) \
-not -path '*/.git/*' -print0 | sort -z

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve glob expansion for explicit paths

When a caller sets explicit paths to a globbed root such as packages/*, this quoted array passes the * literally to find; the previous inline action used find $PATHS, so the shell expanded those roots before find ran. Because the resulting find: ... No such file or directory happens inside the process substitution, mapfile still succeeds and the lane can continue with those scripts omitted, weakening ShellCheck coverage for consumers that already rely on globbed roots. Either preserve the old expansion semantics deliberately or fail when a supplied root matches nothing.

Useful? React with 👍 / 👎.

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.

shellcheck action: add extra-glob input for extensionless sourced bash files

1 participant