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 3 commits into
Conversation
SEP-1686 makes a task-augmented `tools/call` answer with a `CreateTaskResult`,
but the `tools/call` result rows 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. All four pre-2026 versions get the arm, because all four share
`v2025.CallToolRequest` and therefore already parse `params.task`: answering it
at one of them and not the others is the same incoherence one version along.
`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.
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.
📚 Documentation preview
|
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.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
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
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
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.
Four review follow-ups, none of them behaviour changes to the task path itself.
Three sentences claimed a `CreateTaskResult` answers a task-augmented call on a
"handshake-era connection". That term means all four pre-2026 revisions
(`HANDSHAKE_PROTOCOL_VERSIONS`), and only 2025-11-25 defines the shape, so the
section contradicted its own recipe, which already gates on
`ctx.protocol_version`. The earlier revisions still hand `params.task` to the
handler, so a server author testing against 2025-06-18, which is what most
deployed clients negotiate, would have followed the prose into an opaque
internal error. All three now say 2025-11-25 and name why the others reject it.
The tasks capability recipe only worked over stdio. It built an
`InitializationOptions` to pass to `Server.run()`, but the streamable HTTP
manager and the SSE app call `create_initialization_options()` themselves and
take no override, so on the SDK's primary transport there was nowhere to put it.
Overriding that method instead reaches every transport and is the same length.
`_is_excluded` replaces a membership test that could not tell pydantic's
`{"field": True}` (drop the field) from `{"field": {"subkey"}}` (keep the field,
descend into it), so a required nullable field was left out of a dump that
pydantic had kept it in, producing a body that fails its own schema. The two
now agree on every include/exclude shape, which a test pins.
`ServerResult`'s docstring claimed to be every result payload a server can
return. It never was: the five 2025-11-25 task results are all absent, four of
them producible today through `add_request_handler`, and the 2025-11-25 schema's
own `ServerResult` omits them too. The docstring now records the exclusion the
way the sibling unions already do, rather than the union growing one arbitrary
arm of the five.
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docs/migration.md">
<violation number="1" location="docs/migration.md:1689">
P2: The pasted subclass advertises `tasks` to every handshake client, not just 2025-11-25. `create_initialization_options()` receives no negotiated protocol version, and the runner returns its capabilities unchanged after negotiating older handshake revisions; those revisions cannot accept the advertised task-augmented `tools/call` result. Please qualify this recipe as requiring a 2025-11-25-only endpoint/negotiation policy (or document a version-aware response layer) rather than presenting it as a safe advertisement for a server that also serves older handshake clients.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| class TasksServer(Server[Any]): | ||
| def create_initialization_options(self, *args: Any, **kwargs: Any) -> InitializationOptions: | ||
| options = super().create_initialization_options(*args, **kwargs) | ||
| tasks = ServerTasksCapability( |
There was a problem hiding this comment.
P2: The pasted subclass advertises tasks to every handshake client, not just 2025-11-25. create_initialization_options() receives no negotiated protocol version, and the runner returns its capabilities unchanged after negotiating older handshake revisions; those revisions cannot accept the advertised task-augmented tools/call result. Please qualify this recipe as requiring a 2025-11-25-only endpoint/negotiation policy (or document a version-aware response layer) rather than presenting it as a safe advertisement for a server that also serves older handshake clients.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/migration.md, line 1689:
<comment>The pasted subclass advertises `tasks` to every handshake client, not just 2025-11-25. `create_initialization_options()` receives no negotiated protocol version, and the runner returns its capabilities unchanged after negotiating older handshake revisions; those revisions cannot accept the advertised task-augmented `tools/call` result. Please qualify this recipe as requiring a 2025-11-25-only endpoint/negotiation policy (or document a version-aware response layer) rather than presenting it as a safe advertisement for a server that also serves older handshake clients.</comment>
<file context>
@@ -1680,23 +1680,20 @@ Two things to know about handlers registered this way. They serve every negotiat
+class TasksServer(Server[Any]):
+ def create_initialization_options(self, *args: Any, **kwargs: Any) -> InitializationOptions:
+ options = super().create_initialization_options(*args, **kwargs)
+ tasks = ServerTasksCapability(
+ list=TasksListCapability(),
+ cancel=TasksCancelCapability(),
</file context>
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.
1 participant
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 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