From c8089dabcd6a1f6e67fbe5e99f4d4b5c5ce1f485 Mon Sep 17 00:00:00 2001 From: Ted Malone Date: Wed, 22 Jul 2026 04:16:45 -0700 Subject: [PATCH] fix(slack): include bot's own prior replies in cold-start thread context MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _fetch_thread_context unconditionally filtered out the bot's own prior replies, so cold-start sessions (bot posts a thread root, user replies later, no active session) lost every assistant turn and the agent could not reconstruct the prior conversation. The circular-context concern the filter guarded against does not apply here: the call site is gated by _has_active_session_for_thread, so this method only runs when there is no session history to duplicate. Self-bot replies are now kept and labelled with an explicit [assistant] prefix (skipping user-name resolution — the label already communicates authorship). Third-party bot posts and the bot-authored thread parent keep their existing treatment. Fixes #38861. Salvaged from #38936 by @temalo, rebased from the pre-plugin gateway/platforms/slack.py layout onto plugins/platforms/slack/adapter.py (preserving the [unverified] trust-tag handling added on main since). --- plugins/platforms/slack/adapter.py | 46 +++++++++++++----- tests/gateway/test_slack_approval_buttons.py | 50 +++++++++++++------- 2 files changed, 67 insertions(+), 29 deletions(-) diff --git a/plugins/platforms/slack/adapter.py b/plugins/platforms/slack/adapter.py index 0d55878d5975..0bb6f26351d9 100644 --- a/plugins/platforms/slack/adapter.py +++ b/plugins/platforms/slack/adapter.py @@ -5090,31 +5090,42 @@ class SlackAdapter(BasePlatformAdapter): self._team_bot_user_ids.get(msg_team) if msg_team else None ) or self._bot_user_id - # Exclude only our own prior bot replies (circular context). - # Keep: - # - the thread parent even if it was posted by a bot - # (e.g. a cron job summary we are now replying to); - # - other bots' child messages (useful third-party context). - if ( + # Identify our own prior bot replies. These are kept on the + # cold-start path (the only path that reaches this method — + # the call site is guarded by _has_active_session_for_thread) + # so the agent can reconstruct its own prior turns (#38861). + # When an active session exists, this method is not called and + # the session history already carries those replies — so there + # is no risk of circular duplication. + # + # Self-bot replies are labelled with an explicit ``[assistant]`` + # prefix so the agent can distinguish its own prior turns + # from user messages and from third-party bot posts. + is_self_bot_reply = ( is_bot and not is_parent and self_bot_uid and msg_user == self_bot_uid - ): - continue + ) msg_text = self._render_message_text(msg, bot_uid=bot_uid) if not msg_text: continue - prefix = "[thread parent] " if is_parent else "" + # Strip bot mentions from context messages + if bot_uid: + msg_text = msg_text.replace(f"<@{bot_uid}>", "").strip() + + if is_parent: + prefix = "[thread parent] " + elif is_self_bot_reply: + prefix = "[assistant] " + else: + prefix = "" display_user = msg_user or "unknown" # Prefer the bot's own name when the message is a bot post. if is_bot and not display_user: display_user = msg.get("username") or "bot" - name = await self._resolve_user_name( - display_user, chat_id=channel_id, team_id=team_id - ) # Mark senders not on the allowlist as [unverified] so the LLM # treats their content as background reference rather than @@ -5128,7 +5139,16 @@ class SlackAdapter(BasePlatformAdapter): if is_authorized is False: trust_tag = "[unverified] " - context_parts.append(f"{prefix}{trust_tag}{name}: {msg_text}") + if is_self_bot_reply: + # Skip user-name resolution for self-bot replies — the + # ``[assistant]`` prefix already communicates authorship, + # and the resolved name would just be our own bot handle. + context_parts.append(f"{prefix}{msg_text}") + else: + name = await self._resolve_user_name( + display_user, chat_id=channel_id, team_id=team_id + ) + context_parts.append(f"{prefix}{trust_tag}{name}: {msg_text}") if is_parent: parent_text = msg_text diff --git a/tests/gateway/test_slack_approval_buttons.py b/tests/gateway/test_slack_approval_buttons.py index 099139ee03b8..fa0256c4f631 100644 --- a/tests/gateway/test_slack_approval_buttons.py +++ b/tests/gateway/test_slack_approval_buttons.py @@ -514,24 +514,28 @@ class TestSlackThreadContext: assert "<@U_BOT>" not in context @pytest.mark.asyncio - async def test_skips_bot_messages(self): - """Self-bot child replies are skipped to avoid circular context, - but non-self bots (e.g. cron posts, third-party integrations) are kept. + async def test_includes_self_bot_replies_as_assistant_on_cold_start(self): + """Cold-start contract (issue #38861): self-bot replies in the thread + must be included in the context, labelled with an ``[assistant]`` + prefix so the agent can reconstruct its own prior turns. This method + only runs on the cold-start path (guarded at the call site by + ``_has_active_session_for_thread``) — when an active session exists, + the session history already carries those replies, so there is no + risk of circular duplication. - Regression guard for the fix in _fetch_thread_context: previously ALL - bot messages were dropped, which lost context when the bot was replying - to a cron-posted thread parent.""" + Third-party bots (e.g. deploy notifications) must still be kept, + attributed to their display name.""" adapter = _make_adapter() mock_client = adapter._team_clients["T1"] mock_client.conversations_replies = AsyncMock(return_value={ "messages": [ {"ts": "1000.0", "user": "U1", "text": "Parent"}, - # Self-bot reply -> must be skipped (circular) + # Self-bot reply -> kept on cold-start, prefixed [assistant] { "ts": "1000.1", "bot_id": "B_SELF", "user": "U_BOT", - "text": "Previous bot self-reply (should be skipped)", + "text": "Previous bot self-reply", }, # Third-party bot child -> kept (useful context) { @@ -552,10 +556,13 @@ class TestSlackThreadContext: channel_id="C1", thread_ts="1000.0", current_ts="1000.2", team_id="T1" ) - assert "Previous bot self-reply" not in context assert "Alice: Parent" in context - # Third-party bot message must now be included + # Self-bot reply must now be included with [assistant] label + assert "[assistant] Previous bot self-reply" in context + # Third-party bot message must still be included assert "Deploy succeeded" in context + # The [assistant] label must NOT leak to user messages + assert "[assistant] Alice" not in context @pytest.mark.asyncio async def test_empty_thread(self): @@ -751,14 +758,19 @@ class TestSlackThreadContext: assert "Incident triggered" in text assert "https://example.example/incident/42" in text - """Parent (non-self bot) is kept, self-bot child replies are dropped, - user replies are kept.""" + + @pytest.mark.asyncio + async def test_fetch_thread_context_includes_self_bot_replies_with_assistant_label(self): + """Cold-start: parent (non-self bot) kept with [thread parent], + self-bot child replies kept with [assistant] label, user replies + kept unchanged. The cold-start path is the ONLY caller of this + method; circular-context risk does not apply here (see #38861).""" adapter = _make_adapter() mock_client = adapter._team_clients["T1"] mock_client.conversations_replies = AsyncMock(return_value={ "messages": [ {"ts": "1000.0", "bot_id": "B_CRON", "text": "Cron summary"}, - # Self-bot child reply -> excluded + # Self-bot child reply -> kept with [assistant] label { "ts": "1000.1", "bot_id": "B_SELF", @@ -779,7 +791,8 @@ class TestSlackThreadContext: assert "Cron summary" in context assert "[thread parent]" in context - assert "Previous self reply" not in context + # Self-bot child reply is now kept with the [assistant] label + assert "[assistant] Previous self reply" in context assert "Follow-up question" in context assert "Current" not in context @@ -808,7 +821,7 @@ class TestSlackThreadContext: "team": "T2", "text": "Cross-workspace bot reply", }, - # Self-bot for T2 — must be skipped + # Self-bot for T2 — kept with the [assistant] label { "ts": "2000.2", "bot_id": "B_SELF_T2", @@ -827,7 +840,12 @@ class TestSlackThreadContext: assert "Parent T2" in context assert "Cross-workspace bot reply" in context - assert "Own T2 bot reply" not in context + # T2's own self-bot reply is kept with the [assistant] label + # (cold-start path includes self-replies; see #38861). The + # per-workspace filter still applies: this assertion confirms + # we use T2's bot id, not T1's, when deciding what counts as + # self-bot. + assert "[assistant] Own T2 bot reply" in context @pytest.mark.asyncio async def test_fetch_thread_context_current_ts_excluded(self):