fix(mattermost): keep thread-mode replies in the Mattermost thread#20874
fix(mattermost): keep thread-mode replies in the Mattermost thread#20874potatosalad wants to merge 4 commits into
Conversation
|
I have been using this on top of v0.13 for several days with no issues; would be happy to see it merged |
|
Agreed. We need this to make Mattermost useful. Otherwise I cannot use this integration at all. I had coded up a fix but this PR is much better than mine! |
|
@teknium1 Did the changes that were manually merged address everything that this pull request addresses or should I rebase and resolve merge conflicts? |
|
Hey @potatosalad — I run Hermes against a Mattermost server and this one drives me up the wall, so I went digging to answer the question you asked @teknium1 (whether the manual merge already covered this). From what I can tell, it didn't — not fully. What landed ( The other thing worth knowing: the reason it won't rebase cleanly isn't that it's obsolete — the adapter got moved out from under it. Figured that was worth flagging since you'd asked whether it was already covered. |
|
@legard Ah cool, I'll get it rebased soon then. |
… thread
Two bugs surfaced when MATTERMOST_REPLY_MODE=thread was enabled:
1. Replies inside an existing thread failed with HTTP 400 from
POST /api/v4/posts:
api.post.create_post.root_id.app_error
"Invalid RootId parameter."
The adapter was passing reply_to (= the user's message id) straight
through as root_id, but Mattermost requires root_id to reference the
thread *root* — a post that has no root_id of its own. When the user
replied inside an existing thread, their post id was a reply, so the
server rejected it. Reproduced with a direct API call: a root_id
pointing at a reply returns 400 with the same error code; pointing at
a top-level post succeeds.
Fix: add a small _resolve_thread_root helper that does one cached
GET /posts/{id} lookup and returns the post's root_id when present,
otherwise the id itself. The three send sites (text, single media,
multi-image) all route reply_to through the helper, so root_id always
points at a real thread root. Missing/deleted posts fall back to the
original id so callers see the same error they would have seen
pre-fix instead of a silent rewrite.
2. The typing indicator showed up in the main channel even when the
user was conversing inside a thread. The dispatcher already passes
metadata={"thread_id": event.source.thread_id} into send_typing, but
the adapter ignored it and only sent {"channel_id": chat_id}.
Fix: when metadata carries a thread_id, forward it as Mattermost's
parent_id field so "hermes is typing…" appears inside the thread
the user just messaged from.
Tests: existing test_send_with_thread_reply updated to mock the new
_api_get lookup (still asserts that a root post is used unchanged), plus
new coverage for resolution-from-reply, lookup caching, missing-post
fallback, and parent_id behaviour in send_typing.
…hread
Even after threading was wired up for the main reply, tool-call previews,
reasoning chunks, background-task notifications, and other intermediate
sends still landed in the main channel.
Cause: the dispatcher fires those sends as
adapter.send(chat_id, content, metadata={"thread_id": ...})
— with no `reply_to` — but the Mattermost adapter was only computing
`root_id` from `reply_to`. With no `reply_to`, no `root_id` got attached,
so the post defaulted to the main channel. Media helpers had a related
bug: `send_image` / `send_image_file` / `send_document` / `send_voice`
/ `send_video` accept `metadata` but were dropping it on the way to the
internal `_send_url_as_file` / `_send_local_file` helpers, and
`send_multiple_images` never set `root_id` at all.
Fix: factor a `_root_id_for_payload(reply_to, metadata)` helper that
prefers an explicit `reply_to` (resolved to the thread root) and falls
back to `metadata['thread_id']` (already a root post id from
`event.source.thread_id`). Use it at every outbound-post site:
- `send` (text)
- `_send_url_as_file` (image URLs)
- `_send_local_file` (local image / file / audio / video)
- `send_multiple_images` (Mattermost-native multi-image posts)
Plumb `metadata` through the public media methods to the internal helpers.
Reply mode "off" continues to suppress threading entirely; the helper
returns `None` in that case.
Tests: covers the metadata-only fallback (root_id from thread_id),
explicit `reply_to` winning over metadata, reply_mode "off" suppressing
both, and `metadata` being forwarded through every public media method to
its internal helper.
Thread mode now treats handled top-level channel posts as the root of the bot's reply thread, so progress messages, streaming chunks, media sends, and follow-up replies share the same thread metadata and Hermes session key. DMs stay unthreaded unless Mattermost provides a real root_id so their stable DM session behavior is preserved. Also avoid caching failed post lookups in _resolve_thread_root, since _api_get returns an empty dict for transient API/network failures as well as missing posts. When reply_to resolution fails, prefer trusted dispatcher thread metadata before falling back to the original reply id. Typing indicators now honor reply_mode too: parent_id is only sent when thread mode is enabled, matching the actual response routing. Tests cover top-level channel thread metadata, DM preservation, lookup failure caching, metadata fallback, and reply_mode-gated typing.
c8c8926 to
54966ac
Compare
|
Hi @teknium1, when you get a chance, could you review this? It's an important issue for corporate users relying on the agent, and I'd really value your call on whether it's something you'd consider merging. |
What does this PR do?
Fixes Mattermost thread reply mode end to end while keeping Mattermost DMs flat.
Before this branch, Hermes could lose Mattermost thread context in several places:
root_id, which Mattermost rejects becauseroot_idmust point at the original thread rootreply_to, so they could leak back into the parent channel instead of staying in the user's threadroot_id/ typingparent_idpayloads whenMATTERMOST_REPLY_MODE=threadwas enabled, which breaks continuation inside Mattermost DMsThe adapter now resolves reply posts to real thread roots, preserves thread metadata across normal sends, typing indicators, progress/status updates, and media sends, anchors top-level channel/group turns to the user's post, and treats DMs as flat conversations regardless of thread mode. Channel/group threading behavior is preserved.
Related Issue
#4221 - [Bug]: Mattermost threaded conversations ignore follow-ups and leak tool output to the main channel
#12063 - Mattermost adapter ignores metadata.thread_id, so gateway thread replies post to channel root
Type of Change
Changes Made
gateway/platforms/mattermost.pyroot_idselection so normal text sends, URL/file attachments, local media uploads, batched image posts, fallback text sends, and progress/intermediate sends can use the same thread metadata behavior.metadata["thread_id"]when there is no explicitreply_to, keeping progress, status, tool, media, and other intermediate messages inside the active Mattermost thread.parent_idfor channel/group threads.thread_idso the rest of the turn stays anchored to that user's thread.chat_id.root_idand typingparent_idfor DMs, even when thread mode is enabled andreply_toormetadata["thread_id"]is present.root_idvalues so accidental old DM thread replies do not split the stable DM session.tests/gateway/test_mattermost.pyroot_id.website/docs/user-guide/messaging/mattermost.mdHow to Test
scripts/run_tests.sh tests/gateway/test_mattermost.py -q.reply_mode: threadorMATTERMOST_REPLY_MODE=thread.root_id.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) - Mattermost messaging docs updatedcli-config.yaml.exampleif I added/changed config keys - N/A, no config keys changedCONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows - N/A, no architecture/workflow changeScreenshots / Logs
python3 -m py_compile gateway/platforms/mattermost.py tests/gateway/test_mattermost.pypassed.scripts/run_tests.sh tests/gateway/test_mattermost.py tests/gateway/test_send_multiple_images.pydid not run in this checkout because no virtualenv was found at.venv,venv, or$HOME/.hermes/hermes-agent/venv; the availablepython3also does not have project dependencies such aspytestandyamlinstalled.