refactor(media): normalize inbound media type defaults (#16228)

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", () => {

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<T extends Record<string, unknown>>(
   ctx: T,
   opts: FinalizeInboundContextOptions = {},
@@ -73,5 +90,35 @@ export function finalizeInboundContext<T extends Record<string, unknown>>(
   // 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;
 }

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", () => {

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<T, R>(
+  items: T[],
+  limit: number,
+  fn: (item: T) => Promise<R>,
+): Promise<R[]> {
+  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<SlackFile, SlackMediaResult | null>(
+    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;
 }
 

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,