fix(cli): Fail dangling replayed tool calls (#5624)

This commit is contained in:
jinye 2026-06-22 20:04:33 +08:00 committed by GitHub
parent 580a72410f
commit bf70079137
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
3 changed files with 576 additions and 61 deletions

View file

@ -5,7 +5,10 @@
*/
import { describe, it, expect, vi, beforeEach } from 'vitest';
import { HistoryReplayer } from './HistoryReplayer.js';
import {
HistoryReplayer,
MISSING_TOOL_RESULT_MESSAGE,
} from './HistoryReplayer.js';
import type { SessionContext } from './types.js';
import type {
Config,
@ -18,10 +21,22 @@ import type {
describe('HistoryReplayer', () => {
let mockContext: SessionContext;
let sendUpdateSpy: ReturnType<typeof vi.fn>;
let setActiveRecordIdSpy: ReturnType<typeof vi.fn>;
let sentUpdateContexts: Array<{
activeRecordId: string | null;
activeRecordTimestamp: string | undefined;
}>;
let replayer: HistoryReplayer;
beforeEach(() => {
let activeRecordId: string | null = null;
let activeRecordTimestamp: string | undefined;
sentUpdateContexts = [];
sendUpdateSpy = vi.fn().mockResolvedValue(undefined);
setActiveRecordIdSpy = vi.fn((id: string | null, timestamp?: string) => {
activeRecordId = id;
activeRecordTimestamp = timestamp;
});
const mockToolRegistry = {
getTool: vi.fn().mockReturnValue(null),
} as unknown as ToolRegistry;
@ -31,13 +46,21 @@ describe('HistoryReplayer', () => {
config: {
getToolRegistry: () => mockToolRegistry,
} as unknown as Config,
sendUpdate: sendUpdateSpy,
};
sendUpdate: vi.fn(async (update) => {
sentUpdateContexts.push({ activeRecordId, activeRecordTimestamp });
await sendUpdateSpy(update);
}),
setActiveRecordId: setActiveRecordIdSpy,
} as unknown as SessionContext;
replayer = new HistoryReplayer(mockContext);
});
const toEpochMs = (ts: string) => new Date(ts).getTime();
const sentUpdates = () =>
sendUpdateSpy.mock.calls.map(
(call: unknown[]) => call[0] as Record<string, unknown>,
);
const createUserRecord = (text: string): ChatRecord => ({
uuid: 'user-uuid',
@ -268,6 +291,7 @@ describe('HistoryReplayer', () => {
},
}),
);
expect(sendUpdateSpy).toHaveBeenCalledTimes(1);
});
it('should use function call id as callId when available', async () => {
@ -295,6 +319,334 @@ describe('HistoryReplayer', () => {
}),
);
});
it('should fail dangling function calls after replay completes', async () => {
const record: ChatRecord = {
...createAssistantRecord(''),
message: {
role: 'model',
parts: [
{
functionCall: {
id: 'call-missing',
name: 'run_shell_command',
args: { command: 'sleep 10' },
},
},
],
},
};
await replayer.replay([record]);
const updates = sentUpdates();
expect(updates.map((update) => update['sessionUpdate'])).toEqual([
'tool_call',
'tool_call_update',
]);
expect(updates[1]).toMatchObject({
sessionUpdate: 'tool_call_update',
toolCallId: 'call-missing',
status: 'failed',
content: [
{
type: 'content',
content: {
type: 'text',
text: MISSING_TOOL_RESULT_MESSAGE,
},
},
],
_meta: {
toolName: 'run_shell_command',
provenance: 'builtin',
timestamp: toEpochMs(record.timestamp),
},
});
expect(setActiveRecordIdSpy).toHaveBeenCalledWith(
record.uuid,
record.timestamp,
);
expect(sentUpdateContexts[1]).toEqual({
activeRecordId: record.uuid,
activeRecordTimestamp: record.timestamp,
});
});
it('should not synthesize missing-result failures for calls without source ids', async () => {
const records: ChatRecord[] = [
{
...createAssistantRecord(''),
message: {
role: 'model',
parts: [
{
functionCall: {
name: 'read_file',
args: { path: 'test.ts' },
},
},
],
},
},
createToolResultRecord('read_file', 'File contents here'),
];
await replayer.replay(records);
const updates = sentUpdates();
expect(updates.map((update) => update['sessionUpdate'])).toEqual([
'tool_call',
'tool_call_update',
]);
expect(updates[1]).toMatchObject({
toolCallId: 'call-123',
status: 'completed',
});
});
it('should fail dangling calls before rethrowing replay errors', async () => {
const danglingRecord: ChatRecord = {
...createAssistantRecord(''),
message: {
role: 'model',
parts: [
{
functionCall: {
id: 'call-missing',
name: 'run_shell_command',
args: { command: 'sleep 10' },
},
},
],
},
};
const failingRecord = createUserRecord('this send fails');
sendUpdateSpy.mockImplementation(
async (update: Record<string, unknown>) => {
if (update['sessionUpdate'] === 'user_message_chunk') {
throw new Error('replay failed');
}
},
);
await expect(
replayer.replay([danglingRecord, failingRecord]),
).rejects.toThrow('replay failed');
const updates = sentUpdates();
expect(updates.map((update) => update['sessionUpdate'])).toEqual([
'tool_call',
'user_message_chunk',
'tool_call_update',
]);
expect(updates[2]).toMatchObject({
toolCallId: 'call-missing',
status: 'failed',
});
});
it('should throw dangling errors and continue failing later dangling calls', async () => {
const record: ChatRecord = {
...createAssistantRecord(''),
message: {
role: 'model',
parts: [
{
functionCall: {
id: 'call-a',
name: 'read_file',
args: { path: 'a.ts' },
},
},
{
functionCall: {
id: 'call-b',
name: 'read_file',
args: { path: 'b.ts' },
},
},
],
},
};
sendUpdateSpy.mockImplementation(
async (update: Record<string, unknown>) => {
if (
update['sessionUpdate'] === 'tool_call_update' &&
update['toolCallId'] === 'call-a'
) {
throw new Error('first synthetic failure failed');
}
},
);
await expect(replayer.replay([record])).rejects.toThrow(
'first synthetic failure failed',
);
const updates = sentUpdates();
expect(updates.map((update) => update['sessionUpdate'])).toEqual([
'tool_call',
'tool_call',
'tool_call_update',
'tool_call_update',
]);
expect(updates[2]).toMatchObject({
toolCallId: 'call-a',
status: 'failed',
});
expect(updates[3]).toMatchObject({
toolCallId: 'call-b',
status: 'failed',
});
});
it('should aggregate replay and dangling cleanup errors', async () => {
const danglingRecord: ChatRecord = {
...createAssistantRecord(''),
message: {
role: 'model',
parts: [
{
functionCall: {
id: 'call-missing',
name: 'run_shell_command',
args: { command: 'sleep 10' },
},
},
],
},
};
const failingRecord = createUserRecord('this send fails');
sendUpdateSpy.mockImplementation(
async (update: Record<string, unknown>) => {
if (update['sessionUpdate'] === 'user_message_chunk') {
throw new Error('replay failed');
}
if (update['sessionUpdate'] === 'tool_call_update') {
throw new Error('dangling cleanup failed');
}
},
);
let caughtError: unknown;
try {
await replayer.replay([danglingRecord, failingRecord]);
} catch (error) {
caughtError = error;
}
expect(caughtError).toBeInstanceOf(AggregateError);
expect((caughtError as AggregateError).message).toBe(
'Replay and dangling-cleanup both failed',
);
expect(
(caughtError as AggregateError).errors.map((error) =>
error instanceof Error ? error.message : String(error),
),
).toEqual(['replay failed', 'dangling cleanup failed']);
});
it('should not fail function calls that have matching tool results', async () => {
const records: ChatRecord[] = [
{
...createAssistantRecord(''),
message: {
role: 'model',
parts: [
{
functionCall: {
id: 'call-123',
name: 'read_file',
args: { path: 'test.ts' },
},
},
],
},
},
createToolResultRecord('read_file', 'File contents here'),
];
await replayer.replay(records);
const updates = sentUpdates();
expect(updates.map((update) => update['sessionUpdate'])).toEqual([
'tool_call',
'tool_call_update',
]);
expect(updates[1]).toMatchObject({
toolCallId: 'call-123',
status: 'completed',
});
});
it('should only fail dangling calls when matched and dangling calls are mixed', async () => {
const records: ChatRecord[] = [
{
...createAssistantRecord(''),
message: {
role: 'model',
parts: [
{
functionCall: {
id: 'call-123',
name: 'read_file',
args: { path: 'test.ts' },
},
},
{
functionCall: {
id: 'call-missing',
name: 'run_shell_command',
args: { command: 'sleep 10' },
},
},
],
},
},
createToolResultRecord('read_file', 'File contents here'),
];
await replayer.replay(records);
const updates = sentUpdates();
expect(updates.map((update) => update['sessionUpdate'])).toEqual([
'tool_call',
'tool_call',
'tool_call_update',
'tool_call_update',
]);
expect(updates[2]).toMatchObject({
toolCallId: 'call-123',
status: 'completed',
});
expect(updates[3]).toMatchObject({
toolCallId: 'call-missing',
status: 'failed',
});
});
it('should not track skipped TodoWrite starts as dangling tool calls', async () => {
const record: ChatRecord = {
...createAssistantRecord(''),
message: {
role: 'model',
parts: [
{
functionCall: {
id: 'todo-call',
name: 'todo_write',
args: { todos: [] },
},
},
],
},
};
await replayer.replay([record]);
expect(sendUpdateSpy).not.toHaveBeenCalled();
});
});
describe('tool result replay', () => {
@ -393,6 +745,40 @@ describe('HistoryReplayer', () => {
}),
);
});
it('should use functionResponse id as callId when toolCallResult.callId is missing', async () => {
const record: ChatRecord = {
...createToolResultRecord('test_tool'),
uuid: 'fallback-uuid',
message: {
role: 'user',
parts: [
{
functionResponse: {
id: 'response-call-id',
name: 'test_tool',
response: { result: 'ok' },
},
},
],
},
toolCallResult: {
callId: undefined as unknown as string,
responseParts: [],
resultDisplay: 'Result',
error: undefined,
errorType: undefined,
},
};
await replayer.replay([record]);
expect(sendUpdateSpy).toHaveBeenCalledWith(
expect.objectContaining({
toolCallId: 'response-call-id',
}),
);
});
});
describe('system records', () => {
@ -426,7 +812,7 @@ describe('HistoryReplayer', () => {
{ text: "I'll read that file for you.", thought: true },
{
functionCall: {
id: 'call-read',
id: 'call-123',
name: 'read_file',
args: { path: 'test.ts' },
},

View file

@ -18,6 +18,17 @@ import type { SessionContext } from './types.js';
import { MessageEmitter } from './emitters/MessageEmitter.js';
import { ToolCallEmitter } from './emitters/ToolCallEmitter.js';
export const MISSING_TOOL_RESULT_MESSAGE =
'Tool result missing from saved history; the previous run likely ended ' +
'before this tool completed.';
interface PendingReplayToolCall {
callId: string;
toolName: string;
timestamp?: string;
recordId: string;
}
/**
* Handles replaying session history on session load.
*
@ -29,6 +40,10 @@ export class HistoryReplayer {
private readonly ctx: SessionContext;
private readonly messageEmitter: MessageEmitter;
private readonly toolCallEmitter: ToolCallEmitter;
private readonly pendingReplayToolCalls = new Map<
string,
PendingReplayToolCall
>();
constructor(ctx: SessionContext) {
this.ctx = ctx;
@ -42,8 +57,39 @@ export class HistoryReplayer {
* @param records - Array of chat records to replay
*/
async replay(records: ChatRecord[]): Promise<void> {
for (const record of records) {
await this.replayRecord(record);
this.pendingReplayToolCalls.clear();
try {
let replayError: unknown;
try {
for (const record of records) {
await this.replayRecord(record);
}
} catch (error) {
replayError = error;
}
let danglingError: unknown;
try {
await this.failDanglingToolCalls();
} catch (error) {
danglingError = error;
}
if (replayError && danglingError) {
throw new AggregateError(
[replayError, danglingError],
'Replay and dangling-cleanup both failed',
);
}
if (replayError) {
throw replayError;
}
if (danglingError) {
throw danglingError;
}
} finally {
this.pendingReplayToolCalls.clear();
this.setActiveRecordId(null);
}
}
@ -52,70 +98,84 @@ export class HistoryReplayer {
*/
private async replayRecord(record: ChatRecord): Promise<void> {
this.setActiveRecordId(record.uuid, record.timestamp);
switch (record.type) {
case 'user':
// Notification/cron records hold raw XML/prompt the user never
// typed; replay the friendly displayText so the assistant's reply
// has an antecedent in the ACP transcript.
if (record.subtype === 'notification' || record.subtype === 'cron') {
const displayText = (
record.systemPayload as NotificationRecordPayload | undefined
)?.displayText;
if (displayText) {
await this.messageEmitter.emitUserMessage(
displayText,
try {
switch (record.type) {
case 'user':
// Notification/cron records hold raw XML/prompt the user never
// typed; replay the friendly displayText so the assistant's reply
// has an antecedent in the ACP transcript.
if (record.subtype === 'notification' || record.subtype === 'cron') {
const displayText = (
record.systemPayload as NotificationRecordPayload | undefined
)?.displayText;
if (displayText) {
await this.messageEmitter.emitUserMessage(
displayText,
record.timestamp,
);
}
break;
}
if (record.subtype === 'mid_turn_user_message') {
const displayText = (
record.systemPayload as NotificationRecordPayload | undefined
)?.displayText;
if (displayText) {
await this.messageEmitter.emitUserMessage(
displayText,
record.timestamp,
);
} else if (record.message) {
await this.replayContent(
record.message,
'user',
record.timestamp,
record.uuid,
);
}
break;
}
if (record.message) {
await this.replayContent(
record.message,
'user',
record.timestamp,
record.uuid,
);
}
break;
}
if (record.subtype === 'mid_turn_user_message') {
const displayText = (
record.systemPayload as NotificationRecordPayload | undefined
)?.displayText;
if (displayText) {
await this.messageEmitter.emitUserMessage(
displayText,
case 'assistant':
if (record.message) {
await this.replayContent(
record.message,
'assistant',
record.timestamp,
record.uuid,
);
} else if (record.message) {
await this.replayContent(record.message, 'user', record.timestamp);
}
if (record.usageMetadata) {
await this.replayUsageMetadata(record.usageMetadata);
}
break;
}
if (record.message) {
await this.replayContent(record.message, 'user', record.timestamp);
}
break;
case 'assistant':
if (record.message) {
await this.replayContent(
record.message,
'assistant',
record.timestamp,
);
}
if (record.usageMetadata) {
await this.replayUsageMetadata(record.usageMetadata);
}
break;
case 'tool_result':
await this.replayToolResult(record);
break;
case 'tool_result':
await this.replayToolResult(record);
break;
case 'system':
if (record.subtype === 'slash_command') {
await this.replaySlashCommandResult(record);
}
// Other system subtypes (compression, telemetry, at_command) are skipped.
break;
case 'system':
if (record.subtype === 'slash_command') {
await this.replaySlashCommandResult(record);
}
// Other system subtypes (compression, telemetry, at_command) are skipped.
break;
default:
break;
default:
break;
}
} finally {
this.setActiveRecordId(null);
}
this.setActiveRecordId(null);
}
/**
@ -130,6 +190,7 @@ export class HistoryReplayer {
content: Content,
role: 'user' | 'assistant',
timestamp?: string,
recordId?: string,
): Promise<void> {
for (const part of content.parts ?? []) {
// Text content
@ -146,15 +207,25 @@ export class HistoryReplayer {
// Function call (tool start)
if ('functionCall' in part && part.functionCall) {
const functionName = part.functionCall.name ?? '';
const callId = part.functionCall.id ?? `${functionName}-${Date.now()}`;
const sourceCallId = part.functionCall.id;
const callId = sourceCallId ?? `${functionName}-${Date.now()}`;
await this.toolCallEmitter.emitStart({
const emitted = await this.toolCallEmitter.emitStart({
toolName: functionName,
callId,
args: part.functionCall.args as Record<string, unknown>,
status: 'in_progress',
timestamp,
});
if (emitted && role === 'assistant' && recordId && sourceCallId) {
this.pendingReplayToolCalls.set(callId, {
callId,
toolName: functionName,
timestamp,
recordId,
});
}
}
}
}
@ -179,7 +250,8 @@ export class HistoryReplayer {
}
const result = record.toolCallResult;
const callId = result?.callId ?? record.uuid;
const callId = this.getToolResultCallId(record);
this.pendingReplayToolCalls.delete(callId);
// Extract tool name from the function response in message if available
const toolName = this.extractToolNameFromRecord(record);
@ -210,6 +282,30 @@ export class HistoryReplayer {
}
}
private async failDanglingToolCalls(): Promise<void> {
let firstError: unknown;
for (const pending of this.pendingReplayToolCalls.values()) {
this.setActiveRecordId(pending.recordId, pending.timestamp);
try {
await this.toolCallEmitter.emitResult({
toolName: pending.toolName,
callId: pending.callId,
success: false,
message: [],
error: new Error(MISSING_TOOL_RESULT_MESSAGE),
timestamp: pending.timestamp,
});
} catch (error) {
firstError ??= error;
} finally {
this.setActiveRecordId(null);
}
}
if (firstError) {
throw firstError;
}
}
/**
* Emits token usage from a AgentResultDisplay execution summary, if present.
*/
@ -283,6 +379,29 @@ export class HistoryReplayer {
return '';
}
private getToolResultCallId(record: ChatRecord): string {
const resultCallId = record.toolCallResult?.callId;
if (typeof resultCallId === 'string' && resultCallId.length > 0) {
return resultCallId;
}
return this.extractFunctionResponseIdFromRecord(record) ?? record.uuid;
}
private extractFunctionResponseIdFromRecord(
record: ChatRecord,
): string | undefined {
if (record.message?.parts) {
for (const part of record.message.parts) {
const id =
'functionResponse' in part ? part.functionResponse?.id : undefined;
if (typeof id === 'string' && id.length > 0) {
return id;
}
}
}
return undefined;
}
private setActiveRecordId(recordId: string | null, timestamp?: string): void {
const context = this.ctx as unknown as {
setActiveRecordId?: (id: string | null, timestamp?: string) => void;

View file

@ -221,6 +221,16 @@ describe('daemon selectors', () => {
block({ kind: 'tool', status: 'in_progress' }),
]),
).toBe('responding');
expect(
selectDaemonTranscriptStreamingState([
block({ kind: 'tool', status: 'failed' }),
]),
).toBe('idle');
expect(
selectDaemonTranscriptStreamingState([
block({ kind: 'tool', status: 'completed' }),
]),
).toBe('idle');
});
it('falls back to prompt status when transcript is idle', () => {