diff --git a/src/core/__tests__/read-file-maxReadFileLine.test.ts b/src/core/__tests__/read-file-maxReadFileLine.test.ts index c3bf80a0433..d668ce333b0 100644 --- a/src/core/__tests__/read-file-maxReadFileLine.test.ts +++ b/src/core/__tests__/read-file-maxReadFileLine.test.ts @@ -10,7 +10,22 @@ import { Cline } from "../Cline" // Mock dependencies jest.mock("../../integrations/misc/line-counter") jest.mock("../../integrations/misc/read-lines") -jest.mock("../../integrations/misc/extract-text") +jest.mock("../../integrations/misc/extract-text", () => { + const actual = jest.requireActual("../../integrations/misc/extract-text") + // Create a spy on the actual addLineNumbers function + const addLineNumbersSpy = jest.spyOn(actual, "addLineNumbers") + + return { + ...actual, + // Expose the spy so tests can access it + __addLineNumbersSpy: addLineNumbersSpy, + extractTextFromFile: jest.fn(), + } +}) + +// Get a reference to the spy +const addLineNumbersSpy = jest.requireMock("../../integrations/misc/extract-text").__addLineNumbersSpy + jest.mock("../../services/tree-sitter") jest.mock("isbinaryfile") jest.mock("../ignore/RooIgnoreController", () => ({ @@ -46,9 +61,9 @@ describe("read_file tool with maxReadFileLine setting", () => { const testFilePath = "test/file.txt" const absoluteFilePath = "/test/file.txt" const fileContent = "Line 1\nLine 2\nLine 3\nLine 4\nLine 5" - const numberedFileContent = "1 | Line 1\n2 | Line 2\n3 | Line 3\n4 | Line 4\n5 | Line 5" + const numberedFileContent = "1 | Line 1\n2 | Line 2\n3 | Line 3\n4 | Line 4\n5 | Line 5\n" const sourceCodeDef = "\n\n# file.txt\n1--5 | Content" - const expectedFullFileXml = `\n ${testFilePath}\n \n${numberedFileContent}\n \n` + const expectedFullFileXml = `${testFilePath}\n\n${numberedFileContent}\n` // Mocked functions with correct types const mockedCountFileLines = countFileLines as jest.MockedFunction @@ -58,6 +73,10 @@ describe("read_file tool with maxReadFileLine setting", () => { const mockedParseSourceCodeDefinitionsForFile = parseSourceCodeDefinitionsForFile as jest.MockedFunction< typeof parseSourceCodeDefinitionsForFile > + + // Variable to control what content is used by the mock - set in beforeEach + let mockInputContent = "" + const mockedIsBinaryFile = isBinaryFile as jest.MockedFunction const mockedPathResolve = path.resolve as jest.MockedFunction @@ -74,13 +93,19 @@ describe("read_file tool with maxReadFileLine setting", () => { // Setup mocks for file operations mockedIsBinaryFile.mockResolvedValue(false) - mockedAddLineNumbers.mockImplementation((content: string, startLine = 1) => { - return content - .split("\n") - .map((line, i) => `${i + startLine} | ${line}`) - .join("\n") + + // Set the default content for the mock + mockInputContent = fileContent + + // Setup the extractTextFromFile mock implementation with the current mockInputContent + mockedExtractTextFromFile.mockImplementation((filePath) => { + const actual = jest.requireActual("../../integrations/misc/extract-text") + return Promise.resolve(actual.addLineNumbers(mockInputContent)) }) + // No need to setup the extractTextFromFile mock implementation here + // as it's already defined at the module level + // Setup mock provider mockProvider = { getState: jest.fn(), @@ -105,16 +130,32 @@ describe("read_file tool with maxReadFileLine setting", () => { /** * Helper function to execute the read file tool with different maxReadFileLine settings */ - async function executeReadFileTool(maxReadFileLine: number, totalLines = 5): Promise { + async function executeReadFileTool( + params: Partial = {}, + options: { + maxReadFileLine?: number + totalLines?: number + skipAddLineNumbersCheck?: boolean // Flag to skip addLineNumbers check + } = {}, + ): Promise { // Configure mocks based on test scenario + const maxReadFileLine = options.maxReadFileLine ?? 500 + const totalLines = options.totalLines ?? 5 + mockProvider.getState.mockResolvedValue({ maxReadFileLine }) mockedCountFileLines.mockResolvedValue(totalLines) + // Reset the spy before each test + addLineNumbersSpy.mockClear() + // Create a tool use object const toolUse: ReadFileToolUse = { type: "tool_use", name: "read_file", - params: { path: testFilePath }, + params: { + path: testFilePath, + ...params, + }, partial: false, } @@ -133,16 +174,23 @@ describe("read_file tool with maxReadFileLine setting", () => { (param: string, value: string) => value, ) + // Verify addLineNumbers was called appropriately + if (!options.skipAddLineNumbersCheck) { + expect(addLineNumbersSpy).toHaveBeenCalled() + } else { + expect(addLineNumbersSpy).not.toHaveBeenCalled() + } + return toolResult } describe("when maxReadFileLine is negative", () => { it("should read the entire file using extractTextFromFile", async () => { - // Setup - mockedExtractTextFromFile.mockResolvedValue(numberedFileContent) + // Setup - use default mockInputContent + mockInputContent = fileContent // Execute - const result = await executeReadFileTool(-1) + const result = await executeReadFileTool({}, { maxReadFileLine: -1 }) // Verify expect(mockedExtractTextFromFile).toHaveBeenCalledWith(absoluteFilePath) @@ -157,8 +205,15 @@ describe("read_file tool with maxReadFileLine setting", () => { // Setup - for maxReadFileLine = 0, the implementation won't call readLines mockedParseSourceCodeDefinitionsForFile.mockResolvedValue(sourceCodeDef) - // Execute - const result = await executeReadFileTool(0) + // Execute - skip addLineNumbers check as it's not called for maxReadFileLine=0 + const result = await executeReadFileTool( + {}, + { + maxReadFileLine: 0, + totalLines: 5, + skipAddLineNumbersCheck: true, + }, + ) // Verify expect(mockedExtractTextFromFile).not.toHaveBeenCalled() @@ -167,8 +222,15 @@ describe("read_file tool with maxReadFileLine setting", () => { absoluteFilePath, mockCline.rooIgnoreController, ) - expect(result).toContain("[Showing only 0 of 5 total lines") - expect(result).toContain(sourceCodeDef) + + // Verify XML structure + expect(result).toContain(`${testFilePath}`) + expect(result).toContain("Showing only 0 of 5 total lines") + expect(result).toContain("") + expect(result).toContain("") + expect(result).toContain(sourceCodeDef.trim()) + expect(result).toContain("") + expect(result).not.toContain(" { mockedParseSourceCodeDefinitionsForFile.mockResolvedValue(sourceCodeDef) // Execute - const result = await executeReadFileTool(3) + const result = await executeReadFileTool({}, { maxReadFileLine: 3 }) // Verify - check behavior but not specific implementation details expect(mockedExtractTextFromFile).not.toHaveBeenCalled() @@ -189,11 +251,21 @@ describe("read_file tool with maxReadFileLine setting", () => { absoluteFilePath, mockCline.rooIgnoreController, ) + + // Verify XML structure + expect(result).toContain(`${testFilePath}`) + expect(result).toContain('') expect(result).toContain("1 | Line 1") expect(result).toContain("2 | Line 2") expect(result).toContain("3 | Line 3") - expect(result).toContain("[Showing only 3 of 5 total lines") - expect(result).toContain(sourceCodeDef) + expect(result).toContain("") + expect(result).toContain("Showing only 3 of 5 total lines") + expect(result).toContain("") + expect(result).toContain("") + expect(result).toContain(sourceCodeDef.trim()) + expect(result).toContain("") + expect(result).toContain("") + expect(result).toContain(sourceCodeDef.trim()) }) }) @@ -201,10 +273,10 @@ describe("read_file tool with maxReadFileLine setting", () => { it("should use extractTextFromFile when maxReadFileLine > totalLines", async () => { // Setup mockedCountFileLines.mockResolvedValue(5) // File shorter than maxReadFileLine - mockedExtractTextFromFile.mockResolvedValue(numberedFileContent) + mockInputContent = fileContent // Execute - const result = await executeReadFileTool(10, 5) + const result = await executeReadFileTool({}, { maxReadFileLine: 10, totalLines: 5 }) // Verify expect(mockedExtractTextFromFile).toHaveBeenCalledWith(absoluteFilePath) @@ -214,15 +286,17 @@ describe("read_file tool with maxReadFileLine setting", () => { it("should read with extractTextFromFile when file has few lines", async () => { // Setup mockedCountFileLines.mockResolvedValue(3) // File shorter than maxReadFileLine - mockedExtractTextFromFile.mockResolvedValue(numberedFileContent) + mockInputContent = fileContent // Execute - const result = await executeReadFileTool(5, 3) + const result = await executeReadFileTool({}, { maxReadFileLine: 5, totalLines: 3 }) // Verify expect(mockedExtractTextFromFile).toHaveBeenCalledWith(absoluteFilePath) expect(mockedReadLines).not.toHaveBeenCalled() - expect(result).toBe(expectedFullFileXml) + // Create a custom expected XML with lines="1-3" since totalLines is 3 + const expectedXml = `${testFilePath}\n\n${numberedFileContent}\n` + expect(result).toBe(expectedXml) }) }) @@ -230,53 +304,64 @@ describe("read_file tool with maxReadFileLine setting", () => { it("should always use extractTextFromFile regardless of maxReadFileLine", async () => { // Setup mockedIsBinaryFile.mockResolvedValue(true) - mockedExtractTextFromFile.mockResolvedValue(numberedFileContent) + // For binary files, we're using a maxReadFileLine of 3 and totalLines is assumed to be 3 + mockedCountFileLines.mockResolvedValue(3) + + // For binary files, we need a special mock implementation that doesn't use addLineNumbers + // Save the original mock implementation + const originalMockImplementation = mockedExtractTextFromFile.getMockImplementation() + // Create a special mock implementation that doesn't call addLineNumbers + mockedExtractTextFromFile.mockImplementation(() => { + return Promise.resolve(numberedFileContent) + }) + + // Reset the spy to clear any previous calls + addLineNumbersSpy.mockClear() + + // Execute - skip addLineNumbers check as we're directly providing the numbered content + const result = await executeReadFileTool( + {}, + { + maxReadFileLine: 3, + totalLines: 3, + skipAddLineNumbersCheck: true, + }, + ) - // Execute - const result = await executeReadFileTool(3) + // Restore the original mock implementation after the test + mockedExtractTextFromFile.mockImplementation(originalMockImplementation) // Verify expect(mockedExtractTextFromFile).toHaveBeenCalledWith(absoluteFilePath) expect(mockedReadLines).not.toHaveBeenCalled() - expect(result).toBe(expectedFullFileXml) + // Create a custom expected XML with lines="1-3" for binary files + const expectedXml = `${testFilePath}\n\n${numberedFileContent}\n` + expect(result).toBe(expectedXml) }) }) describe("with range parameters", () => { it("should honor start_line and end_line when provided", async () => { // Setup - const rangeToolUse: ReadFileToolUse = { - type: "tool_use", - name: "read_file", - params: { - path: testFilePath, - start_line: "2", - end_line: "4", - }, - partial: false, - } - mockedReadLines.mockResolvedValue("Line 2\nLine 3\nLine 4") - // Import the tool implementation dynamically - const { readFileTool } = require("../tools/readFileTool") - - // Execute the tool - let rangeResult: string | undefined - await readFileTool( - mockCline, - rangeToolUse, - mockCline.ask, - jest.fn(), - (result: string) => { - rangeResult = result - }, - (param: string, value: string) => value, - ) + // Execute using executeReadFileTool with range parameters + const rangeResult = await executeReadFileTool({ + start_line: "2", + end_line: "4", + }) // Verify expect(mockedReadLines).toHaveBeenCalledWith(absoluteFilePath, 3, 1) // end_line - 1, start_line - 1 - expect(mockedAddLineNumbers).toHaveBeenCalledWith(expect.any(String), 2) // start with proper line numbers + expect(addLineNumbersSpy).toHaveBeenCalledWith(expect.any(String), 2) // start with proper line numbers + + // Verify XML structure with lines attribute + expect(rangeResult).toContain(`${testFilePath}`) + expect(rangeResult).toContain(``) + expect(rangeResult).toContain("2 | Line 2") + expect(rangeResult).toContain("3 | Line 3") + expect(rangeResult).toContain("4 | Line 4") + expect(rangeResult).toContain("") }) }) }) diff --git a/src/core/__tests__/read-file-xml.test.ts b/src/core/__tests__/read-file-xml.test.ts new file mode 100644 index 00000000000..dda287376af --- /dev/null +++ b/src/core/__tests__/read-file-xml.test.ts @@ -0,0 +1,614 @@ +import * as path from "path" +import { countFileLines } from "../../integrations/misc/line-counter" +import { readLines } from "../../integrations/misc/read-lines" +import { extractTextFromFile, addLineNumbers } from "../../integrations/misc/extract-text" +import { parseSourceCodeDefinitionsForFile } from "../../services/tree-sitter" +import { isBinaryFile } from "isbinaryfile" +import { ReadFileToolUse } from "../assistant-message" +import { Cline } from "../Cline" + +// Mock dependencies +jest.mock("../../integrations/misc/line-counter") +jest.mock("../../integrations/misc/read-lines") +jest.mock("../../integrations/misc/extract-text", () => { + const actual = jest.requireActual("../../integrations/misc/extract-text") + // Create a spy on the actual addLineNumbers function + const addLineNumbersSpy = jest.spyOn(actual, "addLineNumbers") + + return { + ...actual, + // Expose the spy so tests can access it + __addLineNumbersSpy: addLineNumbersSpy, + extractTextFromFile: jest.fn().mockImplementation((filePath) => { + // Use the actual addLineNumbers function + const content = mockInputContent + return Promise.resolve(actual.addLineNumbers(content)) + }), + } +}) + +// Get a reference to the spy +const addLineNumbersSpy = jest.requireMock("../../integrations/misc/extract-text").__addLineNumbersSpy + +// Variable to control what content is used by the mock +let mockInputContent = "" +jest.mock("../../services/tree-sitter") +jest.mock("isbinaryfile") +jest.mock("../ignore/RooIgnoreController", () => ({ + RooIgnoreController: class { + initialize() { + return Promise.resolve() + } + validateAccess() { + return true + } + }, +})) +jest.mock("fs/promises", () => ({ + mkdir: jest.fn().mockResolvedValue(undefined), + writeFile: jest.fn().mockResolvedValue(undefined), + readFile: jest.fn().mockResolvedValue("{}"), +})) +jest.mock("../../utils/fs", () => ({ + fileExistsAtPath: jest.fn().mockReturnValue(true), +})) + +// Mock path +jest.mock("path", () => { + const originalPath = jest.requireActual("path") + return { + ...originalPath, + resolve: jest.fn().mockImplementation((...args) => args.join("/")), + } +}) + +describe("read_file tool XML output structure", () => { + // Test data + const testFilePath = "test/file.txt" + const absoluteFilePath = "/test/file.txt" + const fileContent = "Line 1\nLine 2\nLine 3\nLine 4\nLine 5" + const numberedFileContent = "1 | Line 1\n2 | Line 2\n3 | Line 3\n4 | Line 4\n5 | Line 5\n" + const sourceCodeDef = "\n\n# file.txt\n1--5 | Content" + + // Mocked functions with correct types + const mockedCountFileLines = countFileLines as jest.MockedFunction + const mockedReadLines = readLines as jest.MockedFunction + const mockedExtractTextFromFile = extractTextFromFile as jest.MockedFunction + const mockedParseSourceCodeDefinitionsForFile = parseSourceCodeDefinitionsForFile as jest.MockedFunction< + typeof parseSourceCodeDefinitionsForFile + > + const mockedIsBinaryFile = isBinaryFile as jest.MockedFunction + const mockedPathResolve = path.resolve as jest.MockedFunction + + // Mock instances + const mockCline: any = {} + let mockProvider: any + let toolResult: string | undefined + + beforeEach(() => { + jest.clearAllMocks() + + // Setup path resolution + mockedPathResolve.mockReturnValue(absoluteFilePath) + + // Setup mocks for file operations + mockedIsBinaryFile.mockResolvedValue(false) + + // Set the default content for the mock + mockInputContent = fileContent + + // Setup mock provider + mockProvider = { + getState: jest.fn().mockResolvedValue({ maxReadFileLine: 500 }), + deref: jest.fn().mockReturnThis(), + } + + // Setup Cline instance with mock methods + mockCline.cwd = "/" + mockCline.task = "Test" + mockCline.providerRef = mockProvider + mockCline.rooIgnoreController = { + validateAccess: jest.fn().mockReturnValue(true), + } + mockCline.say = jest.fn().mockResolvedValue(undefined) + mockCline.ask = jest.fn().mockResolvedValue(true) + mockCline.presentAssistantMessage = jest.fn() + mockCline.sayAndCreateMissingParamError = jest.fn().mockResolvedValue("Missing required parameter") + + // Reset tool result + toolResult = undefined + }) + + /** + * Helper function to execute the read file tool with custom parameters + */ + async function executeReadFileTool( + params: Partial = {}, + options: { + totalLines?: number + maxReadFileLine?: number + isBinary?: boolean + validateAccess?: boolean + skipAddLineNumbersCheck?: boolean // Flag to skip addLineNumbers check + } = {}, + ): Promise { + // Configure mocks based on test scenario + const totalLines = options.totalLines ?? 5 + const maxReadFileLine = options.maxReadFileLine ?? 500 + const isBinary = options.isBinary ?? false + const validateAccess = options.validateAccess ?? true + + mockProvider.getState.mockResolvedValue({ maxReadFileLine }) + mockedCountFileLines.mockResolvedValue(totalLines) + mockedIsBinaryFile.mockResolvedValue(isBinary) + mockCline.rooIgnoreController.validateAccess = jest.fn().mockReturnValue(validateAccess) + + // Create a tool use object + const toolUse: ReadFileToolUse = { + type: "tool_use", + name: "read_file", + params: { + path: testFilePath, + ...params, + }, + partial: false, + } + + // Import the tool implementation dynamically to avoid hoisting issues + const { readFileTool } = require("../tools/readFileTool") + + // Reset the spy's call history before each test + addLineNumbersSpy.mockClear() + + // Execute the tool + await readFileTool( + mockCline, + toolUse, + mockCline.ask, + jest.fn(), + (result: string) => { + toolResult = result + }, + (param: string, value: string) => value, + ) + // Verify addLineNumbers was called (unless explicitly skipped) + if (!options.skipAddLineNumbersCheck) { + expect(addLineNumbersSpy).toHaveBeenCalled() + } else { + // For cases where we expect addLineNumbers NOT to be called + expect(addLineNumbersSpy).not.toHaveBeenCalled() + } + + return toolResult + } + + describe("Basic XML Structure Tests", () => { + it("should produce XML output with no unnecessary indentation", async () => { + // Setup - use default mockInputContent (fileContent) + mockInputContent = fileContent + + // Execute + const result = await executeReadFileTool() + + // Verify + expect(result).toBe( + `${testFilePath}\n\n${numberedFileContent}\n`, + ) + }) + + it("should follow the correct XML structure format", async () => { + // Setup - use default mockInputContent (fileContent) + mockInputContent = fileContent + + // Execute + const result = await executeReadFileTool() + + // Verify using regex to check structure + const xmlStructureRegex = new RegExp( + `^${testFilePath}\\n\\n.*\\n$`, + "s", + ) + expect(result).toMatch(xmlStructureRegex) + }) + }) + + describe("Line Range Tests", () => { + it("should include lines attribute when start_line is specified", async () => { + // Setup + const startLine = 2 + mockedReadLines.mockResolvedValue( + fileContent + .split("\n") + .slice(startLine - 1) + .join("\n"), + ) + + // Execute + const result = await executeReadFileTool({ start_line: startLine.toString() }) + + // Verify + expect(result).toContain(``) + }) + + it("should include lines attribute when end_line is specified", async () => { + // Setup + const endLine = 3 + mockedReadLines.mockResolvedValue(fileContent.split("\n").slice(0, endLine).join("\n")) + + // Execute + const result = await executeReadFileTool({ end_line: endLine.toString() }) + + // Verify + expect(result).toContain(``) + }) + + it("should include lines attribute when both start_line and end_line are specified", async () => { + // Setup + const startLine = 2 + const endLine = 4 + mockedReadLines.mockResolvedValue( + fileContent + .split("\n") + .slice(startLine - 1, endLine) + .join("\n"), + ) + + // Execute + const result = await executeReadFileTool({ + start_line: startLine.toString(), + end_line: endLine.toString(), + }) + + // Verify + expect(result).toContain(``) + }) + + it("should include lines attribute even when no range is specified", async () => { + // Setup - use default mockInputContent (fileContent) + mockInputContent = fileContent + + // Execute + const result = await executeReadFileTool() + + // Verify + expect(result).toContain(`\n`) + }) + + it("should include content when maxReadFileLine=0 and range is specified", async () => { + // Setup + const maxReadFileLine = 0 + const startLine = 2 + const endLine = 4 + const totalLines = 10 + + mockedReadLines.mockResolvedValue( + fileContent + .split("\n") + .slice(startLine - 1, endLine) + .join("\n"), + ) + + // Execute + const result = await executeReadFileTool( + { + start_line: startLine.toString(), + end_line: endLine.toString(), + }, + { maxReadFileLine, totalLines }, + ) + + // Verify + // Should include content tag with line range + expect(result).toContain(``) + + // Should NOT include definitions (range reads never show definitions) + expect(result).not.toContain("") + + // Should NOT include truncation notice + expect(result).not.toContain(`Showing only ${maxReadFileLine} of ${totalLines} total lines`) + }) + + it("should include content when maxReadFileLine=0 and only start_line is specified", async () => { + // Setup + const maxReadFileLine = 0 + const startLine = 3 + const totalLines = 10 + + mockedReadLines.mockResolvedValue( + fileContent + .split("\n") + .slice(startLine - 1) + .join("\n"), + ) + + // Execute + const result = await executeReadFileTool( + { + start_line: startLine.toString(), + }, + { maxReadFileLine, totalLines }, + ) + + // Verify + // Should include content tag with line range + expect(result).toContain(``) + + // Should NOT include definitions (range reads never show definitions) + expect(result).not.toContain("") + + // Should NOT include truncation notice + expect(result).not.toContain(`Showing only ${maxReadFileLine} of ${totalLines} total lines`) + }) + + it("should include content when maxReadFileLine=0 and only end_line is specified", async () => { + // Setup + const maxReadFileLine = 0 + const endLine = 3 + const totalLines = 10 + + mockedReadLines.mockResolvedValue(fileContent.split("\n").slice(0, endLine).join("\n")) + + // Execute + const result = await executeReadFileTool( + { + end_line: endLine.toString(), + }, + { maxReadFileLine, totalLines }, + ) + + // Verify + // Should include content tag with line range + expect(result).toContain(``) + + // Should NOT include definitions (range reads never show definitions) + expect(result).not.toContain("") + + // Should NOT include truncation notice + expect(result).not.toContain(`Showing only ${maxReadFileLine} of ${totalLines} total lines`) + }) + + it("should include full range content when maxReadFileLine=5 and content has more than 5 lines", async () => { + // Setup + const maxReadFileLine = 5 + const startLine = 2 + const endLine = 8 + const totalLines = 10 + + // Create mock content with 7 lines (more than maxReadFileLine) + const rangeContent = Array(endLine - startLine + 1) + .fill("Range line content") + .join("\n") + + mockedReadLines.mockResolvedValue(rangeContent) + + // Execute + const result = await executeReadFileTool( + { + start_line: startLine.toString(), + end_line: endLine.toString(), + }, + { maxReadFileLine, totalLines }, + ) + + // Verify + // Should include content tag with the full requested range (not limited by maxReadFileLine) + expect(result).toContain(``) + + // Should NOT include definitions (range reads never show definitions) + expect(result).not.toContain("") + + // Should NOT include truncation notice + expect(result).not.toContain(`Showing only ${maxReadFileLine} of ${totalLines} total lines`) + + // Should contain all the requested lines, not just maxReadFileLine lines + expect(result).toBeDefined() + if (result) { + expect(result.split("\n").length).toBeGreaterThan(maxReadFileLine) + } + }) + }) + + describe("Notice and Definition Tags Tests", () => { + it("should include notice tag for truncated files", async () => { + // Setup + const maxReadFileLine = 3 + const totalLines = 10 + mockedReadLines.mockResolvedValue(fileContent.split("\n").slice(0, maxReadFileLine).join("\n")) + + // Execute + const result = await executeReadFileTool({}, { maxReadFileLine, totalLines }) + + // Verify + expect(result).toContain(`Showing only ${maxReadFileLine} of ${totalLines} total lines`) + }) + + it("should include list_code_definition_names tag when source code definitions are available", async () => { + // Setup + const maxReadFileLine = 3 + const totalLines = 10 + mockedReadLines.mockResolvedValue(fileContent.split("\n").slice(0, maxReadFileLine).join("\n")) + mockedParseSourceCodeDefinitionsForFile.mockResolvedValue(sourceCodeDef) + + // Execute + const result = await executeReadFileTool({}, { maxReadFileLine, totalLines }) + + // Verify + // Use regex to match the tag content regardless of whitespace + expect(result).toMatch( + new RegExp( + `[\\s\\S]*${sourceCodeDef.trim()}[\\s\\S]*`, + ), + ) + }) + + it("should only have definitions, no content when maxReadFileLine=0", async () => { + // Setup + const maxReadFileLine = 0 + const totalLines = 10 + // Mock content with exactly 10 lines to match totalLines + const rawContent = Array(10).fill("Line content").join("\n") + mockInputContent = rawContent + mockedParseSourceCodeDefinitionsForFile.mockResolvedValue(sourceCodeDef) + + // Execute - skip addLineNumbers check as it's not called for maxReadFileLine=0 + const result = await executeReadFileTool({}, { maxReadFileLine, totalLines, skipAddLineNumbersCheck: true }) + + // Verify + expect(result).toContain(`Showing only 0 of ${totalLines} total lines`) + // Use regex to match the tag content regardless of whitespace + expect(result).toMatch( + new RegExp( + `[\\s\\S]*${sourceCodeDef.trim()}[\\s\\S]*`, + ), + ) + expect(result).not.toContain(` { + // Setup + const maxReadFileLine = 0 + const totalLines = 10 + // Mock that no source code definitions are available + mockedParseSourceCodeDefinitionsForFile.mockResolvedValue("") + // Mock content with exactly 10 lines to match totalLines + const rawContent = Array(10).fill("Line content").join("\n") + mockInputContent = rawContent + + // Execute - skip addLineNumbers check as it's not called for maxReadFileLine=0 + const result = await executeReadFileTool({}, { maxReadFileLine, totalLines, skipAddLineNumbersCheck: true }) + + // Verify + // Should include notice + expect(result).toContain( + `${testFilePath}\nShowing only 0 of ${totalLines} total lines. Use start_line and end_line if you need to read more\n`, + ) + // Should not include list_code_definition_names tag since there are no definitions + expect(result).not.toContain("") + // Should not include content tag for non-empty files with maxReadFileLine=0 + expect(result).not.toContain(" { + it("should include error tag for invalid path", async () => { + // Setup - missing path parameter + const toolUse: ReadFileToolUse = { + type: "tool_use", + name: "read_file", + params: {}, + partial: false, + } + + // Import the tool implementation dynamically + const { readFileTool } = require("../tools/readFileTool") + + // Execute the tool + await readFileTool( + mockCline, + toolUse, + mockCline.ask, + jest.fn(), + (result: string) => { + toolResult = result + }, + (param: string, value: string) => value, + ) + + // Verify + expect(toolResult).toContain(``) + expect(toolResult).not.toContain(` { + // Execute - skip addLineNumbers check as it returns early with an error + const result = await executeReadFileTool({ start_line: "invalid" }, { skipAddLineNumbersCheck: true }) + + // Verify + expect(result).toContain(`${testFilePath}Invalid start_line value`) + expect(result).not.toContain(` { + // Execute - skip addLineNumbers check as it returns early with an error + const result = await executeReadFileTool({ end_line: "invalid" }, { skipAddLineNumbersCheck: true }) + + // Verify + expect(result).toContain(`${testFilePath}Invalid end_line value`) + expect(result).not.toContain(` { + // Execute - skip addLineNumbers check as it returns early with an error + const result = await executeReadFileTool({}, { validateAccess: false, skipAddLineNumbersCheck: true }) + + // Verify + expect(result).toContain(`${testFilePath}`) + expect(result).not.toContain(` { + it("should handle empty files correctly with maxReadFileLine=-1", async () => { + // Setup - use empty string + mockInputContent = "" + const maxReadFileLine = -1 + const totalLines = 0 + mockedCountFileLines.mockResolvedValue(totalLines) + + // Execute + const result = await executeReadFileTool({}, { maxReadFileLine, totalLines }) + + // Verify + // Empty files should include a content tag and notice + expect(result).toBe(`${testFilePath}\nFile is empty\n`) + // And make sure there's no error + expect(result).not.toContain(``) + }) + + it("should handle empty files correctly with maxReadFileLine=0", async () => { + // Setup - use empty string + mockInputContent = "" + const maxReadFileLine = 0 + const totalLines = 0 + mockedCountFileLines.mockResolvedValue(totalLines) + + // Execute + const result = await executeReadFileTool({}, { maxReadFileLine, totalLines }) + + // Verify + // Empty files should include a content tag and notice even with maxReadFileLine=0 + expect(result).toBe(`${testFilePath}\nFile is empty\n`) + }) + + it("should handle binary files correctly", async () => { + // Setup + // For binary content, we need to override the mock since we don't use addLineNumbers + mockedExtractTextFromFile.mockResolvedValue("Binary content") + + // Execute - skip addLineNumbers check as we're directly mocking extractTextFromFile + const result = await executeReadFileTool({}, { isBinary: true, skipAddLineNumbersCheck: true }) + + // Verify + expect(result).toBe( + `${testFilePath}\n\nBinary content\n`, + ) + expect(mockedExtractTextFromFile).toHaveBeenCalledWith(absoluteFilePath) + }) + + it("should handle file read errors correctly", async () => { + // Setup + const errorMessage = "File not found" + // For error cases, we need to override the mock to simulate a failure + mockedExtractTextFromFile.mockRejectedValue(new Error(errorMessage)) + + // Execute - skip addLineNumbers check as it throws an error + const result = await executeReadFileTool({}, { skipAddLineNumbersCheck: true }) + + // Verify + expect(result).toContain( + `${testFilePath}Error reading file: ${errorMessage}`, + ) + expect(result).not.toContain(`${errorMsg}`) return } @@ -66,7 +67,7 @@ export async function readFileTool( // Invalid start_line cline.consecutiveMistakeCount++ await cline.say("error", `Failed to parse start_line: ${startLineStr}`) - pushToolResult(formatResponse.toolError("Invalid start_line value")) + pushToolResult(`${relPath}Invalid start_line value`) return } startLine -= 1 // Convert to 0-based index @@ -80,7 +81,7 @@ export async function readFileTool( // Invalid end_line cline.consecutiveMistakeCount++ await cline.say("error", `Failed to parse end_line: ${endLineStr}`) - pushToolResult(formatResponse.toolError("Invalid end_line value")) + pushToolResult(`${relPath}Invalid end_line value`) return } @@ -91,8 +92,8 @@ export async function readFileTool( const accessAllowed = cline.rooIgnoreController?.validateAccess(relPath) if (!accessAllowed) { await cline.say("rooignore_error", relPath) - pushToolResult(formatResponse.toolError(formatResponse.rooIgnoreError(relPath))) - + const errorMsg = formatResponse.rooIgnoreError(relPath) + pushToolResult(`${relPath}${errorMsg}`) return } @@ -159,23 +160,69 @@ export async function readFileTool( content = res[0].length > 0 ? addLineNumbers(res[0]) : "" const result = res[1] if (result) { - sourceCodeDef = `\n\n${result}` + sourceCodeDef = `${result}` } } else { // Read entire file content = await extractTextFromFile(absolutePath) } + // Create variables to store XML components + let xmlInfo = "" + let contentTag = "" + // Add truncation notice if applicable if (isFileTruncated) { - content += `\n\n[Showing only ${maxReadFileLine} of ${totalLines} total lines. Use start_line and end_line if you need to read more]${sourceCodeDef}` + xmlInfo += `Showing only ${maxReadFileLine} of ${totalLines} total lines. Use start_line and end_line if you need to read more\n` + + // Add source code definitions if available + if (sourceCodeDef) { + xmlInfo += `${sourceCodeDef}\n` + } + } + + // Empty files (zero lines) + if (content === "" && totalLines === 0) { + // Always add self-closing content tag and notice for empty files + contentTag = `` + xmlInfo += `File is empty\n` + } + // Range reads should always show content regardless of maxReadFileLine + else if (isRangeRead) { + // Create content tag with line range information + let lineRangeAttr = "" + const displayStartLine = startLine !== undefined ? startLine + 1 : 1 + const displayEndLine = endLine !== undefined ? endLine + 1 : totalLines + lineRangeAttr = ` lines="${displayStartLine}-${displayEndLine}"` + + // Maintain exact format expected by tests + contentTag = `\n${content}\n` + } + // maxReadFileLine=0 for non-range reads + else if (maxReadFileLine === 0) { + // Skip content tag for maxReadFileLine=0 (definitions only mode) + contentTag = "" + } + // Normal case: non-empty files with content (non-range reads) + else { + // For non-range reads, always show line range + let lines = totalLines + if (maxReadFileLine >= 0 && totalLines > maxReadFileLine) { + lines = maxReadFileLine + } + const lineRangeAttr = ` lines="1-${lines}"` + + // Maintain exact format expected by tests + contentTag = `\n${content}\n` } // Format the result into the required XML structure - const xmlResult = `\n ${relPath}\n \n${content}\n \n` + const xmlResult = `${relPath}\n${contentTag}${xmlInfo}` pushToolResult(xmlResult) } } catch (error) { + const errorMsg = error instanceof Error ? error.message : String(error) + pushToolResult(`${relPath || ""}Error reading file: ${errorMsg}`) await handleError("reading file", error) } } diff --git a/src/integrations/misc/__tests__/extract-text.test.ts b/src/integrations/misc/__tests__/extract-text.test.ts index f7dd0af4e2e..4107d4399a1 100644 --- a/src/integrations/misc/__tests__/extract-text.test.ts +++ b/src/integrations/misc/__tests__/extract-text.test.ts @@ -9,32 +9,63 @@ import { describe("addLineNumbers", () => { it("should add line numbers starting from 1 by default", () => { const input = "line 1\nline 2\nline 3" - const expected = "1 | line 1\n2 | line 2\n3 | line 3" + const expected = "1 | line 1\n2 | line 2\n3 | line 3\n" expect(addLineNumbers(input)).toBe(expected) }) it("should add line numbers starting from specified line number", () => { const input = "line 1\nline 2\nline 3" - const expected = "10 | line 1\n11 | line 2\n12 | line 3" + const expected = "10 | line 1\n11 | line 2\n12 | line 3\n" expect(addLineNumbers(input, 10)).toBe(expected) }) it("should handle empty content", () => { - expect(addLineNumbers("")).toBe("1 | ") - expect(addLineNumbers("", 5)).toBe("5 | ") + expect(addLineNumbers("")).toBe("") + expect(addLineNumbers("", 5)).toBe("5 | \n") }) it("should handle single line content", () => { - expect(addLineNumbers("single line")).toBe("1 | single line") - expect(addLineNumbers("single line", 42)).toBe("42 | single line") + expect(addLineNumbers("single line")).toBe("1 | single line\n") + expect(addLineNumbers("single line", 42)).toBe("42 | single line\n") }) it("should pad line numbers based on the highest line number", () => { const input = "line 1\nline 2" // When starting from 99, highest line will be 100, so needs 3 spaces padding - const expected = " 99 | line 1\n100 | line 2" + const expected = " 99 | line 1\n100 | line 2\n" expect(addLineNumbers(input, 99)).toBe(expected) }) + + it("should preserve trailing newline without adding extra line numbers", () => { + const input = "line 1\nline 2\n" + const expected = "1 | line 1\n2 | line 2\n" + expect(addLineNumbers(input)).toBe(expected) + }) + + it("should handle multiple blank lines correctly", () => { + const input = "line 1\n\n\n\nline 2" + const expected = "1 | line 1\n2 | \n3 | \n4 | \n5 | line 2\n" + expect(addLineNumbers(input)).toBe(expected) + }) + + it("should handle multiple trailing newlines correctly", () => { + const input = "line 1\nline 2\n\n\n" + const expected = "1 | line 1\n2 | line 2\n3 | \n4 | \n" + expect(addLineNumbers(input)).toBe(expected) + }) + + it("should handle numbered trailing newline correctly", () => { + const input = "Line 1\nLine 2\nLine 3\nLine 4\nLine 5\nLine 6\nLine 7\nLine 8\nLine 9\nLine 10\n\n" + const expected = + " 1 | Line 1\n 2 | Line 2\n 3 | Line 3\n 4 | Line 4\n 5 | Line 5\n 6 | Line 6\n 7 | Line 7\n 8 | Line 8\n 9 | Line 9\n10 | Line 10\n11 | \n" + expect(addLineNumbers(input)).toBe(expected) + }) + + it("should handle only blank lines with offset correctly", () => { + const input = "\n\n\n" + const expected = "10 | \n11 | \n12 | \n" + expect(addLineNumbers(input, 10)).toBe(expected) + }) }) describe("everyLineHasLineNumbers", () => { diff --git a/src/integrations/misc/__tests__/read-lines.test.ts b/src/integrations/misc/__tests__/read-lines.test.ts index ce0bfc0de33..14456d24f1d 100644 --- a/src/integrations/misc/__tests__/read-lines.test.ts +++ b/src/integrations/misc/__tests__/read-lines.test.ts @@ -18,17 +18,23 @@ describe("nthline", () => { describe("readLines function", () => { it("should read lines from start when from_line is not provided", async () => { const lines = await readLines(testFile, 2) - expect(lines).toEqual(["Line 1", "Line 2", "Line 3"].join("\n")) + // Expect lines with trailing newline because it exists in the file at that point + const expected = ["Line 1", "Line 2", "Line 3"].join("\n") + "\n" + expect(lines).toEqual(expected) }) it("should read a range of lines from a file", async () => { const lines = await readLines(testFile, 3, 1) - expect(lines).toEqual(["Line 2", "Line 3", "Line 4"].join("\n")) + // Expect lines with trailing newline because it exists in the file at that point + const expected = ["Line 2", "Line 3", "Line 4"].join("\n") + "\n" + expect(lines).toEqual(expected) }) it("should read lines when to_line equals from_line", async () => { const lines = await readLines(testFile, 2, 2) - expect(lines).toEqual("Line 3") + // Expect line with trailing newline because it exists in the file at that point + const expected = "Line 3\n" + expect(lines).toEqual(expected) }) it("should throw error for negative to_line", async () => { @@ -39,15 +45,15 @@ describe("nthline", () => { it("should handle negative from_line by clamping to 0", async () => { const lines = await readLines(testFile, 3, -1) - expect(lines).toEqual(["Line 1", "Line 2", "Line 3", "Line 4"].join("\n")) + expect(lines).toEqual(["Line 1", "Line 2", "Line 3", "Line 4"].join("\n") + "\n") }) it("should floor non-integer line numbers", async () => { const linesWithNonIntegerStart = await readLines(testFile, 3, 1.5) - expect(linesWithNonIntegerStart).toEqual(["Line 2", "Line 3", "Line 4"].join("\n")) + expect(linesWithNonIntegerStart).toEqual(["Line 2", "Line 3", "Line 4"].join("\n") + "\n") const linesWithNonIntegerEnd = await readLines(testFile, 3.5) - expect(linesWithNonIntegerEnd).toEqual(["Line 1", "Line 2", "Line 3", "Line 4"].join("\n")) + expect(linesWithNonIntegerEnd).toEqual(["Line 1", "Line 2", "Line 3", "Line 4"].join("\n") + "\n") }) it("should throw error when from_line > to_line", async () => { @@ -64,5 +70,63 @@ describe("nthline", () => { it("should throw error if from_line is beyond file length", async () => { await expect(readLines(testFile, 20, 15)).rejects.toThrow("does not exist") }) + + // Helper function to create a temporary file, run a test, and clean up + async function withTempFile(filename: string, content: string, testFn: (filepath: string) => Promise) { + const filepath = path.join(__dirname, filename) + await fs.writeFile(filepath, content) + try { + await testFn(filepath) + } finally { + await fs.unlink(filepath) + } + } + + it("should handle empty files", async () => { + await withTempFile("empty.txt", "", async (filepath) => { + await expect(readLines(filepath, 0, 0)).rejects.toThrow("does not exist") + }) + }) + + it("should handle files with only one line without carriage return", async () => { + await withTempFile("single-line-no-cr.txt", "Single line", async (filepath) => { + const lines = await readLines(filepath, 0, 0) + expect(lines).toEqual("Single line") + }) + }) + + it("should handle files with only one line with carriage return", async () => { + await withTempFile("single-line-with-cr.txt", "Single line\n", async (filepath) => { + const lines = await readLines(filepath, 0, 0) + expect(lines).toEqual("Single line\n") + }) + }) + + it("should read the entire file when no startLine or endLine is specified", async () => { + const content = await readLines(testFile) + expect(content).toEqual(Array.from({ length: 10 }, (_, i) => `Line ${i + 1}`).join("\n")) + }) + + it("should handle files with different line endings", async () => { + await withTempFile("mixed-endings.txt", "Line 1\rLine 2\r\nLine 3\n", async (filepath) => { + const lines = await readLines(filepath, 2) + expect(lines).toEqual("Line 1\rLine 2\r\nLine 3\n") + }) + }) + + it("should handle files with Unicode characters", async () => { + await withTempFile("unicode.txt", "Line 1 😀\nLine 2 你好\nLine 3 こんにちは\n", async (filepath) => { + const lines = await readLines(filepath, 1) + expect(lines).toEqual("Line 1 😀\nLine 2 你好\n") + }) + }) + + it("should handle files containing only carriage returns", async () => { + await withTempFile("cr-only.txt", "\n\n\n\n\n", async (filepath) => { + // Read lines 1-3 (second, third, and fourth lines) + const lines = await readLines(filepath, 3, 1) + expect(lines).toEqual("\n\n\n") + }) + }) }) }) diff --git a/src/integrations/misc/extract-text.ts b/src/integrations/misc/extract-text.ts index 04604cbd266..1616370b7e7 100644 --- a/src/integrations/misc/extract-text.ts +++ b/src/integrations/misc/extract-text.ts @@ -55,14 +55,29 @@ async function extractTextFromIPYNB(filePath: string): Promise { } export function addLineNumbers(content: string, startLine: number = 1): string { + // If content is empty, return empty string - empty files should not have line numbers + // If content is empty but startLine > 1, return "startLine | " because we know the file is not empty + // but the content is empty at that line offset + if (content === "") { + return startLine === 1 ? "" : `${startLine} | \n` + } + + // Split into lines and handle trailing newlines const lines = content.split("\n") + const lastLineEmpty = lines[lines.length - 1] === "" + if (lastLineEmpty) { + lines.pop() + } + const maxLineNumberWidth = String(startLine + lines.length - 1).length - return lines + const numberedContent = lines .map((line, index) => { const lineNumber = String(startLine + index).padStart(maxLineNumberWidth, " ") return `${lineNumber} | ${line}` }) .join("\n") + + return numberedContent + "\n" } // Checks if every line in the content has line numbers prefixed (e.g., "1 | content" or "123 | content") // Line numbers must be followed by a single pipe character (not double pipes) diff --git a/src/integrations/misc/read-lines.ts b/src/integrations/misc/read-lines.ts index 2b3780aeed8..1c5db87acb3 100644 --- a/src/integrations/misc/read-lines.ts +++ b/src/integrations/misc/read-lines.ts @@ -53,35 +53,64 @@ export function readLines(filepath: string, endLine?: number, startLine?: number ) } - let cursor = 0 - const lines: string[] = [] + // Set up stream const input = createReadStream(filepath) - const rl = createInterface({ input }) + let buffer = "" + let lineCount = 0 + let result = "" - rl.on("line", (line) => { - // Only collect lines within the specified range - if (cursor >= effectiveStartLine && (endLine === undefined || cursor <= endLine)) { - lines.push(line) - } + // Handle errors + input.on("error", reject) + + // Process data chunks directly + input.on("data", (chunk) => { + // Add chunk to buffer + buffer += chunk.toString() + + let pos = 0 + let nextNewline = buffer.indexOf("\n", pos) + + // Process complete lines in the buffer + while (nextNewline !== -1) { + // If we're in the target range, add this line to the result + if (lineCount >= effectiveStartLine && (endLine === undefined || lineCount <= endLine)) { + result += buffer.substring(pos, nextNewline + 1) // Include the newline + } + + // Move position and increment line counter + pos = nextNewline + 1 + lineCount++ + + // If we've reached the end line, we can stop + if (endLine !== undefined && lineCount > endLine) { + input.destroy() + resolve(result) + return + } - // Close stream after reaching to_line (if specified) - if (endLine !== undefined && cursor === endLine) { - rl.close() - input.close() - resolve(lines.join("\n")) + // Find next newline + nextNewline = buffer.indexOf("\n", pos) } - cursor++ + // Trim buffer - keep only the incomplete line + buffer = buffer.substring(pos) }) - rl.on("error", reject) - + // Handle end of file input.on("end", () => { - // If we collected some lines but didn't reach to_line, return what we have - if (lines.length > 0) { - resolve(lines.join("\n")) - } else { + // Process any remaining data in buffer (last line without newline) + if (buffer.length > 0) { + if (lineCount >= effectiveStartLine && (endLine === undefined || lineCount <= endLine)) { + result += buffer + } + lineCount++ + } + + // Check if we found any lines in the requested range + if (lineCount <= effectiveStartLine) { reject(outOfRangeError(filepath, effectiveStartLine)) + } else { + resolve(result) } }) })