From 4d590f9254ae4d1ad61ee8e54ad1bec2d3d5a1cf Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sun, 18 Jan 2026 00:15:02 +0000 Subject: [PATCH] refactor(slack): centralize target parsing --- src/agents/tools/slack-actions.test.ts | 14 +++++ src/agents/tools/slack-actions.ts | 34 +++++------ src/channels/plugins/normalize-target.ts | 31 ++-------- src/channels/plugins/slack.actions.ts | 5 +- src/commands/channels/capabilities.ts | 18 +++--- src/slack/send.ts | 34 ++--------- src/slack/targets.test.ts | 59 ++++++++++++++++++ src/slack/targets.ts | 78 ++++++++++++++++++++++++ 8 files changed, 190 insertions(+), 83 deletions(-) create mode 100644 src/slack/targets.test.ts create mode 100644 src/slack/targets.ts diff --git a/src/agents/tools/slack-actions.test.ts b/src/agents/tools/slack-actions.test.ts index 9e2959a07..92f20e66a 100644 --- a/src/agents/tools/slack-actions.test.ts +++ b/src/agents/tools/slack-actions.test.ts @@ -48,6 +48,20 @@ describe("handleSlackAction", () => { expect(reactSlackMessage).toHaveBeenCalledWith("C1", "123.456", "✅"); }); + it("strips channel: prefix for channelId params", async () => { + const cfg = { channels: { slack: { botToken: "tok" } } } as ClawdbotConfig; + await handleSlackAction( + { + action: "react", + channelId: "channel:C1", + messageId: "123.456", + emoji: "✅", + }, + cfg, + ); + expect(reactSlackMessage).toHaveBeenCalledWith("C1", "123.456", "✅"); + }); + it("removes reactions on empty emoji", async () => { const cfg = { channels: { slack: { botToken: "tok" } } } as ClawdbotConfig; await handleSlackAction( diff --git a/src/agents/tools/slack-actions.ts b/src/agents/tools/slack-actions.ts index 84659cd8f..73706b2a4 100644 --- a/src/agents/tools/slack-actions.ts +++ b/src/agents/tools/slack-actions.ts @@ -17,6 +17,7 @@ import { sendSlackMessage, unpinSlackMessage, } from "../../slack/actions.js"; +import { parseSlackTarget, resolveSlackChannelId } from "../../slack/targets.js"; import { withNormalizedTimestamp } from "../date-time.js"; import { createActionGate, jsonResult, readReactionParams, readStringParam } from "./common.js"; @@ -52,10 +53,9 @@ function resolveThreadTsFromContext( // No context or missing required fields if (!context?.currentThreadTs || !context?.currentChannelId) return undefined; - // Normalize target (strip "channel:" prefix if present) - const normalizedTarget = targetChannel.startsWith("channel:") - ? targetChannel.slice("channel:".length) - : targetChannel; + const parsedTarget = parseSlackTarget(targetChannel, { defaultKind: "channel" }); + if (!parsedTarget || parsedTarget.kind !== "channel") return undefined; + const normalizedTarget = parsedTarget.id; // Different channel - don't inject if (normalizedTarget !== context.currentChannelId) return undefined; @@ -76,6 +76,12 @@ export async function handleSlackAction( cfg: ClawdbotConfig, context?: SlackActionContext, ): Promise> { + const resolveChannelId = () => + resolveSlackChannelId( + readStringParam(params, "channelId", { + required: true, + }), + ); const action = readStringParam(params, "action", { required: true }); const accountId = readStringParam(params, "accountId"); const account = resolveSlackAccount({ cfg, accountId }); @@ -109,7 +115,7 @@ export async function handleSlackAction( if (!isActionEnabled("reactions")) { throw new Error("Slack reactions are disabled."); } - const channelId = readStringParam(params, "channelId", { required: true }); + const channelId = resolveChannelId(); const messageId = readStringParam(params, "messageId", { required: true }); if (action === "react") { const { emoji, remove, isEmpty } = readReactionParams(params, { @@ -166,8 +172,8 @@ export async function handleSlackAction( // 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) { + const parsedTarget = parseSlackTarget(to, { defaultKind: "channel" }); + if (parsedTarget?.kind === "channel" && parsedTarget.id === context.currentChannelId) { context.hasRepliedRef.value = true; } } @@ -175,9 +181,7 @@ export async function handleSlackAction( return jsonResult({ ok: true, result }); } case "editMessage": { - const channelId = readStringParam(params, "channelId", { - required: true, - }); + const channelId = resolveChannelId(); const messageId = readStringParam(params, "messageId", { required: true, }); @@ -192,9 +196,7 @@ export async function handleSlackAction( return jsonResult({ ok: true }); } case "deleteMessage": { - const channelId = readStringParam(params, "channelId", { - required: true, - }); + const channelId = resolveChannelId(); const messageId = readStringParam(params, "messageId", { required: true, }); @@ -206,9 +208,7 @@ export async function handleSlackAction( return jsonResult({ ok: true }); } case "readMessages": { - const channelId = readStringParam(params, "channelId", { - required: true, - }); + const channelId = resolveChannelId(); const limitRaw = params.limit; const limit = typeof limitRaw === "number" && Number.isFinite(limitRaw) ? limitRaw : undefined; @@ -237,7 +237,7 @@ export async function handleSlackAction( if (!isActionEnabled("pins")) { throw new Error("Slack pins are disabled."); } - const channelId = readStringParam(params, "channelId", { required: true }); + const channelId = resolveChannelId(); if (action === "pinMessage") { const messageId = readStringParam(params, "messageId", { required: true, diff --git a/src/channels/plugins/normalize-target.ts b/src/channels/plugins/normalize-target.ts index c19c0fe7a..267cafaed 100644 --- a/src/channels/plugins/normalize-target.ts +++ b/src/channels/plugins/normalize-target.ts @@ -1,32 +1,10 @@ -import { normalizeWhatsAppTarget } from "../../whatsapp/normalize.js"; import { parseDiscordTarget } from "../../discord/targets.js"; +import { parseSlackTarget } from "../../slack/targets.js"; +import { normalizeWhatsAppTarget } from "../../whatsapp/normalize.js"; export function normalizeSlackMessagingTarget(raw: string): string | undefined { - const trimmed = raw.trim(); - if (!trimmed) return undefined; - const mentionMatch = trimmed.match(/^<@([A-Z0-9]+)>$/i); - if (mentionMatch) return `user:${mentionMatch[1]}`.toLowerCase(); - if (trimmed.startsWith("user:")) { - const id = trimmed.slice(5).trim(); - return id ? `user:${id}`.toLowerCase() : undefined; - } - if (trimmed.startsWith("channel:")) { - const id = trimmed.slice(8).trim(); - return id ? `channel:${id}`.toLowerCase() : undefined; - } - if (trimmed.startsWith("slack:")) { - const id = trimmed.slice(6).trim(); - return id ? `user:${id}`.toLowerCase() : undefined; - } - if (trimmed.startsWith("@")) { - const id = trimmed.slice(1).trim(); - return id ? `user:${id}`.toLowerCase() : undefined; - } - if (trimmed.startsWith("#")) { - const id = trimmed.slice(1).trim(); - return id ? `channel:${id}`.toLowerCase() : undefined; - } - return `channel:${trimmed}`.toLowerCase(); + const target = parseSlackTarget(raw, { defaultKind: "channel" }); + return target?.normalized; } export function looksLikeSlackTargetId(raw: string): boolean { @@ -40,6 +18,7 @@ export function looksLikeSlackTargetId(raw: string): boolean { } export function normalizeDiscordMessagingTarget(raw: string): string | undefined { + // Default bare IDs to channels so routing is stable across tool actions. const target = parseDiscordTarget(raw, { defaultKind: "channel" }); return target?.normalized; } diff --git a/src/channels/plugins/slack.actions.ts b/src/channels/plugins/slack.actions.ts index efa3a7b92..295e6634c 100644 --- a/src/channels/plugins/slack.actions.ts +++ b/src/channels/plugins/slack.actions.ts @@ -1,6 +1,7 @@ import { createActionGate, readNumberParam, readStringParam } from "../../agents/tools/common.js"; import { handleSlackAction, type SlackActionContext } from "../../agents/tools/slack-actions.js"; import { listEnabledSlackAccounts } from "../../slack/accounts.js"; +import { resolveSlackChannelId } from "../../slack/targets.js"; import type { ChannelMessageActionAdapter, ChannelMessageActionContext, @@ -60,7 +61,9 @@ export function createSlackActions(providerId: string): ChannelMessageActionAdap const accountId = ctx.accountId ?? undefined; const toolContext = ctx.toolContext as SlackActionContext | undefined; const resolveChannelId = () => - readStringParam(params, "channelId") ?? readStringParam(params, "to", { required: true }); + resolveSlackChannelId( + readStringParam(params, "channelId") ?? readStringParam(params, "to", { required: true }), + ); if (action === "send") { const to = readStringParam(params, "to", { required: true }); diff --git a/src/commands/channels/capabilities.ts b/src/commands/channels/capabilities.ts index 00a7b1cab..4b668d3d2 100644 --- a/src/commands/channels/capabilities.ts +++ b/src/commands/channels/capabilities.ts @@ -1,8 +1,8 @@ import { getChannelPlugin, listChannelPlugins } from "../../channels/plugins/index.js"; import { resolveChannelDefaultAccountId } from "../../channels/plugins/helpers.js"; -import { normalizeDiscordMessagingTarget } from "../../channels/plugins/normalize-target.js"; import type { ChannelCapabilities, ChannelPlugin } from "../../channels/plugins/types.js"; import { fetchChannelPermissionsDiscord } from "../../discord/send.js"; +import { parseDiscordTarget } from "../../discord/targets.js"; import { danger } from "../../globals.js"; import type { ClawdbotConfig } from "../../config/config.js"; import { defaultRuntime, type RuntimeEnv } from "../../runtime.js"; @@ -88,24 +88,24 @@ function formatSupport(capabilities?: ChannelCapabilities) { function summarizeDiscordTarget(raw?: string): DiscordTargetSummary | undefined { if (!raw) return undefined; - const normalized = normalizeDiscordMessagingTarget(raw); - if (!normalized) return { raw }; - if (normalized.startsWith("channel:")) { + const target = parseDiscordTarget(raw, { defaultKind: "channel" }); + if (!target) return { raw }; + if (target.kind === "channel") { return { raw, - normalized, + normalized: target.normalized, kind: "channel", - channelId: normalized.slice("channel:".length), + channelId: target.id, }; } - if (normalized.startsWith("user:")) { + if (target.kind === "user") { return { raw, - normalized, + normalized: target.normalized, kind: "user", }; } - return { raw, normalized }; + return { raw, normalized: target.normalized }; } function formatDiscordIntents(intents?: { diff --git a/src/slack/send.ts b/src/slack/send.ts index 6058d8b2c..1f867e9ac 100644 --- a/src/slack/send.ts +++ b/src/slack/send.ts @@ -7,6 +7,7 @@ import { loadWebMedia } from "../web/media.js"; import type { SlackTokenSource } from "./accounts.js"; import { resolveSlackAccount } from "./accounts.js"; import { markdownToSlackMrkdwnChunks } from "./format.js"; +import { parseSlackTarget } from "./targets.js"; import { resolveSlackBotToken } from "./token.js"; const SLACK_TEXT_LIMIT = 4000; @@ -57,38 +58,11 @@ function resolveToken(params: { } function parseRecipient(raw: string): SlackRecipient { - const trimmed = raw.trim(); - if (!trimmed) { + const target = parseSlackTarget(raw); + if (!target) { throw new Error("Recipient is required for Slack sends"); } - const mentionMatch = trimmed.match(/^<@([A-Z0-9]+)>$/i); - if (mentionMatch) { - return { kind: "user", id: mentionMatch[1] }; - } - if (trimmed.startsWith("user:")) { - return { kind: "user", id: trimmed.slice("user:".length) }; - } - if (trimmed.startsWith("channel:")) { - return { kind: "channel", id: trimmed.slice("channel:".length) }; - } - if (trimmed.startsWith("slack:")) { - return { kind: "user", id: trimmed.slice("slack:".length) }; - } - if (trimmed.startsWith("@")) { - const candidate = trimmed.slice(1); - if (!/^[A-Z0-9]+$/i.test(candidate)) { - throw new Error("Slack DMs require a user id (use user: or <@id>)"); - } - return { kind: "user", id: candidate }; - } - if (trimmed.startsWith("#")) { - const candidate = trimmed.slice(1); - if (!/^[A-Z0-9]+$/i.test(candidate)) { - throw new Error("Slack channels require a channel id (use channel:)"); - } - return { kind: "channel", id: candidate }; - } - return { kind: "channel", id: trimmed }; + return { kind: target.kind, id: target.id }; } async function resolveChannelId( diff --git a/src/slack/targets.test.ts b/src/slack/targets.test.ts new file mode 100644 index 000000000..c25d2ab1f --- /dev/null +++ b/src/slack/targets.test.ts @@ -0,0 +1,59 @@ +import { describe, expect, it } from "vitest"; + +import { normalizeSlackMessagingTarget } from "../channels/plugins/normalize-target.js"; +import { parseSlackTarget, resolveSlackChannelId } from "./targets.js"; + +describe("parseSlackTarget", () => { + it("parses user mentions and prefixes", () => { + expect(parseSlackTarget("<@U123>")).toMatchObject({ + kind: "user", + id: "U123", + normalized: "user:u123", + }); + expect(parseSlackTarget("user:U456")).toMatchObject({ + kind: "user", + id: "U456", + normalized: "user:u456", + }); + expect(parseSlackTarget("slack:U789")).toMatchObject({ + kind: "user", + id: "U789", + normalized: "user:u789", + }); + }); + + it("parses channel targets", () => { + expect(parseSlackTarget("channel:C123")).toMatchObject({ + kind: "channel", + id: "C123", + normalized: "channel:c123", + }); + expect(parseSlackTarget("#C999")).toMatchObject({ + kind: "channel", + id: "C999", + normalized: "channel:c999", + }); + }); + + it("rejects invalid @ and # targets", () => { + expect(() => parseSlackTarget("@bob-1")).toThrow(/Slack DMs require a user id/); + expect(() => parseSlackTarget("#general-1")).toThrow(/Slack channels require a channel id/); + }); +}); + +describe("resolveSlackChannelId", () => { + it("strips channel: prefix and accepts raw ids", () => { + expect(resolveSlackChannelId("channel:C123")).toBe("C123"); + expect(resolveSlackChannelId("C123")).toBe("C123"); + }); + + it("rejects user targets", () => { + expect(() => resolveSlackChannelId("user:U123")).toThrow(/channel id is required/i); + }); +}); + +describe("normalizeSlackMessagingTarget", () => { + it("defaults raw ids to channels", () => { + expect(normalizeSlackMessagingTarget("C123")).toBe("channel:c123"); + }); +}); diff --git a/src/slack/targets.ts b/src/slack/targets.ts new file mode 100644 index 000000000..9ec8939d3 --- /dev/null +++ b/src/slack/targets.ts @@ -0,0 +1,78 @@ +export type SlackTargetKind = "user" | "channel"; + +export type SlackTarget = { + kind: SlackTargetKind; + id: string; + raw: string; + normalized: string; +}; + +type SlackTargetParseOptions = { + defaultKind?: SlackTargetKind; +}; + +function normalizeTargetId(kind: SlackTargetKind, id: string) { + return `${kind}:${id}`.toLowerCase(); +} + +function buildTarget(kind: SlackTargetKind, id: string, raw: string): SlackTarget { + return { + kind, + id, + raw, + normalized: normalizeTargetId(kind, id), + }; +} + +export function parseSlackTarget( + raw: string, + options: SlackTargetParseOptions = {}, +): SlackTarget | undefined { + const trimmed = raw.trim(); + if (!trimmed) return undefined; + const mentionMatch = trimmed.match(/^<@([A-Z0-9]+)>$/i); + if (mentionMatch) { + return buildTarget("user", mentionMatch[1], trimmed); + } + if (trimmed.startsWith("user:")) { + const id = trimmed.slice("user:".length).trim(); + return id ? buildTarget("user", id, trimmed) : undefined; + } + if (trimmed.startsWith("channel:")) { + const id = trimmed.slice("channel:".length).trim(); + return id ? buildTarget("channel", id, trimmed) : undefined; + } + if (trimmed.startsWith("slack:")) { + const id = trimmed.slice("slack:".length).trim(); + return id ? buildTarget("user", id, trimmed) : undefined; + } + if (trimmed.startsWith("@")) { + const candidate = trimmed.slice(1).trim(); + if (!/^[A-Z0-9]+$/i.test(candidate)) { + throw new Error("Slack DMs require a user id (use user: or <@id>)"); + } + return buildTarget("user", candidate, trimmed); + } + if (trimmed.startsWith("#")) { + const candidate = trimmed.slice(1).trim(); + if (!/^[A-Z0-9]+$/i.test(candidate)) { + throw new Error("Slack channels require a channel id (use channel:)"); + } + return buildTarget("channel", candidate, trimmed); + } + if (options.defaultKind) { + return buildTarget(options.defaultKind, trimmed, trimmed); + } + return buildTarget("channel", trimmed, trimmed); +} + +export function resolveSlackChannelId(raw: string): string { + const target = parseSlackTarget(raw, { defaultKind: "channel" }); + if (!target) { + throw new Error("Slack channel id is required."); + } + if (target.kind !== "channel") { + throw new Error("Slack channel id is required (use channel:)."); + } + return target.id; +}