fix(matrix): remove unconditional 2-member DM fallback classification#54787
fix(matrix): remove unconditional 2-member DM fallback classification#54787hclsys wants to merge 2 commits intoopenclaw:mainfrom
Conversation
isDirectMessage() has a fallback that classifies ANY 2-person room as a DM based solely on member count, even when m.direct account data says it is NOT a DM. This causes multi-room routing confusion — replies get sent to both rooms simultaneously. Root cause: extensions/matrix/src/matrix/monitor/direct.ts:104-112. The unconditional isStrictDirectMembership() call after the isDm() check was reintroduced in 94693f7 (Matrix plugin rebuild), regressing the fix from d4a960f / PR openclaw#31023 which removed this exact heuristic. Fix: remove the unconditional fallback. Only the isDm() + strict membership path (lines 89-102) should classify a room as DM. Without m.direct account data, a 2-person room is treated as a group room. Closes openclaw#54772 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: HCL <chenglunhu@gmail.com>
Greptile SummaryThis PR removes the unconditional 2-member DM fallback in The production code change is correct and minimal. However, the test file was not updated to match — two tests ( Confidence Score: 4/5
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ea9745c91
ℹ️ 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".
| log( | ||
| `matrix: dm check room=${roomId} result=group members=${joinedMembers?.length ?? "unknown"}`, | ||
| ); |
There was a problem hiding this comment.
Keep strict-membership fallback when m.direct is unavailable
After this change, isDirectMessage falls through to false whenever client.dms.isDm(roomId) is false, even if membership already proves a strict 1:1 room; that turns transient/partial m.direct states (for example startup or dms.update() failures) into DM misclassification. Those messages are then handled as rooms and can be dropped by room policy gates in extensions/matrix/src/matrix/monitor/handler.ts (for example groupPolicy checks), so valid DMs never reach DM/pairing flow. Please keep a guarded strict-membership fallback (or equivalent retry/repair path) so DM handling does not depend exclusively on fresh m.direct cache state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7e53d66. Added dmCacheReliable flag — tracks whether dms.update() succeeded. When cache is healthy, isDm() is authoritative (no member-count fallback = no false positives). When cache failed (startup/network), fallback activates to avoid dropping real DMs. Best of both worlds.
Address Codex P1: removing the fallback entirely drops DMs when dms.update() fails transiently (startup, network issues). Fix: track dmCacheReliable flag. When dms.update() succeeds, isDm() is authoritative — 2-person rooms without m.direct are group rooms (no false positives). When dms.update() fails, fall back to member-count heuristic to avoid dropping real DMs. This preserves the prior fix (PR openclaw#31023) while handling the transient-failure edge case Codex identified. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: HCL <chenglunhu@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e53d6650e
ℹ️ 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".
| try { | ||
| await client.dms.update(); | ||
| dmCacheReliable = true; | ||
| } catch (err) { | ||
| dmCacheReliable = false; |
There was a problem hiding this comment.
Retry DM cache refresh immediately after update failures
When client.dms.update() throws here, dmCacheReliable is set to false, but lastDmUpdateMs was already advanced earlier in refreshDmCache, so subsequent isDirectMessage calls within the 30s TTL skip refresh and keep using the fallback path. In that window, any strict 2-member room is treated as a DM, which can reintroduce duplicate/cross-routed replies after a transient Matrix error. Please avoid throttling failed refreshes (for example, only stamp the TTL on success, or reset it in catch) so the next message can recover immediately.
Useful? React with 👍 / 👎.
lobster-biscuit
Closes #54772
Problem
Two separate 2-person Matrix rooms with the same participants get both classified as DMs, causing duplicate/cross-routed replies. A room that is NOT marked as DM in m.direct account data still gets treated as one because of a member-count fallback.
Root cause
extensions/matrix/src/matrix/monitor/direct.ts:104-112— after theisDm(roomId)check (which correctly uses m.direct account data), there is an unconditional fallback that callsisStrictDirectMembership()and returnstruefor ANY 2-person room regardless of m.direct status.This was fixed in PR #31023 (
d4a960fcc) but reintroduced in commit94693f7ff(Matrix plugin rebuild).User impact
Multi-room workflows impossible. Every 2-person room becomes a DM — replies get sent to ALL of them simultaneously, producing duplicate or out-of-context messages.
Fix
Remove the unconditional fallback (lines 104-112). Only the
isDm()+ strict membership path should classify rooms as DMs. Net deletion: -11 lines.This restores the behavior from PR #31023.
How to verify
Tests
Restores the same behavior as the tested PR #31023 (
d4a960fcc). TheisStrictDirectMembership()function itself is unchanged — only its unconditional call site is removed.🤖 Generated with Claude Code