more code cleanup

Author: Thomas Stromberg

README.md

@@ -213,24 +213,30 @@ The library fetches the following event kinds:
 - **Bot detection** (marks events from bots with `"bot": true`)
 - **Mention extraction** (populated in `targets` field for comments/reviews)
 - **Question detection** (marks comments containing questions)
-- **Caching support** via `prx.NewCacheClient()` for reduced API calls
+- **Caching support** via `prx.WithCacheStore()` for reduced API calls
 - **Structured logging** with slog
 - **Retry logic** with exponential backoff for API reliability
 
 ## Caching
 
-For applications that need to fetch the same PR data repeatedly:
+Caching is enabled by default with disk persistence to the user cache directory.
+
+For custom cache directories:
+
+```go
+store, err := prx.NewCacheStore("/tmp/prx-cache")
+client := prx.NewClient(token, prx.WithCacheStore(store))
+```
+
+To disable caching persistence (memory-only):
 
 ```go
-// Create a caching client
-cacheDir := "/tmp/prx-cache"
-client, err := prx.NewCacheClient(token, cacheDir)
+import "github.com/codeGROOVE-dev/sfcache/pkg/store/null"
 
-// Fetch with caching (uses updated_at timestamp for cache invalidation)
-data, err := client.PullRequest(ctx, "owner", "repo", 123, time.Now())
+client := prx.NewClient(token, prx.WithCacheStore(null.New[string, prx.PullRequestData]()))
 ```
 
-Cache files are automatically cleaned up after 20 days.
+Cache entries expire after 20 days.
 
 ## Authentication
 

pkg/prx/cache.go

@@ -5,11 +5,17 @@ package prx
 
 import (
 	"context"
+	"crypto/sha256"
+	"encoding/hex"
 	"encoding/json"
+	"errors"
+	"fmt"
 	"log/slog"
 	"net/http"
 	"os"
 	"path/filepath"
+	"strconv"
+	"strings"
 	"time"
 
 	"github.com/codeGROOVE-dev/sfcache"
@@ -21,6 +27,10 @@ const (
 	maxIdleConns        = 100
 	maxIdleConnsPerHost = 10
 	idleConnTimeoutSec  = 90
+
+	// Cache TTL constants.
+	prCacheTTL            = 20 * 24 * time.Hour // 20 days
+	collaboratorsCacheTTL = 4 * time.Hour
 )
 
 // PRStore is the interface for PR cache storage backends.
@@ -187,3 +197,40 @@ func (c *Client) PullRequestWithReferenceTime(
 	}
 	return &result, nil
 }
+
+// Close releases cache resources.
+func (c *Client) Close() error {
+	if c.prCache != nil {
+		return c.prCache.Close()
+	}
+	return nil
+}
+
+// NewCacheStore creates a cache store backed by the given directory.
+// This is a convenience function for use with WithCacheStore.
+func NewCacheStore(dir string) (PRStore, error) {
+	dir = filepath.Clean(dir)
+	if !filepath.IsAbs(dir) {
+		return nil, errors.New("cache directory must be absolute path")
+	}
+	if err := os.MkdirAll(dir, 0o700); err != nil {
+		return nil, fmt.Errorf("creating cache directory: %w", err)
+	}
+	store, err := localfs.New[string, PullRequestData]("prx-pr", dir)
+	if err != nil {
+		return nil, fmt.Errorf("creating PR cache store: %w", err)
+	}
+	return store, nil
+}
+
+// prCacheKey generates a cache key for PR data.
+func prCacheKey(owner, repo string, prNumber int) string {
+	key := strings.Join([]string{"graphql", "pr_graphql", owner, repo, strconv.Itoa(prNumber)}, "/")
+	hash := sha256.Sum256([]byte(key))
+	return hex.EncodeToString(hash[:])
+}
+
+// collaboratorsCacheKey generates a cache key for collaborators data.
+func collaboratorsCacheKey(owner, repo string) string {
+	return fmt.Sprintf("%s/%s", owner, repo)
+}

pkg/prx/client.go

@@ -2,7 +2,6 @@ package prx
 
 import (
 	"context"
-	"encoding/json"
 	"log/slog"
 	"net/http"
 	"net/http/httptest"
@@ -15,62 +14,12 @@ func TestCacheClient(t *testing.T) {
 	// Create temporary cache directory
 	cacheDir := t.TempDir()
 
-	// Create test server
+	// Create test server that handles GraphQL and REST endpoints
 	requestCount := 0
 	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		requestCount++
 
 		switch r.URL.Path {
-		case "/repos/test/repo/pulls/1":
-			pr := githubPullRequest{
-				Number:    1,
-				Title:     "Test PR",
-				Body:      "Test body",
-				CreatedAt: time.Now().Add(-24 * time.Hour),
-				UpdatedAt: time.Now().Add(-2 * time.Hour),
-				User:      &githubUser{Login: "testuser"},
-				State:     "closed",
-				ClosedAt:  time.Now().Add(-1 * time.Hour),
-			}
-			pr.Head.SHA = "abc123"
-			if err := json.NewEncoder(w).Encode(pr); err != nil {
-				http.Error(w, err.Error(), http.StatusInternalServerError)
-				return
-			}
-		case "/repos/test/repo/pulls/1/commits":
-			commit := &githubPullRequestCommit{
-				Author: &githubUser{Login: "testuser"},
-			}
-			commit.Commit.Author.Date = time.Now().Add(-12 * time.Hour)
-			commit.Commit.Message = "Test commit"
-			if err := json.NewEncoder(w).Encode([]*githubPullRequestCommit{commit}); err != nil {
-				http.Error(w, err.Error(), http.StatusInternalServerError)
-				return
-			}
-		case "/repos/test/repo/commits/abc123/status":
-			// Combined status endpoint - return empty response
-			if _, err := w.Write([]byte(`{"state": "success", "statuses": []}`)); err != nil {
-				http.Error(w, err.Error(), http.StatusInternalServerError)
-				return
-			}
-		case "/repos/test/repo/branches//protection":
-			// Branch protection endpoint - return empty protection
-			if _, err := w.Write([]byte(`{}`)); err != nil {
-				http.Error(w, err.Error(), http.StatusInternalServerError)
-				return
-			}
-		case "/repos/test/repo/branches//protection/required_status_checks":
-			// Required status checks endpoint - return no required checks
-			if _, err := w.Write([]byte(`{}`)); err != nil {
-				http.Error(w, err.Error(), http.StatusInternalServerError)
-				return
-			}
-		case "/repos/test/repo/rulesets":
-			// Rulesets endpoint - return empty rulesets
-			if _, err := w.Write([]byte(`[]`)); err != nil {
-				http.Error(w, err.Error(), http.StatusInternalServerError)
-				return
-			}
 		case "/graphql":
 			// GraphQL endpoint - return a minimal PR response
 			response := `{"data": {"repository": {"pullRequest": {
@@ -107,8 +56,13 @@ func TestCacheClient(t *testing.T) {
 				http.Error(w, err.Error(), http.StatusInternalServerError)
 				return
 			}
+		case "/repos/test/repo/rulesets":
+			if _, err := w.Write([]byte(`[]`)); err != nil {
+				http.Error(w, err.Error(), http.StatusInternalServerError)
+				return
+			}
 		default:
-			// Check runs endpoint expects a different format
+			// Check runs endpoint
 			if r.URL.Path == "/repos/test/repo/commits/abc123/check-runs" {
 				if _, err := w.Write([]byte(`{"check_runs": []}`)); err != nil {
 					http.Error(w, err.Error(), http.StatusInternalServerError)
@@ -125,14 +79,16 @@ func TestCacheClient(t *testing.T) {
 	}))
 	defer server.Close()
 
-	// Create cache client with test server
-	client, err := NewCacheClient("test-token", cacheDir,
+	// Create cache store and client with test server
+	store, err := NewCacheStore(cacheDir)
+	if err != nil {
+		t.Fatalf("Failed to create cache store: %v", err)
+	}
+	client := NewClient("test-token",
+		WithCacheStore(store),
 		WithHTTPClient(&http.Client{Transport: &http.Transport{}}),
 		WithLogger(slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelDebug}))),
 	)
-	if err != nil {
-		t.Fatalf("Failed to create cache client: %v", err)
-	}
 	defer func() {
 		if closeErr := client.Close(); closeErr != nil {
 			t.Errorf("Failed to close client: %v", closeErr)

pkg/prx/cache_test.go pkg/prx/client_cache_test.gopkg/prx/cache_test.go renamed to pkg/prx/client_cache_test.go

@@ -5,7 +5,7 @@ import (
 	"testing"
 )
 
-func TestNewCacheClient(t *testing.T) {
+func TestNewCacheStore(t *testing.T) {
 	tmpDir := t.TempDir()
 
 	tests := []struct {
@@ -27,7 +27,7 @@ func TestNewCacheClient(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			client, err := NewCacheClient("test-token", tt.dir)
+			store, err := NewCacheStore(tt.dir)
 			if tt.wantErr {
 				if err == nil {
 					t.Errorf("Expected error but got none")
@@ -36,14 +36,9 @@ func TestNewCacheClient(t *testing.T) {
 				if err != nil {
 					t.Errorf("Unexpected error: %v", err)
 				}
-				if client == nil {
-					t.Errorf("Expected client but got nil")
+				if store == nil {
+					t.Errorf("Expected store but got nil")
 				}
-				defer func() {
-					if closeErr := client.Close(); closeErr != nil {
-						t.Errorf("Failed to close client: %v", closeErr)
-					}
-				}()
 			}
 		})
 	}
@@ -109,12 +104,13 @@ func TestCollaboratorsCacheKey(t *testing.T) {
 	}
 }
 
-func TestCacheClientClose(t *testing.T) {
+func TestClientClose(t *testing.T) {
 	tmpDir := t.TempDir()
-	client, err := NewCacheClient("test-token", tmpDir)
+	store, err := NewCacheStore(tmpDir)
 	if err != nil {
-		t.Fatalf("Failed to create cache client: %v", err)
+		t.Fatalf("Failed to create cache store: %v", err)
 	}
+	client := NewClient("test-token", WithCacheStore(store))
 
 	// Close should not error
 	if err := client.Close(); err != nil {

pkg/prx/cache_extended_test.go pkg/prx/client_cachestore_test.gopkg/prx/cache_extended_test.go renamed to pkg/prx/client_cachestore_test.go

@@ -135,10 +135,11 @@ func TestClient_PullRequestWithCache(t *testing.T) {
 	defer server.Close()
 
 	httpClient := &http.Client{Transport: http.DefaultTransport}
-	client, err := NewCacheClient("test-token", tmpDir, WithHTTPClient(httpClient))
+	store, err := NewCacheStore(tmpDir)
 	if err != nil {
-		t.Fatalf("Failed to create cache client: %v", err)
+		t.Fatalf("Failed to create cache store: %v", err)
 	}
+	client := NewClient("test-token", WithCacheStore(store), WithHTTPClient(httpClient))
 
 	// Override the API URL
 	client.github = &githubClient{