fix(gateway): key Slack handoffs the way Slack thread replies are keyed
`/handoff slack` bound the CLI session under `slack🧵<channel>:<ts>` while every inbound reply in that thread resolves (via the Slack adapter's source shape) to `slack:dm|group:<team>:<channel>:<ts>`. After a gateway restart the reply key found no binding and the gateway opened a fresh empty session, orphaning the handed-off one (#111896). The handoff destination now mirrors the adapter: chat_type `dm` for a D… home channel, else `group`, plus the workspace scope_id (home channel provenance, falling back to the adapter's channel→team map). Channel handoffs were equally affected (`thread` vs `group`), so the fix covers both, not only DMs. Discord and Telegram destinations are unchanged. Co-authored-by: KoNit-K <124019182+KoNit-K@users.noreply.github.com>
This commit is contained in:
@@ -1497,6 +1497,15 @@ class GatewayStartupMixin:
|
||||
platform == Platform.TELEGRAM and looks_like_telegram_private_chat_id(home_chat_id)
|
||||
)
|
||||
is_thread = bool(new_thread_id) and not is_telegram_private_chat
|
||||
chat_type = "thread" if is_thread else "dm"
|
||||
scope_id = None
|
||||
if platform == Platform.SLACK:
|
||||
# Slack keys a thread reply on the parent channel's type ("dm" for a D… channel, else
|
||||
# "group") plus the workspace id — never on a "thread" slot. Mirror the adapter's inbound
|
||||
# source shape or the first reply after a restart lands on a different key (#111896).
|
||||
chat_type = "dm" if home_chat_id.startswith("D") else "group"
|
||||
scope_for_chat = getattr(transport.adapter, "scope_id_for_chat", None)
|
||||
scope_id = home.scope_id or (scope_for_chat(home_chat_id) if callable(scope_for_chat) else None)
|
||||
# Discord builds in-thread messages with ``chat_id == thread id``: key on the thread's OWN id.
|
||||
dest_source = SessionSource(
|
||||
platform=platform,
|
||||
@@ -1504,9 +1513,10 @@ class GatewayStartupMixin:
|
||||
is_thread and platform == Platform.DISCORD and effective_thread_id
|
||||
) else home_chat_id,
|
||||
chat_name=home.name,
|
||||
chat_type="thread" if is_thread else "dm",
|
||||
chat_type=chat_type,
|
||||
user_id=home_chat_id if is_telegram_private_chat else "system:handoff",
|
||||
user_name="Handoff", thread_id=effective_thread_id, profile=profile_name,
|
||||
scope_id=scope_id,
|
||||
)
|
||||
return self._HandoffDestination(
|
||||
platform=platform, platform_name=platform_name, transport=transport, home=home,
|
||||
|
||||
@@ -21,7 +21,12 @@ messages with ``chat_id = parent_channel``, so the parent channel is correct
|
||||
for those platforms and the guard must NOT apply to them.
|
||||
"""
|
||||
|
||||
from gateway.config import Platform
|
||||
import asyncio
|
||||
from types import SimpleNamespace
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
|
||||
from gateway.config import GatewayConfig, HomeChannel, Platform, PlatformConfig
|
||||
from gateway.run import GatewayRunner
|
||||
from gateway.session import SessionSource, build_session_key
|
||||
|
||||
|
||||
@@ -43,22 +48,6 @@ def _organic_discord_thread_key(thread_id: str, parent_id: str, user_id: str) ->
|
||||
return build_session_key(source, thread_sessions_per_user=False)
|
||||
|
||||
|
||||
def _organic_slack_thread_key(channel_id: str, thread_ts: str, user_id: str) -> str:
|
||||
"""Key the Slack adapter produces for a message in a thread.
|
||||
|
||||
Mirrors plugins/platforms/slack/adapter.py: chat_id is the parent channel,
|
||||
chat_type is "group", thread_id is the thread timestamp.
|
||||
"""
|
||||
source = SessionSource(
|
||||
platform=Platform.SLACK,
|
||||
chat_id=str(channel_id),
|
||||
chat_type="group",
|
||||
user_id=user_id,
|
||||
thread_id=str(thread_ts),
|
||||
)
|
||||
return build_session_key(source, thread_sessions_per_user=False)
|
||||
|
||||
|
||||
def _handoff_key(
|
||||
platform: Platform,
|
||||
home_chat_id: str,
|
||||
@@ -115,26 +104,43 @@ def test_discord_handoff_key_does_not_use_parent_channel():
|
||||
assert handoff != buggy, "handoff regressed to keying on the parent channel"
|
||||
|
||||
|
||||
def test_slack_handoff_key_uses_parent_channel_not_thread_id():
|
||||
"""Slack adapter keys organic thread messages with chat_id=channel_id
|
||||
(parent), not the thread ts. The fix must NOT apply to Slack — otherwise
|
||||
the handoff key would use the thread ts as chat_id, breaking the match."""
|
||||
channel_id = "C12345678"
|
||||
thread_ts = "1690000000.123456"
|
||||
user_id = "U123456"
|
||||
def _slack_handoff_destination(channel_id: str, thread_ts: str, team_id: str):
|
||||
"""Run the real handoff destination/key path against a Slack home channel."""
|
||||
config = GatewayConfig(platforms={Platform.SLACK: PlatformConfig(enabled=True, token="test")})
|
||||
config.platforms[Platform.SLACK].home_channel = HomeChannel(
|
||||
platform=Platform.SLACK, chat_id=channel_id, name="home", scope_id=team_id)
|
||||
adapter = MagicMock()
|
||||
adapter.create_handoff_thread = AsyncMock(return_value=thread_ts)
|
||||
runner = object.__new__(GatewayRunner)
|
||||
runner.config = config
|
||||
runner.adapters = {Platform.SLACK: adapter}
|
||||
runner.session_store = None
|
||||
with patch("gateway.delivery.resolve_delivery_transport",
|
||||
lambda *_a: SimpleNamespace(adapter=adapter, send=AsyncMock())):
|
||||
dest = asyncio.run(runner._handoff_resolve_destination(
|
||||
{"id": "cli-session", "title": "work", "handoff_platform": "slack"}, profile_name=None))
|
||||
return dest, runner._handoff_session_key(dest, profile_name=None)
|
||||
|
||||
organic = _organic_slack_thread_key(channel_id, thread_ts, user_id)
|
||||
handoff = _handoff_key(Platform.SLACK, channel_id, thread_ts)
|
||||
|
||||
# The handoff uses chat_type="thread" while Slack organic uses "group",
|
||||
# so these keys differ in the chat_type slot (a pre-existing mismatch,
|
||||
# NOT caused by this fix). The important assertion is that the handoff
|
||||
# does NOT use the thread_ts as chat_id (the regression this guard prevents).
|
||||
assert "thread_ts" not in handoff or thread_ts not in handoff.split(":")[-2:-1], (
|
||||
f"handoff key {handoff!r} incorrectly uses thread ts as chat_id"
|
||||
)
|
||||
# Verify the handoff key still contains the parent channel_id
|
||||
assert channel_id in handoff, (
|
||||
f"handoff key {handoff!r} lost the parent channel id — "
|
||||
"the Discord-specific guard leaked into Slack"
|
||||
)
|
||||
def _organic_slack_reply_key(channel_id: str, thread_ts: str, team_id: str, chat_type: str) -> str:
|
||||
"""Key the Slack adapter builds for a thread reply (``_build_message_event``): parent channel
|
||||
as chat_id, ``dm``/``group`` from the channel type, workspace id as scope_id."""
|
||||
return build_session_key(SessionSource(
|
||||
platform=Platform.SLACK, chat_id=channel_id, chat_type=chat_type, user_id="U123456",
|
||||
thread_id=thread_ts, scope_id=team_id), thread_sessions_per_user=False)
|
||||
|
||||
|
||||
def test_slack_dm_handoff_key_matches_the_thread_reply_key():
|
||||
"""/handoff into a Slack DM must bind the key the next in-thread reply resolves to, or a
|
||||
gateway restart forks the thread onto a fresh empty session (#111896)."""
|
||||
dest, handoff = _slack_handoff_destination("D0C1HFBMQAX", "1789474088.089709", "T0C2HL96FH6")
|
||||
assert handoff == _organic_slack_reply_key("D0C1HFBMQAX", "1789474088.089709", "T0C2HL96FH6", "dm")
|
||||
assert dest.source.chat_id == "D0C1HFBMQAX"
|
||||
|
||||
|
||||
def test_slack_channel_handoff_key_matches_the_thread_reply_key():
|
||||
"""Channel handoffs key on the parent channel (not the thread ts) with the ``group`` layout the
|
||||
adapter uses for channel replies (#111896)."""
|
||||
dest, handoff = _slack_handoff_destination("C0CHANNEL01", "1789474088.089709", "T0C2HL96FH6")
|
||||
assert handoff == _organic_slack_reply_key("C0CHANNEL01", "1789474088.089709", "T0C2HL96FH6", "group")
|
||||
assert dest.source.chat_id == "C0CHANNEL01"
|
||||
|
||||
Reference in New Issue
Block a user