Improve assignment logic & code quality

Author: tstromberg

.gitignore

@@ -30,3 +30,6 @@ go.work.sum
 # Editor/IDE
 # .idea/
 # .vscode/
+
+# added by lint-install
+out/

better-reviewers

@@ -2,6 +2,7 @@ package main
 
 import (
 	"context"
+	"net/http"
 	"testing"
 )
 
@@ -21,7 +22,7 @@ func TestIsUserBot(t *testing.T) {
 		{"Bot with bot- prefix", "bot-user", true},
 		{"Bot with bot_ prefix", "bot_scanner", true},
 		{"Bot with .bot suffix", "scanner.bot", true},
-		
+
 		// Specific known bots
 		{"GitHub Actions", "github-actions", true},
 		{"GitHub Actions with bracket", "github-actions[bot]", true},
@@ -35,7 +36,7 @@ func TestIsUserBot(t *testing.T) {
 		{"Jenkins", "jenkins", true},
 		{"Mergify", "mergify[bot]", true},
 		{"Stale bot", "stale[bot]", true},
-		
+
 		// Organization/service patterns
 		{"Octo STS", "octo-sts", true},
 		{"Octocat", "octocat", true},
@@ -53,7 +54,7 @@ func TestIsUserBot(t *testing.T) {
 		{"Admin account", "cluster-admin", true},
 		{"Security account", "security-scanner", true},
 		{"Compliance account", "compliance-checker", true},
-		
+
 		// Valid human users
 		{"Regular user", "johndoe", false},
 		{"User with dash", "john-doe", false},
@@ -64,7 +65,7 @@ func TestIsUserBot(t *testing.T) {
 		{"PR author", "ajayk", false},
 		{"Reviewer", "tstromberg", false},
 		{"Another reviewer", "vavilen84", false},
-		
+
 		// Edge cases - users that might look like bots but aren't
 		{"User with 'bot' in name", "abbott", false},
 		{"User with 'test' in name", "atestuser", false},
@@ -87,23 +88,27 @@ func TestIsUserBot(t *testing.T) {
 func TestIsValidReviewer(t *testing.T) {
 	// Create a mock GitHub client that returns specific user types
 	mockClient := &GitHubClient{
-		userCache: newUserCache(),
+		httpClient: &http.Client{},
+		userCache:  &userCache{users: make(map[string]*userInfo)},
+		token:      "test-token",
 	}
-	
+
 	// Pre-populate the cache with test data
 	mockClient.userCache.users["octo-sts"] = &userInfo{login: "octo-sts", userType: userTypeOrg}
 	mockClient.userCache.users["github"] = &userInfo{login: "github", userType: userTypeOrg}
 	mockClient.userCache.users["dependabot[bot]"] = &userInfo{login: "dependabot[bot]", userType: userTypeBot}
+	mockClient.userCache.users["github-actions"] = &userInfo{login: "github-actions", userType: userTypeBot}
+	mockClient.userCache.users["deploy-service"] = &userInfo{login: "deploy-service", userType: userTypeBot}
 	mockClient.userCache.users["johndoe"] = &userInfo{login: "johndoe", userType: userTypeUser}
 	mockClient.userCache.users["sergiodj"] = &userInfo{login: "sergiodj", userType: userTypeUser}
-	
+
 	rf := &ReviewerFinder{
 		client: mockClient,
 	}
-	
+
 	ctx := context.Background()
 	pr := &PullRequest{Owner: "test", Repository: "repo"}
-	
+
 	tests := []struct {
 		name      string
 		username  string
@@ -115,12 +120,12 @@ func TestIsValidReviewer(t *testing.T) {
 		{"Bot with API confirmation", "dependabot[bot]", false},
 		{"Pattern-based bot", "github-actions", false},
 		{"Service account", "deploy-service", false},
-		
+
 		// Should be valid
 		{"Regular user", "johndoe", true},
 		{"Contributor", "sergiodj", true},
 	}
-	
+
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			got := rf.isValidReviewer(ctx, pr, tt.username)
@@ -159,7 +164,7 @@ func TestGetUserType(t *testing.T) {
 			wantUserType: userTypeUser,
 		},
 	}
-	
+
 	// These would be integration tests with a mock HTTP client
 	// For now, we're testing the core logic
 	for _, tt := range tests {
@@ -168,4 +173,4 @@ func TestGetUserType(t *testing.T) {
 			// Implementation would require mocking the HTTP client
 		})
 	}
-}
+}

bot_detection_test.go

@@ -7,53 +7,68 @@ import (
 
 // cacheEntry holds a cached value with expiration.
 type cacheEntry struct {
-	value      interface{}
+	value      any
 	expiration time.Time
 }
 
 // cache provides thread-safe caching with TTL.
 type cache struct {
-	mu      sync.RWMutex
 	entries map[string]cacheEntry
+	mu      sync.RWMutex
 	ttl     time.Duration
 }
 
-// newCache creates a new cache with the given TTL.
-func newCache(ttl time.Duration) *cache {
-	return &cache{
-		entries: make(map[string]cacheEntry),
-		ttl:     ttl,
-	}
-}
-
-// get retrieves a value from cache if not expired.
-func (c *cache) get(key string) (interface{}, bool) {
+// value retrieves a value from cache if not expired.
+func (c *cache) value(key string) (any, bool) {
 	c.mu.RLock()
-	defer c.mu.RUnlock()
-	
 	entry, exists := c.entries[key]
-	if !exists || time.Now().After(entry.expiration) {
+	c.mu.RUnlock()
+
+	if !exists {
+		return nil, false
+	}
+
+	// Check expiration without holding the lock
+	if time.Now().After(entry.expiration) {
+		// Remove expired entry
+		c.mu.Lock()
+		delete(c.entries, key)
+		c.mu.Unlock()
 		return nil, false
 	}
-	
+
 	return entry.value, true
 }
 
 // set stores a value in cache with TTL.
-func (c *cache) set(key string, value interface{}) {
+func (c *cache) set(key string, value any) {
+	c.setWithTTL(key, value, c.ttl)
+}
+
+// setWithTTL stores a value in cache with custom TTL.
+func (c *cache) setWithTTL(key string, value any, ttl time.Duration) {
 	c.mu.Lock()
 	defer c.mu.Unlock()
-	
+
 	c.entries[key] = cacheEntry{
 		value:      value,
-		expiration: time.Now().Add(c.ttl),
+		expiration: time.Now().Add(ttl),
 	}
 }
 
-// clear removes all entries from cache.
-func (c *cache) clear() {
-	c.mu.Lock()
-	defer c.mu.Unlock()
-	
-	c.entries = make(map[string]cacheEntry)
-}
+// cleanupExpired periodically removes expired entries.
+func (c *cache) cleanupExpired() {
+	ticker := time.NewTicker(5 * time.Minute)
+	defer ticker.Stop()
+
+	for range ticker.C {
+		c.mu.Lock()
+		now := time.Now()
+		for key, entry := range c.entries {
+			if now.After(entry.expiration) {
+				delete(c.entries, key)
+			}
+		}
+		c.mu.Unlock()
+	}
+}

cache.go

@@ -2,26 +2,75 @@ package main
 
 import "time"
 
-// Selection methods for reviewer choice tracking
+// Selection methods for reviewer choice tracking.
 const (
 	selectionAssignee          = "assignee-expert"
 	selectionAuthorOverlap     = "author-overlap"
-	selectionAuthorDirectory   = "author-directory" 
+	selectionAuthorDirectory   = "author-directory"
 	selectionAuthorProject     = "author-project"
 	selectionReviewerCommenter = "reviewer-commenter"
 	selectionReviewerOverlap   = "reviewer-overlap"
 	selectionReviewerDirectory = "reviewer-directory"
 	selectionReviewerProject   = "reviewer-project"
 )
 
-// Configuration constants
+// Configuration constants.
 const (
 	httpTimeout       = 120 // seconds
 	maxRetries        = 3
-	retryDelay        = 2 // seconds
-	nearbyLines       = 3 // lines within this distance count as "nearby"
-	maxFilesToAnalyze = 10
-	maxHistoricalPRs  = 50 // With caching, we can afford more lookups
-	maxRecentPRs      = 20 // Reasonable limit for recent PRs
-	cacheTTL          = 15 * time.Minute // Cache results for 15 minutes
-)
+	retryDelay        = 2                   // seconds
+	nearbyLines       = 3                   // lines within this distance count as "nearby"
+	maxFilesToAnalyze = 3                   // Focus on 3 files with largest delta to reduce API calls
+	maxHistoricalPRs  = 2                   // Limit to 2 PRs per file to reduce API calls
+	maxRecentPRs      = 50                  // Increased to get better candidate pool
+	cacheTTL          = 24 * time.Hour      // Default cache TTL for most items
+	prCacheTTL        = 20 * 24 * time.Hour // Cache PRs for 20 days (use updated_at to invalidate)
+	searchCacheTTL    = 15 * time.Minute    // Cache search results for 15 minutes
+
+	// Specific cache TTLs for different data types
+	userTypeCacheTTL         = 30 * 24 * time.Hour // User type never changes
+	repoContributorsCacheTTL = 4 * time.Hour       // 4 hours - catch people returning from vacation
+	directoryOwnersCacheTTL  = 3 * 24 * time.Hour  // Directory ownership changes slowly
+	recentPRsCacheTTL        = 1 * time.Hour       // Recent PRs for active repos
+	fileHistoryCacheTTL      = 3 * 24 * time.Hour  // File history changes slowly
+
+	// API and pagination limits.
+	perPageLimit = 100 // GitHub API per_page limit
+
+	// Analysis parameters.
+	topReviewersLimit = 10 // Number of top reviewers to find
+	progressBarWidth  = 40 // Width of progress bar in characters
+
+	// Overlap scoring parameters.
+	overlapDecayDays  = 30.0 // Days for recency weight decay
+	nearbyMatchWeight = 0.5  // Weight for nearby line matches
+
+	// Recency window parameters (in days).
+	recencyWindow1 = 4  // First window: 4 days
+	recencyWindow2 = 8  // Second window: 8 days
+	recencyWindow3 = 16 // Third window: 16 days
+	recencyWindow4 = 32 // Fourth window: 32 days
+	selectionRolls = 10 // Number of random rolls for weighted selection
+
+	// Scoring parameters.
+	topCandidatesToLog = 5   // Number of top candidates to log
+	maxContextScore    = 100 // Maximum context score for candidates
+
+	// Scoring weights (must sum to 100)
+	fileOverlapWeight = 40.0 // Weight for file overlap score
+	recencyWeight     = 35.0 // Weight for recency score
+	expertiseWeight   = 25.0 // Weight for domain expertise score
+
+	// File significance multipliers
+	prodCodeMultiplier     = 1.5 // Production code vs test code
+	criticalFileMultiplier = 1.3 // Main.go, handlers, etc.
+	refactoringMultiplier  = 1.2 // More deletions than additions
+
+	// Retry parameters handled by external library
+
+	// PR URL parsing.
+	minURLParts = 4 // Minimum parts in PR URL
+
+	// GraphQL constants.
+	graphQLNodes = "nodes" // Common GraphQL field name
+)