feat: add shellcheck gate for actions/setup/sh - #49761
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds error-level ShellCheck validation for setup shell scripts.
Changes:
- Adds
shellcheck-setup-sh. - Integrates it into
lint.
Show a summary per file
| File | Description |
|---|---|
| Makefile | Defines and wires in the ShellCheck lint gate. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Balanced
| .PHONY: shellcheck-setup-sh | ||
| shellcheck-setup-sh: | ||
| @echo "Running shellcheck on actions/setup/sh..." | ||
| @shellcheck --severity=error actions/setup/sh/*.sh |
There was a problem hiding this comment.
Addressed in dba99b9 — /home/runner/work/gh-aw/gh-aw/Makefile now installs pinned ShellCheck v0.11.0 via make install-shellcheck, deps-dev includes that installer, and make shellcheck-setup-sh uses the managed binary path instead of assuming a host-provided shellcheck.
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
@copilot add a ci job that runs the shellcheck linting |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: feat: add shellcheck gate for actions/setup/sh
The change is minimal and well-scoped — one new shellcheck-setup-sh Make target wired into lint. The logic is correct.
An existing comment already flags the blocking issue: shellcheck is invoked by lint but is not installed by deps-dev or any documented setup step, so CI and new contributors will hit a missing-binary failure on first run.
No other blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 27.1 AIC · ⌖ 7.49 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Verdict: Approve with a note
Small, low-risk addition (8 lines) wiring shellcheck --severity=error into the lint target for actions/setup/sh/*.sh.
Notes
- The dependency gap (shellcheck not installed by
deps-dev/tools, only implicitly available because GitHub-hostedubuntu-latestrunners ship it preinstalled) is already flagged in an existing review comment on this PR — not duplicating it here. - No other correctness, security, or maintainability issues found in this diff; the change is scoped correctly and the
.PHONYtarget and error message are consistent with neighboring targets.
🔎 Code quality review by PR Code Quality Reviewer · auto · 14.8 AIC · ⌖ 4.52 AIC · ⊞ 7.8K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — requesting changes on three issues before merging.
📋 Key Themes & Highlights
Issues to address
-
Missing binary guard (line 1026) —
shellcheckis not installed bydeps-dev; any developer or CI runner without it gets an unhelpfulcommand not foundfailure. Add a pre-flight guard matching thegolintpattern, or install shellcheck indeps-dev. -
Shallow glob (line 1026) —
actions/setup/sh/*.shskips sub-directories and errors when no files match. Replace withfind actions/setup/sh -name '*.sh' -print0 | xargs -0 shellcheck --severity=error. -
Missing help entry — The new
shellcheck-setup-shtarget is undocumented in thehelpblock; every other lint target has an entry.
Positive Highlights
- ✅ Clean, minimal change that wires directly into the existing
lintpipeline - ✅ Correct use of
--severity=errorto avoid blocking on pre-existing style noise - ✅ Good
.PHONYdeclaration
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 44.6 AIC · ⌖ 8.47 AIC · ⊞ 7.1K
Comment /matt to run again
Comments that could not be inline-anchored
Makefile:1026
[/codebase-design] The shellcheck binary is not installed by deps-dev, so make lint will fail with command not found on machines that lack it.
<details>
<summary>💡 Suggested fix</summary>
Add a pre-flight guard (mirroring the golint pattern at lines 756–762):
shellcheck-setup-sh:
`@if` ! command -v shellcheck >/dev/null 2>&1; then \n echo "shellcheck is not installed. Run: brew install shellcheck / apt-get install shellcheck"; \n exit 1; \n fi
`@shellcheck` --s…
</details>
<details><summary>Makefile:1031</summary>
**[/codebase-design]** The glob `actions/setup/sh/*.sh` only covers the top-level directory. If scripts are nested (e.g. sub-directories), they are silently skipped. Also, if no `.sh` files exist at that path the shell expansion fails with `No such file or directory`.
<details>
<summary>💡 Suggested fix</summary>
Use `find` for recursive coverage and handle the empty-match case:
```makefile
`@find` actions/setup/sh -name '*.sh' -print0 | xargs -0 shellcheck --severity=error`find ... | …
Makefile:1310
[/codebase-design] The help target at this line is missing an entry for the new shellcheck-setup-sh target. All other added targets in this Makefile are documented in the help block.
<details>
<summary>💡 Suggested fix</summary>
Add after line 1310:
`@echo` " shellcheck-setup-sh - Run shellcheck on actions/setup/sh scripts at error severity"</details>
@copilot please address this.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot Please address the open review feedback on this PR, then run the pr-finisher skill. Open review thread(s):
Also refresh the branch as needed and re-run formatting/checks before pushing.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in dba99b9 — I applied the open ShellCheck review feedback, refreshed the branch state by fetching |
PR Triage
Automated triage — see full report issue for details. Structured data: {
"action": "defer",
"category": "feature",
"pr_number": 49761,
"risk": "low"
}
|
|
🎉 This pull request is included in a new release. Release: |
Adds
shellcheckat--severity=erroronactions/setup/sh/*.shas a linting gate wired into the existinglinttarget, which propagates automatically to bothagent-report-progress-no-testandagent-report-progress.Changes
shellcheck-setup-shtarget — runsshellcheck --severity=error actions/setup/sh/*.shlinttarget — addsshellcheck-setup-shas a dependency alongsidelint-action-sh--severity=erroris the baseline (all scripts currently pass); style-level warnings are not enforced to avoid blocking on pre-existing noise.