Improve actionpins internal test coverage for fallback and pin-resolution edge cases

docs/adr/42834-table-driven-subtests-for-actionpins-pin-resolution.md

@@ -0,0 +1,44 @@
+# ADR-42834: Table-Driven Subtests for actionpins Pin-Resolution Coverage
+
+**Date**: 2026-07-02
+**Status**: Draft
+**Deciders**: pelikhan (PR author)
+
+---
+
+### Context
+
+The `pkg/actionpins` package implements critical GitHub Actions pin-resolution logic: semver fallback selection, exact-match hardcoded-pin resolution, and per-call warning deduplication via `PinContext.Warnings`. Prior to this PR, several internal branches (`findCompatiblePin`, `resolveExactHardcodedPin`, `resolveNonStrictHardcodedPin`) had no test coverage, leaving regression risk invisible. Test functions that did exist used flat top-level assertions with no subtest grouping, making failure attribution coarse-grained. The package also emits user-visible warnings to stderr as a side effect of resolution; that behavior was entirely untested.
+
+### Decision
+
+We will convert actionpins internal tests to table-driven subtests (`t.Run`) for parameterized cases and introduce `testutil.CaptureStderr` for asserting stderr side-effects of resolution functions. New dedicated test functions will cover the previously untested branches: semver major-version fallback, exact-version and exact-SHA resolution, no-match paths, and warning-dedup semantics across repeated calls.
+
+### Alternatives Considered
+
+#### Alternative 1: Flat top-level test functions (status quo)
+
+Continue adding one `TestX` function per scenario without subtests. Simple to write and requires no `testutil` helper. Rejected because failure output names only the top-level function, making it harder to identify which of many inputs triggered the failure; it also encourages copy-paste duplication across similar cases.
+
+#### Alternative 2: Interface-based stderr mock / io.Writer injection
+
+Inject an `io.Writer` into the resolution functions to capture output in tests, rather than adding a `testutil.CaptureStderr` helper that redirects `os.Stderr`. Considered because it avoids OS-level file descriptor manipulation in tests. Rejected because it would require changing the signatures of internal (unexported) functions or adding an exported `Writer` field to `PinContext`, widening the public surface of the package solely for test concerns. The `testutil.CaptureStderr` approach keeps production code unchanged.
+
+### Consequences
+
+#### Positive
+- Subtests provide precise failure attribution (the failing subtest name identifies the exact input, e.g., `TestFindCompatiblePin_SemverFallback/exact-major`) with no extra tooling.
+- Previously untested resolution branches (semver fallback, exact-match, no-match, warning dedup) are now covered, reducing regression risk for critical pin-resolution logic.
+- Table-driven structure consolidates related cases (e.g., two separate `TestGetContainerPin_MCPGatewayV*` functions merged into one) and makes adding future cases low-friction.
+
+#### Negative
+- Test file grows significantly (+170 lines), increasing the maintenance surface.
+- `testutil.CaptureStderr` introduces a shared test-helper dependency; future changes to that utility can affect these tests indirectly.
+
+#### Neutral
+- No production code (`actionpins.go`) is modified; the decision affects only the test file and the `testutil` package import.
+- Adoption of `assert.Zero` over `assert.Equal(t, 0, ...)` is a stylistic cleanup bundled into this PR; no behavioral change.
+
+---
+
+*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*

pkg/actionpins/actionpins_internal_test.go

@@ -8,6 +8,7 @@ import (
 	"testing"
 
 	"github.com/github/gh-aw/pkg/constants"
+	"github.com/github/gh-aw/pkg/testutil"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -32,15 +33,33 @@ func TestBuildByRepoIndex_GroupsByRepoAndSortsDescending(t *testing.T) {
 }
 
 func TestCountPinKeyMismatches_ReturnsOnlyVersionMismatches(t *testing.T) {
-	entries := map[string]ActionPin{
-		"actions/checkout@v5": {Repo: "actions/checkout", Version: "v5", SHA: "sha-1"},
-		"actions/setup-go@v5": {Repo: "actions/setup-go", Version: "v4", SHA: "sha-2"},
-		"invalid-key":         {Repo: "actions/cache", Version: "v4", SHA: "sha-3"},
-	}
+	t.Run("returns zero for empty entries", func(t *testing.T) {
+		assert.Zero(t, countPinKeyMismatches(map[string]ActionPin{}), "Expected empty input to produce zero mismatches")
+	})
 
-	count := countPinKeyMismatches(entries)
+	t.Run("returns zero when all key versions match", func(t *testing.T) {
+		entries := map[string]ActionPin{
+			"actions/checkout@v5": {Repo: "actions/checkout", Version: "v5", SHA: "sha-1"},
+			"actions/setup-go@v4": {Repo: "actions/setup-go", Version: "v4", SHA: "sha-2"},
+		}
+		assert.Zero(t, countPinKeyMismatches(entries), "Expected zero mismatches when key versions match pin versions")
+	})
 
-	assert.Equal(t, 1, count, "Expected only one key/version mismatch to be counted")
+	t.Run("ignores invalid keys without version suffix", func(t *testing.T) {
+		entries := map[string]ActionPin{
+			"invalid-key": {Repo: "actions/cache", Version: "v4", SHA: "sha-3"},
+		}
+		assert.Zero(t, countPinKeyMismatches(entries), "Expected invalid keys without @version to be ignored")
+	})
+
+	t.Run("counts only true key-version mismatches", func(t *testing.T) {
+		entries := map[string]ActionPin{
+			"actions/checkout@v5": {Repo: "actions/checkout", Version: "v5", SHA: "sha-1"},
+			"actions/setup-go@v5": {Repo: "actions/setup-go", Version: "v4", SHA: "sha-2"},
+			"invalid-key":         {Repo: "actions/cache", Version: "v4", SHA: "sha-3"},
+		}
+		assert.Equal(t, 1, countPinKeyMismatches(entries), "Expected only one key/version mismatch to be counted")
+	})
 }
 
 func TestInitWarnings_InitializesAndPreservesMap(t *testing.T) {
@@ -113,6 +132,104 @@ func TestFormatPinnedActionWithResolution_ConsistentVersionComment(t *testing.T)
 	}
 }
 
+func TestFindCompatiblePin_SemverFallback(t *testing.T) {
+	pins := []ActionPin{
+		{Repo: "actions/checkout", Version: "v5.2.0", SHA: "sha-v5-2"},
+		{Repo: "actions/checkout", Version: "v5.0.0", SHA: "sha-v5-0"},
+		{Repo: "actions/checkout", Version: "v4.9.9", SHA: "sha-v4-9"},
+	}
+
+	tests := []struct {
+		name          string
+		version       string
+		wantFound     bool
+		wantVersion   string
+		availablePins []ActionPin
+	}{
+		{
+			name:          "exact-major",
+			version:       "v5",
+			wantFound:     true,
+			wantVersion:   "v5.2.0",
+			availablePins: pins,
+		},
+		{
+			// major-compatible-match: requesting v5.0.0 from a list that contains only v5.0.0 and
+			// v4.9.9 returns v5.0.0 — the function uses major-compatible matching, not exact-version
+			// matching. The only v5.x present happens to be v5.0.0, so the result looks exact.
+			name:        "major-compatible-match",
+			version:     "v5.0.0",
+			wantFound:   true,
+			wantVersion: "v5.0.0",
+			availablePins: []ActionPin{
+				{Repo: "actions/checkout", Version: "v5.0.0", SHA: "sha-v5-0"},
+				{Repo: "actions/checkout", Version: "v4.9.9", SHA: "sha-v4-9"},
+			},
+		},
+		{
+			// first-compatible-not-exact: requesting v5.0.0 from [v5.2.0, v5.0.0] returns v5.2.0
+			// because findCompatiblePin returns the first major-compatible match, not the exact one.
+			// This is a consequence of the pre-sorted (descending) slice contract; see
+			// returns-first-compatible-not-highest for the complementary case with an unsorted list.
+			name:          "first-compatible-not-exact",
+			version:       "v5.0.0",
+			wantFound:     true,
+			wantVersion:   "v5.2.0",
+			availablePins: pins,
+		},
+		{
+			// returns-first-compatible-not-highest: list order determines the result.
+			// With an unsorted list [v5.0.0, v5.2.0], requesting v5 returns v5.0.0 (the first
+			// major-compatible entry), not v5.2.0. Callers (e.g. resolveNonStrictHardcodedPin) must
+			// supply a pre-sorted (descending) slice via GetActionPinsByRepo to get the highest pin.
+			name:        "returns-first-compatible-not-highest",
+			version:     "v5",
+			wantFound:   true,
+			wantVersion: "v5.0.0",
+			availablePins: []ActionPin{
+				{Repo: "actions/checkout", Version: "v5.0.0", SHA: "sha-low"},
+				{Repo: "actions/checkout", Version: "v5.2.0", SHA: "sha-high"},
+			},
+		},
+		{
+			// minor-version-constraint: IsCompatible performs major-only comparison, so
+			// requesting v5.1 from [v5.2.0, v5.0.0] returns v5.2.0 (same major, first match).
+			name:        "minor-version-constraint",
+			version:     "v5.1",
+			wantFound:   true,
+			wantVersion: "v5.2.0",
+			availablePins: []ActionPin{
+				{Repo: "actions/checkout", Version: "v5.2.0", SHA: "sha-a"},
+				{Repo: "actions/checkout", Version: "v5.0.0", SHA: "sha-b"},
+			},
+		},
+		{
+			name:          "no-match",
+			version:       "v6",
+			wantFound:     false,
+			availablePins: pins,
+		},
+		{
+			name:          "empty-version",
+			version:       "",
+			wantFound:     false,
+			availablePins: pins,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			pin, found := findCompatiblePin(tt.availablePins, tt.version)
+			assert.Equal(t, tt.wantFound, found)
+			if tt.wantFound {
+				assert.Equal(t, tt.wantVersion, pin.Version)
+			} else {
+				assert.Equal(t, ActionPin{}, pin, "expected zero-value ActionPin when not found")
+			}
+		})
+	}
+}
+
 func TestFindVersionBySHA_ReturnsVersionForKnownSHA(t *testing.T) {
 	t.Run("returns version for a known SHA in embedded data", func(t *testing.T) {
 		pins := GetActionPinsByRepo("actions/checkout")
@@ -142,24 +259,33 @@ func TestGetContainerPin_ReturnsPinnedImage(t *testing.T) {
 	assert.Contains(t, pin.PinnedImage, "@sha256:", "Expected pinned image to include digest")
 }
 
-func TestGetContainerPin_MCPGatewayV036IsPinned(t *testing.T) {
-	const image = "ghcr.io/github/gh-aw-mcpg:v0.3.6"
-
-	pin, ok := GetContainerPin(image)
-	require.True(t, ok, "Expected embedded container pin for %s", image)
-	assert.Equal(t, image, pin.Image, "Expected image name to match key")
-	assert.Equal(t, "sha256:2bb8eef86006a4c5963c55616a9c51c32f27bfdecb023b8aa6f91f6718d9171c", pin.Digest, "Expected v0.3.6 digest to match")
-	assert.Equal(t, image+"@sha256:2bb8eef86006a4c5963c55616a9c51c32f27bfdecb023b8aa6f91f6718d9171c", pin.PinnedImage, "Expected pinned image to include v0.3.6 digest")
-}
-
-func TestGetContainerPin_MCPGatewayV039IsPinned(t *testing.T) {
-	const image = "ghcr.io/github/gh-aw-mcpg:v0.3.9"
+func TestGetContainerPin_MCPGatewayVersionsArePinned(t *testing.T) {
+	tests := []struct {
+		name   string
+		image  string
+		digest string
+	}{
+		{
+			name:   "v0.3.6",
+			image:  "ghcr.io/github/gh-aw-mcpg:v0.3.6",
+			digest: "sha256:2bb8eef86006a4c5963c55616a9c51c32f27bfdecb023b8aa6f91f6718d9171c",
+		},
+		{
+			name:   "v0.3.9",
+			image:  "ghcr.io/github/gh-aw-mcpg:v0.3.9",
+			digest: "sha256:64828b42a4482f58fab16509d7f8f495a6d97c972a98a68aff20543531ac0388",
+		},
+	}
 
-	pin, ok := GetContainerPin(image)
-	require.True(t, ok, "Expected embedded container pin for %s", image)
-	assert.Equal(t, image, pin.Image, "Expected image name to match key")
-	assert.Equal(t, "sha256:64828b42a4482f58fab16509d7f8f495a6d97c972a98a68aff20543531ac0388", pin.Digest, "Expected v0.3.9 digest to match")
-	assert.Equal(t, image+"@sha256:64828b42a4482f58fab16509d7f8f495a6d97c972a98a68aff20543531ac0388", pin.PinnedImage, "Expected pinned image to include v0.3.9 digest")
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			pin, ok := GetContainerPin(tt.image)
+			require.True(t, ok, "Expected embedded container pin for %s", tt.image)
+			assert.Equal(t, tt.image, pin.Image, "Expected image name to match key")
+			assert.Equal(t, tt.digest, pin.Digest, "Expected digest to match for %s", tt.image)
+			assert.Equal(t, tt.image+"@"+tt.digest, pin.PinnedImage, "Expected pinned image to include digest for %s", tt.image)
+		})
+	}
 }
 
 func TestGetContainerPin_DefaultMCPImagesArePinned(t *testing.T) {
@@ -199,7 +325,7 @@ func TestResolveActionPinDynamically_SkipsForSHAInput(t *testing.T) {
 
 	assert.False(t, ok, "Expected no dynamic resolution for SHA input")
 	assert.Empty(t, result, "Expected empty result when dynamic resolution is skipped")
-	assert.Equal(t, 0, resolver.called, "Expected resolver not to be called for SHA input")
+	assert.Zero(t, resolver.called, "Expected resolver not to be called for SHA input")
 }
 
 func TestResolveActionPinFromHardcodedPins_StrictModeNoFallback(t *testing.T) {
@@ -220,6 +346,94 @@ func TestResolveExactHardcodedPin_BySHA(t *testing.T) {
 	assert.Contains(t, result, "sha-v5", "Expected result to include matched SHA")
 }
 
+func TestResolveExactHardcodedPin_ByVersion(t *testing.T) {
+	pins := []ActionPin{{Repo: "actions/checkout", Version: "v5.0.0", SHA: "sha-v5"}}
+
+	result, ok := resolveExactHardcodedPin("actions/checkout", "v5.0.0", false, pins)
+
+	require.True(t, ok, "Expected exact version match to resolve")
+	assert.Contains(t, result, "sha-v5", "Expected result to include matched SHA")
+	assert.Contains(t, result, "v5.0.0", "Expected result to include matched version")
+}
+
+func TestResolveExactHardcodedPin_NoMatch(t *testing.T) {
+	pins := []ActionPin{{Repo: "actions/checkout", Version: "v5.0.0", SHA: "sha-v5"}}
+
+	result, ok := resolveExactHardcodedPin("actions/checkout", "v4.0.0", false, pins)
+
+	assert.False(t, ok, "Expected no resolution when version does not match and input is not SHA")
+	assert.Empty(t, result, "Expected empty result when no exact match is found")
+}
+
+func TestResolveExactHardcodedPin_VersionTakesPrecedenceOverSHA(t *testing.T) {
+	// When isAlreadySHA=false, only the version-match path runs; the SHA-match loop is
+	// skipped entirely. This test uses a pin whose Version and SHA fields are identical
+	// to make the path selection explicit: the version loop matches and returns before
+	// the SHA loop would ever execute.
+	pins := []ActionPin{{Repo: "actions/checkout", Version: "sha-v5", SHA: "sha-v5"}}
+	result, ok := resolveExactHardcodedPin("actions/checkout", "sha-v5", false, pins)
+	require.True(t, ok, "Expected version-match path to resolve when isAlreadySHA=false")
+	assert.Contains(t, result, "sha-v5", "Expected result to include the matched pin's SHA/version")
+}
+
+func TestResolveNonStrictHardcodedPin_SelectsHighestCompatible(t *testing.T) {
+	pins := []ActionPin{
+		{Repo: "actions/checkout", Version: "v5.2.0", SHA: "sha-v5-2"},
+		{Repo: "actions/checkout", Version: "v5.0.0", SHA: "sha-v5-0"},
+		{Repo: "actions/checkout", Version: "v4.9.9", SHA: "sha-v4-9"},
+	}
+
+	t.Run("uses semver-compatible pin when available", func(t *testing.T) {
+		ctx := &PinContext{Warnings: make(map[string]bool)}
+		var result string
+
+		stderrOutput := testutil.CaptureStderr(t, func() {
+			result = resolveNonStrictHardcodedPin("actions/checkout", "v5", pins, ctx)
+		})
+
+		assert.Contains(t, result, "sha-v5-2", "Expected highest semver-compatible pin to be selected")
+		assert.True(t, ctx.Warnings["actions/checkout@v5"], "Expected warning cache key to be recorded")
+		// Assert on the stable unstyled message fragment to avoid ANSI-code fragility.
+		assert.Contains(t, stderrOutput, "using hardcoded pin for actions/checkout@v5.2.0", "Expected warning to be emitted for compatible fallback")
+	})
+
+	t.Run("deduplicates warning on compatible path", func(t *testing.T) {
+		ctx := &PinContext{Warnings: make(map[string]bool)}
+
+		stderrOutput := testutil.CaptureStderr(t, func() {
+			resolveNonStrictHardcodedPin("actions/checkout", "v5", pins, ctx)
+			resolveNonStrictHardcodedPin("actions/checkout", "v5", pins, ctx)
+		})
+
+		assert.Equal(t, 1, strings.Count(stderrOutput, "using hardcoded pin for actions/checkout@v5.2.0"),
+			"Expected warning emitted exactly once for repeated compatible resolution")
+		assert.Len(t, ctx.Warnings, 1)
+	})
+}
+
+func TestResolveNonStrictHardcodedPin_FallsBackToHighestWhenNoCompatible(t *testing.T) {
+	pins := []ActionPin{
+		{Repo: "actions/checkout", Version: "v5.2.0", SHA: "sha-v5-2"},
+		{Repo: "actions/checkout", Version: "v5.0.0", SHA: "sha-v5-0"},
+		{Repo: "actions/checkout", Version: "v4.9.9", SHA: "sha-v4-9"},
+	}
+
+	ctx := &PinContext{Warnings: make(map[string]bool)}
+	var first, second string
+
+	stderrOutput := testutil.CaptureStderr(t, func() {
+		first = resolveNonStrictHardcodedPin("actions/checkout", "v9", pins, ctx)
+		second = resolveNonStrictHardcodedPin("actions/checkout", "v9", pins, ctx)
+	})
+
+	assert.Contains(t, first, "sha-v5-2", "Expected fallback to highest available pin when no compatible version exists")
+	assert.Contains(t, second, "sha-v5-2", "Expected consistent fallback result on repeated calls")
+	assert.True(t, ctx.Warnings["actions/checkout@v9"], "Expected warning cache key to be recorded")
+	assert.Len(t, ctx.Warnings, 1, "Expected warning deduplication for repeated resolution attempts")
+	// Assert on the stable unstyled message fragment to avoid ANSI-code fragility.
+	assert.Equal(t, 1, strings.Count(stderrOutput, "using hardcoded pin for actions/checkout@v5.2.0"), "Expected warning to be emitted exactly once for repeated fallback resolution")
+}
+
 func TestResolveActionPinFromHardcodedPins_SkipHardcodedFallback(t *testing.T) {
 	t.Run("returns false immediately when SkipHardcodedFallback is set", func(t *testing.T) {
 		ctx := &PinContext{SkipHardcodedFallback: true, Warnings: make(map[string]bool)}