diff --git a/docs/adr/43871-actionpins-spec-tests-verify-behavioral-contracts.md b/docs/adr/43871-actionpins-spec-tests-verify-behavioral-contracts.md new file mode 100644 index 00000000000..dbf7afc2b12 --- /dev/null +++ b/docs/adr/43871-actionpins-spec-tests-verify-behavioral-contracts.md @@ -0,0 +1,49 @@ +# ADR-43871: actionpins Spec Tests Verify Behavioral Contracts, Not Structural Type Assertions + +**Date**: 2026-07-07 +**Status**: Draft +**Deciders**: pelikhan, copilot-swe-agent + +--- + +### Context + +The `pkg/actionpins` package exposes a public API for resolving and formatting action pin references (SHA passthrough, embedded fallback, dynamic resolution, context propagation, mapping application). Prior spec tests included a category of low-signal assertions that validated struct field values by constructing a type and asserting each field matched what was just set (e.g., `ActionPin`, `ActionYAMLInput`, `ContainerPin`, `ResolutionFailure`). These tests did not exercise resolution logic or behavioral contracts—they could pass even if the resolution functions had serious bugs. Meanwhile, several high-value resolution paths (unknown full-SHA passthrough, invalid mapping skip, `PinContext.Ctx` forwarding to the resolver, `ResolveLatestActionPin` fallback on enforce error) had no coverage. + +### Decision + +We will remove struct-field-accessor tests that offer no behavioral signal and replace the coverage budget with tests that exercise the resolution API's observable contracts: SHA passthrough format, invalid mapping skip behavior, `PinContext.Ctx` exact forwarding (identity and cancellation state), `ResolveLatestActionPin` fallback-on-error, and enforce-mode failure classification. We will also consolidate repeated enforce-mode sub-tests into a table-driven test and replace manual channel synchronization in the concurrency test with `sync.WaitGroup`. + +### Alternatives Considered + +#### Alternative 1: Keep Struct-Field Tests and Add Behavioral Tests on Top + +Preserve all existing struct-field assertions and add the new behavioral tests alongside them. This would increase total test count without reducing noise. + +Not chosen because it retains low-signal tests that obscure the spec's intent, increase maintenance cost when type shapes evolve, and provide false confidence. The signal-to-noise ratio of the test file degrades; reviewers must sift through field-assertion boilerplate to find the behavioral contracts. + +#### Alternative 2: Elevate to Integration Tests at a Higher Call Level + +Replace unit-level public-API tests with integration-style tests that exercise the full resolution pipeline from an end-to-end workflow fixture (e.g., a real GitHub Actions workflow YAML with mixed pinned and unpinned references). + +Not chosen because integration tests at the workflow level would be slower, harder to isolate, and would not exercise the specific behavioral branches (exact context forwarding, invalid mapping, fallback-after-error) as precisely. The public API surface of `actionpins` is the right contract boundary for specification tests. + +### Consequences + +#### Positive +- Tests now document and enforce observable behavioral contracts (what the API does) rather than structural facts (what types look like), making them more likely to catch real regressions. +- Table-driven refactor of `ResolveActionPin_EnforcePinned` reduces repetition and makes adding new enforce-mode scenarios a one-line struct literal. +- `sync.WaitGroup` pattern in the concurrency test is idiomatic Go and removes the need for a goroutine-count channel that could theoretically produce false positives. +- Newly covered resolution paths (SHA passthrough, mapping skip, context forwarding, fallback-on-error) close gaps that previously allowed silent behavioral regressions. + +#### Negative +- Struct type shapes (`ActionPin`, `ActionYAMLInput`, `ContainerPin`, `ResolutionFailure`) are no longer explicitly validated in tests; if a field is renamed or removed, only the behavioral tests that happen to use that field will catch it. +- The test helper `testSHAResolver` now captures call arguments (`capturedCtx`, `capturedRepo`, `capturedRef`), making it a stateful stub; this works for single-call tests but would require reset logic for multi-call scenarios. + +#### Neutral +- The test file line count changes from more lines of lower-signal assertions to fewer, more precise behavioral assertions — net delta is still a modest addition because new coverage was added. +- `require.NotEmpty` replaces `assert.NotEmpty` at points where follow-on assertions depend on a non-empty result; this is a test-correctness improvement that prevents misleading assertion failures on nil dereference but does not change the behavioral contract being tested. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* diff --git a/pkg/actionpins/spec_test.go b/pkg/actionpins/spec_test.go index f627708228e..ffb0ea0cf4f 100644 --- a/pkg/actionpins/spec_test.go +++ b/pkg/actionpins/spec_test.go @@ -6,6 +6,7 @@ import ( "context" "errors" "strings" + "sync" "testing" "github.com/stretchr/testify/assert" @@ -14,6 +15,12 @@ import ( "github.com/github/gh-aw/pkg/actionpins" ) +type testContextKey string + +const testContextPropagationKey testContextKey = "actionpins.resolve.ctx" + +const testResolvedSHA = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" + // TestSpec_PublicAPI_FormatPinnedActionReference validates the documented format "repo@sha # version". func TestSpec_PublicAPI_FormatPinnedActionReference(t *testing.T) { tests := []struct { @@ -227,53 +234,89 @@ func TestSpec_PublicAPI_ResolveActionPin_NilContext(t *testing.T) { "nil ctx should resolve from embedded pins with correct SHA and format") } +// TestSpec_PublicAPI_ResolveActionPin_UnknownFullSHAReturnsFormattedReference validates that +// an unknown full SHA is returned in the formatted "repo@sha # sha" form when it does not +// appear in the embedded pins. +func TestSpec_PublicAPI_ResolveActionPin_UnknownFullSHAReturnsFormattedReference(t *testing.T) { + unknownSHA := "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + + result, err := actionpins.ResolveActionPin("actions/checkout", unknownSHA, nil) + require.NoError(t, err) + assert.Equal(t, + actionpins.FormatPinnedActionReference("actions/checkout", unknownSHA, unknownSHA), + result, + "unknown SHA should be returned as repo@sha # sha") +} + // TestSpec_PublicAPI_ResolveActionPin_EnforcePinned validates unresolved pin handling in enforce mode. func TestSpec_PublicAPI_ResolveActionPin_EnforcePinned(t *testing.T) { - t.Run("returns error when EnforcePinned=true and pin is unresolved", func(t *testing.T) { - var failures []actionpins.ResolutionFailure - ctx := &actionpins.PinContext{ - EnforcePinned: true, - Warnings: make(map[string]bool), - RecordResolutionFailure: func(f actionpins.ResolutionFailure) { - failures = append(failures, f) - }, - } - _, err := actionpins.ResolveActionPin("does-not-exist/x", "v1", ctx) - require.Error(t, err, "enforce mode should return an error when pin is unresolved") - assert.Contains(t, err.Error(), "unable to pin action", - "enforce mode error should mention unable to pin action") - require.Len(t, failures, 1, "failure should be audited when enforce mode errors") - assert.Equal(t, actionpins.ResolutionErrorTypePinNotFound, failures[0].ErrorType, - "unresolved pin in enforce mode should audit pin_not_found") - }) - - t.Run("AllowActionRefs downgrades to warning with no error", func(t *testing.T) { - ctx := &actionpins.PinContext{EnforcePinned: true, AllowActionRefs: true, Warnings: make(map[string]bool)} - result, err := actionpins.ResolveActionPin("does-not-exist/x", "v1", ctx) - require.NoError(t, err, "AllowActionRefs should downgrade unresolved pin enforcement to warning") - assert.Empty(t, result, "AllowActionRefs downgrade should keep unresolved result empty") - assert.True(t, ctx.Warnings[actionpins.FormatCacheKey("does-not-exist/x", "v1")], - "AllowActionRefs should record warning dedup key") - }) + tests := []struct { + name string + resolver actionpins.SHAResolver + allowActionRefs bool + wantErr bool + wantErrContains string + wantFailureType actionpins.ResolutionErrorType + wantFailureCount int + wantWarningKey bool + }{ + { + name: "returns error when EnforcePinned=true and pin is unresolved", + wantErr: true, + wantErrContains: "unable to pin action", + wantFailureType: actionpins.ResolutionErrorTypePinNotFound, + wantFailureCount: 1, + }, + { + name: "AllowActionRefs downgrades to warning with no error", + allowActionRefs: true, + wantFailureType: actionpins.ResolutionErrorTypePinNotFound, + wantFailureCount: 1, + wantWarningKey: true, + }, + { + name: "dynamic resolver fails with EnforcePinned=true returns error", + resolver: &testSHAResolver{err: errors.New("network error")}, + wantErr: true, + wantErrContains: "unable to pin action", + wantFailureType: actionpins.ResolutionErrorTypeDynamicResolutionFailed, + wantFailureCount: 1, + }, + } - t.Run("dynamic resolver fails with EnforcePinned=true returns error", func(t *testing.T) { - var failures []actionpins.ResolutionFailure - ctx := &actionpins.PinContext{ - Resolver: &testSHAResolver{err: errors.New("network error")}, - EnforcePinned: true, - Warnings: make(map[string]bool), - RecordResolutionFailure: func(f actionpins.ResolutionFailure) { - failures = append(failures, f) - }, - } - _, err := actionpins.ResolveActionPin("does-not-exist/x", "v1", ctx) - require.Error(t, err, "EnforcePinned should return an error when dynamic resolution fails") - assert.Contains(t, err.Error(), "unable to pin action", - "error message should mention unable to pin action") - require.Len(t, failures, 1, "failure should be audited when dynamic resolution fails in enforce mode") - assert.Equal(t, actionpins.ResolutionErrorTypeDynamicResolutionFailed, failures[0].ErrorType, - "dynamic resolver failure should classify as dynamic_resolution_failed") - }) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var failures []actionpins.ResolutionFailure + ctx := &actionpins.PinContext{ + Resolver: tt.resolver, + EnforcePinned: true, + AllowActionRefs: tt.allowActionRefs, + Warnings: make(map[string]bool), + RecordResolutionFailure: func(f actionpins.ResolutionFailure) { + failures = append(failures, f) + }, + } + + result, err := actionpins.ResolveActionPin("does-not-exist/x", "v1", ctx) + if tt.wantErr { + require.Error(t, err, "enforce mode should return an error for this scenario") + assert.Contains(t, err.Error(), tt.wantErrContains) + assert.Empty(t, result, "erroring enforce mode should not return a pinned reference") + } else { + require.NoError(t, err, "AllowActionRefs should downgrade unresolved pin enforcement to a warning") + assert.Empty(t, result, "downgraded unresolved result should remain empty") + } + + require.Len(t, failures, tt.wantFailureCount, "resolution failures should be audited consistently") + if tt.wantFailureCount > 0 { + assert.Equal(t, tt.wantFailureType, failures[0].ErrorType) + } + if tt.wantWarningKey { + assert.True(t, ctx.Warnings[actionpins.FormatCacheKey("does-not-exist/x", "v1")], + "warning downgrade should record the dedup key") + } + }) + } } // TestSpec_PublicAPI_ResolveActionPin_SkipHardcodedFallback validates that setting @@ -300,7 +343,7 @@ func TestSpec_PublicAPI_ResolveActionPin_SkipHardcodedFallback(t *testing.T) { } result, err := actionpins.ResolveActionPin("actions/checkout", "v4", ctx) require.NoError(t, err, "hardcoded pin lookup should succeed when SkipHardcodedFallback is false") - assert.NotEmpty(t, result, "SkipHardcodedFallback=false should allow hardcoded pin lookup") + require.NotEmpty(t, result, "SkipHardcodedFallback=false should allow hardcoded pin lookup") assert.Contains(t, result, "actions/checkout@", "result should reference actions/checkout") }) } @@ -323,6 +366,36 @@ func TestSpec_PublicAPI_ResolveLatestActionPin(t *testing.T) { }) } +// TestSpec_PublicAPI_ResolveLatestActionPin_FallbackOnEnforceError validates that +// ResolveLatestActionPin falls back to the embedded latest-pin reference when ResolveActionPin +// errors, and returns empty when no embedded fallback exists. +func TestSpec_PublicAPI_ResolveLatestActionPin_FallbackOnEnforceError(t *testing.T) { + t.Run("known repo falls back to embedded latest pin after enforce error", func(t *testing.T) { + known := "actions/checkout" + latestPin, ok := actionpins.GetLatestActionPinByRepo(known) + require.True(t, ok, "expected embedded pins for known repository") + + ctx := &actionpins.PinContext{ + Resolver: &testSHAResolver{err: errors.New("network error")}, + EnforcePinned: true, + SkipHardcodedFallback: true, + Warnings: make(map[string]bool), + } + result := actionpins.ResolveLatestActionPin(known, ctx) + assert.Equal(t, + actionpins.FormatPinnedActionReference(known, latestPin.SHA, latestPin.Version), + result, + "known repo should fall back to the embedded latest pin when ResolveActionPin errors") + }) + + t.Run("unknown repo returns empty when no embedded fallback exists", func(t *testing.T) { + ctx := &actionpins.PinContext{EnforcePinned: true, Warnings: make(map[string]bool)} + + result := actionpins.ResolveLatestActionPin("does-not-exist/x", ctx) + assert.Empty(t, result, "unknown repo should return empty when no embedded fallback exists") + }) +} + // TestSpec_Types_PinContext validates the documented PinContext type fields. func TestSpec_Types_PinContext(t *testing.T) { t.Run("strict mode disables non-exact fallback", func(t *testing.T) { @@ -363,33 +436,6 @@ func TestSpec_DesignDecision_FormatConsistency(t *testing.T) { assert.Contains(t, reference, version, "reference should contain version comment") } -// TestSpec_Types_ActionPin validates the documented ActionPin type structure. -// Spec: Repo, Version, SHA fields plus optional Inputs map. -func TestSpec_Types_ActionPin(t *testing.T) { - pin := actionpins.ActionPin{ - Repo: "actions/checkout", - Version: "v5", - SHA: "abcdef1234567890abcdef1234567890abcdef12", - } - assert.Equal(t, "actions/checkout", pin.Repo, "ActionPin.Repo field") - assert.Equal(t, "v5", pin.Version, "ActionPin.Version field") - assert.Equal(t, "abcdef1234567890abcdef1234567890abcdef12", pin.SHA, "ActionPin.SHA field") - assert.Nil(t, pin.Inputs, "ActionPin.Inputs should be nil when not set") -} - -// TestSpec_Types_ActionYAMLInput validates the documented ActionYAMLInput type structure. -// Spec: Description, Required, Default fields. -func TestSpec_Types_ActionYAMLInput(t *testing.T) { - input := actionpins.ActionYAMLInput{ - Description: "The branch to checkout", - Required: true, - Default: "main", - } - assert.Equal(t, "The branch to checkout", input.Description, "ActionYAMLInput.Description field") - assert.True(t, input.Required, "ActionYAMLInput.Required field") - assert.Equal(t, "main", input.Default, "ActionYAMLInput.Default field") -} - // TestSpec_Types_ActionPinsData validates the documented ActionPinsData container type. // Spec: ActionPinsData is a JSON container used to load embedded pin entries. func TestSpec_Types_ActionPinsData(t *testing.T) { @@ -421,17 +467,23 @@ func TestSpec_PublicAPI_ResolveActionPin_EmbeddedMatch(t *testing.T) { ctx := &actionpins.PinContext{StrictMode: false, Warnings: make(map[string]bool)} result, err := actionpins.ResolveActionPin(known, latestPin.Version, ctx) require.NoError(t, err, "embedded-only ResolveActionPin should not error for known pin") - assert.NotEmpty(t, result, "should return non-empty pinned reference for known embedded pin") + require.NotEmpty(t, result, "should return non-empty pinned reference for known embedded pin") assert.Contains(t, result, latestPin.SHA, "resolved reference should contain the pin SHA") } // testSHAResolver is a fake SHAResolver used in tests. type testSHAResolver struct { - sha string - err error + sha string + err error + capturedCtx context.Context + capturedRepo string + capturedRef string } -func (r *testSHAResolver) ResolveSHA(_ context.Context, _, _ string) (string, error) { +func (r *testSHAResolver) ResolveSHA(ctx context.Context, repo, version string) (string, error) { + r.capturedCtx = ctx + r.capturedRepo = repo + r.capturedRef = version return r.sha, r.err } @@ -499,19 +551,6 @@ func TestSpec_PublicAPI_GetContainerPin(t *testing.T) { }) } -// TestSpec_Types_ContainerPin validates the documented ContainerPin type structure. -// Spec: Image, Digest, PinnedImage fields. -func TestSpec_Types_ContainerPin(t *testing.T) { - pin := actionpins.ContainerPin{ - Image: "ghcr.io/some/image:v1", - Digest: "sha256:abc123", - PinnedImage: "ghcr.io/some/image@sha256:abc123", - } - assert.Equal(t, "ghcr.io/some/image:v1", pin.Image, "ContainerPin.Image field") - assert.Equal(t, "sha256:abc123", pin.Digest, "ContainerPin.Digest field") - assert.Equal(t, "ghcr.io/some/image@sha256:abc123", pin.PinnedImage, "ContainerPin.PinnedImage field") -} - // TestSpec_Constants_ResolutionErrorType validates the documented ResolutionErrorType constant values. // Spec table: ResolutionErrorTypeDynamicResolutionFailed="dynamic_resolution_failed", // ResolutionErrorTypePinNotFound="pin_not_found". @@ -522,19 +561,6 @@ func TestSpec_Constants_ResolutionErrorType(t *testing.T) { "ResolutionErrorTypePinNotFound should equal the documented value") } -// TestSpec_Types_ResolutionFailure validates the documented ResolutionFailure type structure. -// Spec: "Captures an unresolved action-ref pinning event (repo, ref, error type)". -func TestSpec_Types_ResolutionFailure(t *testing.T) { - failure := actionpins.ResolutionFailure{ - Repo: "unknown/action", - Ref: "v1", - ErrorType: actionpins.ResolutionErrorTypePinNotFound, - } - assert.Equal(t, "unknown/action", failure.Repo, "ResolutionFailure.Repo field") - assert.Equal(t, "v1", failure.Ref, "ResolutionFailure.Ref field") - assert.Equal(t, actionpins.ResolutionErrorTypePinNotFound, failure.ErrorType, "ResolutionFailure.ErrorType field") -} - // TestSpec_PublicAPI_RecordResolutionFailure validates the documented auditing behavior: // PinContext.RecordResolutionFailure collects ResolutionFailure events for unresolved pins, // classified with ResolutionErrorTypePinNotFound when no usable pin is found. @@ -585,18 +611,17 @@ func TestSpec_ThreadSafety_ConcurrentGetActionPinsByRepo(t *testing.T) { const goroutines = 10 const repo = "actions/checkout" results := make([][]actionpins.ActionPin, goroutines) - done := make(chan int, goroutines) + var wg sync.WaitGroup for i := range goroutines { + wg.Add(1) go func(idx int) { + defer wg.Done() results[idx] = actionpins.GetActionPinsByRepo(repo) - done <- idx }(i) } - for range goroutines { - <-done - } + wg.Wait() require.NotEmpty(t, results[0], "baseline goroutine 0 should return pins") for i := 1; i < goroutines; i++ { @@ -621,12 +646,36 @@ func TestSpec_PublicAPI_ResolveActionPin_DynamicHappyPath(t *testing.T) { } result, err := actionpins.ResolveActionPin(known, "v4", ctx) require.NoError(t, err, "dynamic resolver success should not return an error") - assert.NotEmpty(t, result, "should return a non-empty pinned reference when resolver succeeds") + require.NotEmpty(t, result, "should return a non-empty pinned reference when resolver succeeds") assert.Contains(t, result, latestPin.SHA, "result should contain the SHA returned by the resolver") assert.True(t, strings.HasPrefix(result, known+"@"), "result should start with repo@sha in the documented format") } +// TestSpec_PublicAPI_ResolveActionPin_UsesProvidedContext validates that PinContext.Ctx +// is forwarded to the resolver instead of being replaced with context.Background(). +func TestSpec_PublicAPI_ResolveActionPin_UsesProvidedContext(t *testing.T) { + resolver := &testSHAResolver{sha: testResolvedSHA} + baseCtx := context.WithValue(context.Background(), testContextPropagationKey, "context-propagation-marker") + providedCtx, cancel := context.WithCancel(baseCtx) + // Pre-cancel the context so the resolver can verify that ResolveActionPin forwards the + // exact caller-provided context object, including its cancellation state, not just values. + cancel() + + result, err := actionpins.ResolveActionPin("actions/checkout", "v4", &actionpins.PinContext{ + Ctx: providedCtx, + Resolver: resolver, + Warnings: make(map[string]bool), + }) + require.NoError(t, err) + assert.Same(t, providedCtx, resolver.capturedCtx, "resolver should receive the exact PinContext.Ctx value") + assert.Equal(t, context.Canceled, resolver.capturedCtx.Err(), "resolver should observe the canceled context") + assert.Equal(t, "context-propagation-marker", resolver.capturedCtx.Value(testContextPropagationKey), "resolver should receive context values") + assert.Equal(t, "actions/checkout", resolver.capturedRepo, "resolver should receive the original repo") + assert.Equal(t, "v4", resolver.capturedRef, "resolver should receive the original version") + assert.Contains(t, result, resolver.sha, "successful resolution should use the resolver-provided SHA") +} + // TestSpec_PublicAPI_ResolveLatestActionPin_NonNilContext validates that a non-nil PinContext // is forwarded correctly to the embedded pin resolution path. func TestSpec_PublicAPI_ResolveLatestActionPin_NonNilContext(t *testing.T) { @@ -708,4 +757,22 @@ func TestSpec_PublicAPI_ResolveActionPin_AppliesMapping(t *testing.T) { require.NoError(t, err) assert.Contains(t, result, "actions/checkout@", "result should reference the original repo when no mapping exists") }) + + t.Run("invalid mapping value is skipped", func(t *testing.T) { + baseline, err := actionpins.ResolveActionPin("actions/checkout", "v4", &actionpins.PinContext{ + Warnings: make(map[string]bool), + }) + require.NoError(t, err) + + ctx := &actionpins.PinContext{ + Warnings: make(map[string]bool), + Mappings: map[string]string{ + "actions/checkout@v4": "actions/checkout", + }, + } + result, err := actionpins.ResolveActionPin("actions/checkout", "v4", ctx) + require.NoError(t, err, "invalid mapping should be skipped without error") + assert.Equal(t, baseline, result, "invalid mapping should leave resolution behavior unchanged") + assert.NotContains(t, ctx.Warnings, "map:actions/checkout@v4", "invalid mappings should not record mapping notifications") + }) }