fix(mattermost): return 'unknown' ChatType when channel type is not resolvable#57970
fix(mattermost): return 'unknown' ChatType when channel type is not resolvable#57970dave wants to merge 7 commits into
Conversation
Greptile SummaryThis PR correctly fixes the root cause of a misrouting bug: when Key changes:
One inconsistency found: In Confidence Score: 5/5
|
This comment was marked as spam.
This comment was marked as spam.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41bb3489fb
ℹ️ 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".
8d10072 to
1b5302e
Compare
|
All bot review comments addressed:
CI and cc @mukhtharcm — this is a prerequisite for #57565 (thread-id fix with replyTo off). |
…solvable Previously, mapMattermostChannelTypeToChatType returned "channel" for null/undefined input, masking failed channel lookups as valid routing state. All dispatch paths (handlePost, dispatchButtonClick, handleReactionEvent, resolveSessionKey) now early-exit on "unknown" instead of silently routing to the wrong session. monitor-auth's unknown-channel deny path now correctly sets kind/chatType to "unknown". SlashInvocationAuth updated to discriminated union; resolveSlackReplyToMode guards against "unknown" indexing. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
…anch Addresses Greptile review comment: the local channel-lookup-failure path in authorizeSlashInvocation was still returning kind: "channel", inconsistent with the discriminated union refactor and monitor-auth's unknown-channel deny. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Addresses contributor review suggestion: add a comment explaining why we return the base session key early when kind is 'unknown', and why this is safe (handlePost drops the post before the session key is used for routing). Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
…eturning coerced key Addresses Codex P1 review: returning a coerced 'channel' session key caused the system event to be enqueued into the wrong session before dispatchButtonClick's guard ever ran. Throwing instead causes the caller's try/catch in interactions.ts to skip enqueueSystemEvent entirely, truly failing closed. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
…onKey The throw for unknown kind was placed after resolveAgentRoute, doing unnecessary work. Move it earlier to also eliminate the kind coercion in the peer object. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
1b5302e to
49c8e18
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 49c8e1805b
ℹ️ 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".
| if (kind === "unknown") { | ||
| logVerboseMessage( | ||
| `mattermost: drop post (cannot determine channel type for ${channelId}, post=${post.id ?? "?"})`, | ||
| ); | ||
| return; |
There was a problem hiding this comment.
Avoid deduping posts before unknown-channel drop
This new kind === "unknown" early return drops the post after recentInboundMessages.check(...) has already marked its message ID as seen, so transient channel-type resolution failures can now cause avoidable message loss. If Mattermost resends/replays the same post ID after channel lookup recovers, the retry is treated as a duplicate and skipped for the full dedupe TTL instead of being processed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0eb76ff. Moved the unknown-channel guard to run before recentInboundMessages.check(), using payload.data?.channel_type which is available immediately without any async lookup. This way a transient unknown-channel post is dropped before its ID is registered in the dedup tracker.
…tracker Addresses Codex review: the kind === "unknown" guard was placed after recentInboundMessages.check(), so a transient unknown-channel post would mark its ID as seen before being dropped. If Mattermost replayed the same post after channel recovery, it would be incorrectly deduped. Move the check before the dedup registration, using payload.data.channel_type which is available immediately without a channelInfo lookup. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0eb76ff8d6
ℹ️ 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".
| const channelTypeFromEvent = payload.data?.channel_type; | ||
| if (mapMattermostChannelTypeToChatType(channelTypeFromEvent) === "unknown") { | ||
| logVerboseMessage( |
There was a problem hiding this comment.
Defer unknown-channel drop until fallback lookup runs
handlePost now returns early when payload.data?.channel_type maps to "unknown", but channel_type is optional on MattermostEventPayload and this return happens before resolveChannelInfo(channelId) is consulted. In the channel_type-missing case, we drop the message immediately even when the channel API lookup could still provide a valid type, so inbound posts are lost unnecessarily. This guard should use the existing channelTypeFromEvent ?? channelInfo?.type fallback path before deciding to drop.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3779f15. The early guard now calls resolveChannelInfo when channelTypeFromEvent maps to unknown — if the API returns a valid type we continue normally, only dropping when both the event field AND the API fallback fail. The second resolveChannelInfo call in the normal path hits the cache (same tick) so no extra network request is made.
Codex review: dropping immediately when payload.data.channel_type is missing lost messages that the API lookup could have recovered. Now the early guard only drops when both the event field AND the resolveChannelInfo API fallback return unknown. The second resolveChannelInfo call hits the cache (same tick) so no extra network request is made. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3779f15360
ℹ️ 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".
|
|
||
| const channelInfo = await resolveChannelInfo(channelId); | ||
| const channelType = payload.data?.channel_type ?? channelInfo?.type ?? undefined; | ||
| const channelType = channelTypeFromEvent ?? channelInfo?.type ?? undefined; |
There was a problem hiding this comment.
Use fallback channel type when event type is blank
handlePost now does a fallback lookup when payload.data.channel_type maps to "unknown", but the later assignment still uses channelTypeFromEvent ?? channelInfo?.type. If Mattermost sends an empty string for channel_type, that expression keeps the empty string (because it is not null/undefined), so kind becomes "unknown" and the post is dropped even when resolveChannelInfo(channelId) returned a valid type. This creates avoidable message loss on malformed-but-recoverable events.
Useful? React with 👍 / 👎.
|
Thanks for the context here. I swept through the related work, and this is now duplicate or superseded. Close as superseded: current main still has the Mattermost missing-channel-type bug, but the same remaining work is now tracked by a newer, narrower Mattermost-only PR with maintainer routing and better proof, while this branch still has a functional fallback bug and widens the public ChatType/SDK contract. So I’m closing this here and keeping the remaining discussion on the canonical linked item. Review detailsBest possible solution: Use #78530 or an equivalent Mattermost-local fail-closed implementation, then rebase the dependent Mattermost thread-routing PR onto that result. Do we have a high-confidence way to reproduce the issue? Yes. Source inspection on current main shows missing Mattermost channel type still maps to Is this the best way to solve the issue? No. Failing closed is the right direction, but this branch still drops recoverable blank event types and broadens shared ChatType/SDK contracts; the Mattermost-local replacement PR is the narrower maintainable path. Security review: Security review cleared: The diff does not add dependencies, workflows, package resolution changes, secret handling, permissions, or new command execution surfaces; the concerns are functional/API-contract ones. What I checked:
Likely related people:
Codex review notes: model gpt-5.5, reasoning high; reviewed against d4c751998910. |
Summary
mapMattermostChannelTypeToChatTypereturned"channel"fornull/undefinedinput, making a failed channel lookup indistinguishable from a successful public-channel lookup.mapMattermostChannelTypeToChatType(null/undefined)now returns"unknown". All dispatch paths (handlePost,dispatchButtonClick,handleReactionEvent,resolveSessionKey) early-exit on"unknown"with a verbose log drop instead of silently misrouting."unknown"added toChatType,MattermostChatTypeKey, and theok: falsebranch ofMattermostCommandAuthDecision. Type cascade fixes inSlashInvocationAuth,resolveSlackReplyToMode, andbuildChannelOutboundSessionRoute."x") still returns"channel"— only the null/undefined case is affected.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause / Regression History (if applicable)
mapMattermostChannelTypeToChatTypehad no guard for the null/undefined case — it fell through toreturn "channel", treating a missing channel type as a successful public-channel match."unknown", so a wrong fallback was structurally invisible.Regression Test Plan (if applicable)
monitor-gating.test.ts,monitor.channel-kind.test.tsmapMattermostChannelTypeToChatType(undefined)andmapMattermostChannelTypeToChatType(null)return"unknown", not"channel"."channel"forundefined).User-visible / Behavior Changes
When a Mattermost channel lookup fails (channel type missing from event), the post/reaction/button click is now silently dropped (with a verbose log) instead of being misrouted to the wrong agent session. In practice this affects only broken/degenerated WebSocket events — normal Mattermost events always include a channel type.
Diagram (if applicable)
Security Impact (required)
Repro + Verification
Environment
Steps
channel_type(or simulate by patchingresolveChannelInfoto returnnull)Expected
mattermost: drop post (cannot determine channel type ...)logged at verbose levelActual (before fix)
Evidence
mapMattermostChannelTypeToChatType(undefined)expectation changed from"channel"to"unknown")Human Verification (required)
tsgotype check passes clean. Unit tests pass:monitor-gating.test.ts,monitor.channel-kind.test.ts,monitor-auth.test.ts. Pre-commit checks (pnpm check) pass."x") still return"channel"— only null/undefined is affected.Review Conversations
If a bot review conversation is addressed by this PR, resolve that conversation yourself. Do not leave bot review conversation cleanup for maintainers.
Compatibility / Migration
Risks and Mitigations