nyrn
/
openclaw
mirror of https://github.com/openclaw/openclaw.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

fix: address Codex P1/P2 review — narrow regex and move guard into deliverViaDirect 

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) <noreply@anthropic.com>
This commit is contained in:
Sergio 2026-03-15 17:37:10 -05:00
parent fb456b4f3f
commit 085a6988c1
2 changed files with 22 additions and 92 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

63
src/cron/isolated-agent/delivery-dispatch.error-guard.test.ts
View File

@ -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<string, unknown>).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<string, unknown>).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();
});
});

51
src/cron/isolated-agent/delivery-dispatch.ts
View File

@ -290,7 +290,6 @@ async function retryTransientDirectCronDelivery<T>(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",