Add regression tests for gh-aw-firewall digest pinning at default version (v0.27.22)#43315
Conversation
…sion (v0.27.22) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
❌ Design Decision Gate 🏗️ failed to deliver outputs during design decision gate check. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ 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 container images are emitted as digest-pinned references (not tag-only) when compiling workflows, specifically for the current default AWF version (v0.27.22) and including the newer cli-proxy image.
Changes:
- Extend
TestApplyContainerPinsto validate all four default firewall images resolve to the expected embedded digests. - Add a compile-level regression test that compiles a workflow using the default AWF version with
cli-proxyenabled and asserts pinned metadata appears in the compiled lock output. - Add a changeset entry describing the fix/regression guard.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/docker_pin_test.go | Adds a new table-driven test case asserting embedded pin resolution for all four AWF sidecar images at 0.27.22. |
| pkg/workflow/docker_firewall_pin_compile_test.go | Adds a compile regression test for default AWF version pinning (including cli-proxy) and imports constants for version/registry. |
| .changeset/patch-fix-firewall-digest-pinning-0-27-22.md | Adds release note entry for the firewall digest-pinning regression coverage/fix. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Low
| compiler := NewCompiler() | ||
| if err := compiler.CompileWorkflow(testFile); err != nil { | ||
| t.Fatalf("Failed to compile workflow: %v", err) | ||
| } |
|
🛠️ Agentic Maintenance updated this pull request branch. |
|
Warning The 🏗️ 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 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
|
🧪 Test Quality Sentinel Report\n\n✅ Test Quality Score: 100/100 — Excellent\n\n> Analyzed 2 test(s): 2 design, 0 implementation, 0 violation(s).\n\n
📊 Metrics (2 tests)\n\n| Metric | Value |\n|---|---|\n| Analyzed | 2 (Go: 2, JS: 0) |\n| ✅ Design | 2 (100%) |\n| |
There was a problem hiding this comment.
REQUEST_CHANGES — The regression test structure has three issues that collectively undermine the coverage guarantee this PR is making.
### Blocking issues (2 high/medium)
1. Hardcoded digests decouple from DefaultFirewallVersion (compile test, lines 132–136)
defaultTag is derived from the constant but the four SHA-256 values are literals. On the next version bump the tag advances while the digests stay at 0.27.22; the test fails with confusing manifest-header errors. Worse, the test doesn't actually validate the embedded pins table — it compares two independently-authored literal sets. See inline comment.
2. docker_pin_test.go table case hardcodes 0.27.22 and will silently stop tracking the default (lines 47–57)
The case name says "default firewall images" but every image ref is a literal. When the version constant advances, this case keeps passing against the old version — precisely leaving the regression gap the PR claims to close. Both the images list and expected digests must be derived from the constant.
### Non-blocking issue (medium)
3. Missing negative assertion (compile test, line 140)
The tests verify pinned refs appear but never assert that bare tag-only refs (the v0.82.2 bug form) are absent. If both forms coexist in the lock file output, all current assertions pass green. A negative check per image is needed for the test to actually detect the regression it guards.
🔎 Code quality review by PR Code Quality Reviewer · 123 AIC · ⌖ 5.66 AIC · ⊞ 5.4K
Comment /review to run again
| yamlStr := string(yaml) | ||
|
|
||
| registry := string(constants.DefaultFirewallRegistry) | ||
| expectedPins := map[string]string{ |
There was a problem hiding this comment.
Hardcoded digests will immediately stale when DefaultFirewallVersion is bumped, producing misleading test failures.
💡 Details and suggested fix
defaultTag is derived dynamically from constants.DefaultFirewallVersion, but the four SHA-256 digests are hardcoded literals. On the next routine version bump, defaultTag changes to the new tag while the expected digests stay at 0.27.22 — the manifest-header assertions fail with opaque "expected pinned metadata for ..." errors that give no hint of the version mismatch.
More critically, the test is not actually cross-checking the embedded pins table: it compares one independently-authored set of literals against another. If a digest is wrong in both places they agree silently.
Derive expected digests from the same lookup the production code uses:
expectedPins := make(map[string]string, 4)
for _, sidecar := range []string{"agent", "api-proxy", "cli-proxy", "squid"} {
image := registry + "/" + sidecar + ":" + defaultTag
pin, ok := actionpins.GetContainerPin(image)
if !ok {
t.Fatalf("no embedded pin for %s — update action_pins.json", image)
}
expectedPins[image] = pin.Digest
}This forces a make recompile when the version is bumped, keeps the test self-consistent, and surfaces a clear failure message when any sidecar is missing from the embedded table.
| { | ||
| name: "all four default firewall images (v0.27.22) pinned from embedded table", | ||
| images: []string{ | ||
| "ghcr.io/github/gh-aw-firewall/agent:0.27.22", |
There was a problem hiding this comment.
Test case claims to cover 'the default firewall images' but hardcodes 0.27.22 — silently stops testing the default version the moment it is bumped.
💡 Details and suggested fix
The case name is "all four default firewall images (v0.27.22) pinned from embedded table", and all four image refs and all four expected digest strings embed 0.27.22 as a literal. When constants.DefaultFirewallVersion advances to v0.27.23 (or beyond), this case:
- Continues to pass — it still exercises the embedded table entry for the old version, which will remain in
action_pins.json. - Gives zero coverage of the new default version — exactly the regression gap this PR says it's closing.
The intent is clearly to test whatever version the constant points at. Use the constant:
{
name: "all four default firewall images (" + string(constants.DefaultFirewallVersion) + ") pinned from embedded table",
images: func() []string {
tag := strings.TrimPrefix(string(constants.DefaultFirewallVersion), "v")
reg := constants.DefaultFirewallRegistry
return []string{
reg + "/agent:" + tag,
reg + "/api-proxy:" + tag,
reg + "/cli-proxy:" + tag,
reg + "/squid:" + tag,
}
}(),
// ... derive expectedRefs and expectedDigests from actionpins.GetContainerPin similarly
}This ensures the table case tracks the default version automatically and fails loudly when action_pins.json is not updated after a version bump.
| } | ||
|
|
||
| for image, digest := range expectedPins { | ||
| pinnedImage := image + "@" + digest |
There was a problem hiding this comment.
No assertion that bare tag-only references are absent — the original v0.82.2 bug would silently pass.
💡 Details and suggested fix
The test asserts that pinnedImage (e.g. registry/agent:0.27.22@sha256:...) appears in the lock file YAML, but never asserts that the bare tag form (e.g. registry/agent:0.27.22 without a digest) is absent from the Docker image list.
The v0.82.2 regression was that tag-only references were emitted. If both forms coexist in the output the test passes green while the bug is still present. Add an absence check alongside each presence check:
for image, digest := range expectedPins {
pinnedImage := image + "@" + digest
// ... existing presence assertions ...
// Regression guard: bare tag must not appear as a standalone pull reference.
// Split on the digest separator to get just the tag portion.
if strings.Contains(yamlStr, "- "+image+"\n") || strings.Contains(yamlStr, "\'" + image + "\'") {
t.Errorf("Tag-only reference still present for %s — digest pinning regression", image)
}
}The exact assertion depends on how docker pull entries are formatted in the lock file, but the point is: a negative check is required or this test cannot detect the exact bug it was written to guard against.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — commenting with targeted suggestions; no blocking correctness issues.
📋 Key Themes & Highlights
Key Themes
- Hardcoded version string in unit test (
docker_pin_test.go) — using the literal"0.27.22"instead of deriving fromconstants.DefaultFirewallVersionmeans the test will drift silently when the default bumps. - Fragile JSON key-ordering assertion — the manifest-header check embeds
"image":"...","digest":"...","pinned_image":"..."as a single substring; a serialisation field-order change would break the test for structural, not semantic, reasons. - Partial
imageTagassertions — individual substring checks (defaultTag + ",", lone digest fragments) allow a malformed or incompleteimageTagto pass. - Non-deterministic map iteration — iterating
expectedPins(amap[string]string) produces non-deterministic failure output; a slice of structs would be more debuggable.
Positive Highlights
- ✅ Excellent coverage of a real regression: all four firewall images (
agent,api-proxy,cli-proxy,squid) are verified at every level — manifest header, download comment, and AWF configimageTag. - ✅ The compile-level test correctly derives
defaultTagfromconstants.DefaultFirewallVersion— the unit test should follow the same pattern. - ✅ Using
features.cli-proxy: trueto trigger the new sidecar is exactly the right setup. - ✅ Clean, well-documented test function comment explaining the regression context.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 90 AIC · ⌖ 5.67 AIC · ⊞ 6.6K
Comment /matt to run again
| { | ||
| name: "all four default firewall images (v0.27.22) pinned from embedded table", | ||
| images: []string{ | ||
| "ghcr.io/github/gh-aw-firewall/agent:0.27.22", |
There was a problem hiding this comment.
[/tdd] The image refs hardcode "0.27.22" instead of deriving from constants.DefaultFirewallVersion. When the default version bumps, the old entries remain in the embedded table so the test continues passing — silently losing coverage of the new default.
💡 Suggested fix
Derive the version dynamically, mirroring the compile test:
defaultTag := strings.TrimPrefix(string(constants.DefaultFirewallVersion), "v")
images: []string{
"ghcr.io/github/gh-aw-firewall/agent:" + defaultTag,
"ghcr.io/github/gh-aw-firewall/api-proxy:" + defaultTag,
"ghcr.io/github/gh-aw-firewall/cli-proxy:" + defaultTag,
"ghcr.io/github/gh-aw-firewall/squid:" + defaultTag,
},The expected digests would need to be looked up from the embedded table at runtime via the same applyContainerPins helper, or pulled from a helper that exposes the pin table lookup.
@copilot please address this.
| if err != nil { | ||
| t.Fatalf("Failed to read lock file: %v", err) | ||
| } | ||
|
|
There was a problem hiding this comment.
[/tdd] The manifest-header check uses a raw string-contains assertion that embeds JSON key ordering: "image":"...","digest":"...","pinned_image":"...". If the struct ever serialises in a different field order, the test fails for structural reasons unrelated to the actual pinning behaviour.
💡 Suggested improvement
Assert each field independently:
assert.Contains(t, yamlStr, `"image":"`+image+`"`)
assert.Contains(t, yamlStr, `"digest":"`+digest+`"`)
assert.Contains(t, yamlStr, `"pinned_image":"`+pinnedImage+`"`)Or deserialise the YAML and inspect the manifest header struct directly, which is more robust and gives clearer failure messages.
@copilot please address this.
| `imageTag`, | ||
| defaultTag + `,`, | ||
| `agent=sha256:55f06588411008b7148eb64b8dfe28602a0cce3675b36c6b190b54aca138468e`, | ||
| `api-proxy=sha256:afb9ff9140b17d38871dfb9dbac5ff8689ea634c2f91c435da2825192d4881c1`, |
There was a problem hiding this comment.
[/tdd] The imageTag assertion checks substrings like defaultTag + "," and individual digest fragments rather than verifying the complete, correctly-formatted imageTag value. A partial or malformed imageTag (e.g. missing one image, wrong separator) would still pass all checks.
💡 Suggested improvement
Build the expected full imageTag value and assert the entire string is present:
expectedImageTag := fmt.Sprintf(
"%s,agent=%s,api-proxy=%s,cli-proxy=%s,squid=%s",
defaultTag,
"sha256:55f065...",
"sha256:afb9ff...",
"sha256:e23e16...",
"sha256:3cdcc1...",
)
assert.Contains(t, yamlStr, `"imageTag":"`+expectedImageTag+`"`)This validates the complete, properly ordered value rather than disjoint fragments.
@copilot please address this.
|
|
||
| compiler := NewCompiler() | ||
| if err := compiler.CompileWorkflow(testFile); err != nil { | ||
| t.Fatalf("Failed to compile workflow: %v", err) |
There was a problem hiding this comment.
[/tdd] The iteration over a map (expectedPins) is non-deterministic in Go. If a future failure message names an image, the order may vary across runs, making flaky-test diagnosis harder. The existing TestCompileWorkflow_FirewallImagesPinnedForAWF0270 has the same pattern, but given this is a new test it is worth noting.
💡 Suggested improvement
Define expected pairs in a deterministic slice instead:
type imagePin struct{ image, digest string }
expectedPins := []imagePin{
{registry + "/agent:" + defaultTag, "sha256:55f065..."},
{registry + "/api-proxy:" + defaultTag, "sha256:afb9ff..."},
{registry + "/cli-proxy:" + defaultTag, "sha256:e23e16..."},
{registry + "/squid:" + defaultTag, "sha256:3cdcc1..."},
}
for _, ep := range expectedPins { ... }Predictable ordering makes test output easier to read and diff.
@copilot please address this.
There was a problem hiding this comment.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 178.1 AIC · ⌖ 6.06 AIC · ⊞ 4.9K
| // Verify the AWF config imageTag field includes digests for all four sidecar images. | ||
| for _, imageTagPart := range []string{ | ||
| `imageTag`, | ||
| defaultTag + `,`, |
There was a problem hiding this comment.
The imageTag assertions (lines 153–162) hardcode the same four SHA-256 digests that were already collected into expectedPins two blocks above. Consider reusing expectedPins to keep the two assertion sets in sync automatically — if a digest changes in action_pins.json, the fix currently must be applied in two places in this function, risking inconsistency.
@copilot please address this.
|
Hey Both the compile-level test ( This PR looks ready for review. ✅
|
|
🛠️ Agentic Maintenance updated this pull request branch. |
In v0.82.2, the four
gh-aw-firewallsidecar images (agent,api-proxy,cli-proxy,squid) were emitted tag-only (nodigest/pinned_image) in consumer-compiled lock files. The embedded pins inaction_pins.jsonare correct for0.27.22; the gap was that the only compile-level regression test targeted the older pinned versionv0.27.0, leaving the current default and the newcli-proxyimage (added in v0.82) uncovered.Changes
docker_firewall_pin_compile_test.go— AddsTestCompileWorkflow_FirewallImagesPinnedForDefaultVersion: compiles a workflow at the default AWF version withfeatures.cli-proxy: trueand asserts all four firewall images appear with correctdigest/pinned_imagein the manifest header, download comment, and AWF configimageTagfield.docker_pin_test.go— Adds a table caseall four default firewall images (v0.27.22) pinned from embedded tabletoTestApplyContainerPins, verifying each image resolves to its expected SHA-256 via the embedded pins with no cache present..changeset/patch-fix-firewall-digest-pinning-0-27-22.md— Changeset entry.