From e81488481c324b03116b0a79912b80708880f6d9 Mon Sep 17 00:00:00 2001 From: piekstra Date: Wed, 3 Jun 2026 08:32:23 -0400 Subject: [PATCH 1/4] fix: send DMs by user ID or @handle slck messages send rejected the user ID. ResolveChannel only recognized C/G/D channel-ID prefixes, so a U.../W... user ID fell through to the channel-name lookup, got lowercased, and failed with "channel '...' not found". Resolve user IDs (U.../W...) and @handles to an IM channel via conversations.open (requires im:write) before posting. Because channel resolution is shared by every messages command, history/react/etc. now work against DMs too. Fixes #152 --- README.md | 4 + internal/client/channel_resolver.go | 100 ++++++++++++++++++- internal/client/channel_resolver_test.go | 119 +++++++++++++++++++++++ internal/client/client.go | 26 +++++ internal/cmd/messages/send.go | 10 +- 5 files changed, 253 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index da0c2f4..d5e9b5f 100644 --- a/README.md +++ b/README.md @@ -547,6 +547,10 @@ slck messages send C1234567890 "Hello, *world*!" # Send using --channel flag (alternative to positional arg) slck messages send --channel general "Hello team" +# Send a direct message to a user (by user ID or @handle; requires im:write scope) +slck messages send U1234567890 "Hey, got a minute?" +slck messages send @alice "Hey, got a minute?" + # Send from stdin (use "-" as text argument) echo "Hello from stdin" | slck messages send C1234567890 - cat message.txt | slck messages send C1234567890 - diff --git a/internal/client/channel_resolver.go b/internal/client/channel_resolver.go index 062ae50..1bdcbff 100644 --- a/internal/client/channel_resolver.go +++ b/internal/client/channel_resolver.go @@ -5,10 +5,34 @@ import ( "strings" ) -// ResolveChannel takes a channel identifier (ID or name) and returns the channel ID. -// If the input looks like a channel ID (starts with C, G, or D), it's returned as-is. -// Otherwise, it's treated as a channel name and looked up via the Slack API. +// ResolveChannel takes a conversation identifier and returns the channel ID to +// act on. It accepts: +// - channel IDs (C/G/D...) returned as-is +// - channel names ("general", "#general") looked up via the Slack API +// - user IDs (U.../W...) a DM is opened and the IM channel ID returned +// - user handles ("@alice") resolved to a user ID, then a DM is opened +// +// Opening a DM uses conversations.open and requires the im:write scope. func (c *Client) ResolveChannel(channel string) (string, error) { + if channel == "" { + return "", fmt.Errorf("channel cannot be empty") + } + + // "@handle" -> resolve the username to a user ID, then open a DM. + if handle := strings.TrimPrefix(channel, "@"); handle != channel { + userID, err := c.resolveUserHandle(handle) + if err != nil { + return "", err + } + return c.OpenDM(userID) + } + + // A bare user ID (U.../W...) -> open a DM and return its IM channel ID. + // chat.postMessage rejects raw user IDs, so the DM must be opened first. + if IsUserID(channel) { + return c.OpenDM(channel) + } + // Strip leading # if present (common user mistake) channel = strings.TrimPrefix(channel, "#") @@ -60,6 +84,76 @@ func IsChannelID(s string) bool { return hasDigit } +// IsUserID returns true if the string looks like a Slack user ID. +// User IDs start with: +// - U = regular user +// - W = Enterprise Grid user +// +// Like channel IDs, they are uppercase alphanumeric and contain at least one +// digit, which distinguishes them from plain words (e.g. "U" or "Us"). +func IsUserID(s string) bool { + if len(s) < 2 { + return false + } + if s[0] != 'U' && s[0] != 'W' { + return false + } + rest := s[1:] + hasDigit := false + for _, c := range rest { + if c >= '0' && c <= '9' { + hasDigit = true + } else if c < 'A' || c > 'Z' { + return false // not uppercase letter or digit + } + } + return hasDigit +} + +// resolveUserHandle resolves a Slack @handle to a user ID. The leading "@" may +// be included or omitted. It matches the username (the @name, which is unique) +// first, then falls back to display name, and errors if the handle is unknown +// or ambiguous. +func (c *Client) resolveUserHandle(handle string) (string, error) { + handle = strings.TrimPrefix(handle, "@") + if handle == "" { + return "", fmt.Errorf("user handle cannot be empty") + } + + users, err := c.ListUsers(1000) + if err != nil { + return "", fmt.Errorf("failed to list users: %w", err) + } + + var byName, byDisplay []User + for _, u := range users { + switch { + case strings.EqualFold(u.Name, handle): + byName = append(byName, u) + case strings.EqualFold(u.Profile.DisplayName, handle): + byDisplay = append(byDisplay, u) + } + } + + matches := byName + if len(matches) == 0 { + matches = byDisplay + } + + switch len(matches) { + case 0: + return "", fmt.Errorf("user '@%s' not found. Use 'slck users search' to find a user", handle) + case 1: + return matches[0].ID, nil + default: + ids := make([]string, len(matches)) + for i, u := range matches { + ids[i] = u.ID + } + return "", fmt.Errorf("user '@%s' is ambiguous (matches %s); use the user ID instead", handle, strings.Join(ids, ", ")) + } +} + // lookupChannelByName searches for a channel by name and returns its ID. func (c *Client) lookupChannelByName(name string) (string, error) { // Normalize the name (lowercase, strip #) diff --git a/internal/client/channel_resolver_test.go b/internal/client/channel_resolver_test.go index 9986170..1e07dc1 100644 --- a/internal/client/channel_resolver_test.go +++ b/internal/client/channel_resolver_test.go @@ -120,3 +120,122 @@ func TestResolveChannel_Empty(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "cannot be empty") } + +func TestIsUserID(t *testing.T) { + tests := []struct { + input string + expected bool + }{ + {"U07D13N5AMV", true}, // regular user + {"U123456", true}, + {"W012ABC34", true}, // Enterprise Grid user + {"USERNAME", false}, // pure letters - treated as a name + {"u07d13n5amv", false}, // lowercase + {"C123456", false}, // channel ID, not a user ID + {"B123456", false}, // bot ID + {"U", false}, // too short + {"", false}, + } + + for _, tt := range tests { + t.Run(tt.input, func(t *testing.T) { + assert.Equal(t, tt.expected, IsUserID(tt.input)) + }) + } +} + +// TestResolveChannel_UserID covers issue #152: a bare user ID must open a DM +// (conversations.open) rather than being lowercased and rejected as a channel. +func TestResolveChannel_UserID(t *testing.T) { + var gotUsers string + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, "/conversations.open", r.URL.Path) + var body struct { + Users string `json:"users"` + } + _ = json.NewDecoder(r.Body).Decode(&body) + gotUsers = body.Users + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]interface{}{ + "ok": true, + "channel": map[string]interface{}{"id": "D0123456789"}, + }) + })) + defer server.Close() + + c := NewWithConfig(server.URL, "test-token", nil) + + result, err := c.ResolveChannel("U07D13N5AMV") + require.NoError(t, err) + assert.Equal(t, "D0123456789", result) + assert.Equal(t, "U07D13N5AMV", gotUsers, "user ID should be passed to conversations.open unchanged") +} + +func TestResolveChannel_Handle(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch r.URL.Path { + case "/users.list": + _ = json.NewEncoder(w).Encode(map[string]interface{}{ + "ok": true, + "members": []map[string]interface{}{ + {"id": "U111", "name": "alice"}, + {"id": "U222", "name": "bob"}, + }, + }) + case "/conversations.open": + _ = json.NewEncoder(w).Encode(map[string]interface{}{ + "ok": true, + "channel": map[string]interface{}{"id": "D0000000001"}, + }) + default: + t.Errorf("unexpected path %s", r.URL.Path) + } + })) + defer server.Close() + + c := NewWithConfig(server.URL, "test-token", nil) + + result, err := c.ResolveChannel("@alice") + require.NoError(t, err) + assert.Equal(t, "D0000000001", result) +} + +func TestResolveChannel_HandleNotFound(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, "/users.list", r.URL.Path) + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]interface{}{ + "ok": true, + "members": []map[string]interface{}{{"id": "U111", "name": "alice"}}, + }) + })) + defer server.Close() + + c := NewWithConfig(server.URL, "test-token", nil) + + _, err := c.ResolveChannel("@nobody") + require.Error(t, err) + assert.Contains(t, err.Error(), "not found") + assert.Contains(t, err.Error(), "slck users search") +} + +func TestResolveChannel_HandleAmbiguous(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]interface{}{ + "ok": true, + "members": []map[string]interface{}{ + {"id": "U111", "profile": map[string]interface{}{"display_name": "Sam"}}, + {"id": "U222", "profile": map[string]interface{}{"display_name": "Sam"}}, + }, + }) + })) + defer server.Close() + + c := NewWithConfig(server.URL, "test-token", nil) + + _, err := c.ResolveChannel("@Sam") + require.Error(t, err) + assert.Contains(t, err.Error(), "ambiguous") +} diff --git a/internal/client/client.go b/internal/client/client.go index 21033ff..a9992c0 100644 --- a/internal/client/client.go +++ b/internal/client/client.go @@ -491,6 +491,32 @@ func (c *Client) GetUserInfo(userID string) (*User, error) { return &result.User, nil } +// OpenDM opens (or returns the existing) direct-message conversation with a user +// and returns its IM channel ID (D...). It wraps conversations.open, which is +// idempotent: calling it for a user you already have a DM with returns the same +// channel rather than creating a new one. Requires the im:write scope. +func (c *Client) OpenDM(userID string) (string, error) { + body, err := c.post("conversations.open", map[string]interface{}{ + "users": userID, + }) + if err != nil { + return "", err + } + + var result struct { + Channel struct { + ID string `json:"id"` + } `json:"channel"` + } + if err := json.Unmarshal(body, &result); err != nil { + return "", err + } + if result.Channel.ID == "" { + return "", fmt.Errorf("conversations.open returned no channel id for user %s", userID) + } + return result.Channel.ID, nil +} + // SendMessage sends a message to a channel. // Text can be empty if blocks are provided (Slack API allows this). // The unfurl parameter controls whether link previews are shown (unfurl_links and unfurl_media). diff --git a/internal/cmd/messages/send.go b/internal/cmd/messages/send.go index ad2096c..4c7f192 100644 --- a/internal/cmd/messages/send.go +++ b/internal/cmd/messages/send.go @@ -35,8 +35,12 @@ func newSendCmd() *cobra.Command { cmd := &cobra.Command{ Use: "send [text]", - Short: "Send a message to a channel", - Long: `Send a message to a channel. + Short: "Send a message to a channel or user", + Long: `Send a message to a channel or user. + +The destination can be a channel ID (C…), a channel name ("general"), a user ID +(U…) or a user handle ("@alice"). Passing a user opens a direct message, which +requires the im:write scope. By default, messages are sent using Slack Block Kit formatting for a more refined appearance. Use --simple to send plain text messages instead. @@ -101,7 +105,7 @@ The channel can also be specified via --channel instead of as a positional argum }, } - cmd.Flags().StringVar(&opts.channel, "channel", "", "Channel name or ID (alternative to positional argument)") + cmd.Flags().StringVar(&opts.channel, "channel", "", "Channel/user name or ID (alternative to positional argument)") cmd.Flags().StringVar(&opts.threadTS, "thread", "", "Thread timestamp for reply") cmd.Flags().StringVar(&opts.blocksJSON, "blocks", "", "Inline Block Kit JSON array (for simple blocks)") cmd.Flags().StringVar(&opts.blocksFile, "blocks-file", "", "Read blocks from JSON file (recommended for complex payloads)") From ca420282f7e0c0ff8a1c3d2827401506a3f1b9eb Mon Sep 17 00:00:00 2001 From: piekstra Date: Wed, 3 Jun 2026 08:42:42 -0400 Subject: [PATCH 2/4] docs: clarify ID-validation comment in channel resolver The else-if branch is only reached for non-digits, so 'or digit' was redundant. Reword to 'not an uppercase letter' in both IsChannelID and IsUserID. Addresses review feedback on #183. --- internal/client/channel_resolver.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/client/channel_resolver.go b/internal/client/channel_resolver.go index 1bdcbff..bb74714 100644 --- a/internal/client/channel_resolver.go +++ b/internal/client/channel_resolver.go @@ -78,7 +78,7 @@ func IsChannelID(s string) bool { if c >= '0' && c <= '9' { hasDigit = true } else if c < 'A' || c > 'Z' { - return false // not uppercase letter or digit + return false // not an uppercase letter } } return hasDigit @@ -104,7 +104,7 @@ func IsUserID(s string) bool { if c >= '0' && c <= '9' { hasDigit = true } else if c < 'A' || c > 'Z' { - return false // not uppercase letter or digit + return false // not an uppercase letter } } return hasDigit From f851d46a6548db84914337ef26d4016ff123d91e Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Thu, 4 Jun 2026 08:28:16 -0400 Subject: [PATCH 3/4] fix: keep DM opening out of channel resolver --- internal/client/channel_resolver.go | 45 +++--- internal/client/channel_resolver_test.go | 80 +++++++++-- internal/cmd/canvas/canvas_test.go | 33 +++++ internal/cmd/channels/channels_test.go | 28 ++++ internal/cmd/messages/delete.go | 2 +- internal/cmd/messages/history.go | 2 +- internal/cmd/messages/messages_test.go | 170 +++++++++++++++++++++++ internal/cmd/messages/permalink.go | 2 +- internal/cmd/messages/react.go | 2 +- internal/cmd/messages/send.go | 2 +- internal/cmd/messages/thread.go | 2 +- internal/cmd/messages/unreact.go | 2 +- internal/cmd/messages/update.go | 2 +- 13 files changed, 335 insertions(+), 37 deletions(-) diff --git a/internal/client/channel_resolver.go b/internal/client/channel_resolver.go index bb74714..e6668ff 100644 --- a/internal/client/channel_resolver.go +++ b/internal/client/channel_resolver.go @@ -9,30 +9,11 @@ import ( // act on. It accepts: // - channel IDs (C/G/D...) returned as-is // - channel names ("general", "#general") looked up via the Slack API -// - user IDs (U.../W...) a DM is opened and the IM channel ID returned -// - user handles ("@alice") resolved to a user ID, then a DM is opened -// -// Opening a DM uses conversations.open and requires the im:write scope. func (c *Client) ResolveChannel(channel string) (string, error) { if channel == "" { return "", fmt.Errorf("channel cannot be empty") } - // "@handle" -> resolve the username to a user ID, then open a DM. - if handle := strings.TrimPrefix(channel, "@"); handle != channel { - userID, err := c.resolveUserHandle(handle) - if err != nil { - return "", err - } - return c.OpenDM(userID) - } - - // A bare user ID (U.../W...) -> open a DM and return its IM channel ID. - // chat.postMessage rejects raw user IDs, so the DM must be opened first. - if IsUserID(channel) { - return c.OpenDM(channel) - } - // Strip leading # if present (common user mistake) channel = strings.TrimPrefix(channel, "#") @@ -49,6 +30,32 @@ func (c *Client) ResolveChannel(channel string) (string, error) { return c.lookupChannelByName(channel) } +// ResolveMessageDestination takes a message destination and returns a Slack +// conversation ID suitable for message APIs. User IDs and handles are resolved +// by opening a DM; channel-like inputs are delegated to ResolveChannel. +func (c *Client) ResolveMessageDestination(destination string) (string, error) { + if destination == "" { + return "", fmt.Errorf("channel cannot be empty") + } + + // "@handle" -> resolve the username to a user ID, then open a DM. + if handle := strings.TrimPrefix(destination, "@"); handle != destination { + userID, err := c.resolveUserHandle(handle) + if err != nil { + return "", err + } + return c.OpenDM(userID) + } + + // A bare user ID (U.../W...) -> open a DM and return its IM channel ID. + // chat.postMessage rejects raw user IDs, so the DM must be opened first. + if IsUserID(destination) { + return c.OpenDM(destination) + } + + return c.ResolveChannel(destination) +} + // IsChannelID returns true if the string looks like a Slack channel ID. // Channel IDs start with: // - C = public channel diff --git a/internal/client/channel_resolver_test.go b/internal/client/channel_resolver_test.go index 1e07dc1..3a43ce6 100644 --- a/internal/client/channel_resolver_test.go +++ b/internal/client/channel_resolver_test.go @@ -144,9 +144,37 @@ func TestIsUserID(t *testing.T) { } } -// TestResolveChannel_UserID covers issue #152: a bare user ID must open a DM -// (conversations.open) rather than being lowercased and rejected as a channel. -func TestResolveChannel_UserID(t *testing.T) { +func TestResolveChannel_UserIDDoesNotOpenDM(t *testing.T) { + var openedDM bool + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch r.URL.Path { + case "/conversations.open": + openedDM = true + t.Errorf("ResolveChannel should not open DMs") + case "/conversations.list": + _ = json.NewEncoder(w).Encode(map[string]interface{}{ + "ok": true, + "channels": []map[string]interface{}{}, + }) + default: + t.Errorf("unexpected path %s", r.URL.Path) + } + })) + defer server.Close() + + c := NewWithConfig(server.URL, "test-token", nil) + + _, err := c.ResolveChannel("U07D13N5AMV") + require.Error(t, err) + assert.False(t, openedDM) + assert.Contains(t, err.Error(), "channel 'u07d13n5amv' not found") +} + +// TestResolveMessageDestination_UserID covers issue #152: a bare user ID must +// open a DM (conversations.open) rather than being lowercased and rejected as a +// channel. +func TestResolveMessageDestination_UserID(t *testing.T) { var gotUsers string server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { assert.Equal(t, "/conversations.open", r.URL.Path) @@ -165,13 +193,39 @@ func TestResolveChannel_UserID(t *testing.T) { c := NewWithConfig(server.URL, "test-token", nil) - result, err := c.ResolveChannel("U07D13N5AMV") + result, err := c.ResolveMessageDestination("U07D13N5AMV") require.NoError(t, err) assert.Equal(t, "D0123456789", result) assert.Equal(t, "U07D13N5AMV", gotUsers, "user ID should be passed to conversations.open unchanged") } -func TestResolveChannel_Handle(t *testing.T) { +func TestResolveMessageDestination_EnterpriseGridUserID(t *testing.T) { + var gotUsers string + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, "/conversations.open", r.URL.Path) + var body struct { + Users string `json:"users"` + } + _ = json.NewDecoder(r.Body).Decode(&body) + gotUsers = body.Users + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]interface{}{ + "ok": true, + "channel": map[string]interface{}{"id": "D0123456789"}, + }) + })) + defer server.Close() + + c := NewWithConfig(server.URL, "test-token", nil) + + result, err := c.ResolveMessageDestination("W012ABC34") + require.NoError(t, err) + assert.Equal(t, "D0123456789", result) + assert.Equal(t, "W012ABC34", gotUsers, "Enterprise Grid user ID should be passed through unchanged") +} + +func TestResolveMessageDestination_Handle(t *testing.T) { + var gotOpenUsers string server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") switch r.URL.Path { @@ -184,6 +238,11 @@ func TestResolveChannel_Handle(t *testing.T) { }, }) case "/conversations.open": + var body struct { + Users string `json:"users"` + } + _ = json.NewDecoder(r.Body).Decode(&body) + gotOpenUsers = body.Users _ = json.NewEncoder(w).Encode(map[string]interface{}{ "ok": true, "channel": map[string]interface{}{"id": "D0000000001"}, @@ -196,12 +255,13 @@ func TestResolveChannel_Handle(t *testing.T) { c := NewWithConfig(server.URL, "test-token", nil) - result, err := c.ResolveChannel("@alice") + result, err := c.ResolveMessageDestination("@alice") require.NoError(t, err) assert.Equal(t, "D0000000001", result) + assert.Equal(t, "U111", gotOpenUsers) } -func TestResolveChannel_HandleNotFound(t *testing.T) { +func TestResolveMessageDestination_HandleNotFound(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { assert.Equal(t, "/users.list", r.URL.Path) w.Header().Set("Content-Type", "application/json") @@ -214,13 +274,13 @@ func TestResolveChannel_HandleNotFound(t *testing.T) { c := NewWithConfig(server.URL, "test-token", nil) - _, err := c.ResolveChannel("@nobody") + _, err := c.ResolveMessageDestination("@nobody") require.Error(t, err) assert.Contains(t, err.Error(), "not found") assert.Contains(t, err.Error(), "slck users search") } -func TestResolveChannel_HandleAmbiguous(t *testing.T) { +func TestResolveMessageDestination_HandleAmbiguous(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") _ = json.NewEncoder(w).Encode(map[string]interface{}{ @@ -235,7 +295,7 @@ func TestResolveChannel_HandleAmbiguous(t *testing.T) { c := NewWithConfig(server.URL, "test-token", nil) - _, err := c.ResolveChannel("@Sam") + _, err := c.ResolveMessageDestination("@Sam") require.Error(t, err) assert.Contains(t, err.Error(), "ambiguous") } diff --git a/internal/cmd/canvas/canvas_test.go b/internal/cmd/canvas/canvas_test.go index bde3629..07c95a7 100644 --- a/internal/cmd/canvas/canvas_test.go +++ b/internal/cmd/canvas/canvas_test.go @@ -136,6 +136,39 @@ func TestRunCreate_Channel_Success(t *testing.T) { } } +func TestRunCreate_ChannelUserIDDoesNotOpenDM(t *testing.T) { + openedDM := false + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch r.URL.Path { + case "/conversations.open": + openedDM = true + t.Errorf("canvas create --channel should not open DMs") + case "/conversations.list": + _ = json.NewEncoder(w).Encode(map[string]interface{}{ + "ok": true, + "channels": []map[string]interface{}{}, + }) + default: + t.Errorf("unexpected path %s", r.URL.Path) + } + })) + defer server.Close() + + c := client.NewWithConfig(server.URL, "test-token", nil) + opts := &createOptions{channel: "U07D13N5AMV", text: "# Channel Doc"} + err := runCreate(opts, c) + if err == nil { + t.Fatal("expected user ID channel lookup to fail") + } + if openedDM { + t.Fatal("unexpected conversations.open call") + } + if !strings.Contains(err.Error(), "channel 'u07d13n5amv' not found") { + t.Fatalf("expected channel not found error, got %v", err) + } +} + func TestRunEdit_NoContent(t *testing.T) { opts := &editOptions{} err := runEdit("F12345", opts, nil) diff --git a/internal/cmd/channels/channels_test.go b/internal/cmd/channels/channels_test.go index e3fe359..d32b8e7 100644 --- a/internal/cmd/channels/channels_test.go +++ b/internal/cmd/channels/channels_test.go @@ -131,6 +131,34 @@ func TestRunGet_NotFound(t *testing.T) { assert.Contains(t, err.Error(), "channel_not_found") } +func TestRunGet_UserIDDoesNotOpenDM(t *testing.T) { + var openedDM bool + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch r.URL.Path { + case "/conversations.open": + openedDM = true + t.Errorf("channels get should not open DMs") + case "/conversations.list": + _ = json.NewEncoder(w).Encode(map[string]interface{}{ + "ok": true, + "channels": []map[string]interface{}{}, + }) + default: + t.Errorf("unexpected path %s", r.URL.Path) + } + })) + defer server.Close() + + c := client.NewWithConfig(server.URL, "test-token", nil) + opts := &getOptions{} + + err := runGet("U07D13N5AMV", opts, c) + require.Error(t, err) + assert.False(t, openedDM) + assert.Contains(t, err.Error(), "channel 'u07d13n5amv' not found") +} + func TestRunCreate_Success(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { assert.Equal(t, "/conversations.create", r.URL.Path) diff --git a/internal/cmd/messages/delete.go b/internal/cmd/messages/delete.go index bbcc719..914d64f 100644 --- a/internal/cmd/messages/delete.go +++ b/internal/cmd/messages/delete.go @@ -71,7 +71,7 @@ func runDelete(channel, timestamp string, opts *deleteOptions, c *client.Client) } // Resolve channel name to ID if needed - channelID, err := c.ResolveChannel(channel) + channelID, err := c.ResolveMessageDestination(channel) if err != nil { return err } diff --git a/internal/cmd/messages/history.go b/internal/cmd/messages/history.go index 457b867..20e212b 100644 --- a/internal/cmd/messages/history.go +++ b/internal/cmd/messages/history.go @@ -42,7 +42,7 @@ func runHistory(channel string, opts *historyOptions, c *client.Client) error { } // Resolve channel name to ID if needed - channelID, err := c.ResolveChannel(channel) + channelID, err := c.ResolveMessageDestination(channel) if err != nil { return err } diff --git a/internal/cmd/messages/messages_test.go b/internal/cmd/messages/messages_test.go index 16a8c88..4e5cba4 100644 --- a/internal/cmd/messages/messages_test.go +++ b/internal/cmd/messages/messages_test.go @@ -316,6 +316,176 @@ func TestRunSend_Success(t *testing.T) { require.NoError(t, err) } +func TestRunSend_UserIDOpensDM(t *testing.T) { + var gotOpenUsers string + var gotPostChannel string + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch r.URL.Path { + case "/conversations.open": + var body struct { + Users string `json:"users"` + } + _ = json.NewDecoder(r.Body).Decode(&body) + gotOpenUsers = body.Users + _ = json.NewEncoder(w).Encode(map[string]interface{}{ + "ok": true, + "channel": map[string]interface{}{"id": "D0123456789"}, + }) + case "/chat.postMessage": + var body map[string]interface{} + _ = json.NewDecoder(r.Body).Decode(&body) + gotPostChannel, _ = body["channel"].(string) + assert.Equal(t, "Hello World", body["text"]) + _ = json.NewEncoder(w).Encode(map[string]interface{}{ + "ok": true, + "ts": "1234567890.123456", + "channel": "D0123456789", + }) + default: + t.Errorf("unexpected path %s", r.URL.Path) + } + })) + defer server.Close() + + c := client.NewWithConfig(server.URL, "test-token", nil) + opts := &sendOptions{simple: true} + + err := runSend("U07D13N5AMV", "Hello World", opts, c) + require.NoError(t, err) + assert.Equal(t, "U07D13N5AMV", gotOpenUsers) + assert.Equal(t, "D0123456789", gotPostChannel) +} + +func TestMessageCommands_UserIDDestinationsOpenDM(t *testing.T) { + const ( + userID = "U07D13N5AMV" + dmID = "D0123456789" + ts = "1234567890.123456" + ) + + postChannel := func(t *testing.T, r *http.Request) string { + t.Helper() + var body map[string]interface{} + _ = json.NewDecoder(r.Body).Decode(&body) + channel, _ := body["channel"].(string) + return channel + } + + tests := []struct { + name string + apiPath string + run func(*client.Client) error + channel func(*testing.T, *http.Request) string + response map[string]interface{} + }{ + { + name: "history", + apiPath: "/conversations.history", + run: func(c *client.Client) error { + return runHistory(userID, &historyOptions{limit: 1}, c) + }, + channel: func(t *testing.T, r *http.Request) string { + return r.URL.Query().Get("channel") + }, + response: map[string]interface{}{"ok": true, "messages": []map[string]interface{}{}}, + }, + { + name: "thread", + apiPath: "/conversations.replies", + run: func(c *client.Client) error { + return runThread(userID, ts, &threadOptions{limit: 1}, c) + }, + channel: func(t *testing.T, r *http.Request) string { + return r.URL.Query().Get("channel") + }, + response: map[string]interface{}{"ok": true, "messages": []map[string]interface{}{}}, + }, + { + name: "permalink", + apiPath: "/chat.getPermalink", + run: func(c *client.Client) error { + return runPermalink(userID, ts, &permalinkOptions{}, c) + }, + channel: func(t *testing.T, r *http.Request) string { + return r.URL.Query().Get("channel") + }, + response: map[string]interface{}{"ok": true, "permalink": "https://example.slack.com/archives/D0123456789/p1234567890123456"}, + }, + { + name: "react", + apiPath: "/reactions.add", + run: func(c *client.Client) error { + return runReact(userID, ts, "thumbsup", &reactOptions{}, c) + }, + channel: postChannel, + response: map[string]interface{}{"ok": true}, + }, + { + name: "unreact", + apiPath: "/reactions.remove", + run: func(c *client.Client) error { + return runUnreact(userID, ts, "thumbsup", &unreactOptions{}, c) + }, + channel: postChannel, + response: map[string]interface{}{"ok": true}, + }, + { + name: "update", + apiPath: "/chat.update", + run: func(c *client.Client) error { + return runUpdate(userID, ts, "Updated", &updateOptions{simple: true}, c) + }, + channel: postChannel, + response: map[string]interface{}{"ok": true}, + }, + { + name: "delete", + apiPath: "/chat.delete", + run: func(c *client.Client) error { + return runDelete(userID, ts, &deleteOptions{force: true}, c) + }, + channel: postChannel, + response: map[string]interface{}{"ok": true}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var gotOpenUsers string + apiCalled := false + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch r.URL.Path { + case "/conversations.open": + var body struct { + Users string `json:"users"` + } + _ = json.NewDecoder(r.Body).Decode(&body) + gotOpenUsers = body.Users + _ = json.NewEncoder(w).Encode(map[string]interface{}{ + "ok": true, + "channel": map[string]interface{}{"id": dmID}, + }) + case tt.apiPath: + apiCalled = true + assert.Equal(t, dmID, tt.channel(t, r)) + _ = json.NewEncoder(w).Encode(tt.response) + default: + t.Errorf("unexpected path %s", r.URL.Path) + } + })) + defer server.Close() + + c := client.NewWithConfig(server.URL, "test-token", nil) + + require.NoError(t, tt.run(c)) + assert.Equal(t, userID, gotOpenUsers) + assert.True(t, apiCalled) + }) + } +} + func TestRunSend_WithThread(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { var body map[string]interface{} diff --git a/internal/cmd/messages/permalink.go b/internal/cmd/messages/permalink.go index 5b9f045..edc2c68 100644 --- a/internal/cmd/messages/permalink.go +++ b/internal/cmd/messages/permalink.go @@ -44,7 +44,7 @@ func runPermalink(channel, timestamp string, _ *permalinkOptions, c *client.Clie } } - channelID, err := c.ResolveChannel(channel) + channelID, err := c.ResolveMessageDestination(channel) if err != nil { return err } diff --git a/internal/cmd/messages/react.go b/internal/cmd/messages/react.go index 707371a..d969b1b 100644 --- a/internal/cmd/messages/react.go +++ b/internal/cmd/messages/react.go @@ -43,7 +43,7 @@ func runReact(channel, timestamp, emoji string, opts *reactOptions, c *client.Cl } // Resolve channel name to ID if needed - channelID, err := c.ResolveChannel(channel) + channelID, err := c.ResolveMessageDestination(channel) if err != nil { return err } diff --git a/internal/cmd/messages/send.go b/internal/cmd/messages/send.go index 4c7f192..0f68630 100644 --- a/internal/cmd/messages/send.go +++ b/internal/cmd/messages/send.go @@ -221,7 +221,7 @@ func runSend(channel, text string, opts *sendOptions, c *client.Client) error { } // Resolve channel name to ID if needed - channelID, err := c.ResolveChannel(channel) + channelID, err := c.ResolveMessageDestination(channel) if err != nil { return err } diff --git a/internal/cmd/messages/thread.go b/internal/cmd/messages/thread.go index 9166473..f27387b 100644 --- a/internal/cmd/messages/thread.go +++ b/internal/cmd/messages/thread.go @@ -46,7 +46,7 @@ func runThread(channel, threadTS string, opts *threadOptions, c *client.Client) } // Resolve channel name to ID if needed - channelID, err := c.ResolveChannel(channel) + channelID, err := c.ResolveMessageDestination(channel) if err != nil { return err } diff --git a/internal/cmd/messages/unreact.go b/internal/cmd/messages/unreact.go index 40e7c5f..2ff9bdf 100644 --- a/internal/cmd/messages/unreact.go +++ b/internal/cmd/messages/unreact.go @@ -43,7 +43,7 @@ func runUnreact(channel, timestamp, emoji string, opts *unreactOptions, c *clien } // Resolve channel name to ID if needed - channelID, err := c.ResolveChannel(channel) + channelID, err := c.ResolveMessageDestination(channel) if err != nil { return err } diff --git a/internal/cmd/messages/update.go b/internal/cmd/messages/update.go index 3b3e088..7dc5d9f 100644 --- a/internal/cmd/messages/update.go +++ b/internal/cmd/messages/update.go @@ -56,7 +56,7 @@ func runUpdate(channel, timestamp, text string, opts *updateOptions, c *client.C } // Resolve channel name to ID if needed - channelID, err := c.ResolveChannel(channel) + channelID, err := c.ResolveMessageDestination(channel) if err != nil { return err } From 88ee61e95a5aa3053086df7f41c8d80c43a7cac5 Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Thu, 4 Jun 2026 08:30:03 -0400 Subject: [PATCH 4/4] fix: resolve handles across all user pages --- internal/client/channel_resolver.go | 2 +- internal/client/channel_resolver_test.go | 71 +++++++++++++++++++++++- internal/client/client.go | 38 +++++++++++++ internal/client/client_test.go | 49 ++++++++++++++++ 4 files changed, 157 insertions(+), 3 deletions(-) diff --git a/internal/client/channel_resolver.go b/internal/client/channel_resolver.go index e6668ff..5d72bb6 100644 --- a/internal/client/channel_resolver.go +++ b/internal/client/channel_resolver.go @@ -127,7 +127,7 @@ func (c *Client) resolveUserHandle(handle string) (string, error) { return "", fmt.Errorf("user handle cannot be empty") } - users, err := c.ListUsers(1000) + users, err := c.ListAllUsers() if err != nil { return "", fmt.Errorf("failed to list users: %w", err) } diff --git a/internal/client/channel_resolver_test.go b/internal/client/channel_resolver_test.go index 3a43ce6..f3c6953 100644 --- a/internal/client/channel_resolver_test.go +++ b/internal/client/channel_resolver_test.go @@ -261,6 +261,58 @@ func TestResolveMessageDestination_Handle(t *testing.T) { assert.Equal(t, "U111", gotOpenUsers) } +func TestResolveMessageDestination_HandleFoundAfterFirstPage(t *testing.T) { + userListCalls := 0 + var gotOpenUsers string + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch r.URL.Path { + case "/users.list": + userListCalls++ + if userListCalls == 1 { + assert.Empty(t, r.URL.Query().Get("cursor")) + _ = json.NewEncoder(w).Encode(map[string]interface{}{ + "ok": true, + "members": []map[string]interface{}{ + {"id": "U111", "name": "bob"}, + }, + "response_metadata": map[string]string{"next_cursor": "cursor456"}, + }) + return + } + assert.Equal(t, "cursor456", r.URL.Query().Get("cursor")) + _ = json.NewEncoder(w).Encode(map[string]interface{}{ + "ok": true, + "members": []map[string]interface{}{ + {"id": "U222", "name": "alice"}, + }, + "response_metadata": map[string]string{"next_cursor": ""}, + }) + case "/conversations.open": + var body struct { + Users string `json:"users"` + } + _ = json.NewDecoder(r.Body).Decode(&body) + gotOpenUsers = body.Users + _ = json.NewEncoder(w).Encode(map[string]interface{}{ + "ok": true, + "channel": map[string]interface{}{"id": "D0000000002"}, + }) + default: + t.Errorf("unexpected path %s", r.URL.Path) + } + })) + defer server.Close() + + c := NewWithConfig(server.URL, "test-token", nil) + + result, err := c.ResolveMessageDestination("@alice") + require.NoError(t, err) + assert.Equal(t, "D0000000002", result) + assert.Equal(t, 2, userListCalls) + assert.Equal(t, "U222", gotOpenUsers) +} + func TestResolveMessageDestination_HandleNotFound(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { assert.Equal(t, "/users.list", r.URL.Path) @@ -280,15 +332,29 @@ func TestResolveMessageDestination_HandleNotFound(t *testing.T) { assert.Contains(t, err.Error(), "slck users search") } -func TestResolveMessageDestination_HandleAmbiguous(t *testing.T) { +func TestResolveMessageDestination_HandleAmbiguousAcrossPages(t *testing.T) { + userListCalls := 0 server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") + assert.Equal(t, "/users.list", r.URL.Path) + userListCalls++ + if userListCalls == 1 { + _ = json.NewEncoder(w).Encode(map[string]interface{}{ + "ok": true, + "members": []map[string]interface{}{ + {"id": "U111", "profile": map[string]interface{}{"display_name": "Sam"}}, + }, + "response_metadata": map[string]string{"next_cursor": "cursor456"}, + }) + return + } + assert.Equal(t, "cursor456", r.URL.Query().Get("cursor")) _ = json.NewEncoder(w).Encode(map[string]interface{}{ "ok": true, "members": []map[string]interface{}{ - {"id": "U111", "profile": map[string]interface{}{"display_name": "Sam"}}, {"id": "U222", "profile": map[string]interface{}{"display_name": "Sam"}}, }, + "response_metadata": map[string]string{"next_cursor": ""}, }) })) defer server.Close() @@ -298,4 +364,5 @@ func TestResolveMessageDestination_HandleAmbiguous(t *testing.T) { _, err := c.ResolveMessageDestination("@Sam") require.Error(t, err) assert.Contains(t, err.Error(), "ambiguous") + assert.Equal(t, 2, userListCalls) } diff --git a/internal/client/client.go b/internal/client/client.go index a9992c0..c49433e 100644 --- a/internal/client/client.go +++ b/internal/client/client.go @@ -471,6 +471,44 @@ func (c *Client) ListUsers(limit int) ([]User, error) { return allUsers, nil } +// ListAllUsers returns all users, handling Slack cursor pagination. +func (c *Client) ListAllUsers() ([]User, error) { + var allUsers []User + cursor := "" + + for { + params := url.Values{} + params.Set("limit", "200") + if cursor != "" { + params.Set("cursor", cursor) + } + + body, err := c.get("users.list", params) + if err != nil { + return nil, err + } + + var result struct { + Members []User `json:"members"` + ResponseMetadata struct { + NextCursor string `json:"next_cursor"` + } `json:"response_metadata"` + } + if err := json.Unmarshal(body, &result); err != nil { + return nil, err + } + + allUsers = append(allUsers, result.Members...) + + if result.ResponseMetadata.NextCursor == "" { + break + } + cursor = result.ResponseMetadata.NextCursor + } + + return allUsers, nil +} + // GetUserInfo returns user details func (c *Client) GetUserInfo(userID string) (*User, error) { params := url.Values{} diff --git a/internal/client/client_test.go b/internal/client/client_test.go index 679df2d..6c62385 100644 --- a/internal/client/client_test.go +++ b/internal/client/client_test.go @@ -1100,6 +1100,55 @@ func TestClient_ListUsers_PaginationWithLimit(t *testing.T) { } } +func TestClient_ListAllUsers_Pagination(t *testing.T) { + callCount := 0 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + callCount++ + var resp map[string]interface{} + + if callCount == 1 { + if r.URL.Query().Get("cursor") != "" { + t.Errorf("expected no cursor on first request, got %s", r.URL.Query().Get("cursor")) + } + resp = map[string]interface{}{ + "ok": true, + "members": []map[string]interface{}{ + {"id": "U1", "name": "user1"}, + {"id": "U2", "name": "user2"}, + }, + "response_metadata": map[string]string{"next_cursor": "cursor456"}, + } + } else { + if r.URL.Query().Get("cursor") != "cursor456" { + t.Errorf("expected cursor=cursor456, got %s", r.URL.Query().Get("cursor")) + } + resp = map[string]interface{}{ + "ok": true, + "members": []map[string]interface{}{ + {"id": "U3", "name": "user3"}, + }, + "response_metadata": map[string]string{"next_cursor": ""}, + } + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(resp) + })) + defer server.Close() + + client := NewWithConfig(server.URL, "test-token", nil) + users, err := client.ListAllUsers() + + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if callCount != 2 { + t.Errorf("expected 2 API calls, got %d", callCount) + } + if len(users) != 3 { + t.Errorf("expected 3 users, got %d", len(users)) + } +} + // --- Search Tests --- func TestClient_SearchMessages_Success(t *testing.T) {