Fix nested MCP integer coercion#10633
Conversation
Coerce whole-number MCP tool arguments through nested JSON Schema shapes, including object properties, array items, oneOf/anyOf/allOf branches, nullable integer types, and local refs. Fixes warpdotdev#10596
github-actions
Bot
added
the
external-contributor
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR replaces the top-level-only MCP integer coercion helper with a recursive JSON Schema walker and adds coverage for nested objects, arrays, refs, nullable integer types, and composition branches.
Concerns
anyOf/oneOfschemas are traversed branch-by-branch without checking which branch the current value actually matches, so alternate branches can coerce fields that are not integers for the selected schema.additionalPropertiesis applied to all object members, including properties explicitly declared inproperties, which does not match JSON Schema semantics and can coerce named number fields.- The new f64 range guard can still accept 2^63 because
i64::MAX as f64rounds up, and the subsequent cast saturates toi64::MAX.
Verdict
Found: 0 critical, 3 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| continue; | ||
| } | ||
|
|
||
| for keyword in ["allOf", "anyOf", "oneOf"] { |
There was a problem hiding this comment.
anyOf/oneOf branch without checking whether the current value matches that branch, so an alternate schema can coerce 1.0 to 1 for a field whose selected branch declares type: number. Only apply allOf unconditionally, and select a matching anyOf/oneOf branch before coercing.
|
|
||
| if let Some(additional_properties) = schema.get("additionalProperties") { | ||
| if additional_properties.is_object() { | ||
| for property_value in object.values_mut() { |
There was a problem hiding this comment.
additionalProperties must only apply to keys not declared in properties; iterating over every object value also coerces named fields. A schema with properties.price as number and integer additionalProperties would turn price: 1.0 into 1.
| if !float.is_finite() || float.fract() != 0.0 { | ||
| return; | ||
| } | ||
| if float < i64::MIN as f64 || float > i64::MAX as f64 { |
There was a problem hiding this comment.
2^63 because i64::MAX as f64 rounds up; Rust's float-to-int cast then saturates to i64::MAX, changing the argument value. Reject values at and above the first unrepresentable upper bound before assigning.
princepal9120
force-pushed
the
prince/10596-mcp-integer-coercion
branch
from
2191830 to
e8d220f
Compare
|
Addressed the review feedback in the latest push:
Local validation completed: /oz-review |
|
This PR is not linked to an issue that is marked with Issue-state enforcement details:
Readiness check:
To continue, link this PR to a same-repo issue such as Powered by Oz |
There was a problem hiding this comment.
This PR is not linked to an issue that is marked with ready-to-implement.
Issue-state enforcement details:
-
Associated same-repo issues checked: #10596
-
Required readiness label:
ready-to-implement
Readiness check:
- #10596: missing
ready-to-implement; readiness labels present: none
To continue, link this PR to a same-repo issue such as Closes #123 in the PR description, and make sure that issue has ready-to-implement.
Powered by Oz
|
Hi @princepal9120 — a reviewer requested changes on this PR and it hasn't had activity from you in 23 days. When you get a chance, please push updates or reply to the review so a reviewer can take another look. Without activity, this PR will be automatically closed after 30 days of inactivity. |
|
/oz-review The three schema coercion concerns from the previous review have been addressed in the latest commits:
Issue #10596 also has the ready-to-implement label. Requesting re-review. |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR extends MCP integer coercion to recurse through nested object schemas, arrays, local $refs, nullable integer types, composition branches, and additionalProperties cases before dispatching tool calls.
Concerns
- The new branch-selection helper can select the wrong
anyOf/oneOfbranch because it only checks broad JSON types and present property schemas, ignoring common branch-discriminating keywords such asrequired,const/enum, andadditionalProperties: false. That can still coerce values according to a branch that the argument object should not match.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
|
||
| for keyword in ["anyOf", "oneOf"] { | ||
| if let Some(schemas) = schema.get(keyword).and_then(|schemas| schemas.as_array()) { | ||
| if let Some(nested_schema) = schemas.iter().find(|nested_schema| { |
There was a problem hiding this comment.
schema_matches_value ignores required, const/enum, and additionalProperties: false, so find can select a oneOf/anyOf branch just because its optional properties type-check. OpenAPI-derived schemas commonly use those keywords to discriminate branches, and picking the wrong branch can still coerce floats to integers for a schema branch that should not apply; honor those keywords or skip coercion when branch matching is ambiguous.
1 participant
Summary
$refs.anyOf/oneOfbranch before coercing, applyadditionalPropertiesonly to undeclared keys, and reject the f64 upper i64 boundary.Why
Strict MCP servers reject
5.0for fields declared as integers. The previous helper only handled top-leveltype: integerproperties, so common OpenAPI-derived schemas still leaked float JSON.Tests
git diff --checkrustfmt app/src/ai/blocklist/action_model/execute/call_mcp_tool.rs app/src/ai/blocklist/action_model/execute/call_mcp_tool_tests.rscargo test -p warp call_mcp_tool, but this machine ran out of disk while compiling dependencies.Fixes #10596