Let a legacy-era tools/call answer with a CreateTaskResult - #3161
Let a legacy-era tools/call answer with a CreateTaskResult#3161maxisbey wants to merge 1 commit into
Conversation
📚 Documentation preview
|
There was a problem hiding this comment.
All reported issues were addressed across 17 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
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.
| # 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) |
There was a problem hiding this comment.
🟡 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:
task = Task(task_id='t1', status='working', created_at='x', last_updated_at='y', ttl=None)task.model_dump(by_alias=True, exclude_none=True, include={'__all__': True})→{'taskId': 't1', 'status': 'working', 'createdAt': 'x', 'lastUpdatedAt': 'y'}—ttlis gone.- Pydantic's include filter kept
ttl(__all__includes every field); onlyexclude_noneremoved 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. - But line 68's check sees
'ttl' not in {'__all__'}and skips restoration, yielding a body that fails the 2025-11-25 surface (Task.ttlis required). A caller spelling "include everything" — semantically identical to passing noincludeat all — silently gets the pre-PR broken behavior back. Same failure applies toLoggingMessageNotificationParams.dataandJSONRPCError.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/Ellipsis → False, 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.
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
🔴 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.
Lets a server on a 2025-11-25 connection answer a task-augmented
tools/callwith aCreateTaskResult, 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.taskexists, everyTask*type is inmcp_types, thetaskscapability fields survive the initialize sieve, andTool.execution.taskSupportsurvives thetools/listsieve. ButSERVER_RESULTS[("tools/call", "2025-11-25")]holdsCallToolResultalone, so:CreateTaskResultgets-32603 "Handler returned an invalid result"{"content": [...], "task": {...}}succeeds and thetaskkey is silently dropped by theextra="ignore"sieveValidationErrorinsend_requestbefore its caller sees anything, so it cannot even read thetaskIdto cancel the task it just causedThe v1.x line has
CreateTaskResultin both itsServerResultTypeandClientResultTypeunions, 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 advertisecapabilities.tasksfor.docs/whats-new.mdpromises 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/tasksextension (SEP-2663), a different protocol that reuses some method names. ACreateTaskResulton a 2026tools/callstill fails, which is correct.What changed
The result arm.
AnyCallToolResult = CallToolResult | CreateTaskResultis generated into the 2025-11-25 surface package (mcp_types._v2025_11_25, private per #3191) next to the 2026 aliases, and thetools/callrows inSERVER_RESULTSuse it. The 2025-11-25 schema states the augmented-result rule in prose only, leavingCreateTaskResultout of its ownServerResultunion, so the alias is spelled in the generator's epilogue rather than derived. The arms have disjoint required fields (contentvstask) and discriminate identically in either order.Only the 2025-11-25 row gets it. The earlier pre-2026 rows share
v2025.CallToolRequestand so already parseparams.task, but that is an artifact of the shared schema era rather than a licence to answer: a client sendingtaskto 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 exportedparse_server_resultcan parse everything the surface now admits.The handler type. The lowlevel
Server'son_call_toolreturn union gainsCreateTaskResult. 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 passesexclude_none=True. That flag cannot tell a required null from an unset optional, so it droppedttland produced a body that failed the very surface it had just been validated against. Models carrying such a field now take aKeepRequiredNullablebase that puts it back, and only that: a field the caller removed withinclude/exclude, or one never set at all, stays absent.Two live bugs of the same shape fall out of the rule.
LoggingMessageNotificationParams.datais 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.idis the other: required and nullable per JSON-RPC 2.0, where"id": nullmeans the request id could not be determined._streamable_http_modern.pypatched it back by hand at one call site, with a comment reading "exclude_nonewould otherwise drop it". That patch is deleted; the base covers every writer.Four notes on the shape of
KeepRequiredNullable:model_rebuild(); resolving early would silently see nothing for a forward-referenced field and make the base inert.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,ListToolsResultwith 20 tools went from 9.0 to 49.5 us. Per model it measures the same as before (8.9 us).tests/types/test_parity.pyapplies 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.dict[str, Any]orAny) 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 fromSPEC_CLIENT_METHODS, so the runner skips both its inbound gate and its outbound sieve for them andServer.add_request_handlerserves them today. Adding registry rows would add those names to that version-flattened set, which makesMethodBindingrejecttasks/getoutright and gates the 2026 tasks extension's owntasks/gettoMETHOD_NOT_FOUND. The docs describe theadd_request_handlerroute instead, along with its sharp edges: the handler serves every negotiated version, a raised exception on a custom method reaches the client unmapped, andget_capabilitieswill not advertisetasksfor you. The same era caveat applies toon_call_toolitself: the handler is given the version-free params model, which carriestaskat every version, soctx.protocol_versionis the era test andparams.taskis only the opt-in within it.How Has This Been Tested?
tests/interaction/lowlevel/test_tools.pygains a public-API test under a newtools:call:task-augmentedrequirement, running over all four transports. It fails onmainwithValidationError for CallToolResult.tests/interaction/lowlevel/test_logging.pygains one for the null-datanotification, which onmainnever reaches the client at all.tests/types/covers the widened rows, attlround trip throughserialize_server_resultwith both null and a number, the required-nullable invariant across both surfaces and the monolith, and that the base leavesinclude/exclude/non-exclude_nonedumps alone.tools/call→tasks/getpolling →tasks/result→tasks/list→tasks/cancel, withcapabilities.tasksadvertised andtaskSupport: "optional"on the tool. The same server on a 2026-07-28 connection correctly refuses theCreateTaskResult.pyright, coverage at 100%, and the generator's--checkdrift 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_nonedump instead of being dropped, which is what the schema always said should happen.Types of changes
Checklist
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_toolstill resolves to the two core arms, so a client consuming a task drops tosend_requestwith 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/createMessageorelicitation/createwith aCreateTaskResult; theirCLIENT_RESULTSrows 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_handlerhas noprotocol_versionsparameter, so a handler for a method whose meaning differs across revisions cannot be scoped (MethodBindingcan). And an exception escaping a custom-method handler reaches the client as an error with code0carrying 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