diff --git a/extensions/mattermost/src/mattermost/monitor.test.ts b/extensions/mattermost/src/mattermost/monitor.test.ts index 88075f1cb360..35625b15de32 100644 --- a/extensions/mattermost/src/mattermost/monitor.test.ts +++ b/extensions/mattermost/src/mattermost/monitor.test.ts @@ -9,6 +9,7 @@ import { canFinalizeMattermostPreviewInPlace, deliverMattermostReplyWithDraftPreview, evaluateMattermostMentionGate, + formatMattermostFinalDeliveryOutcomeLog, MattermostRetryableInboundError, processMattermostReplayGuardedPost, resolveMattermostReactionChannelId, @@ -555,6 +556,74 @@ describe("deliverMattermostReplyWithDraftPreview", () => { }); }); +describe("formatMattermostFinalDeliveryOutcomeLog", () => { + it("logs delivered only for visible text and media outcomes", () => { + expect( + formatMattermostFinalDeliveryOutcomeLog({ + outcome: "text", + payload: { text: "hello" } as never, + to: "channel:town-square", + accountId: "default", + agentId: "agent-1", + }), + ).toBe("delivered reply to channel:town-square"); + + expect( + formatMattermostFinalDeliveryOutcomeLog({ + outcome: "media", + payload: { mediaUrl: "https://example.com/a.png" } as never, + to: "channel:town-square", + accountId: "default", + agentId: "agent-1", + }), + ).toBe("delivered reply to channel:town-square"); + }); + + it("does not log delivered for empty no-send outcomes without diagnostic violations", () => { + expect( + formatMattermostFinalDeliveryOutcomeLog({ + outcome: "empty", + payload: { text: " \n\t " } as never, + to: "channel:town-square", + accountId: "default", + agentId: "agent-1", + }), + ).toBeUndefined(); + }); + + it("logs a diagnostic for substantive empty outcomes", () => { + expect( + formatMattermostFinalDeliveryOutcomeLog({ + outcome: "empty", + payload: { text: "work result" } as never, + to: "channel:town-square", + accountId: "default", + agentId: "agent-1", + }), + ).toBe( + "mattermost no-visible-reply: no-visible-reply-after-final-delivery" + + " to=channel:town-square" + + " accountId=default" + + " agentId=agent-1" + + " outcome=empty" + + " finalTextLength=11" + + " mediaUrlCount=0", + ); + }); + + it("does not log reasoning-suppressed outcomes", () => { + expect( + formatMattermostFinalDeliveryOutcomeLog({ + outcome: "reasoning_skipped", + payload: { text: "Reasoning: hidden" } as never, + to: "channel:town-square", + accountId: "default", + agentId: "agent-1", + }), + ).toBeUndefined(); + }); +}); + describe("shouldFinalizeMattermostPreviewAfterDispatch", () => { it("reuses the preview only for a single eligible final payload", () => { expect( diff --git a/extensions/mattermost/src/mattermost/monitor.ts b/extensions/mattermost/src/mattermost/monitor.ts index c6f85814f0cb..94881b559428 100644 --- a/extensions/mattermost/src/mattermost/monitor.ts +++ b/extensions/mattermost/src/mattermost/monitor.ts @@ -71,7 +71,10 @@ import { formatMattermostNoVisibleReplyLog, } from "./no-visible-reply-diagnostic.js"; import { runWithReconnect } from "./reconnect.js"; -import { deliverMattermostReplyPayload } from "./reply-delivery.js"; +import { + deliverMattermostReplyPayload, + type MattermostReplyDeliveryOutcome, +} from "./reply-delivery.js"; import type { ChannelAccountSnapshot, ChatType, @@ -383,6 +386,31 @@ export async function deliverMattermostReplyWithDraftPreview( }); } +export function formatMattermostFinalDeliveryOutcomeLog(params: { + outcome: MattermostReplyDeliveryOutcome; + payload: ReplyPayload; + to: string; + accountId: string; + agentId: string | undefined; +}): string | undefined { + const violation = evaluateMattermostNoVisibleReply({ + outcome: params.outcome, + payload: params.payload, + }); + if (violation) { + return formatMattermostNoVisibleReplyLog({ + violation, + to: params.to, + accountId: params.accountId, + agentId: params.agentId, + }); + } + if (params.outcome === "text" || params.outcome === "media") { + return `delivered reply to ${params.to}`; + } + return undefined; +} + export function resolveMattermostEffectiveReplyToId(params: { kind: ChatType; postId?: string | null; @@ -1700,18 +1728,15 @@ export async function monitorMattermostProvider(opts: MonitorMattermostOpts = {} tableMode, sendMessage: sendMessageMattermost, }); - const violation = evaluateMattermostNoVisibleReply({ outcome, payload }); - if (violation) { - runtime.log?.( - formatMattermostNoVisibleReplyLog({ - violation, - to, - accountId: account.accountId, - agentId: route.agentId, - }), - ); - } else if (outcome !== "reasoning_skipped") { - runtime.log?.(`delivered reply to ${to}`); + const deliveryLog = formatMattermostFinalDeliveryOutcomeLog({ + outcome, + payload, + to, + accountId: account.accountId, + agentId: route.agentId, + }); + if (deliveryLog) { + runtime.log?.(deliveryLog); } }, }); diff --git a/extensions/mattermost/src/mattermost/no-visible-reply-diagnostic.test.ts b/extensions/mattermost/src/mattermost/no-visible-reply-diagnostic.test.ts index d516a54e291f..0b51b6eecf39 100644 --- a/extensions/mattermost/src/mattermost/no-visible-reply-diagnostic.test.ts +++ b/extensions/mattermost/src/mattermost/no-visible-reply-diagnostic.test.ts @@ -31,7 +31,7 @@ describe("evaluateMattermostNoVisibleReply", () => { }); }); - it("counts entries from mediaUrls[] alongside the singular mediaUrl", () => { + it("follows the SDK legacy media fallback when counting media URLs", () => { const violation = evaluateMattermostNoVisibleReply({ outcome: "empty", payload: { @@ -39,7 +39,7 @@ describe("evaluateMattermostNoVisibleReply", () => { mediaUrls: ["https://example.org/b.png", "https://example.org/c.png"], }, }); - expect(violation?.mediaUrlCount).toBe(3); + expect(violation?.mediaUrlCount).toBe(2); }); it("does not flag reasoning_skipped outcome (intentional suppression)", () => { diff --git a/extensions/mattermost/src/mattermost/no-visible-reply-diagnostic.ts b/extensions/mattermost/src/mattermost/no-visible-reply-diagnostic.ts index b3eca6417cda..847ffac45913 100644 --- a/extensions/mattermost/src/mattermost/no-visible-reply-diagnostic.ts +++ b/extensions/mattermost/src/mattermost/no-visible-reply-diagnostic.ts @@ -1,3 +1,4 @@ +import { countOutboundMedia } from "openclaw/plugin-sdk/reply-payload"; import type { ReplyPayload } from "openclaw/plugin-sdk/reply-runtime"; import type { MattermostReplyDeliveryOutcome } from "./reply-delivery.js"; @@ -8,14 +9,6 @@ export type MattermostNoVisibleReplyViolation = { mediaUrlCount: number; }; -function countMediaUrls(payload: ReplyPayload): number { - const single = typeof payload.mediaUrl === "string" && payload.mediaUrl.length > 0 ? 1 : 0; - const list = Array.isArray(payload.mediaUrls) - ? payload.mediaUrls.filter((url) => typeof url === "string" && url.length > 0).length - : 0; - return single + list; -} - /** * Detects the #80501 symptom: `deliverMattermostReplyPayload` accepted a * substantive (non-reasoning) payload, called the underlying @@ -38,7 +31,7 @@ export function evaluateMattermostNoVisibleReply(params: { return null; } const finalText = typeof params.payload.text === "string" ? params.payload.text.trim() : ""; - const mediaUrlCount = countMediaUrls(params.payload); + const mediaUrlCount = countOutboundMedia(params.payload); // If the payload had no text and no media even nominally, the run had // nothing to send and "empty" is the correct outcome — do not flag. if (finalText.length === 0 && mediaUrlCount === 0) { diff --git a/extensions/mattermost/src/mattermost/reply-delivery.test.ts b/extensions/mattermost/src/mattermost/reply-delivery.test.ts index b253df035e16..7a09c178f258 100644 --- a/extensions/mattermost/src/mattermost/reply-delivery.test.ts +++ b/extensions/mattermost/src/mattermost/reply-delivery.test.ts @@ -190,14 +190,13 @@ describe("deliverMattermostReplyPayload", () => { accountId: "default", mediaUrl, replyToId: "root-post", - mediaLocalRoots: [ - path.join(os.tmpdir(), "openclaw"), + mediaLocalRoots: expect.arrayContaining([ path.join(stateDir, "media"), path.join(stateDir, "canvas"), path.join(stateDir, "workspace"), path.join(stateDir, "sandboxes"), path.join(stateDir, `workspace-${agentId}`), - ], + ]), }); } finally { if (previousStateDir === undefined) {