fix(scripts): remove dead spec_ref var in sync-spec.sh (SC2034)#60
Merged
Conversation
`spec_ref` was declared and commented in the resolution block but never assigned to or read from anywhere in the script. The actual code reads `SPEC_REF` (the env var) directly. Shellcheck SC2034 caught the leftover. Verification: - `shellcheck scripts/sync-spec.sh` → 0 findings. - `bash scripts/sync-spec.sh --help` prints the documented banner unchanged.
Closes the gap surfaced by the SC2034 finding on sync-spec.sh: the local pre-push hook and CI both lacked any shell linting, so dead variables and other shellcheck-grade bugs shipped silently. New step lints every `*.sh` plus everything under `scripts/hooks/` (so the pre-push hook itself is linted). Uses `git ls-files` rather than `find` so vendored / `.gitignore`d scripts aren't scanned. Severity threshold "warning" catches real bugs (unused vars, quoting issues, missing exits, command-not-found patterns) while filtering info/style noise. Specifically lets through SC2016 on `scripts/prose-check.sh:196` (intentional single-quote `jaq` filter) without needing an inline disable directive. Skip path mirrors the existing `cargo deny` pattern: if `shellcheck` is not installed, print a one-line note and continue rather than failing. Maintainer machine has it via Homebrew. Verified locally: the full hook (`bash scripts/hooks/pre-push`) runs end-to-end with the new step passing on the current tree. Step renumbered (5 -> 6 -> 7) in the source comments to keep the in-file numbering consistent. ## Coverage gap surfaced by sibling-repo audit While adding this, swept the sibling agentnative-* repos and the shared `brettdavies/.github` reusable workflows. Coverage map: | repo | pre-push shellcheck | CI shellcheck | .sh count | | --------------------- | ------------------- | ----------------------------------- | --------- | | agentnative-cli | yes (this PR) | no | ~7 | | agentnative-spec | no | no | 3 | | agentnative-site | no | no | 32 | | agentnative-skill | no pre-push hook | yes (`ludeeus/action-shellcheck@2`) | 3 | | brettdavies/.github | n/a | no (rust-ci.yml has no shellcheck) | n/a | agentnative-site is the largest exposure (32 .sh files with zero linting). agentnative-skill is the only repo with any shellcheck today and it runs in CI only. The shared `rust-ci.yml` does not run shellcheck, so adding it there would close the gap across every Rust consumer. This PR only closes the cli pre-push gap. CI-side and sibling-repo coverage left for follow-up.
25 tasks
brettdavies
added a commit
to brettdavies/.github
that referenced
this pull request
May 27, 2026
Lints every tracked *.sh plus scripts/hooks/* with shellcheck --severity=warning, mirroring the local pre-push hook added in brettdavies/agentnative-cli#60. Closes the CI-side gap for every Rust consumer of this workflow (agentnative-cli, agentnative-spec, agentnative-site) in one stroke. Uses apt-get install shellcheck + a run: block rather than a third-party action, matching the convention in the rest of rust-ci.yml (cargo fmt / clippy / test / publish all shell out via run:; third-party actions reserved for non-trivial functionality like rust-cache and cargo-deny).
brettdavies
added a commit
to brettdavies/.github
that referenced
this pull request
May 27, 2026
## Summary Add a `shellcheck` job to the reusable `rust-ci.yml` workflow so every Rust consumer (`agentnative-cli`, `agentnative-spec`, `agentnative-site`) gains CI-side shell linting in one stroke. Closes the gap surfaced in [brettdavies/agentnative-cli#60](brettdavies/agentnative-cli#60), which added the same step to the local pre-push hook but found no equivalent in CI. The job installs `shellcheck` via `apt-get` and runs `shellcheck --severity=warning` against every tracked `*.sh` file plus everything under `scripts/hooks/`. Uses `git ls-files` so vendored or `.gitignore`d scripts are excluded. The `--severity=warning` floor catches SC2034 (unused vars, what motivated #60), SC2086 (missing quoting), SC2155 (`local var=$(...)` masking exit codes), and the rest of the warning tier while filtering info/style noise. ## Changelog ### Added - `rust-ci.yml` reusable workflow now includes a `shellcheck` job that lints every tracked `*.sh` and `scripts/hooks/*` with `shellcheck --severity=warning`. Consumer repos pinning `@main` will pick it up on their next workflow run. ## Type of Change - [x] `feat`: New feature (non-breaking change which adds functionality) - [x] `ci`: CI/CD configuration changes ## Related Issues/Stories - Story: n/a - Issue: n/a - Architecture: closes the CI-side shell-linting gap identified in the coverage table in brettdavies/agentnative-cli#60. - Related PRs: brettdavies/agentnative-cli#60 (local pre-push hook side of the same gap). ## Testing - [x] Manual testing completed - [x] All tests passing **Test Summary:** - `python3 -c "import yaml; yaml.safe_load(open('.github/workflows/rust-ci.yml'))"` → OK. - `actionlint .github/workflows/rust-ci.yml` → 0 findings. - The embedded `run:` bash block was extracted to a tempfile and run through `shellcheck --severity=warning` itself → 0 findings. - Consumer-side: the agentnative-cli pre-push hook already runs the same `shellcheck --severity=warning` against the same file set; CI will mirror that local result. ## Files Modified **Modified:** - `.github/workflows/rust-ci.yml`: new `shellcheck` job added between `package-check` and `changelog-check`. Runs in parallel with the other jobs on `ubuntu-22.04`. **Created:** - None. **Renamed:** - None. **Deleted:** - None. ## Action choice rationale Native `apt-get install shellcheck` + a `run:` block, not `ludeeus/action-shellcheck`. Matches the convention in the rest of `rust-ci.yml`: third-party Actions are used only when they bring non-trivial functionality (`Swatinem/rust-cache`, `EmbarkStudios/cargo-deny-action`, `actions/github-script`); everything else shells out via `run:` (`cargo fmt`, `cargo clippy`, `cargo test`, `cargo publish --dry-run`). A two-line `apt-get install` does the same job without pinning another third-party SHA. The local pre-push hook in `agentnative-cli` uses the identical invocation (`git ls-files '*.sh'` + `scripts/hooks/*`, `--severity=warning`), so CI and the local gate enforce the same floor against the same file set. ## Breaking Changes - [x] No breaking changes Additive: an extra parallel job. Existing jobs are untouched. ## Deployment Notes - [x] No special deployment steps required Consumer repos pinning `uses: brettdavies/.github/.github/workflows/rust-ci.yml@main` get the new job on their next workflow run. Repos pinning a SHA need to bump the pin. ## Checklist - [x] Code follows project conventions and style guidelines - [x] Commit messages follow Conventional Commits - [x] Self-review of code completed - [x] Tests added/updated and passing - [x] No new warnings or errors introduced - [x] Changes are backward compatible
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
shellcheck scripts/sync-spec.shflagged a dead variable (spec_ref) introduced by the recent--refrefactor in #59. Removed. Zero behavior change.While here, closed the process gap that allowed it to ship: added
shellcheckto the local pre-push hook. Also audited the sibling agentnative-* repos and the sharedbrettdavies/.githubreusable workflow for the same gap; results in the Coverage Gap section below.Changelog
Added
scripts/hooks/pre-pushnow runsshellcheck --severity=warningagainst every tracked*.shplus everything underscripts/hooks/(so the hook itself is linted). Usesgit ls-filesso vendored /.gitignored scripts are excluded. Skips with a one-line note ifshellcheckis not installed, matching the existingcargo denypattern.Fixed
spec_refvariable inscripts/sync-spec.sh(SC2034). Declared during the--refrefactor in feat(scripts): sync-spec.sh --ref flag + gh api transport #59 but never read; the actual code uses theSPEC_REFenv var directly.Type of Change
fix: Bug fix (dead-code removal)feat: Pre-push hook gains a shellcheck stepRelated Issues/Stories
Testing
Test Summary:
shellcheck scripts/sync-spec.sh→ 0 findings (was 1 before).shellcheck --severity=warning <every tracked .sh + scripts/hooks/*>→ 0 findings across the whole tree.bash scripts/hooks/pre-pushruns end-to-end on the current tree: fmt, clippy, test, deny, shellcheck, windows-grep, windows-clippy all pass.bash scripts/sync-spec.sh --helpbanner unchanged.cargo build --releaseclean.Files Modified
Modified:
scripts/sync-spec.sh: removed thespec_ref=""declaration and its stale comment line.scripts/hooks/pre-push: added shellcheck step as step 5, renumbered windows-grep / windows-clippy to 6 / 7 in the source comments.Created:
Renamed:
Deleted:
Severity threshold rationale
--severity=warningcatches:local var=$(...)masking exit codes)It deliberately skips info/style noise. Specifically lets through SC2016 on
scripts/prose-check.sh:196(intentional single-quotejaqfilter where the dollar-signs are jaq syntax, not shell variables) without needing an inline# shellcheck disable=directive.Tightening to
--severity=infolater is a one-line change if you want stricter enforcement.Coverage gap across the agentnative-* ecosystem
Subagent sweep of the three sibling repos plus the shared reusable workflow:
.shfilesagentnative-cliagentnative-specagentnative-siteagentnative-skillludeeus/action-shellcheck@2.0.0on./scripts/, style severity)brettdavies/.githubrust-ci.ymlhas no shellcheck step; just fmt/clippy/test/deny/package/changelog)Summary: the only repo with shellcheck today is
agentnative-skill, and it runs in CI only. The sharedrust-ci.ymlthatagentnative-cli,agentnative-spec, andagentnative-siteall delegate to does NOT run shellcheck, so adding it there would close the gap across every Rust consumer in one stroke.Follow-up work (not in this PR)
brettdavies/.github'srust-ci.yml(closes the gap for cli + spec + site simultaneously), or add a job locally inagentnative-cli/.github/workflows/ci.yml.agentnative-spec/scripts/hooks/pre-pushandagentnative-site/scripts/hooks/pre-push. Site has 32.shfiles, so the payoff is highest there.Breaking Changes
Pre-push step is opt-in (skips silently if
shellcheckis not installed).Deployment Notes
Maintainers who want the new step should
brew install shellcheck(or apt equivalent). Otherwise the existing pipeline behavior is preserved.Checklist