Skip to content

Commit e6e6fa9

Browse files
authored
Merge pull request #63 from codeGROOVE-dev/reliable
channel DM message refactor
2 parents 619399c + 569d301 commit e6e6fa9

File tree

6 files changed

+235
-80
lines changed

6 files changed

+235
-80
lines changed

go.mod

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,8 @@ require (
77
github.com/codeGROOVE-dev/gh-mailto v0.0.0-20251024133418-149270eb16a9
88
github.com/codeGROOVE-dev/gsm v0.0.0-20251019065141-833fe2363d22
99
github.com/codeGROOVE-dev/retry v1.3.0
10-
github.com/codeGROOVE-dev/sprinkler v0.0.0-20251027213037-05bb80a9db89
11-
github.com/codeGROOVE-dev/turnclient v0.0.0-20251022064427-5a712e1e10e6
10+
github.com/codeGROOVE-dev/sprinkler v0.0.0-20251028184624-4d8c8315a53a
11+
github.com/codeGROOVE-dev/turnclient v0.0.0-20251028130307-1f85c9aa43c4
1212
github.com/golang-jwt/jwt/v5 v5.3.0
1313
github.com/google/go-github/v50 v50.2.0
1414
github.com/gorilla/mux v1.8.1
@@ -47,7 +47,7 @@ require (
4747
golang.org/x/net v0.46.0 // indirect
4848
golang.org/x/sys v0.37.0 // indirect
4949
golang.org/x/text v0.30.0 // indirect
50-
google.golang.org/api v0.253.0 // indirect
50+
google.golang.org/api v0.254.0 // indirect
5151
google.golang.org/genproto v0.0.0-20251022142026-3a174f9686a8 // indirect
5252
google.golang.org/genproto/googleapis/api v0.0.0-20251022142026-3a174f9686a8 // indirect
5353
google.golang.org/genproto/googleapis/rpc v0.0.0-20251022142026-3a174f9686a8 // indirect

go.sum

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,12 @@ github.com/codeGROOVE-dev/sprinkler v0.0.0-20251027150242-98f387d0502a h1:bMRtyB
3333
github.com/codeGROOVE-dev/sprinkler v0.0.0-20251027150242-98f387d0502a/go.mod h1:/kd3ncsRNldD0MUpbtp5ojIzfCkyeXB7JdOrpuqG7Gg=
3434
github.com/codeGROOVE-dev/sprinkler v0.0.0-20251027213037-05bb80a9db89 h1:8Z3SM90hy1nuK2r2yhtv4HwitnO9si4GzVRktRDQ68g=
3535
github.com/codeGROOVE-dev/sprinkler v0.0.0-20251027213037-05bb80a9db89/go.mod h1:/kd3ncsRNldD0MUpbtp5ojIzfCkyeXB7JdOrpuqG7Gg=
36+
github.com/codeGROOVE-dev/sprinkler v0.0.0-20251028184624-4d8c8315a53a h1:P1qUAC4QG4zNH31y0R4ZJdnqpqHM+chKrTiI86GlIaw=
37+
github.com/codeGROOVE-dev/sprinkler v0.0.0-20251028184624-4d8c8315a53a/go.mod h1:/kd3ncsRNldD0MUpbtp5ojIzfCkyeXB7JdOrpuqG7Gg=
3638
github.com/codeGROOVE-dev/turnclient v0.0.0-20251022064427-5a712e1e10e6 h1:7FCmaftkl362oTZHVJyUg+xhxqfQFx+JisBf7RgklL8=
3739
github.com/codeGROOVE-dev/turnclient v0.0.0-20251022064427-5a712e1e10e6/go.mod h1:fYwtN9Ql6lY8t2WvCfENx+mP5FUwjlqwXCLx9CVLY20=
40+
github.com/codeGROOVE-dev/turnclient v0.0.0-20251028130307-1f85c9aa43c4 h1:si9tMEo5SXpDuDXGkJ1zNnnpP8TbmakrkNujAbpKlqA=
41+
github.com/codeGROOVE-dev/turnclient v0.0.0-20251028130307-1f85c9aa43c4/go.mod h1:bFWMd0JeaJY0kSIO5AcRQdJLXF3Fo3eKclE49vmIZes=
3842
github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
3943
github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
4044
github.com/envoyproxy/go-control-plane v0.13.4 h1:zEqyPVyku6IvWCFwux4x9RxkLOMUL+1vC9xUFv5l2/M=
@@ -127,6 +131,8 @@ gonum.org/v1/gonum v0.16.0 h1:5+ul4Swaf3ESvrOnidPp4GZbzf0mxVQpDCYUQE7OJfk=
127131
gonum.org/v1/gonum v0.16.0/go.mod h1:fef3am4MQ93R2HHpKnLk4/Tbh/s0+wqD5nfa6Pnwy4E=
128132
google.golang.org/api v0.253.0 h1:apU86Eq9Q2eQco3NsUYFpVTfy7DwemojL7LmbAj7g/I=
129133
google.golang.org/api v0.253.0/go.mod h1:PX09ad0r/4du83vZVAaGg7OaeyGnaUmT/CYPNvtLCbw=
134+
google.golang.org/api v0.254.0 h1:jl3XrGj7lRjnlUvZAbAdhINTLbsg5dbjmR90+pTQvt4=
135+
google.golang.org/api v0.254.0/go.mod h1:5BkSURm3D9kAqjGvBNgf0EcbX6Rnrf6UArKkwBzAyqQ=
130136
google.golang.org/genproto v0.0.0-20251022142026-3a174f9686a8 h1:a12a2/BiVRxRWIqBbfqoSK6tgq8cyUgMnEI81QlPge0=
131137
google.golang.org/genproto v0.0.0-20251022142026-3a174f9686a8/go.mod h1:1Ic78BnpzY8OaTCmzxJDP4qC9INZPbGZl+54RKjtyeI=
132138
google.golang.org/genproto/googleapis/api v0.0.0-20251022142026-3a174f9686a8 h1:mepRgnBZa07I4TRuomDE4sTIYieg/osKmzIf4USdWS4=

internal/bot/bot.go

Lines changed: 29 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -1375,37 +1375,19 @@ func (c *Coordinator) processPRForChannel(
13751375
// Build what the message SHOULD be based on current PR state
13761376
// Then compare to what it IS - update if different
13771377
if !wasNewlyCreated {
1378-
// Determine expected prefix based on workflow state and next actions
1379-
var expectedPrefix string
1380-
if checkResult != nil {
1381-
slog.Info("determining message update emoji from analysis",
1382-
"pr", fmt.Sprintf("%s/%s#%d", owner, repo, prNumber),
1383-
"workflow_state", checkResult.Analysis.WorkflowState,
1384-
"next_action_count", len(checkResult.Analysis.NextAction),
1385-
"pr_state_fallback", prState)
1386-
expectedPrefix = notify.PrefixForAnalysis(checkResult.Analysis.WorkflowState, checkResult.Analysis.NextAction)
1387-
} else {
1388-
slog.Info("no analysis available - using state-based emoji fallback for message update",
1389-
"pr", fmt.Sprintf("%s/%s#%d", owner, repo, prNumber),
1390-
"pr_state", prState)
1391-
expectedPrefix = notify.PrefixForState(prState)
1392-
}
13931378
domain := c.configManager.Domain(owner)
1394-
urlWithState := event.PullRequest.HTMLURL + c.getStateQueryParam(prState)
1395-
1396-
expectedText := fmt.Sprintf("%s %s <%s|%s#%d> · %s",
1397-
expectedPrefix,
1398-
event.PullRequest.Title,
1399-
urlWithState,
1400-
repo,
1401-
prNumber,
1402-
event.PullRequest.User.Login,
1403-
)
1404-
1405-
nextActions := c.formatNextActions(ctx, checkResult, owner, domain)
1406-
if nextActions != "" {
1407-
expectedText += fmt.Sprintf(" → %s", nextActions)
1379+
params := notify.MessageParams{
1380+
CheckResult: checkResult,
1381+
Owner: owner,
1382+
Repo: repo,
1383+
PRNumber: prNumber,
1384+
Title: event.PullRequest.Title,
1385+
Author: event.PullRequest.User.Login,
1386+
HTMLURL: event.PullRequest.HTMLURL,
1387+
Domain: domain,
1388+
UserMapper: c.userMapper,
14081389
}
1390+
expectedText := notify.FormatChannelMessageBase(ctx, params) + notify.FormatNextActionsSuffix(ctx, params)
14091391

14101392
// Compare expected vs actual - update if different
14111393
if currentText != expectedText {
@@ -1629,36 +1611,21 @@ func (c *Coordinator) createPRThread(ctx context.Context, channel, owner, repo s
16291611
Number int `json:"number"`
16301612
}, checkResult *turn.CheckResponse,
16311613
) (threadTS string, messageText string, err error) {
1632-
// Get emoji prefix based on workflow state and next actions
1633-
var prefix string
1634-
if checkResult != nil {
1635-
slog.Info("determining new thread emoji from analysis",
1636-
"pr", fmt.Sprintf("%s/%s#%d", owner, repo, number),
1637-
"workflow_state", checkResult.Analysis.WorkflowState,
1638-
"next_action_count", len(checkResult.Analysis.NextAction),
1639-
"pr_state_fallback", prState)
1640-
prefix = notify.PrefixForAnalysis(checkResult.Analysis.WorkflowState, checkResult.Analysis.NextAction)
1641-
} else {
1642-
// Fallback to state-based prefix if no checkResult available
1643-
slog.Info("no analysis available - using state-based emoji fallback for new thread",
1644-
"pr", fmt.Sprintf("%s/%s#%d", owner, repo, number),
1645-
"pr_state", prState)
1646-
prefix = notify.PrefixForState(prState)
1647-
}
1648-
1649-
// Add state query param to URL for debugging
1650-
urlWithState := pr.HTMLURL + c.getStateQueryParam(prState)
1651-
16521614
// Format initial message WITHOUT user mentions (fast path)
16531615
// Format: :emoji: Title repo#123 · author
1654-
initialText := fmt.Sprintf("%s %s <%s|%s#%d> · %s",
1655-
prefix,
1656-
pr.Title,
1657-
urlWithState,
1658-
repo,
1659-
number,
1660-
pr.User.Login,
1661-
)
1616+
domain := c.configManager.Domain(owner)
1617+
params := notify.MessageParams{
1618+
CheckResult: checkResult,
1619+
Owner: owner,
1620+
Repo: repo,
1621+
PRNumber: number,
1622+
Title: pr.Title,
1623+
Author: pr.User.Login,
1624+
HTMLURL: pr.HTMLURL,
1625+
Domain: domain,
1626+
UserMapper: c.userMapper,
1627+
}
1628+
initialText := notify.FormatChannelMessageBase(ctx, params)
16621629

16631630
// Resolve channel name to ID for consistent API calls
16641631
resolvedChannel := c.slack.ResolveChannelID(ctx, channel)
@@ -1694,7 +1661,6 @@ func (c *Coordinator) createPRThread(ctx context.Context, channel, owner, repo s
16941661

16951662
// Asynchronously add user mentions once email lookups complete
16961663
// This avoids blocking thread creation on slow email lookups (13-20 seconds each)
1697-
domain := c.configManager.Domain(owner)
16981664
if checkResult != nil && len(checkResult.Analysis.NextAction) > 0 {
16991665
// SECURITY NOTE: Use detached context to complete message enrichment even during shutdown.
17001666
// Operations bounded by 60-second timeout, allowing time for:
@@ -1710,14 +1676,15 @@ func (c *Coordinator) createPRThread(ctx context.Context, channel, owner, repo s
17101676
capturedNumber := number
17111677
capturedChannel := resolvedChannel
17121678
capturedInitialText := initialText
1679+
capturedParams := params
17131680
go func() {
17141681
defer enrichCancel()
17151682

17161683
// Perform email lookups in background
1717-
nextActions := c.formatNextActions(enrichCtx, checkResult, capturedOwner, domain)
1718-
if nextActions != "" {
1684+
nextActionsSuffix := notify.FormatNextActionsSuffix(enrichCtx, capturedParams)
1685+
if nextActionsSuffix != "" {
17191686
// Update message with user mentions
1720-
enrichedText := capturedInitialText + fmt.Sprintf(" → %s", nextActions)
1687+
enrichedText := capturedInitialText + nextActionsSuffix
17211688
if err := c.slack.UpdateMessage(enrichCtx, capturedChannel, capturedThreadTS, enrichedText); err != nil {
17221689
slog.Warn("failed to update thread with user mentions (async enrichment)",
17231690
logFieldPR, fmt.Sprintf(prFormatString, capturedOwner, capturedRepo, capturedNumber),
@@ -1729,7 +1696,7 @@ func (c *Coordinator) createPRThread(ctx context.Context, channel, owner, repo s
17291696
logFieldPR, fmt.Sprintf(prFormatString, capturedOwner, capturedRepo, capturedNumber),
17301697
logFieldChannel, capturedChannel,
17311698
"thread_ts", capturedThreadTS,
1732-
"next_actions", nextActions)
1699+
"next_actions_suffix", nextActionsSuffix)
17331700
}
17341701
}
17351702
}()

internal/notify/notify.go

Lines changed: 182 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"context"
66
"fmt"
77
"log/slog"
8+
"strings"
89
"time"
910

1011
"github.com/codeGROOVE-dev/slacker/internal/slack"
@@ -16,6 +17,187 @@ const (
1617
defaultReminderDMDelayMinutes = 65 // Default delay in minutes before sending DM if user tagged in channel
1718
)
1819

20+
// UserMapper interface for formatting user mentions in messages.
21+
type UserMapper interface {
22+
FormatUserMentions(ctx context.Context, githubUsers []string, owner, domain string) string
23+
}
24+
25+
// MessageParams contains all parameters needed to format a channel message.
26+
type MessageParams struct {
27+
CheckResult *turn.CheckResponse
28+
Owner string
29+
Repo string
30+
PRNumber int
31+
Title string
32+
Author string
33+
HTMLURL string
34+
Domain string
35+
UserMapper UserMapper
36+
}
37+
38+
// FormatChannelMessageBase formats the base Slack channel message for a PR (without next actions).
39+
// This is the single source of truth for channel message formatting.
40+
// It handles emoji determination and base message assembly.
41+
// Use FormatNextActionsSuffix to add next actions when needed.
42+
func FormatChannelMessageBase(ctx context.Context, params MessageParams) string {
43+
pr := params.CheckResult.PullRequest
44+
a := params.CheckResult.Analysis
45+
prID := fmt.Sprintf("%s/%s#%d", params.Owner, params.Repo, params.PRNumber)
46+
47+
// Log all decision factors for debugging
48+
kinds := make([]string, 0, len(a.NextAction))
49+
for user, action := range a.NextAction {
50+
kinds = append(kinds, fmt.Sprintf("%s:%s", user, action.Kind))
51+
}
52+
53+
slog.Info("formatting channel message",
54+
"pr", prID,
55+
"pr_state", pr.State,
56+
"merged", pr.Merged,
57+
"draft", pr.Draft,
58+
"workflow_state", a.WorkflowState,
59+
"next_action_count", len(a.NextAction),
60+
"next_actions", kinds,
61+
"checks_failing", a.Checks.Failing,
62+
"checks_pending", a.Checks.Pending,
63+
"checks_waiting", a.Checks.Waiting,
64+
"approved", a.Approved,
65+
"unresolved_comments", a.UnresolvedComments)
66+
67+
// Determine emoji and state parameter
68+
var emoji, state string
69+
70+
// Handle merged/closed states first (most definitive)
71+
if pr.Merged {
72+
emoji, state = ":rocket:", "?st=merged"
73+
slog.Info("using :rocket: emoji - PR is merged", "pr", prID, "merged_at", pr.MergedAt)
74+
} else if pr.State == "closed" {
75+
emoji, state = ":x:", "?st=closed"
76+
slog.Info("using :x: emoji - PR is closed but not merged", "pr", prID)
77+
} else if a.WorkflowState == "newly_published" {
78+
emoji, state = ":new:", "?st=newly_published"
79+
slog.Info("using :new: emoji - newly published PR", "pr", prID, "workflow_state", a.WorkflowState)
80+
} else if len(a.NextAction) > 0 {
81+
action := PrimaryAction(a.NextAction)
82+
emoji = PrefixForAction(action)
83+
state = stateParam(params.CheckResult)
84+
slog.Info("using emoji from primary next_action", "pr", prID, "primary_action", action, "emoji", emoji, "state_param", state)
85+
} else {
86+
emoji, state = fallbackEmoji(params.CheckResult)
87+
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")
88+
}
89+
90+
return fmt.Sprintf("%s %s <%s|%s#%d> · %s",
91+
emoji,
92+
params.Title,
93+
params.HTMLURL+state,
94+
params.Repo,
95+
params.PRNumber,
96+
params.Author)
97+
}
98+
99+
// FormatNextActionsSuffix formats the next actions suffix for a channel message.
100+
// Returns empty string if no next actions are present.
101+
// Call this after FormatChannelMessageBase to build a complete message with user mentions.
102+
func FormatNextActionsSuffix(ctx context.Context, params MessageParams) string {
103+
if params.CheckResult == nil || len(params.CheckResult.Analysis.NextAction) == 0 {
104+
return ""
105+
}
106+
107+
actions := formatNextActionsInternal(ctx, params.CheckResult.Analysis.NextAction, params.Owner, params.Domain, params.UserMapper)
108+
if actions != "" {
109+
return fmt.Sprintf(" → %s", actions)
110+
}
111+
return ""
112+
}
113+
114+
// stateParam returns the URL state parameter based on PR analysis.
115+
func stateParam(r *turn.CheckResponse) string {
116+
pr := r.PullRequest
117+
a := r.Analysis
118+
119+
if pr.Draft {
120+
return "?st=tests_running"
121+
}
122+
if a.Checks.Failing > 0 {
123+
return "?st=tests_broken"
124+
}
125+
if a.Checks.Pending > 0 || a.Checks.Waiting > 0 {
126+
return "?st=tests_running"
127+
}
128+
if a.Approved {
129+
if a.UnresolvedComments > 0 {
130+
return "?st=changes_requested"
131+
}
132+
return "?st=approved"
133+
}
134+
return "?st=awaiting_review"
135+
}
136+
137+
// fallbackEmoji determines emoji when no workflow_state or next_actions are available.
138+
func fallbackEmoji(r *turn.CheckResponse) (emoji, state string) {
139+
pr := r.PullRequest
140+
a := r.Analysis
141+
142+
if pr.Draft {
143+
return ":test_tube:", "?st=tests_running"
144+
}
145+
if a.Checks.Failing > 0 {
146+
return ":cockroach:", "?st=tests_broken"
147+
}
148+
if a.Checks.Pending > 0 || a.Checks.Waiting > 0 {
149+
return ":test_tube:", "?st=tests_running"
150+
}
151+
if a.Approved {
152+
if a.UnresolvedComments > 0 {
153+
return ":carpentry_saw:", "?st=changes_requested"
154+
}
155+
return ":white_check_mark:", "?st=approved"
156+
}
157+
return ":hourglass:", "?st=awaiting_review"
158+
}
159+
160+
// formatNextActionsInternal formats next actions with user mentions (internal helper).
161+
func formatNextActionsInternal(ctx context.Context, nextActions map[string]turn.Action, owner, domain string, userMapper UserMapper) string {
162+
if len(nextActions) == 0 {
163+
return ""
164+
}
165+
166+
// Group users by action kind, filtering out _system users
167+
actionGroups := make(map[string][]string)
168+
for user, action := range nextActions {
169+
actionKind := string(action.Kind)
170+
// Skip _system users but still track the action exists
171+
if user != "_system" {
172+
actionGroups[actionKind] = append(actionGroups[actionKind], user)
173+
} else if _, exists := actionGroups[actionKind]; !exists {
174+
// Action only has _system - create empty slice to track it exists
175+
actionGroups[actionKind] = []string{}
176+
}
177+
}
178+
179+
// Format each action group
180+
var parts []string
181+
for actionKind, users := range actionGroups {
182+
// Convert snake_case to space-separated words
183+
actionName := strings.ReplaceAll(actionKind, "_", " ")
184+
185+
// Format user mentions (will be empty if only _system was assigned)
186+
userMentions := userMapper.FormatUserMentions(ctx, users, owner, domain)
187+
188+
// If action has users, format as "action: users"
189+
// If no users (was only _system), just show the action
190+
if userMentions != "" {
191+
parts = append(parts, fmt.Sprintf("%s: %s", actionName, userMentions))
192+
} else {
193+
parts = append(parts, actionName)
194+
}
195+
}
196+
197+
// Use semicolons to separate different actions (commas are used between users)
198+
return strings.Join(parts, "; ")
199+
}
200+
19201
// Manager handles user notifications across multiple workspaces.
20202
type Manager struct {
21203
slackManager *slack.Manager

internal/notify/tracker_test.go

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -291,13 +291,13 @@ func TestNotificationTracker_KeyFormats(t *testing.T) {
291291

292292
// Test that various special characters in keys work correctly
293293
tests := []struct {
294-
name string
295-
workspace string
296-
user string
297-
owner string
298-
repo string
299-
prNumber int
300-
channelID string
294+
name string
295+
workspace string
296+
user string
297+
owner string
298+
repo string
299+
prNumber int
300+
channelID string
301301
}{
302302
{
303303
name: "simple keys",

0 commit comments

Comments
 (0)