diff --git a/src/auto-reply/inbound.test.ts b/src/auto-reply/inbound.test.ts index a56c03457c7..4cae3e34cac 100644 --- a/src/auto-reply/inbound.test.ts +++ b/src/auto-reply/inbound.test.ts @@ -116,6 +116,43 @@ describe("finalizeInboundContext", () => { finalizeInboundContext(ctx, { forceBodyForCommands: true }); expect(ctx.BodyForCommands).toBe("say hi"); }); + + it("fills MediaType/MediaTypes defaults only when media exists", () => { + const withMedia: MsgContext = { + Body: "hi", + MediaPath: "/tmp/file.bin", + }; + const outWithMedia = finalizeInboundContext(withMedia); + expect(outWithMedia.MediaType).toBe("application/octet-stream"); + expect(outWithMedia.MediaTypes).toEqual(["application/octet-stream"]); + + const withoutMedia: MsgContext = { Body: "hi" }; + const outWithoutMedia = finalizeInboundContext(withoutMedia); + expect(outWithoutMedia.MediaType).toBeUndefined(); + expect(outWithoutMedia.MediaTypes).toBeUndefined(); + }); + + it("pads MediaTypes to match MediaPaths/MediaUrls length", () => { + const ctx: MsgContext = { + Body: "hi", + MediaPaths: ["/tmp/a", "/tmp/b"], + MediaTypes: ["image/png"], + }; + const out = finalizeInboundContext(ctx); + expect(out.MediaType).toBe("image/png"); + expect(out.MediaTypes).toEqual(["image/png", "application/octet-stream"]); + }); + + it("derives MediaType from MediaTypes when missing", () => { + const ctx: MsgContext = { + Body: "hi", + MediaPath: "/tmp/a", + MediaTypes: ["image/jpeg"], + }; + const out = finalizeInboundContext(ctx); + expect(out.MediaType).toBe("image/jpeg"); + expect(out.MediaTypes).toEqual(["image/jpeg"]); + }); }); describe("inbound dedupe", () => { diff --git a/src/auto-reply/reply/inbound-context.ts b/src/auto-reply/reply/inbound-context.ts index daeeecc8852..8f3e60857f2 100644 --- a/src/auto-reply/reply/inbound-context.ts +++ b/src/auto-reply/reply/inbound-context.ts @@ -10,6 +10,8 @@ export type FinalizeInboundContextOptions = { forceConversationLabel?: boolean; }; +const DEFAULT_MEDIA_TYPE = "application/octet-stream"; + function normalizeTextField(value: unknown): string | undefined { if (typeof value !== "string") { return undefined; @@ -17,6 +19,21 @@ function normalizeTextField(value: unknown): string | undefined { return normalizeInboundTextNewlines(value); } +function normalizeMediaType(value: unknown): string | undefined { + if (typeof value !== "string") { + return undefined; + } + const trimmed = value.trim(); + return trimmed.length > 0 ? trimmed : undefined; +} + +function countMediaEntries(ctx: MsgContext): number { + const pathCount = Array.isArray(ctx.MediaPaths) ? ctx.MediaPaths.length : 0; + const urlCount = Array.isArray(ctx.MediaUrls) ? ctx.MediaUrls.length : 0; + const single = ctx.MediaPath || ctx.MediaUrl ? 1 : 0; + return Math.max(pathCount, urlCount, single); +} + export function finalizeInboundContext>( ctx: T, opts: FinalizeInboundContextOptions = {}, @@ -73,5 +90,35 @@ export function finalizeInboundContext>( // Always set. Default-deny when upstream forgets to populate it. normalized.CommandAuthorized = normalized.CommandAuthorized === true; + // MediaType/MediaTypes alignment: + // - No media: do not inject defaults. + // - Media present: ensure MediaType is always set, and MediaTypes is padded to match + // MediaPaths/MediaUrls length when possible. + const mediaCount = countMediaEntries(normalized); + if (mediaCount > 0) { + const mediaType = normalizeMediaType(normalized.MediaType); + const rawMediaTypes = Array.isArray(normalized.MediaTypes) ? normalized.MediaTypes : undefined; + const normalizedMediaTypes = rawMediaTypes?.map((entry) => normalizeMediaType(entry)); + + let mediaTypesFinal: string[] | undefined; + if (normalizedMediaTypes && normalizedMediaTypes.length > 0) { + const filled = normalizedMediaTypes.slice(); + while (filled.length < mediaCount) { + filled.push(undefined); + } + mediaTypesFinal = filled.map((entry) => entry ?? DEFAULT_MEDIA_TYPE); + } else if (mediaType) { + mediaTypesFinal = [mediaType]; + while (mediaTypesFinal.length < mediaCount) { + mediaTypesFinal.push(DEFAULT_MEDIA_TYPE); + } + } else { + mediaTypesFinal = Array.from({ length: mediaCount }, () => DEFAULT_MEDIA_TYPE); + } + + normalized.MediaTypes = mediaTypesFinal; + normalized.MediaType = mediaType ?? mediaTypesFinal[0] ?? DEFAULT_MEDIA_TYPE; + } + return normalized as T & FinalizedMsgContext; } diff --git a/src/slack/monitor/media.test.ts b/src/slack/monitor/media.test.ts index 8d38d17c473..7303245bcca 100644 --- a/src/slack/monitor/media.test.ts +++ b/src/slack/monitor/media.test.ts @@ -346,20 +346,33 @@ describe("resolveSlackMedia", () => { }); 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" }, + vi.spyOn(mediaStore, "saveMediaBuffer").mockImplementation(async (buffer) => { + const text = Buffer.from(buffer).toString("utf8"); + if (text.includes("image a")) { + return { path: "/tmp/a.jpg", contentType: "image/jpeg" }; + } + if (text.includes("image b")) { + return { path: "/tmp/b.png", contentType: "image/png" }; + } + return { path: "/tmp/unknown", contentType: "application/octet-stream" }; }); - mockFetch.mockResolvedValueOnce(responseA).mockResolvedValueOnce(responseB); + mockFetch.mockImplementation(async (input) => { + const url = String(input); + if (url.includes("/a.jpg")) { + return new Response(Buffer.from("image a"), { + status: 200, + headers: { "content-type": "image/jpeg" }, + }); + } + if (url.includes("/b.png")) { + return new Response(Buffer.from("image b"), { + status: 200, + headers: { "content-type": "image/png" }, + }); + } + return new Response("Not Found", { status: 404 }); + }); const result = await resolveSlackMedia({ files: [ @@ -376,6 +389,37 @@ describe("resolveSlackMedia", () => { expect(result![1].path).toBe("/tmp/b.png"); expect(result![1].placeholder).toBe("[Slack file: b.png]"); }); + + it("caps downloads to 8 files for large multi-attachment messages", async () => { + const saveMediaBufferMock = vi.spyOn(mediaStore, "saveMediaBuffer").mockResolvedValue({ + path: "/tmp/x.jpg", + contentType: "image/jpeg", + }); + + mockFetch.mockImplementation(async () => { + return new Response(Buffer.from("image data"), { + status: 200, + headers: { "content-type": "image/jpeg" }, + }); + }); + + const files = Array.from({ length: 9 }, (_, idx) => ({ + url_private: `https://files.slack.com/file-${idx}.jpg`, + name: `file-${idx}.jpg`, + mimetype: "image/jpeg", + })); + + const result = await resolveSlackMedia({ + files, + token: "xoxb-test-token", + maxBytes: 1024 * 1024, + }); + + expect(result).not.toBeNull(); + expect(result).toHaveLength(8); + expect(saveMediaBufferMock).toHaveBeenCalledTimes(8); + expect(mockFetch).toHaveBeenCalledTimes(8); + }); }); describe("resolveSlackThreadHistory", () => { diff --git a/src/slack/monitor/media.ts b/src/slack/monitor/media.ts index 21fcda50925..33390e45a5d 100644 --- a/src/slack/monitor/media.ts +++ b/src/slack/monitor/media.ts @@ -139,6 +139,33 @@ export type SlackMediaResult = { }; const MAX_SLACK_MEDIA_FILES = 8; +const MAX_SLACK_MEDIA_CONCURRENCY = 3; + +async function mapLimit( + items: T[], + limit: number, + fn: (item: T) => Promise, +): Promise { + if (items.length === 0) { + return []; + } + const results: R[] = []; + results.length = items.length; + let nextIndex = 0; + const workerCount = Math.max(1, Math.min(limit, items.length)); + await Promise.all( + Array.from({ length: workerCount }, async () => { + while (true) { + const idx = nextIndex++; + if (idx >= items.length) { + return; + } + results[idx] = await fn(items[idx]); + } + }), + ); + return results; +} /** * Downloads all files attached to a Slack message and returns them as an array. @@ -152,43 +179,50 @@ export async function resolveSlackMedia(params: { const files = params.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; - } - try { - // Note: fetchRemoteMedia calls fetchImpl(url) with the URL string today and - // handles size limits internally. Provide a fetcher that uses auth once, then lets - // the redirect chain continue without credentials. - const fetchImpl = createSlackMediaFetch(params.token); - const fetched = await fetchRemoteMedia({ - url, - fetchImpl, - filePathHint: file.name, - maxBytes: params.maxBytes, - }); - if (fetched.buffer.byteLength > params.maxBytes) { - continue; + + const resolved = await mapLimit( + limitedFiles, + MAX_SLACK_MEDIA_CONCURRENCY, + async (file) => { + const url = file.url_private_download ?? file.url_private; + if (!url) { + return null; } - const effectiveMime = resolveSlackMediaMimetype(file, fetched.contentType); - const saved = await saveMediaBuffer( - fetched.buffer, - effectiveMime, - "inbound", - params.maxBytes, - ); - const label = fetched.fileName ?? file.name; - results.push({ - path: saved.path, - contentType: effectiveMime ?? saved.contentType, - placeholder: label ? `[Slack file: ${label}]` : "[Slack file]", - }); - } catch { - // Ignore download failures and try the next file. - } - } + try { + // Note: fetchRemoteMedia calls fetchImpl(url) with the URL string today and + // handles size limits internally. Provide a fetcher that uses auth once, then lets + // the redirect chain continue without credentials. + const fetchImpl = createSlackMediaFetch(params.token); + const fetched = await fetchRemoteMedia({ + url, + fetchImpl, + filePathHint: file.name, + maxBytes: params.maxBytes, + }); + if (fetched.buffer.byteLength > params.maxBytes) { + return null; + } + const effectiveMime = resolveSlackMediaMimetype(file, fetched.contentType); + const saved = await saveMediaBuffer( + fetched.buffer, + effectiveMime, + "inbound", + params.maxBytes, + ); + const label = fetched.fileName ?? file.name; + const contentType = effectiveMime ?? saved.contentType; + return { + path: saved.path, + ...(contentType ? { contentType } : {}), + placeholder: label ? `[Slack file: ${label}]` : "[Slack file]", + }; + } catch { + return null; + } + }, + ); + + const results = resolved.filter((entry): entry is SlackMediaResult => Boolean(entry)); return results.length > 0 ? results : null; } diff --git a/src/slack/monitor/message-handler/prepare.ts b/src/slack/monitor/message-handler/prepare.ts index 7c24f1e0527..7e3ccf5f978 100644 --- a/src/slack/monitor/message-handler/prepare.ts +++ b/src/slack/monitor/message-handler/prepare.ts @@ -561,9 +561,6 @@ 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 @@ -606,7 +603,7 @@ export async function prepareSlackMessage(params: { Timestamp: message.ts ? Math.round(Number(message.ts) * 1000) : undefined, WasMentioned: isRoomish ? effectiveWasMentioned : undefined, MediaPath: firstMedia?.path, - MediaType: firstMediaType, + MediaType: firstMedia?.contentType, MediaUrl: firstMedia?.path, MediaPaths: effectiveMedia && effectiveMedia.length > 0 ? effectiveMedia.map((m) => m.path) : undefined, @@ -614,7 +611,7 @@ export async function prepareSlackMessage(params: { effectiveMedia && effectiveMedia.length > 0 ? effectiveMedia.map((m) => m.path) : undefined, MediaTypes: effectiveMedia && effectiveMedia.length > 0 - ? effectiveMedia.map((m) => m.contentType ?? "application/octet-stream") + ? effectiveMedia.map((m) => m.contentType ?? "") : undefined, CommandAuthorized: commandAuthorized, OriginatingChannel: "slack" as const,