From 8ddd238d1cd4f3bfa49bb84692000cefc3927bf7 Mon Sep 17 00:00:00 2001 From: Chane Date: Fri, 20 Mar 2026 19:48:49 -0700 Subject: [PATCH] Handle Slack file-only thread starters for media inheritance --- extensions/slack/src/monitor/media.test.ts | 32 ++++ extensions/slack/src/monitor/media.ts | 3 +- .../message-handler/prepare-thread-context.ts | 9 +- .../monitor/message-handler/prepare.test.ts | 172 ++++++++++++++++++ extensions/slack/src/monitor/monitor.test.ts | 43 ++++- 5 files changed, 253 insertions(+), 6 deletions(-) diff --git a/extensions/slack/src/monitor/media.test.ts b/extensions/slack/src/monitor/media.test.ts index 9ac0bc0eeb1..3600a3f91b4 100644 --- a/extensions/slack/src/monitor/media.test.ts +++ b/extensions/slack/src/monitor/media.test.ts @@ -13,6 +13,7 @@ import { resolveSlackAttachmentContent, resolveSlackMedia, resolveSlackThreadHistory, + resolveSlackThreadStarter, } from "./media.js"; // Store original fetch @@ -657,6 +658,37 @@ describe("resolveSlackAttachmentContent", () => { }); }); +describe("resolveSlackThreadStarter", () => { + it("returns file-only root messages so thread replies can hydrate inherited media", async () => { + const replies = vi.fn().mockResolvedValueOnce({ + messages: [ + { + text: " ", + user: "U1", + ts: "1.000", + files: [{ id: "F1", name: "thread-image.png" }], + }, + ], + }); + const client = { + conversations: { replies }, + } as unknown as Parameters[0]["client"]; + + const result = await resolveSlackThreadStarter({ + channelId: "C1", + threadTs: "1.000", + client, + }); + + expect(result).toEqual({ + text: "", + userId: "U1", + ts: "1.000", + files: [{ id: "F1", name: "thread-image.png" }], + }); + }); +}); + describe("resolveSlackThreadHistory", () => { afterEach(() => { vi.restoreAllMocks(); diff --git a/extensions/slack/src/monitor/media.ts b/extensions/slack/src/monitor/media.ts index ef574a7381c..58cb6071b45 100644 --- a/extensions/slack/src/monitor/media.ts +++ b/extensions/slack/src/monitor/media.ts @@ -394,7 +394,8 @@ export async function resolveSlackThreadStarter(params: { })) as { messages?: Array<{ text?: string; user?: string; ts?: string; files?: SlackFile[] }> }; const message = response?.messages?.[0]; const text = (message?.text ?? "").trim(); - if (!message || !text) { + const hasFiles = Boolean(message?.files?.length); + if (!message || (!text && !hasFiles)) { return null; } const starter: SlackThreadStarter = { diff --git a/extensions/slack/src/monitor/message-handler/prepare-thread-context.ts b/extensions/slack/src/monitor/message-handler/prepare-thread-context.ts index 5d4020f1b46..1081fc1b8b0 100644 --- a/extensions/slack/src/monitor/message-handler/prepare-thread-context.ts +++ b/extensions/slack/src/monitor/message-handler/prepare-thread-context.ts @@ -51,9 +51,12 @@ export async function resolveSlackThreadContextData(params: { } const starter = params.threadStarter; - if (starter?.text) { - threadStarterBody = starter.text; - const snippet = starter.text.replace(/\s+/g, " ").slice(0, 80); + if (starter) { + const starterText = starter.text.trim(); + if (starterText) { + threadStarterBody = starterText; + } + const snippet = starterText.replace(/\s+/g, " ").slice(0, 80); threadLabel = `Slack thread ${params.roomLabel}${snippet ? `: ${snippet}` : ""}`; if (!params.effectiveDirectMedia && starter.files && starter.files.length > 0) { threadStarterMedia = await resolveSlackMedia({ diff --git a/extensions/slack/src/monitor/message-handler/prepare.test.ts b/extensions/slack/src/monitor/message-handler/prepare.test.ts index a57614afaeb..9eb9f4e1a5b 100644 --- a/extensions/slack/src/monitor/message-handler/prepare.test.ts +++ b/extensions/slack/src/monitor/message-handler/prepare.test.ts @@ -10,6 +10,7 @@ import { resolveThreadSessionKeys } from "../../../../../src/routing/session-key import type { ResolvedSlackAccount } from "../../accounts.js"; import type { SlackMessageEvent } from "../../types.js"; import type { SlackMonitorContext } from "../context.js"; +import * as slackMedia from "../media.js"; import { prepareSlackMessage } from "./prepare.js"; import { createInboundSlackTestContext, createSlackTestAccount } from "./prepare.test-helpers.js"; @@ -226,6 +227,49 @@ describe("slack prepareSlackMessage inbound contract", () => { expect(prepared!.ctxPayload.RawBody).toContain("[Forwarded message from Bob]\nForwarded hello"); }); + it("populates finalized media fields for forwarded shared attachment media", async () => { + const attachmentSpy = vi + .spyOn(slackMedia, "resolveSlackAttachmentContent") + .mockResolvedValueOnce({ + text: "[Forwarded message from Bob]\nForwarded hello", + media: [ + { + path: "/tmp/forwarded.jpg", + contentType: "image/jpeg", + placeholder: "[Forwarded image: forwarded.jpg]", + }, + { + path: "/tmp/forwarded-file.png", + contentType: "image/png", + placeholder: "[Slack file: forwarded-file.png]", + }, + ], + }); + + try { + const prepared = await prepareWithDefaultCtx( + createSlackMessage({ + text: "", + attachments: [{ is_share: true, author_name: "Bob", text: "Forwarded hello" }], + }), + ); + + expect(prepared).toBeTruthy(); + expect(prepared!.ctxPayload.RawBody).toContain( + "[Forwarded message from Bob]\nForwarded hello", + ); + expect(prepared!.ctxPayload.MediaPath).toBe("/tmp/forwarded.jpg"); + expect(prepared!.ctxPayload.MediaType).toBe("image/jpeg"); + expect(prepared!.ctxPayload.MediaPaths).toEqual([ + "/tmp/forwarded.jpg", + "/tmp/forwarded-file.png", + ]); + expect(prepared!.ctxPayload.MediaTypes).toEqual(["image/jpeg", "image/png"]); + } finally { + attachmentSpy.mockRestore(); + } + }); + it("ignores non-forward attachments when no direct text/files are present", async () => { const prepared = await prepareWithDefaultCtx( createSlackMessage({ @@ -457,6 +501,134 @@ describe("slack prepareSlackMessage inbound contract", () => { expect(replies).toHaveBeenCalledTimes(2); }); + it("populates finalized media fields for direct Slack media", async () => { + const mediaSpy = vi.spyOn(slackMedia, "resolveSlackMedia").mockResolvedValueOnce([ + { + path: "/tmp/direct-a.png", + contentType: "image/png", + placeholder: "[Slack file: direct-a.png]", + }, + { + path: "/tmp/direct-b.jpg", + contentType: "image/jpeg", + placeholder: "[Slack file: direct-b.jpg]", + }, + ]); + + try { + const prepared = await prepareWithDefaultCtx( + createSlackMessage({ + text: "please inspect", + files: [ + { id: "F1", name: "direct-a.png", url_private: "https://files.slack.com/a" }, + { id: "F2", name: "direct-b.jpg", url_private: "https://files.slack.com/b" }, + ], + }), + ); + + expect(prepared).toBeTruthy(); + expect(prepared!.ctxPayload.MediaPath).toBe("/tmp/direct-a.png"); + expect(prepared!.ctxPayload.MediaType).toBe("image/png"); + expect(prepared!.ctxPayload.MediaPaths).toEqual(["/tmp/direct-a.png", "/tmp/direct-b.jpg"]); + expect(prepared!.ctxPayload.MediaTypes).toEqual(["image/png", "image/jpeg"]); + } finally { + mediaSpy.mockRestore(); + } + }); + + it("inherits thread-root media into finalized media fields when reply has no direct media", async () => { + const { storePath } = makeTmpStorePath(); + const replies = vi.fn(); + const slackCtx = createThreadSlackCtx({ + cfg: { + session: { store: storePath }, + channels: { slack: { enabled: true, replyToMode: "all", groupPolicy: "open" } }, + } as OpenClawConfig, + replies, + }); + slackCtx.resolveUserName = async (id: string) => ({ name: id === "U1" ? "Alice" : "Bob" }); + slackCtx.resolveChannelName = async () => ({ name: "general", type: "channel" }); + + const starterSpy = vi.spyOn(slackMedia, "resolveSlackThreadStarter").mockResolvedValue({ + text: "", + userId: "U2", + ts: "100.000", + files: [{ id: "F1", name: "root.png", url_private: "https://files.slack.com/root" }], + }); + const mediaSpy = vi + .spyOn(slackMedia, "resolveSlackMedia") + .mockResolvedValueOnce(null) + .mockResolvedValueOnce([ + { + path: "/tmp/root.png", + contentType: "image/png", + placeholder: "[Slack file: root.png]", + }, + ]); + + try { + const prepared = await prepareThreadMessage(slackCtx, { + text: "what is in the image above?", + ts: "101.000", + }); + + expect(prepared).toBeTruthy(); + expect(prepared!.ctxPayload.MediaPath).toBe("/tmp/root.png"); + expect(prepared!.ctxPayload.MediaType).toBe("image/png"); + expect(prepared!.ctxPayload.MediaPaths).toEqual(["/tmp/root.png"]); + expect(prepared!.ctxPayload.MediaTypes).toEqual(["image/png"]); + expect(prepared!.ctxPayload.ThreadStarterBody).toBeUndefined(); + expect(starterSpy).toHaveBeenCalledTimes(1); + } finally { + starterSpy.mockRestore(); + mediaSpy.mockRestore(); + } + }); + + it("preserves ordered multi-image arrays in finalized media fields", async () => { + const mediaSpy = vi.spyOn(slackMedia, "resolveSlackMedia").mockResolvedValueOnce([ + { + path: "/tmp/01.png", + contentType: "image/png", + placeholder: "[Slack file: 01.png]", + }, + { + path: "/tmp/02.webp", + contentType: "image/webp", + placeholder: "[Slack file: 02.webp]", + }, + { + path: "/tmp/03.jpg", + contentType: "image/jpeg", + placeholder: "[Slack file: 03.jpg]", + }, + ]); + + try { + const prepared = await prepareWithDefaultCtx( + createSlackMessage({ + text: "compare these", + files: [ + { id: "F1", name: "01.png", url_private: "https://files.slack.com/1" }, + { id: "F2", name: "02.webp", url_private: "https://files.slack.com/2" }, + { id: "F3", name: "03.jpg", url_private: "https://files.slack.com/3" }, + ], + }), + ); + + expect(prepared).toBeTruthy(); + expect(prepared!.ctxPayload.MediaPath).toBe("/tmp/01.png"); + expect(prepared!.ctxPayload.MediaPaths).toEqual([ + "/tmp/01.png", + "/tmp/02.webp", + "/tmp/03.jpg", + ]); + expect(prepared!.ctxPayload.MediaTypes).toEqual(["image/png", "image/webp", "image/jpeg"]); + } finally { + mediaSpy.mockRestore(); + } + }); + it("skips loading thread history when thread session already exists in store (bloat fix)", async () => { const { storePath } = makeTmpStorePath(); const cfg = { diff --git a/extensions/slack/src/monitor/monitor.test.ts b/extensions/slack/src/monitor/monitor.test.ts index 6741700ba5c..81e609271d2 100644 --- a/extensions/slack/src/monitor/monitor.test.ts +++ b/extensions/slack/src/monitor/monitor.test.ts @@ -149,9 +149,15 @@ const baseParams = () => ({ }); type ThreadStarterClient = Parameters[0]["client"]; +type ThreadStarterReplyMessage = { + text?: string; + user?: string; + ts?: string; + files?: SlackMessageEvent["files"]; +}; function createThreadStarterRepliesClient( - response: { messages?: Array<{ text?: string; user?: string; ts?: string }> } = { + response: { messages?: ThreadStarterReplyMessage[] } = { messages: [{ text: "root message", user: "U1", ts: "1000.1" }], }, ): { replies: ReturnType; client: ThreadStarterClient } { @@ -351,7 +357,7 @@ describe("resolveSlackThreadStarter cache", () => { expect(replies).toHaveBeenCalledTimes(2); }); - it("does not cache empty starter text", async () => { + it("does not cache empty starter messages without files", async () => { const { replies, client } = createThreadStarterRepliesClient({ messages: [{ text: " ", user: "U1", ts: "1000.1" }], }); @@ -372,6 +378,39 @@ describe("resolveSlackThreadStarter cache", () => { expect(replies).toHaveBeenCalledTimes(2); }); + it("caches file-only thread starters so replies can inherit parent media", async () => { + const { replies, client } = createThreadStarterRepliesClient({ + messages: [ + { + text: " ", + user: "U1", + ts: "1000.1", + files: [{ id: "F1", name: "thread-image.png" }], + }, + ], + }); + + const first = await resolveSlackThreadStarter({ + channelId: "C1", + threadTs: "1000.1", + client, + }); + const second = await resolveSlackThreadStarter({ + channelId: "C1", + threadTs: "1000.1", + client, + }); + + expect(first).toEqual({ + text: "", + userId: "U1", + ts: "1000.1", + files: [{ id: "F1", name: "thread-image.png" }], + }); + expect(second).toEqual(first); + expect(replies).toHaveBeenCalledTimes(1); + }); + it("evicts oldest entries once cache exceeds bounded size", async () => { const { replies, client } = createThreadStarterRepliesClient();