From 4de80807b9262e28dd67437cc35e9d21152ebaa8 Mon Sep 17 00:00:00 2001 From: Vincent Koc Date: Fri, 24 Apr 2026 15:11:40 -0700 Subject: [PATCH] fix(plugins): bound tool result middleware details --- CHANGELOG.md | 1 + .../harness/tool-result-middleware.test.ts | 66 +++++++++++++- src/agents/harness/tool-result-middleware.ts | 89 ++++++++++++++++--- 3 files changed, 140 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 13d97286a4c..75e214704c9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -63,6 +63,7 @@ Docs: https://docs.openclaw.ai ### Fixes +- Plugin SDK/tool-result transforms: bound middleware `details`, validate in-place result mutations, and mark fail-closed middleware fallbacks with canonical `error` status. Thanks @vincentkoc. - Discord/gateway: prevent startup from getting stuck at `awaiting gateway readiness` when Carbon gateway registration races with a lifecycle reconnect. Fixes #52372. (#68159) Thanks @IVY-AI-gif. - Plugins/cache: restore plugin command and interactive handler registries on loader cache hits without resetting interactive callback dedupe, so cached external plugins keep slash commands and callback handlers available after reloads. Fixes #71100. Thanks @BomBastikDE. - Gateway/OpenAI-compatible: report non-zero token usage for `/v1/chat/completions` when the agent run has only last-call usage metadata available. Fixes #71118. (#71242) Thanks @RenzoMXD. diff --git a/src/agents/harness/tool-result-middleware.test.ts b/src/agents/harness/tool-result-middleware.test.ts index 330b7ca375c..69cea6e2dd0 100644 --- a/src/agents/harness/tool-result-middleware.test.ts +++ b/src/agents/harness/tool-result-middleware.test.ts @@ -24,13 +24,13 @@ describe("createAgentToolResultMiddlewareRunner", () => { }, ], details: { - status: "failed", + status: "error", middlewareError: true, }, }); }); - it("discard invalid middleware results and keeps the previous result", async () => { + it("fails closed for invalid middleware results", async () => { const original = { content: [{ type: "text" as const, text: "raw" }], details: {} }; const runner = createAgentToolResultMiddlewareRunner({ harness: "codex-app-server" }, [ () => ({ result: { content: "not an array" } as never }), @@ -43,7 +43,67 @@ describe("createAgentToolResultMiddlewareRunner", () => { result: original, }); - expect(result).toBe(original); + expect(result.details).toEqual({ status: "error", middlewareError: true }); + }); + + it("fails closed when middleware mutates the current result into an invalid shape", async () => { + const runner = createAgentToolResultMiddlewareRunner({ harness: "pi" }, [ + (event) => { + event.result.content = "not an array" as never; + return undefined; + }, + ]); + + const result = await runner.applyToolResultMiddleware({ + toolCallId: "call-1", + toolName: "exec", + args: {}, + result: { content: [{ type: "text", text: "raw" }], details: {} }, + }); + + expect(result.details).toEqual({ status: "error", middlewareError: true }); + }); + + it("rejects oversized middleware details", async () => { + const runner = createAgentToolResultMiddlewareRunner({ harness: "codex-app-server" }, [ + () => ({ + result: { + content: [{ type: "text", text: "compacted" }], + details: { payload: "x".repeat(100_001) }, + }, + }), + ]); + + const result = await runner.applyToolResultMiddleware({ + toolCallId: "call-1", + toolName: "exec", + args: {}, + result: { content: [{ type: "text", text: "raw" }], details: {} }, + }); + + expect(result.details).toEqual({ status: "error", middlewareError: true }); + }); + + it("rejects cyclic middleware details", async () => { + const details: Record = {}; + details.self = details; + const runner = createAgentToolResultMiddlewareRunner({ harness: "codex-app-server" }, [ + () => ({ + result: { + content: [{ type: "text", text: "compacted" }], + details, + }, + }), + ]); + + const result = await runner.applyToolResultMiddleware({ + toolCallId: "call-1", + toolName: "exec", + args: {}, + result: { content: [{ type: "text", text: "raw" }], details: {} }, + }); + + expect(result.details).toEqual({ status: "error", middlewareError: true }); }); it("accepts well-formed middleware results", async () => { diff --git a/src/agents/harness/tool-result-middleware.ts b/src/agents/harness/tool-result-middleware.ts index e8e80e14ffe..a94866478c4 100644 --- a/src/agents/harness/tool-result-middleware.ts +++ b/src/agents/harness/tool-result-middleware.ts @@ -12,6 +12,9 @@ const log = createSubsystemLogger("agents/harness"); const MAX_MIDDLEWARE_CONTENT_BLOCKS = 200; const MAX_MIDDLEWARE_TEXT_CHARS = 100_000; const MAX_MIDDLEWARE_IMAGE_DATA_CHARS = 5_000_000; +const MAX_MIDDLEWARE_DETAILS_BYTES = 100_000; +const MAX_MIDDLEWARE_DETAILS_DEPTH = 20; +const MAX_MIDDLEWARE_DETAILS_KEYS = 1_000; function isRecord(value: unknown): value is Record { return value !== null && typeof value === "object" && !Array.isArray(value); @@ -35,6 +38,61 @@ function isValidMiddlewareContentBlock(value: unknown): boolean { return false; } +function isValidMiddlewareDetails( + value: unknown, + state: { keys: number; bytes: number; seen: WeakSet } = { + keys: 0, + bytes: 0, + seen: new WeakSet(), + }, + depth = 0, +): boolean { + if (value === undefined || value === null) { + return true; + } + if (depth > MAX_MIDDLEWARE_DETAILS_DEPTH) { + return false; + } + if (typeof value === "string") { + state.bytes += value.length; + return state.bytes <= MAX_MIDDLEWARE_DETAILS_BYTES; + } + if (typeof value === "number" || typeof value === "boolean") { + state.bytes += String(value).length; + return state.bytes <= MAX_MIDDLEWARE_DETAILS_BYTES; + } + if (typeof value !== "object") { + return false; + } + if (state.seen.has(value)) { + return false; + } + state.seen.add(value); + if (Array.isArray(value)) { + state.keys += value.length; + if (state.keys > MAX_MIDDLEWARE_DETAILS_KEYS) { + return false; + } + for (const entry of value) { + if (!isValidMiddlewareDetails(entry, state, depth + 1)) { + return false; + } + } + return true; + } + for (const [key, entry] of Object.entries(value)) { + state.keys += 1; + state.bytes += key.length; + if (state.keys > MAX_MIDDLEWARE_DETAILS_KEYS || state.bytes > MAX_MIDDLEWARE_DETAILS_BYTES) { + return false; + } + if (!isValidMiddlewareDetails(entry, state, depth + 1)) { + return false; + } + } + return true; +} + function isValidMiddlewareToolResult(value: unknown): value is OpenClawAgentToolResult { if (!isRecord(value) || !Array.isArray(value.content)) { return false; @@ -42,7 +100,9 @@ function isValidMiddlewareToolResult(value: unknown): value is OpenClawAgentTool if (value.content.length > MAX_MIDDLEWARE_CONTENT_BLOCKS) { return false; } - return value.content.every(isValidMiddlewareContentBlock); + return ( + value.content.every(isValidMiddlewareContentBlock) && isValidMiddlewareDetails(value.details) + ); } function buildMiddlewareFailureResult(): OpenClawAgentToolResult { @@ -54,7 +114,7 @@ function buildMiddlewareFailureResult(): OpenClawAgentToolResult { }, ], details: { - status: "failed", + status: "error", middlewareError: true, }, }; @@ -72,17 +132,20 @@ export function createAgentToolResultMiddlewareRunner( for (const handler of handlers) { try { const next = await handler({ ...event, result: current }, ctx); - if (next?.result) { - if (isValidMiddlewareToolResult(next.result)) { - current = next.result; - } else { - log.warn( - `[${ctx.harness}] discarded invalid tool result middleware output for ${truncateUtf16Safe( - event.toolName, - 120, - )}`, - ); - } + // Middleware may mutate event.result in place for legacy Pi parity. + // Validate the current object after every handler so in-place writes + // cannot bypass the same shape and size bounds as returned results. + const candidate = next?.result ?? current; + if (isValidMiddlewareToolResult(candidate)) { + current = candidate; + } else { + log.warn( + `[${ctx.harness}] discarded invalid tool result middleware output for ${truncateUtf16Safe( + event.toolName, + 120, + )}`, + ); + return buildMiddlewareFailureResult(); } } catch { log.warn(