From da279041aba2aa71637fbba750bc0c27779d4f72 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Thu, 28 May 2026 00:32:28 +0100 Subject: [PATCH] fix(discord): suppress recovered tool warnings (#87451) --- .../monitor/message-handler.process.test.ts | 100 ++++++++++++------ .../src/monitor/message-handler.process.ts | 56 ++++++++-- src/auto-reply/reply-payload.ts | 4 + src/plugin-sdk/reply-payload.ts | 1 + 4 files changed, 121 insertions(+), 40 deletions(-) diff --git a/extensions/discord/src/monitor/message-handler.process.test.ts b/extensions/discord/src/monitor/message-handler.process.test.ts index 259c72f5da71..7b5e4c09ff0e 100644 --- a/extensions/discord/src/monitor/message-handler.process.test.ts +++ b/extensions/discord/src/monitor/message-handler.process.test.ts @@ -7,6 +7,7 @@ import { import type { ReplyPayload } from "openclaw/plugin-sdk/reply-dispatch-runtime"; import * as runtimeEnvModule from "openclaw/plugin-sdk/runtime-env"; import { beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; +import { setReplyPayloadMetadata } from "../../../../src/auto-reply/reply-payload.js"; import type { DiscordMessagePreflightContext } from "./message-handler.preflight.js"; const sendMocks = vi.hoisted(() => ({ @@ -51,6 +52,16 @@ const editMessageDiscord = deliveryMocks.editMessageDiscord; const deliverDiscordReply = deliveryMocks.deliverDiscordReply; const createDiscordDraftStream = deliveryMocks.createDiscordDraftStream; +function createNonTerminalToolWarningPayload(): ReplyPayload { + return setReplyPayloadMetadata( + { + text: "⚠️ 🛠️ `run openclaw definitely-not-a-real-subcommand (agent)` failed", + isError: true, + }, + { nonTerminalToolErrorWarning: true }, + ); +} + vi.mock("../send.js", () => ({ reactMessageDiscord: async ( channelId: string, @@ -2237,14 +2248,11 @@ describe("processDiscordMessage draft streaming", () => { expect(deliverDiscordReply).toHaveBeenCalledTimes(1); }); - it("keeps finalized previews when later tool warning finals are delivered", async () => { + it("drops later tool warning finals after preview final replies", async () => { const draftStream = createMockDraftStreamForTest(); dispatchInboundMessage.mockImplementationOnce(async (params?: DispatchInboundParams) => { await params?.dispatcher.sendFinalReply({ text: "delivery survived" }); - await params?.dispatcher.sendFinalReply({ - text: "⚠️ 🛠️ `run openclaw definitely-not-a-real-subcommand (agent)` failed", - isError: true, - } as never); + await params?.dispatcher.sendFinalReply(createNonTerminalToolWarningPayload()); return { queuedFinal: true, counts: { final: 2, tool: 0, block: 0 } }; }); @@ -2257,24 +2265,13 @@ describe("processDiscordMessage draft streaming", () => { expectPreviewEditContent("delivery survived"); expect(draftStream.clear).not.toHaveBeenCalled(); expect(draftStream.messageId()).toBe("preview-1"); - expect(deliverDiscordReply).toHaveBeenCalledTimes(1); - expect(firstMockArg(deliverDiscordReply, "deliverDiscordReply")).toMatchObject({ - replies: [ - { - text: "⚠️ 🛠️ `run openclaw definitely-not-a-real-subcommand (agent)` failed", - isError: true, - }, - ], - }); + expect(deliverDiscordReply).not.toHaveBeenCalled(); }); - it("keeps draft previews when tool warning finals arrive before recovered replies", async () => { + it("drops earlier tool warning finals when recovered replies arrive", async () => { const draftStream = createMockDraftStreamForTest(); dispatchInboundMessage.mockImplementationOnce(async (params?: DispatchInboundParams) => { - await params?.dispatcher.sendFinalReply({ - text: "⚠️ 🛠️ `run openclaw definitely-not-a-real-subcommand (agent)` failed", - isError: true, - } as never); + await params?.dispatcher.sendFinalReply(createNonTerminalToolWarningPayload()); await params?.dispatcher.sendFinalReply({ text: "delivery recovered" }); return { queuedFinal: true, counts: { final: 2, tool: 0, block: 0 } }; }); @@ -2288,6 +2285,24 @@ describe("processDiscordMessage draft streaming", () => { expectPreviewEditContent("delivery recovered"); expect(draftStream.clear).not.toHaveBeenCalled(); expect(draftStream.messageId()).toBe("preview-1"); + expect(deliverDiscordReply).not.toHaveBeenCalled(); + }); + + it("delivers tool warning finals when no recovered reply is available", async () => { + const draftStream = createMockDraftStreamForTest(); + dispatchInboundMessage.mockImplementationOnce(async (params?: DispatchInboundParams) => { + await params?.dispatcher.sendFinalReply(createNonTerminalToolWarningPayload()); + return { queuedFinal: true, counts: { final: 1, tool: 0, block: 0 } }; + }); + + const ctx = await createAutomaticSourceDeliveryContext({ + discordConfig: { streamMode: "partial", maxLinesPerMessage: 5 }, + }); + + await runProcessDiscordMessage(ctx); + + expect(editMessageDiscord).not.toHaveBeenCalled(); + expect(draftStream.clear).toHaveBeenCalledTimes(1); expect(deliverDiscordReply).toHaveBeenCalledTimes(1); expect(firstMockArg(deliverDiscordReply, "deliverDiscordReply")).toMatchObject({ replies: [ @@ -2299,6 +2314,36 @@ describe("processDiscordMessage draft streaming", () => { }); }); + it("keeps mutating tool warning finals after successful-looking replies", async () => { + const draftStream = createMockDraftStreamForTest(); + dispatchInboundMessage.mockImplementationOnce(async (params?: DispatchInboundParams) => { + await params?.dispatcher.sendFinalReply({ text: "Done." }); + await params?.dispatcher.sendFinalReply({ + text: "⚠️ 🛠️ `write file (agent)` failed", + isError: true, + } as never); + return { queuedFinal: true, counts: { final: 2, tool: 0, block: 0 } }; + }); + + const ctx = await createAutomaticSourceDeliveryContext({ + discordConfig: { streamMode: "partial", maxLinesPerMessage: 5 }, + }); + + await runProcessDiscordMessage(ctx); + + expectPreviewEditContent("Done."); + expect(draftStream.clear).not.toHaveBeenCalled(); + expect(deliverDiscordReply).toHaveBeenCalledTimes(1); + expect(firstMockArg(deliverDiscordReply, "deliverDiscordReply")).toMatchObject({ + replies: [ + { + text: "⚠️ 🛠️ `write file (agent)` failed", + isError: true, + }, + ], + }); + }); + it("suppresses reasoning payload delivery to Discord", async () => { mockDispatchSingleBlockReply({ text: "thinking...", isReasoning: true }); await processStreamOffDiscordMessage(); @@ -2448,17 +2493,14 @@ describe("processDiscordMessage draft streaming", () => { expectPreviewEditContent("done"); }); - it("keeps finalized progress previews when later tool warning finals are delivered", async () => { + it("drops later tool warning finals after progress preview final replies", async () => { const draftStream = createMockDraftStreamForTest(); dispatchInboundMessage.mockImplementationOnce(async (params?: DispatchInboundParams) => { await params?.replyOptions?.onToolStart?.({ name: "exec", phase: "start" }); await params?.replyOptions?.onItemEvent?.({ progressText: "exec done" }); await params?.dispatcher.sendFinalReply({ text: "delivery survived" }); - await params?.dispatcher.sendFinalReply({ - text: "⚠️ 🛠️ `run openclaw definitely-not-a-real-subcommand (agent)` failed", - isError: true, - } as never); + await params?.dispatcher.sendFinalReply(createNonTerminalToolWarningPayload()); return { queuedFinal: true, counts: { final: 2, tool: 0, block: 0 } }; }); @@ -2479,15 +2521,7 @@ describe("processDiscordMessage draft streaming", () => { expectPreviewEditContent("delivery survived"); expect(draftStream.clear).not.toHaveBeenCalled(); expect(draftStream.messageId()).toBe("preview-1"); - expect(deliverDiscordReply).toHaveBeenCalledTimes(1); - expect(firstMockArg(deliverDiscordReply, "deliverDiscordReply")).toMatchObject({ - replies: [ - { - text: "⚠️ 🛠️ `run openclaw definitely-not-a-real-subcommand (agent)` failed", - isError: true, - }, - ], - }); + expect(deliverDiscordReply).not.toHaveBeenCalled(); }); it("uses raw tool-progress detail in Discord progress drafts", async () => { diff --git a/extensions/discord/src/monitor/message-handler.process.ts b/extensions/discord/src/monitor/message-handler.process.ts index db15f841bb02..311f8fcb4293 100644 --- a/extensions/discord/src/monitor/message-handler.process.ts +++ b/extensions/discord/src/monitor/message-handler.process.ts @@ -37,6 +37,7 @@ import { createChannelHistoryWindow } from "openclaw/plugin-sdk/reply-history"; import { buildTtsSupplementMediaPayload, getReplyPayloadTtsSupplement, + isReplyPayloadNonTerminalToolErrorWarning, resolveSendableOutboundReplyParts, } from "openclaw/plugin-sdk/reply-payload"; import type { ReplyDispatchKind, ReplyPayload } from "openclaw/plugin-sdk/reply-runtime"; @@ -104,6 +105,13 @@ function formatDiscordReplyDeliveryFailure(params: { return `discord ${params.kind} reply failed (${context}): ${String(params.err)}`; } +function isFallbackOnlyToolWarningFinal(payload: ReplyPayload): boolean { + if (payload.isError !== true || !isReplyPayloadNonTerminalToolErrorWarning(payload)) { + return false; + } + return !resolveSendableOutboundReplyParts(payload).hasMedia; +} + type DiscordReplySkipReason = "aborted before delivery" | "reasoning payload"; export function formatDiscordReplySkip(params: { @@ -537,6 +545,16 @@ export async function processDiscordMessage( draftPreview.markFinalReplyStarted(); observer?.onFinalReplyStart?.(); }; + let userFacingFinalDelivered = false; + let pendingToolWarningFinal: + | { payload: ReplyPayload; info: { kind: ReplyDispatchKind } } + | undefined; + const markUserFacingFinalDelivered = () => { + userFacingFinalDelivered = true; + pendingToolWarningFinal = undefined; + draftPreview.markFinalReplyDelivered(); + observer?.onFinalReplyDelivered?.(); + }; const beforeDiscordPayloadDelivery = ( payload: ReplyPayload, info: { kind: ReplyDispatchKind }, @@ -570,7 +588,7 @@ export async function processDiscordMessage( return null; } } - if (info.kind === "final") { + if (info.kind === "final" && !isFallbackOnlyToolWarningFinal(payload)) { draftPreview.markFinalReplyStarted(); } return payload; @@ -579,6 +597,7 @@ export async function processDiscordMessage( const deliverDiscordPayload = async ( payload: ReplyPayload, info: { kind: ReplyDispatchKind }, + options?: { allowFallbackOnlyToolWarning?: boolean }, ) => { if (isProcessAborted(abortSignal)) { // Surface so operators don't chase missing replies when an abort @@ -606,6 +625,16 @@ export async function processDiscordMessage( ); return { visibleReplySent: false }; } + if ( + isFinal && + !options?.allowFallbackOnlyToolWarning && + isFallbackOnlyToolWarningFinal(payload) + ) { + if (!userFacingFinalDelivered) { + pendingToolWarningFinal = { payload, info }; + } + return { visibleReplySent: false }; + } if (isFinal) { draftPreview.markFinalReplyStarted(); } @@ -679,10 +708,9 @@ export async function processDiscordMessage( }); }, onPreviewFinalized: () => { - draftPreview.markFinalReplyDelivered(); + markUserFacingFinalDelivered(); draftPreview.markPreviewFinalized(); replyReference.markSent(); - observer?.onFinalReplyDelivered?.(); }, buildSupplementalPayload: () => ttsSupplement ? buildTtsSupplementMediaPayload(effectivePayload) : undefined, @@ -759,9 +787,8 @@ export async function processDiscordMessage( return true; }, onNormalDelivered: () => { - draftPreview.markFinalReplyDelivered(); + markUserFacingFinalDelivered(); replyReference.markSent(); - observer?.onFinalReplyDelivered?.(); }, }); if (result.kind !== "normal-skipped") { @@ -807,8 +834,7 @@ export async function processDiscordMessage( }); replyReference.markSent(); if (isFinal && payload.isError !== true) { - draftPreview.markFinalReplyDelivered(); - observer?.onFinalReplyDelivered?.(); + markUserFacingFinalDelivered(); } return { visibleReplySent: true }; }; @@ -837,6 +863,21 @@ export async function processDiscordMessage( null; let dispatchError = false; let dispatchAborted = false; + const deliverPendingToolWarningFinalIfNeeded = async () => { + if (!pendingToolWarningFinal || userFacingFinalDelivered || isProcessAborted(abortSignal)) { + return; + } + const pending = pendingToolWarningFinal; + pendingToolWarningFinal = undefined; + try { + await deliverDiscordPayload(pending.payload, pending.info, { + allowFallbackOnlyToolWarning: true, + }); + } catch (err) { + dispatchError = true; + onDiscordDeliveryError(err, pending.info); + } + }; try { if (isProcessAborted(abortSignal)) { dispatchAborted = true; @@ -1026,6 +1067,7 @@ export async function processDiscordMessage( dispatchAborted = true; return; } + await deliverPendingToolWarningFinalIfNeeded(); } catch (err) { if (isProcessAborted(abortSignal)) { dispatchAborted = true; diff --git a/src/auto-reply/reply-payload.ts b/src/auto-reply/reply-payload.ts index 4202a32d62de..52bb77f7e65b 100644 --- a/src/auto-reply/reply-payload.ts +++ b/src/auto-reply/reply-payload.ts @@ -184,6 +184,10 @@ export function getReplyPayloadMetadata(payload: object): ReplyPayloadMetadata | return replyPayloadMetadata.get(payload); } +export function isReplyPayloadNonTerminalToolErrorWarning(payload: object): boolean { + return getReplyPayloadMetadata(payload)?.nonTerminalToolErrorWarning === true; +} + export function copyReplyPayloadMetadata(source: object, payload: T): T { const metadata = getReplyPayloadMetadata(source); return metadata ? setReplyPayloadMetadata(payload, metadata) : payload; diff --git a/src/plugin-sdk/reply-payload.ts b/src/plugin-sdk/reply-payload.ts index fb50b0cd2c12..fe8602bd575c 100644 --- a/src/plugin-sdk/reply-payload.ts +++ b/src/plugin-sdk/reply-payload.ts @@ -13,6 +13,7 @@ export type { ReplyPayloadTtsSupplement } from "../auto-reply/reply-payload.js"; export { buildTtsSupplementMediaPayload, getReplyPayloadTtsSupplement, + isReplyPayloadNonTerminalToolErrorWarning, isReplyPayloadTtsSupplement, markReplyPayloadAsTtsSupplement, } from "../auto-reply/reply-payload.js";