mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-08-28 05:27:24 +00:00
fix: validate deserialized API messages to prevent undefined content errors
Root cause: JSON.parse() in readApiMessages() deserializes data with zero validation, allowing corrupted or legacy data to violate type contracts (e.g., undefined content). This causes downstream crashes in functions like filterNonAnthropicBlocks() that assume message.content is always string | ContentBlock[] per the type system, but receive undefined/null values from corrupted historical data. Solution: - Add validateApiMessage() to sanitize deserialized messages at the boundary - Fix undefined/null/non-array content by converting to empty array - Filter out malformed messages with invalid roles or non-object structure - Filter out invalid content blocks missing the required type field - Add comprehensive test coverage (12 tests covering all edge cases) Fixes: https://www.reddit.com/r/RooCode/s/yUXVFMYu3P Related to closed PR #10954 (which treated symptom, not cause)
This commit is contained in:
Roo Code
parent
953c7773c0
commit
bbdcab393a
2 changed files with 337 additions and 2 deletions
241
src/core/task-persistence/__tests__/apiMessages.spec.ts
Normal file
241
src/core/task-persistence/__tests__/apiMessages.spec.ts
Normal file
|
|
@ -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)
|
||||
})
|
||||
})
|
||||
})
|
||||
|
|
@ -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}`,
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue