From 7696a20e3fcdfa41c13a90d5c7bcc0fc3099e174 Mon Sep 17 00:00:00 2001 From: Sawyer Date: Thu, 10 Sep 2026 01:04:26 -0700 Subject: [PATCH] Show failed file-tool errors on the collapsed transcript line Failed reads painted a cross and the path only, so a missing blob or file looked identical until the row was expanded. The error belongs on the collapsed line with the mark. --- src/tui/tool-rows.test.ts | 78 +++++++++++++++++++++++++++++++++------ src/tui/tool-rows.ts | 42 +++++++++++++++++---- 2 files changed, 101 insertions(+), 19 deletions(-) diff --git a/src/tui/tool-rows.test.ts b/src/tui/tool-rows.test.ts index 43c01cdbf..dc5c38bbb 100644 --- a/src/tui/tool-rows.test.ts +++ b/src/tui/tool-rows.test.ts @@ -10,7 +10,6 @@ import { withTestRenderer } from "./harness"; import { attachSessionBridge, createRecordingPort } from "./runtime-bridge"; import { createAppShell } from "./shell/index"; import { - isCollapsibleRow, paintStreamRow, toolSentenceLines, type RowLayout, @@ -22,6 +21,12 @@ const LAYOUT: RowLayout = { width: 72, multiAgent: false }; const painted = (row: StreamRow): string => paintStreamRow(row, LAYOUT).content; +const collapsed = (row: StreamRow): string => + toolSentenceLines(row) + .flat() + .map((segment) => segment.text) + .join(""); + const LINEAR_ISSUES = JSON.stringify({ issues: [ { id: "1", title: "First" }, @@ -81,7 +86,7 @@ describe("a call and its answer", () => { expect(rows[0]?.detail).toBeUndefined(); }); - test("mark the row failed, keeping the failure out of the collapsed line", () => { + test("mark the row failed and put the error on the collapsed line", () => { const rows: StreamRow[] = []; pushToolCall(rows, { name: "fetch", @@ -95,7 +100,61 @@ describe("a call and its answer", () => { expect(rows.length).toBe(1); expect(rows[0]?.failed).toBe(true); expect(painted(defined(rows[0]))).toContain("×"); - expect(rows[0]?.detail?.length).toBeGreaterThan(0); + expect(collapsed(defined(rows[0]))).toContain("connection refused"); + }); + + test("a failed read_file of a missing tool-output URI shows the error on the collapsed line", () => { + const rows: StreamRow[] = []; + pushToolCall(rows, { + name: "read_file", + arguments: JSON.stringify({ path: "tool-output:///missing-blob" }), + }); + pushToolResult(rows, { + name: "read_file", + content: 'Blob not found for key: "missing-blob"', + isError: true, + }); + expect(rows[0]?.failed).toBe(true); + expect(painted(defined(rows[0]))).toContain("×"); + expect(collapsed(defined(rows[0]))).toContain("Blob not found"); + expect(collapsed(defined(rows[0]))).toContain( + "tool-output:///missing-blob", + ); + }); + + test("a failed read_file of a missing filesystem path shows the error on the collapsed line", () => { + const rows: StreamRow[] = []; + pushToolCall(rows, { + name: "read_file", + arguments: JSON.stringify({ path: "/no/such/file.ts" }), + }); + pushToolResult(rows, { + name: "read_file", + content: "file not found: /no/such/file.ts", + isError: true, + }); + expect(rows[0]?.failed).toBe(true); + expect(painted(defined(rows[0]))).toContain("×"); + expect(collapsed(defined(rows[0]))).toContain("file not found"); + expect(collapsed(defined(rows[0]))).toContain("/no/such/file.ts"); + }); + + test("a successful read_file keeps the path as the subject and the success mark", () => { + const rows: StreamRow[] = []; + pushToolCall(rows, { + name: "read_file", + arguments: JSON.stringify({ path: "src/a.ts" }), + }); + pushToolResult(rows, { + name: "read_file", + content: "export const a = 1;\n", + }); + expect(rows[0]?.failed).toBeUndefined(); + expect(painted(defined(rows[0]))).toContain("✓"); + expect(painted(defined(rows[0]))).not.toContain("×"); + expect(collapsed(defined(rows[0]))).toContain("src/a.ts"); + expect(collapsed(defined(rows[0]))).not.toContain("file not found"); + expect(collapsed(defined(rows[0]))).not.toContain("Blob not found"); }); test("a resolved sub-agent dispatch drops its live elapsed-time trailer for the real answer", () => { @@ -256,12 +315,9 @@ describe("parallel calls to the same tool", () => { expect(rows[1]?.pending).toBe(true); }); - // Acceptance criterion: a failed sub-agent surfaces its error inline - // (expandable), not a bare mark with nothing behind it. `mergeToolRows` / - // `toolResultRow` already carry the failed result's own text into `detail` - // — untouched by this fix, but only reachable per-call once results resolve - // to the right row instead of a neighbour's. - test("a failed call keeps its error text behind the expand arrow", () => { + // A failed call must show its error on the collapsed line, not only behind + // the expand arrow. + test("a failed call shows its error text on the collapsed line", () => { const rows: StreamRow[] = []; pushToolCall(rows, { name: "spawn_agent", @@ -278,8 +334,8 @@ describe("parallel calls to the same tool", () => { callId: "c1", }); expect(rows[0]?.failed).toBe(true); - expect(isCollapsibleRow(defined(rows[0]))).toBe(true); - expect(rows[0]?.detail?.[0]?.[0]?.text).toContain("boom"); + expect(painted(defined(rows[0]))).toContain("×"); + expect(collapsed(defined(rows[0]))).toContain("boom"); }); }); diff --git a/src/tui/tool-rows.ts b/src/tui/tool-rows.ts index 7948944dc..5972454b5 100644 --- a/src/tui/tool-rows.ts +++ b/src/tui/tool-rows.ts @@ -44,16 +44,38 @@ function appendRunLine( /** Longest an answer's own words may run before they belong behind the arrow. */ const MAX_ADDENDUM = 40; +/** Failed-result addendum: same budget as `mergedToolCollapsedPreview` errors. */ +const MAX_ERROR_ADDENDUM = 72; + +/** + * Flatten a failed payload the way the collapsed log preview does: one line, + * abbreviated, so the operator can read why without expanding. + */ +function failedAddendum(payload: string): string | undefined { + const oneLine = payload + .split("\n") + .map((line) => line.trim()) + .filter((line) => line.length > 0) + .join(" "); + if (oneLine.length === 0) return undefined; + return oneLine.length <= MAX_ERROR_ADDENDUM + ? oneLine + : `${oneLine.slice(0, MAX_ERROR_ADDENDUM - 1)}…`; +} + /** - * What an answer adds to the line its call already wrote: a count, a short - * status — never prose, and never the payload itself. A fetched page, a file - * body or a search dump says nothing on one line and would push the subject - * (the URL, the path, the query) off the row, so anything unbounded is left - * behind the expand key. + * What an answer adds to the line its call already wrote. A success contributes + * a count or a short status — never prose, and never the payload itself. A + * failure contributes the error, abbreviated, because that is the one thing + * the operator must be able to read without pressing expand. A fetched page, a + * file body or a search dump says nothing on one line and would push the + * subject (the URL, the path, the query) off the row, so anything unbounded + * is left behind the expand key. */ export function resultAddendum(result: StreamRow): string | undefined { const payload = result.text.trim(); if (payload.length === 0) return undefined; + if (result.failed === true) return failedAddendum(payload); const records = extractMcpRecords(payload); if (records !== null) return countNoun(records.items.length, "result"); const lines = payload.split("\n"); @@ -73,7 +95,8 @@ function countNoun(count: number, noun: string): string { * The row keeps saying what the call was — the URL fetched, the path read, the * query searched. That is the stable identifier, and it is the one thing the * payload can never be trusted to reproduce. The answer contributes the marker, - * a short factual addendum where it has one, and the body behind the arrow. + * a short factual addendum where it has one (the error, when it failed), and + * the body behind the arrow. */ export function mergeToolRows(call: StreamRow, result: StreamRow): StreamRow { const failed = result.failed === true; @@ -83,11 +106,14 @@ export function mergeToolRows(call: StreamRow, result: StreamRow): StreamRow { stat: _stat, ...answered } = call; - const addendum = failed ? undefined : resultAddendum(result); + const addendum = resultAddendum(result); // A live sub-agent's elapsed-time trailer is scaffolding for the wait, not a // fact about the call the way a diff's own +/- count is — the answer's stat // must win over it rather than being shadowed by whatever it last read. - const callStat = call.agentWorking !== undefined ? undefined : call.stat; + // A failure's error likewise beats a leftover +/- count: the operator needs + // the reason, not a diff that did not land. + const callStat = + failed || call.agentWorking !== undefined ? undefined : call.stat; const base: StreamRow = { ...answered, text: result.text,