diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 3e05f500c..ddd175ba8 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -431,6 +431,7 @@ - Integrated testing guidance directly into the main system prompt for improved workflow cohesion - Moved testing guidance into the main system prompt and removed the bundled Tester subagent. +- 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 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 1fa927256..51e419f44 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) @@ -240,7 +310,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 }>; @@ -308,7 +378,7 @@ export class ToolExecutionComponent extends Container implements NativeScrollbac args: any, options: ToolExecutionOptions = {}, tool: AgentTool | undefined, - ui: TUI, + ui: ToolExecutionUi, cwd: string = getProjectDir(), _toolCallId?: string, ) { @@ -902,8 +972,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 @@ -934,7 +1013,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 @@ -991,7 +1078,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) }); } @@ -1038,7 +1129,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 @@ -1059,7 +1159,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