Skip to content

fix(mattermost): harden threaded root session isolation - #18280

Closed
randomsnowflake wants to merge 1 commit into
NousResearch:mainfrom
randomsnowflake:fix/mattermost-parallel-thread-sessions
Closed

fix(mattermost): harden threaded root session isolation#18280
randomsnowflake wants to merge 1 commit into
NousResearch:mainfrom
randomsnowflake:fix/mattermost-parallel-thread-sessions

Conversation

@randomsnowflake

@randomsnowflake randomsnowflake commented May 1, 2026

Copy link
Copy Markdown

Summary

Harden Mattermost root-thread session isolation and cover the real session-key path.

Current upstream already seeds top-level channel posts as thread roots when reply_mode="thread". This revision incorporates the maintainer review instead of broadening that behavior:

  • retain the reply_mode="thread" boundary
  • retain flat-mode and DM session behavior
  • prove two top-level Mattermost roots build distinct build_session_key() values
  • prove flat mode does not silently become per-post sessions
  • ignore textless posts with no files after mention stripping, preventing a bare mention / blank wake-up from creating an empty agent turn

Why this remains useful

The original root-thread bug is now implemented upstream, but the previous tests stopped at the adapter's source.thread_id. The added regression exercises the downstream session-key contract that caused the real production symptom: unrelated Mattermost root posts sharing one active-session guard.

The blank-post guard is the remaining behavior fix. It prevents an empty wake-up from becoming a blank user turn after the bot mention is stripped.

Scope

Two files, one commit:

  • plugins/platforms/mattermost/adapter.py
  • tests/gateway/test_mattermost.py

No flat-mode or DM compatibility change.

Current rebase

base: 94c944363 feat(tui): show the plan catalog in /subscription on Free (#68357)
head: 6ecac2612 fix(mattermost): isolate root threads as sessions

Tests

venv/bin/python -m pytest tests/gateway/test_mattermost.py tests/gateway/test_send_multiple_images.py -q
87 passed, 2 warnings in 9.59s

The adapter/session regression confirms:

  • threaded root A and root B build distinct Mattermost session keys
  • replies retain their existing root
  • flat mode remains channel/user scoped
  • DMs remain channel scoped
  • blank posts without attachments are ignored

@randomsnowflake

randomsnowflake commented May 1, 2026

Copy link
Copy Markdown
Author

Additional context from running Mattermost in production:

The Mattermost integration has been broken for threaded usage since at least 0.7 , and possibly earlier. I can see that the integration has been improving, but this specific issue is still a showstopper for using the bridge properly in threaded mode.

Without this fix, separate Mattermost root threads in the same channel collapse into one Hermes session and block/interrupt each other. That makes the threaded interface effectively unusable for real parallel conversations.

Thank you for taking a look and for merging this. My previous PRs have all been closed - this is not AI slop, it fixes an actual reproducable issue.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery labels May 1, 2026
@randomsnowflake

Copy link
Copy Markdown
Author

Updated this PR branch on top of the v2026.5.7 release tag and re-ran the targeted Mattermost regression suite.

Local verification before pushing:

  • Hermes Agent v0.13.0 (2026.5.7)
  • python -m pytest tests/gateway/test_mattermost.py tests/gateway/test_send_multiple_images.py -o 'addopts=' -q69 passed

The branch now contains the release baseline plus the Mattermost root-thread session isolation fix.

@randomsnowflake

Copy link
Copy Markdown
Author

Update after the v2026.5.16 / v0.14.0 release:

This PR is still required. I checked the clean upstream release/tag and the bug still reproduces there.

Semantic repro against upstream v2026.5.16:

top-level post p1: agent:main:mattermost:channel:channel_A:u1
top-level post p2: agent:main:mattermost:channel:channel_A:u1
same? True

That means two new top-level Mattermost root posts in the same channel still collapse into the same Hermes gateway session upstream. Replies inside existing Mattermost threads are distinct, but new root posts are not isolated.

I updated this PR branch on top of v2026.5.16 / current main and adapted the patch to the newer gateway thread metadata helper. The fork branch now points at:

39fa3eae1 fix(mattermost): isolate root threads as sessions

Post-patch semantic repro:

root_a root_a agent:main:mattermost:channel:chan_456:root_a
root_b root_b agent:main:mattermost:channel:chan_456:root_b
distinct? True

Local validation run after rebasing/updating:

python3 -m py_compile gateway/platforms/mattermost.py tests/gateway/test_mattermost.py tests/gateway/test_send_multiple_images.py
python -m pytest tests/gateway/test_mattermost.py tests/gateway/test_send_multiple_images.py -q -o 'addopts='

68 passed in 3.66s

Note: I did not run the entire repository test suite locally; the targeted Mattermost/gateway regression suite passes, and CI should cover the broader repo matrix.

@randomsnowflake

Copy link
Copy Markdown
Author

Updated this PR branch onto current upstream main (origin/main at 526a1e24b) after Mattermost moved into the plugin platform tree.\n\nThe fix still appears required upstream: current plugins/platforms/mattermost/adapter.py derives inbound thread scope as post.get("root_id") or None, so fresh top-level Mattermost root posts still have no thread/session key and collapse into the channel/user session.\n\nPorted branch head now contains only the Mattermost root-thread session isolation patch on top of latest main.\n\nLocal verification before pushing:\n\ntext\nscripts/run_tests.sh tests/gateway/test_mattermost.py tests/gateway/test_send_multiple_images.py\n\n68 tests passed, 0 failed\n

@randomsnowflake
randomsnowflake force-pushed the fix/mattermost-parallel-thread-sessions branch from 9940f73 to 267f71c Compare June 14, 2026 09:06
@randomsnowflake

Copy link
Copy Markdown
Author

Follow-up: upstream main advanced while I was updating this, so I rebased the PR branch again.\n\nCurrent base is now origin/main at 4936a49a; branch head is the Mattermost-only fix commit rebased on top of that.

@randomsnowflake
randomsnowflake force-pushed the fix/mattermost-parallel-thread-sessions branch 4 times, most recently from f98fab1 to 6fde8ec Compare June 20, 2026 06:46
@randomsnowflake
randomsnowflake force-pushed the fix/mattermost-parallel-thread-sessions branch from 6fde8ec to 7374077 Compare June 27, 2026 18:29
@randomsnowflake

Copy link
Copy Markdown
Author

Rebased this PR again onto current origin/main and force-pushed a clean, minimal branch.

Current state:

base: f53b184c4 fix(ci): pass secrets down to docker workflows
head: 7374077ef fix(mattermost): isolate root threads as sessions

I re-checked current upstream semantics before updating. The bug is still present upstream for fresh top-level Mattermost posts:

top-level A key: agent:main:mattermost:channel:channel_A:u1
top-level B key: agent:main:mattermost:channel:channel_A:u1
same top-level key? True

That means upstream fixes/changes around Mattermost threading are adjacent, but they do not isolate new root posts with no root_id.

I also trimmed/confirmed the PR shape:

1 commit
2 files changed, 77 insertions(+), 13 deletions(-)
plugins/platforms/mattermost/adapter.py
tests/gateway/test_mattermost.py

Verification on the rebased branch:

venv/bin/python -m pytest tests/gateway/test_mattermost.py -q
62 passed, 2 warnings in 7.72s

Additional local adapter/session E2E check confirms:

PASS Mattermost thread E2E
active root keys were distinct:
  agent:main:mattermost:channel:chan_e2e:root_A
  agent:main:mattermost:channel:chan_e2e:root_B
queued reply key: agent:main:mattermost:channel:chan_e2e:root_A
explicit metadata outbound root_id: root_A

This is still the narrow fix for #18279: fresh Mattermost root posts need to become Hermes thread/session roots by using the post id when Mattermost has no root_id.

@randomsnowflake

Copy link
Copy Markdown
Author

Rebased again onto current upstream main and force-pushed a clean Mattermost-only branch.

Current state:

base: 4d7f8ade3 feat(install): warn pip/Homebrew installs are unsupported (CLI, TUI, desktop) (#57225)
head: 129915d414ab5aa639b07fbd0374278f984fc5a6 fix(mattermost): isolate root threads as sessions

No semantic redesign was needed. The patch still does the same narrow thing: use the Mattermost post id as Hermes thread_id for fresh non-DM root posts that have no root_id, while leaving replies and DMs alone. The only adaptation is surrounding context from newer upstream Mattermost code near slash-command whitespace handling.

Fresh local verification on the rebased branch:

venv/bin/python -m pytest tests/gateway/test_mattermost.py tests/honcho_plugin/test_cli.py -q
100 passed, 2 warnings in 16.20s

Additional adapter-level E2E routing check:

mattermost adapter e2e routing ok: two root posts get distinct session thread_ids; reply stays in root thread; DM remains channel-scoped; blank wake ignored

I also installed and dogfooded this locally on Hermes Agent v0.18.2 (2026.7.7.2) with the PR carried on top of upstream main.

@llealloo

llealloo commented Jul 8, 2026

Copy link
Copy Markdown

Thank you for staying on top of this.

I wonder if we could get all of the people who have been force pushing amazing fixes and features for the Hermes Mattermost integration to collectively start a Messaging Platform Adapter Plugin called something like hermes-mattermost-plus.

Just a thought.

@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 coverage.

Problems

  • Current main already implements top-level root session IDs for threaded Mattermost replies: plugins/platforms/mattermost/adapter.py:869-876, added in 5a0e0d35b, with regression coverage at tests/gateway/test_mattermost.py:1029-1085.
  • This diff removes the reply_mode="thread" boundary. Since the adapter defaults to reply_mode="off" (plugins/platforms/mattermost/adapter.py:95-99) and threaded session keys suppress normal per-user partitioning (gateway/session.py:941-956), the change makes every flat top-level post a separate session. That is a compatibility/design change, not the already-fixed threaded case.

Suggested changes

  • Retain the thread-mode condition unless flat-mode per-post sessions are an explicitly approved behavior.
  • If that broader behavior is intended, add a regression through build_session_key() or the active-session path rather than asserting only the adapter source field.

Automated hermes-sweeper review.

Comment thread plugins/platforms/mattermost/adapter.py Outdated
# root in CRT. Use the post id so concurrent root posts do not share
# one Hermes session/active-session guard. DMs keep the historical
# channel-scoped session behavior.
thread_id = post.get("root_id") or (post_id if chat_type != "dm" else None)

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.

Current main intentionally limits top-level post IDs to reply_mode="thread" (plugins/platforms/mattermost/adapter.py:869-876). Removing that boundary changes default flat-mode channel conversations into one session per post through gateway/session.py:941-956; please retain the condition unless that compatibility change is explicitly intended.

@teknium1 teknium1 added the sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state label Jul 12, 2026
@teknium1 teknium1 added 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 area/sessions Session lifecycle, resume, persistence, history labels Jul 12, 2026
@randomsnowflake randomsnowflake changed the title fix(mattermost): isolate root threads as sessions fix(mattermost): harden threaded root session isolation Jul 21, 2026
@randomsnowflake
randomsnowflake force-pushed the fix/mattermost-parallel-thread-sessions branch from 129915d to dabac62 Compare July 21, 2026 06:37
@randomsnowflake

Copy link
Copy Markdown
Author

Updated in response to the maintainer review:

  • retained the reply_mode="thread" boundary
  • retained flat-mode and DM behavior
  • added a regression through build_session_key() proving independent top-level roots do not share an active-session key
  • retained the blank-post guard as the remaining behavior fix

Fresh verification on the rebased head dabac6221:

pytest tests/gateway/test_mattermost.py tests/gateway/test_send_multiple_images.py -q
87 passed, 2 warnings in 9.17s

I also deployed the v2026.7.20 release plus this carried commit and verified real Mattermost API thread routing: two distinct roots each received a reply whose persisted root_id matched the intended root.

@randomsnowflake
randomsnowflake force-pushed the fix/mattermost-parallel-thread-sessions branch from dabac62 to 6ecac26 Compare July 21, 2026 06:52
@randomsnowflake

Copy link
Copy Markdown
Author

Closing this because the threaded root-session isolation is now implemented upstream and is present in v2026.8.3 (notably via 5a0e0d3). I re-verified the release semantics: separate top-level Mattermost roots build distinct session keys in reply_mode="thread". The remaining blank-post guard is a separate, smaller behavior change and does not justify keeping this threading PR open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/sessions Session lifecycle, resume, persistence, history comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists 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-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