diff --git a/src/core/task-persistence/apiMessages.ts b/src/core/task-persistence/apiMessages.ts index f846aaf13f..204eb71599 100644 --- a/src/core/task-persistence/apiMessages.ts +++ b/src/core/task-persistence/apiMessages.ts @@ -81,3 +81,46 @@ export async function saveApiMessages({ const filePath = path.join(taskDir, GlobalFileNames.apiConversationHistory) await safeWriteJson(filePath, messages) } + +/** + * Transaction helper for safe read-modify-write operations on API messages. + * Ensures atomic updates by reading, modifying, and writing under a conceptual lock. + * + * @param taskId - The task ID + * @param globalStoragePath - The global storage path + * @param updater - A pure function that takes the current messages and returns the updated messages + * @param options - Optional configuration + * @returns The updated messages + */ +export async function transactApiMessages({ + taskId, + globalStoragePath, + updater, + options = {}, +}: { + taskId: string + globalStoragePath: string + updater: (messages: ApiMessage[]) => ApiMessage[] + options?: { + allowEmpty?: boolean + } +}): Promise { + // Read current state + const currentMessages = await readApiMessages({ taskId, globalStoragePath }) + + // Apply the pure updater function + const updatedMessages = updater(currentMessages) + + // Guard against unintentional empty writes + if (updatedMessages.length === 0 && currentMessages.length > 0 && !options.allowEmpty) { + console.warn( + `[transactApiMessages] Preventing empty write for taskId: ${taskId}. Current has ${currentMessages.length} messages. Use allowEmpty: true to force.`, + ) + return currentMessages // Return unchanged + } + + // Commit the changes + await saveApiMessages({ messages: updatedMessages, taskId, globalStoragePath }) + + return updatedMessages +} diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index cf16df8dcc..4ecde0b2e8 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -582,7 +582,16 @@ export class Task extends EventEmitter implements TaskLike { await this.saveApiConversationHistory() } - async overwriteApiConversationHistory(newHistory: ApiMessage[]) { + async overwriteApiConversationHistory(newHistory: ApiMessage[], allowEmpty: boolean = false) { + // Guard against unintentional empty writes + if (newHistory.length === 0 && this.apiConversationHistory.length > 0 && !allowEmpty) { + console.warn( + `[Task#overwriteApiConversationHistory] Preventing empty write for taskId: ${this.taskId}. ` + + `Current has ${this.apiConversationHistory.length} messages. Use allowEmpty: true to force.`, + ) + return // Don't overwrite with empty array unless explicitly allowed + } + this.apiConversationHistory = newHistory await this.saveApiConversationHistory() } @@ -1436,7 +1445,15 @@ export class Task extends EventEmitter implements TaskLike { throw new Error("Unexpected: Last message is not a user or assistant message") } } else { - throw new Error("Unexpected: No existing API conversation history") + // Handle empty API conversation history gracefully instead of throwing + // This prevents the "chat locks until reopen" failure mode + this.say( + "text", + "[TASK RESUMPTION] Previous conversation history was empty. Starting with a fresh baseline.", + ) + // Initialize with empty arrays to allow the task to continue + modifiedApiConversationHistory = [] + modifiedOldUserContent = [] } let newUserContent: Anthropic.Messages.ContentBlockParam[] = [...modifiedOldUserContent] diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 9abddc6d96..0d32baaaf2 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -918,8 +918,10 @@ export class ClineProvider await task.overwriteClineMessages(task.clineMessages.slice(0, messageIndex)) if (apiConversationHistoryIndex !== -1) { + // Allow empty writes for edit operations after checkpoint restoration await task.overwriteApiConversationHistory( task.apiConversationHistory.slice(0, apiConversationHistoryIndex), + true, // allowEmpty: true for edit operations ) } @@ -1453,6 +1455,7 @@ export class ClineProvider if (historyItem) { const { getTaskDirectoryPath } = await import("../../utils/storage") + const { readApiMessages } = await import("../task-persistence/apiMessages") const globalStoragePath = this.contextProxy.globalStorageUri.fsPath const taskDirPath = await getTaskDirectoryPath(globalStoragePath, id) const apiConversationHistoryFilePath = path.join(taskDirPath, GlobalFileNames.apiConversationHistory) @@ -1460,7 +1463,8 @@ export class ClineProvider const fileExists = await fileExistsAtPath(apiConversationHistoryFilePath) if (fileExists) { - const apiConversationHistory = JSON.parse(await fs.readFile(apiConversationHistoryFilePath, "utf8")) + // Use the helper reader for unified behavior/logging instead of direct JSON.parse + const apiConversationHistory = await readApiMessages({ taskId: id, globalStoragePath }) return { historyItem, diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index bcc9d544c2..0ec064fbf6 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -1239,11 +1239,10 @@ describe("ClineProvider", () => { ]) // Verify only API messages before the deleted message were kept - expect(mockCline.overwriteApiConversationHistory).toHaveBeenCalledWith([ - mockApiHistory[0], - mockApiHistory[1], - mockApiHistory[2], - ]) + expect(mockCline.overwriteApiConversationHistory).toHaveBeenCalledWith( + [mockApiHistory[0], mockApiHistory[1], mockApiHistory[2]], + true, + ) // createTaskWithHistoryItem is only called when restoring checkpoints or aborting tasks expect((provider as any).createTaskWithHistoryItem).not.toHaveBeenCalled() @@ -1339,7 +1338,7 @@ describe("ClineProvider", () => { expect(mockCline.overwriteClineMessages).toHaveBeenCalledWith([]) // Verify correct API messages were kept - expect(mockCline.overwriteApiConversationHistory).toHaveBeenCalledWith([]) + expect(mockCline.overwriteApiConversationHistory).toHaveBeenCalledWith([], true) // The new flow calls webviewMessageHandler recursively with askResponse // We need to verify the recursive call happened by checking if the handler was called again @@ -3049,7 +3048,7 @@ describe("ClineProvider - Comprehensive Edit/Delete Edge Cases", () => { // Verify messages were edited correctly - the ORIGINAL user message and all subsequent messages are removed expect(mockCline.overwriteClineMessages).toHaveBeenCalledWith([mockMessages[0]]) - expect(mockCline.overwriteApiConversationHistory).toHaveBeenCalledWith([{ ts: 1000 }]) + expect(mockCline.overwriteApiConversationHistory).toHaveBeenCalledWith([{ ts: 1000 }], true) // Verify submitUserMessage was called with the edited content expect(mockCline.submitUserMessage).toHaveBeenCalledWith("Edited message with preserved images", undefined) }) @@ -3675,7 +3674,10 @@ describe("ClineProvider - Comprehensive Edit/Delete Edge Cases", () => { // Should handle large payloads without issues - keeps messages before the deleted one expect(mockCline.overwriteClineMessages).toHaveBeenCalledWith([mockMessages[0], mockMessages[1]]) - expect(mockCline.overwriteApiConversationHistory).toHaveBeenCalledWith([{ ts: 1000 }, { ts: 2000 }]) + expect(mockCline.overwriteApiConversationHistory).toHaveBeenCalledWith( + [{ ts: 1000 }, { ts: 2000 }], + true, + ) }) }) diff --git a/src/core/webview/__tests__/webviewMessageHandler.delete.spec.ts b/src/core/webview/__tests__/webviewMessageHandler.delete.spec.ts index 28f6ba9cf8..0f4d25b8a8 100644 --- a/src/core/webview/__tests__/webviewMessageHandler.delete.spec.ts +++ b/src/core/webview/__tests__/webviewMessageHandler.delete.spec.ts @@ -112,7 +112,7 @@ describe("webviewMessageHandler delete functionality", () => { // When message is not found in API history (index is -1), // API history should be truncated from the first API message at/after the deleted timestamp (fallback) - expect(getCurrentTaskMock.overwriteApiConversationHistory).toHaveBeenCalledWith([]) + expect(getCurrentTaskMock.overwriteApiConversationHistory).toHaveBeenCalledWith([], true) }) it("should handle deletion when exact apiConversationHistoryIndex is found", async () => { @@ -142,9 +142,10 @@ describe("webviewMessageHandler delete functionality", () => { { ts: 900, say: "user", text: "Previous message" }, ]) - expect(getCurrentTaskMock.overwriteApiConversationHistory).toHaveBeenCalledWith([ - { ts: 900, role: "user", content: { type: "text", text: "Previous message" } }, - ]) + expect(getCurrentTaskMock.overwriteApiConversationHistory).toHaveBeenCalledWith( + [{ ts: 900, role: "user", content: { type: "text", text: "Previous message" } }], + true, + ) }) it("should handle deletion when message not found in clineMessages", async () => { @@ -204,7 +205,7 @@ describe("webviewMessageHandler delete functionality", () => { expect(getCurrentTaskMock.overwriteClineMessages).toHaveBeenCalledWith([]) // API history should be truncated from first message at/after deleted timestamp (fallback) - expect(getCurrentTaskMock.overwriteApiConversationHistory).toHaveBeenCalledWith([]) + expect(getCurrentTaskMock.overwriteApiConversationHistory).toHaveBeenCalledWith([], true) }) it("should preserve messages before the deleted one", async () => { @@ -236,10 +237,13 @@ describe("webviewMessageHandler delete functionality", () => { ]) // API history should be truncated at the exact index - expect(getCurrentTaskMock.overwriteApiConversationHistory).toHaveBeenCalledWith([ - { ts: 1000, role: "user", content: { type: "text", text: "First message" } }, - { ts: 1500, role: "assistant", content: { type: "text", text: "First response" } }, - ]) + expect(getCurrentTaskMock.overwriteApiConversationHistory).toHaveBeenCalledWith( + [ + { ts: 1000, role: "user", content: { type: "text", text: "First message" } }, + { ts: 1500, role: "assistant", content: { type: "text", text: "First response" } }, + ], + true, + ) }) }) }) diff --git a/src/core/webview/__tests__/webviewMessageHandler.edit.spec.ts b/src/core/webview/__tests__/webviewMessageHandler.edit.spec.ts index d467f5cd92..63349b4b0b 100644 --- a/src/core/webview/__tests__/webviewMessageHandler.edit.spec.ts +++ b/src/core/webview/__tests__/webviewMessageHandler.edit.spec.ts @@ -135,7 +135,7 @@ describe("webviewMessageHandler - Edit Message with Timestamp Fallback", () => { ) // API history should be truncated from first message at/after edited timestamp (fallback) - expect(mockCurrentTask.overwriteApiConversationHistory).toHaveBeenCalledWith([]) + expect(mockCurrentTask.overwriteApiConversationHistory).toHaveBeenCalledWith([], true) }) it("should preserve messages before the edited message when message not in API history", async () => { @@ -197,13 +197,16 @@ describe("webviewMessageHandler - Edit Message with Timestamp Fallback", () => { ]) // API history should be truncated from the first API message at/after the edited timestamp (fallback) - expect(mockCurrentTask.overwriteApiConversationHistory).toHaveBeenCalledWith([ - { - ts: earlierMessageTs, - role: "user", - content: [{ type: "text", text: "Earlier message" }], - }, - ]) + expect(mockCurrentTask.overwriteApiConversationHistory).toHaveBeenCalledWith( + [ + { + ts: earlierMessageTs, + role: "user", + content: [{ type: "text", text: "Earlier message" }], + }, + ], + true, + ) }) it("should not use fallback when exact apiConversationHistoryIndex is found", async () => { @@ -248,7 +251,7 @@ describe("webviewMessageHandler - Edit Message with Timestamp Fallback", () => { // Both should be truncated at index 0 expect(mockCurrentTask.overwriteClineMessages).toHaveBeenCalledWith([]) - expect(mockCurrentTask.overwriteApiConversationHistory).toHaveBeenCalledWith([]) + expect(mockCurrentTask.overwriteApiConversationHistory).toHaveBeenCalledWith([], true) }) it("should handle case where no API messages match timestamp criteria", async () => { @@ -385,6 +388,6 @@ describe("webviewMessageHandler - Edit Message with Timestamp Fallback", () => { expect(mockCurrentTask.overwriteClineMessages).toHaveBeenCalledWith([]) // API history should be truncated from first message at/after edited timestamp (fallback) - expect(mockCurrentTask.overwriteApiConversationHistory).toHaveBeenCalledWith([]) + expect(mockCurrentTask.overwriteApiConversationHistory).toHaveBeenCalledWith([], true) }) }) diff --git a/src/core/webview/webviewMessageHandler.ts b/src/core/webview/webviewMessageHandler.ts index accb66f6e9..55e2ca1a88 100644 --- a/src/core/webview/webviewMessageHandler.ts +++ b/src/core/webview/webviewMessageHandler.ts @@ -108,8 +108,10 @@ export const webviewMessageHandler = async ( await currentCline.overwriteClineMessages(currentCline.clineMessages.slice(0, messageIndex)) if (apiConversationHistoryIndex !== -1) { + // Allow empty writes for edit/delete operations await currentCline.overwriteApiConversationHistory( currentCline.apiConversationHistory.slice(0, apiConversationHistoryIndex), + true, // allowEmpty: true for edit/delete operations ) } }