Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ebe3a651e0
ℹ️ 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c14cd5e650
ℹ️ 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ec852ea9a
ℹ️ 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00ca197bd8
ℹ️ 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7224bcaf12
ℹ️ 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 884beac524
ℹ️ 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".
| errors = validator.iter_errors(candidate_arguments) | ||
| if all(error.validator == "required" for error in errors): | ||
| candidates.append((candidate_properties, candidate_arguments)) |
There was a problem hiding this comment.
Reject incomplete branches once arguments are complete
When alternatives are distinguished by required properties rather than a discriminator, this accepts an invalid branch even after the complete arguments have parsed. For example, with one branch requiring an object-valued payload and another requiring query, an input containing only payload validates the first candidate after coercion, while the second candidate is also retained because its sole error is required; since two candidates remain, payload is returned as a string. Ignore required-only errors only while complete is false, and require full validation at completion.
Useful? React with 👍 / 👎.
| candidate_schema = base_schema | { | ||
| "allOf": [*base_schema["allOf"], branch] | ||
| } |
There was a problem hiding this comment.
Keep unevaluatedProperties adjacent to the selected branch
For a Draft 2020-12 schema with root oneOf/anyOf and unevaluatedProperties: false, moving the root remainder and selected branch into separate allOf subschemas changes validation semantics: the unevaluatedProperties keyword in the remainder cannot see properties evaluated by the sibling branch, so a valid branch instance is rejected as containing unevaluated fields. No candidate is then selected, leaving branch-specific object or array arguments in their projected string form. Construct the candidate so the selected branch remains adjacent to root-level unevaluatedProperties.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3018725ef9
ℹ️ 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".
| elif current_schema != candidate_schema: | ||
| properties[name] = {} |
There was a problem hiding this comment.
Preserve common types across non-identical branch schemas
When every anyOf/oneOf branch declares the same argument as an object or array but applies different nested constraints, the schemas are unequal and this collapses the property hint to {}. Detectors such as Qwen then infer string, so a valid nested argument remains JSON-encoded text even though every alternative agrees on its container type—a common case for discriminated unions with branch-specific payload fields. Combine the candidate schemas under the relevant combinator, or at least preserve their common inferred type, instead of requiring exact schema equality.
Useful? React with 👍 / 👎.
|
@JustinTong0323 Could you help to review this? Thanks! |
|
fixed with #36626 |
Motivation
Fixes #32286 and generalizes the MiniMax-specific approach in #32299.
Several XML-style tool-call detectors infer argument types only from
tool.function.parameters["properties"]. Valid JSON Schemas that declare properties below a root-levelallOf,anyOf, oroneOftherefore hide those type hints. In the reported MiniMax-M3 case, numbers become strings and arrays can leak raw parser tags instead of producing typed arguments.This change exposes those properties at the shared
FunctionCallParserlayer without changing the original tool schema or adding parser-specific JSON Schema logic.Modifications
get_tool_parser_property_hints, an explicitly lossy detector-facing property projection:allOf,anyOf, andoneOf;{}so detectors keep their conservative string fallback.detector_toolsonce inFunctionCallParserusing Pydantic copies only when the projected properties differ.detector_toolsto non-streaming parsing, streaming increments, and stream finalization.self.toolsand the original schemas unchanged for structural tags, constrained decoding, and validation.number/stringList/numberListschema shape.The projection is intentionally a static type-hint view. It is not complete JSON Schema evaluation and does not select or validate branches at runtime. Exact branch-dependent interpretation would require buffering a complete tool call before emitting arguments, which is incompatible with incremental streaming.
Accuracy Tests
Not applicable to model inference. This changes deterministic post-processing of tool-call arguments and does not change model execution or generated tokens.
The regression verifies:
allOf,anyOf, andoneOf;Speed Tests and Profiling
Not applicable to model execution. The projection is computed once when a
FunctionCallParseris constructed; the streaming parse path performs no branch validation or schema mutation.Validation
git diff --checkpassed.maincontains two production files and one test file.Checklist
Review and Merge Process
/tag-and-rerun-ci,/tag-run-ci-label,/rerun-failed-ciCI States
Latest PR Test (Base): ❌ Run #32994620403
Latest PR Test (Extra): ❌ Run #32994619524
Latest PR Test (AMD ROCm 7.2): ❌ Run #32994620722