Add repository-level grype ignore policy for no-fix libc6 CVEs - #52924
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
|
✅ PR Code Quality Reviewer completed the code quality review. Reviewed PR #52924 and found no actionable changed-line issues requiring GitHub write comments; submitting no-op for required safe output.
|
|
✅ Ponytail Reviewer completed successfully! Ponytail review: no over-engineering found. The change mirrors the existing .grant.yaml pattern (grantPolicyFile/buildDockerReadonlyFileMount), reuses existing mount-validation helpers, and the config data (.grype.yaml) is intentionally scoped per-CVE rather than duplicated logic. Lean already. Ship.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overall the changes are well-structured and follow the established grant pattern. The .grype.yaml config, documentation, and tests are all consistent.
One non-blocking concern was flagged inline: the scan-result cache keys only on imageRef while the result now also depends on configFile. This is safe in the current single-config-per-run flow, but it is a latent bug if the cache is ever reused across different config contexts.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 28.7 AIC · ⌖ 8 AIC · ⊞ 5.6K
| // and passed to grype via --config. | ||
| func grypeRunOnImage(imageRef, configFile string, verbose bool) (*grypeOutput, error) { | ||
| // Check cache first. | ||
| if result, err, ok := grypeScanResultCache.get(imageRef); ok { |
There was a problem hiding this comment.
The cache in grypeRunOnImage keys on imageRef alone, but the scan result now also depends on configFile. If two calls in the same process use different configs for the same image reference, the second call returns the first call's cached result, silently ignoring the different config.
In the current wiring this is safe (config is resolved once per compile run), but it is a latent bug worth guarding against.
Suggested fix: include the config path in the cache key, e.g. cacheKey := imageRef + "|" + configFile.
@copilot please address this.
…CVEs Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (126 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
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — requesting changes on a correctness issue with the result cache and a fragile environment-coupled test.
📋 Key Themes & Highlights
Key Themes
- Cache correctness: The scan result cache key is
imageRefalone, but results now vary byconfigFile. This is low-risk today (config is resolved once per run and passed identically to allgrypeRunOnImagecalls), but is a latent correctness bug for tests and future callers. - Environment-coupled test:
TestGrypeConfigFileResolvesRepositoryPolicyassumes the test is executed from within the gh-aw git checkout. It should skip gracefully when that assumption doesn't hold. - Unnecessary runtime validation of a constant:
validateContainerMountPath(grypeContainerConfigPath)is called on a fixed constant on every invocation; the error branch is unreachable.
Positive Highlights
- ✅ Excellent refactor:
grypeDockerArgsis cleanly testable and mirrors the pattern ingrant.go. - ✅ Risk-acceptance rules are tightly scoped (specific CVE ID + package type) so new vulnerabilities aren't silently suppressed.
- ✅ Documentation in
CONTRIBUTING.mdis actionable — it explains when to add and when to remove a rule. - ✅ Read-only volume mount reuses the existing validated
buildDockerReadonlyFileMounthelper.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 45.8 AIC · ⌖ 8.78 AIC · ⊞ 7.7K
Comment /matt to run again
| // and passed to grype via --config. | ||
| func grypeRunOnImage(imageRef, configFile string, verbose bool) (*grypeOutput, error) { | ||
| // Check cache first. | ||
| if result, err, ok := grypeScanResultCache.get(imageRef); ok { |
There was a problem hiding this comment.
[/codebase-design] Cache key is imageRef alone, but scan results now vary by configFile too. A result cached without a config will be returned for a subsequent call with a config (or vice-versa) in the same process.
💡 Suggested fix
Include the config path in the cache key:
cacheKey := imageRef
if configFile != "" {
cacheKey = imageRef + "\x00" + configFile
}
if result, err, ok := grypeScanResultCache.get(cacheKey); ok {
...
}
// and store with the same key
grypeScanResultCache.set(cacheKey, &output)This is low-risk in production (config is resolved once per compile run), but it is a correctness issue for tests and future callers.
@copilot please address this.
| info, err := os.Stat(configFile) | ||
| if err != nil || !info.Mode().IsRegular() { | ||
| grypeLog.Printf("No grype config found at %s", configFile) | ||
| return "" |
There was a problem hiding this comment.
[/codebase-design] validateContainerMountPath is called on grypeContainerConfigPath (a compile-time constant) on every invocation. This validation can never fail and adds dead-error-path noise. Consider validating it once (e.g., in a TestMain or init guard) or asserting it at compile time with a constant-fold check.
💡 Detail
grypeContainerConfigPath = "/tmp/gh-aw-grype-config.yaml" is a fixed constant that cannot change at runtime. validateContainerMountPath will always succeed for it, so the error branch at line 262 can never be reached. Either remove the runtime call and use the constant directly (trusting the existing test TestGrypeDockerArgs_WithConfig), or add a package-level init() assertion:
func init() {
if _, err := validateContainerMountPath(grypeContainerConfigPath); err != nil {
panic("invalid grypeContainerConfigPath constant: " + err.Error())
}
}@copilot please address this.
| func TestGrypeConfigFileResolvesRepositoryPolicy(t *testing.T) { | ||
| configFile := grypeConfigFile() | ||
| if configFile == "" { | ||
| t.Fatal("Expected repository grype config to be found") |
There was a problem hiding this comment.
[/tdd] TestGrypeConfigFileResolvesRepositoryPolicy is an environment-coupled test: it passes only when the test is run from inside the gh-aw git checkout. In a different working directory (e.g., a temp dir or a CI runner that checks out to an unusual path) grypeConfigFile() may return "" and the test fails with a misleading message.
💡 Suggestion
Either skip the test when not in a git checkout, or use a table-driven approach with a synthetic git root:
func TestGrypeConfigFileResolvesRepositoryPolicy(t *testing.T) {
if _, err := gitutil.FindGitRoot(); err != nil {
t.Skip("not inside a git checkout")
}
configFile := grypeConfigFile()
if configFile == "" {
t.Fatal("Expected repository grype config to be found")
}
...
}This makes the test self-documenting about its environmental requirement and avoids false failures.
@copilot please address this.
There was a problem hiding this comment.
Pull request overview
Adds repository-level Grype risk-acceptance rules for unpatched libc6 CVEs.
Changes:
- Loads optional
.grype.yamlpolicies during container scans. - Adds targeted CVE exceptions and documentation.
- Adds argument/config-resolution tests.
Show a summary per file
| File | Description |
|---|---|
.grype.yaml |
Defines vulnerability exceptions. |
pkg/cli/grype.go |
Loads and mounts Grype configuration. |
pkg/cli/grype_test.go |
Tests configuration and Docker arguments. |
docs/src/content/docs/setup/cli.md |
Documents --grype configuration behavior. |
CONTRIBUTING.md |
Documents exception governance. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Balanced
| - vulnerability: CVE-2026-5450 | ||
| reason: "Debian lists no fixed libc6 version; risk-accepted until a patched base image ships." | ||
| package: | ||
| name: libc6 | ||
| type: deb | ||
| - vulnerability: CVE-2026-5928 | ||
| reason: "Debian lists no fixed libc6 version; risk-accepted until a patched base image ships." | ||
| package: | ||
| name: libc6 | ||
| type: deb | ||
| - vulnerability: CVE-2026-5435 | ||
| reason: "Debian lists no fixed libc6 version; risk-accepted until a patched base image ships." | ||
| package: | ||
| name: libc6 | ||
| type: deb |
| func grypeRunOnImage(imageRef string, verbose bool) (*grypeOutput, error) { | ||
| // When configFile is non-empty it is mounted read-only into the scanner container | ||
| // and passed to grype via --config. | ||
| func grypeRunOnImage(imageRef, configFile string, verbose bool) (*grypeOutput, error) { |
|
@copilot Quick triage for maintainer-ready follow-up: Please refresh the branch if GitHub allows it, address the remaining reviewer feedback, and run the Outstanding review items (newest first):
Failed checks from the compact candidate set:
Branch update was requested automatically for this run when GitHub allows it.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the cache and portable-test feedback in |
|
@copilot Quick triage for maintainer-ready follow-up: Please refresh the branch if GitHub allows it, resolve the remaining open review threads, and run the Outstanding review items (newest first):
Failed checks from the compact candidate set:
Branch update was requested automatically for this run when GitHub allows it.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the outstanding review points in |
|
@copilot Quick triage for maintainer-ready follow-up: Please refresh the branch if GitHub allows it and run the The latest author reply says the previously listed review points were addressed, and GraphQL returned no unresolved review threads. Outstanding maintainer-facing follow-up:
Failed checks from the compact candidate set:
Run context: https://github.com/github/gh-aw/actions/runs/31899148351
|
|
🎉 This pull request is included in a new release. Release: |
The daily container scan flags Critical/High findings in
ghcr.io/github/github-mcp-server:v1.9.0that have no available remediation:v1.9.0is the latest upstream release and its tag digest is unchanged, and the glibc CVEs (CVE-2026-5450,CVE-2026-5928,CVE-2026-5435) have no fixed version published by Debian. Since the scan workflow gates on[Critical], this adds the risk-acceptance mechanism the issue recommends.Findings triage
libc6CVE-2026-5450 / 5928 / 5435golang.org/x/textGO-2026-5970go.modis already on v0.41.0. Needs an upstream rebuildignore-packagesblock in.grant.yaml(#52912)Changes
pkg/cli/grype.go—--grypenow picks up an optional repo-root.grype.yaml, mounts it read-only into the scanner container, and passes--config, mirroring how--grantconsumes.grant.yaml. Docker arg construction moved into a testablegrypeDockerArgs; behaviour is unchanged when the file is absent (including outside a git checkout)..grype.yaml(new) — per-CVE ignore rules scoped tolibc6/debwith areasoneach, so newly disclosedlibc6vulnerabilities are still reported and rules can be deleted once Debian ships a patch.setup/cli.mdand a "Container Vulnerability Exceptions" section inCONTRIBUTING.mddescribing when a rule is acceptable and when to remove it.Grype moves matched rules into
ignoredMatches, so they drop out of thematchesarray the compiler renders and no longer trip the Critical gate. Scoping by CVE ID rather than package keeps futurelibc6findings visible.Run context: https://github.com/github/gh-aw/actions/runs/31896433106> Generated by 👨🍳 PR Sous Chef · gpt54 · 9.01 AIC · ⌖ 5.84 AIC · ⊞ 8.7K · ◷