diff --git a/src/services/partialService.test.ts b/src/services/partialService.test.ts new file mode 100644 index 000000000..a216c2982 --- /dev/null +++ b/src/services/partialService.test.ts @@ -0,0 +1,196 @@ +/* eslint-disable @typescript-eslint/unbound-method */ +import { describe, test, expect, beforeEach, mock } from "bun:test"; +import { PartialService } from "./partialService"; +import type { HistoryService } from "./historyService"; +import type { Config } from "@/config"; +import type { CmuxMessage } from "@/types/message"; +import { Ok } from "@/types/result"; + +// Mock Config +const createMockConfig = (): Config => { + return { + getSessionDir: mock((workspaceId: string) => `/tmp/test-sessions/${workspaceId}`), + } as unknown as Config; +}; + +// Mock HistoryService +const createMockHistoryService = (): HistoryService => { + return { + appendToHistory: mock(() => Promise.resolve(Ok(undefined))), + getHistory: mock(() => Promise.resolve(Ok([]))), + updateHistory: mock(() => Promise.resolve(Ok(undefined))), + truncateAfterMessage: mock(() => Promise.resolve(Ok(undefined))), + clearHistory: mock(() => Promise.resolve(Ok(undefined))), + } as unknown as HistoryService; +}; + +describe("PartialService - Error Recovery", () => { + let partialService: PartialService; + let mockConfig: Config; + let mockHistoryService: HistoryService; + + beforeEach(() => { + mockConfig = createMockConfig(); + mockHistoryService = createMockHistoryService(); + partialService = new PartialService(mockConfig, mockHistoryService); + }); + + test("commitToHistory should strip error metadata and commit parts from errored partial", async () => { + const workspaceId = "test-workspace"; + const erroredPartial: CmuxMessage = { + id: "msg-1", + role: "assistant", + metadata: { + historySequence: 1, + timestamp: Date.now(), + model: "test-model", + partial: true, + error: "Stream error occurred", + errorType: "network", + }, + parts: [ + { type: "text", text: "Hello, I was processing when" }, + { type: "text", text: " the error occurred" }, + ], + }; + + // Mock readPartial to return errored partial + partialService.readPartial = mock(() => Promise.resolve(erroredPartial)); + + // Mock deletePartial + partialService.deletePartial = mock(() => Promise.resolve(Ok(undefined))); + + // Mock getHistory to return no existing messages + mockHistoryService.getHistory = mock(() => Promise.resolve(Ok([]))); + + // Call commitToHistory + const result = await partialService.commitToHistory(workspaceId); + + // Should succeed + expect(result.success).toBe(true); + + // Should have called appendToHistory with cleaned metadata (no error/errorType) + const appendToHistory = mockHistoryService.appendToHistory as ReturnType; + expect(appendToHistory).toHaveBeenCalledTimes(1); + const appendedMessage = appendToHistory.mock.calls[0][1] as CmuxMessage; + + expect(appendedMessage.id).toBe("msg-1"); + expect(appendedMessage.parts).toEqual(erroredPartial.parts); + expect(appendedMessage.metadata?.error).toBeUndefined(); + expect(appendedMessage.metadata?.errorType).toBeUndefined(); + expect(appendedMessage.metadata?.historySequence).toBe(1); + + // Should have deleted the partial after committing + const deletePartial = partialService.deletePartial as ReturnType; + expect(deletePartial).toHaveBeenCalledWith(workspaceId); + }); + + test("commitToHistory should update existing placeholder when errored partial has more parts", async () => { + const workspaceId = "test-workspace"; + const erroredPartial: CmuxMessage = { + id: "msg-1", + role: "assistant", + metadata: { + historySequence: 1, + timestamp: Date.now(), + model: "test-model", + partial: true, + error: "Stream error occurred", + errorType: "network", + }, + parts: [ + { type: "text", text: "Accumulated content before error" }, + { + type: "dynamic-tool", + toolCallId: "call-1", + toolName: "bash", + state: "input-available", + input: { script: "echo test" }, + }, + ], + }; + + const existingPlaceholder: CmuxMessage = { + id: "msg-1", + role: "assistant", + metadata: { + historySequence: 1, + timestamp: Date.now(), + model: "test-model", + partial: true, + }, + parts: [], // Empty placeholder + }; + + // Mock readPartial to return errored partial + partialService.readPartial = mock(() => Promise.resolve(erroredPartial)); + + // Mock deletePartial + partialService.deletePartial = mock(() => Promise.resolve(Ok(undefined))); + + // Mock getHistory to return existing placeholder + mockHistoryService.getHistory = mock(() => Promise.resolve(Ok([existingPlaceholder]))); + + // Call commitToHistory + const result = await partialService.commitToHistory(workspaceId); + + // Should succeed + expect(result.success).toBe(true); + + // Should have called updateHistory (not append) with cleaned metadata + const updateHistory = mockHistoryService.updateHistory as ReturnType; + const appendToHistory = mockHistoryService.appendToHistory as ReturnType; + expect(updateHistory).toHaveBeenCalledTimes(1); + expect(appendToHistory).not.toHaveBeenCalled(); + + const updatedMessage = updateHistory.mock.calls[0][1] as CmuxMessage; + + expect(updatedMessage.parts).toEqual(erroredPartial.parts); + expect(updatedMessage.metadata?.error).toBeUndefined(); + expect(updatedMessage.metadata?.errorType).toBeUndefined(); + + // Should have deleted the partial after updating + const deletePartial = partialService.deletePartial as ReturnType; + expect(deletePartial).toHaveBeenCalledWith(workspaceId); + }); + + test("commitToHistory should skip empty errored partial", async () => { + const workspaceId = "test-workspace"; + const emptyErrorPartial: CmuxMessage = { + id: "msg-1", + role: "assistant", + metadata: { + historySequence: 1, + timestamp: Date.now(), + model: "test-model", + partial: true, + error: "Network error", + errorType: "network", + }, + parts: [], // Empty - no content accumulated before error + }; + + // Mock readPartial to return empty errored partial + partialService.readPartial = mock(() => Promise.resolve(emptyErrorPartial)); + + // Mock deletePartial + partialService.deletePartial = mock(() => Promise.resolve(Ok(undefined))); + + // Mock getHistory to return no existing messages + mockHistoryService.getHistory = mock(() => Promise.resolve(Ok([]))); + + // Call commitToHistory + const result = await partialService.commitToHistory(workspaceId); + + // Should succeed + expect(result.success).toBe(true); + + // Should NOT call appendToHistory for empty message (no value to preserve) + const appendToHistory = mockHistoryService.appendToHistory as ReturnType; + expect(appendToHistory).not.toHaveBeenCalled(); + + // Should still delete the partial (cleanup) + const deletePartial = partialService.deletePartial as ReturnType; + expect(deletePartial).toHaveBeenCalledWith(workspaceId); + }); +}); diff --git a/src/services/partialService.ts b/src/services/partialService.ts index e1909c05f..f293825e3 100644 --- a/src/services/partialService.ts +++ b/src/services/partialService.ts @@ -119,16 +119,17 @@ export class PartialService { */ async commitToHistory(workspaceId: string): Promise> { try { - const partial = await this.readPartial(workspaceId); + let partial = await this.readPartial(workspaceId); if (!partial) { return Ok(undefined); // No partial to commit } - // Don't commit errored partials to chat.jsonl - // Errored messages are transient failures, not committed history - // This prevents error accumulation when editing messages multiple times + // Strip error metadata if present, but commit the accumulated parts + // Error metadata is transient (UI-only), accumulated parts are persisted + // This ensures resumptions don't lose progress from failed streams if (partial.metadata?.error) { - return await this.deletePartial(workspaceId); + const { error, errorType, ...cleanMetadata } = partial.metadata; + partial = { ...partial, metadata: cleanMetadata }; } const partialSeq = partial.metadata?.historySequence; @@ -150,8 +151,9 @@ export class PartialService { ); const shouldCommit = - !existingMessage || // No message with this sequence yet - (partial.parts?.length ?? 0) > (existingMessage.parts?.length ?? 0); // Partial has more parts + (!existingMessage || // No message with this sequence yet + (partial.parts?.length ?? 0) > (existingMessage.parts?.length ?? 0)) && // Partial has more parts + (partial.parts?.length ?? 0) > 0; // Don't commit empty messages if (shouldCommit) { if (existingMessage) {