diff --git a/pkg/bot/integration_test.go b/pkg/bot/integration_test.go index c7bd2d3..f9e40fc 100644 --- a/pkg/bot/integration_test.go +++ b/pkg/bot/integration_test.go @@ -107,7 +107,7 @@ func TestTurnclientEmojiIntegration(t *testing.T) { }{ { name: "newly published takes precedence", - workflowState: "newly_published", + workflowState: string(turn.StateNewlyPublished), nextActions: map[string]turn.Action{ "author": {Kind: turn.ActionFixTests}, }, @@ -196,7 +196,7 @@ func TestTurnclientEmojiIntegration(t *testing.T) { // Verify consistency: if there's a primary action, its emoji should match // (except for newly_published and merged edge cases) - if tt.workflowState != "newly_published" && tt.workflowState != "merged" && tt.expectedAction != "" { + if tt.workflowState != string(turn.StateNewlyPublished) && tt.workflowState != "merged" && tt.expectedAction != "" { actionEmoji := notify.PrefixForAction(tt.expectedAction) if emoji != actionEmoji { t.Errorf("%s: emoji mismatch - PrefixForAnalysis=%q but PrefixForAction(%q)=%q", diff --git a/pkg/notify/format_test.go b/pkg/notify/format_test.go index 7bbfb6d..8dcb064 100644 --- a/pkg/notify/format_test.go +++ b/pkg/notify/format_test.go @@ -382,7 +382,7 @@ func TestFormatChannelMessageBase(t *testing.T) { CheckResult: &turn.CheckResponse{ PullRequest: prx.PullRequest{}, Analysis: turn.Analysis{ - WorkflowState: "newly_published", + WorkflowState: string(turn.StateNewlyPublished), }, }, Owner: "test-owner", @@ -395,24 +395,154 @@ func TestFormatChannelMessageBase(t *testing.T) { contains: []string{":new:", "New PR", "test-repo#44", "testuser", "?st=newly_published"}, }, { - name: "with next actions", + name: "IN_DRAFT", params: MessageParams{ CheckResult: &turn.CheckResponse{ PullRequest: prx.PullRequest{}, Analysis: turn.Analysis{ - NextAction: map[string]turn.Action{ - "user1": {Kind: turn.ActionReview}, - }, + WorkflowState: string(turn.StateInDraft), }, }, Owner: "test-owner", Repo: "test-repo", PRNumber: 45, - Title: "PR needs review", + Title: "Draft PR", Author: "testuser", HTMLURL: "https://github.com/test-owner/test-repo/pull/45", }, - contains: []string{":hourglass:", "PR needs review", "test-repo#45", "testuser"}, + contains: []string{":construction:", "Draft PR", "test-repo#45", "testuser", "?st=draft"}, + }, + { + name: "PUBLISHED_WAITING_FOR_TESTS with broken tests", + params: MessageParams{ + CheckResult: &turn.CheckResponse{ + PullRequest: prx.PullRequest{}, + Analysis: turn.Analysis{ + WorkflowState: string(turn.StatePublishedWaitingForTests), + NextAction: map[string]turn.Action{ + "author": {Kind: turn.ActionFixTests}, + }, + }, + }, + Owner: "test-owner", + Repo: "test-repo", + PRNumber: 46, + Title: "PR with broken tests", + Author: "testuser", + HTMLURL: "https://github.com/test-owner/test-repo/pull/46", + }, + contains: []string{":cockroach:", "PR with broken tests", "test-repo#46", "testuser", "?st=tests_broken"}, + }, + { + name: "PUBLISHED_WAITING_FOR_TESTS with pending tests", + params: MessageParams{ + CheckResult: &turn.CheckResponse{ + PullRequest: prx.PullRequest{}, + Analysis: turn.Analysis{ + WorkflowState: string(turn.StatePublishedWaitingForTests), + NextAction: map[string]turn.Action{ + "author": {Kind: turn.ActionTestsPending}, + }, + }, + }, + Owner: "test-owner", + Repo: "test-repo", + PRNumber: 47, + Title: "PR with tests running", + Author: "testuser", + HTMLURL: "https://github.com/test-owner/test-repo/pull/47", + }, + contains: []string{":test_tube:", "PR with tests running", "test-repo#47", "testuser", "?st=tests_running"}, + }, + { + name: "TESTED_WAITING_FOR_ASSIGNMENT", + params: MessageParams{ + CheckResult: &turn.CheckResponse{ + PullRequest: prx.PullRequest{}, + Analysis: turn.Analysis{ + WorkflowState: string(turn.StateTestedWaitingForAssignment), + }, + }, + Owner: "test-owner", + Repo: "test-repo", + PRNumber: 48, + Title: "PR needs reviewer assignment", + Author: "testuser", + HTMLURL: "https://github.com/test-owner/test-repo/pull/48", + }, + contains: []string{":shrug:", "PR needs reviewer assignment", "test-repo#48", "testuser", "?st=awaiting_assignment"}, + }, + { + name: "ASSIGNED_WAITING_FOR_REVIEW", + params: MessageParams{ + CheckResult: &turn.CheckResponse{ + PullRequest: prx.PullRequest{}, + Analysis: turn.Analysis{ + WorkflowState: string(turn.StateAssignedWaitingForReview), + }, + }, + Owner: "test-owner", + Repo: "test-repo", + PRNumber: 49, + Title: "PR awaiting review", + Author: "testuser", + HTMLURL: "https://github.com/test-owner/test-repo/pull/49", + }, + contains: []string{":hourglass:", "PR awaiting review", "test-repo#49", "testuser", "?st=awaiting_review"}, + }, + { + name: "REVIEWED_NEEDS_REFINEMENT", + params: MessageParams{ + CheckResult: &turn.CheckResponse{ + PullRequest: prx.PullRequest{}, + Analysis: turn.Analysis{ + WorkflowState: string(turn.StateReviewedNeedsRefinement), + }, + }, + Owner: "test-owner", + Repo: "test-repo", + PRNumber: 50, + Title: "PR needs changes", + Author: "testuser", + HTMLURL: "https://github.com/test-owner/test-repo/pull/50", + }, + contains: []string{":carpentry_saw:", "PR needs changes", "test-repo#50", "testuser", "?st=changes_requested"}, + }, + { + name: "REFINED_WAITING_FOR_APPROVAL", + params: MessageParams{ + CheckResult: &turn.CheckResponse{ + PullRequest: prx.PullRequest{}, + Analysis: turn.Analysis{ + WorkflowState: string(turn.StateRefinedWaitingForApproval), + }, + }, + Owner: "test-owner", + Repo: "test-repo", + PRNumber: 51, + Title: "PR awaiting approval", + Author: "testuser", + HTMLURL: "https://github.com/test-owner/test-repo/pull/51", + }, + contains: []string{":hourglass:", "PR awaiting approval", "test-repo#51", "testuser", "?st=awaiting_approval"}, + }, + { + name: "APPROVED_WAITING_FOR_MERGE", + params: MessageParams{ + CheckResult: &turn.CheckResponse{ + PullRequest: prx.PullRequest{}, + Analysis: turn.Analysis{ + WorkflowState: string(turn.StateApprovedWaitingForMerge), + }, + }, + Owner: "test-owner", + Repo: "test-repo", + PRNumber: 52, + Title: "PR approved and ready", + Author: "testuser", + HTMLURL: "https://github.com/test-owner/test-repo/pull/52", + }, + contains: []string{":white_check_mark:", "PR approved and ready", "test-repo#52", "testuser", "?st=approved"}, }, } diff --git a/pkg/notify/notify.go b/pkg/notify/notify.go index f7794b7..552ac96 100644 --- a/pkg/notify/notify.go +++ b/pkg/notify/notify.go @@ -76,15 +76,18 @@ func FormatChannelMessageBase(ctx context.Context, params MessageParams) string } else if pr.State == "closed" { emoji, state = ":x:", "?st=closed" slog.Info("using :x: emoji - PR is closed but not merged", "pr", prID) - } else if a.WorkflowState == "newly_published" { - emoji, state = ":new:", "?st=newly_published" - slog.Info("using :new: emoji - newly published PR", "pr", prID, "workflow_state", a.WorkflowState) + } else if a.WorkflowState != "" { + // Use WorkflowState as primary signal (most reliable source of truth) + emoji, state = emojiFromWorkflowState(a.WorkflowState, a.NextAction) + slog.Info("using emoji from workflow_state", "pr", prID, "workflow_state", a.WorkflowState, "emoji", emoji, "state_param", state) } else if len(a.NextAction) > 0 { + // Fallback to NextAction if no WorkflowState (shouldn't normally happen) action := PrimaryAction(a.NextAction) emoji = PrefixForAction(action) state = stateParam(params.CheckResult) - slog.Info("using emoji from primary next_action", "pr", prID, "primary_action", action, "emoji", emoji, "state_param", state) + slog.Info("using emoji from primary next_action (no workflow_state)", "pr", prID, "primary_action", action, "emoji", emoji, "state_param", state) } else { + // Final fallback based on PR properties emoji, state = fallbackEmoji(params.CheckResult) //nolint:revive // line length acceptable for structured logging slog.Info("using fallback emoji - no workflow_state or next_actions", "pr", prID, "emoji", emoji, "state_param", state, "fallback_reason", "empty_workflow_state_and_next_actions") @@ -114,6 +117,62 @@ func FormatNextActionsSuffix(ctx context.Context, params MessageParams) string { return "" } +// emojiFromWorkflowState determines emoji based on WorkflowState as the primary signal. +// Uses NextAction for additional granularity in specific states (e.g., test failures). +func emojiFromWorkflowState(workflowState string, nextActions map[string]turn.Action) (emoji, state string) { + switch workflowState { + case string(turn.StateNewlyPublished): + return ":new:", "?st=newly_published" + + case string(turn.StateInDraft): + return ":construction:", "?st=draft" + + case string(turn.StatePublishedWaitingForTests): + // Use NextAction to distinguish between broken tests vs pending tests + if len(nextActions) > 0 { + action := PrimaryAction(nextActions) + if action == string(turn.ActionFixTests) { + return ":cockroach:", "?st=tests_broken" + } + if action == string(turn.ActionTestsPending) { + return ":test_tube:", "?st=tests_running" + } + } + // Default to tests running + return ":test_tube:", "?st=tests_running" + + case string(turn.StateTestedWaitingForAssignment): + // Waiting for reviewers to be assigned + return ":shrug:", "?st=awaiting_assignment" + + case string(turn.StateAssignedWaitingForReview): + // Waiting for assigned reviewers to review + return ":hourglass:", "?st=awaiting_review" + + case string(turn.StateReviewedNeedsRefinement): + // Author needs to address feedback/comments + return ":carpentry_saw:", "?st=changes_requested" + + case string(turn.StateRefinedWaitingForApproval): + // Waiting for final approval after refinements + return ":hourglass:", "?st=awaiting_approval" + + case string(turn.StateApprovedWaitingForMerge): + // Approved and ready to merge + return ":white_check_mark:", "?st=approved" + + default: + // Unknown workflow state - fall back to NextAction + slog.Warn("unknown workflow state, falling back to NextAction", + "workflow_state", workflowState) + if len(nextActions) > 0 { + action := PrimaryAction(nextActions) + return PrefixForAction(action), "?st=unknown" + } + return ":postal_horn:", "?st=unknown" + } +} + // stateParam returns the URL state parameter based on PR analysis. func stateParam(r *turn.CheckResponse) string { pr := r.PullRequest @@ -376,8 +435,8 @@ func PrimaryAction(nextActions map[string]turn.Action) string { // 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. +// 1. Use WorkflowState as primary signal (most reliable) +// 2. Fall back to NextAction if no WorkflowState func PrefixForAnalysis(workflowState string, nextActions map[string]turn.Action) string { // Log input for debugging emoji selection actionKinds := make([]string, 0, len(nextActions)) @@ -389,25 +448,27 @@ func PrefixForAnalysis(workflowState string, nextActions map[string]turn.Action) "next_actions_count", len(nextActions), "next_actions", actionKinds) - // Special case: newly published PRs always show :new: - if workflowState == "newly_published" { - slog.Debug("using :new: emoji for newly published PR") - return ":new:" + // Use WorkflowState as primary signal + if workflowState != "" { + emoji, _ := emojiFromWorkflowState(workflowState, nextActions) + slog.Debug("determined emoji from workflow_state", + "workflow_state", workflowState, + "emoji", emoji) + return emoji } - // Determine primary action and return corresponding emoji + // Fallback to NextAction if no WorkflowState primaryAction := PrimaryAction(nextActions) if primaryAction != "" { emoji := PrefixForAction(primaryAction) - slog.Debug("determined emoji from primary action", + slog.Debug("determined emoji from primary action (no workflow_state)", "primary_action", primaryAction, "emoji", emoji) return emoji } - // Fallback if no actions - slog.Info("no primary action found - using fallback emoji", - "workflow_state", workflowState, + // Final fallback if no workflow state or actions + slog.Info("no workflow_state or primary action - using fallback emoji", "next_actions_count", len(nextActions), "fallback_emoji", ":postal_horn:") return ":postal_horn:" diff --git a/pkg/notify/prefix_test.go b/pkg/notify/prefix_test.go index edd078f..5110704 100644 --- a/pkg/notify/prefix_test.go +++ b/pkg/notify/prefix_test.go @@ -13,91 +13,113 @@ func TestPrefixForAnalysis(t *testing.T) { nextAction map[string]turn.Action expectedEmoji string }{ + // Test all WorkflowState constants { - name: "newly published always shows :new:", - workflowState: "newly_published", + name: "NEWLY_PUBLISHED shows :new:", + workflowState: string(turn.StateNewlyPublished), nextAction: map[string]turn.Action{ - "author": {Kind: turn.ActionFixTests}, + "author": {Kind: turn.ActionRequestReviewers}, }, expectedEmoji: ":new:", }, { - name: "publish_draft shows :construction:", - workflowState: "awaiting_action", + name: "IN_DRAFT shows :construction:", + workflowState: string(turn.StateInDraft), nextAction: map[string]turn.Action{ "author": {Kind: turn.ActionPublishDraft}, }, expectedEmoji: ":construction:", }, { - name: "fix_tests shows :cockroach:", - workflowState: "blocked", + name: "PUBLISHED_WAITING_FOR_TESTS with broken tests shows :cockroach:", + workflowState: string(turn.StatePublishedWaitingForTests), nextAction: map[string]turn.Action{ "author": {Kind: turn.ActionFixTests}, }, expectedEmoji: ":cockroach:", }, { - name: "tests_pending shows :test_tube:", - workflowState: "blocked", + name: "PUBLISHED_WAITING_FOR_TESTS with pending tests shows :test_tube:", + workflowState: string(turn.StatePublishedWaitingForTests), nextAction: map[string]turn.Action{ "author": {Kind: turn.ActionTestsPending}, }, expectedEmoji: ":test_tube:", }, { - name: "review shows :hourglass:", - workflowState: "awaiting_review", + name: "PUBLISHED_WAITING_FOR_TESTS defaults to :test_tube:", + workflowState: string(turn.StatePublishedWaitingForTests), + nextAction: map[string]turn.Action{}, + expectedEmoji: ":test_tube:", + }, + { + name: "TESTED_WAITING_FOR_ASSIGNMENT shows :shrug:", + workflowState: string(turn.StateTestedWaitingForAssignment), + nextAction: map[string]turn.Action{ + "author": {Kind: turn.ActionRequestReviewers}, + }, + expectedEmoji: ":shrug:", + }, + { + name: "ASSIGNED_WAITING_FOR_REVIEW shows :hourglass:", + workflowState: string(turn.StateAssignedWaitingForReview), nextAction: map[string]turn.Action{ "reviewer": {Kind: turn.ActionReview}, }, expectedEmoji: ":hourglass:", }, { - name: "resolve_comments shows :carpentry_saw:", - workflowState: "changes_requested", + name: "REVIEWED_NEEDS_REFINEMENT shows :carpentry_saw:", + workflowState: string(turn.StateReviewedNeedsRefinement), nextAction: map[string]turn.Action{ "author": {Kind: turn.ActionResolveComments}, }, expectedEmoji: ":carpentry_saw:", }, { - name: "approve shows :white_check_mark:", - workflowState: "approved", + name: "REFINED_WAITING_FOR_APPROVAL shows :hourglass:", + workflowState: string(turn.StateRefinedWaitingForApproval), nextAction: map[string]turn.Action{ - "maintainer": {Kind: turn.ActionApprove}, + "reviewer": {Kind: turn.ActionApprove}, }, - expectedEmoji: ":white_check_mark:", + expectedEmoji: ":hourglass:", }, { - name: "merge shows :rocket:", - workflowState: "approved", + name: "APPROVED_WAITING_FOR_MERGE shows :white_check_mark:", + workflowState: string(turn.StateApprovedWaitingForMerge), nextAction: map[string]turn.Action{ "author": {Kind: turn.ActionMerge}, }, - expectedEmoji: ":rocket:", + expectedEmoji: ":white_check_mark:", }, + // Test NextAction fallback when no WorkflowState { - name: "publish_draft has highest priority over fix_tests", - workflowState: "blocked", + name: "no workflow_state but publish_draft action shows :construction:", + workflowState: "", nextAction: map[string]turn.Action{ - "author": {Kind: turn.ActionPublishDraft}, - "reviewer": {Kind: turn.ActionFixTests}, + "author": {Kind: turn.ActionPublishDraft}, }, expectedEmoji: ":construction:", }, { - name: "fix_tests has higher priority than review", - workflowState: "blocked", + name: "no workflow_state but fix_tests action shows :cockroach:", + workflowState: "", nextAction: map[string]turn.Action{ - "author": {Kind: turn.ActionFixTests}, - "reviewer": {Kind: turn.ActionReview}, + "author": {Kind: turn.ActionFixTests}, + }, + expectedEmoji: ":cockroach:", + }, + { + name: "unknown workflow_state falls back to next_action", + workflowState: "UNKNOWN_STATE", + nextAction: map[string]turn.Action{ + "author": {Kind: turn.ActionFixTests}, }, expectedEmoji: ":cockroach:", }, { - name: "empty next_action shows fallback", - workflowState: "unknown", + name: "no workflow_state and no next_action shows :postal_horn:", + workflowState: "", nextAction: map[string]turn.Action{}, expectedEmoji: ":postal_horn:", },