Fix recurring gh-aw-firewall digest-pin loss on DefaultFirewallVersion bumps - #51423
Conversation
…ng, record resolution failures Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Thanks for tackling the digest-pinning regression in #51248! 👋 This is an important security fix—preventing silent sandbox changes via mutable tag repush is critical. Here are a few things to complete:
When you're ready to continue, here's a prompt for your agent:
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Prevents firewall digest pins from being pruned and surfaces unresolved pins in compiled manifests.
Changes:
- Retains historical firewall container pins.
- Records unresolved firewall pin failures.
- Adds compile and pruning regression tests.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/docker.go |
Records missing firewall pins. |
pkg/workflow/docker_firewall_pin_compile_test.go |
Tests manifest pinning and failures. |
pkg/workflow/action_cache.go |
Exempts firewall pins from pruning. |
pkg/workflow/action_cache_container_pin_test.go |
Tests pruning behavior. |
pkg/cli/update_container_pins_test.go |
Tests CLI cache retention. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Balanced
| // security-load-bearing containers (they confine the agent sandbox), so | ||
| // record the miss as a resolution failure for lock-file auditing instead | ||
| // of silently shipping an unpinned tag (see gh-aw#51248). | ||
| if workflowData != nil && strings.HasPrefix(img, constants.DefaultFirewallRegistry+"/") { |
| if setutil.Contains(knownImages, image) { | ||
| continue | ||
| } | ||
| if strings.HasPrefix(image, constants.DefaultFirewallRegistry+"/") { |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "api.individual.githubcopilot.com"See Network Configuration for more information.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
|
✅ 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.
Review: Fix recurring gh-aw-firewall digest-pin loss on DefaultFirewallVersion bumps
The fix is well-structured and addresses the root cause: PruneStaleContainerPins now exempts all ghcr.io/github/gh-aw-firewall/ images from pruning, and applyContainerPins records a resolution_failure when an AWF image tag has no cached digest pin.
Existing review comments already flagged the two most important concerns:
-
docker.goline 258 — The resolution-failure path is reached only for truly unresolved images (thecontinueon line 249 guards the already-pinned path), so the existing comment is more cautionary than blocking. -
action_cache.goline 180 — The exemption is forward-only; pins already dropped before this fix are not automatically restored. Worth noting in release notes or a follow-up.
The tests are thorough: TestUpdateContainerPins_KeepsStaleFirewallEntries, TestPruneStaleContainerPins, and TestCompileWorkflow_UnresolvedFirewallImageRecordsResolutionFailure together give solid coverage. No blocking issues found beyond the already-commented concerns.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 26.4 AIC · ⌖ 7.21 AIC · ⊞ 5.5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on two correctness issues before merge.
📋 Key Themes & Highlights
Key Themes
- Root cause addressed correctly: exempting
DefaultFirewallRegistryimages fromPruneStaleContainerPinsis the right fix and well-documented. - Detection layer is good: surfacing unresolved pins as
resolution_failuresinstead of silently shipping bare tags closes the observability gap described in #51248. - Test coverage is thorough: golden compile test + regression test + unit tests for the pruning exemption form a solid regression suite.
Correctness Issues
docker.goline 258 — TheHasPrefixguard triggers even whenimgalready contains@sha256:, creating a spuriousresolution_failurefor an already-pinned reference. Add!strings.Contains(img, "@sha256:")to the condition.docker_firewall_pin_compile_test.goline 319 —imageTagstrips the leadingvbut there is no confirmation that the compiler-side tag construction does the same. If theReffield actually contains thevprefix, the test will always fail with a misleading message. Verify consistency and add the actual failures to the error message for diagnosability.- Operational gap (noted in review comment, not blocking code) — The exemption prevents future pin loss but does not restore already-lost historical pins. A follow-up
make sync-action-pinsrun is needed post-merge.
Positive Highlights
- ✅ Excellent commit comment explaining why the exemption is safe (cheap, few KB per version).
- ✅
TestUpdateContainerPins_KeepsStaleFirewallEntriesdirectly tests the regression scenario from #51248. - ✅
TestCompileWorkflow_AllManifestContainersArePinnedis the generic golden test the issue called for — will catch future additions that forget pinning.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 37.7 AIC · ⌖ 7.8 AIC · ⊞ 7.1K
Comment /matt to run again
| // security-load-bearing containers (they confine the agent sandbox), so | ||
| // record the miss as a resolution failure for lock-file auditing instead | ||
| // of silently shipping an unpinned tag (see gh-aw#51248). | ||
| if workflowData != nil && strings.HasPrefix(img, constants.DefaultFirewallRegistry+"/") { |
There was a problem hiding this comment.
[/diagnosing-bugs] The HasPrefix check fires for an already-digest-pinned reference (e.g. ghcr.io/github/gh-aw-firewall/agent:0.27.0@sha256:abc) — this would add a spurious resolution_failure for an image that is already pinned.
💡 Suggested guard
Skip recording the failure when the image string already contains a digest:
if workflowData != nil &&
strings.HasPrefix(img, constants.DefaultFirewallRegistry+"/") &&
!strings.Contains(img, "`@sha256`:") {
// record resolution failure ...
}@copilot please address this.
| if setutil.Contains(knownImages, image) { | ||
| continue | ||
| } | ||
| if strings.HasPrefix(image, constants.DefaultFirewallRegistry+"/") { |
There was a problem hiding this comment.
[/diagnosing-bugs] The exemption prevents future pruning but does not restore any pins that were already lost before this PR lands. Any consumer workflow that was broken by a previous DefaultFirewallVersion bump still needs a one-time make sync-action-pins run (or equivalent) to repopulate the embedded catalogs (pkg/actionpins/data/action_pins.json, pkg/workflow/data/action_pins.json).
💡 Suggestion
Add a note in the PR description or a follow-up task to re-resolve missing historical AWF pins so the binary ships with a complete catalog. This is a post-merge operational step, not a code change, but without it the fix only prevents future losses — it does not heal the current state.
@copilot please address this.
| } | ||
| if len(unpinned) > 0 { | ||
| t.Errorf("Expected every manifest container entry to carry digest/pinned_image, but %d were unpinned: %v", len(unpinned), unpinned) | ||
| } |
There was a problem hiding this comment.
[/tdd] imageTag is computed with strings.TrimPrefix(unresolvedVersion, "v") but unresolvedVersion is "v0.1.2-unresolved-test" — so imageTag is "0.1.2-unresolved-test". Verify that the AWF image construction path also strips the v prefix the same way; if the compiler keeps the v prefix the assertion f.Ref == imageTag will always fail and the test will report a false-negative ("no resolution failure found") even when the feature works.
💡 Suggestion
Add a sub-assertion or log the actual manifest.ResolutionFailures slice when the test fails so it's immediately clear what Ref value was actually recorded:
if !found {
t.Errorf("Expected resolution failure for %s:%s, got: %+v",
agentRepo, imageTag, manifest.ResolutionFailures)
}Also confirm the compiler-side tag normalisation matches before merging.
@copilot please address this.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 82/100 — Excellent
📊 Metrics (21 tests)
i️ NotesInflation note: Standout tests:
Minor improvement opportunity: Several Verdict
References: Run #31268764740
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (276 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 Quick triage nudge for this PR. Please address the current blocking review feedback, link the committed ADR in the PR body, refresh the branch if needed, run the Open review context (newest first):
Run: https://github.com/github/gh-aw/actions/runs/31269773406
|
PR Triage
Fourth recurrence of firewall digest-pin loss on version bumps — exempts gh-aw-firewall images from stale-pin pruning and adds golden/regression tests (
|
|
🎉 This pull request is included in a new release. Release: |
gh-aw-firewall/{agent,api-proxy,squid,cli-proxy}images were silently emitted as bare tags instead of digest-pinned references (image:tag@sha256:...) in compiled lock files wheneverconstants.DefaultFirewallVersionwas bumped — a fourth recurrence of the same class of bug (previously "fixed" in #38561, #43307, #44040).Root cause
ActionCache.PruneStaleContainerPinsremoves any container pin no longer referenced by locally compiled lock files. When gh-aw's own default AWF version is bumped, the previous version's pin becomes unreferenced in gh-aw's own repo and gets pruned from.github/aw/actions-lock.json.make sync-action-pinsthen mirrors that loss into the embeddedpkg/actionpins/data/action_pins.json/pkg/workflow/data/action_pins.jsoncatalogs shipped in the binary, breaking digest pinning for any consumer workflow explicitly pinning that now-previous version — with no trace inresolution_failures.Changes
pkg/workflow/action_cache.go):PruneStaleContainerPinsnow skips any image underconstants.DefaultFirewallRegistry, so resolved AWF digest pins are retained across version bumps instead of being dropped.pkg/workflow/docker.go):applyContainerPinsrecords aresolution_failuresmanifest entry when a gh-aw-firewall image has no cache/embedded pin, instead of silently shipping an unpinned tag.TestCompileWorkflow_AllManifestContainersArePinned, which parses the compiled lock file's manifest and asserts everycontainers[]entry carries apinned_image— the regression-catching test suggested in the issue.TestCompileWorkflow_UnresolvedFirewallImageRecordsResolutionFailureverifies an unresolvable AWF version now surfaces a resolution failure rather than compiling silently.pkg/workflow/action_cache_container_pin_test.go,pkg/cli/update_container_pins_test.go).