Merge pull request #89 from tstromberg/reliable

Add delayed posting ("when")

Author: tstromberg

pkg/bot/bot.go

@@ -853,6 +853,78 @@ func (*Coordinator) extractBlockedUsersFromTurnclient(checkResult *turn.CheckRes
 	return blockedUsers
 }
 
+// shouldPostThread determines if a PR thread should be posted based on configured threshold.
+// Returns (shouldPost bool, reason string).
+func (c *Coordinator) shouldPostThread(
+	checkResult *turn.CheckResponse,
+	when string,
+) (bool, string) {
+	if checkResult == nil {
+		return false, "no_check_result"
+	}
+
+	pr := checkResult.PullRequest
+	analysis := checkResult.Analysis
+
+	// Terminal states ALWAYS post (ensure visibility)
+	if pr.Merged {
+		return true, "pr_merged"
+	}
+	if pr.State == "closed" {
+		return true, "pr_closed"
+	}
+
+	switch when {
+	case "immediate":
+		return true, "immediate_mode"
+
+	case "assigned":
+		// Post when PR has assignees
+		if len(pr.Assignees) > 0 {
+			return true, fmt.Sprintf("has_%d_assignees", len(pr.Assignees))
+		}
+		return false, "no_assignees"
+
+	case "blocked":
+		// Post when someone needs to take action
+		blockedUsers := c.extractBlockedUsersFromTurnclient(checkResult)
+		if len(blockedUsers) > 0 {
+			return true, fmt.Sprintf("blocked_on_%d_users", len(blockedUsers))
+		}
+		return false, "not_blocked_yet"
+
+	case "passing":
+		// Post when tests pass - use WorkflowState as primary signal
+		switch analysis.WorkflowState {
+		case string(turn.StateAssignedWaitingForReview),
+			string(turn.StateReviewedNeedsRefinement),
+			string(turn.StateRefinedWaitingForApproval),
+			string(turn.StateApprovedWaitingForMerge):
+			return true, fmt.Sprintf("workflow_state_%s", analysis.WorkflowState)
+
+		case string(turn.StateNewlyPublished),
+			string(turn.StateInDraft),
+			string(turn.StatePublishedWaitingForTests),
+			string(turn.StateTestedWaitingForAssignment):
+			return false, fmt.Sprintf("waiting_for_%s", analysis.WorkflowState)
+
+		default:
+			// Fallback: check test status directly
+			if analysis.Checks.Failing > 0 {
+				return false, "tests_failing"
+			}
+			if analysis.Checks.Pending > 0 || analysis.Checks.Waiting > 0 {
+				return false, "tests_pending"
+			}
+			return true, "tests_passed_fallback"
+		}
+
+	default:
+		slog.Warn("invalid when value, defaulting to immediate", "when", when)
+		return true, "invalid_config_default_immediate"
+	}
+}
+
 // formatNextActions formats NextAction map into a compact string like "fix tests: user1, user2; review: user3".
 // It groups users by action kind and formats each action as "action_name: user1, user2".
 // Multiple actions are separated by semicolons.
@@ -1082,7 +1154,8 @@ func (c *Coordinator) processPRForChannel(
 		oldState = threadInfo.LastState
 	}
 
-	// Find or create thread
+	// Check if thread already exists
+	cacheKey := fmt.Sprintf("%s/%s#%d:%s", owner, repo, prNumber, channelID)
 	pullRequestStruct := struct {
 		CreatedAt time.Time `json:"created_at"`
 		User      struct {
@@ -1098,6 +1171,36 @@ func (c *Coordinator) processPRForChannel(
 		Number:    event.PullRequest.Number,
 		CreatedAt: event.PullRequest.CreatedAt,
 	}
+
+	_, _, threadExists := c.findPRThread(ctx, cacheKey, channelID, owner, repo, prNumber, prState, pullRequestStruct)
+
+	// If thread doesn't exist, check if we should create it based on "when" threshold
+	if !threadExists {
+		when := c.configManager.When(owner, channelName)
+
+		if when != "immediate" {
+			shouldPost, reason := c.shouldPostThread(checkResult, when)
+
+			if !shouldPost {
+				slog.Debug("not creating thread - threshold not met",
+					"workspace", c.workspaceName,
+					logFieldPR, fmt.Sprintf(prFormatString, owner, repo, prNumber),
+					"channel", channelDisplay,
+					"when", when,
+					"reason", reason)
+				return nil // Don't create thread yet - next event will check again
+			}
+
+			slog.Info("creating thread - threshold met",
+				"workspace", c.workspaceName,
+				logFieldPR, fmt.Sprintf(prFormatString, owner, repo, prNumber),
+				"channel", channelDisplay,
+				"when", when,
+				"reason", reason)
+		}
+	}
+
+	// Find or create thread
 	threadTS, wasNewlyCreated, currentText, err := c.findOrCreatePRThread(ctx, threadCreationParams{
 		ChannelID:   channelID,
 		ChannelName: channelName,

pkg/bot/bot_test.go

@@ -6,7 +6,9 @@ import (
 	"os"
 	"testing"
 
+	"github.com/codeGROOVE-dev/prx/pkg/prx"
 	"github.com/codeGROOVE-dev/slacker/pkg/bot/cache"
+	"github.com/codeGROOVE-dev/turnclient/pkg/turn"
 	"github.com/slack-go/slack"
 )
 
@@ -309,3 +311,234 @@ func TestThreadCache_Set(t *testing.T) {
 		t.Errorf("expected channel ID %s, got %s", threadInfo.ChannelID, retrieved.ChannelID)
 	}
 }
+
+func TestShouldPostThread(t *testing.T) {
+	coord := &Coordinator{}
+
+	tests := []struct {
+		name           string
+		when           string
+		checkResult    *turn.CheckResponse
+		wantPost       bool
+		wantReasonPart string // Part of the reason string to check for
+	}{
+		{
+			name: "immediate mode always posts",
+			when: "immediate",
+			checkResult: &turn.CheckResponse{
+				PullRequest: prx.PullRequest{
+					State: "open",
+				},
+			},
+			wantPost:       true,
+			wantReasonPart: "immediate_mode",
+		},
+		{
+			name: "merged PR always posts regardless of when",
+			when: "passing",
+			checkResult: &turn.CheckResponse{
+				PullRequest: prx.PullRequest{
+					State:  "closed",
+					Merged: true,
+				},
+			},
+			wantPost:       true,
+			wantReasonPart: "pr_merged",
+		},
+		{
+			name: "closed PR always posts regardless of when",
+			when: "passing",
+			checkResult: &turn.CheckResponse{
+				PullRequest: prx.PullRequest{
+					State:  "closed",
+					Merged: false,
+				},
+			},
+			wantPost:       true,
+			wantReasonPart: "pr_closed",
+		},
+		{
+			name: "assigned: posts when has assignees",
+			when: "assigned",
+			checkResult: &turn.CheckResponse{
+				PullRequest: prx.PullRequest{
+					State:     "open",
+					Assignees: []string{"user1", "user2"},
+				},
+			},
+			wantPost:       true,
+			wantReasonPart: "has_2_assignees",
+		},
+		{
+			name: "assigned: does not post when no assignees",
+			when: "assigned",
+			checkResult: &turn.CheckResponse{
+				PullRequest: prx.PullRequest{
+					State:     "open",
+					Assignees: []string{},
+				},
+			},
+			wantPost:       false,
+			wantReasonPart: "no_assignees",
+		},
+		{
+			name: "blocked: posts when users are blocked",
+			when: "blocked",
+			checkResult: &turn.CheckResponse{
+				PullRequest: prx.PullRequest{
+					State: "open",
+				},
+				Analysis: turn.Analysis{
+					NextAction: map[string]turn.Action{
+						"user1": {Kind: "review"},
+						"user2": {Kind: "approve"},
+					},
+				},
+			},
+			wantPost:       true,
+			wantReasonPart: "blocked_on_2_users",
+		},
+		{
+			name: "blocked: does not post when no users blocked",
+			when: "blocked",
+			checkResult: &turn.CheckResponse{
+				PullRequest: prx.PullRequest{
+					State: "open",
+				},
+				Analysis: turn.Analysis{
+					NextAction: map[string]turn.Action{},
+				},
+			},
+			wantPost:       false,
+			wantReasonPart: "not_blocked_yet",
+		},
+		{
+			name: "blocked: ignores _system sentinel",
+			when: "blocked",
+			checkResult: &turn.CheckResponse{
+				PullRequest: prx.PullRequest{
+					State: "open",
+				},
+				Analysis: turn.Analysis{
+					NextAction: map[string]turn.Action{
+						"_system": {Kind: "processing"},
+					},
+				},
+			},
+			wantPost:       false,
+			wantReasonPart: "not_blocked_yet",
+		},
+		{
+			name: "passing: posts when in review state",
+			when: "passing",
+			checkResult: &turn.CheckResponse{
+				PullRequest: prx.PullRequest{
+					State: "open",
+				},
+				Analysis: turn.Analysis{
+					WorkflowState: string(turn.StateAssignedWaitingForReview),
+				},
+			},
+			wantPost:       true,
+			wantReasonPart: "workflow_state",
+		},
+		{
+			name: "passing: does not post when tests pending",
+			when: "passing",
+			checkResult: &turn.CheckResponse{
+				PullRequest: prx.PullRequest{
+					State: "open",
+				},
+				Analysis: turn.Analysis{
+					WorkflowState: string(turn.StatePublishedWaitingForTests),
+				},
+			},
+			wantPost:       false,
+			wantReasonPart: "waiting_for",
+		},
+		{
+			name: "passing: uses fallback when workflow state unknown and tests passing",
+			when: "passing",
+			checkResult: &turn.CheckResponse{
+				PullRequest: prx.PullRequest{
+					State: "open",
+				},
+				Analysis: turn.Analysis{
+					WorkflowState: "unknown_state",
+					Checks: turn.Checks{
+						Passing: 5,
+						Failing: 0,
+						Pending: 0,
+						Waiting: 0,
+					},
+				},
+			},
+			wantPost:       true,
+			wantReasonPart: "tests_passed_fallback",
+		},
+		{
+			name: "passing: uses fallback when tests failing",
+			when: "passing",
+			checkResult: &turn.CheckResponse{
+				PullRequest: prx.PullRequest{
+					State: "open",
+				},
+				Analysis: turn.Analysis{
+					WorkflowState: "unknown_state",
+					Checks: turn.Checks{
+						Passing: 3,
+						Failing: 2,
+					},
+				},
+			},
+			wantPost:       false,
+			wantReasonPart: "tests_failing",
+		},
+		{
+			name:           "nil check result returns false",
+			when:           "passing",
+			checkResult:    nil,
+			wantPost:       false,
+			wantReasonPart: "no_check_result",
+		},
+		{
+			name: "invalid when value defaults to immediate",
+			when: "invalid_value",
+			checkResult: &turn.CheckResponse{
+				PullRequest: prx.PullRequest{
+					State: "open",
+				},
+			},
+			wantPost:       true,
+			wantReasonPart: "invalid_config",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			gotPost, gotReason := coord.shouldPostThread(tt.checkResult, tt.when)
+
+			if gotPost != tt.wantPost {
+				t.Errorf("shouldPostThread() gotPost = %v, wantPost %v", gotPost, tt.wantPost)
+			}
+
+			if tt.wantReasonPart != "" && !contains(gotReason, tt.wantReasonPart) {
+				t.Errorf("shouldPostThread() reason = %q, want to contain %q", gotReason, tt.wantReasonPart)
+			}
+		})
+	}
+}
+
+// Helper function to check if a string contains a substring
+func contains(s, substr string) bool {
+	return len(s) >= len(substr) && (s == substr || len(s) > len(substr) && (s[:len(substr)] == substr || s[len(s)-len(substr):] == substr || containsMiddle(s, substr)))
+}
+
+func containsMiddle(s, substr string) bool {
+	for i := 0; i <= len(s)-len(substr); i++ {
+		if s[i:i+len(substr)] == substr {
+			return true
+		}
+	}
+	return false
+}

pkg/bot/integration_test.go

@@ -386,6 +386,10 @@ func (m *mockConfigManager) ReminderDMDelay(org, channel string) int {
 	return m.dmDelay
 }
 
+func (m *mockConfigManager) When(org, channel string) string {
+	return "immediate" // Default for tests
+}
+
 func (m *mockConfigManager) ChannelsForRepo(org, repo string) []string {
 	if m.channelsFunc != nil {
 		return m.channelsFunc(org, repo)

pkg/bot/interfaces.go

@@ -54,6 +54,7 @@ type ConfigManager interface {
 	WorkspaceName(org string) string
 	ChannelsForRepo(org, repo string) []string
 	ReminderDMDelay(org, channelName string) int
+	When(org, channelName string) string
 	SetGitHubClient(org string, client any)
 	SetWorkspaceName(workspaceName string)
 }