Copilot
AI
changed the title
feat: improve shellcheck integration - disabled by default, parallel execution
feat: shellcheck disabled by default, opt-in via --shellcheck/--validate, parallel execution
pelikhan marked this pull request as ready for review
August 3, 2026 01:48Copilot AI review requested due to automatic review settings
August 3, 2026 01:48Pull 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.
| 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
- Files reviewed: 8/8 changed files
- Comments generated: 5
- Review effort level: Balanced
Comment on lines +428 to +430
Comment on lines +434 to +436
| stepErr = runShellcheckOnScriptViaDocker(ctx, s, shellcheckDefaultIgnoreCodes, verbose) | ||
| } else { | ||
| stepErr = runShellcheckOnScript(step, shellcheckDefaultIgnoreCodes, verbose) | ||
| stepErr = runShellcheckOnScript(s, shellcheckDefaultIgnoreCodes, verbose) |
| results[idx] = result{err: stepErr} | ||
| }(i, step) | ||
| } | ||
| wg.Wait() |
| 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)") |
| 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)") |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| 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() { |
| shellcheckLog.Printf("shellcheck complete: steps=%d, issues=%d", len(allSteps), totalIssues) | ||
|
|
||
| if firstErr != nil { | ||
| if strict && firstErr != nil { |
| stepErr = runShellcheckOnScriptViaDocker(ctx, s, shellcheckDefaultIgnoreCodes, verbose) | ||
| } else { | ||
| stepErr = runShellcheckOnScript(step, shellcheckDefaultIgnoreCodes, verbose) | ||
| stepErr = runShellcheckOnScript(s, shellcheckDefaultIgnoreCodes, verbose) |
| 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() { |
🧪 Test Quality Sentinel Report
🔶 Test Quality Score: 30/100 — Needs improvement
📊 Metrics (2 tests)Analyzed 2 test(s): 0 design, 2 implementation, 0 violation(s).
| Metric | Value |
|---|---|
| Analyzed | 2 (Go: 2, JS: 0) |
| ✅ Design | 0 (0%) |
| ⚠️ Implementation | 2 (100%) |
| Edge/error coverage | 0 (0%) |
| Duplicate clusters | 0 |
| Inflation | No |
| 🚨 Violations | 0 |
| Test | File | Classification | Issues |
|---|---|---|---|
TestCompileConfig (struct field addition) |
compile_command_test.go |
implementation_test | Happy-path only; verifies struct field value, not behavior |
TestCompileConfig_DefaultValues (Shellcheck check) |
compile_command_test.go |
implementation_test | Verifies struct zero-value default, not runtime behavior |
| Comment-only change | compile_pipeline_yamllint_test.go |
N/A | — |
TestCompileConfig (compile_command_test.go) — implementation_test. Adds Shellcheck: false to a struct literal. Verifies a compile-time field value with no behavioral assertion. Low value if deleted.
TestCompileConfig_DefaultValues (compile_command_test.go) — implementation_test. Checks that config.Shellcheck is false on a zero-value struct. Confirms Go zero-value semantics, not that the flag correctly disables shellcheck at runtime.
Missing coverage for the core feature: The PR title is "shellcheck disabled by default, opt-in via --shellcheck/--validate, parallel execution". No test verifies: that shellcheck runs when Shellcheck: true is set; that shellcheck is skipped when Shellcheck: false; that --validate flag enables shellcheck; or parallel execution behavior.
Verdict
❌ Failed. 100% implementation tests (threshold: 30%). The core behavioral contracts of the feature (opt-in flag triggers shellcheck, parallel execution) are untested.
🧪 Test quality analysis by Test Quality Sentinel · sonnet46 · 73.1 AIC · ⌖ 8.35 AIC · ⊞ 8.5K · ◷
Comment /review to run again
…-shellcheck no-op, multi-step tests, changeset Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Merged