diff --git a/src/core/tools/__tests__/readFileTool.spec.ts b/src/core/tools/__tests__/readFileTool.spec.ts index fb6ba3d119..a8a4b19ea1 100644 --- a/src/core/tools/__tests__/readFileTool.spec.ts +++ b/src/core/tools/__tests__/readFileTool.spec.ts @@ -1435,6 +1435,51 @@ describe("read_file tool XML output structure", () => { partial: false, } + // Create a spy for handleError + const handleErrorSpy = vi.fn() + + // Execute + await readFileTool( + mockCline, + toolUse, + mockCline.ask, + handleErrorSpy, + (result: ToolResponse) => { + toolResult = result + }, + (param: ToolParamName, content?: string) => content ?? "", + ) + + // Verify error is returned for empty path + expect(toolResult).toContain("") + expect(toolResult).toContain("No valid file paths found") + expect(toolResult).toContain("Ensure each element contains a non-empty element") + expect(handleErrorSpy).toHaveBeenCalled() + }) + + it("should work without line_range parameter (line_range is optional)", async () => { + // Test that line_range is truly optional + const argsWithoutLineRange = ` + +test/file.txt + +` + const toolUse: ReadFileToolUse = { + type: "tool_use", + name: "read_file", + params: { args: argsWithoutLineRange }, + partial: false, + } + + // Setup mocks + mockedCountFileLines.mockResolvedValue(5) + mockedExtractTextFromFile.mockResolvedValue("1 | Line 1\n2 | Line 2\n3 | Line 3\n4 | Line 4\n5 | Line 5") + mockProvider.getState.mockResolvedValue({ + maxReadFileLine: -1, + maxImageFileSize: 20, + maxTotalImageSize: 20, + }) + // Execute await readFileTool( mockCline, @@ -1447,9 +1492,82 @@ describe("read_file tool XML output structure", () => { (param: ToolParamName, content?: string) => content ?? "", ) - // Verify error is returned for empty path + // Verify the file was read successfully without line_range + expect(toolResult).toContain("test/file.txt") + expect(toolResult).toContain('') + expect(toolResult).not.toContain("") + expect(toolResult).not.toContain("line_range") + }) + + it("should provide helpful error for malformed XML missing path element", async () => { + // Test case simulating what Grok might send - file element with missing path + const malformedArgs = ` + + + +` + const toolUse: ReadFileToolUse = { + type: "tool_use", + name: "read_file", + params: { args: malformedArgs }, + partial: false, + } + + // Create a spy for handleError + const handleErrorSpy = vi.fn() + + // Execute + await readFileTool( + mockCline, + toolUse, + mockCline.ask, + handleErrorSpy, + (result: ToolResponse) => { + toolResult = result + }, + (param: ToolParamName, content?: string) => content ?? "", + ) + + // Verify helpful error is returned expect(toolResult).toContain("") - expect(mockCline.sayAndCreateMissingParamError).toHaveBeenCalled() + expect(toolResult).toContain("No valid file paths found") + expect(toolResult).toContain("Ensure each element contains a non-empty element") + expect(handleErrorSpy).toHaveBeenCalled() + }) + + it("should provide error when file element has no path child at all", async () => { + // Test case where file element exists but has no path child element + const malformedArgs = ` + + +` + const toolUse: ReadFileToolUse = { + type: "tool_use", + name: "read_file", + params: { args: malformedArgs }, + partial: false, + } + + // Execute + await readFileTool( + mockCline, + toolUse, + mockCline.ask, + vi.fn(), + (result: ToolResponse) => { + toolResult = result + }, + (param: ToolParamName, content?: string) => content ?? "", + ) + + // When file element has no path child, it falls through to the general error + expect(toolResult).toContain("") + expect(toolResult).toContain("Missing required parameter") + // This is handled by sayAndCreateMissingParamError + expect(mockCline.sayAndCreateMissingParamError).toHaveBeenCalledWith( + "read_file", + "args with valid file paths. Expected format: filepath", + ) }) }) }) diff --git a/src/core/tools/readFileTool.ts b/src/core/tools/readFileTool.ts index 01427f4d9d..1e4807c879 100644 --- a/src/core/tools/readFileTool.ts +++ b/src/core/tools/readFileTool.ts @@ -131,11 +131,21 @@ export async function readFileTool( const parsed = parseXml(argsXmlTag) as any const files = Array.isArray(parsed.file) ? parsed.file : [parsed.file].filter(Boolean) + // Track invalid entries for better error reporting + const invalidEntries: string[] = [] + for (const file of files) { - if (!file.path) continue // Skip if no path in a file entry + // Check if path exists and is not empty after trimming + const filePath = typeof file.path === "string" ? file.path.trim() : file.path + + if (!filePath) { + // Track this invalid entry + invalidEntries.push(JSON.stringify(file).substring(0, 100)) + continue // Skip if no path in a file entry + } const fileEntry: FileEntry = { - path: file.path, + path: filePath, lineRanges: [], } @@ -153,8 +163,16 @@ export async function readFileTool( } fileEntries.push(fileEntry) } + + // If we had invalid entries but no valid ones, provide a helpful error + if (fileEntries.length === 0 && invalidEntries.length > 0) { + const errorMessage = `No valid file paths found in read_file args. Invalid entries: ${invalidEntries.join(", ")}. Ensure each element contains a non-empty element.` + await handleError("parsing read_file args", new Error(errorMessage)) + pushToolResult(`${errorMessage}`) + return + } } catch (error) { - const errorMessage = `Failed to parse read_file XML args: ${error instanceof Error ? error.message : String(error)}` + const errorMessage = `Failed to parse read_file XML args: ${error instanceof Error ? error.message : String(error)}. Expected format: filepath` await handleError("parsing read_file args", new Error(errorMessage)) pushToolResult(`${errorMessage}`) return @@ -186,7 +204,10 @@ export async function readFileTool( if (fileEntries.length === 0) { cline.consecutiveMistakeCount++ cline.recordToolError("read_file") - const errorMsg = await cline.sayAndCreateMissingParamError("read_file", "args (containing valid file paths)") + const errorMsg = await cline.sayAndCreateMissingParamError( + "read_file", + "args with valid file paths. Expected format: filepath", + ) pushToolResult(`${errorMsg}`) return }