Merge pull request #59 from codeGROOVE-dev/reliable

Add more logging to usermap failures

Author: tstromberg

.claude/CLAUDE.md

@@ -139,6 +139,7 @@ slacker/
 - `reactions:write` - Add and edit emoji reactions
 - `team:read` - **View workspace name and domain (required for workspace validation)**
 - `users:read` - View people in the workspace
+- `users:read.email` - **Access user email addresses (required for GitHub→Slack user mapping)**
 
 ### State Management
 - Track PR states and transitions for notification logic

internal/bot/bot.go

@@ -654,7 +654,7 @@ func (c *Coordinator) handlePullRequestEventWithData(ctx context.Context, owner,
 		dmCtx, dmCancel := context.WithTimeout(context.WithoutCancel(ctx), 15*time.Second)
 		go func() {
 			defer dmCancel()
-			c.sendDMNotifications(dmCtx, workspaceID, owner, repo, prNumber, uniqueUsers, event, prState)
+			c.sendDMNotifications(dmCtx, workspaceID, owner, repo, prNumber, uniqueUsers, event, prState, checkResult)
 		}()
 	} else {
 		slog.Info("no users blocking PR - no notifications needed",
@@ -682,6 +682,7 @@ func (c *Coordinator) sendDMNotifications(
 		Number int `json:"number"`
 	},
 	prState string,
+	checkResult *turn.CheckResponse,
 ) {
 	slog.Info("starting DM notification batch",
 		logFieldPR, fmt.Sprintf(prFormatString, owner, repo, prNumber),
@@ -726,6 +727,11 @@ func (c *Coordinator) sendDMNotifications(
 			State:   prState,
 			HTMLURL: event.PullRequest.HTMLURL,
 		}
+		// Add workflow state and next actions if available
+		if checkResult != nil {
+			prInfo.WorkflowState = checkResult.Analysis.WorkflowState
+			prInfo.NextAction = checkResult.Analysis.NextAction
+		}
 
 		err = c.notifier.NotifyUser(ctx, workspaceID, slackUserID, tagInfo.ChannelID, channelName, prInfo)
 		if err != nil {
@@ -895,7 +901,13 @@ func (c *Coordinator) updateDMMessagesForPR(ctx context.Context, pr prUpdateInfo
 	}
 
 	// Format the DM message (same format as initial send)
-	prefix := notify.PrefixForState(prState)
+	// Determine prefix based on workflow state and next actions
+	var prefix string
+	if checkResult != nil {
+		prefix = notify.PrefixForAnalysis(checkResult.Analysis.WorkflowState, checkResult.Analysis.NextAction)
+	} else {
+		prefix = notify.PrefixForState(prState)
+	}
 	var action string
 	switch prState {
 	case "newly_published":
@@ -1232,7 +1244,13 @@ func (c *Coordinator) processPRForChannel(
 	// Build what the message SHOULD be based on current PR state
 	// Then compare to what it IS - update if different
 	if !wasNewlyCreated {
-		expectedPrefix := notify.PrefixForState(prState)
+		// Determine expected prefix based on workflow state and next actions
+		var expectedPrefix string
+		if checkResult != nil {
+			expectedPrefix = notify.PrefixForAnalysis(checkResult.Analysis.WorkflowState, checkResult.Analysis.NextAction)
+		} else {
+			expectedPrefix = notify.PrefixForState(prState)
+		}
 		domain := c.configManager.Domain(owner)
 		urlWithState := event.PullRequest.HTMLURL + c.getStateQueryParam(prState)
 
@@ -1469,8 +1487,14 @@ func (c *Coordinator) createPRThread(ctx context.Context, channel, owner, repo s
 	Number  int    `json:"number"`
 }, checkResult *turn.CheckResponse,
 ) (threadTS string, messageText string, err error) {
-	// Get state-based prefix
-	prefix := notify.PrefixForState(prState)
+	// Get emoji prefix based on workflow state and next actions
+	var prefix string
+	if checkResult != nil {
+		prefix = notify.PrefixForAnalysis(checkResult.Analysis.WorkflowState, checkResult.Analysis.NextAction)
+	} else {
+		// Fallback to state-based prefix if no checkResult available
+		prefix = notify.PrefixForState(prState)
+	}
 
 	// Add state query param to URL for debugging
 	urlWithState := pr.HTMLURL + c.getStateQueryParam(prState)

internal/notify/notify.go

@@ -8,6 +8,7 @@ import (
 	"time"
 
 	"github.com/codeGROOVE-dev/slacker/internal/slack"
+	"github.com/codeGROOVE-dev/turnclient/pkg/turn"
 )
 
 // Constants for notification defaults.
@@ -77,17 +78,21 @@ func (m *Manager) Run(ctx context.Context) error {
 
 // PRInfo contains the minimal information needed to notify about a PR.
 type PRInfo struct {
-	Owner   string
-	Repo    string
-	Title   string
-	Author  string
-	State   string
-	HTMLURL string
-	Number  int
+	Owner         string
+	Repo          string
+	Title         string
+	Author        string
+	State         string // Deprecated: use WorkflowState and NextAction for emoji determination
+	HTMLURL       string
+	Number        int
+	WorkflowState string                 // Workflow state from turnclient Analysis
+	NextAction    map[string]turn.Action // Next actions from turnclient Analysis
 }
 
 // PrefixForState returns the emoji prefix for a given PR state.
 // Exported for use by bot package to ensure consistent PR state display.
+//
+// Deprecated: Use PrefixForAnalysis instead to derive emoji from NextAction.
 func PrefixForState(prState string) string {
 	switch prState {
 	case "newly_published":
@@ -111,6 +116,95 @@ func PrefixForState(prState string) string {
 	}
 }
 
+// PrefixForAction returns the emoji prefix for a given action kind.
+func PrefixForAction(action string) string {
+	switch action {
+	case "publish_draft":
+		return ":construction:"
+	case "fix_tests":
+		return ":cockroach:"
+	case "tests_pending":
+		return ":test_tube:"
+	case "review", "re_review", "request_reviewers":
+		return ":hourglass:"
+	case "resolve_comments", "respond", "review_discussion":
+		return ":carpentry_saw:"
+	case "approve":
+		return ":white_check_mark:"
+	case "merge":
+		return ":rocket:"
+	default:
+		return ":postal_horn:"
+	}
+}
+
+// actionPriority returns a priority score for action kinds (lower = higher priority).
+// Used to determine which emoji to show when multiple actions are pending.
+func actionPriority(action string) int {
+	switch action {
+	case "publish_draft":
+		return 1
+	case "fix_tests":
+		return 2
+	case "tests_pending":
+		return 3
+	case "fix_conflict":
+		return 4
+	case "resolve_comments", "respond", "review_discussion":
+		return 5
+	case "review", "re_review", "request_reviewers":
+		return 6
+	case "approve":
+		return 7
+	case "merge":
+		return 8
+	default:
+		return 99
+	}
+}
+
+// PrimaryAction determines the primary action kind from a NextAction map.
+// Returns the highest-priority action (lowest priority score).
+func PrimaryAction(nextActions map[string]turn.Action) string {
+	if len(nextActions) == 0 {
+		return ""
+	}
+
+	var primaryAction string
+	minPriority := 999
+
+	for _, action := range nextActions {
+		kind := string(action.Kind)
+		priority := actionPriority(kind)
+		if priority < minPriority {
+			minPriority = priority
+			primaryAction = kind
+		}
+	}
+
+	return primaryAction
+}
+
+// PrefixForAnalysis returns the emoji prefix based on workflow state and next actions.
+// This is the primary function for determining PR emoji - it handles the logic:
+// 1. If workflow_state == "newly_published" → ":new:"
+// 2. Otherwise → emoji based on primary next_action
+func PrefixForAnalysis(workflowState string, nextActions map[string]turn.Action) string {
+	// Special case: newly published PRs always show :new:
+	if workflowState == "newly_published" {
+		return ":new:"
+	}
+
+	// Determine primary action and return corresponding emoji
+	primaryAction := PrimaryAction(nextActions)
+	if primaryAction != "" {
+		return PrefixForAction(primaryAction)
+	}
+
+	// Fallback if no actions
+	return ":postal_horn:"
+}
+
 // NotifyUser sends a smart notification to a user about a PR using the configured logic.
 // Implements delayed DM logic: if user was tagged in channel, delay by configured time.
 // If user is not in channel where tagged, send DM immediately.
@@ -245,8 +339,14 @@ func (m *Manager) NotifyUser(ctx context.Context, workspaceID, userID, channelID
 	}
 
 	// Format notification message using same style as channel messages
-	// Use state-based emoji prefix like channel messages do
-	prefix := PrefixForState(pr.State)
+	// Determine emoji prefix based on workflow state and next actions
+	var prefix string
+	if pr.WorkflowState != "" {
+		prefix = PrefixForAnalysis(pr.WorkflowState, pr.NextAction)
+	} else {
+		// Fallback to state if workflow state not available
+		prefix = PrefixForState(pr.State)
+	}
 
 	// Format: :emoji: Title <url|repo#123> · author → action
 	var action string

internal/usermapping/usermapping.go

@@ -138,10 +138,16 @@ func (s *Service) SlackHandle(ctx context.Context, githubUsername, organization,
 // Extracted from SlackHandle to work with singleflight pattern.
 func (s *Service) doLookup(ctx context.Context, githubUsername, organization, domain string) (string, error) {
 	// Get emails for GitHub user with organization context
+	slog.Info("starting email lookup for GitHub user",
+		"github_user", githubUsername,
+		"organization", organization,
+		"domain", domain)
+
 	result, err := s.githubLookup.Lookup(ctx, githubUsername, organization)
 	if err != nil {
 		slog.Warn("failed to get emails for GitHub user",
 			"github_user", githubUsername,
+			"organization", organization,
 			"error", err)
 		return "", err
 	}
@@ -154,10 +160,17 @@ func (s *Service) doLookup(ctx context.Context, githubUsername, organization, do
 			emails[i] = addr.Email
 		}
 
-		slog.Debug("found emails for GitHub user",
+		slog.Info("found emails for GitHub user via lookup",
 			"github_user", githubUsername,
 			"email_count", len(emails),
-			"emails", emails)
+			"emails", emails,
+			"methods", func() []string {
+				methods := make([]string, len(result.Addresses))
+				for i, addr := range result.Addresses {
+					methods[i] = addr.Methods[0] // Show first method
+				}
+				return methods
+			}())
 
 		// Try finding Slack matches with all emails first
 		matches := s.findSlackMatches(ctx, githubUsername, emails)
@@ -208,28 +221,38 @@ func (s *Service) doLookup(ctx context.Context, githubUsername, organization, do
 			}
 		}
 		// Finally, try intelligent guessing
-		slog.Debug("trying intelligent email guessing",
+		slog.Info("starting intelligent email guessing",
 			"github_user", githubUsername,
-			"domain", domain)
+			"organization", organization,
+			"domain", domain,
+			"found_addresses_count", len(result.Addresses))
 
 		guessResult, err := s.githubLookup.Guess(ctx, githubUsername, organization, ghmailto.GuessOptions{
 			Domain: domain,
 		})
 		if err != nil {
 			slog.Warn("email guessing failed",
 				"github_user", githubUsername,
+				"organization", organization,
 				"domain", domain,
 				"error", err)
 		} else if len(guessResult.Guesses) > 0 {
 			guessedEmails := make([]string, len(guessResult.Guesses))
+			confidences := make([]int, len(guessResult.Guesses))
+			patterns := make([]string, len(guessResult.Guesses))
 			for i, addr := range guessResult.Guesses {
 				guessedEmails[i] = addr.Email
+				confidences[i] = addr.Confidence
+				patterns[i] = addr.Pattern
 			}
 
-			slog.Debug("generated email guesses",
+			slog.Info("generated email guesses for Slack lookup",
 				"github_user", githubUsername,
 				"domain", domain,
-				"guesses", guessedEmails)
+				"guess_count", len(guessedEmails),
+				"guesses", guessedEmails,
+				"confidences", confidences,
+				"patterns", patterns)
 
 			matches := s.findSlackMatches(ctx, githubUsername, guessedEmails)
 			if len(matches) > 0 {
@@ -243,13 +266,25 @@ func (s *Service) doLookup(ctx context.Context, githubUsername, organization, do
 				s.cacheMapping(bestMatch)
 				return bestMatch.SlackUserID, nil
 			}
+			slog.Warn("email guesses generated but no Slack matches found",
+				"github_user", githubUsername,
+				"domain", domain,
+				"guesses_tried", len(guessedEmails),
+				"guesses", guessedEmails)
+		} else {
+			slog.Warn("email guessing completed but no guesses generated",
+				"github_user", githubUsername,
+				"domain", domain)
 		}
 	}
 
 	// No matches found through any method
-	slog.Info("no Slack mapping found for GitHub user",
+	slog.Warn("no Slack mapping found for GitHub user after exhausting all methods",
 		"github_user", githubUsername,
+		"organization", organization,
+		"domain", domain,
 		"tried_direct_emails", len(emails) > 0,
+		"direct_emails_tried", emails,
 		"tried_domain_filtering", domain != "",
 		"tried_guessing", domain != "")
 
@@ -387,15 +422,31 @@ func (s *Service) cacheMapping(mapping *UserMapping) {
 func (s *Service) findSlackMatches(ctx context.Context, githubUsername string, emails []string) []*UserMapping {
 	var matches []*UserMapping
 
+	slog.Info("starting Slack user lookup phase",
+		"github_user", githubUsername,
+		"email_count", len(emails),
+		"emails", emails)
+
 	// Search for each email in Slack user directory
-	for _, email := range emails {
+	for i, email := range emails {
 		// Normalize email to lowercase for consistent matching
 		normalizedEmail := strings.ToLower(email)
+		slog.Info("attempting Slack API lookup",
+			"github_user", githubUsername,
+			"attempt", i+1,
+			"of", len(emails),
+			"original_email", email,
+			"normalized_email", normalizedEmail)
+
 		user, err := s.slackClient.GetUserByEmailContext(ctx, normalizedEmail)
 		if err != nil {
-			slog.Debug("no Slack user found for email",
+			// Log detailed error info to help debug Slack API issues
+			slog.Warn("Slack API lookup failed for email",
+				"github_user", githubUsername,
 				"email", email,
-				"error", err)
+				"normalized_email", normalizedEmail,
+				"error_type", fmt.Sprintf("%T", err),
+				"error_message", err.Error())
 			continue
 		}
 
@@ -420,12 +471,20 @@ func (s *Service) findSlackMatches(ctx context.Context, githubUsername string, e
 
 		matches = append(matches, mapping)
 
-		slog.Debug("found Slack user match",
+		slog.Info("found Slack user match",
+			"github_user", githubUsername,
 			"email", email,
-			"slack_user", user.Name,
-			"confidence", confidence)
+			"slack_user_id", user.ID,
+			"slack_user_name", user.Name,
+			"confidence", confidence,
+			"is_primary_email", user.Profile.Email == email)
 	}
 
+	slog.Info("Slack user lookup phase complete",
+		"github_user", githubUsername,
+		"emails_tried", len(emails),
+		"matches_found", len(matches))
+
 	return matches
 }