fix(tool_parser): coerce XML tool-call args by declared schema type (qwen_xml, glm4_moe)#1841
fix(tool_parser): coerce XML tool-call args by declared schema type (qwen_xml, glm4_moe)#1841key4ng wants to merge 4 commits into
Conversation
qwen_xml and glm4_moe blindly JSON-parsed XML parameter values, coercing
string-typed args ("4", "true", "[60,30]", "{...}") to int/bool/array/object.
BFCL JavaScript schemas declare many params as string, so the coerced type was
wrong and the call was rejected (simple_javascript: qwen -32, glm -40 vs vLLM,
which keeps them as strings).
Coerce by the function's declared schema (helpers::param_types_for_function +
coerce_by_schema_type): a `string` param stays a string, typed params are
parsed, and unknown types fall back to the previous inference. minimax_m2
already did this. The gateway already passes tools via parse_complete_with_tools;
both parsers now honor it in the complete and streaming paths.
Signed-off-by: key4ng <rukeyang@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughTool parsers now coerce tool-call arguments by declared schema when tools are provided, while preserving prior inference behavior when no tools are supplied. Both complete and streaming parsing paths were updated to accept tools. ChangesSchema-aware coercion for tool parsers
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the schema-aware coercion changes in both qwen_xml and glm4_moe parsers. The implementation correctly mirrors the existing minimax_m2 pattern — param_types_for_function + coerce_by_schema_type are threaded through both the complete and streaming paths, with proper fallback to blind inference (safe_val/infer_value) when no schema is available. The extracted infer_value and coerce_value functions are behaviorally equivalent to the code they replaced. Test coverage is solid — string-typed params with numeric/bool/array/object values, non-string typed params, and the no-schema path are all exercised.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces schema-aware parameter coercion for both the GLM-4 MoE and Qwen XML tool parsers. By checking the declared parameter types from the tools' JSON schemas, the parsers can now preserve string types for values that look like numbers, booleans, arrays, or objects, matching the behavior of vLLM. The review feedback suggests optimizing the Qwen XML parser by replacing the HashMap-based parameter type lookup with a zero-allocation helper function to avoid costly heap allocations and string cloning, especially during hot streaming paths.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/tool_parser/src/parsers/glm4_moe.rs`:
- Line 282: Add a streaming regression test for schema-aware coercion in the GLM
incremental path. The issue is that `parse_tool_calls_from_text` in
`Glm4MoeParser` forwards `tools` during streaming, but the current coverage only
validates `parse_complete_with_tools`. Add a test that drives the
incremental/streaming parser path and asserts tool argument coercion behaves the
same as the complete parse path, using the relevant `Glm4MoeParser` parsing
methods and tool-schema inputs to catch regressions there.
In `@crates/tool_parser/src/parsers/qwen_xml.rs`:
- Around line 245-257: The schema-aware logic in parse_and_stream_parameters
needs coverage for streamed parameter fragments, not just complete parses. Add
an incremental test around qwen_xml parsing that feeds partial XML fragments
into parse_and_stream_parameters/parse_xml_format, concatenates the emitted
ToolCallItem parameters, and verifies schema coercion keeps string-typed fields
as strings even when values look like 4 or true. Use the existing parser helpers
and current_function_name-based type lookup so the test exercises the same
streaming path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0e6db6f5-364d-4957-a15c-815363f9d6d3
📒 Files selected for processing (2)
crates/tool_parser/src/parsers/glm4_moe.rscrates/tool_parser/src/parsers/qwen_xml.rs
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24ba480bd3
ℹ️ 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".
Add parse_incremental tests for qwen_xml and glm4_moe asserting string-typed params stay strings (and non-string ones coerce) during streaming, not just parse_complete_with_tools (CodeRabbit). Signed-off-by: key4ng <rukeyang@gmail.com>
coerce_by_schema_type kept a string param verbatim, so a model emitting a JSON
string literal ("4") for a string param produced '"4"' (extra quotes) — a
regression vs the old safe_val path. Unwrap a JSON string literal to its content
while still keeping bare 4/true/[60,30] as strings. Fixes the qwen_xml/glm4_moe
string fast-path and keeps minimax_m2 (same helper) consistent.
Signed-off-by: key4ng <rukeyang@gmail.com>
'unparseable' -> plain wording; no code change. Signed-off-by: key4ng <rukeyang@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c26fa182d6
ℹ️ 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".
|
|
||
| // Coerce by the function's declared schema so e.g. a `string` parameter | ||
| // whose value looks numeric/bool/array stays a string (matches vLLM). | ||
| let param_types = helpers::param_types_for_function(tools, &function_name); |
There was a problem hiding this comment.
When a tool schema represents an optional string with a JSON Schema union such as {"anyOf":[{"type":"string"},{"type":"null"}]} or "type":["string","null"], param_types_for_function produces no entry because it only reads scalar type values. This newly added schema-aware path then calls coerce_value(..., None) and falls back to the old inference, so <parameter=limit>4</parameter> is still emitted as numeric 4 instead of string "4" for nullable string parameters; the same schema lookup is used by the GLM path as well.
Useful? React with 👍 / 👎.
1 participant
Description
Problem
JavaScript collapse — root cause found, fixable (the clearest issue)
SMG's
qwen_xmlandglm4_moetool parsers blindly type-coerce XML parameter values, ignoring the schema:isCompletetrue"true"✓true(bool)limit4"4"✓4(int)coordinates[60,30]"[60,30]"✓[60,30](list)chart{...}"{...}"✓{...}(dict)qwen_xml::safe_valandglm4_moedoserde_json::from_str(value)and keep the parsed type. JS schemas declare many params as String, so coercion produces the wrong type → BFCL rejects. vLLM keeps them as strings (schema-correct).The fix already exists in-repo:
minimax_m2useshelpers::param_types_for_function+helpers::coerce_by_schema_type(schema-aware) — which is why minimax is only −6 vs qwen −32 / glm −40. The gateway already passestoolsto the parser (parse_complete_with_tools);qwen_xmljust doesn't override it (ignores tools), andglm4_moeparses blindly. Fix: make both use schema-aware coercion likeminimax_m2. This should also recover much of qwen/glm'slive_multiple(−8.5) andmulti_turnloss (same String-param coercion across categories).Observed on the nightly BFCL A/B (
simple_javascript, SMG vs vLLM): qwen3.6 −32, glm-5.2 −40, deepseek-v4 −10, minimax −6 (minimax already schema-aware).Solution
qwen_xmlandglm4_moenow coerce each XML argument by its declared JSON-schema type viahelpers::param_types_for_function+helpers::coerce_by_schema_type:stringparameter keeps a numeric/bool/array/object-looking value as a string,integer/number/boolean/object/array) are parsed,Both parsers now thread
toolsthrough the complete (parse_complete_with_tools) and streaming (parse_incremental) paths;parse_complete(no tools) preserves the old inference. Mirrors the existingminimax_m2implementation.Changes
crates/tool_parser/src/parsers/qwen_xml.rs—parse_complete_with_toolsoverride + schema-awarecoerce_value(keepssafe_valas the no-schema fallback); thread tools through streaming.crates/tool_parser/src/parsers/glm4_moe.rs— schema-aware coercion inparse_arguments(extracted blind inference intoinfer_valueas the fallback);parse_complete_with_toolsoverride; thread tools through.Test Plan
cargo test -p tool-parser(lib + integration, incl. existing qwen/glm suites) all pass; new coercion tests added.cargo +nightly fmtandcargo clippy -p tool-parser --all-targets --all-features -- -D warningsclean.Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspasses (tool_parser crate)Summary by CodeRabbit
stringcoercion to unwrap JSON string literals (while preserving prior behavior when no tools/schema are provided).