-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: improve XML parameter extraction for Qwen3-Coder and other models #7407
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
…wen3-Coder and other models - Add support for XML tags with spaces in various positions - Handle malformed opening tags like '< tool>' and '<tool >' - Handle malformed closing tags like '</ param>' and '< /param>' - Check for multiple malformed tag patterns during parsing This fixes the issue where certain models (particularly Qwen3-Coder) generate slightly malformed XML tags that were not being parsed correctly, resulting in 'missing parameter' errors. Fixes #7406
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 wrote code that handles broken XML. The irony is not lost on my circuits.
| } else { | ||
| // Check for malformed closing tags (e.g., missing slash or spaces) | ||
| // Some models might generate </path > or < /path> instead of </path> | ||
| const malformedClosingPatterns = [ |
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.
Critical: This PR adds new functionality but doesn't include any tests for these malformed tag patterns. Could we add test cases like:
| const malformedClosingPatterns = [ | |
| // In AssistantMessageParser.spec.ts | |
| it("should handle malformed opening tags with spaces", () => { | |
| const message = "< read_file>< path>test.ts</ path></ read_file>" | |
| const result = streamChunks(parser, message) | |
| // assertions... | |
| }) |
| // Check for malformed closing tags (e.g., missing slash or spaces) | ||
| // Some models might generate </path > or < /path> instead of </path> | ||
| const malformedClosingPatterns = [ | ||
| `</ ${this.currentParamName}>`, // space after slash |
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 malformed tag patterns are duplicated in three places (here, lines 136-140, and 197-201). Consider extracting into a helper function:
| `</ ${this.currentParamName}>`, // space after slash | |
| function getMalformedTagVariants(tagName: string): string[] { | |
| return [ | |
| `< ${tagName}>`, // space after opening bracket | |
| `<${tagName} >`, // space before closing bracket | |
| `< ${tagName} >`, // spaces on both sides | |
| ] | |
| } |
| `< / ${this.currentParamName} >`, // spaces everywhere | ||
| ] | ||
|
|
||
| for (const pattern of malformedClosingPatterns) { |
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.
Performance consideration: We're checking 4 patterns sequentially for each tag. Would a single regex be more efficient? Something like:
| for (const pattern of malformedClosingPatterns) { | |
| const malformedPattern = new RegExp(`<\s*/?\s*${this.currentParamName}\s*>`) | |
| if (currentParamValue.match(malformedPattern)) { | |
| // Handle malformed closing tag | |
| } |
| continue | ||
| } else { | ||
| // Check for malformed closing tags (e.g., missing slash or spaces) | ||
| // Some models might generate </path > or < /path> instead of </path> |
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.
Could we add a comment explaining why we're checking these patterns? Future maintainers would benefit from understanding this handles models like Qwen3-Coder that generate malformed XML.
| `</ ${this.currentParamName}>`, // space after slash | ||
| `< /${this.currentParamName}>`, // space before slash | ||
| `</${this.currentParamName} >`, // space before closing bracket | ||
| `< / ${this.currentParamName} >`, // spaces everywhere |
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.
Edge case consideration: What about tags with multiple spaces like < / path >? The current patterns might miss some variations. Is this intentional to only handle the most common cases?
Summary
This PR fixes issue #7406 where certain LLM models (particularly Qwen3-Coder) generate slightly malformed XML tags with extra spaces, causing 'missing parameter' errors.
Problem
Users were experiencing errors like:
Roo tried to use apply_diff without value for required parameter 'path'. Retrying...Roo tried to use search_and_replace without value for required parameter 'path'. Retrying...Roo tried to use write_to_file without value for required parameter 'path'. Retrying...Root Cause
Some LLM models generate XML tags with extra spaces in various positions:
< tool>instead of<tool><tool >instead of<tool></ param>instead of</param>< /param>instead of</param>The existing parser was not handling these malformed tags, causing it to miss required parameters.
Solution
Enhanced the
AssistantMessageParserto detect and handle malformed XML tags by:Changes
src/core/assistant-message/AssistantMessageParser.tsto handle malformed XML tagsTesting
Impact
This fix will resolve the parameter extraction issues for users of Qwen3-Coder and other models that generate slightly malformed XML, improving compatibility across different LLM providers.
Fixes #7406
Important
Enhanced
AssistantMessageParserto handle malformed XML tags with extra spaces, resolving parameter extraction issues for Qwen3-Coder and similar models.AssistantMessageParserinAssistantMessageParser.tsto handle malformed XML tags with extra spaces.< tool>,<tool >,</ param>, and< /param>.This description was created by
for ede1a1c. You can customize this summary. It will automatically update as commits are pushed.