fix(mattermost): keep server file name when the download fails (#129556)

* fix(mattermost): keep server file name when the download fails #129140 propagates the attachment file name into the model context, but only on the successful-download path. The failed-download fallback already calls /files/{id}/info and reads mime_type while discarding the name the same response carries — and on that branch the name is the only context the model gets about the attachment, since there is no local file. Read it and forward it like the success path does; blank names stay omitted. * fix(mattermost): surface unavailable attachment filenames to agents Preserve server-provided attachment names through the Mattermost-owned unavailable-media notice, safely bound model-visible metadata, and cover the actual posted-event dispatch boundary. Co-authored-by: goffern <goffern@users.noreply.github.com> --------- Co-authored-by: goffern <goffern@users.noreply.github.com> Co-authored-by: Peter Steinberger <steipete@gmail.com>
2026-08-27 21:07:01 -06:00 · 2026-08-26 13:08:42 +08:00
parent 0d80630110
commit 632343efe8
3 changed files with 142 additions and 58 deletions
@@ -51,11 +51,42 @@ describe("mattermost monitor resources", () => {
        body: "quarterly files",
        nativeMedia: [{}, {}],
        materializedMedia: [
-          { path: "/tmp/q1.pdf", contentType: "application/pdf" },
-          { kind: "audio" },
+          { path: "/tmp/q1.pdf", contentType: "application/pdf", fileName: "available.pdf" },
+          { kind: "audio", fileName: "quarterly recording.mp3" },
        ],
      }),
-    ).toBe("quarterly files\n\n[mattermost attachment unavailable]");
+    ).toBe('quarterly files\n\n[mattermost attachment unavailable] "quarterly recording.mp3"');
+  });
+
+  it.each([
+    {
+      fileName: '../../private/report]\u0000\n".pdf',
+      expected: '[mattermost attachment unavailable] "report].pdf"',
+    },
+    { fileName: "   ", expected: "[mattermost attachment unavailable]" },
+  ])("safely formats unavailable attachment names: $fileName", ({ fileName, expected }) => {
+    expect(
+      formatMattermostInboundMediaText({
+        body: "",
+        nativeMedia: [{}],
+        materializedMedia: [{ kind: "document", fileName }],
+      }),
+    ).toBe(expected);
+  });
+
+  it("bounds multiple unavailable attachment names without losing their count", () => {
+    const materializedMedia = Array.from({ length: 4 }, (_, index) => ({
+      kind: "document" as const,
+      fileName: `${index}-${"a".repeat(250)}.pdf`,
+    }));
+    const result = formatMattermostInboundMediaText({
+      body: "",
+      nativeMedia: materializedMedia.map(() => ({})),
+      materializedMedia,
+    });
+
+    expect(result).toContain("[mattermost 4 attachments unavailable]");
+    expect(result.length).toBeLessThanOrEqual(560);
  });

  it("keeps successfully materialized media-only text empty", () => {
@@ -186,7 +217,11 @@ describe("mattermost monitor resources", () => {

    await expect(resources.resolveMattermostMedia(["file-image", "file-audio"])).resolves.toEqual([
      { path: "/tmp/file.png", contentType: "image/png", kind: "image" },
-      { contentType: "audio/mpeg", kind: "audio" },
+      {
+        contentType: "audio/mpeg",
+        fileName: "private-unavailable-recording.mp3",
+        kind: "audio",
+      },
    ]);
    expect(request).toHaveBeenCalledTimes(1);
  });
@@ -195,6 +230,8 @@ describe("mattermost monitor resources", () => {
    const saveRemoteMedia = vi.fn().mockRejectedValue(new Error("download failed"));
    const request = vi.fn(async (requestPath: string) => ({
      mime_type: requestPath.includes("video") ? "video/mp4" : "application/pdf",
+      // Blank server-side names must be omitted, not forwarded as empty strings.
+      name: requestPath.includes("video") ? "   " : undefined,
    }));
    const resources = createMattermostMonitorResources({
      accountId: "default",
@@ -15,7 +15,9 @@ import {
  asDateTimestampMs,
  resolveExpiresAtMsFromDurationMs,
 } from "openclaw/plugin-sdk/number-runtime";
+import { sanitizeUntrustedFileName } from "openclaw/plugin-sdk/security-runtime";
 import { normalizeStringEntries } from "openclaw/plugin-sdk/string-coerce-runtime";
+import { truncateUtf16Safe } from "openclaw/plugin-sdk/text-utility-runtime";
 import {
  buildMattermostApiUrl,
  fetchMattermostChannel,
@@ -49,7 +51,7 @@ export function formatMattermostPendingMediaText(params: {
 export function formatMattermostInboundMediaText(params: {
  body: string;
  nativeMedia: readonly MediaPlaceholderTextFact[];
-  materializedMedia: readonly MediaPlaceholderTextFact[];
+  materializedMedia: readonly ChannelInboundMediaInput[];
 }): string {
  const materializedCount = params.materializedMedia.filter(
    (media) => Boolean(media.path) || Boolean(media.url),
@@ -58,9 +60,17 @@ export function formatMattermostInboundMediaText(params: {
  if (unavailableCount === 0) {
    return params.body;
  }
+  const unavailableFileNames = params.materializedMedia
+    .filter((media) => !media.path && !media.url && media.fileName)
+    .map((media) => sanitizeUntrustedFileName(media.fileName ?? "", ""))
+    .filter(Boolean)
+    .join(", ");
+  const fileNameNotice = unavailableFileNames
+    ? ` ${JSON.stringify(truncateUtf16Safe(unavailableFileNames, 512))}`
+    : "";
  return formatInboundMediaUnavailableText({
    body: params.body,
-    notice: `[mattermost ${unavailableCount > 1 ? `${unavailableCount} attachments` : "attachment"} unavailable]`,
+    notice: `[mattermost ${unavailableCount > 1 ? `${unavailableCount} attachments` : "attachment"} unavailable]${fileNameNotice}`,
  });
 }

@@ -179,17 +189,19 @@ export function createMattermostMonitorResources(params: {
        });
      } catch (err) {
        logger.debug?.(`mattermost: failed to download file ${fileId}: ${String(err)}`);
-        let contentType: string | undefined;
+        let info: { mime_type?: string | null; name?: string | null } | undefined;
        try {
-          const info = await client.request<{ mime_type?: string | null }>(`/files/${fileId}/info`);
-          contentType = info.mime_type?.trim() || undefined;
+          info = await client.request(`/files/${fileId}/info`);
        } catch (infoErr) {
          logger.debug?.(
            `mattermost: failed to resolve metadata for file ${fileId}: ${String(infoErr)}`,
          );
        }
+        const contentType = info?.mime_type?.trim() || undefined;
+        const fileName = info?.name?.trim();
        out.push({
          contentType,
+          ...(fileName ? { fileName } : {}),
          kind: mediaKindFromMime(contentType) ?? "unknown",
        });
      }
@@ -815,61 +815,96 @@ describe("mattermost inbound user posts", () => {
    expect(statusSink).not.toHaveBeenCalledWith(expect.objectContaining({ lifecycle: "ready" }));
  });

-  it("does not enqueue regular user posts as system events", async () => {
-    const socket = new FakeWebSocket();
-    const abortController = new AbortController();
-    mockState.abortController = abortController;
+  it.each([
+    {
+      label: "plain text",
+      fileIds: [],
+      failedMedia: [],
+      expectedBody: "hello from mattermost",
+    },
+    {
+      label: "an unavailable named attachment",
+      fileIds: ["file-1"],
+      failedMedia: [
+        {
+          contentType: "application/pdf",
+          fileName: "quarterly report.pdf",
+          kind: "document",
+        },
+      ],
+      expectedBody:
+        'hello from mattermost\n\n[mattermost attachment unavailable] "quarterly report.pdf"',
+    },
+  ])(
+    "does not enqueue regular posts with $label as system events",
+    async ({ fileIds, failedMedia, expectedBody }) => {
+      const socket = new FakeWebSocket();
+      const abortController = new AbortController();
+      mockState.abortController = abortController;
+      mockState.resolveMattermostMedia.mockResolvedValueOnce(failedMedia);

-    const monitor = monitorMattermostProvider({
-      config: testConfig,
-      runtime: testRuntime(),
-      abortSignal: abortController.signal,
-      webSocketFactory: () => socket,
-    });
+      const monitor = monitorMattermostProvider({
+        config: testConfig,
+        runtime: testRuntime(),
+        abortSignal: abortController.signal,
+        webSocketFactory: () => socket,
+      });

-    await vi.waitFor(() => {
-      expect(socket.openListenerCount).toBeGreaterThan(0);
-    });
-    socket.emitOpen();
+      await vi.waitFor(() => {
+        expect(socket.openListenerCount).toBeGreaterThan(0);
+      });
+      socket.emitOpen();

-    await socket.emitMessage({
-      event: "posted",
-      data: {
-        channel_id: "chan-1",
-        channel_name: "town-square",
-        channel_display_name: "Town Square",
-        sender_name: "alice",
-        post: JSON.stringify({
-          id: "post-inbound-system-event-regular",
+      await socket.emitMessage({
+        event: "posted",
+        data: {
+          channel_id: "chan-1",
+          channel_name: "town-square",
+          channel_display_name: "Town Square",
+          sender_name: "alice",
+          post: JSON.stringify({
+            id: "post-inbound-system-event-regular",
+            channel_id: "chan-1",
+            user_id: "user-1",
+            message: "hello from mattermost",
+            ...(fileIds.length > 0 ? { file_ids: fileIds } : {}),
+            create_at: 1_714_000_000_000,
+          }),
+        },
+        broadcast: {
          channel_id: "chan-1",
          user_id: "user-1",
-          message: "hello from mattermost",
-          create_at: 1_714_000_000_000,
-        }),
-      },
-      broadcast: {
-        channel_id: "chan-1",
-        user_id: "user-1",
-      },
-    });
-    socket.emitClose(1000);
-    await monitor;
+        },
+      });
+      socket.emitClose(1000);
+      await monitor;

-    expect(mockState.enqueueSystemEvent).not.toHaveBeenCalled();
-    expect(mockState.dispatchInboundMessage).toHaveBeenCalledTimes(1);
-    expect(mockState.deliveryPlanObserver).toHaveBeenCalledExactlyOnceWith(true);
-    const ctx = mockState.dispatchInboundMessage.mock.calls.at(0)?.[0].ctx;
-    expect(ctx?.BodyForAgent).toBe("hello from mattermost");
-    expect(ctx?.ConversationLabel).toBe("Town Square id:chan-1");
-    expect(ctx?.MessageSid).toBe("post-inbound-system-event-regular");
-    expect(ctx?.ConversationRouteContextObserved).toBe(true);
-    expect(ctx?.ConversationRoutePeerId).toBe("chan-1");
-    expect(ctx?.GroupSpace).toBe("team-1");
-    expect(ctx?.NativeChannelId).toBe("chan-1");
-    expect(ctx?.InboundAccessAuthorized).toBe(true);
-    expect(ctx?.OriginatingChannel).toBe("mattermost");
-    expect(ctx?.Provider).toBe("mattermost");
-  });
+      expect(mockState.enqueueSystemEvent).not.toHaveBeenCalled();
+      expect(mockState.dispatchInboundMessage).toHaveBeenCalledTimes(1);
+      expect(mockState.deliveryPlanObserver).toHaveBeenCalledExactlyOnceWith(true);
+      const ctx = mockState.dispatchInboundMessage.mock.calls.at(0)?.[0].ctx;
+      expect(ctx?.BodyForAgent).toBe(expectedBody);
+      if (failedMedia.length > 0) {
+        expect(ctx?.media).toEqual([
+          expect.objectContaining({
+            contentType: "application/pdf",
+            fileName: "quarterly report.pdf",
+          }),
+        ]);
+        expect(ctx?.media?.[0]?.path).toBeUndefined();
+        expect(ctx?.media?.[0]?.url).toBeUndefined();
+      }
+      expect(ctx?.ConversationLabel).toBe("Town Square id:chan-1");
+      expect(ctx?.MessageSid).toBe("post-inbound-system-event-regular");
+      expect(ctx?.ConversationRouteContextObserved).toBe(true);
+      expect(ctx?.ConversationRoutePeerId).toBe("chan-1");
+      expect(ctx?.GroupSpace).toBe("team-1");
+      expect(ctx?.NativeChannelId).toBe("chan-1");
+      expect(ctx?.InboundAccessAuthorized).toBe(true);
+      expect(ctx?.OriginatingChannel).toBe("mattermost");
+      expect(ctx?.Provider).toBe("mattermost");
+    },
+  );

  it("formats current and pending-history timestamps in the configured user timezone", async () => {
    const socket = new FakeWebSocket();