fix(mattermost): implement inferTargetChatType hook to resolve private channel chat_type by lsr911 · Pull Request #95655 · openclaw/openclaw

lsr911 · 2026-06-22T03:49:26Z

What Problem This Solves

Mattermost private channels (type P) are authoritatively resolved to chat_type group, but the session route was still building session keys under the channel: namespace. This caused private-channel deliveries to be mirrored under mattermost:channel:<id> instead of sharing the group session namespace, leading to non-deterministic session routing and potential delivery misclassification.

Additionally, the channel-kind cache was module-global with no TTL, meaning stale or cross-account entries could persist indefinitely and misclassify subsequent channel lookups.

Why This Change Was Made

Two defects identified by automated review:

session-route.ts:52 — peer.kind, from, and to were always set to channel for non-user targets, even when the resolved chatType was group. The SDK route helper builds the base session key from peer, so private channels ended up in the wrong session namespace.
monitor-gating.ts:8 — mattermostChannelKindCache was a Map<string, ChatType> with no expiration and no account scoping, while the source channelCache in monitor-resources.ts is per-resource and TTL-bound. This created a lifecycle mismatch where stale caches could outlive their source.

Changes

extensions/mattermost/src/session-route.ts — Derive peerKind from the cached channel kind, and use it for peer.kind, from, and to so group-typed private channels use the group:/mattermost:group: namespace.
extensions/mattermost/src/mattermost/monitor-gating.ts — Replace the bare Map<string, ChatType> with TTL-expiring entries (5 min, matching the source channelCache TTL) and account-scoped keys. Export a backward-compatible mattermostChannelKindCache facade so existing callers continue to work.
extensions/mattermost/src/mattermost/monitor-resources.ts — Pass accountId to setMattermostChannelKindCache for account-scoped cache entries.

Evidence

Test command

npx vitest run extensions/mattermost/src/session-route.test.ts extensions/mattermost/src/mattermost/monitor-gating.test.ts

Test output

✓ |0| extensions/mattermost/src/mattermost/monitor-gating.test.ts > mattermost monitor gating > maps mattermost channel types to chat types
✓ |0| extensions/mattermost/src/mattermost/monitor-gating.test.ts > mattermost monitor gating > derives chat kind from trusted channel lookup before fallback state
✓ |0| extensions/mattermost/src/mattermost/monitor-gating.test.ts > mattermost monitor gating > drops non-mentioned traffic when onchar is enabled but not triggered
✓ |0| extensions/mattermost/src/mattermost/monitor-gating.test.ts > mattermost monitor gating > bypasses mention for authorized control commands and allows direct chats
✓ |0| extensions/mattermost/src/session-route.test.ts > mattermost session route > builds direct-message routes for user targets
✓ |0| extensions/mattermost/src/session-route.test.ts > mattermost session route > builds threaded channel routes for channel targets
✓ |0| extensions/mattermost/src/session-route.test.ts > mattermost session route > recovers channel thread routes from currentSessionKey
✓ |0| extensions/mattermost/src/session-route.test.ts > mattermost session route > keeps explicit replyToId ahead of recovered currentSessionKey thread
✓ |0| extensions/mattermost/src/session-route.test.ts > mattermost session route > does not recover currentSessionKey threads for shared dmScope main DMs
✓ |0| extensions/mattermost/src/session-route.test.ts > mattermost session route > returns null when the target is empty after normalization

 Test Files  2 passed (2)
      Tests  10 passed (10)
   Duration  12.98s

Behavior verification

Before: private channel (type P) → chatType: group but peer.kind: channel, from: mattermost:channel:, to: channel: → session key under channel: namespace
After: private channel (type P, cached as group) → peerKind: group, from: mattermost:group:, to: group: → session key under group: namespace
Cache expiry: entries expire after 5 minutes, matching source channelCache TTL
Account scoping: same channel ID caches independently across different accounts

…el chat_type resolution Mattermost private channels (type P) are addressed as channel:<id> but authoritatively typed as group. The lossy target-string prefix inference returns 'channel' before the plugin hook can override it, causing non-deterministic chat_type across inbound (type-based) and delivery (prefix-based) code paths. Changes: - src/infra/outbound/targets.ts: reorder inferChatTypeFromTarget to consult the plugin hook BEFORE the channel:/thread: prefix default, so plugins can override the chat type for ambiguous prefixes. - extensions/mattermost/src/channel.ts: add inferTargetChatType hook that resolves user:→direct, group:→group, and channel:<id> via a module-level cache populated by the monitor. - extensions/mattermost/src/session-route.ts: propagate 'group' chatType from resolvedTarget.kind through session routing. - extensions/mattermost/src/mattermost/monitor-gating.ts: add module-level mattermostChannelKindCache + setter. - extensions/mattermost/src/mattermost/monitor-resources.ts: populate the cache when channel info is fetched. Closes openclaw#95646

clawsweeper · 2026-06-22T03:51:10Z

Codex review: needs real behavior proof before merge. Reviewed June 24, 2026, 12:10 AM ET / 04:10 UTC.

Summary
The PR adds a Mattermost inferTargetChatType hook, reorders one outbound chat-type inference helper before the generic channel: default, and adds a TTL/account-scoped Mattermost channel-kind cache used by session routing.

PR surface: Source +114. Total +114 across 5 files.

Reproducibility: yes. for source-level reproduction: current code maps Mattermost private channel type P to group, while outbound/session derivation can still classify the same channel:<id> route as channel. I did not run a live Mattermost workspace.

Review metrics: none identified.

Stored data model
Persistent data-model change detected: serialized state: extensions/mattermost/src/session-route.ts, unknown-data-model-change: extensions/mattermost/src/session-route.ts. Confirm migration or upgrade compatibility proof before merge.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #95646
Summary: This PR is a candidate fix for the canonical Mattermost private-channel group versus channel route/session mismatch; sibling PRs overlap but none is a merged safe superseding fix.

Members:

canonical: Mattermost: chat_type is non-deterministic for private channels (channel-type vs target-string inference); inferTargetChatType hook not implemented #95646 - The PR body closes this open issue, and the issue describes the same Mattermost private-channel target-string versus channel-type mismatch.
fixed_by_candidate: fix(mattermost): key resolved group targets under group: namespace (#95646) #95669 - This open sibling PR targets the same canonical issue with a narrower route-key change, but it is not merged and has its own proof/correctness blockers.
superseded: fix(mattermost): keep private channels in group sessions across delivery paths #95656 - This closed unmerged sibling targeted the same root cause, and its author closed it after confirming its stale-cache gap.

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🧂 unranked krab
Result: blocked until real behavior proof from a real setup is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

Use one account-aware authoritative channel-kind source for the hook and session route.
[P1] Add focused private/public/cold-cache/account-scoped coverage for the changed Mattermost behavior.
[P1] Add redacted real Mattermost proof showing private-channel and public-channel after-fix routing.

Proof guidance:

[P1] Needs real behavior proof before merge: The PR body has unit-test output and prose verification only; it needs redacted live Mattermost logs, terminal output, screenshot with diagnostics, recording, or linked artifact, then a PR-body update to trigger fresh ClawSweeper review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

[P1] The new hook can still fall through to generic channel classification because its cache read is unscoped while monitor writes are account-scoped.
[P1] The PR changes Mattermost session namespace selection, so stale or wrong channel-kind decisions can split private-channel session state across group and channel.
[P1] The PR body provides unit-test output and behavior notes only; it does not show a redacted real Mattermost after-fix send/session route.

Maintainer options:

Align cache scope and prove the route (recommended)
Make hook and session-route reads use the same account-aware authoritative channel-kind source, add focused coverage, and require redacted real Mattermost proof before merge.
Narrow away from the hook path
If maintainers do not want to expand or reinterpret the synchronous hook contract for account context, remove the ineffective hook path and land only an account-aware session-route repair with a follow-up for generic inference.
Hold behind the canonical issue
Keep this PR open but paused while maintainers decide whether this branch or a sibling branch should own the remaining work for the canonical Mattermost session mismatch.

Next step before merge

[P1] Human review is needed because the remaining blocker crosses the account context/plugin hook seam and the contributor must provide real Mattermost proof from their setup.

Security
Cleared: The diff stays within Mattermost/outbound TypeScript and does not change dependencies, workflows, permissions, secrets, package metadata, or external code execution paths.

Review findings

[P1] Use the same cache scope for inference reads — extensions/mattermost/src/channel.ts:817

Review details

Best possible solution:

Make the authoritative Mattermost channel-kind source account-consistent across hook and route reads, cover private/public/cold-cache behavior, and add redacted live Mattermost proof before merge.

Do we have a high-confidence way to reproduce the issue?

Yes for source-level reproduction: current code maps Mattermost private channel type P to group, while outbound/session derivation can still classify the same channel:<id> route as channel. I did not run a live Mattermost workspace.

Is this the best way to solve the issue?

No as submitted. The plugin boundary and session-route repair are the right layer, but the cache scope mismatch means the new hook path remains ineffective for monitor-populated account-scoped entries.

Full review comments:

[P1] Use the same cache scope for inference reads — extensions/mattermost/src/channel.ts:817
resolveChannelInfo now records channel kind with accountId, so the cache key becomes accountId:channelId, but this hook reads only channelId. That means channel:<private-id> still falls through to generic channel classification for the hook path this PR is trying to fix; pass account context through the hook path or use one authoritative account-aware source consistently.
Confidence: 0.91

Overall correctness: patch is incorrect
Overall confidence: 0.91

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against cf86a9799c3f.

Label changes

Label justifications:

P2: This is a bounded Mattermost channel/session-routing bugfix with limited blast radius but real session-state and delivery impact.
merge-risk: 🚨 session-state: The PR changes Mattermost session namespace selection and can still leave private-channel inference in the wrong namespace when the cache read misses.
merge-risk: 🚨 message-delivery: Wrong private-channel classification can route threaded, scheduled, or heartbeat Mattermost deliveries through the wrong conversation namespace.
rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🧂 unranked krab.
status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body has unit-test output and prose verification only; it needs redacted live Mattermost logs, terminal output, screenshot with diagnostics, recording, or linked artifact, then a PR-body update to trigger fresh ClawSweeper review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence reviewed

PR surface:

Source +114. Total +114 across 5 files.

View PR surface stats

Area	Files	Added	Removed	Net
Source	5	122	8	+114
Tests	0	0	0	0
Docs	0	0	0	0
Config	0	0	0	0
Generated	0	0	0	0
Other	0	0	0	0
Total	5	122	8	+114

What I checked:

PR-head hook read mismatch: At PR head, inferTargetChatType reads mattermostChannelKindCache.get(channelId) without an account id, so it cannot see entries stored under account-scoped keys. (extensions/mattermost/src/channel.ts:817, 6ddd3b3ee3ea)
PR-head scoped cache writes: The monitor writes cached channel type with accountId, so private-channel cache entries are stored in the account-scoped namespace. (extensions/mattermost/src/mattermost/monitor-resources.ts:131, 6ddd3b3ee3ea)
PR-head key contract: makeCacheKey changes the cache key when accountId is provided, confirming that scoped writes and unscoped reads address different entries. (extensions/mattermost/src/mattermost/monitor-gating.ts:20, 6ddd3b3ee3ea)
Plugin hook contract: The current plugin SDK hook accepts only { to }, while resolveOutboundSessionRoute receives accountId, making the account-scoped hook repair a plugin API/owner-boundary concern. (src/channels/plugins/types.core.ts:591, cf86a9799c3f)
Current shipped route behavior: Latest release v2026.6.10 still builds all non-user Mattermost outbound routes as channel, so the canonical bug is not already fixed in shipped code. (extensions/mattermost/src/session-route.ts:36, aa69b12d0086)
History provenance: Blame and path history connect the current route, chat-type mapping, and shared target inference code to recent routing work plus the earlier Mattermost private-channel-as-group mapping. (extensions/mattermost/src/session-route.ts:11, 009d6b261aad)

Likely related people:

vincentkoc: Current blame for the central Mattermost session route, shared target inference, and channel-type mapping points to recent routing work by Vincent Koc. (role: recent area contributor; confidence: high; commits: 009d6b261aad; files: extensions/mattermost/src/session-route.ts, src/infra/outbound/targets.ts, extensions/mattermost/src/mattermost/monitor-gating.ts)
gumadeiras: History shows Gustavo Madeira Santana moved Mattermost outbound session routing behind the plugin boundary, which is the helper this PR changes. (role: introduced behavior boundary; confidence: medium; commits: d6c13d9dc01e; files: extensions/mattermost/src/session-route.ts, extensions/mattermost/src/channel.ts)
steipete: History shows Peter Steinberger landed earlier Mattermost private-channel-as-group behavior and adjacent plugin delivery seam refactors. (role: adjacent chat-type routing contributor; confidence: medium; commits: 355b4c62bcc2, d163278e9cdf; files: extensions/mattermost/src/mattermost/monitor-gating.ts, extensions/mattermost/src/mattermost/monitor.ts, src/channels/plugins/types.core.ts)

What the crustacean ranks mean

🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works

ClawSweeper keeps one durable marker-backed review comment per issue or PR.
Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
Maintainers can also comment @clawsweeper review to request a fresh review only.
Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

- Remove non-existent 'openclaw/plugin-sdk/chat-type' import - Remove unused 'mapMattermostChannelTypeToChatType' import - Remove unused 'setMattermostChannelKindCache' from monitor.ts

Per ClawSweeper review: resolvedTarget.kind === 'group' is not authoritative when the cache is cold — core target resolution can classify channel: as group, moving public channel deliveries into the wrong session namespace. Revert the isGroup check so session-route chatType stays 'channel' for non-user targets. The inferTargetChatType hook + targets.ts reorder still correctly resolve chatType for delivery (resolveHeartbeatDeliveryChatType path), while the session route preserves the existing channel namespace for cold-cache public channels.

Per ClawSweeper review: the heartbeat delivery path (resolveHeartbeatDeliveryTargetWithSessionRoute) calls the session-route resolver which unconditionally returned 'channel' for non-user targets, undoing the inferTargetChatType hook's correct 'group' for private channels. Use the monitor-populated mattermostChannelKindCache in the session route: - Cache hit 'group' → chatType: 'group' (authoritative private channel) - Cache hit 'channel' → chatType: 'channel' (public channel) - Cold miss → chatType: 'channel' (safe default, no regression) The cache is populated by the Mattermost monitor when channel info is fetched from the API, so entries carry authoritative channel type.

…or private channels - Align peer.kind, from, and to in session-route with the resolved chatType so private-channel group routes share the same session-key namespace instead of using the channel: prefix for group-typed channels. - Add TTL expiry (5 min) to mattermostChannelKindCache so stale entries cannot persist beyond the source channelCache lifecycle. - Include accountId in cache keys when available to prevent cross-account cache pollution. Fixes: peer.kind mismatch flagged by ClawSweeper (session-route.ts:52) Fixes: unscoped/immortal cache flagged by ClawSweeper (monitor-gating.ts:8) Ref: openclaw#95646

lsr911 · 2026-06-22T12:10:29Z

@clawsweeper re-review

clawsweeper · 2026-06-22T12:10:32Z

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Fixes oxlint error in monitor-gating.ts line 31.

Fixes TS2345: params.accountId is string | null | undefined but getMattermostChannelKind expects string | undefined.

lsr911 · 2026-06-23T21:08:57Z

Gentle ping — this PR has been waiting for review for 5 days. The ClawSweeper review requested real behavior proof; happy to add it when guidance is available.

lsr911 · 2026-06-25T20:35:58Z

Closing after 5+ weeks with no maintainer review activity. ClawSweeper rated unranked krab (needs proof). The Mattermost private channel fix in this branch can be resubmitted when maintainer guidance is available.

openclaw-barnacle Bot added channel: mattermost Channel integration: mattermost size: S labels Jun 22, 2026

fix(mattermost): remove unused imports causing check-prod-types failure

4d668d1

- Remove non-existent 'openclaw/plugin-sdk/chat-type' import - Remove unused 'mapMattermostChannelTypeToChatType' import - Remove unused 'setMattermostChannelKindCache' from monitor.ts

clawsweeper Bot mentioned this pull request Jun 22, 2026

fix(mattermost): keep private channels in group sessions across delivery paths #95656

Closed

clawsweeper Bot mentioned this pull request Jun 22, 2026

fix(mattermost): key resolved group targets under group: namespace (#95646) #95669

Open

lsr911 force-pushed the fix/issue-95646-Mattermost-chat_type-is-non-d branch from 49cf24c to 82e8943 Compare June 22, 2026 12:09

lsr911 added 2 commits June 22, 2026 20:23

fix(mattermost): add missing curly braces to if statement (eslint curly)

6494813

Fixes oxlint error in monitor-gating.ts line 31.

fix(mattermost): coerce null accountId to undefined for cache lookup

6ddd3b3

Fixes TS2345: params.accountId is string | null | undefined but getMattermostChannelKind expects string | undefined.

This was referenced Jun 24, 2026

fix(mattermost): resolve private-channel chat_type from the real channel kind, not the lossy target prefix #96521

Closed

fix(mattermost): key private channels as group on outbound routing #96645

Open

lsr911 closed this Jun 25, 2026

iloveleon19 mentioned this pull request Jun 29, 2026

Mattermost: chat_type is non-deterministic for private channels (channel-type vs target-string inference); inferTargetChatType hook not implemented #95646

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(mattermost): implement inferTargetChatType hook to resolve private channel chat_type#95655

fix(mattermost): implement inferTargetChatType hook to resolve private channel chat_type#95655
lsr911 wants to merge 7 commits into
openclaw:mainfrom
lsr911:fix/issue-95646-Mattermost-chat_type-is-non-d

lsr911 commented Jun 22, 2026 •

edited

Loading

Uh oh!

clawsweeper Bot commented Jun 22, 2026 •

edited

Loading

Uh oh!

lsr911 commented Jun 22, 2026

Uh oh!

clawsweeper Bot commented Jun 22, 2026 •

edited

Loading

Uh oh!

lsr911 commented Jun 23, 2026

Uh oh!

lsr911 commented Jun 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Uh oh!

Conversation

lsr911 commented Jun 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What Problem This Solves

Why This Change Was Made

Changes

Evidence

Test command

Test output

Behavior verification

Related

Uh oh!

clawsweeper Bot commented Jun 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lsr911 commented Jun 22, 2026

Uh oh!

clawsweeper Bot commented Jun 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lsr911 commented Jun 23, 2026

Uh oh!

lsr911 commented Jun 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

lsr911 commented Jun 22, 2026 •

edited

Loading

clawsweeper Bot commented Jun 22, 2026 •

edited

Loading

clawsweeper Bot commented Jun 22, 2026 •

edited

Loading