Skip to content

Commit 11d7d67

Browse files
roomoteroomote
committed
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
1 parent 0ce4e89 commit 11d7d67

File tree

4 files changed

+577
-1
lines changed

4 files changed

+577
-1
lines changed

‎src/core/assistant-message/__tests__/parseAssistantMessageV2-issue7664.spec.ts‎

Lines changed: 183 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,183 @@
1+
import { parseAssistantMessageV2 } from "../parseAssistantMessageV2"
2+
import { ToolUse } from "../../../shared/tools"
3+
4+
describe("parseAssistantMessageV2 - Issue #7664", () => {
5+
describe("handling malformed XML structure from LLMs", () => {
6+
it("should handle the exact structure from juliettefournier-econ's example", () => {
7+
// This is the exact structure that was causing issues
8+
const message = `I'll read that file for you.
9+
10+
<read_file>
11+
<args>
12+
<file>
13+
<path>src/shared/infrastructure/supabase/factory.py</path>
14+
<line_range>10-20</line_range>
15+
</file>
16+
</args>
17+
</read_file>`
18+
19+
const result = parseAssistantMessageV2(message)
20+
21+
// Should have text and tool use
22+
expect(result).toHaveLength(2)
23+
expect(result[0].type).toBe("text")
24+
expect(result[1].type).toBe("tool_use")
25+
26+
const toolUse = result[1] as ToolUse
27+
expect(toolUse.name).toBe("read_file")
28+
expect(toolUse.params.args).toBeDefined()
29+
expect(toolUse.params.args).toContain("<file>")
30+
expect(toolUse.params.args).toContain("<path>src/shared/infrastructure/supabase/factory.py</path>")
31+
expect(toolUse.params.args).toContain("<line_range>10-20</line_range>")
32+
})
33+
34+
it("should handle structure with no spaces between XML tags", () => {
35+
const message = `<read_file><args><file><path>test.py</path><line_range>1-10</line_range></file></args></read_file>`
36+
37+
const result = parseAssistantMessageV2(message)
38+
39+
expect(result).toHaveLength(1)
40+
const toolUse = result[0] as ToolUse
41+
expect(toolUse.type).toBe("tool_use")
42+
expect(toolUse.name).toBe("read_file")
43+
expect(toolUse.params.args).toBeDefined()
44+
expect(toolUse.params.args).toContain("<path>test.py</path>")
45+
expect(toolUse.params.args).toContain("<line_range>1-10</line_range>")
46+
})
47+
48+
it("should handle structure with mixed spacing and newlines", () => {
49+
const message = `<read_file>
50+
<args>
51+
<file><path> src/test.py </path>
52+
<line_range>1-10</line_range>
53+
</file>
54+
</args>
55+
</read_file>`
56+
57+
const result = parseAssistantMessageV2(message)
58+
59+
expect(result).toHaveLength(1)
60+
const toolUse = result[0] as ToolUse
61+
expect(toolUse.type).toBe("tool_use")
62+
expect(toolUse.name).toBe("read_file")
63+
expect(toolUse.params.args).toBeDefined()
64+
// The args should preserve the internal structure
65+
expect(toolUse.params.args).toContain("<file>")
66+
expect(toolUse.params.args).toContain("<path>")
67+
expect(toolUse.params.args).toContain("src/test.py")
68+
expect(toolUse.params.args).toContain("</path>")
69+
expect(toolUse.params.args).toContain("<line_range>1-10</line_range>")
70+
})
71+
72+
it("should handle empty path element", () => {
73+
const message = `<read_file>
74+
<args>
75+
<file>
76+
<path></path>
77+
<line_range>10-20</line_range>
78+
</file>
79+
</args>
80+
</read_file>`
81+
82+
const result = parseAssistantMessageV2(message)
83+
84+
expect(result).toHaveLength(1)
85+
const toolUse = result[0] as ToolUse
86+
expect(toolUse.type).toBe("tool_use")
87+
expect(toolUse.name).toBe("read_file")
88+
expect(toolUse.params.args).toBeDefined()
89+
expect(toolUse.params.args).toContain("<path></path>")
90+
})
91+
92+
it("should handle self-closing path element", () => {
93+
const message = `<read_file>
94+
<args>
95+
<file>
96+
<path/>
97+
<line_range>10-20</line_range>
98+
</file>
99+
</args>
100+
</read_file>`
101+
102+
const result = parseAssistantMessageV2(message)
103+
104+
expect(result).toHaveLength(1)
105+
const toolUse = result[0] as ToolUse
106+
expect(toolUse.type).toBe("tool_use")
107+
expect(toolUse.name).toBe("read_file")
108+
expect(toolUse.params.args).toBeDefined()
109+
expect(toolUse.params.args).toContain("<path/>")
110+
})
111+
112+
it("should handle multiple files with varying structures", () => {
113+
const message = `<read_file>
114+
<args>
115+
<file>
116+
<path> ./file1.ts </path>
117+
</file>
118+
<file>
119+
<path>
120+
./file2.ts
121+
</path>
122+
<line_range>10-20</line_range>
123+
</file>
124+
</args>
125+
</read_file>`
126+
127+
const result = parseAssistantMessageV2(message)
128+
129+
expect(result).toHaveLength(1)
130+
const toolUse = result[0] as ToolUse
131+
expect(toolUse.type).toBe("tool_use")
132+
expect(toolUse.name).toBe("read_file")
133+
expect(toolUse.params.args).toBeDefined()
134+
// Check that both files are present
135+
expect(toolUse.params.args).toContain("./file1.ts")
136+
expect(toolUse.params.args).toContain("./file2.ts")
137+
expect(toolUse.params.args).toContain("<line_range>10-20</line_range>")
138+
})
139+
140+
it("should handle partial/incomplete tool use", () => {
141+
const message = `<read_file>
142+
<args>
143+
<file>
144+
<path>test.py</path>`
145+
// Message ends abruptly
146+
147+
const result = parseAssistantMessageV2(message)
148+
149+
expect(result).toHaveLength(1)
150+
const toolUse = result[0] as ToolUse
151+
expect(toolUse.type).toBe("tool_use")
152+
expect(toolUse.name).toBe("read_file")
153+
expect(toolUse.partial).toBe(true)
154+
expect(toolUse.params.args).toBeDefined()
155+
expect(toolUse.params.args).toContain("<path>test.py</path>")
156+
})
157+
})
158+
159+
describe("args parameter trimming behavior", () => {
160+
it("should trim args parameter content", () => {
161+
const message = `<read_file>
162+
<args>
163+
164+
<file>
165+
<path>test.py</path>
166+
</file>
167+
168+
</args>
169+
</read_file>`
170+
171+
const result = parseAssistantMessageV2(message)
172+
173+
expect(result).toHaveLength(1)
174+
const toolUse = result[0] as ToolUse
175+
expect(toolUse.params.args).toBeDefined()
176+
// args should be trimmed
177+
expect(toolUse.params.args).not.toMatch(/^\s+/)
178+
expect(toolUse.params.args).not.toMatch(/\s+$/)
179+
expect(toolUse.params.args).toMatch(/^<file>/)
180+
expect(toolUse.params.args).toMatch(/<\/file>$/)
181+
})
182+
})
183+
})

‎src/core/tools/__tests__/readFileTool-empty-path.spec.ts‎

Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,130 @@
1+
import { vi, describe, it, expect, beforeEach } from "vitest"
2+
import { readFileTool } from "../readFileTool"
3+
import { Task } from "../../task/Task"
4+
import { ReadFileToolUse } from "../../../shared/tools"
5+
6+
describe("readFileTool - empty path handling", () => {
7+
let mockCline: any
8+
let mockAskApproval: any
9+
let mockHandleError: any
10+
let mockPushToolResult: any
11+
let mockRemoveClosingTag: any
12+
13+
beforeEach(() => {
14+
mockCline = {
15+
cwd: "/test",
16+
consecutiveMistakeCount: 0,
17+
recordToolError: vi.fn(),
18+
sayAndCreateMissingParamError: vi.fn().mockResolvedValue("Missing parameter error"),
19+
say: vi.fn(),
20+
rooIgnoreController: undefined,
21+
api: {
22+
getModel: () => ({ info: { supportsImages: false } }),
23+
},
24+
}
25+
26+
mockAskApproval = vi.fn().mockResolvedValue(true)
27+
mockHandleError = vi.fn()
28+
mockPushToolResult = vi.fn()
29+
mockRemoveClosingTag = vi.fn()
30+
})
31+
32+
it("should provide clear error message for empty path elements", async () => {
33+
const toolUse: ReadFileToolUse = {
34+
type: "tool_use",
35+
name: "read_file",
36+
params: {
37+
args: `<file><path></path></file>`,
38+
},
39+
partial: false,
40+
}
41+
42+
await readFileTool(
43+
mockCline as unknown as Task,
44+
toolUse,
45+
mockAskApproval,
46+
mockHandleError,
47+
mockPushToolResult,
48+
mockRemoveClosingTag,
49+
)
50+
51+
// Should handle the error with a clear message
52+
expect(mockHandleError).toHaveBeenCalledWith(
53+
"parsing read_file args",
54+
expect.objectContaining({
55+
message: "All file paths are empty or missing. Please provide valid file paths in the <path> elements.",
56+
}),
57+
)
58+
59+
// Should push the error result
60+
expect(mockPushToolResult).toHaveBeenCalledWith(
61+
`<files><error>All file paths are empty or missing. Please provide valid file paths in the <path> elements.</error></files>`,
62+
)
63+
})
64+
65+
it("should provide clear error message for whitespace-only path elements", async () => {
66+
const toolUse: ReadFileToolUse = {
67+
type: "tool_use",
68+
name: "read_file",
69+
params: {
70+
args: `<file><path> </path></file>`,
71+
},
72+
partial: false,
73+
}
74+
75+
await readFileTool(
76+
mockCline as unknown as Task,
77+
toolUse,
78+
mockAskApproval,
79+
mockHandleError,
80+
mockPushToolResult,
81+
mockRemoveClosingTag,
82+
)
83+
84+
// Should handle the error with a clear message
85+
expect(mockHandleError).toHaveBeenCalledWith(
86+
"parsing read_file args",
87+
expect.objectContaining({
88+
message: "All file paths are empty or missing. Please provide valid file paths in the <path> elements.",
89+
}),
90+
)
91+
92+
// Should push the error result
93+
expect(mockPushToolResult).toHaveBeenCalledWith(
94+
`<files><error>All file paths are empty or missing. Please provide valid file paths in the <path> elements.</error></files>`,
95+
)
96+
})
97+
98+
// Note: Testing the case where some paths are empty but others are valid
99+
// would require extensive mocking of file system operations.
100+
// The core functionality is tested by the other tests which verify
101+
// that empty paths are properly detected and reported.
102+
103+
it("should handle multiple empty paths", async () => {
104+
const toolUse: ReadFileToolUse = {
105+
type: "tool_use",
106+
name: "read_file",
107+
params: {
108+
args: `<file><path></path></file><file><path> </path></file><file><path/></file>`,
109+
},
110+
partial: false,
111+
}
112+
113+
await readFileTool(
114+
mockCline as unknown as Task,
115+
toolUse,
116+
mockAskApproval,
117+
mockHandleError,
118+
mockPushToolResult,
119+
mockRemoveClosingTag,
120+
)
121+
122+
// Should handle the error with a clear message
123+
expect(mockHandleError).toHaveBeenCalledWith(
124+
"parsing read_file args",
125+
expect.objectContaining({
126+
message: "All file paths are empty or missing. Please provide valid file paths in the <path> elements.",
127+
}),
128+
)
129+
})
130+
})

‎src/core/tools/readFileTool.ts‎

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -131,8 +131,18 @@ export async function readFileTool(
131131
const parsed = parseXml(argsXmlTag) as any
132132
const files = Array.isArray(parsed.file) ? parsed.file : [parsed.file].filter(Boolean)
133133

134+
// Track empty paths for better error reporting
135+
let emptyPathCount = 0
136+
let totalFileCount = 0
137+
134138
for (const file of files) {
135-
if (!file.path) continue // Skip if no path in a file entry
139+
totalFileCount++
140+
141+
// Check for empty or whitespace-only paths
142+
if (!file.path || (typeof file.path === "string" && file.path.trim() === "")) {
143+
emptyPathCount++
144+
continue // Skip if no path or empty path in a file entry
145+
}
136146

137147
const fileEntry: FileEntry = {
138148
path: file.path,
@@ -153,6 +163,14 @@ export async function readFileTool(
153163
}
154164
fileEntries.push(fileEntry)
155165
}
166+
167+
// If all paths were empty, provide a more specific error
168+
if (emptyPathCount > 0 && fileEntries.length === 0) {
169+
const errorMessage = `All file paths are empty or missing. Please provide valid file paths in the <path> elements.`
170+
await handleError("parsing read_file args", new Error(errorMessage))
171+
pushToolResult(`<files><error>${errorMessage}</error></files>`)
172+
return
173+
}
156174
} catch (error) {
157175
const errorMessage = `Failed to parse read_file XML args: ${error instanceof Error ? error.message : String(error)}`
158176
await handleError("parsing read_file args", new Error(errorMessage))

0 commit comments

Comments
 (0)