Skip to content
This repository was archived by the owner on May 15, 2026. It is now read-only.

Commit d9caa4e

Browse files
committed
refactor: remove legacy Bedrock code path and update tests to use user-role summaries
- Simplified getMessagesSinceLastSummary by removing ~47 lines of legacy Bedrock-specific code - Summary messages are always created with role: 'user' (fresh-start model), making the assistant-role workaround dead code - Updated all test files to use user-role summaries consistently: - nested-condense.spec.ts: removed legacy assistant-role describe block - condense.spec.ts: removed legacy Bedrock test case - index.spec.ts: updated 3 tests - rewind-after-condense.spec.ts: updated 6+ tests - webviewMessageHandler.delete.spec.ts: updated 6 API history mocks - context-management.spec.ts: updated 4 mockSummarizeResponse objects
1 parent 57044f6 commit d9caa4e

7 files changed

Lines changed: 87 additions & 289 deletions

File tree

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

Lines changed: 1 addition & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -250,7 +250,7 @@ Line 2
250250
it("should not summarize messages that already contain a recent summary with no new messages", async () => {
251251
const messages: ApiMessage[] = [
252252
{ role: "user", content: "First message with /command" },
253-
{ role: "assistant", content: "Previous summary", isSummary: true },
253+
{ role: "user", content: "Previous summary", isSummary: true },
254254
]
255255

256256
const result = await summarizeConversation(messages, mockApiHandler, "System prompt", taskId, false)
@@ -413,20 +413,5 @@ Line 2
413413
expect(result[1]).toEqual(messages[4])
414414
expect(result[2]).toEqual(messages[5])
415415
})
416-
417-
it("should prepend first user message when summary starts with assistant", () => {
418-
const messages: ApiMessage[] = [
419-
{ role: "user", content: "Original first message" },
420-
{ role: "assistant", content: "Summary content", isSummary: true },
421-
{ role: "user", content: "After summary" },
422-
]
423-
424-
const result = getMessagesSinceLastSummary(messages)
425-
426-
// Should prepend original first message for Bedrock compatibility
427-
expect(result[0]).toEqual(messages[0]) // Original first user message
428-
expect(result[1]).toEqual(messages[1]) // The summary
429-
expect(result[2]).toEqual(messages[2])
430-
})
431416
})
432417
})

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

Lines changed: 13 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -252,38 +252,36 @@ describe("getMessagesSinceLastSummary", () => {
252252
expect(result).toEqual(messages)
253253
})
254254

255-
it("should return messages since the last summary (preserves original first user message when needed)", () => {
255+
it("should return messages since the last summary", () => {
256256
const messages: ApiMessage[] = [
257257
{ role: "user", content: "Hello", ts: 1 },
258258
{ role: "assistant", content: "Hi there", ts: 2 },
259-
{ role: "assistant", content: "Summary of conversation", ts: 3, isSummary: true },
260-
{ role: "user", content: "How are you?", ts: 4 },
261-
{ role: "assistant", content: "I'm good", ts: 5 },
259+
{ role: "user", content: "Summary of conversation", ts: 3, isSummary: true },
260+
{ role: "assistant", content: "How are you?", ts: 4 },
261+
{ role: "user", content: "I'm good", ts: 5 },
262262
]
263263

264264
const result = getMessagesSinceLastSummary(messages)
265265
expect(result).toEqual([
266-
{ role: "user", content: "Hello", ts: 1 },
267-
{ role: "assistant", content: "Summary of conversation", ts: 3, isSummary: true },
268-
{ role: "user", content: "How are you?", ts: 4 },
269-
{ role: "assistant", content: "I'm good", ts: 5 },
266+
{ role: "user", content: "Summary of conversation", ts: 3, isSummary: true },
267+
{ role: "assistant", content: "How are you?", ts: 4 },
268+
{ role: "user", content: "I'm good", ts: 5 },
270269
])
271270
})
272271

273272
it("should handle multiple summary messages and return since the last one", () => {
274273
const messages: ApiMessage[] = [
275274
{ role: "user", content: "Hello", ts: 1 },
276-
{ role: "assistant", content: "First summary", ts: 2, isSummary: true },
277-
{ role: "user", content: "How are you?", ts: 3 },
278-
{ role: "assistant", content: "Second summary", ts: 4, isSummary: true },
279-
{ role: "user", content: "What's new?", ts: 5 },
275+
{ role: "user", content: "First summary", ts: 2, isSummary: true },
276+
{ role: "assistant", content: "How are you?", ts: 3 },
277+
{ role: "user", content: "Second summary", ts: 4, isSummary: true },
278+
{ role: "assistant", content: "What's new?", ts: 5 },
280279
]
281280

282281
const result = getMessagesSinceLastSummary(messages)
283282
expect(result).toEqual([
284-
{ role: "user", content: "Hello", ts: 1 },
285-
{ role: "assistant", content: "Second summary", ts: 4, isSummary: true },
286-
{ role: "user", content: "What's new?", ts: 5 },
283+
{ role: "user", content: "Second summary", ts: 4, isSummary: true },
284+
{ role: "assistant", content: "What's new?", ts: 5 },
287285
])
288286
})
289287

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

Lines changed: 14 additions & 167 deletions
Original file line numberDiff line numberDiff line change
@@ -129,157 +129,8 @@ describe("nested condensing scenarios", () => {
129129
})
130130
})
131131

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-
281132
describe("getMessagesSinceLastSummary behavior with full vs effective history", () => {
282-
it("should behave differently when called with full history vs effective history", () => {
133+
it("should return consistent results when called with full history vs effective history", () => {
283134
const condenseId = "condense-1"
284135

285136
const fullHistory: ApiMessage[] = [
@@ -305,58 +156,54 @@ describe("nested condensing scenarios", () => {
305156
// Both should return the same messages when summary is user role
306157
expect(fromFullHistory.length).toBe(fromEffectiveHistory.length)
307158

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
159+
// Both should start with the summary
311160
expect(fromFullHistory[0].isSummary).toBe(true)
312161
expect(fromEffectiveHistory[0].isSummary).toBe(true)
313162
})
314163

315-
it("BUG SCENARIO: Bedrock fix should not include condensed original task", () => {
164+
it("should not include condensed original task in effective history", () => {
316165
const condenseId1 = "condense-1"
317166
const condenseId2 = "condense-2"
318167

319-
// Scenario: Two condenses, first summary is assistant role (legacy)
168+
// Scenario: Two nested condenses with user-role summaries
320169
const fullHistory: ApiMessage[] = [
321170
{ role: "user", content: "Original task - should NOT appear", ts: 100, condenseParent: condenseId1 },
322171
{ role: "assistant", content: "Old response", ts: 200, condenseParent: condenseId1 },
323-
// Legacy assistant-role summary, then condensed again
172+
// First summary (user role, fresh-start model), then condensed again
324173
{
325-
role: "assistant",
326-
content: "Summary 1",
174+
role: "user",
175+
content: [{ type: "text", text: "Summary 1" }],
327176
ts: 299,
328177
isSummary: true,
329178
condenseId: condenseId1,
330179
condenseParent: condenseId2,
331180
},
332-
{ role: "user", content: "After S1", ts: 300, condenseParent: condenseId2 },
333-
// Second summary (still assistant for legacy consistency in this test)
181+
{ role: "assistant", content: "After S1", ts: 300, condenseParent: condenseId2 },
182+
// Second summary (user role, fresh-start model)
334183
{
335-
role: "assistant",
336-
content: "Summary 2",
184+
role: "user",
185+
content: [{ type: "text", text: "Summary 2" }],
337186
ts: 399,
338187
isSummary: true,
339188
condenseId: condenseId2,
340189
},
341-
{ role: "user", content: "Current message", ts: 400 },
190+
{ role: "assistant", content: "Current message", ts: 400 },
342191
]
343192

344193
const effectiveHistory = getEffectiveApiHistory(fullHistory)
345194
expect(effectiveHistory.length).toBe(2) // Summary2 + Current message
346195

347196
const messagesSinceLastSummary = getMessagesSinceLastSummary(effectiveHistory)
348197

349-
// CRITICAL BUG CHECK: The original task should NEVER be included
198+
// The original task should NOT be included
350199
const hasOriginalTask = messagesSinceLastSummary.some((m) =>
351200
typeof m.content === "string"
352201
? m.content.includes("Original task")
353202
: JSON.stringify(m.content).includes("Original task"),
354203
)
355-
356-
// This assertion documents the expected behavior
357204
expect(hasOriginalTask).toBe(false)
358205

359-
// Also verify Summary1 is not included
206+
// Summary1 should not be included (it was condensed)
360207
const hasSummary1 = messagesSinceLastSummary.some((m) => m.condenseId === condenseId1)
361208
expect(hasSummary1).toBe(false)
362209
})

0 commit comments

Comments
 (0)