-
Notifications
You must be signed in to change notification settings - Fork 2.5k
fix: improve error messages for empty path elements in read_file tool #7801
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- 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
|
|
||
| // 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.` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new logic correctly detects empty/whitespace-only path elements and provides a clear error. Consider using the i18n translation function (e.g., t(...)) for the error message (currently hardcoded) so that user‐facing text remains localizable.
| const errorMessage = `All file paths are empty or missing. Please provide valid file paths in the <path> elements.` | |
| const errorMessage = t("tools:readFile.allPathsEmpty") |
This comment was generated because it violated a code review rule: irule_C0ez7Rji6ANcGkkX.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reviewed my own code and found it surprisingly coherent. Must be a bug in my review algorithm.
| } | ||
|
|
||
| // If all paths were empty, provide a more specific error | ||
| if (emptyPathCount > 0 && fileEntries.length === 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this intentional? The code tracks both emptyPathCount and totalFileCount, but the condition only checks emptyPathCount > 0. Could we simplify this to just check if fileEntries.length === 0 after processing all files? The current logic works but seems more complex than necessary.
| totalFileCount++ | ||
|
|
||
| // Check for empty or whitespace-only paths | ||
| if (!file.path || (typeof file.path === "string" && file.path.trim() === "")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider extracting this empty path checking logic into a small helper function like isEmptyPath(path) for better readability and reusability:
| if (!file.path || (typeof file.path === "string" && file.path.trim() === "")) { | |
| const isEmptyPath = (path: any): boolean => !path || (typeof path === "string" && path.trim() === ""); | |
| // Check for empty or whitespace-only paths | |
| if (isEmptyPath(file.path)) { |
Summary
This PR improves error handling and messaging when the
read_filetool encounters empty or whitespace-only path elements in XML arguments.Problem
Users were receiving a confusing error message "Missing value for required parameter 'path'" when:
<path></path>)<path> </path>)<path/>)This was particularly confusing because the XML structure appeared correct, but the path value was being treated as missing after trimming.
Solution
Related Issue
Fixes #7664
Testing
src/core/tools/__tests__/readFileTool-empty-path.spec.tswith tests for various empty path scenariossrc/utils/__tests__/xml-whitespace.spec.tssrc/core/assistant-message/__tests__/parseAssistantMessageV2-issue7664.spec.tsNote
While investigating this issue, I discovered that the specific problem reported by @juliettefournier-econ was actually due to the AI assistant (Grok Code Fast) using an invalid
line_rangeparameter that doesn't exist for theread_filetool. However, this PR still provides value by improving error messages for empty path scenarios, which will help users debug similar issues more easily.Important
Improves error handling in
readFileToolfor empty paths, providing specific error messages and adding comprehensive tests.readFileToolfor empty or whitespace-only<path>elements in XML arguments.readFileTool-empty-path.spec.tsto test error handling for empty paths.xml-whitespace.spec.tsto test XML parsing with various whitespace scenarios.parseAssistantMessageV2-issue7664.spec.tsto test handling of malformed XML structures.read_filetool.This description was created by
for 11d7d67. You can customize this summary. It will automatically update as commits are pushed.