Skip to content

Commit 78828e7

Browse files
committed
fix: implement non-destructive condense with metadata tracking
- Add condenseId and condenseParent fields to ApiMessage type - Update condense logic to preserve all messages with metadata tags - Filter condensed messages during API assembly based on active summaries - Implement hygiene operations to clean orphaned references after truncation - Add comprehensive tests for non-destructive condense behavior - Update existing tests to match new behavior This fixes the issue where manual condense was breaking delete/edit operations by replacing persisted history, causing timestamp-based lookups to fail. Fixes #8295
1 parent 97f9686 commit 78828e7

File tree

8 files changed

+409
-20
lines changed

8 files changed

+409
-20
lines changed

packages/types/src/message.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -225,6 +225,8 @@ export const clineMessageSchema = z.object({
225225
reasoning_summary: z.string().optional(),
226226
})
227227
.optional(),
228+
condenseId: z.string().optional(),
229+
condenseParent: z.string().optional(),
228230
})
229231
.optional(),
230232
})

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

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -84,8 +84,9 @@ describe("Condense", () => {
8484
expect(summaryMessage?.content).toBe("Mock summary of the conversation")
8585

8686
// 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)
87+
// With non-destructive condense, we keep ALL messages plus the summary
88+
// [first message, middle messages (tagged), summary, last N messages]
89+
expect(result.messages.length).toBe(messages.length + 1) // All original messages + 1 summary
8990

9091
// Verify the last N messages are preserved
9192
const lastMessages = result.messages.slice(-N_MESSAGES_TO_KEEP)

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

Lines changed: 24 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -188,21 +188,33 @@ describe("summarizeConversation", () => {
188188
expect(maybeRemoveImageBlocks).toHaveBeenCalled()
189189

190190
// Verify the structure of the result
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
191+
// With non-destructive condense, all original messages are preserved plus the summary
192+
expect(result.messages.length).toBe(messages.length + 1) // All original messages + summary
193193

194194
// Check that the first message is preserved
195195
expect(result.messages[0]).toEqual(messages[0])
196196

197-
// Check that the summary message was inserted correctly
198-
const summaryMessage = result.messages[1]
199-
expect(summaryMessage.role).toBe("assistant")
200-
expect(summaryMessage.content).toBe("This is a summary")
201-
expect(summaryMessage.isSummary).toBe(true)
197+
// Find the summary message (it should be inserted after the first message and before tail)
198+
const summaryMessage = result.messages.find((m) => m.isSummary)
199+
expect(summaryMessage).toBeDefined()
200+
expect(summaryMessage?.role).toBe("assistant")
201+
expect(summaryMessage?.content).toBe("This is a summary")
202+
expect(summaryMessage?.isSummary).toBe(true)
203+
expect(summaryMessage?.condenseId).toBeDefined()
204+
205+
// Check that middle messages are tagged with condenseParent
206+
const middleMessages = result.messages.slice(1, messages.length - N_MESSAGES_TO_KEEP + 1)
207+
middleMessages.forEach((msg) => {
208+
if (!msg.isSummary) {
209+
expect(msg.condenseParent).toBe(summaryMessage?.condenseId)
210+
}
211+
})
202212

203-
// Check that the last N_MESSAGES_TO_KEEP messages are preserved
204-
const lastMessages = messages.slice(-N_MESSAGES_TO_KEEP)
205-
expect(result.messages.slice(-N_MESSAGES_TO_KEEP)).toEqual(lastMessages)
213+
// Check that the last N_MESSAGES_TO_KEEP messages are preserved without condenseParent
214+
const tailMessages = result.messages.slice(-N_MESSAGES_TO_KEEP)
215+
tailMessages.forEach((msg) => {
216+
expect(msg.condenseParent).toBeUndefined()
217+
})
206218

207219
// Check the cost and token counts
208220
expect(result.cost).toBe(0.05)
@@ -424,8 +436,8 @@ describe("summarizeConversation", () => {
424436
)
425437

426438
// Should successfully summarize
427-
// Result should be: first message + summary + last N messages
428-
expect(result.messages.length).toBe(1 + 1 + N_MESSAGES_TO_KEEP) // First + summary + last N
439+
// With non-destructive condense, all original messages are preserved plus the summary
440+
expect(result.messages.length).toBe(messages.length + 1) // All original messages + summary
429441
expect(result.cost).toBe(0.03)
430442
expect(result.summary).toBe("Concise summary")
431443
expect(result.error).toBeUndefined()
Lines changed: 281 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,281 @@
1+
// npx vitest src/core/condense/__tests__/non-destructive-condense.spec.ts
2+
3+
import { describe, it, expect, beforeEach, vi } from "vitest"
4+
import { summarizeConversation } from "../index"
5+
import { ApiMessage } from "../../task-persistence/apiMessages"
6+
import { ApiHandler } from "../../../api"
7+
8+
// Mock the translation function
9+
vi.mock("../../../i18n", () => ({
10+
t: (key: string) => key,
11+
}))
12+
13+
// Mock TelemetryService
14+
vi.mock("@roo-code/telemetry", () => ({
15+
TelemetryService: {
16+
instance: {
17+
captureContextCondensed: vi.fn(),
18+
},
19+
},
20+
}))
21+
22+
describe("Non-destructive condense", () => {
23+
let mockApiHandler: ApiHandler
24+
let messages: ApiMessage[]
25+
26+
beforeEach(() => {
27+
// Create a mock API handler
28+
mockApiHandler = {
29+
createMessage: vi.fn().mockImplementation(() => {
30+
// Return an async generator that yields a summary
31+
return (async function* () {
32+
yield { type: "text", text: "This is a summary of the conversation" }
33+
yield { type: "usage", totalCost: 0.01, outputTokens: 50 }
34+
})()
35+
}),
36+
countTokens: vi.fn().mockResolvedValue(100),
37+
getModel: vi.fn().mockReturnValue({ info: {} }),
38+
} as any
39+
40+
// Create test messages
41+
messages = [
42+
{ role: "user", content: "First message", ts: 1000 },
43+
{ role: "assistant", content: "Response 1", ts: 2000 },
44+
{ role: "user", content: "Second message", ts: 3000 },
45+
{ role: "assistant", content: "Response 2", ts: 4000 },
46+
{ role: "user", content: "Third message", ts: 5000 },
47+
{ role: "assistant", content: "Response 3", ts: 6000 },
48+
{ role: "user", content: "Fourth message", ts: 7000 },
49+
{ role: "assistant", content: "Response 4", ts: 8000 },
50+
]
51+
})
52+
53+
describe("summarizeConversation", () => {
54+
it("should preserve all messages with condenseParent tags", async () => {
55+
const result = await summarizeConversation(
56+
messages,
57+
mockApiHandler,
58+
"system prompt",
59+
"task-123",
60+
1000, // prevContextTokens
61+
false,
62+
)
63+
64+
// Should not have an error
65+
expect(result.error).toBeUndefined()
66+
67+
// Should have more messages than before (all original + summary)
68+
expect(result.messages.length).toBeGreaterThan(messages.length)
69+
70+
// First message should be preserved
71+
expect(result.messages[0]).toEqual(messages[0])
72+
73+
// Should have a summary message with condenseId
74+
const summaryMessage = result.messages.find((m) => m.isSummary)
75+
expect(summaryMessage).toBeDefined()
76+
expect(summaryMessage?.condenseId).toBeDefined()
77+
expect(summaryMessage?.condenseId).toMatch(/^condense-\d+-[a-z0-9]+$/)
78+
79+
// Middle messages should have condenseParent
80+
const middleMessages = result.messages.filter((m) => m.condenseParent)
81+
expect(middleMessages.length).toBeGreaterThan(0)
82+
expect(middleMessages.every((m) => m.condenseParent === summaryMessage?.condenseId)).toBe(true)
83+
84+
// Last N messages should not have condenseParent
85+
const tailMessages = result.messages.slice(-3) // N_MESSAGES_TO_KEEP = 3
86+
expect(tailMessages.every((m) => !m.condenseParent)).toBe(true)
87+
})
88+
89+
it("should generate unique condenseId for each condensation", async () => {
90+
const result1 = await summarizeConversation(
91+
messages,
92+
mockApiHandler,
93+
"system prompt",
94+
"task-123",
95+
1000,
96+
false,
97+
)
98+
99+
const result2 = await summarizeConversation(
100+
messages,
101+
mockApiHandler,
102+
"system prompt",
103+
"task-123",
104+
1000,
105+
false,
106+
)
107+
108+
const summaryMessage1 = result1.messages.find((m) => m.isSummary)
109+
const summaryMessage2 = result2.messages.find((m) => m.isSummary)
110+
111+
expect(summaryMessage1?.condenseId).toBeDefined()
112+
expect(summaryMessage2?.condenseId).toBeDefined()
113+
expect(summaryMessage1?.condenseId).not.toEqual(summaryMessage2?.condenseId)
114+
})
115+
116+
it("should not condense if not enough messages", async () => {
117+
const shortMessages = messages.slice(0, 3)
118+
const result = await summarizeConversation(
119+
shortMessages,
120+
mockApiHandler,
121+
"system prompt",
122+
"task-123",
123+
1000,
124+
false,
125+
)
126+
127+
expect(result.error).toBe("common:errors.condense_not_enough_messages")
128+
expect(result.messages).toEqual(shortMessages)
129+
})
130+
131+
it("should not condense if recent summary exists", async () => {
132+
const messagesWithSummary: ApiMessage[] = [
133+
...messages.slice(0, -2),
134+
{ role: "assistant" as const, content: "Previous summary", ts: 6500, isSummary: true },
135+
...messages.slice(-2),
136+
]
137+
138+
const result = await summarizeConversation(
139+
messagesWithSummary,
140+
mockApiHandler,
141+
"system prompt",
142+
"task-123",
143+
1000,
144+
false,
145+
)
146+
147+
expect(result.error).toBe("common:errors.condensed_recently")
148+
expect(result.messages).toEqual(messagesWithSummary)
149+
})
150+
})
151+
152+
describe("Message filtering with active summaries", () => {
153+
it("should filter out messages with condenseParent matching active summary", () => {
154+
const messagesWithCondense: ApiMessage[] = [
155+
{ role: "user", content: "First", ts: 1000 },
156+
{ role: "user", content: "Second", ts: 2000, condenseParent: "condense-123-abc" },
157+
{ role: "assistant", content: "Response", ts: 3000, condenseParent: "condense-123-abc" },
158+
{
159+
role: "assistant",
160+
content: "Summary",
161+
ts: 4000,
162+
isSummary: true,
163+
condenseId: "condense-123-abc",
164+
},
165+
{ role: "user", content: "Latest", ts: 5000 },
166+
]
167+
168+
// Simulate filtering logic from Task.attemptApiRequest
169+
const activeCondenseIds = new Set(
170+
messagesWithCondense.filter((m) => m.isSummary && m.condenseId).map((m) => m.condenseId!),
171+
)
172+
173+
const effectiveHistory = messagesWithCondense.filter(
174+
(m) => !m.condenseParent || !activeCondenseIds.has(m.condenseParent),
175+
)
176+
177+
// Should filter out the middle messages with condenseParent
178+
expect(effectiveHistory.length).toBe(3)
179+
expect(effectiveHistory[0].content).toBe("First")
180+
expect(effectiveHistory[1].content).toBe("Summary")
181+
expect(effectiveHistory[2].content).toBe("Latest")
182+
})
183+
184+
it("should include messages with orphaned condenseParent", () => {
185+
const messagesWithOrphan: ApiMessage[] = [
186+
{ role: "user", content: "First", ts: 1000 },
187+
{ role: "user", content: "Second", ts: 2000, condenseParent: "condense-old-xyz" }, // Orphaned
188+
{ role: "assistant", content: "Response", ts: 3000 },
189+
]
190+
191+
// No active summaries
192+
const activeCondenseIds = new Set(
193+
messagesWithOrphan.filter((m) => m.isSummary && m.condenseId).map((m) => m.condenseId!),
194+
)
195+
196+
const effectiveHistory = messagesWithOrphan.filter(
197+
(m) => !m.condenseParent || !activeCondenseIds.has(m.condenseParent),
198+
)
199+
200+
// Should include the orphaned message since its condenseParent doesn't match any active summary
201+
expect(effectiveHistory.length).toBe(3)
202+
})
203+
})
204+
205+
describe("Nested condense support", () => {
206+
it("should handle multiple condensations with different condenseIds", async () => {
207+
// First condensation
208+
const result1 = await summarizeConversation(
209+
messages,
210+
mockApiHandler,
211+
"system prompt",
212+
"task-123",
213+
1000,
214+
false,
215+
)
216+
217+
// Add more messages
218+
const extendedMessages: ApiMessage[] = [
219+
...result1.messages,
220+
{ role: "user" as const, content: "Fifth message", ts: 9000 },
221+
{ role: "assistant" as const, content: "Response 5", ts: 10000 },
222+
{ role: "user" as const, content: "Sixth message", ts: 11000 },
223+
{ role: "assistant" as const, content: "Response 6", ts: 12000 },
224+
]
225+
226+
// Second condensation
227+
const result2 = await summarizeConversation(
228+
extendedMessages,
229+
mockApiHandler,
230+
"system prompt",
231+
"task-123",
232+
1000,
233+
false,
234+
)
235+
236+
// Should have two different summaries with different condenseIds
237+
const summaries = result2.messages.filter((m) => m.isSummary)
238+
expect(summaries.length).toBeGreaterThanOrEqual(1)
239+
240+
// Messages should have different condenseParent values
241+
const condenseParents = new Set(
242+
result2.messages.filter((m) => m.condenseParent).map((m) => m.condenseParent),
243+
)
244+
expect(condenseParents.size).toBeGreaterThanOrEqual(1)
245+
})
246+
})
247+
248+
describe("Rollback behavior", () => {
249+
it("should support rollback by removing summary and cleaning condenseParent", () => {
250+
const messagesWithCondense: ApiMessage[] = [
251+
{ role: "user", content: "First", ts: 1000 },
252+
{ role: "user", content: "Second", ts: 2000, condenseParent: "condense-123-abc" },
253+
{ role: "assistant", content: "Response", ts: 3000, condenseParent: "condense-123-abc" },
254+
{
255+
role: "assistant",
256+
content: "Summary",
257+
ts: 4000,
258+
isSummary: true,
259+
condenseId: "condense-123-abc",
260+
},
261+
{ role: "user", content: "Latest", ts: 5000 },
262+
]
263+
264+
// Simulate rollback: remove summary
265+
const afterRollback = messagesWithCondense.filter((m) => !m.isSummary)
266+
267+
// Simulate hygiene: clean orphaned condenseParent
268+
const activeCondenseIds = new Set(afterRollback.filter((m) => m.condenseId).map((m) => m.condenseId!))
269+
270+
afterRollback.forEach((m) => {
271+
if (m.condenseParent && !activeCondenseIds.has(m.condenseParent)) {
272+
delete m.condenseParent
273+
}
274+
})
275+
276+
// All messages should have condenseParent removed
277+
expect(afterRollback.every((m) => !m.condenseParent)).toBe(true)
278+
expect(afterRollback.length).toBe(4)
279+
})
280+
})
281+
})

src/core/condense/index.ts

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -181,15 +181,28 @@ export async function summarizeConversation(
181181
return { ...response, cost, error }
182182
}
183183

184+
// Generate a unique condenseId for this condensation
185+
const condenseId = `condense-${Date.now()}-${Math.random().toString(36).substr(2, 9)}`
186+
187+
// Create the summary message with condenseId
184188
const summaryMessage: ApiMessage = {
185189
role: "assistant",
186190
content: summary,
187191
ts: keepMessages[0].ts,
188192
isSummary: true,
193+
condenseId: condenseId,
189194
}
190195

191-
// Reconstruct messages: [first message, summary, last N messages]
192-
const newMessages = [firstMessage, summaryMessage, ...keepMessages]
196+
// Tag all middle messages (between first and tail) with condenseParent
197+
// Middle messages are those that were summarized but not kept
198+
const middleMessages = messages.slice(1, -N_MESSAGES_TO_KEEP).map((msg) => ({
199+
...msg,
200+
condenseParent: condenseId,
201+
}))
202+
203+
// Reconstruct messages: [first message, tagged middle messages, summary, last N messages]
204+
// This preserves ALL messages, with middle ones tagged for filtering
205+
const newMessages = [firstMessage, ...middleMessages, summaryMessage, ...keepMessages]
193206

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

0 commit comments

Comments
 (0)