fix(Slack): resolve Slack channels by raw ID and enumerate joined channels - #15947
Closed
hhuang91 wants to merge 1 commit into
Closed
fix(Slack): resolve Slack channels by raw ID and enumerate joined channels#15947hhuang91 wants to merge 1 commit into
hhuang91 wants to merge 1 commit into
Conversation
…nnels send_message(target='slack:<channel_id>') failed with "Could not resolve" because _parse_target_ref had no Slack branch — Slack's uppercase alphanumeric IDs fell through to channel-name resolution, which only matched by name. As a fallback, the agent would retry with bare target='slack' and post to the home channel instead. Three fixes: - _parse_target_ref recognizes Slack IDs (C/G/D/U/W prefix) as explicit targets so the name-resolver is bypassed entirely. - resolve_channel_name tries a case-sensitive raw-ID match before the existing name match, so any platform's IDs resolve cleanly. - _build_slack now actually calls users.conversations against each workspace's AsyncWebClient (paginated), instead of only returning session-history entries. This populates the directory with public and private channels the bot has joined, so action='list' shows them and they can also be addressed by name. Errors from one workspace don't block others. build_channel_directory becomes async (Slack web calls require it). The two async-context callers in gateway/run.py are awaited; the cron ticker thread call bridges via asyncio.run_coroutine_threadsafe. Slack bot needs channels:read and groups:read scopes for full enumeration; missing scopes degrade gracefully per-workspace. addressing NousResearch#15927
Collaborator
Contributor
|
Merged via #16198 — your commit was cherry-picked onto current main with your authorship preserved (d889ad7). Thanks for both filing the issue with a clear three-layer fix plan AND following through with the full implementation. The Small tweak in a follow-up commit: tightened the regex from |
Open
12 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
When asking Hermes to send a Slack message to a channel other than the
current one, two failure modes appeared:
send_message(target='slack:<channel_id>')returnedCould not resolve '<channel_id>' on slack, even though the bot was a member of the channel and the ID was valid.target='slack'form and silently post to the platform's home channel instead of
the requested one.
In-channel and DM conversations worked fine — the bug only surfaced
on outbound cross-channel sends.
Root cause
Three compounding gaps in the
send_message→channel_directorypath, all specific to Slack:
tools/send_message_tool.py::_parse_target_refhad explicithandlers for Telegram/Discord/Feishu/Weixin/Matrix/phone IDs, but
no Slack branch. Slack's uppercase-alphanumeric IDs (
C…,G…,D…,U…,W…) failed the existingtarget_ref.lstrip("-").isdigit()check and fell through with
is_explicit=False, forcing the valueinto the name-resolver.
gateway/channel_directory.py::resolve_channel_nameonlymatched against
channel["name"], neverchannel["id"]. So evenwhen a channel was in the directory, supplying its raw ID didn't
resolve.
gateway/channel_directory.py::_build_slacknever actuallycalled the Slack web API — it just delegated to
_build_from_sessions, so the directory only knew about channelsthe bot had received inbound messages in. Channels the bot had
joined but never been talked to in were absent from
action='list'and unaddressable by name.The "redirected to home channel" symptom was the model's own
fallback: after the resolver error, it retried with bare
target='slack', which the schema documents as "uses home channel"— compounding the visible misbehavior.
Fix
_parse_target_refrecognizes Slack channel/user IDs as explicittargets via a precompiled regex (
[CGDUW][A-Z0-9]{8,}), so theybypass name resolution entirely.
resolve_channel_nameadds a case-sensitive raw-ID match stepbefore the existing case-insensitive name match. Defense in depth
for any platform: a raw ID always resolves to itself if present in
the directory.
_build_slacknow callsusers.conversationsagainst eachworkspace's
AsyncWebClient, paginates viaresponse_metadata.next_cursor, and merges in DM entries fromsession history. Per-workspace errors are isolated. Requires
channels:readandgroups:readSlack scopes for fullenumeration; missing scopes degrade gracefully for that workspace
alone.
build_channel_directorybecomesasync(Slack web calls requireit). Two async-context callers in
gateway/run.pyare awaited;the cron-ticker thread call bridges via
asyncio.run_coroutine_threadsafe(...).result(timeout=30).Why this approach
matches the pattern every other platform already uses in
_parse_target_ref— it's the smallest change that brings Slack toparity.
resolve_channel_nameis platform-agnostic: it costs one cheap loop and prevents this entire class of
bug from recurring on any future platform whose ID format isn't
caught by
_parse_target_ref.users.conversationsoverconversations.listbecause itscopes to channels the bot is actually a member of, requires
fewer permissions, and matches how Slack itself recommends bots
enumerate their own workspace presence.
requestsbecause the adapter alreadyowns the typed
AsyncWebClients with multi-workspace tokens, proxyconfig, and retry semantics — reusing them avoids a parallel
HTTP path and keeps token plumbing in one place.
Tests
24 new test cases:
tests/tools/test_send_message_tool.py::TestParseTargetRefSlack(7) —public/private/DM/user IDs, whitespace, lowercase/short rejection,
isolation from other platforms.
tests/gateway/test_channel_directory.py::TestResolveChannelName::test_id_match_takes_precedence_over_name(1) —raw IDs resolve to themselves, lowercase still falls through to
name matching.
tests/gateway/test_channel_directory.py::TestBuildSlack(7) —no-clients fallback, single-page list, pagination via cursor,
per-workspace error isolation, session-DM merge with dedup,
missing-id/name skip,
ok=Falsehandling.Existing
test_failed_write_preserves_previous_cacheupdated toasyncio.run(build_channel_directory({})).Full test run: 118 passed (1 pre-existing Windows
/tmp/failureunrelated to this change).
Related Issue
Fixes #15927
Type of Change
Changes Made
Three fixes:
How to Test
Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/A