Repository navigation
fix: preserve explicit nulls in MCP tool arguments - #303
liangzhechen655 wants to merge 7 commits into
Conversation
Eigenwise
left a comment
There was a problem hiding this comment.
Source review of eabd141ae4fec8598f0aff96f83fc9ee0dbc7016: preserving explicit nulls for nullable arguments is useful, but mcp_factory.py:182 also forwards null for nonnullable optional fields. For an optional property advertised as type: string without a default, the existing generated model accepts an explicitly supplied value=None. The new dump sends {"value": null}; the old dump omitted it. A server validating that advertised schema rejects the new request.
Preserve the distinction between genuinely nullable arguments and locally accepted None for a nonnullable field. Add that nonnullable regression while retaining the required-null, explicit optional-null, omission, default, zero and false cases. Check both fresh and persistent session forwarding.
This finding is source-derived. I haven't run this head's tests or measured its per-function coverage.
Eigenwise
left a comment
There was a problem hiding this comment.
Rechecked 301f1eb7c947c24678c4ccacf2d6b60cfaf10b43. Plain nonnullable None is now omitted, nullable null and supplied values are retained, and the per-tool referenced-type case passes. Normal CI passed all 35 new cases, 435 tests total with 3 MiniMax skips; formatting and lint passed.
One advertised-nullability gap remains at mcp_factory.py:67-69: an optional value referencing $defs.Value = {"enum": ["brief"]} is converted to Any, so the TypeAdapter check accepts None and forwards {"value": null}. The advertised enum rejects that request; the base omitted the value. Referenced nonnull const and enum inside anyOf have the same gap. An outer type: string with a nullable anyOf also must not forward null.
Preserve null restrictions through references and applicable schema combinations when deciding whether to forward None. Add focused generated-tool argument assertions for these cases in both session paths, keeping the current passing presence cases. These counterexamples are source-derived, not locally executed.
|
Additional integration-review finding on 301f1eb: exclude_unset recurses into nested models, but the nullable-argument filter only covers top-level fields. For cfg referencing an object with optional non-nullable name and a defaulted level, an explicit nested name: null is now forwarded where main stripped it. Please cover referenced nested objects and arrays of objects, preserving explicit null only where that nested schema permits it. This change also stops sending omitted schema defaults, so please call that payload change out in the description. The existing referenced-enum/const nullability review remains open. Please integrate current main before the next round. |
…-arguments # Conflicts: # atomic-agents/atomic_agents/connectors/mcp/mcp_factory.py
|
Thanks for the follow-up examples. I integrated current main ( The PR description and MCP documentation now call out that omitted schema defaults are not sent. Callers can supply a default explicitly when it must be included in the request. All 35 earlier presence/session cases pass. Of 34 additional generated-tool cases, 20 fail against the integrated pre-fix factory and 14 are passing controls; all 34 now pass in fresh and persistent session modes, with the forwarded request checked against the advertised JSON Schema. The full core suite passes 885 tests with 3 credential-gated MiniMax skips. Black, Flake8, locked dependency sync and the wheel build pass. |
Eigenwise
left a comment
There was a problem hiding this comment.
Rechecked 29c6ce75. The previously reported referenced and nested nullability cases are repaired, and all 79 serializer/null-argument tests pass locally.
Two remaining cases reproduce through generated tools at the session.call_tool boundary:
- Valid null lost by union branch order,
argument_serializer.py:59-63. GiveconfigananyOfwith two object branches. Both have optionalname; the first declaresnameas string, the second as string-or-null. Input{"config":{"name":null}}is valid through the second branch, but the request becomes{"config":{}}. Reversing the branches preserves null. The serializer strips the value to fit an earlier branch before checking the branch that already accepts the original input. Preserve valid explicit nulls independently of branch order. - Known sibling constraint bypassed after an unresolved reference,
argument_serializer.py:19-24. An optional property declared as{"$ref":"https://example.invalid/schema.json","type":"string"}forwards an explicit null. Reversing those two keys omits it. Catching an unresolved reference currently accepts the whole schema before the known string constraint is checked. Keep the no-retrieval fallback, but still enforce known sibling constraints regardless of key order.
Both defects also reproduce in direct serializer calls. These checks used mocked MCP sessions and an empty reference registry. No remote schema was fetched. Please cover both cases alongside the existing controls.
|
Thanks for reproducing both cases. I integrated current main (
All 117 serializer/argument-presence cases pass, retaining the original 79. Of the 38 additional cases, 18 fail against the previous serializer and 20 are passing controls; all now pass. Generated-tool regressions assert the forwarded request in both fresh and persistent sessions. The full core suite passes 951 tests with 3 credential-gated MiniMax skips; repository-wide Black, Flake8 and the whitespace check pass. |
Eigenwise
left a comment
There was a problem hiding this comment.
Rechecked e06d69c. Both earlier cases are repaired: explicit null survives in either union branch order (including nested in arrays), and the unresolved-reference fallback no longer bypasses sibling constraints in either key order. Local $refs resolve against the root schema, the registry stays empty, and no network attempt is made. All 117 serializer/presence tests pass locally, and the direct dependency declarations are fine since both libraries were already transitive through mcp.
One new regression against main, reproduced through generated tools at the session.call_tool boundary in both fresh and persistent session modes:
- Malformed or exotic server schemas now abort the tool call before the existing call exception handling. Minimal case:
inputSchema{"type":"object","properties":{"value":{"allOf":null}}}with arguments{"value":{}}raisesTypeErrorfromargument_serializer.py:60; main forwards{"value":{}}. Two variants atargument_serializer.py:32: a property{"type":"exotic"}withvalue=Noneraisesjsonschema.exceptions.UnknownType, and{"enum":null}withvalue=NoneraisesTypeError. Main forwards{}for both. Six candidate exceptions versus six successful calls on main, same inputs.
Please keep the serializer best-effort: when validator construction or a subschema check fails on an advertised schema, fall back to forwarding what main would have sent instead of raising, and add regressions for these three shapes. Everything else looks ready. These checks used mocked MCP sessions, no remote schema was fetched.
|
Heads-up: #300 just merged to main with the same two |
Generated MCP tools drop explicit
Nonearguments throughexclude_none=True, including parameters whose advertised input schema allows null. Conversely, the generated Pydantic model can acceptNonefor optional fields whose advertised schema disallows it.Preserve caller presence with
exclude_unset=True, then filter object members against the original input schema. This works through local references, nested objects and arrays of objects in both fresh and persistent session modes. Array positions, zero, false and explicitly supplied defaults are preserved.For structured unions, try branches that accept the original input before attempting to omit nulls to fit another branch. A valid explicit null then survives either branch order, including when a separate common constraint requires filtering. An unresolved reference alone retains the transformer's permissive fallback, but sibling type/enum/const and composition constraints still apply regardless of keyword order. Validation uses an empty reference registry and does not retrieve remote schemas.
Integrated current
main(14cf8eb), retaining #313's dictionary-schema handling.jsonschemaandreferencing, already transitive MCP dependencies, are declared directly; no locked package versions change.Payload compatibility
Omitted optional arguments, including non-null schema defaults, are no longer sent. Servers can apply their own defaults; callers that need a default in the request must supply it explicitly. Explicit
Nonefor a nonnullable optional object member is omitted, including nested members. Genuinely nullable members retain explicit null. This behavior is documented indocs/examples/mcp_agent.md.Validation
Windows / Python 3.13, current locked dependencies:
session.call_toolarguments in both session modes. Resolvable advertised schemas are also checked with JSON Schema validation.git diff --check, pass.