fix: UpdateContainerPins wipes containers section on every gh aw update run#41262
fix: UpdateContainerPins wipes containers section on every gh aw update run#41262Copilot wants to merge 3 commits into
Conversation
…ent pruning all container pins Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes gh aw update unintentionally pruning all container pins from .github/aw/actions-lock.json when compiled .lock.yml files already contain digest-pinned references (e.g. image:tag@sha256:...). The change normalizes image keys used for pruning so they match the base-tag keys used in the container pins map.
Changes:
- Strip the
@sha256:...digest suffix when building theknownImagesset used byPruneStaleContainerPins, preventing false “stale” pruning. - Add a regression test covering the pinned-lock-file scenario (but it should exercise
UpdateContainerPinsend-to-end to avoid duplicating production logic).
Show a summary per file
| File | Description |
|---|---|
pkg/cli/update_container_pins.go |
Normalizes lock-file image refs to base tags before pruning container pins. |
pkg/cli/update_container_pins_test.go |
Adds a regression test for pinned lock-file refs (currently duplicates normalization logic rather than calling UpdateContainerPins). |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 2
| import ( | ||
| "os" | ||
| "path/filepath" | ||
| "strings" | ||
| "testing" |
| // Build imageSet the same way UpdateContainerPins does: strip @sha256: suffix so | ||
| // the keys match the base-tag keys used in the container pins map. | ||
| imageSet := make(map[string]struct{}) | ||
| for _, img := range images { | ||
| base, _, _ := strings.Cut(img, "@sha256:") | ||
| imageSet[base] = struct{}{} | ||
| } | ||
| assert.Equal(t, map[string]struct{}{"ghcr.io/github/gh-aw-firewall/agent:0.27.9": {}}, imageSet) | ||
|
|
||
| // With the correct imageSet (base tags), PruneStaleContainerPins should keep the pin. | ||
| cache := workflow.NewActionCache(tmpDir) | ||
| require.NoError(t, cache.Load()) | ||
|
|
||
| pruned := cache.PruneStaleContainerPins(imageSet) | ||
| assert.Equal(t, 0, pruned, "no container pins should be pruned when lock files use pinned references") | ||
|
|
||
| pin, ok := cache.GetContainerPin("ghcr.io/github/gh-aw-firewall/agent:0.27.9") | ||
| require.True(t, ok, "container pin should still be present") | ||
| assert.Equal(t, "sha256:13f522853a688bfe24b04adbbe40b68101e8ef4b6fe0b636068527141bf1c269", pin.Digest) | ||
|
|
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #41262 does not have the 'implementation' label and has 79 new lines (≤100) in business logic directories. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
![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 /diagnose and /tdd — the production fix is correct; two test quality observations noted.
📋 Key Themes & Highlights
Key Themes
- Test duplicates production logic (line 225): the
imageSet-building loop is re-implemented in the test rather than exercised through the production path, creating a latent regression blind spot. - Weak file assertion (line 241):
cache.Save()is never called before the finalos.ReadFilecheck, so that assertion always passes and adds no safety — it reads the original setup fixture unchanged.
Positive Highlights
- ✅ Root cause correctly identified and fixed with a single surgical
strings.Cutcall — exactly the right approach. - ✅ Regression test is well-named, well-commented, and covers the exact failure scenario end-to-end at the component level.
- ✅ The core assertions (
pruned == 0andGetContainerPin) are strong guards against regressions. - ✅ No unrelated behaviour changes; the fix is confined to the key-normalisation step.
Note on end-to-end coverage
Because every image in this lock file is already digest-pinned, UpdateContainerPins would skip all Docker calls (line 127 early-return). A future improvement could call UpdateContainerPins directly here for full integration coverage without needing a Docker daemon.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 60.6 AIC · ⌖ 7.79 AIC · ⊞ 6.5K
| base, _, _ := strings.Cut(img, "@sha256:") | ||
| imageSet[base] = struct{}{} | ||
| } | ||
| assert.Equal(t, map[string]struct{}{"ghcr.io/github/gh-aw-firewall/agent:0.27.9": {}}, imageSet) |
There was a problem hiding this comment.
[/tdd] The test re-implements the imageSet-building loop from UpdateContainerPins (lines 220–224), duplicating production logic. If that loop changes, this test may not detect the regression — it just verifies its own copy of the logic.
💡 Suggested improvement
Either:
- Extract a small
buildNormalizedImageSet(images []string) map[string]struct{}helper, then call it from bothUpdateContainerPinsand the test — so the test exercises the real implementation. - Or call
UpdateContainerPinsend-to-end. No Docker daemon is needed here because all images in the lock file are already digest-pinned (line 127strings.Contains(image, "@sha256:")exits early for all of them). An e2e call would provide stronger regression coverage.
The pruned == 0 assertion below is the real guard; this block mostly documents the logic rather than testing the production path.
| // Verify the lock file is unchanged (the existing pin covers this image). | ||
| data, err := os.ReadFile(actionsLockPath) | ||
| require.NoError(t, err) | ||
| assert.Contains(t, string(data), "ghcr.io/github/gh-aw-firewall/agent:0.27.9", "container pin should still be in actions-lock.json") |
There was a problem hiding this comment.
[/tdd] This file assertion always passes regardless of whether the bug is present. cache.Save() is never called in this test, so actionsLockPath still contains the content written during setup — the file was never modified.
💡 How to make this assertion meaningful
Option A — remove it: the pruned == 0 check and GetContainerPin assertions already cover the invariant at the in-memory level, which is where the bug manifests.
Option B — make it a real disk-level guard by adding a Save+reload cycle:
require.NoError(t, cache.Save())
data, err := os.ReadFile(actionsLockPath)
require.NoError(t, err)
assert.Contains(t, string(data), "ghcr.io/github/gh-aw-firewall/agent:0.27.9")This would catch regressions where pruning happens correctly in-memory but Save serialises the wrong state.
There was a problem hiding this comment.
🔎 Code quality review by PR Code Quality Reviewer · 109.9 AIC · ⌖ 6.4 AIC · ⊞ 5.2K
| // Verify the lock file is unchanged (the existing pin covers this image). | ||
| data, err := os.ReadFile(actionsLockPath) | ||
| require.NoError(t, err) | ||
| assert.Contains(t, string(data), "ghcr.io/github/gh-aw-firewall/agent:0.27.9", "container pin should still be in actions-lock.json") |
There was a problem hiding this comment.
Trivially-true assertion provides no signal: cache.Save() is never called in this test, so the file on disk is byte-for-byte identical to what os.WriteFile wrote at setup — the assertion cannot fail under any code path exercised here.
💡 Details
The comment says "Verify the lock file is unchanged", implying a post-save check, but since cache.Save() is never called the file trivially cannot have changed. The actual regression (pruning firing when it should not) is already fully caught at:
assert.Equal(t, 0, pruned, "no container pins should be pruned...")When pruned == 0 the cache is not dirty, so even if Save() were called it would be a no-op. The assert.Contains adds noise without coverage. Consider removing it, or — better — follow the pattern in TestUpdateContainerPins_PrunesStaleEntries by calling cache.Save() and then reading the file back to meaningfully verify the on-disk state.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics & Test Classification (1 test analyzed)
Go: 1 ( Assertion detail (13 total):
Scaffolding
|
|
@copilot refresh the branch and rerun checks, then summarize any remaining blockers.
|
|
@copilot review all comments and address the unresolved review feedback in pkg/cli/update_container_pins_test.go.
|
|
`` Please address the review feedback in pkg/cli/update_container_pins_test.go, then re-run the focused tests.
|
gh aw updatewas deleting the entire"containers"section fromactions-lock.jsonon every run. After the first successful pin run, lock files embed digest-pinned references (image:tag@sha256:...).collectImagesFromLockFilesreturns those full strings, but container pin keys use plain base tags (image:tag).PruneStaleContainerPinsfound zero key matches and pruned everything.Changes
update_container_pins.go— strip@sha256:...suffix withstrings.Cutwhen buildingimageSet, so keys match the base-tag keys in the container pins map:update_container_pins_test.go— addsTestUpdateContainerPins_PinnedLockFilesPreserveContainerPinsto cover the case where lock files already contain digest-pinned references.