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..c71bac88d53 --- /dev/null +++ b/src/core/task-persistence/__tests__/apiMessages.spec.ts @@ -0,0 +1,241 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest" +import * as fs from "fs/promises" +import * as path from "path" +import { readApiMessages, saveApiMessages } from "../apiMessages" + +// Mock dependencies +vi.mock("../../../utils/fs") +vi.mock("../../../utils/storage") +vi.mock("fs/promises") +vi.mock("../../../utils/safeWriteJson") + +const { fileExistsAtPath } = await import("../../../utils/fs") +const { getTaskDirectoryPath } = await import("../../../utils/storage") +const { safeWriteJson } = await import("../../../utils/safeWriteJson") + +describe("apiMessages", () => { + const mockTaskId = "test-task-id" + const mockGlobalStoragePath = "/mock/storage" + const mockTaskDir = "/mock/storage/tasks/test-task-id" + const mockFilePath = path.join(mockTaskDir, "api_conversation_history.json") + + beforeEach(() => { + vi.clearAllMocks() + vi.mocked(getTaskDirectoryPath).mockResolvedValue(mockTaskDir) + }) + + afterEach(() => { + vi.resetAllMocks() + }) + + describe("readApiMessages - validation", () => { + it("should successfully read and return valid messages", async () => { + const validMessages = [ + { role: "user", content: "Hello" }, + { role: "assistant", content: [{ type: "text", text: "Hi there!" }] }, + ] + + vi.mocked(fileExistsAtPath).mockResolvedValue(true) + vi.mocked(fs.readFile).mockResolvedValue(JSON.stringify(validMessages)) + + const result = await readApiMessages({ taskId: mockTaskId, globalStoragePath: mockGlobalStoragePath }) + + expect(result).toHaveLength(2) + expect(result[0]).toEqual({ role: "user", content: "Hello" }) + expect(result[1]).toEqual({ role: "assistant", content: [{ type: "text", text: "Hi there!" }] }) + }) + + it("should fix messages with undefined content", async () => { + const messagesWithUndefinedContent = [ + { role: "user", content: "Valid" }, + { role: "assistant", content: undefined }, + { role: "user", content: "Continue" }, + ] + + vi.mocked(fileExistsAtPath).mockResolvedValue(true) + vi.mocked(fs.readFile).mockResolvedValue(JSON.stringify(messagesWithUndefinedContent)) + + const result = await readApiMessages({ taskId: mockTaskId, globalStoragePath: mockGlobalStoragePath }) + + expect(result).toHaveLength(3) + expect(result[0].content).toBe("Valid") + expect(result[1].content).toEqual([]) // Fixed from undefined to empty array + expect(result[2].content).toBe("Continue") + }) + + it("should fix messages with null content", async () => { + const messagesWithNullContent = [ + { role: "user", content: "Valid" }, + { role: "assistant", content: null }, + { role: "user", content: "Continue" }, + ] + + vi.mocked(fileExistsAtPath).mockResolvedValue(true) + vi.mocked(fs.readFile).mockResolvedValue(JSON.stringify(messagesWithNullContent)) + + const result = await readApiMessages({ taskId: mockTaskId, globalStoragePath: mockGlobalStoragePath }) + + expect(result).toHaveLength(3) + expect(result[0].content).toBe("Valid") + expect(result[1].content).toEqual([]) // Fixed from null to empty array + expect(result[2].content).toBe("Continue") + }) + + it("should fix messages with non-array, non-string content", async () => { + const messagesWithInvalidContent = [ + { role: "user", content: "Valid" }, + { role: "assistant", content: { invalid: "object" } }, + { role: "user", content: "Continue" }, + ] + + vi.mocked(fileExistsAtPath).mockResolvedValue(true) + vi.mocked(fs.readFile).mockResolvedValue(JSON.stringify(messagesWithInvalidContent)) + + const result = await readApiMessages({ taskId: mockTaskId, globalStoragePath: mockGlobalStoragePath }) + + expect(result).toHaveLength(3) + expect(result[0].content).toBe("Valid") + expect(result[1].content).toEqual([]) // Fixed from object to empty array + expect(result[2].content).toBe("Continue") + }) + + it("should filter out messages with invalid roles", async () => { + const messagesWithInvalidRoles = [ + { role: "user", content: "Valid" }, + { role: "invalid_role", content: "Bad message" }, + { role: "assistant", content: "Also valid" }, + ] + + vi.mocked(fileExistsAtPath).mockResolvedValue(true) + vi.mocked(fs.readFile).mockResolvedValue(JSON.stringify(messagesWithInvalidRoles)) + + const result = await readApiMessages({ taskId: mockTaskId, globalStoragePath: mockGlobalStoragePath }) + + expect(result).toHaveLength(2) + expect(result[0].role).toBe("user") + expect(result[1].role).toBe("assistant") + }) + + it("should filter out completely malformed messages (not objects)", async () => { + const messagesWithNonObjects = [ + { role: "user", content: "Valid" }, + null, + "string message", + 123, + { role: "assistant", content: "Also valid" }, + ] + + vi.mocked(fileExistsAtPath).mockResolvedValue(true) + vi.mocked(fs.readFile).mockResolvedValue(JSON.stringify(messagesWithNonObjects)) + + const result = await readApiMessages({ taskId: mockTaskId, globalStoragePath: mockGlobalStoragePath }) + + expect(result).toHaveLength(2) + expect(result[0].role).toBe("user") + expect(result[1].role).toBe("assistant") + }) + + it("should filter out invalid blocks from message content arrays", async () => { + const messagesWithInvalidBlocks = [ + { + role: "assistant", + content: [ + { type: "text", text: "Valid text" }, + null, // Invalid block + { invalid: "no type field" }, // Invalid block + { type: "text", text: "Another valid text" }, + ], + }, + ] + + vi.mocked(fileExistsAtPath).mockResolvedValue(true) + vi.mocked(fs.readFile).mockResolvedValue(JSON.stringify(messagesWithInvalidBlocks)) + + const result = await readApiMessages({ taskId: mockTaskId, globalStoragePath: mockGlobalStoragePath }) + + expect(result).toHaveLength(1) + expect(Array.isArray(result[0].content)).toBe(true) + expect((result[0].content as any[]).length).toBe(2) + expect((result[0].content as any[])[0]).toEqual({ type: "text", text: "Valid text" }) + expect((result[0].content as any[])[1]).toEqual({ type: "text", text: "Another valid text" }) + }) + + it("should return empty array when parsedData is not an array", async () => { + const nonArrayData = { invalid: "not an array" } + + vi.mocked(fileExistsAtPath).mockResolvedValue(true) + vi.mocked(fs.readFile).mockResolvedValue(JSON.stringify(nonArrayData)) + + const result = await readApiMessages({ taskId: mockTaskId, globalStoragePath: mockGlobalStoragePath }) + + expect(result).toEqual([]) + }) + + it("should preserve valid metadata fields (ts, isSummary, etc.)", async () => { + const messagesWithMetadata = [ + { + role: "user", + content: "Test", + ts: 1234567890, + isSummary: true, + condenseId: "c-123", + truncationId: "t-456", + }, + ] + + vi.mocked(fileExistsAtPath).mockResolvedValue(true) + vi.mocked(fs.readFile).mockResolvedValue(JSON.stringify(messagesWithMetadata)) + + const result = await readApiMessages({ taskId: mockTaskId, globalStoragePath: mockGlobalStoragePath }) + + expect(result).toHaveLength(1) + expect(result[0].ts).toBe(1234567890) + expect(result[0].isSummary).toBe(true) + expect(result[0].condenseId).toBe("c-123") + expect(result[0].truncationId).toBe("t-456") + }) + + it("should handle old claude_messages.json format with validation", async () => { + const oldFormatMessages = [ + { role: "user", content: "Valid" }, + { role: "assistant", content: undefined }, // Should be fixed + ] + + // New format doesn't exist, old format exists + vi.mocked(fileExistsAtPath) + .mockResolvedValueOnce(false) // New format + .mockResolvedValueOnce(true) // Old format + + vi.mocked(fs.readFile).mockResolvedValue(JSON.stringify(oldFormatMessages)) + // Mock unlink to prevent errors + ;(fs as any).unlink = vi.fn().mockResolvedValue(undefined) + + const result = await readApiMessages({ taskId: mockTaskId, globalStoragePath: mockGlobalStoragePath }) + + expect(result).toHaveLength(2) + expect(result[0].content).toBe("Valid") + expect(result[1].content).toEqual([]) // Fixed from undefined + }) + + it("should return empty array when no file exists", async () => { + vi.mocked(fileExistsAtPath).mockResolvedValue(false) + + const result = await readApiMessages({ taskId: mockTaskId, globalStoragePath: mockGlobalStoragePath }) + + expect(result).toEqual([]) + }) + }) + + describe("saveApiMessages", () => { + it("should call safeWriteJson with correct parameters", async () => { + const messages = [ + { role: "user", content: "Test" }, + { role: "assistant", content: [{ type: "text", text: "Response" }] }, + ] as any[] + + await saveApiMessages({ messages, taskId: mockTaskId, globalStoragePath: mockGlobalStoragePath }) + + expect(safeWriteJson).toHaveBeenCalledWith(mockFilePath, messages) + }) + }) +}) diff --git a/src/core/task-persistence/apiMessages.ts b/src/core/task-persistence/apiMessages.ts index 097679e4a7b..826bc6c1fde 100644 --- a/src/core/task-persistence/apiMessages.ts +++ b/src/core/task-persistence/apiMessages.ts @@ -9,6 +9,62 @@ import { fileExistsAtPath } from "../../utils/fs" import { GlobalFileNames } from "../../shared/globalFileNames" import { getTaskDirectoryPath } from "../../utils/storage" +/** + * Validates and sanitizes a deserialized API message to ensure it meets type contract. + * Fixes common corruption issues like undefined/null/non-array content. + * + * @param message - Raw deserialized message object + * @param taskId - Task ID for logging purposes + * @param index - Message index for logging purposes + * @returns Validated message or null if the message is irreparably malformed + */ +function validateApiMessage(message: any, taskId: string, index: number): ApiMessage | null { + // Basic structure validation + if (!message || typeof message !== "object") { + console.error( + `[readApiMessages] Invalid message at index ${index} for task ${taskId}: not an object. Skipping.`, + ) + return null + } + + // Validate role + if (message.role !== "user" && message.role !== "assistant") { + console.error( + `[readApiMessages] Invalid message role at index ${index} for task ${taskId}: "${message.role}". Skipping.`, + ) + return null + } + + // Validate and fix content field + if (message.content === undefined || message.content === null) { + // Content is missing - fix it with empty array for array content or empty string for string content + console.warn( + `[readApiMessages] Message at index ${index} for task ${taskId} has undefined/null content. Fixing with empty array.`, + ) + message.content = [] + } else if (!Array.isArray(message.content) && typeof message.content !== "string") { + // Content is wrong type - try to recover + console.warn( + `[readApiMessages] Message at index ${index} for task ${taskId} has invalid content type (${typeof message.content}). Fixing with empty array.`, + ) + message.content = [] + } else if (Array.isArray(message.content)) { + // Content is an array - validate each block has a type + const validatedContent = message.content.filter((block: any) => { + if (!block || typeof block !== "object" || typeof block.type !== "string") { + console.warn( + `[readApiMessages] Removing invalid content block at index ${index} for task ${taskId}: missing or invalid type field.`, + ) + return false + } + return true + }) + message.content = validatedContent + } + + return message as ApiMessage +} + export type ApiMessage = Anthropic.MessageParam & { ts?: number isSummary?: boolean @@ -56,7 +112,26 @@ export async function readApiMessages({ `[Roo-Debug] readApiMessages: Found API conversation history file, but it's empty (parsed as []). TaskId: ${taskId}, Path: ${filePath}`, ) } - return parsedData + + // Validate and sanitize each message + if (!Array.isArray(parsedData)) { + console.error( + `[readApiMessages] Parsed data is not an array for task ${taskId}. Returning empty array.`, + ) + return [] + } + + const validatedMessages = parsedData + .map((msg, index) => validateApiMessage(msg, taskId, index)) + .filter((msg): msg is ApiMessage => msg !== null) + + if (validatedMessages.length < parsedData.length) { + console.warn( + `[readApiMessages] Filtered out ${parsedData.length - validatedMessages.length} invalid messages for task ${taskId}`, + ) + } + + return validatedMessages } catch (error) { console.error( `[Roo-Debug] readApiMessages: Error parsing API conversation history file. TaskId: ${taskId}, Path: ${filePath}, Error: ${error}`, @@ -75,8 +150,27 @@ export async function readApiMessages({ `[Roo-Debug] readApiMessages: Found OLD API conversation history file (claude_messages.json), but it's empty (parsed as []). TaskId: ${taskId}, Path: ${oldPath}`, ) } + + // Validate and sanitize each message + if (!Array.isArray(parsedData)) { + console.error( + `[readApiMessages] Parsed old data is not an array for task ${taskId}. Returning empty array.`, + ) + return [] + } + + const validatedMessages = parsedData + .map((msg, index) => validateApiMessage(msg, taskId, index)) + .filter((msg): msg is ApiMessage => msg !== null) + + if (validatedMessages.length < parsedData.length) { + console.warn( + `[readApiMessages] Filtered out ${parsedData.length - validatedMessages.length} invalid messages from old format for task ${taskId}`, + ) + } + await fs.unlink(oldPath) - return parsedData + return validatedMessages } catch (error) { console.error( `[Roo-Debug] readApiMessages: Error parsing OLD API conversation history file (claude_messages.json). TaskId: ${taskId}, Path: ${oldPath}, Error: ${error}`,