fix(mattermost): make thread routing configurable and reliable - #20874
fix(mattermost): make thread routing configurable and reliable#20874potatosalad wants to merge 5 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. |
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. |
|
I am new to Hermes and trying out MatterMost as the chat interface. An issue I've seen is that when you start a thread Hermes can lose context of earlier messages. For example I asked it to write a skill about my kubernetes setup; ~30 mins later when I asked for a follow up modification in the same thread, it had no idea what I was talking about:
Would this fix also resolve that issue? 🤔 I can try to test out this PR on my instance perhaps... |
Yes, that's one of the issues that this PR fixes for me, too. |
|
I'm concerned with flattening DMs in thread mode. If I send a DM and the mode is I'm 100% in agreement with the rest of this. I just don’t think DM threads should all collapse into one flat DM session when thread mode is enabled. For workflows where bots send multiple separate task/status messages by DM, those threads need to stay scoped independently or the contexts bleed together. Cron probably needs to be looked at as well. If a cron message is delivered automatically and I reply to it in a thread, the Hermes agent should know the context I’m replying to instead of me having to explain what the thread is about. |
|
Current-state check against current
@potatosalad could you rebase once more and resolve that test-file conflict? I don't think this should be closed as obsolete: current |
_thread_root_for_send() returns metadata["thread_id"] directly as the Mattermost root_id. Mattermost requires root_id to be the root of a thread and rejects a reply's ID with "Invalid RootId parameter", and a recorded thread_id is not guaranteed to be a root — delayed deliveries and synthesized metadata can carry a reply's ID. Resolve it through _resolve_root_id() the same way the reply_to branch below already does. A failed lookup returns None and may be transient, so fall back to the recorded ID rather than dropping the thread. This trades the metadata path's no-lookup fast path for one GET per thread — _thread_root_cache absorbs every subsequent message, so the cost is per thread, not per message. test_thread_metadata_avoids_redundant_root_lookup pinned the old fast path and is replaced by a cache-behavior test asserting exactly one lookup across two sends. test_explicit_dm_thread_metadata_is_always _preserved kept its intent; its bare AsyncMock() only worked while the metadata path skipped the lookup, so it now returns real post data.
fix(mattermost): resolve metadata thread root before use
2b02df8 to
13bebda
Compare
|
@teknium1 This has been open for over three months. Is it still on track to merge, or are there remaining changes needed? |
|
I took another pass over this from the perspective of our Mattermost-based Hermes orgs. This PR covers the Mattermost behavior we have been carrying a small local patch for: task-rooted Mattermost sessions where top-level DMs can become their own Hermes session roots and replies under that Mattermost root reuse the same Hermes session. With The explicit split between channel and DM auto-threading is useful — for our operator/agent UX we want both enabled, but I can see why upstream defaults may stay conservative. The metadata-root precedence/resolution and preserving real existing roots are also important for avoiding the “same visible Mattermost thread, different Hermes session” failure mode. So from our usage: this PR is material and would replace downstream patching for better Mattermost support, assuming the new settings are configured explicitly. |
What does this PR do?
Rebases the remaining Mattermost thread work onto current
main, drops behavior that has already landed upstream, and makes automatic threading explicit for channels and direct messages.Current
mainalready contains the original PR's progress/media metadata plumbing and top-level channel root behavior. Those duplicate commits are no longer part of this branch.Remaining changes
Keep typing indicators inside the active Mattermost thread.
Cache successful post-to-root lookups while allowing transient lookup failures to retry.
Prefer trusted gateway thread metadata and preserve real Mattermost thread roots.
Add independent settings for automatic thread creation:
Add
MATTERMOST_AUTO_THREADandMATTERMOST_DM_AUTO_THREADoverrides.Deprecate
MATTERMOST_REPLY_MODEas an environment compatibility fallback. When neither new setting is present, legacythreadenables both policies; either new setting can override that fallback independently.Preserve explicit channel and DM thread roots regardless of the automatic-thread settings.
Resolve and cache the Mattermost channel type for uncached outbound targets, including cron/notification paths.
Retain the existing silent no-op for missing local files; no local path is posted to Mattermost.
Behavior
auto_thread: truedm_auto_thread: trueThe automatic-thread settings affect synthetic roots only. They never flatten a real existing thread.
Cron scope
This PR makes uncached outbound Mattermost targets classify correctly and preserves a real thread when a user replies there. Seeding the content of a proactively delivered cron brief into a newly created Mattermost thread session is cross-cutting scheduler/session work and remains separate from this adapter-focused PR.
Related work
Covered or consolidated here
metadata.thread_idrouting, real-root preservation, typing indicators, and side-channel sends.auto_threadanddm_auto_threadsettings allow either behavior without flattening an existing thread.Already upstream and deliberately omitted from this branch
Invalid RootIdfailures.mainalready contains.Related follow-ups not claimed by this PR
@mentionchecks after the bot is engaged in a thread (sticky thread ownership).mattermost.reply_modeYAML bridge bug. This PR makesmattermost.auto_threadandmattermost.dm_auto_threadcanonical and retainsMATTERMOST_REPLY_MODEonly as a deprecated environment fallback; it does not claim to repair the deprecated YAML key.Validation
scripts/run_tests.sh tests/gateway/test_mattermost.py tests/gateway/test_send_multiple_images.py -q— 60 passedscripts/run_tests.sh tests/gateway/test_config.py tests/gateway/test_platform_registry.py -q— 90 passedruff checkon changed Python filespython3 -m py_compileon changed Python filesgit diff --checkRebased onto current
origin/main(a3bcb2c23). All five commits in the PR range are GPG-signed. Contributor commitc84653107retains its original authorship beneath signed merge commit9237a811c.Screenshot