mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-08-28 05:27:24 +00:00
fix: prevent chat history truncation for long conversations
- Changed findMessageIndices to use exact timestamp matching instead of 1-second buffer - Added validation and logging in message persistence layer - Added safeguards to prevent accidental data loss during message operations - Added comprehensive tests for the timestamp matching fix Fixes #6932
This commit is contained in:
Roo Code
parent
2a974e8bf6
commit
cd0f0d658b
4 changed files with 324 additions and 3 deletions
|
|
@ -77,7 +77,43 @@ export async function saveApiMessages({
|
|||
taskId: string
|
||||
globalStoragePath: string
|
||||
}) {
|
||||
// Validate messages before saving to prevent data corruption
|
||||
if (!Array.isArray(messages)) {
|
||||
console.error(
|
||||
`[Roo-Debug] saveApiMessages: Invalid messages format - expected array, got ${typeof messages}. TaskId: ${taskId}`,
|
||||
)
|
||||
throw new Error("Invalid messages format for saving")
|
||||
}
|
||||
|
||||
// Log warning for unusually large conversations
|
||||
if (messages.length > 1000) {
|
||||
console.warn(
|
||||
`[Roo-Debug] saveApiMessages: Saving large conversation with ${messages.length} messages. TaskId: ${taskId}`,
|
||||
)
|
||||
}
|
||||
|
||||
// Validate message structure
|
||||
for (let i = 0; i < messages.length; i++) {
|
||||
const msg = messages[i]
|
||||
if (!msg || typeof msg !== "object") {
|
||||
console.error(
|
||||
`[Roo-Debug] saveApiMessages: Invalid message at index ${i} - expected object, got ${typeof msg}. TaskId: ${taskId}`,
|
||||
)
|
||||
throw new Error(`Invalid message structure at index ${i}`)
|
||||
}
|
||||
if (!msg.role || (msg.role !== "user" && msg.role !== "assistant")) {
|
||||
console.error(
|
||||
`[Roo-Debug] saveApiMessages: Invalid message role at index ${i} - got "${msg.role}". TaskId: ${taskId}`,
|
||||
)
|
||||
throw new Error(`Invalid message role at index ${i}`)
|
||||
}
|
||||
}
|
||||
|
||||
const taskDir = await getTaskDirectoryPath(globalStoragePath, taskId)
|
||||
const filePath = path.join(taskDir, GlobalFileNames.apiConversationHistory)
|
||||
|
||||
// Log the save operation for debugging
|
||||
console.log(`[Roo-Debug] saveApiMessages: Saving ${messages.length} messages to ${filePath}. TaskId: ${taskId}`)
|
||||
|
||||
await safeWriteJson(filePath, messages)
|
||||
}
|
||||
|
|
|
|||
|
|
@ -36,7 +36,26 @@ export type SaveTaskMessagesOptions = {
|
|||
}
|
||||
|
||||
export async function saveTaskMessages({ messages, taskId, globalStoragePath }: SaveTaskMessagesOptions) {
|
||||
// Validate messages before saving to prevent data corruption
|
||||
if (!Array.isArray(messages)) {
|
||||
console.error(
|
||||
`[Roo-Debug] saveTaskMessages: Invalid messages format - expected array, got ${typeof messages}. TaskId: ${taskId}`,
|
||||
)
|
||||
throw new Error("Invalid messages format for saving")
|
||||
}
|
||||
|
||||
// Log warning for unusually large conversations
|
||||
if (messages.length > 1000) {
|
||||
console.warn(
|
||||
`[Roo-Debug] saveTaskMessages: Saving large conversation with ${messages.length} messages. TaskId: ${taskId}`,
|
||||
)
|
||||
}
|
||||
|
||||
const taskDir = await getTaskDirectoryPath(globalStoragePath, taskId)
|
||||
const filePath = path.join(taskDir, GlobalFileNames.uiMessages)
|
||||
|
||||
// Log the save operation for debugging
|
||||
console.log(`[Roo-Debug] saveTaskMessages: Saving ${messages.length} UI messages to ${filePath}. TaskId: ${taskId}`)
|
||||
|
||||
await safeWriteJson(filePath, messages)
|
||||
}
|
||||
|
|
|
|||
|
|
@ -0,0 +1,229 @@
|
|||
import { describe, it, expect, vi, beforeEach } from "vitest"
|
||||
import { webviewMessageHandler } from "../webviewMessageHandler"
|
||||
import type { ClineProvider } from "../ClineProvider"
|
||||
import type { ClineMessage } from "@roo-code/types"
|
||||
|
||||
describe("webviewMessageHandler - findMessageIndices", () => {
|
||||
let mockClineProvider: any
|
||||
let mockTask: any
|
||||
|
||||
beforeEach(() => {
|
||||
// Create mock messages with specific timestamps
|
||||
const mockMessages: ClineMessage[] = [
|
||||
{ ts: 1000, type: "say", say: "user_feedback", text: "Message 1" },
|
||||
{ ts: 1500, type: "say", say: "user_feedback", text: "Message 2" },
|
||||
{ ts: 2000, type: "say", say: "user_feedback", text: "Message 3" },
|
||||
{ ts: 2999, type: "say", say: "user_feedback", text: "Message 4" }, // Within 1 second of message 3
|
||||
{ ts: 3000, type: "say", say: "user_feedback", text: "Message 5" },
|
||||
]
|
||||
|
||||
const mockApiHistory = [
|
||||
{ ts: 1000, role: "user", content: "API Message 1" },
|
||||
{ ts: 1500, role: "assistant", content: "API Message 2" },
|
||||
{ ts: 2000, role: "user", content: "API Message 3" },
|
||||
{ ts: 2999, role: "assistant", content: "API Message 4" },
|
||||
{ ts: 3000, role: "user", content: "API Message 5" },
|
||||
]
|
||||
|
||||
mockTask = {
|
||||
taskId: "test-task-id",
|
||||
clineMessages: mockMessages,
|
||||
apiConversationHistory: mockApiHistory,
|
||||
overwriteClineMessages: vi.fn(),
|
||||
overwriteApiConversationHistory: vi.fn(),
|
||||
handleWebviewAskResponse: vi.fn(),
|
||||
}
|
||||
|
||||
mockClineProvider = {
|
||||
getCurrentTask: vi.fn(() => mockTask),
|
||||
getTaskWithId: vi.fn().mockResolvedValue({
|
||||
historyItem: { ts: Date.now(), task: "Test", tokensIn: 0, tokensOut: 0 },
|
||||
}),
|
||||
createTaskWithHistoryItem: vi.fn(),
|
||||
postMessageToWebview: vi.fn(),
|
||||
} as unknown as ClineProvider
|
||||
})
|
||||
|
||||
describe("deleteMessage with exact timestamp matching", () => {
|
||||
it("should delete only the message with exact timestamp match", async () => {
|
||||
// First, show the delete dialog
|
||||
await webviewMessageHandler(mockClineProvider, {
|
||||
type: "deleteMessage",
|
||||
value: 2000, // Delete message at timestamp 2000
|
||||
})
|
||||
|
||||
// Verify dialog was shown
|
||||
expect(mockClineProvider.postMessageToWebview).toHaveBeenCalledWith({
|
||||
type: "showDeleteMessageDialog",
|
||||
messageTs: 2000,
|
||||
})
|
||||
|
||||
// Now confirm the deletion
|
||||
await webviewMessageHandler(mockClineProvider, {
|
||||
type: "deleteMessageConfirm",
|
||||
messageTs: 2000,
|
||||
})
|
||||
|
||||
// Should delete from index 2 onwards (messages 3, 4, 5)
|
||||
expect(mockTask.overwriteClineMessages).toHaveBeenCalledWith([
|
||||
mockTask.clineMessages[0],
|
||||
mockTask.clineMessages[1],
|
||||
])
|
||||
|
||||
expect(mockTask.overwriteApiConversationHistory).toHaveBeenCalledWith([
|
||||
mockTask.apiConversationHistory[0],
|
||||
mockTask.apiConversationHistory[1],
|
||||
])
|
||||
})
|
||||
|
||||
it("should not delete messages within 1 second buffer when using exact matching", async () => {
|
||||
// Delete message at timestamp 3000
|
||||
await webviewMessageHandler(mockClineProvider, {
|
||||
type: "deleteMessage",
|
||||
value: 3000,
|
||||
})
|
||||
|
||||
await webviewMessageHandler(mockClineProvider, {
|
||||
type: "deleteMessageConfirm",
|
||||
messageTs: 3000,
|
||||
})
|
||||
|
||||
// Should delete only from index 4 (message 5), NOT from index 3 (message 4 at 2999)
|
||||
expect(mockTask.overwriteClineMessages).toHaveBeenCalledWith([
|
||||
mockTask.clineMessages[0],
|
||||
mockTask.clineMessages[1],
|
||||
mockTask.clineMessages[2],
|
||||
mockTask.clineMessages[3], // Message at 2999 should be preserved
|
||||
])
|
||||
|
||||
expect(mockTask.overwriteApiConversationHistory).toHaveBeenCalledWith([
|
||||
mockTask.apiConversationHistory[0],
|
||||
mockTask.apiConversationHistory[1],
|
||||
mockTask.apiConversationHistory[2],
|
||||
mockTask.apiConversationHistory[3], // API message at 2999 should be preserved
|
||||
])
|
||||
})
|
||||
|
||||
it("should handle case when message timestamp is not found", async () => {
|
||||
// Try to delete a message with non-existent timestamp
|
||||
await webviewMessageHandler(mockClineProvider, {
|
||||
type: "deleteMessage",
|
||||
value: 9999,
|
||||
})
|
||||
|
||||
await webviewMessageHandler(mockClineProvider, {
|
||||
type: "deleteMessageConfirm",
|
||||
messageTs: 9999,
|
||||
})
|
||||
|
||||
// Should not call overwrite methods since message wasn't found
|
||||
expect(mockTask.overwriteClineMessages).not.toHaveBeenCalled()
|
||||
expect(mockTask.overwriteApiConversationHistory).not.toHaveBeenCalled()
|
||||
expect(mockClineProvider.createTaskWithHistoryItem).not.toHaveBeenCalled()
|
||||
})
|
||||
})
|
||||
|
||||
describe("editMessage with exact timestamp matching", () => {
|
||||
it("should edit only the message with exact timestamp match", async () => {
|
||||
// First, show the edit dialog
|
||||
await webviewMessageHandler(mockClineProvider, {
|
||||
type: "submitEditedMessage",
|
||||
value: 2000,
|
||||
editedMessageContent: "Edited message content",
|
||||
})
|
||||
|
||||
// Verify dialog was shown
|
||||
expect(mockClineProvider.postMessageToWebview).toHaveBeenCalledWith({
|
||||
type: "showEditMessageDialog",
|
||||
messageTs: 2000,
|
||||
text: "Edited message content",
|
||||
images: undefined,
|
||||
})
|
||||
|
||||
// Now confirm the edit
|
||||
await webviewMessageHandler(mockClineProvider, {
|
||||
type: "editMessageConfirm",
|
||||
messageTs: 2000,
|
||||
text: "Edited message content",
|
||||
})
|
||||
|
||||
// Should delete from index 2 onwards before adding the edited message
|
||||
expect(mockTask.overwriteClineMessages).toHaveBeenCalledWith([
|
||||
mockTask.clineMessages[0],
|
||||
mockTask.clineMessages[1],
|
||||
])
|
||||
|
||||
expect(mockTask.overwriteApiConversationHistory).toHaveBeenCalledWith([
|
||||
mockTask.apiConversationHistory[0],
|
||||
mockTask.apiConversationHistory[1],
|
||||
])
|
||||
})
|
||||
|
||||
it("should not affect messages within 1 second buffer when editing", async () => {
|
||||
// Edit message at timestamp 3000
|
||||
await webviewMessageHandler(mockClineProvider, {
|
||||
type: "submitEditedMessage",
|
||||
value: 3000,
|
||||
editedMessageContent: "Edited message at 3000",
|
||||
})
|
||||
|
||||
await webviewMessageHandler(mockClineProvider, {
|
||||
type: "editMessageConfirm",
|
||||
messageTs: 3000,
|
||||
text: "Edited message at 3000",
|
||||
})
|
||||
|
||||
// Should delete only from index 4, preserving message at 2999
|
||||
expect(mockTask.overwriteClineMessages).toHaveBeenCalledWith([
|
||||
mockTask.clineMessages[0],
|
||||
mockTask.clineMessages[1],
|
||||
mockTask.clineMessages[2],
|
||||
mockTask.clineMessages[3], // Message at 2999 should be preserved
|
||||
])
|
||||
})
|
||||
})
|
||||
|
||||
describe("edge cases", () => {
|
||||
it("should handle empty message arrays gracefully", async () => {
|
||||
mockTask.clineMessages = []
|
||||
mockTask.apiConversationHistory = []
|
||||
|
||||
await webviewMessageHandler(mockClineProvider, {
|
||||
type: "deleteMessage",
|
||||
value: 1000,
|
||||
})
|
||||
|
||||
await webviewMessageHandler(mockClineProvider, {
|
||||
type: "deleteMessageConfirm",
|
||||
messageTs: 1000,
|
||||
})
|
||||
|
||||
// Should not throw errors and should not call overwrite methods
|
||||
expect(mockTask.overwriteClineMessages).not.toHaveBeenCalled()
|
||||
expect(mockTask.overwriteApiConversationHistory).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it("should handle messages with duplicate timestamps correctly", async () => {
|
||||
// Create messages with duplicate timestamps
|
||||
mockTask.clineMessages = [
|
||||
{ ts: 1000, type: "say", say: "user_feedback", text: "Message 1" },
|
||||
{ ts: 2000, type: "say", say: "user_feedback", text: "Message 2a" },
|
||||
{ ts: 2000, type: "say", say: "user_feedback", text: "Message 2b" }, // Duplicate timestamp
|
||||
{ ts: 3000, type: "say", say: "user_feedback", text: "Message 3" },
|
||||
]
|
||||
|
||||
await webviewMessageHandler(mockClineProvider, {
|
||||
type: "deleteMessage",
|
||||
value: 2000,
|
||||
})
|
||||
|
||||
await webviewMessageHandler(mockClineProvider, {
|
||||
type: "deleteMessageConfirm",
|
||||
messageTs: 2000,
|
||||
})
|
||||
|
||||
// Should delete from the first occurrence of timestamp 2000
|
||||
expect(mockTask.overwriteClineMessages).toHaveBeenCalledWith([mockTask.clineMessages[0]])
|
||||
})
|
||||
})
|
||||
})
|
||||
|
|
@ -70,10 +70,10 @@ export const webviewMessageHandler = async (
|
|||
* Shared utility to find message indices based on timestamp
|
||||
*/
|
||||
const findMessageIndices = (messageTs: number, currentCline: any) => {
|
||||
const timeCutoff = messageTs - 1000 // 1 second buffer before the message
|
||||
const messageIndex = currentCline.clineMessages.findIndex((msg: ClineMessage) => msg.ts && msg.ts >= timeCutoff)
|
||||
// Use exact timestamp matching to prevent unintended deletion of unrelated messages
|
||||
const messageIndex = currentCline.clineMessages.findIndex((msg: ClineMessage) => msg.ts === messageTs)
|
||||
const apiConversationHistoryIndex = currentCline.apiConversationHistory.findIndex(
|
||||
(msg: ApiMessage) => msg.ts && msg.ts >= timeCutoff,
|
||||
(msg: ApiMessage) => msg.ts === messageTs,
|
||||
)
|
||||
return { messageIndex, apiConversationHistoryIndex }
|
||||
}
|
||||
|
|
@ -86,6 +86,25 @@ export const webviewMessageHandler = async (
|
|||
messageIndex: number,
|
||||
apiConversationHistoryIndex: number,
|
||||
) => {
|
||||
// Validate indices before deletion to prevent accidental data loss
|
||||
if (messageIndex < 0 || messageIndex >= currentCline.clineMessages.length) {
|
||||
console.error(
|
||||
`[Chat History Protection] Invalid message index ${messageIndex} for clineMessages array of length ${currentCline.clineMessages.length}`,
|
||||
)
|
||||
throw new Error("Invalid message index for deletion")
|
||||
}
|
||||
|
||||
// Log the deletion for debugging
|
||||
const messagesToDelete = currentCline.clineMessages.length - messageIndex
|
||||
const apiMessagesToDelete =
|
||||
apiConversationHistoryIndex !== -1
|
||||
? currentCline.apiConversationHistory.length - apiConversationHistoryIndex
|
||||
: 0
|
||||
|
||||
console.log(
|
||||
`[Chat History] Deleting ${messagesToDelete} UI messages and ${apiMessagesToDelete} API messages from index ${messageIndex}`,
|
||||
)
|
||||
|
||||
// Delete this message and all that follow
|
||||
await currentCline.overwriteClineMessages(currentCline.clineMessages.slice(0, messageIndex))
|
||||
|
||||
|
|
@ -118,6 +137,11 @@ export const webviewMessageHandler = async (
|
|||
|
||||
if (messageIndex !== -1) {
|
||||
try {
|
||||
// Log the operation for debugging
|
||||
console.log(
|
||||
`[Chat History] Delete operation requested for message at timestamp ${messageTs}, found at index ${messageIndex}`,
|
||||
)
|
||||
|
||||
const { historyItem } = await provider.getTaskWithId(currentCline.taskId)
|
||||
|
||||
// Delete this message and all subsequent messages
|
||||
|
|
@ -131,6 +155,10 @@ export const webviewMessageHandler = async (
|
|||
`Error deleting message: ${error instanceof Error ? error.message : String(error)}`,
|
||||
)
|
||||
}
|
||||
} else {
|
||||
console.warn(
|
||||
`[Chat History] Message with timestamp ${messageTs} not found for deletion. Total messages: ${currentCline.clineMessages.length}`,
|
||||
)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -165,6 +193,11 @@ export const webviewMessageHandler = async (
|
|||
|
||||
if (messageIndex !== -1) {
|
||||
try {
|
||||
// Log the operation for debugging
|
||||
console.log(
|
||||
`[Chat History] Edit operation requested for message at timestamp ${messageTs}, found at index ${messageIndex}`,
|
||||
)
|
||||
|
||||
// Edit this message and delete subsequent
|
||||
await removeMessagesThisAndSubsequent(currentCline, messageIndex, apiConversationHistoryIndex)
|
||||
|
||||
|
|
@ -185,6 +218,10 @@ export const webviewMessageHandler = async (
|
|||
`Error editing message: ${error instanceof Error ? error.message : String(error)}`,
|
||||
)
|
||||
}
|
||||
} else {
|
||||
console.warn(
|
||||
`[Chat History] Message with timestamp ${messageTs} not found for editing. Total messages: ${currentCline.clineMessages.length}`,
|
||||
)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue