Skip to content

fix(fast-agent): coerce integer, array and object XML parameters - #544

Open
goranai wants to merge 2 commits into
droidrun:developfrom
goranai:feature/xml-coerce-json-schema-types
Open

goranai wants to merge 2 commits into
droidrun:developfrom
goranai:feature/xml-coerce-json-schema-types

Conversation

@goranai

@goranai goranai commented Nov 12, 2026

Copy link
Copy Markdown

What

_coerce_param in mobilerun/agent/fast_agent/xml_parser.py only converted boolean, number and list. MCP tools declare parameters with JSON Schema types (integer, array, object), copied unchanged by schema_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 to main.

Fixes #543

Changes

  • integer: parsed as an int. A whole float such as 5.0 or 1e3 gives an int. 2.5, nan, inf, 1e999 and text set the parameter error.
  • array: handled like the existing list.
  • object: parsed JSON must be an object, otherwise the parameter error is set.
  • A value nested so deep that json.loads raises RecursionError now sets the parameter error for list, array and object (before, list let the exception escape).

Behavior changes beyond the bug case

  • Any tool that declares integer, array or object, 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_param for string values. A tool that does its own json.loads on an object parameter would now get a dict. An object string that is not a JSON object now gives a parameter error instead of passing through.
  • The type map from ToolRegistry.get_param_types is flat across tools and MCP tools win. So an MCP tool that declares a parameter named like a built-in string parameter (for example text or message) as integer, array or object makes the built-in type and complete calls fail with an expected-type error. The same already happens today for number. A per-tool type map is the real fix and I left it out of this PR.
  • integer uses Python int(), so unicode digits and NBSP padding are accepted (59 of 20000 fuzz cases), the same as the existing number branch. Whole floats beyond 2^53 lose precision like JSON clients do.
  • number, boolean, string and unknown types are unchanged. 0 of 147 neighbor rows changed for those types.

Tests

  • 4 new tests in tests/test_fast_agent_xml_parser.py. They fail on main (17 failed subtests) and pass with the change.
  • Targeted run (xml parser, native tools, multi block, malformed guard): 96 passed (57 subtests).
  • Whole suite, 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).
  • End to end with a schema-validating MCP server (mcp 1.30.0): rejected on main, accepted with this change.
  • Fuzz of 20000 cases per type against a JSON reference: all strings on main. With the change there are 0 mis-typed values, 0 wrong values, 0 wrongly rejected and 0 exceptions.
  • black --check (26.10.1) and ruff check pass. bandit findings (B405 and B314 on ElementTree) and the one mypy error are the same on main and not on lines this change touches.
  • CI has not run on this PR yet.

Validated with Claude Opus 5.5.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@lutz-grex lutz-grex left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Keep coercion scoped to the invoked tool. The new branches at mobilerun/agent/fast_agent/xml_parser.py:397–446 use the flat map from ToolRegistry.get_param_types. MCP tools are registered after the built-ins, so an MCP object parameter called message makes complete(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.
  2. Avoid rounding before deciding whether a value is an integer (xml_parser.py:403–407). For example, "1.0000000000000001" currently becomes integer 1 because 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 input null becomes [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 :433 could 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>
@goranai

goranai commented Nov 13, 2026

Copy link
Copy Markdown
Author

Thanks for the careful review. All points are addressed in d160a1e.

  1. Coercion is now scoped to the invoked tool. I added ToolRegistry.get_tool_param_types(), which returns a {tool: {param: type}} map. FastAgent passes it to both the XML parser and the native tool parser through a new tool_param_types argument. Each call is coerced with its own tool's types only. The old flat param_types argument still works for existing callers. New test test_coercion_is_scoped_to_the_invoked_tool registers a built-in complete with message: string, then an MCP tool (via schema_to_parameters) with message: object. It checks that complete(success=true, message="Done") keeps the string and the MCP call still gets its object, on both the XML and the native path.

  2. Integer parsing now uses decimal.Decimal. A value is accepted only when it is finite and exactly integral, so "1.0000000000000001" and "9007199254740992.5" are rejected. "9007199254740993" and "9007199254740993.0" keep every digit. Values longer than Python's int digit limit are rejected, so "1e5000" stays cheap. The test now covers these boundary cases.

(a) null is rejected for an array or list schema. It becomes None only when the schema type list allows it, for example ["array", "null"]. Test test_array_rejects_null_unless_the_schema_allows_it covers both.

(b) The array error now names the declared type, for example "expected array" or "expected list". New native test test_string_encoded_object_and_integer_arguments_are_coerced sends string-encoded object and integer arguments, plus a wrong object and a fractional integer.

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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants