Skip to content

Commit eb84253

Browse files
committed
Scope the CreateTaskResult arm to the revision that defines it
Review follow-up, four corrections. The arm goes back to `2025-11-25` alone. It was widened to all four pre-2026 rows on the reasoning that they share `v2025.CallToolRequest` and so already parse `params.task`, which made refusing to answer at 2025-06-18 look like the same incoherence one revision along. That was the wrong conclusion: the request side accepting `task` at revisions that predate SEP-1686 is an artifact of the shared schema era, and matching it on the response side propagates the artifact instead of containing it. A client that sends `task` to a 2024-11-05 server is already off-spec, and a clean INTERNAL_ERROR serves it better than a result shape its revision cannot parse. The rows now say what each revision defines, which is also what the requirement's `added_in` window claims. `params.task` is not an era test, and the migration guide said it was. The handler receives the version-free params model, which carries `task` at every version, so a client can set it on a 2026-07-28 connection and drive a handler straight into a result that revision rejects. The recipe now gates on `ctx.protocol_version`. The field cache moves from a `ClassVar` to a module-level cached function. The `ClassVar` was inherited by every subclass, so correctness rested on the serializer remembering to read `cls.__dict__` rather than the attribute; keying a `cache` on the class removes the hazard along with twenty lines. The restored key now also honours `serialize_by_alias`, not just the `by_alias` argument, so a config-driven dump cannot come back mixed-convention. `tests/types/test_parity.py` now shares `admits_none` with the runtime instead of inlining a copy that had already drifted, and its docstring no longer calls itself the authority: sharing the predicate makes it a consistency check between the schema-side rule and the annotation-side one, which is what it actually is. The generator's base patcher matches whatever bases codegen chose, so a `$def` that composes through `allOf` no longer aborts regeneration.
1 parent 5b4fcec commit eb84253

6 files changed

Lines changed: 68 additions & 69 deletions

File tree

docs/migration.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1663,7 +1663,7 @@ The task types stay, so a server can still serve Tasks ([SEP-1686](https://githu
16631663
async def call_tool(
16641664
ctx: ServerRequestContext, params: CallToolRequestParams
16651665
) -> CallToolResult | CreateTaskResult:
1666-
if params.task is None:
1666+
if params.task is None or ctx.protocol_version != "2025-11-25":
16671667
return CallToolResult(content=[TextContent(text=run_now())])
16681668
return CreateTaskResult(task=await store.submit(params))
16691669

@@ -1678,7 +1678,7 @@ server.add_request_handler("tasks/get", GetTaskRequestParams, get_task)
16781678

16791679
Two things to know about handlers registered this way. They serve every negotiated version, so a server that also answers 2026-era clients should check `ctx.protocol_version` and reject anything outside 2025-11-25; the method names collide with the 2026 tasks extension but the payloads are not compatible. And their results are not validated against a per-version surface, so raise `MCPError` for the failure cases rather than letting an exception escape: an unhandled one reaches the client as an unmapped error carrying the exception text.
16801680

1681-
The same era check belongs in `on_call_tool`. Its return type admits a `CreateTaskResult` at every version because the signature has no version to key on, but 2026-07-28 dropped tasks from the core protocol and rejects the shape, turning it into an opaque internal error. Returning one only when `params.task` is set keeps that unreachable for a well-behaved client, since a client that never learned the field never sets it.
1681+
The same era check belongs in `on_call_tool`, and `params.task` is not a substitute for it. The handler receives the version-free params model, which carries `task` at every version, so a client can set the field on a 2026-07-28 connection and reach a handler that then answers with a `CreateTaskResult`, which that revision rejects as an opaque internal error. Gate on `ctx.protocol_version == "2025-11-25"` and treat `params.task` as the opt-in within that era, not as the era test.
16821682

16831683
`Server.get_capabilities` does not derive a `tasks` capability from the registered handlers, and a spec-compliant client will not augment a request until it sees one, so build the advertisement explicitly:
16841684

scripts/gen_surface_types.py

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -255,21 +255,25 @@ def _admits_null(prop: dict[str, Any]) -> bool:
255255

256256

257257
def keep_required_nullable(source: str, classes: frozenset[str]) -> str:
258-
"""Give each of `classes` `KeepRequiredNullable` as a second base."""
258+
"""Append `KeepRequiredNullable` to each of `classes`'s base list.
259+
260+
Matches whatever bases codegen chose, since a `$def` that composes through `allOf` is
261+
emitted with its composed bases rather than a bare `WireModel`.
262+
"""
259263
for name in sorted(classes):
260264
source, count = re.subn(
261-
rf"^class {name}\(WireModel\):$",
262-
f"class {name}(WireModel, KeepRequiredNullable):",
265+
rf"^class {name}\((?P<bases>[^)]+)\):$",
266+
rf"class {name}(\g<bases>, KeepRequiredNullable):",
263267
source,
264268
flags=re.MULTILINE,
265269
)
266270
if count != 1:
267-
raise SystemExit(f"expected one `class {name}(WireModel)` to patch, found {count}")
271+
raise SystemExit(f"expected one `class {name}(...)` to patch, found {count}")
268272
if classes:
269-
source = source.replace(
270-
"from mcp_types._wire_base import WireModel",
271-
"from mcp_types._wire_base import KeepRequiredNullable, WireModel",
272-
)
273+
import_line = "from mcp_types._wire_base import WireModel"
274+
if import_line not in source:
275+
raise SystemExit(f"cannot import KeepRequiredNullable: {import_line!r} not found")
276+
source = source.replace(import_line, "from mcp_types._wire_base import KeepRequiredNullable, WireModel")
273277
return source
274278

275279

Lines changed: 38 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -1,54 +1,54 @@
11
"""Shared pydantic bases for the generated `mcp_types.v*` packages and the monolith."""
22

3-
from typing import Any, ClassVar, Final, get_args
3+
from functools import cache
4+
from typing import Any, get_args
45

56
from pydantic import BaseModel, ConfigDict, SerializationInfo, SerializerFunctionWrapHandler, model_serializer
67

7-
_UNSET: Final[object] = object()
8-
"""Tells a field explicitly set to `None` apart from one that was never set."""
9-
108

119
class WireModel(BaseModel):
1210
"""Base for generated wire models: enables `populate_by_name`; subclasses set `extra` themselves."""
1311

1412
model_config = ConfigDict(populate_by_name=True)
1513

1614

15+
def admits_none(annotation: Any) -> bool:
16+
"""Whether a field's annotation accepts `None`, including a bare `Any`."""
17+
return annotation is Any or annotation is None or type(None) in get_args(annotation)
18+
19+
20+
@cache
21+
def _nullable_required_fields(model: type[BaseModel]) -> tuple[tuple[str, str], ...]:
22+
"""The `(attribute, wire alias)` pairs `exclude_none` must not drop from `model`.
23+
24+
Resolved on first dump rather than at class creation: the generated modules use
25+
`from __future__ import annotations` and finish with `model_rebuild()`, so a forward
26+
reference is still a string while the class body runs and would resolve to nothing.
27+
Keyed on the concrete class, since subclasses add fields (`GetTaskResult` is `Result`
28+
plus `Task`).
29+
"""
30+
return tuple(
31+
(name, field.serialization_alias or field.alias or name)
32+
for name, field in model.model_fields.items()
33+
if field.is_required() and admits_none(field.annotation)
34+
)
35+
36+
1737
class KeepRequiredNullable(BaseModel):
1838
"""Base for models carrying a required nullable field, e.g. `Task.ttl` (`number | null`).
1939
2040
Every dump path passes `exclude_none=True` to omit unset optionals, but that cannot tell an
2141
unset optional from a required field whose value is legitimately null, so it drops both and
2242
leaves a body that fails the schema it was just validated against. This puts the required
23-
ones back, and only those: a field the caller filtered out with `include`/`exclude`, or one
24-
that was never set at all, stays absent.
43+
ones back, and only those: a field the caller filtered out with `include`/`exclude` stays
44+
absent, because there `exclude_none` is not why it went.
2545
26-
Mixed in only where it is needed, since a wrap serializer costs per dump:
27-
`scripts/gen_surface_types.py` derives the surface classes from the schema and the monolith
28-
counterparts take it by hand, with `tests/types/test_parity.py` checking the set is complete.
46+
Mixed in only where it is needed, since a wrap serializer costs per dump and pydantic walks
47+
one per model in the tree: `scripts/gen_surface_types.py` derives the surface classes from
48+
the schema and the monolith counterparts take it by hand, with `tests/types/test_parity.py`
49+
checking the resulting set against every built model.
2950
"""
3051

31-
_nullable_required_fields: ClassVar[tuple[tuple[str, str], ...] | None] = None
32-
"""`(attribute, wire alias)` per required nullable field; resolved on first dump, then cached."""
33-
34-
@classmethod
35-
def _resolve_nullable_required(cls) -> tuple[tuple[str, str], ...]:
36-
"""Find the fields `exclude_none` must not drop, once per concrete class.
37-
38-
Deferred to first use rather than `__pydantic_init_subclass__`: the generated modules
39-
use `from __future__ import annotations` and finish with `model_rebuild()`, so a forward
40-
reference is still a string at class-creation time and would resolve to nothing. Each
41-
class resolves its own set, since subclasses add fields (`GetTaskResult` is `Result`
42-
plus `Task`).
43-
"""
44-
resolved = tuple(
45-
(name, field.serialization_alias or field.alias or name)
46-
for name, field in cls.model_fields.items()
47-
if field.is_required() and _admits_none(field.annotation)
48-
)
49-
cls._nullable_required_fields = resolved
50-
return resolved
51-
5252
@model_serializer(mode="wrap")
5353
def _keep_required_nullable(self, handler: SerializerFunctionWrapHandler, info: SerializationInfo):
5454
# The return is deliberately unannotated: pydantic builds the serialization JSON schema
@@ -57,25 +57,16 @@ def _keep_required_nullable(self, handler: SerializerFunctionWrapHandler, info:
5757
data = handler(self)
5858
if not info.exclude_none:
5959
return data
60-
# `__dict__`, not attribute lookup: a subclass must resolve its own set rather than
61-
# inherit whichever ancestor happened to be dumped first.
62-
cls = type(self)
63-
fields = cls.__dict__.get("_nullable_required_fields")
64-
if fields is None:
65-
fields = cls._resolve_nullable_required()
66-
for name, alias in fields:
67-
if self.__dict__.get(name, _UNSET) is not None:
60+
# `serialize_by_alias` is the config-level spelling of the `by_alias` argument; reading
61+
# only the argument would restore the one key under a spelling the rest of the dump did
62+
# not use.
63+
by_alias = info.by_alias or type(self).model_config.get("serialize_by_alias", False)
64+
for name, alias in _nullable_required_fields(type(self)):
65+
if getattr(self, name, None) is not None:
6866
continue
6967
if (info.include is not None and name not in info.include) or (
7068
info.exclude is not None and name in info.exclude
7169
):
72-
continue # the caller filtered this field out; exclude_none is not why it is gone
73-
data.setdefault(alias if info.by_alias else name, None)
70+
continue
71+
data.setdefault(alias if by_alias else name, None)
7472
return data
75-
76-
77-
def _admits_none(annotation: Any) -> bool:
78-
"""Whether a field's annotation accepts `None`, including a bare `Any`."""
79-
if annotation is Any or annotation is None:
80-
return True
81-
return type(None) in get_args(annotation)

src/mcp-types/mcp_types/methods.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -244,7 +244,7 @@
244244
("resources/subscribe", "2024-11-05"): v2025.EmptyResult,
245245
("resources/templates/list", "2024-11-05"): v2025.ListResourceTemplatesResult,
246246
("resources/unsubscribe", "2024-11-05"): v2025.EmptyResult,
247-
("tools/call", "2024-11-05"): v2025.AnyCallToolResult,
247+
("tools/call", "2024-11-05"): v2025.CallToolResult,
248248
("tools/list", "2024-11-05"): v2025.ListToolsResult,
249249
# 2025-03-26
250250
("completion/complete", "2025-03-26"): v2025.CompleteResult,
@@ -258,7 +258,7 @@
258258
("resources/subscribe", "2025-03-26"): v2025.EmptyResult,
259259
("resources/templates/list", "2025-03-26"): v2025.ListResourceTemplatesResult,
260260
("resources/unsubscribe", "2025-03-26"): v2025.EmptyResult,
261-
("tools/call", "2025-03-26"): v2025.AnyCallToolResult,
261+
("tools/call", "2025-03-26"): v2025.CallToolResult,
262262
("tools/list", "2025-03-26"): v2025.ListToolsResult,
263263
# 2025-06-18
264264
("completion/complete", "2025-06-18"): v2025.CompleteResult,
@@ -272,7 +272,7 @@
272272
("resources/subscribe", "2025-06-18"): v2025.EmptyResult,
273273
("resources/templates/list", "2025-06-18"): v2025.ListResourceTemplatesResult,
274274
("resources/unsubscribe", "2025-06-18"): v2025.EmptyResult,
275-
("tools/call", "2025-06-18"): v2025.AnyCallToolResult,
275+
("tools/call", "2025-06-18"): v2025.CallToolResult,
276276
("tools/list", "2025-06-18"): v2025.ListToolsResult,
277277
# 2025-11-25
278278
("completion/complete", "2025-11-25"): v2025.CompleteResult,

tests/types/test_methods.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -220,7 +220,7 @@
220220
("resources/subscribe", "2024-11-05"): v2025.EmptyResult,
221221
("resources/templates/list", "2024-11-05"): v2025.ListResourceTemplatesResult,
222222
("resources/unsubscribe", "2024-11-05"): v2025.EmptyResult,
223-
("tools/call", "2024-11-05"): (v2025.CallToolResult, v2025.CreateTaskResult),
223+
("tools/call", "2024-11-05"): v2025.CallToolResult,
224224
("tools/list", "2024-11-05"): v2025.ListToolsResult,
225225
("completion/complete", "2025-03-26"): v2025.CompleteResult,
226226
("initialize", "2025-03-26"): v2025.InitializeResult,
@@ -233,7 +233,7 @@
233233
("resources/subscribe", "2025-03-26"): v2025.EmptyResult,
234234
("resources/templates/list", "2025-03-26"): v2025.ListResourceTemplatesResult,
235235
("resources/unsubscribe", "2025-03-26"): v2025.EmptyResult,
236-
("tools/call", "2025-03-26"): (v2025.CallToolResult, v2025.CreateTaskResult),
236+
("tools/call", "2025-03-26"): v2025.CallToolResult,
237237
("tools/list", "2025-03-26"): v2025.ListToolsResult,
238238
("completion/complete", "2025-06-18"): v2025.CompleteResult,
239239
("initialize", "2025-06-18"): v2025.InitializeResult,
@@ -246,7 +246,7 @@
246246
("resources/subscribe", "2025-06-18"): v2025.EmptyResult,
247247
("resources/templates/list", "2025-06-18"): v2025.ListResourceTemplatesResult,
248248
("resources/unsubscribe", "2025-06-18"): v2025.EmptyResult,
249-
("tools/call", "2025-06-18"): (v2025.CallToolResult, v2025.CreateTaskResult),
249+
("tools/call", "2025-06-18"): v2025.CallToolResult,
250250
("tools/list", "2025-06-18"): v2025.ListToolsResult,
251251
("completion/complete", "2025-11-25"): v2025.CompleteResult,
252252
("initialize", "2025-11-25"): v2025.InitializeResult,

tests/types/test_parity.py

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4,15 +4,14 @@
44

55
import inspect
66
from types import ModuleType
7-
from typing import Any, get_args
87

98
import mcp_types as monolith
109
import mcp_types._types as _types
1110
import mcp_types.jsonrpc as jsonrpc
1211
import mcp_types.v2025_11_25 as v2025_11_25
1312
import mcp_types.v2026_07_28 as v2026_07_28
1413
import pytest
15-
from mcp_types._wire_base import KeepRequiredNullable
14+
from mcp_types._wire_base import KeepRequiredNullable, admits_none
1615
from pydantic import BaseModel
1716

1817
SURFACES: tuple[ModuleType, ...] = (v2025_11_25, v2026_07_28)
@@ -231,7 +230,7 @@ def _models_with_a_required_nullable_field() -> list[tuple[str, type[BaseModel],
231230
nullable_required = frozenset(
232231
field.serialization_alias or field.alias or field_name
233232
for field_name, field in obj.model_fields.items()
234-
if field.is_required() and (field.annotation is Any or type(None) in get_args(field.annotation))
233+
if field.is_required() and admits_none(field.annotation)
235234
)
236235
if nullable_required:
237236
found.append((f"{module.__name__}.{name}", obj, nullable_required))
@@ -250,8 +249,11 @@ def test_models_with_a_required_nullable_field_survive_an_exclude_none_dump(
250249
) -> None:
251250
"""`exclude_none=True` would drop such a field, leaving a body that fails its own schema.
252251
253-
`KeepRequiredNullable` puts it back. This walk is over the built models, which is the
254-
authority; `scripts/gen_surface_types.py` reaches the same set from the raw schema.
252+
`KeepRequiredNullable` puts it back. This walk deliberately shares `admits_none` with the
253+
runtime, so it is a consistency check rather than an oracle: what it catches is a model the
254+
generator's independent schema-side rule (`_admits_null`) failed to give the base, or a
255+
hand-written model nobody remembered. A spelling both rules miss is caught by neither, and
256+
only an end-to-end test of that message would find it.
255257
"""
256258
assert issubclass(cls, KeepRequiredNullable), (
257259
f"{qualname} needs the KeepRequiredNullable base. For a generated model, regenerate the "
@@ -260,7 +262,9 @@ def test_models_with_a_required_nullable_field_survive_an_exclude_none_dump(
260262
"in _types.py and jsonrpc.py take the base directly."
261263
)
262264
instance = cls.model_construct(**dict.fromkeys(cls.model_fields))
263-
assert nullable_required <= instance.model_dump(by_alias=True, mode="json", exclude_none=True).keys()
265+
dumped = instance.model_dump(by_alias=True, mode="json", exclude_none=True)
266+
assert nullable_required <= dumped.keys()
267+
assert all(dumped[alias] is None for alias in nullable_required)
264268

265269

266270
def test_every_surface_class_is_accounted_for() -> None:

0 commit comments

Comments
 (0)