Merge pull request #67 from codeGROOVE-dev/reliable

Don't duplicate DMs

Author: tstromberg

go.mod

@@ -6,12 +6,13 @@ require (
 	github.com/codeGROOVE-dev/ds9 v0.6.0
 	github.com/codeGROOVE-dev/gh-mailto v0.0.0-20251024133418-149270eb16a9
 	github.com/codeGROOVE-dev/gsm v0.0.0-20251019065141-833fe2363d22
-	github.com/codeGROOVE-dev/prx v0.0.0-20251028202628-9f237ee71356
+	github.com/codeGROOVE-dev/prx v0.0.0-20251030022101-ff906928a1e4
 	github.com/codeGROOVE-dev/retry v1.3.0
 	github.com/codeGROOVE-dev/sprinkler v0.0.0-20251029020504-57f2ea3ae37a
-	github.com/codeGROOVE-dev/turnclient v0.0.0-20251028130307-1f85c9aa43c4
+	github.com/codeGROOVE-dev/turnclient v0.0.0-20251030022425-bc3b14acf75e
 	github.com/golang-jwt/jwt/v5 v5.3.0
 	github.com/google/go-github/v50 v50.2.0
+	github.com/google/uuid v1.6.0
 	github.com/gorilla/mux v1.8.1
 	github.com/shurcooL/githubv4 v0.0.0-20240727222349-48295856cce7
 	github.com/slack-go/slack v0.17.3
@@ -26,7 +27,6 @@ require (
 	github.com/cloudflare/circl v1.6.1 // indirect
 	github.com/google/go-cmp v0.7.0 // indirect
 	github.com/google/go-querystring v1.1.0 // indirect
-	github.com/google/uuid v1.6.0 // indirect
 	github.com/gorilla/websocket v1.5.3 // indirect
 	github.com/kr/pretty v0.3.1 // indirect
 	github.com/rogpeppe/go-internal v1.14.1 // indirect

go.sum

@@ -8,14 +8,14 @@ github.com/codeGROOVE-dev/gh-mailto v0.0.0-20251024133418-149270eb16a9 h1:eyWcEZ
 github.com/codeGROOVE-dev/gh-mailto v0.0.0-20251024133418-149270eb16a9/go.mod h1:4Hr2ySB8dcpeZqZq/7UbXdEJ/5RK9coYGHvW90ZfieE=
 github.com/codeGROOVE-dev/gsm v0.0.0-20251019065141-833fe2363d22 h1:gtN3rOc6YspO646BkcOxBhPjEqKUz+jl175jIqglfDg=
 github.com/codeGROOVE-dev/gsm v0.0.0-20251019065141-833fe2363d22/go.mod h1:KV+w19ubP32PxZPE1hOtlCpTaNpF0Bpb32w5djO8UTg=
-github.com/codeGROOVE-dev/prx v0.0.0-20251028202628-9f237ee71356 h1:lHoHnylLAp7/7BMhdiTh9Z2+p4ATcQ7aFcgqxOFGzE4=
-github.com/codeGROOVE-dev/prx v0.0.0-20251028202628-9f237ee71356/go.mod h1:FEy3gz9IYDXWnKWkoDSL+pWu6rujxbBSrF4w5A8QSK0=
+github.com/codeGROOVE-dev/prx v0.0.0-20251030022101-ff906928a1e4 h1:DSuoUwP3oyR4cHrX0cUh9c7CtYjXNIcyCmqpIwHilIU=
+github.com/codeGROOVE-dev/prx v0.0.0-20251030022101-ff906928a1e4/go.mod h1:FEy3gz9IYDXWnKWkoDSL+pWu6rujxbBSrF4w5A8QSK0=
 github.com/codeGROOVE-dev/retry v1.3.0 h1:/+ipAWRJLL6y1R1vprYo0FSjSBvH6fE5j9LKXjpD54g=
 github.com/codeGROOVE-dev/retry v1.3.0/go.mod h1:8OgefgV1XP7lzX2PdKlCXILsYKuz6b4ZpHa/20iLi8E=
 github.com/codeGROOVE-dev/sprinkler v0.0.0-20251029020504-57f2ea3ae37a h1:iLjcbNpCKXT8ZAlGGesh0x9g/ntgG5OCuvM2QJ9+27E=
 github.com/codeGROOVE-dev/sprinkler v0.0.0-20251029020504-57f2ea3ae37a/go.mod h1:/kd3ncsRNldD0MUpbtp5ojIzfCkyeXB7JdOrpuqG7Gg=
-github.com/codeGROOVE-dev/turnclient v0.0.0-20251028130307-1f85c9aa43c4 h1:si9tMEo5SXpDuDXGkJ1zNnnpP8TbmakrkNujAbpKlqA=
-github.com/codeGROOVE-dev/turnclient v0.0.0-20251028130307-1f85c9aa43c4/go.mod h1:bFWMd0JeaJY0kSIO5AcRQdJLXF3Fo3eKclE49vmIZes=
+github.com/codeGROOVE-dev/turnclient v0.0.0-20251030022425-bc3b14acf75e h1:WXHdC8o5KmP5CwkQRiGVywYzsj93fjkRPq7clhfZPq0=
+github.com/codeGROOVE-dev/turnclient v0.0.0-20251030022425-bc3b14acf75e/go.mod h1:dVS3MlJDgL6WkfurJAyS7I9Fe1yxxoxxarjVifY5bIo=
 github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E=
 github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
 github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=

pkg/notify/interfaces.go

@@ -25,6 +25,7 @@ type SlackClient interface {
 
 	// DM operations
 	SendDirectMessage(ctx context.Context, userID, text string) (dmChannelID, messageTS string, err error)
+	UpdateDMMessage(ctx context.Context, userID, prURL, newText string) error
 	HasRecentDMAboutPR(ctx context.Context, userID, prURL string) (bool, error)
 	SaveDMMessageInfo(ctx context.Context, userID, prURL, dmChannelID, messageTS, message string) error
 

pkg/notify/notify.go

@@ -4,12 +4,14 @@ package notify
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"log/slog"
 	"sort"
 	"strings"
 	"time"
 
+	"github.com/codeGROOVE-dev/slacker/pkg/slack"
 	"github.com/codeGROOVE-dev/slacker/pkg/state"
 	"github.com/codeGROOVE-dev/turnclient/pkg/turn"
 	"github.com/google/uuid"
@@ -881,6 +883,33 @@ func (m *Manager) NotifyUser(ctx context.Context, workspaceID, userID, channelID
 		"action_required", action,
 		"message", message)
 
+	// Try to update existing DM first (if one exists in our state store)
+	// UpdateDMMessage returns ErrNoDMToUpdate if no DM exists
+	updateErr := slackClient.UpdateDMMessage(ctx, userID, pr.HTMLURL, message)
+	if updateErr == nil {
+		// Successfully updated existing DM
+		slog.Info("successfully updated existing DM with new PR state",
+			"user", userID,
+			"pr", fmt.Sprintf("%s/%s#%d", pr.Owner, pr.Repo, pr.Number),
+			"pr_state", pr.State,
+			"action_required", action)
+		return nil
+	}
+
+	// Check if it's because no DM exists (expected case for first notification)
+	if !errors.Is(updateErr, slack.ErrNoDMToUpdate) {
+		// Actual error occurred during update - log warning but continue to send new DM
+		slog.Warn("failed to update existing DM, will send new one",
+			"user", userID,
+			"pr", fmt.Sprintf("%s/%s#%d", pr.Owner, pr.Repo, pr.Number),
+			"error", updateErr)
+	}
+
+	slog.Debug("no existing DM to update, will check for recent DMs and potentially send new one",
+		"user", userID,
+		"pr", fmt.Sprintf("%s/%s#%d", pr.Owner, pr.Repo, pr.Number),
+		"reason", "DM not in state store or too old")
+
 	// Check if we recently sent a DM about this PR (prevents duplicates during rolling deployments)
 	hasRecent, err := slackClient.HasRecentDMAboutPR(ctx, userID, pr.HTMLURL)
 	if err != nil {

pkg/notify/notify_user_test.go

@@ -6,6 +6,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/codeGROOVE-dev/slacker/pkg/slack"
 	"github.com/codeGROOVE-dev/slacker/pkg/state"
 	slackapi "github.com/slack-go/slack"
 )
@@ -16,6 +17,7 @@ type mockSlackClient struct {
 	isUserInChannelFunc    func(ctx context.Context, channelID, userID string) bool
 	userTimezoneFunc       func(ctx context.Context, userID string) (string, error)
 	sendDirectMessageFunc  func(ctx context.Context, userID, text string) (dmChannelID, messageTS string, err error)
+	updateDMMessageFunc    func(ctx context.Context, userID, prURL, newText string) error
 	hasRecentDMAboutPRFunc func(ctx context.Context, userID, prURL string) (bool, error)
 	saveDMMessageInfoFunc  func(ctx context.Context, userID, prURL, dmChannelID, messageTS, message string) error
 	apiFunc                func() *slackapi.Client
@@ -49,6 +51,14 @@ func (m *mockSlackClient) SendDirectMessage(ctx context.Context, userID, text st
 	return "D123", "1234567890.123456", nil
 }
 
+func (m *mockSlackClient) UpdateDMMessage(ctx context.Context, userID, prURL, newText string) error {
+	if m.updateDMMessageFunc != nil {
+		return m.updateDMMessageFunc(ctx, userID, prURL, newText)
+	}
+	// Default: return ErrNoDMToUpdate (no DM exists to update)
+	return slack.ErrNoDMToUpdate
+}
+
 func (m *mockSlackClient) HasRecentDMAboutPR(ctx context.Context, userID, prURL string) (bool, error) {
 	if m.hasRecentDMAboutPRFunc != nil {
 		return m.hasRecentDMAboutPRFunc(ctx, userID, prURL)

pkg/slack/client_additional_test.go

@@ -18,9 +18,9 @@ func TestUpdateDMMessage(t *testing.T) {
 
 		prURL := "https://github.com/test/repo/pull/123"
 		err := client.UpdateDMMessage(ctx, "U001", prURL, "New text")
-		// Should not error when state store is nil (graceful degradation)
-		if err != nil {
-			t.Fatalf("unexpected error: %v", err)
+		// Should return ErrNoDMToUpdate when state store is nil
+		if !errors.Is(err, ErrNoDMToUpdate) {
+			t.Fatalf("expected ErrNoDMToUpdate, got: %v", err)
 		}
 	})
 }

pkg/slack/slack.go

@@ -8,6 +8,7 @@ import (
 	"crypto/sha256"
 	"encoding/hex"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
 	"log/slog"
@@ -24,6 +25,12 @@ import (
 	"github.com/slack-go/slack/slackevents"
 )
 
+// Errors
+var (
+	// ErrNoDMToUpdate indicates no DM exists to update.
+	ErrNoDMToUpdate = errors.New("no DM found to update")
+)
+
 // Constants for input validation.
 const (
 	maxCommandInputLength = 200
@@ -460,9 +467,14 @@ func (c *Client) SendDirectMessage(ctx context.Context, userID, text string) (dm
 
 	var msgTS string
 	// Then send message with retry
+	// Disable unfurling for GitHub links in DMs.
+	options := []slack.MsgOption{
+		slack.MsgOptionText(text, false),
+		slack.MsgOptionDisableLinkUnfurl(),
+	}
 	err = retry.Do(
 		func() error {
-			_, ts, err := c.api.PostMessageContext(ctx, channelID, slack.MsgOptionText(text, false))
+			_, ts, err := c.api.PostMessageContext(ctx, channelID, options...)
 			if err != nil {
 				if isRateLimitError(err) {
 					slog.Warn("rate limited sending DM, backing off", "user", userID)
@@ -522,14 +534,15 @@ func (c *Client) SaveDMMessageInfo(ctx context.Context, userID, prURL, channelID
 }
 
 // UpdateDMMessage updates a previously sent DM message.
+// Returns nil and logs debug if no DM exists, returns error if update fails.
 func (c *Client) UpdateDMMessage(ctx context.Context, userID, prURL, newText string) error {
 	c.stateStoreMu.RLock()
 	store := c.stateStore
 	c.stateStoreMu.RUnlock()
 
 	if store == nil {
 		slog.Debug("no state store configured, cannot update DM", "user", userID, "pr_url", prURL)
-		return nil
+		return ErrNoDMToUpdate
 	}
 
 	// Get stored DM message info
@@ -539,7 +552,7 @@ func (c *Client) UpdateDMMessage(ctx context.Context, userID, prURL, newText str
 			"user", userID,
 			"pr_url", prURL,
 			"reason", "never sent or too old")
-		return nil
+		return ErrNoDMToUpdate
 	}
 
 	// Update the message using Slack API