From 2d519d8f89f672a0247440f70602e4c39f1f1cd3 Mon Sep 17 00:00:00 2001 From: roboomp Date: Tue, 14 Jul 2026 16:56:20 +0000 Subject: [PATCH] fix(tui): guard plugin tool renderer components against render crashes Plugin/custom tool renderers can return a component whose render() throws (e.g. a plugin styling its header off an object without a bold method, producing TypeError: th.bold is not a function). ToolExecutionComponent only caught the renderCall/renderResult factory, not the child component's later render() pass, so the exception escaped and crashed the transcript. Wrap every renderer-returned call/result component in SafeToolRendererComponent, which catches render() errors and falls back to the tool label (call) or raw result text (result), logging once per component. Fixes #4978 --- packages/coding-agent/CHANGELOG.md | 4 + .../modes/components/tool-execution.test.ts | 101 ++++++++++++++ .../src/modes/components/tool-execution.ts | 126 ++++++++++++++++-- 3 files changed, 222 insertions(+), 9 deletions(-) create mode 100644 packages/coding-agent/src/modes/components/tool-execution.test.ts diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 78d22734a..5355980a9 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed a crash when a plugin/custom tool renderer returns a component that throws during its later `render()` pass (e.g. `TypeError: th.bold is not a function` from a plugin that styles its header off an object without a `bold` method). `ToolExecutionComponent` now wraps every renderer-returned call/result component so a throwing `render()` degrades to the safe fallback (tool label or raw result text) instead of taking down the transcript ([#4978](https://github.com/can1357/oh-my-pi/issues/4978)). + ## [16.3.14] - 2026-07-09 ### Fixed diff --git a/packages/coding-agent/src/modes/components/tool-execution.test.ts b/packages/coding-agent/src/modes/components/tool-execution.test.ts new file mode 100644 index 000000000..1408f80d7 --- /dev/null +++ b/packages/coding-agent/src/modes/components/tool-execution.test.ts @@ -0,0 +1,101 @@ +import { beforeAll, describe, expect, it } from "bun:test"; +import type { AgentTool } from "@oh-my-pi/pi-agent-core"; +import { type Component, Text } from "@oh-my-pi/pi-tui"; +import { Settings } from "../../config/settings"; +import { getThemeByName, setThemeInstance, theme } from "../theme/theme"; +import { ToolExecutionComponent, type ToolExecutionUi } from "./tool-execution"; + +class BoldTypeErrorComponent implements Component { + render(_width: number): readonly string[] { + throw new TypeError("th.bold is not a function"); + } +} + +function visibleText(lines: readonly string[]): string { + let text = lines.join("\n"); + text = text.replace(/\x1b\]8;[^\x1b\x07]*(?:\x07|\x1b\\)/g, ""); + text = text.replace(/\x1b\[[0-9;]*m/g, ""); + return text; +} + +describe("ToolExecutionComponent custom renderer failures", () => { + beforeAll(async () => { + await Settings.init({ inMemory: true }); + const loaded = await getThemeByName("dark"); + if (!loaded) throw new Error("theme unavailable"); + setThemeInstance(loaded); + }); + + it("falls back to the custom tool label when a renderCall child component throws during render", () => { + const tool: AgentTool = { + name: "graphify_graph", + label: "Graphify Graph", + description: "renders a graph", + parameters: { type: "object", additionalProperties: true }, + renderCall() { + return new BoldTypeErrorComponent(); + }, + async execute() { + return { content: [{ type: "text", text: "ok" }] }; + }, + }; + const ui: ToolExecutionUi = { + requestRender() {}, + requestComponentRender(_component: Component) {}, + resetDisplay() {}, + }; + const component = new ToolExecutionComponent( + "graphify_graph", + {}, + { showImages: false }, + tool, + ui, + process.cwd(), + ); + let text = ""; + + expect(() => { + text = visibleText(component.render(80)); + }).not.toThrow(); + expect(text).toContain("Graphify Graph"); + }); + + it("preserves raw result text when a renderResult child component throws during render", () => { + const rawResultText = "raw result survives child renderer failure"; + const tool: AgentTool = { + name: "crashy_result_renderer", + label: "Crashy Result Renderer", + description: "renders result output", + parameters: { type: "object", additionalProperties: true }, + renderCall() { + return new Text(theme.fg("toolTitle", theme.bold("Crashy Result Renderer")), 0, 0); + }, + renderResult() { + return new BoldTypeErrorComponent(); + }, + async execute() { + return { content: [{ type: "text", text: rawResultText }] }; + }, + }; + const ui: ToolExecutionUi = { + requestRender() {}, + requestComponentRender(_component: Component) {}, + resetDisplay() {}, + }; + const component = new ToolExecutionComponent( + "crashy_result_renderer", + {}, + { showImages: false }, + tool, + ui, + process.cwd(), + ); + component.updateResult({ content: [{ type: "text", text: rawResultText }] }, false); + let text = ""; + + expect(() => { + text = visibleText(component.render(80)); + }).not.toThrow(); + expect(text).toContain(rawResultText); + }); +}); diff --git a/packages/coding-agent/src/modes/components/tool-execution.ts b/packages/coding-agent/src/modes/components/tool-execution.ts index de9f9c483..fae15856e 100644 --- a/packages/coding-agent/src/modes/components/tool-execution.ts +++ b/packages/coding-agent/src/modes/components/tool-execution.ts @@ -40,7 +40,7 @@ import { } from "../../tools/render-utils"; import { type FirstResultViewportRepaint, toolRenderers } from "../../tools/renderers"; import { TODO_STRIKE_TOTAL_FRAMES, type TodoToolDetails } from "../../tools/todo"; -import { isFramedBlockComponent, renderStatusLine, WidthAwareText } from "../../tui"; +import { isFramedBlockComponent, markFramedBlockComponent, renderStatusLine, WidthAwareText } from "../../tui"; import { sanitizeWithOptionalSixelPassthrough } from "../../utils/sixel"; import { renderDiff } from "./diff"; @@ -148,6 +148,68 @@ function getArgsWithStreamedTextInput(args: unknown): unknown { return input === undefined ? args : { ...record, input }; } +type ToolRendererStage = "call" | "result"; + +class SafeToolRendererComponent implements Component { + #toolName: string; + #stage: ToolRendererStage; + #component: Component; + #fallback: () => Component | undefined; + #warned = false; + readonly wantsKeyRelease: boolean | undefined; + + constructor( + toolName: string, + stage: ToolRendererStage, + component: Component, + fallback: () => Component | undefined, + ) { + this.#toolName = toolName; + this.#stage = stage; + this.#component = component; + this.#fallback = fallback; + this.wantsKeyRelease = component.wantsKeyRelease; + if (isFramedBlockComponent(component)) { + markFramedBlockComponent(this); + } + } + + render(width: number): readonly string[] { + try { + return this.#component.render(width); + } catch (err) { + if (!this.#warned) { + this.#warned = true; + logger.warn("Tool renderer failed", { tool: this.#toolName, stage: this.#stage, error: String(err) }); + } + return this.#fallback()?.render(width) ?? []; + } + } + + handleInput(data: string): void { + const handleInput = this.#component.handleInput; + if (handleInput === undefined) return; + handleInput.call(this.#component, data); + } + + invalidate(): void { + const invalidate = this.#component.invalidate; + if (invalidate === undefined) return; + invalidate.call(this.#component); + } + + setIgnoreTight(ignore: boolean): void { + const setIgnoreTight = this.#component.setIgnoreTight; + if (setIgnoreTight === undefined) return; + setIgnoreTight.call(this.#component, ignore); + } + + dispose(): void { + const dispose = this.#component.dispose; + if (dispose === undefined) return; + dispose.call(this.#component); + } +} /** * Transcript-side probe telling a block whether it is still inside the live * (repaintable) region. Implemented by `TranscriptContainer`; injected rather @@ -157,6 +219,14 @@ export interface TranscriptLiveRegionProbe { isBlockInLiveRegion(component: Component): boolean; } +/** Minimal TUI surface ToolExecutionComponent uses to schedule repaints and share image budget. */ +export interface ToolExecutionUi { + requestRender(): void; + requestComponentRender(component: Component): void; + resetDisplay(): void; + imageBudget?: TUI["imageBudget"]; +} + export interface ToolExecutionOptions { snapshots?: SnapshotStore; showImages?: boolean; // default: true (only used if terminal supports images) @@ -238,7 +308,7 @@ export class ToolExecutionComponent extends Container implements NativeScrollbac // forcing the common image-free result to re-shape on every resize tick. #renderedImageCount = 0; #tool?: AgentTool; - #ui: TUI; + #ui: ToolExecutionUi; #cwd: string; #result?: { content: Array<{ type: string; text?: string; data?: string; mimeType?: string }>; @@ -306,7 +376,7 @@ export class ToolExecutionComponent extends Container implements NativeScrollbac args: any, options: ToolExecutionOptions = {}, tool: AgentTool | undefined, - ui: TUI, + ui: ToolExecutionUi, cwd: string = getProjectDir(), _toolCallId?: string, ) { @@ -901,8 +971,17 @@ export class ToolExecutionComponent extends Container implements NativeScrollbac if (tool.renderCall) { try { const callArgs = this.#getCallArgsForRender(); - const callComponent = tool.renderCall(callArgs, this.#renderState, theme); - if (callComponent) this.#contentBox.addChild(callComponent as Component); + const callComponent = tool.renderCall(callArgs, this.#renderState, theme) as Component | undefined; + if (callComponent) { + this.#contentBox.addChild( + new SafeToolRendererComponent( + this.#toolName, + "call", + callComponent, + () => new Text(theme.fg("toolTitle", theme.bold(this.#toolLabel)), 0, 0), + ), + ); + } } catch (err) { logger.warn("Tool renderer failed", { tool: this.#toolName, error: String(err) }); // Fall back to default on error @@ -933,7 +1012,15 @@ export class ToolExecutionComponent extends Container implements NativeScrollbac theme, this.#args, ); - if (resultComponent) this.#contentBox.addChild(resultComponent); + if (resultComponent) { + this.#contentBox.addChild( + new SafeToolRendererComponent(this.#toolName, "result", resultComponent, () => { + const output = this.#getTextOutput(); + if (!output) return undefined; + return new Text(theme.fg("toolOutput", replaceTabs(output)), 0, 0); + }), + ); + } } catch (err) { logger.warn("Tool renderer failed", { tool: this.#toolName, error: String(err) }); // Fall back to showing raw output on error @@ -990,7 +1077,11 @@ export class ToolExecutionComponent extends Container implements NativeScrollbac this.#renderState, theme, ); - if (resultComponent) fileBox.addChild(resultComponent); + if (resultComponent) { + fileBox.addChild( + new SafeToolRendererComponent(this.#toolName, "result", resultComponent, () => undefined), + ); + } } catch (err) { logger.warn("Tool renderer failed", { tool: this.#toolName, error: String(err) }); } @@ -1037,7 +1128,16 @@ export class ToolExecutionComponent extends Container implements NativeScrollbac try { const callArgs = this.#getCallArgsForRender(); const callComponent = renderer.renderCall(callArgs, this.#renderState, theme); - if (callComponent) this.#contentBox.addChild(callComponent); + if (callComponent) { + this.#contentBox.addChild( + new SafeToolRendererComponent( + this.#toolName, + "call", + callComponent, + () => new Text(theme.fg("toolTitle", theme.bold(this.#toolLabel)), 0, 0), + ), + ); + } } catch (err) { logger.warn("Tool renderer failed", { tool: this.#toolName, error: String(err) }); // Fall back to default on error @@ -1058,7 +1158,15 @@ export class ToolExecutionComponent extends Container implements NativeScrollbac theme, this.#getCallArgsForRender(), ); - if (resultComponent) this.#contentBox.addChild(resultComponent); + if (resultComponent) { + this.#contentBox.addChild( + new SafeToolRendererComponent(this.#toolName, "result", resultComponent, () => { + const output = this.#getTextOutput(); + if (!output) return undefined; + return new Text(theme.fg("toolOutput", replaceTabs(output)), 0, 0); + }), + ); + } } catch (err) { logger.warn("Tool renderer failed", { tool: this.#toolName, error: String(err) }); // Fall back to showing raw output on error