fix(native-chat): stop a collapsed run claiming success when a tool call failed (#21151)

* 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
This commit is contained in:
Brennan Benson
2026-09-16 22:58:44 -07:00
committed by GitHub
parent 69787e763a
commit 68ea3b92e3
7 changed files with 231 additions and 8 deletions
@@ -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(<NativeChatToolRun blocks={blocks} expandSignal={false} />)
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(<NativeChatToolRun blocks={blocks} expandSignal={false} />)
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(<NativeChatToolRun blocks={blocks} expandSignal={false} />)
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' },
@@ -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}
</span>
)}
{/* 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. */
<span
aria-label={translate(
'components.native-chat.tool.failedCallsLabel',
NATIVE_CHAT_TOOL_ACTIVITY_COPY.failedCallsLabel,
{ value0: failedCallCount }
)}
className="shrink-0 font-mono text-[11px] text-muted-foreground transition-colors group-hover:text-foreground/80"
>
{translate(
'components.native-chat.tool.failedCount',
NATIVE_CHAT_TOOL_ACTIVITY_COPY.failedCount,
{ value0: failedCallCount }
)}
</span>
) : null}
{/* Only a stated success is marked done — see nativeChatToolRunOutcome. */}
{structuredActivityUi && runSucceeded ? (
<Check aria-hidden className="size-3 shrink-0 text-muted-foreground" />
) : null}
{/* Chevron is revealed on hover when collapsed and points down when open. */}
+2
View File
@@ -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",
+2
View File
@@ -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}}",
+6 -1
View File
@@ -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
@@ -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 runs 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)
})
})
@@ -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
}
}