From 4c355a28a3ede44e6fa428cd544ef2dd631b9835 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sun, 22 Feb 2026 15:30:42 +0100 Subject: [PATCH] refactor: centralize tool-error visibility policy --- .../run/payloads.errors.test.ts | 78 +++++++++---------- .../run/payloads.test-helpers.ts | 9 ++- .../pi-embedded-runner/run/payloads.test.ts | 34 +++++--- src/agents/pi-embedded-runner/run/payloads.ts | 37 +++++---- 4 files changed, 91 insertions(+), 67 deletions(-) diff --git a/src/agents/pi-embedded-runner/run/payloads.errors.test.ts b/src/agents/pi-embedded-runner/run/payloads.errors.test.ts index 94240b31baa..97e3d188bb9 100644 --- a/src/agents/pi-embedded-runner/run/payloads.errors.test.ts +++ b/src/agents/pi-embedded-runner/run/payloads.errors.test.ts @@ -113,10 +113,10 @@ describe("buildEmbeddedRunPayloads", () => { lastToolError: { toolName: "browser", error: "tab not found" }, }); - expect(payloads).toHaveLength(1); - expect(payloads[0]?.isError).toBe(true); - expect(payloads[0]?.text).toContain("Browser"); - expect(payloads[0]?.text).not.toContain("tab not found"); + expectSingleToolErrorPayload(payloads, { + title: "Browser", + absentDetail: "tab not found", + }); }); it("does not add tool error fallback when assistant output exists", () => { @@ -237,18 +237,6 @@ describe("buildEmbeddedRunPayloads", () => { expect(payloads).toHaveLength(0); }); - it("still shows mutating tool errors when messages.suppressToolErrors is enabled", () => { - const payloads = buildPayloads({ - lastToolError: { toolName: "write", error: "connection timeout" }, - config: { messages: { suppressToolErrors: true } }, - }); - - expect(payloads).toHaveLength(1); - expect(payloads[0]?.isError).toBe(true); - expect(payloads[0]?.text).toContain("Write"); - expect(payloads[0]?.text).not.toContain("connection timeout"); - }); - it("suppresses mutating tool errors when suppressToolErrorWarnings is enabled", () => { const payloads = buildPayloads({ lastToolError: { toolName: "exec", error: "command not found" }, @@ -258,15 +246,35 @@ describe("buildEmbeddedRunPayloads", () => { expect(payloads).toHaveLength(0); }); - it("shows recoverable tool errors for mutating tools", () => { - const payloads = buildPayloads({ - lastToolError: { toolName: "message", meta: "reply", error: "text required" }, - }); - - expect(payloads).toHaveLength(1); - expect(payloads[0]?.isError).toBe(true); - expect(payloads[0]?.text).toContain("Message"); - expect(payloads[0]?.text).not.toContain("required"); + it.each([ + { + name: "still shows mutating tool errors when messages.suppressToolErrors is enabled", + payload: { + lastToolError: { toolName: "write", error: "connection timeout" }, + config: { messages: { suppressToolErrors: true } }, + }, + title: "Write", + absentDetail: "connection timeout", + }, + { + name: "shows recoverable tool errors for mutating tools", + payload: { + lastToolError: { toolName: "message", meta: "reply", error: "text required" }, + }, + title: "Message", + absentDetail: "required", + }, + { + name: "shows non-recoverable tool failure summaries to the user", + payload: { + lastToolError: { toolName: "browser", error: "connection timeout" }, + }, + title: "Browser", + absentDetail: "connection timeout", + }, + ])("$name", ({ payload, title, absentDetail }) => { + const payloads = buildPayloads(payload); + expectSingleToolErrorPayload(payloads, { title, absentDetail }); }); it("shows mutating tool errors even when assistant output exists", () => { @@ -323,27 +331,15 @@ describe("buildEmbeddedRunPayloads", () => { expect(payloads[0]?.text).toBe(warningText); }); - it("shows non-recoverable tool failure summaries to the user", () => { - const payloads = buildPayloads({ - lastToolError: { toolName: "browser", error: "connection timeout" }, - }); - - // Non-recoverable errors should still be shown - expect(payloads).toHaveLength(1); - expect(payloads[0]?.isError).toBe(true); - expect(payloads[0]?.text).toContain("Browser"); - expect(payloads[0]?.text).not.toContain("connection timeout"); - }); - it("includes non-recoverable tool error details when verbose mode is on", () => { const payloads = buildPayloads({ lastToolError: { toolName: "browser", error: "connection timeout" }, verboseLevel: "on", }); - expect(payloads).toHaveLength(1); - expect(payloads[0]?.isError).toBe(true); - expect(payloads[0]?.text).toContain("Browser"); - expect(payloads[0]?.text).toContain("connection timeout"); + expectSingleToolErrorPayload(payloads, { + title: "Browser", + detail: "connection timeout", + }); }); }); diff --git a/src/agents/pi-embedded-runner/run/payloads.test-helpers.ts b/src/agents/pi-embedded-runner/run/payloads.test-helpers.ts index 9cb6bb5a22c..f3c4d2cea37 100644 --- a/src/agents/pi-embedded-runner/run/payloads.test-helpers.ts +++ b/src/agents/pi-embedded-runner/run/payloads.test-helpers.ts @@ -32,10 +32,15 @@ export function expectSinglePayloadText( export function expectSingleToolErrorPayload( payloads: RunPayloads, - params: { title: string; detail: string }, + params: { title: string; detail?: string; absentDetail?: string }, ): void { expect(payloads).toHaveLength(1); expect(payloads[0]?.isError).toBe(true); expect(payloads[0]?.text).toContain(params.title); - expect(payloads[0]?.text).toContain(params.detail); + if (typeof params.detail === "string") { + expect(payloads[0]?.text).toContain(params.detail); + } + if (typeof params.absentDetail === "string") { + expect(payloads[0]?.text).not.toContain(params.absentDetail); + } } diff --git a/src/agents/pi-embedded-runner/run/payloads.test.ts b/src/agents/pi-embedded-runner/run/payloads.test.ts index f1762ec636d..5d950f2ee10 100644 --- a/src/agents/pi-embedded-runner/run/payloads.test.ts +++ b/src/agents/pi-embedded-runner/run/payloads.test.ts @@ -29,21 +29,35 @@ describe("buildEmbeddedRunPayloads tool-error warnings", () => { verboseLevel: "off", }); - expect(payloads).toHaveLength(1); - expect(payloads[0]?.isError).toBe(true); - expect(payloads[0]?.text).toContain("Write"); - expect(payloads[0]?.text).not.toContain("permission denied"); + expectSingleToolErrorPayload(payloads, { + title: "Write", + absentDetail: "permission denied", + }); }); - it("includes mutating tool error details when verbose mode is on", () => { + it.each([ + { + name: "includes details for mutating tool failures when verbose is on", + verboseLevel: "on" as const, + detail: "permission denied", + absentDetail: undefined, + }, + { + name: "includes details for mutating tool failures when verbose is full", + verboseLevel: "full" as const, + detail: "permission denied", + absentDetail: undefined, + }, + ])("$name", ({ verboseLevel, detail, absentDetail }) => { const payloads = buildPayloads({ lastToolError: { toolName: "write", error: "permission denied" }, - verboseLevel: "on", + verboseLevel, }); - expect(payloads).toHaveLength(1); - expect(payloads[0]?.isError).toBe(true); - expect(payloads[0]?.text).toContain("Write"); - expect(payloads[0]?.text).toContain("permission denied"); + expectSingleToolErrorPayload(payloads, { + title: "Write", + detail, + absentDetail, + }); }); }); diff --git a/src/agents/pi-embedded-runner/run/payloads.ts b/src/agents/pi-embedded-runner/run/payloads.ts index 530d24d3699..78e70d0616a 100644 --- a/src/agents/pi-embedded-runner/run/payloads.ts +++ b/src/agents/pi-embedded-runner/run/payloads.ts @@ -28,6 +28,10 @@ type LastToolError = { mutatingAction?: boolean; actionFingerprint?: string; }; +type ToolErrorWarningPolicy = { + showWarning: boolean; + includeDetails: boolean; +}; const RECOVERABLE_TOOL_ERROR_KEYWORDS = [ "required", @@ -44,30 +48,37 @@ function isRecoverableToolError(error: string | undefined): boolean { return RECOVERABLE_TOOL_ERROR_KEYWORDS.some((keyword) => errorLower.includes(keyword)); } -function shouldShowToolErrorWarning(params: { +function isVerboseToolDetailEnabled(level?: VerboseLevel): boolean { + return level === "on" || level === "full"; +} + +function resolveToolErrorWarningPolicy(params: { lastToolError: LastToolError; hasUserFacingReply: boolean; suppressToolErrors: boolean; suppressToolErrorWarnings?: boolean; verboseLevel?: VerboseLevel; -}): boolean { +}): ToolErrorWarningPolicy { + const includeDetails = isVerboseToolDetailEnabled(params.verboseLevel); if (params.suppressToolErrorWarnings) { - return false; + return { showWarning: false, includeDetails }; } const normalizedToolName = params.lastToolError.toolName.trim().toLowerCase(); - const verboseEnabled = params.verboseLevel === "on" || params.verboseLevel === "full"; - if ((normalizedToolName === "exec" || normalizedToolName === "bash") && !verboseEnabled) { - return false; + if ((normalizedToolName === "exec" || normalizedToolName === "bash") && !includeDetails) { + return { showWarning: false, includeDetails }; } const isMutatingToolError = params.lastToolError.mutatingAction ?? isLikelyMutatingToolName(params.lastToolError.toolName); if (isMutatingToolError) { - return true; + return { showWarning: true, includeDetails }; } if (params.suppressToolErrors) { - return false; + return { showWarning: false, includeDetails }; } - return !params.hasUserFacingReply && !isRecoverableToolError(params.lastToolError.error); + return { + showWarning: !params.hasUserFacingReply && !isRecoverableToolError(params.lastToolError.error), + includeDetails, + }; } export function buildEmbeddedRunPayloads(params: { @@ -256,7 +267,7 @@ export function buildEmbeddedRunPayloads(params: { } if (params.lastToolError) { - const shouldShowToolError = shouldShowToolErrorWarning({ + const warningPolicy = resolveToolErrorWarningPolicy({ lastToolError: params.lastToolError, hasUserFacingReply: hasUserFacingAssistantReply, suppressToolErrors: Boolean(params.config?.messages?.suppressToolErrors), @@ -266,16 +277,14 @@ export function buildEmbeddedRunPayloads(params: { // Always surface mutating tool failures so we do not silently confirm actions that did not happen. // Otherwise, keep the previous behavior and only surface non-recoverable failures when no reply exists. - if (shouldShowToolError) { + if (warningPolicy.showWarning) { const toolSummary = formatToolAggregate( params.lastToolError.toolName, params.lastToolError.meta ? [params.lastToolError.meta] : undefined, { markdown: useMarkdown }, ); - const verboseErrorDetailsEnabled = - params.verboseLevel === "on" || params.verboseLevel === "full"; const errorSuffix = - verboseErrorDetailsEnabled && params.lastToolError.error + warningPolicy.includeDetails && params.lastToolError.error ? `: ${params.lastToolError.error}` : ""; const warningText = `⚠️ ${toolSummary} failed${errorSuffix}`;