diff --git a/src/core/assistant-message/__tests__/parseAssistantMessageV2-issue7664.spec.ts b/src/core/assistant-message/__tests__/parseAssistantMessageV2-issue7664.spec.ts new file mode 100644 index 0000000000..7eff52e8ea --- /dev/null +++ b/src/core/assistant-message/__tests__/parseAssistantMessageV2-issue7664.spec.ts @@ -0,0 +1,183 @@ +import { parseAssistantMessageV2 } from "../parseAssistantMessageV2" +import { ToolUse } from "../../../shared/tools" + +describe("parseAssistantMessageV2 - Issue #7664", () => { + describe("handling malformed XML structure from LLMs", () => { + it("should handle the exact structure from juliettefournier-econ's example", () => { + // This is the exact structure that was causing issues + const message = `I'll read that file for you. + + + + +src/shared/infrastructure/supabase/factory.py +10-20 + + +` + + const result = parseAssistantMessageV2(message) + + // Should have text and tool use + expect(result).toHaveLength(2) + expect(result[0].type).toBe("text") + expect(result[1].type).toBe("tool_use") + + const toolUse = result[1] as ToolUse + expect(toolUse.name).toBe("read_file") + expect(toolUse.params.args).toBeDefined() + expect(toolUse.params.args).toContain("") + expect(toolUse.params.args).toContain("src/shared/infrastructure/supabase/factory.py") + expect(toolUse.params.args).toContain("10-20") + }) + + it("should handle structure with no spaces between XML tags", () => { + const message = `test.py1-10` + + const result = parseAssistantMessageV2(message) + + expect(result).toHaveLength(1) + const toolUse = result[0] as ToolUse + expect(toolUse.type).toBe("tool_use") + expect(toolUse.name).toBe("read_file") + expect(toolUse.params.args).toBeDefined() + expect(toolUse.params.args).toContain("test.py") + expect(toolUse.params.args).toContain("1-10") + }) + + it("should handle structure with mixed spacing and newlines", () => { + const message = ` + + src/test.py + 1-10 + + +` + + const result = parseAssistantMessageV2(message) + + expect(result).toHaveLength(1) + const toolUse = result[0] as ToolUse + expect(toolUse.type).toBe("tool_use") + expect(toolUse.name).toBe("read_file") + expect(toolUse.params.args).toBeDefined() + // The args should preserve the internal structure + expect(toolUse.params.args).toContain("") + expect(toolUse.params.args).toContain("") + expect(toolUse.params.args).toContain("src/test.py") + expect(toolUse.params.args).toContain("") + expect(toolUse.params.args).toContain("1-10") + }) + + it("should handle empty path element", () => { + const message = ` + + + +10-20 + + +` + + const result = parseAssistantMessageV2(message) + + expect(result).toHaveLength(1) + const toolUse = result[0] as ToolUse + expect(toolUse.type).toBe("tool_use") + expect(toolUse.name).toBe("read_file") + expect(toolUse.params.args).toBeDefined() + expect(toolUse.params.args).toContain("") + }) + + it("should handle self-closing path element", () => { + const message = ` + + + +10-20 + + +` + + const result = parseAssistantMessageV2(message) + + expect(result).toHaveLength(1) + const toolUse = result[0] as ToolUse + expect(toolUse.type).toBe("tool_use") + expect(toolUse.name).toBe("read_file") + expect(toolUse.params.args).toBeDefined() + expect(toolUse.params.args).toContain("") + }) + + it("should handle multiple files with varying structures", () => { + const message = ` + + + ./file1.ts + + + + ./file2.ts + +10-20 + + +` + + const result = parseAssistantMessageV2(message) + + expect(result).toHaveLength(1) + const toolUse = result[0] as ToolUse + expect(toolUse.type).toBe("tool_use") + expect(toolUse.name).toBe("read_file") + expect(toolUse.params.args).toBeDefined() + // Check that both files are present + expect(toolUse.params.args).toContain("./file1.ts") + expect(toolUse.params.args).toContain("./file2.ts") + expect(toolUse.params.args).toContain("10-20") + }) + + it("should handle partial/incomplete tool use", () => { + const message = ` + + +test.py` + // Message ends abruptly + + const result = parseAssistantMessageV2(message) + + expect(result).toHaveLength(1) + const toolUse = result[0] as ToolUse + expect(toolUse.type).toBe("tool_use") + expect(toolUse.name).toBe("read_file") + expect(toolUse.partial).toBe(true) + expect(toolUse.params.args).toBeDefined() + expect(toolUse.params.args).toContain("test.py") + }) + }) + + describe("args parameter trimming behavior", () => { + it("should trim args parameter content", () => { + const message = ` + + + + test.py + + + +` + + const result = parseAssistantMessageV2(message) + + expect(result).toHaveLength(1) + const toolUse = result[0] as ToolUse + expect(toolUse.params.args).toBeDefined() + // args should be trimmed + expect(toolUse.params.args).not.toMatch(/^\s+/) + expect(toolUse.params.args).not.toMatch(/\s+$/) + expect(toolUse.params.args).toMatch(/^/) + expect(toolUse.params.args).toMatch(/<\/file>$/) + }) + }) +}) diff --git a/src/core/tools/__tests__/readFileTool-empty-path.spec.ts b/src/core/tools/__tests__/readFileTool-empty-path.spec.ts new file mode 100644 index 0000000000..0f3cf78524 --- /dev/null +++ b/src/core/tools/__tests__/readFileTool-empty-path.spec.ts @@ -0,0 +1,130 @@ +import { vi, describe, it, expect, beforeEach } from "vitest" +import { readFileTool } from "../readFileTool" +import { Task } from "../../task/Task" +import { ReadFileToolUse } from "../../../shared/tools" + +describe("readFileTool - empty path handling", () => { + let mockCline: any + let mockAskApproval: any + let mockHandleError: any + let mockPushToolResult: any + let mockRemoveClosingTag: any + + beforeEach(() => { + mockCline = { + cwd: "/test", + consecutiveMistakeCount: 0, + recordToolError: vi.fn(), + sayAndCreateMissingParamError: vi.fn().mockResolvedValue("Missing parameter error"), + say: vi.fn(), + rooIgnoreController: undefined, + api: { + getModel: () => ({ info: { supportsImages: false } }), + }, + } + + mockAskApproval = vi.fn().mockResolvedValue(true) + mockHandleError = vi.fn() + mockPushToolResult = vi.fn() + mockRemoveClosingTag = vi.fn() + }) + + it("should provide clear error message for empty path elements", async () => { + const toolUse: ReadFileToolUse = { + type: "tool_use", + name: "read_file", + params: { + args: ``, + }, + partial: false, + } + + await readFileTool( + mockCline as unknown as Task, + toolUse, + mockAskApproval, + mockHandleError, + mockPushToolResult, + mockRemoveClosingTag, + ) + + // Should handle the error with a clear message + expect(mockHandleError).toHaveBeenCalledWith( + "parsing read_file args", + expect.objectContaining({ + message: "All file paths are empty or missing. Please provide valid file paths in the elements.", + }), + ) + + // Should push the error result + expect(mockPushToolResult).toHaveBeenCalledWith( + `All file paths are empty or missing. Please provide valid file paths in the elements.`, + ) + }) + + it("should provide clear error message for whitespace-only path elements", async () => { + const toolUse: ReadFileToolUse = { + type: "tool_use", + name: "read_file", + params: { + args: ` `, + }, + partial: false, + } + + await readFileTool( + mockCline as unknown as Task, + toolUse, + mockAskApproval, + mockHandleError, + mockPushToolResult, + mockRemoveClosingTag, + ) + + // Should handle the error with a clear message + expect(mockHandleError).toHaveBeenCalledWith( + "parsing read_file args", + expect.objectContaining({ + message: "All file paths are empty or missing. Please provide valid file paths in the elements.", + }), + ) + + // Should push the error result + expect(mockPushToolResult).toHaveBeenCalledWith( + `All file paths are empty or missing. Please provide valid file paths in the elements.`, + ) + }) + + // Note: Testing the case where some paths are empty but others are valid + // would require extensive mocking of file system operations. + // The core functionality is tested by the other tests which verify + // that empty paths are properly detected and reported. + + it("should handle multiple empty paths", async () => { + const toolUse: ReadFileToolUse = { + type: "tool_use", + name: "read_file", + params: { + args: ` `, + }, + partial: false, + } + + await readFileTool( + mockCline as unknown as Task, + toolUse, + mockAskApproval, + mockHandleError, + mockPushToolResult, + mockRemoveClosingTag, + ) + + // Should handle the error with a clear message + expect(mockHandleError).toHaveBeenCalledWith( + "parsing read_file args", + expect.objectContaining({ + message: "All file paths are empty or missing. Please provide valid file paths in the elements.", + }), + ) + }) +}) diff --git a/src/core/tools/readFileTool.ts b/src/core/tools/readFileTool.ts index 01427f4d9d..ddf00ad748 100644 --- a/src/core/tools/readFileTool.ts +++ b/src/core/tools/readFileTool.ts @@ -131,8 +131,18 @@ export async function readFileTool( const parsed = parseXml(argsXmlTag) as any const files = Array.isArray(parsed.file) ? parsed.file : [parsed.file].filter(Boolean) + // Track empty paths for better error reporting + let emptyPathCount = 0 + let totalFileCount = 0 + for (const file of files) { - if (!file.path) continue // Skip if no path in a file entry + totalFileCount++ + + // Check for empty or whitespace-only paths + if (!file.path || (typeof file.path === "string" && file.path.trim() === "")) { + emptyPathCount++ + continue // Skip if no path or empty path in a file entry + } const fileEntry: FileEntry = { path: file.path, @@ -153,6 +163,14 @@ export async function readFileTool( } fileEntries.push(fileEntry) } + + // If all paths were empty, provide a more specific error + if (emptyPathCount > 0 && fileEntries.length === 0) { + const errorMessage = `All file paths are empty or missing. Please provide valid file paths in the elements.` + 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)}` await handleError("parsing read_file args", new Error(errorMessage)) diff --git a/src/utils/__tests__/xml-whitespace.spec.ts b/src/utils/__tests__/xml-whitespace.spec.ts new file mode 100644 index 0000000000..fe9ed13eca --- /dev/null +++ b/src/utils/__tests__/xml-whitespace.spec.ts @@ -0,0 +1,245 @@ +import { parseXml } from "../xml" + +describe("XML Parser Whitespace and Structure Handling", () => { + describe("whitespace handling in read_file args", () => { + it("should handle spaces around path values", () => { + const xml = ` + + + ./test/file.ts + +` + + const result = parseXml(xml) as any + expect(result.args.file.path).toBe("./test/file.ts") // trimValues should remove spaces + }) + + it("should handle newlines and tabs in path values", () => { + const xml = ` + + + + ./test/file.ts + + +` + + const result = parseXml(xml) as any + expect(result.args.file.path).toBe("./test/file.ts") + }) + + it("should handle multiple files with varying whitespace", () => { + const xml = ` + + + ./file1.ts + + + + ./file2.ts + + + + ./file3.ts + +` + + const result = parseXml(xml) as any + expect(Array.isArray(result.args.file)).toBe(true) + expect(result.args.file[0].path).toBe("./file1.ts") + expect(result.args.file[1].path).toBe("./file2.ts") + expect(result.args.file[2].path).toBe("./file3.ts") + }) + }) + + describe("problematic structures from issue #7664", () => { + it("should handle the exact structure from juliettefournier-econ's example", () => { + // This is the exact structure that was causing issues + const xml = ` + + +src/shared/infrastructure/supabase/factory.py +10-20 + +` + + const result = parseXml(xml) as any + expect(result.args.file.path).toBe("src/shared/infrastructure/supabase/factory.py") + expect(result.args.file.line_range).toBe("10-20") + }) + + it("should handle structure with no spaces between tags", () => { + const xml = `src/test.py1-10` + + const result = parseXml(xml) as any + expect(result.args.file.path).toBe("src/test.py") + expect(result.args.file.line_range).toBe("1-10") + }) + + it("should handle structure with mixed spacing", () => { + const xml = ` + src/test.py + 1-10 + +` + + const result = parseXml(xml) as any + expect(result.args.file.path).toBe("src/test.py") + expect(result.args.file.line_range).toBe("1-10") + }) + + it("should handle empty or whitespace-only path gracefully", () => { + const xml1 = ` + + + + +` + + const xml2 = ` + + + + +` + + const result1 = parseXml(xml1) as any + const result2 = parseXml(xml2) as any + + // Empty string after trimming + expect(result1.args.file.path).toBe("") + expect(result2.args.file.path).toBe("") + }) + + it("should handle missing path element", () => { + const xml = ` + + + 10-20 + +` + + const result = parseXml(xml) as any + expect(result.args.file.path).toBeUndefined() + expect(result.args.file.line_range).toBe("10-20") + }) + + it("should handle self-closing tags", () => { + const xml = ` + + + + 10-20 + +` + + const result = parseXml(xml) as any + expect(result.args.file.path).toBe("") + expect(result.args.file.line_range).toBe("10-20") + }) + }) + + describe("edge cases with malformed XML", () => { + it("should handle unclosed tags gracefully", () => { + const xml = ` + + + test.py` + + // The parser is lenient and may not throw on some malformed XML + const result = parseXml(xml) as any + // Just verify it doesn't crash + expect(result).toBeDefined() + }) + + it("should handle mismatched tags", () => { + const xml = ` + + + test.py + +` + + // The parser is lenient and may not throw on some malformed XML + const result = parseXml(xml) as any + // Just verify it doesn't crash + expect(result).toBeDefined() + }) + }) + + describe("complex nested structures", () => { + it("should handle deeply nested file structures", () => { + const xml = ` + + + ./deep/nested/path/file.ts + + test + 2024-01-01 + + 1-100 + +` + + const result = parseXml(xml) as any + expect(result.args.file.path).toBe("./deep/nested/path/file.ts") + expect(result.args.file.line_range).toBe("1-100") + expect(result.args.file.metadata.author).toBe("test") + expect(result.args.file.metadata.date).toBe("2024-01-01") + }) + + it("should handle multiple line_range elements", () => { + const xml = ` + + + test.py + 10-20 + 30-40 + +` + + const result = parseXml(xml) as any + expect(result.args.file.path).toBe("test.py") + expect(Array.isArray(result.args.file.line_range)).toBe(true) + expect(result.args.file.line_range).toEqual(["10-20", "30-40"]) + }) + }) + + describe("special characters in paths", () => { + it("should handle paths with spaces", () => { + const xml = ` + + + ./my folder/my file.ts + +` + + const result = parseXml(xml) as any + expect(result.args.file.path).toBe("./my folder/my file.ts") + }) + + it("should handle paths with special characters", () => { + const xml = ` + + + ./test@file#2024$.ts + +` + + const result = parseXml(xml) as any + expect(result.args.file.path).toBe("./test@file#2024$.ts") + }) + + it("should handle Windows-style paths", () => { + const xml = ` + + + C:\\Users\\test\\file.ts + +` + + const result = parseXml(xml) as any + expect(result.args.file.path).toBe("C:\\Users\\test\\file.ts") + }) + }) +})