Skip to content

fix: preserve explicit nulls in MCP tool arguments - #303

Open
liangzhechen655 wants to merge 7 commits into
Eigenwise:mainfrom
liangzhechen655:fix/mcp-explicit-null-arguments
Open

liangzhechen655 wants to merge 7 commits into
Eigenwise:mainfrom
liangzhechen655:fix/mcp-explicit-null-arguments

Conversation

@liangzhechen655

@liangzhechen655 liangzhechen655 commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

Generated MCP tools drop explicit None arguments through exclude_none=True, including parameters whose advertised input schema allows null. Conversely, the generated Pydantic model can accept None for 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. jsonschema and referencing, 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 None for a nonnullable optional object member is omitted, including nested members. Genuinely nullable members retain explicit null. This behavior is documented in docs/examples/mcp_agent.md.

Validation

Windows / Python 3.13, current locked dependencies:

  • All 117 serializer/argument-presence tests pass, including the original 79 cases. The 38 additional review cases cover union branch order, arrays, common constraints, unresolved local/remote references, sibling keyword order and composition restrictions. Against the previous PR serializer, 18 fail and 20 are passing controls; all pass with this follow-up.
  • Generated-tool tests assert the actual session.call_tool arguments in both session modes. Resolvable advertised schemas are also checked with JSON Schema validation.
  • Full core suite: 951 passed, 3 credential-gated MiniMax integration tests skipped; 99% total statement coverage.
  • Repository-wide Black and Flake8, plus git diff --check, pass.
  • MCP transports and sessions are mocked; no live LLM or external MCP integration run is claimed.

@Eigenwise Eigenwise left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

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 Eigenwise left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

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.

@Eigenwise

Copy link
Copy Markdown
Owner

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.

@liangzhechen655

Copy link
Copy Markdown
Contributor Author

Thanks for the follow-up examples. I integrated current main (2a26bbc) and replaced the top-level Pydantic-type nullability check with serialization against the advertised input schema. It now handles referenced enum/const restrictions and outer constraints, and recurses into referenced objects and arrays of objects. Explicit null is retained only where the relevant schema allows it; zero, false, explicitly supplied defaults and array positions are preserved.

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 Eigenwise left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

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. Give config an anyOf with two object branches. Both have optional name; the first declares name as 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.

@liangzhechen655

Copy link
Copy Markdown
Contributor Author

Thanks for reproducing both cases. I integrated current main (14cf8eb) and addressed them:

  • Union serialization now prefers branches that accept the original value before trying to omit nulls to fit a different branch. The nullable/nonnullable object example preserves name: null in either order, including inside arrays and when a separate common constraint needs filtering.
  • The unresolved-reference fallback is scoped to the $ref keyword rather than accepting the whole schema. Known sibling type/enum/const constraints and constraints inside compositions remain active in either keyword order. The registry remains empty, with no remote schema retrieval.

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 Eigenwise left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

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":{}} raises TypeError from argument_serializer.py:60; main forwards {"value":{}}. Two variants at argument_serializer.py:32: a property {"type":"exotic"} with value=None raises jsonschema.exceptions.UnknownType, and {"enum":null} with value=None raises TypeError. 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.

@Eigenwise

Copy link
Copy Markdown
Owner

Heads-up: #300 just merged to main with the same two pyproject.toml dependency lines, so please integrate main when you push the containment fix; the dependency hunk should then collapse to nothing.

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