Skip to content

fix(slack): ensure distinct sessions for public channels in replyToMode="all"#75995

Open
dchekmarev wants to merge 2 commits intoopenclaw:mainfrom
dchekmarev:fix/messsage-wrong-thread
Open

fix(slack): ensure distinct sessions for public channels in replyToMode="all"#75995
dchekmarev wants to merge 2 commits intoopenclaw:mainfrom
dchekmarev:fix/messsage-wrong-thread

Conversation

@dchekmarev
Copy link
Copy Markdown

@dchekmarev dchekmarev commented May 2, 2026 •
edited
Loading

Summary

  • Fixes Slack: top-level channel messages with replyToMode="all" cause dual-session processing #64078
  • Fixes [Bug]: slack responses delivered to wrong thread or outside of thread #75969
  • Problem: Slack public channels ignored replyToMode="all", forcing multiple top-level messages into a single shared session.
  • Why it matters: This caused race conditions and context leakage. Example: If a CRON job posts a report and a user simultaneously sends a new message, the AI incorrectly treats both as the same conversation. This mixes automated data with human chat and breaks thread isolation.
  • What changed:
    • Refined canonicalThreadId logic to allow autoThreadId for public rooms when replyToMode="all" is active.
    • Added a comprehensive 16-case test matrix covering all combinations of channel types and routing modes.
    • Updated existing prepare.thread-session-key.test.ts to align with the fixed routing contract.

Change Type

  • Bug fix (production code + tests)

Scope

  • Integrations (Slack)

Root Cause

The resolveSlackRoutingContext had a strict guard that forced roomThreadId (shared session) for all isRoomish channels. This blocked autoThreadId from being used for top-level messages even when the user explicitly requested distinct threads via replyToMode="all".

Regression Test Plan

  • Coverage level: Unit tests with exhaustive matrix.
  • Target test: extensions/slack/src/monitor/message-handler/prepare-routing.race-condition.test.ts
  • Scenario: Ensure that only Public Rooms in all mode yield distinct sessions, while Group DMs, DMs, and other modes (off, first, batched) maintain their original shared-session behavior to prevent "thread explosion."

Routing State Matrix (Verified)

Channel Type replyToMode Expected Session Result Status
Room (Public) all Distinct (:thread:<ts>) Distinct ✅ Pass (Fails prior fix)
Room (Public) off / first / batched Shared (Channel) Shared ✅ Pass
Group DM Any Shared (Channel) Shared ✅ Pass
Direct Message Any Shared (User) Shared ✅ Pass
Fallback all Distinct (:thread:<ts>) Distinct ✅ Pass (Fails prior fix)

Evidence

All 16 tests in prepare-routing.race-condition.test.ts are passing on the latest main.
Verified that root message seeding correctly propagates to child replies (thread inheritance).

Human Verification

  • Verified that replyToMode="off", "first", and "batched" still result in shared sessions (zero regression for default users).
  • Verified that Group DMs remain "flat" to maintain standard multi-user chat UX.

Compatibility / Migration

  • Backward compatible? Yes. Existing shared-session behavior is preserved for all modes except all.
  • Config/env changes? No.
  • Migration needed? No.

@openclaw-barnacle openclaw-barnacle Bot added channel: slack Channel integration: slack size: S triage: blank-template Candidate: PR template appears mostly untouched. triage: refactor-only Candidate: refactor/cleanup-only PR without maintainer context. triage: test-only-no-bug Candidate: test-only change has no linked bug or behavior evidence. labels May 2, 2026
@clawsweeper
Copy link
Copy Markdown
Contributor

clawsweeper Bot commented May 2, 2026 •
edited
Loading

Codex review: needs real behavior proof before merge.

Summary
The PR changes Slack routing so top-level public-channel messages in replyToMode="all" use per-message thread session keys, adds Slack routing/session regression tests, and updates CHANGELOG.md.

Reproducibility: yes. by source inspection. On current main, two top-level public Slack room messages with replyToMode="all", isRoom=true, and isRoomish=true compute autoThreadId but route through an undefined roomThreadId, so the session key falls back to the shared channel session.

Real behavior proof
Needs real behavior proof before merge: The PR body/comments contain test claims but no after-fix Slack/runtime logs, terminal output, screenshot, recording, or linked artifact; redacted real behavior proof is needed before merge. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, ask a maintainer to comment @clawsweeper re-review.

Next step before merge
Contributor should add redacted real Slack/OpenClaw proof to the PR body; after that, ClawSweeper should re-review automatically or a maintainer can comment @clawsweeper re-review.

Security
Cleared: The diff only touches Slack routing logic, Slack tests, and the changelog; it does not alter dependencies, CI, secrets, permissions, packaging, or code execution paths.

Review details

Best possible solution:

Land the narrow Slack routing fix after the contributor adds redacted real Slack/runtime proof and the targeted Slack routing tests pass.

Do we have a high-confidence way to reproduce the issue?

Yes, by source inspection. On current main, two top-level public Slack room messages with replyToMode="all", isRoom=true, and isRoomish=true compute autoThreadId but route through an undefined roomThreadId, so the session key falls back to the shared channel session.

Is this the best way to solve the issue?

Yes, subject to real behavior proof. The PR is the narrowest maintainable direction I found: let public rooms use autoThreadId in all mode while preserving shared sessions for DMs, group DMs, and non-all public-room modes.

Acceptance criteria:

  • pnpm test extensions/slack/src/monitor/message-handler/prepare-routing.race-condition.test.ts extensions/slack/src/monitor/message-handler/prepare.thread-session-key.test.ts
  • pnpm test extensions/slack/src/monitor/message-handler/prepare.test.ts

What I checked:

Likely related people:

  • bek91: Commit aac83e00cfe7 added seeded Slack thread-root routing and expanded the session-key tests this PR now updates. (role: introduced adjacent inbound thread-session routing behavior; confidence: high; commits: aac83e00cfe7; files: extensions/slack/src/monitor/message-handler/prepare-routing.ts, extensions/slack/src/monitor/message-handler/prepare.thread-session-key.test.ts, extensions/slack/src/monitor/message-handler/prepare.ts)
  • pablohrcarvalho: Commit 11d34700c0fe is the earlier Slack channel/group thread-session isolation work referenced by the current routing comments and changelog. (role: introduced earlier channel thread-session isolation behavior; confidence: medium; commits: 11d34700c0fe; files: extensions/slack/src/monitor/message-handler/prepare-routing.ts, CHANGELOG.md)
  • calder-sandy: Commit 93ac2b43fb9a added top-level Slack DM auto-thread session isolation, an adjacent contract the current PR compares against. (role: adjacent replyToMode="all" session owner; confidence: medium; commits: 93ac2b43fb9a; files: extensions/slack/src/monitor/message-handler/prepare-routing.ts, CHANGELOG.md)
  • steipete: Recent commits and blame show repeated maintenance of the Slack prepare/routing test surface, including the channel runtime refactor and current-main touched files. (role: recent maintainer of Slack prepare routing; confidence: medium; commits: 38c65b40966c, eeef4864494f, 6697c6179cf4; files: extensions/slack/src/monitor/message-handler/prepare.ts, extensions/slack/src/monitor/message-handler/prepare-routing.ts, extensions/slack/src/monitor/message-handler/prepare.thread-session-key.test.ts)

Remaining risk / open question:

  • Contributor-supplied real Slack/OpenClaw proof is missing, so the external PR should not merge yet.
  • This read-only review did not run the targeted Slack tests; validation here is from source inspection and the hydrated PR context.

Codex review notes: model gpt-5.5, reasoning high; reviewed against e53ec522c27f.

Re-review progress:

@dchekmarev dchekmarev force-pushed the fix/messsage-wrong-thread branch 2 times, most recently from e770ce2 to 39e7e3e Compare May 2, 2026 08:53
@dchekmarev dchekmarev force-pushed the fix/messsage-wrong-thread branch from 39e7e3e to a7fe4c9 Compare May 2, 2026 09:32
@dchekmarev
Copy link
Copy Markdown
Author

dchekmarev commented May 2, 2026

@steipete I’m submitting this draft as a TDD-based proof of a Slack routing bug.

The Problem:
Currently, resolveSlackRoutingContext forces all top-level messages in isRoomish channels into a single shared session. This prevents "thread explosion," but it causes a critical race condition when replyToMode="all" is active. If multiple messages are sent simultaneously, their AI responses get routed to the same session, leading to context leakage and incorrect threading (addresses #64078, #75969).

What this PR does:
I've added a comprehensive test matrix in prepare-routing.race-condition.test.ts to document the current vs. expected behavior:

  • Public Channels: Tests are set to fail for replyToMode="all" (Expected: Distinct session per message).

  • Group DMs / DMs: I've intentionally kept these as "Shared" sessions in the tests to maintain the existing flat-chat UX and avoid regressions.

  • Other modes (off, first): Verified they still use shared sessions as intended.

This PR contains tests only. My goal is to align on this routing contract before I implement the narrow fix in prepare-routing.ts.

P.S. I have also updated prepare.thread-session-key.test.ts to align with this new contract. Since that test previously asserted shared sessions for replyToMode="all", it was effectively codifying the bug I'm now addressing.

Looking forward to your feedback!

@clawsweeper clawsweeper Bot mentioned this pull request May 2, 2026
Open
@dchekmarev dchekmarev force-pushed the fix/messsage-wrong-thread branch from 82e92a0 to 1103141 Compare May 2, 2026 12:22
@clawsweeper clawsweeper Bot mentioned this pull request May 2, 2026
Open
@dchekmarev dchekmarev force-pushed the fix/messsage-wrong-thread branch 2 times, most recently from 99b699b to 80fca27 Compare May 4, 2026 02:48
@dchekmarev dchekmarev changed the title [WIP] test: add session linkage verification for Slack roomish channels with replyToMode=all fix(slack): ensure distinct sessions for public channels in all reply mode May 4, 2026
@dchekmarev dchekmarev force-pushed the fix/messsage-wrong-thread branch from 62c9a59 to 224a248 Compare May 4, 2026 03:09
@dchekmarev dchekmarev marked this pull request as ready for review May 4, 2026 03:10
@dchekmarev
Copy link
Copy Markdown
Author

dchekmarev commented May 4, 2026

Hey @steipete since you've been active on Slack-related updates recently, I've updated this PR with the production fix. All matrix tests are now passing. Ready for review.

@dchekmarev dchekmarev force-pushed the fix/messsage-wrong-thread branch from ef1315c to e4faa12 Compare May 4, 2026 07:42
@dchekmarev dchekmarev changed the title fix(slack): ensure distinct sessions for public channels in all reply mode fix(slack): ensure distinct sessions for public channels in replyToMode="all" May 4, 2026
@dchekmarev dchekmarev force-pushed the fix/messsage-wrong-thread branch 5 times, most recently from 9ccb69d to 9be5775 Compare May 4, 2026 13:59
@dchekmarev dchekmarev force-pushed the fix/messsage-wrong-thread branch 2 times, most recently from 0caf972 to b9fdada Compare May 5, 2026 07:46
@openclaw-barnacle openclaw-barnacle Bot added the triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. label May 5, 2026
@dchekmarev dchekmarev force-pushed the fix/messsage-wrong-thread branch from b9fdada to 7bd98f5 Compare May 5, 2026 10:09
@dchekmarev dchekmarev marked this pull request as draft May 7, 2026 13:37
@dchekmarev dchekmarev marked this pull request as ready for review May 7, 2026 13:37
@dchekmarev dchekmarev force-pushed the fix/messsage-wrong-thread branch from 7bd98f5 to fbe53f7 Compare May 9, 2026 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

channel: slack Channel integration: slack size: S triage: blank-template Candidate: PR template appears mostly untouched. triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. triage: refactor-only Candidate: refactor/cleanup-only PR without maintainer context. triage: test-only-no-bug Candidate: test-only change has no linked bug or behavior evidence.

Projects

None yet

Milestone

No milestone

1 participant

@dchekmarev