-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: Add XML auto-repair for broken tool calls from OpenAI-compatible providers #7158
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
… providers - Add XML repair utility function to fix missing opening brackets in XML tags - Add configuration option openAiXmlAutoRepair in OpenAI Compatible settings - Integrate XML repair into OpenAI and BaseOpenAiCompatibleProvider streaming - Add comprehensive tests for XML repair functionality - Add UI checkbox for enabling XML auto-repair in settings Fixes #7156
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.
Reviewing my own code is like debugging in production - technically possible but morally questionable.
| <div> | ||
| <Checkbox | ||
| checked={apiConfiguration?.openAiXmlAutoRepair ?? false} | ||
| onChange={handleInputChange("openAiXmlAutoRepair", noTransform)}> |
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.
Missing i18n translations! The keys settings:providers.xmlAutoRepair and settings:providers.xmlAutoRepairDescription are not defined in any locale files. This will display raw translation keys in the UI.
You'll need to add these translations to all locale files in webview-ui/src/i18n/locales/*/settings.json
|
|
||
| if (delta?.content) { | ||
| // Apply XML repair if enabled for OpenAI-compatible providers | ||
| const content = this.options.openAiXmlAutoRepair ? repairBrokenXml(delta.content) : delta.content |
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: This calls repairBrokenXml() on every streaming chunk. For large responses with many chunks, this could impact performance. Consider:
- Batching chunks before processing
- Only checking for broken patterns first with
hasBrokenXmlPattern()before attempting repair - Implementing a streaming-aware repair that maintains state between chunks
| /** | ||
| * Checks if the XML has a valid structure with proper opening and closing tags | ||
| */ | ||
| function hasValidXmlStructure(xml: string): boolean { |
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.
This function hasValidXmlStructure() is defined but never used anywhere in the codebase. Should we remove it or integrate it into the repair logic to skip processing when XML is already valid?
| // Track open tags to determine if we need opening or closing tags | ||
| const openTags: string[] = [] | ||
|
|
||
| for (let line of lines) { |
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 concern: What happens with multiple broken patterns on the same line like read_file>args>? The current logic processes line by line but might not handle all combinations correctly. Consider adding test cases for these scenarios.
| /** | ||
| * Configuration for XML auto-repair behavior | ||
| */ | ||
| export interface XmlAutoRepairConfig { |
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 XmlAutoRepairConfig interface includes fields for future enhancements (useSmallModel, repairModelId) that aren't implemented yet. Consider either:
- Removing these until needed
- Adding a TODO comment explaining the future plan
- Documenting that these are reserved for future use
| import { describe, it, expect } from "vitest" | ||
| import { repairBrokenXml, hasBrokenXmlPattern } from "../xml-repair" | ||
|
|
||
| describe("xml-repair", () => { |
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.
Good unit test coverage! However, consider adding integration tests that verify the repair works correctly in the actual streaming context where it's used. This would help catch issues with chunk boundaries and partial XML fragments.
Fixes #7156
Summary
This PR adds an XML auto-repair feature to fix broken XML tool calls from certain LLM providers (like Targon) that don't properly format XML responses, specifically missing opening brackets in their XML tags.
Changes
src/api/utils/xml-repair.ts) to detect and fix broken XML patternsopenAiXmlAutoRepairin OpenAI Compatible provider settingsHow it works
The XML repair function detects patterns like:
And repairs them to:
Testing
Usage
Users can enable this feature by:
This is particularly useful for providers like Targon that have issues with XML formatting in their responses.
Important
Adds XML auto-repair feature for OpenAI-compatible providers, fixing broken XML tags and integrating a configuration option and UI checkbox.
createMessage()inbase-openai-compatible-provider.tsandopenai.ts.openAiXmlAutoRepairinprovider-settings.ts.repairBrokenXml()function inxml-repair.tsto fix broken XML patterns.hasBrokenXmlPattern()to detect broken XML.OpenAICompatible.tsxto enable/disable XML auto-repair.xml-repair.spec.ts.This description was created by
for 3bf47ad. You can customize this summary. It will automatically update as commits are pushed.