mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-22 16:25:58 +00:00
fix(slack): include bot's own prior replies in cold-start thread context
_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).
This commit is contained in:
Ted Malone
Teknium
parent
2d71e9e9bc
commit
c8089dabcd
2 changed files with 67 additions and 29 deletions
|
|
@ -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
|
||||
|
||||
|
|
|
|||
|
|
@ -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):
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue