Repository navigation
Preserve nulls required by structured output schemas - #1929
mikemikimike wants to merge 2 commits into
Conversation
|
Adding a data point on why this matters beyond the TypeScript 1.x client in #1915: it breaks interop with the other official SDKs' current clients, too. I hit it in a companion sample for my book. A C# server returns a positional record with nullable members (
Both clients validate Until this ships, the per-member workaround that worked for me is |
tarekgh
left a comment
tarekgh
left a comment
There was a problem hiding this comment.
This fixes a real interoperability issue: required nullable properties must be present in structured output, and Python and TypeScript clients reject results when those properties are omitted.
I found one narrow compatibility regression with explicit output schemas, noted inline. Two related ignore-policy gaps also remain, but those predate this PR.
There was a problem hiding this comment.
🟡 Changes recommended
Globally including nulls can invalidate previously valid explicit-schema responses whose optional properties prohibit null.
1 open finding
What changed in this PR
Addresses #1915 by preserving nulls in structured tool results to match required nullable output properties.
Changes:
- Adds separate serializer options for schema-backed structured content, leaving text serialization unchanged.
- Adds record and class regression tests across two protocol versions.
| File | Description |
|---|---|
| tests/ModelContextProtocol.Tests/Server/McpServerToolTests.cs | Tests nullable output properties against their schemas. |
| src/ModelContextProtocol.Core/Server/AIFunctionMcpServerTool.cs | Preserves nulls during schema-backed structured serialization. |
🧠 Review effort: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Signed-off-by: mikemikimike <13286568797@163.com>
There was a problem hiding this comment.
🟡 Changes recommended
Schema traversal remains incomplete, and restoration can replace actual values with null or invalidate previously valid output.
5 open issues
1 resolved since last review
Review effort: Balanced
| if (resultNode is JsonObject resultObject && schemaNode is JsonObject schemaObject) | ||
| { | ||
| JsonArray? required = schemaObject["required"] as JsonArray; | ||
| JsonObject? properties = schemaObject["properties"] as JsonObject; |
| if (properties is not null) | ||
| { | ||
| foreach (KeyValuePair<string, JsonNode?> property in resultObject) | ||
| { | ||
| if (property.Value is not null && properties.TryGetPropertyValue(property.Key, out JsonNode? propertySchema)) |
| properties.TryGetPropertyValue(propertyName, out JsonNode? propertySchema) && | ||
| SchemaAllowsNull(propertySchema)) | ||
| { | ||
| resultObject[propertyName] = null; |
| else if (resultNode is JsonArray resultArray && | ||
| schemaNode is JsonObject { } arraySchema && | ||
| arraySchema["items"] is JsonNode itemSchema) | ||
| { | ||
| foreach (JsonNode? item in resultArray) | ||
| { | ||
| if (item is not null) | ||
| { | ||
| changed |= AddMissingRequiredNullableProperties(item, itemSchema); | ||
| } | ||
| } | ||
| } |
| } | ||
| } | ||
|
|
||
| return false; |
|
@mikemikimike thanks for last updates. Could you please look at the copilot comments and address them? |
Fixes #1915
Issue: #1915
When structured tool output has an output schema and serializer options ignore null values, required nullable properties are omitted from
structuredContent. Preserve explicit nulls for this serialization path so the result matches its schema while leaving ordinary text serialization unchanged.Add regression coverage for record and class outputs with null and non-null values across the current protocol and
2025-11-25.Validation passed:
dotnet testin Debug and Release across the repository test suites;dotnet format whitespace --verify-no-changes; Windows x64 NativeAOT publish and execution; Release NuGet pack; and DocFX with warnings treated as errors. The Core suites passed 2,398 tests per net8.0/net9.0/net10.0 target (4 skipped) and 2,081 tests on net472 (294 skipped); ASP.NET Core passed 635 tests per modern target (30 skipped); Analyzer passed 57 tests.