Skip to content

Fix: handle malformed XML in follow-up suggestions and improve error … #3444

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Fix: handle malformed XML in follow-up suggestions and improve error … #3444

wants to merge 1 commit into from

Conversation

@d-oit
Copy link
Contributor

@d-oit d-oit commented May 10, 2025

Related GitHub Issue

Closes: 3404

Test Procedure

see issue

Type of Change

  • 🐛 Bug Fix: Non-breaking change that fixes an issue.
  • ✨ New Feature: Non-breaking change that adds functionality.
  • 💥 Breaking Change: Fix or feature that would cause existing functionality to not work as expected.
  • ♻️ Refactor: Code change that neither fixes a bug nor adds a feature.
  • 💅 Style: Changes that do not affect the meaning of the code (white-space, formatting, etc.).
  • 📚 Documentation: Updates to documentation files.
  • ⚙️ Build/CI: Changes to the build process or CI configuration.
  • 🧹 Chore: Other changes that don't modify src or test files.

Pre-Submission Checklist

  • Issue Linked: This PR is linked to an approved GitHub Issue (see "Related GitHub Issue" above).
  • Scope: My changes are focused on the linked issue (one major feature/fix per PR).
  • Self-Review: I have performed a thorough self-review of my code.
  • Code Quality:
    • My code adheres to the project's style guidelines.
    • There are no new linting errors or warnings (npm run lint).
    • All debug code (e.g., console.log) has been removed.
  • Testing:
    • New and/or updated tests have been added to cover my changes.
    • All tests pass locally (npm test).
    • The application builds successfully with my changes.
  • Branch Hygiene: My branch is up-to-date (rebased) with the main branch.
  • Documentation Impact: I have considered if my changes require documentation updates (see "Documentation Updates" section below).
  • Changeset: A changeset has been created using npm run changeset if this PR includes user-facing changes or dependency updates.
  • Contribution Guidelines: I have read and agree to the Contributor Guidelines.

Screenshots / Videos

Documentation Updates

Does this PR necessitate updates to user-facing documentation?

  • No documentation updates are required.
  • Yes, documentation updates are required. (Please describe what needs to be updated or link to a PR in the docs repository).

Changes Made:

Changes Made

Modifications to src/core/tools/askFollowupQuestionTool.ts

  • Improved XML Parsing:

    • The follow_up parameter, containing suggestion XML, is now consistently wrapped in a <suggestions> root element before parsing.
    • This change (lines 42–44) ensures parsing succeeds even if the AI provides an XML fragment (e.g., only <suggest> tags without a root), preventing "Invalid operations xml format" errors.

  • Updated Error Messages (Lines 49–50):

    • Internal logging message via cline.say changed from:
      • "Failed to parse operations: ..." → "Failed to parse follow-up suggestions: ..."
    • The error message passed to formatResponse.toolError changed from:
      • "Invalid operations xml format" → "Invalid suggestions format. Please provide well-formed XML with tags."
    • These changes provide clearer feedback for correcting malformed suggestion XML.

New Test File: src/core/tools/__tests__/askFollowupQuestionTool.test.ts

  • Purpose: Introduced a comprehensive Jest test suite for the askFollowupQuestionTool.

  • Test Coverage Includes:

    • Basic Functionality:
      • Valid questions and suggestions.
      • Handling partial/streaming tool usage.
    • Error Handling:
      • Missing question parameters.
      • General cline.ask errors.
      • Various XML parsing failures (including malformed XML examples related to missing root tags).
    • Suggestion Handling:
      • Multiple <suggest> elements.
      • Single suggestion scenarios.
      • No suggestions present.

Impact

  • Increased Robustness:

    • More resilient XML parsing—handles absent root elements gracefully.

  • Improved Error Reporting:

    • Error messages are clearer and more actionable, improving debugging and AI feedback quality.

  • Enhanced Maintainability & Reliability:

    • The test suite adds regression safety and ensures expected behavior across a range of scenarios.

  • Direct Issue Resolution:

    • Specifically addresses the "Invalid operations xml format" issue by improving parsing and adding targeted test coverage.

Additional Notes

@changeset-bot
Copy link

changeset-bot bot commented May 10, 2025

⚠️ No Changeset found

Latest commit: e7094bf

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

samhvw8
samhvw8 reviewed May 10, 2025
View reviewed changes
src/core/tools/askFollowupQuestionTool.ts
// Ensure we have well-formed XML by wrapping in a root element
// This handles XML fragments that lack a single root
const wrappedXml = `<suggestions>${follow_up}</suggestions>`
parsedSuggest = parseXml(wrappedXml, ["suggest"]) as { suggest: Suggest[] | Suggest }
Copy link
Contributor

@samhvw8 samhvw8 May 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@d-oit it's weird, it should escape when we using ["suggest"] in stop node o.o

Copy link
Contributor Author

@d-oit d-oit May 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not finished, draft PR.
In the end I should not throw an error -> see issue, user prompt.

Copy link
Contributor Author

@d-oit d-oit May 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was only a quick tryout. If you have a better fix go ahead.
Not on the computer anymore this weekend.

And I wanted to check the time effort of the new PR rules.

@d-oit d-oit closed this May 10, 2025
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap May 10, 2025
bgilbert6 pushed a commit to bgilbert6/Roo-Code that referenced this pull request May 14, 2025
@arafatkatze
Migrate Browsertools settings to the webview from Vscode settings (Ro…
976a8fa 
…oCodeInc#3444)

* Removing redundant settings

* Add chromeExecutablePath to BrowserSettings and UpdateBrowserSettingsRequest

- Introduced optional chromeExecutablePath field in BrowserSettings and UpdateBrowserSettingsRequest.
- Updated updateBrowserSettings function to merge new settings with existing ones, preserving previous values.
- Enhanced BrowserSession to check for the chromeExecutablePath in global state.
- Modified BrowserSettingsSection to include a UI input for specifying the Chrome executable path.

* Removing browser stuff

* Removing browser stuff

* Removing browser stuff

* Removing browser stuff

* Add cute animation

* Add cute animation

* Add cute animation

* Add cute animation

* Add cute animation

* Add cute animation

* Add cute animation

* adding stuff

---------

Co-authored-by: Cline Evaluation <[email protected]>
@d-oit d-oit deleted the fix/invalid-operations-xml-format branch June 4, 2025 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mrubens mrubens Awaiting requested review from mrubens mrubens will be requested when the pull request is marked ready for review mrubens is a code owner

@cte cte Awaiting requested review from cte cte will be requested when the pull request is marked ready for review cte is a code owner

@jr jr Awaiting requested review from jr jr will be requested when the pull request is marked ready for review jr is a code owner

1 more reviewer

@samhvw8 samhvw8 samhvw8 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@d-oit @samhvw8