Skip to content

Revert "Revert "feat: change defaultToolProtocol default from xml to native"" #10186

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
Merged
mrubens merged 2 commits into from
Dec 18, 2025
Merged

Revert "Revert "feat: change defaultToolProtocol default from xml to native"" #10186

mrubens merged 2 commits into from
Dec 18, 2025

Conversation

@mrubens
Copy link
Collaborator

@mrubens mrubens commented Dec 18, 2025 •
edited by ellipsis-dev bot
Loading

Reverts #9956

Important

Revert default tool protocol from XML to Native, updating resolveToolProtocol.ts, ApiOptions.tsx, and related tests.

  • Behavior:
    • Change default tool protocol from XML to Native in resolveToolProtocol.ts.
    • Update ApiOptions.tsx to reflect Native as the default tool protocol.
  • Tests:
    • Update resolveToolProtocol.spec.ts to test Native as the default fallback.
    • Adjust test descriptions and expectations to align with Native default in resolveToolProtocol.spec.ts.
  • Misc:
    • Fix order of getMcpHub mock in Task.spec.ts.

This description was created by Ellipsis for daf2b97. You can customize this summary. It will automatically update as commits are pushed.

@mrubens mrubens requested review from cte and jr as code owners December 18, 2025 14:49
@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Dec 18, 2025
@roomote
Copy link
Contributor

roomote bot commented Dec 18, 2025 •
edited
Loading

Rooviewer Clock   See task on Roo Cloud

Review completed. No issues found.

This PR is a "revert of a revert" that re-applies the change to switch the default tool protocol from XML to Native for models that support native tools. The changes are correct and safe because:

  • The safety guard at line 20 in resolveToolProtocol.ts ensures native fallback only applies when modelInfo.supportsNativeTools === true
  • Models without native support or with undefined modelInfo correctly default to XML
  • Frontend (ApiOptions.tsx) and backend logic are properly synchronized
  • Tests cover the expected behaviors including edge cases

The latest commit (daf2b97) correctly removes a duplicate getMcpHub mock property from the test file.

Previous reviews

Mention @roomote in a comment to request specific changes to this pull request or fix all unresolved issues.

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Dec 18, 2025
@mrubens
Copy link
Collaborator Author

mrubens commented Dec 18, 2025

@roomote run pnpm check-types and fix the problem please

@hannesrudolph hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Dec 18, 2025
@roomote
Copy link
Contributor

roomote bot commented Dec 18, 2025 •
edited
Loading

Fixaroo Clock   See task on Roo Cloud

Fixed the type error by removing duplicate getMcpHub property in Task.spec.ts. All local checks passed.

View commit | Revert commit

@mrubens mrubens merged commit 3cb2c1d into main Dec 18, 2025
10 checks passed
@mrubens mrubens deleted the revert-9956-revert-9892-feature/default-tool-protocol-to-native branch December 18, 2025 15:31
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Dec 18, 2025
@github-project-automation github-project-automation bot moved this from Triage to Done in Roo Code Roadmap Dec 18, 2025
@cte cte mentioned this pull request Dec 18, 2025
Merged
@AhrenFullStop AhrenFullStop mentioned this pull request Dec 19, 2025
Closed
@roomote roomote bot mentioned this pull request Dec 19, 2025
Closed
7 tasks
@nabilfreeman nabilfreeman mentioned this pull request Jan 5, 2026
Closed
@nabilfreeman nabilfreeman mentioned this pull request Jan 6, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@daniel-lxs daniel-lxs daniel-lxs approved these changes

@roomote roomote[bot] roomote[bot] approved these changes

@cte cte Awaiting requested review from cte cte is a code owner

@jr jr Awaiting requested review from jr jr is a code owner

Assignees

No one assigned

Labels

Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. lgtm This PR has been approved by a maintainer size:M This PR changes 30-99 lines, ignoring generated files.

Projects

Roo Code Roadmap
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@mrubens @daniel-lxs @hannesrudolph @roomote