From e1e2818a6ab0bb4e8426cd80b03f4e03b5c9882f Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Tue, 25 Aug 2026 14:46:28 -0700 Subject: [PATCH] fix(mattermost): preserve presentations in normal agent replies (#129579) --- extensions/mattermost/src/channel.ts | 55 +---- .../mattermost/monitor-draft-delivery.test.ts | 55 +++++ .../src/mattermost/monitor-draft-delivery.ts | 5 +- .../src/mattermost/reply-delivery.test.ts | 192 +++++++++++++++++- .../src/mattermost/reply-delivery.ts | 50 ++--- extensions/mattermost/src/normalize.ts | 42 ++++ 6 files changed, 320 insertions(+), 79 deletions(-) diff --git a/extensions/mattermost/src/channel.ts b/extensions/mattermost/src/channel.ts index 63e7c71b1e7e..3c827f059f59 100644 --- a/extensions/mattermost/src/channel.ts +++ b/extensions/mattermost/src/channel.ts @@ -28,10 +28,7 @@ import { createChannelDirectoryAdapter } from "openclaw/plugin-sdk/directory-run import { buildPassiveProbedChannelStatusSummary } from "openclaw/plugin-sdk/extension-shared"; import { type MessagePresentation, - normalizeMessagePresentation, - renderMessagePresentationFallbackText, resolveMessagePresentationButtonAction, - resolveMessagePresentationControlValue, } from "openclaw/plugin-sdk/interactive-runtime"; import { createLazyRuntimeModule } from "openclaw/plugin-sdk/lazy-runtime"; import { resolvePayloadMediaUrls, sendTextMediaPayload } from "openclaw/plugin-sdk/reply-payload"; @@ -73,6 +70,7 @@ import type { MattermostSendResult } from "./mattermost/send.js"; import { looksLikeMattermostTargetId, normalizeMattermostMessagingTarget, + resolveMattermostPresentation, requiresMattermostMediaUpload, } from "./normalize.js"; import { collectRuntimeConfigAssignments, secretTargetRegistryEntries } from "./secret-contract.js"; @@ -83,33 +81,6 @@ import type { MattermostConfig } from "./types.js"; const loadMattermostChannelRuntime = createLazyRuntimeModule(() => import("./channel.runtime.js")); -function buildMattermostPresentationButtons(presentation: MessagePresentation) { - return presentation.blocks - .filter((block) => block.type === "buttons") - .map((block) => - block.buttons.flatMap((button) => { - if (button.action) { - return []; - } - const value = resolveMessagePresentationControlValue(button); - return value - ? [ - { - id: value, - text: button.label, - callback_data: value, - context: { - callback_data: value, - }, - style: button.style, - }, - ] - : []; - }), - ) - .filter((row) => row.length > 0); -} - const MATTERMOST_PRESENTATION_CAPABILITIES = { supported: true, buttons: true, @@ -572,15 +543,10 @@ const mattermostMessageActions: ChannelMessageActionAdapter = { throw new Error("Mattermost send requires a target (to)."); } - const presentation = normalizeMessagePresentation(params.presentation); - const message = presentation - ? renderMessagePresentationFallbackText({ - text: typeof params.message === "string" ? params.message : "", - presentation, - }) - : typeof params.message === "string" - ? params.message - : ""; + const { text: message, buttons } = resolveMattermostPresentation({ + text: typeof params.message === "string" ? params.message : undefined, + presentation: params.presentation, + }); // Mattermost post root_id is the thread root. A generic replyTo can name // the current child post, so prefer threadId unless the caller supplied the // Mattermost-specific replyToId root directly. @@ -591,8 +557,6 @@ const mattermostMessageActions: ChannelMessageActionAdapter = { const resolvedAccountId = accountId || undefined; const mediaUrl = resolveMattermostSendAttachmentMedia(params); - const buttons = presentation ? buildMattermostPresentationButtons(presentation) : []; - const result = await ( await loadMattermostChannelRuntime() ).sendMessageMattermost(to, message, { @@ -803,15 +767,14 @@ const mattermostOutbound: ChannelOutboundAdapter = { if (payload.mediaUrls && payload.mediaUrls.length > 1) { return null; } - const buttons = buildMattermostPresentationButtons(presentation); - const hasButtons = buttons.some((row) => row.length > 0); - if (!hasButtons && !hasMattermostPresentationNavigation(presentation)) { + const { text, buttons } = resolveMattermostPresentation({ text: payload.text, presentation }); + if (!buttons.length && !hasMattermostPresentationNavigation(presentation)) { return null; } return { ...payload, - text: renderMessagePresentationFallbackText({ text: payload.text, presentation }), - ...(hasButtons + text, + ...(buttons.length ? { channelData: { ...payload.channelData, diff --git a/extensions/mattermost/src/mattermost/monitor-draft-delivery.test.ts b/extensions/mattermost/src/mattermost/monitor-draft-delivery.test.ts index 6ecc2db90260..99717b9d3b0b 100644 --- a/extensions/mattermost/src/mattermost/monitor-draft-delivery.test.ts +++ b/extensions/mattermost/src/mattermost/monitor-draft-delivery.test.ts @@ -147,6 +147,41 @@ describe("deliverMattermostReplyWithDraftPreview", () => { }); }); + it.each([ + { + name: "native value buttons", + presentation: { + blocks: [{ type: "buttons" as const, buttons: [{ label: "Open", value: "open" }] }], + }, + }, + { + name: "navigation URLs", + presentation: { + blocks: [ + { + type: "buttons" as const, + buttons: [{ label: "Docs", url: "https://example.com/docs" }], + }, + ], + }, + }, + ])("delivers $name instead of losing them in a preview edit", async ({ presentation }) => { + const draftStream = createDraftStreamMock(); + const deliverFinal = createDeliverFinalMock(); + const payload = { text: "Choose an option", presentation }; + + await deliverDraftPreview({ + payload, + draftStream, + effectiveReplyToId: "thread-root-1", + deliverPayload: deliverFinal, + }); + + expect(deliverFinal).toHaveBeenCalledExactlyOnceWith(payload); + expect(updateMattermostPostSpy).not.toHaveBeenCalled(); + expect(draftStream.clear).toHaveBeenCalledTimes(1); + }); + it("reports a final already published in a sealed preview generation", async () => { const draftStream = createDraftStreamMock(null); const deliverFinal = createDeliverFinalMock(); @@ -174,6 +209,26 @@ describe("deliverMattermostReplyWithDraftPreview", () => { }); }); + it("delivers unsent presentation controls after preview text was already published", async () => { + const draftStream = createDraftStreamMock(null); + const deliverFinal = createDeliverFinalMock(); + const confirmedDelivery = createConfirmedPreviewDelivery("sealed-post-1", "Already visible"); + const presentation = { + blocks: [{ type: "buttons" as const, buttons: [{ label: "Open", value: "open" }] }], + }; + + const result = await deliverDraftPreview({ + payload: { text: "Already visible", presentation }, + draftStream, + effectiveReplyToId: "thread-root-1", + resolvePreviewFinalText: () => ({ alreadyDelivered: true, confirmedDelivery }), + deliverPayload: deliverFinal, + }); + + expect(deliverFinal).toHaveBeenCalledExactlyOnceWith({ text: "", presentation }); + expect(result.messageIds).toEqual(["sealed-post-1", "delivered-post-1"]); + }); + it("still delivers media when the text is already published", async () => { const draftStream = createDraftStreamMock(null); const confirmedDelivery = createConfirmedPreviewDelivery("sealed-post-1", "Already visible"); diff --git a/extensions/mattermost/src/mattermost/monitor-draft-delivery.ts b/extensions/mattermost/src/mattermost/monitor-draft-delivery.ts index 1c47e48c388e..fc882426aa27 100644 --- a/extensions/mattermost/src/mattermost/monitor-draft-delivery.ts +++ b/extensions/mattermost/src/mattermost/monitor-draft-delivery.ts @@ -134,15 +134,18 @@ export async function deliverMattermostReplyWithDraftPreview( previewFinalDeliveryText = previewFinalResolution?.deliveryText; previewFinalTextAlreadyDelivered = previewFinalResolution?.alreadyDelivered === true && payload.isError !== true; + // A text-only preview cannot finalize unsent presentation content or controls. useConfirmedPreviewAsWholeFinal = previewFinalTextAlreadyDelivered && - !resolveSendableOutboundReplyParts(payload).hasMedia; + !resolveSendableOutboundReplyParts(payload).hasMedia && + !payload.presentation; const previewFinalText = previewFinalResolution?.editText; if ( (hasMedia && !ttsSupplement) || typeof previewFinalText !== "string" || payload.isError || + payload.presentation || !canFinalizeMattermostPreviewInPlace({ kind: params.kind, previewRootId: params.effectiveReplyToId, diff --git a/extensions/mattermost/src/mattermost/reply-delivery.test.ts b/extensions/mattermost/src/mattermost/reply-delivery.test.ts index 0a5891ed4479..a373630e98a3 100644 --- a/extensions/mattermost/src/mattermost/reply-delivery.test.ts +++ b/extensions/mattermost/src/mattermost/reply-delivery.test.ts @@ -2,7 +2,7 @@ import path from "node:path"; import { isChannelPartialDeliveryError } from "openclaw/plugin-sdk/channel-inbound"; import { createMessageReceiptFromOutboundResults } from "openclaw/plugin-sdk/channel-outbound"; -import type { ChunkMode } from "openclaw/plugin-sdk/reply-runtime"; +import type { ChunkMode, ReplyPayload } from "openclaw/plugin-sdk/reply-runtime"; import { createOpenClawTestState } from "openclaw/plugin-sdk/test-state"; import { describe, expect, it, vi } from "vitest"; import type { OpenClawConfig, PluginRuntime } from "../../runtime-api.js"; @@ -49,7 +49,7 @@ function createSendMessageMock() { async ( _to: string, content: string, - _opts: SendMattermostMessageOptions, + opts: SendMattermostMessageOptions, ): Promise => { const messageId = `post-${++sendCount}`; return { @@ -58,7 +58,10 @@ function createSendMessageMock() { content: content.trim(), receipt: createMessageReceiptFromOutboundResults({ results: [{ channel: "mattermost", messageId, channelId: "channel-1" }], - kind: "text", + kind: + "buttons" in opts && Array.isArray(opts.buttons) && opts.buttons.length + ? "card" + : "text", }), }; }, @@ -66,6 +69,189 @@ function createSendMessageMock() { } describe("deliverMattermostReplyPayload", () => { + it.each<{ + name: string; + payload: ReplyPayload; + expectedText: string; + expectedButtonValue?: string; + }>([ + { + name: "presentation-only title", + payload: { presentation: { title: "Build complete", blocks: [] } }, + expectedText: "Build complete", + }, + { + name: "presentation-only text", + payload: { presentation: { blocks: [{ type: "text", text: "Release finished" }] } }, + expectedText: "Release finished", + }, + { + name: "presentation-only native value button", + payload: { + presentation: { + blocks: [{ type: "buttons", buttons: [{ label: "Open", value: "open" }] }], + }, + }, + expectedText: "- Open", + expectedButtonValue: "open", + }, + { + name: "presentation-only URL button", + payload: { + presentation: { + blocks: [ + { + type: "buttons", + buttons: [{ label: "Docs", url: "https://example.com/docs" }], + }, + ], + }, + }, + expectedText: "- Docs: https://example.com/docs", + }, + { + name: "presentation-only select", + payload: { + presentation: { + blocks: [ + { + type: "select", + placeholder: "Environment", + options: [{ label: "Production", value: "production" }], + }, + ], + }, + }, + expectedText: "Environment:\n- Production", + }, + { + name: "authored text with native buttons", + payload: { + text: "Deploy finished", + presentation: { + blocks: [{ type: "buttons", buttons: [{ label: "Open", value: "open" }] }], + }, + }, + expectedText: "Deploy finished\n\n- Open", + expectedButtonValue: "open", + }, + { + name: "authored fallback without duplicate presentation text", + payload: { + text: "Already formatted", + presentationTextMode: "fallback", + presentation: { blocks: [{ type: "text", text: "Generated summary" }] }, + }, + expectedText: "Already formatted", + }, + { + name: "typed callback action stays text-only", + payload: { + presentation: { + blocks: [ + { + type: "buttons", + buttons: [ + { + label: "Inspect", + action: { type: "callback", value: "private-callback-token" }, + }, + ], + }, + ], + }, + }, + expectedText: "- Inspect", + }, + ])( + "delivers $name through the normal reply owner", + async ({ payload, expectedText, expectedButtonValue }) => { + const sendMessage = createSendMessageMock(); + const result = await deliverMattermostReplyPayload({ + core: createReplyDeliveryCore(), + cfg: {}, + payload, + channelId: "town-square", + accountId: "default", + replyToId: "root-post", + textLimit: 4000, + tableMode: "off", + sendMessage, + }); + + expect(sendMessage).toHaveBeenCalledTimes(1); + expect(sendMessage).toHaveBeenCalledWith( + "channel:town-square", + expectedText, + expect.objectContaining({ accountId: "default", replyToId: "root-post" }), + ); + const options = sendMessage.mock.calls[0]![2]; + if (expectedButtonValue) { + expect(options).toMatchObject({ + buttons: [[{ id: expectedButtonValue, callback_data: expectedButtonValue }]], + }); + } else { + expect("buttons" in options ? options.buttons : undefined).toBeUndefined(); + } + expect(result).toMatchObject({ outcome: "text", visibleReplySent: true }); + expect(result.receipt?.parts[0]?.kind).toBe(expectedButtonValue ? "card" : "text"); + }, + ); + + it("attaches presentation controls only to the first visible text chunk", async () => { + const sendMessage = createSendMessageMock(); + const core = createReplyDeliveryCore(); + core.channel.text.chunkMarkdownTextWithMode = vi.fn(() => ["alpha", "beta"]); + + await deliverMattermostReplyPayload({ + core, + cfg: {}, + payload: { + text: "alpha beta", + presentation: { + blocks: [{ type: "buttons", buttons: [{ label: "Open", value: "open" }] }], + }, + }, + channelId: "town-square", + accountId: "default", + textLimit: 6, + tableMode: "off", + sendMessage, + }); + + expect(sendMessage).toHaveBeenCalledTimes(2); + expect(sendMessage.mock.calls[0]![2]).toMatchObject({ + buttons: [[{ id: "open", callback_data: "open" }]], + }); + expect("buttons" in sendMessage.mock.calls[1]![2]).toBe(false); + }); + + it("keeps multiple-media presentations on the text/media fallback path", async () => { + const sendMessage = createSendMessageMock(); + + await deliverMattermostReplyPayload({ + core: createReplyDeliveryCore(), + cfg: {}, + payload: { + mediaUrls: ["https://example.com/1.png", "https://example.com/2.png"], + presentation: { + blocks: [{ type: "buttons", buttons: [{ label: "Open", value: "open" }] }], + }, + }, + channelId: "town-square", + accountId: "default", + textLimit: 4000, + tableMode: "off", + sendMessage, + }); + + expect(sendMessage).toHaveBeenCalledTimes(2); + expect(sendMessage.mock.calls[0]![1]).toBe("- Open"); + for (const call of sendMessage.mock.calls) { + expect("buttons" in call[2]).toBe(false); + } + }); + it("suppresses payloads flagged as reasoning", async () => { const sendMessage = createSendMessageMock(); const cfg = {} satisfies OpenClawConfig; diff --git a/extensions/mattermost/src/mattermost/reply-delivery.ts b/extensions/mattermost/src/mattermost/reply-delivery.ts index c8fe5d2aefa3..ec57e7d9e9e0 100644 --- a/extensions/mattermost/src/mattermost/reply-delivery.ts +++ b/extensions/mattermost/src/mattermost/reply-delivery.ts @@ -16,7 +16,7 @@ import { resolveSendableOutboundReplyParts, } from "openclaw/plugin-sdk/reply-payload"; import type { ReplyPayload } from "openclaw/plugin-sdk/reply-runtime"; -import { requiresMattermostMediaUpload } from "../normalize.js"; +import { requiresMattermostMediaUpload, resolveMattermostPresentation } from "../normalize.js"; import type { MattermostSendResult } from "./send.js"; type MarkdownTableMode = Parameters[1]; @@ -31,6 +31,7 @@ type SendMattermostMessage = ( mediaLocalRoots?: readonly string[]; requireMediaUpload?: boolean; replyToId?: string; + buttons?: Array; }, ) => Promise; @@ -75,11 +76,9 @@ export async function deliverMattermostReplyPayload(params: { suppression: { reason: "no_visible_result" }, }; } + const presentation = resolveMattermostPresentation(params.payload); const reply = resolveSendableOutboundReplyParts(params.payload, { - text: params.core.channel.text.convertMarkdownTables( - params.payload.text ?? "", - params.tableMode, - ), + text: params.core.channel.text.convertMarkdownTables(presentation.text, params.tableMode), }); const mediaLocalRoots = getAgentScopedMediaLocalRoots(params.cfg, params.agentId); const chunkMode = params.core.channel.text.resolveChunkMode( @@ -89,7 +88,21 @@ export async function deliverMattermostReplyPayload(params: { ); const results: MattermostSendResult[] = []; const acceptedContents: string[] = []; - const deliveryTarget = `channel:${params.channelId}`; + const sendAccepted = async (text: string, mediaUrl?: string) => { + const result = await params.sendMessage(`channel:${params.channelId}`, text, { + cfg: params.cfg, + accountId: params.accountId, + ...(mediaUrl ? { mediaUrl, mediaLocalRoots } : {}), + // Local media must upload successfully instead of silently posting only its caption. + ...(requiresMattermostMediaUpload(mediaUrl) ? { requireMediaUpload: true } : {}), + ...(results.length === 0 && reply.mediaUrls.length < 2 && presentation.buttons.length + ? { buttons: presentation.buttons } + : {}), + replyToId: params.replyToId, + }); + results.push(result); + acceptedContents.push(result.content); + }; let outcome: Exclude; try { outcome = await deliverTextOrMediaReply({ @@ -97,29 +110,8 @@ export async function deliverMattermostReplyPayload(params: { text: reply.text, chunkText: (value) => params.core.channel.text.chunkMarkdownTextWithMode(value, params.textLimit, chunkMode), - sendText: async (chunk) => { - const result = await params.sendMessage(deliveryTarget, chunk, { - cfg: params.cfg, - accountId: params.accountId, - replyToId: params.replyToId, - }); - results.push(result); - acceptedContents.push(result.content); - }, - sendMedia: async ({ mediaUrl, caption }) => { - // Require upload for local media so a failure surfaces instead of - // silently posting the caption alone (mirrors channel.ts send paths). - const result = await params.sendMessage(deliveryTarget, caption ?? "", { - cfg: params.cfg, - accountId: params.accountId, - mediaUrl, - mediaLocalRoots, - ...(requiresMattermostMediaUpload(mediaUrl) ? { requireMediaUpload: true } : {}), - replyToId: params.replyToId, - }); - results.push(result); - acceptedContents.push(result.content); - }, + sendText: sendAccepted, + sendMedia: ({ mediaUrl, caption }) => sendAccepted(caption ?? "", mediaUrl), }); } catch (error: unknown) { const failedPartial = isChannelPartialDeliveryError(error) ? error.deliveryResult : undefined; diff --git a/extensions/mattermost/src/normalize.ts b/extensions/mattermost/src/normalize.ts index 7cf7e7b98400..f61478207e9b 100644 --- a/extensions/mattermost/src/normalize.ts +++ b/extensions/mattermost/src/normalize.ts @@ -1,6 +1,48 @@ // Mattermost helper module supports normalize behavior. +import { + normalizeMessagePresentation, + renderMessagePresentationFallbackText, + resolveMessagePresentationControlValue, +} from "openclaw/plugin-sdk/interactive-runtime"; import { normalizeLowercaseStringOrEmpty } from "openclaw/plugin-sdk/string-coerce-runtime"; +export function resolveMattermostPresentation(params: { + text?: string; + presentation?: unknown; + presentationTextMode?: "fallback"; +}) { + const presentation = normalizeMessagePresentation(params.presentation); + const text = + !presentation || (params.presentationTextMode === "fallback" && params.text !== undefined) + ? (params.text ?? "") + : renderMessagePresentationFallbackText({ text: params.text, presentation }); + const buttons = presentation + ? presentation.blocks + .filter((block) => block.type === "buttons") + .map((block) => + block.buttons.flatMap((button) => { + if (button.action) { + return []; + } + const value = resolveMessagePresentationControlValue(button); + return value + ? [ + { + id: value, + text: button.label, + callback_data: value, + context: { callback_data: value }, + style: button.style, + }, + ] + : []; + }), + ) + .filter((row) => row.length > 0) + : []; + return { text, buttons }; +} + export function normalizeMattermostMessagingTarget(raw: string): string | undefined { const trimmed = raw.trim(); if (!trimmed) {