fix(slack): classify D-prefix DMs correctly when channel_type disagrees#10630
Closed
mcaxtr wants to merge 6 commits intoopenclaw:mainfrom
Closed
fix(slack): classify D-prefix DMs correctly when channel_type disagrees#10630mcaxtr wants to merge 6 commits intoopenclaw:mainfrom
mcaxtr wants to merge 6 commits intoopenclaw:mainfrom
Conversation
…w#10476) * docs(markdownlint): enable autofixable rules except list numbering * docs(zalo): fix malformed bot platform link
…aw#10499) * docs(install): revamp installer internals for readability and accuracy Restructure the installer internals page with better flow and Mintlify components (CardGroup, Steps, Tabs, AccordionGroup). All flags, env vars, and behavioral descriptions cross-checked against install.sh, install-cli.sh, and install.ps1 source code. - Add CardGroup chooser and Quick Commands section at top - Organize each script into consistent Flow → Examples → Reference pattern - Move flags/env var tables into collapsible Accordions - Consolidate troubleshooting into AccordionGroup at bottom - Add missing flags (--version, --beta, --verbose, --help, etc.) - Add missing env vars (OPENCLAW_VERSION, OPENCLAW_BETA, etc.) - Document install-cli.sh fully (was one paragraph) - Fix non-interactive checkout detection behavior (defaults to npm) - Use --proto/--tlsv1.2 in curl examples to match script usage - No content deleted; all original info preserved or relocated * fix(docs): correct in-page anchor hrefs for installer cards * docs(install): replace CardGroup with table for installer overview
Comment on lines
+785
to
+792
| username: overrides?.botAsAuthor ? "bot" : "otheruser", | ||
| discriminator: "0", | ||
| }, | ||
| })), | ||
| }, | ||
| } as unknown as Parameters<DiscordReactionListener["handle"]>[0]; | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
Invalid cfg type cast
makeReactionListenerParams() sets cfg to {} as ReturnType<... ? never : never>, which resolves to never and makes cfg unusable/incorrectly typed at compile time. This should be a concrete LoadedConfig/config type expected by DiscordReactionListener (or a minimal stub of that type), otherwise TS builds can fail or the test will rely on as unknown as casts elsewhere.
Suggested change
| username: overrides?.botAsAuthor ? "bot" : "otheruser", | |
| discriminator: "0", | |
| }, | |
| })), | |
| }, | |
| } as unknown as Parameters<DiscordReactionListener["handle"]>[0]; | |
| } | |
| cfg: {} as import("../config/load.js").LoadedConfig, |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/discord/monitor.test.ts
Line: 785:792
Comment:
**Invalid cfg type cast**
`makeReactionListenerParams()` sets `cfg` to `{} as ReturnType<... ? never : never>`, which resolves to `never` and makes `cfg` unusable/incorrectly typed at compile time. This should be a concrete `LoadedConfig`/config type expected by `DiscordReactionListener` (or a minimal stub of that type), otherwise TS builds can fail or the test will rely on `as unknown as` casts elsewhere.
```suggestion
cfg: {} as import("../config/load.js").LoadedConfig,
```
How can I resolve this? If you propose a fix, please make it concise.
Contributor
Author
|
Closing in favor of a clean PR with only the relevant changes (the original included unrelated commits from fork sync). |
18 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #8421
Slack DMs (channel IDs starting with
D) are misclassified as channels/groups when the event payload provides achannel_typethat disagrees with the channel ID prefix. This causes DMs to bypassdmScope: "main"routing and create separate per-channel sessions instead of merging with the main session.lobster-biscuit
Repro Steps
session.dmScope: "main"withchannels.slack.dm.policy: "open"agent:main:slack:channel:d0acp6b1t8vinstead ofagent:main:mainRoot Cause
normalizeSlackChannelType()incontext.tstrusts the event'schannel_typefield even when it contradicts the channel ID prefix. Slack DM channel IDs always start withD, which definitively indicates a DM. When Slack sends an incorrectchannel_type(e.g.,"channel"or"group"for a D-prefix ID), the normalization function returns the wrong type without cross-validating against the ID prefix. This causes downstream routing to usepeer.kind = "channel"instead of"dm", completely bypassing dmScope session logic.Behavior Changes
normalizeSlackChannelType()now cross-validates: when a channel ID starts withD(inferred type"im"), any contradictingchannel_typeis overridden to"im"prepareSlackMessage,isChannelAllowed,resolveSlackSystemEventSessionKeyCodebase and GitHub Search
slack DM misclassified,slack dm kind,slack dmScope): 0 competing PRsnormalizeSlackChannelTypeto confirm the fix covers all code pathsTests
pnpm build— passespnpm check— passes (0 warnings, 0 errors)pnpm test— full suite passesNew tests (6 total, all fail before fix, pass after):
context.test.ts(3 new):overrides wrong channel_type for D-prefix DM channels— verifies"channel","group","mpim"all resolve to"im"for D-prefix IDspreserves correct channel_type for D-prefix DM channels— sanity check that"im"stays"im"does not override G-prefix channel_type— verifies ambiguous G-prefix preserves provided typeprepare.inbound-contract.test.ts(2 new):classifies D-prefix DMs correctly even when channel_type is wrong— integration test: D-prefix +channel_type: "channel"→isDirectMessage: true, session routes toagent:main:main,ChatType: "direct"classifies D-prefix DMs when channel_type is missing— integration test: D-prefix + nochannel_type→ correctly inferred as DMEvidence
Before fix:
After fix:
Greptile Overview
Greptile Summary
channel_typeagainst the channel ID prefix (D-prefix always treated asim).dmScope: "main"even with missing/incorrectchannel_type.peer.kind/peer.idbased on channel type.Confidence Score: 4/5