Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 49 additions & 0 deletions docs/adr/43343-regression-tests-for-firewall-digest-pinning.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
# ADR-43343: Two-Layer Regression Tests for gh-aw-firewall Digest Pinning

**Date**: 2026-07-04
**Status**: Draft
**Deciders**: pelikhan, copilot-swe-agent

---

### Context

Consumer-compiled lock files produced by gh-aw v0.82.2 emitted all four `gh-aw-firewall` images (`agent`, `api-proxy`, `cli-proxy`, `squid`) with tag-only references — no `digest` or `pinned_image` field. The embedded fallback pins in `action_pins.json` were already correct for the default version (`0.27.22`), but the existing regression tests only covered the older `0.27.0` version and did not cover `cli-proxy` (introduced in v0.82) at all. This coverage gap meant the regression shipped silently: no test failed, yet consumers received unpinned images. Without a test guard, any future `DefaultFirewallVersion` bump or sidecar addition carries the same silent-regression risk.

### Decision

We will add two regression tests tied to `constants.DefaultFirewallVersion` that together verify end-to-end digest pinning for all current firewall sidecars:

1. **Unit test** (`TestApplyContainerPins_DefaultFirewallVersion` in `docker_pin_test.go`) — asserts that each of the four images has a valid entry in the embedded pin table (non-empty `Digest` and `PinnedImage`), confirming `action_pins.json` coverage.
2. **End-to-end compile test** (`TestCompileWorkflow_FirewallImagesPinnedForDefaultVersion` in `docker_firewall_pin_compile_test.go`) — compiles a minimal workflow with `tools.github.mode: gh-proxy` (so `cli-proxy` is included) and asserts that the resulting lock file contains full `image`/`digest`/`pinned_image` metadata and correctly encoded `--image-tag` AWF config for all four images.

By binding both tests to `constants.DefaultFirewallVersion`, any version bump that does not refresh `action_pins.json` will immediately cause CI failures rather than silently shipping unpinned images.

### Alternatives Considered

#### Alternative 1: Fix action_pins.json without adding tests

Correct the embedded pin data for `0.27.22` and `cli-proxy` without introducing new tests. This removes the immediate regression but leaves the root cause — absence of a test guard — unaddressed. The next `DefaultFirewallVersion` bump or new sidecar addition would carry the same silent-regression risk. Rejected because it treats the symptom rather than the systemic gap.

#### Alternative 2: Unit-only test (skip the compile test)

Add the pin-table unit test (`TestApplyContainerPins_DefaultFirewallVersion`) but omit the compile test. This would catch missing `action_pins.json` entries but would not detect integration-level failures where the pin table is populated yet the compiler fails to emit `digest`/`pinned_image` fields into the lock file. The compile test provides a separate failure mode that the unit test cannot cover. Rejected because the historical regression was an integration-level failure (correct pin data, incorrect lock-file output), which only an end-to-end compile test would have caught.

### Consequences

#### Positive
- Future `DefaultFirewallVersion` bumps will fail CI immediately if `action_pins.json` is not updated with correct digests for all sidecars.
- The `cli-proxy` sidecar (new in v0.82) is now explicitly covered alongside the three legacy images, closing the coverage gap that allowed the v0.82.2 regression.
- Both tests use `constants.DefaultFirewallVersion` rather than a hardcoded version string, so they automatically track the canonical version constant.

#### Negative
- The compile test hardcodes specific digest SHA-256 values for the `0.27.22` release; when firewall images are rebuilt (e.g., base-image security patches), the test expected values must be updated even if the logic is unchanged.
- Two test layers (unit + compile) increase the test maintenance surface. Every time the compiler's lock-file schema changes, both tests may need updating.

#### Neutral
- The compile test requires `tools.github.mode: gh-proxy` in the workflow frontmatter; this is a test-only detail that does not affect production behavior.
- No production source files were changed in this PR — the change is confined to `_test.go` files, so there is no runtime impact on consumers.

---

*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*
79 changes: 79 additions & 0 deletions pkg/workflow/docker_firewall_pin_compile_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"strings"
"testing"

"github.com/github/gh-aw/pkg/constants"
"github.com/github/gh-aw/pkg/stringutil"
"github.com/github/gh-aw/pkg/testutil"
)
Expand Down Expand Up @@ -80,3 +81,81 @@ Test workflow.`
}
}
}

// TestCompileWorkflow_FirewallImagesPinnedForDefaultVersion is a regression test for
// gh-aw#43307: the four gh-aw-firewall images at the current default version

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.

// (constants.DefaultFirewallVersion) must all be digest-pinned in consumer lock files
// even when no local action-cache is present. This covers the cli-proxy image
// introduced in v0.82 as well as the three legacy images (agent, api-proxy, squid).
func TestCompileWorkflow_FirewallImagesPinnedForDefaultVersion(t *testing.T) {
// Strip the leading "v" to get the Docker image tag (mirrors getAWFImageTag).
imageTag := strings.TrimPrefix(string(constants.DefaultFirewallVersion), "v")

// Enable tools.github.mode=gh-proxy so that the cli-proxy sidecar container is
// included in the Docker pull list and therefore also pinned in the lock file.
frontmatter := `---
on: workflow_dispatch
engine: claude
network:
allowed:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.

- defaults
tools:
github:
mode: gh-proxy
---

# Test

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.

Test workflow.`

tmpDir := testutil.TempDir(t, "docker-firewall-pins-default-version-test")
testFile := filepath.Join(tmpDir, "test-workflow.md")
if err := os.WriteFile(testFile, []byte(frontmatter), 0644); err != nil {
t.Fatal(err)
}

compiler := NewCompiler()
if err := compiler.CompileWorkflow(testFile); err != nil {
t.Fatalf("Failed to compile workflow: %v", err)
}

lockFile := stringutil.MarkdownToLockFile(testFile)
yaml, err := os.ReadFile(lockFile)
if err != nil {
t.Fatalf("Failed to read lock file: %v", err)
}

yamlStr := string(yaml)

expectedPins := map[string]string{
"ghcr.io/github/gh-aw-firewall/agent:" + imageTag: "sha256:55f06588411008b7148eb64b8dfe28602a0cce3675b36c6b190b54aca138468e",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"ghcr.io/github/gh-aw-firewall/api-proxy:" + imageTag: "sha256:afb9ff9140b17d38871dfb9dbac5ff8689ea634c2f91c435da2825192d4881c1",
"ghcr.io/github/gh-aw-firewall/cli-proxy:" + imageTag: "sha256:e23e1604241f579b418e6522d938285b57ada31bc27742a65c90ee2250b1755c",
"ghcr.io/github/gh-aw-firewall/squid:" + imageTag: "sha256:3cdcc1e2b4b4fe602ba69fd3e21aac7ac512d5c1fce24df4ce69dc4f98164b59",
}

for image, digest := range expectedPins {
pinnedImage := image + "@" + digest
if !strings.Contains(yamlStr, `"image":"`+image+`","digest":"`+digest+`","pinned_image":"`+pinnedImage+`"`) {
t.Errorf("Expected manifest header to include pinned metadata for %s", image)
}
if !strings.Contains(yamlStr, "# - "+pinnedImage) {
t.Errorf("Expected pinned container comment for %s", image)
}
if !strings.Contains(yamlStr, pinnedImage) {
t.Errorf("Expected pinned download reference for %s", image)
}
}

for _, imageTagPart := range []string{
`imageTag`,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

imageTag + `,`,
`agent=sha256:55f06588411008b7148eb64b8dfe28602a0cce3675b36c6b190b54aca138468e`,
`api-proxy=sha256:afb9ff9140b17d38871dfb9dbac5ff8689ea634c2f91c435da2825192d4881c1`,
`cli-proxy=sha256:e23e1604241f579b418e6522d938285b57ada31bc27742a65c90ee2250b1755c`,
`squid=sha256:3cdcc1e2b4b4fe602ba69fd3e21aac7ac512d5c1fce24df4ce69dc4f98164b59`,
} {
if !strings.Contains(yamlStr, imageTagPart) {
t.Errorf("Expected AWF config JSON to include %s", imageTagPart)
}
}
}
26 changes: 26 additions & 0 deletions pkg/workflow/docker_pin_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
package workflow

import (
"strings"
"testing"

"github.com/github/gh-aw/pkg/constants"
Expand Down Expand Up @@ -105,6 +106,31 @@ func TestApplyContainerPins(t *testing.T) {
}
}

// TestApplyContainerPins_DefaultFirewallVersion is a regression test for gh-aw#43307:
// all four gh-aw-firewall images at constants.DefaultFirewallVersion (including cli-proxy,
// which was new in v0.82) must have entries in the embedded pin table so that consumer
// compiles without a local cache still emit digest-pinned references.
// 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"}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Intentional omission: If agent-act is no longer used at the default version, remove it from the specs list in buildAWFImageTagWithDigests so there's no silent skip.
  2. Gap to fix: Add agent-act:0.27.22 to action_pins.json and add "agent-act" to the sidecar list in both tests with an assertion that its digest appears in the AWF config imageTag.

Either way, update the test comment to accurately describe what images are (and are not) covered.


for _, sidecar := range sidecars {
image := constants.DefaultFirewallRegistry + "/" + sidecar + ":" + imageTag
t.Run(sidecar, func(t *testing.T) {
pin, ok := getEmbeddedContainerPin(image)
require.True(t, ok, "embedded pin must exist for %s", image)
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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.

require.Len(t, refs, 1)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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)
Comment on lines +123 to +129
})
}
}

// TestCollectDockerImages_StoresInWorkflowData verifies that collectDockerImages
// populates workflowData.DockerImages and DockerImagePins with the collected image refs.
func TestCollectDockerImages_StoresInWorkflowData(t *testing.T) {
Expand Down
Loading