fix(coding-agent): guarded task renderer against non-array yield slot

renderAgentResult and the live-progress sibling cast
extractedToolData?.yield to Array<{ data }> and called ?.map without
checking the actual runtime shape. Optional chaining only short-circuits
on null/undefined, so any stray non-array value (a single yield object
landing in the slot) made .map undefined and threw
TypeError: completeData?.map is not a function — taking down every
`review` task render.

Both sites now route through a new normalizeYieldData helper (next to
normalizeReportFindings) that returns an array of yield records: it
preserves arrays unchanged, wraps a single object as a 1-element array
so the verdict still renders, and drops primitives. Added a regression
test exercising the result branch, the progress branch, the primitive
fall-through, and the canonical array shape — all of the failing-branch
ones reproduce the crash on the pre-fix renderer.

Fixes #1987
This commit is contained in:
roboomp
2026-06-06 11:16:44 +00:00
parent d9f5a8c7ac
commit 1ffa6dc6fb
3 changed files with 171 additions and 8 deletions
+4
View File
@@ -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
+33 -8
View File
@@ -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)";
@@ -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<string, unknown>): 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<string, unknown[]>,
};
}
function makeCompletedProgress(extractedToolData: Record<string, unknown>): 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<string, unknown[]>,
};
}
async function renderResultText(extractedToolData: Record<string, unknown>): Promise<string> {
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<string, unknown>): Promise<string> {
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");
});
});