Skip to content

fix(mattermost): preserve threaded delivery targets - #45654

Open
yong2bba wants to merge 1 commit into
NousResearch:mainfrom
yong2bba:fix/mattermost-thread-delivery
Open

fix(mattermost): preserve threaded delivery targets#45654
yong2bba wants to merge 1 commit into
NousResearch:mainfrom
yong2bba:fix/mattermost-thread-delivery

Conversation

@yong2bba

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes several Mattermost outbound delivery paths that can lose thread context or fail when a live gateway adapter is present:

  • send_message(target="mattermost:<channel_id>:<root_post_id>") now parses Mattermost threaded targets instead of treating <channel_id>:<root_post_id> as one chat id.
  • Mattermost delivery honors metadata.thread_id / metadata.root_id as the effective Mattermost root_id when reply_to is absent.
  • send_message avoids directly awaiting the live Mattermost adapter from a potentially different event loop, which can trigger aiohttp's Timeout context manager should be used inside a task error.
  • Mattermost REST calls include a stable Hermes User-Agent.

Related Issue

No issue yet. This PR documents and fixes the reproduced bug directly.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • tools/send_message_tool.py
    • Add Mattermost <channel_id>[:<root_post_id>] explicit target parsing.
    • Route Mattermost send_message through standalone delivery instead of the live adapter cross-loop path.
  • plugins/platforms/mattermost/adapter.py
    • Use metadata.thread_id / metadata.root_id as the effective threaded reply root when reply_to is absent.
    • Add a stable User-Agent header for REST and file-upload calls.
  • tests/tools/test_send_message_target_parse.py
    • Cover Mattermost channel and threaded target parsing.
  • tests/cron/test_scheduler.py
    • Cover explicit mattermost:<channel_id>:<root_post_id> cron delivery target parsing.
  • tests/gateway/test_mattermost.py
    • Cover metadata.thread_id -> Mattermost root_id behavior.

How to Test

Regression tests run locally:

./venv/bin/python -m pytest \
  tests/tools/test_send_message_target_parse.py \
  tests/gateway/test_mattermost.py::TestMattermostSend \
  tests/cron/test_scheduler.py::TestResolveDeliveryTarget::test_mattermost_explicit_thread_target \
  -q

Result:

11 passed in 0.31s

I also verified the fix against a live Mattermost gateway after restarting the Hermes gateway process:

  • send_message(target="mattermost:<channel_id>:<root_post_id>") succeeded and the Mattermost API showed root_id=<root_post_id>.
  • Cron explicit delivery to mattermost:<channel_id>:<root_post_id> succeeded and preserved root_id.
  • Cron deliver=origin back to a Mattermost DM thread succeeded and preserved root_id.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS

Documentation and Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Screenshots / Logs

Targeted regression test output:

...........                                                              [100%]
11 passed in 0.31s

Parse Mattermost channel/root-post targets, honor metadata thread IDs as root_id in the adapter, and avoid cross-event-loop live adapter sends from send_message. Add regression coverage for Mattermost target parsing, cron explicit targets, and metadata thread delivery.
@yong2bba

Copy link
Copy Markdown
Contributor Author

Related: #35343 also addresses the Mattermost aiohttp cross-loop failure. This PR intentionally covers that symptom only as part of a broader threaded-delivery fix: Mattermost target parsing for <channel_id>:<root_post_id>, cron explicit target parsing, metadata.thread_id -> Mattermost root_id, and regression tests for those paths. If #35343 lands first, the cross-loop portion here can be adjusted while preserving the thread-target fixes.

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/gateway Gateway runner, session dispatch, delivery comp/tools Tool registry, model_tools, toolsets labels Jun 13, 2026

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review Summary

Verdict: Approved

Correctness

  • Mattermost adapter now reads thread_id or root_id from metadata as fallback when reply_to is not set, preserving threaded delivery context.
  • Adds User-Agent: HermesAgent-Mattermost/1.0 header for better server-side identification.
  • Properly resolves root ID for CRT (threading) in all delivery paths.

Testing

  • Tests in test_mattermost.py and test_send_message_target_parse.py cover threaded delivery.

No Issues Found

  • No secrets, no debug artifacts.

Reviewed by Hermes Agent

@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 the focused Mattermost delivery coverage. The explicit target parsing remains needed on current main: tools/send_message_tool.py:531-616 has no Mattermost branch, while cron/scheduler.py:1128-1159 relies on that parser for explicit delivery thread IDs.

Problems

  • tests/tools/test_send_message_target_parse.py changes the E.164 regression input to +155****4567; that is not E.164-shaped, so the test no longer covers the parser boundary it names.
  • The new mattermost:<channel_id>:<root_post_id> syntax is not documented in SEND_MESSAGE_SCHEMA (tools/send_message_tool.py:217).

Suggested changes

  • Restore a valid fictitious E.164-shaped test value.
  • Update the target schema description/example for Mattermost threaded delivery.
  • Salvage only the still-needed parser/cron work onto current main. The adapter metadata-thread behavior is already implemented more broadly by merged commit 5a0e0d35b (#46885), so those overlapping hunks should not be reapplied unchanged.

This is an automated hermes-sweeper review.

def test_e164_target_still_requires_phone_platform() -> None:
assert _parse_target_ref("matrix", "+15551234567")[2] is False
assert _parse_target_ref("matrix", "+155****4567")[2] is False

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.

Please keep this a valid fictitious E.164-shaped number (for example +15555550123). +155****4567 cannot match _E164_TARGET_RE, so this no longer verifies that E.164 targets are rejected on non-phone platforms.

@teknium1 teknium1 added sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 14, 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/tools Tool registry, model_tools, toolsets P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants