Repository navigation
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
lutz-grex
left a comment
There was a problem hiding this comment.
Thanks for covering the JSON Schema types and turning deeply nested JSON into a parameter error. I found two correctness issues to address before merging:
- Keep coercion scoped to the invoked tool. The new branches at
mobilerun/agent/fast_agent/xml_parser.py:397–446use the flat map fromToolRegistry.get_param_types. MCP tools are registered after the built-ins, so an MCP object parameter calledmessagemakescomplete(success=true, message="Done")fail with “expected object.” The native path applies the same map to string arguments. This failure is newly introduced for integer/array/object schemas even though the shared-map problem predates the PR. Please prevent unrelated tools from changing each other's coercion, with a regression case covering a built-in and an MCP tool sharing a parameter name. - Avoid rounding before deciding whether a value is an integer (
xml_parser.py:403–407). For example,"1.0000000000000001"currently becomes integer1because the intermediate float loses the fractional part. Please parse the decimal/exponent form without losing precision before checking integrality and converting, and add a fractional precision-boundary case.
Two smaller points:
xml_parser.py:424–430: array inputnullbecomes[None]. Please reject it for an array-only schema, or preserve null only when the schema actually permits it. An optional parameter may be omitted; optional does not itself mean nullable.- A native-tool test with string-encoded object/integer arguments would cover the second path. The array error at
:433could also use the declared type name rather than always saying “list.”
Coerce each XML and native tool call with its own tool's parameter types (new ToolRegistry.get_tool_param_types), so an MCP tool's `message: object` no longer breaks complete(message="Done"). Check integer values with Decimal so "1.0000000000000001" is rejected instead of rounded to 1. Reject null for array parameters unless the schema type list allows null, and name the declared type in the error. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for the careful review. All points are addressed in d160a1e.
(a) (b) The array error now names the declared type, for example "expected array" or "expected list". New native test The fake registries in the FastAgent tests now return per-tool maps. Tests: the fast agent, native tools, secret replay and open app test files pass (129 passed). The full suite has 1606 passed and 9 failed. The same 9 fail on the branch without this change on my Windows machine (file encoding and permission tests), so they are not related. ruff and black are clean. Validated with Claude Opus 5.5. |
What
_coerce_paraminmobilerun/agent/fast_agent/xml_parser.pyonly convertedboolean,numberandlist. MCP tools declare parameters with JSON Schema types (integer,array,object), copied unchanged byschema_to_parameters. In the XML tool call path those values reached the tool as raw strings.This PR targets
develop. The diff also applies cleanly tomain.Fixes #543
Changes
integer: parsed as an int. A whole float such as5.0or1e3gives an int.2.5,nan,inf,1e999and text set the parameter error.array: handled like the existinglist.object: parsed JSON must be an object, otherwise the parameter error is set.json.loadsraisesRecursionErrornow sets the parameter error forlist,arrayandobject(before,listlet the exception escape).Behavior changes beyond the bug case
integer,arrayorobject, including user custom tools, now gets an int, list or dict instead of a string. This holds in the XML path and in the native path, because both call_coerce_paramfor string values. A tool that does its ownjson.loadson an object parameter would now get a dict. Anobjectstring that is not a JSON object now gives a parameter error instead of passing through.ToolRegistry.get_param_typesis flat across tools and MCP tools win. So an MCP tool that declares a parameter named like a built-in string parameter (for exampletextormessage) as integer, array or object makes the built-intypeandcompletecalls fail with an expected-type error. The same already happens today fornumber. A per-tool type map is the real fix and I left it out of this PR.integeruses Pythonint(), so unicode digits and NBSP padding are accepted (59 of 20000 fuzz cases), the same as the existingnumberbranch. Whole floats beyond 2^53 lose precision like JSON clients do.number,boolean,stringand unknown types are unchanged. 0 of 147 neighbor rows changed for those types.Tests
tests/test_fast_agent_xml_parser.py. They fail on main (17 failed subtests) and pass with the change.python -m pytest -q -p no:cacheprovider tests: 9 failed, 1464 passed, 1 skipped on main and 9 failed, 1468 passed, 1 skipped with this change. The same 9 fail on both (Windows environment, for example the mcp stdio tests).black --check(26.10.1) andruff checkpass.banditfindings (B405 and B314 on ElementTree) and the onemypyerror are the same on main and not on lines this change touches.Validated with Claude Opus 5.5.