From 47e0bf522fbf5287a18e15d84798e841a17afaf6 Mon Sep 17 00:00:00 2001 From: Vincent Koc Date: Sun, 15 Mar 2026 20:38:18 -0700 Subject: [PATCH] Slack: fix review regressions --- src/channels/plugins/actions/actions.test.ts | 19 ++++++++ .../outbound/slack.sendpayload.test.ts | 44 +++++++++++++++++++ src/channels/plugins/outbound/slack.ts | 36 +++++++++++---- .../message-action-runner.context.test.ts | 26 +++++++++++ src/infra/outbound/message-action-runner.ts | 13 +----- src/plugin-sdk/slack-message-actions.ts | 8 ++-- src/plugins/interactive.ts | 1 + 7 files changed, 124 insertions(+), 23 deletions(-) diff --git a/src/channels/plugins/actions/actions.test.ts b/src/channels/plugins/actions/actions.test.ts index bf75f9997d2..cd33be0a3e2 100644 --- a/src/channels/plugins/actions/actions.test.ts +++ b/src/channels/plugins/actions/actions.test.ts @@ -1292,6 +1292,25 @@ describe("slack actions adapter", () => { } }); + it("does not attach empty blocks to plain media sends", async () => { + handleSlackAction.mockClear(); + + await runSlackAction("send", { + to: "channel:C1", + message: "", + media: "https://example.com/image.png", + }); + + const [params] = handleSlackAction.mock.calls[0] ?? []; + expect(params).toMatchObject({ + action: "sendMessage", + to: "channel:C1", + content: "", + mediaUrl: "https://example.com/image.png", + }); + expect(params).not.toHaveProperty("blocks"); + }); + it("rejects edit when both message and blocks are missing", async () => { const { cfg, actions } = slackHarness(); diff --git a/src/channels/plugins/outbound/slack.sendpayload.test.ts b/src/channels/plugins/outbound/slack.sendpayload.test.ts index 8c6b0806254..ffa86d8ed95 100644 --- a/src/channels/plugins/outbound/slack.sendpayload.test.ts +++ b/src/channels/plugins/outbound/slack.sendpayload.test.ts @@ -100,4 +100,48 @@ describe("slackOutbound sendPayload", () => { await expect(run()).rejects.toThrow(/blocks must be an array/i); expect(sendMock).not.toHaveBeenCalled(); }); + + it("sends media before a separate interactive blocks message", async () => { + const { run, sendMock, to } = createHarness({ + payload: { + text: "Approval required", + mediaUrl: "https://example.com/image.png", + interactive: { + blocks: [ + { + type: "buttons", + buttons: [{ label: "Allow", value: "pluginbind:approval-123:o" }], + }, + ], + }, + }, + sendResults: [{ messageId: "sl-media" }, { messageId: "sl-controls" }], + }); + + const result = await run(); + + expect(sendMock).toHaveBeenCalledTimes(2); + expect(sendMock).toHaveBeenNthCalledWith( + 1, + to, + "", + expect.objectContaining({ + mediaUrl: "https://example.com/image.png", + }), + ); + expect(sendMock.mock.calls[0]?.[2]).not.toHaveProperty("blocks"); + expect(sendMock).toHaveBeenNthCalledWith( + 2, + to, + "Approval required", + expect.objectContaining({ + blocks: [ + expect.objectContaining({ + type: "actions", + }), + ], + }), + ); + expect(result).toMatchObject({ channel: "slack", messageId: "sl-controls" }); + }); }); diff --git a/src/channels/plugins/outbound/slack.ts b/src/channels/plugins/outbound/slack.ts index cd79f627fbd..9785abf503a 100644 --- a/src/channels/plugins/outbound/slack.ts +++ b/src/channels/plugins/outbound/slack.ts @@ -20,6 +20,16 @@ import { const SLACK_MAX_BLOCKS = 50; +function resolveRenderedInteractiveBlocks( + interactive?: InteractiveReply, +): SlackBlock[] | undefined { + if (!interactive) { + return undefined; + } + const blocks = buildSlackInteractiveBlocks(interactive); + return blocks.length > 0 ? blocks : undefined; +} + function resolveSlackSendIdentity(identity?: OutboundIdentity): SlackSendIdentity | undefined { if (!identity) { return undefined; @@ -116,15 +126,15 @@ function resolveSlackBlocks(payload: { interactive?: InteractiveReply; }) { const slackData = payload.channelData?.slack; - const renderedInteractive = buildSlackInteractiveBlocks(payload.interactive); + const renderedInteractive = resolveRenderedInteractiveBlocks(payload.interactive); if (!slackData || typeof slackData !== "object" || Array.isArray(slackData)) { - return renderedInteractive.length > 0 ? renderedInteractive : undefined; + return renderedInteractive; } let existingBlocks: SlackBlock[] | undefined; existingBlocks = parseSlackBlocksInput((slackData as { blocks?: unknown }).blocks) as | SlackBlock[] | undefined; - const mergedBlocks = [...(existingBlocks ?? []), ...renderedInteractive]; + const mergedBlocks = [...(existingBlocks ?? []), ...(renderedInteractive ?? [])]; if (mergedBlocks.length === 0) { return undefined; } @@ -173,17 +183,16 @@ export const slackOutbound: ChannelOutboundAdapter = { identity: ctx.identity, }); } - const lastResult = await sendPayloadMediaSequence({ - text: payload.text ?? "", + await sendPayloadMediaSequence({ + text: "", mediaUrls, - send: async ({ text, mediaUrl, isFirst }) => + send: async ({ text, mediaUrl }) => await sendSlackOutboundMessage({ cfg: ctx.cfg, to: ctx.to, text, mediaUrl, mediaLocalRoots: ctx.mediaLocalRoots, - blocks: isFirst ? blocks : undefined, accountId: ctx.accountId, deps: ctx.deps, replyToId: ctx.replyToId, @@ -191,7 +200,18 @@ export const slackOutbound: ChannelOutboundAdapter = { identity: ctx.identity, }), }); - return lastResult ?? { channel: "slack", messageId: "" }; + return await sendSlackOutboundMessage({ + cfg: ctx.cfg, + to: ctx.to, + text: payload.text ?? "", + mediaLocalRoots: ctx.mediaLocalRoots, + blocks, + accountId: ctx.accountId, + deps: ctx.deps, + replyToId: ctx.replyToId, + threadId: ctx.threadId, + identity: ctx.identity, + }); }, sendText: async ({ cfg, to, text, accountId, deps, replyToId, threadId, identity }) => { return await sendSlackOutboundMessage({ diff --git a/src/infra/outbound/message-action-runner.context.test.ts b/src/infra/outbound/message-action-runner.context.test.ts index 185ff2bf648..c6a8388af57 100644 --- a/src/infra/outbound/message-action-runner.context.test.ts +++ b/src/infra/outbound/message-action-runner.context.test.ts @@ -186,6 +186,32 @@ describe("runMessageAction context isolation", () => { ).rejects.toThrow(/message required/i); }); + it("requires message when send only includes shared interactive payloads", async () => { + await expect( + runDrySend({ + cfg: { + channels: { + telegram: { + botToken: "telegram-test", + }, + }, + } as OpenClawConfig, + actionParams: { + channel: "telegram", + target: "123456", + interactive: { + blocks: [ + { + type: "buttons", + buttons: [{ label: "Approve", value: "approve" }], + }, + ], + }, + }, + }), + ).rejects.toThrow(/message required/i); + }); + it.each([ { name: "structured poll params", diff --git a/src/infra/outbound/message-action-runner.ts b/src/infra/outbound/message-action-runner.ts index 74f5839f3b4..b3033c5cf21 100644 --- a/src/infra/outbound/message-action-runner.ts +++ b/src/infra/outbound/message-action-runner.ts @@ -13,7 +13,6 @@ import type { ChannelThreadingToolContext, } from "../../channels/plugins/types.js"; import type { OpenClawConfig } from "../../config/config.js"; -import { normalizeInteractiveReply } from "../../interactive/payload.js"; import { getAgentScopedMediaLocalRoots } from "../../media/local-roots.js"; import { hasPollCreationParams, resolveTelegramPollVisibility } from "../../poll-params.js"; import { resolvePollMaxSelections } from "../../polls.js"; @@ -406,11 +405,10 @@ async function handleSendAction(ctx: ResolvedActionContext): Promise