mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-04-25 00:51:20 +00:00
fix(gateway): remove DM thread session seeding to prevent cross-thread contamination (#7084)
The session store was copying the ENTIRE parent DM transcript into new thread sessions. This caused unrelated conversations to bleed across threads in Slack DMs. The Slack adapter already handles thread context correctly via _fetch_thread_context() (conversations.replies API), which fetches only the actual thread messages. The session-level seeding was both redundant and harmful. No other platform (Telegram, Discord) uses DM threads, so the seeding code path was only triggered by Slack — where it conflicted with the adapter-level context. Tests updated to assert thread isolation: all thread sessions start empty, platform adapters are responsible for injecting thread context. Salvage of PR #5868 (jarvisxyz). Reported by norbert on Discord.
This commit is contained in:
parent
aad40f6d0c
commit
b39ea46488
2 changed files with 43 additions and 107 deletions
|
|
@ -770,41 +770,6 @@ class SessionStore:
|
||||||
except Exception as e:
|
except Exception as e:
|
||||||
print(f"[gateway] Warning: Failed to create SQLite session: {e}")
|
print(f"[gateway] Warning: Failed to create SQLite session: {e}")
|
||||||
|
|
||||||
# Seed new DM thread sessions with parent DM session history.
|
|
||||||
# When a bot reply creates a Slack thread and the user responds in it,
|
|
||||||
# the thread gets a new session (keyed by thread_ts). Without seeding,
|
|
||||||
# the thread session starts with zero context — the user's original
|
|
||||||
# question and the bot's answer are invisible. Fix: copy the parent
|
|
||||||
# DM session's transcript into the new thread session so context carries
|
|
||||||
# over while still keeping threads isolated from each other.
|
|
||||||
if (
|
|
||||||
source.chat_type == "dm"
|
|
||||||
and source.thread_id
|
|
||||||
and entry.created_at == entry.updated_at # brand-new session
|
|
||||||
and not was_auto_reset
|
|
||||||
):
|
|
||||||
parent_source = SessionSource(
|
|
||||||
platform=source.platform,
|
|
||||||
chat_id=source.chat_id,
|
|
||||||
chat_type="dm",
|
|
||||||
user_id=source.user_id,
|
|
||||||
# no thread_id — this is the parent DM session
|
|
||||||
)
|
|
||||||
parent_key = self._generate_session_key(parent_source)
|
|
||||||
with self._lock:
|
|
||||||
parent_entry = self._entries.get(parent_key)
|
|
||||||
if parent_entry and parent_entry.session_id != entry.session_id:
|
|
||||||
try:
|
|
||||||
parent_history = self.load_transcript(parent_entry.session_id)
|
|
||||||
if parent_history:
|
|
||||||
self.rewrite_transcript(entry.session_id, parent_history)
|
|
||||||
logger.info(
|
|
||||||
"[Session] Seeded DM thread session %s with %d messages from parent %s",
|
|
||||||
entry.session_id, len(parent_history), parent_entry.session_id,
|
|
||||||
)
|
|
||||||
except Exception as e:
|
|
||||||
logger.warning("[Session] Failed to seed thread session: %s", e)
|
|
||||||
|
|
||||||
return entry
|
return entry
|
||||||
|
|
||||||
def update_session(
|
def update_session(
|
||||||
|
|
|
||||||
|
|
@ -1,19 +1,17 @@
|
||||||
"""Tests for DM thread session seeding.
|
"""Tests for DM thread session isolation.
|
||||||
|
|
||||||
When a bot reply creates a thread in a DM (e.g. Slack), the user's reply
|
DM thread sessions must start empty — no parent transcript seeding.
|
||||||
in that thread gets a new session (keyed by thread_ts). The seeding logic
|
Thread context is handled by platform adapters (e.g. Slack's
|
||||||
copies the parent DM session's transcript into the new thread session so
|
_fetch_thread_context fetches actual thread replies via the API).
|
||||||
the bot retains context of the original conversation.
|
Session-level seeding was removed because it copied the ENTIRE parent
|
||||||
|
DM transcript, causing unrelated conversations to bleed across threads.
|
||||||
|
|
||||||
Covers:
|
Covers:
|
||||||
- Basic seeding: parent transcript copied to new thread session
|
- Thread sessions start empty (no parent seeding)
|
||||||
- No seeding for group/channel chats
|
- Group/channel thread sessions also start empty
|
||||||
- No seeding when parent session doesn't exist
|
- Multiple threads from same parent are independent
|
||||||
- No seeding on auto-reset sessions
|
- Existing thread sessions are not mutated on re-access
|
||||||
- No seeding on existing (non-new) thread sessions
|
- Cross-platform: consistent behavior for Slack, Telegram, Discord
|
||||||
- Parent transcript is not mutated by seeding
|
|
||||||
- Multiple threads from same parent each get independent copies
|
|
||||||
- Cross-platform: works for any platform with DM threads (Slack, Telegram, Discord)
|
|
||||||
"""
|
"""
|
||||||
|
|
||||||
import pytest
|
import pytest
|
||||||
|
|
@ -60,48 +58,41 @@ PARENT_HISTORY = [
|
||||||
]
|
]
|
||||||
|
|
||||||
|
|
||||||
class TestDMThreadSeeding:
|
class TestDMThreadIsolation:
|
||||||
"""Core seeding behavior."""
|
"""Thread sessions must start empty — no parent transcript seeding."""
|
||||||
|
|
||||||
def test_thread_session_seeded_from_parent(self, store):
|
def test_thread_session_starts_empty(self, store):
|
||||||
"""New DM thread session should contain the parent's transcript."""
|
"""New DM thread session should NOT inherit parent's transcript."""
|
||||||
# Create parent DM session with history
|
|
||||||
parent_source = _dm_source()
|
parent_source = _dm_source()
|
||||||
parent_entry = store.get_or_create_session(parent_source)
|
parent_entry = store.get_or_create_session(parent_source)
|
||||||
for msg in PARENT_HISTORY:
|
for msg in PARENT_HISTORY:
|
||||||
store.append_to_transcript(parent_entry.session_id, msg)
|
store.append_to_transcript(parent_entry.session_id, msg)
|
||||||
|
|
||||||
# Create thread session (user replied in thread)
|
|
||||||
thread_source = _dm_source(thread_id="1234567890.000001")
|
thread_source = _dm_source(thread_id="1234567890.000001")
|
||||||
thread_entry = store.get_or_create_session(thread_source)
|
thread_entry = store.get_or_create_session(thread_source)
|
||||||
|
|
||||||
# Thread should have parent's history
|
|
||||||
thread_transcript = store.load_transcript(thread_entry.session_id)
|
thread_transcript = store.load_transcript(thread_entry.session_id)
|
||||||
assert len(thread_transcript) == 2
|
assert len(thread_transcript) == 0
|
||||||
assert thread_transcript[0]["content"] == "What's the weather?"
|
|
||||||
assert thread_transcript[1]["content"] == "It's sunny and 72°F."
|
|
||||||
|
|
||||||
def test_parent_transcript_not_mutated(self, store):
|
def test_parent_transcript_unaffected_by_thread(self, store):
|
||||||
"""Seeding should not alter the parent session's transcript."""
|
"""Creating a thread session should not alter parent's transcript."""
|
||||||
parent_source = _dm_source()
|
parent_source = _dm_source()
|
||||||
parent_entry = store.get_or_create_session(parent_source)
|
parent_entry = store.get_or_create_session(parent_source)
|
||||||
for msg in PARENT_HISTORY:
|
for msg in PARENT_HISTORY:
|
||||||
store.append_to_transcript(parent_entry.session_id, msg)
|
store.append_to_transcript(parent_entry.session_id, msg)
|
||||||
|
|
||||||
# Create thread and add a message to it
|
|
||||||
thread_source = _dm_source(thread_id="1234567890.000001")
|
thread_source = _dm_source(thread_id="1234567890.000001")
|
||||||
thread_entry = store.get_or_create_session(thread_source)
|
thread_entry = store.get_or_create_session(thread_source)
|
||||||
store.append_to_transcript(thread_entry.session_id, {
|
store.append_to_transcript(thread_entry.session_id, {
|
||||||
"role": "user", "content": "thread-only message"
|
"role": "user", "content": "thread-only message"
|
||||||
})
|
})
|
||||||
|
|
||||||
# Parent should still have only its original messages
|
|
||||||
parent_transcript = store.load_transcript(parent_entry.session_id)
|
parent_transcript = store.load_transcript(parent_entry.session_id)
|
||||||
assert len(parent_transcript) == 2
|
assert len(parent_transcript) == 2
|
||||||
assert all(m["content"] != "thread-only message" for m in parent_transcript)
|
assert all(m["content"] != "thread-only message" for m in parent_transcript)
|
||||||
|
|
||||||
def test_multiple_threads_get_independent_copies(self, store):
|
def test_multiple_threads_are_independent(self, store):
|
||||||
"""Each thread from the same parent gets its own copy."""
|
"""Each thread from the same parent starts empty and stays independent."""
|
||||||
parent_source = _dm_source()
|
parent_source = _dm_source()
|
||||||
parent_entry = store.get_or_create_session(parent_source)
|
parent_entry = store.get_or_create_session(parent_source)
|
||||||
for msg in PARENT_HISTORY:
|
for msg in PARENT_HISTORY:
|
||||||
|
|
@ -118,49 +109,43 @@ class TestDMThreadSeeding:
|
||||||
thread_b_source = _dm_source(thread_id="2222.000002")
|
thread_b_source = _dm_source(thread_id="2222.000002")
|
||||||
thread_b_entry = store.get_or_create_session(thread_b_source)
|
thread_b_entry = store.get_or_create_session(thread_b_source)
|
||||||
|
|
||||||
# Thread B should have parent history, not thread A's additions
|
# Thread B starts empty
|
||||||
thread_b_transcript = store.load_transcript(thread_b_entry.session_id)
|
thread_b_transcript = store.load_transcript(thread_b_entry.session_id)
|
||||||
assert len(thread_b_transcript) == 2
|
assert len(thread_b_transcript) == 0
|
||||||
assert all(m["content"] != "thread A message" for m in thread_b_transcript)
|
|
||||||
|
|
||||||
# Thread A should have parent history + its own message
|
# Thread A has only its own message
|
||||||
thread_a_transcript = store.load_transcript(thread_a_entry.session_id)
|
thread_a_transcript = store.load_transcript(thread_a_entry.session_id)
|
||||||
assert len(thread_a_transcript) == 3
|
assert len(thread_a_transcript) == 1
|
||||||
|
assert thread_a_transcript[0]["content"] == "thread A message"
|
||||||
|
|
||||||
def test_existing_thread_session_not_reseeded(self, store):
|
def test_existing_thread_session_preserved(self, store):
|
||||||
"""Returning to an existing thread session should not re-copy parent history."""
|
"""Returning to an existing thread session should not reset it."""
|
||||||
parent_source = _dm_source()
|
parent_source = _dm_source()
|
||||||
parent_entry = store.get_or_create_session(parent_source)
|
parent_entry = store.get_or_create_session(parent_source)
|
||||||
for msg in PARENT_HISTORY:
|
for msg in PARENT_HISTORY:
|
||||||
store.append_to_transcript(parent_entry.session_id, msg)
|
store.append_to_transcript(parent_entry.session_id, msg)
|
||||||
|
|
||||||
# Create thread session
|
|
||||||
thread_source = _dm_source(thread_id="1234567890.000001")
|
thread_source = _dm_source(thread_id="1234567890.000001")
|
||||||
thread_entry = store.get_or_create_session(thread_source)
|
thread_entry = store.get_or_create_session(thread_source)
|
||||||
store.append_to_transcript(thread_entry.session_id, {
|
store.append_to_transcript(thread_entry.session_id, {
|
||||||
"role": "user", "content": "follow-up"
|
"role": "user", "content": "follow-up"
|
||||||
})
|
})
|
||||||
|
|
||||||
# Add more to parent after thread was created
|
# Get the same thread session again
|
||||||
store.append_to_transcript(parent_entry.session_id, {
|
|
||||||
"role": "user", "content": "new parent message"
|
|
||||||
})
|
|
||||||
|
|
||||||
# Get the same thread session again (not new — created_at != updated_at)
|
|
||||||
thread_entry_again = store.get_or_create_session(thread_source)
|
thread_entry_again = store.get_or_create_session(thread_source)
|
||||||
assert thread_entry_again.session_id == thread_entry.session_id
|
assert thread_entry_again.session_id == thread_entry.session_id
|
||||||
|
|
||||||
# Should still have 3 messages (2 seeded + 1 follow-up), not re-seeded
|
# Should still have only its own message
|
||||||
thread_transcript = store.load_transcript(thread_entry_again.session_id)
|
thread_transcript = store.load_transcript(thread_entry_again.session_id)
|
||||||
assert len(thread_transcript) == 3
|
assert len(thread_transcript) == 1
|
||||||
assert thread_transcript[2]["content"] == "follow-up"
|
assert thread_transcript[0]["content"] == "follow-up"
|
||||||
|
|
||||||
|
|
||||||
class TestDMThreadSeedingEdgeCases:
|
class TestDMThreadIsolationEdgeCases:
|
||||||
"""Edge cases and conditions where seeding should NOT happen."""
|
"""Edge cases — threads always start empty regardless of context."""
|
||||||
|
|
||||||
def test_no_seeding_for_group_threads(self, store):
|
def test_group_thread_starts_empty(self, store):
|
||||||
"""Group/channel threads should not trigger seeding."""
|
"""Group/channel threads should also start empty."""
|
||||||
parent_source = _group_source()
|
parent_source = _group_source()
|
||||||
parent_entry = store.get_or_create_session(parent_source)
|
parent_entry = store.get_or_create_session(parent_source)
|
||||||
for msg in PARENT_HISTORY:
|
for msg in PARENT_HISTORY:
|
||||||
|
|
@ -172,7 +157,7 @@ class TestDMThreadSeedingEdgeCases:
|
||||||
thread_transcript = store.load_transcript(thread_entry.session_id)
|
thread_transcript = store.load_transcript(thread_entry.session_id)
|
||||||
assert len(thread_transcript) == 0
|
assert len(thread_transcript) == 0
|
||||||
|
|
||||||
def test_no_seeding_without_parent_session(self, store):
|
def test_thread_without_parent_session_starts_empty(self, store):
|
||||||
"""Thread session without a parent DM session should start empty."""
|
"""Thread session without a parent DM session should start empty."""
|
||||||
thread_source = _dm_source(thread_id="1234567890.000001")
|
thread_source = _dm_source(thread_id="1234567890.000001")
|
||||||
thread_entry = store.get_or_create_session(thread_source)
|
thread_entry = store.get_or_create_session(thread_source)
|
||||||
|
|
@ -180,34 +165,21 @@ class TestDMThreadSeedingEdgeCases:
|
||||||
thread_transcript = store.load_transcript(thread_entry.session_id)
|
thread_transcript = store.load_transcript(thread_entry.session_id)
|
||||||
assert len(thread_transcript) == 0
|
assert len(thread_transcript) == 0
|
||||||
|
|
||||||
def test_no_seeding_with_empty_parent(self, store):
|
def test_dm_without_thread_starts_empty(self, store):
|
||||||
"""If parent session exists but has no transcript, thread starts empty."""
|
"""Top-level DMs (no thread_id) should start empty as always."""
|
||||||
parent_source = _dm_source()
|
|
||||||
store.get_or_create_session(parent_source)
|
|
||||||
# No messages appended to parent
|
|
||||||
|
|
||||||
thread_source = _dm_source(thread_id="1234567890.000001")
|
|
||||||
thread_entry = store.get_or_create_session(thread_source)
|
|
||||||
|
|
||||||
thread_transcript = store.load_transcript(thread_entry.session_id)
|
|
||||||
assert len(thread_transcript) == 0
|
|
||||||
|
|
||||||
def test_no_seeding_for_dm_without_thread_id(self, store):
|
|
||||||
"""Top-level DMs (no thread_id) should not trigger seeding."""
|
|
||||||
source = _dm_source()
|
source = _dm_source()
|
||||||
entry = store.get_or_create_session(source)
|
entry = store.get_or_create_session(source)
|
||||||
|
|
||||||
# Should just be a normal empty session
|
|
||||||
transcript = store.load_transcript(entry.session_id)
|
transcript = store.load_transcript(entry.session_id)
|
||||||
assert len(transcript) == 0
|
assert len(transcript) == 0
|
||||||
|
|
||||||
|
|
||||||
class TestDMThreadSeedingCrossPlatform:
|
class TestDMThreadIsolationCrossPlatform:
|
||||||
"""Verify seeding works for platforms beyond Slack."""
|
"""Verify thread isolation is consistent across all platforms."""
|
||||||
|
|
||||||
@pytest.mark.parametrize("platform", [Platform.SLACK, Platform.TELEGRAM, Platform.DISCORD])
|
@pytest.mark.parametrize("platform", [Platform.SLACK, Platform.TELEGRAM, Platform.DISCORD])
|
||||||
def test_seeding_works_across_platforms(self, store, platform):
|
def test_thread_starts_empty_across_platforms(self, store, platform):
|
||||||
"""DM thread seeding should work for any platform that uses thread_id."""
|
"""DM thread sessions start empty regardless of platform."""
|
||||||
parent_source = _dm_source(platform=platform)
|
parent_source = _dm_source(platform=platform)
|
||||||
parent_entry = store.get_or_create_session(parent_source)
|
parent_entry = store.get_or_create_session(parent_source)
|
||||||
for msg in PARENT_HISTORY:
|
for msg in PARENT_HISTORY:
|
||||||
|
|
@ -217,5 +189,4 @@ class TestDMThreadSeedingCrossPlatform:
|
||||||
thread_entry = store.get_or_create_session(thread_source)
|
thread_entry = store.get_or_create_session(thread_source)
|
||||||
|
|
||||||
thread_transcript = store.load_transcript(thread_entry.session_id)
|
thread_transcript = store.load_transcript(thread_entry.session_id)
|
||||||
assert len(thread_transcript) == 2
|
assert len(thread_transcript) == 0
|
||||||
assert thread_transcript[0]["content"] == "What's the weather?"
|
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue