From 84046cbad81828aedab5333bcab3e8c97560dbf7 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Fri, 9 Jan 2026 22:09:02 +0100 Subject: [PATCH] fix(slack): mrkdwn + thread edge cases (#464) (thanks @austinm911) --- CHANGELOG.md | 1 + src/agents/tools/slack-actions.test.ts | 46 +++++++++++ src/agents/tools/slack-actions.ts | 13 ++++ src/auto-reply/reply/agent-runner.ts | 11 ++- src/slack/format.test.ts | 16 +++- src/slack/format.ts | 104 +++++++++++++++++++++---- src/slack/monitor.test.ts | 47 +++++++---- src/slack/monitor.ts | 33 ++++---- 8 files changed, 222 insertions(+), 49 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7b05a71cf..e1e487355 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ - Docs: link Hetzner guide from install + platforms docs. (#592) — thanks @steipete - Providers: add Microsoft Teams provider with polling, attachments, and CLI send support. (#404) — thanks @onutc - Slack: honor reply tags + replyToMode while keeping threaded replies in-thread. (#574) — thanks @bolismauro +- Slack: configurable reply threading (`slack.replyToMode`) + proper mrkdwn formatting for outbound messages. (#464) — thanks @austinm911 - Discord: avoid category parent overrides for channel allowlists and refactor thread context helpers. (#588) — thanks @steipete - Discord: fix forum thread starters and cache channel lookups for thread context. (#585) — thanks @thewilloftheshadow - Discord: log gateway disconnect/reconnect events at info and add verbose gateway metrics. (#595) — thanks @steipete diff --git a/src/agents/tools/slack-actions.test.ts b/src/agents/tools/slack-actions.test.ts index 034470ae1..e2e85c7b1 100644 --- a/src/agents/tools/slack-actions.test.ts +++ b/src/agents/tools/slack-actions.test.ts @@ -197,6 +197,52 @@ describe("handleSlackAction", () => { ); }); + it("replyToMode=first marks hasRepliedRef even when threadTs is explicit", async () => { + const cfg = { slack: { botToken: "tok" } } as ClawdbotConfig; + sendSlackMessage.mockClear(); + const hasRepliedRef = { value: false }; + const context = { + currentChannelId: "C123", + currentThreadTs: "1111111111.111111", + replyToMode: "first" as const, + hasRepliedRef, + }; + + await handleSlackAction( + { + action: "sendMessage", + to: "channel:C123", + content: "Explicit", + threadTs: "2222222222.222222", + }, + cfg, + context, + ); + expect(sendSlackMessage).toHaveBeenLastCalledWith( + "channel:C123", + "Explicit", + { + mediaUrl: undefined, + threadTs: "2222222222.222222", + }, + ); + expect(hasRepliedRef.value).toBe(true); + + await handleSlackAction( + { action: "sendMessage", to: "channel:C123", content: "Second" }, + cfg, + context, + ); + expect(sendSlackMessage).toHaveBeenLastCalledWith( + "channel:C123", + "Second", + { + mediaUrl: undefined, + threadTs: undefined, + }, + ); + }); + it("replyToMode=first without hasRepliedRef does not thread", async () => { const cfg = { slack: { botToken: "tok" } } as ClawdbotConfig; sendSlackMessage.mockClear(); diff --git a/src/agents/tools/slack-actions.ts b/src/agents/tools/slack-actions.ts index f54e15b34..863e921d6 100644 --- a/src/agents/tools/slack-actions.ts +++ b/src/agents/tools/slack-actions.ts @@ -152,6 +152,19 @@ export async function handleSlackAction( mediaUrl: mediaUrl ?? undefined, threadTs: threadTs ?? undefined, }); + + // Keep "first" mode consistent even when the agent explicitly provided + // threadTs: once we send a message to the current channel, consider the + // first reply "used" so later tool calls don't auto-thread again. + if (context?.hasRepliedRef && context.currentChannelId) { + const normalizedTarget = to.startsWith("channel:") + ? to.slice("channel:".length) + : to; + if (normalizedTarget === context.currentChannelId) { + context.hasRepliedRef.value = true; + } + } + return jsonResult({ ok: true, result }); } case "editMessage": { diff --git a/src/auto-reply/reply/agent-runner.ts b/src/auto-reply/reply/agent-runner.ts index 345f09082..03ab41f75 100644 --- a/src/auto-reply/reply/agent-runner.ts +++ b/src/auto-reply/reply/agent-runner.ts @@ -86,13 +86,22 @@ function buildSlackThreadingContext(params: { hasRepliedRef: undefined, }; } + + // If we're already inside a thread, never jump replies out of it (even in + // replyToMode="off"/"first"). This keeps tool calls consistent with the + // auto-reply path. + const configuredReplyToMode = config?.slack?.replyToMode ?? "off"; + const effectiveReplyToMode = sessionCtx.ThreadLabel + ? ("all" as const) + : configuredReplyToMode; + return { // Extract channel from "channel:C123" format currentChannelId: sessionCtx.To?.startsWith("channel:") ? sessionCtx.To.slice("channel:".length) : undefined, currentThreadTs: sessionCtx.ReplyToId, - replyToMode: config?.slack?.replyToMode ?? "off", + replyToMode: effectiveReplyToMode, hasRepliedRef, }; } diff --git a/src/slack/format.test.ts b/src/slack/format.test.ts index 05f19dd0a..6a5f7fdde 100644 --- a/src/slack/format.test.ts +++ b/src/slack/format.test.ts @@ -38,11 +38,23 @@ describe("markdownToSlackMrkdwn", () => { expect(res).toBe("see docs (https://example.com)"); }); + it("does not duplicate bare URLs", () => { + const res = markdownToSlackMrkdwn("see https://example.com"); + expect(res).toBe("see https://example.com"); + }); + it("escapes unsafe characters", () => { const res = markdownToSlackMrkdwn("a & b < c > d"); expect(res).toBe("a & b < c > d"); }); + it("preserves Slack angle-bracket markup (mentions/links)", () => { + const res = markdownToSlackMrkdwn( + "hi <@U123> see and ", + ); + expect(res).toBe("hi <@U123> see and "); + }); + it("escapes raw HTML", () => { const res = markdownToSlackMrkdwn("nope"); expect(res).toBe("<b>nope</b>"); @@ -68,9 +80,9 @@ describe("markdownToSlackMrkdwn", () => { expect(res).toBe("*Title*"); }); - it("renders blockquotes with escaped angle bracket", () => { + it("renders blockquotes", () => { const res = markdownToSlackMrkdwn("> Quote"); - expect(res).toBe("> Quote"); + expect(res).toBe("> Quote"); }); it("handles adjacent list items", () => { diff --git a/src/slack/format.ts b/src/slack/format.ts index 75281858d..3d43a50d6 100644 --- a/src/slack/format.ts +++ b/src/slack/format.ts @@ -12,7 +12,9 @@ type RenderEnv = { const md = new MarkdownIt({ html: false, - linkify: true, + // Slack will auto-link plain URLs; keeping linkify off avoids double-rendering + // (e.g. "https://x.com" becoming "https://x.com (https://x.com)"). + linkify: false, breaks: false, typographer: false, }); @@ -21,15 +23,62 @@ md.enable("strikethrough"); /** * Escape special characters for Slack mrkdwn format. - * Slack requires escaping &, <, > to prevent injection. + * + * By default, Slack uses angle-bracket markup for mentions and links + * (e.g. "<@U123>", ""). We preserve those tokens so agents + * can intentionally include them, while escaping other uses of "<" and ">". */ -function escapeSlackMrkdwn(text: string): string { +function escapeSlackMrkdwnSegment(text: string): string { return text .replace(/&/g, "&") .replace(//g, ">"); } +const SLACK_ANGLE_TOKEN_RE = /<[^>\n]+>/g; + +function isAllowedSlackAngleToken(token: string): boolean { + if (!token.startsWith("<") || !token.endsWith(">")) return false; + const inner = token.slice(1, -1); + return ( + inner.startsWith("@") || + inner.startsWith("#") || + inner.startsWith("!") || + inner.startsWith("mailto:") || + inner.startsWith("tel:") || + inner.startsWith("http://") || + inner.startsWith("https://") || + inner.startsWith("slack://") + ); +} + +function escapeSlackMrkdwnText(text: string): string { + if (!text.includes("&") && !text.includes("<") && !text.includes(">")) { + return text; + } + + SLACK_ANGLE_TOKEN_RE.lastIndex = 0; + const out: string[] = []; + let lastIndex = 0; + + for ( + let match = SLACK_ANGLE_TOKEN_RE.exec(text); + match; + match = SLACK_ANGLE_TOKEN_RE.exec(text) + ) { + const matchIndex = match.index ?? 0; + out.push(escapeSlackMrkdwnSegment(text.slice(lastIndex, matchIndex))); + const token = match[0] ?? ""; + out.push( + isAllowedSlackAngleToken(token) ? token : escapeSlackMrkdwnSegment(token), + ); + lastIndex = matchIndex + token.length; + } + + out.push(escapeSlackMrkdwnSegment(text.slice(lastIndex))); + return out.join(""); +} + function getListStack(env: RenderEnv): ListState[] { if (!env.slackListStack) env.slackListStack = []; return env.slackListStack; @@ -41,7 +90,7 @@ function getLinkStack(env: RenderEnv): { href: string }[] { } md.renderer.rules.text = (tokens, idx) => - escapeSlackMrkdwn(tokens[idx]?.content ?? ""); + escapeSlackMrkdwnText(tokens[idx]?.content ?? ""); md.renderer.rules.softbreak = () => "\n"; md.renderer.rules.hardbreak = () => "\n"; @@ -55,7 +104,7 @@ md.renderer.rules.paragraph_close = (_tokens, _idx, _opts, env) => { md.renderer.rules.heading_open = () => "*"; md.renderer.rules.heading_close = () => "*\n\n"; -md.renderer.rules.blockquote_open = () => "> "; +md.renderer.rules.blockquote_open = () => "> "; md.renderer.rules.blockquote_close = () => "\n"; md.renderer.rules.bullet_list_open = (_tokens, _idx, _opts, env) => { @@ -99,15 +148,14 @@ md.renderer.rules.s_open = () => "~"; md.renderer.rules.s_close = () => "~"; md.renderer.rules.code_inline = (tokens, idx) => - `\`${escapeSlackMrkdwn(tokens[idx]?.content ?? "")}\``; + `\`${escapeSlackMrkdwnSegment(tokens[idx]?.content ?? "")}\``; md.renderer.rules.code_block = (tokens, idx) => - `\`\`\`\n${escapeSlackMrkdwn(tokens[idx]?.content ?? "")}\`\`\`\n`; + `\`\`\`\n${escapeSlackMrkdwnSegment(tokens[idx]?.content ?? "")}\`\`\`\n`; md.renderer.rules.fence = (tokens, idx) => - `\`\`\`\n${escapeSlackMrkdwn(tokens[idx]?.content ?? "")}\`\`\`\n`; + `\`\`\`\n${escapeSlackMrkdwnSegment(tokens[idx]?.content ?? "")}\`\`\`\n`; -// Slack links use format md.renderer.rules.link_open = (tokens, idx, _opts, env) => { const href = tokens[idx]?.attrGet("href") ?? ""; const stack = getLinkStack(env as RenderEnv); @@ -118,20 +166,20 @@ md.renderer.rules.link_close = (_tokens, _idx, _opts, env) => { const stack = getLinkStack(env as RenderEnv); const link = stack.pop(); if (link?.href) { - return ` (${escapeSlackMrkdwn(link.href)})`; + return ` (${escapeSlackMrkdwnSegment(link.href)})`; } return ""; }; md.renderer.rules.image = (tokens, idx) => { const alt = tokens[idx]?.content ?? ""; - return escapeSlackMrkdwn(alt); + return escapeSlackMrkdwnSegment(alt); }; md.renderer.rules.html_block = (tokens, idx) => - escapeSlackMrkdwn(tokens[idx]?.content ?? ""); + escapeSlackMrkdwnSegment(tokens[idx]?.content ?? ""); md.renderer.rules.html_inline = (tokens, idx) => - escapeSlackMrkdwn(tokens[idx]?.content ?? ""); + escapeSlackMrkdwnSegment(tokens[idx]?.content ?? ""); md.renderer.rules.table_open = () => ""; md.renderer.rules.table_close = () => ""; @@ -148,6 +196,30 @@ md.renderer.rules.td_close = () => "\t"; md.renderer.rules.hr = () => "\n"; +function protectSlackAngleLinks(markdown: string): { + markdown: string; + tokens: string[]; +} { + const tokens: string[] = []; + const protectedMarkdown = (markdown ?? "").replace( + /<(?:https?:\/\/|mailto:|tel:|slack:\/\/)[^>\n]+>/g, + (match) => { + const id = tokens.length; + tokens.push(match); + return `⟦clawdbot-slacktok:${id}⟧`; + }, + ); + return { markdown: protectedMarkdown, tokens }; +} + +function restoreSlackAngleLinks(text: string, tokens: string[]): string { + let out = text; + for (let i = 0; i < tokens.length; i++) { + out = out.replaceAll(`⟦clawdbot-slacktok:${i}⟧`, tokens[i] ?? ""); + } + return out; +} + /** * Convert standard Markdown to Slack mrkdwn format. * @@ -161,10 +233,12 @@ md.renderer.rules.hr = () => "\n"; */ export function markdownToSlackMrkdwn(markdown: string): string { const env: RenderEnv = {}; - const rendered = md.render(markdown ?? "", env); - return rendered + const protectedLinks = protectSlackAngleLinks(markdown ?? ""); + const rendered = md.render(protectedLinks.markdown, env); + const normalized = rendered .replace(/[ \t]+\n/g, "\n") .replace(/\t+\n/g, "\n") .replace(/\n{3,}/g, "\n\n") .trimEnd(); + return restoreSlackAngleLinks(normalized, protectedLinks.tokens); } diff --git a/src/slack/monitor.test.ts b/src/slack/monitor.test.ts index 73a6ac873..0a2b4dba6 100644 --- a/src/slack/monitor.test.ts +++ b/src/slack/monitor.test.ts @@ -56,23 +56,26 @@ describe("slack groupPolicy gating", () => { describe("resolveSlackThreadTs", () => { const threadTs = "1234567890.123456"; + const messageTs = "9999999999.999999"; describe("replyToMode=off", () => { - it("returns baseThreadTs when in a thread", () => { + it("returns incomingThreadTs when in a thread", () => { expect( resolveSlackThreadTs({ replyToMode: "off", - baseThreadTs: threadTs, + incomingThreadTs: threadTs, + messageTs, hasReplied: false, }), ).toBe(threadTs); }); - it("returns baseThreadTs even after replies (stays in thread)", () => { + it("returns incomingThreadTs even after replies (stays in thread)", () => { expect( resolveSlackThreadTs({ replyToMode: "off", - baseThreadTs: threadTs, + incomingThreadTs: threadTs, + messageTs, hasReplied: true, }), ).toBe(threadTs); @@ -82,7 +85,8 @@ describe("resolveSlackThreadTs", () => { expect( resolveSlackThreadTs({ replyToMode: "off", - baseThreadTs: undefined, + incomingThreadTs: undefined, + messageTs, hasReplied: false, }), ).toBeUndefined(); @@ -90,21 +94,34 @@ describe("resolveSlackThreadTs", () => { }); describe("replyToMode=first", () => { - it("returns baseThreadTs for first reply", () => { + it("returns incomingThreadTs when in a thread (always stays threaded)", () => { expect( resolveSlackThreadTs({ replyToMode: "first", - baseThreadTs: threadTs, + incomingThreadTs: threadTs, + messageTs, hasReplied: false, }), ).toBe(threadTs); }); - it("returns undefined for subsequent replies (goes to main channel)", () => { + it("returns messageTs for first reply when not in a thread", () => { expect( resolveSlackThreadTs({ replyToMode: "first", - baseThreadTs: threadTs, + incomingThreadTs: undefined, + messageTs, + hasReplied: false, + }), + ).toBe(messageTs); + }); + + it("returns undefined for subsequent replies when not in a thread (goes to main channel)", () => { + expect( + resolveSlackThreadTs({ + replyToMode: "first", + incomingThreadTs: undefined, + messageTs, hasReplied: true, }), ).toBeUndefined(); @@ -112,24 +129,26 @@ describe("resolveSlackThreadTs", () => { }); describe("replyToMode=all", () => { - it("returns baseThreadTs for first reply", () => { + it("returns incomingThreadTs when in a thread", () => { expect( resolveSlackThreadTs({ replyToMode: "all", - baseThreadTs: threadTs, + incomingThreadTs: threadTs, + messageTs, hasReplied: false, }), ).toBe(threadTs); }); - it("returns baseThreadTs for subsequent replies (all go to thread)", () => { + it("returns messageTs when not in a thread (starts thread)", () => { expect( resolveSlackThreadTs({ replyToMode: "all", - baseThreadTs: threadTs, + incomingThreadTs: undefined, + messageTs, hasReplied: true, }), - ).toBe(threadTs); + ).toBe(messageTs); }); }); }); diff --git a/src/slack/monitor.ts b/src/slack/monitor.ts index b8773316c..a83d1452f 100644 --- a/src/slack/monitor.ts +++ b/src/slack/monitor.ts @@ -1074,13 +1074,8 @@ export async function monitorSlackProvider(opts: MonitorSlackOpts = {}) { message, replyToMode, }); - // Base thread timestamp: where should first reply go? - // - "off": only thread if already in a thread - // - "first"/"all": start thread under the message - const baseThreadTs = - replyToMode === "off" - ? message.thread_ts - : (message.thread_ts ?? message.ts); + const messageTs = message.ts ?? message.event_ts; + const incomingThreadTs = message.thread_ts; let didSetStatus = false; // Shared mutable ref for tracking if a reply was sent (used by both // auto-reply path and tool path for "first" threading mode). @@ -1100,7 +1095,8 @@ export async function monitorSlackProvider(opts: MonitorSlackOpts = {}) { deliver: async (payload) => { const effectiveThreadTs = resolveSlackThreadTs({ replyToMode, - baseThreadTs, + incomingThreadTs, + messageTs, hasReplied: hasRepliedRef.value, }); await deliverReplies({ @@ -1984,20 +1980,23 @@ export function isSlackRoomAllowedByPolicy(params: { */ export function resolveSlackThreadTs(params: { replyToMode: "off" | "first" | "all"; - baseThreadTs: string | undefined; + incomingThreadTs: string | undefined; + messageTs: string | undefined; hasReplied: boolean; }): string | undefined { - const { replyToMode, baseThreadTs, hasReplied } = params; - if (replyToMode === "off") { - // Always stay in thread if already in one - return baseThreadTs; - } + const { replyToMode, incomingThreadTs, messageTs, hasReplied } = params; + if (incomingThreadTs) return incomingThreadTs; + if (!messageTs) return undefined; if (replyToMode === "all") { // All replies go to thread - return baseThreadTs; + return messageTs; } - // "first": only first reply goes to thread - return hasReplied ? undefined : baseThreadTs; + if (replyToMode === "first") { + // "first": only first reply goes to thread + return hasReplied ? undefined : messageTs; + } + // "off": never start a thread + return undefined; } async function deliverSlackSlashReplies(params: {