feat(mattermost): add 'auto' reply mode — flat DMs, threaded channels - #33149
feat(mattermost): add 'auto' reply mode — flat DMs, threaded channels#33149ugoenyioha wants to merge 1 commit into
Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for addressing the DM/threading conflict. The requested auto behavior is still absent from current main, but this patch needs a rework before it can be salvaged.
Problems
- At
plugins/platforms/mattermost/adapter.py:264, an unknown type is treated as threadable. Howeverget_chat_info()returns"channel"when its API lookup fails (:317-321), so a transient DM lookup failure can still sendroot_id. - Current main centralizes thread-root selection, metadata roots, and final-notify fallback in
_thread_root_for_send()/_post_preserving_thread()(plugins/platforms/mattermost/adapter.py:163-215); the old inline-call-site patch no longer covers that structure. - The PR changes no tests or user-facing mode declarations.
plugin.yaml:34-36andwebsite/docs/user-guide/messaging/mattermost.md:202-212still describe onlythread|off.
Suggested changes
- Implement auto selection in the centralized resolver, preserve metadata-root handling, default unknown lookup results to flat, and add auto-mode tests plus documentation/config prompt updates.
Automated hermes-sweeper review.
| if not ch_type: | ||
| info = await self.get_chat_info(chat_id) | ||
| ch_type = info["type"] | ||
| return ch_type != "dm" |
There was a problem hiding this comment.
get_chat_info() returns type: "channel" when its API lookup fails (lines 317-321), so a transient lookup failure in a DM reaches this branch and sends root_id anyway. Treat unknown/failed lookup as flat, or seed the channel-type cache from the inbound WebSocket event.
`thread` keeps busy channels legible but looks strange in a DM, where a one-on-one conversation ends up nested inside itself; `off` avoids that but gives up thread tidiness everywhere. `auto` picks per conversation: thread in channels, stay flat in DMs and group DMs. Reimplemented against the current centralized structure, per review of NousResearch#33149. The previous revision patched the inline call sites, which no longer exist -- thread-root selection now lives in `_thread_root_for_send()`, so the mode decision belongs there and metadata roots keep working for `auto` the same way they do for `thread`. An indeterminate channel type always falls back to flat. Two specific traps: - The lookup does NOT go through `get_chat_info()`. That helper reports "channel" when its API call fails, and "channel" is the threadable answer, so a transient DM lookup failure would have silently started threading inside a DM. `_auto_mode_should_thread()` reads the raw type and treats a failure, a missing type, or an unrecognised code as unknown → flat. - The decision keys on the RAW Mattermost type code, not `_CHANNEL_TYPE_MAP`, which folds `P` (private channel) into "group" alongside real group DMs. A private channel is a channel and should thread; going through the map would have forced it flat. Failed lookups are deliberately not cached, so a later send can still learn the real type once the API recovers. Successful ones are, since a channel's type does not change. On the inbound path the same decision is shared via `_mode_threads_raw_type()`, where the websocket event already carries the channel type and no lookup is needed. `thread` mode behaviour there is unchanged (it already excluded DMs). Also updates the user-facing declarations the review flagged as missing: `plugin.yaml` prompt/description and the reply-mode table in the Mattermost guide, including the flat-on-unknown behaviour. Tests: 12 new cases covering channels vs DMs vs group DMs, failed lookup, unrecognised code, cache hit and cache-miss-on-failure, metadata roots in both channel and DM, no-root short circuit, and the inbound matrix for all three modes.
102bd77 to
b2c0cc5
Compare
|
Reworked against current
One thing I found beyond the review: the decision keys on the raw type code rather than |
|
Closing in favour of #47079, which is the better design. Recording why, since the AI triage note above concluded "not a duplicate" and that reading deserves a response. The triage comparison was diff-shaped: it saw distinct code in each PR and inferred complementary value. But the distinctive parts of this PR — fail-closed channel classification, private-channel handling — exist only because my rule needs to know the channel type. #47079's rule doesn't, so those parts protect against a problem its design never has. Concretely, the two
#47079's is better on the points that matter:
The honest case for keeping both is narrow: mine is really " One durable finding worth keeping regardless — posting it on #47079 too, since it will outlive this PR: Thanks @sidorovanthon — no changes needed on your side. |
Summary
MATTERMOST_REPLY_MODEsupports onlythread(always nest) andoff(always flat).threadkeeps a busy channel legible but looks wrong in a DM, where a one-on-one conversation ends up nested inside itself;offgives up thread tidiness everywhere.autodecides per conversation: thread in channels, stay flat in DMs and group DMs.Reimplemented against current
mainper review — the previous revision patched inline call sites that no longer exist, since thread-root selection is now centralised in_thread_root_for_send().Root cause of the trap this had to avoid:
get_chat_info()returns{"type": "channel"}when its API lookup fails, and"channel"is the threadable answer. Routing anautodecision through it means a transient DM lookup failure silently starts threading inside a DM.Changes
plugins/platforms/mattermost/adapter.py:_auto_mode_should_thread()(new) resolves the channel type fromchannels/<id>directly, not viaget_chat_info(), and treats a failed lookup, a missing type, or an unrecognised code as unknown → flat. Successful lookups are cached (a channel's type doesn't change); failures are deliberately not cached, so a later send can still learn the real type._thread_root_for_send()takeschat_id(already in scope at all four call sites) and consults the new resolver inauto; metadata roots (metadata["thread_id"]/["root_id"]) keep working forautoexactly as forthread._mode_threads_raw_type()(new) shares the same decision on the inbound path, where the websocket event already carries the channel type so no lookup is needed.threadbehaviour there is unchanged (it already excluded DMs)._AUTO_THREAD_CHANNEL_TYPES = {"O","P"}keys on the raw Mattermost code rather than_CHANNEL_TYPE_MAP, which foldsP(private channel) into"group"alongside real group DMs — a private channel is a channel and should thread.plugins/platforms/mattermost/plugin.yaml: description and prompt now listthread|off|auto.website/docs/user-guide/messaging/mattermost.md: reply-mode table documentsauto, including the flat-on-unknown behaviour.tests/gateway/test_mattermost.py: 12 new tests.Validation
threadoffautoOPGDpytest tests/gateway/test_mattermost.py -qpytest tests/gateway/test_send_multiple_images.py -qNew cases: channels vs DMs vs group DMs; failed lookup stays flat; unrecognised code stays flat; failed lookup is not cached; successful type cached and looked up once across sends; metadata roots honoured in a channel and ignored in a DM; no-root short circuit skips the lookup; inbound matrix for all three modes across all four codes.
Overlap with #47079 — flagging rather than competing
#47079 by @sidorovanthon also adds
reply_mode "auto", opened after this one. Two open PRs for one feature is the worst outcome, so rather than leave that implicit — the two differ in coverage and a maintainer may well prefer parts of each:_thread_root_for_send()+ inbound resolverP) vs group DM (G)_CHANNEL_TYPE_MAPhermes_cli/gateway.pymattermost.mdmattermost.md+environment-variables.mdHappy to close this in favour of #47079, or to fold the unknown-type handling and the
P/Gdistinction into it — whichever the platforms maintainer prefers. Flagging @sidorovanthon.Notes
mattermost.reply_modefromconfig.yamlis dropped by the plugin's YAML→env bridge. Until that lands,reply_mode: autoset inconfig.yamlwon't reach this code — onlyMATTERMOST_REPLY_MODEwill.