Skip to content

fix(mattermost): keep thread root and replies in one session - #37144

Open
brendanstennett wants to merge 1 commit into
NousResearch:mainfrom
brendanstennett:fix/mattermost-thread-session
Open

fix(mattermost): keep thread root and replies in one session#37144
brendanstennett wants to merge 1 commit into
NousResearch:mainfrom
brendanstennett:fix/mattermost-thread-session

Conversation

@brendanstennett

Copy link
Copy Markdown

What does this PR do?

Fixes a context-loss bug in the Mattermost adapter when MATTERMOST_REPLY_MODE=thread.

In thread mode, the agent lost all conversation context the moment a user
followed up inside a thread — every threaded reply was treated as a brand-new
conversation.

Root cause: in Mattermost the root post of a thread has an empty root_id,
while its replies carry root_id=<root post id>. The adapter derived the
gateway thread_id straight from root_id, so the opening message produced
session key …:dm:<chat> (no thread suffix) while every reply in the bot's
thread produced …:dm:<chat>:<root_id>. The two keys never matched, so the
first follow-up always started a fresh, empty session.

When reply_mode is "thread" the bot nests its replies under the user's
original post, so every later reply arrives with root_id=<that post id>. The
fix seeds thread_id from the post's own id for root posts in thread mode, so
the root message and all of its replies resolve to a single session key. The
fallback is guarded on reply_mode == "thread"; under "off" the bot posts
flat replies and root posts intentionally stay un-threaded, so we don't spawn a
stray per-post session.

Related Issue

Fixes #18279

#18279 ("Mattermost root threads share one session and block parallel
conversations") identifies the same root cause this PR fixes — root posts enter
the gateway with thread_id = post.get("root_id") or None, so for a root post
(empty root_id) thread_id is None and build_session_key() keys by
channel only, splitting the root message and its threaded replies across
different sessions.

Scope note vs. #18279: that issue proposes setting thread_id = post.id for
non-DM top-level posts and explicitly keeps DM behavior unchanged. This PR
instead guards the fallback on reply_mode == "thread" and applies it to all
chat types including DMs, because the reported failure occurred in a DM
(observed session keys were …:mattermost:dm:…) — the non-DM-only fix would not
resolve it. The reply_mode == "thread" form still satisfies #18279's
parallel-conversation goal: distinct root posts get distinct post.id values,
hence distinct sessions.

Related (not fixed here):

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • plugins/platforms/mattermost/adapter.py — in _handle_ws_event, seed
    thread_id from the post's own id for root posts (empty root_id) when
    reply_mode == "thread", so a thread's root and replies share one session
    key. Behavior under reply_mode == "off" is unchanged.
  • tests/gateway/test_mattermost.py — add TestMattermostThreadSessionContinuity
    covering root/reply thread_id derivation in both reply modes, plus an
    end-to-end assertion that the root post and a threaded reply build the same
    build_session_key.

How to Test

  1. Run the adapter's test suite — all pass, including the 4 new continuity tests:
    pytest tests/gateway/test_mattermost.py -q47 passed.
  2. Prove the tests guard the bug: temporarily revert the two added lines in
    adapter.py and re-run — test_thread_mode_root_post_seeds_thread_id_from_own_id
    and test_root_and_reply_share_session_key_in_thread_mode fail, with the diff
    showing …:dm:<chat> vs …:dm:<chat>:<root_id> (the two mismatched keys).
  3. Manual repro (optional): with MATTERMOST_REPLY_MODE=thread, DM/mention the
    bot, then reply inside the thread it creates. Before the fix the follow-up
    starts a fresh session (no memory of the first message); after the fix the
    thread retains context across replies.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: Ubuntu (Linux 7.0.0-22-generic), Python 3.11

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Screenshots / Logs

The mismatched session keys that caused the bug, surfaced by the regression
test when the fix is reverted:

- agent:main:mattermost:dm:chan_456:root_post_123   (a threaded reply)
+ agent:main:mattermost:dm:chan_456                  (the opening message)

With the fix applied, both resolve to agent:main:mattermost:dm:chan_456:root_post_123.

With MATTERMOST_REPLY_MODE=thread the agent lost all context as soon as
the user followed up inside a thread — every threaded reply was treated
as a brand-new conversation.

In Mattermost a root post (the first message of a thread) has an empty
root_id, while its replies carry root_id=<root post id>. The adapter
derived thread_id straight from root_id, so the opening message produced
a session key without a thread suffix (…:dm:<chat>) while every reply in
the bot's thread produced …:dm:<chat>:<root_id>. The two keys never
matched, so the first follow-up always started a fresh, empty session.

When reply_mode is "thread" the bot nests its replies under the user's
original post, so every later reply arrives with root_id=<that post id>.
Seed thread_id from the post's own id for root posts in thread mode so
the root message and all of its replies resolve to a single session key.
The fallback is guarded on reply_mode == "thread"; under "off" the bot
posts flat replies and root posts stay un-threaded, so a stray per-post
session is never created.

Adds TestMattermostThreadSessionContinuity covering the root/reply
thread_id derivation in both modes plus an end-to-end assertion that the
root post and a threaded reply build the same session key.
@alt-glitch alt-glitch added type/bug Something isn't working comp/plugins Plugin system and bundled plugins platform/matrix Matrix adapter (E2EE) P3 Low — cosmetic, nice to have and removed platform/matrix Matrix adapter (E2EE) labels Jun 2, 2026
crisap94 added a commit to crisap94/hermes-agent that referenced this pull request Jun 10, 2026
…ity)

The Mattermost adapter only saw the single message it was @mentioned in and
required a fresh @mention every turn. The Slack adapter already solves both;
this ports that pattern to Mattermost.

- In-thread auto-response: after the first @mention in a thread, subsequent
  messages in that thread auto-trigger the bot. Opt out with
  MATTERMOST_STRICT_MENTION=true (mirrors SLACK_STRICT_MENTION).
- Thread context: on the first turn in a pre-existing thread, prior messages
  are fetched (GET /posts/{root}/thread) and injected via channel_context.
- Security: injected thread history is filtered to allowlisted authors
  (MATTERMOST_ALLOWED_USERS / *_ALLOW_ALL_USERS). The authz layer only checks
  the triggering author; channel_context bypasses it, so unfiltered history
  would leak non-allowlisted users' messages into the model.
- Session-key parity: _has_active_session_for_thread passes the adapter's real
  chat_type so the key matches what handle_message persists.

Adds TestMattermostThreadBehavior (12 cases) and documents
MATTERMOST_STRICT_MENTION + the allowlist-filtered context behavior.

Related: NousResearch#38362, NousResearch#38152, NousResearch#37144 (thread-context PRs without the allowlist filter).
@crisap94

Copy link
Copy Markdown

👋 Heads-up + a vote of support: this root-post session-continuity fix is a prerequisite for in-thread auto-response on Mattermost, which I'm proposing in #43804. Without it, a root-post @mention registers under …:<chat> while follow-ups arrive under …:<chat>:<root_id>, so the bot loses the thread.

To keep #43804 reviewable standalone I included the same 2-line fix there with credit to you — happy to rebase and drop it the moment this lands so there's no duplication. Either order works; just flagging the dependency. Thanks for this fix 🙏

crisap94 added a commit to crisap94/hermes-agent that referenced this pull request Jun 10, 2026
After the bot is @mentioned in a thread, subsequent messages in that thread
auto-trigger it without a new @mention (Slack parity). Opt out with
MATTERMOST_STRICT_MENTION=true.

- _mentioned_threads tracks engaged threads; _has_active_session_for_thread
  passes the adapter's real chat_type so the session key matches what
  handle_message persists.
- Includes the root-post session-continuity fix (same as NousResearch#37144 by
  @brendanstennett): a Mattermost root post has an empty root_id, so in thread
  mode thread_id is seeded from the post's own id to keep the root and its
  replies on one session. Auto-response depends on this, so it is included here
  with credit; happy to drop it if NousResearch#37144 lands first.
- Auto-response does not bypass authz: every message is still authorized by
  author downstream.

Adds TestMattermostThreadSessionContinuity + TestMattermostInThreadAutoResponse
and documents MATTERMOST_STRICT_MENTION.

Related: NousResearch#37144 (session fix, credited)
@matt-pulsipher

Copy link
Copy Markdown

User impact: this is the bug that makes the channel unusable. The user's first message in a thread starts session A, the first reply in the thread starts session B, and the agent loses all context on the threaded follow-up. Would love to see this land.

wernerhp added a commit to wernerhp/hermes-agent that referenced this pull request Jul 2, 2026
… (PR NousResearch#37144)

Root DM posts had thread_id=None; replies carried root_id=<root>.
Result: two different session keys → agent lost all context on first reply.

Prior fix excluded DM channels (channel_type_raw != 'D' guard).
This removes that exclusion so DMs get the same treatment:
  thread_id = post.get('root_id') or post_id (in thread mode)

Port of upstream PR NousResearch#37144 extended to cover DM channel type.
wernerhp added a commit to wernerhp/hermes-agent that referenced this pull request Jul 2, 2026
…ix (NousResearch#37144)

The prior test asserted thread_id=None for DM root posts — which was the
old broken behaviour that caused session context loss on first threaded reply.

Commit a062424fb intentionally removed the 'channel_type_raw != D' guard
so DMs get the same root-post seeding as channels.  Update the test to:
  - Rename and doc the test to describe the fixed behaviour
  - Assert thread_id == post_id for DM root posts (correct)
  - Add companion test: DM replies carry root_id → same session key
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for isolating the root/reply session-key mismatch and adding focused regression coverage.

Problems

  • Current main has since added a related fallback at plugins/platforms/mattermost/adapter.py:869-876, but it explicitly excludes DMs (channel_type_raw != "D"). The PR is now conflicting, so its additive hunk cannot be cherry-picked as-is.
  • tests/gateway/test_mattermost.py:1060-1085 currently asserts that a threaded DM root leaves source.thread_id unset. The PR’s intended DM behavior needs to deliberately replace that contract, not add a second, contradictory test path.

Suggested changes

  • When salvaging, resolve the current conditional and update the existing DM test into a root/reply build_session_key() continuity regression. gateway/session.py:886-915 confirms that a populated DM thread_id is the discriminator needed for the two inbound posts to share the thread session.

This is an automated hermes-sweeper review.

@alt-glitch alt-glitch added the sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state label Jul 13, 2026
@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform area/sessions Session lifecycle, resume, persistence, history labels Jul 13, 2026
wernerhp added a commit to wernerhp/hermes-agent that referenced this pull request Aug 4, 2026
… (PR NousResearch#37144)

Root DM posts had thread_id=None; replies carried root_id=<root>.
Result: two different session keys → agent lost all context on first reply.

Prior fix excluded DM channels (channel_type_raw != 'D' guard).
This removes that exclusion so DMs get the same treatment:
  thread_id = post.get('root_id') or post_id (in thread mode)

Port of upstream PR NousResearch#37144 extended to cover DM channel type.
wernerhp added a commit to wernerhp/hermes-agent that referenced this pull request Aug 4, 2026
…ix (NousResearch#37144)

The prior test asserted thread_id=None for DM root posts — which was the
old broken behaviour that caused session context loss on first threaded reply.

Commit a062424fb intentionally removed the 'channel_type_raw != D' guard
so DMs get the same root-post seeding as channels.  Update the test to:
  - Rename and doc the test to describe the fixed behaviour
  - Assert thread_id == post_id for DM root posts (correct)
  - Add companion test: DM replies carry root_id → same session key
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/sessions Session lifecycle, resume, persistence, history comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Mattermost root threads share one session and block parallel conversations

5 participants