Skip to content

Commit 3dfa526

Browse files
daniel-lxsdaniel-lxs
authored
Fix: Preserve first message during conversation condensing (#7910)
1 parent 33fe6fb commit 3dfa526

File tree

3 files changed

+275
-6
lines changed

3 files changed

+275
-6
lines changed

‎src/core/condense/__tests__/condense.spec.ts‎

Lines changed: 260 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,260 @@
1+
// npx vitest src/core/condense/__tests__/condense.spec.ts
2+
3+
import { Anthropic } from "@anthropic-ai/sdk"
4+
import type { ModelInfo } from "@roo-code/types"
5+
import { TelemetryService } from "@roo-code/telemetry"
6+
7+
import { BaseProvider } from "../../../api/providers/base-provider"
8+
import { ApiMessage } from "../../task-persistence/apiMessages"
9+
import { summarizeConversation, getMessagesSinceLastSummary, N_MESSAGES_TO_KEEP } from "../index"
10+
11+
// Create a mock ApiHandler for testing
12+
class MockApiHandler extends BaseProvider {
13+
createMessage(): any {
14+
// Mock implementation for testing - returns an async iterable stream
15+
const mockStream = {
16+
async *[Symbol.asyncIterator]() {
17+
yield { type: "text", text: "Mock summary of the conversation" }
18+
yield { type: "usage", inputTokens: 100, outputTokens: 50, totalCost: 0.01 }
19+
},
20+
}
21+
return mockStream
22+
}
23+
24+
getModel(): { id: string; info: ModelInfo } {
25+
return {
26+
id: "test-model",
27+
info: {
28+
contextWindow: 100000,
29+
maxTokens: 50000,
30+
supportsPromptCache: true,
31+
supportsImages: false,
32+
inputPrice: 0,
33+
outputPrice: 0,
34+
description: "Test model",
35+
},
36+
}
37+
}
38+
39+
override async countTokens(content: Array<Anthropic.Messages.ContentBlockParam>): Promise<number> {
40+
// Simple token counting for testing
41+
let tokens = 0
42+
for (const block of content) {
43+
if (block.type === "text") {
44+
tokens += Math.ceil(block.text.length / 4) // Rough approximation
45+
}
46+
}
47+
return tokens
48+
}
49+
}
50+
51+
const mockApiHandler = new MockApiHandler()
52+
const taskId = "test-task-id"
53+
54+
describe("Condense", () => {
55+
beforeEach(() => {
56+
if (!TelemetryService.hasInstance()) {
57+
TelemetryService.createInstance([])
58+
}
59+
})
60+
61+
describe("summarizeConversation", () => {
62+
it("should preserve the first message when summarizing", async () => {
63+
const messages: ApiMessage[] = [
64+
{ role: "user", content: "First message with /prr command content" },
65+
{ role: "assistant", content: "Second message" },
66+
{ role: "user", content: "Third message" },
67+
{ role: "assistant", content: "Fourth message" },
68+
{ role: "user", content: "Fifth message" },
69+
{ role: "assistant", content: "Sixth message" },
70+
{ role: "user", content: "Seventh message" },
71+
{ role: "assistant", content: "Eighth message" },
72+
{ role: "user", content: "Ninth message" },
73+
]
74+
75+
const result = await summarizeConversation(messages, mockApiHandler, "System prompt", taskId, 5000, false)
76+
77+
// Verify the first message is preserved
78+
expect(result.messages[0]).toEqual(messages[0])
79+
expect(result.messages[0].content).toBe("First message with /prr command content")
80+
81+
// Verify we have a summary message
82+
const summaryMessage = result.messages.find((msg) => msg.isSummary)
83+
expect(summaryMessage).toBeTruthy()
84+
expect(summaryMessage?.content).toBe("Mock summary of the conversation")
85+
86+
// Verify we have the expected number of messages
87+
// [first message, summary, last N messages]
88+
expect(result.messages.length).toBe(1 + 1 + N_MESSAGES_TO_KEEP)
89+
90+
// Verify the last N messages are preserved
91+
const lastMessages = result.messages.slice(-N_MESSAGES_TO_KEEP)
92+
expect(lastMessages).toEqual(messages.slice(-N_MESSAGES_TO_KEEP))
93+
})
94+
95+
it("should preserve slash command content in the first message", async () => {
96+
const slashCommandContent = "/prr #123 - Fix authentication bug"
97+
const messages: ApiMessage[] = [
98+
{ role: "user", content: slashCommandContent },
99+
{ role: "assistant", content: "I'll help you fix that authentication bug" },
100+
{ role: "user", content: "The issue is with JWT tokens" },
101+
{ role: "assistant", content: "Let me examine the JWT implementation" },
102+
{ role: "user", content: "It's failing on refresh" },
103+
{ role: "assistant", content: "I found the issue" },
104+
{ role: "user", content: "Great, can you fix it?" },
105+
{ role: "assistant", content: "Here's the fix" },
106+
{ role: "user", content: "Thanks!" },
107+
]
108+
109+
const result = await summarizeConversation(messages, mockApiHandler, "System prompt", taskId, 5000, false)
110+
111+
// The first message with slash command should be intact
112+
expect(result.messages[0].content).toBe(slashCommandContent)
113+
expect(result.messages[0]).toEqual(messages[0])
114+
})
115+
116+
it("should handle complex first message content", async () => {
117+
const complexContent: Anthropic.Messages.ContentBlockParam[] = [
118+
{ type: "text", text: "/mode code" },
119+
{ type: "text", text: "Additional context from the user" },
120+
]
121+
122+
const messages: ApiMessage[] = [
123+
{ role: "user", content: complexContent },
124+
{ role: "assistant", content: "Switching to code mode" },
125+
{ role: "user", content: "Write a function" },
126+
{ role: "assistant", content: "Here's the function" },
127+
{ role: "user", content: "Add error handling" },
128+
{ role: "assistant", content: "Added error handling" },
129+
{ role: "user", content: "Add tests" },
130+
{ role: "assistant", content: "Tests added" },
131+
{ role: "user", content: "Perfect!" },
132+
]
133+
134+
const result = await summarizeConversation(messages, mockApiHandler, "System prompt", taskId, 5000, false)
135+
136+
// The first message with complex content should be preserved
137+
expect(result.messages[0].content).toEqual(complexContent)
138+
expect(result.messages[0]).toEqual(messages[0])
139+
})
140+
141+
it("should return error when not enough messages to summarize", async () => {
142+
const messages: ApiMessage[] = [
143+
{ role: "user", content: "First message with /command" },
144+
{ role: "assistant", content: "Second message" },
145+
{ role: "user", content: "Third message" },
146+
{ role: "assistant", content: "Fourth message" },
147+
]
148+
149+
const result = await summarizeConversation(messages, mockApiHandler, "System prompt", taskId, 5000, false)
150+
151+
// Should return an error since we have only 4 messages (first + 3 to keep)
152+
expect(result.error).toBeDefined()
153+
expect(result.messages).toEqual(messages) // Original messages unchanged
154+
expect(result.summary).toBe("")
155+
})
156+
157+
it("should not summarize messages that already contain a recent summary", async () => {
158+
const messages: ApiMessage[] = [
159+
{ role: "user", content: "First message with /command" },
160+
{ role: "assistant", content: "Old message" },
161+
{ role: "user", content: "Message before summary" },
162+
{ role: "assistant", content: "Response" },
163+
{ role: "user", content: "Another message" },
164+
{ role: "assistant", content: "Previous summary", isSummary: true }, // Summary in last N messages
165+
{ role: "user", content: "Final message" },
166+
]
167+
168+
const result = await summarizeConversation(messages, mockApiHandler, "System prompt", taskId, 5000, false)
169+
170+
// Should return an error due to recent summary in last N messages
171+
expect(result.error).toBeDefined()
172+
expect(result.messages).toEqual(messages)
173+
expect(result.summary).toBe("")
174+
})
175+
176+
it("should handle empty summary from API gracefully", async () => {
177+
// Mock handler that returns empty summary
178+
class EmptyMockApiHandler extends MockApiHandler {
179+
override createMessage(): any {
180+
const mockStream = {
181+
async *[Symbol.asyncIterator]() {
182+
yield { type: "text", text: "" }
183+
yield { type: "usage", inputTokens: 100, outputTokens: 0, totalCost: 0.01 }
184+
},
185+
}
186+
return mockStream
187+
}
188+
}
189+
190+
const emptyHandler = new EmptyMockApiHandler()
191+
const messages: ApiMessage[] = [
192+
{ role: "user", content: "First message" },
193+
{ role: "assistant", content: "Second" },
194+
{ role: "user", content: "Third" },
195+
{ role: "assistant", content: "Fourth" },
196+
{ role: "user", content: "Fifth" },
197+
{ role: "assistant", content: "Sixth" },
198+
{ role: "user", content: "Seventh" },
199+
]
200+
201+
const result = await summarizeConversation(messages, emptyHandler, "System prompt", taskId, 5000, false)
202+
203+
expect(result.error).toBeDefined()
204+
expect(result.messages).toEqual(messages)
205+
expect(result.cost).toBeGreaterThan(0)
206+
})
207+
})
208+
209+
describe("getMessagesSinceLastSummary", () => {
210+
it("should return all messages when no summary exists", () => {
211+
const messages: ApiMessage[] = [
212+
{ role: "user", content: "First message" },
213+
{ role: "assistant", content: "Second message" },
214+
{ role: "user", content: "Third message" },
215+
]
216+
217+
const result = getMessagesSinceLastSummary(messages)
218+
expect(result).toEqual(messages)
219+
})
220+
221+
it("should return messages since last summary including the summary", () => {
222+
const messages: ApiMessage[] = [
223+
{ role: "user", content: "First message" },
224+
{ role: "assistant", content: "Second message" },
225+
{ role: "assistant", content: "Summary content", isSummary: true },
226+
{ role: "user", content: "Message after summary" },
227+
{ role: "assistant", content: "Final message" },
228+
]
229+
230+
const result = getMessagesSinceLastSummary(messages)
231+
232+
// Should include a user message prefix for Bedrock compatibility, the summary, and messages after
233+
expect(result[0].role).toBe("user")
234+
expect(result[0].content).toBe("Please continue from the following summary:")
235+
expect(result[1]).toEqual(messages[2]) // The summary
236+
expect(result[2]).toEqual(messages[3])
237+
expect(result[3]).toEqual(messages[4])
238+
})
239+
240+
it("should handle multiple summaries and return from the last one", () => {
241+
const messages: ApiMessage[] = [
242+
{ role: "user", content: "First message" },
243+
{ role: "assistant", content: "First summary", isSummary: true },
244+
{ role: "user", content: "Middle message" },
245+
{ role: "assistant", content: "Second summary", isSummary: true },
246+
{ role: "user", content: "Recent message" },
247+
{ role: "assistant", content: "Final message" },
248+
]
249+
250+
const result = getMessagesSinceLastSummary(messages)
251+
252+
// Should only include from the last summary
253+
expect(result[0].role).toBe("user")
254+
expect(result[0].content).toBe("Please continue from the following summary:")
255+
expect(result[1]).toEqual(messages[3]) // Second summary
256+
expect(result[2]).toEqual(messages[4])
257+
expect(result[3]).toEqual(messages[5])
258+
})
259+
})
260+
})

‎src/core/condense/__tests__/index.spec.ts‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -188,11 +188,14 @@ describe("summarizeConversation", () => {
188188
expect(maybeRemoveImageBlocks).toHaveBeenCalled()
189189

190190
// Verify the structure of the result
191-
// The result should be: original messages (except last N) + summary + last N messages
192-
expect(result.messages.length).toBe(messages.length + 1) // Original + summary
191+
// The result should be: first message + summary + last N messages
192+
expect(result.messages.length).toBe(1 + 1 + N_MESSAGES_TO_KEEP) // First + summary + last N
193+
194+
// Check that the first message is preserved
195+
expect(result.messages[0]).toEqual(messages[0])
193196

194197
// Check that the summary message was inserted correctly
195-
const summaryMessage = result.messages[result.messages.length - N_MESSAGES_TO_KEEP - 1]
198+
const summaryMessage = result.messages[1]
196199
expect(summaryMessage.role).toBe("assistant")
197200
expect(summaryMessage.content).toBe("This is a summary")
198201
expect(summaryMessage.isSummary).toBe(true)
@@ -395,7 +398,8 @@ describe("summarizeConversation", () => {
395398
)
396399

397400
// Should successfully summarize
398-
expect(result.messages.length).toBe(messages.length + 1) // Original + summary
401+
// Result should be: first message + summary + last N messages
402+
expect(result.messages.length).toBe(1 + 1 + N_MESSAGES_TO_KEEP) // First + summary + last N
399403
expect(result.cost).toBe(0.03)
400404
expect(result.summary).toBe("Concise summary")
401405
expect(result.error).toBeUndefined()

‎src/core/condense/index.ts‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,11 @@ export async function summarizeConversation(
100100
)
101101

102102
const response: SummarizeResponse = { messages, cost: 0, summary: "" }
103-
const messagesToSummarize = getMessagesSinceLastSummary(messages.slice(0, -N_MESSAGES_TO_KEEP))
103+
104+
// Always preserve the first message (which may contain slash command content)
105+
const firstMessage = messages[0]
106+
// Get messages to summarize, excluding the first message and last N messages
107+
const messagesToSummarize = getMessagesSinceLastSummary(messages.slice(1, -N_MESSAGES_TO_KEEP))
104108

105109
if (messagesToSummarize.length <= 1) {
106110
const error =
@@ -184,7 +188,8 @@ export async function summarizeConversation(
184188
isSummary: true,
185189
}
186190

187-
const newMessages = [...messages.slice(0, -N_MESSAGES_TO_KEEP), summaryMessage, ...keepMessages]
191+
// Reconstruct messages: [first message, summary, last N messages]
192+
const newMessages = [firstMessage, summaryMessage, ...keepMessages]
188193

189194
// Count the tokens in the context for the next API request
190195
// We only estimate the tokens in summaryMesage if outputTokens is 0, otherwise we use outputTokens

0 commit comments

Comments
 (0)