feat: shellcheck linting phase for run steps in compile pipeline - #49762
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot --shellcheck should always be enabled by default if available. Add --no-shellcheck flag and flag it as error in strict mode. |
…t mode when unavailable - Rename `Shellcheck bool` to `NoShellcheck bool` in CompileConfig; shellcheck now runs by default when available - Replace `--shellcheck` CLI flag with `--no-shellcheck` to explicitly opt out - In strict mode, return an error if shellcheck is not installed; in validate-only mode, emit a warning as before - Update MCP compile tool schema: `shellcheck` → `no-shellcheck` field - Fix yamllint test helper to set NoShellcheck=true so it stays independent of shellcheck behavior Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. Changes in commit
|
PR Triage
Automated triage — see full report issue for details. Structured data: {
"action": "defer",
"category": "feature",
"pr_number": 49762,
"risk": "medium"
}
|
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
There was a problem hiding this comment.
Pull request overview
Adds shellcheck linting for compiled workflow run: steps, including CLI/MCP wiring and tests.
Changes:
- Extracts shell scripts from lock files and invokes system shellcheck.
- Integrates linting into both compilation paths.
- Adds configuration, CLI/MCP options, and unit tests.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/shellcheck.go |
Implements extraction and shellcheck execution. |
pkg/cli/shellcheck_test.go |
Tests extraction and shell handling. |
pkg/cli/mcp_tools_readonly.go |
Adds MCP opt-out parameter. |
pkg/cli/compile_pipeline.go |
Runs shellcheck after compilation. |
pkg/cli/compile_pipeline_yamllint_test.go |
Isolates yamllint tests. |
pkg/cli/compile_orchestrator.go |
Checks shellcheck availability. |
pkg/cli/compile_external_tools.go |
Exposes the shellcheck wrapper. |
pkg/cli/compile_config.go |
Adds shellcheck configuration. |
pkg/cli/compile_command_test.go |
Tests configuration defaults. |
cmd/gh-aw/main.go |
Registers the CLI opt-out flag. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 10/10 changed files
- Comments generated: 5
- Review effort level: Balanced
| compileCmd.Flags().Bool("grype", false, "Run grype vulnerability scanner on container images referenced in compiled .lock.yml files (uses Docker image "+cli.GrypeImage+")") | ||
| 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)") |
There was a problem hiding this comment.
The maintainer explicitly requested shellcheck to be default-on with --no-shellcheck as the opt-out (see @pelikhan's comment: "shellcheck should always be enabled by default if available. Add --no-shellcheck flag"). The --no-shellcheck flag at cmd/gh-aw/main.go:778 and the NoShellcheck bool field in CompileConfig implement exactly that: shellcheck runs automatically when available, with --no-shellcheck to disable it. This is an intentional design change from the original opt-in proposal.
| } | ||
|
|
||
| // Warn or error when shellcheck is enabled (the default) but not installed. | ||
| if !config.NoShellcheck && !isShellcheckAvailable() { |
| shell, _ := step["shell"].(string) | ||
| if !isShellcheckableShell(shell) { |
| for _, code := range ignoreCodes { | ||
| args = append(args, "--exclude="+code) | ||
| } | ||
| args = append(args, tmpFile.Name()) |
There was a problem hiding this comment.
Addressed in a previous commit by adding sanitizeGHAExpressions() which replaces every ${{ ... }} expression with the identifier __GHA_EXPR__ before passing the script to shellcheck. This prevents SC1073/SC1083 parse errors while keeping the script structurally valid shell. SC2016 is retained as a safety net for any edge cases the regex might miss.
| // Display findings – replace the temp file path with the lock file for clarity. | ||
| output := strings.ReplaceAll(stdout.String(), tmpFile.Name(), filepath.Base(info.LockFile)) | ||
| if stderr.Len() > 0 { | ||
| output += strings.ReplaceAll(stderr.String(), tmpFile.Name(), filepath.Base(info.LockFile)) |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 85/100 — Excellent
📊 Metrics (7 tests)
Verdict
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…ipeline Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@copilot resolve the merge conflicts on this branch. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…d playwright-cli line) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Resolved. Merged |
…e-with-linting Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
The branch is up to date with |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in commit The fix: always pass Changes in
|
|
@copilot Please continue triage on this PR. Remaining reviewer-visible blockers:
Run details: https://github.com/github/gh-aw/actions/runs/30758413653
|
…r-code-quality-reviewer Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…reviewer Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot Please continue triage on this PR. Remaining reviewer-visible blockers:
Branch refresh was requested for this PR. Run details: https://github.com/github/gh-aw/actions/runs/30759528407
|
Both failures in that run are fixed in commits
Both pass locally. CI needs a re-trigger to confirm on the new HEAD. |
Adds
--shellchecktogh aw compileto run shellcheck onrun:step scripts extracted from compiled lock files.What changes
pkg/cli/shellcheck.go— new file with core integration: extracts bash/shrun:steps from lock file YAML, writes each script to a temp file, invokesshellcheck --format=gcc, and suppresses known GitHub Actions false positives by defaultpkg/cli/compile_config.go— addsShellcheck boolfieldpkg/cli/compile_external_tools.go— addsRunShellcheckOnLockFilespublic wrapperpkg/cli/compile_pipeline.go— wires shellcheck into bothcompileSpecificFilesandcompileAllFilesInDirectoryafter compilationpkg/cli/compile_orchestrator.go— warns when--shellcheck --validateis set but the shellcheck binary is not in PATHcmd/gh-aw/main.go— registers--shellcheckCLI flagpkg/cli/mcp_tools_readonly.go— exposesshellcheckin MCP compile tool schemapkg/cli/shellcheck_test.go— unit tests for shell filter, YAML extraction, step label, ignore codes, and binary-unavailable skipKey behaviours
pwsh,python,cmd, etc.shellcheckbinary (no Docker); silently skips when unavailable--validatemode, emits a warning if shellcheck is not in PATHSC2016—${{ }}expressions in single-quoted stringsSC1090/SC1091— dynamically sourced scripts not resolvable at lint timeRun: https://github.com/github/gh-aw/actions/runs/30750494960
run: https://github.com/github/gh-aw/actions/runs/30751969165