-
Notifications
You must be signed in to change notification settings - Fork 2.6k
refactor: simplify apply_diff XML schema #6812
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
- Remove backticks from XML content blocks in tool descriptions - Eliminate redundant args wrapper from XML structure - Remove duplicate start_line specification (keep only in-content markers) - Update parsing logic to handle simplified schema - Maintain backward compatibility with legacy format BREAKING CHANGE: The apply_diff tool now uses a simplified XML schema without backticks inside content blocks and without the args wrapper. The diff content is now placed directly in the diff element.
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.
Thank you for your contribution! I've reviewed the changes and have some suggestions for improvement. Overall, this is a good refactoring that simplifies the XML schema as intended. The backward compatibility implementation looks solid, and the cleaner schema will make the tool easier to use.
| diffContent = String(diff) | ||
| // Extract start_line from the diff content if present | ||
| const startLineMatch = diffContent.match(/:start_line:\s*(\d+)/) | ||
| startLine = startLineMatch ? parseInt(startLineMatch[1]) : undefined |
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 extracts :start_line: from the diff content here, but the documentation in multi-file-search-replace.ts no longer mentions this format. Should we update the documentation to clarify that :start_line: can still be used within the diff content for cases where it's needed?
| </diff> | ||
| </file> | ||
| </args> | ||
| Expected structure (simplified): |
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 error message shows the simplified structure, which is great! However, would it be helpful to also mention that the legacy format is still supported for backward compatibility? This could help users who are migrating from the old format.
| startLine = diff.start_line ? parseInt(diff.start_line) : undefined | ||
| } else { | ||
| // New simplified format - diff is the content string directly | ||
| diffContent = String(diff) |
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 adding a type guard or validation here to ensure diff is actually a string in the simplified format case. While String(diff) works, explicit type checking would make the code more robust:
| diffContent = String(diff) | |
| // New simplified format - diff is the content string directly | |
| if (typeof diff !== 'string') { | |
| throw new Error('Invalid diff format: expected string for simplified format'); | |
| } | |
| diffContent = diff; |
| // For legacy format, diff has content and start_line properties | ||
| if (typeof diff === "object" && diff.content) { | ||
| // Legacy format with content wrapper | ||
| diffContent = diff.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.
Since this maintains backward compatibility but changes the recommended format, would it be helpful to add a deprecation notice when the legacy format is detected? This could guide users to migrate to the cleaner schema over time. For example:
| diffContent = diff.content | |
| // Legacy format with content wrapper | |
| console.warn('[Deprecation] The <content> wrapper in apply_diff is deprecated. Please use the simplified format with diff content directly.'); | |
| diffContent = diff.content |
|
This broke the tool |
This is the best solution! 👍After investigating the issue thoroughly, this PR provides the cleanest fix. By removing the backticks entirely from the XML examples, we eliminate the root cause of the parsing errors without needing:
The simplified schema is much cleaner and more maintainable. This should be the preferred solution for fixing issue #4852. Summary of approaches:
I recommend merging this PR and closing the others. |
|
Not sure if it's related, but I found that RooCode fails on this prompt: It just writes an empty file. Would this fix it? |
Summary
This PR simplifies the XML schema for the
apply_difftool by removing unnecessary complexity and improving clarity.Changes
1. Removed Backticks from XML Content
2. Eliminated Redundant
argsWrapper<args><file>...</file></args><file>...</file>(files are direct children)3. Removed Duplicate
start_lineSpecificationstart_linecould be specified both as XML element and within content:start_line:) when needed4. Simplified Diff Structure
<diff><content>...</content></diff><diff>...</diff>(content directly in diff element)Example of New Schema
Benefits
Testing
Breaking Changes
While backward compatibility is maintained, this changes the recommended XML schema format for the
apply_difftool. Documentation and examples have been updated accordingly.