Repository navigation
Validate an explicit null structuredContent against the output schema - #3621
Conversation
The client treated a `structuredContent` of JSON null the same as an absent field and raised "has an output schema but did not return structured content". Since 2026-07-28 null is a legal structured result, so only an absent field is reported as missing now; an explicit null is validated against the tool's output schema like any other value. Fixes #3345
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Additional findings (no inline location):
-
🟡
src/mcp/client/session.py— nit: PR checklist — AGENTS.md asks that 2026-07-28 spec behaviour have a matching conformance-suite scenario that passes against this SDK, and to tell the user if none exists. The PR cites the 2026-07-28 schema as the source for nullstructuredContentbut the description says nothing about a conformance scenario, and nothing under.github/actions/conformancereferences one. Fix: name the matching client conformance scenario in the PR description, or state that none exists so an issue can be raised on the conformance repo.Why this was flagged
Nothing fails at runtime. The instruction guards against SDK behaviour drifting from the spec without an external check; the condition under which it bites is a later regression in null handling that no CI leg would catch. Whether this counts as a "new feature" is arguable (the author frames it as a validation fix), and I could not query the conformance repo from this environment to confirm whether a null-structuredContent client scenario exists, so this is only a request for the author to state it.
Verification: AGENTS.md (base 449070c), Testing section: "New features from the 2026-07-28 spec must have a matching test in the [conformance suite] that passes against this SDK (CI runs it via .github/workflows/conformance.yml). If no matching test exists, stop and tell the user so they can raise an issue on the conformance repo."
|
|
||
|
|
||
| # --- null structuredContent --- | ||
| # The results are scripted: a server built on this SDK leaves a null `structuredContent` off the wire. |
There was a problem hiding this comment.
🟡 (optional) Maintainers get three new raw-wire tests whose docstrings do not say why the public API could not produce the input, which the test-quality skill requires of every such test. The only justification is a section comment at tests/client/test_session.py:1832-1833; the docstrings at 1843-1844, 1861-1862 and 1879-1880 omit it. The comment's claim is also incomplete: a lowlevel middleware returning a dict reaches the wire via _dump_result at src/mcp/server/runner.py:120-124 unchanged, so an SDK server can put "structuredContent": null on the wire. Fix: each _ScriptedDispatcher test's docstring states why the typed API cannot produce the input, or drive the null case through a middleware helper instead. [also at: tests/client/test_session.py:1863 - nit: Maintainers triaging a later failure of this test cannot tell from its docstring whether it pins spec-mandated or SDK-defined behaviour.; tests/client/test_session.py:1881 - nit: Maintainers get a scripted-wire test for an input the public API can already produce, against the test-quality skill's last-resort rule.]
Why this was flagged
The skill at .claude/skills/test-quality/SKILL.md:36-39 says scripting a peer over raw streams is a last resort and "Every such test's docstring states why the public API couldn't do it." The three tests added at tests/client/test_session.py:1842, 1860 and 1878 use _ScriptedDispatcher and their docstrings state spec provenance only. The file-level comment at tests/client/test_session.py:1833 ("a server built on this SDK leaves a null structuredContent off the wire") is the dismissal's basis, but it is not in a docstring and it is not fully true: _dump_result in src/mcp/server/runner.py:110-124 only applies exclude_none to BaseModel results and copies a dict result as-is, so a Server.middleware short-circuit returning {"content": [], "structuredContent": None, "resultType": "complete"} sends an explicit null that the public Client path could then validate. The rule's own guidance ("First ask what it would take for the public API to express it — often a small helper suffices") applies.
Verification: nit. Triggering condition: any maintainer reviewing or triaging these tests. .claude/skills/test-quality/SKILL.md:36-39 says "Every such test's docstring states why the public API couldn't do it." The docstrings at tests/client/test_session.py:1843-1844, 1861-1862 and 1879-1880 say nothing about why the public API could not produce the input. Nothing fails at runtime.
| from referencing.exceptions import Unresolvable | ||
|
|
||
| if result.structured_content is None: | ||
| if result.structured_content is None and "structured_content" not in result.model_fields_set: |
There was a problem hiding this comment.
🟡 nit (optional): AGENTS.md says any change to an existing public API's observable behaviour is a maintainer design decision that should generally be avoided: this line changes what ClientSession.validate_tool_result / call_tool do for an explicit null structuredContent (used to raise "did not return structured content"; now returns the result when the schema allows null, or raises a different RuntimeError message when it doesn't), on every protocol version. Fix: have a maintainer explicitly sign off on the behaviour change in the PR (or gate it) rather than landing it as a plain fix.
Why this was flagged
Nothing fails at runtime; this is a contract check. Callers on 2.x that matched the exact "has an output schema but did not return structured content" message, or relied on call_tool raising for a null structuredContent, see different behaviour after upgrade. Mitigating facts for the maintainer: the PR description lists the change under "What users will notice" with before/after messages, cites the 2026-07-28 schema allowing null and TypeScript/Go/Rust SDK parity, and the only wire producer of a null today is a non-SDK server (this SDK's own server drops null from the wire), so the blast radius is small.
Verification: AGENTS.md (base commit, "Branching Model") says verbatim: "v2 is released; its public API is a compatibility contract for the 2.x line. Removals, renames, or any change to an existing API's signature or observable behaviour (including ones softened by a @ deprecated shim) is a design decision a maintainer makes explicitly, and should generally be avoided."
| from referencing.exceptions import Unresolvable | ||
|
|
||
| if result.structured_content is None: | ||
| if result.structured_content is None and "structured_content" not in result.model_fields_set: |
There was a problem hiding this comment.
🟡 nit (optional): AGENTS.md requires a docs/ update in the same PR when a change affects user-visible behaviour: this line changes what call_tool does with an explicit null structuredContent (now validated against the output schema instead of always raising "did not return structured content"), and the PR touches no page under docs/. Fix: in the same PR, update the page that documents client-side output-schema validation (docs/advanced/low-level-server.md, the "Invalid structured content returned by tool" paragraph, found via the mkdocs.yml nav) to say an absent field is reported as missing while an explicit null is validated like any other value.
Why this was flagged
Nothing fails at runtime. A user reading the docs would not learn that a null structuredContent is accepted when the schema permits null (or rejected as a schema mismatch otherwise) while an absent field is still "missing"; the docs currently never describe the missing-field error at all, so the gap is small and the PR description already spells out the user-visible differences. The maintainer may judge a one-sentence doc change sufficient, or that no page covers it.
Verification: AGENTS.md (base 449070c) "## Documentation": "When a change affects public API or user-visible behaviour, update the relevant page(s) under docs/ in the same PR." The diff changes src/mcp/client/session.py:1139 to if result.structured_content is None and "structured_content" not in result.model_fields_set:, so ClientSession.call_tool/validate_tool_result now accepts an explicit null structuredContent the schema permits and reports a schema-mismatch error (different message) otherwise, where it previously always raised the "did not return structured content" error; the PR's own description enumerates these under "What users will notice".
Fixes #3345.
What was wrong
When a tool declares an output schema and the server returns
"structuredContent": null, the client raisedTool ... has an output schema but did not return structured contentwithout checking the schema. An explicit null and a field that was left out were treated as the same thing.structuredContentvalue: "any JSON value (object, array, string, number, boolean, or null) that conforms to the tool's outputSchema" (schema.ts).0,falseand""were already validated. Null was the only JSON value treated as "nothing".undefined, and validates null (client.ts). The Go and Rust SDKs also keep null distinct from absent.What changes
ClientSession.validate_tool_result: structured content counts as missing only when the value isNoneand the field was not set on the result (model_fields_set).What users will notice
Only results with an explicit null
structuredContent, for a tool that declares an output schema, behave differently:{}, orpropertieswithouttype): the call used to raise and now returns the result, because null is valid against such a schema.RuntimeError, with a different message.Tool ... has an output schema but did not return structured contentInvalid structured content returned by tool ...: None is not of type ...CallToolResultbuilt in code withstructured_content=Nonepassed explicitly counts as an explicit null too.validate_tool_resultdirectly, or returned by an extension's result resolver.CallToolResult(content=[...])without the argument is still "missing".Unchanged:
structuredContentfield still raises the "did not return structured content" error, whatever the schema allows.isErrorresults and tools without an output schema are still not validated.Not included
structuredContentoff the wire, so it cannot return a top-level null result itself. That is a separate change on the sending side.How it was checked
tests/client/test_session.pydriveClientSession.call_toolon a 2026-07-28 session against scripted wire results, since an SDK server cannot produce the null:mainand pass with the change; the third passes on both../scripts/testpasses with 100% coverage; ruff and pyright are clean.AI Disclaimer