diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index e14f2353f..42745fe86 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed `task` renderer crashing the TUI with `TypeError: completeData?.map is not a function` when a subagent's `extractedToolData.yield` slot held a non-array value. `renderAgentResult` (and the live-progress sibling) cast the slot to `Array<{ data }>` and called `?.map`, but optional chaining short-circuits only on `null`/`undefined`, so a plain object made `.map` `undefined` and threw — taking down every `review` task render. Both sites now go through `normalizeYieldData`, which wraps a single object as a 1-element array and drops primitives ([#1987](https://github.com/can1357/oh-my-pi/issues/1987)) + ## [15.9.5] - 2026-06-05 ### Added diff --git a/packages/coding-agent/src/task/render.ts b/packages/coding-agent/src/task/render.ts index 5babac27e..defea6f62 100644 --- a/packages/coding-agent/src/task/render.ts +++ b/packages/coding-agent/src/task/render.ts @@ -117,6 +117,26 @@ function normalizeReportFindings(value: unknown): ReportFindingDetails[] { return findings; } +/** + * Normalize the `yield` slot of `extractedToolData` into an array of + * yield-detail records. The subprocess executor always populates this slot as + * `unknown[]` (see `executor.ts` `extractData` handler), but the renderer + * MUST also tolerate a stray single object — optional chaining short-circuits + * on `null`/`undefined` only, so calling `.map` on a plain object would throw + * `TypeError: completeData?.map is not a function` and crash the TUI. + * A single object is wrapped as a 1-element array so the review verdict still + * renders; non-object primitives drop out. + */ +function normalizeYieldData(value: unknown): Array<{ data: unknown }> { + if (Array.isArray(value)) { + return value.filter((item): item is { data: unknown } => item !== null && typeof item === "object"); + } + if (value !== null && typeof value === "object") { + return [value as { data: unknown }]; + } + return []; +} + function formatJsonScalar(value: unknown, _theme: Theme): string { if (value === null) return "null"; if (typeof value === "string") { @@ -671,12 +691,12 @@ function renderAgentProgress( if (progress.extractedToolData) { // For completed tasks, check for review verdict from yield tool if (progress.status === "completed") { - const completeData = progress.extractedToolData.yield as Array<{ data: unknown }> | undefined; + const completeData = normalizeYieldData(progress.extractedToolData.yield); const reportFindingData = normalizeReportFindings(progress.extractedToolData.report_finding); const reviewData = completeData - ?.map(c => c.data as SubmitReviewDetails) + .map(c => c.data as SubmitReviewDetails) .filter(d => d && typeof d === "object" && "overall_correctness" in d); - if (reviewData && reviewData.length > 0) { + if (reviewData.length > 0) { const summary = reviewData[reviewData.length - 1]; const findings = reportFindingData; lines.push(...renderReviewResult(summary, findings, continuePrefix, expanded, theme)); @@ -912,16 +932,21 @@ function renderAgentResult(result: SingleResult, isLast: boolean, expanded: bool ); } // Check for review result (yield with review schema + report_finding) - const completeData = result.extractedToolData?.yield as Array<{ data: unknown }> | undefined; + // Check for review result (yield with review schema + report_finding). + // `normalizeYieldData` guards against a stray non-array `yield` slot — + // optional chaining on `.map` only short-circuits on null/undefined and + // would otherwise crash the renderer with `TypeError: completeData?.map + // is not a function` when the slot is a plain object (see issue #1987). + const completeData = normalizeYieldData(result.extractedToolData?.yield); const reportFindingData = normalizeReportFindings(result.extractedToolData?.report_finding); // Extract review verdict from yield tool's data field if it matches SubmitReviewDetails const reviewData = completeData - ?.map(c => c.data as SubmitReviewDetails) + .map(c => c.data as SubmitReviewDetails) .filter(d => d && typeof d === "object" && "overall_correctness" in d); - const submitReviewData = reviewData && reviewData.length > 0 ? reviewData : undefined; + const submitReviewData = reviewData.length > 0 ? reviewData : undefined; - if (submitReviewData && submitReviewData.length > 0) { + if (submitReviewData) { // Use combined review renderer const summary = submitReviewData[submitReviewData.length - 1]; const findings = reportFindingData; @@ -929,7 +954,7 @@ function renderAgentResult(result: SingleResult, isLast: boolean, expanded: bool return lines; } if (reportFindingData.length > 0) { - const hasCompleteData = completeData && completeData.length > 0; + const hasCompleteData = completeData.length > 0; const message = hasCompleteData ? "Review verdict missing expected fields" : "Review incomplete (yield not called)"; diff --git a/packages/coding-agent/test/task/render-yield-shape.test.ts b/packages/coding-agent/test/task/render-yield-shape.test.ts new file mode 100644 index 000000000..f59dd4058 --- /dev/null +++ b/packages/coding-agent/test/task/render-yield-shape.test.ts @@ -0,0 +1,134 @@ +import { afterAll, beforeAll, describe, expect, it } from "bun:test"; +import { resetSettingsForTest, Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; +import { getThemeByName, setThemeInstance } from "@oh-my-pi/pi-coding-agent/modes/theme/theme"; +import type { AgentProgress, SingleResult, TaskToolDetails } from "@oh-my-pi/pi-coding-agent/task"; +import { taskToolRenderer } from "@oh-my-pi/pi-coding-agent/task/render"; + +// Regression for #1987: when a subagent stores a non-array value in +// `extractedToolData.yield`, the renderer cast it to `Array<{ data }>` and +// then called `?.map`. Optional chaining only short-circuits on null/undefined, +// so a plain object made `.map` undefined and crashed the TUI with +// `TypeError: completeData?.map is not a function`. The renderer must tolerate +// both shapes (array and single object) without throwing, on both the live +// progress branch (`renderAgentProgress`) and the final result branch +// (`renderAgentResult`). +describe("task renderer: malformed yield slot (#1987)", () => { + beforeAll(async () => { + resetSettingsForTest(); + await Settings.init({ inMemory: true, cwd: process.cwd() }); + const theme = await getThemeByName("dark"); + expect(theme).toBeDefined(); + setThemeInstance(theme!); + }); + + afterAll(() => { + resetSettingsForTest(); + }); + + const reviewVerdict = { + overall_correctness: "correct", + confidence: 0.92, + explanation: "Looks good.", + }; + + function makeCompletedResult(extractedToolData: Record): SingleResult { + return { + index: 0, + id: "reviewer", + agent: "reviewer", + agentSource: "bundled", + task: "review the patch", + assignment: "review the patch", + description: "review the patch", + exitCode: 0, + output: "", + stderr: "", + truncated: false, + durationMs: 250, + tokens: 100, + // Cast deliberately: production typings declare `unknown[]`, but the + // renderer must defend against a stray non-array value — that's + // exactly what this regression test exercises. + extractedToolData: extractedToolData as Record, + }; + } + + function makeCompletedProgress(extractedToolData: Record): AgentProgress { + return { + index: 0, + id: "reviewer", + agent: "reviewer", + agentSource: "bundled", + status: "completed", + task: "review the patch", + assignment: "review the patch", + description: "review the patch", + recentTools: [], + recentOutput: [], + toolCount: 1, + tokens: 100, + cost: 0, + durationMs: 250, + extractedToolData: extractedToolData as Record, + }; + } + + async function renderResultText(extractedToolData: Record): Promise { + const theme = (await getThemeByName("dark"))!; + const details: TaskToolDetails = { + projectAgentsDir: null, + results: [makeCompletedResult(extractedToolData)], + totalDurationMs: 250, + }; + const component = taskToolRenderer.renderResult( + { content: [{ type: "text", text: "" }], details }, + { expanded: false, isPartial: false, spinnerFrame: 0 }, + theme, + ); + return Bun.stripANSI(component.render(160).join("\n")); + } + + async function renderProgressText(extractedToolData: Record): Promise { + const theme = (await getThemeByName("dark"))!; + const details: TaskToolDetails = { + projectAgentsDir: null, + results: [], + totalDurationMs: 250, + progress: [makeCompletedProgress(extractedToolData)], + }; + const component = taskToolRenderer.renderResult( + { content: [{ type: "text", text: "" }], details }, + { expanded: false, isPartial: true, spinnerFrame: 0 }, + theme, + ); + return Bun.stripANSI(component.render(160).join("\n")); + } + + it("does not throw and still surfaces the verdict when yield is a single object (result branch)", async () => { + const text = await renderResultText({ + yield: { data: reviewVerdict, status: "success" }, + }); + expect(text).toContain("correct"); + }); + + it("does not throw and still surfaces the verdict when yield is a single object (progress branch)", async () => { + const text = await renderProgressText({ + yield: { data: reviewVerdict, status: "success" }, + }); + expect(text).toContain("correct"); + }); + + it("does not throw when yield is a non-object primitive (both branches)", async () => { + // Primitives can't carry a verdict — renderer must drop them silently + // instead of crashing. + await expect(renderResultText({ yield: "not-an-array" })).resolves.toBeString(); + await expect(renderProgressText({ yield: 42 })).resolves.toBeString(); + }); + + it("still renders the canonical array shape unchanged", async () => { + const text = await renderResultText({ + yield: [{ data: reviewVerdict, status: "success" }], + }); + expect(text).toContain("correct"); + }); +});