From e40d7abda9ff42d4fb421ebcd52ceb2b5c7a623b Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sat, 25 Apr 2026 00:38:01 +0100 Subject: [PATCH] fix(slack): preserve real thread anchors --- CHANGELOG.md | 1 + extensions/slack/src/channel.test.ts | 121 ++++++++++++++++++ extensions/slack/src/channel.ts | 25 ++-- extensions/slack/src/monitor.test-helpers.ts | 3 + extensions/slack/src/outbound-adapter.test.ts | 58 +++++++++ extensions/slack/src/outbound-adapter.ts | 7 +- extensions/slack/src/thread-ts.test.ts | 34 +++++ extensions/slack/src/thread-ts.ts | 23 ++++ .../slack/src/threading-tool-context.ts | 3 +- src/auto-reply/reply/route-reply.test.ts | 25 +++- 10 files changed, 287 insertions(+), 13 deletions(-) create mode 100644 extensions/slack/src/thread-ts.test.ts create mode 100644 extensions/slack/src/thread-ts.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 634e0c5755f..8b9d2780d5c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ Docs: https://docs.openclaw.ai - Slack/native streaming: suppress reasoning-only payloads before `chat.startStream`/`appendStream`, so Claude extended-thinking blocks no longer appear as visible Slack messages. Fixes #59687. Thanks @vision-ifc. - Slack/block replies: keep multi-part block deliveries in the first Slack reply thread when `replyToMode` is `first`, matching text reply threading instead of leaking later blocks into the channel. Fixes #49341. Thanks @pholmstr and @xiwuqi. - Slack/thread broadcasts: process `thread_broadcast` events as user messages so replies sent with "Also send to channel" reach the agent instead of becoming metadata-only system events. Fixes #56605 and #4351. Thanks @clawSean and @jlowin. +- Slack/threading: ignore internal reply ids when choosing Slack `thread_ts` values, so resumed replies keep the real Slack thread anchor instead of leaking to the channel root. Fixes #68790. Thanks @MonkeyLeeT and @martingarramon. - Agents/failover: stop body-less HTTP 400/422 proxy failures from defaulting to `"format"` classification, so embedded retries surface the opaque provider failure instead of falling into a compaction loop. Fixes #66462. (#67024) Thanks @altaywtf and @HongzhuLiu. - Plugins/loader: use cached discovery-mode snapshot loads for read-only plugin capability lookups, keep snapshot caches isolated from active Gateway registries, and make same-plugin channel/HTTP route re-registration idempotent so repeated snapshot or hot-reload paths no longer rerun full plugin side effects or accumulate duplicate surfaces. Fixes #51781, #52031, #54181, and #57514. Thanks @livingghost, @okuyam2y, @ShionEria, and @bbshih. - Plugins/loader: reuse the compatible active Gateway registry for broad runtime plugin ensure calls after a gateway-bindable boot load, so non-bundled plugins no longer re-run `register()` during the same boot path. Fixes #69250. Thanks @markthebest12. diff --git a/extensions/slack/src/channel.test.ts b/extensions/slack/src/channel.test.ts index b79d92319e2..0853796a705 100644 --- a/extensions/slack/src/channel.test.ts +++ b/extensions/slack/src/channel.test.ts @@ -456,6 +456,127 @@ describe("slackPlugin outbound", () => { expect(result).toEqual({ channel: "slack", messageId: "m-media" }); }); + it("falls back to threadId when replyToId is not a Slack thread timestamp", async () => { + const sendSlack = vi.fn().mockResolvedValue({ messageId: "m-text" }); + const sendText = requireSlackSendText(); + + const result = await sendText({ + cfg, + to: "C123", + text: "hello", + accountId: "default", + replyToId: "msg-internal-1", + threadId: "1712345678.123456", + deps: { sendSlack }, + }); + + expect(sendSlack).toHaveBeenCalledWith( + "C123", + "hello", + expect.objectContaining({ + threadTs: "1712345678.123456", + }), + ); + expect(result).toEqual({ channel: "slack", messageId: "m-text" }); + }); + + it("does not stringify numeric Slack thread ids", async () => { + const sendSlack = vi.fn().mockResolvedValue({ messageId: "m-text" }); + const sendText = requireSlackSendText(); + + await sendText({ + cfg, + to: "C123", + text: "hello", + accountId: "default", + threadId: 1712345678.123456, + deps: { sendSlack }, + }); + + expect(sendSlack).toHaveBeenCalledWith( + "C123", + "hello", + expect.objectContaining({ + threadTs: undefined, + }), + ); + }); + + it("falls back to auto-thread lookup when replyToId is not a Slack thread timestamp", () => { + const resolveAutoThreadId = slackPlugin.threading?.resolveAutoThreadId; + if (!resolveAutoThreadId) { + throw new Error("slack threading.resolveAutoThreadId unavailable"); + } + + const threadId = resolveAutoThreadId({ + cfg, + to: "channel:C123", + replyToId: "msg-internal-1", + toolContext: { + currentChannelId: "C123", + currentThreadTs: "1712345678.123456", + replyToMode: "all", + }, + }); + + expect(threadId).toBe("1712345678.123456"); + }); + + it("does not recover invalid Slack auto-thread anchors", () => { + const resolveAutoThreadId = slackPlugin.threading?.resolveAutoThreadId; + if (!resolveAutoThreadId) { + throw new Error("slack threading.resolveAutoThreadId unavailable"); + } + + const threadId = resolveAutoThreadId({ + cfg, + to: "channel:C123", + replyToId: "msg-internal-1", + toolContext: { + currentChannelId: "C123", + currentThreadTs: "thread-root", + replyToMode: "all", + }, + }); + + expect(threadId).toBeUndefined(); + }); + + it("does not stringify numeric thread ids in tool context", () => { + const buildToolContext = slackPlugin.threading?.buildToolContext; + if (!buildToolContext) { + throw new Error("slack threading.buildToolContext unavailable"); + } + + const context = buildToolContext({ + cfg, + context: { + To: "channel:C123", + MessageThreadId: 1712345678.123456, + }, + }); + + expect(context?.currentThreadTs).toBeUndefined(); + }); + + it("falls back to threadId in reply transport when replyToId is not a Slack thread timestamp", () => { + const resolveReplyTransport = slackPlugin.threading?.resolveReplyTransport; + if (!resolveReplyTransport) { + throw new Error("slack threading.resolveReplyTransport unavailable"); + } + + expect( + resolveReplyTransport({ + cfg, + replyToId: "msg-internal-1", + threadId: "1712345678.123456", + }), + ).toEqual({ + replyToId: "1712345678.123456", + threadId: null, + }); + }); + it("forwards mediaLocalRoots for sendMedia", async () => { const sendSlack = vi.fn().mockResolvedValue({ messageId: "m-media-local" }); const sendMedia = requireSlackSendMedia(); diff --git a/extensions/slack/src/channel.ts b/extensions/slack/src/channel.ts index 8028c84d3af..382a3b601e7 100644 --- a/extensions/slack/src/channel.ts +++ b/extensions/slack/src/channel.ts @@ -61,6 +61,7 @@ import { slackConfigAdapter, } from "./shared.js"; import { parseSlackTarget } from "./target-parsing.js"; +import { normalizeSlackThreadTsCandidate, resolveSlackThreadTsValue } from "./thread-ts.js"; import { buildSlackThreadingToolContext } from "./threading-tool-context.js"; // Lazy SDK loaders. The dynamic import is hidden behind a string-literal @@ -218,7 +219,7 @@ async function resolveSlackSendContext(params: { const token = getTokenForOperation(account, "write"); const botToken = account.botToken?.trim(); const tokenOverride = token && token !== botToken ? token : undefined; - const threadTsValue = params.replyToId ?? params.threadId; + const threadTsValue = resolveSlackThreadTsValue(params); return { send, threadTsValue, tokenOverride }; } @@ -608,14 +609,16 @@ export const slackPlugin: ChannelPlugin = crea allowExplicitReplyTagsWhenOff: false, buildToolContext: (params) => buildSlackThreadingToolContext(params), resolveAutoThreadId: ({ to, toolContext, replyToId }) => - replyToId + normalizeSlackThreadTsCandidate(replyToId) ? undefined - : resolveSlackAutoThreadId({ - to, - toolContext, - }), + : normalizeSlackThreadTsCandidate( + resolveSlackAutoThreadId({ + to, + toolContext, + }), + ), resolveReplyTransport: ({ threadId, replyToId }) => ({ - replyToId: replyToId ?? (threadId != null && threadId !== "" ? String(threadId) : undefined), + replyToId: resolveSlackThreadTsValue({ replyToId, threadId }), threadId: null, }), }, @@ -632,7 +635,7 @@ export const slackPlugin: ChannelPlugin = crea payload, }), sendPayload: async (ctx) => { - const { send, tokenOverride } = await resolveSlackSendContext({ + const { send, threadTsValue, tokenOverride } = await resolveSlackSendContext({ cfg: ctx.cfg, accountId: ctx.accountId ?? undefined, deps: ctx.deps, @@ -642,6 +645,8 @@ export const slackPlugin: ChannelPlugin = crea const { slackOutbound } = await loadSlackOutboundAdapterModule(); return await slackOutbound.sendPayload!({ ...ctx, + replyToId: threadTsValue, + threadId: null, deps: { ...ctx.deps, slack: async ( @@ -669,7 +674,7 @@ export const slackPlugin: ChannelPlugin = crea }); return await send(to, text, { cfg, - threadTs: threadTsValue != null ? String(threadTsValue) : undefined, + threadTs: threadTsValue, accountId: accountId ?? undefined, ...(tokenOverride ? { token: tokenOverride } : {}), }); @@ -696,7 +701,7 @@ export const slackPlugin: ChannelPlugin = crea cfg, mediaUrl, mediaLocalRoots, - threadTs: threadTsValue != null ? String(threadTsValue) : undefined, + threadTs: threadTsValue, accountId: accountId ?? undefined, ...(tokenOverride ? { token: tokenOverride } : {}), }); diff --git a/extensions/slack/src/monitor.test-helpers.ts b/extensions/slack/src/monitor.test-helpers.ts index 3825d70f7e6..0016aac6641 100644 --- a/extensions/slack/src/monitor.test-helpers.ts +++ b/extensions/slack/src/monitor.test-helpers.ts @@ -265,6 +265,9 @@ vi.mock("@slack/bolt", () => { command() { /* no-op */ } + use() { + /* no-op */ + } start = vi.fn().mockResolvedValue(undefined); stop = vi.fn().mockResolvedValue(undefined); } diff --git a/extensions/slack/src/outbound-adapter.test.ts b/extensions/slack/src/outbound-adapter.test.ts index 74b94a3019b..cdd2eaf0e03 100644 --- a/extensions/slack/src/outbound-adapter.test.ts +++ b/extensions/slack/src/outbound-adapter.test.ts @@ -115,4 +115,62 @@ describe("slackOutbound", () => { ); expect(result).toEqual({ channel: "slack", messageId: "m-blocks" }); }); + + it("falls back to threadId when payload replyToId is not a Slack thread timestamp", async () => { + sendMessageSlackMock.mockResolvedValueOnce({ messageId: "m-blocks" }); + + await slackOutbound.sendPayload!({ + cfg, + to: "C123", + text: "", + replyToId: "msg-internal-1", + threadId: "1712345678.123456", + payload: { + text: "fallback text", + channelData: { + slack: { + blocks: [{ type: "divider" }], + }, + }, + }, + accountId: "default", + }); + + expect(sendMessageSlackMock).toHaveBeenCalledWith( + "C123", + "fallback text", + expect.objectContaining({ + threadTs: "1712345678.123456", + }), + ); + }); + + it("does not thread payloads without a valid Slack thread timestamp", async () => { + sendMessageSlackMock.mockResolvedValueOnce({ messageId: "m-blocks" }); + + await slackOutbound.sendPayload!({ + cfg, + to: "C123", + text: "", + replyToId: "msg-internal-1", + threadId: "thread-root", + payload: { + text: "fallback text", + channelData: { + slack: { + blocks: [{ type: "divider" }], + }, + }, + }, + accountId: "default", + }); + + expect(sendMessageSlackMock).toHaveBeenCalledWith( + "C123", + "fallback text", + expect.objectContaining({ + threadTs: undefined, + }), + ); + }); }); diff --git a/extensions/slack/src/outbound-adapter.ts b/extensions/slack/src/outbound-adapter.ts index b51773306b4..d002b60329f 100644 --- a/extensions/slack/src/outbound-adapter.ts +++ b/extensions/slack/src/outbound-adapter.ts @@ -27,6 +27,7 @@ import { import { compileSlackInteractiveReplies } from "./interactive-replies.js"; import { SLACK_TEXT_LIMIT } from "./limits.js"; import type { SlackSendIdentity } from "./send.js"; +import { resolveSlackThreadTsValue } from "./thread-ts.js"; const SLACK_MAX_BLOCKS = 50; type SlackSendFn = typeof import("./send.runtime.js").sendMessageSlack; @@ -84,9 +85,13 @@ async function sendSlackOutboundMessage(params: { resolveOutboundSendDep(params.deps, "slack") ?? (await loadSlackSendRuntime()).sendMessageSlack; const slackIdentity = resolveSlackSendIdentity(params.identity); + const threadTs = resolveSlackThreadTsValue({ + replyToId: params.replyToId, + threadId: params.threadId, + }); const result = await send(params.to, params.text, { cfg: params.cfg, - threadTs: params.replyToId ?? (params.threadId != null ? String(params.threadId) : undefined), + threadTs, accountId: params.accountId ?? undefined, ...(params.mediaUrl ? { diff --git a/extensions/slack/src/thread-ts.test.ts b/extensions/slack/src/thread-ts.test.ts new file mode 100644 index 00000000000..27b32595b86 --- /dev/null +++ b/extensions/slack/src/thread-ts.test.ts @@ -0,0 +1,34 @@ +import { describe, expect, it } from "vitest"; +import { normalizeSlackThreadTsCandidate, resolveSlackThreadTsValue } from "./thread-ts.js"; + +describe("Slack thread_ts resolution", () => { + it("accepts trimmed Slack timestamp strings", () => { + expect(normalizeSlackThreadTsCandidate(" 1712345678.123456 ")).toBe("1712345678.123456"); + }); + + it("rejects internal reply ids", () => { + expect(normalizeSlackThreadTsCandidate("msg-internal-1")).toBeUndefined(); + }); + + it("rejects numeric thread ids instead of stringifying them", () => { + expect(normalizeSlackThreadTsCandidate(1712345678.123456)).toBeUndefined(); + }); + + it("falls back from invalid replyToId to valid threadId", () => { + expect( + resolveSlackThreadTsValue({ + replyToId: "msg-internal-1", + threadId: "1712345678.123456", + }), + ).toBe("1712345678.123456"); + }); + + it("validates fallback threadId before using it", () => { + expect( + resolveSlackThreadTsValue({ + replyToId: "msg-internal-1", + threadId: "thread-root", + }), + ).toBeUndefined(); + }); +}); diff --git a/extensions/slack/src/thread-ts.ts b/extensions/slack/src/thread-ts.ts new file mode 100644 index 00000000000..3fa586fedac --- /dev/null +++ b/extensions/slack/src/thread-ts.ts @@ -0,0 +1,23 @@ +import { normalizeOptionalString } from "openclaw/plugin-sdk/text-runtime"; + +const SLACK_THREAD_TS_PATTERN = /^\d+\.\d+$/; + +export function normalizeSlackThreadTsCandidate( + value?: string | number | null, +): string | undefined { + if (typeof value !== "string") { + return undefined; + } + const normalized = normalizeOptionalString(value); + return normalized && SLACK_THREAD_TS_PATTERN.test(normalized) ? normalized : undefined; +} + +export function resolveSlackThreadTsValue(params: { + replyToId?: string | number | null; + threadId?: string | number | null; +}): string | undefined { + return ( + normalizeSlackThreadTsCandidate(params.replyToId) ?? + normalizeSlackThreadTsCandidate(params.threadId) + ); +} diff --git a/extensions/slack/src/threading-tool-context.ts b/extensions/slack/src/threading-tool-context.ts index a10f5604b64..8d8f6fbf630 100644 --- a/extensions/slack/src/threading-tool-context.ts +++ b/extensions/slack/src/threading-tool-context.ts @@ -5,6 +5,7 @@ import type { import type { OpenClawConfig } from "openclaw/plugin-sdk/config-runtime"; import { normalizeOptionalString } from "openclaw/plugin-sdk/text-runtime"; import { resolveSlackAccount, resolveSlackReplyToMode } from "./accounts.js"; +import { normalizeSlackThreadTsCandidate } from "./thread-ts.js"; export function buildSlackThreadingToolContext(params: { cfg: OpenClawConfig; @@ -28,7 +29,7 @@ export function buildSlackThreadingToolContext(params: { : normalizeOptionalString(params.context.NativeChannelId); return { currentChannelId, - currentThreadTs: threadId != null ? String(threadId) : undefined, + currentThreadTs: normalizeSlackThreadTsCandidate(threadId), replyToMode: effectiveReplyToMode, hasRepliedRef: params.hasRepliedRef, }; diff --git a/src/auto-reply/reply/route-reply.test.ts b/src/auto-reply/reply/route-reply.test.ts index c084130a64b..444bfc8671f 100644 --- a/src/auto-reply/reply/route-reply.test.ts +++ b/src/auto-reply/reply/route-reply.test.ts @@ -61,11 +61,19 @@ const slackMessaging: ChannelMessagingAdapter = { const slackThreading: ChannelThreadingAdapter = { resolveReplyTransport: ({ threadId, replyToId }) => ({ - replyToId: replyToId ?? (threadId != null && threadId !== "" ? String(threadId) : undefined), + replyToId: resolveSlackThreadTsCandidate(replyToId) ?? resolveSlackThreadTsCandidate(threadId), threadId: null, }), }; +function resolveSlackThreadTsCandidate(value?: string | number | null): string | undefined { + if (typeof value !== "string") { + return undefined; + } + const normalized = value.trim(); + return /^\d+\.\d+$/.test(normalized) ? normalized : undefined; +} + const mattermostThreading: ChannelThreadingAdapter = { resolveReplyTransport: ({ threadId, replyToId }) => ({ replyToId: replyToId ?? (threadId != null && threadId !== "" ? String(threadId) : undefined), @@ -489,6 +497,21 @@ describe("routeReply", () => { }); }); + it("uses Slack threadId when routed replyToId is an internal message id", async () => { + await routeReply({ + payload: { text: "hi", replyToId: "msg-internal-1" }, + channel: "slack", + to: "channel:C123", + threadId: "1710000000.9999", + cfg: {} as never, + }); + expectLastDelivery({ + channel: "slack", + replyToId: "1710000000.9999", + threadId: null, + }); + }); + it("uses threadId as replyToId for Mattermost when replyToId is missing", async () => { await routeReply({ payload: { text: "hi" },