From 632343efe84aa687dccc7e270d13433ae99eeb5c Mon Sep 17 00:00:00 2001 From: goffern <10464170+goffern@users.noreply.github.com> Date: Wed, 26 Aug 2026 13:08:42 +0800 Subject: [PATCH] fix(mattermost): keep server file name when the download fails (#129556) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 --------- Co-authored-by: goffern Co-authored-by: Peter Steinberger --- .../src/mattermost/monitor-resources.test.ts | 45 +++++- .../src/mattermost/monitor-resources.ts | 22 ++- .../monitor.inbound-system-event.test.ts | 133 +++++++++++------- 3 files changed, 142 insertions(+), 58 deletions(-) diff --git a/extensions/mattermost/src/mattermost/monitor-resources.test.ts b/extensions/mattermost/src/mattermost/monitor-resources.test.ts index dcc0034ad59a..d680e0c5e6ec 100644 --- a/extensions/mattermost/src/mattermost/monitor-resources.test.ts +++ b/extensions/mattermost/src/mattermost/monitor-resources.test.ts @@ -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", diff --git a/extensions/mattermost/src/mattermost/monitor-resources.ts b/extensions/mattermost/src/mattermost/monitor-resources.ts index e628291fd981..e2ca0f365fa0 100644 --- a/extensions/mattermost/src/mattermost/monitor-resources.ts +++ b/extensions/mattermost/src/mattermost/monitor-resources.ts @@ -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", }); } diff --git a/extensions/mattermost/src/mattermost/monitor.inbound-system-event.test.ts b/extensions/mattermost/src/mattermost/monitor.inbound-system-event.test.ts index 65686110db31..1e3d991c0fd8 100644 --- a/extensions/mattermost/src/mattermost/monitor.inbound-system-event.test.ts +++ b/extensions/mattermost/src/mattermost/monitor.inbound-system-event.test.ts @@ -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();