mirror of
https://github.com/openclaw/openclaw.git
synced 2026-05-26 09:25:13 +00:00
fix(mattermost): diagnose silent final-delivery completions
`deliverMattermostReplyPayload` accepted a substantive (non-reasoning) reply payload, called the shared `deliverTextOrMediaReply`, and dropped its `"empty"|"text"|"media"` return value on the floor. When the underlying chunker or media-resolution produced no text and no media to send, the function returned `Promise<void>` and the caller in `monitor.ts` unconditionally logged `delivered reply to <channel>` — masking a silent completion where no Mattermost API send ever happened (the symptom in #80501). Thread the outcome through the helper, evaluate it against the original payload to distinguish intentional reasoning suppression from a substantive payload that vanished, and log a structured `mattermost no-visible-reply` diagnostic for the substantive-vanished case. The misleading "delivered reply to" log now only fires on actual visible delivery; reasoning-skipped payloads correctly stay silent. No behavior change: visible-delivery decisions, preview-finalization, and the existing reasoning-suppression contract are untouched. Operators can now grep the new diagnostic to detect the failure class instead of seeing the agent appear to go silent. Fixes #80501.
This commit is contained in:
committed by
Peter Steinberger
parent
10d2f41c83
commit
6789fe248b
@@ -151,6 +151,7 @@ Docs: https://docs.openclaw.ai
|
||||
- Telegram/groups: in single-account setups, treat an explicit empty `accounts.<id>.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 <channel>` 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)
|
||||
|
||||
@@ -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}`);
|
||||
}
|
||||
},
|
||||
});
|
||||
},
|
||||
|
||||
@@ -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");
|
||||
});
|
||||
});
|
||||
@@ -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}`
|
||||
);
|
||||
}
|
||||
@@ -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");
|
||||
});
|
||||
});
|
||||
|
||||
@@ -21,6 +21,14 @@ type SendMattermostMessage = (
|
||||
},
|
||||
) => Promise<unknown>;
|
||||
|
||||
/**
|
||||
* 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<void> {
|
||||
}): Promise<MattermostReplyDeliveryOutcome> {
|
||||
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) =>
|
||||
|
||||
Reference in New Issue
Block a user