Skip to content

Commit cd0f0d6

Browse files
roomoteroomote
committed
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
1 parent 2a974e8 commit cd0f0d6

File tree

4 files changed

+324
-3
lines changed

4 files changed

+324
-3
lines changed

‎src/core/task-persistence/apiMessages.ts‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,43 @@ export async function saveApiMessages({
7777
taskId: string
7878
globalStoragePath: string
7979
}) {
80+
// Validate messages before saving to prevent data corruption
81+
if (!Array.isArray(messages)) {
82+
console.error(
83+
`[Roo-Debug] saveApiMessages: Invalid messages format - expected array, got ${typeof messages}. TaskId: ${taskId}`,
84+
)
85+
throw new Error("Invalid messages format for saving")
86+
}
87+
88+
// Log warning for unusually large conversations
89+
if (messages.length > 1000) {
90+
console.warn(
91+
`[Roo-Debug] saveApiMessages: Saving large conversation with ${messages.length} messages. TaskId: ${taskId}`,
92+
)
93+
}
94+
95+
// Validate message structure
96+
for (let i = 0; i < messages.length; i++) {
97+
const msg = messages[i]
98+
if (!msg || typeof msg !== "object") {
99+
console.error(
100+
`[Roo-Debug] saveApiMessages: Invalid message at index ${i} - expected object, got ${typeof msg}. TaskId: ${taskId}`,
101+
)
102+
throw new Error(`Invalid message structure at index ${i}`)
103+
}
104+
if (!msg.role || (msg.role !== "user" && msg.role !== "assistant")) {
105+
console.error(
106+
`[Roo-Debug] saveApiMessages: Invalid message role at index ${i} - got "${msg.role}". TaskId: ${taskId}`,
107+
)
108+
throw new Error(`Invalid message role at index ${i}`)
109+
}
110+
}
111+
80112
const taskDir = await getTaskDirectoryPath(globalStoragePath, taskId)
81113
const filePath = path.join(taskDir, GlobalFileNames.apiConversationHistory)
114+
115+
// Log the save operation for debugging
116+
console.log(`[Roo-Debug] saveApiMessages: Saving ${messages.length} messages to ${filePath}. TaskId: ${taskId}`)
117+
82118
await safeWriteJson(filePath, messages)
83119
}

‎src/core/task-persistence/taskMessages.ts‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,26 @@ export type SaveTaskMessagesOptions = {
3636
}
3737

3838
export async function saveTaskMessages({ messages, taskId, globalStoragePath }: SaveTaskMessagesOptions) {
39+
// Validate messages before saving to prevent data corruption
40+
if (!Array.isArray(messages)) {
41+
console.error(
42+
`[Roo-Debug] saveTaskMessages: Invalid messages format - expected array, got ${typeof messages}. TaskId: ${taskId}`,
43+
)
44+
throw new Error("Invalid messages format for saving")
45+
}
46+
47+
// Log warning for unusually large conversations
48+
if (messages.length > 1000) {
49+
console.warn(
50+
`[Roo-Debug] saveTaskMessages: Saving large conversation with ${messages.length} messages. TaskId: ${taskId}`,
51+
)
52+
}
53+
3954
const taskDir = await getTaskDirectoryPath(globalStoragePath, taskId)
4055
const filePath = path.join(taskDir, GlobalFileNames.uiMessages)
56+
57+
// Log the save operation for debugging
58+
console.log(`[Roo-Debug] saveTaskMessages: Saving ${messages.length} UI messages to ${filePath}. TaskId: ${taskId}`)
59+
4160
await safeWriteJson(filePath, messages)
4261
}

‎src/core/webview/__tests__/webviewMessageHandler.findMessageIndices.spec.ts‎

Lines changed: 229 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,229 @@
1+
import { describe, it, expect, vi, beforeEach } from "vitest"
2+
import { webviewMessageHandler } from "../webviewMessageHandler"
3+
import type { ClineProvider } from "../ClineProvider"
4+
import type { ClineMessage } from "@roo-code/types"
5+
6+
describe("webviewMessageHandler - findMessageIndices", () => {
7+
let mockClineProvider: any
8+
let mockTask: any
9+
10+
beforeEach(() => {
11+
// Create mock messages with specific timestamps
12+
const mockMessages: ClineMessage[] = [
13+
{ ts: 1000, type: "say", say: "user_feedback", text: "Message 1" },
14+
{ ts: 1500, type: "say", say: "user_feedback", text: "Message 2" },
15+
{ ts: 2000, type: "say", say: "user_feedback", text: "Message 3" },
16+
{ ts: 2999, type: "say", say: "user_feedback", text: "Message 4" }, // Within 1 second of message 3
17+
{ ts: 3000, type: "say", say: "user_feedback", text: "Message 5" },
18+
]
19+
20+
const mockApiHistory = [
21+
{ ts: 1000, role: "user", content: "API Message 1" },
22+
{ ts: 1500, role: "assistant", content: "API Message 2" },
23+
{ ts: 2000, role: "user", content: "API Message 3" },
24+
{ ts: 2999, role: "assistant", content: "API Message 4" },
25+
{ ts: 3000, role: "user", content: "API Message 5" },
26+
]
27+
28+
mockTask = {
29+
taskId: "test-task-id",
30+
clineMessages: mockMessages,
31+
apiConversationHistory: mockApiHistory,
32+
overwriteClineMessages: vi.fn(),
33+
overwriteApiConversationHistory: vi.fn(),
34+
handleWebviewAskResponse: vi.fn(),
35+
}
36+
37+
mockClineProvider = {
38+
getCurrentTask: vi.fn(() => mockTask),
39+
getTaskWithId: vi.fn().mockResolvedValue({
40+
historyItem: { ts: Date.now(), task: "Test", tokensIn: 0, tokensOut: 0 },
41+
}),
42+
createTaskWithHistoryItem: vi.fn(),
43+
postMessageToWebview: vi.fn(),
44+
} as unknown as ClineProvider
45+
})
46+
47+
describe("deleteMessage with exact timestamp matching", () => {
48+
it("should delete only the message with exact timestamp match", async () => {
49+
// First, show the delete dialog
50+
await webviewMessageHandler(mockClineProvider, {
51+
type: "deleteMessage",
52+
value: 2000, // Delete message at timestamp 2000
53+
})
54+
55+
// Verify dialog was shown
56+
expect(mockClineProvider.postMessageToWebview).toHaveBeenCalledWith({
57+
type: "showDeleteMessageDialog",
58+
messageTs: 2000,
59+
})
60+
61+
// Now confirm the deletion
62+
await webviewMessageHandler(mockClineProvider, {
63+
type: "deleteMessageConfirm",
64+
messageTs: 2000,
65+
})
66+
67+
// Should delete from index 2 onwards (messages 3, 4, 5)
68+
expect(mockTask.overwriteClineMessages).toHaveBeenCalledWith([
69+
mockTask.clineMessages[0],
70+
mockTask.clineMessages[1],
71+
])
72+
73+
expect(mockTask.overwriteApiConversationHistory).toHaveBeenCalledWith([
74+
mockTask.apiConversationHistory[0],
75+
mockTask.apiConversationHistory[1],
76+
])
77+
})
78+
79+
it("should not delete messages within 1 second buffer when using exact matching", async () => {
80+
// Delete message at timestamp 3000
81+
await webviewMessageHandler(mockClineProvider, {
82+
type: "deleteMessage",
83+
value: 3000,
84+
})
85+
86+
await webviewMessageHandler(mockClineProvider, {
87+
type: "deleteMessageConfirm",
88+
messageTs: 3000,
89+
})
90+
91+
// Should delete only from index 4 (message 5), NOT from index 3 (message 4 at 2999)
92+
expect(mockTask.overwriteClineMessages).toHaveBeenCalledWith([
93+
mockTask.clineMessages[0],
94+
mockTask.clineMessages[1],
95+
mockTask.clineMessages[2],
96+
mockTask.clineMessages[3], // Message at 2999 should be preserved
97+
])
98+
99+
expect(mockTask.overwriteApiConversationHistory).toHaveBeenCalledWith([
100+
mockTask.apiConversationHistory[0],
101+
mockTask.apiConversationHistory[1],
102+
mockTask.apiConversationHistory[2],
103+
mockTask.apiConversationHistory[3], // API message at 2999 should be preserved
104+
])
105+
})
106+
107+
it("should handle case when message timestamp is not found", async () => {
108+
// Try to delete a message with non-existent timestamp
109+
await webviewMessageHandler(mockClineProvider, {
110+
type: "deleteMessage",
111+
value: 9999,
112+
})
113+
114+
await webviewMessageHandler(mockClineProvider, {
115+
type: "deleteMessageConfirm",
116+
messageTs: 9999,
117+
})
118+
119+
// Should not call overwrite methods since message wasn't found
120+
expect(mockTask.overwriteClineMessages).not.toHaveBeenCalled()
121+
expect(mockTask.overwriteApiConversationHistory).not.toHaveBeenCalled()
122+
expect(mockClineProvider.createTaskWithHistoryItem).not.toHaveBeenCalled()
123+
})
124+
})
125+
126+
describe("editMessage with exact timestamp matching", () => {
127+
it("should edit only the message with exact timestamp match", async () => {
128+
// First, show the edit dialog
129+
await webviewMessageHandler(mockClineProvider, {
130+
type: "submitEditedMessage",
131+
value: 2000,
132+
editedMessageContent: "Edited message content",
133+
})
134+
135+
// Verify dialog was shown
136+
expect(mockClineProvider.postMessageToWebview).toHaveBeenCalledWith({
137+
type: "showEditMessageDialog",
138+
messageTs: 2000,
139+
text: "Edited message content",
140+
images: undefined,
141+
})
142+
143+
// Now confirm the edit
144+
await webviewMessageHandler(mockClineProvider, {
145+
type: "editMessageConfirm",
146+
messageTs: 2000,
147+
text: "Edited message content",
148+
})
149+
150+
// Should delete from index 2 onwards before adding the edited message
151+
expect(mockTask.overwriteClineMessages).toHaveBeenCalledWith([
152+
mockTask.clineMessages[0],
153+
mockTask.clineMessages[1],
154+
])
155+
156+
expect(mockTask.overwriteApiConversationHistory).toHaveBeenCalledWith([
157+
mockTask.apiConversationHistory[0],
158+
mockTask.apiConversationHistory[1],
159+
])
160+
})
161+
162+
it("should not affect messages within 1 second buffer when editing", async () => {
163+
// Edit message at timestamp 3000
164+
await webviewMessageHandler(mockClineProvider, {
165+
type: "submitEditedMessage",
166+
value: 3000,
167+
editedMessageContent: "Edited message at 3000",
168+
})
169+
170+
await webviewMessageHandler(mockClineProvider, {
171+
type: "editMessageConfirm",
172+
messageTs: 3000,
173+
text: "Edited message at 3000",
174+
})
175+
176+
// Should delete only from index 4, preserving message at 2999
177+
expect(mockTask.overwriteClineMessages).toHaveBeenCalledWith([
178+
mockTask.clineMessages[0],
179+
mockTask.clineMessages[1],
180+
mockTask.clineMessages[2],
181+
mockTask.clineMessages[3], // Message at 2999 should be preserved
182+
])
183+
})
184+
})
185+
186+
describe("edge cases", () => {
187+
it("should handle empty message arrays gracefully", async () => {
188+
mockTask.clineMessages = []
189+
mockTask.apiConversationHistory = []
190+
191+
await webviewMessageHandler(mockClineProvider, {
192+
type: "deleteMessage",
193+
value: 1000,
194+
})
195+
196+
await webviewMessageHandler(mockClineProvider, {
197+
type: "deleteMessageConfirm",
198+
messageTs: 1000,
199+
})
200+
201+
// Should not throw errors and should not call overwrite methods
202+
expect(mockTask.overwriteClineMessages).not.toHaveBeenCalled()
203+
expect(mockTask.overwriteApiConversationHistory).not.toHaveBeenCalled()
204+
})
205+
206+
it("should handle messages with duplicate timestamps correctly", async () => {
207+
// Create messages with duplicate timestamps
208+
mockTask.clineMessages = [
209+
{ ts: 1000, type: "say", say: "user_feedback", text: "Message 1" },
210+
{ ts: 2000, type: "say", say: "user_feedback", text: "Message 2a" },
211+
{ ts: 2000, type: "say", say: "user_feedback", text: "Message 2b" }, // Duplicate timestamp
212+
{ ts: 3000, type: "say", say: "user_feedback", text: "Message 3" },
213+
]
214+
215+
await webviewMessageHandler(mockClineProvider, {
216+
type: "deleteMessage",
217+
value: 2000,
218+
})
219+
220+
await webviewMessageHandler(mockClineProvider, {
221+
type: "deleteMessageConfirm",
222+
messageTs: 2000,
223+
})
224+
225+
// Should delete from the first occurrence of timestamp 2000
226+
expect(mockTask.overwriteClineMessages).toHaveBeenCalledWith([mockTask.clineMessages[0]])
227+
})
228+
})
229+
})

‎src/core/webview/webviewMessageHandler.ts‎

Lines changed: 40 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -70,10 +70,10 @@ export const webviewMessageHandler = async (
7070
* Shared utility to find message indices based on timestamp
7171
*/
7272
const findMessageIndices = (messageTs: number, currentCline: any) => {
73-
const timeCutoff = messageTs - 1000 // 1 second buffer before the message
74-
const messageIndex = currentCline.clineMessages.findIndex((msg: ClineMessage) => msg.ts && msg.ts >= timeCutoff)
73+
// Use exact timestamp matching to prevent unintended deletion of unrelated messages
74+
const messageIndex = currentCline.clineMessages.findIndex((msg: ClineMessage) => msg.ts === messageTs)
7575
const apiConversationHistoryIndex = currentCline.apiConversationHistory.findIndex(
76-
(msg: ApiMessage) => msg.ts && msg.ts >= timeCutoff,
76+
(msg: ApiMessage) => msg.ts === messageTs,
7777
)
7878
return { messageIndex, apiConversationHistoryIndex }
7979
}
@@ -86,6 +86,25 @@ export const webviewMessageHandler = async (
8686
messageIndex: number,
8787
apiConversationHistoryIndex: number,
8888
) => {
89+
// Validate indices before deletion to prevent accidental data loss
90+
if (messageIndex < 0 || messageIndex >= currentCline.clineMessages.length) {
91+
console.error(
92+
`[Chat History Protection] Invalid message index ${messageIndex} for clineMessages array of length ${currentCline.clineMessages.length}`,
93+
)
94+
throw new Error("Invalid message index for deletion")
95+
}
96+
97+
// Log the deletion for debugging
98+
const messagesToDelete = currentCline.clineMessages.length - messageIndex
99+
const apiMessagesToDelete =
100+
apiConversationHistoryIndex !== -1
101+
? currentCline.apiConversationHistory.length - apiConversationHistoryIndex
102+
: 0
103+
104+
console.log(
105+
`[Chat History] Deleting ${messagesToDelete} UI messages and ${apiMessagesToDelete} API messages from index ${messageIndex}`,
106+
)
107+
89108
// Delete this message and all that follow
90109
await currentCline.overwriteClineMessages(currentCline.clineMessages.slice(0, messageIndex))
91110

@@ -118,6 +137,11 @@ export const webviewMessageHandler = async (
118137

119138
if (messageIndex !== -1) {
120139
try {
140+
// Log the operation for debugging
141+
console.log(
142+
`[Chat History] Delete operation requested for message at timestamp ${messageTs}, found at index ${messageIndex}`,
143+
)
144+
121145
const { historyItem } = await provider.getTaskWithId(currentCline.taskId)
122146

123147
// Delete this message and all subsequent messages
@@ -131,6 +155,10 @@ export const webviewMessageHandler = async (
131155
`Error deleting message: ${error instanceof Error ? error.message : String(error)}`,
132156
)
133157
}
158+
} else {
159+
console.warn(
160+
`[Chat History] Message with timestamp ${messageTs} not found for deletion. Total messages: ${currentCline.clineMessages.length}`,
161+
)
134162
}
135163
}
136164
}
@@ -165,6 +193,11 @@ export const webviewMessageHandler = async (
165193

166194
if (messageIndex !== -1) {
167195
try {
196+
// Log the operation for debugging
197+
console.log(
198+
`[Chat History] Edit operation requested for message at timestamp ${messageTs}, found at index ${messageIndex}`,
199+
)
200+
168201
// Edit this message and delete subsequent
169202
await removeMessagesThisAndSubsequent(currentCline, messageIndex, apiConversationHistoryIndex)
170203

@@ -185,6 +218,10 @@ export const webviewMessageHandler = async (
185218
`Error editing message: ${error instanceof Error ? error.message : String(error)}`,
186219
)
187220
}
221+
} else {
222+
console.warn(
223+
`[Chat History] Message with timestamp ${messageTs} not found for editing. Total messages: ${currentCline.clineMessages.length}`,
224+
)
188225
}
189226
}
190227
}

0 commit comments

Comments
 (0)