diff --git a/packages/cli/src/acp-integration/session/HistoryReplayer.test.ts b/packages/cli/src/acp-integration/session/HistoryReplayer.test.ts index cf15142437..c00b239578 100644 --- a/packages/cli/src/acp-integration/session/HistoryReplayer.test.ts +++ b/packages/cli/src/acp-integration/session/HistoryReplayer.test.ts @@ -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; + let setActiveRecordIdSpy: ReturnType; + 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, + ); 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) => { + 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) => { + 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) => { + 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' }, }, diff --git a/packages/cli/src/acp-integration/session/HistoryReplayer.ts b/packages/cli/src/acp-integration/session/HistoryReplayer.ts index ab4bba1d77..c64625f283 100644 --- a/packages/cli/src/acp-integration/session/HistoryReplayer.ts +++ b/packages/cli/src/acp-integration/session/HistoryReplayer.ts @@ -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 { - 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 { 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 { 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, 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 { + 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; diff --git a/packages/webui/src/daemon/session/selectors.test.ts b/packages/webui/src/daemon/session/selectors.test.ts index bc3a5728e6..9eb570e51c 100644 --- a/packages/webui/src/daemon/session/selectors.test.ts +++ b/packages/webui/src/daemon/session/selectors.test.ts @@ -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', () => {