feat: shellcheck disabled by default, opt-in via --shellcheck/--validate, parallel execution - #49880
Conversation
…idate, run in parallel Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Makes shellcheck opt-in while retaining --validate integration and parallelizing run-step linting.
Changes:
- Replaces
--no-shellcheckwith--shellcheck. - Enables shellcheck through
--shellcheckor--validate. - Runs shellcheck steps concurrently and simplifies MCP handling.
Show a summary per file
| File | Description |
|---|---|
cmd/gh-aw/main.go |
Adds and wires the new CLI flag. |
pkg/cli/compile_command_test.go |
Updates default configuration assertions. |
pkg/cli/compile_config.go |
Replaces the opt-out configuration field. |
pkg/cli/compile_orchestrator.go |
Limits availability warnings to explicit requests. |
pkg/cli/compile_pipeline.go |
Enables shellcheck conditionally in both pipelines. |
pkg/cli/compile_pipeline_yamllint_test.go |
Removes obsolete shellcheck suppression. |
pkg/cli/mcp_tools_readonly.go |
Removes unavailable-shellcheck warning injection. |
pkg/cli/shellcheck.go |
Adds parallel shellcheck execution and aggregation. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 8/8 changed files
- Comments generated: 5
- Review effort level: Balanced
| for i, step := range allSteps { | ||
| wg.Add(1) | ||
| go func(idx int, s runStepInfo) { |
There was a problem hiding this comment.
Fixed in 09a6d65. Added shellcheckMaxConcurrency = 8 semaphore following the same pattern as forecast_compute.go:209-245. Each goroutine acquires a semaphore slot before launching the shellcheck process, preventing exhaustion of process/fd limits on large workflow sets.
| stepErr = runShellcheckOnScriptViaDocker(ctx, s, shellcheckDefaultIgnoreCodes, verbose) | ||
| } else { | ||
| stepErr = runShellcheckOnScript(step, shellcheckDefaultIgnoreCodes, verbose) | ||
| stepErr = runShellcheckOnScript(s, shellcheckDefaultIgnoreCodes, verbose) |
There was a problem hiding this comment.
Fixed in 09a6d65. Both helpers now return ([]byte, error) rather than writing to os.Stderr directly. The goroutine captures the returned bytes; after wg.Wait(), output is flushed to os.Stderr in step order, keeping each diagnostic block intact.
| results[idx] = result{err: stepErr} | ||
| }(i, step) | ||
| } | ||
| wg.Wait() |
There was a problem hiding this comment.
Added TestRunShellcheckOnLockFiles_MultiStep in 09a6d65. The test uses a Docker stub with three run steps (step-a, step-b, step-c) where only step-b triggers a finding. It verifies: (1) all 3 steps are invoked in both modes, (2) diagnostics are attributed to step-b, (3) non-strict mode returns nil despite the failing step, and (4) strict mode returns an error after all steps complete.
| compileCmd.Flags().Bool("grant", false, "Run grant license scanner on container images referenced in compiled .lock.yml files (uses Docker image "+cli.GrantImage+")") | ||
| compileCmd.Flags().Bool("yamllint", false, "Run yamllint YAML linter on generated .lock.yml files (uses Docker image "+cli.YamllintImage+")") | ||
| compileCmd.Flags().Bool("no-shellcheck", false, "Disable shellcheck linting of run step scripts (shellcheck runs by default when available)") | ||
| compileCmd.Flags().Bool("shellcheck", false, "Run shellcheck linting of run step scripts (also enabled by --validate)") |
There was a problem hiding this comment.
Fixed in 09a6d65. Added a major changeset (.changeset/shellcheck-opt-in-default.md) documenting the breaking behavior change and migration guide. Also retained --no-shellcheck as a deprecated no-op so existing scripts continue to work — it prints a deprecation warning when used.
| compileCmd.Flags().Bool("grant", false, "Run grant license scanner on container images referenced in compiled .lock.yml files (uses Docker image "+cli.GrantImage+")") | ||
| compileCmd.Flags().Bool("yamllint", false, "Run yamllint YAML linter on generated .lock.yml files (uses Docker image "+cli.YamllintImage+")") | ||
| compileCmd.Flags().Bool("no-shellcheck", false, "Disable shellcheck linting of run step scripts (shellcheck runs by default when available)") | ||
| compileCmd.Flags().Bool("shellcheck", false, "Run shellcheck linting of run step scripts (also enabled by --validate)") |
There was a problem hiding this comment.
Fixed in 09a6d65. Updated --validate description to: "Enable GitHub Actions workflow schema validation, container image validation, action SHA validation, and run step shell linting (shellcheck)".
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the implementation label and has 64 new lines of code in business logic directories (threshold: 100). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — requesting changes on three correctness/UX issues in the parallel shellcheck implementation.
📋 Key Themes & Highlights
Issues Found
- Availability warning gap (
compile_orchestrator.go:71) — the guard that warns when shellcheck is unavailable only fires for--shellcheck, not for--validate. Users who pass--validateget silent skip. - Strict-mode no longer fails fast (
shellcheck.go:457) — the old sequential loop aborted on the first failure; the new parallel implementation waits for all goroutines before checking errors. This is a silent semantic change that may surprise users in CI who expect an early exit. - Interleaved stderr in parallel runs (
shellcheck.go:436) — bothrunShellcheckOnScriptandrunShellcheckOnScriptViaDockerwrite headers and bodies toos.Stderrdirectly inside goroutines, producing garbled output at scale.
Positive Highlights
- ✅ Clean opt-in inversion; aligns shellcheck with poutine/zizmor/actionlint pattern
- ✅ Dead code in
mcp_tools_readonly.gocleanly removed - ✅ Good goroutine fan-out pattern with
sync.WaitGroupand indexed results slice
Existing review comments (Copilot) already flag the unbounded goroutine pool and the missing multi-step test — those are worth addressing too.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 38.3 AIC · ⌖ 11.1 AIC · ⊞ 7.1K
Comment /matt to run again
| if !config.NoShellcheck && !config.NoEmit && !isShellcheckAvailable() { | ||
| // is never invoked. When the binary is absent, Docker is used as a fallback (lazy | ||
| // — only when there are scripts to lint). Only warn/error when neither is available. | ||
| if config.Shellcheck && !config.NoEmit && !isShellcheckAvailable() { |
There was a problem hiding this comment.
[/codebase-design] Availability warning gap: this guard fires only for config.Shellcheck but the pipeline enables shellcheck for config.Validate too (see compile_pipeline.go). When --validate is passed without --shellcheck and neither the binary nor Docker is available, shellcheck silently skips with no user-facing feedback.
💡 Suggested fix
Change the condition to mirror the pipeline:
if (config.Shellcheck || config.Validate) && !config.NoEmit && !isShellcheckAvailable() {That keeps the existing comment about explicit --shellcheck, or refine it to mention --validate as well.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 09a6d65. Changed the guard condition from config.Shellcheck to config.Shellcheck || config.Validate in compile_orchestrator.go so the availability warning/error fires when shellcheck is triggered via either flag.
| shellcheckLog.Printf("shellcheck complete: steps=%d, issues=%d", len(allSteps), totalIssues) | ||
|
|
||
| if firstErr != nil { | ||
| if strict && firstErr != nil { |
There was a problem hiding this comment.
[/codebase-design] Strict mode now collects all errors before checking — it no longer fails fast. The old doc comment promised "fail fast in strict mode", and that semantic is gone. If a large codebase has hundreds of failing steps, all goroutines will still run to completion. Either preserve early cancellation via context.WithCancel or explicitly document that strict mode now reports-all-then-errors instead of failing on first.
💡 Option: cancel remaining goroutines on first error
ctx, cancel := context.WithCancel(ctx)
defer cancel()
// inside goroutine, after detecting stepErr:
if stepErr != nil && strict {
cancel()
}Or, if the full-scan behaviour is intentional, update the function doc comment accordingly.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 09a6d65. Chose the "document it" path: the doc comment for runShellcheckOnLockFiles now explicitly states "In both strict and non-strict modes every step is checked before returning. When strict is true, any step failure causes a non-nil error to be returned after all steps have been checked (reports-all-then-errors)." The full-scan behaviour is intentional — it gives users a complete picture of all issues in one pass.
| stepErr = runShellcheckOnScriptViaDocker(ctx, s, shellcheckDefaultIgnoreCodes, verbose) | ||
| } else { | ||
| stepErr = runShellcheckOnScript(step, shellcheckDefaultIgnoreCodes, verbose) | ||
| stepErr = runShellcheckOnScript(s, shellcheckDefaultIgnoreCodes, verbose) |
There was a problem hiding this comment.
[/codebase-design] runShellcheckOnScript and runShellcheckOnScriptViaDocker write directly to os.Stderr inside goroutines. With many steps in flight simultaneously, their output lines interleave unpredictably. Collect each step's output into a buffer and flush sequentially after wg.Wait().
💡 Pattern
type result struct {
err error
output []byte // captured stderr for this step
}
// flush in order after wg.Wait()
for _, r := range results {
if len(r.output) > 0 {
os.Stderr.Write(r.output)
}
}@copilot please address this.
There was a problem hiding this comment.
Fixed in 09a6d65. runShellcheckOnScript and runShellcheckOnScriptViaDocker now return ([]byte, error) instead of writing directly to os.Stderr. Each goroutine captures its diagnostic output; after wg.Wait() completes, output is flushed to os.Stderr in step order so diagnostic blocks are never interleaved.
There was a problem hiding this comment.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 31.3 AIC · ⌖ 9.42 AIC · ⊞ 5.4K
| if !config.NoShellcheck && !config.NoEmit && !isShellcheckAvailable() { | ||
| // is never invoked. When the binary is absent, Docker is used as a fallback (lazy | ||
| // — only when there are scripts to lint). Only warn/error when neither is available. | ||
| if config.Shellcheck && !config.NoEmit && !isShellcheckAvailable() { |
There was a problem hiding this comment.
The availability guard only fires when config.Shellcheck is true, but compile_pipeline.go also enables shellcheck when config.Validate is true (via config.Shellcheck || config.Validate). When a user runs --validate on a host where neither shellcheck binary nor Docker is available, shellcheck is silently skipped — no warning, no error in strict mode.
The condition should mirror the pipeline:
if (config.Shellcheck || config.Validate) && !config.NoEmit && !isShellcheckAvailable() {@copilot please address this.
There was a problem hiding this comment.
Fixed in 09a6d65 — same as the companion thread above. The condition now mirrors the pipeline: (config.Shellcheck || config.Validate) && !config.NoEmit && !isShellcheckAvailable().
🧪 Test Quality Sentinel Report🔶 Test Quality Score: 30/100 — Needs improvement
📊 Metrics (2 tests)
|
There was a problem hiding this comment.
Verdict: Request changes\n\nBlocking issues remain unresolved from the prior review round — no new inline comments added to avoid duplication.\n\n💡 Blocking themes (already flagged, unresolved)
\n\n- Unbounded goroutine fan-out (shellcheck.go ~L420-441): every run step across all lock files now spawns its own goroutine invoking a subprocess or docker run with no concurrency cap. Large workflow sets can exhaust FDs / overwhelm the Docker daemon. Needs a bounded worker pool (semaphore or errgroup.SetLimit).\n- Concurrent stderr writes can interleave (shellcheck.go ~L436): runShellcheckOnScript/runShellcheckOnScriptViaDocker each write header + body to os.Stderr in separate calls; under the new parallel fan-out, output from different steps can interleave, corrupting diagnostics.\n- No test coverage for the new parallel fan-out/aggregation (shellcheck.go ~L441): existing tests only exercise a single-step lock file; nothing verifies multi-step invocation, error aggregation, or strict-mode fail behavior under concurrency.\n- Breaking CLI change without migration path (main.go ~L778): --no-shellcheck is removed and default behavior inverted (opt-in vs opt-out) with no changeset/migration note, despite repo policy requiring both for breaking CLI changes.\n- --validate help text is stale (main.go ~L778): --validate now also enables shellcheck as a side effect, but its flag description does not mention this, so users cannot anticipate the new linting from --help.\n\nThese should be addressed before merge; none are style nits — they affect resource exhaustion risk, correctness of concurrent output, test coverage of new concurrency logic, and CLI compatibility guarantees.\n\n
💡 Blocking themes (already flagged, unresolved)
\n\n- Unbounded goroutine fan-out (shellcheck.go ~L420-441): every run step across all lock files now spawns its own goroutine invoking a subprocess or docker run with no concurrency cap. Large workflow sets can exhaust FDs / overwhelm the Docker daemon. Needs a bounded worker pool (semaphore or errgroup.SetLimit).\n- Concurrent stderr writes can interleave (shellcheck.go ~L436): runShellcheckOnScript/runShellcheckOnScriptViaDocker each write header + body to os.Stderr in separate calls; under the new parallel fan-out, output from different steps can interleave, corrupting diagnostics.\n- No test coverage for the new parallel fan-out/aggregation (shellcheck.go ~L441): existing tests only exercise a single-step lock file; nothing verifies multi-step invocation, error aggregation, or strict-mode fail behavior under concurrency.\n- Breaking CLI change without migration path (main.go ~L778): --no-shellcheck is removed and default behavior inverted (opt-in vs opt-out) with no changeset/migration note, despite repo policy requiring both for breaking CLI changes.\n- --validate help text is stale (main.go ~L778): --validate now also enables shellcheck as a side effect, but its flag description does not mention this, so users cannot anticipate the new linting from --help.\n\nThese should be addressed before merge; none are style nits — they affect resource exhaustion risk, correctness of concurrent output, test coverage of new concurrency logic, and CLI compatibility guarantees.\n\n🔎 Code quality review by PR Code Quality Reviewer · auto · 79.9 AIC · ⌖ 5.66 AIC · ⊞ 7.9K
Comment /review to run again
|
@copilot run pr-finisher skill |
|
@copilot run pr-finisher skill |
…-shellcheck no-op, multi-step tests, changeset Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. Addressed all 9 unresolved review threads in commit 09a6d65:
Local validation: |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Ran pr-finisher pass and pushed a follow-up fix in |
Fixed in |
|
🎉 This pull request is included in a new release. Release: |
Shellcheck previously ran by default (opt-out via
--no-shellcheck), inconsistent with all other checkers (poutine, zizmor, actionlint, etc.) which are opt-in. This aligns shellcheck with that pattern and adds parallel execution.Changes
CompileConfig:NoShellcheck bool→Shellcheck bool; shellcheck now off by default--no-shellcheckremoved, replaced with--shellcheck;--validatealso enables shellcheckrunShellcheckOnLockFilesnow collects all steps across lock files first, then fans out via goroutines +sync.WaitGroupinstead of sequential per-step loopcompile_orchestrator.go: availability warning/error only emitted when--shellcheckis explicit (not via--validatetransitive enable), keeping MCP compile paths cleanmcp_tools_readonly.go: removed--no-shellchecksuppression,shellcheckUnavailableWarning, andinjectShellcheckUnavailableWarning— no longer needed since shellcheck is opt-in and silently skips when unavailable