From b9a378c72abade72b57fae1f52bd1d85c03474f4 Mon Sep 17 00:00:00 2001 From: Roo Code Date: Mon, 21 Jul 2025 20:12:02 +0000 Subject: [PATCH] fix: only run deduplicateReadFileHistory when read_file tool is used - Removed deduplication call from attemptApiRequest which was running on every API request - Added deduplication call in readFileTool after successful file reads - Added comprehensive tests to verify deduplication only runs when files are actually read - This optimization reduces unnecessary processing on non-read_file API requests --- src/core/task/Task.ts | 2 - .../readFileTool.deduplication.spec.ts | 290 ++++++++++++++++++ src/core/tools/readFileTool.ts | 7 + 3 files changed, 297 insertions(+), 2 deletions(-) create mode 100644 src/core/tools/__tests__/readFileTool.deduplication.spec.ts diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 1c520de86e..23abf7c4c7 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -1670,8 +1670,6 @@ export class Task extends EventEmitter { profileThresholds = {}, } = state ?? {} - this.deduplicateReadFileHistory() - // Get condensing configuration for automatic triggers const customCondensingPrompt = state?.customCondensingPrompt const condensingApiConfigId = state?.condensingApiConfigId diff --git a/src/core/tools/__tests__/readFileTool.deduplication.spec.ts b/src/core/tools/__tests__/readFileTool.deduplication.spec.ts new file mode 100644 index 0000000000..7c64fb26a7 --- /dev/null +++ b/src/core/tools/__tests__/readFileTool.deduplication.spec.ts @@ -0,0 +1,290 @@ +// npx vitest src/core/tools/__tests__/readFileTool.deduplication.spec.ts + +import { describe, it, expect, beforeEach, vi } from "vitest" +import { readFileTool } from "../readFileTool" +import { Task } from "../../task/Task" +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("[]"), + access: vi.fn().mockResolvedValue(undefined), + } +}) + +vi.mock("fs", () => ({ + existsSync: vi.fn().mockReturnValue(true), + readFileSync: vi.fn().mockReturnValue("test file content"), +})) + +vi.mock("isbinaryfile", () => ({ + isBinaryFile: vi.fn().mockResolvedValue(false), +})) + +vi.mock("../../../integrations/misc/line-counter", () => ({ + countFileLines: vi.fn().mockResolvedValue(10), +})) + +vi.mock("../../../integrations/misc/extract-text", () => ({ + extractTextFromFile: vi.fn().mockResolvedValue("1 | test content\n2 | more content"), + addLineNumbers: vi.fn((content) => content), + getSupportedBinaryFormats: vi.fn().mockReturnValue([".pdf", ".docx"]), +})) + +describe("readFileTool deduplication", () => { + let task: Task + let mockProvider: any + let mockApiConfig: ProviderSettings + let mockBlock: any + let mockAskApproval: any + let mockHandleError: any + let mockPushToolResult: any + let mockRemoveClosingTag: any + + 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, + }) + + // Add spy on deduplicateReadFileHistory + vi.spyOn(task, "deduplicateReadFileHistory") + + // Setup existing 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: "existing content of app.ts" }, + { type: "text", text: "metadata" }, + ], + ts: Date.now() - 5 * 60 * 1000, // 5 minutes ago + }, + ] as ApiMessage[] + + // Mock tool use block + mockBlock = { + partial: false, + params: { + args: `src/app.ts`, + }, + } + + // Mock callbacks + mockAskApproval = vi.fn().mockResolvedValue({ + response: "yesButtonClicked", + text: undefined, + images: undefined, + }) + mockHandleError = vi.fn() + mockPushToolResult = vi.fn() + mockRemoveClosingTag = vi.fn() + + // Mock task methods + task.ask = mockAskApproval + task.say = vi.fn().mockResolvedValue(undefined) + task.recordToolError = vi.fn() + task.sayAndCreateMissingParamError = vi.fn() + task.fileContextTracker = { + trackFileContext: vi.fn().mockResolvedValue(undefined), + } as any + task.rooIgnoreController = { + validateAccess: vi.fn().mockReturnValue(true), + } as any + }) + + it("should call deduplicateReadFileHistory after successful read_file operation", async () => { + await readFileTool(task, mockBlock, mockAskApproval, mockHandleError, mockPushToolResult, mockRemoveClosingTag) + + // Verify deduplication was called + expect(task.deduplicateReadFileHistory).toHaveBeenCalledTimes(1) + + // Verify the file was read successfully + expect(mockPushToolResult).toHaveBeenCalled() + const result = mockPushToolResult.mock.calls[0][0] + expect(result).toContain("") + expect(result).toContain("src/app.ts") + expect(result).toContain("") + }) + + it("should deduplicate history correctly after read_file", async () => { + await readFileTool(task, mockBlock, mockAskApproval, mockHandleError, mockPushToolResult, mockRemoveClosingTag) + + // 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 existing entry is still intact + const thirdMessage = task.apiConversationHistory[2] + expect(thirdMessage.content).toHaveLength(3) // Content preserved + expect(thirdMessage.content[1]).toEqual({ type: "text", text: "existing content of app.ts" }) + }) + + it("should not call deduplicateReadFileHistory when read_file is denied", async () => { + // Mock denial response + mockAskApproval.mockResolvedValue({ + response: "noButtonClicked", + text: "User denied", + images: undefined, + }) + + await readFileTool(task, mockBlock, mockAskApproval, mockHandleError, mockPushToolResult, mockRemoveClosingTag) + + // Verify deduplication was NOT called + expect(task.deduplicateReadFileHistory).not.toHaveBeenCalled() + + // Verify the result shows denial + expect(mockPushToolResult).toHaveBeenCalled() + const result = mockPushToolResult.mock.calls[0][0] + expect(result).toContain("Denied by user") + }) + + it("should not call deduplicateReadFileHistory when read_file has an error", async () => { + // Mock RooIgnore validation failure + task.rooIgnoreController!.validateAccess = vi.fn().mockReturnValue(false) + + await readFileTool(task, mockBlock, mockAskApproval, mockHandleError, mockPushToolResult, mockRemoveClosingTag) + + // Verify deduplication was NOT called + expect(task.deduplicateReadFileHistory).not.toHaveBeenCalled() + }) + + it("should call deduplicateReadFileHistory for multiple approved files", async () => { + // Mock block with multiple files + mockBlock.params.args = ` + src/app.ts + src/utils.ts + ` + + // Mock batch approval + mockAskApproval.mockResolvedValue({ + response: "yesButtonClicked", + text: undefined, + images: undefined, + }) + + await readFileTool(task, mockBlock, mockAskApproval, mockHandleError, mockPushToolResult, mockRemoveClosingTag) + + // Verify deduplication was called once after all files were processed + expect(task.deduplicateReadFileHistory).toHaveBeenCalledTimes(1) + + // Verify both files were read + const result = mockPushToolResult.mock.calls[0][0] + expect(result).toContain("src/app.ts") + expect(result).toContain("src/utils.ts") + }) + + it("should call deduplicateReadFileHistory when some files are approved in batch", async () => { + // Mock block with multiple files + mockBlock.params.args = ` + src/app.ts + src/utils.ts + ` + + // Mock the batch approval to simulate the webview's response + // We need to intercept the ask call to get the actual keys used + let actualKeys: string[] = [] + mockAskApproval.mockImplementation(async (type: string, message: string) => { + if (type === "tool") { + const parsed = JSON.parse(message) + if (parsed.batchFiles) { + actualKeys = parsed.batchFiles.map((f: any) => f.key) + } + } + // Return individual permissions with correct keys + return { + response: "objectResponse", + text: JSON.stringify({ + [actualKeys[0]]: true, // Approve first file + [actualKeys[1]]: false, // Deny second file + }), + images: undefined, + } + }) + + await readFileTool(task, mockBlock, mockAskApproval, mockHandleError, mockPushToolResult, mockRemoveClosingTag) + + // Verify deduplication was called since at least one file was approved + expect(task.deduplicateReadFileHistory).toHaveBeenCalledTimes(1) + }) + + it("should not call deduplicateReadFileHistory when all files are denied in batch", async () => { + // Mock block with multiple files + mockBlock.params.args = ` + src/app.ts + src/utils.ts + ` + + // Mock batch denial + mockAskApproval.mockResolvedValue({ + response: "noButtonClicked", + text: "All files denied", + images: undefined, + }) + + await readFileTool(task, mockBlock, mockAskApproval, mockHandleError, mockPushToolResult, mockRemoveClosingTag) + + // Verify deduplication was NOT called + expect(task.deduplicateReadFileHistory).not.toHaveBeenCalled() + }) +}) diff --git a/src/core/tools/readFileTool.ts b/src/core/tools/readFileTool.ts index 6de8dd5642..3010337aee 100644 --- a/src/core/tools/readFileTool.ts +++ b/src/core/tools/readFileTool.ts @@ -589,6 +589,13 @@ export async function readFileTool( // No status message, just push the files XML pushToolResult(filesXml) } + + // Deduplicate read_file history after successful read operations + // This optimizes context length by keeping only the most recent read_file result for each file + const successfulReads = fileResults.filter((result) => result.status === "approved" && result.xmlContent) + if (successfulReads.length > 0) { + cline.deduplicateReadFileHistory() + } } catch (error) { // Handle all errors using per-file format for consistency const relPath = fileEntries[0]?.path || "unknown"