Skip to content

Let a legacy-era tools/call answer with a CreateTaskResult - #3161

Open
maxisbey wants to merge 1 commit into
mainfrom
legacy-tasks
Open

Let a legacy-era tools/call answer with a CreateTaskResult#3161
maxisbey wants to merge 1 commit into
mainfrom
legacy-tasks

Conversation

@maxisbey

@maxisbey maxisbey commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Lets a server on a 2025-11-25 connection answer a task-augmented tools/call with a CreateTaskResult, which SEP-1686 requires and the SDK currently makes impossible.

Motivation and Context

v2 ships the request half of SEP-1686 tasks and not the response half. CallToolRequestParams.task exists, every Task* type is in mcp_types, the tasks capability fields survive the initialize sieve, and Tool.execution.taskSupport survives the tools/list sieve. But SERVER_RESULTS[("tools/call", "2025-11-25")] holds CallToolResult alone, so:

  • a handler returning a CreateTaskResult gets -32603 "Handler returned an invalid result"
  • a handler returning {"content": [...], "task": {...}} succeeds and the task key is silently dropped by the extra="ignore" sieve
  • a client receiving one raises ValidationError in send_request before its caller sees anything, so it cannot even read the taskId to cancel the task it just caused

The v1.x line has CreateTaskResult in both its ServerResultType and ClientResultType unions, so a v1.x server can express that response and a v2 server cannot. What v2 announced removing was the experimental task runtime; the protocol coverage went with it, on a revision this SDK still negotiates and still lets a server advertise capabilities.tasks for. docs/whats-new.md promises that "serving the new revision does not strand a client on the old one".

2026-07-28 removed tasks from the core protocol in favour of the io.modelcontextprotocol/tasks extension (SEP-2663), a different protocol that reuses some method names. A CreateTaskResult on a 2026 tools/call still fails, which is correct.

What changed

The result arm. AnyCallToolResult = CallToolResult | CreateTaskResult is generated into the 2025-11-25 surface package (mcp_types._v2025_11_25, private per #3191) next to the 2026 aliases, and the tools/call rows in SERVER_RESULTS use it. The 2025-11-25 schema states the augmented-result rule in prose only, leaving CreateTaskResult out of its own ServerResult union, so the alias is spelled in the generator's epilogue rather than derived. The arms have disjoint required fields (content vs task) and discriminate identically in either order.

Only the 2025-11-25 row gets it. The earlier pre-2026 rows share v2025.CallToolRequest and so already parse params.task, but that is an artifact of the shared schema era rather than a licence to answer: a client sending task to a 2024-11-05 server is off-spec, and a clean INTERNAL_ERROR serves it better than a result shape its revision has no definition for. MONOLITH_RESULTS["tools/call"] gains the arm too, so the exported parse_server_result can parse everything the surface now admits.

The handler type. The lowlevel Server's on_call_tool return union gains CreateTaskResult. Return types are covariant, so every handler that compiles today still compiles.

Task.ttl. It is required and nullable (number | null, null meaning unlimited retention), and every dump path in the SDK passes exclude_none=True. That flag cannot tell a required null from an unset optional, so it dropped ttl and produced a body that failed the very surface it had just been validated against. Models carrying such a field now take a KeepRequiredNullable base that puts it back, and only that: a field the caller removed with include/exclude, or one never set at all, stays absent.

Two live bugs of the same shape fall out of the rule. LoggingMessageNotificationParams.data is required and declared with no type, so null is a legal value. ctx.log("info", None) dropped the key, and the receiving session rejected the notification against its own schema before dispatch: the message vanished with no error to either side, at every protocol version. It now arrives, with a test.

JSONRPCError.id is the other: required and nullable per JSON-RPC 2.0, where "id": null means the request id could not be determined. _streamable_http_modern.py patched it back by hand at one call site, with a comment reading "exclude_none would otherwise drop it". That patch is deleted; the base covers every writer.

Four notes on the shape of KeepRequiredNullable:

  • The field set resolves on first dump rather than at class creation, because the generated modules defer annotations and finish with model_rebuild(); resolving early would silently see nothing for a forward-referenced field and make the base inert.
  • It is applied per model, not on MCPModel/WireModel. A wrap serializer costs per dump and pushes pydantic off its fast path for every model in the tree; on the base classes, ListToolsResult with 20 tools went from 9.0 to 49.5 us. Per model it measures the same as before (8.9 us).
  • The generator derives which classes need it from the schema, so it is not a list anyone maintains, and tests/types/test_parity.py applies the same rule to the built models across _types, jsonrpc, and both surfaces. The test is the authority: a spelling the generator's schema walk does not recognise fails the suite rather than the wire, and says so.
  • Its return is deliberately unannotated. Pydantic builds the serialization JSON schema from that signature, and annotating it (as dict[str, Any] or Any) collapses the whole model's serialization schema to an opaque object for anyone generating schemas over these types.

The tasks/* lifecycle methods are deliberately untouched. They are absent from SPEC_CLIENT_METHODS, so the runner skips both its inbound gate and its outbound sieve for them and Server.add_request_handler serves them today. Adding registry rows would add those names to that version-flattened set, which makes MethodBinding reject tasks/get outright and gates the 2026 tasks extension's own tasks/get to METHOD_NOT_FOUND. The docs describe the add_request_handler route instead, along with its sharp edges: the handler serves every negotiated version, a raised exception on a custom method reaches the client unmapped, and get_capabilities will not advertise tasks for you. The same era caveat applies to on_call_tool itself: the handler is given the version-free params model, which carries task at every version, so ctx.protocol_version is the era test and params.task is only the opt-in within it.

How Has This Been Tested?

  • tests/interaction/lowlevel/test_tools.py gains a public-API test under a new tools:call:task-augmented requirement, running over all four transports. It fails on main with ValidationError for CallToolResult.
  • tests/interaction/lowlevel/test_logging.py gains one for the null-data notification, which on main never reaches the client at all.
  • tests/types/ covers the widened rows, a ttl round trip through serialize_server_result with both null and a number, the required-nullable invariant across both surfaces and the monolith, and that the base leaves include/exclude/non-exclude_none dumps alone.
  • Driven by hand end to end against a real stdio subprocess server: task-augmented tools/calltasks/get polling → tasks/resulttasks/listtasks/cancel, with capabilities.tasks advertised and taskSupport: "optional" on the tool. The same server on a 2026-07-28 connection correctly refuses the CreateTaskResult.
  • Full suite, pyright, coverage at 100%, and the generator's --check drift guard all pass.

Breaking Changes

None. The handler union only widens, which is covariant. The one behaviour change is that a required field whose value is null now appears in an exclude_none dump instead of being dropped, which is what the schema always said should happen.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

The SDK is supplying vocabulary here, not a task runtime. There is no task store, no polling helper, and no capability enforcement, and this PR does not add any: a server author brings those, exactly as they would for any other stateful protocol feature. That is a deliberate stopping point rather than a first slice. For the same reason ClientSession.call_tool still resolves to the two core arms, so a client consuming a task drops to send_request with an explicit result type, as the docs and the new test both do.

The 2025-11-25 spec also allows a client to answer a task-augmented sampling/createMessage or elicitation/create with a CreateTaskResult; their CLIENT_RESULTS rows are still single-arm. That is the same defect in the same table, left out to keep this to one direction, and the docs say so rather than claiming the whole vocabulary works.

Two pre-existing behaviours surfaced while testing and are worth separate looks. Server.add_request_handler has no protocol_versions parameter, so a handler for a method whose meaning differs across revisions cannot be scoped (MethodBinding can). And an exception escaping a custom-method handler reaches the client as an error with code 0 carrying the exception text, which is neither a valid JSON-RPC code nor safe to echo. The docs added here warn about both rather than working around them.

AI Disclaimer

@github-actions

github-actions Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

📚 Documentation preview

Preview https://pr-3161.mcp-python-docs.pages.dev
Deployment https://bfb66068.mcp-python-docs.pages.dev
Commit d6f6c8c
Triggered by @maxisbey
Updated 2026-07-27 23:23:40 UTC

@maxisbey
maxisbey marked this pull request as ready for review July 24, 2026 15:04

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

All reported issues were addressed across 17 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread src/mcp-types/mcp_types/_wire_base.py Outdated

@claude claude Bot 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.

Beyond the inline findings, the KeepRequiredNullable wrap serializer was also examined for resurrecting never-set fields (violating exclude_unset) — ruled out as a live issue: a validated instance always has its required fields set, no SDK dump path passes exclude_unset, and the only reach is unvalidated model_construct, where the restored null still matches the schema. I reproduced that corner on this branch to confirm the scope.

Extended reasoning...

This run's finder agents raised (and verifiers refuted) a claim that KeepRequiredNullable in src/mcp-types/mcp_types/_wire_base.py violates the exclude_unset contract by restoring fields that were never set. I re-checked this directly against the branch: the serializer keys off info.exclude_none only and does setdefault when getattr(self, name, None) is None, so a field left unset via model_construct under exclude_none + exclude_unset does reappear as null. The refutation nonetheless holds as not-a-real-bug: required fields cannot be unset on any validated instance, the SDK's own dump paths (serialize_server_result, the transports) never pass exclude_unset, and the resurrected null is exactly what the schema requires for these required-nullable fields, so no invalid wire body can result. The inline nits (docs overstating the era gate, the requirement-manifest double-entry, and the un-widened public ServerResult union) stand on their own; the serializer and result-arm changes are otherwise the parts deserving the human reviewer's closest look, and the era-scoping of the widened row to 2025-11-25 matches the code.

Comment thread docs/advanced/low-level-server.md Outdated
Comment thread tests/interaction/_requirements.py
Comment thread src/mcp-types/mcp_types/methods.py

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

All reported issues were addressed across 6 files (changes from recent commits).

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread docs/migration.md

@claude claude Bot 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.

Beyond the inline nit on the model-level __all__ include/exclude wildcard, this run also examined and ruled out two candidates: the KeepRequiredNullable docstring's claim that a never-set field stays absent out of an exclude_none dump (behaviour matches the docstring), and the newly wire-live Task.ttl rejecting spec-legal non-integer values via the schema.ts number-as-integer rendering (not reproducible on this branch).

Extended reasoning...

Bugs were found this run (one nit-level inline comment on the __all__ wildcard handling in KeepRequiredNullable), so per policy no approval or standalone deferral is posted — the inline comment already signals what needs attention. This note only records what else was examined and refuted this run, since no prior run of mine left such a note: the docstring-accuracy candidate for KeepRequiredNullable and the Task.ttl integer-vs-number candidate were both investigated by verifier agents and concluded not to be real issues. My three findings from the previous run were all addressed in 269c219 (two fixed, one declined with reasoning that holds), so nothing from that round remains open.

Comment on lines +62 to +87
# only the argument would restore the one key under a spelling the rest of the dump did
# not use.
by_alias = info.by_alias or type(self).model_config.get("serialize_by_alias", False)
for name, alias in _nullable_required_fields(type(self)):
if getattr(self, name, None) is not None:
continue
if info.include is not None and name not in info.include:
continue
if _is_excluded(name, info.exclude):
continue
data.setdefault(alias if by_alias else name, None)
return data


def _is_excluded(name: str, exclude: Any) -> bool:
"""Whether `exclude` drops `name` outright, as opposed to selecting within it.

A mapping entry carrying anything other than `True`/`...` descends into the field, so
pydantic keeps the field itself and its null still has to go back.
"""
if exclude is None:
return False
if isinstance(exclude, Mapping):
marker: Any = cast("Mapping[Any, Any]", exclude).get(name)
return marker is True or marker is Ellipsis
return name in cast("Container[Any]", exclude)

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.

🟡 KeepRequiredNullable's include/exclude checks look up only the field's own key, but pydantic also honors a model-level __all__ wildcard in both mappings: include={'__all__': True} + exclude_none=True drops a required null (e.g. Task.ttl), producing exactly the schema-invalid body this base exists to prevent, while exclude={'__all__': True} + exclude_none=True spuriously restores the excluded field as {'ttl': None}. Fix by consulting the wildcard in both checks: treat name as excluded when exclude.get(name) or exclude.get('__all__') is True/Ellipsis, and as included when include is None, name in include, or '__all__' in include.

Extended reasoning...

What the bug is. Pydantic's include/exclude mappings honor a special __all__ key at model level: on a plain BaseModel, model_dump(include={'__all__': True}) keeps every field and model_dump(exclude={'__all__': True}) returns {} (verified on this branch). KeepRequiredNullable._keep_required_nullable (src/mcp-types/mcp_types/_wire_base.py:68) and _is_excluded (lines 76-87) only look up the field's own key — name not in info.include and exclude.get(name) — so a model-level wildcard is invisible to both checks, and the base makes the wrong call in both directions.

Divergence 1 — the required null is dropped. Step-by-step, reproduced on this branch:

  1. task = Task(task_id='t1', status='working', created_at='x', last_updated_at='y', ttl=None)
  2. task.model_dump(by_alias=True, exclude_none=True, include={'__all__': True}){'taskId': 't1', 'status': 'working', 'createdAt': 'x', 'lastUpdatedAt': 'y'}ttl is gone.
  3. Pydantic's include filter kept ttl (__all__ includes every field); only exclude_none removed the null. Per the class's own contract ("a field the caller filtered out with include/exclude stays absent, because there exclude_none is not why it went" — and its converse), the null must be restored.
  4. But line 68's check sees 'ttl' not in {'__all__'} and skips restoration, yielding a body that fails the 2025-11-25 surface (Task.ttl is required). A caller spelling "include everything" — semantically identical to passing no include at all — silently gets the pre-PR broken behavior back. Same failure applies to LoggingMessageNotificationParams.data and JSONRPCError.id.

Divergence 2 — an excluded field is spuriously restored. task.model_dump(by_alias=True, exclude_none=True, exclude={'__all__': True}){'ttl': None} (same with exclude={'__all__': ...}). Without exclude_none the same dump is {}, so pydantic excluded every field via the wildcard — yet _is_excluded('ttl', {'__all__': True}) does exclude.get('ttl')None → not True/EllipsisFalse, and the base re-adds ttl. This directly violates the docstring invariant that tests/types/test_parity.py::test_keep_required_nullable_only_restores_what_exclude_none_removed pins for every other spelling of exclusion.

Why the 269c219 fix missed it. That commit (responding to cubic's P2 about descending-map markers) added the True/Ellipsis marker check to _is_excluded, and the author reported checking eleven shapes including an "__all__ map" — but the __all__ shape actually tested was the nested form (exclude={'tasks': {'__all__': {'ttl'}}}), where pydantic narrows the filter to {'ttl'} before it reaches the model's SerializationInfo. The model-level wildcard path is genuinely untested and unhandled.

Impact and fix. No SDK-internal dump path passes include/exclude at all (serialize_server_result and the transport writers use only by_alias/mode/exclude_none), so nothing in any SDK flow breaks on merge — the trigger requires an external consumer of the public mcp-types package combining exclude_none=True with the model-level __all__ spelling on an exported model (Task, GetTaskResult, CancelTaskResult, LoggingMessageNotificationParams, JSONRPCError). That is legal public API but an unusual spelling, so this is a polish item rather than a blocker. The fix is small and local: in _keep_required_nullable, treat name as included when include is None, name in include, or '__all__' in include (pydantic keeps a field for any marker under its key, including False — verified include={'data': False} keeps data on a plain model); in _is_excluded, also check exclude.get('__all__') for a True/Ellipsis marker. A test pinning include={'__all__': True} and exclude={'__all__': True} alongside the existing spellings in test_keep_required_nullable_only_restores_what_exclude_none_removed would close the gap the nested-form test left open.

SEP-1686 makes a task-augmented `tools/call` answer with a `CreateTaskResult`,
but `SERVER_RESULTS[("tools/call", "2025-11-25")]` held `CallToolResult` alone.
The request half shipped and the response half did not, so a server returning
a task got `-32603 "Handler returned an invalid result"`, a server returning
both shapes had `task` silently sieved away, and a client receiving one raised
`ValidationError` before its caller saw anything. The v1.x line could express
that response; v2 cannot, on a revision the SDK still negotiates and still lets
a server advertise `capabilities.tasks` for.

Give the row its second arm as a generated `AnyCallToolResult`, alongside the
2026 aliases, and widen the lowlevel `Server`'s `on_call_tool` to match. Return
types are covariant, so a handler that only returns `CallToolResult` is
unaffected. Only the 2025-11-25 row gets the arm. The earlier handshake
revisions share `_v2025.CallToolRequest` and so already parse `params.task`, but
that is an artifact of the shared schema era, not a licence to answer: they
predate SEP-1686, and a clean INTERNAL_ERROR serves an off-spec client better
than a result shape its revision never defined. `MONOLITH_RESULTS["tools/call"]`
gains the arm too, so `parse_server_result` covers everything the surface admits.

`Task.ttl` is required and nullable ("null for unlimited"), and every dump path
passes `exclude_none=True`, which cannot tell a required null from an unset
optional and dropped it, leaving a body that fails the surface it was just
validated against. Rather than patch the 27 dump sites, models carrying such a
field now take a `KeepRequiredNullable` base that puts it back, and only that:
a field the caller filtered out with `include`/`exclude`, or one that was never
set, stays absent. The generator derives which classes need it from the schema
and a test asserts the same rule against the built models, so a nullable-required
field in a future revision is covered without anyone remembering. The field set
resolves on first dump rather than at class creation, because the generated
modules defer annotations and finish with `model_rebuild()`, so resolving early
would silently see nothing for a forward-referenced field.

That rule also reaches two live bugs of the same shape.
`LoggingMessageNotificationParams.data` is required and untyped, so
`ctx.log("info", None)` dropped the key and the receiving session rejected the
notification before dispatch: the message vanished with no error to either side,
at every protocol version. And `JSONRPCError.id` is required-and-nullable per
JSON-RPC 2.0, which the streamable-HTTP writer worked around by hand at one call
site; that patch is deleted and the base covers every writer.

The base is applied per model rather than to `MCPModel`/`WireModel`: a wrap
serializer costs per dump and pushes pydantic off its fast path for every model
in the tree, which measured 5x on a 20-tool `tools/list`. Its return is
deliberately unannotated, because an annotation there collapses the model's
serialization JSON schema to an opaque object.

The `tasks/*` lifecycle methods are left where they already work. They are
absent from `SPEC_CLIENT_METHODS`, so `Server.add_request_handler` serves them
today; adding registry rows would put those names into that version-flattened
set, which would make it illegal for an extension to bind `tasks/get` and would
gate the 2026 tasks extension's own calls to METHOD_NOT_FOUND. 2026-07-28 keeps
rejecting a `CreateTaskResult` on `tools/call`, which is correct: tasks left the
core protocol there.

@claude claude Bot 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.

Beyond the inline finding on the unpatched integer rendering of the task ttl fields, this run also examined whether KeepRequiredNullable could inject a never-set field into exclude_unset dumps — ruled out: the wrap serializer only intervenes when exclude_none is set, and a required nullable field is always set on a validated instance, so an exclude_unset dump is passed through untouched.

Extended reasoning...

The one bug found this run (Task.ttl/pollInterval/TaskMetadata.ttl rendered as integer in the vendored 2025-11-25 schema, made live on the send_request validation path by AnyCallToolResult) is posted inline and is reason enough for the author to revisit before merge, so no approval. This note only records the additional candidate that finder agents raised and verifiers refuted — the claim that KeepRequiredNullable would fabricate a key in exclude_unset dumps — so a later review pass has a record that the exclude_unset interaction was already examined. It is informational, not a guarantee: the serializer's early return on 'not info.exclude_none' and the impossibility of an unset required field on a validated model are why it holds.

("resources/templates/list", "2025-11-25"): v2025.ListResourceTemplatesResult,
("resources/unsubscribe", "2025-11-25"): v2025.EmptyResult,
("tools/call", "2025-11-25"): v2025.CallToolResult,
("tools/call", "2025-11-25"): v2025.AnyCallToolResult,

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.

🔴 The vendored 2025-11-25 schema misrenders Task.ttl (number | null upstream), Task.pollInterval (number), and TaskMetadata.ttl (number) as integer — the exact ts→json defect SCHEMA_PATCHES exists to correct — and this PR makes all three fields live via AnyCallToolResult without patching them, so a spec-legal fractional value (e.g. ttl: 1500.5 or pollInterval: 250.5 from a non-SDK server) raises ValidationError in ClientSession.send_request before the caller can read the taskId — recreating the strandedness this PR's motivation says it fixes. Fix is the established mechanical pattern: add the three sites to SCHEMA_PATCHES['2025-11-25'] in scripts/gen_surface_types.py, regenerate, and widen the monolith Task.ttl/Task.poll_interval/TaskMetadata.ttl to int | float | None (matching the existing NumberSchema treatment).

Extended reasoning...

What the bug is. The upstream 2025-11-25 schema.ts declares TaskMetadata.ttl?: number, Task.ttl: number | null, and Task.pollInterval?: number — all TypeScript number, no integer annotation. The vendored schema/2025-11-25.json renders all three as integer (Task.ttl: ["integer","null"], Task.pollInterval: integer, TaskMetadata.ttl: integer), while e.g. ProgressNotificationParams.progress in the same file correctly renders as number — so these are misrendered sites of the selective ts→json defect that SCHEMA_PATCHES in scripts/gen_surface_types.py documents and exists to correct ('renders TypeScript number as JSON Schema integer at these sites; patch the JSON before codegen so floats validate', with a TODO to drop once upstream fixes the renderer). The PR patches none of the three, so the generated v2025.Task.ttl/poll_interval and TaskMetadata.ttl are int-typed, and the hand-written monolith mirrors them (_types.py Task.ttl/Task.poll_interval/TaskMetadata.ttl).\n\nWhy this PR makes it live. Before this PR the defect was latent: no SERVER_RESULTS/MONOLITH_RESULTS row admitted a Task, so a CreateTaskResult body never validated anywhere. This PR adds ("tools/call", "2025-11-25"): v2025.AnyCallToolResult (methods.py:289) and the CreateTaskResult arm to MONOLITH_RESULTS["tools/call"], and docs/migration.md instructs clients to consume the result via ClientSession.send_request. That path calls _methods.validate_server_result on the raw body before the caller's TypeAdapter runs.\n\nStep-by-step proof (all reproduced on this branch at d6f6c8c):\n1. A non-SDK 2025-11-25 server answers a task-augmented tools/call with {"task": {..., "ttl": 1500.5}} or {..., "ttl": null, "pollInterval": 250.5} — legal per schema.ts, where both fields are number (milliseconds).\n2. ClientSession.send_request calls validate_server_result("tools/call", "2025-11-25", raw), which validates against v2025.AnyCallToolResult.\n3. The CreateTaskResult arm rejects the fractional value (int_from_float), the union falls through, and ValidationError propagates out of send_request.\n4. The caller never sees the body, so it cannot read the taskId to poll or cancel the task it just caused — verbatim the strandedness the PR's motivation section describes fixing ('it cannot even read the taskId to cancel the task it just caused'). Integral floats (60000.0) coerce fine under pydantic lax mode; only genuinely fractional values fail, which is exactly why the PR's own tests (parametrized over [None, 60_000]) never hit it.\n\nOther affected paths. Server side: a handler returning a dict body with a fractional ttl (natural when computing remaining retention from time.monotonic()/datetime deltas, which yield floats) fails serialize_server_result, which the runner maps to the opaque -32603 "Handler returned an invalid result". Inbound request side: validate_client_request("tools/call", "2025-11-25", {"name": "render", "task": {"ttl": 60000.5}}) raises → INVALID_PARAMS for a schema.ts-legal request (this arm predates the PR, but it shares the one root cause and one fix, and this PR is what documents and ships the feature).\n\nWhy nothing else prevents it. The surface validators are deliberately strict against the vendored schema, so wherever the vendored JSON is wrong, spec-conformant wire data is rejected — which is precisely why SCHEMA_PATCHES exists. The repo's own precedent treats this exact class as a defect to patch, not intended strictness: NumberSchema.default/maximum/minimum are already patched to ["integer","number"] 'so codegen emits int | float and pydantic smart-union preserves ints on round-trip', and the ElicitResult/JSONValue null arms get the same superset-leniency treatment.\n\nHow to fix. Add three entries to SCHEMA_PATCHES["2025-11-25"] in scripts/gen_surface_types.py("$defs/Task/properties/ttl/type", ["integer","null"], ["integer","number","null"]), ("$defs/Task/properties/pollInterval/type", "integer", ["integer","number"]), ("$defs/TaskMetadata/properties/ttl/type", "integer", ["integer","number"]) — regenerate, and widen the monolith Task.ttl/Task.poll_interval/TaskMetadata.ttl in src/mcp-types/mcp_types/_types.py to int | float | None, exactly the treatment the NumberSchema fields already receive. A [None, 60_000, 1500.5] parametrization of test_serialize_server_result_keeps_a_required_nullable_task_ttl would pin it.

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.

1 participant