fix(slack): mrkdwn + thread edge cases (#464) (thanks @austinm911)

2026-01-09 22:09:02 +01:00
parent 8890fbcf38
commit 84046cbad8
8 changed files with 222 additions and 49 deletions
--- 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
--- 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();
--- 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": {
--- 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,
  };
 }
--- 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 &amp; b &lt; c &gt; d");
  });

+  it("preserves Slack angle-bracket markup (mentions/links)", () => {
+    const res = markdownToSlackMrkdwn(
+      "hi <@U123> see <https://example.com|docs> and <!here>",
+    );
+    expect(res).toBe("hi <@U123> see <https://example.com|docs> and <!here>");
+  });
+
  it("escapes raw HTML", () => {
    const res = markdownToSlackMrkdwn("<b>nope</b>");
    expect(res).toBe("&lt;b&gt;nope&lt;/b&gt;");
@@ -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("&gt; Quote");
+    expect(res).toBe("> Quote");
  });

  it("handles adjacent list items", () => {
--- 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>", "<https://…|text>"). 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, "&amp;")
    .replace(/</g, "&lt;")
    .replace(/>/g, "&gt;");
 }

+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 = () => "&gt; ";
+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 <url|text> 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);
 }
--- 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);
    });
  });
 });
--- 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: {