Repository navigation
Preserve nulls required by structured output schemas - #1929
mikemikimike wants to merge 8 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
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
Resolve references and composition branches during traversal · New Handle dictionary values via additionalProperties schemas · New Preserve omitted required members' original non-null values · New Apply prefixItems before the fallback items schema · New Recognize null acceptance in empty and const-null schemas · New
1 resolved since last review
Review effort: Balanced
|
@mikemikimike thanks for last updates. Could you please look at the copilot comments and address them? |
Signed-off-by: mikemikimike <13286568797@163.com>
There was a problem hiding this comment.
🟡 Changes recommended
The restoration logic can return incorrect values, invalidate previously valid responses, and miss required nullable properties.
6 open issues
Resolve required properties across combined schema definitions · New Restore values omitted by member-level default-ignore conditions · New Validate candidate repairs before selecting schema branches · New Support all mathematically integral JSON numbers · New Compare const and enum values structurally · New Evaluate not conditions when selecting nullable branches · New
5 resolved since last review
Review effort: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The recovery path can bypass application redaction filters, and unresolved correctness and performance issues remain.
7 open issues
Source recovery bypasses ShouldSerialize redaction filters · New Valid responses incur unnecessary second serialization · New Nested repairs stop before restoring child properties · New Source lookup performs quadratic work for complete values · New Branch repair search misses required nullable property · New patternProperties skipped for explicitly named properties · New Fallback recovery drops property-level NumberHandling · New
6 resolved since last review
Evaluate not conditions when selecting nullable branches Compare const and enum values structurally Support all mathematically integral JSON numbers Validate candidate repairs before selecting schema branches Restore values omitted by member-level default-ignore conditions Resolve required properties across combined schema definitions
Review effort: Balanced
> [!NOTE] > AI-generated by GitHub Copilot.
| _ => false, | ||
| }; | ||
|
|
||
| if (propertyInfo?.ShouldSerialize is not null && !omittedByMemberIgnore) |
There was a problem hiding this comment.
Could we keep caller-provided ShouldSerialize filters authoritative even when the member-level ignore condition also matches? The earlier filter fix still permits an excluded non-null value to be published.
I reproduced this with [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingDefault)] public int Secret { get; init; }, Secret = 0, and a resolver that sets this property's ShouldSerialize to (_, _) => false. An explicit output schema requires secret as integer or null. Both serialization snapshots omit the property, but omittedByMemberIgnore is true, so this guard allows source recovery and the response becomes {"secret":0} despite the caller's exclusion predicate.
The same repro also fails with int? Secret = 0. In that case, IsDefaultValue additionally treats nullable zero as default by unwrapping the nullable type, although default(int?) is null and the member ignore condition alone would not omit zero.
Please preserve caller-defined exclusion predicates without confusing them with the serializer's own ignore-condition predicates, and correct the nullable-default check. Regression coverage should include both the non-nullable and nullable zero cases; fixing only nullable-default detection would leave the first bypass intact.
| repairedResult = originalResult.DeepClone(); | ||
| foreach (RepairCandidate candidate in candidates) | ||
| { | ||
| ApplyRepairCandidate(repairedResult, candidate); |
There was a problem hiding this comment.
Could intermediate repairs remain isolated to an applicable composition branch rather than applying every collected candidate? The nested-parent and broad-union regressions now pass individually, but their combination still fails.
I reproduced this under global WhenWritingDefault with an output containing Tag = "0", a default-valued child struct whose nullable Note has member-level WhenWritingNull, and Other = null. The schema is:
{
"type": "object",
"oneOf": [
{
"properties": {
"tag": { "const": "0" },
"child": {
"type": "object",
"properties": { "note": { "type": ["string", "null"] } },
"required": ["note"]
}
},
"required": ["tag", "child"],
"additionalProperties": false
},
{
"properties": {
"tag": { "const": "1" },
"other": { "type": ["string", "null"] }
},
"required": ["tag", "other"],
"additionalProperties": false
}
]
}The first pass discovers child:{} and other:null. No candidate set validates yet because child.note has not been discovered. This fallback then inserts both candidates. The next pass can add child.note, but other is now an extra property forbidden by the matching tag-0 branch. Later passes only add missing properties and cannot remove it, so repair gives up and returns the original invalid {"tag":"0"}.
Please isolate or backtrack intermediate branch repairs, and add a regression asserting the result contains child.note:null without other.
| } | ||
|
|
||
| if (instanceNode is JsonObject dependencyInstance && | ||
| schemaObject["dependentSchemas"] is JsonObject dependentSchemas) |
There was a problem hiding this comment.
Could we also collect names from applicable dependentRequired entries? The new dependentSchemas traversal fixes the previously reported case, but the equivalent dependent-required form is still skipped:
{
"type": "object",
"properties": {
"id": { "type": "string" },
"note": { "type": ["string", "null"] }
},
"dependentRequired": { "id": ["note"] }
}With a tool returning Id = "42", Note = null under the default null-omitting options, I reproduced structured content remaining {"id":"42"}. Validation reports the missing dependency, but candidate collection never adds note, even though the complete snapshot contains its actual null value.
Please merge each active dependentRequired list into the instance's required-name context when its trigger property is present, and add a regression asserting note:null is restored.
There was a problem hiding this comment.
🟡 Changes recommended
Recovery can fail tool calls or leave repairable structured output invalid in converter, nested-union, and dependency cases.
3 open issues
7 resolved since last review
Source recovery bypasses ShouldSerialize redaction filters Fallback recovery drops property-level NumberHandling patternProperties skipped for explicitly named properties Branch repair search misses required nullable property Source lookup performs quadratic work for complete values Nested repairs stop before restoring child properties Valid responses incur unnecessary second serialization
Review effort: Balanced
| JsonElement completeResult = JsonSerializer.SerializeToElement( | ||
| originalResult, _serializerTypeInfoIncludingDefaults.Value); |
| repairedResult = originalResult.DeepClone(); | ||
| foreach (RepairCandidate candidate in candidates) | ||
| { | ||
| ApplyRepairCandidate(repairedResult, candidate); |
| if (instanceNode is JsonObject dependencyInstance && | ||
| schemaObject["dependentSchemas"] is JsonObject dependentSchemas) |


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.