From c76288bdf13a50195504c4685c373e2c8766b61e Mon Sep 17 00:00:00 2001 From: Tanwa Arpornthip <72845369+CommanderCrowCode@users.noreply.github.com> Date: Sat, 14 Feb 2026 20:16:02 +0700 Subject: [PATCH] fix(slack): download all files in multi-image messages (#15447) * fix(slack): download all files in multi-image messages resolveSlackMedia() previously returned after downloading the first file, causing multi-image Slack messages to lose all but the first attachment. This changes the function to collect all successfully downloaded files into an array, matching the pattern already used by Telegram, Line, Discord, and iMessage adapters. The prepare handler now populates MediaPaths, MediaUrls, and MediaTypes arrays so downstream media processing (vision, sandbox staging, media notes) works correctly with multiple attachments. Fixes #11892, #7536 Co-Authored-By: Claude Opus 4.6 * fix(slack): preserve MediaTypes index alignment with MediaPaths/MediaUrls The filter(Boolean) on MediaTypes removed entries with undefined contentType, shrinking the array and breaking index correlation with MediaPaths and MediaUrls. Downstream code (media-note.ts, attachments.ts) requires these arrays to have equal lengths for correct per-attachment MIME type lookup. Replace filter(Boolean) with a nullish coalescing fallback to "application/octet-stream". Co-Authored-By: Claude Opus 4.6 * fix(slack): align MediaType fallback and tests (#15447) (thanks @CommanderCrowCode) * fix: unblock plugin-sdk account-id typing (#15447) --------- Co-authored-by: Claude Opus 4.6 Co-authored-by: Peter Steinberger --- extensions/feishu/package.json | 3 ++ pnpm-lock.yaml | 24 +++++-------- scripts/write-plugin-sdk-entry-dts.ts | 18 ++++++---- src/media/server.test.ts | 12 +++---- src/slack/monitor/media.test.ts | 38 +++++++++++++++++++- src/slack/monitor/media.ts | 31 ++++++++++------ src/slack/monitor/message-handler/prepare.ts | 24 ++++++++++--- 7 files changed, 107 insertions(+), 43 deletions(-) diff --git a/extensions/feishu/package.json b/extensions/feishu/package.json index 72e49b72f6..25cd2f0d45 100644 --- a/extensions/feishu/package.json +++ b/extensions/feishu/package.json @@ -8,6 +8,9 @@ "@sinclair/typebox": "0.34.48", "zod": "^4.3.6" }, + "devDependencies": { + "openclaw": "workspace:*" + }, "openclaw": { "extensions": [ "./index.ts" diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index c85cf9c574..714bfc4147 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -312,6 +312,10 @@ importers: zod: specifier: ^4.3.6 version: 4.3.6 + devDependencies: + openclaw: + specifier: workspace:* + version: link:../.. extensions/google-antigravity-auth: devDependencies: @@ -6845,7 +6849,7 @@ snapshots: '@larksuiteoapi/node-sdk@1.59.0': dependencies: - axios: 1.13.5 + axios: 1.13.5(debug@4.4.3) lodash.identity: 3.0.0 lodash.merge: 4.6.2 lodash.pickby: 4.6.0 @@ -6861,7 +6865,7 @@ snapshots: dependencies: '@types/node': 24.10.13 optionalDependencies: - axios: 1.13.5 + axios: 1.13.5(debug@4.4.3) transitivePeerDependencies: - debug @@ -7064,7 +7068,7 @@ snapshots: '@azure/core-auth': 1.10.1 '@azure/msal-node': 3.8.7 '@microsoft/agents-activity': 1.2.3 - axios: 1.13.5 + axios: 1.13.5(debug@4.4.3) jsonwebtoken: 9.0.3 jwks-rsa: 3.2.2 object-path: 0.11.8 @@ -8011,7 +8015,7 @@ snapshots: '@slack/types': 2.20.0 '@slack/web-api': 7.14.0 '@types/express': 5.0.6 - axios: 1.13.5 + axios: 1.13.5(debug@4.4.3) express: 5.2.1 path-to-regexp: 8.3.0 raw-body: 3.0.2 @@ -8057,7 +8061,7 @@ snapshots: '@slack/types': 2.20.0 '@types/node': 25.2.3 '@types/retry': 0.12.0 - axios: 1.13.5 + axios: 1.13.5(debug@4.4.3) eventemitter3: 5.0.4 form-data: 2.5.4 is-electron: 2.2.2 @@ -8951,14 +8955,6 @@ snapshots: aws4@1.13.2: {} - axios@1.13.5: - dependencies: - follow-redirects: 1.15.11 - form-data: 2.5.4 - proxy-from-env: 1.1.0 - transitivePeerDependencies: - - debug - axios@1.13.5(debug@4.4.3): dependencies: follow-redirects: 1.15.11(debug@4.4.3) @@ -9542,8 +9538,6 @@ snapshots: flatbuffers@24.12.23: {} - follow-redirects@1.15.11: {} - follow-redirects@1.15.11(debug@4.4.3): optionalDependencies: debug: 4.4.3 diff --git a/scripts/write-plugin-sdk-entry-dts.ts b/scripts/write-plugin-sdk-entry-dts.ts index 25d0631590..674f89ed13 100644 --- a/scripts/write-plugin-sdk-entry-dts.ts +++ b/scripts/write-plugin-sdk-entry-dts.ts @@ -1,9 +1,15 @@ import fs from "node:fs"; import path from "node:path"; -// `tsc` emits the entry d.ts at `dist/plugin-sdk/plugin-sdk/index.d.ts` because -// the source lives at `src/plugin-sdk/index.ts` and `rootDir` is `src/`. -// Keep a stable `dist/plugin-sdk/index.d.ts` alongside `index.js` for TS users. -const out = path.join(process.cwd(), "dist/plugin-sdk/index.d.ts"); -fs.mkdirSync(path.dirname(out), { recursive: true }); -fs.writeFileSync(out, 'export * from "./plugin-sdk/index";\n', "utf8"); +// `tsc` emits declarations under `dist/plugin-sdk/plugin-sdk/*` because the source lives +// at `src/plugin-sdk/*` and `rootDir` is `src/`. +// +// Our package export map points subpath `types` at `dist/plugin-sdk/.d.ts`, so we +// generate stable entry d.ts files that re-export the real declarations. +const entrypoints = ["index", "account-id"] as const; +for (const entry of entrypoints) { + const out = path.join(process.cwd(), `dist/plugin-sdk/${entry}.d.ts`); + fs.mkdirSync(path.dirname(out), { recursive: true }); + // NodeNext: reference the runtime specifier with `.js`, TS will map it to `.d.ts`. + fs.writeFileSync(out, `export * from "./plugin-sdk/${entry}.js";\n`, "utf8"); +} diff --git a/src/media/server.test.ts b/src/media/server.test.ts index 6273f1d8a7..fda4c0486a 100644 --- a/src/media/server.test.ts +++ b/src/media/server.test.ts @@ -46,7 +46,7 @@ describe("media server", () => { await fs.writeFile(file, "hello"); const server = await startMediaServer(0, 5_000); const port = (server.address() as AddressInfo).port; - const res = await fetch(`http://localhost:${port}/media/file1`); + const res = await fetch(`http://127.0.0.1:${port}/media/file1`); expect(res.status).toBe(200); expect(await res.text()).toBe("hello"); await waitForFileRemoval(file); @@ -60,7 +60,7 @@ describe("media server", () => { await fs.utimes(file, past / 1000, past / 1000); const server = await startMediaServer(0, 1_000); const port = (server.address() as AddressInfo).port; - const res = await fetch(`http://localhost:${port}/media/old`); + const res = await fetch(`http://127.0.0.1:${port}/media/old`); expect(res.status).toBe(410); await expect(fs.stat(file)).rejects.toThrow(); await new Promise((r) => server.close(r)); @@ -70,7 +70,7 @@ describe("media server", () => { const server = await startMediaServer(0, 5_000); const port = (server.address() as AddressInfo).port; // URL-encoded "../" to bypass client-side path normalization - const res = await fetch(`http://localhost:${port}/media/%2e%2e%2fpackage.json`); + const res = await fetch(`http://127.0.0.1:${port}/media/%2e%2e%2fpackage.json`); expect(res.status).toBe(400); expect(await res.text()).toBe("invalid path"); await new Promise((r) => server.close(r)); @@ -83,7 +83,7 @@ describe("media server", () => { const server = await startMediaServer(0, 5_000); const port = (server.address() as AddressInfo).port; - const res = await fetch(`http://localhost:${port}/media/link-out`); + const res = await fetch(`http://127.0.0.1:${port}/media/link-out`); expect(res.status).toBe(400); expect(await res.text()).toBe("invalid path"); await new Promise((r) => server.close(r)); @@ -94,7 +94,7 @@ describe("media server", () => { await fs.writeFile(file, "hello"); const server = await startMediaServer(0, 5_000); const port = (server.address() as AddressInfo).port; - const res = await fetch(`http://localhost:${port}/media/invalid%20id`); + const res = await fetch(`http://127.0.0.1:${port}/media/invalid%20id`); expect(res.status).toBe(400); expect(await res.text()).toBe("invalid path"); await new Promise((r) => server.close(r)); @@ -106,7 +106,7 @@ describe("media server", () => { await fs.truncate(file, MEDIA_MAX_BYTES + 1); const server = await startMediaServer(0, 5_000); const port = (server.address() as AddressInfo).port; - const res = await fetch(`http://localhost:${port}/media/big`); + const res = await fetch(`http://127.0.0.1:${port}/media/big`); expect(res.status).toBe(413); expect(await res.text()).toBe("too large"); await new Promise((r) => server.close(r)); diff --git a/src/slack/monitor/media.test.ts b/src/slack/monitor/media.test.ts index dd1b3b41ac..8d38d17c47 100644 --- a/src/slack/monitor/media.test.ts +++ b/src/slack/monitor/media.test.ts @@ -267,6 +267,7 @@ describe("resolveSlackMedia", () => { }); expect(result).not.toBeNull(); + expect(result).toHaveLength(1); // saveMediaBuffer should receive the overridden audio/mp4 expect(saveMediaBufferMock).toHaveBeenCalledWith( expect.any(Buffer), @@ -276,7 +277,7 @@ describe("resolveSlackMedia", () => { ); // Returned contentType must be the overridden value, not the // re-detected video/mp4 from saveMediaBuffer - expect(result!.contentType).toBe("audio/mp4"); + expect(result![0]?.contentType).toBe("audio/mp4"); }); it("preserves original MIME for non-voice Slack files", async () => { @@ -304,12 +305,14 @@ describe("resolveSlackMedia", () => { }); expect(result).not.toBeNull(); + expect(result).toHaveLength(1); expect(saveMediaBufferMock).toHaveBeenCalledWith( expect.any(Buffer), "video/mp4", "inbound", 16 * 1024 * 1024, ); + expect(result![0]?.contentType).toBe("video/mp4"); }); it("falls through to next file when first file returns error", async () => { @@ -338,8 +341,41 @@ describe("resolveSlackMedia", () => { }); expect(result).not.toBeNull(); + expect(result).toHaveLength(1); expect(mockFetch).toHaveBeenCalledTimes(2); }); + + it("returns all successfully downloaded files as an array", async () => { + vi.spyOn(mediaStore, "saveMediaBuffer") + .mockResolvedValueOnce({ path: "/tmp/a.jpg", contentType: "image/jpeg" }) + .mockResolvedValueOnce({ path: "/tmp/b.png", contentType: "image/png" }); + + const responseA = new Response(Buffer.from("image a"), { + status: 200, + headers: { "content-type": "image/jpeg" }, + }); + const responseB = new Response(Buffer.from("image b"), { + status: 200, + headers: { "content-type": "image/png" }, + }); + + mockFetch.mockResolvedValueOnce(responseA).mockResolvedValueOnce(responseB); + + const result = await resolveSlackMedia({ + files: [ + { url_private: "https://files.slack.com/a.jpg", name: "a.jpg" }, + { url_private: "https://files.slack.com/b.png", name: "b.png" }, + ], + token: "xoxb-test-token", + maxBytes: 1024 * 1024, + }); + + expect(result).toHaveLength(2); + expect(result![0].path).toBe("/tmp/a.jpg"); + expect(result![0].placeholder).toBe("[Slack file: a.jpg]"); + expect(result![1].path).toBe("/tmp/b.png"); + expect(result![1].placeholder).toBe("[Slack file: b.png]"); + }); }); describe("resolveSlackThreadHistory", () => { diff --git a/src/slack/monitor/media.ts b/src/slack/monitor/media.ts index e634a30dcb..21fcda5092 100644 --- a/src/slack/monitor/media.ts +++ b/src/slack/monitor/media.ts @@ -132,17 +132,28 @@ function resolveSlackMediaMimetype( return mime; } +export type SlackMediaResult = { + path: string; + contentType?: string; + placeholder: string; +}; + +const MAX_SLACK_MEDIA_FILES = 8; + +/** + * Downloads all files attached to a Slack message and returns them as an array. + * Returns `null` when no files could be downloaded. + */ export async function resolveSlackMedia(params: { files?: SlackFile[]; token: string; maxBytes: number; -}): Promise<{ - path: string; - contentType?: string; - placeholder: string; -} | null> { +}): Promise { const files = params.files ?? []; - for (const file of files) { + const limitedFiles = + files.length > MAX_SLACK_MEDIA_FILES ? files.slice(0, MAX_SLACK_MEDIA_FILES) : files; + const results: SlackMediaResult[] = []; + for (const file of limitedFiles) { const url = file.url_private_download ?? file.url_private; if (!url) { continue; @@ -169,16 +180,16 @@ export async function resolveSlackMedia(params: { params.maxBytes, ); const label = fetched.fileName ?? file.name; - return { + results.push({ path: saved.path, contentType: effectiveMime ?? saved.contentType, placeholder: label ? `[Slack file: ${label}]` : "[Slack file]", - }; + }); } catch { - // Ignore download failures and fall through to the next file. + // Ignore download failures and try the next file. } } - return null; + return results.length > 0 ? results : null; } export type SlackThreadStarter = { diff --git a/src/slack/monitor/message-handler/prepare.ts b/src/slack/monitor/message-handler/prepare.ts index 55e5f2b08d..7c24f1e052 100644 --- a/src/slack/monitor/message-handler/prepare.ts +++ b/src/slack/monitor/message-handler/prepare.ts @@ -342,7 +342,8 @@ export async function prepareSlackMessage(params: { token: ctx.botToken, maxBytes: ctx.mediaMaxBytes, }); - const rawBody = (message.text ?? "").trim() || media?.placeholder || ""; + const mediaPlaceholder = media ? media.map((m) => m.placeholder).join(" ") : undefined; + const rawBody = (message.text ?? "").trim() || mediaPlaceholder || ""; if (!rawBody) { return null; } @@ -488,8 +489,9 @@ export async function prepareSlackMessage(params: { maxBytes: ctx.mediaMaxBytes, }); if (threadStarterMedia) { + const starterPlaceholders = threadStarterMedia.map((m) => m.placeholder).join(", "); logVerbose( - `slack: hydrated thread starter file ${threadStarterMedia.placeholder} from root message`, + `slack: hydrated thread starter file ${starterPlaceholders} from root message`, ); } } @@ -558,6 +560,10 @@ export async function prepareSlackMessage(params: { // Use thread starter media if current message has none const effectiveMedia = media ?? threadStarterMedia; + const firstMedia = effectiveMedia?.[0]; + const firstMediaType = firstMedia + ? (firstMedia.contentType ?? "application/octet-stream") + : undefined; const inboundHistory = isRoomish && ctx.historyLimit > 0 @@ -599,9 +605,17 @@ export async function prepareSlackMessage(params: { ThreadLabel: threadLabel, Timestamp: message.ts ? Math.round(Number(message.ts) * 1000) : undefined, WasMentioned: isRoomish ? effectiveWasMentioned : undefined, - MediaPath: effectiveMedia?.path, - MediaType: effectiveMedia?.contentType, - MediaUrl: effectiveMedia?.path, + MediaPath: firstMedia?.path, + MediaType: firstMediaType, + MediaUrl: firstMedia?.path, + MediaPaths: + effectiveMedia && effectiveMedia.length > 0 ? effectiveMedia.map((m) => m.path) : undefined, + MediaUrls: + effectiveMedia && effectiveMedia.length > 0 ? effectiveMedia.map((m) => m.path) : undefined, + MediaTypes: + effectiveMedia && effectiveMedia.length > 0 + ? effectiveMedia.map((m) => m.contentType ?? "application/octet-stream") + : undefined, CommandAuthorized: commandAuthorized, OriginatingChannel: "slack" as const, OriginatingTo: slackTo,