From 9a699c7037647cd2ca96559e9da20bb094577dee Mon Sep 17 00:00:00 2001 From: Shakker Date: Sun, 19 Jul 2026 16:24:31 +0100 Subject: [PATCH] fix: keep chat errors out of assistant messages --- src/gateway/server-chat.agent-events.test.ts | 23 ++++---- src/gateway/server-chat.ts | 14 ----- src/gateway/server-methods/chat-broadcast.ts | 18 ------ .../chat.directive-tags.test.ts | 30 ++++------ .../chat.error-broadcast.test.ts | 59 ++++++++----------- 5 files changed, 47 insertions(+), 97 deletions(-) diff --git a/src/gateway/server-chat.agent-events.test.ts b/src/gateway/server-chat.agent-events.test.ts index 89b07f3849b2..821a576f2e2e 100644 --- a/src/gateway/server-chat.agent-events.test.ts +++ b/src/gateway/server-chat.agent-events.test.ts @@ -3235,17 +3235,17 @@ describe("agent event handler", () => { }, }); - expect( - expectDefined( - chatBroadcastCalls(broadcast)[0], - "chatBroadcastCalls(broadcast)[0] test invariant", - )[1], - ).toMatchObject({ + const payload = expectDefined( + chatBroadcastCalls(broadcast)[0], + "chatBroadcastCalls(broadcast)[0] test invariant", + )[1] as Record; + expect(payload).toMatchObject({ runId: "client-timeout", state: "error", stopReason: "timeout", errorMessage: "agent provider timeout", }); + expect(payload).not.toHaveProperty("message"); }); it.each([ @@ -4126,20 +4126,17 @@ describe("agent event handler", () => { state?: string; errorKind?: string; errorMessage?: string; - message?: { role?: string; content?: Array<{ type?: string; text?: string }> }; }; expect(payload.state).toBe("error"); expect(payload.errorKind).toBe("rate_limit"); expect(payload.errorMessage).toContain("Too many requests"); - expect(payload.message?.role).toBe("assistant"); - expect(payload.message?.content?.[0]?.text).toContain("Too many requests"); + expect(payload).not.toHaveProperty("message"); const nodePayload = sessionChatCalls(nodeSendToSession).at(-1)?.[2] as { errorKind?: string; - message?: { role?: string; content?: Array<{ type?: string; text?: string }> }; }; expect(nodePayload.errorKind).toBe("rate_limit"); - expect(nodePayload.message?.content?.[0]?.text).toContain("Too many requests"); + expect(nodePayload).not.toHaveProperty("message"); }); it("suppresses delayed lifecycle chat errors for active chat.send runs while still cleaning up", () => { @@ -4204,12 +4201,14 @@ describe("agent event handler", () => { ([, payload]) => (payload as { state?: string }).state === "error", ); expect(chatErrors).toHaveLength(1); - expectPayloadFields(chatErrors[0]?.[1], { + const errorPayload = chatErrors[0]?.[1] as Record; + expectPayloadFields(errorPayload, { runId: "run-chat-send", sessionKey: "session-chat-send", state: "error", errorMessage: "chat.send failed", }); + expect(errorPayload).not.toHaveProperty("message"); expect(chatRunState.registry.peek("run-chat-send")).toBeUndefined(); expect(clearAgentRunContext).toHaveBeenCalledWith("run-chat-send"); expect(agentRunSeq.has("run-chat-send")).toBe(false); diff --git a/src/gateway/server-chat.ts b/src/gateway/server-chat.ts index e2c8846ed7e1..a1b2a4509852 100644 --- a/src/gateway/server-chat.ts +++ b/src/gateway/server-chat.ts @@ -219,19 +219,6 @@ const CHAT_ERROR_KINDS = new Set([ "unknown", ]); -function buildChatErrorMessage(error: unknown): Record | undefined { - const raw = error ? formatForLog(error).trim() : ""; - if (!raw) { - return undefined; - } - const text = raw.startsWith("⚠️") || raw.startsWith("Error:") ? raw : `Error: ${raw}`; - return { - role: "assistant", - content: [{ type: "text", text }], - timestamp: Date.now(), - }; -} - function readChatErrorKind(value: unknown): ChatErrorKind | undefined { return typeof value === "string" && CHAT_ERROR_KINDS.has(value as ChatErrorKind) ? (value as ChatErrorKind) @@ -1120,7 +1107,6 @@ export function createAgentEventHandler({ seq, state: "error" as const, errorMessage: error ? formatForLog(error) : undefined, - message: buildChatErrorMessage(error), ...(errorKind && { errorKind }), ...(stopReason && { stopReason }), }; diff --git a/src/gateway/server-methods/chat-broadcast.ts b/src/gateway/server-methods/chat-broadcast.ts index 300be02523a0..7e5bad0540f8 100644 --- a/src/gateway/server-methods/chat-broadcast.ts +++ b/src/gateway/server-methods/chat-broadcast.ts @@ -145,7 +145,6 @@ export function broadcastChatError(params: { }): void { const seq = nextChatSeq(params.context, params.runId); const payloadAgentId = params.sessionKey === "global" ? params.agentId : undefined; - const errorText = params.errorMessage?.trim(); const payload = { runId: params.runId, sessionKey: params.sessionKey, @@ -153,23 +152,6 @@ export function broadcastChatError(params: { seq, state: "error" as const, errorMessage: params.errorMessage, - ...(errorText - ? { - message: { - role: "assistant", - content: [ - { - type: "text", - text: - errorText.startsWith("⚠️") || errorText.startsWith("Error:") - ? errorText - : `Error: ${errorText}`, - }, - ], - timestamp: Date.now(), - }, - } - : {}), }; params.context.broadcast("chat", payload, { sessionKeys: resolveGlobalAwareNodeChatDeliveryKeys({ diff --git a/src/gateway/server-methods/chat.directive-tags.test.ts b/src/gateway/server-methods/chat.directive-tags.test.ts index aa3637caee98..2fcfcada9e70 100644 --- a/src/gateway/server-methods/chat.directive-tags.test.ts +++ b/src/gateway/server-methods/chat.directive-tags.test.ts @@ -2344,7 +2344,7 @@ describe("chat directive tag stripping for non-streaming final payloads", () => ); }); - it("broadcasts an error terminal after an internal-ui source reply final", async () => { + it("does not broadcast an error terminal after an internal-ui source reply final", async () => { await createTranscriptFixture("openclaw-chat-send-agent-source-reply-error-"); mockState.triggerAgentRunStart = true; const sourceReply = setReplyPayloadMetadata( @@ -2375,35 +2375,28 @@ describe("chat directive tag stripping for non-streaming final payloads", () => const respond = vi.fn(); const context = createChatContext(); - await runNonStreamingChatSend({ + const broadcast = await runNonStreamingChatSend({ context, respond, idempotencyKey: "idem-agent-source-reply-error", message: "hello from codex", - waitFor: "dedupe", }); - const broadcasts = (context.broadcast as unknown as ReturnType).mock.calls.map( - ([, payload]) => payload as Record, - ); - expect(broadcasts[0]).toMatchObject({ + expect(broadcast).toMatchObject({ runId: "idem-agent-source-reply-error", sessionKey: "main", state: "final", }); - expect(extractFirstTextBlock(broadcasts[0])).toBe("Codex source reply"); - expect(broadcasts[1]).toMatchObject({ - runId: "idem-agent-source-reply-error", - sessionKey: "main", - state: "error", - errorMessage: "tool warning", - }); + expect(extractFirstTextBlock(broadcast)).toBe("Codex source reply"); + const errorBroadcasts = ( + context.broadcast as unknown as ReturnType + ).mock.calls.filter(([, payload]) => (payload as { state?: unknown })?.state === "error"); + expect(errorBroadcasts).toStrictEqual([]); const dedupe = context.dedupe.get("chat:idem-agent-source-reply-error"); - expect(dedupe?.ok).toBe(false); + expect(dedupe?.ok).toBe(true); expect(dedupe?.payload).toMatchObject({ runId: "idem-agent-source-reply-error", - status: "error", - summary: "tool warning", + status: "ok", }); }); @@ -2450,6 +2443,7 @@ describe("chat directive tag stripping for non-streaming final payloads", () => state: "error", errorMessage: "Model login expired. Re-authenticate, then try again.", }); + expect(broadcasts[0]).not.toHaveProperty("message"); const assistantEntries = await readActiveAssistantTranscriptMessages(); expect(assistantEntries).toHaveLength(1); expect(assistantEntries[0]?.content).toStrictEqual([ @@ -2493,6 +2487,7 @@ describe("chat directive tag stripping for non-streaming final payloads", () => state: "error", errorMessage, }); + expect(broadcast).not.toHaveProperty("message"); const finalBroadcasts = ( context.broadcast as unknown as ReturnType ).mock.calls.filter(([, payload]) => (payload as { state?: unknown })?.state === "final"); @@ -2528,6 +2523,7 @@ describe("chat directive tag stripping for non-streaming final payloads", () => state: "error", errorMessage, }); + expect(broadcast).not.toHaveProperty("message"); const dedupe = context.dedupe.get("chat:idem-agent-returned-error"); expect(dedupe?.ok).toBe(false); expect(dedupe?.payload).toMatchObject({ diff --git a/src/gateway/server-methods/chat.error-broadcast.test.ts b/src/gateway/server-methods/chat.error-broadcast.test.ts index 26c226923552..ac79487b1312 100644 --- a/src/gateway/server-methods/chat.error-broadcast.test.ts +++ b/src/gateway/server-methods/chat.error-broadcast.test.ts @@ -158,25 +158,19 @@ describe("chat.send error broadcast", () => { expect.any(Object), ); - // Verify broadcastChatError was called (via context.broadcast) - expect(ctx.broadcast).toHaveBeenCalledWith( - "chat", - expect.objectContaining({ - runId: "test-run-1", - state: "error", - errorMessage: expect.stringContaining("LLM timeout"), - message: expect.objectContaining({ - role: "assistant", - content: [ - expect.objectContaining({ - type: "text", - text: expect.stringContaining("LLM timeout"), - }), - ], - }), - }), - { sessionKeys: ["agent:main:main"] }, - ); + const payload = expectDefined(ctx.broadcast.mock.calls[0], "error broadcast")[1] as Record< + string, + unknown + >; + expect(payload).toMatchObject({ + runId: "test-run-1", + state: "error", + errorMessage: expect.stringContaining("LLM timeout"), + }); + expect(payload).not.toHaveProperty("message"); + expect(ctx.broadcast).toHaveBeenCalledWith("chat", payload, { + sessionKeys: ["agent:main:main"], + }); }); it("scopes selected-agent global errors to the linked agent", async () => { @@ -222,22 +216,15 @@ describe("chat.send error broadcast", () => { state: "error", }), ); - expect(ctx.nodeSendToSession).toHaveBeenCalledWith( - "global", - "chat", - expect.objectContaining({ - agentId: "main", - state: "error", - message: expect.objectContaining({ - role: "assistant", - content: [ - expect.objectContaining({ - type: "text", - text: expect.stringContaining("LLM timeout"), - }), - ], - }), - }), - ); + const globalPayload = expectDefined( + ctx.nodeSendToSession.mock.calls.find(([sessionKey]) => sessionKey === "global"), + "global node error payload", + )[2] as Record; + expect(globalPayload).toMatchObject({ + agentId: "main", + state: "error", + errorMessage: expect.stringContaining("LLM timeout"), + }); + expect(globalPayload).not.toHaveProperty("message"); }); });