diff --git a/CHANGELOG.md b/CHANGELOG.md index bb96adacd45..4f1453e1d9b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,10 @@ Docs: https://docs.openclaw.ai ## Unreleased +### Fixes + +- Slack: stop retry-driven duplicate replies when draft-finalization edits fail ambiguously, and log configured allowlisted users/channels by readable name instead of raw IDs. + ## 2026.3.31-beta.1 ### Breaking diff --git a/extensions/slack/src/actions.ts b/extensions/slack/src/actions.ts index 72790b50cc8..73e0e72ccf7 100644 --- a/extensions/slack/src/actions.ts +++ b/extensions/slack/src/actions.ts @@ -4,7 +4,7 @@ import { logVerbose } from "openclaw/plugin-sdk/runtime-env"; import { resolveSlackAccount } from "./accounts.js"; import { buildSlackBlocksFallbackText } from "./blocks-fallback.js"; import { validateSlackBlocksArray } from "./blocks-input.js"; -import { createSlackWebClient } from "./client.js"; +import { createSlackWebClient, createSlackWriteClient } from "./client.js"; import { resolveSlackMedia } from "./monitor/media.js"; import type { SlackMediaResult } from "./monitor/media.js"; import { sendMessageSlack } from "./send.js"; @@ -64,9 +64,11 @@ function normalizeEmoji(raw: string) { return trimmed.replace(/^:+|:+$/g, ""); } -async function getClient(opts: SlackActionClientOpts = {}) { +async function getClient(opts: SlackActionClientOpts = {}, mode: "read" | "write" = "read") { const token = resolveToken(opts.token, opts.accountId); - return opts.client ?? createSlackWebClient(token); + return ( + opts.client ?? (mode === "write" ? createSlackWriteClient(token) : createSlackWebClient(token)) + ); } async function resolveBotUserId(client: WebClient) { @@ -83,7 +85,7 @@ export async function reactSlackMessage( emoji: string, opts: SlackActionClientOpts = {}, ) { - const client = await getClient(opts); + const client = await getClient(opts, "write"); await client.reactions.add({ channel: channelId, timestamp: messageId, @@ -97,7 +99,7 @@ export async function removeSlackReaction( emoji: string, opts: SlackActionClientOpts = {}, ) { - const client = await getClient(opts); + const client = await getClient(opts, "write"); await client.reactions.remove({ channel: channelId, timestamp: messageId, @@ -110,7 +112,7 @@ export async function removeOwnSlackReactions( messageId: string, opts: SlackActionClientOpts = {}, ): Promise { - const client = await getClient(opts); + const client = await getClient(opts, "write"); const userId = await resolveBotUserId(client); const reactions = await listSlackReactions(channelId, messageId, { client }); const toRemove = new Set(); @@ -192,7 +194,7 @@ export async function editSlackMessage( content: string, opts: SlackActionClientOpts & { blocks?: (Block | KnownBlock)[] } = {}, ) { - const client = await getClient(opts); + const client = await getClient(opts, "write"); const blocks = opts.blocks == null ? undefined : validateSlackBlocksArray(opts.blocks); const trimmedContent = content.trim(); await client.chat.update({ @@ -208,7 +210,7 @@ export async function deleteSlackMessage( messageId: string, opts: SlackActionClientOpts = {}, ) { - const client = await getClient(opts); + const client = await getClient(opts, "write"); await client.chat.delete({ channel: channelId, ts: messageId, @@ -271,7 +273,7 @@ export async function pinSlackMessage( messageId: string, opts: SlackActionClientOpts = {}, ) { - const client = await getClient(opts); + const client = await getClient(opts, "write"); await client.pins.add({ channel: channelId, timestamp: messageId }); } @@ -280,7 +282,7 @@ export async function unpinSlackMessage( messageId: string, opts: SlackActionClientOpts = {}, ) { - const client = await getClient(opts); + const client = await getClient(opts, "write"); await client.pins.remove({ channel: channelId, timestamp: messageId }); } diff --git a/extensions/slack/src/client.test.ts b/extensions/slack/src/client.test.ts index dc4c5a8ee13..249c8445616 100644 --- a/extensions/slack/src/client.test.ts +++ b/extensions/slack/src/client.test.ts @@ -13,15 +13,24 @@ vi.mock("@slack/web-api", () => { }); let createSlackWebClient: typeof import("./client.js").createSlackWebClient; +let createSlackWriteClient: typeof import("./client.js").createSlackWriteClient; let resolveSlackWebClientOptions: typeof import("./client.js").resolveSlackWebClientOptions; +let resolveSlackWriteClientOptions: typeof import("./client.js").resolveSlackWriteClientOptions; let SLACK_DEFAULT_RETRY_OPTIONS: typeof import("./client.js").SLACK_DEFAULT_RETRY_OPTIONS; +let SLACK_WRITE_RETRY_OPTIONS: typeof import("./client.js").SLACK_WRITE_RETRY_OPTIONS; let WebClient: ReturnType; beforeEach(async () => { vi.resetModules(); const slackWebApi = await import("@slack/web-api"); - ({ createSlackWebClient, resolveSlackWebClientOptions, SLACK_DEFAULT_RETRY_OPTIONS } = - await import("./client.js")); + ({ + createSlackWebClient, + createSlackWriteClient, + resolveSlackWebClientOptions, + resolveSlackWriteClientOptions, + SLACK_DEFAULT_RETRY_OPTIONS, + SLACK_WRITE_RETRY_OPTIONS, + } = await import("./client.js")); WebClient = slackWebApi.WebClient as unknown as ReturnType; }); @@ -50,4 +59,22 @@ describe("slack web client config", () => { }), ); }); + + it("applies the write retry config when none is provided", () => { + const options = resolveSlackWriteClientOptions(); + + expect(options.retryConfig).toEqual(SLACK_WRITE_RETRY_OPTIONS); + }); + + it("passes no-retry config into the write client by default", () => { + createSlackWriteClient("xoxb-test", { timeout: 4321 }); + + expect(WebClient).toHaveBeenCalledWith( + "xoxb-test", + expect.objectContaining({ + timeout: 4321, + retryConfig: SLACK_WRITE_RETRY_OPTIONS, + }), + ); + }); }); diff --git a/extensions/slack/src/client.ts b/extensions/slack/src/client.ts index f792bd22a0d..59ad358a27a 100644 --- a/extensions/slack/src/client.ts +++ b/extensions/slack/src/client.ts @@ -8,6 +8,10 @@ export const SLACK_DEFAULT_RETRY_OPTIONS: RetryOptions = { randomize: true, }; +export const SLACK_WRITE_RETRY_OPTIONS: RetryOptions = { + retries: 0, +}; + export function resolveSlackWebClientOptions(options: WebClientOptions = {}): WebClientOptions { return { ...options, @@ -15,6 +19,17 @@ export function resolveSlackWebClientOptions(options: WebClientOptions = {}): We }; } +export function resolveSlackWriteClientOptions(options: WebClientOptions = {}): WebClientOptions { + return { + ...options, + retryConfig: options.retryConfig ?? SLACK_WRITE_RETRY_OPTIONS, + }; +} + export function createSlackWebClient(token: string, options: WebClientOptions = {}) { return new WebClient(token, resolveSlackWebClientOptions(options)); } + +export function createSlackWriteClient(token: string, options: WebClientOptions = {}) { + return new WebClient(token, resolveSlackWriteClientOptions(options)); +} diff --git a/extensions/slack/src/monitor/message-handler/dispatch.ts b/extensions/slack/src/monitor/message-handler/dispatch.ts index 052757a9d35..1172b48ccaf 100644 --- a/extensions/slack/src/monitor/message-handler/dispatch.ts +++ b/extensions/slack/src/monitor/message-handler/dispatch.ts @@ -17,7 +17,7 @@ import { createReplyDispatcherWithTyping } from "openclaw/plugin-sdk/reply-runti import type { ReplyPayload } from "openclaw/plugin-sdk/reply-runtime"; import { danger, logVerbose, shouldLogVerbose } from "openclaw/plugin-sdk/runtime-env"; import { resolvePinnedMainDmOwnerFromAllowlist } from "openclaw/plugin-sdk/security-runtime"; -import { editSlackMessage, reactSlackMessage, removeSlackReaction } from "../../actions.js"; +import { reactSlackMessage, removeSlackReaction } from "../../actions.js"; import { createSlackDraftStream } from "../../draft-stream.js"; import { normalizeSlackOutboundText } from "../../format.js"; import { SLACK_TEXT_LIMIT } from "../../limits.js"; @@ -37,6 +37,7 @@ import { readSlackReplyBlocks, resolveSlackThreadTs, } from "../replies.js"; +import { finalizeSlackPreviewEdit } from "./preview-finalize.js"; import type { PreparedSlackMessage } from "./types.js"; function sleep(ms: number): Promise { @@ -447,17 +448,16 @@ export async function dispatchPreparedSlackMessage(prepared: PreparedSlackMessag if (canFinalizeViaPreviewEdit) { draftStream?.stop(); try { - await editSlackMessage( - draftChannelId, - draftMessageId, - normalizeSlackOutboundText(trimmedFinalText), - { - token: ctx.botToken, - accountId: account.accountId, - client: ctx.app.client, - ...(slackBlocks?.length ? { blocks: slackBlocks } : {}), - }, - ); + await finalizeSlackPreviewEdit({ + client: ctx.app.client, + token: ctx.botToken, + accountId: account.accountId, + channelId: draftChannelId, + messageId: draftMessageId, + text: normalizeSlackOutboundText(trimmedFinalText), + ...(slackBlocks?.length ? { blocks: slackBlocks } : {}), + threadTs: usedReplyThreadTs ?? statusThreadTs, + }); observedReplyDelivery = true; return; } catch (err) { diff --git a/extensions/slack/src/monitor/message-handler/preview-finalize.test.ts b/extensions/slack/src/monitor/message-handler/preview-finalize.test.ts new file mode 100644 index 00000000000..7ffa983b5b3 --- /dev/null +++ b/extensions/slack/src/monitor/message-handler/preview-finalize.test.ts @@ -0,0 +1,109 @@ +import type { WebClient } from "@slack/web-api"; +import { beforeEach, describe, expect, it, vi } from "vitest"; + +const editSlackMessageMock = vi.fn(); + +vi.mock("../../actions.js", () => ({ + editSlackMessage: (...args: unknown[]) => + editSlackMessageMock(...(args as Parameters)), +})); + +let finalizeSlackPreviewEdit: typeof import("./preview-finalize.js").finalizeSlackPreviewEdit; +let __testing: typeof import("./preview-finalize.js").__testing; + +function createClient(overrides?: { + historyMessages?: Array>; + replyMessages?: Array>; +}) { + return { + conversations: { + history: vi.fn(async () => ({ messages: overrides?.historyMessages ?? [] })), + replies: vi.fn(async () => ({ messages: overrides?.replyMessages ?? [] })), + }, + } as unknown as WebClient; +} + +describe("finalizeSlackPreviewEdit", () => { + beforeEach(async () => { + vi.resetModules(); + editSlackMessageMock.mockReset(); + ({ finalizeSlackPreviewEdit, __testing } = await import("./preview-finalize.js")); + }); + + it("treats a thrown edit as success when history readback already matches", async () => { + editSlackMessageMock.mockRejectedValueOnce(new Error("socket closed")); + const client = createClient({ + historyMessages: [{ ts: "171234.567", text: "fair. poke harder then 🦞" }], + }); + + await expect( + finalizeSlackPreviewEdit({ + client, + token: "xoxb-test", + channelId: "C123", + messageId: "171234.567", + text: "fair. poke harder then 🦞", + }), + ).resolves.toBeUndefined(); + + expect(client.conversations.history as unknown as ReturnType).toHaveBeenCalled(); + }); + + it("checks threaded replies via conversations.replies", async () => { + editSlackMessageMock.mockRejectedValueOnce(new Error("socket closed")); + const client = createClient({ + replyMessages: [{ ts: "171234.567", text: "done" }], + }); + + await expect( + finalizeSlackPreviewEdit({ + client, + token: "xoxb-test", + channelId: "C123", + messageId: "171234.567", + threadTs: "170000.111", + text: "done", + }), + ).resolves.toBeUndefined(); + + expect( + client.conversations.replies as unknown as ReturnType, + ).toHaveBeenCalledWith( + expect.objectContaining({ + channel: "C123", + ts: "170000.111", + latest: "171234.567", + }), + ); + }); + + it("rethrows when readback does not match the expected final text", async () => { + editSlackMessageMock.mockRejectedValueOnce(new Error("socket closed")); + const client = createClient({ + historyMessages: [{ ts: "171234.567", text: "partial draft" }], + }); + + await expect( + finalizeSlackPreviewEdit({ + client, + token: "xoxb-test", + channelId: "C123", + messageId: "171234.567", + text: "final answer", + }), + ).rejects.toThrow("socket closed"); + }); + + it("requires matching blocks when finalizing a blocks-only edit", async () => { + const blocks = [{ type: "section", text: { type: "mrkdwn", text: "*Done*" } }] as const; + + expect( + __testing.buildExpectedSlackEditText({ + text: "", + blocks: blocks as unknown as Parameters< + typeof __testing.buildExpectedSlackEditText + >[0]["blocks"], + }), + ).toBe("*Done*"); + }); +}); diff --git a/extensions/slack/src/monitor/message-handler/preview-finalize.ts b/extensions/slack/src/monitor/message-handler/preview-finalize.ts new file mode 100644 index 00000000000..b970e67daf3 --- /dev/null +++ b/extensions/slack/src/monitor/message-handler/preview-finalize.ts @@ -0,0 +1,145 @@ +import type { Block, KnownBlock, WebClient } from "@slack/web-api"; +import { logVerbose } from "openclaw/plugin-sdk/runtime-env"; +import { editSlackMessage } from "../../actions.js"; +import { buildSlackBlocksFallbackText } from "../../blocks-fallback.js"; +import { normalizeSlackOutboundText } from "../../format.js"; + +type SlackReadbackMessage = { + ts?: string; + text?: string; + blocks?: unknown[]; +}; + +function buildExpectedSlackEditText(params: { + text: string; + blocks?: (Block | KnownBlock)[]; +}): string { + const trimmed = normalizeSlackOutboundText(params.text.trim()); + if (trimmed) { + return trimmed; + } + if (params.blocks?.length) { + return buildSlackBlocksFallbackText(params.blocks); + } + return " "; +} + +function blocksMatch(expected?: (Block | KnownBlock)[], actual?: unknown[]): boolean { + if (!expected?.length) { + return !actual?.length; + } + if (!actual?.length) { + return false; + } + return JSON.stringify(expected) === JSON.stringify(actual); +} + +async function readSlackMessageAfterEditError(params: { + client: WebClient; + token: string; + channelId: string; + messageId: string; + threadTs?: string; +}): Promise { + if (params.threadTs) { + const replyResult = await params.client.conversations.replies({ + token: params.token, + channel: params.channelId, + ts: params.threadTs, + latest: params.messageId, + inclusive: true, + limit: 100, + }); + const reply = (replyResult.messages ?? []).find( + (message) => (message as SlackReadbackMessage | undefined)?.ts === params.messageId, + ) as SlackReadbackMessage | undefined; + return reply ?? null; + } + + const historyResult = await params.client.conversations.history({ + token: params.token, + channel: params.channelId, + latest: params.messageId, + oldest: params.messageId, + inclusive: true, + limit: 1, + }); + const message = historyResult.messages?.[0] as SlackReadbackMessage | undefined; + if (!message?.ts || message.ts !== params.messageId) { + return null; + } + return message; +} + +async function didSlackPreviewEditApplyAfterError(params: { + client: WebClient; + token: string; + channelId: string; + messageId: string; + text: string; + blocks?: (Block | KnownBlock)[]; + threadTs?: string; +}): Promise { + const readback = await readSlackMessageAfterEditError(params); + if (!readback) { + return false; + } + const expectedText = buildExpectedSlackEditText({ + text: params.text, + blocks: params.blocks, + }); + const actualText = normalizeSlackOutboundText((readback.text ?? "").trim()); + if (params.blocks?.length) { + return actualText === expectedText && blocksMatch(params.blocks, readback.blocks); + } + return actualText === expectedText; +} + +export async function finalizeSlackPreviewEdit(params: { + client: WebClient; + token: string; + accountId?: string; + channelId: string; + messageId: string; + text: string; + blocks?: (Block | KnownBlock)[]; + threadTs?: string; +}): Promise { + try { + await editSlackMessage(params.channelId, params.messageId, params.text, { + token: params.token, + accountId: params.accountId, + client: params.client, + ...(params.blocks?.length ? { blocks: params.blocks } : {}), + }); + return; + } catch (err) { + try { + const applied = await didSlackPreviewEditApplyAfterError({ + client: params.client, + token: params.token, + channelId: params.channelId, + messageId: params.messageId, + text: params.text, + blocks: params.blocks, + threadTs: params.threadTs, + }); + if (applied) { + logVerbose( + `slack: preview final edit response failed but readback matched message ${params.channelId}/${params.messageId}; suppressing duplicate fallback send`, + ); + return; + } + } catch (readbackErr) { + logVerbose(`slack: preview final edit readback failed (${String(readbackErr)})`); + } + throw err; + } +} + +export const __testing = { + buildExpectedSlackEditText, + blocksMatch, + didSlackPreviewEditApplyAfterError, + readSlackMessageAfterEditError, +}; diff --git a/extensions/slack/src/monitor/provider.allowlist.test.ts b/extensions/slack/src/monitor/provider.allowlist.test.ts new file mode 100644 index 00000000000..2a686f2fcbd --- /dev/null +++ b/extensions/slack/src/monitor/provider.allowlist.test.ts @@ -0,0 +1,26 @@ +import { describe, expect, it } from "vitest"; +import { __testing } from "./provider.js"; + +describe("slack allowlist log formatting", () => { + it("prints channel names alongside ids", () => { + expect( + __testing.formatSlackChannelResolved({ + input: "C0AQXEG6QFJ", + resolved: true, + id: "C0AQXEG6QFJ", + name: "openclawtest", + }), + ).toBe("C0AQXEG6QFJ→openclawtest (id:C0AQXEG6QFJ)"); + }); + + it("prints user names alongside ids", () => { + expect( + __testing.formatSlackUserResolved({ + input: "U090HHQ029J", + resolved: true, + id: "U090HHQ029J", + name: "steipete", + }), + ).toBe("U090HHQ029J→steipete (id:U090HHQ029J)"); + }); +}); diff --git a/extensions/slack/src/monitor/provider.ts b/extensions/slack/src/monitor/provider.ts index 63cf6b76829..83796646a4d 100644 --- a/extensions/slack/src/monitor/provider.ts +++ b/extensions/slack/src/monitor/provider.ts @@ -33,8 +33,8 @@ import { resolveSlackAccount } from "../accounts.js"; import { resolveSlackWebClientOptions } from "../client.js"; import { normalizeSlackWebhookPath, registerSlackHttpHandler } from "../http/index.js"; import { SLACK_TEXT_LIMIT } from "../limits.js"; -import { resolveSlackChannelAllowlist } from "../resolve-channels.js"; -import { resolveSlackUserAllowlist } from "../resolve-users.js"; +import { resolveSlackChannelAllowlist, type SlackChannelResolution } from "../resolve-channels.js"; +import { resolveSlackUserAllowlist, type SlackUserResolution } from "../resolve-users.js"; import { resolveSlackAppToken, resolveSlackBotToken } from "../token.js"; import { normalizeAllowList } from "./allow-list.js"; import { resolveSlackSlashCommandConfig } from "./commands.js"; @@ -169,6 +169,38 @@ function publishSlackDisconnectedStatus( }); } +function formatSlackResolvedLabel(params: { + input: string; + id: string; + name?: string; + extra?: string[]; +}): string { + const extras = params.extra?.filter(Boolean) ?? []; + const suffix = + extras.length > 0 ? ` (id:${params.id}, ${extras.join(", ")})` : ` (id:${params.id})`; + return `${params.input}→${params.name ?? params.id}${suffix}`; +} + +function formatSlackChannelResolved(entry: SlackChannelResolution): string { + const id = entry.id ?? entry.input; + return formatSlackResolvedLabel({ + input: entry.input, + id, + name: entry.name, + extra: entry.archived ? ["archived"] : [], + }); +} + +function formatSlackUserResolved(entry: SlackUserResolution): string { + const id = entry.id ?? entry.input; + return formatSlackResolvedLabel({ + input: entry.input, + id, + name: entry.name, + extra: entry.note ? [entry.note] : [], + }); +} + export async function monitorSlackProvider(opts: MonitorSlackOpts = {}) { const cfg = opts.config ?? loadConfig(); const runtime: RuntimeEnv = opts.runtime ?? createNonExitingRuntime(); @@ -411,7 +443,7 @@ export async function monitorSlackProvider(opts: MonitorSlackOpts = {}) { unresolved.push(entry.input); continue; } - mapping.push(`${entry.input}→${entry.id}${entry.archived ? " (archived)" : ""}`); + mapping.push(formatSlackChannelResolved(entry)); const existing = nextChannels[entry.id] ?? {}; nextChannels[entry.id] = { ...source, ...existing }; } @@ -434,12 +466,7 @@ export async function monitorSlackProvider(opts: MonitorSlackOpts = {}) { const { mapping, unresolved, additions } = buildAllowlistResolutionSummary( resolvedUsers, { - formatResolved: (entry) => { - const note = (entry as { note?: string }).note - ? ` (${(entry as { note?: string }).note})` - : ""; - return `${entry.input}→${entry.id}${note}`; - }, + formatResolved: formatSlackUserResolved, }, ); allowFrom = mergeAllowlist({ existing: allowFrom, additions }); @@ -462,8 +489,12 @@ export async function monitorSlackProvider(opts: MonitorSlackOpts = {}) { token: resolveToken, entries: Array.from(userEntries), }); - const { resolvedMap, mapping, unresolved } = - buildAllowlistResolutionSummary(resolvedUsers); + const { resolvedMap, mapping, unresolved } = buildAllowlistResolutionSummary( + resolvedUsers, + { + formatResolved: formatSlackUserResolved, + }, + ); const nextChannels = patchAllowlistUsersInConfigEntries({ entries: channelsConfig, @@ -591,6 +622,8 @@ export { isNonRecoverableSlackAuthError } from "./reconnect-policy.js"; export const resolveSlackRuntimeGroupPolicy = resolveOpenProviderRuntimeGroupPolicy; export const __testing = { + formatSlackChannelResolved, + formatSlackUserResolved, publishSlackConnectedStatus, publishSlackDisconnectedStatus, resolveSlackRuntimeGroupPolicy: resolveOpenProviderRuntimeGroupPolicy, diff --git a/extensions/slack/src/send.ts b/extensions/slack/src/send.ts index 6cbcf217fc8..87e63138dfa 100644 --- a/extensions/slack/src/send.ts +++ b/extensions/slack/src/send.ts @@ -15,7 +15,7 @@ import type { SlackTokenSource } from "./accounts.js"; import { resolveSlackAccount } from "./accounts.js"; import { buildSlackBlocksFallbackText } from "./blocks-fallback.js"; import { validateSlackBlocksArray } from "./blocks-input.js"; -import { createSlackWebClient } from "./client.js"; +import { createSlackWriteClient } from "./client.js"; import { markdownToSlackMrkdwnChunks } from "./format.js"; import { SLACK_TEXT_LIMIT } from "./limits.js"; import { loadOutboundMediaFromUrl } from "./runtime-api.js"; @@ -327,7 +327,7 @@ export async function sendMessageSlack( fallbackToken: account.botToken, fallbackSource: account.botTokenSource, }); - const client = opts.client ?? createSlackWebClient(token); + const client = opts.client ?? createSlackWriteClient(token); const recipient = parseRecipient(to); const { channelId } = await resolveChannelId(client, recipient, { accountId: account.accountId,