Skip to content

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

Closed
ZengWen-DT wants to merge 2 commits into
openclaw:mainfrom
ZengWen-DT:fix/95646-mattermost-chat-type
Closed

fix(mattermost): keep private channels in group sessions across delivery paths#95656
ZengWen-DT wants to merge 2 commits into
openclaw:mainfrom
ZengWen-DT:fix/95646-mattermost-chat-type

Conversation

@ZengWen-DT

@ZengWen-DT ZengWen-DT commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

A Mattermost private channel (P/G) is addressed as channel:<id> — the
same target prefix as a public channel — yet its authoritative chat type is
group. Inbound classification keys it under group:<id>, but every
delivery-side path that derived chat type from the target string returned
channel at the channel: rule before consulting the per-plugin
messaging.inferTargetChatType hook. Mattermost also never implemented that
hook. So outbound replies, scheduled/threaded subagent announcements, and
heartbeat deliveries to a private channel landed under a phantom
channel:<id> session namespace, split from the inbound group:<id> one —
fail-closed delivery or a conversation split across two session keys.

Closes #95646

Why This Change Was Made

The previous revision of this PR only fixed resolveMattermostOutboundSessionRoute.
That left the two generic string-inference paths the issue calls out
(inferChatTypeFromTarget and announce-delivery inferDeliveryTargetChatType)
still collapsing private channels to channel. This revision closes the whole
decision surface:

  • Implements messaging.inferTargetChatType for Mattermost, backed by an
    id-keyed chat-type cache warmed both by outbound target resolution
    (/api/v4/channels/<id> type lookup) and by inbound monitor channel reads
    (resolveChannelInfo). The cache lives in a leaf module (chat-type-cache.ts)
    with no client/runtime deps, so the sync hook stays off the lazy
    channel.runtime chunk (build-verified, no INEFFECTIVE_DYNAMIC_IMPORT).
  • Consults the plugin hook before the generic channel: default in both
    inferChatTypeFromTarget (src/infra/outbound/targets.ts) and
    inferDeliveryTargetChatType (src/agents/subagent-announce-delivery.ts).
    Plugins that do not classify the target return undefined and keep the exact
    existing channel default — so this is behavior-preserving for every other
    channel.
  • Keeps the resolved group kind in peer/from in the outbound session
    route so the derived session key matches the inbound group:<id> namespace
    (this addresses the sibling PR fix(mattermost): implement inferTargetChatType hook to resolve private channel chat_type #95655's reported gap, where chatType changed
    but peer/from stayed channel).

Because the hook is synchronous ({ to } => ChatType | undefined), it answers
authoritatively only when the channel type is already known (resolved outbound
or seen inbound). Otherwise it returns undefined and core keeps the safe
channel default. The cache is refreshed on every resolveChannelInfo (5-min
monitor TTL), so a private↔public conversion self-corrects within the TTL.

This stays inside the Mattermost plugin boundary plus the two generic
target→chat-type inference sites; it adds no config, migration, or new
channel-specific policy in core.

User Impact

Mattermost private-channel replies, scheduled/threaded subagent announcements,
and heartbeat deliveries now stay in the same group:<id> session namespace as
the inbound conversation instead of creating a phantom public-channel session.
Public channels still resolve to channel; direct messages still resolve to
direct. No other channel changes behavior (non-implementing plugins keep the
prior channel:/thread: default).

Evidence

Dependency premise — Mattermost upstream channel types (O open, P private,
D direct, G group), server/public/model/channel.go L25-31:
https://github.com/mattermost/mattermost/blob/master/server/public/model/channel.go#L25-L31

New behavior tests (group reached through the hook before the generic
channel default):

$ node scripts/run-vitest.mjs extensions/mattermost/src/mattermost/chat-type-cache.test.ts src/infra/outbound/targets.test.ts
extensions/.../chat-type-cache.test.ts > resolves a recorded private channel as group  ✓
extensions/.../chat-type-cache.test.ts > keeps a recorded public channel as channel    ✓
src/infra/outbound/targets.test.ts > consults the plugin hook before the generic channel default  ✓  (chatType === "group")
src/infra/outbound/targets.test.ts > falls back to the channel default when the hook returns undefined  ✓  (chatType === "channel")

Affected suites, full pass (no regressions across the two reordered generic
inference sites and the whole Mattermost plugin):

$ node scripts/run-vitest.mjs extensions/mattermost src/infra/outbound/targets.test.ts src/agents/subagent-announce-delivery.test.ts
src/agents/subagent-announce-delivery.test.ts   Tests 101 passed (101)
src/infra/outbound/targets.test.ts              Tests  81 passed (81)
extensions/mattermost (41 files)                Tests 507 passed (507)
[test] passed 3 Vitest shards

Type, build, cycles, format:

$ pnpm tsgo:core && pnpm tsgo:extensions
# exit 0

$ pnpm build
# exit 0, no [INEFFECTIVE_DYNAMIC_IMPORT]  (sync hook leaf module stays off the lazy channel.runtime chunk)

$ pnpm check:import-cycles
Import cycle check: 0 runtime value cycle(s).

$ pnpm exec oxfmt --check --threads=1 <changed files>
All matched files use the correct format.

What was not tested: no live Mattermost workspace round-trip — this checkout
has no Mattermost credentials. The fix is network-before classification
logic (it reads the authoritative channel type and chooses a session namespace;
the Mattermost send target stays channel:<id>), which the unit + plugin-level
tests above drive through the real resolver, the real inferTargetChatType
hook, and both generic inference sites. Live workspace proof would only re-show
the same deterministic classification.

AI-assisted (Claude). Proof above was run manually in a clean worktree.

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

clawsweeper Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 22, 2026, 3:13 PM ET / 19:13 UTC.

Summary
The PR adds Mattermost target chat-type inference, records resolved channel types, reorders generic chat-type inference to consult plugin hooks for channel: targets, and preserves resolved group routes in Mattermost outbound session keys.

PR surface: Source +137, Tests +155. Total +292 across 12 files.

Reproducibility: yes. for source-level reproduction: current main maps Mattermost private channel type P to group, while outbound/session derivation can still collapse channel:<id> targets to channel. I did not establish a live Mattermost workspace repro in this read-only review.

Review metrics: none identified.

Stored data model
Persistent data-model change detected: persistent cache schema: extensions/mattermost/src/channel.ts, persistent cache schema: extensions/mattermost/src/mattermost/chat-type-cache.test.ts, persistent cache schema: extensions/mattermost/src/mattermost/monitor-resources.ts, persistent cache schema: extensions/mattermost/src/mattermost/target-resolution.ts, serialized state: extensions/mattermost/src/session-route.test.ts, serialized state: 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 session namespace split; sibling PRs overlap but do not safely supersede this PR.

Members:

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: 🦐 gold shrimp
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:

  • [P1] Add TTL or refresh semantics for the Mattermost channel-kind/chat-type caches and cover stale public/private type changes.
  • [P1] Add redacted live Mattermost proof showing private-channel delivery/session namespace and public-channel behavior after the fix.
  • Update the PR body with that proof so a fresh ClawSweeper review can reassess the merge gate.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body contains unit/plugin test output and explicitly says no live Mattermost workspace round-trip was run, so after-fix real behavior proof is still missing. 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 PR changes Mattermost session-key classification for private-channel delivery paths, so stale or wrong channel-type decisions can split session state across group and channel namespaces.
  • [P1] Threaded, scheduled, or heartbeat deliveries can be misrouted if channel:<id> inference uses a stale cached type after Mattermost channel visibility/type changes.
  • [P1] The contributor has not provided redacted live Mattermost workspace proof, so CI and unit tests do not prove the real transport/session path.

Maintainer options:

  1. Fix cache lifetime and prove the route (recommended)
    Add bounded refresh or expiry for the Mattermost channel-kind/chat-type caches, cover stale public/private type changes in tests, and add redacted live Mattermost proof before merge.
  2. Hold for contributor proof
    Keep the PR open while the contributor supplies real Mattermost logs, terminal output, screenshot, or recording with private details redacted.
  3. Accept unit-only proof and stale-cache risk
    Maintainers may intentionally merge with green CI only, but they would own the risk that real Mattermost type changes keep using an old session namespace.

Next step before merge

  • [P1] The PR needs contributor/maintainer handling because real Mattermost proof is missing and the cache-lifetime finding should be addressed on the PR before merge; automation cannot prove the contributor's live workspace.

Security
Cleared: The diff stays in Mattermost TypeScript and tests, reuses the existing SSRF-guarded Mattermost client path, and does not change dependencies, workflows, secrets, or package metadata.

Review findings

  • [P2] Expire Mattermost channel-kind decisions — extensions/mattermost/src/mattermost/target-resolution.ts:155-156
Review details

Best possible solution:

Land the Mattermost fix only after channel-kind caching has TTL/refresh semantics matching authoritative lookups and redacted live proof shows private-channel delivery stays in group while public channels stay channel.

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

Yes for source-level reproduction: current main maps Mattermost private channel type P to group, while outbound/session derivation can still collapse channel:<id> targets to channel. I did not establish a live Mattermost workspace repro in this read-only review.

Is this the best way to solve the issue?

No as submitted. The plugin-boundary fix shape is right, but the new channel-kind cache needs bounded refresh semantics and the PR needs real Mattermost proof before it is the best merge candidate.

Full review comments:

  • [P2] Expire Mattermost channel-kind decisions — extensions/mattermost/src/mattermost/target-resolution.ts:155-156
    mattermostOpaqueTargetCache now stores the resolved group/channel kind forever. After a channel is looked up once, a later public/private conversion returns the cached kind before re-fetching /channels/<id>, and line 156 also warms the sync hook from that stale value; this can keep deliveries in the old session namespace even though the PR body says conversions self-correct within the monitor TTL. Add matching TTL/refresh semantics for the kind caches and cover the stale-type case.
    Confidence: 0.82

Overall correctness: patch is incorrect
Overall confidence: 0.84

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦐 gold shrimp.
  • add 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 contains unit/plugin test output and explicitly says no live Mattermost workspace round-trip was run, so after-fix real behavior proof is still missing. 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.
  • remove rating: 🦪 silver shellfish: Current PR rating is rating: 🧂 unranked krab, so this older rating label is no longer current.
  • remove status: 🛠️ actively grinding: Current PR status label is status: 📣 needs proof.

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: 🚨 message-delivery: The changed Mattermost classification can route threaded, scheduled, or heartbeat delivery under the wrong namespace if the channel type is stale or wrong.
  • merge-risk: 🚨 session-state: The PR changes Mattermost session-key chat type for private-channel routes, which can split or stale session state if the cache decision drifts from the authoritative channel type.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦐 gold shrimp.
  • 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 contains unit/plugin test output and explicitly says no live Mattermost workspace round-trip was run, so after-fix real behavior proof is still missing. 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 +137, Tests +155. Total +292 across 12 files.

View PR surface stats
Area Files Added Removed Net
Source 7 167 30 +137
Tests 5 155 0 +155
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 12 322 30 +292

What I checked:

Likely related people:

  • vincentkoc: Current blame and recent history for Mattermost target/session routing and shared target inference point to Vincent's recent routing cleanup in this checkout. (role: recent area contributor; confidence: high; commits: 65adb1358149, c645ec4555c0; files: extensions/mattermost/src/session-route.ts, extensions/mattermost/src/mattermost/target-resolution.ts, src/infra/outbound/targets.ts)
  • gumadeiras: History shows Gustavo moved Mattermost outbound session routing behind the plugin boundary, which is the central owner boundary 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 landed earlier Mattermost private-channel-as-group behavior and adjacent monitor/runtime refactors that define the inbound side of this mismatch. (role: adjacent chat-type routing contributor; confidence: medium; commits: 355b4c62bcc2, 005b25e9d42a; files: extensions/mattermost/src/mattermost/monitor.ts, extensions/mattermost/src/mattermost/monitor-gating.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.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. labels Jun 22, 2026
@ZengWen-DT
ZengWen-DT marked this pull request as ready for review June 22, 2026 04:03
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. labels Jun 22, 2026
@clawsweeper clawsweeper Bot added merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. labels Jun 22, 2026
@ZengWen-DT
ZengWen-DT force-pushed the fix/95646-mattermost-chat-type branch from c4ea217 to 061020c Compare June 22, 2026 08:16
@openclaw-barnacle openclaw-barnacle Bot added agents Agent runtime and tooling size: M and removed size: S labels Jun 22, 2026
@ZengWen-DT ZengWen-DT changed the title fix(mattermost): keep private channel deliveries in group sessions fix(mattermost): keep private channels in group sessions across delivery paths Jun 22, 2026
@ZengWen-DT

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
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.

…ery paths

Mattermost private channels (P/G) are addressed as channel:<id>, the same
prefix as public channels, but their authoritative chat type is group. The
generic inferChatTypeFromTarget and announce-delivery inference returned
channel at the channel: rule before consulting the per-plugin
inferTargetChatType hook, so outbound/scheduled/heartbeat deliveries to a
private channel landed under a phantom channel:<id> session namespace split
from the inbound group:<id> one.

- Implement Mattermost messaging.inferTargetChatType, backed by an id-keyed
  chat-type cache warmed by outbound target resolution and inbound monitor
  channel reads (leaf module so the sync hook stays off the lazy
  channel.runtime chunk).
- Consult the plugin hook before the generic channel: default in both
  inferChatTypeFromTarget and announce-delivery inference; plugins that do not
  classify the target return undefined and keep the existing channel default.
- Keep the outbound session route resolved group kind in peer/from so the
  session key matches the inbound namespace.

Closes openclaw#95646
@ZengWen-DT
ZengWen-DT force-pushed the fix/95646-mattermost-chat-type branch from 061020c to d7c11b5 Compare June 22, 2026 09:24
@clawsweeper clawsweeper Bot added status: 🛠️ actively grinding The PR author has acted after the latest ClawSweeper review and work remains. and removed status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jun 22, 2026
@ZengWen-DT

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Addressed the prior P1 rank-up move and the [P2] finding: send.test.ts now mocks/resets/defaults fetchMattermostChannel on the ./client.js mock, matching the named export that target-resolution.ts imports. Full Mattermost suite + the two generic inference sites are green on this head, and all CI checks pass. The remaining open item is live-workspace proof, which this checkout cannot produce (no Mattermost credentials); the fix is network-before classification logic driven end-to-end by the unit/plugin tests.

@clawsweeper

clawsweeper Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
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.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 🛠️ actively grinding The PR author has acted after the latest ClawSweeper review and work remains. labels Jun 22, 2026
@clawsweeper clawsweeper Bot added the status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. label Jun 22, 2026
@ZengWen-DT

Copy link
Copy Markdown
Contributor Author

Closing this in favor of the sibling PRs on the same canonical issue (#95646).

Two reasons this isn't the right merge candidate:

  • Correctness gap I confirmed: mattermostOpaqueTargetCache (extensions/mattermost/src/mattermost/target-resolution.ts) caches the resolved group/channel kind with no TTL and only re-records on a cache miss, so the outbound path keeps returning a stale kind after a private↔public conversion. The "self-corrects within the 5-min TTL" claim in the body only holds for the inbound monitor path (resolveChannelInfo, 5-min channelCache), not the outbound-resolved path. ClawSweeper's [P2] finding is right.
  • Proof wall: the real fix needs a live Mattermost type-conversion round-trip, which I can't reproduce here (no Mattermost workspace).

Deferring to #95655 / #95669 for the canonical fix.

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

Labels

agents Agent runtime and tooling channel: mattermost Channel integration: mattermost merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. P2 Normal backlog priority with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: M status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

1 participant