mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-08-28 05:27:24 +00:00
fix: improve error messages for empty path elements in read_file tool
- Added better error handling for empty or whitespace-only path elements - Shows clear error message 'All file paths are empty or missing' instead of generic 'Missing value for required parameter' - Added comprehensive tests for XML parsing with whitespace and empty paths - Fixes issue #7664 where users were confused by misleading error messages
This commit is contained in:
Roo Code
parent
0ce4e891fd
commit
11d7d67dd4
4 changed files with 577 additions and 1 deletions
|
|
@ -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.
|
||||
|
||||
<read_file>
|
||||
<args>
|
||||
<file>
|
||||
<path>src/shared/infrastructure/supabase/factory.py</path>
|
||||
<line_range>10-20</line_range>
|
||||
</file>
|
||||
</args>
|
||||
</read_file>`
|
||||
|
||||
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("<file>")
|
||||
expect(toolUse.params.args).toContain("<path>src/shared/infrastructure/supabase/factory.py</path>")
|
||||
expect(toolUse.params.args).toContain("<line_range>10-20</line_range>")
|
||||
})
|
||||
|
||||
it("should handle structure with no spaces between XML tags", () => {
|
||||
const message = `<read_file><args><file><path>test.py</path><line_range>1-10</line_range></file></args></read_file>`
|
||||
|
||||
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("<path>test.py</path>")
|
||||
expect(toolUse.params.args).toContain("<line_range>1-10</line_range>")
|
||||
})
|
||||
|
||||
it("should handle structure with mixed spacing and newlines", () => {
|
||||
const message = `<read_file>
|
||||
<args>
|
||||
<file><path> src/test.py </path>
|
||||
<line_range>1-10</line_range>
|
||||
</file>
|
||||
</args>
|
||||
</read_file>`
|
||||
|
||||
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("<file>")
|
||||
expect(toolUse.params.args).toContain("<path>")
|
||||
expect(toolUse.params.args).toContain("src/test.py")
|
||||
expect(toolUse.params.args).toContain("</path>")
|
||||
expect(toolUse.params.args).toContain("<line_range>1-10</line_range>")
|
||||
})
|
||||
|
||||
it("should handle empty path element", () => {
|
||||
const message = `<read_file>
|
||||
<args>
|
||||
<file>
|
||||
<path></path>
|
||||
<line_range>10-20</line_range>
|
||||
</file>
|
||||
</args>
|
||||
</read_file>`
|
||||
|
||||
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("<path></path>")
|
||||
})
|
||||
|
||||
it("should handle self-closing path element", () => {
|
||||
const message = `<read_file>
|
||||
<args>
|
||||
<file>
|
||||
<path/>
|
||||
<line_range>10-20</line_range>
|
||||
</file>
|
||||
</args>
|
||||
</read_file>`
|
||||
|
||||
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("<path/>")
|
||||
})
|
||||
|
||||
it("should handle multiple files with varying structures", () => {
|
||||
const message = `<read_file>
|
||||
<args>
|
||||
<file>
|
||||
<path> ./file1.ts </path>
|
||||
</file>
|
||||
<file>
|
||||
<path>
|
||||
./file2.ts
|
||||
</path>
|
||||
<line_range>10-20</line_range>
|
||||
</file>
|
||||
</args>
|
||||
</read_file>`
|
||||
|
||||
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("<line_range>10-20</line_range>")
|
||||
})
|
||||
|
||||
it("should handle partial/incomplete tool use", () => {
|
||||
const message = `<read_file>
|
||||
<args>
|
||||
<file>
|
||||
<path>test.py</path>`
|
||||
// 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("<path>test.py</path>")
|
||||
})
|
||||
})
|
||||
|
||||
describe("args parameter trimming behavior", () => {
|
||||
it("should trim args parameter content", () => {
|
||||
const message = `<read_file>
|
||||
<args>
|
||||
|
||||
<file>
|
||||
<path>test.py</path>
|
||||
</file>
|
||||
|
||||
</args>
|
||||
</read_file>`
|
||||
|
||||
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(/^<file>/)
|
||||
expect(toolUse.params.args).toMatch(/<\/file>$/)
|
||||
})
|
||||
})
|
||||
})
|
||||
130
src/core/tools/__tests__/readFileTool-empty-path.spec.ts
Normal file
130
src/core/tools/__tests__/readFileTool-empty-path.spec.ts
Normal file
|
|
@ -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: `<file><path></path></file>`,
|
||||
},
|
||||
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 <path> elements.",
|
||||
}),
|
||||
)
|
||||
|
||||
// Should push the error result
|
||||
expect(mockPushToolResult).toHaveBeenCalledWith(
|
||||
`<files><error>All file paths are empty or missing. Please provide valid file paths in the <path> elements.</error></files>`,
|
||||
)
|
||||
})
|
||||
|
||||
it("should provide clear error message for whitespace-only path elements", async () => {
|
||||
const toolUse: ReadFileToolUse = {
|
||||
type: "tool_use",
|
||||
name: "read_file",
|
||||
params: {
|
||||
args: `<file><path> </path></file>`,
|
||||
},
|
||||
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 <path> elements.",
|
||||
}),
|
||||
)
|
||||
|
||||
// Should push the error result
|
||||
expect(mockPushToolResult).toHaveBeenCalledWith(
|
||||
`<files><error>All file paths are empty or missing. Please provide valid file paths in the <path> elements.</error></files>`,
|
||||
)
|
||||
})
|
||||
|
||||
// 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: `<file><path></path></file><file><path> </path></file><file><path/></file>`,
|
||||
},
|
||||
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 <path> elements.",
|
||||
}),
|
||||
)
|
||||
})
|
||||
})
|
||||
|
|
@ -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 <path> elements.`
|
||||
await handleError("parsing read_file args", new Error(errorMessage))
|
||||
pushToolResult(`<files><error>${errorMessage}</error></files>`)
|
||||
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))
|
||||
|
|
|
|||
245
src/utils/__tests__/xml-whitespace.spec.ts
Normal file
245
src/utils/__tests__/xml-whitespace.spec.ts
Normal file
|
|
@ -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 = `
|
||||
<args>
|
||||
<file>
|
||||
<path> ./test/file.ts </path>
|
||||
</file>
|
||||
</args>`
|
||||
|
||||
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 = `
|
||||
<args>
|
||||
<file>
|
||||
<path>
|
||||
./test/file.ts
|
||||
</path>
|
||||
</file>
|
||||
</args>`
|
||||
|
||||
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 = `
|
||||
<args>
|
||||
<file>
|
||||
<path> ./file1.ts </path>
|
||||
</file>
|
||||
<file>
|
||||
<path>
|
||||
./file2.ts
|
||||
</path>
|
||||
</file>
|
||||
<file>
|
||||
<path> ./file3.ts </path>
|
||||
</file>
|
||||
</args>`
|
||||
|
||||
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 = `
|
||||
<args>
|
||||
<file>
|
||||
<path>src/shared/infrastructure/supabase/factory.py</path>
|
||||
<line_range>10-20</line_range>
|
||||
</file>
|
||||
</args>`
|
||||
|
||||
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 = `<args><file><path>src/test.py</path><line_range>1-10</line_range></file></args>`
|
||||
|
||||
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 = `<args>
|
||||
<file><path> src/test.py </path>
|
||||
<line_range>1-10</line_range>
|
||||
</file>
|
||||
</args>`
|
||||
|
||||
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 = `
|
||||
<args>
|
||||
<file>
|
||||
<path></path>
|
||||
</file>
|
||||
</args>`
|
||||
|
||||
const xml2 = `
|
||||
<args>
|
||||
<file>
|
||||
<path> </path>
|
||||
</file>
|
||||
</args>`
|
||||
|
||||
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 = `
|
||||
<args>
|
||||
<file>
|
||||
<line_range>10-20</line_range>
|
||||
</file>
|
||||
</args>`
|
||||
|
||||
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 = `
|
||||
<args>
|
||||
<file>
|
||||
<path/>
|
||||
<line_range>10-20</line_range>
|
||||
</file>
|
||||
</args>`
|
||||
|
||||
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 = `
|
||||
<args>
|
||||
<file>
|
||||
<path>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 = `
|
||||
<args>
|
||||
<file>
|
||||
<path>test.py</file>
|
||||
</path>
|
||||
</args>`
|
||||
|
||||
// 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 = `
|
||||
<args>
|
||||
<file>
|
||||
<path>./deep/nested/path/file.ts</path>
|
||||
<metadata>
|
||||
<author>test</author>
|
||||
<date>2024-01-01</date>
|
||||
</metadata>
|
||||
<line_range>1-100</line_range>
|
||||
</file>
|
||||
</args>`
|
||||
|
||||
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 = `
|
||||
<args>
|
||||
<file>
|
||||
<path>test.py</path>
|
||||
<line_range>10-20</line_range>
|
||||
<line_range>30-40</line_range>
|
||||
</file>
|
||||
</args>`
|
||||
|
||||
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 = `
|
||||
<args>
|
||||
<file>
|
||||
<path>./my folder/my file.ts</path>
|
||||
</file>
|
||||
</args>`
|
||||
|
||||
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 = `
|
||||
<args>
|
||||
<file>
|
||||
<path>./test@file#2024$.ts</path>
|
||||
</file>
|
||||
</args>`
|
||||
|
||||
const result = parseXml(xml) as any
|
||||
expect(result.args.file.path).toBe("./test@file#2024$.ts")
|
||||
})
|
||||
|
||||
it("should handle Windows-style paths", () => {
|
||||
const xml = `
|
||||
<args>
|
||||
<file>
|
||||
<path>C:\\Users\\test\\file.ts</path>
|
||||
</file>
|
||||
</args>`
|
||||
|
||||
const result = parseXml(xml) as any
|
||||
expect(result.args.file.path).toBe("C:\\Users\\test\\file.ts")
|
||||
})
|
||||
})
|
||||
})
|
||||
Loading…
Add table
Reference in a new issue