From 3ab626743ec9f0e8bc238abc2309f73acca1256d Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Tue, 28 Oct 2025 11:37:53 -0400 Subject: [PATCH] Add more logging to usermap failures --- .claude/CLAUDE.md | 1 + internal/bot/bot.go | 34 ++++++-- internal/notify/notify.go | 118 +++++++++++++++++++++++++--- internal/usermapping/usermapping.go | 85 +++++++++++++++++--- 4 files changed, 211 insertions(+), 27 deletions(-) diff --git a/.claude/CLAUDE.md b/.claude/CLAUDE.md index 95ced27..9846e59 100644 --- a/.claude/CLAUDE.md +++ b/.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 diff --git a/internal/bot/bot.go b/internal/bot/bot.go index cbe8320..db9d1ff 100644 --- a/internal/bot/bot.go +++ b/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) diff --git a/internal/notify/notify.go b/internal/notify/notify.go index f4c3a90..9daf679 100644 --- a/internal/notify/notify.go +++ b/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 · author → action var action string diff --git a/internal/usermapping/usermapping.go b/internal/usermapping/usermapping.go index bdb3605..40ef390 100644 --- a/internal/usermapping/usermapping.go +++ b/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,9 +221,11 @@ 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, @@ -218,18 +233,26 @@ func (s *Service) doLookup(ctx context.Context, githubUsername, organization, do 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 }