fix: add XML tool call parsing fallback for Qwen3-coder via Ollama#6882
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3ee90bcba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
nice to have these tests
|
I added a commit to address the comments:
This change is to prevent noisy duplicate response text showing up in the conversation flow. |
|
thanks @kuccello - looks nice - BTW I am working on a extensive model/extension combo to test with open models - and this will solve a recurring issue I see in test suites. |
|
@kuccello so I'm concerned about doing more stuff like this in the openai provider as it puts main openai at risk.. you can see conditional body handling scattered per provider in some cases, such as this in the tetrate one // Handle Google-compatible model responses differently
if is_google_model(payload) {
return handle_response_google_compat(response).await;
}
// For OpenAI-compatible models, parse the response body to JSON
let response_body = handle_response_openai_compat(response)
.await
.map_err(|e| ProviderError::RequestFailed(format!("Failed to parse response: {e}")))?;Agree normal responses are a lot less tricky than tool responses. #6832 introduces a different type For now, it would be best to move this code to the ollama provider because it has templating that isn't guaranteed to act the same in other providers. Since it is left alone and not using If you run into a problem in refactoring and would prefer a hand, I can try to reproduce your work in a way that doesn't affect the main OpenAiProvider. |
|
@codefromthecrypt @kuccello we could also move to a branch so it is covered by e2e tests with live providers while at it? really good to get this in |
|
@codefromthecrypt I will make an adjustment based on your comment |
codefromthecrypt
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround!
nit try to remove some of the single line comments and also rename stream_ollama_compat -> stream_ollama since the _compat thing is not really relevant here.
b8c76de to
86dfeb5
Compare
|
Here's a summary of the changes for the Qwen3-coder XML tool call fix: ProblemWhen using Qwen3-coder through Ollama with many tools (6+), the model outputs XML-style tool calls instead of the expected JSON format: <function=developer__text_editor><parameter=command>view</parameter><parameter=path>/some/path</parameter></function>Instead of: {"tool_calls": [{"function": {"name": "developer__text_editor", "arguments": {"command": "view", "path": "/some/path"}}}]}Architectural SolutionI implemented an Ollama-specific post-processing wrapper that isolates the XML parsing logic to the Ollama provider only, without modifying the core OpenAI format handling. Files Changed
Key Design Decisions
Data FlowWhy This Approach?
|
There was a problem hiding this comment.
this is an abstraction break, as we are putting provider-specific logic into the openai_compatible.rs
can you please contain changes related to ollama to files named ollama?
252948e to
fe2aca1
Compare
|
All comments addressed |
When using Qwen3-coder model through Ollama with many tools (6+), the model outputs XML-style tool calls in the content field instead of using the native JSON tool_calls format. This causes Goose to fail to execute any tools. This commit adds XML parsing as a fallback mechanism: - Create formats/ollama.rs with XML tool call parsing logic - Add parse_xml_tool_calls() to parse <function=name><parameter=key>value</parameter></function> format - Wrap standard OpenAI response parsing with XML fallback for Ollama - Add response_to_streaming_message_ollama() that buffers text when XML markers detected - Update ollama.rs to use Ollama-specific format module and streaming - Add comprehensive unit tests for XML parsing The fix is backward-compatible and only activates when JSON tool_calls are absent, ensuring existing providers continue to work normally. The XML parsing is isolated to the Ollama provider only. Tested with Qwen3-coder:latest via Ollama with 11 developer tools. Signed-off-by: kuccello <kuccello@users.noreply.github.com>
84311d0 to
80a7719
Compare
|
@kuccello thanks for your patience. very helpful! |
* origin/main: (55 commits) test(mcp): add image tool test and consolidate MCP test fixtures (#7019) fix: remove Option from model listing return types, propagate errors (#7074) fix: lazy provider creation for goose acp (#7026) (#7066) Smoke tests: split compaction test and use debug build (#6984) fix(deps): trim bat to resolve RUSTSEC-2024-0320 (#7061) feat: expose AGENT_SESSION_ID env var to extension child processes (#7072) fix: add XML tool call parsing fallback for Qwen3-coder via Ollama (#6882) Remove clippy too_many_lines lint and decompose long functions (#7064) refactor: move disable_session_naming into AgentConfig (#7062) Add global config switch to disable automatic session naming (#7052) docs: add blog post - 8 Things You Didn't Know About Code Mode (#7059) fix: ensure animated elements are visible when prefers-reduced-motion is enabled (#7047) Show recommended model on failture (#7040) feat(ui): add session content search via API (#7050) docs: fix img url (#7053) Desktop UI for deleting custom providers (#7042) Add blog post: How I Used RPI to Build an OpenClaw Alternative (#7051) Remove build-dependencies section from Cargo.toml (#6946) add /rp-why skill blog post (#6997) fix: fix snake_case function names in code_execution instructions (#7035) ... # Conflicts: # scripts/test_subrecipes.sh
* main: (101 commits) fix: lazy provider creation for goose acp (#7026) (#7066) Smoke tests: split compaction test and use debug build (#6984) fix(deps): trim bat to resolve RUSTSEC-2024-0320 (#7061) feat: expose AGENT_SESSION_ID env var to extension child processes (#7072) fix: add XML tool call parsing fallback for Qwen3-coder via Ollama (#6882) Remove clippy too_many_lines lint and decompose long functions (#7064) refactor: move disable_session_naming into AgentConfig (#7062) Add global config switch to disable automatic session naming (#7052) docs: add blog post - 8 Things You Didn't Know About Code Mode (#7059) fix: ensure animated elements are visible when prefers-reduced-motion is enabled (#7047) Show recommended model on failture (#7040) feat(ui): add session content search via API (#7050) docs: fix img url (#7053) Desktop UI for deleting custom providers (#7042) Add blog post: How I Used RPI to Build an OpenClaw Alternative (#7051) Remove build-dependencies section from Cargo.toml (#6946) add /rp-why skill blog post (#6997) fix: fix snake_case function names in code_execution instructions (#7035) Document max_turns settings for recipes and subagents (#7044) feat: update Groq declarative data with Preview Models (#7023) ...
* origin/main: (54 commits) chore: strip posthog for sessions/models/daily only (#7079) tidy: clean up old benchmark and add gym (#7081) fix: use command.process_group(0) for CLI providers, not just MCP (#7083) added build notify (#6891) test(mcp): add image tool test and consolidate MCP test fixtures (#7019) fix: remove Option from model listing return types, propagate errors (#7074) fix: lazy provider creation for goose acp (#7026) (#7066) Smoke tests: split compaction test and use debug build (#6984) fix(deps): trim bat to resolve RUSTSEC-2024-0320 (#7061) feat: expose AGENT_SESSION_ID env var to extension child processes (#7072) fix: add XML tool call parsing fallback for Qwen3-coder via Ollama (#6882) Remove clippy too_many_lines lint and decompose long functions (#7064) refactor: move disable_session_naming into AgentConfig (#7062) Add global config switch to disable automatic session naming (#7052) docs: add blog post - 8 Things You Didn't Know About Code Mode (#7059) fix: ensure animated elements are visible when prefers-reduced-motion is enabled (#7047) Show recommended model on failture (#7040) feat(ui): add session content search via API (#7050) docs: fix img url (#7053) Desktop UI for deleting custom providers (#7042) ...
|
🚀 !!! Thank you all |
…lock#6882) Signed-off-by: kuccello <kuccello@users.noreply.github.com> Co-authored-by: kuccello <kuccello@users.noreply.github.com>
…lock#6882) Signed-off-by: kuccello <kuccello@users.noreply.github.com> Co-authored-by: kuccello <kuccello@users.noreply.github.com>
When using Qwen3-coder model through Ollama with many tools (6+), the model outputs XML-style tool calls in the content field instead of using the native JSON tool_calls format. This causes Goose to fail to execute any tools.
This commit adds XML parsing as a fallback mechanism:
The fix is backward-compatible and only activates when JSON tool_calls are absent, ensuring existing providers continue to work normally.
Tested with Qwen3-coder:latest via Ollama with 11 developer tools.
Summary
Type of Change
AI Assistance
Testing
cargo test -p goose --lib -- formats::openai::tests- greencargo fmt -p goose && cargo clippy -p goose --lib -- -D warningsRelated Issues
Relates to #6883
Discussion: LINK (if any)
Screenshots/Demos (for UX changes)
Before:
After: