[fp-enhancer] Improve pkg/actionpins: extract pure helpers, eliminate duplicate init

docs/adr/27523-extract-pure-helpers-in-actionpins.md

@@ -0,0 +1,73 @@
+# ADR-27523: Extract Pure Helper Functions in pkg/actionpins
+
+**Date**: 2026-04-21
+**Status**: Draft
+**Deciders**: pelikhan
+
+---
+
+## Part 1 — Narrative (Human-Friendly)
+
+### Context
+
+`pkg/actionpins` initializes cached state through `sync.Once` closures. Over time, the closures accumulated inline logic — grouping pins by repository, validating key/version consistency, and nil-initializing warning maps — that could not be exercised in isolation. Two separate `if ctx.Warnings == nil` guards existed in different branches of `ResolveActionPin`, creating a latent risk that a new branch would miss the guard. The package is part of an ongoing functional-programming (FP) enhancer cycle applied across the repository's packages.
+
+### Decision
+
+We will extract the three inline behaviours — `buildByRepoIndex`, `countPinKeyMismatches`, and `initWarnings` — from `sync.Once` closures and duplicated call sites into named, pure (or near-pure) package-level helper functions. The extracted helpers take their inputs as parameters and return their outputs; they carry no hidden state and produce no observable side effects beyond their return value and diagnostic logging. The `sync.Once` closure becomes a thin orchestration shell that calls these helpers. This approach makes each helper independently unit-testable and removes the duplicated nil-guard pattern.
+
+### Alternatives Considered
+
+#### Alternative 1: Keep Logic Inline
+
+The status quo. All logic remains embedded in the `sync.Once` closures and at each `ResolveActionPin` branch. No new functions are introduced. This was rejected because it preserves the untestability of the business logic and keeps the duplicate nil-guard risk; adding further validations would continue to grow the closure body.
+
+#### Alternative 2: Struct-Based Lazy Initializer
+
+Wrap the cached state and its initialisation in a dedicated struct type with a method that performs the `sync.Once` work. This provides stronger encapsulation and would group related state. It was not chosen because it represents a larger structural change that goes beyond the zero-behavior-change constraint of this refactor cycle, and the package is not large enough yet to justify the added indirection.
+
+### Consequences
+
+#### Positive
+- `buildByRepoIndex` and `countPinKeyMismatches` can now be unit-tested directly without triggering global cache side-effects.
+- The single `initWarnings` call site eliminates the duplicate nil-guard pattern, reducing the chance a future code path omits the guard.
+- The `sync.Once` closure body shrinks to high-level orchestration, improving readability.
+
+#### Negative
+- The package surface area grows with three additional exported-package-internal functions (visible within the package only), adding names that future contributors must learn.
+- The diagnostic logging inside `countPinKeyMismatches` is a side effect that makes the function impure; tests that call it will emit log output unless logging is redirected.
+
+#### Neutral
+- No behaviour change: all existing tests continue to pass without modification.
+- The change is scoped to `pkg/actionpins`; the ongoing FP enhancer cycle is expected to apply similar patterns to `pkg/agentdrain` and subsequent packages.
+
+---
+
+## Part 2 — Normative Specification (RFC 2119)
+
+> The key words **MUST**, **MUST NOT**, **REQUIRED**, **SHALL**, **SHALL NOT**, **SHOULD**, **SHOULD NOT**, **RECOMMENDED**, **MAY**, and **OPTIONAL** in this section are to be interpreted as described in [RFC 2119](https://www.rfc-editor.org/rfc/rfc2119).
+
+### Helper Function Design
+
+1. Extracted helper functions **MUST** accept all required data as explicit parameters and **MUST NOT** read or write package-level variables directly.
+2. Helper functions that compute a value **MUST** return that value rather than mutating a caller-provided variable.
+3. Helper functions **MUST NOT** be exported beyond the package unless they are needed by external callers.
+4. Helper functions that produce log output as a side effect **SHOULD** document that behaviour in a comment so callers can anticipate test output.
+
+### Nil-Guard Pattern
+
+1. Any code path in `ResolveActionPin` that may write to `ctx.Warnings` **MUST** call `initWarnings(ctx)` before performing the write.
+2. Duplicate inline `if ctx.Warnings == nil` blocks **MUST NOT** be reintroduced; all nil-guard logic **MUST** be delegated to `initWarnings`.
+
+### Testability
+
+1. Every extracted pure helper function **SHOULD** have at least one unit test exercising its primary behaviour.
+2. Tests for extracted helpers **MUST** use the internal test package (`package actionpins`) or be placed in `actionpins_internal_test.go` to access unexported symbols.
+
+### Conformance
+
+An implementation is considered conformant with this ADR if it satisfies all **MUST** and **MUST NOT** requirements above. Failure to meet any **MUST** or **MUST NOT** requirement constitutes non-conformance.
+
+---
+
+*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/24726693548) workflow. The PR author must review, complete, and finalize this document before the PR can merge.*

pkg/actionpins/actionpins.go

@@ -107,23 +107,8 @@ func getActionPins() []ActionPin {
 			panic(fmt.Sprintf("failed to load action pins: %v", err))
 		}
 
-		mismatchCount := 0
-		for key, pin := range data.Entries {
-			if idx := strings.LastIndex(key, "@"); idx != -1 {
-				keyVersion := key[idx+1:]
-				if keyVersion != pin.Version {
-					mismatchCount++
-					shortSHA := pin.SHA
-					if len(pin.SHA) > 8 {
-						shortSHA = pin.SHA[:8]
-					}
-					log.Printf("WARNING: Key/version mismatch in action_pins.json: key=%s has version=%s but pin.Version=%s (sha=%s)",
-						key, keyVersion, pin.Version, shortSHA)
-				}
-			}
-		}
-		if mismatchCount > 0 {
-			log.Printf("Found %d key/version mismatches in action_pins.json", mismatchCount)
+		if n := countPinKeyMismatches(data.Entries); n > 0 {
+			log.Printf("Found %d key/version mismatches in action_pins.json", n)
 		}
 
 		pins := make([]ActionPin, 0, len(data.Entries))
@@ -141,25 +126,51 @@ func getActionPins() []ActionPin {
 		log.Printf("Successfully unmarshaled and sorted %d action pins from JSON", len(pins))
 		cachedActionPins = pins
 
-		byRepo := make(map[string][]ActionPin, len(pins))
-		for _, pin := range pins {
-			byRepo[pin.Repo] = append(byRepo[pin.Repo], pin)
-		}
-		for repo, repoPins := range byRepo {
-			sort.Slice(repoPins, func(i, j int) bool {
-				v1 := strings.TrimPrefix(repoPins[i].Version, "v")
-				v2 := strings.TrimPrefix(repoPins[j].Version, "v")
-				return semverutil.Compare(v1, v2) > 0
-			})
-			byRepo[repo] = repoPins
-		}
-		cachedActionPinsByRepo = byRepo
-		log.Printf("Built per-repo action pin index for %d repos", len(byRepo))
+		cachedActionPinsByRepo = buildByRepoIndex(pins)
+		log.Printf("Built per-repo action pin index for %d repos", len(cachedActionPinsByRepo))
 	})
 
 	return cachedActionPins
 }
 
+// countPinKeyMismatches returns the number of entries where the key version does not
+// match pin.Version, logging each mismatch for diagnostics.
+func countPinKeyMismatches(entries map[string]ActionPin) int {
+	count := 0
+	for key, pin := range entries {
+		if idx := strings.LastIndex(key, "@"); idx != -1 {
+			keyVersion := key[idx+1:]
+			if keyVersion != pin.Version {
+				count++
+				shortSHA := pin.SHA
+				if len(pin.SHA) > 8 {
+					shortSHA = pin.SHA[:8]
+				}
+				log.Printf("WARNING: Key/version mismatch in action_pins.json: key=%s has version=%s but pin.Version=%s (sha=%s)",
+					key, keyVersion, pin.Version, shortSHA)
+			}
+		}
+	}
+	return count
+}
+
+// buildByRepoIndex groups pins by repository and sorts each group by version descending.
+func buildByRepoIndex(pins []ActionPin) map[string][]ActionPin {
+	byRepo := make(map[string][]ActionPin, len(pins))
+	for _, pin := range pins {
+		byRepo[pin.Repo] = append(byRepo[pin.Repo], pin)
+	}
+	for repo, repoPins := range byRepo {
+		sort.Slice(repoPins, func(i, j int) bool {
+			v1 := strings.TrimPrefix(repoPins[i].Version, "v")
+			v2 := strings.TrimPrefix(repoPins[j].Version, "v")
+			return semverutil.Compare(v1, v2) > 0
+		})
+		byRepo[repo] = repoPins
+	}
+	return byRepo
+}
+
 // GetActionPinsByRepo returns the sorted (version-descending) list of action pins
 // for the given repository. Returns nil if the repo has no pins.
 func GetActionPinsByRepo(repo string) []ActionPin {
@@ -234,6 +245,13 @@ func findCompatiblePin(pins []ActionPin, version string) (ActionPin, bool) {
 	return ActionPin{}, false
 }
 
+// initWarnings ensures ctx.Warnings is initialized, avoiding nil map writes.
+func initWarnings(ctx *PinContext) {
+	if ctx.Warnings == nil {
+		ctx.Warnings = make(map[string]bool)
+	}
+}
+
 func notifyResolutionFailure(ctx *PinContext, actionRepo, version string, errorType ResolutionErrorType) {
 	if ctx == nil || ctx.RecordResolutionFailure == nil {
 		return
@@ -310,9 +328,7 @@ func ResolveActionPin(actionRepo, version string, ctx *PinContext) (string, erro
 			}
 
 			if !isAlreadySHA {
-				if ctx.Warnings == nil {
-					ctx.Warnings = make(map[string]bool)
-				}
+				initWarnings(ctx)
 				cacheKey := FormatCacheKey(actionRepo, version)
 				if !ctx.Warnings[cacheKey] {
 					warningMsg := fmt.Sprintf("Unable to resolve %s@%s dynamically, using hardcoded pin for %s@%s",
@@ -332,9 +348,7 @@ func ResolveActionPin(actionRepo, version string, ctx *PinContext) (string, erro
 		return FormatReference(actionRepo, version, version), nil
 	}
 
-	if ctx.Warnings == nil {
-		ctx.Warnings = make(map[string]bool)
-	}
+	initWarnings(ctx)
 	cacheKey := FormatCacheKey(actionRepo, version)
 	errorType := ResolutionErrorTypePinNotFound
 	if ctx.Resolver != nil {

pkg/actionpins/actionpins_internal_test.go

@@ -0,0 +1,62 @@
+//go:build !integration
+
+package actionpins
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestBuildByRepoIndex_GroupsByRepoAndSortsDescending(t *testing.T) {
+	pins := []ActionPin{
+		{Repo: "actions/checkout", Version: "v4.0.0", SHA: "sha-v4"},
+		{Repo: "actions/checkout", Version: "v5.0.0", SHA: "sha-v5"},
+		{Repo: "actions/setup-go", Version: "v5.1.0", SHA: "sha-go-v5-1"},
+		{Repo: "actions/setup-go", Version: "v5.0.0", SHA: "sha-go-v5-0"},
+	}
+
+	byRepo := buildByRepoIndex(pins)
+
+	require.Len(t, byRepo["actions/checkout"], 2, "Expected checkout pins to be grouped")
+	assert.Equal(t, "v5.0.0", byRepo["actions/checkout"][0].Version, "Expected checkout pins sorted by newest version first")
+	assert.Equal(t, "v4.0.0", byRepo["actions/checkout"][1].Version, "Expected checkout pins sorted by newest version first")
+
+	require.Len(t, byRepo["actions/setup-go"], 2, "Expected setup-go pins to be grouped")
+	assert.Equal(t, "v5.1.0", byRepo["actions/setup-go"][0].Version, "Expected setup-go pins sorted by newest version first")
+	assert.Equal(t, "v5.0.0", byRepo["actions/setup-go"][1].Version, "Expected setup-go pins sorted by newest version first")
+}
+
+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"},
+	}
+
+	count := countPinKeyMismatches(entries)
+
+	assert.Equal(t, 1, count, "Expected only one key/version mismatch to be counted")
+}
+
+func TestInitWarnings_InitializesAndPreservesMap(t *testing.T) {
+	t.Run("initializes nil warnings map", func(t *testing.T) {
+		ctx := &PinContext{}
+
+		initWarnings(ctx)
+
+		require.NotNil(t, ctx.Warnings, "Expected warnings map to be initialized")
+		assert.Empty(t, ctx.Warnings, "Expected initialized warnings map to be empty")
+	})
+
+	t.Run("preserves existing warnings map", func(t *testing.T) {
+		existing := map[string]bool{"actions/checkout@v5": true}
+		ctx := &PinContext{Warnings: existing}
+
+		initWarnings(ctx)
+
+		require.NotNil(t, ctx.Warnings, "Expected warnings map to remain initialized")
+		assert.Equal(t, existing, ctx.Warnings, "Expected existing warnings entries to be preserved")
+	})
+}