Merge pull request #118 from tstromberg/others

change dashboard URL to my.reviewGOOSE.dev & add test coverage

Author: tstromberg

cmd/reviewGOOSE/cache.go

@@ -2,101 +2,79 @@ package main
 
 import (
 	"context"
-	"crypto/sha256"
-	"encoding/hex"
 	"encoding/json"
 	"fmt"
 	"log/slog"
-	"os"
-	"path/filepath"
-	"strings"
 	"time"
 
+	"github.com/codeGROOVE-dev/goose/pkg/prcache"
 	"github.com/codeGROOVE-dev/goose/pkg/safebrowse"
 	"github.com/codeGROOVE-dev/retry"
 	"github.com/codeGROOVE-dev/turnclient/pkg/turn"
 )
 
-type cacheEntry struct {
-	Data      *turn.CheckResponse `json:"data"`
-	CachedAt  time.Time           `json:"cached_at"`
-	UpdatedAt time.Time           `json:"updated_at"`
-}
-
 // checkCache checks the cache for a PR and returns the cached data if valid.
 // Returns (data, hit, running) where running indicates incomplete tests.
-func (app *App) checkCache(path, url string, updatedAt time.Time) (data *turn.CheckResponse, hit, running bool) {
-	b, err := os.ReadFile(path)
-	if err != nil {
-		if !os.IsNotExist(err) {
-			slog.Debug("[CACHE] Cache file read error", "url", url, "error", err)
+func (app *App) checkCache(cacheManager *prcache.Manager, path, url string, updatedAt time.Time) (data *turn.CheckResponse, hit, running bool) {
+	// State check function for incomplete tests
+	stateCheck := func(d any) bool {
+		if m, ok := d.(map[string]any); ok {
+			if pr, ok := m["pull_request"].(map[string]any); ok {
+				if state, ok := pr["test_state"].(string); ok {
+					incomplete := state == "running" || state == "queued" || state == "pending"
+					// Only bypass for recently updated PRs
+					if incomplete && time.Since(updatedAt) < time.Hour {
+						return true
+					}
+				}
+			}
 		}
-		return nil, false, false
+		return false
 	}
 
-	var e cacheEntry
-	if err := json.Unmarshal(b, &e); err != nil {
-		slog.Warn("Failed to unmarshal cache data", "url", url, "error", err)
-		if err := os.Remove(path); err != nil {
-			slog.Debug("Failed to remove corrupted cache file", "error", err)
-		}
+	// Determine TTL based on whether we expect tests to be running
+	ttl := cacheTTL
+	bypassTTL := runningTestsCacheBypass
+
+	result, err := cacheManager.Get(path, updatedAt, ttl, bypassTTL, stateCheck)
+	if err != nil {
+		slog.Debug("[CACHE] Cache error", "url", url, "error", err)
 		return nil, false, false
 	}
-	if e.Data == nil {
-		slog.Warn("Cache entry missing data", "url", url)
-		if err := os.Remove(path); err != nil {
-			slog.Debug("Failed to remove corrupted cache file", "error", err)
-		}
-		return nil, false, false
+
+	if !result.Hit {
+		return nil, false, result.ShouldBypass
 	}
 
-	// Determine TTL based on test state - use shorter TTL for incomplete tests
-	state := e.Data.PullRequest.TestState
-	incomplete := state == "running" || state == "queued" || state == "pending"
-	ttl := cacheTTL
-	if incomplete {
-		ttl = runningTestsCacheTTL
+	// Extract turn.CheckResponse from cached data
+	if result.Entry == nil || result.Entry.Data == nil {
+		return nil, false, false
 	}
 
-	// Check if cache is expired or PR updated
-	if time.Since(e.CachedAt) >= ttl || !e.UpdatedAt.Equal(updatedAt) {
-		if !e.UpdatedAt.Equal(updatedAt) {
-			slog.Debug("[CACHE] Cache miss - PR updated",
-				"url", url,
-				"cached_pr_time", e.UpdatedAt.Format(time.RFC3339),
-				"current_pr_time", updatedAt.Format(time.RFC3339))
-		} else {
-			slog.Debug("[CACHE] Cache miss - TTL expired",
-				"url", url,
-				"cached_at", e.CachedAt.Format(time.RFC3339),
-				"cache_age", time.Since(e.CachedAt).Round(time.Second),
-				"ttl", ttl,
-				"test_state", state)
-		}
-		return nil, false, incomplete
+	// Convert map back to CheckResponse
+	dataBytes, err := json.Marshal(result.Entry.Data)
+	if err != nil {
+		slog.Warn("Failed to marshal cached data", "url", url, "error", err)
+		return nil, false, false
 	}
 
-	// Invalidate cache for incomplete tests on recently-updated PRs to catch completion
-	// Skip this for PRs not updated in over an hour - their pending tests are likely stale
-	age := time.Since(e.CachedAt)
-	if incomplete && age < runningTestsCacheBypass && time.Since(updatedAt) < time.Hour {
-		slog.Debug("[CACHE] Cache invalidated - tests incomplete and cache entry is fresh",
-			"url", url,
-			"test_state", state,
-			"cache_age", age.Round(time.Minute),
-			"cached_at", e.CachedAt.Format(time.RFC3339))
-		return nil, false, true
+	var response turn.CheckResponse
+	if err := json.Unmarshal(dataBytes, &response); err != nil {
+		slog.Warn("Failed to unmarshal cached data", "url", url, "error", err)
+		return nil, false, false
 	}
 
 	slog.Debug("[CACHE] Cache hit",
 		"url", url,
-		"cached_at", e.CachedAt.Format(time.RFC3339),
-		"cache_age", time.Since(e.CachedAt).Round(time.Second),
-		"pr_updated_at", e.UpdatedAt.Format(time.RFC3339))
+		"cached_at", result.Entry.CachedAt.Format(time.RFC3339),
+		"cache_age", time.Since(result.Entry.CachedAt).Round(time.Second),
+		"pr_updated_at", result.Entry.UpdatedAt.Format(time.RFC3339))
+
 	if app.healthMonitor != nil {
 		app.healthMonitor.recordCacheAccess(true)
 	}
-	return e.Data, true, false
+
+	return &response, true, false
 }
 
 // turnData fetches Turn API data with caching.
@@ -110,21 +88,20 @@ func (app *App) turnData(ctx context.Context, url string, updatedAt time.Time) (
 		return nil, false, fmt.Errorf("invalid URL: %w", err)
 	}
 
-	// Create cache key from URL and updated timestamp
-	key := fmt.Sprintf("%s-%s", url, updatedAt.Format(time.RFC3339))
-	h := sha256.Sum256([]byte(key))
-	path := filepath.Join(app.cacheDir, hex.EncodeToString(h[:])[:16]+".json")
+	// Create cache manager and path
+	cacheManager := prcache.NewManager(app.cacheDir)
+	cacheKey := prcache.CacheKey(url, updatedAt)
+	path := cacheManager.CachePath(cacheKey)
 
 	slog.Debug("[CACHE] Checking cache",
 		"url", url,
 		"updated_at", updatedAt.Format(time.RFC3339),
-		"cache_key", key,
-		"cache_file", filepath.Base(path))
+		"cache_key", cacheKey)
 
 	// Check cache unless --no-cache flag is set
 	var running bool
 	if !app.noCache {
-		data, hit, r := app.checkCache(path, url, updatedAt)
+		data, hit, r := app.checkCache(cacheManager, path, url, updatedAt)
 		if hit {
 			return data, true, nil
 		}
@@ -203,18 +180,11 @@ func (app *App) turnData(ctx context.Context, url string, updatedAt time.Time) (
 
 	// Save to cache (don't fail if caching fails)
 	if !app.noCache && data != nil {
-		e := cacheEntry{Data: data, CachedAt: time.Now(), UpdatedAt: updatedAt}
-		b, err := json.Marshal(e)
-		if err != nil {
-			slog.Error("Failed to marshal cache data", "url", url, "error", err)
-		} else if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil {
-			slog.Error("Failed to create cache directory", "error", err)
-		} else if err := os.WriteFile(path, b, 0o600); err != nil {
-			slog.Error("Failed to write cache file", "error", err)
+		if err := cacheManager.Put(path, data, updatedAt); err != nil {
+			slog.Error("Failed to save cache", "url", url, "error", err)
 		} else {
 			slog.Debug("[CACHE] Saved to cache",
 				"url", url,
-				"cache_file", filepath.Base(path),
 				"test_state", data.PullRequest.TestState)
 		}
 	}
@@ -224,33 +194,8 @@ func (app *App) turnData(ctx context.Context, url string, updatedAt time.Time) (
 
 // cleanupOldCache removes cache files older than the cleanup interval (15 days).
 func (app *App) cleanupOldCache() {
-	entries, err := os.ReadDir(app.cacheDir)
-	if err != nil {
-		slog.Error("Failed to read cache directory for cleanup", "error", err)
-		return
-	}
-
-	var cleaned, errs int
-	for _, e := range entries {
-		if !strings.HasSuffix(e.Name(), ".json") {
-			continue
-		}
-		info, err := e.Info()
-		if err != nil {
-			slog.Error("Failed to get file info for cache entry", "entry", e.Name(), "error", err)
-			errs++
-			continue
-		}
-		if time.Since(info.ModTime()) > cacheCleanupInterval {
-			p := filepath.Join(app.cacheDir, e.Name())
-			if err := os.Remove(p); err != nil {
-				slog.Error("Failed to remove old cache file", "file", p, "error", err)
-				errs++
-			} else {
-				cleaned++
-			}
-		}
-	}
+	cacheManager := prcache.NewManager(app.cacheDir)
+	cleaned, errs := cacheManager.CleanupOldFiles(cacheCleanupInterval)
 
 	if cleaned > 0 || errs > 0 {
 		slog.Info("Cache cleanup completed", "removed", cleaned, "errors", errs)

cmd/reviewGOOSE/deadlock_test.go

@@ -4,6 +4,8 @@ import (
 	"sync"
 	"testing"
 	"time"
+
+	"github.com/codeGROOVE-dev/goose/pkg/ratelimit"
 )
 
 // TestConcurrentMenuOperations tests that concurrent menu operations don't cause deadlocks
@@ -14,7 +16,7 @@ func TestConcurrentMenuOperations(t *testing.T) {
 		hiddenOrgs:         make(map[string]bool),
 		seenOrgs:           make(map[string]bool),
 		blockedPRTimes:     make(map[string]time.Time),
-		browserRateLimiter: NewBrowserRateLimiter(startupGracePeriod, 5, defaultMaxBrowserOpensDay),
+		browserRateLimiter: ratelimit.NewBrowserRateLimiter(startupGracePeriod, 5, defaultMaxBrowserOpensDay),
 		systrayInterface:   &MockSystray{},
 		incoming: []PR{
 			{Repository: "org1/repo1", Number: 1, Title: "Fix bug", URL: "https://github.com/org1/repo1/pull/1"},
@@ -101,7 +103,7 @@ func TestMenuClickDeadlockScenario(t *testing.T) {
 		hiddenOrgs:         make(map[string]bool),
 		seenOrgs:           make(map[string]bool),
 		blockedPRTimes:     make(map[string]time.Time),
-		browserRateLimiter: NewBrowserRateLimiter(startupGracePeriod, 5, defaultMaxBrowserOpensDay),
+		browserRateLimiter: ratelimit.NewBrowserRateLimiter(startupGracePeriod, 5, defaultMaxBrowserOpensDay),
 		systrayInterface:   &MockSystray{},
 		incoming: []PR{
 			{Repository: "org1/repo1", Number: 1, Title: "Test PR", URL: "https://github.com/org1/repo1/pull/1"},
@@ -142,7 +144,7 @@ func TestRapidMenuClicks(t *testing.T) {
 		hiddenOrgs:         make(map[string]bool),
 		seenOrgs:           make(map[string]bool),
 		blockedPRTimes:     make(map[string]time.Time),
-		browserRateLimiter: NewBrowserRateLimiter(startupGracePeriod, 5, defaultMaxBrowserOpensDay),
+		browserRateLimiter: ratelimit.NewBrowserRateLimiter(startupGracePeriod, 5, defaultMaxBrowserOpensDay),
 		systrayInterface:   &MockSystray{},
 		lastSearchAttempt:  time.Now().Add(-15 * time.Second), // Allow first click
 		incoming: []PR{