From c0c519289231f1ba73b8df6888c9cd9868f5bd40 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Tue, 21 Apr 2026 10:04:50 +0000 Subject: [PATCH 1/3] refactor(actionpins): extract pure helpers and eliminate duplicate warnings init - Extract buildByRepoIndex() as a pure function from sync.Once closure, grouping and sorting pins by repo. Improves testability and separates concerns from the initialization path. - Extract countPinKeyMismatches() to validate pin entry consistency outside the initialization closure, making the logic independently inspectable. - Extract initWarnings() to eliminate two identical lazy-init blocks for ctx.Warnings in ResolveActionPin, reducing mutation duplication. All existing tests pass. No behavior changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pkg/actionpins/actionpins.go | 88 +++++++++++++++++++++--------------- 1 file changed, 51 insertions(+), 37 deletions(-) diff --git a/pkg/actionpins/actionpins.go b/pkg/actionpins/actionpins.go index 2a560b27421..40bfa4631b7 100644 --- a/pkg/actionpins/actionpins.go +++ b/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 { From 4766e7d9ffe1bd086e93b30ca7a5c33479943848 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 21 Apr 2026 13:56:42 +0000 Subject: [PATCH 2/3] test(actionpins): add helper coverage for extracted pure functions Agent-Logs-Url: https://github.com/github/gh-aw/sessions/951ca789-b015-485d-b1e2-89ae9685c175 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/actionpins/actionpins_internal_test.go | 62 ++++++++++++++++++++++ 1 file changed, 62 insertions(+) create mode 100644 pkg/actionpins/actionpins_internal_test.go diff --git a/pkg/actionpins/actionpins_internal_test.go b/pkg/actionpins/actionpins_internal_test.go new file mode 100644 index 00000000000..20f8065e7bf --- /dev/null +++ b/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") + }) +} From 5b37e8b7dab4b3469a4fbe9e8e326cbab9cebc9d Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Tue, 21 Apr 2026 14:06:31 +0000 Subject: [PATCH 3/3] docs(adr): add draft ADR-27523 for extracting pure helpers in pkg/actionpins Generated by Design Decision Gate workflow run 24726693548. Co-Authored-By: Claude Sonnet 4.6 --- ...7523-extract-pure-helpers-in-actionpins.md | 73 +++++++++++++++++++ 1 file changed, 73 insertions(+) create mode 100644 docs/adr/27523-extract-pure-helpers-in-actionpins.md diff --git a/docs/adr/27523-extract-pure-helpers-in-actionpins.md b/docs/adr/27523-extract-pure-helpers-in-actionpins.md new file mode 100644 index 00000000000..fede10f3039 --- /dev/null +++ b/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.*