feat: shellcheck Docker fallback for systems without native binary; re-enable in MCP - #49826
Conversation
…n MCP Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a Docker-based ShellCheck fallback and enables ShellCheck during MCP compilation.
Changes:
- Runs SHA-pinned ShellCheck through Docker when unavailable locally.
- Updates availability checks and MCP compilation flags.
- Extends image constant tests.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/shellcheck.go |
Implements Docker fallback execution. |
pkg/cli/shellcheck_test.go |
Updates unavailable-tool test. |
pkg/cli/mcp_tools_readonly.go |
Enables ShellCheck for MCP compilation. |
pkg/cli/docker_images.go |
Adds the pinned ShellCheck image. |
pkg/cli/docker_images_test.go |
Validates the new image constant. |
pkg/cli/compile_orchestrator.go |
Accounts for Docker availability. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 4
- Review effort level: Balanced
| output := strings.ReplaceAll(stdout.String(), "-:", "script:") | ||
| if stderr.Len() > 0 { | ||
| output += strings.ReplaceAll(stderr.String(), "-:", "script:") | ||
| } | ||
|
|
||
| if output != "" { | ||
| fmt.Fprintf(os.Stderr, "%s\n", console.FormatWarningMessage("shellcheck findings in "+stepLabel(info)+":")) | ||
| fmt.Fprint(os.Stderr, output) | ||
| } | ||
|
|
||
| if err != nil { | ||
| var exitErr *exec.ExitError | ||
| if errors.As(err, &exitErr) { | ||
| if exitErr.ExitCode() == 1 { | ||
| // Exit code 1 means shellcheck found issues; already printed above. | ||
| return fmt.Errorf("shellcheck found issues in %s", stepLabel(info)) | ||
| } | ||
| } | ||
| return fmt.Errorf("shellcheck (docker) failed: %w", err) | ||
| } |
| // Shellcheck output goes to stderr (captured in subprocess error buffer only) and | ||
| // therefore never reaches the LLM via the JSON response — always disable it. | ||
| cmdArgs := []string{"compile", "--validate", "--json", "--no-shellcheck"} | ||
| cmdArgs := []string{"compile", "--validate", "--json"} |
| // This is the fallback path when the shellcheck binary is not installed locally | ||
| // (e.g. on Windows). It mirrors the behaviour of runShellcheckOnScript: findings are | ||
| // printed to stderr and an error is returned when shellcheck reports issues. | ||
| func runShellcheckOnScriptViaDocker(ctx context.Context, info runStepInfo, ignoreCodes []string, verbose bool) error { |
|
|
||
| // Silently skip when shellcheck is not installed. The orchestrator is responsible | ||
| // for warning the user in --validate mode. | ||
| ctx := context.Background() |
|
@copilot Please continue triage on this PR. Remaining visible blockers (newest first):
Failed checks:
Run details: https://github.com/github/gh-aw/actions/runs/30763117443
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based 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 /diagnosing-bugs and /tdd — requesting changes on three correctness and coverage issues.
📋 Key Themes & Highlights
Issues Found
- stderr mixing (correctness) — Docker pull/daemon output unconditionally appended to shellcheck findings, causing false positive warnings on first run (line 358–361,
shellcheck.go) - context propagation (correctness) —
context.Background()used insiderunShellcheckOnLockFiles, breaking cancellation and timeout propagation through Docker operations (line 383,shellcheck.go) - Missing test coverage (test quality) —
runShellcheckOnScriptViaDockerhas no tests; the only new test covers the double-unavailable skip path, leaving all Docker execution paths untested (shellcheck_test.go)
Positive Highlights
- ✅ SHA-pinned image constant with comment explaining fallback purpose
- ✅ Lazy fallback — Docker only invoked when scripts exist and binary is absent
- ✅
#nosec G204with accurate justification - ✅ stdin-based piping avoids temporary files and volume mounts
- ✅ Re-enabling shellcheck in MCP is a clean one-liner removal of
--no-shellcheck
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 28.1 AIC · ⌖ 18.8 AIC · ⊞ 7.1K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/cli/shellcheck.go:360
[/diagnosing-bugs] Docker stderr is unconditionally appended to the findings output (line 358–361), so a clean first-pull run will print misleading "shellcheck findings" warnings containing image-pull progress or daemon diagnostics—even when shellcheck found nothing.
<details>
<summary>💡 Suggestion</summary>
Only include stderr when the command actually failed, or strip it from the success path:
if err != nil && stderr.Len() > 0 {
output += strings.ReplaceAll(stderr.String(), …
</details>
<details><summary>pkg/cli/shellcheck.go:383</summary>
**[/diagnosing-bugs]** `context.Background()` is used for both `IsDockerAvailable` and `runShellcheckOnScriptViaDocker`, disconnecting these operations from the compilation context. If the caller cancels (timeout, Ctrl-C), Docker containers can be left running and the pipeline won't respect cancellation.
<details>
<summary>💡 Suggestion</summary>
Thread the caller's context through `runShellcheckOnLockFiles`:
```go
func runShellcheckOnLockFiles(ctx context.Context, lockFiles []string, verbos…
</details>
<details><summary>pkg/cli/shellcheck_test.go:305</summary>
**[/tdd]** The Docker execution path has no test coverage. The updated test only verifies the "neither available" skip path. The new `runShellcheckOnScriptViaDocker` function has no tests for: stdin/args correctness, shellcheck exit 1 (findings), Docker failure, or context cancellation.
<details>
<summary>💡 Suggested test scaffolding</summary>
Add a fake `docker` wrapper on `PATH` (similar to how other tools are tested with stubs) covering:
- **Success**: fake docker exits 0 → function retu…
</details>
There was a problem hiding this comment.
Review: shellcheck Docker fallback
The approach (SHA-pinned image, lazy fallback, re-enabled in MCP) is solid. Five blocking/notable issues need to be resolved:
| # | Location | Issue |
|---|---|---|
| 1 | shellcheck.go:360 |
Docker stderr (pull output, daemon diagnostics) is concatenated into output, causing false "shellcheck findings" banners |
| 2 | mcp_tools_readonly.go:196 |
Shellcheck findings go to stderr which MCP consumers never see; needs to surface in JSON or be explicitly documented as intentional |
| 3 | shellcheck.go:305 |
runShellcheckOnScriptViaDocker has zero test coverage |
| 4 | shellcheck.go:383 |
context.Background() disconnects Docker calls from compilation cancellation |
| 5 | shellcheck.go:337 |
No user-visible progress during Docker image pull — silent hang risk on first use |
Issues 1–4 were flagged in prior inline comments; issue 5 is new.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 24.8 AIC · ⌖ 10.3 AIC · ⊞ 5.4K
| cmd.Stderr = &stderr | ||
|
|
||
| err := cmd.Run() | ||
|
|
There was a problem hiding this comment.
When Docker is available but koalaman/shellcheck has never been pulled, the first compile will silently block for potentially several minutes while the image downloads. The log line added above (shellcheck binary not found in PATH; using Docker container fallback) fires before any scripts are checked, but there is no user-visible progress indicator during the pull. Consider printing a one-time FormatInfoMessage to stderr such as "Pulling shellcheck Docker image (first use)..." before the first docker run call, or using the existing pullDockerImageIfNotPresent helper used by other tools. Without this, users on slow connections may believe the tool has hung.
@copilot please address this.
🧪 Test Quality Sentinel Report
📊 Metrics (37 tests)
|
There was a problem hiding this comment.
Verdict: Request changes
The Docker fallback mechanics are reasonable, but the MCP re-enable is functionally a no-op and the new Docker code path is untested.
💡 Themes
- Blocking:
mcp_tools_readonly.godrops--no-shellcheckbut the MCP subprocess wrapper (runMCPExecOutput) only captures stdout viacmd.Output(). Shellcheck findings are written to stderr and are therefore still invisible to MCP callers on success — the exact problem the removed comment warned about. This needs an actual fix (merge stderr into the JSON response, or switch to combined output + parse), not just a flag removal. - Non-blocking but real: no test coverage exists for
runShellcheckOnScriptViaDockeror the new fallback branch inrunShellcheckOnLockFiles(arg construction, stdin piping, output parsing, exit-code handling, Docker failure) — only the "neither available" skip path is tested. - Minor:
context.Background()inrunShellcheckOnLockFilesbreaks cancellation propagation for the new Docker calls; the gcc-output-:→script:string rewrite is unanchored and can corrupt finding text containing that substring.
The SHA-pinned image constant, lazy Docker fallback design, and orchestrator pre-gate logic are solid.
🔎 Code quality review by PR Code Quality Reviewer · auto · 62.4 AIC · ⌖ 5.61 AIC · ⊞ 7.9K
Comment /review to run again
| // Shellcheck output goes to stderr (captured in subprocess error buffer only) and | ||
| // therefore never reaches the LLM via the JSON response — always disable it. | ||
| cmdArgs := []string{"compile", "--validate", "--json", "--no-shellcheck"} | ||
| cmdArgs := []string{"compile", "--validate", "--json"} |
There was a problem hiding this comment.
Removing --no-shellcheck does not make shellcheck findings reach MCP callers — this re-enable is a no-op for its stated goal.
💡 Details
runMCPExecOutput (line 249) calls cmd.Output(), which returns stdout only and discards stderr entirely. But runShellcheckOnScript/runShellcheckOnScriptViaDocker write all findings to os.Stderr (shellcheck.go ~268, ~342), never to stdout. On a successful compile invocation (exit 0, non-strict mode), cmd.Output() returns stdout JSON with no error and the stderr shellcheck warnings are silently discarded.
So removing --no-shellcheck re-enables the linter to run, but its findings still never surface to the LLM/MCP caller — the exact problem the removed comment described. Fix by merging shellcheck stderr into the JSON payload or switching to combined output and parsing warnings out separately.
|
|
||
| // Silently skip when shellcheck is not installed. The orchestrator is responsible | ||
| // for warning the user in --validate mode. | ||
| ctx := context.Background() |
There was a problem hiding this comment.
context.Background() here severs cancellation for the new Docker path from the caller's context — a slow/hung docker run or availability check can't be cancelled.
💡 Details
runShellcheckOnLockFiles creates its own root context instead of accepting one from the caller. Both IsDockerAvailable(ctx) and each runShellcheckOnScriptViaDocker(ctx, ...) invocation use exec.CommandContext, which is specifically designed to be cancelled — but with context.Background() that mechanism is dead code for this call path. In the MCP tool flow (mcp_tools_readonly.go), the outer request context is already checked for cancellation elsewhere (ctx.Done() after Docker image prep), so this inconsistency means a hung shellcheck-in-Docker invocation cannot be aborted even though the surrounding request infrastructure supports it.
// current
func runShellcheckOnLockFiles(lockFiles []string, verbose bool, strict bool) error {
...
ctx := context.Background()
// suggested
func runShellcheckOnLockFiles(ctx context.Context, lockFiles []string, verbose bool, strict bool) error {
...| "--rm", | ||
| "-i", | ||
| ShellcheckImage, | ||
| "--shell=" + shellcheckShell(info.Shell), |
There was a problem hiding this comment.
The new Docker execution path (runShellcheckOnScriptViaDocker) has zero test coverage — only the "neither binary nor Docker available" skip branch is exercised.
💡 Details
shellcheck_test.go was updated only to widen the existing skip test (PATH="" now also hides Docker). Nothing exercises: correct docker run argument construction (image, --shell=, --exclude= codes, stdin -), the stdin piping of the sanitized script, gcc-format output parsing/-:→script: rewriting, ShellCheck exit code 1 vs. other failures, or Docker invocation failure (e.g., daemon disappearing mid-run). A fake docker executable on PATH (similar to how other Docker-dependent tests in this repo mock docker) would let these branches be verified without a real container runtime.
| // In gcc format with stdin, shellcheck prefixes findings with "-:LINE:COL: ...". | ||
| // Replace the leading "-:" (stdin indicator) with "script:" so that reported | ||
| // positions are clearly relative to the run: script snippet. | ||
| output := strings.ReplaceAll(stdout.String(), "-:", "script:") |
There was a problem hiding this comment.
Naive strings.ReplaceAll(stdout, "-:", "script:") can corrupt finding text, not just the stdin path prefix.
💡 Details
This blindly rewrites every occurrence of the literal substring "-:" anywhere in shellcheck's gcc-format output, not just the leading stdin-indicator prefix at the start of each finding line. If a shellcheck message or the quoted offending code fragment happens to contain -: (e.g. note: -: unexpected token style text, or a flag like curl -: something echoed in context), it will also be rewritten to script:, producing garbled/misleading output. A safer approach anchors the replacement to line starts, e.g. a regex like ^-: per line or strings.Replace bounded to the known prefix pattern ("-:" + lineNum + ":").
Note: the pre-existing binary path (runShellcheckOnScript) has an analogous issue with tmpFile.Name() replacement, but that's out of scope for this PR — flagging only the newly introduced Docker variant here since it's part of the diff.
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (112 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in I triaged the Windows Integration failure and reviewer blockers, then pushed fixes for:
Local validation passed via Maintainer handoff: CI on this new head is stale in cloud-agent runs and must be re-triggered by a maintainer (close/reopen PR, workflow_dispatch, or maintainer push) before merge. |
|
@copilot Please continue triage on this PR. Remaining visible blockers (newest first):
Failed checks:
Run details: https://github.com/github/gh-aw/actions/runs/30765233636
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in I triaged run Local validation run:
|
|
@copilot merge main and fix https://github.com/github/gh-aw/actions/runs/30765569404/job/91544697896 |
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot resolve the merge conflicts on this branch. |
Addressed in I continued triage and inspected both failed jobs at log level:
Local validation completed:
Maintainer handoff: CI must be re-triggered by a maintainer (close/reopen PR, workflow_dispatch, or maintainer push) to verify the new head in GitHub Actions. |
|
@copilot resolve the merge conflicts on this branch. |
|
@copilot Please continue triage on this PR. Remaining visible blockers (newest first):
Run details: https://github.com/github/gh-aw/actions/runs/30767332278
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Resolved in
Done in |
Resolved. The merge conflicts on this branch were fixed in
Triage complete: the branch is no longer in merge-conflict state (resolved in |
|
🎉 This pull request is included in a new release. Release: |
shellcheck was silently skipped on systems without the native binary (Windows) and was explicitly disabled in MCP compilation. This adds a lazy Docker container fallback and re-enables shellcheck in MCP.
Changes
docker_images.go— SHA-pinnedShellcheckImage(koalaman/shellcheck:v0.10.0) alongside all other container pinsshellcheck.go—runShellcheckOnScriptViaDocker()pipes scripts to the container via stdin;runShellcheckOnLockFiles()uses Docker only when the binary is absent and Docker is available (lazy — no pull until there are bash/sh steps to lint)compile_orchestrator.go— Pre-gate strict/validate warning now fires only when both binary and Docker are unavailablemcp_tools_readonly.go— Removed--no-shellcheck; shellcheck now runs during MCP compilation via binary or Docker fallbackdocker_images_test.go— AddedShellcheckImageto the constants testFallback precedence