From 68ea3b92e317bd2b898266e478a5d1c4cd03e500 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Wed, 16 Sep 2026 22:58:44 -0700 Subject: [PATCH] fix(native-chat): stop a collapsed run claiming success when a tool call failed (#21151) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(native-chat): stop a collapsed run claiming success when a tool call failed A settled activity group drew its completion mark whenever no call in it was `running`. That is not a success test: a tool call is `running`, `completed` or `failed`, so a run whose call failed had nothing running, took the mark, and asserted success over a failure the reader could only find by expanding the run. Success is now stated rather than inferred. `nativeChatToolRunSucceeded` grants the mark only to a run that is settled, has nothing still running, and has no failed call — a call's own `failed` verdict or an error result, the same composite test the task-list, edit-card and ask-row readers already use. A call with no lifecycle state is neither, so legacy transcripts still settle. A collapsed run that did contain failures now says so in the header, as a quiet `N failed` in the header's own mono type with a spoken `Failed tool calls: N`. Text only: a tool error is routine work, so no destructive tint and no swapped glyph. The count is taken over every call in the run, not the latest. * fix(native-chat): count failed tool calls without result mispairing --- .../native-chat/NativeChatToolRun.test.tsx | 39 ++++++- .../native-chat/NativeChatToolRun.tsx | 32 +++++- .../src/i18n/en-runtime-required.json | 2 + src/renderer/src/i18n/locales/en.json | 2 + src/shared/native-chat-tool-activity.ts | 7 +- .../native-chat-tool-run-outcome.test.ts | 107 ++++++++++++++++++ src/shared/native-chat-tool-run-outcome.ts | 50 ++++++++ 7 files changed, 231 insertions(+), 8 deletions(-) create mode 100644 src/shared/native-chat-tool-run-outcome.test.ts create mode 100644 src/shared/native-chat-tool-run-outcome.ts diff --git a/src/renderer/src/components/native-chat/NativeChatToolRun.test.tsx b/src/renderer/src/components/native-chat/NativeChatToolRun.test.tsx index febddb3dbbf..6c043b92374 100644 --- a/src/renderer/src/components/native-chat/NativeChatToolRun.test.tsx +++ b/src/renderer/src/components/native-chat/NativeChatToolRun.test.tsx @@ -443,7 +443,7 @@ describe('NativeChatToolRun', () => { expect(container.querySelector('.animate-pulse')).toBeNull() }) - it('keeps failed tool runs visually neutral while collapsed', () => { + it('refuses the completion mark to a collapsed run whose call failed', () => { const blocks: NativeChatBlock[] = [ { type: 'tool-call', name: 'shell', input: { command: 'false' }, state: 'failed' }, { type: 'tool-result', output: 'exit 1', isError: true } @@ -451,11 +451,46 @@ describe('NativeChatToolRun', () => { const { container } = render() - expect(container.querySelector('.lucide-check')).toBeInTheDocument() + // The defect: nothing was running, so the header inherited a check and + // asserted success over a failure only expanding the run would reveal. + expect(container.querySelector('.lucide-check')).toBeNull() + expect(runHeader(container)).toHaveTextContent('1 failed') + expect(runHeader(container)).toHaveAccessibleName(/Failed tool calls: 1/) + // Quiet text, not a severity escalation: no destructive tint, no swapped glyph. expect(container.querySelector('.lucide-circle-alert')).toBeNull() + expect(container.querySelector('[class*="destructive"]')).toBeNull() + // The detail still belongs behind the disclosure. expect(screen.queryByText('exit 1')).toBeNull() }) + it('counts every failed call in a run, not just the last one', () => { + const blocks: NativeChatBlock[] = [ + { type: 'tool-call', name: 'shell', input: { command: 'a' }, state: 'failed' }, + { type: 'tool-result', output: 'exit 1', isError: true }, + { type: 'tool-call', name: 'shell', input: { command: 'b' }, state: 'failed' }, + { type: 'tool-result', output: 'exit 2', isError: true }, + { type: 'tool-call', name: 'shell', input: { command: 'c' }, state: 'completed' }, + { type: 'tool-result', output: 'ok' } + ] + + const { container } = render() + + expect(runHeader(container)).toHaveTextContent('2 failed') + expect(container.querySelector('.lucide-check')).toBeNull() + }) + + it('says nothing and keeps the mark when every call in the run succeeded', () => { + const blocks: NativeChatBlock[] = [ + { type: 'tool-call', name: 'shell', input: { command: 'a' }, state: 'completed' }, + { type: 'tool-result', output: 'ok' } + ] + + const { container } = render() + + expect(runHeader(container)).not.toHaveTextContent('failed') + expect(container.querySelector('.lucide-check')).toBeInTheDocument() + }) + it('keeps settled tool activity behind the completed turn disclosure', () => { const blocks: NativeChatBlock[] = [ { type: 'tool-call', name: 'shell', input: { command: 'git log -1' }, state: 'failed' }, diff --git a/src/renderer/src/components/native-chat/NativeChatToolRun.tsx b/src/renderer/src/components/native-chat/NativeChatToolRun.tsx index 648161c6d7d..e79a344ffc0 100644 --- a/src/renderer/src/components/native-chat/NativeChatToolRun.tsx +++ b/src/renderer/src/components/native-chat/NativeChatToolRun.tsx @@ -25,6 +25,7 @@ import { selectActiveToolCall } from '../../../../shared/native-chat-tool-activity' import { nativeChatToolRunIconName } from '../../../../shared/native-chat-tool-icon' +import { nativeChatToolRunOutcome } from '../../../../shared/native-chat-tool-run-outcome' import { nativeChatAskRunBlocks, nativeChatAskRunSubject @@ -123,9 +124,9 @@ export function NativeChatToolRun({ : null const isSettled = headerActiveCall == null const askIsActive = selectActiveToolCall(unansweredAsks, { activeTurnIsWorking }) !== null - const hasRunningCall = headerBlocks.some( - (block) => isToolCallBlock(block) && block.state === 'running' - ) + const { succeeded: runSucceeded, failedCallCount } = nativeChatToolRunOutcome(headerBlocks, { + activeTurnIsWorking + }) // The turn caret opens the activity group while each child tool stays collapsed. const expandToolLines = expandOverride === undefined ? open : false // Diffing every edit is the run's most expensive work, so a collapsed run — @@ -273,8 +274,29 @@ export function NativeChatToolRun({ {fallbackLabel} )} - {/* A running item cannot inherit completion from its turn. */} - {structuredActivityUi && !hasRunningCall ? ( + {failedCallCount > 0 ? ( + /* Outside the truncating member list, so the one thing the reader + cannot afford to miss survives a pane too narrow to print it. + Quiet text in the header's own type, not a destructive tint or a + swapped glyph: a tool error is routine work, and the failing + line's own detail is one click away. */ + + {translate( + 'components.native-chat.tool.failedCount', + NATIVE_CHAT_TOOL_ACTIVITY_COPY.failedCount, + { value0: failedCallCount } + )} + + ) : null} + {/* Only a stated success is marked done — see nativeChatToolRunOutcome. */} + {structuredActivityUi && runSucceeded ? ( ) : null} {/* Chevron is revealed on hover when collapsed and points down when open. */} diff --git a/src/renderer/src/i18n/en-runtime-required.json b/src/renderer/src/i18n/en-runtime-required.json index 6713f97059c..f25236cc48c 100644 --- a/src/renderer/src/i18n/en-runtime-required.json +++ b/src/renderer/src/i18n/en-runtime-required.json @@ -2677,6 +2677,8 @@ "tool": { "countN": "{{value0}} tool calls", "countOne": "1 tool call", + "failedCallsLabel": "Failed tool calls: {{value0}}", + "failedCount": "{{value0}} failed", "moreCalls": "+{{value0}} more", "ranCommandManyToolsSummary": "Ran {{commandCount}} command and used {{toolCount}} tools", "ranCommandOneToolSummary": "Ran {{commandCount}} command and used {{toolCount}} tool", diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index 8cfb88a7989..f6672e203af 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -17194,6 +17194,8 @@ "countOne": "1 tool call", "countN": "{{value0}} tool calls", "moreCalls": "+{{value0}} more", + "failedCount": "{{value0}} failed", + "failedCallsLabel": "Failed tool calls: {{value0}}", "runningPreview": "Running {{preview}}", "runningCommand": "Running command", "runningNamedPreview": "Running {{toolName}} {{preview}}", diff --git a/src/shared/native-chat-tool-activity.ts b/src/shared/native-chat-tool-activity.ts index 58a1179e9ac..14887ea28c4 100644 --- a/src/shared/native-chat-tool-activity.ts +++ b/src/shared/native-chat-tool-activity.ts @@ -14,7 +14,12 @@ export const NATIVE_CHAT_TOOL_ACTIVITY_COPY = { runningNamed: 'Running {{toolName}}', countOne: '1 tool call', countN: '{{value0}} tool calls', - moreCalls: '+{{value0}} more' + moreCalls: '+{{value0}} more', + /** Quiet decoration on a settled collapsed header; the run's lines carry the + * detail. Count-agnostic wording so one entry serves any number. */ + failedCount: '{{value0}} failed', + /** Spoken form of the same mark — `1 failed` alone does not say failed what. */ + failedCallsLabel: 'Failed tool calls: {{value0}}' } as const /** Tools whose call is a shell command, so the row reads as terminal activity diff --git a/src/shared/native-chat-tool-run-outcome.test.ts b/src/shared/native-chat-tool-run-outcome.test.ts new file mode 100644 index 00000000000..42ca566f95a --- /dev/null +++ b/src/shared/native-chat-tool-run-outcome.test.ts @@ -0,0 +1,107 @@ +import { describe, expect, it } from 'vitest' +import { nativeChatToolRunOutcome } from './native-chat-tool-run-outcome' +import type { NativeChatBlock } from './native-chat-types' + +function call(command: string, state?: 'running' | 'completed' | 'failed'): NativeChatBlock { + return { type: 'tool-call', name: 'shell', input: { command }, state } +} + +function result(output: string, isError?: boolean): NativeChatBlock { + return { type: 'tool-result', output, isError } +} + +describe('nativeChatToolRunOutcome', () => { + it('counts a provider failure verdict', () => { + expect(nativeChatToolRunOutcome([call('a', 'failed'), result('exit 1', true)], {})).toEqual({ + failedCallCount: 1, + succeeded: false + }) + }) + + it('counts an error result on a lane that writes no lifecycle state', () => { + expect(nativeChatToolRunOutcome([call('a'), result('exit 1', true)], {})).toEqual({ + failedCallCount: 1, + succeeded: false + }) + }) + + it('counts every failure, not just the run’s last call', () => { + expect( + nativeChatToolRunOutcome( + [ + call('a', 'failed'), + result('exit 1', true), + call('b', 'failed'), + result('exit 2', true), + call('c', 'completed'), + result('ok') + ], + {} + ).failedCallCount + ).toBe(2) + }) + + it('counts a failed call once, not twice for its error result', () => { + expect( + nativeChatToolRunOutcome([call('a', 'failed'), result('exit 1', true)], {}).failedCallCount + ).toBe(1) + }) + + it('does not misattribute a later error to an outputless completed call', () => { + expect( + nativeChatToolRunOutcome( + [call('a', 'completed'), call('b', 'failed'), result('exit 1', true)], + {} + ).failedCallCount + ).toBe(1) + }) + + it('reports nothing for a clean run', () => { + expect( + nativeChatToolRunOutcome([call('a', 'completed'), result('ok')], {}).failedCallCount + ).toBe(0) + }) + + it('refuses success to a failed run even though nothing is running', () => { + expect( + nativeChatToolRunOutcome([call('a', 'failed'), result('exit 1', true)], {}).succeeded + ).toBe(false) + }) + + it('refuses success to a run whose call is still running', () => { + expect( + nativeChatToolRunOutcome([call('a', 'running')], { activeTurnIsWorking: true }).succeeded + ).toBe(false) + }) + + it('refuses success to a call still running after its turn ended', () => { + expect( + nativeChatToolRunOutcome([call('a', 'running')], { activeTurnIsWorking: false }).succeeded + ).toBe(false) + }) + + it('refuses success while a state-less call rides a working turn', () => { + expect(nativeChatToolRunOutcome([call('a')], { activeTurnIsWorking: true }).succeeded).toBe( + false + ) + }) + + it('grants success to a completed run', () => { + expect(nativeChatToolRunOutcome([call('a', 'completed'), result('ok')], {}).succeeded).toBe( + true + ) + }) + + it('still settles a legacy run that carries no lifecycle state', () => { + expect(nativeChatToolRunOutcome([call('a'), result('ok')], {}).succeeded).toBe(true) + }) + + it('refuses success when one call of several failed', () => { + expect( + nativeChatToolRunOutcome( + [call('a', 'completed'), result('ok'), call('b', 'failed'), result('exit 1', true)], + {} + ).succeeded + ).toBe(false) + }) +}) diff --git a/src/shared/native-chat-tool-run-outcome.ts b/src/shared/native-chat-tool-run-outcome.ts new file mode 100644 index 00000000000..2f255b09b8f --- /dev/null +++ b/src/shared/native-chat-tool-run-outcome.ts @@ -0,0 +1,50 @@ +// A run of tool calls → the two facts its collapsed header may state: whether +// the run succeeded, and how many of its calls did not. +// +// Shared, and separate from the live-activity derivation, because success is a +// claim the header makes on its own. "Nothing is running" is not that claim: +// `failed` is neither running nor a success, so a header that reads one off the +// other marks a failed run done and leaves the failure to be found by expanding +// it. Success must be stated, which is what `nativeChatToolRunOutcome` does. + +import { selectActiveToolCall } from './native-chat-tool-activity' +import type { NativeChatBlock } from './native-chat-types' + +export type NativeChatToolRunOutcome = { + failedCallCount: number + succeeded: boolean +} + +/** Whether the run may be marked done: settled, nothing failed, nothing still + * running. The running test is repeated after `selectActiveToolCall` on + * purpose — that one reports no active call once the turn is known to be over, + * and an item still running cannot inherit completion from its turn. + * + * A call carrying no lifecycle `state` is not a failure and not in flight, so a + * legacy transcript still settles; nothing here demands an explicit `completed` + * that those lanes never wrote. */ +export function nativeChatToolRunOutcome( + blocks: readonly NativeChatBlock[], + { activeTurnIsWorking }: { activeTurnIsWorking?: boolean } +): NativeChatToolRunOutcome { + let failedStateCount = 0 + let errorResultCount = 0 + let hasRunningCall = false + for (const block of blocks) { + if (block.type === 'tool-call') { + failedStateCount += block.state === 'failed' ? 1 : 0 + hasRunningCall ||= block.state === 'running' + } else if (block.type === 'tool-result') { + errorResultCount += block.isError === true ? 1 : 0 + } + } + // Structured lanes carry both signals for one failure; legacy lanes carry only the result. + const failedCallCount = Math.max(failedStateCount, errorResultCount) + return { + failedCallCount, + succeeded: + selectActiveToolCall(blocks, { activeTurnIsWorking }) === null && + !hasRunningCall && + failedCallCount === 0 + } +}