From f296b3d678ee785c7bb3cf8a468393fbd36cf5fa Mon Sep 17 00:00:00 2001 From: MrSnakeDoc Date: Tue, 20 Jan 2026 18:02:24 +0000 Subject: [PATCH 1/9] fix(middleware): propagate modified cmd/args through chain --- internal/middleware/middleware.go | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/internal/middleware/middleware.go b/internal/middleware/middleware.go index d23f507..696d7ec 100644 --- a/internal/middleware/middleware.go +++ b/internal/middleware/middleware.go @@ -43,21 +43,20 @@ func UseMiddlewareChain(middlewares ...MiddlewareFunc) MiddlewareChain { } // Execute middleware chain - // We still need closures here (it's the nature of middleware pattern) - // but we minimize allocations by reusing the pre-stored slice - var chain func(int) error - chain = func(i int) error { + // Chain now properly propagates modified cmd/args through the chain + var chain func(*cobra.Command, []string, int) error + chain = func(currentCmd *cobra.Command, currentArgs []string, i int) error { if i >= mwLen { if orig != nil { - return orig(c, a) + return orig(currentCmd, currentArgs) } return nil } - return mwCopy[i](c, a, func(nc *cobra.Command, na []string) error { - return chain(i + 1) + return mwCopy[i](currentCmd, currentArgs, func(nc *cobra.Command, na []string) error { + return chain(nc, na, i+1) }) } - return chain(0) + return chain(c, a, 0) } return cmd } From 91aeb93b1f4d355005073a0344ed44a95ef62c1e Mon Sep 17 00:00:00 2001 From: MrSnakeDoc Date: Tue, 20 Jan 2026 18:05:30 +0000 Subject: [PATCH 2/9] perf(cache): use strings.Builder in splitLines to reduce allocations --- internal/brew/unified_cache.go | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/internal/brew/unified_cache.go b/internal/brew/unified_cache.go index 5846865..84e096d 100644 --- a/internal/brew/unified_cache.go +++ b/internal/brew/unified_cache.go @@ -6,6 +6,7 @@ import ( "fmt" "os" "path/filepath" + "strings" "sync" "time" @@ -472,22 +473,32 @@ func (c *UnifiedCache) saveToDiskLocked() error { // Helper functions func splitLines(s string) []string { + if s == "" { + return []string{} + } + result := make([]string, 0) - current := "" + var line strings.Builder + line.Grow(64) // Pre-allocate reasonable line size + for _, r := range s { if r == '\n' { - trimmed := trimSpace(current) + trimmed := trimSpace(line.String()) if trimmed != "" { result = append(result, trimmed) } - current = "" + line.Reset() } else { - current += string(r) + line.WriteRune(r) } } - trimmed := trimSpace(current) - if trimmed != "" { - result = append(result, trimmed) + + // Handle last line if no trailing newline + if line.Len() > 0 { + trimmed := trimSpace(line.String()) + if trimmed != "" { + result = append(result, trimmed) + } } return result From 20f9429cd66073e8125c1b461884643b023ba0fb Mon Sep 17 00:00:00 2001 From: MrSnakeDoc Date: Tue, 20 Jan 2026 18:11:12 +0000 Subject: [PATCH 3/9] fix(cache): set Installed flag to true in MarkUpgraded --- internal/brew/unified_cache.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/brew/unified_cache.go b/internal/brew/unified_cache.go index 84e096d..c5aed22 100644 --- a/internal/brew/unified_cache.go +++ b/internal/brew/unified_cache.go @@ -343,6 +343,7 @@ func (c *UnifiedCache) MarkUpgraded(name, newVersion string) error { defer c.mu.Unlock() state := c.Packages[name] + state.Installed = true state.InstalledVersion = newVersion state.LatestVersion = newVersion state.Outdated = false From 1ee081b4c786db612c5dbac2016f369fb9ad5942 Mon Sep 17 00:00:00 2001 From: MrSnakeDoc Date: Tue, 20 Jan 2026 18:13:09 +0000 Subject: [PATCH 4/9] fix(logger): remove double-formatting in writeDirect --- internal/logger/logger.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/internal/logger/logger.go b/internal/logger/logger.go index d1565c6..5800f3f 100644 --- a/internal/logger/logger.go +++ b/internal/logger/logger.go @@ -125,7 +125,7 @@ func Info(msg string, args ...interface{}) { // Fast path: direct write for non-JSON mode if !useJSON.Load() { - writeDirect(formatted) + writeDirect(p.Info(formatted)) return } @@ -285,6 +285,7 @@ func formatMessage(prefix, msg string, args ...interface{}) string { // writeDirect writes directly to output with minimal locking. // Used for non-JSON mode to bypass zap overhead. +// Expects msg to be already fully formatted (emoji + color). func writeDirect(msg string) { mu.RLock() w := out @@ -295,5 +296,5 @@ func writeDirect(msg string) { } // Write directly without holding lock - _, _ = fmt.Fprintln(w, p.Info(msg)) + _, _ = fmt.Fprintln(w, msg) } From 5eec9f6ff677787af1c756493fa95ae351e8bba7 Mon Sep 17 00:00:00 2001 From: MrSnakeDoc Date: Tue, 20 Jan 2026 18:16:20 +0000 Subject: [PATCH 5/9] fix(core): fail fast when cache initialization fails instead of using uninitialized cache --- internal/core/core.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/internal/core/core.go b/internal/core/core.go index b674d01..148ff92 100644 --- a/internal/core/core.go +++ b/internal/core/core.go @@ -103,9 +103,7 @@ func NewBase(config *models.Config, r runner.CommandRunner) *Base { cache, err := brew.GetCache(r) if err != nil { - logger.Debug("failed to initialize unified cache: %v", err) - // Fallback to empty cache - cache = &brew.UnifiedCache{} + logger.Fatal("failed to initialize unified cache: %v", err) } return &Base{ From 1eeeecb695a588c8356cbc6a7d316c0b569a6277 Mon Sep 17 00:00:00 2001 From: MrSnakeDoc Date: Tue, 20 Jan 2026 18:29:29 +0000 Subject: [PATCH 6/9] fix(cache): eliminate race condition in IsInstalled by calling Refresh directly & document error handling strategy in Refresh --- go.mod | 4 +++- internal/brew/unified_cache.go | 10 +++------- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/go.mod b/go.mod index 0d843e9..13a8833 100644 --- a/go.mod +++ b/go.mod @@ -6,6 +6,7 @@ require ( github.com/fatih/color v1.18.0 github.com/olekukonko/tablewriter v1.1.3 github.com/spf13/cobra v1.10.2 + github.com/stretchr/testify v1.10.0 go.uber.org/zap v1.27.1 gopkg.in/yaml.v3 v3.0.1 ) @@ -14,6 +15,7 @@ require ( github.com/clipperhouse/displaywidth v0.7.0 // indirect github.com/clipperhouse/stringish v0.1.1 // indirect github.com/clipperhouse/uax29/v2 v2.3.1 // indirect + github.com/davecgh/go-spew v1.1.1 // indirect github.com/inconshreveable/mousetrap v1.1.0 // indirect github.com/mattn/go-colorable v0.1.14 // indirect github.com/mattn/go-isatty v0.0.20 // indirect @@ -22,8 +24,8 @@ require ( github.com/olekukonko/cat v0.0.0-20250911104152-50322a0618f6 // indirect github.com/olekukonko/errors v1.2.0 // indirect github.com/olekukonko/ll v0.1.4-0.20260115111900-9e59c2286df0 // indirect + github.com/pmezard/go-difflib v1.0.0 // indirect github.com/spf13/pflag v1.0.10 // indirect - github.com/stretchr/testify v1.10.0 // indirect go.uber.org/multierr v1.11.0 // indirect golang.org/x/sys v0.40.0 // indirect gopkg.in/check.v1 v1.0.0-20200227125254-8fa46927fb4f // indirect diff --git a/internal/brew/unified_cache.go b/internal/brew/unified_cache.go index c5aed22..6768264 100644 --- a/internal/brew/unified_cache.go +++ b/internal/brew/unified_cache.go @@ -103,13 +103,8 @@ func ResetCache() { // IsInstalled checks if a package is installed. func (c *UnifiedCache) IsInstalled(name string) bool { - c.mu.RLock() - needsRefresh := c.needsRefreshLocked() - c.mu.RUnlock() - - if needsRefresh { - _ = c.Refresh(context.Background(), false) - } + // Refresh if needed (internally checks TTL and holds appropriate locks) + _ = c.Refresh(context.Background(), false) c.mu.RLock() defer c.mu.RUnlock() @@ -175,6 +170,7 @@ func (c *UnifiedCache) Refresh(ctx context.Context, force bool) error { // Step 1: Get installed packages (brew list) installed, err := c.fetchInstalledPackages() if err != nil { + // Keep existing cache on error - better stale than empty return fmt.Errorf("failed to fetch installed packages: %w", err) } From ae4722a6b0b7da88636a76a780185aef070989bb Mon Sep 17 00:00:00 2001 From: MrSnakeDoc Date: Tue, 20 Jan 2026 18:30:14 +0000 Subject: [PATCH 7/9] test(cache): add unit tests for UnifiedCache core functionality --- internal/brew/unified_cache_test.go | 233 ++++++++++++++++++++++++++++ 1 file changed, 233 insertions(+) create mode 100644 internal/brew/unified_cache_test.go diff --git a/internal/brew/unified_cache_test.go b/internal/brew/unified_cache_test.go new file mode 100644 index 0000000..c4fffba --- /dev/null +++ b/internal/brew/unified_cache_test.go @@ -0,0 +1,233 @@ +package brew + +import ( + "context" + "path/filepath" + "testing" + "time" + + "github.com/MrSnakeDoc/keg/internal/runner" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// mockRunner implements runner.CommandRunner for testing +type testRunner struct { + listOutput string + listErr error + outdatedOutput string + outdatedErr error + infoOutput string + infoErr error +} + +func (m *testRunner) Run(ctx context.Context, timeout time.Duration, mode runner.Mode, cmd string, args ...string) ([]byte, error) { + if cmd == "brew" && len(args) > 0 { + switch args[0] { + case "list": + return []byte(m.listOutput), m.listErr + case "outdated": + return []byte(m.outdatedOutput), m.outdatedErr + case "info": + return []byte(m.infoOutput), m.infoErr + } + } + return []byte{}, nil +} + +func TestUnifiedCache_IsInstalled(t *testing.T) { + r := &testRunner{ + listOutput: "wget\ncurl\njq\n", + } + + cache := &UnifiedCache{ + Packages: map[string]PackageState{ + "wget": {Installed: true, FetchedAt: time.Now()}, + "curl": {Installed: true, FetchedAt: time.Now()}, + }, + LastUpdated: time.Now(), + TTL: 1 * time.Hour, + runner: r, + } + + assert.True(t, cache.IsInstalled("wget")) + assert.True(t, cache.IsInstalled("curl")) + assert.False(t, cache.IsInstalled("nonexistent")) +} + +func TestUnifiedCache_GetInstalledSet(t *testing.T) { + cache := &UnifiedCache{ + Packages: map[string]PackageState{ + "wget": {Installed: true, FetchedAt: time.Now()}, + "curl": {Installed: true, FetchedAt: time.Now()}, + "notins": {Installed: false, FetchedAt: time.Now()}, + }, + } + + installed := cache.GetInstalledSet() + assert.Len(t, installed, 2) + assert.Contains(t, installed, "wget") + assert.Contains(t, installed, "curl") + assert.NotContains(t, installed, "notins") +} + +func TestUnifiedCache_GetOutdatedMap(t *testing.T) { + cache := &UnifiedCache{ + Packages: map[string]PackageState{ + "wget": { + Installed: true, + Outdated: true, + InstalledVersion: "1.0.0", + LatestVersion: "1.1.0", + FetchedAt: time.Now(), + }, + "curl": { + Installed: true, + Outdated: false, + FetchedAt: time.Now(), + }, + "jq": { + Installed: false, + Outdated: true, + InstalledVersion: "1.5.0", + LatestVersion: "1.6.0", + FetchedAt: time.Now(), + }, + }, + } + + outdated := cache.GetOutdatedMap() + assert.Len(t, outdated, 1, "only installed+outdated packages should be returned") + assert.Contains(t, outdated, "wget") + assert.Equal(t, "1.0.0", outdated["wget"].InstalledVersion) + assert.Equal(t, "1.1.0", outdated["wget"].LatestVersion) +} + +func TestUnifiedCache_MarkInstalled(t *testing.T) { + cache := &UnifiedCache{ + Packages: make(map[string]PackageState), + cachePath: filepath.Join(t.TempDir(), "cache.json"), + } + + err := cache.MarkInstalled("wget", "1.0.0") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + assert.True(t, cache.Packages["wget"].Installed) + assert.Equal(t, "1.0.0", cache.Packages["wget"].InstalledVersion) + assert.Equal(t, "1.0.0", cache.Packages["wget"].LatestVersion) + assert.False(t, cache.Packages["wget"].Outdated) +} + +func TestUnifiedCache_MarkUpgraded(t *testing.T) { + cache := &UnifiedCache{ + Packages: map[string]PackageState{ + "wget": { + Installed: true, + Outdated: true, + InstalledVersion: "1.0.0", + LatestVersion: "1.1.0", + }, + }, + cachePath: filepath.Join(t.TempDir(), "cache.json"), + } + + err := cache.MarkUpgraded("wget", "1.1.0") + require.NoError(t, err) + + assert.True(t, cache.Packages["wget"].Installed) + assert.Equal(t, "1.1.0", cache.Packages["wget"].InstalledVersion) + assert.Equal(t, "1.1.0", cache.Packages["wget"].LatestVersion) + assert.False(t, cache.Packages["wget"].Outdated) +} + +func TestUnifiedCache_MarkUpgraded_SetsInstalledFlag(t *testing.T) { + cache := &UnifiedCache{ + Packages: make(map[string]PackageState), + cachePath: filepath.Join(t.TempDir(), "cache.json"), + } + + // Upgrade a package that doesn't exist in cache yet + err := cache.MarkUpgraded("newpkg", "2.0.0") + require.NoError(t, err) + + // Should be marked as installed + assert.True(t, cache.Packages["newpkg"].Installed, "upgraded package should be marked as installed") + assert.Equal(t, "2.0.0", cache.Packages["newpkg"].InstalledVersion) +} + +func TestUnifiedCache_MarkUninstalled(t *testing.T) { + cache := &UnifiedCache{ + Packages: map[string]PackageState{ + "wget": { + Installed: true, + InstalledVersion: "1.0.0", + }, + }, + cachePath: filepath.Join(t.TempDir(), "cache.json"), + } + + err := cache.MarkUninstalled("wget") + require.NoError(t, err) + + assert.False(t, cache.Packages["wget"].Installed) + assert.Empty(t, cache.Packages["wget"].InstalledVersion) + assert.False(t, cache.Packages["wget"].Outdated) +} + +func TestUnifiedCache_RefreshVersions_EmptyList(t *testing.T) { + cache := &UnifiedCache{ + Packages: make(map[string]PackageState), + runner: &testRunner{}, + } + + err := cache.RefreshVersions(context.Background(), []string{}) + require.NoError(t, err) +} + +func TestSplitLines_HandlesVariousInputs(t *testing.T) { + tests := []struct { + name string + input string + expected []string + }{ + { + name: "empty string", + input: "", + expected: []string{}, + }, + { + name: "single line", + input: "wget", + expected: []string{"wget"}, + }, + { + name: "multiple lines", + input: "wget\ncurl\njq", + expected: []string{"wget", "curl", "jq"}, + }, + { + name: "lines with spaces", + input: "wget\n curl \njq", + expected: []string{"wget", "curl", "jq"}, + }, + { + name: "empty lines", + input: "wget\n\ncurl\n\n", + expected: []string{"wget", "curl"}, + }, + { + name: "windows line endings", + input: "wget\r\ncurl\r\njq", + expected: []string{"wget", "curl", "jq"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := splitLines(tt.input) + assert.Equal(t, tt.expected, result) + }) + } +} From e225daeca5886f91dd0d4a62a543a0d095dde46a Mon Sep 17 00:00:00 2001 From: MrSnakeDoc Date: Tue, 20 Jan 2026 18:32:55 +0000 Subject: [PATCH 8/9] fix(logger): protect ColorPrinter access with read lock to prevent race condition --- internal/logger/logger.go | 32 +++++++++++++++++++++++++------- 1 file changed, 25 insertions(+), 7 deletions(-) diff --git a/internal/logger/logger.go b/internal/logger/logger.go index 5800f3f..51bfcf0 100644 --- a/internal/logger/logger.go +++ b/internal/logger/logger.go @@ -120,12 +120,15 @@ func Info(msg string, args ...interface{}) { if !ensureReady() { return } - // Format OUTSIDE the lock (80% of the work) + // Format message outside the lock formatted := formatMessage("✨ ", msg, args...) // Fast path: direct write for non-JSON mode if !useJSON.Load() { - writeDirect(p.Info(formatted)) + mu.RLock() + colorized := p.Info(formatted) + mu.RUnlock() + writeDirect(colorized) return } @@ -142,7 +145,10 @@ func Success(msg string, args ...interface{}) { formatted := formatMessage("✅ ", msg, args...) if !useJSON.Load() { - writeDirect(p.Success(formatted)) + mu.RLock() + colorized := p.Success(formatted) + mu.RUnlock() + writeDirect(colorized) return } @@ -158,7 +164,10 @@ func LogError(msg string, args ...interface{}) { formatted := formatMessage("❌ ", msg, args...) if !useJSON.Load() { - writeDirect(p.Error(formatted)) + mu.RLock() + colorized := p.Error(formatted) + mu.RUnlock() + writeDirect(colorized) return } @@ -174,7 +183,10 @@ func Warn(msg string, args ...interface{}) { formatted := formatMessage("⚠️ ", msg, args...) if !useJSON.Load() { - writeDirect(p.Warning(formatted)) + mu.RLock() + colorized := p.Warning(formatted) + mu.RUnlock() + writeDirect(colorized) return } @@ -199,7 +211,10 @@ func WarnInline(msg string, args ...interface{}) { return } formatted := formatMessage("⚠️ ", msg, args...) - writeDirect(p.Warning(formatted)) + mu.RLock() + colorized := p.Warning(formatted) + mu.RUnlock() + writeDirect(colorized) } func Debug(msg string, args ...interface{}) { @@ -218,7 +233,10 @@ func Debug(msg string, args ...interface{}) { formatted := formatMessage("🛠️ ", msg, args...) if !useJSON.Load() { - writeDirect(p.Debug(formatted)) + mu.RLock() + colorized := p.Debug(formatted) + mu.RUnlock() + writeDirect(colorized) return } From 8f076235e87a4255dbd1d79fff570d53ee1c2956 Mon Sep 17 00:00:00 2001 From: MrSnakeDoc Date: Tue, 20 Jan 2026 18:49:31 +0000 Subject: [PATCH 9/9] test(cache): fix MarkUninstalled test to match actual behavior and correct comment --- internal/brew/unified_cache_test.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/internal/brew/unified_cache_test.go b/internal/brew/unified_cache_test.go index c4fffba..904fd8f 100644 --- a/internal/brew/unified_cache_test.go +++ b/internal/brew/unified_cache_test.go @@ -11,7 +11,7 @@ import ( "github.com/stretchr/testify/require" ) -// mockRunner implements runner.CommandRunner for testing +// testRunner implements runner.CommandRunner for testing type testRunner struct { listOutput string listErr error @@ -164,6 +164,9 @@ func TestUnifiedCache_MarkUninstalled(t *testing.T) { Installed: true, InstalledVersion: "1.0.0", }, + "curl": { + Installed: true, + }, }, cachePath: filepath.Join(t.TempDir(), "cache.json"), } @@ -171,9 +174,10 @@ func TestUnifiedCache_MarkUninstalled(t *testing.T) { err := cache.MarkUninstalled("wget") require.NoError(t, err) - assert.False(t, cache.Packages["wget"].Installed) - assert.Empty(t, cache.Packages["wget"].InstalledVersion) - assert.False(t, cache.Packages["wget"].Outdated) + // Package should be completely removed from cache + assert.NotContains(t, cache.Packages, "wget", "uninstalled package should be deleted from cache") + // Other packages should remain + assert.Contains(t, cache.Packages, "curl") } func TestUnifiedCache_RefreshVersions_EmptyList(t *testing.T) {