Optimize action pinning and caching: eliminate redundant JSON parsing and I/O

pkg/workflow/action_cache.go

@@ -29,6 +29,7 @@ type ActionCacheEntry struct {
 type ActionCache struct {
 	Entries map[string]ActionCacheEntry `json:"entries"` // key: "repo@version"
 	path    string
+	dirty   bool // tracks if cache has unsaved changes
 }
 
 // NewActionCache creates a new action cache instance
@@ -38,6 +39,7 @@ func NewActionCache(repoRoot string) *ActionCache {
 	return &ActionCache{
 		Entries: make(map[string]ActionCacheEntry),
 		path:    cachePath,
+		// dirty is initialized to false (zero value)
 	}
 }
 
@@ -60,14 +62,24 @@ func (c *ActionCache) Load() error {
 		return err
 	}
 
+	// Mark cache as clean after successful load (it matches disk state)
+	c.dirty = false
+
 	actionCacheLog.Printf("Successfully loaded cache with %d entries", len(c.Entries))
 	return nil
 }
 
 // Save saves the cache to disk with sorted entries
 // If the cache is empty, the file is not created or is deleted if it exists
 // Deduplicates entries by keeping only the most precise version reference for each repo+SHA combination
+// Only saves if the cache has been modified (dirty flag is true)
 func (c *ActionCache) Save() error {
+	// Skip saving if cache hasn't been modified
+	if !c.dirty {
+		actionCacheLog.Printf("Cache is clean (no changes), skipping save")
+		return nil
+	}
+
 	actionCacheLog.Printf("Saving action cache to: %s with %d entries", c.path, len(c.Entries))
 
 	// If cache is empty, skip saving and delete the file if it exists
@@ -81,6 +93,7 @@ func (c *ActionCache) Save() error {
 				return err
 			}
 		}
+		c.dirty = false
 		return nil
 	}
 
@@ -110,6 +123,7 @@ func (c *ActionCache) Save() error {
 	}
 
 	actionCacheLog.Print("Successfully saved action cache")
+	c.dirty = false
 	return nil
 }
 
@@ -187,6 +201,7 @@ func (c *ActionCache) Set(repo, version, sha string) {
 		Version: version,
 		SHA:     sha,
 	}
+	c.dirty = true // Mark cache as modified
 }
 
 // GetCachePath returns the path to the cache file

pkg/workflow/action_cache_test.go

@@ -240,6 +240,7 @@ func TestActionCacheEmptySaveDeletesExistingFile(t *testing.T) {
 
 	// Clear cache and save again
 	cache.Entries = make(map[string]ActionCacheEntry)
+	cache.dirty = true // Mark as dirty so save actually processes the empty cache
 	err = cache.Save()
 	if err != nil {
 		t.Fatalf("Failed to save empty cache: %v", err)
@@ -268,6 +269,7 @@ func TestActionCacheDeduplication(t *testing.T) {
 		Version: "v5.0.1",
 		SHA:     "abc123",
 	}
+	cache.dirty = true // Mark as dirty so save processes the cache
 
 	// Verify we have 2 entries before deduplication
 	if len(cache.Entries) != 2 {
@@ -328,6 +330,9 @@ func TestActionCacheDeduplicationMultipleActions(t *testing.T) {
 	// actions/setup-node: no duplicates
 	cache.Set("actions/setup-node", "v6.1.0", "sha3")
 
+	// Since we set Entries directly, we need to mark as dirty for the test
+	cache.dirty = true
+
 	// Verify we have 5 entries before deduplication
 	if len(cache.Entries) != 5 {
 		t.Fatalf("Expected 5 entries before deduplication, got %d", len(cache.Entries))
@@ -451,3 +456,51 @@ func TestIsMorePreciseVersion(t *testing.T) {
 		})
 	}
 }
+
+// TestActionCacheDirtyFlag verifies that the cache dirty flag prevents unnecessary saves
+func TestActionCacheDirtyFlag(t *testing.T) {
+	tmpDir := testutil.TempDir(t, "test-*")
+	cache := NewActionCache(tmpDir)
+
+	// Initially, cache should be clean (no data)
+	err := cache.Save()
+	if err != nil {
+		t.Fatalf("Failed to save empty cache: %v", err)
+	}
+
+	// Cache file should not exist (empty cache)
+	cachePath := cache.GetCachePath()
+	if _, err := os.Stat(cachePath); !os.IsNotExist(err) {
+		t.Error("Empty cache should not create a file")
+	}
+
+	// Add an entry - should mark as dirty
+	cache.Set("actions/checkout", "v5", "abc123")
+
+	// Save should work now
+	err = cache.Save()
+	if err != nil {
+		t.Fatalf("Failed to save dirty cache: %v", err)
+	}
+
+	// Cache file should now exist
+	if _, err := os.Stat(cachePath); err != nil {
+		t.Errorf("Cache file should exist after save: %v", err)
+	}
+
+	// Save again without changes - should skip (cache is clean)
+	// We can't directly verify the skip, but we can ensure it doesn't error
+	err = cache.Save()
+	if err != nil {
+		t.Fatalf("Failed to save clean cache: %v", err)
+	}
+
+	// Add another entry - should mark as dirty again
+	cache.Set("actions/setup-node", "v4", "def456")
+
+	// Save should work
+	err = cache.Save()
+	if err != nil {
+		t.Fatalf("Failed to save dirty cache after modification: %v", err)
+	}
+}

pkg/workflow/action_pins.go

@@ -7,6 +7,7 @@ import (
 	"os"
 	"sort"
 	"strings"
+	"sync"
 
 	"github.com/githubnext/gh-aw/pkg/console"
 	"github.com/githubnext/gh-aw/pkg/logger"
@@ -30,58 +31,70 @@ type ActionPinsData struct {
 	Entries map[string]ActionPin `json:"entries"` // key: "repo@version"
 }
 
-// getActionPins unmarshals and returns the action pins from the embedded JSON
+var (
+	// cachedActionPins holds the parsed and sorted action pins
+	cachedActionPins []ActionPin
+	// actionPinsOnce ensures the action pins are loaded only once
+	actionPinsOnce sync.Once
+)
+
+// getActionPins returns the action pins from the embedded JSON
 // Returns a sorted slice of action pins (by version descending, then by repo name)
-// This is called on-demand rather than cached globally
+// The data is parsed once on first call and cached for subsequent calls
 func getActionPins() []ActionPin {
-	actionPinsLog.Print("Unmarshaling action pins from embedded JSON")
+	actionPinsOnce.Do(func() {
+		actionPinsLog.Print("Unmarshaling action pins from embedded JSON (first call, will be cached)")
 
-	var data ActionPinsData
-	if err := json.Unmarshal(actionPinsJSON, &data); err != nil {
-		actionPinsLog.Printf("Failed to unmarshal action pins JSON: %v", err)
-		panic(fmt.Sprintf("failed to load action pins: %v", err))
-	}
+		var data ActionPinsData
+		if err := json.Unmarshal(actionPinsJSON, &data); err != nil {
+			actionPinsLog.Printf("Failed to unmarshal action pins JSON: %v", err)
+			panic(fmt.Sprintf("failed to load action pins: %v", err))
+		}
 
-	// Detect and log key/version mismatches
-	mismatchCount := 0
-	for key, pin := range data.Entries {
-		// Extract version from key (format: "repo@version")
-		if idx := strings.LastIndex(key, "@"); idx != -1 {
-			keyVersion := key[idx+1:]
-			if keyVersion != pin.Version {
-				mismatchCount++
-				actionPinsLog.Printf("WARNING: Key/version mismatch in action_pins.json: key=%s has version=%s but pin.Version=%s (sha=%s)",
-					key, keyVersion, pin.Version, pin.SHA[:8])
+		// Detect and log key/version mismatches
+		mismatchCount := 0
+		for key, pin := range data.Entries {
+			// Extract version from key (format: "repo@version")
+			if idx := strings.LastIndex(key, "@"); idx != -1 {
+				keyVersion := key[idx+1:]
+				if keyVersion != pin.Version {
+					mismatchCount++
+					// Safely truncate SHA for logging
+					shortSHA := pin.SHA
+					if len(pin.SHA) > 8 {
+						shortSHA = pin.SHA[:8]
+					}
+					actionPinsLog.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 {
-		actionPinsLog.Printf("Found %d key/version mismatches in action_pins.json", mismatchCount)
-	}
+		if mismatchCount > 0 {
+			actionPinsLog.Printf("Found %d key/version mismatches in action_pins.json", mismatchCount)
+		}
 
-	// Convert map to sorted slice
-	pins := make([]ActionPin, 0, len(data.Entries))
-	for _, pin := range data.Entries {
-		pins = append(pins, pin)
-	}
+		// Convert map to sorted slice
+		pins := make([]ActionPin, 0, len(data.Entries))
+		for _, pin := range data.Entries {
+			pins = append(pins, pin)
+		}
 
-	// Sort by version (descending) then by repo name (ascending)
-	for i := 0; i < len(pins); i++ {
-		for j := i + 1; j < len(pins); j++ {
-			// Compare versions first (descending)
-			if pins[i].Version < pins[j].Version {
-				pins[i], pins[j] = pins[j], pins[i]
-			} else if pins[i].Version == pins[j].Version {
-				// Same version, sort by repo name (ascending)
-				if pins[i].Repo > pins[j].Repo {
-					pins[i], pins[j] = pins[j], pins[i]
-				}
+		// Sort by version (descending) then by repo name (ascending)
+		// Use standard library sort for better performance O(n log n) vs O(n²)
+		sort.Slice(pins, func(i, j int) bool {
+			// Compare versions first (descending order - higher version first)
+			if pins[i].Version != pins[j].Version {
+				return pins[i].Version > pins[j].Version
 			}
-		}
-	}
+			// Same version, sort by repo name (ascending order)
+			return pins[i].Repo < pins[j].Repo
+		})
 
-	actionPinsLog.Printf("Successfully unmarshaled and sorted %d action pins from JSON", len(pins))
-	return pins
+		actionPinsLog.Printf("Successfully unmarshaled and sorted %d action pins from JSON", len(pins))
+		cachedActionPins = pins
+	})
+
+	return cachedActionPins
 }
 
 // sortPinsByVersion sorts action pins by version in descending order (highest first)
@@ -152,10 +165,8 @@ func GetActionPinWithData(actionRepo, version string, data *WorkflowData) (strin
 				}
 			}
 
-			// Successfully resolved, save cache
-			if data.ActionCache != nil {
-				_ = data.ActionCache.Save()
-			}
+			// Successfully resolved - cache will be saved at end of compilation
+			actionPinsLog.Printf("Successfully resolved action pin (cache marked dirty, will save at end)")
 			result := actionRepo + "@" + sha + " # " + version
 			actionPinsLog.Printf("Returning pinned reference: %s", result)
 			return result, nil

pkg/workflow/action_pins_test.go

@@ -1042,3 +1042,39 @@ func TestApplyActionPinsToTypedSteps(t *testing.T) {
 		})
 	}
 }
+
+// TestActionPinsCaching verifies that action pins are cached and not re-parsed
+func TestActionPinsCaching(t *testing.T) {
+	// Reset the cache by creating a new sync.Once
+	// Note: In production, this is handled automatically by sync.Once
+
+	// First call - should load and cache
+	pins1 := getActionPins()
+	if len(pins1) == 0 {
+		t.Fatal("No action pins loaded on first call")
+	}
+
+	// Second call - should return cached data (same slice reference)
+	pins2 := getActionPins()
+	if len(pins2) == 0 {
+		t.Fatal("No action pins loaded on second call")
+	}
+
+	// Verify both calls return the same data
+	if len(pins1) != len(pins2) {
+		t.Errorf("Cache returned different number of pins: first=%d, second=%d", len(pins1), len(pins2))
+	}
+
+	// Verify the data is identical by checking a few pins
+	for i := 0; i < len(pins1) && i < 3; i++ {
+		if pins1[i].Repo != pins2[i].Repo {
+			t.Errorf("Pin %d repo mismatch: first=%s, second=%s", i, pins1[i].Repo, pins2[i].Repo)
+		}
+		if pins1[i].Version != pins2[i].Version {
+			t.Errorf("Pin %d version mismatch: first=%s, second=%s", i, pins1[i].Version, pins2[i].Version)
+		}
+		if pins1[i].SHA != pins2[i].SHA {
+			t.Errorf("Pin %d SHA mismatch: first=%s, second=%s", i, pins1[i].SHA, pins2[i].SHA)
+		}
+	}
+}