diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 37ea859cf2..1c520de86e 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -1895,37 +1895,89 @@ export class Task extends EventEmitter { yield* iterator } + /** + * Deduplicates read_file tool results in the conversation history to optimize context length. + * Keeps only the most recent read_file result for each file, removing older occurrences. + * + * This method: + * - Uses a Map for O(n) performance instead of nested loops + * - Preserves messages within the LLM cache window (30 minutes) + * - Validates array operations to prevent runtime errors + * - Uses robust pattern matching for tool identification + * + * @remarks + * The method assumes read_file messages have a specific structure: + * - First content item contains text like "[read_file for path/to/file]" + * - Second content item contains the actual file content + * - Third content item contains additional metadata + */ deduplicateReadFileHistory() { + const CACHE_WINDOW_MS = 30 * 60 * 1000 // 30 minutes + const currentTime = Date.now() + + // Map to track the most recent occurrence of each file + const fileOccurrences = new Map() + + // Pattern to match read_file tool use more robustly + const READ_FILE_PATTERN = /^\[read_file for (.+?)\]/ + + // First pass: identify all read_file occurrences + for (let i = 0; i < this.apiConversationHistory.length; i++) { + const conversation = this.apiConversationHistory[i] + + // Skip non-user messages + if (conversation.role !== "user") continue + + // Validate content structure + const content = conversation.content + if (typeof content === "string" || !Array.isArray(content)) continue + + // Check first item for read_file pattern + const firstItem = content[0] + if (!firstItem || typeof firstItem !== "object" || firstItem.type !== "text") continue + + const match = firstItem.text?.match(READ_FILE_PATTERN) + if (!match) continue + + const filePath = match[1] + const messageTimestamp = conversation.ts + + // Store the most recent occurrence of each file + fileOccurrences.set(filePath, { index: i, timestamp: messageTimestamp }) + } + + // Second pass: remove older occurrences for (let i = this.apiConversationHistory.length - 1; i >= 0; i--) { const conversation = this.apiConversationHistory[i] if (conversation.role !== "user") continue const content = conversation.content - if (typeof content === "string") continue + if (typeof content === "string" || !Array.isArray(content)) continue const firstItem = content[0] - if (typeof firstItem === "string" || !("type" in firstItem) || firstItem.type !== "text") continue + if (!firstItem || typeof firstItem !== "object" || firstItem.type !== "text") continue - const toolUseText = firstItem.text - if (!toolUseText || !toolUseText.startsWith("[read_file for ")) continue + const match = firstItem.text?.match(READ_FILE_PATTERN) + if (!match) continue - for (let j = i - 1; j >= 0; j--) { - const prevConversation = this.apiConversationHistory[j] + const filePath = match[1] + const mostRecent = fileOccurrences.get(filePath) - if (prevConversation.role === "assistant") continue + // Skip if this is the most recent occurrence + if (mostRecent && mostRecent.index === i) continue - const prevContent = prevConversation.content - if (typeof prevContent === "string") continue + // Check if message is within cache window + const messageTimestamp = conversation.ts + if (messageTimestamp && currentTime - messageTimestamp < CACHE_WINDOW_MS) { + // Preserve messages within cache window + continue + } - const prevFirstItem = prevContent[0] - if (typeof prevFirstItem === "string" || !("type" in prevFirstItem) || prevFirstItem.type !== "text") - continue - - if (prevFirstItem.text === toolUseText && prevContent.length === 3) { - prevContent.splice(1, 1) - break - } + // Safely remove the file content (second item) if structure is as expected + if (content.length >= 2 && content[1]?.type === "text") { + // Remove the file content while preserving the tool use and any additional items + content.splice(1, 1) } } } diff --git a/src/core/task/__tests__/Task.deduplicateReadFileHistory.spec.ts b/src/core/task/__tests__/Task.deduplicateReadFileHistory.spec.ts new file mode 100644 index 0000000000..9b28adb3e6 --- /dev/null +++ b/src/core/task/__tests__/Task.deduplicateReadFileHistory.spec.ts @@ -0,0 +1,410 @@ +// npx vitest core/task/__tests__/Task.deduplicateReadFileHistory.spec.ts + +import { describe, it, expect, beforeEach, vi } from "vitest" +import { Task } from "../Task" +import { ClineProvider } from "../../webview/ClineProvider" +import type { ProviderSettings } from "@roo-code/types" +import type { ApiMessage } from "../../task-persistence/apiMessages" +import { TelemetryService } from "@roo-code/telemetry" + +// Mock dependencies +vi.mock("delay", () => ({ + __esModule: true, + default: vi.fn().mockResolvedValue(undefined), +})) + +vi.mock("../../environment/getEnvironmentDetails", () => ({ + getEnvironmentDetails: vi.fn().mockResolvedValue(""), +})) + +vi.mock("../../ignore/RooIgnoreController") + +vi.mock("fs/promises", async (importOriginal) => { + const actual = (await importOriginal()) as Record + return { + ...actual, + mkdir: vi.fn().mockResolvedValue(undefined), + writeFile: vi.fn().mockResolvedValue(undefined), + readFile: vi.fn().mockResolvedValue("[]"), + } +}) + +describe("Task.deduplicateReadFileHistory", () => { + let task: Task + let mockProvider: any + let mockApiConfig: ProviderSettings + + beforeEach(() => { + // Initialize TelemetryService if not already initialized + if (!TelemetryService.hasInstance()) { + TelemetryService.createInstance([]) + } + + // Setup mock provider + mockProvider = { + context: { + globalStorageUri: { fsPath: "/test/storage" }, + }, + postStateToWebview: vi.fn().mockResolvedValue(undefined), + postMessageToWebview: vi.fn().mockResolvedValue(undefined), + updateTaskHistory: vi.fn().mockResolvedValue(undefined), + getState: vi.fn().mockResolvedValue({}), + } + + // Setup mock API configuration + mockApiConfig = { + apiProvider: "anthropic", + apiModelId: "claude-3-5-sonnet-20241022", + apiKey: "test-api-key", + } + + // Create task instance + task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + }) + + it("should remove older read_file entries for the same file", () => { + // Setup conversation history with duplicate read_file entries + task.apiConversationHistory = [ + { + role: "user", + content: [ + { type: "text", text: "[read_file for src/app.ts]" }, + { type: "text", text: "old content of app.ts" }, + { type: "text", text: "metadata" }, + ], + ts: Date.now() - 60 * 60 * 1000, // 1 hour ago + }, + { + role: "assistant", + content: [{ type: "text", text: "I see the file content." }], + }, + { + role: "user", + content: [ + { type: "text", text: "[read_file for src/app.ts]" }, + { type: "text", text: "new content of app.ts" }, + { type: "text", text: "metadata" }, + ], + ts: Date.now() - 5 * 60 * 1000, // 5 minutes ago + }, + ] as ApiMessage[] + + task.deduplicateReadFileHistory() + + // Check that older entry had its content removed + const firstMessage = task.apiConversationHistory[0] + expect(firstMessage.content).toHaveLength(2) // Content was removed + expect(firstMessage.content[0]).toEqual({ type: "text", text: "[read_file for src/app.ts]" }) + expect(firstMessage.content[1]).toEqual({ type: "text", text: "metadata" }) + + // Check that newer entry is intact + const thirdMessage = task.apiConversationHistory[2] + expect(thirdMessage.content).toHaveLength(3) // Content preserved + expect(thirdMessage.content[1]).toEqual({ type: "text", text: "new content of app.ts" }) + }) + + it("should preserve messages within the 30-minute cache window", () => { + const currentTime = Date.now() + + task.apiConversationHistory = [ + { + role: "user", + content: [ + { type: "text", text: "[read_file for src/app.ts]" }, + { type: "text", text: "old content" }, + { type: "text", text: "metadata" }, + ], + ts: currentTime - 20 * 60 * 1000, // 20 minutes ago (within cache window) + }, + { + role: "user", + content: [ + { type: "text", text: "[read_file for src/app.ts]" }, + { type: "text", text: "new content" }, + { type: "text", text: "metadata" }, + ], + ts: currentTime - 5 * 60 * 1000, // 5 minutes ago + }, + ] as ApiMessage[] + + task.deduplicateReadFileHistory() + + // Both messages should be intact due to cache window + expect(task.apiConversationHistory[0].content).toHaveLength(3) + expect(task.apiConversationHistory[1].content).toHaveLength(3) + }) + + it("should handle messages without timestamps", () => { + task.apiConversationHistory = [ + { + role: "user", + content: [ + { type: "text", text: "[read_file for src/app.ts]" }, + { type: "text", text: "content without timestamp" }, + { type: "text", text: "metadata" }, + ], + // No ts property + }, + { + role: "user", + content: [ + { type: "text", text: "[read_file for src/app.ts]" }, + { type: "text", text: "newer content" }, + { type: "text", text: "metadata" }, + ], + ts: Date.now(), + }, + ] as ApiMessage[] + + task.deduplicateReadFileHistory() + + // First message should have content removed (no timestamp means it's old) + expect(task.apiConversationHistory[0].content).toHaveLength(2) + expect(task.apiConversationHistory[1].content).toHaveLength(3) + }) + + it("should handle different file paths correctly", () => { + task.apiConversationHistory = [ + { + role: "user", + content: [ + { type: "text", text: "[read_file for src/app.ts]" }, + { type: "text", text: "app.ts content" }, + { type: "text", text: "metadata" }, + ], + }, + { + role: "user", + content: [ + { type: "text", text: "[read_file for src/utils.ts]" }, + { type: "text", text: "utils.ts content" }, + { type: "text", text: "metadata" }, + ], + }, + { + role: "user", + content: [ + { type: "text", text: "[read_file for src/app.ts]" }, + { type: "text", text: "newer app.ts content" }, + { type: "text", text: "metadata" }, + ], + }, + ] as ApiMessage[] + + task.deduplicateReadFileHistory() + + // First app.ts should have content removed + expect(task.apiConversationHistory[0].content).toHaveLength(2) + + // utils.ts should be intact (no duplicate) + expect(task.apiConversationHistory[1].content).toHaveLength(3) + + // Second app.ts should be intact (most recent) + expect(task.apiConversationHistory[2].content).toHaveLength(3) + }) + + it("should handle malformed messages gracefully", () => { + task.apiConversationHistory = [ + { + role: "user", + content: "string content instead of array", + }, + { + role: "user", + content: [], + }, + { + role: "user", + content: [{ type: "image", source: { type: "base64", media_type: "image/png", data: "..." } }], + }, + { + role: "user", + content: [{ type: "text", text: "Not a read_file message" }], + }, + { + role: "assistant", + content: [ + { type: "text", text: "[read_file for src/app.ts]" }, // Assistant message should be skipped + ], + }, + ] as ApiMessage[] + + // Should not throw + expect(() => task.deduplicateReadFileHistory()).not.toThrow() + + // All messages should remain unchanged + expect(task.apiConversationHistory[0].content).toBe("string content instead of array") + expect(task.apiConversationHistory[1].content).toHaveLength(0) + expect(task.apiConversationHistory[2].content).toHaveLength(1) + expect(task.apiConversationHistory[3].content).toHaveLength(1) + expect(task.apiConversationHistory[4].content).toHaveLength(1) + }) + + it("should handle edge cases in content structure", () => { + task.apiConversationHistory = [ + { + role: "user", + content: [ + { type: "text", text: "[read_file for src/app.ts]" }, + // Missing second item + ], + }, + { + role: "user", + content: [ + { type: "text", text: "[read_file for src/utils.ts]" }, + { type: "image", source: { type: "base64", media_type: "image/png", data: "..." } }, // Not text + { type: "text", text: "metadata" }, + ], + }, + { + role: "user", + content: [ + { type: "text", text: "[read_file for src/app.ts]" }, + { type: "text", text: "content" }, + // Only 2 items instead of expected 3 + ], + }, + ] as ApiMessage[] + + // Should handle gracefully without throwing + expect(() => task.deduplicateReadFileHistory()).not.toThrow() + }) + + it("should match various read_file patterns", () => { + task.apiConversationHistory = [ + { + role: "user", + content: [ + { type: "text", text: "[read_file for path/with spaces/file.ts]" }, + { type: "text", text: "content 1" }, + ], + }, + { + role: "user", + content: [ + { type: "text", text: "[read_file for path/with-dashes/file.ts]" }, + { type: "text", text: "content 2" }, + ], + }, + { + role: "user", + content: [ + { type: "text", text: "[read_file for ../relative/path/file.ts]" }, + { type: "text", text: "content 3" }, + ], + }, + { + role: "user", + content: [ + { type: "text", text: "[read_file for path/with spaces/file.ts]" }, + { type: "text", text: "newer content 1" }, + ], + }, + ] as ApiMessage[] + + task.deduplicateReadFileHistory() + + // First occurrence should have content removed + expect(task.apiConversationHistory[0].content).toHaveLength(1) + + // Others should be intact + expect(task.apiConversationHistory[1].content).toHaveLength(2) + expect(task.apiConversationHistory[2].content).toHaveLength(2) + expect(task.apiConversationHistory[3].content).toHaveLength(2) + }) + + it("should not match invalid read_file patterns", () => { + task.apiConversationHistory = [ + { + role: "user", + content: [ + { type: "text", text: "read_file for src/app.ts" }, // Missing brackets + { type: "text", text: "content" }, + ], + }, + { + role: "user", + content: [ + { type: "text", text: "[read_file src/app.ts]" }, // Missing "for" + { type: "text", text: "content" }, + ], + }, + { + role: "user", + content: [ + { type: "text", text: "[write_file for src/app.ts]" }, // Different tool + { type: "text", text: "content" }, + ], + }, + ] as ApiMessage[] + + const originalHistory = JSON.parse(JSON.stringify(task.apiConversationHistory)) + task.deduplicateReadFileHistory() + + // Nothing should change + expect(task.apiConversationHistory).toEqual(originalHistory) + }) + + it("should handle empty conversation history", () => { + task.apiConversationHistory = [] + + // Should not throw + expect(() => task.deduplicateReadFileHistory()).not.toThrow() + expect(task.apiConversationHistory).toHaveLength(0) + }) + + it("should handle multiple duplicates of the same file", () => { + task.apiConversationHistory = [ + { + role: "user", + content: [ + { type: "text", text: "[read_file for src/app.ts]" }, + { type: "text", text: "version 1" }, + ], + ts: Date.now() - 2 * 60 * 60 * 1000, // 2 hours ago + }, + { + role: "user", + content: [ + { type: "text", text: "[read_file for src/app.ts]" }, + { type: "text", text: "version 2" }, + ], + ts: Date.now() - 1 * 60 * 60 * 1000, // 1 hour ago + }, + { + role: "user", + content: [ + { type: "text", text: "[read_file for src/app.ts]" }, + { type: "text", text: "version 3" }, + ], + ts: Date.now() - 40 * 60 * 1000, // 40 minutes ago + }, + { + role: "user", + content: [ + { type: "text", text: "[read_file for src/app.ts]" }, + { type: "text", text: "version 4 (latest)" }, + ], + ts: Date.now() - 5 * 60 * 1000, // 5 minutes ago + }, + ] as ApiMessage[] + + task.deduplicateReadFileHistory() + + // First two should have content removed (outside cache window) + expect(task.apiConversationHistory[0].content).toHaveLength(1) + expect(task.apiConversationHistory[1].content).toHaveLength(1) + + // Third should be intact (outside cache window but would be removed if not for being older than cache) + expect(task.apiConversationHistory[2].content).toHaveLength(1) + + // Fourth should be intact (most recent) + expect(task.apiConversationHistory[3].content).toHaveLength(2) + }) +})