Fix Slack peer bot-user routing loops - #51627
Conversation
|
Follow-up repo issues created from the same incident:
This PR addresses the source-level Slack bot-user routing bug; the follow-ups cover the Codex/gateway observability/smoke-test work that should not be bundled into this fix. |
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Solid fix for Slack peer bot-user routing loops. The core issue: some Slack bot posts arrive as ordinary-looking message events with a bot user id but without bot_id/subtype=bot_message, causing agent-agent loops.
Key improvements:
- New
_user_is_bot_cachedict and_resolve_user_is_bot()method with proper caching _event_declares_bot_sender()helper consolidates bot detection logic_slack_allow_bots()extracted as a clean config accessor- Applied allow_bots policy to resolved bot users after the initial event-level check
- Good test coverage for both ignore (no mention) and route (explicit mention) cases
- Also cleans up environment variable leaks in test_slack_mention.py
No security concerns. The fix correctly prevents bot-to-bot loops while preserving legitimate peer-agent @mention routing.
Reviewed by Hermes Agent
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating the peer-bot routing case; current main still has the reported gap. plugins/platforms/slack/adapter.py:2600-2615 only applies allow_bots to event-declared bots, while the later routing path permits a reply in a previously mentioned thread (plugins/platforms/slack/adapter.py:2826-2868).
Problems
- Blocking: the new
isinstance(result, dict)guards atplugins/platforms/slack/adapter.py:1884and in_resolve_user_is_bot()reject slack-sdk's productionAsyncSlackResponse. The pinnedslack-sdk==3.40.1response is non-dict but implements.get(), which is also the interface current main uses atplugins/platforms/slack/adapter.py:2091-2102. Live bot users would therefore be cached as non-bots, and_resolve_user_name()would regress to user IDs. - The added tests use plain dict mocks, so they do not cover that production response shape.
Suggested changes
- Accept the response's existing
.get()interface rather than requiringdict, and add a non-dict.get()fixture to both new routing cases.
This is an automated hermes-sweeper review.
| try: | ||
| client = self._get_client(chat_id) if chat_id else self._app.client | ||
| result = await client.users_info(user=user_id) | ||
| if not isinstance(result, dict): |
There was a problem hiding this comment.
AsyncWebClient.users_info() returns slack-sdk's AsyncSlackResponse, which provides .get() but is not a dict. This branch will cache every live response as non-bot (and the tests use only dict mocks), so the peer-bot routing fix never activates in production. Accept mapping-like .get() responses and add a non-dict response fixture.
…ure parity Follow-up rework on the #51627 cherry-pick: - Guard the 5th wake check (parent-mentioned-bot, #24848) against a None parent_text — _fetch_thread_parent_text is typed to return str but tests (and defensive callers) can surface None; 'in None' raised TypeError. - Drop the PR's fixture-level _fetch_thread_parent_text/_fetch_thread_context AsyncMocks from TestThreadReplyHandling/TestAssistantThreadLifecycle: main's #24848 tests exercise the real parent-text path via conversations_replies side effects, and the blanket mocks broke them. - _resolve_user_is_bot reworked to the workspace-scoped (team_id, user_id) cache key introduced by the multi-workspace name cache on main.
…ure parity Follow-up rework on the #51627 cherry-pick: - Guard the 5th wake check (parent-mentioned-bot, #24848) against a None parent_text — _fetch_thread_parent_text is typed to return str but tests (and defensive callers) can surface None; 'in None' raised TypeError. - Drop the PR's fixture-level _fetch_thread_parent_text/_fetch_thread_context AsyncMocks from TestThreadReplyHandling/TestAssistantThreadLifecycle: main's #24848 tests exercise the real parent-text path via conversations_replies side effects, and the blanket mocks broke them. - _resolve_user_is_bot reworked to the workspace-scoped (team_id, user_id) cache key introduced by the multi-workspace name cache on main.
…ure parity Follow-up rework on the #51627 cherry-pick: - Guard the 5th wake check (parent-mentioned-bot, #24848) against a None parent_text — _fetch_thread_parent_text is typed to return str but tests (and defensive callers) can surface None; 'in None' raised TypeError. - Drop the PR's fixture-level _fetch_thread_parent_text/_fetch_thread_context AsyncMocks from TestThreadReplyHandling/TestAssistantThreadLifecycle: main's #24848 tests exercise the real parent-text path via conversations_replies side effects, and the blanket mocks broke them. - _resolve_user_is_bot reworked to the workspace-scoped (team_id, user_id) cache key introduced by the multi-workspace name cache on main.
|
Merged via #69483 — your commit was cherry-picked/reapplied onto current main with your authorship preserved in git history: your peer bot-user routing fix is the base of the bot-loop sub-cluster — reworked onto workspace-scoped cache keys with your authorship, fixes #50973. Thanks for the contribution! |
…ure parity Follow-up rework on the NousResearch#51627 cherry-pick: - Guard the 5th wake check (parent-mentioned-bot, NousResearch#24848) against a None parent_text — _fetch_thread_parent_text is typed to return str but tests (and defensive callers) can surface None; 'in None' raised TypeError. - Drop the PR's fixture-level _fetch_thread_parent_text/_fetch_thread_context AsyncMocks from TestThreadReplyHandling/TestAssistantThreadLifecycle: main's NousResearch#24848 tests exercise the real parent-text path via conversations_replies side effects, and the blanket mocks broke them. - _resolve_user_is_bot reworked to the workspace-scoped (team_id, user_id) cache key introduced by the multi-workspace name cache on main.
Summary
Fix Slack peer-agent routing so Slack bot-user identities are handled by the
allow_botspolicy even when Slack delivers the event as a normalmessagewithoutbot_id,bot_profile, orsubtype=bot_message.This prevents peer-agent status/error/control posts from waking another agent through stale thread/session context unless the current message explicitly mentions that target bot.
Root cause
In a live peer-agent Slack incident, operational warning messages from one agent were delivered as ordinary Slack
messageevents with a botuserid but no bot-message event shape. The adapter only treated messages as bot-originated when fields likebot_id,bot_profile, orsubtype=bot_messagewere present.With
allow_bots: mentions, those bot-user messages could therefore be treated like normal users. In a thread where the parent already mentioned a target agent, active session/thread routing could accept warning/status posts that did not freshly mention the target, causing bot-to-bot wakeups and poisoned session turns.Fix
users.infopath.allow_botsbehavior to bot-user senders, not only Slack events with explicit bot-message fields.allow_bots: mentions, require a fresh current-message mention for bot-user senders.users_inforesponses in tests so they are treated conservatively as non-bot.SLACK_*env mutations do not leak into later routing tests.Verification
Run on a clean branch based on current
origin/main:pytest tests/gateway/test_slack.py::TestMessageRouting tests/gateway/test_slack_mention.py -q70 passed in 4.14spytest tests/gateway/test_slack.py -q211 passed, 4 warnings in 9.86sThe warnings are pre-existing AsyncMock/thread fixture warnings in unrelated Slack tests; the new routing regressions pass.
Operational note
A separate live deployment normalized affected peer-agent profiles to direct-mention-only Slack behavior:
slack.require_mention: trueslack.strict_mention: trueslack.allow_bots: mentionsslack.allowed_channels: ''This PR addresses the source-level routing bug that made
allow_bots: mentionsinsufficient for bot-user-shaped Slack events.