Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 67 additions & 11 deletions src/tui/tool-rows.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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" },
Expand Down Expand Up @@ -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",
Expand All @@ -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", () => {
Expand Down Expand Up @@ -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",
Expand All @@ -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");
});
});

Expand Down
42 changes: 34 additions & 8 deletions src/tui/tool-rows.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand All @@ -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;
Expand All @@ -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,
Expand Down
Loading