From 0f92f75186e69053cdb54063511c5f57dd1506e7 Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Thu, 30 Oct 2025 08:38:28 -0400 Subject: [PATCH 1/2] Add turnclient as a commit->PR lookup fallback --- pkg/bot/bot.go | 147 +++++++++++++++++++++++++++++++++++++- pkg/bot/bot_sprinkler.go | 134 +++++++++++++++++++++++++++++++++- pkg/bot/sprinkler_test.go | 6 ++ 3 files changed, 283 insertions(+), 4 deletions(-) diff --git a/pkg/bot/bot.go b/pkg/bot/bot.go index de0e3f3..6ecf4ec 100644 --- a/pkg/bot/bot.go +++ b/pkg/bot/bot.go @@ -51,6 +51,21 @@ type ThreadCache struct { // ThreadInfo is an alias to state.ThreadInfo to avoid duplication. type ThreadInfo = state.ThreadInfo +// CommitPREntry caches recent commit→PR mappings for fast lookup. +type CommitPREntry struct { + PRNumber int + HeadSHA string + UpdatedAt time.Time +} + +// CommitPRCache provides in-memory caching of commit SHA → PR mappings. +// This allows quick lookup when check events arrive with just a commit SHA, +// avoiding expensive GitHub API calls for recently-seen PRs. +type CommitPRCache struct { + mu sync.RWMutex + entries map[string][]CommitPREntry // "owner/repo" -> recent PRs with commits +} + // Get retrieves thread info for a PR. func (tc *ThreadCache) Get(prKey string) (ThreadInfo, bool) { tc.mu.RLock() @@ -81,6 +96,113 @@ func (tc *ThreadCache) Cleanup(maxAge time.Duration) { } } +// RecordPR records a PR's head commit SHA for commit→PR lookups. +// Entries are kept for 10 minutes to handle check events that arrive shortly after PR events. +func (cpc *CommitPRCache) RecordPR(owner, repo string, prNumber int, headSHA string) { + if headSHA == "" { + return // Skip empty commits + } + + cpc.mu.Lock() + defer cpc.mu.Unlock() + + repoKey := owner + "/" + repo + now := time.Now() + + // Initialize map if needed + if cpc.entries == nil { + cpc.entries = make(map[string][]CommitPREntry) + } + + // Add new entry + entry := CommitPREntry{ + PRNumber: prNumber, + HeadSHA: headSHA, + UpdatedAt: now, + } + + // Get existing entries for this repo + entries := cpc.entries[repoKey] + + // Check if this exact PR+commit combination already exists - update timestamp if so + found := false + for i := range entries { + if entries[i].PRNumber == prNumber && entries[i].HeadSHA == headSHA { + entries[i].UpdatedAt = now // Refresh timestamp + found = true + break + } + } + + if !found { + entries = append(entries, entry) + } + + // Update the map with the modified entries before filtering + cpc.entries[repoKey] = entries + + // Keep only entries from last 10 minutes (check events usually arrive within seconds) + cutoff := now.Add(-10 * time.Minute) + filtered := make([]CommitPREntry, 0, len(entries)) + for i := range entries { + if entries[i].UpdatedAt.After(cutoff) { + filtered = append(filtered, entries[i]) + } + } + + cpc.entries[repoKey] = filtered +} + +// FindPRsForCommit finds PRs in a repo that match the given commit SHA. +// Returns PR numbers if found in recent cache (last 10 minutes), nil otherwise. +func (cpc *CommitPRCache) FindPRsForCommit(owner, repo, commitSHA string) []int { + if commitSHA == "" { + return nil + } + + cpc.mu.RLock() + defer cpc.mu.RUnlock() + + repoKey := owner + "/" + repo + entries, exists := cpc.entries[repoKey] + if !exists { + return nil + } + + // Check which PRs have this commit + var prNumbers []int + for i := range entries { + if entries[i].HeadSHA == commitSHA { + prNumbers = append(prNumbers, entries[i].PRNumber) + } + } + + return prNumbers +} + +// MostRecentPR returns the most recently updated PR number for a repo from the cache. +// Returns 0 if no recent PRs are cached for this repo. +func (cpc *CommitPRCache) MostRecentPR(owner, repo string) int { + cpc.mu.RLock() + defer cpc.mu.RUnlock() + + repoKey := owner + "/" + repo + entries, exists := cpc.entries[repoKey] + if !exists || len(entries) == 0 { + return 0 + } + + // Find the entry with the most recent UpdatedAt timestamp + mostRecent := entries[0] + for i := 1; i < len(entries); i++ { + if entries[i].UpdatedAt.After(mostRecent.UpdatedAt) { + mostRecent = entries[i] + } + } + + return mostRecent.PRNumber +} + // Coordinator coordinates between GitHub, Slack, and notifications for a single org. // //nolint:govet // Field order optimized for logical grouping over memory alignment @@ -94,8 +216,9 @@ type Coordinator struct { configManager *config.Manager notifier *notify.Manager userMapper *usermapping.Service - threadCache *ThreadCache // In-memory cache for fast lookups - eventSemaphore chan struct{} // Limits concurrent event processing (prevents overwhelming APIs) + threadCache *ThreadCache // In-memory cache for fast lookups + commitPRCache *CommitPRCache // Maps commit SHAs to PR numbers for check events + eventSemaphore chan struct{} // Limits concurrent event processing (prevents overwhelming APIs) } // StateStore interface for persistent state - allows dependency injection for testing. @@ -134,6 +257,9 @@ func New( prThreads: make(map[string]ThreadInfo), creating: make(map[string]bool), }, + commitPRCache: &CommitPRCache{ + entries: make(map[string][]CommitPREntry), + }, eventSemaphore: make(chan struct{}, 10), // Allow 10 concurrent events per org } @@ -1546,6 +1672,23 @@ func (c *Coordinator) handlePullRequestFromSprinkler( // Use PR details from turnclient instead of making additional GitHub API call pr := checkResult.PullRequest + // Populate commit→PR cache for all commits in this PR + // This allows us to quickly map check events (which only have commit SHA) back to PRs + // without making expensive GitHub API calls + if len(pr.Commits) > 0 { + slog.Debug("populating commit→PR cache from turnclient response", + logFieldOwner, owner, + logFieldRepo, repo, + "pr_number", prNumber, + "commit_count", len(pr.Commits)) + + for _, commitSHA := range pr.Commits { + if commitSHA != "" { + c.commitPRCache.RecordPR(owner, repo, prNumber, commitSHA) + } + } + } + // Create a synthetic webhook event to reuse existing logic with real PR data event := struct { Action string `json:"action"` diff --git a/pkg/bot/bot_sprinkler.go b/pkg/bot/bot_sprinkler.go index 23c0946..8852ceb 100644 --- a/pkg/bot/bot_sprinkler.go +++ b/pkg/bot/bot_sprinkler.go @@ -11,6 +11,7 @@ import ( "github.com/codeGROOVE-dev/slacker/pkg/state" "github.com/codeGROOVE-dev/sprinkler/pkg/client" + "github.com/codeGROOVE-dev/turnclient/pkg/turn" ) // Constants for URL parsing. @@ -84,13 +85,142 @@ func (c *Coordinator) lookupPRsForCheckEvent(ctx context.Context, event client.E owner := parts[3] repo := parts[4] - slog.Info("sprinkler cache miss - looking up PRs for commit via GitHub API", + // First, check our local commit→PR cache (populated from recent PR events) + // This is MUCH faster than GitHub API and usually works since check events + // arrive seconds after PR events + cachedPRs := c.commitPRCache.FindPRsForCommit(owner, repo, commitSHA) + if len(cachedPRs) > 0 { + slog.Info("found PRs for commit in cache - avoiding GitHub API call", + "organization", organization, + "owner", owner, + "repo", repo, + "commit_sha", commitSHA, + "pr_count", len(cachedPRs), + "pr_numbers", cachedPRs, + "type", event.Type, + "delivery_id", deliveryID, + "cache_hit", true) + return cachedPRs + } + + // Cache miss - try turnclient lookup on most recent PR before falling back to GitHub API + slog.Info("commit→PR cache miss - will try turnclient on recent PR before GitHub API", "organization", organization, "owner", owner, "repo", repo, "commit_sha", commitSHA, "type", event.Type, - "delivery_id", deliveryID) + "delivery_id", deliveryID, + "cache_hit", false, + "reason", "check event arrived before PR event or cache expired") + + // Second attempt: Check if we recently saw a PR for this repo + // If yes, fetch it via turnclient to see if it contains this commit + // This is cheaper than searching all PRs via GitHub API + mostRecentPR := c.commitPRCache.MostRecentPR(owner, repo) + if mostRecentPR > 0 { + slog.Debug("attempting turnclient lookup on most recent PR for repo", + "organization", organization, + "owner", owner, + "repo", repo, + "pr_number", mostRecentPR, + "commit_sha", commitSHA, + "rationale", "check events usually arrive for recently updated PRs") + + // Get GitHub token + githubToken := c.github.InstallationToken(ctx) + if githubToken != "" { + // Create turnclient + turnClient, tcErr := turn.NewDefaultClient() + if tcErr == nil { + turnClient.SetAuthToken(githubToken) + + // Check the recent PR with current timestamp + checkCtx, checkCancel := context.WithTimeout(ctx, 30*time.Second) + prURL := fmt.Sprintf("https://github.com/%s/%s/pull/%d", owner, repo, mostRecentPR) + checkResult, checkErr := turnClient.Check(checkCtx, prURL, owner, time.Now()) + checkCancel() + + if checkErr == nil && checkResult != nil { + // Check if any commits match + for _, prCommit := range checkResult.PullRequest.Commits { + if prCommit == commitSHA { + slog.Info("found commit in most recent PR via turnclient - avoiding GitHub API search", + "organization", organization, + "owner", owner, + "repo", repo, + "pr_number", mostRecentPR, + "commit_sha", commitSHA, + "turnclient_hit", true, + "bonus", "got free PR status update") + + // Populate cache with all commits from this PR + for _, commit := range checkResult.PullRequest.Commits { + if commit != "" { + c.commitPRCache.RecordPR(owner, repo, mostRecentPR, commit) + } + } + + // Process the PR update since we have fresh data + go c.handlePullRequestEventWithData(context.Background(), owner, repo, struct { + Action string `json:"action"` + PullRequest struct { + HTMLURL string `json:"html_url"` + Title string `json:"title"` + CreatedAt time.Time `json:"created_at"` + User struct { + Login string `json:"login"` + } `json:"user"` + Number int `json:"number"` + } `json:"pull_request"` + Number int `json:"number"` + }{ + Action: "synchronize", + PullRequest: struct { + HTMLURL string `json:"html_url"` + Title string `json:"title"` + CreatedAt time.Time `json:"created_at"` + User struct { + Login string `json:"login"` + } `json:"user"` + Number int `json:"number"` + }{ + HTMLURL: prURL, + Title: checkResult.PullRequest.Title, + CreatedAt: checkResult.PullRequest.CreatedAt, + User: struct { + Login string `json:"login"` + }{ + Login: checkResult.PullRequest.Author, + }, + Number: mostRecentPR, + }, + Number: mostRecentPR, + }, checkResult, nil) + + return []int{mostRecentPR} + } + } + + slog.Debug("commit not found in most recent PR - will fall back to GitHub API", + "organization", organization, + "owner", owner, + "repo", repo, + "pr_number", mostRecentPR, + "commit_sha", commitSHA, + "pr_commits_checked", len(checkResult.PullRequest.Commits)) + } + } + } + } + + // Third attempt: Fall back to GitHub API to search all PRs + slog.Info("falling back to GitHub API to search all PRs for commit", + "organization", organization, + "owner", owner, + "repo", repo, + "commit_sha", commitSHA, + "tried_turnclient", mostRecentPR > 0) // Look up PRs for this commit using GitHub API // Allow up to 3 minutes for retries (2 min max delay + buffer) diff --git a/pkg/bot/sprinkler_test.go b/pkg/bot/sprinkler_test.go index b5d28dd..790b1cd 100644 --- a/pkg/bot/sprinkler_test.go +++ b/pkg/bot/sprinkler_test.go @@ -66,6 +66,7 @@ func TestLookupPRsForCheckEvent_Success(t *testing.T) { stateStore: &mockStateStore{}, configManager: config.New(), threadCache: &ThreadCache{prThreads: make(map[string]ThreadInfo), creating: make(map[string]bool)}, + commitPRCache: &CommitPRCache{entries: make(map[string][]CommitPREntry)}, eventSemaphore: make(chan struct{}, 10), } @@ -103,6 +104,7 @@ func TestLookupPRsForCheckEvent_NoCommitSHA(t *testing.T) { stateStore: &mockStateStore{}, configManager: config.New(), threadCache: &ThreadCache{prThreads: make(map[string]ThreadInfo), creating: make(map[string]bool)}, + commitPRCache: &CommitPRCache{entries: make(map[string][]CommitPREntry)}, eventSemaphore: make(chan struct{}, 10), } @@ -136,6 +138,7 @@ func TestLookupPRsForCheckEvent_InvalidURL(t *testing.T) { stateStore: &mockStateStore{}, configManager: config.New(), threadCache: &ThreadCache{prThreads: make(map[string]ThreadInfo), creating: make(map[string]bool)}, + commitPRCache: &CommitPRCache{entries: make(map[string][]CommitPREntry)}, eventSemaphore: make(chan struct{}, 10), } @@ -172,6 +175,7 @@ func TestLookupPRsForCheckEvent_NoPRsFound(t *testing.T) { stateStore: &mockStateStore{}, configManager: config.New(), threadCache: &ThreadCache{prThreads: make(map[string]ThreadInfo), creating: make(map[string]bool)}, + commitPRCache: &CommitPRCache{entries: make(map[string][]CommitPREntry)}, eventSemaphore: make(chan struct{}, 10), } @@ -338,6 +342,7 @@ func TestHandleSprinklerEvent_CheckEventWithCommit(t *testing.T) { stateStore: mockState, configManager: config.New(), threadCache: &ThreadCache{prThreads: make(map[string]ThreadInfo), creating: make(map[string]bool)}, + commitPRCache: &CommitPRCache{entries: make(map[string][]CommitPREntry)}, eventSemaphore: make(chan struct{}, 10), } @@ -376,6 +381,7 @@ func TestLookupPRsForCheckEvent_GitHubAPIFailure(t *testing.T) { stateStore: &mockStateStore{processedEvents: make(map[string]bool)}, configManager: config.New(), threadCache: &ThreadCache{prThreads: make(map[string]ThreadInfo), creating: make(map[string]bool)}, + commitPRCache: &CommitPRCache{entries: make(map[string][]CommitPREntry)}, eventSemaphore: make(chan struct{}, 10), } From 51a250b3ec9bab08fc520e350f31e1ca92253083 Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Thu, 30 Oct 2025 08:38:50 -0400 Subject: [PATCH 2/2] add turnclient as a commit->PR lookup fallback --- pkg/bot/commit_pr_cache_test.go | 506 ++++++++++++++++++++++++++++++++ 1 file changed, 506 insertions(+) create mode 100644 pkg/bot/commit_pr_cache_test.go diff --git a/pkg/bot/commit_pr_cache_test.go b/pkg/bot/commit_pr_cache_test.go new file mode 100644 index 0000000..3e38f3a --- /dev/null +++ b/pkg/bot/commit_pr_cache_test.go @@ -0,0 +1,506 @@ +package bot + +import ( + "context" + "testing" + "time" + + "github.com/codeGROOVE-dev/prx/pkg/prx" + "github.com/codeGROOVE-dev/slacker/pkg/state" + "github.com/codeGROOVE-dev/sprinkler/pkg/client" +) + +// TestCommitPRCache_RecordAndFind tests basic cache operations. +func TestCommitPRCache_RecordAndFind(t *testing.T) { + cache := &CommitPRCache{ + entries: make(map[string][]CommitPREntry), + } + + // Record a PR with a commit + cache.RecordPR("owner", "repo", 123, "abc123") + + // Should find it immediately + prs := cache.FindPRsForCommit("owner", "repo", "abc123") + if len(prs) != 1 || prs[0] != 123 { + t.Errorf("expected [123], got %v", prs) + } + + // Should not find different commit + prs = cache.FindPRsForCommit("owner", "repo", "def456") + if len(prs) != 0 { + t.Errorf("expected empty for unknown commit, got %v", prs) + } + + // Should not find in different repo + prs = cache.FindPRsForCommit("owner", "other-repo", "abc123") + if len(prs) != 0 { + t.Errorf("expected empty for different repo, got %v", prs) + } +} + +// TestCommitPRCache_MultiplePRsSameCommit tests handling of multiple PRs with same commit. +func TestCommitPRCache_MultiplePRsSameCommit(t *testing.T) { + cache := &CommitPRCache{ + entries: make(map[string][]CommitPREntry), + } + + // Same commit in two different PRs (e.g., backport) + cache.RecordPR("owner", "repo", 123, "abc123") + cache.RecordPR("owner", "repo", 456, "abc123") + + prs := cache.FindPRsForCommit("owner", "repo", "abc123") + if len(prs) != 2 { + t.Errorf("expected 2 PRs, got %v", prs) + } + + // Should contain both PR numbers + found123, found456 := false, false + for _, pr := range prs { + if pr == 123 { + found123 = true + } + if pr == 456 { + found456 = true + } + } + + if !found123 || !found456 { + t.Errorf("expected both PRs 123 and 456, got %v", prs) + } +} + +// TestCommitPRCache_UpdateExistingPR tests PR with multiple commits (e.g., force push adds new commit). +func TestCommitPRCache_UpdateExistingPR(t *testing.T) { + cache := &CommitPRCache{ + entries: make(map[string][]CommitPREntry), + } + + // Record PR with initial commit + cache.RecordPR("owner", "repo", 123, "abc123") + + // Add another commit to same PR (force push or additional commit) + cache.RecordPR("owner", "repo", 123, "def456") + + // Both commits should find the PR (cache stores multiple commits per PR) + prs := cache.FindPRsForCommit("owner", "repo", "abc123") + if len(prs) != 1 || prs[0] != 123 { + t.Errorf("expected [123] for first commit, got %v", prs) + } + + prs = cache.FindPRsForCommit("owner", "repo", "def456") + if len(prs) != 1 || prs[0] != 123 { + t.Errorf("expected [123] for second commit, got %v", prs) + } +} + +// TestCommitPRCache_Expiration tests that old entries are cleaned up. +func TestCommitPRCache_Expiration(t *testing.T) { + cache := &CommitPRCache{ + entries: make(map[string][]CommitPREntry), + } + + // Manually add an old entry (11 minutes ago) + cache.mu.Lock() + cache.entries["owner/repo"] = []CommitPREntry{ + { + PRNumber: 123, + HeadSHA: "old123", + UpdatedAt: time.Now().Add(-11 * time.Minute), + }, + } + cache.mu.Unlock() + + // Add a recent entry + cache.RecordPR("owner", "repo", 456, "new456") + + // Old entry should be gone + prs := cache.FindPRsForCommit("owner", "repo", "old123") + if len(prs) != 0 { + t.Errorf("expected old entry to be cleaned up, got %v", prs) + } + + // Recent entry should still be there + prs = cache.FindPRsForCommit("owner", "repo", "new456") + if len(prs) != 1 || prs[0] != 456 { + t.Errorf("expected [456] for recent entry, got %v", prs) + } +} + +// TestCommitPRCache_EmptyCommitSHA tests that empty commits are ignored. +func TestCommitPRCache_EmptyCommitSHA(t *testing.T) { + cache := &CommitPRCache{ + entries: make(map[string][]CommitPREntry), + } + + // Try to record with empty SHA + cache.RecordPR("owner", "repo", 123, "") + + // Should not be recorded + cache.mu.RLock() + entries := cache.entries["owner/repo"] + cache.mu.RUnlock() + + if len(entries) != 0 { + t.Errorf("expected no entries for empty SHA, got %d", len(entries)) + } +} + +// TestCheckEventIntegration_CacheHit tests the full flow when cache has the commit. +func TestCheckEventIntegration_CacheHit(t *testing.T) { + // Setup mock state store + mockStore := &mockStateStore{ + threads: make(map[string]state.ThreadInfo), + } + + // Create coordinator with real commit cache + coord := &Coordinator{ + stateStore: mockStore, + commitPRCache: &CommitPRCache{ + entries: make(map[string][]CommitPREntry), + }, + github: &mockGitHubClientForCache{ + // Mock should NOT be called if cache works + findPRsForCommitFunc: func(ctx context.Context, owner, repo, sha string) ([]int, error) { + t.Error("GitHub API should not be called when cache has the commit") + return nil, nil + }, + }, + } + + // Populate cache as if we just processed a PR event + coord.commitPRCache.RecordPR("testorg", "testrepo", 123, "abc123def456") + + // Simulate a check_run event arriving with just the commit SHA + event := client.Event{ + Type: "check_run", + URL: "https://github.com/testorg/testrepo", + Timestamp: time.Now(), + Raw: map[string]any{ + "commit_sha": "abc123def456", + "delivery_id": "test-delivery-123", + }, + } + + // Call lookupPRsForCheckEvent + prNumbers := coord.lookupPRsForCheckEvent(context.Background(), event, "testorg") + + // Should find PR 123 from cache + if len(prNumbers) != 1 { + t.Fatalf("expected 1 PR, got %d", len(prNumbers)) + } + if prNumbers[0] != 123 { + t.Errorf("expected PR 123, got %d", prNumbers[0]) + } +} + +// TestCheckEventIntegration_CacheMissFallback tests fallback to GitHub API on cache miss. +func TestCheckEventIntegration_CacheMissFallback(t *testing.T) { + // Setup mock state store + mockStore := &mockStateStore{ + threads: make(map[string]state.ThreadInfo), + } + + apiCalled := false + + // Create coordinator with empty cache + coord := &Coordinator{ + stateStore: mockStore, + commitPRCache: &CommitPRCache{ + entries: make(map[string][]CommitPREntry), + }, + github: &mockGitHubClientForCache{ + // Mock SHOULD be called on cache miss + findPRsForCommitFunc: func(ctx context.Context, owner, repo, sha string) ([]int, error) { + apiCalled = true + if sha == "unknown123" { + return []int{456}, nil // Return PR 456 + } + return nil, nil + }, + }, + } + + // Cache is empty - simulate check event arriving before PR event + + // Simulate a check_run event + event := client.Event{ + Type: "check_run", + URL: "https://github.com/testorg/testrepo", + Timestamp: time.Now(), + Raw: map[string]any{ + "commit_sha": "unknown123", + "delivery_id": "test-delivery-456", + }, + } + + // Call lookupPRsForCheckEvent + prNumbers := coord.lookupPRsForCheckEvent(context.Background(), event, "testorg") + + // Should have called GitHub API + if !apiCalled { + t.Error("expected GitHub API to be called on cache miss") + } + + // Should find PR 456 from API + if len(prNumbers) != 1 { + t.Fatalf("expected 1 PR from API, got %d", len(prNumbers)) + } + if prNumbers[0] != 456 { + t.Errorf("expected PR 456 from API, got %d", prNumbers[0]) + } +} + +// TestCachePopulationFromTurnclient tests that cache is populated when processing PRs. +func TestCachePopulationFromTurnclient(t *testing.T) { + // This test verifies that when we receive a PR event and fetch it via turnclient, + // we populate the commit→PR cache with all commits from that PR. + + // Create cache + cache := &CommitPRCache{ + entries: make(map[string][]CommitPREntry), + } + + // Simulate turnclient returning a PR with multiple commits + mockPR := prx.PullRequest{ + Number: 789, + Title: "Test PR", + Author: "testuser", + Commits: []string{"commit1", "commit2", "commit3"}, + } + + // Manually populate cache as the code does + for _, commitSHA := range mockPR.Commits { + if commitSHA != "" { + cache.RecordPR("owner", "repo", mockPR.Number, commitSHA) + } + } + + // Verify all commits are cached + for i, commitSHA := range mockPR.Commits { + prs := cache.FindPRsForCommit("owner", "repo", commitSHA) + if len(prs) != 1 || prs[0] != 789 { + t.Errorf("commit %d (%s): expected PR [789], got %v", i, commitSHA, prs) + } + } +} + +// TestMultipleReposIndependence tests that different repos don't interfere. +func TestMultipleReposIndependence(t *testing.T) { + cache := &CommitPRCache{ + entries: make(map[string][]CommitPREntry), + } + + // Same commit SHA in different repos + cache.RecordPR("owner1", "repo1", 111, "abc123") + cache.RecordPR("owner2", "repo2", 222, "abc123") + cache.RecordPR("owner1", "repo2", 333, "abc123") + + // Each repo should only see its own PR + prs := cache.FindPRsForCommit("owner1", "repo1", "abc123") + if len(prs) != 1 || prs[0] != 111 { + t.Errorf("owner1/repo1: expected [111], got %v", prs) + } + + prs = cache.FindPRsForCommit("owner2", "repo2", "abc123") + if len(prs) != 1 || prs[0] != 222 { + t.Errorf("owner2/repo2: expected [222], got %v", prs) + } + + prs = cache.FindPRsForCommit("owner1", "repo2", "abc123") + if len(prs) != 1 || prs[0] != 333 { + t.Errorf("owner1/repo2: expected [333], got %v", prs) + } +} + +// mockGitHubClientForCache is a minimal mock for commit cache tests. +type mockGitHubClientForCache struct { + findPRsForCommitFunc func(ctx context.Context, owner, repo, sha string) ([]int, error) +} + +func (m *mockGitHubClientForCache) FindPRsForCommit(ctx context.Context, owner, repo, sha string) ([]int, error) { + if m.findPRsForCommitFunc != nil { + return m.findPRsForCommitFunc(ctx, owner, repo, sha) + } + return nil, nil +} + +func (m *mockGitHubClientForCache) Organization() string { + return "testorg" +} + +func (m *mockGitHubClientForCache) Client() any { + return nil +} + +func (m *mockGitHubClientForCache) InstallationToken(ctx context.Context) string { + return "test-token" +} + +func (m *mockGitHubClientForCache) RefreshToken(ctx context.Context) error { + return nil +} + +// TestMostRecentPR tests the MostRecentPR method. +func TestMostRecentPR(t *testing.T) { + cache := &CommitPRCache{ + entries: make(map[string][]CommitPREntry), + } + + // Add multiple PRs with different timestamps + cache.RecordPR("owner", "repo", 100, "commit100") + time.Sleep(10 * time.Millisecond) + cache.RecordPR("owner", "repo", 200, "commit200") + time.Sleep(10 * time.Millisecond) + cache.RecordPR("owner", "repo", 300, "commit300") + + // Most recent should be PR 300 + mostRecent := cache.MostRecentPR("owner", "repo") + if mostRecent != 300 { + t.Errorf("expected most recent PR to be 300, got %d", mostRecent) + } + + // Different repo should return 0 + mostRecent = cache.MostRecentPR("owner", "other-repo") + if mostRecent != 0 { + t.Errorf("expected 0 for unknown repo, got %d", mostRecent) + } + + // Empty cache should return 0 + emptyCache := &CommitPRCache{ + entries: make(map[string][]CommitPREntry), + } + mostRecent = emptyCache.MostRecentPR("owner", "repo") + if mostRecent != 0 { + t.Errorf("expected 0 for empty cache, got %d", mostRecent) + } +} + +// TestMostRecentPR_WithMultipleCommitsPerPR tests that we track the most recent PR correctly +// even when PRs have multiple commits. +func TestMostRecentPR_WithMultipleCommitsPerPR(t *testing.T) { + cache := &CommitPRCache{ + entries: make(map[string][]CommitPREntry), + } + + // PR 100 with multiple commits + cache.RecordPR("owner", "repo", 100, "commit1") + time.Sleep(10 * time.Millisecond) + cache.RecordPR("owner", "repo", 100, "commit2") + time.Sleep(10 * time.Millisecond) + + // PR 200 with a commit added AFTER PR 100's last commit + cache.RecordPR("owner", "repo", 200, "commit3") + + // PR 200 has the most recent update (commit3 was added last) + mostRecent := cache.MostRecentPR("owner", "repo") + if mostRecent != 200 { + t.Errorf("expected most recent PR to be 200 (has newest commit timestamp), got %d", mostRecent) + } + + // Now add another commit to PR 100 after PR 200 + time.Sleep(10 * time.Millisecond) + cache.RecordPR("owner", "repo", 100, "commit4") + + // Now PR 100 should be most recent again + mostRecent = cache.MostRecentPR("owner", "repo") + if mostRecent != 100 { + t.Errorf("expected most recent PR to be 100 after adding commit4, got %d", mostRecent) + } +} + +// TestTurnclientFallback_CacheHasRecentPR tests the turnclient fallback when cache has a recent PR +// but doesn't have the specific commit we're looking for. +func TestTurnclientFallback_CacheHasRecentPR(t *testing.T) { + // This simulates the scenario where: + // 1. Cache miss for specific commit + // 2. Cache HAS a recently seen PR for the repo + // 3. We call turnclient on that PR and find the commit there + + // Note: This is a unit test that verifies the cache logic. + // The actual turnclient integration is tested in sprinkler_test.go + // where we have full Coordinator setup with mocks. + + cache := &CommitPRCache{ + entries: make(map[string][]CommitPREntry), + } + + // Populate cache with PR 123 that has commit "abc123" + cache.RecordPR("testorg", "testrepo", 123, "abc123") + + // Now imagine a check event arrives for commit "def456" which isn't cached yet + // But it belongs to the same PR 123 + + // Step 1: Cache lookup fails + prs := cache.FindPRsForCommit("testorg", "testrepo", "def456") + if len(prs) != 0 { + t.Errorf("cache should not have commit def456 yet, got %v", prs) + } + + // Step 2: We can get the most recent PR + mostRecent := cache.MostRecentPR("testorg", "testrepo") + if mostRecent != 123 { + t.Fatalf("expected most recent PR to be 123, got %d", mostRecent) + } + + // Step 3: Turnclient would tell us that PR 123 contains "def456" + // (in real code, this happens in lookupPRsForCheckEvent) + // After turnclient returns the commit list, we populate the cache: + cache.RecordPR("testorg", "testrepo", 123, "def456") + + // Step 4: Now cache lookup works + prs = cache.FindPRsForCommit("testorg", "testrepo", "def456") + if len(prs) != 1 || prs[0] != 123 { + t.Errorf("after turnclient lookup, cache should have commit def456 mapped to PR 123, got %v", prs) + } +} + +// TestTurnclientFallback_NoRecentPR tests that we fall back to GitHub API +// when the cache has no recent PRs for the repo. +func TestTurnclientFallback_NoRecentPR(t *testing.T) { + cache := &CommitPRCache{ + entries: make(map[string][]CommitPREntry), + } + + // Cache is completely empty for this repo + mostRecent := cache.MostRecentPR("testorg", "unknown-repo") + if mostRecent != 0 { + t.Errorf("expected 0 for repo with no cached PRs, got %d", mostRecent) + } + + // In this case, lookupPRsForCheckEvent would skip turnclient + // and go straight to GitHub API fallback +} + +// TestTurnclientFallback_WrongPR tests when the most recent PR doesn't contain the commit. +func TestTurnclientFallback_WrongPR(t *testing.T) { + cache := &CommitPRCache{ + entries: make(map[string][]CommitPREntry), + } + + // Cache has PR 100 with commits from a different PR + cache.RecordPR("testorg", "testrepo", 100, "commit1") + cache.RecordPR("testorg", "testrepo", 100, "commit2") + + // Most recent PR is 100 + mostRecent := cache.MostRecentPR("testorg", "testrepo") + if mostRecent != 100 { + t.Fatalf("expected most recent PR to be 100, got %d", mostRecent) + } + + // But we're looking for a commit from PR 200 + // Turnclient would check PR 100, not find "commit_from_pr_200" + // and we'd fall back to GitHub API which would find PR 200 + + // Simulate: turnclient checked PR 100, didn't find the commit + // (no cache update happens) + + // Simulate: GitHub API found it in PR 200 + // We'd populate the cache with the GitHub API result: + cache.RecordPR("testorg", "testrepo", 200, "commit_from_pr_200") + + // Now cache has both PRs + prs := cache.FindPRsForCommit("testorg", "testrepo", "commit_from_pr_200") + if len(prs) != 1 || prs[0] != 200 { + t.Errorf("expected to find PR 200, got %v", prs) + } +}