Add regression tests for gh-aw-firewall digest pinning at default version (0.27.22)#43343
Conversation
…sion (0.27.22) Fixes #43307: the four gh-aw-firewall images (agent, api-proxy, cli-proxy, squid) at tag 0.27.22 were emitted tag-only in consumer lock files because there were no tests guarding the embedded pins for the current default version or for the cli-proxy image introduced in v0.82. The embedded action_pins.json already contained correct 0.27.22 digests; this commit adds test coverage to prevent future regressions: - docker_pin_test.go: new table-driven case verifying all four 0.27.22 firewall images resolve via the embedded fallback when the local cache is absent. - docker_firewall_pin_compile_test.go: new end-to-end compile test (TestCompileWorkflow_FirewallImagesPinnedForDefaultVersion) that exercises the default AWF version with tools.github.mode=gh-proxy, asserting that agent, api-proxy, cli-proxy, and squid all appear with full digest pinning in both the manifest containers block and the --image-tag AWF config value. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
| // pinned references. | ||
| name: "embedded firewall pins for default version (0.27.22) — all four images", | ||
| images: []string{ | ||
| "ghcr.io/github/gh-aw-firewall/agent:0.27.22", |
There was a problem hiding this comment.
@copilot use constant to make test resilient to version change Md
There was a problem hiding this comment.
Done. Replaced the hardcoded version/digest table-case with a standalone TestApplyContainerPins_DefaultFirewallVersion that builds image names from constants.DefaultFirewallRegistry/constants.DefaultFirewallVersion and derives expected values from getEmbeddedContainerPin at runtime — so the test automatically tracks future version bumps. Committed in the latest push.
Replace hardcoded "0.27.22" version string and digest literals with a standalone test (TestApplyContainerPins_DefaultFirewallVersion) that drives image names from constants.DefaultFirewallVersion and constants.DefaultFirewallRegistry, and derives expected values from getEmbeddedContainerPin at runtime. The test now automatically tracks future version bumps without needing manual digest updates. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ 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. ✅ |
There was a problem hiding this comment.
Pull request overview
Adds regression coverage to ensure gh-aw-firewall sidecar images are always emitted as digest-pinned references (via embedded pins) at the current default firewall version, including the newer cli-proxy image.
Changes:
- Adds a unit-level regression test to verify embedded pin presence for all four firewall sidecars at
constants.DefaultFirewallVersion. - Adds an end-to-end compile regression test to ensure the generated lock file includes full
image/digest/pinned_imagemetadata and--image-tagdigest embedding for all four sidecars.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/docker_pin_test.go | Adds a regression test that exercises embedded container pin lookup and pin application for all firewall sidecars at the default version. |
| pkg/workflow/docker_firewall_pin_compile_test.go | Adds an end-to-end compile regression test asserting default-version firewall images (including cli-proxy) are digest-pinned in consumer lock files. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Low
| require.NotEmpty(t, pin.Digest, "Digest must be non-empty for %s", image) | ||
| require.NotEmpty(t, pin.PinnedImage, "PinnedImage must be non-empty for %s", image) | ||
|
|
||
| refs, pinEntries := applyContainerPins([]string{image}, nil) | ||
| require.Len(t, refs, 1) | ||
| assert.Equal(t, pin.PinnedImage, refs[0], "resolved ref for %s", image) | ||
| assert.Equal(t, pin.Digest, pinEntries[0].Digest, "digest in manifest entry for %s", image) |
Design Decision Gate - ADR RequiredThis PR makes significant changes to core business logic (105 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 MatterADRs 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
|
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (2 tests)
Verdict
Notes:
References: §28700930007 Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Add regression tests for gh-aw-firewall digest pinning at default version (0.27.22)
Good addition — the gap described in the PR body was real and the two-layer test strategy (unit + compile-level) is the right approach.
docker_pin_test.go — TestApplyContainerPins_DefaultFirewallVersion is clean. It calls getEmbeddedContainerPin dynamically, so it will auto-fail on version bumps without any maintenance.
docker_firewall_pin_compile_test.go — One non-blocking but meaningful improvement opportunity: the compile-level test hard-codes the four sha256: digests while dynamically deriving imageTag from constants.DefaultFirewallVersion. This creates a double-update requirement on every version bump — update action_pins.json and the four literal digests in this test. A lookup from getEmbeddedContainerPin (matching the unit test's pattern) would make it truly self-maintaining. See the inline comment for a concrete suggestion.
No correctness, security, or reliability issues were found. The embedded pin data in action_pins.json confirms all four 0.27.22 entries exist with the correct digests.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 64 AIC · ⌖ 6.22 AIC · ⊞ 4.9K
| yamlStr := string(yaml) | ||
|
|
||
| expectedPins := map[string]string{ | ||
| "ghcr.io/github/gh-aw-firewall/agent:" + imageTag: "sha256:55f06588411008b7148eb64b8dfe28602a0cce3675b36c6b190b54aca138468e", |
There was a problem hiding this comment.
Hardcoded digests vs. dynamic version creates a maintenance trap. The test dynamically derives imageTag from constants.DefaultFirewallVersion, but the four sha256: digests in expectedPins are hardcoded. When DefaultFirewallVersion is bumped, both action_pins.json and these four constants must be updated — exactly the kind of silent omission the PR description says we want to prevent.
Consider deriving the expected digests from the embedded pin table (the same way TestApplyContainerPins_DefaultFirewallVersion does) so the compile-level test stays in sync automatically:
expectedPins := make(map[string]string)
for _, sidecar := range []string{"agent", "api-proxy", "cli-proxy", "squid"} {
image := constants.DefaultFirewallRegistry + "/" + sidecar + ":" + imageTag
pin, ok := getEmbeddedContainerPin(image)
require.True(t, ok, "embedded pin must exist for %s", image)
expectedPins[image] = pin.Digest
}This keeps a single source of truth in action_pins.json and makes the test truly auto-tracking.
@copilot please address this.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — requesting changes on test-correctness and fragility issues in the new regression tests.
📋 Key Themes & Highlights
Key Themes
-
Digest duplication (
docker_firewall_pin_compile_test.go): The E2E test hardcodes all four SHA256 digests as string literals. The unit test (docker_pin_test.go) correctly avoids this by callinggetEmbeddedContainerPinvia constants — the E2E test should do the same, or the two sets of digests will diverge silently on a version bump. -
Missing negative assertion: Neither test asserts the absence of tag-only (unpinned) references, which is the exact failure mode of gh-aw#43307. A compile that emits both a pinned and an unpinned line would still pass.
-
Fragile substring probe (
imageTag + ","): Matches incidental output formatting rather than a structured contract. The per-sidecarsidecar=sha256:...checks are the real guard and should stand alone. -
Assertion clarity: Three overlapping
strings.Containscalls per image are testing the same underlying fact three different ways; a small shared helper would make the intent explicit and reduce noise on failure.
Positive Highlights
- ✅ Excellent coverage choice: adds
cli-proxy(the missing image from v0.82) alongside the three legacy sidecars - ✅ Unit test
TestApplyContainerPins_DefaultFirewallVersioncorrectly derives everything from constants — no duplication - ✅ Clear doc comments that tie each test back to the bug number
- ✅ Using
tools.github.mode: gh-proxyin the E2E frontmatter is a good design decision to forcecli-proxyinto the compile path - ✅ Table-driven sub-tests with
t.Run(sidecar, ...)in the unit test give clean, named failure output
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 81.4 AIC · ⌖ 7.76 AIC · ⊞ 6.6K
Comment /matt to run again
| yamlStr := string(yaml) | ||
|
|
||
| expectedPins := map[string]string{ | ||
| "ghcr.io/github/gh-aw-firewall/agent:" + imageTag: "sha256:55f06588411008b7148eb64b8dfe28602a0cce3675b36c6b190b54aca138468e", |
There was a problem hiding this comment.
[/tdd] Hardcoded SHA256 digests will silently diverge when DefaultFirewallVersion is bumped — the test docs say it "will fail immediately", but that is only true for the unit test in docker_pin_test.go (which calls getEmbeddedContainerPin via constants). The E2E test here hardcodes the digests as string literals, so a version bump that updates action_pins.json but leaves these strings stale will still compile the workflow correctly while making this test pass with incorrect expectations.
💡 Suggested fix
Derive the expected digests from the same embedded table that production code uses, rather than duplicating them as literals:
for _, sidecar := range []string{"agent", "api-proxy", "cli-proxy", "squid"} {
image := constants.DefaultFirewallRegistry + "/" + sidecar + ":" + imageTag
pin, ok := getEmbeddedContainerPin(image)
require.True(t, ok, "no embedded pin for %s", image)
expectedPins[image] = pin.Digest
}This makes the E2E test self-consistent with docker_pin_test.go and eliminates duplicate maintenance.
@copilot please address this.
| mode: gh-proxy | ||
| --- | ||
|
|
||
| # Test |
There was a problem hiding this comment.
[/tdd] The AWF-config string probe imageTag + "," is fragile — it matches a bare image tag followed by a comma in YAML, but this exact substring could be present for reasons unrelated to pinning. A more precise assertion would check the structured key that embeds the version, e.g. asserting the full --image-tag AWF config value contains each sidecar=sha256:... pair.
💡 Why this matters
The check imageTag + "," relies on incidental output formatting. If the serialisation changes (e.g. JSON key ordering, trailing comma removal), this probe will silently stop verifying what it intends to. The existing per-sidecar sidecar=sha256:... assertions on lines 157–160 are correctly precise and serve as the real guard — consider removing the redundant bare-imageTag check, or replace it with a structured assertion on the --image-tag config blob.
@copilot please address this.
| on: workflow_dispatch | ||
| engine: claude | ||
| network: | ||
| allowed: |
There was a problem hiding this comment.
[/tdd] Three overlapping strings.Contains checks for each image (manifest header JSON, comment block, and download reference) test the same data three different ways. If the format of any one of these changes legitimately, all three probes break at once — obscuring which assertion is the actual contract.
💡 Suggestion
Consider consolidating into a helper that captures the intent explicitly:
func assertImagePinned(t *testing.T, yamlStr, image, digest string) {
t.Helper()
pinnedImage := image + "@" + digest
assert.Contains(t, yamlStr, `"pinned_image":"`+pinnedImage+`"`, "manifest header for "+image)
assert.Contains(t, yamlStr, pinnedImage, "download ref for "+image)
}The separate comment-block probe can be kept if you want to assert a specific output section, but give it its own clearly-named assertion.
@copilot please address this.
| require.NotEmpty(t, pin.Digest, "Digest must be non-empty for %s", image) | ||
| require.NotEmpty(t, pin.PinnedImage, "PinnedImage must be non-empty for %s", image) | ||
|
|
||
| refs, pinEntries := applyContainerPins([]string{image}, nil) |
There was a problem hiding this comment.
[/tdd] applyContainerPins is called with a single-element slice and then refs[0] is accessed directly. If applyContainerPins ever returns an empty slice for a failed lookup, this would index-panic rather than produce a useful test failure. The require.Len(t, refs, 1) on the previous line guards against this, which is correct — but the comment above the call says the pin "must exist" without explaining what would cause it not to exist, making the intent slightly unclear to future readers.
💡 Minor suggestion
Add a brief inline comment noting the invariant being checked:
// applyContainerPins should resolve the embedded pin, so exactly one ref is expected.
refs, pinEntries := applyContainerPins([]string{image}, nil)
require.Len(t, refs, 1, "expected pinned ref from embedded table")This mirrors the intent expressed in the unit test for the table case "embedded pin used when cache is absent".
@copilot please address this.
| } | ||
|
|
||
| // TestCompileWorkflow_FirewallImagesPinnedForDefaultVersion is a regression test for | ||
| // gh-aw#43307: the four gh-aw-firewall images at the current default version |
There was a problem hiding this comment.
[/tdd] The test does not verify the absence of tag-only (unpinned) references — which is the precise failure mode described in gh-aw#43307. A compile that writes ghcr.io/github/gh-aw-firewall/agent:0.27.22 without a digest would still pass all current assertions as long as the pinned form also appears elsewhere in the YAML.
💡 Suggested addition
Add a negative assertion after verifying pinned images:
for image := range expectedPins {
// A tag-only reference must NOT appear; every occurrence must be digest-pinned.
assert.NotContains(t, yamlStr, `image: `+image+`\n`,
"tag-only (unpinned) reference must not appear for %s", image)
}This directly encodes the regression condition from the bug.
@copilot please address this.
There was a problem hiding this comment.
REQUEST_CHANGES — The tests add useful coverage but have two issues that need fixing before merge, plus two secondary concerns.
### Blocking issues
-
Hardcoded digest literals (
docker_firewall_pin_compile_test.go:130–133) — The compile test hardcodes four SHA256 digests but the unit test in the same PR correctly reads them fromgetEmbeddedContainerPinat runtime. The compile test's digests will go stale silently on the nextDefaultFirewallVersionbump and fail with a misleading error. Apply the same pattern as the unit test. -
agent-actunpinned and undetected (docker_pin_test.go:116) —buildAWFImageTagWithDigestsqueriesagent-actfor digest inclusion butaction_pins.jsonhas no entry for0.27.22. The silently skipped digest means AWF pullsagent-actby tag, not digest — the same class of regression this PR is supposed to prevent. Neither test covers this; one test comment incorrectly claims to cover "all four images" whenagent-actis a fifth.
### Non-blocking issues
-
"imageTag"literal check is vacuous (docker_firewall_pin_compile_test.go:150) — checks that the JSON field name appears in the lock file, not that the version value is correct. Replace with"imageTag":"+ imageTag. -
require.Len(t, refs, 1)asserts nothing (docker_pin_test.go:127) —applyContainerPinsalways returns same-length slices; this assertion can never fail.
🔎 Code quality review by PR Code Quality Reviewer · 185.4 AIC · ⌖ 7.76 AIC · ⊞ 5.4K
Comment /review to run again
| yamlStr := string(yaml) | ||
|
|
||
| expectedPins := map[string]string{ | ||
| "ghcr.io/github/gh-aw-firewall/agent:" + imageTag: "sha256:55f06588411008b7148eb64b8dfe28602a0cce3675b36c6b190b54aca138468e", |
There was a problem hiding this comment.
Hardcoded digest literals will silently break when DefaultFirewallVersion is bumped — the test claim of automatic version tracking is only half true.
💡 Details and suggested fix
The map keys correctly derive imageTag from constants.DefaultFirewallVersion, but the digest values are raw literals:
"ghcr.io/github/gh-aw-firewall/agent:" + imageTag: "sha256:55f06588...",When DefaultFirewallVersion is bumped, these four digests will no longer match and the test will fail with:
Expected manifest header to include pinned metadata for ghcr.io/github/gh-aw-firewall/agent:0.28.0
...with zero indication that the hardcoded digests are stale. The companion unit test in docker_pin_test.go avoids this correctly — it calls getEmbeddedContainerPin at runtime. Apply the same pattern here:
for _, sidecar := range []string{"agent", "api-proxy", "cli-proxy", "squid"} {
image := constants.DefaultFirewallRegistry + "/" + sidecar + ":" + imageTag
pin, ok := getEmbeddedContainerPin(image)
require.True(t, ok, "no embedded pin for %s", image)
expectedPins[image] = pin.Digest
}This makes the compile test consistent with the unit test and eliminates the maintenance trap entirely.
| } | ||
|
|
||
| for _, imageTagPart := range []string{ | ||
| `imageTag`, |
There was a problem hiding this comment.
The "imageTag" literal check verifies the JSON field name exists, not that the version value is correct — this assertion is vacuous.
💡 Details and suggested fix
for _, imageTagPart := range []string{
`imageTag`, // ← matches the JSON key name, always present
imageTag + `,`, // ← this is the one that actually checks the version
...
}The backtick-quoted imageTag string is a literal JSON key name emitted by any AWF config section. It passes as long as any AWF container config block exists, even if the version value is completely wrong. It provides zero regression protection for the pinning bug described in the PR.
Replace it with a check that validates both key and value:
`"imageTag":"` + imageTag,This ensures the lock file actually encodes the expected version in the AWF config, not just that an AWF config section exists at all.
| require.NotEmpty(t, pin.PinnedImage, "PinnedImage must be non-empty for %s", image) | ||
|
|
||
| refs, pinEntries := applyContainerPins([]string{image}, nil) | ||
| require.Len(t, refs, 1) |
There was a problem hiding this comment.
require.Len(t, refs, 1) can never fail — it asserts nothing about pinning correctness.
💡 Details and suggested fix
applyContainerPins always allocates and returns slices of exactly len(images). Passing a single-element slice guarantees refs has length 1 unconditionally, whether the image was pinned or not. The assertion provides zero regression protection.
Replace it with a check that distinguishes a pinned result from an unchanged one:
require.Len(t, refs, 1)
assert.Contains(t, refs[0], "`@sha256`:", "ref for %s must include digest", image)Or assert the full pinned image directly, which is already done on the next line — in that case, just delete the require.Len call entirely since it adds no value.
| // Using constants means the test automatically tracks version bumps. | ||
| func TestApplyContainerPins_DefaultFirewallVersion(t *testing.T) { | ||
| imageTag := strings.TrimPrefix(string(constants.DefaultFirewallVersion), "v") | ||
| sidecars := []string{"agent", "api-proxy", "cli-proxy", "squid"} |
There was a problem hiding this comment.
agent-act is absent from the sidecar list, leaving a gap that buildAWFImageTagWithDigests will silently swallow.
💡 Details and suggested fix
buildAWFImageTagWithDigests in awf_helpers.go:806 includes agent-act when building the AWF --image-tag config value:
{name: "agent-act", image: constants.DefaultFirewallRegistry + "/agent-act:" + imageTag},However, agent-act:0.27.22 has no entry in action_pins.json (the latest agent-act entry is 0.27.13). At runtime buildAWFImageTagWithDigests silently skips images with no pin, so the AWF config imageTag value is emitted without agent-act=sha256:..., meaning AWF must pull agent-act at the tag rather than by digest.
The new test's sidecar list — ["agent", "api-proxy", "cli-proxy", "squid"] — does not include agent-act, so this gap is invisible. The test comment claims to cover "all four gh-aw-firewall images" but agent-act is a fifth image used in production.
Two options:
- Intentional omission: If
agent-actis no longer used at the default version, remove it from thespecslist inbuildAWFImageTagWithDigestsso there's no silent skip. - Gap to fix: Add
agent-act:0.27.22toaction_pins.jsonand add"agent-act"to the sidecar list in both tests with an assertion that its digest appears in the AWF configimageTag.
Either way, update the test comment to accurately describe what images are (and are not) covered.
|
@copilot please run the
|
Consumer-compiled lock files on v0.82.2 emitted all four
gh-aw-firewallimages (agent,api-proxy,cli-proxy,squid) with tag-only references — nodigest/pinned_image. The embedded pins inaction_pins.jsonwere already correct for0.27.22, but existing tests only covered the old0.27.0version and did not covercli-proxy(new in v0.82) at all. This left no guard against the regression.Changes
docker_pin_test.go— new table case"embedded firewall pins for default version (0.27.22) — all four images"asserts thatagent,api-proxy,cli-proxy, andsquidat0.27.22all resolve via the embedded fallback when no local cache is present.docker_firewall_pin_compile_test.go— new end-to-end testTestCompileWorkflow_FirewallImagesPinnedForDefaultVersioncompiles a workflow at the default AWF version withtools.github.mode: gh-proxy(to includecli-proxy) and asserts that all four images appear with fullimage/digest/pinned_imageentries in the manifest and that their digests are embedded in the--image-tagAWF config value.If
DefaultFirewallVersionis bumped without refreshingaction_pins.json, these tests will now fail immediately rather than silently shipping unpinned images to consumers.