Skip to content

Commit c76288b

Browse files
CommanderCrowCodeclaudesteipete
authored
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 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> Co-authored-by: Peter Steinberger <[email protected]>
1 parent ef70a55 commit c76288b

File tree

7 files changed

+107
-43
lines changed

7 files changed

+107
-43
lines changed

‎extensions/feishu/package.json‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,9 @@
88
"@sinclair/typebox": "0.34.48",
99
"zod": "^4.3.6"
1010
},
11+
"devDependencies": {
12+
"openclaw": "workspace:*"
13+
},
1114
"openclaw": {
1215
"extensions": [
1316
"./index.ts"

‎pnpm-lock.yaml‎

Lines changed: 9 additions & 15 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎scripts/write-plugin-sdk-entry-dts.ts‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,15 @@
11
import fs from "node:fs";
22
import path from "node:path";
33

4-
// `tsc` emits the entry d.ts at `dist/plugin-sdk/plugin-sdk/index.d.ts` because
5-
// the source lives at `src/plugin-sdk/index.ts` and `rootDir` is `src/`.
6-
// Keep a stable `dist/plugin-sdk/index.d.ts` alongside `index.js` for TS users.
7-
const out = path.join(process.cwd(), "dist/plugin-sdk/index.d.ts");
8-
fs.mkdirSync(path.dirname(out), { recursive: true });
9-
fs.writeFileSync(out, 'export * from "./plugin-sdk/index";\n', "utf8");
4+
// `tsc` emits declarations under `dist/plugin-sdk/plugin-sdk/*` because the source lives
5+
// at `src/plugin-sdk/*` and `rootDir` is `src/`.
6+
//
7+
// Our package export map points subpath `types` at `dist/plugin-sdk/<entry>.d.ts`, so we
8+
// generate stable entry d.ts files that re-export the real declarations.
9+
const entrypoints = ["index", "account-id"] as const;
10+
for (const entry of entrypoints) {
11+
const out = path.join(process.cwd(), `dist/plugin-sdk/${entry}.d.ts`);
12+
fs.mkdirSync(path.dirname(out), { recursive: true });
13+
// NodeNext: reference the runtime specifier with `.js`, TS will map it to `.d.ts`.
14+
fs.writeFileSync(out, `export * from "./plugin-sdk/${entry}.js";\n`, "utf8");
15+
}

‎src/media/server.test.ts‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ describe("media server", () => {
4646
await fs.writeFile(file, "hello");
4747
const server = await startMediaServer(0, 5_000);
4848
const port = (server.address() as AddressInfo).port;
49-
const res = await fetch(`http://localhost:${port}/media/file1`);
49+
const res = await fetch(`http://127.0.0.1:${port}/media/file1`);
5050
expect(res.status).toBe(200);
5151
expect(await res.text()).toBe("hello");
5252
await waitForFileRemoval(file);
@@ -60,7 +60,7 @@ describe("media server", () => {
6060
await fs.utimes(file, past / 1000, past / 1000);
6161
const server = await startMediaServer(0, 1_000);
6262
const port = (server.address() as AddressInfo).port;
63-
const res = await fetch(`http://localhost:${port}/media/old`);
63+
const res = await fetch(`http://127.0.0.1:${port}/media/old`);
6464
expect(res.status).toBe(410);
6565
await expect(fs.stat(file)).rejects.toThrow();
6666
await new Promise((r) => server.close(r));
@@ -70,7 +70,7 @@ describe("media server", () => {
7070
const server = await startMediaServer(0, 5_000);
7171
const port = (server.address() as AddressInfo).port;
7272
// URL-encoded "../" to bypass client-side path normalization
73-
const res = await fetch(`http://localhost:${port}/media/%2e%2e%2fpackage.json`);
73+
const res = await fetch(`http://127.0.0.1:${port}/media/%2e%2e%2fpackage.json`);
7474
expect(res.status).toBe(400);
7575
expect(await res.text()).toBe("invalid path");
7676
await new Promise((r) => server.close(r));
@@ -83,7 +83,7 @@ describe("media server", () => {
8383

8484
const server = await startMediaServer(0, 5_000);
8585
const port = (server.address() as AddressInfo).port;
86-
const res = await fetch(`http://localhost:${port}/media/link-out`);
86+
const res = await fetch(`http://127.0.0.1:${port}/media/link-out`);
8787
expect(res.status).toBe(400);
8888
expect(await res.text()).toBe("invalid path");
8989
await new Promise((r) => server.close(r));
@@ -94,7 +94,7 @@ describe("media server", () => {
9494
await fs.writeFile(file, "hello");
9595
const server = await startMediaServer(0, 5_000);
9696
const port = (server.address() as AddressInfo).port;
97-
const res = await fetch(`http://localhost:${port}/media/invalid%20id`);
97+
const res = await fetch(`http://127.0.0.1:${port}/media/invalid%20id`);
9898
expect(res.status).toBe(400);
9999
expect(await res.text()).toBe("invalid path");
100100
await new Promise((r) => server.close(r));
@@ -106,7 +106,7 @@ describe("media server", () => {
106106
await fs.truncate(file, MEDIA_MAX_BYTES + 1);
107107
const server = await startMediaServer(0, 5_000);
108108
const port = (server.address() as AddressInfo).port;
109-
const res = await fetch(`http://localhost:${port}/media/big`);
109+
const res = await fetch(`http://127.0.0.1:${port}/media/big`);
110110
expect(res.status).toBe(413);
111111
expect(await res.text()).toBe("too large");
112112
await new Promise((r) => server.close(r));

‎src/slack/monitor/media.test.ts‎

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -267,6 +267,7 @@ describe("resolveSlackMedia", () => {
267267
});
268268

269269
expect(result).not.toBeNull();
270+
expect(result).toHaveLength(1);
270271
// saveMediaBuffer should receive the overridden audio/mp4
271272
expect(saveMediaBufferMock).toHaveBeenCalledWith(
272273
expect.any(Buffer),
@@ -276,7 +277,7 @@ describe("resolveSlackMedia", () => {
276277
);
277278
// Returned contentType must be the overridden value, not the
278279
// re-detected video/mp4 from saveMediaBuffer
279-
expect(result!.contentType).toBe("audio/mp4");
280+
expect(result![0]?.contentType).toBe("audio/mp4");
280281
});
281282

282283
it("preserves original MIME for non-voice Slack files", async () => {
@@ -304,12 +305,14 @@ describe("resolveSlackMedia", () => {
304305
});
305306

306307
expect(result).not.toBeNull();
308+
expect(result).toHaveLength(1);
307309
expect(saveMediaBufferMock).toHaveBeenCalledWith(
308310
expect.any(Buffer),
309311
"video/mp4",
310312
"inbound",
311313
16 * 1024 * 1024,
312314
);
315+
expect(result![0]?.contentType).toBe("video/mp4");
313316
});
314317

315318
it("falls through to next file when first file returns error", async () => {
@@ -338,8 +341,41 @@ describe("resolveSlackMedia", () => {
338341
});
339342

340343
expect(result).not.toBeNull();
344+
expect(result).toHaveLength(1);
341345
expect(mockFetch).toHaveBeenCalledTimes(2);
342346
});
347+
348+
it("returns all successfully downloaded files as an array", async () => {
349+
vi.spyOn(mediaStore, "saveMediaBuffer")
350+
.mockResolvedValueOnce({ path: "/tmp/a.jpg", contentType: "image/jpeg" })
351+
.mockResolvedValueOnce({ path: "/tmp/b.png", contentType: "image/png" });
352+
353+
const responseA = new Response(Buffer.from("image a"), {
354+
status: 200,
355+
headers: { "content-type": "image/jpeg" },
356+
});
357+
const responseB = new Response(Buffer.from("image b"), {
358+
status: 200,
359+
headers: { "content-type": "image/png" },
360+
});
361+
362+
mockFetch.mockResolvedValueOnce(responseA).mockResolvedValueOnce(responseB);
363+
364+
const result = await resolveSlackMedia({
365+
files: [
366+
{ url_private: "https://files.slack.com/a.jpg", name: "a.jpg" },
367+
{ url_private: "https://files.slack.com/b.png", name: "b.png" },
368+
],
369+
token: "xoxb-test-token",
370+
maxBytes: 1024 * 1024,
371+
});
372+
373+
expect(result).toHaveLength(2);
374+
expect(result![0].path).toBe("/tmp/a.jpg");
375+
expect(result![0].placeholder).toBe("[Slack file: a.jpg]");
376+
expect(result![1].path).toBe("/tmp/b.png");
377+
expect(result![1].placeholder).toBe("[Slack file: b.png]");
378+
});
343379
});
344380

345381
describe("resolveSlackThreadHistory", () => {

‎src/slack/monitor/media.ts‎

Lines changed: 21 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -132,17 +132,28 @@ function resolveSlackMediaMimetype(
132132
return mime;
133133
}
134134

135+
export type SlackMediaResult = {
136+
path: string;
137+
contentType?: string;
138+
placeholder: string;
139+
};
140+
141+
const MAX_SLACK_MEDIA_FILES = 8;
142+
143+
/**
144+
* Downloads all files attached to a Slack message and returns them as an array.
145+
* Returns `null` when no files could be downloaded.
146+
*/
135147
export async function resolveSlackMedia(params: {
136148
files?: SlackFile[];
137149
token: string;
138150
maxBytes: number;
139-
}): Promise<{
140-
path: string;
141-
contentType?: string;
142-
placeholder: string;
143-
} | null> {
151+
}): Promise<SlackMediaResult[] | null> {
144152
const files = params.files ?? [];
145-
for (const file of files) {
153+
const limitedFiles =
154+
files.length > MAX_SLACK_MEDIA_FILES ? files.slice(0, MAX_SLACK_MEDIA_FILES) : files;
155+
const results: SlackMediaResult[] = [];
156+
for (const file of limitedFiles) {
146157
const url = file.url_private_download ?? file.url_private;
147158
if (!url) {
148159
continue;
@@ -169,16 +180,16 @@ export async function resolveSlackMedia(params: {
169180
params.maxBytes,
170181
);
171182
const label = fetched.fileName ?? file.name;
172-
return {
183+
results.push({
173184
path: saved.path,
174185
contentType: effectiveMime ?? saved.contentType,
175186
placeholder: label ? `[Slack file: ${label}]` : "[Slack file]",
176-
};
187+
});
177188
} catch {
178-
// Ignore download failures and fall through to the next file.
189+
// Ignore download failures and try the next file.
179190
}
180191
}
181-
return null;
192+
return results.length > 0 ? results : null;
182193
}
183194

184195
export type SlackThreadStarter = {

‎src/slack/monitor/message-handler/prepare.ts‎

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -342,7 +342,8 @@ export async function prepareSlackMessage(params: {
342342
token: ctx.botToken,
343343
maxBytes: ctx.mediaMaxBytes,
344344
});
345-
const rawBody = (message.text ?? "").trim() || media?.placeholder || "";
345+
const mediaPlaceholder = media ? media.map((m) => m.placeholder).join(" ") : undefined;
346+
const rawBody = (message.text ?? "").trim() || mediaPlaceholder || "";
346347
if (!rawBody) {
347348
return null;
348349
}
@@ -488,8 +489,9 @@ export async function prepareSlackMessage(params: {
488489
maxBytes: ctx.mediaMaxBytes,
489490
});
490491
if (threadStarterMedia) {
492+
const starterPlaceholders = threadStarterMedia.map((m) => m.placeholder).join(", ");
491493
logVerbose(
492-
`slack: hydrated thread starter file ${threadStarterMedia.placeholder} from root message`,
494+
`slack: hydrated thread starter file ${starterPlaceholders} from root message`,
493495
);
494496
}
495497
}
@@ -558,6 +560,10 @@ export async function prepareSlackMessage(params: {
558560

559561
// Use thread starter media if current message has none
560562
const effectiveMedia = media ?? threadStarterMedia;
563+
const firstMedia = effectiveMedia?.[0];
564+
const firstMediaType = firstMedia
565+
? (firstMedia.contentType ?? "application/octet-stream")
566+
: undefined;
561567

562568
const inboundHistory =
563569
isRoomish && ctx.historyLimit > 0
@@ -599,9 +605,17 @@ export async function prepareSlackMessage(params: {
599605
ThreadLabel: threadLabel,
600606
Timestamp: message.ts ? Math.round(Number(message.ts) * 1000) : undefined,
601607
WasMentioned: isRoomish ? effectiveWasMentioned : undefined,
602-
MediaPath: effectiveMedia?.path,
603-
MediaType: effectiveMedia?.contentType,
604-
MediaUrl: effectiveMedia?.path,
608+
MediaPath: firstMedia?.path,
609+
MediaType: firstMediaType,
610+
MediaUrl: firstMedia?.path,
611+
MediaPaths:
612+
effectiveMedia && effectiveMedia.length > 0 ? effectiveMedia.map((m) => m.path) : undefined,
613+
MediaUrls:
614+
effectiveMedia && effectiveMedia.length > 0 ? effectiveMedia.map((m) => m.path) : undefined,
615+
MediaTypes:
616+
effectiveMedia && effectiveMedia.length > 0
617+
? effectiveMedia.map((m) => m.contentType ?? "application/octet-stream")
618+
: undefined,
605619
CommandAuthorized: commandAuthorized,
606620
OriginatingChannel: "slack" as const,
607621
OriginatingTo: slackTo,

0 commit comments

Comments
 (0)