diff --git a/CHANGELOG.md b/CHANGELOG.md index f2d27b49866..fb04c6b100c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -151,6 +151,7 @@ Docs: https://docs.openclaw.ai - Telegram/groups: in single-account setups, treat an explicit empty `accounts..groups: {}` map the same as undefined so the root `channels.telegram.groups` allowlist still applies, instead of silently dropping every group update under the default `groupPolicy: "allowlist"`. Multi-account semantics are unchanged so per-account explicit-empty groups still scope-disable a single account without affecting siblings; the explicit way to block all groups for any account remains `groupPolicy: "disabled"`. Fixes #79427. (#81030) Thanks @kinjitakabe. - Codex (app-server): project user-configured `mcp.servers` into new Codex thread configs, matching the codex-cli runtime's existing `-c mcp_servers=...` behavior so app-server-runtime agents see the same user MCP servers the CLI runtime already exposes. Plugin-curated apps remain attached via the separate `apps` config patch. Fixes #80814. Thanks @kinjitakabe. - Enforce Slack plugin approval button authorization [AI]. (#80899) Thanks @pgondhi987. +- Mattermost: log a structured `mattermost no-visible-reply` diagnostic when a substantive (non-reasoning) final reply payload reaches `deliverMattermostReplyPayload` but the underlying `deliverTextOrMediaReply` returns `"empty"` — previously the run completed with a misleading `delivered reply to ` log even though no Mattermost API send happened, masking silent completions in channel/thread contexts. No behavior change; the diagnostic surfaces the failure so operators can detect it instead of seeing the agent appear to go silent. Fixes #80501. Thanks @robbyproc87. - Recognize PowerShell -ec inline commands [AI]. (#80893) Thanks @pgondhi987. - fix(qqbot): authorize approval button callbacks [AI]. (#80892) Thanks @pgondhi987. - Telegram: render supported HTML tags in streamed and durable replies instead of showing literal markup. (#80977) diff --git a/extensions/mattermost/src/mattermost/monitor.ts b/extensions/mattermost/src/mattermost/monitor.ts index 14ff0eaebd8..c6f85814f0c 100644 --- a/extensions/mattermost/src/mattermost/monitor.ts +++ b/extensions/mattermost/src/mattermost/monitor.ts @@ -66,6 +66,10 @@ import { type MattermostEventPayload, type MattermostWebSocketFactory, } from "./monitor-websocket.js"; +import { + evaluateMattermostNoVisibleReply, + formatMattermostNoVisibleReplyLog, +} from "./no-visible-reply-diagnostic.js"; import { runWithReconnect } from "./reconnect.js"; import { deliverMattermostReplyPayload } from "./reply-delivery.js"; import type { @@ -1680,7 +1684,7 @@ export async function monitorMattermostProvider(opts: MonitorMattermostOpts = {} previewState, logVerboseMessage, deliverFinal: async () => { - await deliverMattermostReplyPayload({ + const outcome = await deliverMattermostReplyPayload({ core, cfg, payload, @@ -1696,7 +1700,19 @@ export async function monitorMattermostProvider(opts: MonitorMattermostOpts = {} tableMode, sendMessage: sendMessageMattermost, }); - runtime.log?.(`delivered reply to ${to}`); + 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}`); + } }, }); }, diff --git a/extensions/mattermost/src/mattermost/no-visible-reply-diagnostic.test.ts b/extensions/mattermost/src/mattermost/no-visible-reply-diagnostic.test.ts new file mode 100644 index 00000000000..d516a54e291 --- /dev/null +++ b/extensions/mattermost/src/mattermost/no-visible-reply-diagnostic.test.ts @@ -0,0 +1,140 @@ +import { describe, expect, it } from "vitest"; +import { + evaluateMattermostNoVisibleReply, + formatMattermostNoVisibleReplyLog, +} from "./no-visible-reply-diagnostic.js"; + +describe("evaluateMattermostNoVisibleReply", () => { + it("flags substantive text payloads that delivered empty (regression: #80501)", () => { + const violation = evaluateMattermostNoVisibleReply({ + outcome: "empty", + payload: { text: "Here is the result of the work I did..." }, + }); + expect(violation).toStrictEqual({ + reason: "no-visible-reply-after-final-delivery", + outcome: "empty", + finalTextLength: "Here is the result of the work I did...".length, + mediaUrlCount: 0, + }); + }); + + it("flags payloads with media URLs that delivered empty (regression: #80501)", () => { + const violation = evaluateMattermostNoVisibleReply({ + outcome: "empty", + payload: { mediaUrl: "https://example.org/a.png" }, + }); + expect(violation).toStrictEqual({ + reason: "no-visible-reply-after-final-delivery", + outcome: "empty", + finalTextLength: 0, + mediaUrlCount: 1, + }); + }); + + it("counts entries from mediaUrls[] alongside the singular mediaUrl", () => { + const violation = evaluateMattermostNoVisibleReply({ + outcome: "empty", + payload: { + mediaUrl: "https://example.org/a.png", + mediaUrls: ["https://example.org/b.png", "https://example.org/c.png"], + }, + }); + expect(violation?.mediaUrlCount).toBe(3); + }); + + it("does not flag reasoning_skipped outcome (intentional suppression)", () => { + expect( + evaluateMattermostNoVisibleReply({ + outcome: "reasoning_skipped", + payload: { text: "Reasoning: hidden" }, + }), + ).toBeNull(); + }); + + it("does not flag text outcome (visible delivery happened)", () => { + expect( + evaluateMattermostNoVisibleReply({ + outcome: "text", + payload: { text: "hello" }, + }), + ).toBeNull(); + }); + + it("does not flag media outcome (visible delivery happened)", () => { + expect( + evaluateMattermostNoVisibleReply({ + outcome: "media", + payload: { mediaUrl: "https://example.org/a.png" }, + }), + ).toBeNull(); + }); + + it("does not flag empty outcome when the payload was nominally empty (no text or media at all)", () => { + expect( + evaluateMattermostNoVisibleReply({ + outcome: "empty", + payload: {}, + }), + ).toBeNull(); + expect( + evaluateMattermostNoVisibleReply({ + outcome: "empty", + payload: { text: "" }, + }), + ).toBeNull(); + expect( + evaluateMattermostNoVisibleReply({ + outcome: "empty", + payload: { text: " \n\t " }, + }), + ).toBeNull(); + }); + + it("trims whitespace when measuring finalTextLength", () => { + const violation = evaluateMattermostNoVisibleReply({ + outcome: "empty", + payload: { text: " hello " }, + }); + expect(violation?.finalTextLength).toBe("hello".length); + }); +}); + +describe("formatMattermostNoVisibleReplyLog", () => { + it("emits a grep-friendly single-line diagnostic with the expected key/value pairs", () => { + const line = formatMattermostNoVisibleReplyLog({ + violation: { + reason: "no-visible-reply-after-final-delivery", + outcome: "empty", + finalTextLength: 137, + mediaUrlCount: 0, + }, + to: "channel:town-square", + accountId: "default", + agentId: "main", + }); + expect(line).toBe( + "mattermost no-visible-reply: no-visible-reply-after-final-delivery" + + " to=channel:town-square" + + " accountId=default" + + " agentId=main" + + " outcome=empty" + + " finalTextLength=137" + + " mediaUrlCount=0", + ); + }); + + it("falls back to unknown when agentId is undefined", () => { + const line = formatMattermostNoVisibleReplyLog({ + violation: { + reason: "no-visible-reply-after-final-delivery", + outcome: "empty", + finalTextLength: 1, + mediaUrlCount: 0, + }, + to: "channel:x", + accountId: "y", + agentId: undefined, + }); + expect(line).toContain("agentId=unknown"); + }); +}); diff --git a/extensions/mattermost/src/mattermost/no-visible-reply-diagnostic.ts b/extensions/mattermost/src/mattermost/no-visible-reply-diagnostic.ts new file mode 100644 index 00000000000..b3eca6417cd --- /dev/null +++ b/extensions/mattermost/src/mattermost/no-visible-reply-diagnostic.ts @@ -0,0 +1,70 @@ +import type { ReplyPayload } from "openclaw/plugin-sdk/reply-runtime"; +import type { MattermostReplyDeliveryOutcome } from "./reply-delivery.js"; + +export type MattermostNoVisibleReplyViolation = { + reason: "no-visible-reply-after-final-delivery"; + outcome: MattermostReplyDeliveryOutcome; + finalTextLength: number; + 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 + * `deliverTextOrMediaReply`, and the outcome was `"empty"` — meaning the + * payload had no text and no media to send, so no Mattermost API call + * happened. The agent's run completes successfully, but no visible + * channel/thread reply ever surfaces to the user. + * + * Returns a structured violation when the outcome is `"empty"` for a payload + * that nominally carried user-facing content (text or media bytes that ended + * up dropped by `resolveSendableOutboundReplyParts`/`sendMediaWithLeadingCaption`). + * Returns `null` for `"reasoning_skipped"` (intentional suppression), + * `"text"`, or `"media"` (successful visible sends). + */ +export function evaluateMattermostNoVisibleReply(params: { + outcome: MattermostReplyDeliveryOutcome; + payload: ReplyPayload; +}): MattermostNoVisibleReplyViolation | null { + if (params.outcome !== "empty") { + return null; + } + const finalText = typeof params.payload.text === "string" ? params.payload.text.trim() : ""; + const mediaUrlCount = countMediaUrls(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) { + return null; + } + return { + reason: "no-visible-reply-after-final-delivery", + outcome: params.outcome, + finalTextLength: finalText.length, + mediaUrlCount, + }; +} + +export function formatMattermostNoVisibleReplyLog(params: { + violation: MattermostNoVisibleReplyViolation; + to: string; + accountId: string; + agentId: string | undefined; +}): string { + return ( + `mattermost no-visible-reply: ${params.violation.reason}` + + ` to=${params.to}` + + ` accountId=${params.accountId}` + + ` agentId=${params.agentId ?? "unknown"}` + + ` outcome=${params.violation.outcome}` + + ` finalTextLength=${params.violation.finalTextLength}` + + ` mediaUrlCount=${params.violation.mediaUrlCount}` + ); +} diff --git a/extensions/mattermost/src/mattermost/reply-delivery.test.ts b/extensions/mattermost/src/mattermost/reply-delivery.test.ts index 3699922aa5f..b253df035e1 100644 --- a/extensions/mattermost/src/mattermost/reply-delivery.test.ts +++ b/extensions/mattermost/src/mattermost/reply-delivery.test.ts @@ -43,7 +43,7 @@ describe("deliverMattermostReplyPayload", () => { const cfg = {} satisfies OpenClawConfig; const core = createReplyDeliveryCore(); - await deliverMattermostReplyPayload({ + const outcome = await deliverMattermostReplyPayload({ core, cfg, payload: { text: "hidden", isReasoning: true }, @@ -57,6 +57,33 @@ describe("deliverMattermostReplyPayload", () => { }); expect(sendMessage).not.toHaveBeenCalled(); + expect(outcome).toBe("reasoning_skipped"); + }); + + it("returns 'empty' for substantive text that produced no send (regression: #80501)", async () => { + const sendMessage = vi.fn(async () => undefined); + const cfg = {} satisfies OpenClawConfig; + const core = createReplyDeliveryCore(); + // Make the markdown table converter strip the text to empty so + // deliverTextOrMediaReply sees an empty chunked text and returns "empty". + core.channel.text.convertMarkdownTables = vi.fn(() => ""); + core.channel.text.chunkMarkdownTextWithMode = vi.fn(() => []); + + const outcome = await deliverMattermostReplyPayload({ + core, + cfg, + payload: { text: "non-trivial input that the converter strips" }, + to: "channel:town-square", + accountId: "default", + agentId: "agent-1", + replyToId: "root-post", + textLimit: 4000, + tableMode: "off", + sendMessage, + }); + + expect(sendMessage).not.toHaveBeenCalled(); + expect(outcome).toBe("empty"); }); it("suppresses reasoning-prefixed payloads even without an explicit flag", async () => { @@ -188,7 +215,7 @@ describe("deliverMattermostReplyPayload", () => { const core = createReplyDeliveryCore(); core.channel.text.chunkMarkdownTextWithMode = vi.fn(() => ["hello"]); - await deliverMattermostReplyPayload({ + const outcome = await deliverMattermostReplyPayload({ core, cfg, payload: { text: "hello" }, @@ -207,5 +234,6 @@ describe("deliverMattermostReplyPayload", () => { accountId: "default", replyToId: "root-post", }); + expect(outcome).toBe("text"); }); }); diff --git a/extensions/mattermost/src/mattermost/reply-delivery.ts b/extensions/mattermost/src/mattermost/reply-delivery.ts index b34c9668be1..a7c12a73e22 100644 --- a/extensions/mattermost/src/mattermost/reply-delivery.ts +++ b/extensions/mattermost/src/mattermost/reply-delivery.ts @@ -21,6 +21,14 @@ type SendMattermostMessage = ( }, ) => Promise; +/** + * Result of `deliverMattermostReplyPayload`. Callers in `monitor.ts` use this + * to distinguish a successful visible send from an intentionally suppressed + * reasoning payload from a substantive payload that ended up sending nothing + * (the silent-completion symptom in #80501). + */ +export type MattermostReplyDeliveryOutcome = "reasoning_skipped" | "empty" | "text" | "media"; + export async function deliverMattermostReplyPayload(params: { core: PluginRuntime; cfg: OpenClawConfig; @@ -32,9 +40,9 @@ export async function deliverMattermostReplyPayload(params: { textLimit: number; tableMode: MarkdownTableMode; sendMessage: SendMattermostMessage; -}): Promise { +}): Promise { if (isReasoningReplyPayload(params.payload)) { - return; + return "reasoning_skipped"; } const reply = resolveSendableOutboundReplyParts(params.payload, { text: params.core.channel.text.convertMarkdownTables( @@ -48,7 +56,7 @@ export async function deliverMattermostReplyPayload(params: { "mattermost", params.accountId, ); - await deliverTextOrMediaReply({ + return await deliverTextOrMediaReply({ payload: params.payload, text: reply.text, chunkText: (value) =>