Add grype container image vulnerability scanning to compile pipeline#47474
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
Adds Docker-based Grype vulnerability scanning to the workflow compilation pipeline.
Changes:
- Adds the
--grypeCLI and MCP option. - Extracts and deduplicates manifest container images for scanning.
- Adds Grype integration and unit tests.
Show a summary per file
| File | Description |
|---|---|
cmd/gh-aw/main.go |
Registers and configures --grype. |
pkg/cli/compile_config.go |
Adds Grype compilation configuration. |
pkg/cli/compile_external_tools.go |
Exposes the batch Grype runner. |
pkg/cli/compile_pipeline.go |
Runs Grype after compilation. |
pkg/cli/docker_images.go |
Adds Grype Docker image preparation. |
pkg/cli/docker_images_test.go |
Updates image-preparation tests. |
pkg/cli/grype.go |
Implements image collection, scanning, caching, and rendering. |
pkg/cli/grype_test.go |
Tests collection, caching, and findings. |
pkg/cli/mcp_tools_readonly.go |
Wires Grype into MCP compilation. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 9/9 changed files
- Comments generated: 5
- Review effort level: Medium
| if runErr != nil { | ||
| var exitErr *exec.ExitError | ||
| if !errors.As(runErr, &exitErr) { | ||
| // Command could not be started (e.g., Docker not found). | ||
| scanErr := fmt.Errorf("grype failed: %w", runErr) | ||
| grypeScanResultCache.setError(imageRef, scanErr) | ||
| return nil, scanErr | ||
| } | ||
| exitCode := exitErr.ExitCode() | ||
| // Exit code 1 means grype found vulnerabilities — that is expected and parseable. | ||
| // Any other non-zero code signals a real tool failure. | ||
| if exitCode != 1 || (parseErr != nil && stdout.Len() == 0) { | ||
| stderrStr := strings.TrimSpace(stderr.String()) | ||
| if stderrStr != "" { | ||
| grypeLog.Printf("grype stderr for %s: %s", imageRef, stderrStr) | ||
| } | ||
| scanErr := fmt.Errorf("grype failed with exit code %d on %s", exitCode, imageRef) | ||
| grypeScanResultCache.setError(imageRef, scanErr) | ||
| return nil, scanErr | ||
| } | ||
| // Exit code 1 with JSON output — vulnerability findings were returned normally. | ||
| } |
| cmd := exec.Command( | ||
| "docker", | ||
| "run", | ||
| "--rm", | ||
| GrypeImage, |
| if args.Grype { | ||
| cmdArgs = append(cmdArgs, "--grype") | ||
| } |
| content, err := os.ReadFile(lockFile) | ||
| if err != nil { | ||
| grypeLog.Printf("Skipping %s: failed to read file: %v", lockFile, err) | ||
| continue |
| // We can't easily test the errorType without capturing stderr, | ||
| // but we can verify the function returns the right count. | ||
| count := grypeDisplayFindings("test-image:latest", output) |
🧪 Test Quality Sentinel Report
📊 Metrics (44 tests)
📋 Test ClassificationDocker Images (27 tests)Design tests (behavioral contracts):
Concurrency/Race tests (6 tests):
Implementation/State tests (10 tests):
Grype (17 tests)Design tests (data contracts):
State tests (3 tests):
Output tests (4 tests):
✅ Verdict
Guideline Status:
Notes:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: grype container image vulnerability scanning
Overall the implementation is well-structured — the cache, deduplication by pinned digest, and integration pattern matching other scanners are all solid. Two issues need addressing before merge:
Blocking:
- Logic bug (
pkg/cli/grype.go:277): The condition(parseErr != nil && stdout.Len() == 0)is dead code —parseErrcan only be non-nil whenstdout.Len() > 0. Fix:if exitCode != 1 || parseErr != nil { - Unpinned Docker Hub image (
pkg/cli/docker_images.go:36):anchore/grype:latestuses Docker Hub with a mutable:latesttag, unlike every other scanner image which usesghcr.io. This is a supply-chain risk. Pin to a specific version or digest.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 46.8 AIC · ⌖ 4.7 AIC · ⊞ 5K
| exitCode := exitErr.ExitCode() | ||
| // Exit code 1 means grype found vulnerabilities — that is expected and parseable. | ||
| // Any other non-zero code signals a real tool failure. | ||
| if exitCode != 1 || (parseErr != nil && stdout.Len() == 0) { |
There was a problem hiding this comment.
The condition (parseErr != nil && stdout.Len() == 0) is dead code and will never be true. parseErr is only assigned inside the stdout.Len() > 0 block (line 262), so stdout.Len() == 0 is always false whenever parseErr != nil.
The intended guard — reject exit code 1 when no parseable JSON was returned — should be:
if exitCode != 1 || parseErr != nil {This correctly treats exit code 1 with valid JSON as "vulnerabilities found" and exit code 1 without parseable JSON as a tool failure.
@copilot please address this.
| PoutineImage = "ghcr.io/boostsecurityio/poutine:latest" | ||
| ActionlintImage = "rhysd/actionlint:1.7.12" | ||
| RunnerGuardImage = "ghcr.io/vigilant-llc/runner-guard:latest" | ||
| GrypeImage = "anchore/grype:latest" |
There was a problem hiding this comment.
Using anchore/grype:latest pulls from Docker Hub with an unpinned mutable tag. All other scanner images in this file use ghcr.io registries (ghcr.io/zizmorcore/zizmor:latest, ghcr.io/boostsecurityio/poutine:latest, etc.). Docker Hub images are subject to rate-limiting and are a supply-chain risk (mutable tag, different trust boundary).
Consider:
- Using the
ghcr.iomirror if one exists for grype, or pin to a specific version tag (e.g.,anchore/grype:v0.85.0) to avoid drift. - Pinning the digest (e.g.,
anchore/grype:v0.85.0@sha256:...) for full immutability, consistent with how workflow containers are pinned.
@copilot please address this.
…y scanning Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (736 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
|
|
@copilot update static analysis agentic workflow |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design, /diagnosing-bugs, and /tdd — requesting changes on three blocking issues.
📋 Key Themes & Highlights
Blocking Issues
- No context propagation —
grypeRunOnImagedoes not accept acontext.Context, so a hung Docker call cannot be cancelled by the compile pipeline. Useexec.CommandContext. - Floating
latesttag —GrypeImage = "anchore/grype:latest"breaks the reproducibility guarantee; all peer tools pin specific versions. - Fragile exit-code guard — The condition
exitCode != 1 || (parseErr != nil && stdout.Len() == 0)conflates two failure modes and will misclassify grype exit code 2 (future policy-threshold exit).
Notable Observations
- Global cache (
grypeScanResultCache) requiresreset()calls in tests to avoid pollution — injecting the cache as a parameter would be cleaner. - Strict-mode test coverage —
runGrypeOnLockFileslacks tests for the strict + findings and strict + scan-error paths. - Misleading error params —
paramsListappendsgrype: falseeven when grype is requested; inherited pattern but worth fixing.
Positive Highlights
- ✅ Excellent deduplication by pinned digest before scanning
- ✅ Clean severity → error-type mapping using existing
CompilerErrorformat - ✅ Good use of the manifest extraction pipeline for image discovery
- ✅ 15 focused unit tests for the cache, collection, and display helpers
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 69.2 AIC · ⌖ 5.25 AIC · ⊞ 6.7K
Comment /matt to run again
|
|
||
| // grypeRunOnImage runs grype on a single container image reference via Docker, | ||
| // using the result cache to avoid re-scanning images already checked in this run. | ||
| func grypeRunOnImage(imageRef string, verbose bool) (*grypeOutput, error) { |
There was a problem hiding this comment.
[/codebase-design] grypeRunOnImage accepts no context.Context, so long-running Docker scans cannot be cancelled when the compile pipeline context is cancelled (e.g. Ctrl+C or a timeout).
💡 Suggested fix
Add a context parameter and switch to exec.CommandContext:
func grypeRunOnImage(ctx context.Context, imageRef string, verbose bool) (*grypeOutput, error) {
cmd := exec.CommandContext(ctx, "docker", "run", "--rm", GrypeImage, imageRef, "-o", "json")
// ...
}The pipeline already checks ctx.Err() before calling this function, but a Docker call that hangs inside the function will still block the process indefinitely.
@copilot please address this.
| exitCode := exitErr.ExitCode() | ||
| // Exit code 1 means grype found vulnerabilities — that is expected and parseable. | ||
| // Any other non-zero code signals a real tool failure. | ||
| if exitCode != 1 || (parseErr != nil && stdout.Len() == 0) { |
There was a problem hiding this comment.
[/diagnosing-bugs] The exit-code guard exitCode != 1 || (parseErr != nil && stdout.Len() == 0) conflates two distinct failure modes and has an off-by-one risk: if grype ever returns exit code 2 for a policy threshold it will be incorrectly treated as a tool failure, silently swallowing JSON output that was already parsed into output.
💡 Suggested approach
Separate the two concerns explicitly:
if exitCode == 1 && stdout.Len() > 0 {
// Grype found vulnerabilities — expected. Parse below.
} else if exitCode != 0 {
// Genuine tool failure.
scanErr := fmt.Errorf("grype exited %d for %s: %s", exitCode, imageRef, stderr.String())
grypeScanResultCache.setError(imageRef, scanErr)
return nil, scanErr
}Also add a test that simulates a non-zero, non-1 exit code with JSON on stdout to confirm the behavior you intend.
@copilot please address this.
| PoutineImage = "ghcr.io/boostsecurityio/poutine:latest" | ||
| ActionlintImage = "rhysd/actionlint:1.7.12" | ||
| RunnerGuardImage = "ghcr.io/vigilant-llc/runner-guard:latest" | ||
| GrypeImage = "anchore/grype:latest" |
There was a problem hiding this comment.
[/codebase-design] GrypeImage = "anchore/grype:latest" uses a floating latest tag. All other tool images in this file are pinned to specific versions (e.g. rhysd/actionlint:1.7.12). Using latest makes scan results non-reproducible and breaks the digest-pinning guarantee that runner-guard enforces on compiled workflows.
💡 Suggested fix
Pin to a specific release, e.g.:
GrypeImage = "anchore/grype:v0.87.0"Check the latest release at https://github.com/anchore/grype/releases and update TestDockerImageConstants to assert the tag format matches.
@copilot please address this.
| } | ||
|
|
||
| // grypeScanResultCache is the process-wide grype result cache. | ||
| var grypeScanResultCache = &grypeCache{ |
There was a problem hiding this comment.
[/codebase-design] grypeScanResultCache is a package-level variable, which means it persists across test runs and can cause cross-test pollution even though reset() is provided. Tests that call reset() directly are a workaround for a design issue.
💡 Suggested fix
Accept the cache as a parameter or inject it via the runner function, keeping the global only as a default:
func grypeRunOnImage(imageRef string, verbose bool, cache *grypeCache) (*grypeOutput, error) { ... }This makes the dependency explicit and allows tests to pass a fresh cache without relying on reset(). The public wrapper RunGrypeOnLockFiles can still use grypeScanResultCache as the default.
@copilot please address this.
| } | ||
| if useGrype { | ||
| tool := "grype" | ||
| requestedTools = append(requestedTools, tool) |
There was a problem hiding this comment.
[/diagnosing-bugs] The Docker-unavailable error message for grype unconditionally appends "grype: false" — copied from the pattern for the other tools but the string is misleading (grype is requested, not disabled).
💡 Details
The bug is in the newly added block:
if useGrype {
tool := "grype"
requestedTools = append(requestedTools, tool)
paramsList = append(paramsList, tool+": false") // ← should be tool+": true"
}Every other tool appends tool+": false" as well, so this is a pre-existing pattern issue — but it produces a confusing error message (grype: false) for a user who explicitly passed --grype. Consider fixing or at minimum add a comment explaining why.
@copilot please address this.
| // vulnerability scanner on each unique image via Docker. | ||
| func runGrypeOnLockFiles(lockFiles []string, verbose bool, strict bool) error { | ||
| if len(lockFiles) == 0 { | ||
| return nil |
There was a problem hiding this comment.
[/tdd] runGrypeOnLockFiles is not tested under strict mode with actual findings — only the no-image and empty-lock-file paths are covered. There are no tests that verify the strict-mode early return when totalFindings > 0 or when len(scanErrors) > 0.
💡 Suggested tests to add
func TestRunGrypeOnLockFiles_StrictMode_ReturnsErrorOnFindings(t *testing.T) { ... }
func TestRunGrypeOnLockFiles_StrictMode_ReturnsErrorOnScanFailure(t *testing.T) { ... }
func TestRunGrypeOnLockFiles_NonStrictMode_WarnsOnScanFailure(t *testing.T) { ... }These are the paths most likely to break silently in production.
@copilot please address this.
There was a problem hiding this comment.
REQUEST_CHANGES — Three correctness bugs in the grype output parser make this scanner unreliable as a security gate.
🔴 Blocking issues (must fix)
-
Cache error lifetime (
grype.go:174): Transient scan errors are permanently cached at process scope. MCP users and repeated compile runs will hit silent no-ops after any Docker blip — no retry ever fires. The "per compile run" caching documented in the header comment is only true for successful results. -
Silent empty result on non-JSON stdout (
grype.go:226): When grype exits 1 but emits a non-JSON error string (image pull failure, bad arg, etc.), theHasPrefix("{")guard skips parsing,parseErrstays nil, and the code returns a zero-finding success. A compromised or unavailable image is silently reported as clean. -
Fragile JSON parse guard (
grype.go:218): TheHasPrefix("{")+ separateUnmarshal(stdout.Bytes())split is ambiguous — BOM, whitespace trimming, and the separateparseErrvariable make the control flow hard to reason about. Usejson.NewDecoderdirectly.
🟡 Non-blocking but should fix
GrypeImage = "anchore/grype:latest"(docker_images.go:142): Unpinned tag makes scan results non-reproducible and may break the JSON schema silently. Every other tool image in this file is pinned.- Error messages log
img.Imageinstead ofimageRef(grype.go:155): Scan failures name the short tag, not the pinned digest that was actually scanned, making them impossible to reproduce.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 74.4 AIC · ⌖ 5.35 AIC · ⊞ 5.7K
Comment /review to run again
Comments that could not be inline-anchored
pkg/cli/grype.go:174
Process-wide scan error cache never resets between compile runs: a transient Docker failure permanently poisons the cache for that image ref, silently skipping all future scans in the same process.
<details>
<summary>💡 Details and suggested fix</summary>
grypeScanResultCache is a package-level singleton. Its setError path caches scan failures permanently for the process lifetime. When --grype is invoked multiple times within the same process (e.g., via the MCP tool), a transient fa…
pkg/cli/grype.go:226
Silent empty result when grype exits 1 with non-JSON stdout: if grype exits with code 1 but outputs a warning string instead of JSON (e.g., image pull failure message), the HasPrefix("{") guard silently skips parsing and returns zero findings — masking the real failure.
<details>
<summary>💡 Details and suggested fix</summary>
The current logic:
if stdout.Len() > 0 && strings.HasPrefix(strings.TrimSpace(stdout.String()), "{") {
parseErr = json.Unmarshal(stdout.Bytes(), &outpu…
</details>
<details><summary>pkg/cli/grype.go:218</summary>
**`HasPrefix("{")` JSON detection is fragile**: a BOM, leading whitespace, or any prefix character causes the parse to be silently skipped, returning zero findings instead of an error.
<details>
<summary>💡 Details and suggested fix</summary>
```go
if stdout.Len() > 0 && strings.HasPrefix(strings.TrimSpace(stdout.String()), "{") {
parseErr = json.Unmarshal(stdout.Bytes(), &output)
}TrimSpace is applied to the string for the prefix check, but stdout.Bytes() (the untrimmed origina…
pkg/cli/grype.go:155
Scan error messages use the original image tag, not the pinned digest actually scanned: when debugging a scan failure, the error log names img.Image (e.g., ubuntu:20.04) but grype was invoked with imageRef (e.g., ubuntu:20.04@sha256:abc...), making it impossible to reproduce the exact invocation from the error message alone.
<details>
<summary>💡 Suggested fix</summary>
// Change:
scanErrors = append(scanErrors, fmt.Sprintf("%s: %v", img.Image, err))
// To:
scanErrors = appe…
</details>
<details><summary>pkg/cli/docker_images.go:142</summary>
**`GrypeImage = "anchore/grype:latest"` is unpinned, unlike every other tool image**: this makes scan results non-reproducible across runs as the grype database and binary silently update, and breaks the consistency guarantee users expect from a security scanner.
<details>
<summary>💡 Details</summary>
Compare to the other tool images:
- `ActionlintImage = "rhysd/actionlint:1.7.12"` — pinned
- `RunnerGuardImage = "ghcr.io/vigilant-llc/runner-guard:latest"` — also unpinned (pre-existing issue)…
</details>
|
Great work on the grype integration! 🔍 This PR adds container image vulnerability scanning to the compile pipeline with excellent coverage: ✅ Comprehensive implementation — The grype feature integrates cleanly with the existing compile pipeline ( ✅ Well-tested — 15 new unit tests cover cache behavior, image collection, deduplication, and edge cases. Tests for the Docker integration updated to match the new signature. ✅ Clear documentation — The PR body explains the design (image extraction, Docker runs, severity mapping) and provides usage examples. ✅ Focused change — All modifications serve the single goal of adding grype scanning. No unrelated changes. This looks ready for review!
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Updated in commit |
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Ran a pr-finisher pass and pushed |
Fixed by commit |
|
🎉 This pull request is included in a new release. Release: |
Adds
--grypeto the compile pipeline (alongside--zizmor,--poutine,--actionlint,--runner-guard) to scan container images referenced in compiled workflows for known CVEs usinganchore/grypevia Docker — no native install required.How it works
gh-aw-manifestheader embedded in each.lock.ymlPinnedImage(digest-pinned ref) before scanning — one Docker run per unique image even across many workflowsgrype <image> -o jsonoutput is parsed and normalized toconsole.CompilerErrorwith severity mapping: Critical/High → error, Medium/Unknown → warning, Low/Negligible → infoChanges
pkg/cli/grype.go— new:collectContainerImagesFromLockFiles,runGrypeOnLockFiles,grypeRunOnImage,grypeDisplayFindings, thread-safegrypeCachepkg/cli/docker_images.go—GrypeImage = "anchore/grype:latest"constant;CheckAndPrepareDockerImagesgains 5thuseGrype boolparampkg/cli/compile_pipeline.go— grype block added to bothcompileSpecificFilesandcompileAllFilesInDirectoryafter runner-guardpkg/cli/compile_config.go,compile_external_tools.go,mcp_tools_readonly.go,cmd/gh-aw/main.go—Grype boolfield and--grypeflag wired end-to-endpkg/cli/grype_test.go— 15 unit tests: cache behavior, image collection, deduplication, severity mapping, edge casespkg/cli/docker_images_test.go— updated all call sites for new signature;GrypeImageadded to constants testUsage