fix(slack): DM thread attachments stay in thread when sent via message tool#45684
fix(slack): DM thread attachments stay in thread when sent via message tool#45684Taskle wants to merge 2 commits intoopenclaw:mainfrom
Conversation
Greptile SummaryThis PR fixes a two-part bug that caused attachments sent via the Root cause:
Fix:
Key observations:
Confidence Score: 5/5
Last reviewed commit: d164fa5 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d164fa5971
ℹ️ 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".
src/slack/threading-tool-context.ts
Outdated
| : to?.startsWith("user:") | ||
| ? to // e.g. "user:U0AC3LBA08M" — preserved for DM thread matching |
There was a problem hiding this comment.
Do not replace DM currentChannelId with the user:... address unconditionally here, because the message tool infers missing target values from toolContext.currentChannelId, and several Slack actions (react, read, edit, delete, pins) subsequently pass that inferred value through resolveSlackChannelId, which rejects user targets. In a Slack DM thread this change turns previously valid implicit-target calls (using D... NativeChannelId) into runtime errors unless callers always provide an explicit channel ID.
Useful? React with 👍 / 👎.
…ge tool
When an agent is in a Slack DM thread and sends an attachment (image, audio,
file) via the message tool, the attachment was posted outside the thread as a
new top-level message instead of continuing in the existing thread.
Root cause: two gaps in the DM thread context chain:
1. buildSlackThreadingToolContext set currentChannelId = undefined for DMs
because it only extracted the channel ID from 'channel:xxx' prefixed To
values. DM To values use the 'user:xxx' format, so currentChannelId was
always undefined in direct messages.
2. resolveSlackAutoThreadId bailed early if parsedTarget.kind !== 'channel',
which excluded user: targets (kind='user') used for DMs.
Together these meant resolveSlackAutoThreadId always returned undefined in DMs,
so the message tool never injected thread_ts when uploading files — even when
the bot was actively replying in a DM thread.
The auto-reply path (deliverReplies) was unaffected because it uses a separate
replyPlan that correctly applies the isThreadReply override. Only explicit
message tool sends (images, TTS audio, attachments) were broken.
Fix:
- buildSlackThreadingToolContext: preserve 'user:xxx' as currentChannelId for
DM sessions so downstream matching works.
- resolveSlackAutoThreadId: build a canonical target address ('user:xxx' for
DMs, raw channel ID for channels) and compare against currentChannelId,
removing the channel-only kind guard.
Tests: 21 new assertions covering channel threads, DM threads, cross-target
isolation, replyToMode gating, and missing-context guards.
Taskle
force-pushed
the
fix/slack-dm-thread-attachment-injection
branch
from
d164fa5 to
84f0344
Compare
…uards - Patch @pierre/[email protected]: add { with: { type: 'json' } } import attributes to bare JSON dynamic imports in shared_highlighter.js, which throw TypeError on Node 24 (required by extensions/diffs tests) - Guard chmod assertions in json-file.test.ts behind process.platform !== 'win32' (chmodSync is a no-op on Windows; assertions always fail) - Skip Homebrew Cellar path tests on Windows via it.skipIf; the regex uses POSIX forward slashes and Homebrew does not exist on Windows
|
Hey @tyler6204 — would appreciate a review when you get a chance. Bug: Attachments sent via the Root cause: Fix: Two-field approach to avoid a secondary regression:
Tests: 30 passing (13 threading-tool-context, 17 message-action-params). Type-check and Greptile jobs are green. The remaining CI failures (extensions, Windows path tests) are pre-existing on Thanks! |
1 participant
Fixes #38409
Reopened from #38410 (closed due to force-push during conflict resolution).
What changed
Two files, minimal diff. All existing tests pass; 21 new assertions added.
src/slack/threading-tool-context.tsbuildSlackThreadingToolContextwas discardingcurrentChannelIdfor DMs because it only handledchannel:xxx-prefixedTovalues. DM sessions haveTo = "user:U0AC3LBA08M", socurrentChannelIdwas alwaysundefined.Fix: also preserve
user:xxxaddresses ascurrentChannelIdfor DMs.src/infra/outbound/message-action-params.tsresolveSlackAutoThreadIdexited early whencurrentChannelIdwasundefined(which it always was in DMs — see above), and even if that were fixed it rejecteduser:targets withkind !=="channel"`.Fix: build a canonical target address (
"user:<id>"for DMs, raw channel ID for channels) and compare againstcurrentChannelId, removing the channel-only kind guard.Why auto-replies were unaffected
deliverRepliesusesreplyPlan.nextThreadTs()which applies theisThreadReply → "all"override directly, never callingresolveSlackAutoThreadId. Only explicitmessagetool sends (TTS audio, images, file uploads) went through the broken path.Test coverage
src/slack/threading-tool-context.test.tssrc/infra/outbound/message-action-params.test.ts(new)