diff --git a/src/core/task-persistence/__tests__/apiMessages.spec.ts b/src/core/task-persistence/__tests__/apiMessages.spec.ts new file mode 100644 index 00000000000..aa725f47442 --- /dev/null +++ b/src/core/task-persistence/__tests__/apiMessages.spec.ts @@ -0,0 +1,86 @@ +// cd src && npx vitest run core/task-persistence/__tests__/apiMessages.spec.ts + +import * as os from "os" +import * as path from "path" +import * as fs from "fs/promises" + +import { readApiMessages } from "../apiMessages" + +let tmpBaseDir: string + +beforeEach(async () => { + tmpBaseDir = await fs.mkdtemp(path.join(os.tmpdir(), "roo-test-api-")) +}) + +describe("apiMessages.readApiMessages", () => { + it("returns empty array when api_conversation_history.json contains invalid JSON", async () => { + const taskId = "task-corrupt-api" + const taskDir = path.join(tmpBaseDir, "tasks", taskId) + await fs.mkdir(taskDir, { recursive: true }) + const filePath = path.join(taskDir, "api_conversation_history.json") + await fs.writeFile(filePath, "<<>>", "utf8") + + const result = await readApiMessages({ + taskId, + globalStoragePath: tmpBaseDir, + }) + + expect(result).toEqual([]) + }) + + it("returns empty array when claude_messages.json fallback contains invalid JSON", async () => { + const taskId = "task-corrupt-fallback" + const taskDir = path.join(tmpBaseDir, "tasks", taskId) + await fs.mkdir(taskDir, { recursive: true }) + + // Only write the old fallback file (claude_messages.json), NOT the new one + const oldPath = path.join(taskDir, "claude_messages.json") + await fs.writeFile(oldPath, "not json at all {[!", "utf8") + + const result = await readApiMessages({ + taskId, + globalStoragePath: tmpBaseDir, + }) + + expect(result).toEqual([]) + + // The corrupted fallback file should NOT be deleted + const stillExists = await fs + .access(oldPath) + .then(() => true) + .catch(() => false) + expect(stillExists).toBe(true) + }) + + it("returns [] when file contains valid JSON that is not an array", async () => { + const taskId = "task-non-array-api" + const taskDir = path.join(tmpBaseDir, "tasks", taskId) + await fs.mkdir(taskDir, { recursive: true }) + const filePath = path.join(taskDir, "api_conversation_history.json") + await fs.writeFile(filePath, JSON.stringify("hello"), "utf8") + + const result = await readApiMessages({ + taskId, + globalStoragePath: tmpBaseDir, + }) + + expect(result).toEqual([]) + }) + + it("returns [] when fallback file contains valid JSON that is not an array", async () => { + const taskId = "task-non-array-fallback" + const taskDir = path.join(tmpBaseDir, "tasks", taskId) + await fs.mkdir(taskDir, { recursive: true }) + + // Only write the old fallback file, NOT the new one + const oldPath = path.join(taskDir, "claude_messages.json") + await fs.writeFile(oldPath, JSON.stringify({ key: "value" }), "utf8") + + const result = await readApiMessages({ + taskId, + globalStoragePath: tmpBaseDir, + }) + + expect(result).toEqual([]) + }) +}) diff --git a/src/core/task-persistence/__tests__/taskMessages.spec.ts b/src/core/task-persistence/__tests__/taskMessages.spec.ts index 98148d6ed61..c6bc360c052 100644 --- a/src/core/task-persistence/__tests__/taskMessages.spec.ts +++ b/src/core/task-persistence/__tests__/taskMessages.spec.ts @@ -12,7 +12,7 @@ vi.mock("../../../utils/safeWriteJson", () => ({ })) // Import after mocks -import { saveTaskMessages } from "../taskMessages" +import { saveTaskMessages, readTaskMessages } from "../taskMessages" let tmpBaseDir: string @@ -66,3 +66,36 @@ describe("taskMessages.saveTaskMessages", () => { expect(persisted).toEqual(messages) }) }) + +describe("taskMessages.readTaskMessages", () => { + it("returns empty array when file contains invalid JSON", async () => { + const taskId = "task-corrupt-json" + // Manually create the task directory and write corrupted JSON + const taskDir = path.join(tmpBaseDir, "tasks", taskId) + await fs.mkdir(taskDir, { recursive: true }) + const filePath = path.join(taskDir, "ui_messages.json") + await fs.writeFile(filePath, "{not valid json!!!", "utf8") + + const result = await readTaskMessages({ + taskId, + globalStoragePath: tmpBaseDir, + }) + + expect(result).toEqual([]) + }) + + it("returns [] when file contains valid JSON that is not an array", async () => { + const taskId = "task-non-array-json" + const taskDir = path.join(tmpBaseDir, "tasks", taskId) + await fs.mkdir(taskDir, { recursive: true }) + const filePath = path.join(taskDir, "ui_messages.json") + await fs.writeFile(filePath, JSON.stringify("hello"), "utf8") + + const result = await readTaskMessages({ + taskId, + globalStoragePath: tmpBaseDir, + }) + + expect(result).toEqual([]) + }) +}) diff --git a/src/core/task-persistence/apiMessages.ts b/src/core/task-persistence/apiMessages.ts index 097679e4a7b..7672f6f7ee6 100644 --- a/src/core/task-persistence/apiMessages.ts +++ b/src/core/task-persistence/apiMessages.ts @@ -51,17 +51,23 @@ export async function readApiMessages({ const fileContent = await fs.readFile(filePath, "utf8") try { const parsedData = JSON.parse(fileContent) - if (Array.isArray(parsedData) && parsedData.length === 0) { + if (!Array.isArray(parsedData)) { + console.warn( + `[readApiMessages] Parsed data is not an array (got ${typeof parsedData}), returning empty. TaskId: ${taskId}, Path: ${filePath}`, + ) + return [] + } + if (parsedData.length === 0) { console.error( `[Roo-Debug] readApiMessages: Found API conversation history file, but it's empty (parsed as []). TaskId: ${taskId}, Path: ${filePath}`, ) } return parsedData } catch (error) { - console.error( - `[Roo-Debug] readApiMessages: Error parsing API conversation history file. TaskId: ${taskId}, Path: ${filePath}, Error: ${error}`, + console.warn( + `[readApiMessages] Error parsing API conversation history file, returning empty. TaskId: ${taskId}, Path: ${filePath}, Error: ${error}`, ) - throw error + return [] } } else { const oldPath = path.join(taskDir, "claude_messages.json") @@ -70,7 +76,13 @@ export async function readApiMessages({ const fileContent = await fs.readFile(oldPath, "utf8") try { const parsedData = JSON.parse(fileContent) - if (Array.isArray(parsedData) && parsedData.length === 0) { + if (!Array.isArray(parsedData)) { + console.warn( + `[readApiMessages] Parsed OLD data is not an array (got ${typeof parsedData}), returning empty. TaskId: ${taskId}, Path: ${oldPath}`, + ) + return [] + } + if (parsedData.length === 0) { console.error( `[Roo-Debug] readApiMessages: Found OLD API conversation history file (claude_messages.json), but it's empty (parsed as []). TaskId: ${taskId}, Path: ${oldPath}`, ) @@ -78,11 +90,11 @@ export async function readApiMessages({ await fs.unlink(oldPath) return parsedData } catch (error) { - console.error( - `[Roo-Debug] readApiMessages: Error parsing OLD API conversation history file (claude_messages.json). TaskId: ${taskId}, Path: ${oldPath}, Error: ${error}`, + console.warn( + `[readApiMessages] Error parsing OLD API conversation history file (claude_messages.json), returning empty. TaskId: ${taskId}, Path: ${oldPath}, Error: ${error}`, ) - // DO NOT unlink oldPath if parsing failed, throw error instead. - throw error + // DO NOT unlink oldPath if parsing failed. + return [] } } } diff --git a/src/core/task-persistence/taskMessages.ts b/src/core/task-persistence/taskMessages.ts index 63a2eefbaae..cee66432d9d 100644 --- a/src/core/task-persistence/taskMessages.ts +++ b/src/core/task-persistence/taskMessages.ts @@ -23,7 +23,21 @@ export async function readTaskMessages({ const fileExists = await fileExistsAtPath(filePath) if (fileExists) { - return JSON.parse(await fs.readFile(filePath, "utf8")) + try { + const parsedData = JSON.parse(await fs.readFile(filePath, "utf8")) + if (!Array.isArray(parsedData)) { + console.warn( + `[readTaskMessages] Parsed data is not an array (got ${typeof parsedData}), returning empty. TaskId: ${taskId}, Path: ${filePath}`, + ) + return [] + } + return parsedData + } catch (error) { + console.warn( + `[readTaskMessages] Failed to parse ${filePath} for task ${taskId}, returning empty: ${error instanceof Error ? error.message : String(error)}`, + ) + return [] + } } return [] diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index f4e41c1bfd7..ef6e956dff8 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -1203,10 +1203,10 @@ export class Task extends EventEmitter implements TaskLike { * tools execute (added in recursivelyMakeClineRequests after streaming completes). * So we usually only need to flush the pending user message with tool_results. */ - public async flushPendingToolResultsToHistory(): Promise { + public async flushPendingToolResultsToHistory(): Promise { // Only flush if there's actually pending content to save if (this.userMessageContent.length === 0) { - return + return true } // CRITICAL: Wait for the assistant message to be saved to API history first. @@ -1236,7 +1236,7 @@ export class Task extends EventEmitter implements TaskLike { // If task was aborted while waiting, don't flush if (this.abort) { - return + return false } // Save the user message with tool_result blocks @@ -1253,25 +1253,58 @@ export class Task extends EventEmitter implements TaskLike { const userMessageWithTs = { ...validatedMessage, ts: Date.now() } this.apiConversationHistory.push(userMessageWithTs as ApiMessage) - await this.saveApiConversationHistory() + const saved = await this.saveApiConversationHistory() + + if (saved) { + // Clear the pending content since it's now saved + this.userMessageContent = [] + } else { + console.warn( + `[Task#${this.taskId}] flushPendingToolResultsToHistory: save failed, retaining pending tool results in memory`, + ) + } - // Clear the pending content since it's now saved - this.userMessageContent = [] + return saved } - private async saveApiConversationHistory() { + private async saveApiConversationHistory(): Promise { try { await saveApiMessages({ - messages: this.apiConversationHistory, + messages: structuredClone(this.apiConversationHistory), taskId: this.taskId, globalStoragePath: this.globalStoragePath, }) + return true } catch (error) { - // In the off chance this fails, we don't want to stop the task. console.error("Failed to save API conversation history:", error) + return false } } + /** + * Public wrapper to retry saving the API conversation history. + * Uses exponential backoff: up to 3 attempts with delays of 100 ms, 500 ms, 1500 ms. + * Used by delegation flow when flushPendingToolResultsToHistory reports failure. + */ + public async retrySaveApiConversationHistory(): Promise { + const delays = [100, 500, 1500] + + for (let attempt = 0; attempt < delays.length; attempt++) { + await new Promise((resolve) => setTimeout(resolve, delays[attempt])) + console.warn( + `[Task#${this.taskId}] retrySaveApiConversationHistory: retry attempt ${attempt + 1}/${delays.length}`, + ) + + const success = await this.saveApiConversationHistory() + + if (success) { + return true + } + } + + return false + } + // Cline Messages private async getSavedClineMessages(): Promise { @@ -1333,10 +1366,10 @@ export class Task extends EventEmitter implements TaskLike { } } - private async saveClineMessages() { + private async saveClineMessages(): Promise { try { await saveTaskMessages({ - messages: this.clineMessages, + messages: structuredClone(this.clineMessages), taskId: this.taskId, globalStoragePath: this.globalStoragePath, }) @@ -1366,8 +1399,10 @@ export class Task extends EventEmitter implements TaskLike { this.debouncedEmitTokenUsage(tokenUsage, this.toolUsage) await this.providerRef.deref()?.updateTaskHistory(historyItem) + return true } catch (error) { console.error("Failed to save Roo messages:", error) + return false } } diff --git a/src/core/task/__tests__/Task.persistence.spec.ts b/src/core/task/__tests__/Task.persistence.spec.ts new file mode 100644 index 00000000000..1e4acc9713b --- /dev/null +++ b/src/core/task/__tests__/Task.persistence.spec.ts @@ -0,0 +1,471 @@ +// cd src && npx vitest run core/task/__tests__/Task.persistence.spec.ts + +import * as os from "os" +import * as path from "path" +import * as vscode from "vscode" + +import type { GlobalState, ProviderSettings } from "@roo-code/types" +import { TelemetryService } from "@roo-code/telemetry" + +import { Task } from "../Task" +import { ClineProvider } from "../../webview/ClineProvider" +import { ContextProxy } from "../../config/ContextProxy" + +// ─── Hoisted mocks ─────────────────────────────────────────────────────────── + +const { + mockSaveApiMessages, + mockSaveTaskMessages, + mockReadApiMessages, + mockReadTaskMessages, + mockTaskMetadata, + mockPWaitFor, +} = vi.hoisted(() => ({ + mockSaveApiMessages: vi.fn().mockResolvedValue(undefined), + mockSaveTaskMessages: vi.fn().mockResolvedValue(undefined), + mockReadApiMessages: vi.fn().mockResolvedValue([]), + mockReadTaskMessages: vi.fn().mockResolvedValue([]), + mockTaskMetadata: vi.fn().mockResolvedValue({ + historyItem: { id: "test-id", ts: Date.now(), task: "test" }, + tokenUsage: { + totalTokensIn: 0, + totalTokensOut: 0, + totalCacheWrites: 0, + totalCacheReads: 0, + totalCost: 0, + contextTokens: 0, + }, + }), + mockPWaitFor: vi.fn().mockResolvedValue(undefined), +})) + +// ─── Module mocks ──────────────────────────────────────────────────────────── + +vi.mock("delay", () => ({ + __esModule: true, + default: vi.fn().mockResolvedValue(undefined), +})) + +vi.mock("execa", () => ({ + execa: vi.fn(), +})) + +vi.mock("fs/promises", async (importOriginal) => { + const actual = (await importOriginal()) as Record + return { + ...actual, + mkdir: vi.fn().mockResolvedValue(undefined), + writeFile: vi.fn().mockResolvedValue(undefined), + readFile: vi.fn().mockResolvedValue("[]"), + unlink: vi.fn().mockResolvedValue(undefined), + rmdir: vi.fn().mockResolvedValue(undefined), + default: { + mkdir: vi.fn().mockResolvedValue(undefined), + writeFile: vi.fn().mockResolvedValue(undefined), + readFile: vi.fn().mockResolvedValue("[]"), + unlink: vi.fn().mockResolvedValue(undefined), + rmdir: vi.fn().mockResolvedValue(undefined), + }, + } +}) + +vi.mock("p-wait-for", () => ({ + default: mockPWaitFor, +})) + +vi.mock("../../task-persistence", () => ({ + saveApiMessages: mockSaveApiMessages, + saveTaskMessages: mockSaveTaskMessages, + readApiMessages: mockReadApiMessages, + readTaskMessages: mockReadTaskMessages, + taskMetadata: mockTaskMetadata, +})) + +vi.mock("vscode", () => { + const mockDisposable = { dispose: vi.fn() } + const mockEventEmitter = { event: vi.fn(), fire: vi.fn() } + const mockTextDocument = { uri: { fsPath: "/mock/workspace/path/file.ts" } } + const mockTextEditor = { document: mockTextDocument } + const mockTab = { input: { uri: { fsPath: "/mock/workspace/path/file.ts" } } } + const mockTabGroup = { tabs: [mockTab] } + + return { + TabInputTextDiff: vi.fn(), + CodeActionKind: { + QuickFix: { value: "quickfix" }, + RefactorRewrite: { value: "refactor.rewrite" }, + }, + window: { + createTextEditorDecorationType: vi.fn().mockReturnValue({ dispose: vi.fn() }), + visibleTextEditors: [mockTextEditor], + tabGroups: { + all: [mockTabGroup], + close: vi.fn(), + onDidChangeTabs: vi.fn(() => ({ dispose: vi.fn() })), + }, + showErrorMessage: vi.fn(), + }, + workspace: { + workspaceFolders: [ + { + uri: { fsPath: "/mock/workspace/path" }, + name: "mock-workspace", + index: 0, + }, + ], + createFileSystemWatcher: vi.fn(() => ({ + onDidCreate: vi.fn(() => mockDisposable), + onDidDelete: vi.fn(() => mockDisposable), + onDidChange: vi.fn(() => mockDisposable), + dispose: vi.fn(), + })), + fs: { + stat: vi.fn().mockResolvedValue({ type: 1 }), + }, + onDidSaveTextDocument: vi.fn(() => mockDisposable), + getConfiguration: vi.fn(() => ({ get: (_key: string, defaultValue: unknown) => defaultValue })), + }, + env: { + uriScheme: "vscode", + language: "en", + }, + EventEmitter: vi.fn().mockImplementation(() => mockEventEmitter), + Disposable: { + from: vi.fn(), + }, + TabInputText: vi.fn(), + } +}) + +vi.mock("../../mentions", () => ({ + parseMentions: vi.fn().mockImplementation((text) => { + return Promise.resolve({ text: `processed: ${text}`, mode: undefined, contentBlocks: [] }) + }), + openMention: vi.fn(), + getLatestTerminalOutput: vi.fn(), +})) + +vi.mock("../../../integrations/misc/extract-text", () => ({ + extractTextFromFile: vi.fn().mockResolvedValue("Mock file content"), +})) + +vi.mock("../../environment/getEnvironmentDetails", () => ({ + getEnvironmentDetails: vi.fn().mockResolvedValue(""), +})) + +vi.mock("../../ignore/RooIgnoreController") + +vi.mock("../../condense", async (importOriginal) => { + const actual = (await importOriginal()) as Record + return { + ...actual, + summarizeConversation: vi.fn().mockResolvedValue({ + messages: [{ role: "user", content: [{ type: "text", text: "continued" }], ts: Date.now() }], + summary: "summary", + cost: 0, + newContextTokens: 1, + }), + } +}) + +vi.mock("../../../utils/storage", () => ({ + getTaskDirectoryPath: vi + .fn() + .mockImplementation((globalStoragePath, taskId) => Promise.resolve(`${globalStoragePath}/tasks/${taskId}`)), + getSettingsDirectoryPath: vi + .fn() + .mockImplementation((globalStoragePath) => Promise.resolve(`${globalStoragePath}/settings`)), +})) + +vi.mock("../../../utils/fs", () => ({ + fileExistsAtPath: vi.fn().mockReturnValue(false), +})) + +// ─── Test suite ────────────────────────────────────────────────────────────── + +describe("Task persistence", () => { + let mockProvider: ClineProvider & Record + let mockApiConfig: ProviderSettings + let mockOutputChannel: vscode.OutputChannel + let mockExtensionContext: vscode.ExtensionContext + + beforeEach(() => { + vi.clearAllMocks() + + if (!TelemetryService.hasInstance()) { + TelemetryService.createInstance([]) + } + + const storageUri = { fsPath: path.join(os.tmpdir(), "test-storage") } + + mockExtensionContext = { + globalState: { + get: vi.fn().mockImplementation((_key: keyof GlobalState) => undefined), + update: vi.fn().mockImplementation((_key, _value) => Promise.resolve()), + keys: vi.fn().mockReturnValue([]), + }, + globalStorageUri: storageUri, + workspaceState: { + get: vi.fn().mockImplementation((_key) => undefined), + update: vi.fn().mockImplementation((_key, _value) => Promise.resolve()), + keys: vi.fn().mockReturnValue([]), + }, + secrets: { + get: vi.fn().mockImplementation((_key) => Promise.resolve(undefined)), + store: vi.fn().mockImplementation((_key, _value) => Promise.resolve()), + delete: vi.fn().mockImplementation((_key) => Promise.resolve()), + }, + extensionUri: { fsPath: "/mock/extension/path" }, + extension: { packageJSON: { version: "1.0.0" } }, + } as unknown as vscode.ExtensionContext + + mockOutputChannel = { + appendLine: vi.fn(), + append: vi.fn(), + clear: vi.fn(), + show: vi.fn(), + hide: vi.fn(), + dispose: vi.fn(), + } as unknown as vscode.OutputChannel + + mockProvider = new ClineProvider( + mockExtensionContext, + mockOutputChannel, + "sidebar", + new ContextProxy(mockExtensionContext), + ) as ClineProvider & Record + + mockApiConfig = { + apiProvider: "anthropic", + apiModelId: "claude-3-5-sonnet-20241022", + apiKey: "test-api-key", + } + + mockProvider.postMessageToWebview = vi.fn().mockResolvedValue(undefined) + mockProvider.postStateToWebview = vi.fn().mockResolvedValue(undefined) + mockProvider.postStateToWebviewWithoutTaskHistory = vi.fn().mockResolvedValue(undefined) + mockProvider.updateTaskHistory = vi.fn().mockResolvedValue(undefined) + }) + + // ── saveApiConversationHistory (via retrySaveApiConversationHistory) ── + + describe("saveApiConversationHistory", () => { + it("returns true on success", async () => { + mockSaveApiMessages.mockResolvedValueOnce(undefined) + + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + + task.apiConversationHistory.push({ + role: "user", + content: [{ type: "text", text: "hello" }], + }) + + const result = await task.retrySaveApiConversationHistory() + expect(result).toBe(true) + }) + + it("returns false on failure", async () => { + vi.useFakeTimers() + + // All 3 retry attempts must fail for retrySaveApiConversationHistory to return false + mockSaveApiMessages + .mockRejectedValueOnce(new Error("fail 1")) + .mockRejectedValueOnce(new Error("fail 2")) + .mockRejectedValueOnce(new Error("fail 3")) + + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + + const promise = task.retrySaveApiConversationHistory() + await vi.runAllTimersAsync() + const result = await promise + + expect(result).toBe(false) + expect(mockSaveApiMessages).toHaveBeenCalledTimes(3) + + vi.useRealTimers() + }) + + it("succeeds on 2nd retry attempt", async () => { + vi.useFakeTimers() + + mockSaveApiMessages.mockRejectedValueOnce(new Error("fail 1")).mockResolvedValueOnce(undefined) // succeeds on 2nd try + + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + + const promise = task.retrySaveApiConversationHistory() + await vi.runAllTimersAsync() + const result = await promise + + expect(result).toBe(true) + expect(mockSaveApiMessages).toHaveBeenCalledTimes(2) + + vi.useRealTimers() + }) + + it("snapshots the array before passing to saveApiMessages", async () => { + mockSaveApiMessages.mockResolvedValueOnce(undefined) + + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + + const originalMsg = { + role: "user" as const, + content: [{ type: "text" as const, text: "snapshot test" }], + } + task.apiConversationHistory.push(originalMsg) + + await task.retrySaveApiConversationHistory() + + expect(mockSaveApiMessages).toHaveBeenCalledTimes(1) + + const callArgs = mockSaveApiMessages.mock.calls[0][0] + // The messages passed should be a COPY, not the live reference + expect(callArgs.messages).not.toBe(task.apiConversationHistory) + // But the content should be the same + expect(callArgs.messages).toEqual(task.apiConversationHistory) + }) + }) + + // ── saveClineMessages ──────────────────────────────────────────────── + + describe("saveClineMessages", () => { + it("returns true on success", async () => { + mockSaveTaskMessages.mockResolvedValueOnce(undefined) + + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + + const result = await (task as Record).saveClineMessages() + expect(result).toBe(true) + }) + + it("returns false on failure", async () => { + mockSaveTaskMessages.mockRejectedValueOnce(new Error("write error")) + + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + + const result = await (task as Record).saveClineMessages() + expect(result).toBe(false) + }) + + it("snapshots the array before passing to saveTaskMessages", async () => { + mockSaveTaskMessages.mockResolvedValueOnce(undefined) + + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + + task.clineMessages.push({ + type: "say", + say: "text", + text: "snapshot test", + ts: Date.now(), + }) + + await (task as Record).saveClineMessages() + + expect(mockSaveTaskMessages).toHaveBeenCalledTimes(1) + + const callArgs = mockSaveTaskMessages.mock.calls[0][0] + // The messages passed should be a COPY, not the live reference + expect(callArgs.messages).not.toBe(task.clineMessages) + // But the content should be the same + expect(callArgs.messages).toEqual(task.clineMessages) + }) + }) + + // ── flushPendingToolResultsToHistory — save failure/success ─────────── + + describe("flushPendingToolResultsToHistory persistence", () => { + it("retains userMessageContent on save failure", async () => { + mockSaveApiMessages.mockRejectedValueOnce(new Error("disk full")) + + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + + // Skip waiting for assistant message + task.assistantMessageSavedToHistory = true + + task.userMessageContent = [ + { + type: "tool_result", + tool_use_id: "tool-fail", + content: "Result that should be retained", + }, + ] + + const saved = await task.flushPendingToolResultsToHistory() + + expect(saved).toBe(false) + // userMessageContent should NOT be cleared on failure + expect(task.userMessageContent.length).toBeGreaterThan(0) + expect(task.userMessageContent[0]).toMatchObject({ + type: "tool_result", + tool_use_id: "tool-fail", + }) + }) + + it("clears userMessageContent on save success", async () => { + mockSaveApiMessages.mockResolvedValueOnce(undefined) + + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + + // Skip waiting for assistant message + task.assistantMessageSavedToHistory = true + + task.userMessageContent = [ + { + type: "tool_result", + tool_use_id: "tool-ok", + content: "Result that should be cleared", + }, + ] + + const saved = await task.flushPendingToolResultsToHistory() + + expect(saved).toBe(true) + // userMessageContent should be cleared on success + expect(task.userMessageContent).toEqual([]) + }) + }) +}) diff --git a/src/core/task/__tests__/flushPendingToolResultsToHistory.spec.ts b/src/core/task/__tests__/flushPendingToolResultsToHistory.spec.ts index f4d78802d29..f19645d9697 100644 --- a/src/core/task/__tests__/flushPendingToolResultsToHistory.spec.ts +++ b/src/core/task/__tests__/flushPendingToolResultsToHistory.spec.ts @@ -21,6 +21,10 @@ vi.mock("execa", () => ({ execa: vi.fn(), })) +vi.mock("../../../utils/safeWriteJson", () => ({ + safeWriteJson: vi.fn().mockResolvedValue(undefined), +})) + vi.mock("fs/promises", async (importOriginal) => { const actual = (await importOriginal()) as Record const mockFunctions = { diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index b598a27c272..5ff0f1c3658 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -1665,31 +1665,40 @@ export class ClineProvider const history = this.getGlobalState("taskHistory") ?? [] const historyItem = history.find((item) => item.id === id) - if (historyItem) { - const { getTaskDirectoryPath } = await import("../../utils/storage") - const globalStoragePath = this.contextProxy.globalStorageUri.fsPath - const taskDirPath = await getTaskDirectoryPath(globalStoragePath, id) - const apiConversationHistoryFilePath = path.join(taskDirPath, GlobalFileNames.apiConversationHistory) - const uiMessagesFilePath = path.join(taskDirPath, GlobalFileNames.uiMessages) - const fileExists = await fileExistsAtPath(apiConversationHistoryFilePath) - - if (fileExists) { - const apiConversationHistory = JSON.parse(await fs.readFile(apiConversationHistoryFilePath, "utf8")) - - return { - historyItem, - taskDirPath, - apiConversationHistoryFilePath, - uiMessagesFilePath, - apiConversationHistory, - } + if (!historyItem) { + throw new Error("Task not found") + } + + const { getTaskDirectoryPath } = await import("../../utils/storage") + const globalStoragePath = this.contextProxy.globalStorageUri.fsPath + const taskDirPath = await getTaskDirectoryPath(globalStoragePath, id) + const apiConversationHistoryFilePath = path.join(taskDirPath, GlobalFileNames.apiConversationHistory) + const uiMessagesFilePath = path.join(taskDirPath, GlobalFileNames.uiMessages) + const fileExists = await fileExistsAtPath(apiConversationHistoryFilePath) + + let apiConversationHistory: Anthropic.MessageParam[] = [] + + if (fileExists) { + try { + apiConversationHistory = JSON.parse(await fs.readFile(apiConversationHistoryFilePath, "utf8")) + } catch (error) { + console.warn( + `[getTaskWithId] api_conversation_history.json corrupted for task ${id}, returning empty history: ${error instanceof Error ? error.message : String(error)}`, + ) } + } else { + console.warn( + `[getTaskWithId] api_conversation_history.json missing for task ${id}, returning empty history`, + ) } - // if we tried to get a task that doesn't exist, remove it from state - // FIXME: this seems to happen sometimes when the json file doesnt save to disk for some reason - await this.deleteTaskFromState(id) - throw new Error("Task not found") + return { + historyItem, + taskDirPath, + apiConversationHistoryFilePath, + uiMessagesFilePath, + apiConversationHistory, + } } async getTaskWithAggregatedCosts(taskId: string): Promise<{ @@ -3182,7 +3191,21 @@ export class ClineProvider // recursivelyMakeClineRequests BEFORE tools start executing. We only need to // flush the pending user message with tool_results. try { - await parent.flushPendingToolResultsToHistory() + const flushSuccess = await parent.flushPendingToolResultsToHistory() + + if (!flushSuccess) { + console.warn(`[delegateParentAndOpenChild] Flush failed for parent ${parentTaskId}, retrying...`) + const retrySuccess = await parent.retrySaveApiConversationHistory() + + if (!retrySuccess) { + console.error( + `[delegateParentAndOpenChild] CRITICAL: Parent ${parentTaskId} API history not persisted to disk. Child return may produce stale state.`, + ) + vscode.window.showWarningMessage( + "Warning: Parent task state could not be saved. The parent task may lose recent context when resumed.", + ) + } + } } catch (error) { this.log( `[delegateParentAndOpenChild] Error flushing pending tool results (non-fatal): ${ diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index b65b137597c..26f6fbd8aba 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -3770,4 +3770,53 @@ describe("ClineProvider - Comprehensive Edit/Delete Edge Cases", () => { }) }) }) + + describe("getTaskWithId", () => { + it("returns empty apiConversationHistory when file is missing", async () => { + const historyItem = { id: "missing-api-file-task", task: "test task", ts: Date.now() } + vi.mocked(mockContext.globalState.get).mockImplementation((key: string) => { + if (key === "taskHistory") { + return [historyItem] + } + return undefined + }) + + const deleteTaskSpy = vi.spyOn(provider, "deleteTaskFromState") + + const result = await (provider as any).getTaskWithId("missing-api-file-task") + + expect(result.historyItem).toEqual(historyItem) + expect(result.apiConversationHistory).toEqual([]) + expect(deleteTaskSpy).not.toHaveBeenCalled() + }) + + it("returns empty apiConversationHistory when file contains invalid JSON", async () => { + const historyItem = { id: "corrupt-api-task", task: "test task", ts: Date.now() } + vi.mocked(mockContext.globalState.get).mockImplementation((key: string) => { + if (key === "taskHistory") { + return [historyItem] + } + return undefined + }) + + // Make fileExistsAtPath return true so the read path is exercised + const fsUtils = await import("../../../utils/fs") + vi.spyOn(fsUtils, "fileExistsAtPath").mockResolvedValue(true) + + // Make readFile return corrupted JSON + const fsp = await import("fs/promises") + vi.mocked(fsp.readFile).mockResolvedValueOnce("{not valid json!!!" as never) + + const deleteTaskSpy = vi.spyOn(provider, "deleteTaskFromState") + + const result = await (provider as any).getTaskWithId("corrupt-api-task") + + expect(result.historyItem).toEqual(historyItem) + expect(result.apiConversationHistory).toEqual([]) + expect(deleteTaskSpy).not.toHaveBeenCalled() + + // Restore the spy + vi.mocked(fsUtils.fileExistsAtPath).mockRestore() + }) + }) })