From d1890839e209c5c03fae8caea9c25352a1fd0ce7 Mon Sep 17 00:00:00 2001 From: eleith Date: Thu, 5 Nov 2026 08:28:52 -0800 Subject: [PATCH] fix(tool-rendering): prefer recorded bash execution duration --- .../tool-rendering-recorded-bash-duration.md | 5 ++ .../tool-rendering/src/bash-timing.test.ts | 19 +++-- extensions/tool-rendering/src/bash-timing.ts | 2 +- extensions/tool-rendering/src/bash.test.ts | 77 ++++++++++++++++--- extensions/tool-rendering/src/bash.ts | 16 ++-- extensions/tool-rendering/src/sdk.test.ts | 34 ++++++++ 6 files changed, 130 insertions(+), 23 deletions(-) create mode 100644 .changeset/tool-rendering-recorded-bash-duration.md diff --git a/.changeset/tool-rendering-recorded-bash-duration.md b/.changeset/tool-rendering-recorded-bash-duration.md new file mode 100644 index 0000000..84556e5 --- /dev/null +++ b/.changeset/tool-rendering-recorded-bash-duration.md @@ -0,0 +1,5 @@ +--- +"@eleith-pi/tool-rendering": patch +--- + +Use Pi's recorded execution duration for completed and restored bash rows, retaining live elapsed updates and the observed-time fallback when duration metadata is unavailable. Do not fabricate zero durations for final-only rows that were never observed running. diff --git a/extensions/tool-rendering/src/bash-timing.test.ts b/extensions/tool-rendering/src/bash-timing.test.ts index 3c6a031..8840d59 100644 --- a/extensions/tool-rendering/src/bash-timing.test.ts +++ b/extensions/tool-rendering/src/bash-timing.test.ts @@ -39,14 +39,21 @@ afterEach(() => { }); describe("BashTiming", () => { - it("is inert until an active, non-error execution is rendered", () => { + it("starts timing only for an observed running execution, never a final-only row", () => { const timing = new BashTiming(); - expect(vi.getTimerCount()).toBe(0); const waiting = context({ executionStarted: false }); - expect(timing.update(waiting, partial)).toBeUndefined(); - expect(timing.update(context({ isError: true }), partial)).toBe(0); - expect(timing.update(context(), final)).toBe(0); - expect(vi.getTimerCount()).toBe(0); + const waitingElapsed = timing.update(waiting, partial); + expect(waitingElapsed).toBeUndefined(); + + const failed = context({ isError: true }); + const failedElapsed = timing.update(failed, partial); + expect(failedElapsed).toBeUndefined(); + + const completed = context(); + const completedElapsed = timing.update(completed, final); + expect(completedElapsed).toBeUndefined(); + const timerCount = vi.getTimerCount(); + expect(timerCount).toBe(0); timing.dispose(); }); diff --git a/extensions/tool-rendering/src/bash-timing.ts b/extensions/tool-rendering/src/bash-timing.ts index fdc3c48..d756b17 100644 --- a/extensions/tool-rendering/src/bash-timing.ts +++ b/extensions/tool-rendering/src/bash-timing.ts @@ -31,7 +31,7 @@ export class BashTiming { const state = context.state as TimingState; const running = context.executionStarted && Boolean(options.isPartial) && !context.isError; let timing = state[timingKey]; - if (!this.disposed && context.executionStarted && (!timing || timing.owner !== this)) { + if (!this.disposed && running && (!timing || timing.owner !== this)) { timing = { owner: this, generation: this.generation, startedAt: Date.now() }; state[timingKey] = timing; } diff --git a/extensions/tool-rendering/src/bash.test.ts b/extensions/tool-rendering/src/bash.test.ts index 08273a1..16eec1a 100644 --- a/extensions/tool-rendering/src/bash.test.ts +++ b/extensions/tool-rendering/src/bash.test.ts @@ -290,22 +290,77 @@ describe("bash renderers", () => { expect(vi.getTimerCount()).toBe(0); }); - it.each([ - [61000, "1m1s"], - [3600000, "1h"], - [3660000, "1h1m"], - ])("retains original long-duration formatting: %s", (ms, expected) => { + it("prefers recorded final duration over the live clock and remains stable on redraw", () => { const { state, render } = fixture(); const ctx = context({ executionStarted: true, isPartial: true }); - render(result(""), partial, theme, ctx); - vi.setSystemTime(1000 + ms); + const emptyOutput = result(""); + const running = render(emptyOutput, partial, theme, ctx); + vi.advanceTimersByTime(2300); + const runningLines = running.render(100); + const runningText = runningLines.join("\n"); + expect(runningText).toContain("2.3s · running"); state.bashTiming.stop(ctx.toolCallId); - const text = render(result("done"), collapsed, theme, { ...ctx, isPartial: false }) - .render(100) - .join("\n"); - expect(text).toContain(`${expected} · ✓ exit 0`); + vi.advanceTimersByTime(5000); + + const completedContext = { + ...ctx, + isPartial: false, + durationMs: 4200, + lastComponent: running, + }; + const completedOutput = result("done"); + const completed = render(completedOutput, collapsed, theme, completedContext); + const completedLines = completed.render(100); + const completedText = completedLines.join("\n"); + expect(completedText).toContain("4.2s · ✓ exit 0"); + expect(completedText).not.toContain("2.3s"); + vi.advanceTimersByTime(5000); + const resizedLines = completed.render(80); + const resizedText = resizedLines.join("\n"); + expect(resizedText).toContain("4.2s · ✓ exit 0"); + const timerCount = vi.getTimerCount(); + expect(timerCount).toBe(0); }); + it.each([ + { durationMs: 0, expected: "0.0s", isError: false }, + { durationMs: 4200, expected: "4.2s", isError: false }, + { durationMs: 4200, expected: "4.2s", isError: true }, + { durationMs: 61000, expected: "1m1s", isError: false }, + { durationMs: 3600000, expected: "1h", isError: false }, + { durationMs: 3660000, expected: "1h1m", isError: false }, + ])( + "renders recorded duration without live timing: $durationMs ms, error=$isError", + ({ durationMs, expected, isError }) => { + const { render } = fixture(); + const ctx = context({ durationMs, isError }); + const output = result("done"); + const component = render(output, collapsed, theme, ctx); + const lines = component.render(100); + const text = lines.join("\n"); + const status = isError ? "✗ error" : "✓ exit 0"; + expect(text).toContain(`${expected} · ${status}`); + const timerCount = vi.getTimerCount(); + expect(timerCount).toBe(0); + }, + ); + + it.each([false, true])( + "does not invent duration for a final-only row (executionStarted=%s)", + (executionStarted) => { + const { render } = fixture(); + const ctx = context({ executionStarted }); + const output = result("done"); + const component = render(output, collapsed, theme, ctx); + const lines = component.render(100); + const text = lines.join("\n"); + expect(text).toContain("✓ exit 0 · collapsed"); + expect(text).not.toContain("0.0s"); + const timerCount = vi.getTimerCount(); + expect(timerCount).toBe(0); + }, + ); + it("keeps an already selected framed renderer and its timer active after hide", () => { const { state, render } = fixture(); const ctx = context({ executionStarted: true, isPartial: true }); diff --git a/extensions/tool-rendering/src/bash.ts b/extensions/tool-rendering/src/bash.ts index 4b40185..94aaa04 100644 --- a/extensions/tool-rendering/src/bash.ts +++ b/extensions/tool-rendering/src/bash.ts @@ -40,8 +40,8 @@ export function buildBashRendering( const renderResult: BashRenderResult = (result, options, theme, context) => { state.bashTiming.update(context, options); - const { state: rowState, isError, isPartial } = context; - const paintContext = { state: rowState, isError, isPartial }; + const { state: rowState, isError, isPartial, durationMs } = context; + const paintContext = { state: rowState, isError, isPartial, durationMs }; return frameComponent(context, (width) => renderBashResult(result, options, theme, paintContext, width), ); @@ -71,11 +71,13 @@ function renderBashResult( result: AgentToolResult, options: BashRenderOptions, theme: BashTheme, - context: Pick, + context: Pick, width: number, ): string { const status = frameStatus(context); - const elapsedMs = bashElapsed(context.state); + let elapsedMs = bashElapsed(context.state); + // Pi's execution duration is authoritative for completed rows, including restored history. + if (!options.isPartial && context.durationMs !== undefined) elapsedMs = context.durationMs; const rawOutput = textFromResult(result); // Only failed commands have a Pi status line; success and partial stdout are arbitrary text. const parsed = context.isError @@ -83,7 +85,11 @@ function renderBashResult( : { body: rawOutput, exitCode: options.isPartial ? null : 0, timedOut: false, aborted: false }; const { body, hidden } = previewBashOutput(parsed.body, options.expanded); const output = context.isError && body ? theme.fg("error", body) : body; - const duration = elapsedMs === undefined ? "" : theme.fg("dim", formatDuration(elapsedMs)); + let duration = ""; + if (elapsedMs !== undefined) { + const formattedDuration = formatDuration(elapsedMs); + duration = theme.fg("dim", formattedDuration); + } const runStatus = options.isPartial ? theme.fg("warning", body ? "running" : "running · no output yet") : bashSummaryLabel(parsed, theme) || (context.isError ? theme.fg("error", "✗ error") : ""); diff --git a/extensions/tool-rendering/src/sdk.test.ts b/extensions/tool-rendering/src/sdk.test.ts index c48ff3a..b9e10e8 100644 --- a/extensions/tool-rendering/src/sdk.test.ts +++ b/extensions/tool-rendering/src/sdk.test.ts @@ -224,6 +224,40 @@ it("uses the resolver chain and keeps streamed rows stable across toggles", asyn isError: false, }); expect(vi.getTimerCount()).toBe(0); + const completedResult = { + content: [{ type: "text" as const, text: "completed output" }], + details: undefined, + isError: false, + durationMs: 4200, + }; + row.updateResult(completedResult); + const completedLines = row.render(80); + const completedText = completedLines.join("\n"); + expect(completedText).toContain("4.2s"); + expect(completedText).not.toContain("2.0s"); + + // Restored rows receive recorded duration without markExecutionStarted(). + const restoredDefinition = resolve("bash"); + const restored = new ToolExecutionComponent( + "bash", + "restored-bash", + { command: "printf restored" }, + {}, + restoredDefinition, + { requestRender } as never, + directory, + ); + restored.setArgsComplete(); + restored.updateResult(completedResult); + restored.setExpanded(true); + restored.invalidate(); + const restoredLines = restored.render(80); + const restoredText = restoredLines.join("\n"); + expect(restoredText).toContain("4.2s"); + expect(restoredText).toContain("expanded"); + const timerCount = vi.getTimerCount(); + expect(timerCount).toBe(0); + // Shutdown also stops an abandoned framed row without fabricating a final result. const abandoned = new ToolExecutionComponent( "bash", -- 2.54.0