Repository navigation
fix(tool_parser): coerce XML tool-call args by declared schema type (qwen_xml, glm4_moe) - #1841
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>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 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🎯 4 (Complex) | ⏱️ ~45 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 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".
|
It fixed the js and multiture bfcl: https://github.com/lightseekorg/smg/actions/runs/28146481892 |
param_types_for_function only read a scalar `type`, so an optional string
declared as {"type": ["string", "null"]} or via anyOf/oneOf produced no
entry and fell back to inference (coercing e.g. "4" to a number). Resolve such
schemas to their non-null type so optional strings stay strings. Shared helper,
so qwen_xml/glm4_moe/minimax_m2 all benefit. (No BFCL schema uses unions today,
so scores are unchanged; this is real-world robustness.)
Signed-off-by: key4ng <rukeyang@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/helpers.rs`:
- Around line 22-31: The `schema_type` helper in `helpers.rs` is collapsing
multi-type unions to the first non-`null` branch, which is incorrect for real
unions. Update the logic that handles
`schema.get("type").and_then(Value::as_array)` and the `anyOf`/`oneOf` traversal
so it only returns a type when there is exactly one distinct non-`null`
candidate; if multiple non-`null` types exist, return `None` so callers keep the
existing fallback inference 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: 650b4b33-b97b-4188-8fc5-820dceccd9aa
📒 Files selected for processing (1)
crates/tool_parser/src/parsers/helpers.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 622575e5e2
ℹ️ 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".
Keep the essential coverage — coerce_by_schema_type + param_types_for_function unit tests, and one end-to-end coercion test per XML parser — and drop the redundant no-schema/streaming variants (same helper, covered elsewhere). Trim verbose doc/inline comments. 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: 31b3ad8f60
ℹ️ 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".
…n, restore tests - Revert the nullable/union schema_type resolver back to a scalar `type` lookup. It collapsed real multi-type unions (["string","object"]) to one branch and swallowed bare null for ["string","null"]; unions/nullable now fall back to inference (same as before this PR, no BFCL impact). - coerce_by_schema_type `number` parses JSON instead of f64 so large integers keep precision (e.g. 9007199254740993). - Restore the streaming-path coercion tests (distinct code path) and the boolean-as-string helper assertion. 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: 503ec8a1d4
ℹ️ 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".
| let key = key_match.as_str().trim().to_string(); | ||
| let value = value_match.as_str(); | ||
| let json_value = safe_val(value); | ||
| let json_value = coerce_value(value, param_types.get(&key).map(String::as_str)); |
There was a problem hiding this comment.
When a tool declares an optional string as {"type":["string","null"]} or anyOf/oneOf, param_types_for_function returns no entry, so this new schema-aware path passes None here and falls back to safe_val; <parameter=limit>4</parameter> is therefore still emitted as numeric 4 instead of string "4". Fresh evidence beyond the earlier resolved thread is that the final helper now explicitly documents dropping union/nullable schemas, so the new Qwen path (and the analogous GLM path) remains ineffective for those optional string args.
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 (e.g.,"\"4\""→4as a string) while preserving prior behavior without schema.