From 44f4bd519458ad0b605d3631e2cf57ce2abed765 Mon Sep 17 00:00:00 2001 From: hannesrudolph Date: Mon, 28 Jul 2025 14:31:41 -0600 Subject: [PATCH] fix: implement proactive file caching for read_file deduplication - Changed from reactive to proactive deduplication approach - Added getRecentFileContent method to check cache before reading files - Modified readFileTool to use cached content when available - Added comprehensive tests for the new caching functionality - Fixed legacy format handling in getRecentFileContent - Updated test mocks to include new methods --- src/core/task/Task.ts | 73 ++++ src/core/task/__tests__/Task.spec.ts | 332 ++++++++++++++++++ src/core/tools/__tests__/readFileTool.spec.ts | 6 + src/core/tools/readFileTool.ts | 40 ++- 4 files changed, 450 insertions(+), 1 deletion(-) diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 5d946ab28d..8364f0c29c 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -329,6 +329,79 @@ export class Task extends EventEmitter { return readApiMessages({ taskId: this.taskId, globalStoragePath: this.globalStoragePath }) } + public async getRecentFileContent(filePath: string): Promise { + // Check if the experimental feature is enabled + const state = await this.providerRef.deref()?.getState() + if (!state?.experiments || !experiments.isEnabled(state.experiments, EXPERIMENT_IDS.READ_FILE_DEDUPLICATION)) { + return null + } + + // Get the cache window from settings + const cacheMinutes = state?.readFileDeduplicationCacheMinutes ?? 5 + if (cacheMinutes === 0) { + // Cache is disabled + return null + } + + const cacheWindowMs = cacheMinutes * 60 * 1000 + const now = Date.now() + + // Check recent conversation history for this file + for (let i = this.apiConversationHistory.length - 1; i >= 0; i--) { + const message = this.apiConversationHistory[i] + + // Only process user messages + if (message.role !== "user") { + continue + } + + // Skip messages outside the cache window + if (message.ts && now - message.ts > cacheWindowMs) { + break + } + + // Process content blocks + if (Array.isArray(message.content)) { + for (const block of message.content) { + if (block.type === "text" && typeof block.text === "string") { + // Check for read_file results in text blocks + const readFileMatch = block.text.match(/\[read_file(?:\s+for\s+'([^']+)')?.*?\]\s*Result:/i) + + if (readFileMatch) { + // Extract file paths from the result content + const resultContent = block.text.substring(block.text.indexOf("Result:") + 7).trim() + + // Handle new XML format + const xmlFileMatches = resultContent.matchAll( + /\s*([^<]+)<\/path>[\s\S]*?]*?>([\s\S]*?)<\/content>/g, + ) + for (const match of xmlFileMatches) { + const matchedPath = match[1].trim() + const content = match[2].trim() + if (matchedPath === filePath) { + return content + } + } + + // Handle legacy format (single file) + if ( + readFileMatch[1] && + readFileMatch[1] === filePath && + !resultContent.includes("") + ) { + // For legacy format, the content is directly after "Result:" + // Remove any leading/trailing whitespace + return resultContent.trim() + } + } + } + } + } + } + + return null + } + public async deduplicateReadFileHistory(): Promise { // Check if the experimental feature is enabled const state = await this.providerRef.deref()?.getState() diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index 267399ba32..b93fbcfa77 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -2105,5 +2105,337 @@ describe("Cline", () => { expect(cline.apiConversationHistory).toHaveLength(2) }) }) + + describe("getRecentFileContent", () => { + let mockProvider: any + let mockApiConfig: any + let task: Task + + beforeEach(() => { + vi.clearAllMocks() + + mockApiConfig = { + apiProvider: "anthropic", + apiKey: "test-key", + } + + mockProvider = { + context: { + globalStorageUri: { fsPath: "/test/storage" }, + }, + getState: vi.fn().mockResolvedValue({ + experiments: { + [EXPERIMENT_IDS.READ_FILE_DEDUPLICATION]: true, + }, + readFileDeduplicationCacheMinutes: 5, + }), + postStateToWebview: vi.fn().mockResolvedValue(undefined), + postMessageToWebview: vi.fn().mockResolvedValue(undefined), + updateTaskHistory: vi.fn().mockResolvedValue(undefined), + } + + task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + }) + + it("should return null when feature is disabled", async () => { + mockProvider.getState.mockResolvedValue({ + experiments: { + [EXPERIMENT_IDS.READ_FILE_DEDUPLICATION]: false, + }, + }) + + const now = Date.now() + task.apiConversationHistory = [ + { + role: "user", + content: [ + { + type: "text" as const, + text: "[read_file for 'test.ts'] Result:\ntest.tstest content", + }, + ], + ts: now - 1000, // 1 second ago + }, + ] + + const result = await task.getRecentFileContent("test.ts") + expect(result).toBeNull() + }) + + it("should return recent file content within cache window", async () => { + const now = Date.now() + task.apiConversationHistory = [ + { + role: "user", + content: [ + { + type: "text" as const, + text: "[read_file for 'test.ts'] Result:\ntest.tsrecent content", + }, + ], + ts: now - 2 * 60 * 1000, // 2 minutes ago (within 5 minute window) + }, + ] + + const result = await task.getRecentFileContent("test.ts") + expect(result).toBe("recent content") + }) + + it("should return null for files outside cache window", async () => { + const now = Date.now() + task.apiConversationHistory = [ + { + role: "user", + content: [ + { + type: "text" as const, + text: "[read_file for 'test.ts'] Result:\ntest.tsold content", + }, + ], + ts: now - 10 * 60 * 1000, // 10 minutes ago (outside 5 minute window) + }, + ] + + const result = await task.getRecentFileContent("test.ts") + expect(result).toBeNull() + }) + + it("should return most recent content when multiple reads exist", async () => { + const now = Date.now() + task.apiConversationHistory = [ + { + role: "user", + content: [ + { + type: "text" as const, + text: "[read_file for 'test.ts'] Result:\ntest.tsold content", + }, + ], + ts: now - 4 * 60 * 1000, // 4 minutes ago + }, + { + role: "assistant", + content: [{ type: "text" as const, text: "Processing..." }], + ts: now - 3 * 60 * 1000, + }, + { + role: "user", + content: [ + { + type: "text" as const, + text: "[read_file for 'test.ts'] Result:\ntest.tsnewer content", + }, + ], + ts: now - 2 * 60 * 1000, // 2 minutes ago + }, + ] + + const result = await task.getRecentFileContent("test.ts") + expect(result).toBe("newer content") + }) + + it("should handle multi-file reads", async () => { + const now = Date.now() + task.apiConversationHistory = [ + { + role: "user", + content: [ + { + type: "text" as const, + text: "[read_file for 'file1.ts', 'file2.ts'] Result:\nfile1.tscontent1file2.tscontent2", + }, + ], + ts: now - 2 * 60 * 1000, + }, + ] + + const result1 = await task.getRecentFileContent("file1.ts") + expect(result1).toBe("content1") + + const result2 = await task.getRecentFileContent("file2.ts") + expect(result2).toBe("content2") + }) + + it("should return null for non-existent files", async () => { + const now = Date.now() + task.apiConversationHistory = [ + { + role: "user", + content: [ + { + type: "text" as const, + text: "[read_file for 'test.ts'] Result:\ntest.tstest content", + }, + ], + ts: now - 1000, + }, + ] + + const result = await task.getRecentFileContent("other.ts") + expect(result).toBeNull() + }) + + it("should handle legacy format", async () => { + const now = Date.now() + task.apiConversationHistory = [ + { + role: "user", + content: [ + { + type: "text" as const, + text: "[read_file for 'legacy.ts'] Result:\nFile content without XML wrapper", + }, + ], + ts: now - 1000, + }, + ] + + const result = await task.getRecentFileContent("legacy.ts") + expect(result).toBe("File content without XML wrapper") + }) + + it("should ignore assistant messages", async () => { + const now = Date.now() + task.apiConversationHistory = [ + { + role: "assistant", + content: [ + { + type: "text" as const, + text: "[read_file for 'test.ts'] Result:\ntest.tsassistant content", + }, + ], + ts: now - 1000, + }, + ] + + const result = await task.getRecentFileContent("test.ts") + expect(result).toBeNull() + }) + + it("should handle messages without timestamps", async () => { + task.apiConversationHistory = [ + { + role: "user", + content: [ + { + type: "text" as const, + text: "[read_file for 'test.ts'] Result:\ntest.tstest content", + }, + ], + // No ts property - should be treated as recent + }, + ] + + const result = await task.getRecentFileContent("test.ts") + expect(result).toBe("test content") + }) + + it("should use custom cache time from settings", async () => { + mockProvider.getState.mockResolvedValue({ + experiments: { + [EXPERIMENT_IDS.READ_FILE_DEDUPLICATION]: true, + }, + readFileDeduplicationCacheMinutes: 10, + }) + + const now = Date.now() + task.apiConversationHistory = [ + { + role: "user", + content: [ + { + type: "text" as const, + text: "[read_file for 'test.ts'] Result:\ntest.tscontent within 10 min", + }, + ], + ts: now - 8 * 60 * 1000, // 8 minutes ago (within 10 minute window) + }, + ] + + const result = await task.getRecentFileContent("test.ts") + expect(result).toBe("content within 10 min") + }) + + it("should handle 0 cache time (no caching)", async () => { + mockProvider.getState.mockResolvedValue({ + experiments: { + [EXPERIMENT_IDS.READ_FILE_DEDUPLICATION]: true, + }, + readFileDeduplicationCacheMinutes: 0, + }) + + const now = Date.now() + task.apiConversationHistory = [ + { + role: "user", + content: [ + { + type: "text" as const, + text: "[read_file for 'test.ts'] Result:\ntest.tsvery recent content", + }, + ], + ts: now - 100, // 0.1 seconds ago + }, + ] + + const result = await task.getRecentFileContent("test.ts") + expect(result).toBeNull() // With 0 cache time, nothing is cached + }) + + it("should handle malformed content gracefully", async () => { + const now = Date.now() + task.apiConversationHistory = [ + { + role: "user", + content: "string content instead of array", // Invalid format + ts: now - 1000, + }, + { + role: "user", + content: [ + { + type: "image" as const, + source: { type: "base64" as const, media_type: "image/png", data: "..." }, + }, + ], // Non-text block + ts: now - 500, + }, + ] + + const result = await task.getRecentFileContent("test.ts") + expect(result).toBeNull() + }) + + it("should handle empty conversation history", async () => { + task.apiConversationHistory = [] + const result = await task.getRecentFileContent("test.ts") + expect(result).toBeNull() + }) + + it("should handle file paths with special characters", async () => { + const now = Date.now() + task.apiConversationHistory = [ + { + role: "user", + content: [ + { + type: "text" as const, + text: "[read_file for '@scope/package/file.ts'] Result:\n@scope/package/file.tsscoped content", + }, + ], + ts: now - 1000, + }, + ] + + const result = await task.getRecentFileContent("@scope/package/file.ts") + expect(result).toBe("scoped content") + }) + }) }) }) diff --git a/src/core/tools/__tests__/readFileTool.spec.ts b/src/core/tools/__tests__/readFileTool.spec.ts index 448425c92b..d629d3f0df 100644 --- a/src/core/tools/__tests__/readFileTool.spec.ts +++ b/src/core/tools/__tests__/readFileTool.spec.ts @@ -130,6 +130,9 @@ describe("read_file tool with maxReadFileLine setting", () => { // Add the deduplicateReadFileHistory method to the mock mockCline.deduplicateReadFileHistory = vi.fn().mockReturnValue(undefined) + // Add the getRecentFileContent method to the mock + mockCline.getRecentFileContent = vi.fn().mockResolvedValue(null) + toolResult = undefined }) @@ -389,6 +392,9 @@ describe("read_file tool XML output structure", () => { // Add the deduplicateReadFileHistory method to the mock mockCline.deduplicateReadFileHistory = vi.fn().mockReturnValue(undefined) + // Add the getRecentFileContent method to the mock + mockCline.getRecentFileContent = vi.fn().mockResolvedValue(null) + toolResult = undefined }) diff --git a/src/core/tools/readFileTool.ts b/src/core/tools/readFileTool.ts index 2e62a82be5..d9d5d840b0 100644 --- a/src/core/tools/readFileTool.ts +++ b/src/core/tools/readFileTool.ts @@ -431,7 +431,45 @@ export async function readFileTool( const fullPath = path.resolve(cline.cwd, relPath) const { maxReadFileLine = -1 } = (await cline.providerRef.deref()?.getState()) ?? {} - // Process approved files + // Check if we have recent content for this file (deduplication) + const recentContent = await cline.getRecentFileContent(relPath) + if (recentContent !== null) { + // We have recent content, use it instead of reading the file again + const lines = recentContent.split("\n") + const totalLines = lines.length + + // Handle range reads + if (fileResult.lineRanges && fileResult.lineRanges.length > 0) { + const rangeResults: string[] = [] + for (const range of fileResult.lineRanges) { + const selectedLines = lines.slice(range.start - 1, range.end).join("\n") + const content = addLineNumbers(selectedLines, range.start) + const lineRangeAttr = ` lines="${range.start}-${range.end}"` + rangeResults.push(`\n${content}`) + } + updateFileResult(relPath, { + xmlContent: `${relPath}\n${rangeResults.join("\n")}\nUsing cached content from recent read\n`, + }) + continue + } + + // Handle normal file read with cached content + const lineRangeAttr = ` lines="1-${totalLines}"` + let xmlInfo = totalLines > 0 ? `\n${recentContent}\n` : `` + + if (totalLines === 0) { + xmlInfo += `File is empty\n` + } else { + xmlInfo += `Using cached content from recent read\n` + } + + updateFileResult(relPath, { + xmlContent: `${relPath}\n${xmlInfo}`, + }) + continue + } + + // Process approved files (no cached content available) try { const [totalLines, isBinary] = await Promise.all([countFileLines(fullPath), isBinaryFile(fullPath)])