From 5ed5e3baaa9a5876a560d905fc6840517f42ca83 Mon Sep 17 00:00:00 2001 From: Sergio Date: Sat, 14 Mar 2026 09:36:16 -0500 Subject: [PATCH 1/6] fix(cron): skip announce delivery when agent output is raw error text MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a cron job's model provider returns an error (e.g. HTTP 500), the error JSON becomes the agent's synthesized text. With `delivery.mode: "announce"`, this raw error dump was delivered to user-facing channels (Discord, Telegram, etc.) as if it were normal content. Add an `isLikelyRawErrorOutput()` guard in `dispatchCronDelivery` that detects common error output patterns (JSON error objects, JS exception strings, HTTP status codes in error context) and skips delivery when matched. The error is still logged internally and visible via `cron runs`, but no longer posted to channels. This makes the behavior consistent regardless of `bestEffort` setting — error runs should never trigger announce delivery. Closes #42243 Co-Authored-By: Claude Opus 4.6 (1M context) --- .../delivery-dispatch.error-guard.test.ts | 288 ++++++++++++++++++ src/cron/isolated-agent/delivery-dispatch.ts | 50 +++ 2 files changed, 338 insertions(+) create mode 100644 src/cron/isolated-agent/delivery-dispatch.error-guard.test.ts diff --git a/src/cron/isolated-agent/delivery-dispatch.error-guard.test.ts b/src/cron/isolated-agent/delivery-dispatch.error-guard.test.ts new file mode 100644 index 00000000000..b4f5fbe1e17 --- /dev/null +++ b/src/cron/isolated-agent/delivery-dispatch.error-guard.test.ts @@ -0,0 +1,288 @@ +/** + * Tests for the error-output delivery guard in cron delivery dispatch. + * + * Bug (#42243): When a cron job's model provider returns an error (e.g. + * HTTP 500), the raw error JSON becomes the agent's synthesized text. + * With `delivery.mode: "announce"`, this error dump was delivered to + * user-facing channels (Discord, Telegram, etc.) as normal content. + * + * Fix: `isLikelyRawErrorOutput()` detects common error patterns in the + * synthesized text and `dispatchCronDelivery` skips delivery when matched. + */ + +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +// --- Module mocks (must be hoisted before imports) --- + +vi.mock("../../agents/subagent-registry.js", () => ({ + countActiveDescendantRuns: vi.fn().mockReturnValue(0), +})); + +vi.mock("../../infra/outbound/deliver.js", () => ({ + deliverOutboundPayloads: vi.fn().mockResolvedValue([{ ok: true }]), +})); + +vi.mock("../../infra/outbound/identity.js", () => ({ + resolveAgentOutboundIdentity: vi.fn().mockReturnValue({}), +})); + +vi.mock("../../infra/outbound/session-context.js", () => ({ + buildOutboundSessionContext: vi.fn().mockReturnValue({}), +})); + +vi.mock("../../cli/outbound-send-deps.js", () => ({ + createOutboundSendDeps: vi.fn().mockReturnValue({}), +})); + +vi.mock("../../logger.js", () => ({ + logWarn: vi.fn(), +})); + +vi.mock("./subagent-followup.js", () => ({ + expectsSubagentFollowup: vi.fn().mockReturnValue(false), + isLikelyInterimCronMessage: vi.fn().mockReturnValue(false), + readDescendantSubagentFallbackReply: vi.fn().mockResolvedValue(undefined), + waitForDescendantSubagentSummary: vi.fn().mockResolvedValue(undefined), +})); + +// Import after mocks +import { deliverOutboundPayloads } from "../../infra/outbound/deliver.js"; +import { logWarn } from "../../logger.js"; +import { + dispatchCronDelivery, + isLikelyRawErrorOutput, + resetCompletedDirectCronDeliveriesForTests, +} from "./delivery-dispatch.js"; +import type { DeliveryTargetResolution } from "./delivery-target.js"; +import type { RunCronAgentTurnResult } from "./run.js"; + +// --------------------------------------------------------------------------- +// Helpers +// --------------------------------------------------------------------------- + +function makeResolvedDelivery(): Extract { + return { + ok: true, + channel: "discord", + to: "channel-42", + accountId: undefined, + threadId: undefined, + mode: "explicit", + }; +} + +function makeWithRunSession() { + return ( + result: Omit, + ): RunCronAgentTurnResult => ({ + ...result, + sessionId: "test-session-id", + sessionKey: "test-session-key", + }); +} + +function makeBaseParams(overrides: { synthesizedText?: string; deliveryRequested?: boolean }) { + const resolvedDelivery = makeResolvedDelivery(); + return { + cfg: {} as never, + cfgWithAgentDefaults: {} as never, + deps: {} as never, + job: { + id: "test-job", + name: "Test Job", + deleteAfterRun: false, + payload: { kind: "agentTurn", message: "hello" }, + } as never, + agentId: "main", + agentSessionKey: "agent:main", + runSessionId: "run-err-guard-123", + runStartedAt: Date.now(), + runEndedAt: Date.now(), + timeoutMs: 30_000, + resolvedDelivery, + deliveryRequested: overrides.deliveryRequested ?? true, + skipHeartbeatDelivery: false, + deliveryBestEffort: false, + deliveryPayloadHasStructuredContent: false, + deliveryPayloads: overrides.synthesizedText ? [{ text: overrides.synthesizedText }] : [], + synthesizedText: overrides.synthesizedText, + summary: overrides.synthesizedText, + outputText: overrides.synthesizedText, + telemetry: undefined, + abortSignal: undefined, + isAborted: () => false, + abortReason: () => "aborted", + withRunSession: makeWithRunSession(), + }; +} + +// --------------------------------------------------------------------------- +// Unit tests for isLikelyRawErrorOutput +// --------------------------------------------------------------------------- + +describe("isLikelyRawErrorOutput", () => { + it("detects JSON error objects from providers", () => { + const openaiError = `{"type":"error","error":{"type":"server_error","code":"server_error","message":"An error occurred while processing your request."}}`; + expect(isLikelyRawErrorOutput(openaiError)).toBe(true); + }); + + it("detects provider error wrapper format", () => { + const codexError = `Codex error: {"type":"error","error":{"type":"server_error","message":"Internal error"}}`; + expect(isLikelyRawErrorOutput(codexError)).toBe(true); + }); + + it("detects JS runtime exceptions", () => { + expect(isLikelyRawErrorOutput("TypeError: Cannot read properties of undefined")).toBe(true); + expect(isLikelyRawErrorOutput("RangeError: Maximum call stack size exceeded")).toBe(true); + expect(isLikelyRawErrorOutput("Error: ECONNREFUSED")).toBe(true); + expect(isLikelyRawErrorOutput("SyntaxError: Unexpected token")).toBe(true); + expect(isLikelyRawErrorOutput("ReferenceError: x is not defined")).toBe(true); + }); + + it("detects common provider error messages in JSON", () => { + const error = `{"message":"An error occurred while processing your request. You can retry your request."}`; + expect(isLikelyRawErrorOutput(error)).toBe(true); + }); + + it("detects JSON error objects with code:invalid_request", () => { + const text = '{"code": "invalid_request", "detail": "bad param"}'; + expect(isLikelyRawErrorOutput(text)).toBe(true); + }); + + it("detects HTTP status code error patterns", () => { + expect(isLikelyRawErrorOutput("error: status code 500 from upstream")).toBe(true); + expect(isLikelyRawErrorOutput("Error response with code 429 - rate limited")).toBe(true); + }); + + it("does not flag normal agent output", () => { + expect(isLikelyRawErrorOutput("Here is your sales report for today.")).toBe(false); + expect(isLikelyRawErrorOutput("No new emails found.")).toBe(false); + expect(isLikelyRawErrorOutput("Task completed successfully.")).toBe(false); + }); + + it("does not flag output containing the word error in normal context", () => { + expect( + isLikelyRawErrorOutput("I found 3 error reports in the email inbox that need attention."), + ).toBe(false); + expect( + isLikelyRawErrorOutput( + "I checked the error logs and everything looks fine. No issues found today.", + ), + ).toBe(false); + }); + + it("does not flag empty or very large text", () => { + expect(isLikelyRawErrorOutput("")).toBe(false); + expect(isLikelyRawErrorOutput(" \n ")).toBe(false); + expect(isLikelyRawErrorOutput("x".repeat(6000))).toBe(false); + }); + + it("does not flag long text even if it starts with an error pattern", () => { + const longText = "Error: something went wrong\n" + "x".repeat(5100); + expect(isLikelyRawErrorOutput(longText)).toBe(false); + }); + + it("handles whitespace-padded error output", () => { + const padded = ` \n {"type":"error","error":{"code":"invalid_request","message":"bad"}} \n`; + expect(isLikelyRawErrorOutput(padded)).toBe(true); + }); +}); + +// --------------------------------------------------------------------------- +// Integration tests through dispatchCronDelivery +// --------------------------------------------------------------------------- + +describe("dispatchCronDelivery — error output guard", () => { + beforeEach(() => { + vi.clearAllMocks(); + resetCompletedDirectCronDeliveriesForTests(); + }); + + afterEach(() => { + vi.unstubAllEnvs(); + }); + + it("skips delivery when synthesized text is a raw JSON error", async () => { + const errorJson = JSON.stringify({ + type: "error", + error: { type: "server_error", message: "Internal server error" }, + }); + const params = makeBaseParams({ synthesizedText: errorJson }); + const state = await dispatchCronDelivery(params); + + expect(state.delivered).toBe(false); + expect(state.deliveryAttempted).toBe(false); + expect(state.result).toBeUndefined(); + expect(deliverOutboundPayloads).not.toHaveBeenCalled(); + expect(logWarn).toHaveBeenCalledWith( + expect.stringContaining("suppressed announce delivery of raw error output"), + ); + }); + + it("skips delivery when synthesized text is a JS exception", async () => { + const params = makeBaseParams({ + synthesizedText: "TypeError: Cannot read properties of undefined (reading 'map')", + }); + const state = await dispatchCronDelivery(params); + + expect(state.delivered).toBe(false); + expect(state.deliveryAttempted).toBe(false); + expect(deliverOutboundPayloads).not.toHaveBeenCalled(); + }); + + it("allows delivery for normal agent output", async () => { + const params = makeBaseParams({ + synthesizedText: "Your daily sales report is ready. 3 new leads today.", + }); + const state = await dispatchCronDelivery(params); + + expect(state.deliveryAttempted).toBe(true); + expect(state.delivered).toBe(true); + expect(deliverOutboundPayloads).toHaveBeenCalledTimes(1); + }); + + it("allows delivery when synthesized text is undefined", async () => { + const params = makeBaseParams({ synthesizedText: undefined }); + const state = await dispatchCronDelivery(params); + + expect(state.result).toBeUndefined(); + }); + + it("skips delivery for HTTP 500 error pattern in text", async () => { + const params = makeBaseParams({ + synthesizedText: "error: status code 500 from model provider", + }); + const state = await dispatchCronDelivery(params); + + expect(state.delivered).toBe(false); + expect(state.deliveryAttempted).toBe(false); + expect(deliverOutboundPayloads).not.toHaveBeenCalled(); + }); + + it("preserves summary and outputText in the returned state even when skipping", async () => { + const errorText = "Error: ECONNREFUSED 127.0.0.1:18789"; + const params = makeBaseParams({ synthesizedText: errorText }); + const state = await dispatchCronDelivery(params); + + expect(state.synthesizedText).toBe(errorText); + expect(state.outputText).toBe(errorText); + expect(state.summary).toBe(errorText); + expect(state.delivered).toBe(false); + }); + + it("guard fires even when delivery is not explicitly requested", async () => { + const errorJson = JSON.stringify({ + type: "error", + error: { type: "server_error", message: "fail" }, + }); + const params = makeBaseParams({ + synthesizedText: errorJson, + deliveryRequested: false, + }); + const state = await dispatchCronDelivery(params); + + expect(state.delivered).toBe(false); + expect(state.deliveryAttempted).toBe(false); + expect(deliverOutboundPayloads).not.toHaveBeenCalled(); + }); +}); diff --git a/src/cron/isolated-agent/delivery-dispatch.ts b/src/cron/isolated-agent/delivery-dispatch.ts index 6ddddf20669..01799f89a84 100644 --- a/src/cron/isolated-agent/delivery-dispatch.ts +++ b/src/cron/isolated-agent/delivery-dispatch.ts @@ -279,6 +279,38 @@ async function retryTransientDirectCronDelivery(params: { } } +/** + * Patterns that identify raw error output from model providers or runtime + * exceptions. When any of these match the synthesized text of a cron run, + * announce delivery is suppressed to avoid posting error JSON dumps to + * user-facing channels. + * + * See: https://github.com/openclaw/openclaw/issues/42243 + */ +const RAW_ERROR_OUTPUT_PATTERNS: readonly RegExp[] = [ + /^\s*\{[\s\S]*"(?:type|error|code)":\s*"(?:error|server_error|invalid_request)/, + /^\s*(?:Error|TypeError|RangeError|SyntaxError|ReferenceError):/, + /\b(?:error|Error)\b.*\b(?:status|code)\b.*\b(?:5\d{2}|4\d{2})\b/, + /^\s*\{[\s\S]*"(?:message|error)":\s*"An error occurred/, + /\berror"?:\s*\{[\s\S]*"(?:type|code)":\s*"(?:server_error|invalid_request|rate_limit)/, +]; + +/** + * Returns `true` when `text` looks like a raw error response rather than + * useful agent output. Used to prevent error JSON dumps from being + * delivered to user-facing channels via announce mode. + * + * Only inspects short texts (<=5000 chars) -- large outputs are assumed to + * be real content that happens to mention an error keyword. + */ +export function isLikelyRawErrorOutput(text: string): boolean { + const trimmed = text.trim(); + if (!trimmed || trimmed.length > 5000) { + return false; + } + return RAW_ERROR_OUTPUT_PATTERNS.some((re) => re.test(trimmed)); +} + export async function dispatchCronDelivery( params: DispatchCronDeliveryParams, ): Promise { @@ -288,6 +320,24 @@ export async function dispatchCronDelivery( let synthesizedText = params.synthesizedText; let deliveryPayloads = params.deliveryPayloads; + // Guard: never deliver raw error output (provider JSON errors, runtime + // exceptions) to user-facing channels. The error is still logged internally + // and visible via `cron runs`, but should not be posted to channels. + // See: https://github.com/openclaw/openclaw/issues/42243 + if (synthesizedText && isLikelyRawErrorOutput(synthesizedText)) { + logWarn( + `[cron:${params.job.id}] suppressed announce delivery of raw error output (${synthesizedText.length} chars)`, + ); + return { + delivered: false, + deliveryAttempted: false, + summary, + outputText, + synthesizedText, + deliveryPayloads, + }; + } + // Shared callers can treat a matching message-tool send as the completed // delivery path. Cron-owned callers keep this false so direct cron delivery // remains the only source of delivered state. From f179173819bc25fda03bb96979eb75749ee0333b Mon Sep 17 00:00:00 2001 From: Sergio Date: Sun, 15 Mar 2026 16:44:58 -0500 Subject: [PATCH 2/6] fix: extend error guard to cover deliveryPayloads path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The delivery path prefers deliveryPayloads over synthesizedText when payloads are non-empty. Error text arriving via deliveryPayloads bypassed the synthesizedText-only guard. Now checks both paths — payloads are only suppressed when every entry matches an error pattern. Addresses review feedback from greptile-apps[bot]. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../delivery-dispatch.error-guard.test.ts | 34 +++++++++++++++++++ src/cron/isolated-agent/delivery-dispatch.ts | 16 +++++++-- 2 files changed, 48 insertions(+), 2 deletions(-) diff --git a/src/cron/isolated-agent/delivery-dispatch.error-guard.test.ts b/src/cron/isolated-agent/delivery-dispatch.error-guard.test.ts index b4f5fbe1e17..638db06f389 100644 --- a/src/cron/isolated-agent/delivery-dispatch.error-guard.test.ts +++ b/src/cron/isolated-agent/delivery-dispatch.error-guard.test.ts @@ -270,6 +270,40 @@ describe("dispatchCronDelivery — error output guard", () => { expect(state.delivered).toBe(false); }); + it("skips delivery when deliveryPayloads contain raw error text", async () => { + const errorJson = JSON.stringify({ + type: "error", + error: { type: "server_error", message: "fail" }, + }); + const params = makeBaseParams({ synthesizedText: undefined }); + // Override deliveryPayloads directly with error text + params.deliveryPayloads = [{ text: errorJson }]; + const state = await dispatchCronDelivery(params); + + expect(state.delivered).toBe(false); + expect(state.deliveryAttempted).toBe(false); + expect(deliverOutboundPayloads).not.toHaveBeenCalled(); + expect(logWarn).toHaveBeenCalledWith( + expect.stringContaining("suppressed announce delivery of raw error output"), + ); + expect(logWarn).toHaveBeenCalledWith(expect.stringContaining("deliveryPayloads")); + }); + + it("allows delivery when only some payloads match error patterns", async () => { + const params = makeBaseParams({ + synthesizedText: "Here is your daily report.", + }); + params.deliveryPayloads = [ + { text: "Here is your daily report." }, + { text: "Error: something went wrong" }, + ]; + const state = await dispatchCronDelivery(params); + + // Mixed payloads should NOT be suppressed — only when ALL payloads are errors + expect(state.delivered).toBe(true); + expect(deliverOutboundPayloads).toHaveBeenCalledTimes(1); + }); + it("guard fires even when delivery is not explicitly requested", async () => { const errorJson = JSON.stringify({ type: "error", diff --git a/src/cron/isolated-agent/delivery-dispatch.ts b/src/cron/isolated-agent/delivery-dispatch.ts index 01799f89a84..2ddbf3711a2 100644 --- a/src/cron/isolated-agent/delivery-dispatch.ts +++ b/src/cron/isolated-agent/delivery-dispatch.ts @@ -323,10 +323,22 @@ export async function dispatchCronDelivery( // Guard: never deliver raw error output (provider JSON errors, runtime // exceptions) to user-facing channels. The error is still logged internally // and visible via `cron runs`, but should not be posted to channels. + // Check both synthesizedText and deliveryPayloads — the delivery path + // prefers deliveryPayloads when non-empty, so error text arriving there + // would bypass a synthesizedText-only guard. // See: https://github.com/openclaw/openclaw/issues/42243 - if (synthesizedText && isLikelyRawErrorOutput(synthesizedText)) { + const errorInSynthesized = synthesizedText && isLikelyRawErrorOutput(synthesizedText); + const errorInPayloads = + !errorInSynthesized && + deliveryPayloads.length > 0 && + deliveryPayloads.every((p) => typeof p.text === "string" && isLikelyRawErrorOutput(p.text)); + if (errorInSynthesized || errorInPayloads) { + const source = errorInSynthesized ? "synthesizedText" : "deliveryPayloads"; + const chars = errorInSynthesized + ? synthesizedText!.length + : deliveryPayloads.reduce((n, p) => n + (p.text?.length ?? 0), 0); logWarn( - `[cron:${params.job.id}] suppressed announce delivery of raw error output (${synthesizedText.length} chars)`, + `[cron:${params.job.id}] suppressed announce delivery of raw error output (${source}, ${chars} chars)`, ); return { delivered: false, From df896b49dfbf8bccbcf2924ef168da96b981b36c Mon Sep 17 00:00:00 2001 From: Sergio Date: Sun, 15 Mar 2026 16:56:56 -0500 Subject: [PATCH 3/6] fix: preserve shared-delivery flags in error guard early return When skipMessagingToolDelivery is true (agent already sent via message tool), the error guard early return was hardcoding delivered/ deliveryAttempted to false, overwriting the existing delivery state. This caused cron metadata to report not-delivered even though the message tool send already happened. Now uses skipMessagingToolDelivery for the initial flag values in the early return, matching the behavior of the rest of the function. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../delivery-dispatch.error-guard.test.ts | 17 +++++++++++++++++ src/cron/isolated-agent/delivery-dispatch.ts | 8 ++++++-- 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/src/cron/isolated-agent/delivery-dispatch.error-guard.test.ts b/src/cron/isolated-agent/delivery-dispatch.error-guard.test.ts index 638db06f389..0df33d927e4 100644 --- a/src/cron/isolated-agent/delivery-dispatch.error-guard.test.ts +++ b/src/cron/isolated-agent/delivery-dispatch.error-guard.test.ts @@ -319,4 +319,21 @@ describe("dispatchCronDelivery — error output guard", () => { expect(state.deliveryAttempted).toBe(false); expect(deliverOutboundPayloads).not.toHaveBeenCalled(); }); + + it("preserves shared-delivery flags when skipMessagingToolDelivery is true", async () => { + const errorJson = JSON.stringify({ + type: "error", + error: { type: "server_error", message: "fail" }, + }); + const params = makeBaseParams({ synthesizedText: errorJson }); + // Simulate shared-delivery path: agent already sent via message tool + (params as Record).skipMessagingToolDelivery = true; + const state = await dispatchCronDelivery(params); + + // Error guard should suppress announce delivery but preserve the + // existing message-tool delivery state. + expect(state.delivered).toBe(true); + expect(state.deliveryAttempted).toBe(true); + expect(deliverOutboundPayloads).not.toHaveBeenCalled(); + }); }); diff --git a/src/cron/isolated-agent/delivery-dispatch.ts b/src/cron/isolated-agent/delivery-dispatch.ts index 2ddbf3711a2..41248c992c7 100644 --- a/src/cron/isolated-agent/delivery-dispatch.ts +++ b/src/cron/isolated-agent/delivery-dispatch.ts @@ -340,9 +340,13 @@ export async function dispatchCronDelivery( logWarn( `[cron:${params.job.id}] suppressed announce delivery of raw error output (${source}, ${chars} chars)`, ); + // Preserve shared-delivery state: when skipMessagingToolDelivery is true, + // the agent already sent via the message tool — suppressing the error + // guard's announce delivery should not overwrite that existing delivery + // state in cron logs/metadata. return { - delivered: false, - deliveryAttempted: false, + delivered: skipMessagingToolDelivery, + deliveryAttempted: skipMessagingToolDelivery, summary, outputText, synthesizedText, From fb456b4f3f5fabf8727fcdf80d851071cff498a1 Mon Sep 17 00:00:00 2001 From: Sergio Date: Sun, 15 Mar 2026 17:07:42 -0500 Subject: [PATCH 4/6] fix: only suppress synthesizedText errors when no valid payloads exist The delivery path prefers deliveryPayloads over synthesizedText when payloads are non-empty. If synthesizedText contains stale error text but deliveryPayloads has valid content (media, structured data), the guard was incorrectly suppressing delivery. Now only checks synthesizedText when there are no active delivery payloads. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../delivery-dispatch.error-guard.test.ts | 16 ++++++++++++++++ src/cron/isolated-agent/delivery-dispatch.ts | 15 +++++++++------ 2 files changed, 25 insertions(+), 6 deletions(-) diff --git a/src/cron/isolated-agent/delivery-dispatch.error-guard.test.ts b/src/cron/isolated-agent/delivery-dispatch.error-guard.test.ts index 0df33d927e4..34ff372a75c 100644 --- a/src/cron/isolated-agent/delivery-dispatch.error-guard.test.ts +++ b/src/cron/isolated-agent/delivery-dispatch.error-guard.test.ts @@ -320,6 +320,22 @@ describe("dispatchCronDelivery — error output guard", () => { expect(deliverOutboundPayloads).not.toHaveBeenCalled(); }); + it("allows delivery when synthesizedText has error but deliveryPayloads has valid content", async () => { + const errorJson = JSON.stringify({ + type: "error", + error: { type: "server_error", message: "fail" }, + }); + const params = makeBaseParams({ synthesizedText: errorJson }); + // Override with valid structured payload — delivery path prefers these + params.deliveryPayloads = [{ text: "Your weekly report is attached." }]; + const state = await dispatchCronDelivery(params); + + // Valid payloads take priority; error in synthesizedText should not + // suppress delivery of non-error payload content. + expect(state.delivered).toBe(true); + expect(deliverOutboundPayloads).toHaveBeenCalledTimes(1); + }); + it("preserves shared-delivery flags when skipMessagingToolDelivery is true", async () => { const errorJson = JSON.stringify({ type: "error", diff --git a/src/cron/isolated-agent/delivery-dispatch.ts b/src/cron/isolated-agent/delivery-dispatch.ts index 41248c992c7..93f652ea479 100644 --- a/src/cron/isolated-agent/delivery-dispatch.ts +++ b/src/cron/isolated-agent/delivery-dispatch.ts @@ -323,14 +323,17 @@ export async function dispatchCronDelivery( // Guard: never deliver raw error output (provider JSON errors, runtime // exceptions) to user-facing channels. The error is still logged internally // and visible via `cron runs`, but should not be posted to channels. - // Check both synthesizedText and deliveryPayloads — the delivery path - // prefers deliveryPayloads when non-empty, so error text arriving there - // would bypass a synthesizedText-only guard. + // + // The delivery path prefers deliveryPayloads when non-empty, so we only + // suppress based on synthesizedText when there are no valid payloads that + // would take priority (e.g., media or structured content). When payloads + // exist, we check whether ALL of them contain error text instead. // See: https://github.com/openclaw/openclaw/issues/42243 - const errorInSynthesized = synthesizedText && isLikelyRawErrorOutput(synthesizedText); + const hasActivePayloads = deliveryPayloads.length > 0; + const errorInSynthesized = + !hasActivePayloads && synthesizedText && isLikelyRawErrorOutput(synthesizedText); const errorInPayloads = - !errorInSynthesized && - deliveryPayloads.length > 0 && + hasActivePayloads && deliveryPayloads.every((p) => typeof p.text === "string" && isLikelyRawErrorOutput(p.text)); if (errorInSynthesized || errorInPayloads) { const source = errorInSynthesized ? "synthesizedText" : "deliveryPayloads"; From 085a6988c1fbac019d1f87beecdcd435ba11b269 Mon Sep 17 00:00:00 2001 From: Sergio Date: Sun, 15 Mar 2026 17:37:10 -0500 Subject: [PATCH 5/6] =?UTF-8?q?fix:=20address=20Codex=20P1/P2=20review=20?= =?UTF-8?q?=E2=80=94=20narrow=20regex=20and=20move=20guard=20into=20delive?= =?UTF-8?q?rViaDirect?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove overly broad HTTP status code pattern that matched legitimate monitoring prose (e.g. "Server returned error code 503"). Move the error guard from top-level dispatchCronDelivery into deliverViaDirect so it runs AFTER subagent orchestration, checking final payloads instead of interim text. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../delivery-dispatch.error-guard.test.ts | 63 +++---------------- src/cron/isolated-agent/delivery-dispatch.ts | 51 ++++----------- 2 files changed, 22 insertions(+), 92 deletions(-) diff --git a/src/cron/isolated-agent/delivery-dispatch.error-guard.test.ts b/src/cron/isolated-agent/delivery-dispatch.error-guard.test.ts index 34ff372a75c..fec7f8e17e8 100644 --- a/src/cron/isolated-agent/delivery-dispatch.error-guard.test.ts +++ b/src/cron/isolated-agent/delivery-dispatch.error-guard.test.ts @@ -149,9 +149,10 @@ describe("isLikelyRawErrorOutput", () => { expect(isLikelyRawErrorOutput(text)).toBe(true); }); - it("detects HTTP status code error patterns", () => { - expect(isLikelyRawErrorOutput("error: status code 500 from upstream")).toBe(true); - expect(isLikelyRawErrorOutput("Error response with code 429 - rate limited")).toBe(true); + it("does not flag HTTP status code error patterns in prose", () => { + expect(isLikelyRawErrorOutput("error: status code 500 from upstream")).toBe(false); + expect(isLikelyRawErrorOutput("Error response with code 429 - rate limited")).toBe(false); + expect(isLikelyRawErrorOutput("Server returned error code 503")).toBe(false); }); it("does not flag normal agent output", () => { @@ -211,11 +212,10 @@ describe("dispatchCronDelivery — error output guard", () => { const state = await dispatchCronDelivery(params); expect(state.delivered).toBe(false); - expect(state.deliveryAttempted).toBe(false); expect(state.result).toBeUndefined(); expect(deliverOutboundPayloads).not.toHaveBeenCalled(); expect(logWarn).toHaveBeenCalledWith( - expect.stringContaining("suppressed announce delivery of raw error output"), + expect.stringContaining("suppressed delivery of raw error output"), ); }); @@ -226,7 +226,6 @@ describe("dispatchCronDelivery — error output guard", () => { const state = await dispatchCronDelivery(params); expect(state.delivered).toBe(false); - expect(state.deliveryAttempted).toBe(false); expect(deliverOutboundPayloads).not.toHaveBeenCalled(); }); @@ -248,17 +247,6 @@ describe("dispatchCronDelivery — error output guard", () => { expect(state.result).toBeUndefined(); }); - it("skips delivery for HTTP 500 error pattern in text", async () => { - const params = makeBaseParams({ - synthesizedText: "error: status code 500 from model provider", - }); - const state = await dispatchCronDelivery(params); - - expect(state.delivered).toBe(false); - expect(state.deliveryAttempted).toBe(false); - expect(deliverOutboundPayloads).not.toHaveBeenCalled(); - }); - it("preserves summary and outputText in the returned state even when skipping", async () => { const errorText = "Error: ECONNREFUSED 127.0.0.1:18789"; const params = makeBaseParams({ synthesizedText: errorText }); @@ -276,17 +264,17 @@ describe("dispatchCronDelivery — error output guard", () => { error: { type: "server_error", message: "fail" }, }); const params = makeBaseParams({ synthesizedText: undefined }); - // Override deliveryPayloads directly with error text + // Override deliveryPayloads directly with error text and route through + // deliverViaDirect (structured content path) so the guard is reached. params.deliveryPayloads = [{ text: errorJson }]; + (params as Record).deliveryPayloadHasStructuredContent = true; const state = await dispatchCronDelivery(params); expect(state.delivered).toBe(false); - expect(state.deliveryAttempted).toBe(false); expect(deliverOutboundPayloads).not.toHaveBeenCalled(); expect(logWarn).toHaveBeenCalledWith( - expect.stringContaining("suppressed announce delivery of raw error output"), + expect.stringContaining("suppressed delivery of raw error output"), ); - expect(logWarn).toHaveBeenCalledWith(expect.stringContaining("deliveryPayloads")); }); it("allows delivery when only some payloads match error patterns", async () => { @@ -304,22 +292,6 @@ describe("dispatchCronDelivery — error output guard", () => { expect(deliverOutboundPayloads).toHaveBeenCalledTimes(1); }); - it("guard fires even when delivery is not explicitly requested", async () => { - const errorJson = JSON.stringify({ - type: "error", - error: { type: "server_error", message: "fail" }, - }); - const params = makeBaseParams({ - synthesizedText: errorJson, - deliveryRequested: false, - }); - const state = await dispatchCronDelivery(params); - - expect(state.delivered).toBe(false); - expect(state.deliveryAttempted).toBe(false); - expect(deliverOutboundPayloads).not.toHaveBeenCalled(); - }); - it("allows delivery when synthesizedText has error but deliveryPayloads has valid content", async () => { const errorJson = JSON.stringify({ type: "error", @@ -335,21 +307,4 @@ describe("dispatchCronDelivery — error output guard", () => { expect(state.delivered).toBe(true); expect(deliverOutboundPayloads).toHaveBeenCalledTimes(1); }); - - it("preserves shared-delivery flags when skipMessagingToolDelivery is true", async () => { - const errorJson = JSON.stringify({ - type: "error", - error: { type: "server_error", message: "fail" }, - }); - const params = makeBaseParams({ synthesizedText: errorJson }); - // Simulate shared-delivery path: agent already sent via message tool - (params as Record).skipMessagingToolDelivery = true; - const state = await dispatchCronDelivery(params); - - // Error guard should suppress announce delivery but preserve the - // existing message-tool delivery state. - expect(state.delivered).toBe(true); - expect(state.deliveryAttempted).toBe(true); - expect(deliverOutboundPayloads).not.toHaveBeenCalled(); - }); }); diff --git a/src/cron/isolated-agent/delivery-dispatch.ts b/src/cron/isolated-agent/delivery-dispatch.ts index 93f652ea479..822754cf8d2 100644 --- a/src/cron/isolated-agent/delivery-dispatch.ts +++ b/src/cron/isolated-agent/delivery-dispatch.ts @@ -290,7 +290,6 @@ async function retryTransientDirectCronDelivery(params: { const RAW_ERROR_OUTPUT_PATTERNS: readonly RegExp[] = [ /^\s*\{[\s\S]*"(?:type|error|code)":\s*"(?:error|server_error|invalid_request)/, /^\s*(?:Error|TypeError|RangeError|SyntaxError|ReferenceError):/, - /\b(?:error|Error)\b.*\b(?:status|code)\b.*\b(?:5\d{2}|4\d{2})\b/, /^\s*\{[\s\S]*"(?:message|error)":\s*"An error occurred/, /\berror"?:\s*\{[\s\S]*"(?:type|code)":\s*"(?:server_error|invalid_request|rate_limit)/, ]; @@ -320,43 +319,6 @@ export async function dispatchCronDelivery( let synthesizedText = params.synthesizedText; let deliveryPayloads = params.deliveryPayloads; - // Guard: never deliver raw error output (provider JSON errors, runtime - // exceptions) to user-facing channels. The error is still logged internally - // and visible via `cron runs`, but should not be posted to channels. - // - // The delivery path prefers deliveryPayloads when non-empty, so we only - // suppress based on synthesizedText when there are no valid payloads that - // would take priority (e.g., media or structured content). When payloads - // exist, we check whether ALL of them contain error text instead. - // See: https://github.com/openclaw/openclaw/issues/42243 - const hasActivePayloads = deliveryPayloads.length > 0; - const errorInSynthesized = - !hasActivePayloads && synthesizedText && isLikelyRawErrorOutput(synthesizedText); - const errorInPayloads = - hasActivePayloads && - deliveryPayloads.every((p) => typeof p.text === "string" && isLikelyRawErrorOutput(p.text)); - if (errorInSynthesized || errorInPayloads) { - const source = errorInSynthesized ? "synthesizedText" : "deliveryPayloads"; - const chars = errorInSynthesized - ? synthesizedText!.length - : deliveryPayloads.reduce((n, p) => n + (p.text?.length ?? 0), 0); - logWarn( - `[cron:${params.job.id}] suppressed announce delivery of raw error output (${source}, ${chars} chars)`, - ); - // Preserve shared-delivery state: when skipMessagingToolDelivery is true, - // the agent already sent via the message tool — suppressing the error - // guard's announce delivery should not overwrite that existing delivery - // state in cron logs/metadata. - return { - delivered: skipMessagingToolDelivery, - deliveryAttempted: skipMessagingToolDelivery, - summary, - outputText, - synthesizedText, - deliveryPayloads, - }; - } - // Shared callers can treat a matching message-tool send as the completed // delivery path. Cron-owned callers keep this false so direct cron delivery // remains the only source of delivered state. @@ -392,6 +354,19 @@ export async function dispatchCronDelivery( if (payloadsForDelivery.length === 0) { return null; } + // Guard: never deliver raw error output (provider JSON errors, runtime + // exceptions) to user-facing channels. Runs AFTER subagent orchestration + // so interim error-shaped parent messages don't suppress valid final output. + // See: https://github.com/openclaw/openclaw/issues/42243 + const allPayloadsAreErrors = payloadsForDelivery.every( + (p) => typeof p.text === "string" && isLikelyRawErrorOutput(p.text), + ); + if (allPayloadsAreErrors) { + logWarn( + `[cron:${params.job.id}] suppressed delivery of raw error output (${payloadsForDelivery.length} payloads)`, + ); + return null; + } if (params.isAborted()) { return params.withRunSession({ status: "error", From 92ab31fb78b0237b2e13a536cc92d307e81d6d7f Mon Sep 17 00:00:00 2001 From: Sergio Date: Sun, 15 Mar 2026 17:52:35 -0500 Subject: [PATCH 6/6] =?UTF-8?q?fix:=20drop=20bare=20Error:=20from=20error?= =?UTF-8?q?=20guard=20=E2=80=94=20too=20broad=20for=20monitoring=20prose?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Only match specific named JS exception types (TypeError, RangeError, SyntaxError, ReferenceError). Bare "Error:" is ambiguous — legitimate monitoring crons can start output with "Error: API latency exceeded threshold" which should not be suppressed. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../isolated-agent/delivery-dispatch.error-guard.test.ts | 8 ++++++-- src/cron/isolated-agent/delivery-dispatch.ts | 2 +- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/cron/isolated-agent/delivery-dispatch.error-guard.test.ts b/src/cron/isolated-agent/delivery-dispatch.error-guard.test.ts index fec7f8e17e8..29ce8a87bff 100644 --- a/src/cron/isolated-agent/delivery-dispatch.error-guard.test.ts +++ b/src/cron/isolated-agent/delivery-dispatch.error-guard.test.ts @@ -134,11 +134,15 @@ describe("isLikelyRawErrorOutput", () => { it("detects JS runtime exceptions", () => { expect(isLikelyRawErrorOutput("TypeError: Cannot read properties of undefined")).toBe(true); expect(isLikelyRawErrorOutput("RangeError: Maximum call stack size exceeded")).toBe(true); - expect(isLikelyRawErrorOutput("Error: ECONNREFUSED")).toBe(true); expect(isLikelyRawErrorOutput("SyntaxError: Unexpected token")).toBe(true); expect(isLikelyRawErrorOutput("ReferenceError: x is not defined")).toBe(true); }); + it("does not flag bare Error: prefix (could be legitimate monitoring prose)", () => { + expect(isLikelyRawErrorOutput("Error: ECONNREFUSED")).toBe(false); + expect(isLikelyRawErrorOutput("Error: API latency exceeded threshold")).toBe(false); + }); + it("detects common provider error messages in JSON", () => { const error = `{"message":"An error occurred while processing your request. You can retry your request."}`; expect(isLikelyRawErrorOutput(error)).toBe(true); @@ -248,7 +252,7 @@ describe("dispatchCronDelivery — error output guard", () => { }); it("preserves summary and outputText in the returned state even when skipping", async () => { - const errorText = "Error: ECONNREFUSED 127.0.0.1:18789"; + const errorText = "TypeError: Cannot read properties of undefined (reading 'send')"; const params = makeBaseParams({ synthesizedText: errorText }); const state = await dispatchCronDelivery(params); diff --git a/src/cron/isolated-agent/delivery-dispatch.ts b/src/cron/isolated-agent/delivery-dispatch.ts index 822754cf8d2..412612a9eba 100644 --- a/src/cron/isolated-agent/delivery-dispatch.ts +++ b/src/cron/isolated-agent/delivery-dispatch.ts @@ -289,7 +289,7 @@ async function retryTransientDirectCronDelivery(params: { */ const RAW_ERROR_OUTPUT_PATTERNS: readonly RegExp[] = [ /^\s*\{[\s\S]*"(?:type|error|code)":\s*"(?:error|server_error|invalid_request)/, - /^\s*(?:Error|TypeError|RangeError|SyntaxError|ReferenceError):/, + /^\s*(?:TypeError|RangeError|SyntaxError|ReferenceError):/, /^\s*\{[\s\S]*"(?:message|error)":\s*"An error occurred/, /\berror"?:\s*\{[\s\S]*"(?:type|code)":\s*"(?:server_error|invalid_request|rate_limit)/, ];