fix: validate deserialized API messages at boundary to prevent crashes

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)
+		})
+	})
+})

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}`,