Skip to content

fix(gateway): route Mattermost replies to thread roots - #33470

Open
ahruslan17 wants to merge 1 commit into
NousResearch:mainfrom
ahruslan17:fix/mattermost-thread-routing
Open

fix(gateway): route Mattermost replies to thread roots#33470
ahruslan17 wants to merge 1 commit into
NousResearch:mainfrom
ahruslan17:fix/mattermost-thread-routing

Conversation

@ahruslan17

@ahruslan17 ahruslan17 commented May 27, 2026

Copy link
Copy Markdown

What does this PR do?

Fixes Mattermost thread routing for outbound replies and attachments.

Mattermost requires root_id to be the root post of a thread. The gateway already passes that root through metadata["thread_id"], but the Mattermost adapter did not consistently use it across outbound paths. For replies inside an existing thread, reply_to can be a nested post id, which can make replies, files, image batches, and typing indicators land outside the intended Mattermost thread or use the wrong post id.

This change makes Mattermost outbound delivery prefer metadata["thread_id"] for thread routing and applies it consistently to text replies, file/image/audio/video uploads, batched image posts, and typing indicators.

Related Issue

No issue filed.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • Tests (adding or improving test coverage)

Changes Made

  • plugins/platforms/mattermost/adapter.py

    • Resolve Mattermost thread roots from metadata["thread_id"].
    • Prefer metadata["thread_id"] over reply_to when setting root_id.
    • Preserve existing _resolve_root_id(reply_to) behavior for reply_to-only threaded sends.
    • Preserve metadata through media helper methods.
    • Add root_id to send_multiple_images() batch posts.
    • Add parent_id to thread-scoped typing indicators.
  • tests/gateway/test_mattermost.py

    • Add coverage for text replies, media helpers, document uploads, multi-image posts, and typing indicators.

How to Test

  1. Run scripts/run_tests.sh tests/gateway/test_mattermost.py.
  2. Send a Mattermost reply from inside an existing thread.
  3. Verify text replies, attachments, multi-image responses, and typing indicators stay in that thread.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits
  • My PR contains only changes related to this fix
  • I've added tests for my changes
  • I've tested on my platform: Linux

Documentation & Housekeeping

  • Documentation update N/A
  • cli-config.yaml.example update N/A
  • CONTRIBUTING.md or AGENTS.md update N/A
  • Cross-platform impact considered: Mattermost REST payload-only change
  • Tool descriptions/schemas update N/A

Test Notes

  • scripts/run_tests.sh tests/gateway/test_mattermost.py could not complete in my local venv because pytest was invoked with --timeout args while pytest-timeout was not installed there.
  • Direct fallback for the same test file passed: .venv/bin/python -m pytest tests/gateway/test_mattermost.py -o addopts="-m 'not integration'" -> 51 passed.

@alt-glitch alt-glitch added type/bug Something isn't working comp/gateway Gateway runner, session dispatch, delivery comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have labels May 27, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for covering the outbound paths. Current main has already landed most of this routing work in 5a0e0d35b94fefae4ff6463c24f53e348f4679e6 (merged PR #46885), including metadata propagation for text, files, and image batches.

Problems

  • The remaining gap is typing: gateway/run.py:16796 passes thread metadata, but plugins/platforms/mattermost/adapter.py:392-395 still posts only channel_id.
  • The PR's adapter.py:313-314 unconditionally emits root_id when metadata has thread_id, bypassing the reply_mode guard. Current main intentionally gates thread roots in _thread_root_for_send at adapter.py:169-176.
  • The stale patch also replaces current _post_preserving_thread calls with raw _api_post calls, which would discard the notify-only broken-root handling at adapter.py:189-215.

Suggested changes

  • Salvage only the typing payload change onto current main, using the existing thread-mode-gated resolver and retaining the current delivery-hygiene helpers.
  • Add an reply_mode="off" metadata regression test.

Automated hermes-sweeper review.

payload["root_id"] = resolved_root
# Mattermost requires root_id to be the thread root post, not a
# nested reply. Gateway callers pass that root in metadata.
if isinstance(metadata, dict) and metadata.get("thread_id"):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

metadata.thread_id must remain subject to reply_mode == "thread". As written, this emits root_id even when the configured reply mode is off; current main's _thread_root_for_send deliberately gates that behavior.

elif reply_to and self._reply_mode == "thread":
payload["root_id"] = await self._resolve_root_id(reply_to)

data = await self._api_post("posts", payload)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not replace the current _post_preserving_thread path with raw _api_post when rebasing. Main now preserves a notify-only fallback for confirmed broken thread roots; salvage the typing change without removing that later delivery-hygiene behavior.

@teknium1 teknium1 added sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages 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 labels Jul 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/gateway Gateway runner, session dispatch, delivery 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-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants