mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-08-28 05:27:24 +00:00
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
This commit is contained in:
parent
97f968673f
commit
78828e7938
8 changed files with 409 additions and 20 deletions
|
|
@ -225,6 +225,8 @@ export const clineMessageSchema = z.object({
|
|||
reasoning_summary: z.string().optional(),
|
||||
})
|
||||
.optional(),
|
||||
condenseId: z.string().optional(),
|
||||
condenseParent: z.string().optional(),
|
||||
})
|
||||
.optional(),
|
||||
})
|
||||
|
|
|
|||
|
|
@ -84,8 +84,9 @@ describe("Condense", () => {
|
|||
expect(summaryMessage?.content).toBe("Mock summary of the conversation")
|
||||
|
||||
// Verify we have the expected number of messages
|
||||
// [first message, summary, last N messages]
|
||||
expect(result.messages.length).toBe(1 + 1 + N_MESSAGES_TO_KEEP)
|
||||
// With non-destructive condense, we keep ALL messages plus the summary
|
||||
// [first message, middle messages (tagged), summary, last N messages]
|
||||
expect(result.messages.length).toBe(messages.length + 1) // All original messages + 1 summary
|
||||
|
||||
// Verify the last N messages are preserved
|
||||
const lastMessages = result.messages.slice(-N_MESSAGES_TO_KEEP)
|
||||
|
|
|
|||
|
|
@ -188,21 +188,33 @@ describe("summarizeConversation", () => {
|
|||
expect(maybeRemoveImageBlocks).toHaveBeenCalled()
|
||||
|
||||
// Verify the structure of the result
|
||||
// The result should be: first message + summary + last N messages
|
||||
expect(result.messages.length).toBe(1 + 1 + N_MESSAGES_TO_KEEP) // First + summary + last N
|
||||
// With non-destructive condense, all original messages are preserved plus the summary
|
||||
expect(result.messages.length).toBe(messages.length + 1) // All original messages + summary
|
||||
|
||||
// Check that the first message is preserved
|
||||
expect(result.messages[0]).toEqual(messages[0])
|
||||
|
||||
// Check that the summary message was inserted correctly
|
||||
const summaryMessage = result.messages[1]
|
||||
expect(summaryMessage.role).toBe("assistant")
|
||||
expect(summaryMessage.content).toBe("This is a summary")
|
||||
expect(summaryMessage.isSummary).toBe(true)
|
||||
// Find the summary message (it should be inserted after the first message and before tail)
|
||||
const summaryMessage = result.messages.find((m) => m.isSummary)
|
||||
expect(summaryMessage).toBeDefined()
|
||||
expect(summaryMessage?.role).toBe("assistant")
|
||||
expect(summaryMessage?.content).toBe("This is a summary")
|
||||
expect(summaryMessage?.isSummary).toBe(true)
|
||||
expect(summaryMessage?.condenseId).toBeDefined()
|
||||
|
||||
// Check that the last N_MESSAGES_TO_KEEP messages are preserved
|
||||
const lastMessages = messages.slice(-N_MESSAGES_TO_KEEP)
|
||||
expect(result.messages.slice(-N_MESSAGES_TO_KEEP)).toEqual(lastMessages)
|
||||
// Check that middle messages are tagged with condenseParent
|
||||
const middleMessages = result.messages.slice(1, messages.length - N_MESSAGES_TO_KEEP + 1)
|
||||
middleMessages.forEach((msg) => {
|
||||
if (!msg.isSummary) {
|
||||
expect(msg.condenseParent).toBe(summaryMessage?.condenseId)
|
||||
}
|
||||
})
|
||||
|
||||
// Check that the last N_MESSAGES_TO_KEEP messages are preserved without condenseParent
|
||||
const tailMessages = result.messages.slice(-N_MESSAGES_TO_KEEP)
|
||||
tailMessages.forEach((msg) => {
|
||||
expect(msg.condenseParent).toBeUndefined()
|
||||
})
|
||||
|
||||
// Check the cost and token counts
|
||||
expect(result.cost).toBe(0.05)
|
||||
|
|
@ -424,8 +436,8 @@ describe("summarizeConversation", () => {
|
|||
)
|
||||
|
||||
// Should successfully summarize
|
||||
// Result should be: first message + summary + last N messages
|
||||
expect(result.messages.length).toBe(1 + 1 + N_MESSAGES_TO_KEEP) // First + summary + last N
|
||||
// With non-destructive condense, all original messages are preserved plus the summary
|
||||
expect(result.messages.length).toBe(messages.length + 1) // All original messages + summary
|
||||
expect(result.cost).toBe(0.03)
|
||||
expect(result.summary).toBe("Concise summary")
|
||||
expect(result.error).toBeUndefined()
|
||||
|
|
|
|||
281
src/core/condense/__tests__/non-destructive-condense.spec.ts
Normal file
281
src/core/condense/__tests__/non-destructive-condense.spec.ts
Normal file
|
|
@ -0,0 +1,281 @@
|
|||
// npx vitest src/core/condense/__tests__/non-destructive-condense.spec.ts
|
||||
|
||||
import { describe, it, expect, beforeEach, vi } from "vitest"
|
||||
import { summarizeConversation } from "../index"
|
||||
import { ApiMessage } from "../../task-persistence/apiMessages"
|
||||
import { ApiHandler } from "../../../api"
|
||||
|
||||
// Mock the translation function
|
||||
vi.mock("../../../i18n", () => ({
|
||||
t: (key: string) => key,
|
||||
}))
|
||||
|
||||
// Mock TelemetryService
|
||||
vi.mock("@roo-code/telemetry", () => ({
|
||||
TelemetryService: {
|
||||
instance: {
|
||||
captureContextCondensed: vi.fn(),
|
||||
},
|
||||
},
|
||||
}))
|
||||
|
||||
describe("Non-destructive condense", () => {
|
||||
let mockApiHandler: ApiHandler
|
||||
let messages: ApiMessage[]
|
||||
|
||||
beforeEach(() => {
|
||||
// Create a mock API handler
|
||||
mockApiHandler = {
|
||||
createMessage: vi.fn().mockImplementation(() => {
|
||||
// Return an async generator that yields a summary
|
||||
return (async function* () {
|
||||
yield { type: "text", text: "This is a summary of the conversation" }
|
||||
yield { type: "usage", totalCost: 0.01, outputTokens: 50 }
|
||||
})()
|
||||
}),
|
||||
countTokens: vi.fn().mockResolvedValue(100),
|
||||
getModel: vi.fn().mockReturnValue({ info: {} }),
|
||||
} as any
|
||||
|
||||
// Create test messages
|
||||
messages = [
|
||||
{ role: "user", content: "First message", ts: 1000 },
|
||||
{ role: "assistant", content: "Response 1", ts: 2000 },
|
||||
{ role: "user", content: "Second message", ts: 3000 },
|
||||
{ role: "assistant", content: "Response 2", ts: 4000 },
|
||||
{ role: "user", content: "Third message", ts: 5000 },
|
||||
{ role: "assistant", content: "Response 3", ts: 6000 },
|
||||
{ role: "user", content: "Fourth message", ts: 7000 },
|
||||
{ role: "assistant", content: "Response 4", ts: 8000 },
|
||||
]
|
||||
})
|
||||
|
||||
describe("summarizeConversation", () => {
|
||||
it("should preserve all messages with condenseParent tags", async () => {
|
||||
const result = await summarizeConversation(
|
||||
messages,
|
||||
mockApiHandler,
|
||||
"system prompt",
|
||||
"task-123",
|
||||
1000, // prevContextTokens
|
||||
false,
|
||||
)
|
||||
|
||||
// Should not have an error
|
||||
expect(result.error).toBeUndefined()
|
||||
|
||||
// Should have more messages than before (all original + summary)
|
||||
expect(result.messages.length).toBeGreaterThan(messages.length)
|
||||
|
||||
// First message should be preserved
|
||||
expect(result.messages[0]).toEqual(messages[0])
|
||||
|
||||
// Should have a summary message with condenseId
|
||||
const summaryMessage = result.messages.find((m) => m.isSummary)
|
||||
expect(summaryMessage).toBeDefined()
|
||||
expect(summaryMessage?.condenseId).toBeDefined()
|
||||
expect(summaryMessage?.condenseId).toMatch(/^condense-\d+-[a-z0-9]+$/)
|
||||
|
||||
// Middle messages should have condenseParent
|
||||
const middleMessages = result.messages.filter((m) => m.condenseParent)
|
||||
expect(middleMessages.length).toBeGreaterThan(0)
|
||||
expect(middleMessages.every((m) => m.condenseParent === summaryMessage?.condenseId)).toBe(true)
|
||||
|
||||
// Last N messages should not have condenseParent
|
||||
const tailMessages = result.messages.slice(-3) // N_MESSAGES_TO_KEEP = 3
|
||||
expect(tailMessages.every((m) => !m.condenseParent)).toBe(true)
|
||||
})
|
||||
|
||||
it("should generate unique condenseId for each condensation", async () => {
|
||||
const result1 = await summarizeConversation(
|
||||
messages,
|
||||
mockApiHandler,
|
||||
"system prompt",
|
||||
"task-123",
|
||||
1000,
|
||||
false,
|
||||
)
|
||||
|
||||
const result2 = await summarizeConversation(
|
||||
messages,
|
||||
mockApiHandler,
|
||||
"system prompt",
|
||||
"task-123",
|
||||
1000,
|
||||
false,
|
||||
)
|
||||
|
||||
const summaryMessage1 = result1.messages.find((m) => m.isSummary)
|
||||
const summaryMessage2 = result2.messages.find((m) => m.isSummary)
|
||||
|
||||
expect(summaryMessage1?.condenseId).toBeDefined()
|
||||
expect(summaryMessage2?.condenseId).toBeDefined()
|
||||
expect(summaryMessage1?.condenseId).not.toEqual(summaryMessage2?.condenseId)
|
||||
})
|
||||
|
||||
it("should not condense if not enough messages", async () => {
|
||||
const shortMessages = messages.slice(0, 3)
|
||||
const result = await summarizeConversation(
|
||||
shortMessages,
|
||||
mockApiHandler,
|
||||
"system prompt",
|
||||
"task-123",
|
||||
1000,
|
||||
false,
|
||||
)
|
||||
|
||||
expect(result.error).toBe("common:errors.condense_not_enough_messages")
|
||||
expect(result.messages).toEqual(shortMessages)
|
||||
})
|
||||
|
||||
it("should not condense if recent summary exists", async () => {
|
||||
const messagesWithSummary: ApiMessage[] = [
|
||||
...messages.slice(0, -2),
|
||||
{ role: "assistant" as const, content: "Previous summary", ts: 6500, isSummary: true },
|
||||
...messages.slice(-2),
|
||||
]
|
||||
|
||||
const result = await summarizeConversation(
|
||||
messagesWithSummary,
|
||||
mockApiHandler,
|
||||
"system prompt",
|
||||
"task-123",
|
||||
1000,
|
||||
false,
|
||||
)
|
||||
|
||||
expect(result.error).toBe("common:errors.condensed_recently")
|
||||
expect(result.messages).toEqual(messagesWithSummary)
|
||||
})
|
||||
})
|
||||
|
||||
describe("Message filtering with active summaries", () => {
|
||||
it("should filter out messages with condenseParent matching active summary", () => {
|
||||
const messagesWithCondense: ApiMessage[] = [
|
||||
{ role: "user", content: "First", ts: 1000 },
|
||||
{ role: "user", content: "Second", ts: 2000, condenseParent: "condense-123-abc" },
|
||||
{ role: "assistant", content: "Response", ts: 3000, condenseParent: "condense-123-abc" },
|
||||
{
|
||||
role: "assistant",
|
||||
content: "Summary",
|
||||
ts: 4000,
|
||||
isSummary: true,
|
||||
condenseId: "condense-123-abc",
|
||||
},
|
||||
{ role: "user", content: "Latest", ts: 5000 },
|
||||
]
|
||||
|
||||
// Simulate filtering logic from Task.attemptApiRequest
|
||||
const activeCondenseIds = new Set(
|
||||
messagesWithCondense.filter((m) => m.isSummary && m.condenseId).map((m) => m.condenseId!),
|
||||
)
|
||||
|
||||
const effectiveHistory = messagesWithCondense.filter(
|
||||
(m) => !m.condenseParent || !activeCondenseIds.has(m.condenseParent),
|
||||
)
|
||||
|
||||
// Should filter out the middle messages with condenseParent
|
||||
expect(effectiveHistory.length).toBe(3)
|
||||
expect(effectiveHistory[0].content).toBe("First")
|
||||
expect(effectiveHistory[1].content).toBe("Summary")
|
||||
expect(effectiveHistory[2].content).toBe("Latest")
|
||||
})
|
||||
|
||||
it("should include messages with orphaned condenseParent", () => {
|
||||
const messagesWithOrphan: ApiMessage[] = [
|
||||
{ role: "user", content: "First", ts: 1000 },
|
||||
{ role: "user", content: "Second", ts: 2000, condenseParent: "condense-old-xyz" }, // Orphaned
|
||||
{ role: "assistant", content: "Response", ts: 3000 },
|
||||
]
|
||||
|
||||
// No active summaries
|
||||
const activeCondenseIds = new Set(
|
||||
messagesWithOrphan.filter((m) => m.isSummary && m.condenseId).map((m) => m.condenseId!),
|
||||
)
|
||||
|
||||
const effectiveHistory = messagesWithOrphan.filter(
|
||||
(m) => !m.condenseParent || !activeCondenseIds.has(m.condenseParent),
|
||||
)
|
||||
|
||||
// Should include the orphaned message since its condenseParent doesn't match any active summary
|
||||
expect(effectiveHistory.length).toBe(3)
|
||||
})
|
||||
})
|
||||
|
||||
describe("Nested condense support", () => {
|
||||
it("should handle multiple condensations with different condenseIds", async () => {
|
||||
// First condensation
|
||||
const result1 = await summarizeConversation(
|
||||
messages,
|
||||
mockApiHandler,
|
||||
"system prompt",
|
||||
"task-123",
|
||||
1000,
|
||||
false,
|
||||
)
|
||||
|
||||
// Add more messages
|
||||
const extendedMessages: ApiMessage[] = [
|
||||
...result1.messages,
|
||||
{ role: "user" as const, content: "Fifth message", ts: 9000 },
|
||||
{ role: "assistant" as const, content: "Response 5", ts: 10000 },
|
||||
{ role: "user" as const, content: "Sixth message", ts: 11000 },
|
||||
{ role: "assistant" as const, content: "Response 6", ts: 12000 },
|
||||
]
|
||||
|
||||
// Second condensation
|
||||
const result2 = await summarizeConversation(
|
||||
extendedMessages,
|
||||
mockApiHandler,
|
||||
"system prompt",
|
||||
"task-123",
|
||||
1000,
|
||||
false,
|
||||
)
|
||||
|
||||
// Should have two different summaries with different condenseIds
|
||||
const summaries = result2.messages.filter((m) => m.isSummary)
|
||||
expect(summaries.length).toBeGreaterThanOrEqual(1)
|
||||
|
||||
// Messages should have different condenseParent values
|
||||
const condenseParents = new Set(
|
||||
result2.messages.filter((m) => m.condenseParent).map((m) => m.condenseParent),
|
||||
)
|
||||
expect(condenseParents.size).toBeGreaterThanOrEqual(1)
|
||||
})
|
||||
})
|
||||
|
||||
describe("Rollback behavior", () => {
|
||||
it("should support rollback by removing summary and cleaning condenseParent", () => {
|
||||
const messagesWithCondense: ApiMessage[] = [
|
||||
{ role: "user", content: "First", ts: 1000 },
|
||||
{ role: "user", content: "Second", ts: 2000, condenseParent: "condense-123-abc" },
|
||||
{ role: "assistant", content: "Response", ts: 3000, condenseParent: "condense-123-abc" },
|
||||
{
|
||||
role: "assistant",
|
||||
content: "Summary",
|
||||
ts: 4000,
|
||||
isSummary: true,
|
||||
condenseId: "condense-123-abc",
|
||||
},
|
||||
{ role: "user", content: "Latest", ts: 5000 },
|
||||
]
|
||||
|
||||
// Simulate rollback: remove summary
|
||||
const afterRollback = messagesWithCondense.filter((m) => !m.isSummary)
|
||||
|
||||
// Simulate hygiene: clean orphaned condenseParent
|
||||
const activeCondenseIds = new Set(afterRollback.filter((m) => m.condenseId).map((m) => m.condenseId!))
|
||||
|
||||
afterRollback.forEach((m) => {
|
||||
if (m.condenseParent && !activeCondenseIds.has(m.condenseParent)) {
|
||||
delete m.condenseParent
|
||||
}
|
||||
})
|
||||
|
||||
// All messages should have condenseParent removed
|
||||
expect(afterRollback.every((m) => !m.condenseParent)).toBe(true)
|
||||
expect(afterRollback.length).toBe(4)
|
||||
})
|
||||
})
|
||||
})
|
||||
|
|
@ -181,15 +181,28 @@ export async function summarizeConversation(
|
|||
return { ...response, cost, error }
|
||||
}
|
||||
|
||||
// Generate a unique condenseId for this condensation
|
||||
const condenseId = `condense-${Date.now()}-${Math.random().toString(36).substr(2, 9)}`
|
||||
|
||||
// Create the summary message with condenseId
|
||||
const summaryMessage: ApiMessage = {
|
||||
role: "assistant",
|
||||
content: summary,
|
||||
ts: keepMessages[0].ts,
|
||||
isSummary: true,
|
||||
condenseId: condenseId,
|
||||
}
|
||||
|
||||
// Reconstruct messages: [first message, summary, last N messages]
|
||||
const newMessages = [firstMessage, summaryMessage, ...keepMessages]
|
||||
// Tag all middle messages (between first and tail) with condenseParent
|
||||
// Middle messages are those that were summarized but not kept
|
||||
const middleMessages = messages.slice(1, -N_MESSAGES_TO_KEEP).map((msg) => ({
|
||||
...msg,
|
||||
condenseParent: condenseId,
|
||||
}))
|
||||
|
||||
// Reconstruct messages: [first message, tagged middle messages, summary, last N messages]
|
||||
// This preserves ALL messages, with middle ones tagged for filtering
|
||||
const newMessages = [firstMessage, ...middleMessages, summaryMessage, ...keepMessages]
|
||||
|
||||
// Count the tokens in the context for the next API request
|
||||
// We only estimate the tokens in summaryMesage if outputTokens is 0, otherwise we use outputTokens
|
||||
|
|
|
|||
|
|
@ -9,7 +9,12 @@ import { fileExistsAtPath } from "../../utils/fs"
|
|||
import { GlobalFileNames } from "../../shared/globalFileNames"
|
||||
import { getTaskDirectoryPath } from "../../utils/storage"
|
||||
|
||||
export type ApiMessage = Anthropic.MessageParam & { ts?: number; isSummary?: boolean }
|
||||
export type ApiMessage = Anthropic.MessageParam & {
|
||||
ts?: number
|
||||
isSummary?: boolean
|
||||
condenseId?: string
|
||||
condenseParent?: string
|
||||
}
|
||||
|
||||
export async function readApiMessages({
|
||||
taskId,
|
||||
|
|
|
|||
|
|
@ -1048,7 +1048,12 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
|
|||
false /* partial */,
|
||||
undefined /* checkpoint */,
|
||||
undefined /* progressStatus */,
|
||||
{ isNonInteractive: true } /* options */,
|
||||
{
|
||||
isNonInteractive: true,
|
||||
metadata: {
|
||||
condenseId: messages.find((m) => m.condenseId)?.condenseId,
|
||||
},
|
||||
} /* options */,
|
||||
contextCondense,
|
||||
)
|
||||
}
|
||||
|
|
@ -2637,7 +2642,17 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
|
|||
}
|
||||
}
|
||||
|
||||
const messagesSinceLastSummary = getMessagesSinceLastSummary(this.apiConversationHistory)
|
||||
// Filter messages based on active summaries (non-destructive condense)
|
||||
const activeCondenseIds = new Set(
|
||||
this.apiConversationHistory.filter((m) => m.isSummary && m.condenseId).map((m) => m.condenseId!),
|
||||
)
|
||||
|
||||
// Filter out messages that have been condensed (have condenseParent that matches an active summary)
|
||||
const effectiveHistory = this.apiConversationHistory.filter(
|
||||
(m) => !m.condenseParent || !activeCondenseIds.has(m.condenseParent),
|
||||
)
|
||||
|
||||
const messagesSinceLastSummary = getMessagesSinceLastSummary(effectiveHistory)
|
||||
let cleanConversationHistory = maybeRemoveImageBlocks(messagesSinceLastSummary, this.api).map(
|
||||
({ role, content }) => ({ role, content }),
|
||||
)
|
||||
|
|
|
|||
|
|
@ -16,7 +16,7 @@ import {
|
|||
import { CloudService } from "@roo-code/cloud"
|
||||
import { TelemetryService } from "@roo-code/telemetry"
|
||||
|
||||
import { type ApiMessage } from "../task-persistence/apiMessages"
|
||||
import { type ApiMessage, saveApiMessages } from "../task-persistence/apiMessages"
|
||||
import { saveTaskMessages } from "../task-persistence"
|
||||
|
||||
import { ClineProvider } from "./ClineProvider"
|
||||
|
|
@ -112,6 +112,66 @@ export const webviewMessageHandler = async (
|
|||
currentCline.apiConversationHistory.slice(0, apiConversationHistoryIndex),
|
||||
)
|
||||
}
|
||||
|
||||
// Perform hygiene: clean up orphaned condenseParent references
|
||||
await performCondenseHygiene(currentCline)
|
||||
}
|
||||
|
||||
/**
|
||||
* Clean up orphaned condenseParent references after truncation
|
||||
*/
|
||||
const performCondenseHygiene = async (currentCline: any) => {
|
||||
// Find all active condenseIds (from remaining summary messages)
|
||||
const activeCondenseIds = new Set<string>()
|
||||
|
||||
// Check API conversation history for active summaries
|
||||
currentCline.apiConversationHistory.forEach((msg: ApiMessage) => {
|
||||
if (msg.isSummary && msg.condenseId) {
|
||||
activeCondenseIds.add(msg.condenseId)
|
||||
}
|
||||
})
|
||||
|
||||
// Check UI messages for active summaries
|
||||
currentCline.clineMessages.forEach((msg: any) => {
|
||||
if (msg.say === "condense_context" && msg.metadata?.condenseId) {
|
||||
activeCondenseIds.add(msg.metadata.condenseId)
|
||||
}
|
||||
})
|
||||
|
||||
// Clean up orphaned condenseParent references in API history
|
||||
let apiHistoryModified = false
|
||||
currentCline.apiConversationHistory.forEach((msg: ApiMessage) => {
|
||||
if (msg.condenseParent && !activeCondenseIds.has(msg.condenseParent)) {
|
||||
delete msg.condenseParent
|
||||
apiHistoryModified = true
|
||||
}
|
||||
})
|
||||
|
||||
// Clean up orphaned condenseParent references in UI messages
|
||||
let uiMessagesModified = false
|
||||
currentCline.clineMessages.forEach((msg: any) => {
|
||||
if (msg.metadata?.condenseParent && !activeCondenseIds.has(msg.metadata.condenseParent)) {
|
||||
delete msg.metadata.condenseParent
|
||||
uiMessagesModified = true
|
||||
}
|
||||
})
|
||||
|
||||
// Save if any modifications were made
|
||||
if (apiHistoryModified) {
|
||||
await saveApiMessages({
|
||||
messages: currentCline.apiConversationHistory,
|
||||
taskId: currentCline.taskId,
|
||||
globalStoragePath: provider.contextProxy.globalStorageUri.fsPath,
|
||||
})
|
||||
}
|
||||
|
||||
if (uiMessagesModified) {
|
||||
await saveTaskMessages({
|
||||
messages: currentCline.clineMessages,
|
||||
taskId: currentCline.taskId,
|
||||
globalStoragePath: provider.contextProxy.globalStorageUri.fsPath,
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue