From 994d60398ba0f30c387f737400be935da34791d9 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 19 Aug 2026 03:29:06 +0000 Subject: [PATCH 1/4] Initial plan From 570deaa501de840b394a59892f4e76ddb05edb4d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 19 Aug 2026 03:38:20 +0000 Subject: [PATCH 2/4] Fix package-level mutable slice/map reassignment (LintMonster) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/actionpins/actionpins_internal_test.go | 4 +- pkg/actionpins/data.go | 49 ++++++++++++++-------- pkg/parser/virtual_fs.go | 21 ++++++---- pkg/parser/virtual_fs_wasm.go | 2 +- 4 files changed, 46 insertions(+), 30 deletions(-) diff --git a/pkg/actionpins/actionpins_internal_test.go b/pkg/actionpins/actionpins_internal_test.go index 86e51b9628c..e58db771ae1 100644 --- a/pkg/actionpins/actionpins_internal_test.go +++ b/pkg/actionpins/actionpins_internal_test.go @@ -420,10 +420,8 @@ func TestGetContainerPin_ReturnsPinnedImage(t *testing.T) { } func TestGetContainerPin_MCPGatewayVersionsArePinned(t *testing.T) { - getActionPins() - var mcpgImages []string - for image := range cachedContainerPins { + for image := range getCachedActionPins().containers { if strings.HasPrefix(image, "ghcr.io/github/gh-aw-mcpg:") { mcpgImages = append(mcpgImages, image) } diff --git a/pkg/actionpins/data.go b/pkg/actionpins/data.go index 09a47d24ae1..2a150817ccb 100644 --- a/pkg/actionpins/data.go +++ b/pkg/actionpins/data.go @@ -19,14 +19,22 @@ var actionPinsLog = logger.New("actionpins:actionpins") //go:embed data/action_pins.json var actionPinsJSON []byte +// actionPinsCache bundles the parsed/derived action pin data behind a single +// pointer so the package-level variable holding it is never a bare slice or +// map that gets reassigned in place; the pointer itself is written exactly +// once (guarded by actionPinsOnce) and is treated as read-only thereafter. +type actionPinsCache struct { + pins []ActionPin + byRepo map[string][]ActionPin + containers map[string]ContainerPin +} + var ( - cachedActionPins []ActionPin - cachedActionPinsByRepo map[string][]ActionPin - cachedContainerPins map[string]ContainerPin - actionPinsOnce sync.Once + cachedPins *actionPinsCache + actionPinsOnce sync.Once ) -func getActionPins() []ActionPin { +func getCachedActionPins() *actionPinsCache { actionPinsOnce.Do(func() { actionPinsLog.Print("Unmarshaling action pins from embedded JSON (first call, will be cached)") @@ -42,19 +50,28 @@ func getActionPins() []ActionPin { }) actionPinsLog.Printf("Successfully unmarshaled and sorted %d action pins from JSON", len(pins)) - cachedActionPins = pins - cachedActionPinsByRepo = buildByRepoIndex(pins) - actionPinsLog.Printf("Built per-repo action pin index for %d repos", len(cachedActionPinsByRepo)) + byRepo := buildByRepoIndex(pins) + actionPinsLog.Printf("Built per-repo action pin index for %d repos", len(byRepo)) + + containers := data.Containers + if containers == nil { + containers = make(map[string]ContainerPin) + } + actionPinsLog.Printf("Loaded %d container pins from JSON", len(containers)) - cachedContainerPins = data.Containers - if cachedContainerPins == nil { - cachedContainerPins = make(map[string]ContainerPin) + cachedPins = &actionPinsCache{ + pins: pins, + byRepo: byRepo, + containers: containers, } - actionPinsLog.Printf("Loaded %d container pins from JSON", len(cachedContainerPins)) }) - return cachedActionPins + return cachedPins +} + +func getActionPins() []ActionPin { + return getCachedActionPins().pins } // loadActionPinsData unmarshals embedded action pin data. @@ -129,8 +146,7 @@ func buildByRepoIndex(pins []ActionPin) map[string][]ActionPin { // 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 { - getActionPins() - return cachedActionPinsByRepo[repo] + return getCachedActionPins().byRepo[repo] } // GetLatestActionPinByRepo returns the latest ActionPin for a given repository, if any. @@ -144,7 +160,6 @@ func GetLatestActionPinByRepo(repo string) (ActionPin, bool) { // GetContainerPin returns a pinned container image by its original image reference. func GetContainerPin(image string) (ContainerPin, bool) { - getActionPins() - pin, ok := cachedContainerPins[image] + pin, ok := getCachedActionPins().containers[image] return pin, ok } diff --git a/pkg/parser/virtual_fs.go b/pkg/parser/virtual_fs.go index 06e51805b05..3e46e487812 100644 --- a/pkg/parser/virtual_fs.go +++ b/pkg/parser/virtual_fs.go @@ -15,10 +15,12 @@ var virtualFsLog = logger.New("parser:virtual_fs") // builtinVirtualFiles holds embedded built-in files registered at startup. // Keys use the "@builtin:" path prefix (e.g. "@builtin:engines/copilot.md"). -// The map is replaced using copy-on-write during registration and then treated -// as read-only; concurrent reads are safe. +// It is stored as a pointer to an immutable map: registration builds a brand +// new map (copy-on-write) and swaps the pointer, so the package-level +// variable itself is never mutated in place, and readers only ever see a +// fully-populated, read-only snapshot. Concurrent reads are safe. var ( - builtinVirtualFiles map[string][]byte + builtinVirtualFiles = &map[string][]byte{} builtinVirtualFilesMu sync.RWMutex ) @@ -33,24 +35,25 @@ func RegisterBuiltinVirtualFile(path string, content []byte) { } builtinVirtualFilesMu.Lock() defer builtinVirtualFilesMu.Unlock() - if existing, ok := builtinVirtualFiles[path]; ok { + current := *builtinVirtualFiles + if existing, ok := current[path]; ok { if !bytes.Equal(existing, content) { panic(fmt.Sprintf("RegisterBuiltinVirtualFile: path %q already registered with different content", path)) } return // idempotent: same content, no-op } virtualFsLog.Printf("Registering builtin virtual file: %s (%d bytes)", path, len(content)) - next := make(map[string][]byte, len(builtinVirtualFiles)+1) - maps.Copy(next, builtinVirtualFiles) + next := make(map[string][]byte, len(current)+1) + maps.Copy(next, current) next[path] = bytes.Clone(content) - builtinVirtualFiles = next + builtinVirtualFiles = &next } // BuiltinVirtualFileExists returns true if the given path is registered as a builtin virtual file. func BuiltinVirtualFileExists(path string) bool { builtinVirtualFilesMu.RLock() defer builtinVirtualFilesMu.RUnlock() - _, ok := builtinVirtualFiles[path] + _, ok := (*builtinVirtualFiles)[path] virtualFsLog.Printf("BuiltinVirtualFileExists: path=%s exists=%t", path, ok) return ok } @@ -116,7 +119,7 @@ const BuiltinPathPrefix = "@builtin:" var readFileFunc = func(path string) ([]byte, error) { builtinVirtualFilesMu.RLock() defer builtinVirtualFilesMu.RUnlock() - content, ok := builtinVirtualFiles[path] + content, ok := (*builtinVirtualFiles)[path] if ok { return bytes.Clone(content), nil } diff --git a/pkg/parser/virtual_fs_wasm.go b/pkg/parser/virtual_fs_wasm.go index ee964e7b977..98e31393784 100644 --- a/pkg/parser/virtual_fs_wasm.go +++ b/pkg/parser/virtual_fs_wasm.go @@ -38,7 +38,7 @@ func init() { // Check builtin virtual files first (embedded engine .md files etc.) builtinVirtualFilesMu.RLock() defer builtinVirtualFilesMu.RUnlock() - builtinContent, builtinOK := builtinVirtualFiles[path] + builtinContent, builtinOK := (*builtinVirtualFiles)[path] if builtinOK { parserLog.Printf("readFileFunc: resolved builtin virtual file: %s", path) return builtinContent, nil From 90a001032ce34599010cdd9ad98f8eed96313f76 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 19 Aug 2026 10:42:03 +0000 Subject: [PATCH 3/4] Harden cache accessor invariants Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- pkg/actionpins/actionpins_internal_test.go | 10 ++++++++++ pkg/actionpins/data.go | 3 +++ pkg/parser/virtual_fs.go | 21 ++++++++++++--------- pkg/parser/virtual_fs_wasm.go | 2 +- 4 files changed, 26 insertions(+), 10 deletions(-) diff --git a/pkg/actionpins/actionpins_internal_test.go b/pkg/actionpins/actionpins_internal_test.go index e58db771ae1..bdd400953f7 100644 --- a/pkg/actionpins/actionpins_internal_test.go +++ b/pkg/actionpins/actionpins_internal_test.go @@ -465,6 +465,16 @@ func TestGetActionPins_CacheCorrectnessOnRepeatedCalls(t *testing.T) { assert.Equal(t, first, second, "Expected repeated calls to getActionPins() to return equal data (cache correctness)") } +func TestGetCachedActionPins_InitializesCache(t *testing.T) { + cache := getCachedActionPins() + + require.NotNil(t, cache, "Expected cache accessor to return initialized data") + assert.NotEmpty(t, cache.pins, "Expected cached action pins") + assert.NotNil(t, cache.byRepo, "Expected cached action pins by repository") + assert.NotNil(t, cache.containers, "Expected cached container pins") + assert.Same(t, cache, getCachedActionPins(), "Expected repeated cache access to return the same cache") +} + func TestResolveActionPinDynamically_SkipsForSHAInput(t *testing.T) { t.Parallel() resolver := &countingResolver{} diff --git a/pkg/actionpins/data.go b/pkg/actionpins/data.go index 2a150817ccb..55d5283fc02 100644 --- a/pkg/actionpins/data.go +++ b/pkg/actionpins/data.go @@ -67,6 +67,9 @@ func getCachedActionPins() *actionPinsCache { } }) + if cachedPins == nil { + panic("action pins cache was not initialized") + } return cachedPins } diff --git a/pkg/parser/virtual_fs.go b/pkg/parser/virtual_fs.go index 3e46e487812..8dc3d69d9fd 100644 --- a/pkg/parser/virtual_fs.go +++ b/pkg/parser/virtual_fs.go @@ -15,12 +15,15 @@ var virtualFsLog = logger.New("parser:virtual_fs") // builtinVirtualFiles holds embedded built-in files registered at startup. // Keys use the "@builtin:" path prefix (e.g. "@builtin:engines/copilot.md"). -// It is stored as a pointer to an immutable map: registration builds a brand -// new map (copy-on-write) and swaps the pointer, so the package-level -// variable itself is never mutated in place, and readers only ever see a -// fully-populated, read-only snapshot. Concurrent reads are safe. +// Registration swaps a pointer to an immutable snapshot rather than assigning +// a map directly. The named snapshot type makes this copy-on-write pattern +// explicit: readers only see fully-populated, read-only snapshots. +type builtinVirtualFileSnapshot struct { + files map[string][]byte +} + var ( - builtinVirtualFiles = &map[string][]byte{} + builtinVirtualFiles = &builtinVirtualFileSnapshot{files: map[string][]byte{}} builtinVirtualFilesMu sync.RWMutex ) @@ -35,7 +38,7 @@ func RegisterBuiltinVirtualFile(path string, content []byte) { } builtinVirtualFilesMu.Lock() defer builtinVirtualFilesMu.Unlock() - current := *builtinVirtualFiles + current := builtinVirtualFiles.files if existing, ok := current[path]; ok { if !bytes.Equal(existing, content) { panic(fmt.Sprintf("RegisterBuiltinVirtualFile: path %q already registered with different content", path)) @@ -46,14 +49,14 @@ func RegisterBuiltinVirtualFile(path string, content []byte) { next := make(map[string][]byte, len(current)+1) maps.Copy(next, current) next[path] = bytes.Clone(content) - builtinVirtualFiles = &next + builtinVirtualFiles = &builtinVirtualFileSnapshot{files: next} } // BuiltinVirtualFileExists returns true if the given path is registered as a builtin virtual file. func BuiltinVirtualFileExists(path string) bool { builtinVirtualFilesMu.RLock() defer builtinVirtualFilesMu.RUnlock() - _, ok := (*builtinVirtualFiles)[path] + _, ok := builtinVirtualFiles.files[path] virtualFsLog.Printf("BuiltinVirtualFileExists: path=%s exists=%t", path, ok) return ok } @@ -119,7 +122,7 @@ const BuiltinPathPrefix = "@builtin:" var readFileFunc = func(path string) ([]byte, error) { builtinVirtualFilesMu.RLock() defer builtinVirtualFilesMu.RUnlock() - content, ok := (*builtinVirtualFiles)[path] + content, ok := builtinVirtualFiles.files[path] if ok { return bytes.Clone(content), nil } diff --git a/pkg/parser/virtual_fs_wasm.go b/pkg/parser/virtual_fs_wasm.go index 98e31393784..ec48a04b376 100644 --- a/pkg/parser/virtual_fs_wasm.go +++ b/pkg/parser/virtual_fs_wasm.go @@ -38,7 +38,7 @@ func init() { // Check builtin virtual files first (embedded engine .md files etc.) builtinVirtualFilesMu.RLock() defer builtinVirtualFilesMu.RUnlock() - builtinContent, builtinOK := (*builtinVirtualFiles)[path] + builtinContent, builtinOK := builtinVirtualFiles.files[path] if builtinOK { parserLog.Printf("readFileFunc: resolved builtin virtual file: %s", path) return builtinContent, nil From 0323cdc77106d77b98bac5ddf4ae4f2358b5606b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 20 Aug 2026 05:59:53 +0000 Subject: [PATCH 4/4] Document action pins cache panic invariant Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- pkg/actionpins/data.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/actionpins/data.go b/pkg/actionpins/data.go index 55d5283fc02..e5751dc356e 100644 --- a/pkg/actionpins/data.go +++ b/pkg/actionpins/data.go @@ -34,6 +34,9 @@ var ( actionPinsOnce sync.Once ) +// getCachedActionPins returns the initialized action pin cache. +// Panics if cache initialization did not complete, which can only follow +// invalid embedded action pin data. func getCachedActionPins() *actionPinsCache { actionPinsOnce.Do(func() { actionPinsLog.Print("Unmarshaling action pins from embedded JSON (first call, will be cached)")