fix(mattermost): prefer metadata thread root over stale reply_to

[imapotato123]

Reworked per the hermes-sweeper review to the narrow salvageable scope.

What changed

Rebased onto current main (d83e85850) and reduced to the single outbound fix. The adapter path migration to plugins/platforms/mattermost/adapter.py (af973e407) is picked up, so the branch no longer conflicts.

Dropped — inbound channel-thread isolation, which landed on main in 5a0e0d35b with coverage at tests/gateway/test_mattermost.py:1029-1085. The DM guard at adapter.py:869-876 is untouched, so DM sessions keep their existing behavior and the test_mattermost_dm_post_does_not_seed_thread_root regression still passes.

Kept — the outbound metadata-precedence case in _thread_root_for_send() (adapter.py:181-194), where main still prefers reply_to over metadata.

The remaining bug

_thread_root_for_send() reads reply_to first and consults metadata["thread_id"] only when reply_to is empty. When both are present and disagree — delayed or progress deliveries where reply_to points at a post in a different thread — the message is posted under the wrong root.

metadata["thread_id"] is the thread root recorded when the inbound post was received, so this makes it authoritative and falls back to reply_to only when metadata carries no root. The fallback path is unchanged for ordinary replies.

Tests

Two added, per the review request:

• test_send_prefers_metadata_thread_id_over_conflicting_reply_to — conflicting reply_to + metadata["thread_id"]. Also asserts the stale reply_to never reaches _resolve_root_id(), which drops an API round-trip on this path.
• test_send_falls_back_to_reply_to_without_metadata_thread_id — guards the fallback.

Verified the first test fails on unpatched main with assert 'other_thread_post_999' == 'root_post_123', and passes with the change.

uv run --with pytest==9.0.2 --with pytest-asyncio==1.3.0 --with pytest-timeout==2.4.0 --with aiohttp==3.13.2 pytest tests/gateway/test_mattermost.py -q -o addopts=
65 passed

Also clean: python3 -m py_compile on both files, git diff --check.

Diff is +49/−3 across 2 files. The previous branch tip is preserved at imapotato123:backup/pr12299-pre-rework if any of the dropped work is wanted later.

[mxnstrexgl]

✓ Automated scan: no security concerns. Mattermost threading fix.

[alt-glitch]

Supersedes #12283. Related to #12063, #12096, #6617 — same Mattermost threading code path.

[lipaysamart]

Are there any merger plans?

[imapotato123]

Heads-up — the original PR included three fixes; the edit_message **kwargs hunk was independently shipped by upstream 6c0c6259 (in v2026.4.23) with a stricter *, finalize: bool = False form and a parametrized regression test across all 7 non-DingTalk adapters. A prior rebase of this branch onto upstream main already silently dropped the now-redundant hunk, so the diff currently reflects only the two genuinely-needed fixes (root_id resolution from metadata["thread_id"] and per-thread session keying for top-level messages). Updating the description to match the actual diff — both remaining fixes still apply, since upstream/main:gateway/platforms/mattermost.py still has the broken behavior at lines 273-274 and 655-656.

[imapotato123]

Rebased onto current main (clean merge). Also cleaned up the commit message to accurately reflect the two fixes in the diff.

This is a small change (+19/−5) fixing two thread-handling bugs in the Mattermost platform — happy to address any feedback. Would appreciate a review when someone has a chance. 🙏

[imapotato123]

Rebased onto current main. This is a small 2-fix PR for Mattermost threading (root_id resolution + per-thread session keying). Would appreciate a review when you get a chance. cc @teknium1

[imapotato123]

Rebased/refreshed this PR onto current main and narrowed the scope to the Mattermost thread/session bugs still missing upstream.

Changes in the refreshed branch:

• Dropped the older root-id hunk that conflicted with upstream's _resolve_root_id() work.
• Preserved upstream's _resolve_root_id(reply_to) fallback for normal replies.
• Added support for metadata["thread_id"] as the authoritative Mattermost thread root for outbound deliveries where reply_to is absent or points at a reply.
• In thread reply mode, top-level Mattermost posts now use their own post id as source.thread_id, so unrelated top-level channel threads do not share one Hermes session.
• Added focused tests for metadata thread roots and top-level thread session isolation.

Validation:

• PYTHONPATH=$PWD /Users/admin/.hermes/hermes-agent/venv/bin/python -m pytest tests/gateway/test_mattermost.py -q -o addopts=
• Result: 46 passed

I also tried the broader tests/gateway/ suite locally, but that run exhausted the macOS open-file limit in unrelated async/voice/webhook tests. The focused Mattermost suite passes cleanly.

[imapotato123]

Refreshed this PR onto current main (72ff3e909). The branch is still a single focused Mattermost threading/session commit.

Validation from the refreshed branch:

• uv run --with pytest==9.0.2 --with pytest-asyncio==1.3.0 --with pytest-timeout==2.4.0 --with aiohttp==3.13.2 pytest tests/gateway/test_mattermost.py -q -o addopts=
  • 46 passed
• python3 -m py_compile gateway/platforms/mattermost.py tests/gateway/test_mattermost.py
• git diff --check origin/main..HEAD

[johnkattenhorn]

Independent confirmation from a real deployment

Just hit this bug on a self-hosted Mattermost install (LXC running Hermes off main at bc3f1f4, single-user EA setup) and can confirm this patch fixes it.

Repro before the patch (reply_mode=thread)

User posts top-level in ~hermes-home: bot replies, creating a thread. User replies inside that thread with a follow-up that requires context from turn 1 ("what was the number I asked you to remember?"). Bot loses context — runs session_search, finds nothing, replies "I don't know what you're asking about."

Inspection of ~/.hermes/sessions/sessions.json showed two distinct sessions for the same user in the same channel:

agent:main:mattermost:group:<channel>:7srqno17s7g5tg5fj6czzsc9ir
  origin.thread_id = null
  (top-level post — keyed by user_id because thread_id was None)

agent:main:mattermost:group:<channel>:55cwfjrtc7f13mhq7ca9z3uyqa
  origin.thread_id = "55cwfjrtc7f13mhq7ca9z3uyqa"
  (thread reply — keyed by Mattermost root_id)

Same user, same channel, but the second turn lands in a fresh session and the conversation is split. This is exactly the failure mode described in #18279.

Day-one workaround was MATTERMOST_REPLY_MODE=off (flat inline replies) — functional, but the thread experience is part of the point of using Mattermost.

After applying this PR

Patched gateway/platforms/mattermost.py with git apply against the diff in this PR, flipped MATTERMOST_REPLY_MODE back to thread, restarted the systemd gateway. Re-ran the same repro. Bot answered correctly with full context.

New sessions.json after the patched turn shows a single session, keyed by the top-level post id:

agent:main:mattermost:group:<channel>:nrgh4e8xwpd8indhf9811e591e
  origin.thread_id = "nrgh4e8xwpd8indhf9811e591e"
  (both top-level and thread reply share this session — keyed by post.id per the new branch in _handle_ws_event)

That matches the intent of the change exactly — thread_id = post.get("id") on top-level posts in thread mode means both turns share the session, and the Slack-style consistency note in the PR description rings true.

Test coverage

The three new tests in this PR cover the exact failure mode I hit:

• test_top_level_post_in_thread_mode_uses_post_id_as_thread_id — top-level post side
• test_send_thread_prefers_metadata_thread_id_over_reply_to — outbound side
• test_send_thread_uses_metadata_thread_id_when_no_reply_to — covers the "cron post into a channel without an inbound reply_to" case (which would have bitten me on the scheduled-summary feature next)

Carrying this locally

For the record — vendored the diff as a local hot-fix on my install until this merges, and documented the carry / drop process in my repo's hermes-agent docs so it doesn't get stuck applied after upstream lands.

Happy to provide more traces / sessions.json fragments if useful. +1 to merging this — it's the smallest diff that fixes the bug, the tests directly cover the failure mode, and it's been validated on a real (if small) deployment.

[alt-glitch]

This was generated by AI during triage.

Verified: diff confirms the two-fix mechanism (metadata["thread_id"] as authoritative outbound root + per-thread session keying where top-level posts in reply_mode=thread key by their own post.id). Independently confirmed on a real self-hosted deployment (@johnkattenhorn, sessions.json traces showing context bleed before / single shared session after the patch). Bumping P3 -> P2: verified session-state bug in a core gateway path (gateway/platforms/mattermost.py, not a plugin) with independent corroboration; workaround (MATTERMOST_REPLY_MODE=off) exists so not higher.

Competing/related cluster on the same session-isolation mechanism: #18280 and #29993 (both OPEN, same root/session-keying approach), #12283 (superseded). This PR (#12299) is the earliest open one and looks canonical (smallest focused diff, composes with upstream's _resolve_root_id(), direct test coverage). Also related to merged salvage #47250 (thread delivery hygiene) and #54193/#54188 (in-thread auto-response, different scope). Not marking any as duplicate — flagging the cluster for a human to pick the canonical one.

[teknium1]

Thanks for the focused Mattermost investigation. Current main already contains the inbound channel-thread isolation, but the remaining outbound metadata-precedence case needs a narrow port.

Problems

• The PR's top-level-post branch does not exclude Mattermost DMs. Current main deliberately guards this at plugins/platforms/mattermost/adapter.py:869-876; without that guard, gateway/session.py:911-914 includes each top-level post ID in the DM session key and splits the conversation per message.
• The changed adapter path was migrated to plugins/platforms/mattermost/adapter.py in af973e407. The inbound behavior was subsequently merged in 5a0e0d35b, including coverage at tests/gateway/test_mattermost.py:1029-1085.

Suggested changes

• Salvage only the remaining outbound behavior into _thread_root_for_send() (plugins/platforms/mattermost/adapter.py:163-176), where current main still prefers reply_to over metadata.
• Add a conflicting reply_to plus metadata["thread_id"] test, and preserve the existing DM regression behavior.

Automated hermes-sweeper review.

[imapotato123]

@teknium1 — reworked to the scope you outlined. Rebased onto d83e85850; the conflict from the af973e407 adapter migration is resolved.

Addressing each point:

• DM exclusion — dropped the top-level-post branch entirely rather than adding a guard to it. The inbound behavior merged in 5a0e0d35b already covers it, and plugins/platforms/mattermost/adapter.py:869-876 is untouched, so gateway/session.py:911-914 never sees a per-post DM key. test_mattermost_dm_post_does_not_seed_thread_root passes unchanged.
• Adapter path migration — picked up; the diff now targets plugins/platforms/mattermost/adapter.py.
• Salvage the outbound behavior into _thread_root_for_send() — done, that's the whole diff now. Metadata root wins over reply_to; reply_to still drives threading when metadata has no root.
• Conflicting reply_to + metadata["thread_id"] test — added, plus a fallback test. The conflict test fails on unpatched main (assert 'other_thread_post_999' == 'root_post_123') and passes with the change.

tests/gateway/test_mattermost.py: 65 passed. Down to +49/−3 across 2 files.

Description updated to match the new scope, and the title now reflects the outbound fix rather than the original session-isolation framing.

[alt-glitch]

This was generated by AI during triage.

Related to #20874: current #12299 narrowly makes recorded metadata.thread_id authoritative when it conflicts with reply_to; #20874 resolves reply_to first and uses metadata as fallback. This is an intended-policy choice, not a duplicate.

[imapotato123]

Small correction on the triage note above, since it affects how these two get compared.

The note says #20874 "resolves reply_to first and uses metadata as fallback." At its current head (21d6f3700, force-pushed 2026-07-30) it does the opposite — _thread_root_for_send() reads metadata["thread_id"] or metadata["root_id"] first and returns immediately when either is set, consulting reply_to only when metadata carries no root:

metadata_root_id = None
if isinstance(metadata, dict):
    metadata_root_id = metadata.get("thread_id") or metadata.get("root_id")
if metadata_root_id:
    return str(metadata_root_id)
if not reply_to:
    return None

That's the same precedence this PR adopts, so the two aren't in policy conflict. The note may have been written against an earlier revision.

Given that, I don't think "not a duplicate" holds for the reason stated. #20874 is a superset — it adds configurable auto-threading, channel-type caching, parent_id on the payload, config knobs and docs, and along the way it covers the outbound precedence fix this PR is now reduced to. If it lands, there is nothing left here.

One behavioral difference does survive, and it runs the other way: #20874 returns metadata_root_id directly, while this PR passes it through _resolve_root_id(). If a metadata["thread_id"] ever points at a reply rather than a thread root, #20874 posts with a non-root root_id and Mattermost rejects it with "Invalid RootId parameter" — the failure _resolve_root_id() exists to prevent. Narrow, but it's the one case this PR handles that #20874 does not.

Happy to close this as superseded and land that hardening as a one-line change on #20874 instead — that's likely the faster path to a fix in main. Deferring to whichever the maintainers prefer.

[imapotato123]

Closing this as superseded by #20874.

As covered above, #20874 adopts the same metadata-over-reply_to precedence this PR was reduced to, and does considerably more around it — configurable auto-threading, channel-type caching, parent_id on the payload, config and docs. There's nothing left here that it doesn't cover.

The one exception was the _resolve_root_id() guard on the metadata path. Rather than let it lapse with this PR, I've opened it against potatosalad's branch so it can land as part of #20874: potatosalad#1. It's a one-line change plus tests, and it does reverse a deliberate no-lookup optimization on their side, which I've flagged explicitly there for them to accept or reject.

Thanks @alt-glitch and @teknium1 for the triage and review, and @johnkattenhorn for reproducing it on a real deployment — that's what moved it from "probably real" to actionable.