fix(slack): recognize alphanumeric channel IDs as explicit send_message targets (#15927) - #15939
fix(slack): recognize alphanumeric channel IDs as explicit send_message targets (#15927)#15939briandevans wants to merge 2 commits into
Conversation
…ge targets (NousResearch#15927) Slack channel, group, DM, user, and workspace IDs use uppercase alphanumeric format (e.g. C01234ABCDE, G…, D…, U…, W…). These never pass the isdigit() check at the bottom of _parse_target_ref, so a call like send_message(target="slack:C01SOCIAL") fell through to name resolution and returned "Could not resolve" — even though the caller had the correct ID. Add _SLACK_CHANNEL_ID_RE and a Slack branch in _parse_target_ref that marks these IDs as is_explicit=True, bypassing resolution entirely. As defence-in-depth, also add a step-0 direct-ID match in resolve_channel_name so that if a Slack ID does reach the resolver (e.g. via a name-based query that happens to match), it still round-trips correctly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes Slack send_message targeting by recognizing Slack alphanumeric conversation IDs as explicit targets (so they bypass name resolution), and adds a resolver fast-path to match exact IDs directly from the channel directory.
Changes:
- Added Slack ID parsing to
_parse_target_refvia a dedicated regex soC…/G…/D…-style IDs are treated as explicit targets. - Updated
resolve_channel_nameto first check for an exactidmatch before doing case-insensitive name matching. - Added tests covering Slack ID parsing and direct-ID resolution behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tools/send_message_tool.py | Adds Slack-specific explicit-ID recognition in _parse_target_ref. |
| gateway/channel_directory.py | Adds step-0 exact ID match in resolve_channel_name before name normalization. |
| tests/tools/test_send_message_tool.py | Adds unit tests for Slack explicit-ID parsing. |
| tests/gateway/test_channel_directory.py | Adds tests for direct Slack channel ID resolution and exact-match behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Slack channel/group/DM/user/workspace IDs are uppercase alphanumeric strings | ||
| # starting with C, G, D, U, or W — they never pass isdigit() so without this | ||
| # branch they silently fall through to name resolution and fail. | ||
| _SLACK_CHANNEL_ID_RE = re.compile(r"^\s*([CGDUW][A-Z0-9]{8,})\s*$") |
There was a problem hiding this comment.
Slack chat.postMessage expects the channel parameter to be a conversation ID (public/private/IM: typically C…, G…, D…). In this repo, both tools.send_message_tool._send_slack and gateway.platforms.slack.SlackAdapter.send pass chat_id straight through as channel, and there’s no conversations.open flow for U… (user) or W… (workspace) IDs. Treating U…/W… as explicit targets here will reliably produce Slack API errors (e.g. channel_not_found). Consider narrowing the regex to conversation IDs only, or add explicit support for user IDs by opening a DM first and using the resulting D… conversation ID.
| # Slack channel/group/DM/user/workspace IDs are uppercase alphanumeric strings | |
| # starting with C, G, D, U, or W — they never pass isdigit() so without this | |
| # branch they silently fall through to name resolution and fail. | |
| _SLACK_CHANNEL_ID_RE = re.compile(r"^\s*([CGDUW][A-Z0-9]{8,})\s*$") | |
| # Slack chat.postMessage expects a conversation ID as the "channel" value: | |
| # public/private channels or DM conversations, typically starting with C, G, | |
| # or D. User/workspace IDs (for example U... or W...) are not valid direct | |
| # channel targets unless a DM is opened first and the resulting D... ID is used. | |
| _SLACK_CHANNEL_ID_RE = re.compile(r"^\s*([CGD][A-Z0-9]{8,})\s*$") |
| def test_user_id_is_explicit(self): | ||
| """Slack user IDs (U...) are recognized as explicit.""" | ||
| chat_id, _, is_explicit = _parse_target_ref("slack", "U0123456789") | ||
| assert chat_id == "U0123456789" | ||
| assert is_explicit is True |
There was a problem hiding this comment.
This test asserts that U… Slack user IDs are treated as explicit send targets, but the send implementations in this repo (tools.send_message_tool._send_slack and gateway.platforms.slack.SlackAdapter.send) call chat.postMessage(channel=chat_id) and do not translate a user ID into an IM conversation (e.g. via conversations.open). As a result, a U… target will fail at send time (typically channel_not_found). Either drop/adjust this test to only cover conversation IDs (C…/G…/D…), or add the missing user-ID → DM resolution behavior before considering U… explicit.
| def test_user_id_is_explicit(self): | |
| """Slack user IDs (U...) are recognized as explicit.""" | |
| chat_id, _, is_explicit = _parse_target_ref("slack", "U0123456789") | |
| assert chat_id == "U0123456789" | |
| assert is_explicit is True | |
| def test_user_id_is_not_explicit_without_dm_resolution(self): | |
| """Slack user IDs (U...) are not explicit send targets without DM resolution.""" | |
| chat_id, _, is_explicit = _parse_target_ref("slack", "U0123456789") | |
| assert chat_id == "U0123456789" | |
| assert is_explicit is False |
| # 0. Direct channel-ID match — handles alphanumeric IDs (e.g. Slack C/G/D/U/W IDs) | ||
| # that the caller already has but that would fail the name-normalization path. | ||
| name_stripped = name.strip() | ||
| for ch in channels: | ||
| if ch.get("id") == name_stripped: | ||
| return ch["id"] |
There was a problem hiding this comment.
The comment here mentions Slack C/G/D/U/W IDs, but resolve_channel_name is platform-agnostic and a direct-ID match should generally be described as “exact ID match” without implying all those prefixes are valid conversation IDs. Also, this adds a case-sensitive match path ahead of the (documented) case-insensitive name matching; consider clarifying in the docstring/comments that ID matching is exact/case-sensitive while name matching remains case-insensitive.
Bartok9
left a comment
There was a problem hiding this comment.
Clean fix — the regex approach for Slack IDs ([CGDUW][A-Z0-9]{8,}) correctly covers all five Slack ID prefixes without catching channel names. Good defense-in-depth with the resolve_channel_name direct-ID match in channel_directory.py alongside the _parse_target_ref fix.
I verified on current main (59b56d44):
_parse_target_ref("slack", ...)previously returnedis_explicit=Falsefor all Slack channel IDs since they failisdigit()- The gateway's
_build_slackalso falls back to_build_from_sessionsrather than callingconversations.list, so the name-resolution path was a dead end for programmatic channel references
One minor observation: the test_channel_id_match_is_exact test asserts case-sensitive matching for the direct-ID path in resolve_channel_name, which is correct since Slack IDs are always uppercase — but worth noting that this means a user manually typing a lowercase ID won't match. The regex approach in _parse_target_ref handles this by only matching uppercase, so both paths are consistent.
Slack's chat.postMessage rejects U... (user) and W... (workspace) IDs directly — those require a conversations.open call first to obtain a D... conversation ID. Treating them as explicit targets would silently route to the send path and produce channel_not_found at send time. Narrow _SLACK_CHANNEL_ID_RE to [CGD] so U/W IDs fall through to the name-resolution path (which will fail gracefully rather than corrupt the send target). Update the test that previously expected U... to be explicit, and clarify the channel_directory fast-path comment to be platform-agnostic. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Thanks @copilot and @alt-glitch — all three Copilot findings are real. Addressed in `458126a9`. 1. Narrow regex to `[CGD]` only (inline comment 1) 2. Fix `test_user_id_is_explicit` (inline comment 2) 3. Comment / docstring clarification (inline comment 3) Re @alt-glitch — positioning vs #15064 and #11101:
The `channel_directory.py` fast-path is the differentiating piece: without it, a raw `C...` ID passed to `resolve_channel_name` would fail the case-insensitive name match and return `None` even though the ID is valid. Happy to defer to whichever the maintainers prefer. |
|
@copilot Good catch on the Slack user ID nuance. The PR's intent is specifically to fix the channel resolution layer ( |
Slack's chat.postMessage API rejects user IDs (U...) and workspace IDs (W...) — they are not valid conversation IDs. Posting to them fails because the API requires a channel ID (C/G/D). To DM a user, the sender must first call conversations.open to obtain a D... ID. Tighten _SLACK_TARGET_RE from [CGDUW] to [CGD] so the send path rejects U/W values as explicit targets and instead falls through to channel- name resolution (where they'll fail with a clear 'could not resolve' error rather than silently getting stuck in a retry loop on the API). Flip the corresponding regression test to assert U/W values are not explicit. Matches the narrower regex briandevans proposed in #15939. Co-authored-by: briandevans <brian@bde.io>
|
Merged via #16198 — your stricter regex ( |
Slack's chat.postMessage API rejects user IDs (U...) and workspace IDs (W...) — they are not valid conversation IDs. Posting to them fails because the API requires a channel ID (C/G/D). To DM a user, the sender must first call conversations.open to obtain a D... ID. Tighten _SLACK_TARGET_RE from [CGDUW] to [CGD] so the send path rejects U/W values as explicit targets and instead falls through to channel- name resolution (where they'll fail with a clear 'could not resolve' error rather than silently getting stuck in a retry loop on the API). Flip the corresponding regression test to assert U/W values are not explicit. Matches the narrower regex briandevans proposed in NousResearch#15939. Co-authored-by: briandevans <brian@bde.io>
Slack's chat.postMessage API rejects user IDs (U...) and workspace IDs (W...) — they are not valid conversation IDs. Posting to them fails because the API requires a channel ID (C/G/D). To DM a user, the sender must first call conversations.open to obtain a D... ID. Tighten _SLACK_TARGET_RE from [CGDUW] to [CGD] so the send path rejects U/W values as explicit targets and instead falls through to channel- name resolution (where they'll fail with a clear 'could not resolve' error rather than silently getting stuck in a retry loop on the API). Flip the corresponding regression test to assert U/W values are not explicit. Matches the narrower regex briandevans proposed in NousResearch#15939. Co-authored-by: briandevans <brian@bde.io>
Slack's chat.postMessage API rejects user IDs (U...) and workspace IDs (W...) — they are not valid conversation IDs. Posting to them fails because the API requires a channel ID (C/G/D). To DM a user, the sender must first call conversations.open to obtain a D... ID. Tighten _SLACK_TARGET_RE from [CGDUW] to [CGD] so the send path rejects U/W values as explicit targets and instead falls through to channel- name resolution (where they'll fail with a clear 'could not resolve' error rather than silently getting stuck in a retry loop on the API). Flip the corresponding regression test to assert U/W values are not explicit. Matches the narrower regex briandevans proposed in NousResearch#15939. Co-authored-by: briandevans <brian@bde.io>
Slack's chat.postMessage API rejects user IDs (U...) and workspace IDs (W...) — they are not valid conversation IDs. Posting to them fails because the API requires a channel ID (C/G/D). To DM a user, the sender must first call conversations.open to obtain a D... ID. Tighten _SLACK_TARGET_RE from [CGDUW] to [CGD] so the send path rejects U/W values as explicit targets and instead falls through to channel- name resolution (where they'll fail with a clear 'could not resolve' error rather than silently getting stuck in a retry loop on the API). Flip the corresponding regression test to assert U/W values are not explicit. Matches the narrower regex briandevans proposed in NousResearch#15939. Co-authored-by: briandevans <brian@bde.io>
Slack's chat.postMessage API rejects user IDs (U...) and workspace IDs (W...) — they are not valid conversation IDs. Posting to them fails because the API requires a channel ID (C/G/D). To DM a user, the sender must first call conversations.open to obtain a D... ID. Tighten _SLACK_TARGET_RE from [CGDUW] to [CGD] so the send path rejects U/W values as explicit targets and instead falls through to channel- name resolution (where they'll fail with a clear 'could not resolve' error rather than silently getting stuck in a retry loop on the API). Flip the corresponding regression test to assert U/W values are not explicit. Matches the narrower regex briandevans proposed in NousResearch#15939. Co-authored-by: briandevans <brian@bde.io>
Summary
_parse_target_refnow recognizes Slack channel/group/DM/user/workspace IDs (C…,G…,D…,U…,W…) as explicit targets, bypassing name resolution.resolve_channel_namegains a step-0 direct-ID match as defense-in-depth for any path that still reaches the resolver with a raw ID.The bug
Slack channel IDs are uppercase alphanumeric strings (e.g.
C01234ABCDE)._parse_target_refhandled explicit IDs for every other platform (Telegram numeric, Discord numeric, Feishuoc_…, Matrix!…/@…, E.164 phone numbers) but had no Slack branch. IDs failed theisdigit()fallback and fell through toresolve_channel_name, which only matches by name — returningNoneand surfacing "Could not resolve 'C01234ABCDE'" to the user even though they had the correct, valid channel ID.The fix
Added
_SLACK_CHANNEL_ID_RE = re.compile(r"^\s*([CGDUW][A-Z0-9]{8,})\s*$")and a platform-guarded branch in_parse_target_ref:Defense-in-depth:
resolve_channel_namenow checks for an exact channel-ID match (step 0, before name normalization) so alphanumeric IDs that reach the resolver still return the correct result.Test plan
TestParseTargetRefSlackdid not exist (79 deselected);test_slack_channel_id_resolves_directlyreturnedNoneinstead of the channel ID../venv/bin/pytest tests/tools/test_send_message_tool.py::TestParseTargetRefSlack tests/gateway/test_channel_directory.py::TestResolveChannelName::test_slack_channel_id_resolves_directly tests/gateway/test_channel_directory.py::TestResolveChannelName::test_channel_id_match_is_exact→ 10 passed./venv/bin/pytest tests/tools/test_send_message_tool.py tests/gateway/test_channel_directory.py→ 114 passed, 0 failuresRelated
_parse_target_refatsend_message_tool.py:307–336had no Slack branch;channel_directory.py:139–153_build_slackfalls back to session data.🤖 Generated with Claude Code