fix(mattermost): keep private channels in group sessions across delivery paths#95656
fix(mattermost): keep private channels in group sessions across delivery paths#95656ZengWen-DT wants to merge 2 commits into
Conversation
|
Codex review: needs real behavior proof before merge. Reviewed June 22, 2026, 3:13 PM ET / 19:13 UTC. Summary PR surface: Source +137, Tests +155. Total +292 across 12 files. Reproducibility: yes. for source-level reproduction: current main maps Mattermost private channel type Review metrics: none identified. Stored data model Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest 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 Do we have a high-confidence way to reproduce the issue? Yes for source-level reproduction: current main maps Mattermost private channel type 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:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 81e1ec467cf0. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +137, Tests +155. Total +292 across 12 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
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
|
c4ea217 to
061020c
Compare
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
…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
061020c to
d7c11b5
Compare
|
@clawsweeper re-review Addressed the prior P1 rank-up move and the [P2] finding: |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
Closing this in favor of the sibling PRs on the same canonical issue (#95646). Two reasons this isn't the right merge candidate:
|
What Problem This Solves
A Mattermost private channel (
P/G) is addressed aschannel:<id>— thesame target prefix as a public channel — yet its authoritative chat type is
group. Inbound classification keys it undergroup:<id>, but everydelivery-side path that derived chat type from the target string returned
channelat thechannel:rule before consulting the per-pluginmessaging.inferTargetChatTypehook. Mattermost also never implemented thathook. 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 inboundgroup:<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
(
inferChatTypeFromTargetand announce-deliveryinferDeliveryTargetChatType)still collapsing private channels to
channel. This revision closes the wholedecision surface:
messaging.inferTargetChatTypefor Mattermost, backed by anid-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.runtimechunk (build-verified, noINEFFECTIVE_DYNAMIC_IMPORT).channel:default in bothinferChatTypeFromTarget(src/infra/outbound/targets.ts) andinferDeliveryTargetChatType(src/agents/subagent-announce-delivery.ts).Plugins that do not classify the target return
undefinedand keep the exactexisting
channeldefault — so this is behavior-preserving for every otherchannel.
groupkind inpeer/fromin the outbound sessionroute 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
chatTypechangedbut
peer/fromstayedchannel).Because the hook is synchronous (
{ to } => ChatType | undefined), it answersauthoritatively only when the channel type is already known (resolved outbound
or seen inbound). Otherwise it returns
undefinedand core keeps the safechanneldefault. The cache is refreshed on everyresolveChannelInfo(5-minmonitor 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 asthe inbound conversation instead of creating a phantom public-channel session.
Public channels still resolve to
channel; direct messages still resolve todirect. No other channel changes behavior (non-implementing plugins keep theprior
channel:/thread:default).Evidence
Dependency premise — Mattermost upstream channel types (
Oopen,Pprivate,Ddirect,Ggroup),server/public/model/channel.goL25-31:https://github.com/mattermost/mattermost/blob/master/server/public/model/channel.go#L25-L31
New behavior tests (
groupreached through the hook before the genericchanneldefault):Affected suites, full pass (no regressions across the two reordered generic
inference sites and the whole Mattermost plugin):
Type, build, cycles, 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-leveltests above drive through the real resolver, the real
inferTargetChatTypehook, 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.