Skip to content

Commit 7696a20

Browse files
committed
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.
1 parent 2a8cf44 commit 7696a20

2 files changed

Lines changed: 101 additions & 19 deletions

File tree

src/tui/tool-rows.test.ts

Lines changed: 67 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ import { withTestRenderer } from "./harness";
1010
import { attachSessionBridge, createRecordingPort } from "./runtime-bridge";
1111
import { createAppShell } from "./shell/index";
1212
import {
13-
isCollapsibleRow,
1413
paintStreamRow,
1514
toolSentenceLines,
1615
type RowLayout,
@@ -22,6 +21,12 @@ const LAYOUT: RowLayout = { width: 72, multiAgent: false };
2221

2322
const painted = (row: StreamRow): string => paintStreamRow(row, LAYOUT).content;
2423

24+
const collapsed = (row: StreamRow): string =>
25+
toolSentenceLines(row)
26+
.flat()
27+
.map((segment) => segment.text)
28+
.join("");
29+
2530
const LINEAR_ISSUES = JSON.stringify({
2631
issues: [
2732
{ id: "1", title: "First" },
@@ -81,7 +86,7 @@ describe("a call and its answer", () => {
8186
expect(rows[0]?.detail).toBeUndefined();
8287
});
8388

84-
test("mark the row failed, keeping the failure out of the collapsed line", () => {
89+
test("mark the row failed and put the error on the collapsed line", () => {
8590
const rows: StreamRow[] = [];
8691
pushToolCall(rows, {
8792
name: "fetch",
@@ -95,7 +100,61 @@ describe("a call and its answer", () => {
95100
expect(rows.length).toBe(1);
96101
expect(rows[0]?.failed).toBe(true);
97102
expect(painted(defined(rows[0]))).toContain("×");
98-
expect(rows[0]?.detail?.length).toBeGreaterThan(0);
103+
expect(collapsed(defined(rows[0]))).toContain("connection refused");
104+
});
105+
106+
test("a failed read_file of a missing tool-output URI shows the error on the collapsed line", () => {
107+
const rows: StreamRow[] = [];
108+
pushToolCall(rows, {
109+
name: "read_file",
110+
arguments: JSON.stringify({ path: "tool-output:///missing-blob" }),
111+
});
112+
pushToolResult(rows, {
113+
name: "read_file",
114+
content: 'Blob not found for key: "missing-blob"',
115+
isError: true,
116+
});
117+
expect(rows[0]?.failed).toBe(true);
118+
expect(painted(defined(rows[0]))).toContain("×");
119+
expect(collapsed(defined(rows[0]))).toContain("Blob not found");
120+
expect(collapsed(defined(rows[0]))).toContain(
121+
"tool-output:///missing-blob",
122+
);
123+
});
124+
125+
test("a failed read_file of a missing filesystem path shows the error on the collapsed line", () => {
126+
const rows: StreamRow[] = [];
127+
pushToolCall(rows, {
128+
name: "read_file",
129+
arguments: JSON.stringify({ path: "/no/such/file.ts" }),
130+
});
131+
pushToolResult(rows, {
132+
name: "read_file",
133+
content: "file not found: /no/such/file.ts",
134+
isError: true,
135+
});
136+
expect(rows[0]?.failed).toBe(true);
137+
expect(painted(defined(rows[0]))).toContain("×");
138+
expect(collapsed(defined(rows[0]))).toContain("file not found");
139+
expect(collapsed(defined(rows[0]))).toContain("/no/such/file.ts");
140+
});
141+
142+
test("a successful read_file keeps the path as the subject and the success mark", () => {
143+
const rows: StreamRow[] = [];
144+
pushToolCall(rows, {
145+
name: "read_file",
146+
arguments: JSON.stringify({ path: "src/a.ts" }),
147+
});
148+
pushToolResult(rows, {
149+
name: "read_file",
150+
content: "export const a = 1;\n",
151+
});
152+
expect(rows[0]?.failed).toBeUndefined();
153+
expect(painted(defined(rows[0]))).toContain("✓");
154+
expect(painted(defined(rows[0]))).not.toContain("×");
155+
expect(collapsed(defined(rows[0]))).toContain("src/a.ts");
156+
expect(collapsed(defined(rows[0]))).not.toContain("file not found");
157+
expect(collapsed(defined(rows[0]))).not.toContain("Blob not found");
99158
});
100159

101160
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", () => {
256315
expect(rows[1]?.pending).toBe(true);
257316
});
258317

259-
// Acceptance criterion: a failed sub-agent surfaces its error inline
260-
// (expandable), not a bare mark with nothing behind it. `mergeToolRows` /
261-
// `toolResultRow` already carry the failed result's own text into `detail`
262-
// — untouched by this fix, but only reachable per-call once results resolve
263-
// to the right row instead of a neighbour's.
264-
test("a failed call keeps its error text behind the expand arrow", () => {
318+
// A failed call must show its error on the collapsed line, not only behind
319+
// the expand arrow.
320+
test("a failed call shows its error text on the collapsed line", () => {
265321
const rows: StreamRow[] = [];
266322
pushToolCall(rows, {
267323
name: "spawn_agent",
@@ -278,8 +334,8 @@ describe("parallel calls to the same tool", () => {
278334
callId: "c1",
279335
});
280336
expect(rows[0]?.failed).toBe(true);
281-
expect(isCollapsibleRow(defined(rows[0]))).toBe(true);
282-
expect(rows[0]?.detail?.[0]?.[0]?.text).toContain("boom");
337+
expect(painted(defined(rows[0]))).toContain("×");
338+
expect(collapsed(defined(rows[0]))).toContain("boom");
283339
});
284340
});
285341

src/tui/tool-rows.ts

Lines changed: 34 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -44,16 +44,38 @@ function appendRunLine(
4444
/** Longest an answer's own words may run before they belong behind the arrow. */
4545
const MAX_ADDENDUM = 40;
4646

47+
/** Failed-result addendum: same budget as `mergedToolCollapsedPreview` errors. */
48+
const MAX_ERROR_ADDENDUM = 72;
49+
50+
/**
51+
* Flatten a failed payload the way the collapsed log preview does: one line,
52+
* abbreviated, so the operator can read why without expanding.
53+
*/
54+
function failedAddendum(payload: string): string | undefined {
55+
const oneLine = payload
56+
.split("\n")
57+
.map((line) => line.trim())
58+
.filter((line) => line.length > 0)
59+
.join(" ");
60+
if (oneLine.length === 0) return undefined;
61+
return oneLine.length <= MAX_ERROR_ADDENDUM
62+
? oneLine
63+
: `${oneLine.slice(0, MAX_ERROR_ADDENDUM - 1)}…`;
64+
}
65+
4766
/**
48-
* What an answer adds to the line its call already wrote: a count, a short
49-
* status — never prose, and never the payload itself. A fetched page, a file
50-
* body or a search dump says nothing on one line and would push the subject
51-
* (the URL, the path, the query) off the row, so anything unbounded is left
52-
* behind the expand key.
67+
* What an answer adds to the line its call already wrote. A success contributes
68+
* a count or a short status — never prose, and never the payload itself. A
69+
* failure contributes the error, abbreviated, because that is the one thing
70+
* the operator must be able to read without pressing expand. A fetched page, a
71+
* file body or a search dump says nothing on one line and would push the
72+
* subject (the URL, the path, the query) off the row, so anything unbounded
73+
* is left behind the expand key.
5374
*/
5475
export function resultAddendum(result: StreamRow): string | undefined {
5576
const payload = result.text.trim();
5677
if (payload.length === 0) return undefined;
78+
if (result.failed === true) return failedAddendum(payload);
5779
const records = extractMcpRecords(payload);
5880
if (records !== null) return countNoun(records.items.length, "result");
5981
const lines = payload.split("\n");
@@ -73,7 +95,8 @@ function countNoun(count: number, noun: string): string {
7395
* The row keeps saying what the call was — the URL fetched, the path read, the
7496
* query searched. That is the stable identifier, and it is the one thing the
7597
* payload can never be trusted to reproduce. The answer contributes the marker,
76-
* a short factual addendum where it has one, and the body behind the arrow.
98+
* a short factual addendum where it has one (the error, when it failed), and
99+
* the body behind the arrow.
77100
*/
78101
export function mergeToolRows(call: StreamRow, result: StreamRow): StreamRow {
79102
const failed = result.failed === true;
@@ -83,11 +106,14 @@ export function mergeToolRows(call: StreamRow, result: StreamRow): StreamRow {
83106
stat: _stat,
84107
...answered
85108
} = call;
86-
const addendum = failed ? undefined : resultAddendum(result);
109+
const addendum = resultAddendum(result);
87110
// A live sub-agent's elapsed-time trailer is scaffolding for the wait, not a
88111
// fact about the call the way a diff's own +/- count is — the answer's stat
89112
// must win over it rather than being shadowed by whatever it last read.
90-
const callStat = call.agentWorking !== undefined ? undefined : call.stat;
113+
// A failure's error likewise beats a leftover +/- count: the operator needs
114+
// the reason, not a diff that did not land.
115+
const callStat =
116+
failed || call.agentWorking !== undefined ? undefined : call.stat;
91117
const base: StreamRow = {
92118
...answered,
93119
text: result.text,

0 commit comments

Comments
 (0)