fix: send DMs by user ID or @handle by piekstra · Pull Request #183 · open-cli-collective/slack-chat-api

piekstra · 2026-06-03T12:32:39Z

Summary

slck messages send <userID> "..." cannot DM a user - it fails with:

Error: channel 'u07d13n5amv' not found. Use 'slck channels list' to see available channels

Root cause: ResolveChannel only treated C/G/D prefixes as conversation IDs. A user ID starts with U or W, so it fell through to channel-name lookup, was lowercased, and failed. DMs were never opened because the client had no conversations.open wrapper.

Fix

Add Client.OpenDM wrapping conversations.open (idempotent; requires im:write).
Add IsUserID plus ResolveMessageDestination for message APIs:
- bare user IDs (U.../W...) open a DM and return the IM channel ID,
- @handle resolves username first, then display name, then opens a DM,
- channel IDs/names continue through channel-only ResolveChannel.
Keep ResolveChannel channel-only/read-oriented so channel/canvas commands do not call conversations.open.
Page through all users for @handle resolution, while preserving the existing ListUsers(limit) behavior used by users list/search.
Docs: README example + send help text.

Verification

TestResolveMessageDestination_UserID - user ID opens a DM and preserves casing.
TestResolveChannel_UserIDDoesNotOpenDM - channel resolver does not open DMs.
TestRunSend_UserIDOpensDM - send path posts to the opened DM channel.
TestRunGet_UserIDDoesNotOpenDM / TestRunCreate_ChannelUserIDDoesNotOpenDM - channel/canvas surfaces stay channel-only.
TestResolveMessageDestination_HandleFoundAfterFirstPage / TestResolveMessageDestination_HandleAmbiguousAcrossPages - handle lookup uses all pages.
TestClient_ListAllUsers_Pagination - full pagination forwards Slack cursors.
make test
make lint

Fixes #152

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.

rianjs

I would not merge this as-is. The core DM regression is fixed, but two design/coverage issues should be addressed first.

Resolver boundary / side effects: ResolveChannel now opens DMs for U..., W..., and @handle inputs. That turns a generic resolver from a read-only channel lookup helper into an action that can initialize Slack state and requires im:write. Because this helper is also used by channel/canvas commands (channels get, channels archive, channels invite, canvas create --channel, etc.), user-shaped input can now call conversations.open before a channel-only operation. I would split this into separate responsibilities: keep ResolveChannel channel-only/read-oriented, add a user resolver for U/W/@handle, and use a message-destination resolver that explicitly opens DMs only from the messages surface.
Handle lookup scale gap: resolveUserHandle calls ListUsers(1000), so larger workspaces can falsely report @handle as not found, or miss display-name ambiguity after the first 1000 users. Bare user IDs are fine, but if @handle is advertised as supported, this should either page through the full user list or clearly document/enforce the limit.

Coverage is good for the direct issue (U... opens a DM, ID casing is preserved, handle success/not-found/ambiguous paths), but it is not adequate for the full blast radius until there are tests proving channel/canvas commands do not accidentally open DMs or require im:write.

rianjs · 2026-06-04T12:32:37Z

Minor

PR body is stale after the architectural fix. The “Fix” section still says ResolveChannel opens DMs for U/W IDs and @handle, and that shared channel resolution makes all message commands work against DMs. The final code correctly moved that behavior to ResolveMessageDestination, so update the PR description before merge to avoid documenting the rejected design.

Verification note: I could not independently run the focused Go tests in this sandbox because httptest cannot bind localhost ports here (operation not permitted). Static review of the final diff found no blocker/major code issues.

rianjs · 2026-06-04T12:36:29Z

Findings

Major: Only send has end-to-end coverage for the new DM flow (internal/cmd/messages/messages_test.go#L319), but the same resolver swap was made in the other message commands too. A wiring regression in delete, history, permalink, react, thread, unreact, or update would still pass the suite because none of those paths are exercised with a user ID or @handle.
Minor: The basic @alice resolver test (internal/client/channel_resolver_test.go#L202) is too permissive. It never checks the users payload sent to /conversations.open, so it would still pass if the lookup picked the wrong user ID and the mock returned the same fixed channel.
Minor: The end-to-end DM-open coverage only uses a U... ID (internal/client/channel_resolver_test.go#L177); W... is only checked by the IsUserID predicate (internal/client/channel_resolver_test.go#L124). If the new DM path mishandles Enterprise Grid user IDs, the current suite would not catch it.

monit-reviewer

Automated PR Review

Reviewed commit: 5de3cb5

Approved with 6 non-blocking suggestions below. Address at your discretion.

Summary

Reviewer	Findings
harness-engineering:harness-architecture-reviewer	1
harness-engineering:harness-enforcement-reviewer	2
harness-engineering:harness-knowledge-reviewer	3

harness-engineering:harness-architecture-reviewer (1 findings)

💡 Suggestion - internal/client/client.go:475

ListAllUsers duplicates the cursor-pagination loop already present in ListUsers — both build params, call users.list, unmarshal the response, and follow next_cursor in an identical pattern. A shared private helper (e.g. listUsersPage) would eliminate the duplication and ensure any future changes to pagination behavior (retries, timeouts, per-page limits) only need to be made in one place.

harness-engineering:harness-enforcement-reviewer (2 findings)

💡 Suggestion - internal/cmd/messages/history.go:45

history, thread, and permalink are read-oriented commands that now call conversations.open (a write operation) whenever a user ID or @handle is supplied. This has two undocumented consequences: (1) it silently creates or resurfaces a DM channel in the caller's Slack sidebar as a side effect of a read command, and (2) it requires the im:write scope, so users with read-only tokens will get an auth failure with no explanation. The same concern applies to thread.go and permalink.go. Consider noting this behavior in each command's Long description and adding im:write to the documented scope requirements.

💡 Suggestion - internal/client/channel_resolver.go:136

resolveUserHandle always fetches every page of users before returning, even when a unique match is found on page 1. For large workspaces this is O(users/200) blocking API calls per @handle resolution. Early termination once ≥2 matches are accumulated would preserve the ambiguity-detection behavior while avoiding unnecessary pagination.

harness-engineering:harness-knowledge-reviewer (3 findings)

💡 Suggestion - internal/client/channel_resolver_test.go:249

The DisplayName single-match success path in resolveUserHandle has no test coverage. TestResolveMessageDestination_HandleAmbiguousAcrossPages covers the ambiguous display_name case and TestResolveMessageDestination_Handle covers the Name match, but a single unique DisplayName match (the byDisplay happy path) is never exercised. A regression in that branch would go undetected.

💡 Suggestion - internal/client/client.go:499

ListAllUsers does not check Slack's application-level ok: false in the users.list response. If the call fails due to a missing scope or revoked token, the function may return an empty slice with no error, causing resolveUserHandle to emit a misleading 'user not found' error instead of surfacing the actual Slack error (e.g. 'missing_scope'). Verify whether c.get already propagates ok: false; if not, add the check here.

💡 Suggestion - internal/client/client.go:551

OpenDM does not check Slack's application-level ok: false. If conversations.open returns {"ok": false, "error": "missing_scope"} (im:write not granted), the code falls through to the empty-channel-ID guard and returns the generic error 'conversations.open returned no channel id for user U...' rather than the actionable Slack error reason. Verify whether c.post already propagates ok: false; if not, add the check here so users get a clear permission error instead of an opaque message.

4 info-level observations excluded. Run with --verbose to include.

2 PR discussion threads considered.

Completed in 5m 48s | $1.51 | sonnet | daemon 0.2.121 | Glorfindel

Field	Value
Model	sonnet
Reviewers	hybrid-synthesis, documentation:docs-reviewer, harness-engineering:harness-architecture-reviewer, harness-engineering:harness-enforcement-reviewer, harness-engineering:harness-knowledge-reviewer, harness-engineering:harness-self-documenting-code-reviewer, security:security-code-auditor
Engine	claude · sonnet
Reviewed by	pr-review-daemon · monit-pr-reviewer
Duration	5m 48s wall · 15m 19s compute (Reviewers: 4m 07s · Synthesis: 1m 38s)
Cost	$1.51
Tokens	458.0k in / 58.8k out
Turns	17

Per-workstream usage

Workstream	Model	In	Out	Cache read	Cache create	Cost
hybrid-synthesis	sonnet	31.1k	6.3k	17.4k	13.7k (1h)	$0.16
documentation:docs-reviewer	haiku	14.5k	3.7k	6.0k	8.5k (1h)	$0.03
harness-engineering:harness-architecture-reviewer	sonnet	88.9k	13.0k	60.5k	28.4k (1h)	$0.33
harness-engineering:harness-enforcement-reviewer	sonnet	83.0k	6.8k	37.1k	45.9k (1h)	$0.30
harness-engineering:harness-knowledge-reviewer	sonnet	162.5k	14.3k	131.7k	30.8k (1h)	$0.38
harness-engineering:harness-self-documenting-code-reviewer	sonnet	53.8k	7.0k	27.8k	26.0k (1h)	$0.23
security:security-code-auditor	haiku	24.1k	7.8k	4.2k	20.0k (1h)	$0.08

_{Re-reviews only run when @monit-reviewer is re-requested as a reviewer — push as many commits as you need, then re-request when ready. PRs targeting branches other than main, master are skipped, even when @monit-reviewer is re-requested.}

monit-reviewer · 2026-06-04T12:46:24Z


 	// Resolve channel name to ID if needed
-	channelID, err := c.ResolveChannel(channel)
+	channelID, err := c.ResolveMessageDestination(channel)


🔵 Low (harness-engineering:harness-enforcement-reviewer): history, thread, and permalink are read-oriented commands that now call conversations.open (a write operation) whenever a user ID or @handle is supplied. This has two undocumented consequences: (1) it silently creates or resurfaces a DM channel in the caller's Slack sidebar as a side effect of a read command, and (2) it requires the im:write scope, so users with read-only tokens will get an auth failure with no explanation. The same concern applies to thread.go and permalink.go. Consider noting this behavior in each command's Long description and adding im:write to the documented scope requirements.

Reply to this thread when addressed.

monit-reviewer · 2026-06-04T12:46:24Z

+	}
+
+	var byName, byDisplay []User
+	for _, u := range users {


🔵 Low (harness-engineering:harness-enforcement-reviewer): resolveUserHandle always fetches every page of users before returning, even when a unique match is found on page 1. For large workspaces this is O(users/200) blocking API calls per @handle resolution. Early termination once ≥2 matches are accumulated would preserve the ambiguity-detection behavior while avoiding unnecessary pagination.

Reply to this thread when addressed.

monit-reviewer · 2026-06-04T12:46:24Z

 }

+// ListAllUsers returns all users, handling Slack cursor pagination.
+func (c *Client) ListAllUsers() ([]User, error) {


🔵 Low (harness-engineering:harness-architecture-reviewer): ListAllUsers duplicates the cursor-pagination loop already present in ListUsers — both build params, call users.list, unmarshal the response, and follow next_cursor in an identical pattern. A shared private helper (e.g. listUsersPage) would eliminate the duplication and ensure any future changes to pagination behavior (retries, timeouts, per-page limits) only need to be made in one place.

Reply to this thread when addressed.

monit-reviewer · 2026-06-04T12:46:24Z

+			_ = json.NewEncoder(w).Encode(map[string]interface{}{
+				"ok":      true,
+				"channel": map[string]interface{}{"id": "D0000000001"},
+			})


🔵 Low (harness-engineering:harness-knowledge-reviewer): The DisplayName single-match success path in resolveUserHandle has no test coverage. TestResolveMessageDestination_HandleAmbiguousAcrossPages covers the ambiguous display_name case and TestResolveMessageDestination_Handle covers the Name match, but a single unique DisplayName match (the byDisplay happy path) is never exercised. A regression in that branch would go undetected.

Reply to this thread when addressed.

monit-reviewer · 2026-06-04T12:46:24Z

+		}
+		if err := json.Unmarshal(body, &result); err != nil {
+			return nil, err
+		}


🔵 Low (harness-engineering:harness-knowledge-reviewer): ListAllUsers does not check Slack's application-level ok: false in the users.list response. If the call fails due to a missing scope or revoked token, the function may return an empty slice with no error, causing resolveUserHandle to emit a misleading 'user not found' error instead of surfacing the actual Slack error (e.g. 'missing_scope'). Verify whether c.get already propagates ok: false; if not, add the check here.

Reply to this thread when addressed.

monit-reviewer · 2026-06-04T12:46:24Z

+	}
+	if err := json.Unmarshal(body, &result); err != nil {
+		return "", err
+	}


🔵 Low (harness-engineering:harness-knowledge-reviewer): OpenDM does not check Slack's application-level ok: false. If conversations.open returns {"ok": false, "error": "missing_scope"} (im:write not granted), the code falls through to the empty-channel-ID guard and returns the generic error 'conversations.open returned no channel id for user U...' rather than the actionable Slack error reason. Verify whether c.post already propagates ok: false; if not, add the check here so users get a clear permission error instead of an opaque message.

Reply to this thread when addressed.

rianjs

The requested resolver-boundary and handle-pagination changes are addressed. Local checks, CI, daemon approval, TDD follow-up, and final architect pass are clean.

@Handles

slck messages send <userID> 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

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.

rianjs requested changes Jun 4, 2026

View reviewed changes

rianjs mentioned this pull request Jun 4, 2026

Unable to send DMs with bot token #152

Closed

rianjs force-pushed the fix/dm-by-user-id-152 branch from 2fb65dd to 5de3cb5 Compare June 4, 2026 12:40

monit-reviewer approved these changes Jun 4, 2026

View reviewed changes

rianjs approved these changes Jun 4, 2026

View reviewed changes

piekstra and others added 4 commits June 4, 2026 08:51

docs: clarify ID-validation comment in channel resolver

ca42028

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.

fix: keep DM opening out of channel resolver

f851d46

fix: resolve handles across all user pages

88ee61e

rianjs force-pushed the fix/dm-by-user-id-152 branch from 5de3cb5 to 88ee61e Compare June 4, 2026 12:51

rianjs merged commit edf1aa0 into main Jun 4, 2026
9 checks passed

rianjs deleted the fix/dm-by-user-id-152 branch June 4, 2026 12:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: send DMs by user ID or @handle#183

fix: send DMs by user ID or @handle#183
rianjs merged 4 commits into
mainfrom
fix/dm-by-user-id-152

piekstra commented Jun 3, 2026 •

edited by rianjs

Loading

Uh oh!

rianjs left a comment

Uh oh!

rianjs commented Jun 4, 2026

Uh oh!

rianjs commented Jun 4, 2026

Uh oh!

monit-reviewer left a comment

Uh oh!

monit-reviewer Jun 4, 2026

Uh oh!

monit-reviewer Jun 4, 2026

Uh oh!

monit-reviewer Jun 4, 2026

Uh oh!

monit-reviewer Jun 4, 2026

Uh oh!

monit-reviewer Jun 4, 2026

Uh oh!

monit-reviewer Jun 4, 2026

Uh oh!

rianjs left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

piekstra commented Jun 3, 2026 • edited by rianjs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Fix

Verification

Uh oh!

rianjs left a comment

Choose a reason for hiding this comment

Uh oh!

rianjs commented Jun 4, 2026

Uh oh!

rianjs commented Jun 4, 2026

Uh oh!

monit-reviewer left a comment

Choose a reason for hiding this comment

Automated PR Review

Summary

Uh oh!

monit-reviewer Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

monit-reviewer Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

monit-reviewer Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

monit-reviewer Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

monit-reviewer Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

monit-reviewer Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

rianjs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

piekstra commented Jun 3, 2026 •

edited by rianjs

Loading