Skip to content

Conversation

@daniel-lxs
Copy link
Member

@daniel-lxs daniel-lxs commented Nov 18, 2025

Problem

The issue occurred when tool results were being formatted. The code was calling resolveToolProtocol() to determine whether to use native or XML protocol formatting. However, if the API configuration changed between when the tool call was made and when the result was being formatted (e.g., user switched profiles), it would use the wrong protocol format.

This caused native protocol tool calls to be formatted with XML-style text headers like [tool_name] Result:, creating malformed message structures that violated API expectations and resulted in 400 errors.

Solution

Instead of recalculating the protocol, we now check for the presence of an ID on the tool call itself:

  • Native protocol tool calls ALWAYS have an ID (set during parsing)
  • XML protocol tool calls NEVER have an ID (parsed from XML text)

This approach is race-condition-free because the ID is an immutable property of each tool call that definitively indicates which protocol was used to create it. It also gracefully handles provider switches - if a provider doesn't support native tools, the tool calls simply won't have IDs and will be formatted correctly for XML protocol.

Changes

  • Modified presentAssistantMessage.ts to determine protocol by checking for tool call ID presence instead of calling resolveToolProtocol()
  • This eliminates the race condition where configuration changes mid-request would cause incorrect protocol formatting
  • All tests pass

Testing

  • Ran full test suite - all tests passing
  • Manual testing confirmed the fix resolves the 400 error scenario

Important

Fix race condition in presentAssistantMessage.ts by determining protocol via tool call ID presence, resolving 400 errors.

  • Behavior:
    • In presentAssistantMessage.ts, determine protocol by checking for tool call ID presence instead of using resolveToolProtocol().
    • Native protocol tool calls have an ID; XML protocol tool calls do not.
    • Eliminates race condition caused by mid-request configuration changes.
  • Testing:
    • All tests pass.
    • Manual testing confirms resolution of 400 error scenario.

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

The issue occurred when tool results were being formatted. The code was
calling resolveToolProtocol() to determine whether to use native or XML
protocol formatting. However, if the API configuration changed between
when the tool call was made and when the result was being formatted
(e.g., user switched profiles), it would use the wrong protocol format.

This caused native protocol tool calls to be formatted with XML-style
text headers like '[tool_name] Result:', creating malformed message
structures that violated API expectations and resulted in 400 errors.

Solution:
Instead of recalculating the protocol, we now check for the presence of
an ID on the tool call itself:
- Native protocol tool calls ALWAYS have an ID (set during parsing)
- XML protocol tool calls NEVER have an ID (parsed from XML text)

This approach is race-condition-free because the ID is an immutable
property of each tool call that definitively indicates which protocol
was used to create it. It also gracefully handles provider switches -
if a provider doesn't support native tools, the tool calls simply won't
have IDs and will be formatted correctly for XML protocol.
@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. bug Something isn't working labels Nov 18, 2025
@roomote
Copy link
Contributor

roomote bot commented Nov 18, 2025

Rooviewer Clock   See task on Roo Cloud

Review Complete - No New Issues Found

Reviewed commit f30d413 which removes a redundant comment. The change is clean and improves code clarity by eliminating duplicate documentation.

Previous reviews

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

@hannesrudolph hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Nov 18, 2025
@daniel-lxs daniel-lxs moved this from Triage to PR [Needs Review] in Roo Code Roadmap Nov 18, 2025
@mrubens mrubens merged commit 1fa12f6 into main Nov 18, 2025
10 checks passed
@github-project-automation github-project-automation bot moved this from PR [Needs Review] to Done in Roo Code Roadmap Nov 18, 2025
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Nov 18, 2025
@mrubens mrubens deleted the fix/native-tool-protocol-race-condition branch November 18, 2025 18:49
mini2s added a commit to zgsm-ai/costrict that referenced this pull request Nov 19, 2025