fix(slack): scope top-level channel messages by channel-only when reply_in_thread=false (#15421) - #41703
Merged
Conversation
…ly_in_thread=false (#15421) Top-level Slack channel messages previously fell back to the message's own ``ts`` as a synthetic ``thread_ts``: thread_ts = event.get("thread_ts") or ts # ts fallback for channels That value flows into ``build_source(thread_id=thread_ts)`` at line 1247. The gateway session store keys sessions by ``(platform, channel_id, thread_id)``, so every top-level channel message ended up on a unique session. Operators who set ``reply_in_thread: false`` in ``config.yaml`` expected all top-level channel messages to share one session (the whole point of that flag) — instead each one spawned a fresh conversation with no context carry-over. ### Fix Three explicit cases in the channel branch: | event.thread_ts | reply_in_thread | thread_ts for session keying | |---|---|---| | non-null (real thread reply) | either | event.thread_ts | | null (top-level) | true (default) | ts (legacy: own-thread sessions) | | null (top-level) | false | **None** (shared channel session) | The outbound-reply gate at line 1264 (``reply_to_message_id = thread_ts if thread_ts != ts else None``) still works correctly in all three cases without further changes: ``None != ts`` is True, so shared-channel top-level messages don't get their reply threaded either — matching the operator's ``reply_in_thread=false`` intent end-to-end. Genuine thread replies still scope per-thread under both modes so multi-person threaded conversations can't collide with unrelated channel chatter. ### Tests (7 new in ``tests/gateway/test_slack_channel_session_scope.py``) All drive the real ``SlackAdapter._handle_slack_message`` code path (not a re-implementation) via the standard pytest fixture pattern used by ``tests/gateway/test_slack.py``. Messages @mention the bot so the mention gate doesn't drop them — the tests are specifically about what happens once the handler decides to emit a ``MessageEvent``. * ``TestChannelSessionScopeDefault`` (2 cases): - Explicit ``reply_in_thread: true`` keeps ``thread_id = ts`` (legacy behaviour — regression guard) - Unset config behaves like ``reply_in_thread: true`` (pins the default) * ``TestChannelSessionScopeShared`` (3 cases): - ``reply_in_thread: false`` + top-level → ``thread_id is None`` (the #15421 bug 1 fix) - ``reply_to_message_id is None`` in the same case (no threaded outbound reply) - Genuine thread reply still scopes per-thread when shared mode is on — only TOP-LEVEL messages collapse to the channel session * ``TestThreadReplyAlwaysScopesByThread`` (2 parametrised cases): - Thread replies get ``thread_id = event.thread_ts`` regardless of ``reply_in_thread`` — critical invariant for multi-thread channels; a regression here would leak per-thread context across threads **Regression guard verified**: reverted the else-branch to the legacy ``thread_ts = event.get("thread_ts") or ts`` one-liner; ``test_top_level_maps_to_none_when_reply_in_thread_false`` correctly failed (asserts ``thread_id is None`` but got ``"1700000000.000003"``). Restored → 182 slack tests pass (175 existing + 7 new). Scope: this fixes #15421 bug 1 only. Bug 2 (sessions.json not persisting across compression) lives elsewhere in the session manager and is left for a separate diff. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ilot #15464) Two findings from Copilot's review on #15464, both addressed: 1. ``event.get("thread_ts")`` truthy vs ``event_thread_ts != ts``: the new channel branch treated ANY truthy ``thread_ts`` as a real thread reply, but three lines below ``is_thread_reply`` is defined with the stricter ``event_thread_ts and event_thread_ts != ts`` invariant. If Slack ever ships a payload where ``thread_ts == ts`` on a thread root, the stricter check would treat it as a top-level message for the ``is_thread_reply`` path but as a thread reply for session keying — divergent behaviour. Aligned this branch to the same ``and event_thread_ts_raw != ts`` invariant. 2. ``test_top_level_reply_to_id_stays_none_when_shared`` docstring had the ternary logic backwards ("None != ts → reply_to_message_id IS set"). The code reads ``reply_to_message_id = thread_ts if thread_ts != ts else None`` — with ``thread_ts = None``, the condition is True so the expression evaluates to ``thread_ts`` itself (None), meaning the reply stays un-threaded. The test asserted the correct end-state; only the explanatory docstring was wrong. Rewrote the docstring to match the actual code flow, with the note that Copilot caught the reversal. 7/7 tests still pass. No behaviour change for the existing test_thread_reply_scopes_by_thread_even_when_shared case because ``event_thread_ts_raw = "1700000000.000000"`` and ``ts = "1700000000.000005"`` are distinct — the new ``!= ts`` guard is a no-op there. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
🔎 Lint report:
|
| Rule | Count |
|---|---|
unresolved-import |
1 |
invalid-parameter-default |
1 |
invalid-assignment |
1 |
First entries
tests/gateway/test_slack_channel_session_scope.py:26: [unresolved-import] unresolved-import: Cannot resolve imported module `pytest`
tests/gateway/test_slack_channel_session_scope.py:52: [invalid-parameter-default] invalid-parameter-default: Default value of type `None` is not assignable to annotated parameter type `str`
tests/gateway/test_slack_channel_session_scope.py:40: [invalid-assignment] invalid-assignment: Object of type `AsyncMock` is not assignable to attribute `handle_message` of type `def handle_message(self, event: MessageEvent) -> CoroutineType[Any, Any, None]`
✅ Fixed issues: none
Unchanged: 5200 pre-existing issues carried over.
Diagnostics are surfaced as warnings — this check never fails the build.
alt-glitch
added
type/bug
1 task
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
3 participants
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.
Summary
Top-level Slack channel messages now share one channel-scoped session when
reply_in_thread: false, so channel context accumulates across messages instead of resetting on every message.Root cause: the channel branch in
_handle_slack_messagefell back to the message's owntsas a syntheticthread_ts(thread_ts = event.get("thread_ts") or ts). That value flowed intobuild_source(thread_id=...), and since the session store keys on(platform, channel_id, thread_id), every top-level message minted a brand-new session. This also explains the issue's "Bug 2" — the emptysessions.jsonwas a symptom of per-message keys evicting on idle-TTL and never re-resolving on restart, not an independent persistence defect.SessionStore._save()is atomic + fsync'd and fires on all create/update paths; a stable channel key makes it persist and re-resolve correctly.Salvage of #15464 (@briandevans) onto current main — both contributor commits cherry-picked with authorship preserved.
Changes
gateway/platforms/slack.py: channel branch now keys sessions in three explicit cases:event.thread_tsreply_in_threadthread_id!= ts)event.thread_tsts(legacy per-thread session)None(shared channel session)event_thread_ts_raw != ts, matching theis_thread_replyinvariant three lines below.tests/gateway/test_slack_channel_session_scope.py: 7 new tests driving the real_handle_slack_messagepath.Validation
reply_in_thread=falseagent:main:slack:group:C:Usessions.jsonpersists across restartscripts/run_tests.sh(CI-equivalent per-file isolation).build_session_key(): top-level msgs collapse to one channel session; thread replies stay isolated.Fixes #15421. Supersedes #15464.
Co-authored with @briandevans (original PR #15464).
Infographic