Skip to content

Commit 9a16603

Browse files
committed
fix: prevent nested condensing from including previously-condensed content (#10942)
The Bedrock fix in getMessagesSinceLastSummary prepends messages[0] when the summary has assistant role, but it didn't check if messages[0] had a condenseParent (was already condensed). This caused previously-condensed content to incorrectly appear in the active context during nested condense operations. Added check for condenseParent on originalFirstMessage before prepending it. When the original message was already condensed, we now create a synthetic user message instead to maintain proper alternation.
1 parent 8834ffe commit 9a16603

File tree

2 files changed

+379
-0
lines changed

2 files changed

+379
-0
lines changed
Lines changed: 364 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,364 @@
1+
import { describe, it, expect } from "vitest"
2+
import { ApiMessage } from "../../task-persistence/apiMessages"
3+
import { getEffectiveApiHistory, getMessagesSinceLastSummary } from "../index"
4+
5+
describe("nested condensing scenarios", () => {
6+
describe("fresh-start model (user-role summaries)", () => {
7+
it("should return only the latest summary and messages after it", () => {
8+
const condenseId1 = "condense-1"
9+
const condenseId2 = "condense-2"
10+
11+
// Simulate history after two nested condenses with user-role summaries
12+
const history: ApiMessage[] = [
13+
// Original task - condensed in first condense
14+
{ role: "user", content: "Build an app", ts: 100, condenseParent: condenseId1 },
15+
// Messages from first condense
16+
{ role: "assistant", content: "Starting...", ts: 200, condenseParent: condenseId1 },
17+
{ role: "user", content: "Add auth", ts: 300, condenseParent: condenseId1 },
18+
// First summary (user role, fresh-start model) - then condensed in second condense
19+
{
20+
role: "user",
21+
content: [{ type: "text", text: "## Summary 1" }],
22+
ts: 399,
23+
isSummary: true,
24+
condenseId: condenseId1,
25+
condenseParent: condenseId2, // Tagged during second condense
26+
},
27+
// Messages after first condense but before second
28+
{ role: "assistant", content: "Auth added", ts: 400, condenseParent: condenseId2 },
29+
{ role: "user", content: "Add database", ts: 500, condenseParent: condenseId2 },
30+
// Second summary (user role, fresh-start model)
31+
{
32+
role: "user",
33+
content: [{ type: "text", text: "## Summary 2" }],
34+
ts: 599,
35+
isSummary: true,
36+
condenseId: condenseId2,
37+
},
38+
// Messages after second condense (kept messages)
39+
{ role: "assistant", content: "Database added", ts: 600 },
40+
{ role: "user", content: "Now test it", ts: 700 },
41+
]
42+
43+
// Step 1: Get effective history
44+
const effectiveHistory = getEffectiveApiHistory(history)
45+
46+
// Should only contain: Summary2, and messages after it
47+
expect(effectiveHistory.length).toBe(3)
48+
expect(effectiveHistory[0].isSummary).toBe(true)
49+
expect(effectiveHistory[0].condenseId).toBe(condenseId2) // Latest summary
50+
expect(effectiveHistory[1].content).toBe("Database added")
51+
expect(effectiveHistory[2].content).toBe("Now test it")
52+
53+
// Verify NO condensed messages are included
54+
const hasCondensedMessages = effectiveHistory.some(
55+
(msg) => msg.condenseParent && history.some((m) => m.isSummary && m.condenseId === msg.condenseParent),
56+
)
57+
expect(hasCondensedMessages).toBe(false)
58+
59+
// Step 2: Get messages since last summary (on effective history)
60+
const messagesSinceLastSummary = getMessagesSinceLastSummary(effectiveHistory)
61+
62+
// Should be the same as effective history since Summary2 is already at the start
63+
expect(messagesSinceLastSummary.length).toBe(3)
64+
expect(messagesSinceLastSummary[0].isSummary).toBe(true)
65+
expect(messagesSinceLastSummary[0].condenseId).toBe(condenseId2)
66+
67+
// CRITICAL: No previous history (Summary1 or original task) should be included
68+
const hasSummary1 = messagesSinceLastSummary.some((m) => m.condenseId === condenseId1)
69+
expect(hasSummary1).toBe(false)
70+
71+
const hasOriginalTask = messagesSinceLastSummary.some((m) => m.content === "Build an app")
72+
expect(hasOriginalTask).toBe(false)
73+
})
74+
75+
it("should handle triple nested condense correctly", () => {
76+
const condenseId1 = "condense-1"
77+
const condenseId2 = "condense-2"
78+
const condenseId3 = "condense-3"
79+
80+
const history: ApiMessage[] = [
81+
// First condense content
82+
{ role: "user", content: "Task", ts: 100, condenseParent: condenseId1 },
83+
{
84+
role: "user",
85+
content: [{ type: "text", text: "## Summary 1" }],
86+
ts: 199,
87+
isSummary: true,
88+
condenseId: condenseId1,
89+
condenseParent: condenseId2,
90+
},
91+
// Second condense content
92+
{ role: "assistant", content: "After S1", ts: 200, condenseParent: condenseId2 },
93+
{
94+
role: "user",
95+
content: [{ type: "text", text: "## Summary 2" }],
96+
ts: 299,
97+
isSummary: true,
98+
condenseId: condenseId2,
99+
condenseParent: condenseId3,
100+
},
101+
// Third condense content
102+
{ role: "assistant", content: "After S2", ts: 300, condenseParent: condenseId3 },
103+
{
104+
role: "user",
105+
content: [{ type: "text", text: "## Summary 3" }],
106+
ts: 399,
107+
isSummary: true,
108+
condenseId: condenseId3,
109+
},
110+
// Current messages
111+
{ role: "assistant", content: "Current work", ts: 400 },
112+
]
113+
114+
const effectiveHistory = getEffectiveApiHistory(history)
115+
116+
// Should only contain Summary3 and current work
117+
expect(effectiveHistory.length).toBe(2)
118+
expect(effectiveHistory[0].condenseId).toBe(condenseId3)
119+
expect(effectiveHistory[1].content).toBe("Current work")
120+
121+
const messagesSinceLastSummary = getMessagesSinceLastSummary(effectiveHistory)
122+
expect(messagesSinceLastSummary.length).toBe(2)
123+
124+
// No previous summaries should be included
125+
const hasPreviousSummaries = messagesSinceLastSummary.some(
126+
(m) => m.condenseId === condenseId1 || m.condenseId === condenseId2,
127+
)
128+
expect(hasPreviousSummaries).toBe(false)
129+
})
130+
})
131+
132+
describe("legacy assistant-role summaries (Bedrock fix scenario)", () => {
133+
it("should NOT duplicate the summary when summary is assistant role", () => {
134+
const condenseId = "condense-1"
135+
136+
const history: ApiMessage[] = [
137+
{ role: "user", content: "Task", ts: 100, condenseParent: condenseId },
138+
{ role: "assistant", content: "Response", ts: 200, condenseParent: condenseId },
139+
// Legacy summary with assistant role
140+
{
141+
role: "assistant",
142+
content: "Summary of work",
143+
ts: 299,
144+
isSummary: true,
145+
condenseId,
146+
},
147+
{ role: "user", content: "Continue", ts: 300 },
148+
]
149+
150+
const effectiveHistory = getEffectiveApiHistory(history)
151+
expect(effectiveHistory.length).toBe(2)
152+
expect(effectiveHistory[0].isSummary).toBe(true)
153+
154+
const messagesSinceLastSummary = getMessagesSinceLastSummary(effectiveHistory)
155+
156+
// The Bedrock fix might trigger, but it should NOT create duplicates
157+
// when the input is already the effective history
158+
const summaryCount = messagesSinceLastSummary.filter((m) => m.isSummary).length
159+
expect(summaryCount).toBe(1) // Only one summary, not duplicated
160+
161+
// Should have summary + "Continue"
162+
expect(messagesSinceLastSummary.length).toBeLessThanOrEqual(3)
163+
})
164+
165+
it("should NOT include original task when called on effective history", () => {
166+
const condenseId = "condense-1"
167+
168+
const history: ApiMessage[] = [
169+
{ role: "user", content: "Original task content", ts: 100, condenseParent: condenseId },
170+
{
171+
role: "assistant",
172+
content: "Legacy summary",
173+
ts: 199,
174+
isSummary: true,
175+
condenseId,
176+
},
177+
{ role: "user", content: "After summary", ts: 200 },
178+
]
179+
180+
const effectiveHistory = getEffectiveApiHistory(history)
181+
const messagesSinceLastSummary = getMessagesSinceLastSummary(effectiveHistory)
182+
183+
// The original task should NOT be in the result
184+
const hasOriginalTask = messagesSinceLastSummary.some((m) => m.content === "Original task content")
185+
expect(hasOriginalTask).toBe(false)
186+
})
187+
188+
describe("BUG: getMessagesSinceLastSummary with full history (summarization input)", () => {
189+
it("should NOT include original task in summarization input when summary is assistant role", () => {
190+
const condenseId = "condense-1"
191+
192+
// Scenario: First condense created an assistant-role summary (legacy)
193+
// Now we're doing a second condense
194+
const fullHistory: ApiMessage[] = [
195+
// Original task - was condensed in first condense
196+
{
197+
role: "user",
198+
content: "Original task that should NOT be in summarization input",
199+
ts: 100,
200+
condenseParent: condenseId,
201+
},
202+
{ role: "assistant", content: "Old response", ts: 200, condenseParent: condenseId },
203+
// Legacy assistant-role summary from first condense
204+
{
205+
role: "assistant", // <-- Legacy: assistant role
206+
content: "First summary",
207+
ts: 299,
208+
isSummary: true,
209+
condenseId,
210+
},
211+
// New messages to be summarized in second condense
212+
{ role: "user", content: "Message after summary", ts: 300 },
213+
{ role: "assistant", content: "Response after summary", ts: 400 },
214+
]
215+
216+
// This simulates what summarizeConversation does when called for manual condense
217+
const messagesToSummarize = getMessagesSinceLastSummary(fullHistory)
218+
219+
// THE BUG: Bedrock fix prepends messages[0] (original task) when summary is assistant role
220+
// This is wrong because:
221+
// 1. The original task was already condensed (has condenseParent)
222+
// 2. It should not be included in the summarization input for the second condense
223+
224+
// Check if original task is incorrectly included
225+
const hasOriginalTask = messagesToSummarize.some(
226+
(m) => typeof m.content === "string" && m.content.includes("Original task"),
227+
)
228+
229+
// This test documents the current BUGGY behavior if it fails
230+
// The fix should make this pass by NOT including the original task
231+
console.log(
232+
"Messages to summarize:",
233+
messagesToSummarize.map((m) => ({
234+
role: m.role,
235+
content: typeof m.content === "string" ? m.content.substring(0, 50) : "[array]",
236+
condenseParent: m.condenseParent,
237+
isSummary: m.isSummary,
238+
})),
239+
)
240+
241+
// EXPECTED: Original task should NOT be included
242+
// ACTUAL (if bug exists): Original task IS included due to Bedrock fix
243+
expect(hasOriginalTask).toBe(false)
244+
})
245+
246+
it("should NOT include condensed messages when preparing summarization input", () => {
247+
const condenseId1 = "condense-1"
248+
249+
const fullHistory: ApiMessage[] = [
250+
// Original condensed messages
251+
{ role: "user", content: "Condensed task", ts: 100, condenseParent: condenseId1 },
252+
{ role: "assistant", content: "Condensed response", ts: 200, condenseParent: condenseId1 },
253+
// First summary (assistant role for legacy)
254+
{
255+
role: "assistant",
256+
content: "Summary of first condense",
257+
ts: 299,
258+
isSummary: true,
259+
condenseId: condenseId1,
260+
},
261+
// Messages to be summarized
262+
{ role: "user", content: "New work", ts: 300 },
263+
{ role: "assistant", content: "New response", ts: 400 },
264+
]
265+
266+
const messagesToSummarize = getMessagesSinceLastSummary(fullHistory)
267+
268+
// Count how many messages with condenseParent are in the result
269+
const condensedMessagesInResult = messagesToSummarize.filter(
270+
(m) => m.condenseParent && m.condenseParent === condenseId1 && !m.isSummary,
271+
)
272+
273+
console.log("Condensed messages in result:", condensedMessagesInResult.length)
274+
275+
// No condensed messages (other than the summary which kicks off the new input) should be included
276+
expect(condensedMessagesInResult.length).toBe(0)
277+
})
278+
})
279+
})
280+
281+
describe("getMessagesSinceLastSummary behavior with full vs effective history", () => {
282+
it("should behave differently when called with full history vs effective history", () => {
283+
const condenseId = "condense-1"
284+
285+
const fullHistory: ApiMessage[] = [
286+
{ role: "user", content: "Original task", ts: 100, condenseParent: condenseId },
287+
{ role: "assistant", content: "Response", ts: 200, condenseParent: condenseId },
288+
{
289+
role: "user",
290+
content: [{ type: "text", text: "Summary" }],
291+
ts: 299,
292+
isSummary: true,
293+
condenseId,
294+
},
295+
{ role: "assistant", content: "After summary", ts: 300 },
296+
]
297+
298+
// Called with FULL history (as in summarizeConversation)
299+
const fromFullHistory = getMessagesSinceLastSummary(fullHistory)
300+
301+
// Called with EFFECTIVE history (as in attemptApiRequest)
302+
const effectiveHistory = getEffectiveApiHistory(fullHistory)
303+
const fromEffectiveHistory = getMessagesSinceLastSummary(effectiveHistory)
304+
305+
// Both should return the same messages when summary is user role
306+
expect(fromFullHistory.length).toBe(fromEffectiveHistory.length)
307+
308+
// The key difference: fromFullHistory[0] references fullHistory,
309+
// while fromEffectiveHistory[0] references effectiveHistory
310+
// With user-role summary, Bedrock fix should NOT trigger in either case
311+
expect(fromFullHistory[0].isSummary).toBe(true)
312+
expect(fromEffectiveHistory[0].isSummary).toBe(true)
313+
})
314+
315+
it("BUG SCENARIO: Bedrock fix should not include condensed original task", () => {
316+
const condenseId1 = "condense-1"
317+
const condenseId2 = "condense-2"
318+
319+
// Scenario: Two condenses, first summary is assistant role (legacy)
320+
const fullHistory: ApiMessage[] = [
321+
{ role: "user", content: "Original task - should NOT appear", ts: 100, condenseParent: condenseId1 },
322+
{ role: "assistant", content: "Old response", ts: 200, condenseParent: condenseId1 },
323+
// Legacy assistant-role summary, then condensed again
324+
{
325+
role: "assistant",
326+
content: "Summary 1",
327+
ts: 299,
328+
isSummary: true,
329+
condenseId: condenseId1,
330+
condenseParent: condenseId2,
331+
},
332+
{ role: "user", content: "After S1", ts: 300, condenseParent: condenseId2 },
333+
// Second summary (still assistant for legacy consistency in this test)
334+
{
335+
role: "assistant",
336+
content: "Summary 2",
337+
ts: 399,
338+
isSummary: true,
339+
condenseId: condenseId2,
340+
},
341+
{ role: "user", content: "Current message", ts: 400 },
342+
]
343+
344+
const effectiveHistory = getEffectiveApiHistory(fullHistory)
345+
expect(effectiveHistory.length).toBe(2) // Summary2 + Current message
346+
347+
const messagesSinceLastSummary = getMessagesSinceLastSummary(effectiveHistory)
348+
349+
// CRITICAL BUG CHECK: The original task should NEVER be included
350+
const hasOriginalTask = messagesSinceLastSummary.some((m) =>
351+
typeof m.content === "string"
352+
? m.content.includes("Original task")
353+
: JSON.stringify(m.content).includes("Original task"),
354+
)
355+
356+
// This assertion documents the expected behavior
357+
expect(hasOriginalTask).toBe(false)
358+
359+
// Also verify Summary1 is not included
360+
const hasSummary1 = messagesSinceLastSummary.some((m) => m.condenseId === condenseId1)
361+
expect(hasSummary1).toBe(false)
362+
})
363+
})
364+
})

src/core/condense/index.ts

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -427,6 +427,21 @@ export function getMessagesSinceLastSummary(messages: ApiMessage[]): ApiMessage[
427427
// Get the original first message (should always be a user message with the task)
428428
const originalFirstMessage = messages[0]
429429
if (originalFirstMessage && originalFirstMessage.role === "user") {
430+
// BUG FIX: Don't prepend the original first message if it has a condenseParent.
431+
// This can happen during nested condensing where the original task was already
432+
// condensed in a previous condense operation. Including it would incorrectly
433+
// add previously-condensed content to the summarization input.
434+
// See: https://github.com/RooCodeInc/Roo-Code/issues/10942 (nested condensing bug)
435+
if (originalFirstMessage.condenseParent) {
436+
// The original first message was already condensed, so we should NOT include it.
437+
// Instead, use a generic placeholder message to satisfy the Bedrock requirement.
438+
const userMessage: ApiMessage = {
439+
role: "user",
440+
content: "Please continue from the following summary:",
441+
ts: messages[0]?.ts ? messages[0].ts - 1 : Date.now(),
442+
}
443+
return [userMessage, ...messagesSinceSummary]
444+
}
430445
// Use the original first message unchanged to maintain full context
431446
return [originalFirstMessage, ...messagesSinceSummary]
432447
} else {

0 commit comments

Comments
 (0)