Skip to content

fix(mattermost): carry thread context to non-inbound reply paths#44283

Merged
mukhtharcm merged 6 commits intoopenclaw:mainfrom
teconomix:fix/mattermost-thread-context-carryover
Mar 14, 2026
Merged

fix(mattermost): carry thread context to non-inbound reply paths#44283
mukhtharcm merged 6 commits intoopenclaw:mainfrom
teconomix:fix/mattermost-thread-context-carryover

Conversation

@teconomix
Copy link
Contributor

Problem

When replyToMode: "all" is active, replies to Mattermost threads only land in the correct thread when triggered by a direct inbound Mattermost message. Any other reply path — TUI/WebUI turns, tool call callbacks, subagent responses, or explicit message tool calls — ignores the thread context and posts to the channel root instead.

Reported in #39759.

Root Cause

Three gaps in the outbound routing path for non-inbound-triggered deliveries:

Gap 1 — dispatch-from-config.ts:
sendPayloadAsync passed ctx.MessageThreadId to routeReply. For TUI/WebUI turns the current context is a webchat surface, so ctx.MessageThreadId is undefined. The session entry's deliveryContext.threadId (set when the session was originally created from an inbound Mattermost message) was never consulted.

Gap 2 — route-reply.ts:
The threadId→replyToId fallback was Slack-only:

replyToId ?? (channelId === "slack" ? String(threadId) : undefined)

Mattermost uses the same root_id mechanic as Slack's thread_ts but was excluded.

Gap 3 — extensions/mattermost/src/channel.ts:
sendText/sendMedia destructured only replyToId, silently ignoring threadId even when it was present in the outbound context.

Fix

All three gaps addressed in a single change:

  1. dispatch-from-config.ts: fall back to deliveryContextFromSession(sessionStoreEntry.entry)?.threadId when ctx.MessageThreadId is absent. This uses the session store's persisted delivery context, which already stores the thread root ID from the original inbound message.

  2. route-reply.ts: extend the threadId→replyToId fallback to include Mattermost alongside Slack.

  3. extensions/mattermost/src/channel.ts: accept threadId in sendText/sendMedia and use it as a defense-in-depth fallback when replyToId is not set.

How to test

  1. Configure channels.mattermost.replyToMode: "all"
  2. Send a message to the bot from a Mattermost channel → bot replies in-thread ✅ (was already working)
  3. Send a follow-up from the TUI/WebUI to the same session → reply lands in thread ✅ (was broken)
  4. Trigger a long tool call that spawns a subagent → final response lands in thread ✅ (was broken)

Tests

  • Added route-reply.test.ts: uses threadId as replyToId for Mattermost when replyToId is missing
  • Added dispatch-from-config.test.ts: falls back to session deliveryContext threadId when current ctx has no MessageThreadId
  • Added channel.test.ts: uses threadId as fallback when replyToId is absent
  • All 83 affected tests pass; pnpm check and pnpm build clean locally.

@openclaw-barnacle openclaw-barnacle bot added channel: mattermost Channel integration: mattermost size: S labels Mar 12, 2026
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 12, 2026

Greptile Summary

This PR fixes Mattermost thread routing for non-inbound reply paths (TUI/WebUI turns, tool-call callbacks, subagent responses) by closing three gaps: extracting threadId from the structured session key in dispatch-from-config.ts, extending the Slack-only threadId → replyToId fallback to include Mattermost in route-reply.ts, and adding a defense-in-depth threadId fallback in the Mattermost channel plugin. The approach is sound — using the session key's :thread: segment rather than the session store avoids resurrecting stale origin.threadId metadata for intentionally non-threaded sessions.

  • dispatch-from-config.ts: routeThreadId is now derived from ctx.MessageThreadId ?? parseSessionThreadInfo(acpDispatchSessionKey).threadId, correctly sourcing thread context from the session key structure rather than the potentially-stale store. All four routeReply call-sites are updated consistently.
  • route-reply.ts: The Slack-only threadId → replyToId coercion is extended to channelId === "mattermost", matching Mattermost's root_id mechanic.
  • channel.ts: Both sendText and sendMedia accept and use threadId as a fallback when replyToId is absent, providing end-to-end defense-in-depth.
  • Tests: All three changed files have new test coverage for the happy path (thread key → replyToId) and the guard (non-thread session key does not revive stale deliveryContext.threadId).
  • CHANGELOG: The entry only describes the defensive "stale-thread prevention" aspect and omits the primary user-visible fix (non-inbound replies now land in-thread). See inline comment.

Confidence Score: 5/5

  • This PR is safe to merge; the logic is correct, well-tested, and narrowly scoped to the Mattermost thread routing path.
  • All three gaps are addressed consistently and symmetrically. The key decision — using the session key structure rather than the session store to derive threadId — is clean and avoids the stale-metadata problem. New tests cover both the positive case (thread context carried through) and the guard case (cleared threads stay cleared). No regressions are introduced in Slack or other channels since the Slack branch is unchanged. The only non-critical issue is the CHANGELOG entry wording.
  • No files require special attention.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: CHANGELOG.md
Line: 190

Comment:
**CHANGELOG entry describes the guard, not the primary fix**

The entry focuses on the defensive aspect ("keep cleared route threads cleared … instead of reviving stale `origin.threadId` metadata") but omits the user-visible primary change: non-inbound reply paths (TUI/WebUI turns, tool-call callbacks, subagent responses) now correctly route to the originating Mattermost thread instead of posting to the channel root.

A developer scanning the changelog to understand why their TUI replies were landing in the channel root would not find this entry. Consider rewording to lead with the positive fix, e.g.:

```suggestion
- Mattermost/thread routing: non-inbound reply paths (TUI/WebUI turns, tool-call callbacks, subagent responses) now correctly route to the originating Mattermost thread when `replyToMode: "all"` is active; also prevents stale `origin.threadId` metadata from resurrecting cleared thread routes. (#44283) thanks @teconomix
```

How can I resolve this? If you propose a fix, please make it concise.

Last reviewed commit: 498de0f

@teconomix teconomix force-pushed the fix/mattermost-thread-context-carryover branch from 2a3a6e2 to e557f20 Compare March 12, 2026 18:04
@teconomix
Copy link
Contributor Author

CI status

22/23 checks pass. The 1 remaining failure (check) is pre-existing and unrelated to this PR.

The check job fails on:

  1. Formatting — 3 CSS files under ui/src/styles/ (grouped.css, layout.css, layout.css) that are not touched by this PR.
  2. TypeScript errors — in extensions/tlon/src/... (@tloncorp/tlon extension) that are also not touched by this PR.

This same failure is present on the last merged PR (#44274) before this one: https://github.com/openclaw/openclaw/actions/runs/23017167737/job/66843452718

The 6 skipped checks (macOS, iOS, Android, docs, release, labels) are expected for a code-only PR.

All checks relevant to the changes in this PR pass:

  • checks (node, extensions, pnpm test:extensions) — Mattermost extension tests
  • checks (node, test, …) — all unit + integration tests across platforms
  • checks-windows (6/6 shards)
  • build-artifacts, compat-node22, install-smoke, protocol

@teconomix
Copy link
Contributor Author

@mukhtharcm — pinging you since you merged #29587 today. This PR is a direct follow-up: it fixes the cases that replyToMode does not yet cover.

With replyToMode: "all", inbound Mattermost replies land in the correct thread — but any non-inbound-triggered reply (TUI/WebUI turn, tool callback, subagent response, message tool call) still posts to the channel root because ctx.MessageThreadId is undefined in those paths. This PR threads those cases through the session-stored lastThreadId that #29587 already persists correctly.

Happy to iterate if you have questions or suggestions.

@teconomix
Copy link
Contributor Author

@greptile-apps please re review

@openclaw-barnacle openclaw-barnacle bot added docs Improvements or additions to documentation channel: slack Channel integration: slack channel: telegram Channel integration: telegram app: ios App: ios app: macos App: macos app: web-ui App: web-ui gateway Gateway runtime scripts Repository scripts commands Command implementations docker Docker and sandbox tooling agents Agent runtime and tooling size: XL and removed size: S labels Mar 12, 2026
@teconomix teconomix force-pushed the fix/mattermost-thread-context-carryover branch from c80b26b to e2deb91 Compare March 12, 2026 20:38
@openclaw-barnacle openclaw-barnacle bot removed docs Improvements or additions to documentation channel: slack Channel integration: slack channel: telegram Channel integration: telegram app: ios App: ios app: macos App: macos app: web-ui App: web-ui gateway Gateway runtime labels Mar 12, 2026
@mukhtharcm mukhtharcm force-pushed the fix/mattermost-thread-context-carryover branch from e2deb91 to ed9247e Compare March 12, 2026 20:52
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ed9247ec58

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3279044b14

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@teconomix
Copy link
Contributor Author

Addressed the Codex review ("Drop lastThreadId fallback").

Verified in src/config/sessions/store.ts:73: lastThreadId is normalised from origin.threadId when deliveryContext.threadId is absent — so using ?? lastThreadId as a fallback would re-acquire the same stale thread the fix was trying to prevent.

Latest commit drops the lastThreadId fallback entirely: only deliveryContext?.threadId is used. Also strengthened the regression test to set lastThreadId: "stale-root" in the mock (matching the real normalised store shape), so the guard is verified against the actual production data.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 408a33d894

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@teconomix
Copy link
Contributor Author

@greptile-apps pls re review

@teconomix
Copy link
Contributor Author

I agree with this.

The current CHANGELOG wording leads with the defensive guard, but the primary user-visible fix is that non-inbound reply paths now route back into the originating Mattermost thread instead of the channel root.

Suggested wording:

@teconomix
Copy link
Contributor Author

Manual test results — 2026-03-13

Tested on prod (v3.12) with replyToMode: "all" active.

Test setup

  • Mattermost group channel with an active thread
  • Dev container running patched build (fix/mattermost-thread-context-carryover)
  • Verified via Mattermost API (root_id field on each post)

Results

# Trigger Before patch After patch
1 Inbound Mattermost reply ✅ thread (fixed by #29587) ✅ thread
2 TUI/WebUI chat.send with deliver: true ❌ channel root ✅ thread
3a Agent reply after tool call (inbound-triggered) ✅ thread ✅ thread
3b Proactive message tool send ❌ channel root ❌ channel root (separate path, not in scope)

Test 2 confirms the core fix: a chat.send with deliver: true routed to a thread-scoped session (agent:...:mattermost:group:...:thread:<id>) now delivers to the correct Mattermost thread. The root_id on the Mattermost post matches the expected thread root post.

The message tool proactive path (3b) is a separate code path and will be tracked in a follow-up issue.

@mukhtharcm mukhtharcm force-pushed the fix/mattermost-thread-context-carryover branch from 498de0f to 16ceeee Compare March 14, 2026 06:27
@openclaw-barnacle openclaw-barnacle bot added channel: signal Channel integration: signal channel: slack Channel integration: slack labels Mar 14, 2026
teconomix and others added 6 commits March 14, 2026 06:31
Fixes a bug where replies triggered from TUI/WebUI or by agent-initiated
sends (tool callbacks, subagent responses, message tool) land in the
Mattermost channel root instead of the originating thread.

Root cause: three gaps in the outbound routing path for turns not
directly triggered by an inbound Mattermost message:

1. dispatch-from-config.ts: sendPayloadAsync passed ctx.MessageThreadId,
   which is undefined for webchat/TUI turns. Now falls back to the
   session entry's deliveryContext.threadId (the lastThreadId stored when
   the session was first created from an inbound Mattermost message).

2. route-reply.ts: threadId was only forwarded as replyToId for Slack.
   Mattermost uses the same root_id mechanic, so the same fallback now
   applies to Mattermost too.

3. channel.ts (Mattermost outbound): sendText/sendMedia only consumed
   replyToId, ignoring threadId. Added threadId as a defense-in-depth
   fallback for any path that sets threadId but not replyToId.

All three gaps in a single PR. Tests added for each fix path.

Fixes openclaw#39759
lastThreadId is normalised from origin.threadId by loadSessionStore, so
using it as a fallback here would re-acquire the same stale thread that
deliveryContext.threadId was intentionally cleared from. Only
deliveryContext.threadId is safe to use as the restored route thread.

Also strengthens the regression test to include the normalised
lastThreadId value that the real store produces, so the guard is
verified against the actual production data shape.

Addresses review feedback from chatgpt-codex-connector.
Do not recover route thread ids from the normalised session store in
non-inbound reply paths. Store normalisation can fold origin.threadId
back into lastThreadId/deliveryContext, which resurrects stale thread
routing after delivery was intentionally cleared.

Instead, restore thread context only from:
- ctx.MessageThreadId (active inbound turn), or
- the active thread-scoped session key (:thread: / :topic:)

Also updates dispatch tests to verify that stale origin/store thread
metadata cannot override a non-thread session key, while a thread-scoped
session key still restores the correct route thread.
@mukhtharcm mukhtharcm force-pushed the fix/mattermost-thread-context-carryover branch from 16ceeee to 2846a6c Compare March 14, 2026 06:52
@mukhtharcm mukhtharcm merged commit 0c926a2 into openclaw:main Mar 14, 2026
29 checks passed
@mukhtharcm
Copy link
Member

Merged via squash.

Thanks @teconomix!

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

Labels

channel: mattermost Channel integration: mattermost channel: signal Channel integration: signal channel: slack Channel integration: slack size: M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants