Improve bot detection

Author: tstromberg

better-reviewers

@@ -0,0 +1,171 @@
+package main
+
+import (
+	"context"
+	"testing"
+)
+
+func TestIsUserBot(t *testing.T) {
+	rf := &ReviewerFinder{}
+	ctx := context.Background()
+
+	tests := []struct {
+		name     string
+		username string
+		wantBot  bool
+	}{
+		// Bot patterns
+		{"Bot with [bot] suffix", "dependabot[bot]", true},
+		{"Bot with -bot suffix", "renovate-bot", true},
+		{"Bot with _bot suffix", "security_bot", true},
+		{"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},
+		{"Dependabot", "dependabot", true},
+		{"Renovate", "renovate", true},
+		{"Greenkeeper", "greenkeeper", true},
+		{"Snyk", "snyk-bot", true},
+		{"Codecov", "codecov", true},
+		{"Travis CI", "travis-ci", true},
+		{"CircleCI", "circleci", true},
+		{"Jenkins", "jenkins", true},
+		{"Mergify", "mergify[bot]", true},
+		{"Stale bot", "stale[bot]", true},
+		
+		// Organization/service patterns
+		{"Octo STS", "octo-sts", true},
+		{"Octocat", "octocat", true},
+		{"Service account with -sts", "my-app-sts", true},
+		{"Service account with -svc", "backend-svc", true},
+		{"Service account", "api-service", true},
+		{"System account", "auth-system", true},
+		{"Automation account", "deploy-automation", true},
+		{"CI account", "project-ci", true},
+		{"CD account", "prod-cd", true},
+		{"Deploy account", "k8s-deploy", true},
+		{"Release account", "release-manager", true},
+		{"Build account", "docker-build", true},
+		{"Test account", "e2e-test", true},
+		{"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},
+		{"User with underscore", "john_doe", false},
+		{"User with numbers", "user123", false},
+		{"Common contributor", "sergiodj", false},
+		{"Common contributor 2", "murraybd", false},
+		{"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},
+		{"User with 'build' in name", "builderman", false},
+		{"User with 'admin' in name", "adminton", false},
+		{"User ending in 'sts'", "roberts", false},
+		{"User ending in 'ci'", "luci", false},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := rf.isUserBot(ctx, tt.username)
+			if got != tt.wantBot {
+				t.Errorf("isUserBot(%q) = %v, want %v", tt.username, got, tt.wantBot)
+			}
+		})
+	}
+}
+
+func TestIsValidReviewer(t *testing.T) {
+	// Create a mock GitHub client that returns specific user types
+	mockClient := &GitHubClient{
+		userCache: newUserCache(),
+	}
+	
+	// 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["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
+		wantValid bool
+	}{
+		// Should be filtered out
+		{"Organization account", "octo-sts", false},
+		{"GitHub org", "github", false},
+		{"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)
+			if got != tt.wantValid {
+				t.Errorf("isValidReviewer(%q) = %v, want %v", tt.username, got, tt.wantValid)
+			}
+		})
+	}
+}
+
+func TestGetUserType(t *testing.T) {
+	// Test the userType detection logic
+	tests := []struct {
+		name         string
+		apiResponse  string
+		wantUserType userType
+	}{
+		{
+			name:         "Organization response",
+			apiResponse:  `{"type": "Organization", "name": "Test Org"}`,
+			wantUserType: userTypeOrg,
+		},
+		{
+			name:         "Bot response",
+			apiResponse:  `{"type": "Bot", "name": "Test Bot"}`,
+			wantUserType: userTypeBot,
+		},
+		{
+			name:         "User response",
+			apiResponse:  `{"type": "User", "name": "John Doe"}`,
+			wantUserType: userTypeUser,
+		},
+		{
+			name:         "Empty type defaults to user",
+			apiResponse:  `{"name": "Unknown"}`,
+			wantUserType: userTypeUser,
+		},
+	}
+	
+	// These would be integration tests with a mock HTTP client
+	// For now, we're testing the core logic
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			// This would test the actual API parsing logic
+			// Implementation would require mocking the HTTP client
+		})
+	}
+}

bot_detection_test.go

@@ -18,6 +18,7 @@ type GitHubClient struct {
 	token      string
 	httpClient *http.Client
 	cache      *cache
+	userCache  *userCache
 }
 
 // PullRequest represents a GitHub pull request.
@@ -81,6 +82,7 @@ func newGitHubClient() (*GitHubClient, error) {
 		token:      token,
 		httpClient: &http.Client{Timeout: time.Duration(httpTimeout) * time.Second},
 		cache:      newCache(cacheTTL),
+		userCache:  newUserCache(),
 	}, nil
 }
 
@@ -377,7 +379,80 @@ func (rf *ReviewerFinder) getRecentPRCommenters(ctx context.Context, owner, repo
 
 // isUserBot checks if a user is a bot.
 func (rf *ReviewerFinder) isUserBot(ctx context.Context, username string) bool {
-	return strings.HasSuffix(username, "[bot]") || strings.HasSuffix(username, "-bot")
+	lower := strings.ToLower(username)
+	
+	// Check for common bot patterns
+	botPatterns := []string{
+		"[bot]",
+		"-bot",
+		"_bot",
+		"bot-",
+		"bot_",
+		".bot",
+		"github-actions",
+		"dependabot",
+		"renovate",
+		"greenkeeper",
+		"snyk",
+		"codecov",
+		"coveralls",
+		"travis",
+		"circleci",
+		"jenkins",
+		"buildkite",
+		"semaphore",
+		"appveyor",
+		"azure-pipelines",
+		"github-classroom",
+		"imgbot",
+		"allcontributors",
+		"whitesource",
+		"mergify",
+		"sonarcloud",
+		"deepsource",
+		"codefactor",
+		"lgtm",
+		"codacy",
+		"hound",
+		"stale",
+	}
+	
+	for _, pattern := range botPatterns {
+		if strings.Contains(lower, pattern) {
+			return true
+		}
+	}
+	
+	// Check for common organization/service account patterns
+	orgPatterns := []string{
+		"octo-sts",
+		"octocat",
+		"-sts",
+		"-svc",
+		"-service",
+		"-system",
+		"-automation",
+		"-ci",
+		"-cd",
+		"-deploy",
+		"-release",
+		"release-manager",
+		"-build",
+		"-test",
+		"-admin",
+		"-security",
+		"security-scanner",
+		"-compliance",
+		"compliance-checker",
+	}
+	
+	for _, pattern := range orgPatterns {
+		if strings.Contains(lower, pattern) {
+			return true
+		}
+	}
+	
+	return false
 }
 
 // hasWriteAccess checks if a user has write access to the repository.

github_client.go

@@ -219,7 +219,25 @@ func (rf *ReviewerFinder) findAssigneeExpert(ctx context.Context, pr *PullReques
 
 // isValidReviewer checks if a user is a valid reviewer.
 func (rf *ReviewerFinder) isValidReviewer(ctx context.Context, pr *PullRequest, username string) bool {
-	return !rf.isUserBot(ctx, username) && rf.hasWriteAccess(ctx, pr.Owner, pr.Repository, username)
+	// First check pattern-based bot detection
+	if rf.isUserBot(ctx, username) {
+		if rf.output != nil && rf.output.verbose {
+			fmt.Printf("    Filtered (bot pattern): %s\n", username)
+		}
+		return false
+	}
+	
+	// Then check GitHub API for user type
+	uType, err := rf.client.getUserType(ctx, username)
+	if err == nil && (uType == userTypeOrg || uType == userTypeBot) {
+		if rf.output != nil && rf.output.verbose {
+			fmt.Printf("    Filtered (%s): %s\n", uType, username)
+		}
+		return false
+	}
+	
+	// Finally check write access
+	return rf.hasWriteAccess(ctx, pr.Owner, pr.Repository, username)
 }
 
 // findExpertReviewer finds the most active reviewer for the changes.

reviewer.go

@@ -0,0 +1,98 @@
+package main
+
+import (
+	"context"
+	"encoding/json"
+	"fmt"
+	"sync"
+)
+
+// userType represents the type of GitHub account.
+type userType string
+
+const (
+	userTypeUser userType = "User"
+	userTypeOrg  userType = "Organization"
+	userTypeBot  userType = "Bot"
+)
+
+// userInfo caches information about a GitHub user.
+type userInfo struct {
+	login    string
+	userType userType
+}
+
+// userCache caches user type information to avoid repeated API calls.
+type userCache struct {
+	mu    sync.RWMutex
+	users map[string]*userInfo
+}
+
+// newUserCache creates a new user cache.
+func newUserCache() *userCache {
+	return &userCache{
+		users: make(map[string]*userInfo),
+	}
+}
+
+// getUserType gets the type of a GitHub account (User, Organization, or Bot).
+func (c *GitHubClient) getUserType(ctx context.Context, username string) (userType, error) {
+	// Check cache first
+	if c.userCache != nil {
+		c.userCache.mu.RLock()
+		if info, exists := c.userCache.users[username]; exists {
+			c.userCache.mu.RUnlock()
+			return info.userType, nil
+		}
+		c.userCache.mu.RUnlock()
+	}
+
+	// Make API call to get user info
+	url := fmt.Sprintf("https://api.github.com/users/%s", username)
+	resp, err := c.makeRequest(ctx, "GET", url, nil)
+	if err != nil {
+		return userTypeUser, err // Default to user on error
+	}
+	defer resp.Body.Close()
+
+	if resp.StatusCode == 404 {
+		// User doesn't exist
+		return userTypeBot, nil
+	}
+
+	if resp.StatusCode != 200 {
+		return userTypeUser, fmt.Errorf("failed to get user info (status %d)", resp.StatusCode)
+	}
+
+	var data struct {
+		Type string `json:"type"`
+		Name string `json:"name"`
+	}
+
+	if err := json.NewDecoder(resp.Body).Decode(&data); err != nil {
+		return userTypeUser, err
+	}
+
+	// Determine user type
+	var uType userType
+	switch data.Type {
+	case "Organization":
+		uType = userTypeOrg
+	case "Bot":
+		uType = userTypeBot
+	default:
+		uType = userTypeUser
+	}
+
+	// Cache the result
+	if c.userCache != nil {
+		c.userCache.mu.Lock()
+		c.userCache.users[username] = &userInfo{
+			login:    username,
+			userType: uType,
+		}
+		c.userCache.mu.Unlock()
+	}
+
+	return uType, nil
+}