From 57044f6aa0d06c624edceaf5d8a905ea6e5e9aaa Mon Sep 17 00:00:00 2001 From: Hannes Rudolph Date: Mon, 26 Jan 2026 17:14:27 -0700 Subject: [PATCH 1/2] 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. --- .../__tests__/nested-condense.spec.ts | 364 ++++++++++++++++++ src/core/condense/index.ts | 15 + 2 files changed, 379 insertions(+) create mode 100644 src/core/condense/__tests__/nested-condense.spec.ts diff --git a/src/core/condense/__tests__/nested-condense.spec.ts b/src/core/condense/__tests__/nested-condense.spec.ts new file mode 100644 index 00000000000..2efcdc5161b --- /dev/null +++ b/src/core/condense/__tests__/nested-condense.spec.ts @@ -0,0 +1,364 @@ +import { describe, it, expect } from "vitest" +import { ApiMessage } from "../../task-persistence/apiMessages" +import { getEffectiveApiHistory, getMessagesSinceLastSummary } from "../index" + +describe("nested condensing scenarios", () => { + describe("fresh-start model (user-role summaries)", () => { + it("should return only the latest summary and messages after it", () => { + const condenseId1 = "condense-1" + const condenseId2 = "condense-2" + + // Simulate history after two nested condenses with user-role summaries + const history: ApiMessage[] = [ + // Original task - condensed in first condense + { role: "user", content: "Build an app", ts: 100, condenseParent: condenseId1 }, + // Messages from first condense + { role: "assistant", content: "Starting...", ts: 200, condenseParent: condenseId1 }, + { role: "user", content: "Add auth", ts: 300, condenseParent: condenseId1 }, + // First summary (user role, fresh-start model) - then condensed in second condense + { + role: "user", + content: [{ type: "text", text: "## Summary 1" }], + ts: 399, + isSummary: true, + condenseId: condenseId1, + condenseParent: condenseId2, // Tagged during second condense + }, + // Messages after first condense but before second + { role: "assistant", content: "Auth added", ts: 400, condenseParent: condenseId2 }, + { role: "user", content: "Add database", ts: 500, condenseParent: condenseId2 }, + // Second summary (user role, fresh-start model) + { + role: "user", + content: [{ type: "text", text: "## Summary 2" }], + ts: 599, + isSummary: true, + condenseId: condenseId2, + }, + // Messages after second condense (kept messages) + { role: "assistant", content: "Database added", ts: 600 }, + { role: "user", content: "Now test it", ts: 700 }, + ] + + // Step 1: Get effective history + const effectiveHistory = getEffectiveApiHistory(history) + + // Should only contain: Summary2, and messages after it + expect(effectiveHistory.length).toBe(3) + expect(effectiveHistory[0].isSummary).toBe(true) + expect(effectiveHistory[0].condenseId).toBe(condenseId2) // Latest summary + expect(effectiveHistory[1].content).toBe("Database added") + expect(effectiveHistory[2].content).toBe("Now test it") + + // Verify NO condensed messages are included + const hasCondensedMessages = effectiveHistory.some( + (msg) => msg.condenseParent && history.some((m) => m.isSummary && m.condenseId === msg.condenseParent), + ) + expect(hasCondensedMessages).toBe(false) + + // Step 2: Get messages since last summary (on effective history) + const messagesSinceLastSummary = getMessagesSinceLastSummary(effectiveHistory) + + // Should be the same as effective history since Summary2 is already at the start + expect(messagesSinceLastSummary.length).toBe(3) + expect(messagesSinceLastSummary[0].isSummary).toBe(true) + expect(messagesSinceLastSummary[0].condenseId).toBe(condenseId2) + + // CRITICAL: No previous history (Summary1 or original task) should be included + const hasSummary1 = messagesSinceLastSummary.some((m) => m.condenseId === condenseId1) + expect(hasSummary1).toBe(false) + + const hasOriginalTask = messagesSinceLastSummary.some((m) => m.content === "Build an app") + expect(hasOriginalTask).toBe(false) + }) + + it("should handle triple nested condense correctly", () => { + const condenseId1 = "condense-1" + const condenseId2 = "condense-2" + const condenseId3 = "condense-3" + + const history: ApiMessage[] = [ + // First condense content + { role: "user", content: "Task", ts: 100, condenseParent: condenseId1 }, + { + role: "user", + content: [{ type: "text", text: "## Summary 1" }], + ts: 199, + isSummary: true, + condenseId: condenseId1, + condenseParent: condenseId2, + }, + // Second condense content + { role: "assistant", content: "After S1", ts: 200, condenseParent: condenseId2 }, + { + role: "user", + content: [{ type: "text", text: "## Summary 2" }], + ts: 299, + isSummary: true, + condenseId: condenseId2, + condenseParent: condenseId3, + }, + // Third condense content + { role: "assistant", content: "After S2", ts: 300, condenseParent: condenseId3 }, + { + role: "user", + content: [{ type: "text", text: "## Summary 3" }], + ts: 399, + isSummary: true, + condenseId: condenseId3, + }, + // Current messages + { role: "assistant", content: "Current work", ts: 400 }, + ] + + const effectiveHistory = getEffectiveApiHistory(history) + + // Should only contain Summary3 and current work + expect(effectiveHistory.length).toBe(2) + expect(effectiveHistory[0].condenseId).toBe(condenseId3) + expect(effectiveHistory[1].content).toBe("Current work") + + const messagesSinceLastSummary = getMessagesSinceLastSummary(effectiveHistory) + expect(messagesSinceLastSummary.length).toBe(2) + + // No previous summaries should be included + const hasPreviousSummaries = messagesSinceLastSummary.some( + (m) => m.condenseId === condenseId1 || m.condenseId === condenseId2, + ) + expect(hasPreviousSummaries).toBe(false) + }) + }) + + describe("legacy assistant-role summaries (Bedrock fix scenario)", () => { + it("should NOT duplicate the summary when summary is assistant role", () => { + const condenseId = "condense-1" + + const history: ApiMessage[] = [ + { role: "user", content: "Task", ts: 100, condenseParent: condenseId }, + { role: "assistant", content: "Response", ts: 200, condenseParent: condenseId }, + // Legacy summary with assistant role + { + role: "assistant", + content: "Summary of work", + ts: 299, + isSummary: true, + condenseId, + }, + { role: "user", content: "Continue", ts: 300 }, + ] + + const effectiveHistory = getEffectiveApiHistory(history) + expect(effectiveHistory.length).toBe(2) + expect(effectiveHistory[0].isSummary).toBe(true) + + const messagesSinceLastSummary = getMessagesSinceLastSummary(effectiveHistory) + + // The Bedrock fix might trigger, but it should NOT create duplicates + // when the input is already the effective history + const summaryCount = messagesSinceLastSummary.filter((m) => m.isSummary).length + expect(summaryCount).toBe(1) // Only one summary, not duplicated + + // Should have summary + "Continue" + expect(messagesSinceLastSummary.length).toBeLessThanOrEqual(3) + }) + + it("should NOT include original task when called on effective history", () => { + const condenseId = "condense-1" + + const history: ApiMessage[] = [ + { role: "user", content: "Original task content", ts: 100, condenseParent: condenseId }, + { + role: "assistant", + content: "Legacy summary", + ts: 199, + isSummary: true, + condenseId, + }, + { role: "user", content: "After summary", ts: 200 }, + ] + + const effectiveHistory = getEffectiveApiHistory(history) + const messagesSinceLastSummary = getMessagesSinceLastSummary(effectiveHistory) + + // The original task should NOT be in the result + const hasOriginalTask = messagesSinceLastSummary.some((m) => m.content === "Original task content") + expect(hasOriginalTask).toBe(false) + }) + + describe("BUG: getMessagesSinceLastSummary with full history (summarization input)", () => { + it("should NOT include original task in summarization input when summary is assistant role", () => { + const condenseId = "condense-1" + + // Scenario: First condense created an assistant-role summary (legacy) + // Now we're doing a second condense + const fullHistory: ApiMessage[] = [ + // Original task - was condensed in first condense + { + role: "user", + content: "Original task that should NOT be in summarization input", + ts: 100, + condenseParent: condenseId, + }, + { role: "assistant", content: "Old response", ts: 200, condenseParent: condenseId }, + // Legacy assistant-role summary from first condense + { + role: "assistant", // <-- Legacy: assistant role + content: "First summary", + ts: 299, + isSummary: true, + condenseId, + }, + // New messages to be summarized in second condense + { role: "user", content: "Message after summary", ts: 300 }, + { role: "assistant", content: "Response after summary", ts: 400 }, + ] + + // This simulates what summarizeConversation does when called for manual condense + const messagesToSummarize = getMessagesSinceLastSummary(fullHistory) + + // THE BUG: Bedrock fix prepends messages[0] (original task) when summary is assistant role + // This is wrong because: + // 1. The original task was already condensed (has condenseParent) + // 2. It should not be included in the summarization input for the second condense + + // Check if original task is incorrectly included + const hasOriginalTask = messagesToSummarize.some( + (m) => typeof m.content === "string" && m.content.includes("Original task"), + ) + + // This test documents the current BUGGY behavior if it fails + // The fix should make this pass by NOT including the original task + console.log( + "Messages to summarize:", + messagesToSummarize.map((m) => ({ + role: m.role, + content: typeof m.content === "string" ? m.content.substring(0, 50) : "[array]", + condenseParent: m.condenseParent, + isSummary: m.isSummary, + })), + ) + + // EXPECTED: Original task should NOT be included + // ACTUAL (if bug exists): Original task IS included due to Bedrock fix + expect(hasOriginalTask).toBe(false) + }) + + it("should NOT include condensed messages when preparing summarization input", () => { + const condenseId1 = "condense-1" + + const fullHistory: ApiMessage[] = [ + // Original condensed messages + { role: "user", content: "Condensed task", ts: 100, condenseParent: condenseId1 }, + { role: "assistant", content: "Condensed response", ts: 200, condenseParent: condenseId1 }, + // First summary (assistant role for legacy) + { + role: "assistant", + content: "Summary of first condense", + ts: 299, + isSummary: true, + condenseId: condenseId1, + }, + // Messages to be summarized + { role: "user", content: "New work", ts: 300 }, + { role: "assistant", content: "New response", ts: 400 }, + ] + + const messagesToSummarize = getMessagesSinceLastSummary(fullHistory) + + // Count how many messages with condenseParent are in the result + const condensedMessagesInResult = messagesToSummarize.filter( + (m) => m.condenseParent && m.condenseParent === condenseId1 && !m.isSummary, + ) + + console.log("Condensed messages in result:", condensedMessagesInResult.length) + + // No condensed messages (other than the summary which kicks off the new input) should be included + expect(condensedMessagesInResult.length).toBe(0) + }) + }) + }) + + describe("getMessagesSinceLastSummary behavior with full vs effective history", () => { + it("should behave differently when called with full history vs effective history", () => { + const condenseId = "condense-1" + + const fullHistory: ApiMessage[] = [ + { role: "user", content: "Original task", ts: 100, condenseParent: condenseId }, + { role: "assistant", content: "Response", ts: 200, condenseParent: condenseId }, + { + role: "user", + content: [{ type: "text", text: "Summary" }], + ts: 299, + isSummary: true, + condenseId, + }, + { role: "assistant", content: "After summary", ts: 300 }, + ] + + // Called with FULL history (as in summarizeConversation) + const fromFullHistory = getMessagesSinceLastSummary(fullHistory) + + // Called with EFFECTIVE history (as in attemptApiRequest) + const effectiveHistory = getEffectiveApiHistory(fullHistory) + const fromEffectiveHistory = getMessagesSinceLastSummary(effectiveHistory) + + // Both should return the same messages when summary is user role + expect(fromFullHistory.length).toBe(fromEffectiveHistory.length) + + // The key difference: fromFullHistory[0] references fullHistory, + // while fromEffectiveHistory[0] references effectiveHistory + // With user-role summary, Bedrock fix should NOT trigger in either case + expect(fromFullHistory[0].isSummary).toBe(true) + expect(fromEffectiveHistory[0].isSummary).toBe(true) + }) + + it("BUG SCENARIO: Bedrock fix should not include condensed original task", () => { + const condenseId1 = "condense-1" + const condenseId2 = "condense-2" + + // Scenario: Two condenses, first summary is assistant role (legacy) + const fullHistory: ApiMessage[] = [ + { role: "user", content: "Original task - should NOT appear", ts: 100, condenseParent: condenseId1 }, + { role: "assistant", content: "Old response", ts: 200, condenseParent: condenseId1 }, + // Legacy assistant-role summary, then condensed again + { + role: "assistant", + content: "Summary 1", + ts: 299, + isSummary: true, + condenseId: condenseId1, + condenseParent: condenseId2, + }, + { role: "user", content: "After S1", ts: 300, condenseParent: condenseId2 }, + // Second summary (still assistant for legacy consistency in this test) + { + role: "assistant", + content: "Summary 2", + ts: 399, + isSummary: true, + condenseId: condenseId2, + }, + { role: "user", content: "Current message", ts: 400 }, + ] + + const effectiveHistory = getEffectiveApiHistory(fullHistory) + expect(effectiveHistory.length).toBe(2) // Summary2 + Current message + + const messagesSinceLastSummary = getMessagesSinceLastSummary(effectiveHistory) + + // CRITICAL BUG CHECK: The original task should NEVER be included + const hasOriginalTask = messagesSinceLastSummary.some((m) => + typeof m.content === "string" + ? m.content.includes("Original task") + : JSON.stringify(m.content).includes("Original task"), + ) + + // This assertion documents the expected behavior + expect(hasOriginalTask).toBe(false) + + // Also verify Summary1 is not included + const hasSummary1 = messagesSinceLastSummary.some((m) => m.condenseId === condenseId1) + expect(hasSummary1).toBe(false) + }) + }) +}) diff --git a/src/core/condense/index.ts b/src/core/condense/index.ts index 0c92087aff1..95c1728bd43 100644 --- a/src/core/condense/index.ts +++ b/src/core/condense/index.ts @@ -390,6 +390,21 @@ export function getMessagesSinceLastSummary(messages: ApiMessage[]): ApiMessage[ // Get the original first message (should always be a user message with the task) const originalFirstMessage = messages[0] if (originalFirstMessage && originalFirstMessage.role === "user") { + // BUG FIX: Don't prepend the original first message if it has a condenseParent. + // This can happen during nested condensing where the original task was already + // condensed in a previous condense operation. Including it would incorrectly + // add previously-condensed content to the summarization input. + // See: https://github.com/RooCodeInc/Roo-Code/issues/10942 (nested condensing bug) + if (originalFirstMessage.condenseParent) { + // The original first message was already condensed, so we should NOT include it. + // Instead, use a generic placeholder message to satisfy the Bedrock requirement. + const userMessage: ApiMessage = { + role: "user", + content: "Please continue from the following summary:", + ts: messages[0]?.ts ? messages[0].ts - 1 : Date.now(), + } + return [userMessage, ...messagesSinceSummary] + } // Use the original first message unchanged to maintain full context return [originalFirstMessage, ...messagesSinceSummary] } else { From d9caa4e91be2699eccb07d416053d8f3caf25d54 Mon Sep 17 00:00:00 2001 From: Hannes Rudolph Date: Mon, 26 Jan 2026 18:24:25 -0700 Subject: [PATCH 2/2] 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 --- src/core/condense/__tests__/condense.spec.ts | 17 +- src/core/condense/__tests__/index.spec.ts | 28 ++- .../__tests__/nested-condense.spec.ts | 181 ++---------------- .../__tests__/rewind-after-condense.spec.ts | 47 +++-- src/core/condense/index.ts | 49 +---- .../__tests__/context-management.spec.ts | 16 +- .../webviewMessageHandler.delete.spec.ts | 38 ++-- 7 files changed, 87 insertions(+), 289 deletions(-) diff --git a/src/core/condense/__tests__/condense.spec.ts b/src/core/condense/__tests__/condense.spec.ts index dcb05cd74df..9d3352d01a5 100644 --- a/src/core/condense/__tests__/condense.spec.ts +++ b/src/core/condense/__tests__/condense.spec.ts @@ -250,7 +250,7 @@ Line 2 it("should not summarize messages that already contain a recent summary with no new messages", async () => { const messages: ApiMessage[] = [ { role: "user", content: "First message with /command" }, - { role: "assistant", content: "Previous summary", isSummary: true }, + { role: "user", content: "Previous summary", isSummary: true }, ] const result = await summarizeConversation(messages, mockApiHandler, "System prompt", taskId, false) @@ -413,20 +413,5 @@ Line 2 expect(result[1]).toEqual(messages[4]) expect(result[2]).toEqual(messages[5]) }) - - it("should prepend first user message when summary starts with assistant", () => { - const messages: ApiMessage[] = [ - { role: "user", content: "Original first message" }, - { role: "assistant", content: "Summary content", isSummary: true }, - { role: "user", content: "After summary" }, - ] - - const result = getMessagesSinceLastSummary(messages) - - // Should prepend original first message for Bedrock compatibility - expect(result[0]).toEqual(messages[0]) // Original first user message - expect(result[1]).toEqual(messages[1]) // The summary - expect(result[2]).toEqual(messages[2]) - }) }) }) diff --git a/src/core/condense/__tests__/index.spec.ts b/src/core/condense/__tests__/index.spec.ts index 8a0f3bea63a..c8bc5ee8ef1 100644 --- a/src/core/condense/__tests__/index.spec.ts +++ b/src/core/condense/__tests__/index.spec.ts @@ -252,38 +252,36 @@ describe("getMessagesSinceLastSummary", () => { expect(result).toEqual(messages) }) - it("should return messages since the last summary (preserves original first user message when needed)", () => { + it("should return messages since the last summary", () => { const messages: ApiMessage[] = [ { role: "user", content: "Hello", ts: 1 }, { role: "assistant", content: "Hi there", ts: 2 }, - { role: "assistant", content: "Summary of conversation", ts: 3, isSummary: true }, - { role: "user", content: "How are you?", ts: 4 }, - { role: "assistant", content: "I'm good", ts: 5 }, + { role: "user", content: "Summary of conversation", ts: 3, isSummary: true }, + { role: "assistant", content: "How are you?", ts: 4 }, + { role: "user", content: "I'm good", ts: 5 }, ] const result = getMessagesSinceLastSummary(messages) expect(result).toEqual([ - { role: "user", content: "Hello", ts: 1 }, - { role: "assistant", content: "Summary of conversation", ts: 3, isSummary: true }, - { role: "user", content: "How are you?", ts: 4 }, - { role: "assistant", content: "I'm good", ts: 5 }, + { role: "user", content: "Summary of conversation", ts: 3, isSummary: true }, + { role: "assistant", content: "How are you?", ts: 4 }, + { role: "user", content: "I'm good", ts: 5 }, ]) }) it("should handle multiple summary messages and return since the last one", () => { const messages: ApiMessage[] = [ { role: "user", content: "Hello", ts: 1 }, - { role: "assistant", content: "First summary", ts: 2, isSummary: true }, - { role: "user", content: "How are you?", ts: 3 }, - { role: "assistant", content: "Second summary", ts: 4, isSummary: true }, - { role: "user", content: "What's new?", ts: 5 }, + { role: "user", content: "First summary", ts: 2, isSummary: true }, + { role: "assistant", content: "How are you?", ts: 3 }, + { role: "user", content: "Second summary", ts: 4, isSummary: true }, + { role: "assistant", content: "What's new?", ts: 5 }, ] const result = getMessagesSinceLastSummary(messages) expect(result).toEqual([ - { role: "user", content: "Hello", ts: 1 }, - { role: "assistant", content: "Second summary", ts: 4, isSummary: true }, - { role: "user", content: "What's new?", ts: 5 }, + { role: "user", content: "Second summary", ts: 4, isSummary: true }, + { role: "assistant", content: "What's new?", ts: 5 }, ]) }) diff --git a/src/core/condense/__tests__/nested-condense.spec.ts b/src/core/condense/__tests__/nested-condense.spec.ts index 2efcdc5161b..3868a22262b 100644 --- a/src/core/condense/__tests__/nested-condense.spec.ts +++ b/src/core/condense/__tests__/nested-condense.spec.ts @@ -129,157 +129,8 @@ describe("nested condensing scenarios", () => { }) }) - describe("legacy assistant-role summaries (Bedrock fix scenario)", () => { - it("should NOT duplicate the summary when summary is assistant role", () => { - const condenseId = "condense-1" - - const history: ApiMessage[] = [ - { role: "user", content: "Task", ts: 100, condenseParent: condenseId }, - { role: "assistant", content: "Response", ts: 200, condenseParent: condenseId }, - // Legacy summary with assistant role - { - role: "assistant", - content: "Summary of work", - ts: 299, - isSummary: true, - condenseId, - }, - { role: "user", content: "Continue", ts: 300 }, - ] - - const effectiveHistory = getEffectiveApiHistory(history) - expect(effectiveHistory.length).toBe(2) - expect(effectiveHistory[0].isSummary).toBe(true) - - const messagesSinceLastSummary = getMessagesSinceLastSummary(effectiveHistory) - - // The Bedrock fix might trigger, but it should NOT create duplicates - // when the input is already the effective history - const summaryCount = messagesSinceLastSummary.filter((m) => m.isSummary).length - expect(summaryCount).toBe(1) // Only one summary, not duplicated - - // Should have summary + "Continue" - expect(messagesSinceLastSummary.length).toBeLessThanOrEqual(3) - }) - - it("should NOT include original task when called on effective history", () => { - const condenseId = "condense-1" - - const history: ApiMessage[] = [ - { role: "user", content: "Original task content", ts: 100, condenseParent: condenseId }, - { - role: "assistant", - content: "Legacy summary", - ts: 199, - isSummary: true, - condenseId, - }, - { role: "user", content: "After summary", ts: 200 }, - ] - - const effectiveHistory = getEffectiveApiHistory(history) - const messagesSinceLastSummary = getMessagesSinceLastSummary(effectiveHistory) - - // The original task should NOT be in the result - const hasOriginalTask = messagesSinceLastSummary.some((m) => m.content === "Original task content") - expect(hasOriginalTask).toBe(false) - }) - - describe("BUG: getMessagesSinceLastSummary with full history (summarization input)", () => { - it("should NOT include original task in summarization input when summary is assistant role", () => { - const condenseId = "condense-1" - - // Scenario: First condense created an assistant-role summary (legacy) - // Now we're doing a second condense - const fullHistory: ApiMessage[] = [ - // Original task - was condensed in first condense - { - role: "user", - content: "Original task that should NOT be in summarization input", - ts: 100, - condenseParent: condenseId, - }, - { role: "assistant", content: "Old response", ts: 200, condenseParent: condenseId }, - // Legacy assistant-role summary from first condense - { - role: "assistant", // <-- Legacy: assistant role - content: "First summary", - ts: 299, - isSummary: true, - condenseId, - }, - // New messages to be summarized in second condense - { role: "user", content: "Message after summary", ts: 300 }, - { role: "assistant", content: "Response after summary", ts: 400 }, - ] - - // This simulates what summarizeConversation does when called for manual condense - const messagesToSummarize = getMessagesSinceLastSummary(fullHistory) - - // THE BUG: Bedrock fix prepends messages[0] (original task) when summary is assistant role - // This is wrong because: - // 1. The original task was already condensed (has condenseParent) - // 2. It should not be included in the summarization input for the second condense - - // Check if original task is incorrectly included - const hasOriginalTask = messagesToSummarize.some( - (m) => typeof m.content === "string" && m.content.includes("Original task"), - ) - - // This test documents the current BUGGY behavior if it fails - // The fix should make this pass by NOT including the original task - console.log( - "Messages to summarize:", - messagesToSummarize.map((m) => ({ - role: m.role, - content: typeof m.content === "string" ? m.content.substring(0, 50) : "[array]", - condenseParent: m.condenseParent, - isSummary: m.isSummary, - })), - ) - - // EXPECTED: Original task should NOT be included - // ACTUAL (if bug exists): Original task IS included due to Bedrock fix - expect(hasOriginalTask).toBe(false) - }) - - it("should NOT include condensed messages when preparing summarization input", () => { - const condenseId1 = "condense-1" - - const fullHistory: ApiMessage[] = [ - // Original condensed messages - { role: "user", content: "Condensed task", ts: 100, condenseParent: condenseId1 }, - { role: "assistant", content: "Condensed response", ts: 200, condenseParent: condenseId1 }, - // First summary (assistant role for legacy) - { - role: "assistant", - content: "Summary of first condense", - ts: 299, - isSummary: true, - condenseId: condenseId1, - }, - // Messages to be summarized - { role: "user", content: "New work", ts: 300 }, - { role: "assistant", content: "New response", ts: 400 }, - ] - - const messagesToSummarize = getMessagesSinceLastSummary(fullHistory) - - // Count how many messages with condenseParent are in the result - const condensedMessagesInResult = messagesToSummarize.filter( - (m) => m.condenseParent && m.condenseParent === condenseId1 && !m.isSummary, - ) - - console.log("Condensed messages in result:", condensedMessagesInResult.length) - - // No condensed messages (other than the summary which kicks off the new input) should be included - expect(condensedMessagesInResult.length).toBe(0) - }) - }) - }) - describe("getMessagesSinceLastSummary behavior with full vs effective history", () => { - it("should behave differently when called with full history vs effective history", () => { + it("should return consistent results when called with full history vs effective history", () => { const condenseId = "condense-1" const fullHistory: ApiMessage[] = [ @@ -305,40 +156,38 @@ describe("nested condensing scenarios", () => { // Both should return the same messages when summary is user role expect(fromFullHistory.length).toBe(fromEffectiveHistory.length) - // The key difference: fromFullHistory[0] references fullHistory, - // while fromEffectiveHistory[0] references effectiveHistory - // With user-role summary, Bedrock fix should NOT trigger in either case + // Both should start with the summary expect(fromFullHistory[0].isSummary).toBe(true) expect(fromEffectiveHistory[0].isSummary).toBe(true) }) - it("BUG SCENARIO: Bedrock fix should not include condensed original task", () => { + it("should not include condensed original task in effective history", () => { const condenseId1 = "condense-1" const condenseId2 = "condense-2" - // Scenario: Two condenses, first summary is assistant role (legacy) + // Scenario: Two nested condenses with user-role summaries const fullHistory: ApiMessage[] = [ { role: "user", content: "Original task - should NOT appear", ts: 100, condenseParent: condenseId1 }, { role: "assistant", content: "Old response", ts: 200, condenseParent: condenseId1 }, - // Legacy assistant-role summary, then condensed again + // First summary (user role, fresh-start model), then condensed again { - role: "assistant", - content: "Summary 1", + role: "user", + content: [{ type: "text", text: "Summary 1" }], ts: 299, isSummary: true, condenseId: condenseId1, condenseParent: condenseId2, }, - { role: "user", content: "After S1", ts: 300, condenseParent: condenseId2 }, - // Second summary (still assistant for legacy consistency in this test) + { role: "assistant", content: "After S1", ts: 300, condenseParent: condenseId2 }, + // Second summary (user role, fresh-start model) { - role: "assistant", - content: "Summary 2", + role: "user", + content: [{ type: "text", text: "Summary 2" }], ts: 399, isSummary: true, condenseId: condenseId2, }, - { role: "user", content: "Current message", ts: 400 }, + { role: "assistant", content: "Current message", ts: 400 }, ] const effectiveHistory = getEffectiveApiHistory(fullHistory) @@ -346,17 +195,15 @@ describe("nested condensing scenarios", () => { const messagesSinceLastSummary = getMessagesSinceLastSummary(effectiveHistory) - // CRITICAL BUG CHECK: The original task should NEVER be included + // The original task should NOT be included const hasOriginalTask = messagesSinceLastSummary.some((m) => typeof m.content === "string" ? m.content.includes("Original task") : JSON.stringify(m.content).includes("Original task"), ) - - // This assertion documents the expected behavior expect(hasOriginalTask).toBe(false) - // Also verify Summary1 is not included + // Summary1 should not be included (it was condensed) const hasSummary1 = messagesSinceLastSummary.some((m) => m.condenseId === condenseId1) expect(hasSummary1).toBe(false) }) diff --git a/src/core/condense/__tests__/rewind-after-condense.spec.ts b/src/core/condense/__tests__/rewind-after-condense.spec.ts index 84fdb63ca86..068f49a8570 100644 --- a/src/core/condense/__tests__/rewind-after-condense.spec.ts +++ b/src/core/condense/__tests__/rewind-after-condense.spec.ts @@ -83,7 +83,7 @@ describe("Rewind After Condense - Issue #8295", () => { const messages: ApiMessage[] = [ { role: "user", content: "First message", ts: 1 }, { role: "assistant", content: "First response", ts: 2, condenseParent: condenseId }, - { role: "assistant", content: "Summary", ts: 3, isSummary: true, condenseId }, + { role: "user", content: "Summary", ts: 3, isSummary: true, condenseId }, ] const cleaned = cleanupAfterTruncation(messages) @@ -97,7 +97,7 @@ describe("Rewind After Condense - Issue #8295", () => { const condenseId2 = "summary-2" const messages: ApiMessage[] = [ { role: "user", content: "Message 1", ts: 1, condenseParent: condenseId1 }, - { role: "assistant", content: "Summary 1", ts: 2, isSummary: true, condenseId: condenseId1 }, + { role: "user", content: "Summary 1", ts: 2, isSummary: true, condenseId: condenseId1 }, { role: "user", content: "Message 2", ts: 3, condenseParent: condenseId2 }, // Summary 2 is NOT present (was truncated) ] @@ -203,8 +203,8 @@ describe("Rewind After Condense - Issue #8295", () => { { role: "user", content: "Start", ts: 1 }, { role: "assistant", content: "Response 1", ts: 2, condenseParent: condenseId }, { role: "user", content: "More", ts: 3, condenseParent: condenseId }, - { role: "assistant", content: "Summary", ts: 4, isSummary: true, condenseId }, - { role: "user", content: "After summary", ts: 5 }, + { role: "user", content: "Summary", ts: 4, isSummary: true, condenseId }, + { role: "assistant", content: "After summary", ts: 5 }, ] // Fresh start model: effective history is summary + messages after it @@ -250,7 +250,7 @@ describe("Rewind After Condense - Issue #8295", () => { { role: "assistant", content: "Response 3", ts: 600, condenseParent: condenseId }, { role: "user", content: "Even more", ts: 700, condenseParent: condenseId }, // Summary gets ts = firstKeptTs - 1 = 999, which is unique - { role: "assistant", content: "Summary", ts: firstKeptTs - 1, isSummary: true, condenseId }, + { role: "user", content: "Summary", ts: firstKeptTs - 1, isSummary: true, condenseId }, // First kept message { role: "user", content: "First kept message", ts: firstKeptTs }, { role: "assistant", content: "Response to first kept", ts: 1100 }, @@ -283,9 +283,9 @@ describe("Rewind After Condense - Issue #8295", () => { const messages: ApiMessage[] = [ { role: "user", content: "Initial", ts: 1 }, - { role: "assistant", content: "Summary", ts: firstKeptTs - 1, isSummary: true, condenseId }, - { role: "user", content: "First kept message", ts: firstKeptTs }, - { role: "assistant", content: "Response", ts: 9 }, + { role: "user", content: "Summary", ts: firstKeptTs - 1, isSummary: true, condenseId }, + { role: "assistant", content: "First kept message", ts: firstKeptTs }, + { role: "user", content: "Response", ts: 9 }, ] // Look up by first kept message's timestamp @@ -330,7 +330,7 @@ describe("Rewind After Condense - Issue #8295", () => { { role: "user", content: "Now the tests", ts: 700, condenseParent: condenseId }, // Summary inserted before first kept message { - role: "assistant", + role: "user", content: "Summary: Built API with validation, working on tests", ts: 799, // msg8.ts - 1 isSummary: true, @@ -345,12 +345,12 @@ describe("Rewind After Condense - Issue #8295", () => { const effective = getEffectiveApiHistory(storageAfterCondense) // Should send exactly 4 messages to LLM: - // 1. Summary (assistant) + // 1. Summary (user) // 2-4. Last 3 kept messages expect(effective.length).toBe(4) // Verify exact order and content - expect(effective[0].role).toBe("assistant") + expect(effective[0].role).toBe("user") expect(effective[0].isSummary).toBe(true) expect(effective[0].content).toBe("Summary: Built API with validation, working on tests") @@ -394,7 +394,7 @@ describe("Rewind After Condense - Issue #8295", () => { // First summary - now ALSO tagged with condenseId2 (from second condense) { - role: "assistant", + role: "user", content: "Summary1: Built auth and database", ts: 799, isSummary: true, @@ -416,7 +416,7 @@ describe("Rewind After Condense - Issue #8295", () => { // Second summary - inserted before the last 3 kept messages { - role: "assistant", + role: "user", content: "Summary2: App complete with auth, DB, API, validation, errors, logging. Now testing.", ts: 1799, // msg18.ts - 1 isSummary: true, @@ -432,12 +432,12 @@ describe("Rewind After Condense - Issue #8295", () => { const effective = getEffectiveApiHistory(storageAfterDoubleCondense) // Should send exactly 4 messages to LLM: - // 1. Summary2 (assistant) - the ACTIVE summary + // 1. Summary2 (user) - the ACTIVE summary // 2-4. Last 3 kept messages expect(effective.length).toBe(4) // Verify exact order and content - expect(effective[0].role).toBe("assistant") + expect(effective[0].role).toBe("user") expect(effective[0].isSummary).toBe(true) expect(effective[0].condenseId).toBe(condenseId2) // Must be the SECOND summary expect(effective[0].content).toContain("Summary2") @@ -477,7 +477,7 @@ describe("Rewind After Condense - Issue #8295", () => { { role: "user", content: "Start task", ts: 100, condenseParent: condenseId }, { role: "assistant", content: "Response 1", ts: 200, condenseParent: condenseId }, { role: "user", content: "Continue", ts: 300, condenseParent: condenseId }, - { role: "assistant", content: "Summary text", ts: 399, isSummary: true, condenseId }, + { role: "user", content: "Summary text", ts: 399, isSummary: true, condenseId }, // Kept messages - should alternate properly { role: "assistant", content: "Response after summary", ts: 400 }, { role: "user", content: "User message", ts: 500 }, @@ -486,10 +486,9 @@ describe("Rewind After Condense - Issue #8295", () => { const effective = getEffectiveApiHistory(storage) - // Verify the sequence: assistant(summary), assistant, user, assistant - // Note: Having two assistant messages in a row (summary + next response) is valid - // because the summary replaces what would have been multiple messages - expect(effective[0].role).toBe("assistant") + // Verify the sequence: user(summary), assistant, user, assistant + // This is the fresh-start model with user-role summaries + expect(effective[0].role).toBe("user") expect(effective[0].isSummary).toBe(true) expect(effective[1].role).toBe("assistant") expect(effective[2].role).toBe("user") @@ -502,10 +501,10 @@ describe("Rewind After Condense - Issue #8295", () => { const storage: ApiMessage[] = [ { role: "user", content: "First", ts: 100, condenseParent: condenseId }, { role: "assistant", content: "Condensed", ts: 200, condenseParent: condenseId }, - { role: "assistant", content: "Summary", ts: 299, isSummary: true, condenseId }, - { role: "user", content: "Kept 1", ts: 300 }, - { role: "assistant", content: "Kept 2", ts: 400 }, - { role: "user", content: "Kept 3", ts: 500 }, + { role: "user", content: "Summary", ts: 299, isSummary: true, condenseId }, + { role: "assistant", content: "Kept 1", ts: 300 }, + { role: "user", content: "Kept 2", ts: 400 }, + { role: "assistant", content: "Kept 3", ts: 500 }, ] const effective = getEffectiveApiHistory(storage) diff --git a/src/core/condense/index.ts b/src/core/condense/index.ts index 95c1728bd43..313bfcebb6b 100644 --- a/src/core/condense/index.ts +++ b/src/core/condense/index.ts @@ -372,53 +372,22 @@ ${commandBlocks} return { messages: newMessages, summary, cost, newContextTokens, condenseId } } -/* Returns the list of all messages since the last summary message, including the summary. Returns all messages if there is no summary. */ +/** + * Returns the list of all messages since the last summary message, including the summary. + * Returns all messages if there is no summary. + * + * Note: Summary messages are always created with role: "user" (fresh-start model), + * so the first message since the last summary is guaranteed to be a user message. + */ export function getMessagesSinceLastSummary(messages: ApiMessage[]): ApiMessage[] { - let lastSummaryIndexReverse = [...messages].reverse().findIndex((message) => message.isSummary) + const lastSummaryIndexReverse = [...messages].reverse().findIndex((message) => message.isSummary) if (lastSummaryIndexReverse === -1) { return messages } const lastSummaryIndex = messages.length - lastSummaryIndexReverse - 1 - const messagesSinceSummary = messages.slice(lastSummaryIndex) - - // Bedrock requires the first message to be a user message. - // We preserve the original first message to maintain context. - // See https://github.com/RooCodeInc/Roo-Code/issues/4147 - if (messagesSinceSummary.length > 0 && messagesSinceSummary[0].role !== "user") { - // Get the original first message (should always be a user message with the task) - const originalFirstMessage = messages[0] - if (originalFirstMessage && originalFirstMessage.role === "user") { - // BUG FIX: Don't prepend the original first message if it has a condenseParent. - // This can happen during nested condensing where the original task was already - // condensed in a previous condense operation. Including it would incorrectly - // add previously-condensed content to the summarization input. - // See: https://github.com/RooCodeInc/Roo-Code/issues/10942 (nested condensing bug) - if (originalFirstMessage.condenseParent) { - // The original first message was already condensed, so we should NOT include it. - // Instead, use a generic placeholder message to satisfy the Bedrock requirement. - const userMessage: ApiMessage = { - role: "user", - content: "Please continue from the following summary:", - ts: messages[0]?.ts ? messages[0].ts - 1 : Date.now(), - } - return [userMessage, ...messagesSinceSummary] - } - // Use the original first message unchanged to maintain full context - return [originalFirstMessage, ...messagesSinceSummary] - } else { - // Fallback to generic message if no original first message exists (shouldn't happen) - const userMessage: ApiMessage = { - role: "user", - content: "Please continue from the following summary:", - ts: messages[0]?.ts ? messages[0].ts - 1 : Date.now(), - } - return [userMessage, ...messagesSinceSummary] - } - } - - return messagesSinceSummary + return messages.slice(lastSummaryIndex) } /** diff --git a/src/core/context-management/__tests__/context-management.spec.ts b/src/core/context-management/__tests__/context-management.spec.ts index 7c5db2d5104..2616ea571df 100644 --- a/src/core/context-management/__tests__/context-management.spec.ts +++ b/src/core/context-management/__tests__/context-management.spec.ts @@ -578,8 +578,8 @@ describe("Context Management", () => { const mockSummarizeResponse: condenseModule.SummarizeResponse = { messages: [ { role: "user", content: "First message" }, - { role: "assistant", content: mockSummary, isSummary: true }, - { role: "user", content: "Last message" }, + { role: "user", content: mockSummary, isSummary: true }, + { role: "assistant", content: "Last message" }, ], summary: mockSummary, cost: mockCost, @@ -751,8 +751,8 @@ describe("Context Management", () => { const mockSummarizeResponse: condenseModule.SummarizeResponse = { messages: [ { role: "user", content: "First message" }, - { role: "assistant", content: mockSummary, isSummary: true }, - { role: "user", content: "Last message" }, + { role: "user", content: mockSummary, isSummary: true }, + { role: "assistant", content: "Last message" }, ], summary: mockSummary, cost: mockCost, @@ -899,8 +899,8 @@ describe("Context Management", () => { const mockSummarizeResponse: condenseModule.SummarizeResponse = { messages: [ { role: "user", content: "First message" }, - { role: "assistant", content: mockSummary, isSummary: true }, - { role: "user", content: "Last message" }, + { role: "user", content: mockSummary, isSummary: true }, + { role: "assistant", content: "Last message" }, ], summary: mockSummary, cost: mockCost, @@ -965,8 +965,8 @@ describe("Context Management", () => { const mockSummarizeResponse: condenseModule.SummarizeResponse = { messages: [ { role: "user", content: "First message" }, - { role: "assistant", content: mockSummary, isSummary: true }, - { role: "user", content: "Last message" }, + { role: "user", content: mockSummary, isSummary: true }, + { role: "assistant", content: "Last message" }, ], summary: mockSummary, cost: mockCost, diff --git a/src/core/webview/__tests__/webviewMessageHandler.delete.spec.ts b/src/core/webview/__tests__/webviewMessageHandler.delete.spec.ts index 541a7106212..1af6b43bc15 100644 --- a/src/core/webview/__tests__/webviewMessageHandler.delete.spec.ts +++ b/src/core/webview/__tests__/webviewMessageHandler.delete.spec.ts @@ -267,7 +267,7 @@ describe("webviewMessageHandler delete functionality", () => { { ts: 100, role: "user", content: "First message", condenseParent: condenseId }, { ts: 200, role: "assistant", content: "Response 1", condenseParent: condenseId }, { ts: 300, role: "user", content: "Second message", condenseParent: condenseId }, - { ts: 799, role: "assistant", content: "Summary", isSummary: true, condenseId }, + { ts: 799, role: "user", content: "Summary", isSummary: true, condenseId }, { ts: 800, role: "assistant", content: "Kept message 1" }, { ts: 900, role: "user", content: "Kept message 2" }, { ts: 1000, role: "assistant", content: "Kept message 3" }, @@ -314,8 +314,8 @@ describe("webviewMessageHandler delete functionality", () => { { ts: 100, role: "user", content: "Task start", condenseParent: condenseId }, { ts: 200, role: "assistant", content: "Response 1", condenseParent: condenseId }, { ts: 300, role: "user", content: "Message 2", condenseParent: condenseId }, - { ts: 999, role: "assistant", content: "Summary", isSummary: true, condenseId }, - { ts: 1000, role: "user", content: "First kept" }, + { ts: 999, role: "user", content: "Summary", isSummary: true, condenseId }, + { ts: 1000, role: "assistant", content: "First kept" }, ] // Delete "Message 2" (ts=300) - this removes summary too, so orphaned tags should be cleared @@ -357,7 +357,7 @@ describe("webviewMessageHandler delete functionality", () => { // First summary - ALSO tagged with condenseId2 from second condense { ts: 799, - role: "assistant", + role: "user", content: "Summary1", isSummary: true, condenseId: condenseId1, @@ -367,7 +367,7 @@ describe("webviewMessageHandler delete functionality", () => { { ts: 1000, role: "assistant", content: "Msg after summary1", condenseParent: condenseId2 }, { ts: 1100, role: "user", content: "More msgs", condenseParent: condenseId2 }, // Second summary - { ts: 1799, role: "assistant", content: "Summary2", isSummary: true, condenseId: condenseId2 }, + { ts: 1799, role: "user", content: "Summary2", isSummary: true, condenseId: condenseId2 }, // Kept messages { ts: 1800, role: "user", content: "Kept1" }, { ts: 1900, role: "assistant", content: "Kept2" }, @@ -407,9 +407,9 @@ describe("webviewMessageHandler delete functionality", () => { // Summary and regular message share timestamp (edge case) getCurrentTaskMock.apiConversationHistory = [ { ts: 900, role: "user", content: "Previous message" }, - { ts: sharedTs, role: "assistant", content: "Summary", isSummary: true, condenseId: "abc" }, - { ts: sharedTs, role: "user", content: "First kept message" }, - { ts: 1100, role: "assistant", content: "Response" }, + { ts: sharedTs, role: "user", content: "Summary", isSummary: true, condenseId: "abc" }, + { ts: sharedTs, role: "assistant", content: "First kept message" }, + { ts: 1100, role: "user", content: "Response" }, ] // Delete at shared timestamp - MessageManager uses ts < cutoffTs, so ALL @@ -450,10 +450,10 @@ describe("webviewMessageHandler delete functionality", () => { { ts: 100, role: "user", content: "Task start", condenseParent: condenseId }, { ts: 200, role: "assistant", content: "Response 1", condenseParent: condenseId }, // Summary timestamp is BEFORE the kept messages (this is the bug scenario) - { ts: 299, role: "assistant", content: "Summary text", isSummary: true, condenseId }, - { ts: 300, role: "user", content: "Message to delete this and after" }, - { ts: 400, role: "assistant", content: "Response 2" }, - { ts: 600, role: "user", content: "Post-condense message" }, + { ts: 299, role: "user", content: "Summary text", isSummary: true, condenseId }, + { ts: 300, role: "assistant", content: "Message to delete this and after" }, + { ts: 400, role: "user", content: "Response 2" }, + { ts: 600, role: "assistant", content: "Post-condense message" }, ] // Delete at ts=300 - this removes condense_context (ts=500), so Summary should be removed too @@ -510,30 +510,30 @@ describe("webviewMessageHandler delete functionality", () => { // First summary (also tagged with condenseId2 from second condense) { ts: 799, - role: "assistant", + role: "user", content: "First summary", isSummary: true, condenseId: condenseId1, condenseParent: condenseId2, }, - { ts: 900, role: "user", content: "After first condense", condenseParent: condenseId2 }, + { ts: 900, role: "assistant", content: "After first condense", condenseParent: condenseId2 }, { ts: 1000, - role: "assistant", + role: "user", content: "Response after 1st condense", condenseParent: condenseId2, }, - { ts: 1100, role: "user", content: "Message to delete this and after" }, + { ts: 1100, role: "assistant", content: "Message to delete this and after" }, // Second summary (timestamp is BEFORE the messages it summarized for sort purposes) { ts: 1799, - role: "assistant", + role: "user", content: "Second summary", isSummary: true, condenseId: condenseId2, }, - { ts: 1900, role: "user", content: "Post second condense" }, - { ts: 2000, role: "assistant", content: "Final response" }, + { ts: 1900, role: "assistant", content: "Post second condense" }, + { ts: 2000, role: "user", content: "Final response" }, ] // Delete at ts=1100 - this removes second condense_context (ts=1800) but keeps first (ts=800)