fix(slack): preserve thread context for Agents & Assistants DM root messages#63840
Conversation
Greptile SummaryThis PR fixes a thread-context loss bug in the Slack Agents & Assistants DM flow. When a DM is opened, Slack sets Confidence Score: 5/5Safe to merge — the fix is narrowly scoped to the thread-root case and is well-covered by the new test. All findings are P2 (a minor redundant ternary arm). The logic is correct, the only affected code path is the !isThreadReply && hasThreadTs case (Agents & Assistants DM roots), existing tests remain green, and a new parameterized test validates the fix across all relevant modes. No files require special attention.
|
| const messageThreadId = isThreadReply | ||
| ? incomingThreadTs | ||
| : params.replyToMode === "all" | ||
| ? messageTs | ||
| : undefined; | ||
| : hasThreadTs | ||
| ? incomingThreadTs | ||
| : params.replyToMode === "all" | ||
| ? messageTs | ||
| : undefined; |
There was a problem hiding this comment.
Since isThreadReply being true implies hasThreadTs is true, the isThreadReply ? incomingThreadTs arm is subsumed by the new hasThreadTs ? incomingThreadTs arm. The two-step ternary can be collapsed to one without changing behavior:
| const messageThreadId = isThreadReply | |
| ? incomingThreadTs | |
| : params.replyToMode === "all" | |
| ? messageTs | |
| : undefined; | |
| : hasThreadTs | |
| ? incomingThreadTs | |
| : params.replyToMode === "all" | |
| ? messageTs | |
| : undefined; | |
| const messageThreadId = hasThreadTs | |
| ? incomingThreadTs | |
| : params.replyToMode === "all" | |
| ? messageTs | |
| : undefined; |
This is a minor clarity suggestion; semantics are identical either way.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/slack/src/threading.ts
Line: 29-35
Comment:
**Simplifiable ternary — first arm is now redundant**
Since `isThreadReply` being `true` implies `hasThreadTs` is `true`, the `isThreadReply ? incomingThreadTs` arm is subsumed by the new `hasThreadTs ? incomingThreadTs` arm. The two-step ternary can be collapsed to one without changing behavior:
```suggestion
const messageThreadId = hasThreadTs
? incomingThreadTs
: params.replyToMode === "all"
? messageTs
: undefined;
```
This is a minor clarity suggestion; semantics are identical either way.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1bdbc97069
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| : hasThreadTs | ||
| ? incomingThreadTs | ||
| : params.replyToMode === "all" |
There was a problem hiding this comment.
Restrict thread-root override to DM assistant threads
This change sets messageThreadId for every event with thread_ts, including auto-created top-level cases where thread_ts === ts and isThreadReply is false. Downstream, buildSlackThreadingToolContext treats any MessageThreadId as an explicit target and forces replyToMode to all (extensions/slack/src/threading-tool-context.ts:19-21), so channel/group conversations configured as off/first/batched can have tool/subagent outputs forced into a thread despite resolveSlackThreadTargets classifying these as non-thread replies. This should be scoped to the intended Agents/Assistants DM-root scenario rather than all thread_ts === ts messages.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1bdbc97069
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| : hasThreadTs | ||
| ? incomingThreadTs |
There was a problem hiding this comment.
Scope thread-root MessageThreadId assignment to DMs only
This branch now sets messageThreadId for any event where thread_ts is present, including top-level thread_ts == ts messages that are intentionally classified as non-thread replies. In channel/group contexts with replyToMode set to off/first/batched, that changes behavior because downstream Slack tool threading treats any MessageThreadId as an explicit thread target and forces threaded follow-ups, so tool/subagent outputs can be posted in a thread even when normal reply mode says not to. This should be gated to the DM Agents/Assistants root-message case (or equivalent chat-type check) rather than all hasThreadTs events.
Useful? React with 👍 / 👎.
1bdbc97 to
108b60c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 108b60c1f1
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| : hasThreadTs | ||
| ? incomingThreadTs |
There was a problem hiding this comment.
Scope thread-root MessageThreadId assignment to DMs
Setting messageThreadId whenever hasThreadTs is true makes non-reply events with thread_ts === ts look like explicit thread targets. Downstream, buildSlackThreadingToolContext treats any MessageThreadId as explicit and forces replyToMode to all, so channel/group turns configured as off/first/batched can have tool/subagent outputs unexpectedly routed into a thread. Fresh evidence: the newly added test in extensions/slack/src/threading.test.ts uses a channel id (C1) while asserting this behavior for non-all modes, confirming the regression now applies outside the DM Assistant-root case.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ca2221a0a
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // buildSlackThreadingToolContext treats any MessageThreadId as an explicit | ||
| // thread target and overrides replyToMode to "all". | ||
| const isDmAssistantThread = | ||
| hasThreadTs && !isThreadReply && params.message.channel_type === "im"; |
There was a problem hiding this comment.
Use normalized DM detection for assistant thread roots
The new isDmAssistantThread branch keys only on params.message.channel_type === "im", but this Slack path already handles events where channel_type is missing or incorrect for D-prefixed DMs (normalized in extensions/slack/src/monitor/message-handler/prepare.ts). In those DM events, root assistant messages with thread_ts === ts still produce messageThreadId: undefined for off/first/batched, so buildSlackThreadingToolContext does not force explicit threading and later tool/subagent sends can still fall back to the channel root. Please derive this condition from normalized channel type (or channel-id inference) instead of the raw event field.
Useful? React with 👍 / 👎.
|
Codex review: needs maintainer review before merge. Workflow note: Future ClawSweeper reviews update this same comment in place. How this review workflow works
Summary Reproducibility: yes. at source level. Current main computes normalized isDirectMessage before routing but does not pass it into resolveSlackThreadContext, so D-prefix DM roots with thread_ts equal to ts and direct reply modes off, first, or batched leave MessageThreadId undefined. PR rating Rank-up moves:
What the crustacean ranks mean
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. Real behavior proof Mantis proof suggestion Risk before merge
Maintainer options:
Next step before merge Security Review detailsBest possible solution: Land this PR, or an equivalent Slack-owned patch, after maintainers accept resolver-level proof or request an optional Slack Desktop transport smoke. Do we have a high-confidence way to reproduce the issue? Yes, at source level. Current main computes normalized isDirectMessage before routing but does not pass it into resolveSlackThreadContext, so D-prefix DM roots with thread_ts equal to ts and direct reply modes off, first, or batched leave MessageThreadId undefined. Is this the best way to solve the issue? Yes. Passing the already-normalized direct-message classification into the existing Slack thread resolver is the narrow maintainable fix, and the latest branch keeps non-DM self-thread roots unset. Label justifications:
What I checked:
Likely related people:
Codex review notes: model gpt-5.5, reasoning high; reviewed against f39f56a096a7. |
|
Codex review: needs changes before merge. What this changes: The PR changes Slack thread-context resolution and tests to set Required change before merge: This is a concrete, narrow PR repair: the desired behavior, affected Slack files, regression boundary, and validation path are clear, and the remaining work is code/test cleanup rather than product or security judgment. Review detailsBest possible solution: Implement the Slack fix using the normalized direct-message classification already available in the message pipeline, preserve Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against acae48b790fa. |
2ca2221 to
37de7dc
Compare
37de7dc to
4824dbd
Compare
4824dbd to
f1ca934
Compare
f1ca934 to
f082ac1
Compare
|
ClawSweeper PR egg ✨ Hatched: 🥚 common Mossy Clawlet Hatch commandComment Hatchability rules:
Rarity: 🥚 common. What is this egg doing here?
|
|
@clawsweeper hatch |
|
🦞👀 I queued a comment sync for this PR. If the egg is hatchable, ClawSweeper will generate the image once and update the existing review comment. |
|
Heads up: this PR needs to be updated against current |
|
@codex rebase and update to main |
|
To use Codex here, create an environment for this repo. |
|
@clawsweeper can you kindly rebase and update to main |
The previous fix unconditionally set messageThreadId when hasThreadTs was true, which affected channel/group messages where thread_ts == ts (auto- created top-level threads). This caused buildSlackThreadingToolContext to force replyToMode to "all" in non-DM contexts, unexpectedly routing tool and subagent outputs into threads. Now the override only applies when channel_type is "im", correctly limiting it to the Slack Agents & Assistants DM root-message scenario. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
f082ac1 to
f8aa242
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8aa24262c
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // where thread_ts == ts. Non-DM self-thread roots must stay unset because | ||
| // downstream tool threading treats MessageThreadId as an explicit thread | ||
| // target and overrides replyToMode to "all". | ||
| const isAssistantDmThreadRoot = hasThreadTs && !isThreadReply && params.isDirectMessage === true; |
There was a problem hiding this comment.
Thread DM root replies with preserved context
When a normalized DM root only has thread_ts == ts (for example the new D-prefixed cases without assistant_thread metadata/cache), this branch now exposes MessageThreadId, so tool/subagent sends are forced into that Slack thread via buildSlackThreadingToolContext. The normal reply path still gets forcedReplyThreadTs only from assistantThreadContext and dispatch.ts recomputes thread targets without the normalized DM flag, so with replyToMode off/first/batched the agent’s final reply can remain unthreaded while tool output goes into the thread. Please carry the same preserved DM-root thread target into the reply delivery plan as well.
Useful? React with 👍 / 👎.
|
Landing proof for the rebased head
Thanks @zozo123. |
steipete
left a comment
There was a problem hiding this comment.
Reviewed the rebased Slack routing fix and focused regression coverage. Local focused tests and current CI are green. Approved for landing.
|
Landed via squash merge.
Thanks @zozo123. |
Preserve Slack Agents & Assistants DM root thread context for tool and subagent replies even when Slack omits or misreports `channel_type`, while leaving non-DM self-thread roots top-level. Fixes openclaw#63659. Thanks @zozo123.
Preserve Slack Agents & Assistants DM root thread context for tool and subagent replies even when Slack omits or misreports `channel_type`, while leaving non-DM self-thread roots top-level. Fixes openclaw#63659. Thanks @zozo123.
Preserve Slack Agents & Assistants DM root thread context for tool and subagent replies even when Slack omits or misreports `channel_type`, while leaving non-DM self-thread roots top-level. Fixes openclaw#63659. Thanks @zozo123.
Summary
Preserves thread context for Agents & Assistants DM root messages in Slack integration.
Closes #63659
Testing
Real behavior proof
thread_ts == tsnow preserveMessageThreadIdwhen the message pipeline has normalized the event as a direct message, including cases where Slack omits or misreports rawchannel_type. Non-DM self-thread roots remain top-level.37de7dc8d6, Node25.2.1, pnpm10.33.2, running the production Slack routing resolver from this branch.pnpm exec tsx -e '<script invoking resolveSlackRoutingContext for D-prefix DM roots and a channel self-thread root>'in the PR worktree.{"case":"dm-root-wrong-channel-type","sessionKey":"agent:main:slack:direct:u3","messageThreadId":"1770408530.000000","replyToId":"1770408530.000000"} {"case":"dm-root-missing-channel-type","sessionKey":"agent:main:slack:direct:u3","messageThreadId":"1770408531.000000","replyToId":"1770408531.000000"} {"case":"channel-self-thread-root","sessionKey":"agent:main:slack:channel:c123","messageThreadId":null,"replyToId":"1770408532.000000"}MessageThreadId; the channel self-thread root kept channel-session routing and did not setMessageThreadId.pnpm check:changedalso passed after the smoke.