fix(mattermost): key resolved group targets under group: namespace (#95646)#95669
fix(mattermost): key resolved group targets under group: namespace (#95646)#95669hansraj316 wants to merge 1 commit into
Conversation
…penclaw#95646) A private channel / group DM is classified group by the inbound path (mapMattermostChannelKind: P/G -> group) but the outbound session route flattened every non-user target to channel, splitting the same conversation across group:<id> and a phantom channel:<id> session key. Honor an authoritative resolved group kind so delivery keys match inbound. A bare channel:<id> target with no resolved kind still resolves to channel since the real channel type is not knowable from the target string alone; full coverage of that path needs live Mattermost repro.
openclaw-barnacle
Bot
added
channel: mattermost
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cab9efc6bb
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // would split the same conversation across two session namespaces (#95646). | ||
| // A bare `channel:<id>` with no resolved kind stays `channel` since the real | ||
| // channel type is not knowable from the target string alone. | ||
| const isGroup = !isUser && resolvedKind === "group"; |
There was a problem hiding this comment.
When a target is a public Mattermost channel resolved through directory or normalization, resolvedTarget.kind can still be group: extensions/mattermost/src/mattermost/directory.ts emits kind: "group" for both O and P channels, and src/infra/outbound/target-resolver.ts classifies channel: inputs as group-like. This line now converts those public-channel sends to mattermost:group:<id>, while inbound mapMattermostChannelKind maps Mattermost O channels to channel, so outbound mirrors/thread recovery for public channels split away from the existing inbound session. Only switch to group when the resolver has proven a private/group channel, or keep ambiguous channel: targets under channel.
Useful? React with 👍 / 👎.
|
Codex review: needs real behavior proof before merge. Reviewed June 29, 2026, 4:21 AM ET / 08:21 UTC. Summary PR surface: Source +9, Tests +21. Total +30 across 2 files. Reproducibility: yes. Source inspection shows current main can produce Review metrics: none identified. Stored data model 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: Use Mattermost-owned authoritative channel type or equivalent resolved metadata so only proven Do we have a high-confidence way to reproduce the issue? Yes. Source inspection shows current main can produce Is this the best way to solve the issue? No. Preserving resolved Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 293f0369d03b. Label changesLabel justifications:
Evidence reviewedPR surface: Source +9, Tests +21. Total +30 across 2 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
|
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
@hansraj316 thanks for the PR. ClawSweeper is still waiting on real behavior proof before this can move forward. Useful proof can be a screenshot, short video, terminal output, copied live output, linked artifact, or redacted logs that show the changed behavior after the fix. Please redact private tokens, phone numbers, private endpoints, customer data, and anything else sensitive. Once proof is added to the PR body or a comment, ClawSweeper or a maintainer can re-check it. |
1 participant
What Problem This Solves
Fixes #95646. The
chat_typereported for a Mattermost private channel was non-deterministic across code paths:groupwhen derived from the channel type (authoritative inbound path,mapMattermostChannelKind:P/G→ group), butchannelwhen derived from the delivery target string (a private channel is addressed aschannel:<id>, same prefix as a public channel). The same conversation was keyed under two session namespaces —group:<id>:thread:<root>(inbound) and a phantomchannel:<id>:thread:<root>(delivery) — so threaded/scheduled deliveries failed to match.Why This Change Was Made
The outbound session route flattened every non-user target to
channel, discarding an authoritativegroupkind that the directory resolver already produces (directory.tsemitskind: "group"for private channels / group DMs). The route now honors that resolvedgroupkind for the session namespace andfrom, aligning delivery with the inbound classification. The delivery target string stayschannel:<id>because Mattermost addresses all channels by id; only the session namespace is type-aware.User Impact
Directory-resolved private-channel / group-DM deliveries now key under the same
group:session as inbound, fixing the split-session / fail-closed delivery. A barechannel:<id>target with no resolved kind still resolves tochannel, since the real channel type is not knowable from the target string alone — that residual path warrants live-Mattermost follow-up and is called out here rather than silently half-fixed.Evidence
extensions/mattermost/src/session-route.ts: honor resolvedgroupkind inpeer.kind/chatType/from.extensions/mattermost/src/session-route.test.tsasserts a resolvedgrouptarget keys under:group:<id>(not:channel:<id>) while keepingto: channel:<id>.pnpm test extensions/mattermost/src/session-route.test.ts✅ (7 passed).