Skip to content

fix(slack): keep outbound replies in thread sessions#96650

Closed
harjothkhara wants to merge 1 commit into
openclaw:mainfrom
harjothkhara:codex/96535-slack-outbound-mirror-session
Closed

fix(slack): keep outbound replies in thread sessions#96650
harjothkhara wants to merge 1 commit into
openclaw:mainfrom
harjothkhara:codex/96535-slack-outbound-mirror-session

Conversation

@harjothkhara

@harjothkhara harjothkhara commented Jun 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Closes #96535

What Problem This Solves

Fixes an issue where Slack thread replies could create new outbound delivery-mirror sessions keyed by a child reply timestamp instead of reusing the existing thread session. This caused assistant-only mirror sessions to accumulate for one Slack conversation.

Why This Change Was Made

Slack outbound session routing now treats inherited reply metadata differently from explicit reply targets. Inherited same-thread sends prefer the recovered/current Slack thread session, while explicit reply targets still keep the existing reply-first behavior. The generic SDK route-helper default remains unchanged for other channels.

User Impact

Slack users should see outbound mirrored replies stay attached to the existing Slack thread session instead of producing near-empty sibling session records for each reply.

Evidence

  • Red proof before fix: node scripts/run-vitest.mjs extensions/slack/src/channel.test.ts -t "uses the current Slack thread session before inherited child reply ids" failed because the route used :thread:1712345688.654321 instead of the current thread :thread:1712345678.123456.
  • Focused proof after fix: node scripts/run-vitest.mjs extensions/slack/src/channel.test.ts src/plugin-sdk/thread-aware-outbound-session-route.test.ts src/infra/outbound/message-action-runner.threading.test.ts passed 3 Vitest shards / 75 tests.
  • git diff --check passed.
  • AI-assisted change. The repo-local autoreview helper was attempted, but the local approval layer blocked it because it would send the unpushed local diff to external model services. I performed a manual two-lens review instead: runtime correctness and contract/scope review found no follow-up changes needed.

@openclaw-barnacle openclaw-barnacle Bot added channel: slack Channel integration: slack size: S labels Jun 25, 2026
@clawsweeper

clawsweeper Bot commented Jun 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 24, 2026, 11:24 PM ET / 03:24 UTC.

Summary
Adds an optional reply-explicitness signal to outbound session-route resolution and makes Slack prefer the current thread session for inherited replies while preserving explicit reply targets.

PR surface: Source +7, Tests +45. Total +52 across 6 files.

Reproducibility: yes. at source level: current main sends Slack replyToId, threadId, and currentSessionKey into a helper whose default precedence picks replyToId before the recovered thread session. I did not run a live Slack workspace replay.

Review metrics: 1 noteworthy metric.

  • Channel route API surface: 1 optional param added. The new replyToIsExplicit field is an extension-facing route signal, so maintainers should notice the additive contract surface before merge.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #96535
Summary: This PR is a candidate fix for the linked source-reproduced Slack outbound mirror session issue.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🐚 platinum hermit
Result: blocked until real behavior proof from a real setup is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P1] Add redacted real Slack evidence showing replies mirror into the existing thread session after the fix.
  • Redact private details such as workspace names, channel IDs, user IDs, tokens, IPs, phone numbers, and non-public endpoints before posting evidence.
  • Update the PR body after adding proof so ClawSweeper can re-review automatically.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body gives focused Vitest and diff-check output only; contributor should add redacted real Slack proof, then update the PR body to trigger re-review or ask a maintainer to comment @clawsweeper re-review.

Mantis proof suggestion
A real Slack smoke can materially prove the transport/session behavior that unit tests cannot show. A maintainer can ask Mantis to capture proof by posting this exact PR comment:

@openclaw-mantis slack desktop smoke: verify threaded Slack replies mirror into the existing thread session and do not create sibling delivery-mirror sessions.

Risk before merge

  • [P1] Merging changes Slack outbound session routing, and the PR has no real Slack after-fix proof showing mirrored replies stay on the existing thread session rather than creating sibling delivery-mirror sessions.

Maintainer options:

  1. Prove Slack Session Routing Live (recommended)
    Ask for redacted Slack workspace or session-store proof showing threaded outbound replies mirror into the existing session after the patch.
  2. Accept Maintainer Proof Override
    A maintainer can intentionally accept the focused source and unit-test proof if live Slack proof is impractical for this small route fix.

Next step before merge

  • [P1] Contributor or maintainer proof action is needed; there is no narrow code repair for ClawSweeper to make from the current review.

Security
Cleared: No concrete security or supply-chain concern was found; the diff is limited to TypeScript route metadata and focused tests, with no dependency, workflow, lockfile, script, or secret-surface changes.

Review details

Best possible solution:

Merge the narrow Slack-specific route fix after redacted real Slack proof or a maintainer proof override confirms outbound mirror entries stay on the existing thread session.

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

Yes, at source level: current main sends Slack replyToId, threadId, and currentSessionKey into a helper whose default precedence picks replyToId before the recovered thread session. I did not run a live Slack workspace replay.

Is this the best way to solve the issue?

Yes, this looks like the narrow maintainable fix: it leaves the shared SDK helper default unchanged and changes only Slack route precedence based on explicit versus inherited reply targeting. A generic precedence flip would be broader and risk other channels.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 643410c1f3c0.

Label changes

Label changes:

  • remove merge-risk: 🚨 compatibility: Current PR review merge-risk labels are merge-risk: 🚨 session-state.

Label justifications:

  • P2: The PR targets a normal-priority Slack session-routing bug with limited blast radius to Slack outbound delivery-mirror state.
  • merge-risk: 🚨 session-state: The diff changes how Slack chooses outbound mirror session keys, so an incorrect merge could mis-associate Slack thread session state.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body gives focused Vitest and diff-check output only; contributor should add redacted real Slack proof, then update the PR body to trigger re-review or ask a maintainer to comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +7, Tests +45. Total +52 across 6 files.

View PR surface stats
Area Files Added Removed Net
Source 4 9 2 +7
Tests 2 45 0 +45
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 6 54 2 +52

Acceptance criteria:

  • [P1] Redacted real Slack workspace/session-store proof or maintainer proof override.
  • [P1] node scripts/run-vitest.mjs extensions/slack/src/channel.test.ts src/plugin-sdk/thread-aware-outbound-session-route.test.ts src/infra/outbound/message-action-runner.threading.test.ts.
  • [P1] git diff --check.

What I checked:

  • Repository policy read: Root AGENTS.md and scoped extension/channel/outbound guides were read fully; their review guidance requires checking the whole channel/session routing boundary and real behavior proof for user-visible channel changes. (AGENTS.md:1, 643410c1f3c0)
  • Current main route precedence: Current main's shared thread-aware route helper defaults to replyToId, then threadId, then recovered currentSession, which explains how a child Slack reply timestamp can win over the existing thread session. (src/plugin-sdk/core.ts:443, 643410c1f3c0)
  • Slack current route input: Current main's Slack route resolver passes replyToId, threadId, and currentSessionKey into the shared helper without a Slack-specific precedence override. (extensions/slack/src/channel.ts:373, 643410c1f3c0)
  • Explicitness exists before route resolution: Current main already computes replyToIsExplicit for outbound sends and passes it to threading normalization, but prepareOutboundMirrorRoute does not pass it into route resolution before this PR. (src/infra/outbound/message-action-threading.ts:205, 643410c1f3c0)
  • PR patch shape: The PR adds replyToIsExplicit to route params, forwards it through outbound mirror routing, and uses it in Slack to choose current-session-first precedence for inherited replies while keeping explicit replies reply-first. (extensions/slack/src/channel.ts:374, c02103a8bcfe)
  • Focused regression coverage: The PR adds Slack route tests for inherited child reply ids versus explicit reply ids, and extends outbound threading tests to assert the explicitness flag is forwarded. (extensions/slack/src/channel.test.ts:475, c02103a8bcfe)

Likely related people:

  • amknight: Authored the merged Mattermost one-root reply-thread fix that introduced the closest sibling pattern for canonicalizing inherited replies to a thread root before mirroring. (role: adjacent one-root transport contributor; confidence: high; commits: 2365a137d88c; files: src/infra/outbound/message-action-threading.ts, src/infra/outbound/message-action-runner.threading.test.ts, extensions/mattermost/src/channel.ts)
  • sandieman2: Recent merged Slack/reply delivery route work touched Slack channel and outbound routing surfaces related to session-key preservation and threaded delivery. (role: recent Slack reply-routing contributor; confidence: high; commits: c67dc59b02b0; files: extensions/slack/src/channel.ts, src/infra/outbound/message-action-threading.ts, src/channels/plugins/types.core.ts)
  • steipete: GitHub history shows repeated Slack channel, channel SDK, and target-resolution refactors shaping the route and plugin seams involved here. (role: historical channel SDK and Slack contributor; confidence: medium; commits: 1507a9701b83, c4f0da00a9fe, 50c77f23c1c1; files: extensions/slack/src/channel.ts, src/plugin-sdk/core.ts, src/channels/plugins/types.core.ts)
  • vincentkoc: Recent channel and delivery-route commits plus coauthorship on the sibling Mattermost route fix make this a plausible reviewer for routing invariants. (role: adjacent channel/outbound contributor; confidence: medium; commits: 2365a137d88c, 7da955fae4ca, 6cb631afc9c5; files: extensions/slack/src/channel.ts, src/infra/outbound/message-action-threading.ts, src/channels/plugins/types.core.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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 keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. labels Jun 25, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 27, 2026
Merged
@clawsweeper clawsweeper Bot removed the merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. label Jun 27, 2026
@steipete

steipete commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Thanks for the fix. #97168 has now landed the same #96535 repair at the narrower Slack-owned reply-transport boundary. That implementation preserves reply-first behavior for explicit and unknown callers, uses the thread root only for known inherited replies, and avoids expanding the generic outbound session-route contract.

Canonical landed commit: 5e572dc

Proof: 72 focused Slack/outbound tests passed; Blacksmith Testbox tbx_01kwdcpeaf5b93cdw6vyk3jfxe passed the changed gate; exact-head CI passed after rerunning one unrelated process-signal timing flake: https://github.com/openclaw/openclaw/actions/runs/28482208765

Closing this as superseded by the landed fix.

@steipete steipete closed this Jun 30, 2026
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 merge-risk: 🚨 session-state 🚨 May lose, corrupt, stale, or mis-associate session, agent, or context state. P2 Normal backlog priority with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: S status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Slack: each outbound reply persists a new single-message delivery-mirror session instead of reusing the conversation's thread session

2 participants

@harjothkhara @steipete