Skip to content

Fix legacy output schemas for delegating server tools - #1928

Merged
tarekgh merged 9 commits into
modelcontextprotocol:mainfrom
Oxygen56:fix/1916-delegating-tool-output-schema
Oct 28, 2026
Merged

tarekgh merged 9 commits into
modelcontextprotocol:mainfrom
Oxygen56:fix/1916-delegating-tool-output-schema

Conversation

@Oxygen56

Copy link
Copy Markdown
Contributor

Fixes #1916

Summary

Tools wrapped in DelegatingMcpServerTool now advertise an output schema matching their structured content to clients on the 2025-06-18 and 2025-11-25 protocols.

Unwrap the delegation chain only to identify SDK-created tools, then apply the existing legacy schema transform to the outer tool's ProtocolTool. Wrapper descriptions and schema constraints are preserved. Custom tools and 2026-07-28 clients retain their existing behavior. No public API is added.

Verification

Validated with the original .NET project/MSBuild and xUnit/VSTest entries, using the prepared local dependencies without restore or downloads. Builds retained warnings-as-errors and included the referenced analyzers, hosting/task/app libraries, and test-server prerequisites.

  • Built tests/ModelContextProtocol.Tests/ModelContextProtocol.Tests.csproj with --no-restore --framework net10.0 --configuration Debug: passed with zero warnings/errors on both baseline and repaired sources.
  • Ran the same focused regression through dotnet test against the selected baseline (3338e88) and repaired sources. The baseline produced 10 expected schema failures and 28 passes; the repair passed all 38 cases. The actual baseline production files were used with the added tests backported temporarily; repaired source bytes were restored before the after-fix build.
  • Ran the broader server/protocol suites through the project test entry: 1,333 passed, zero failed/skipped.
  • git diff --check: passed; the contribution worktree is clean.

Focused test filter:
FullyQualifiedName~DelegatingToolOutputSchemaTests|FullyQualifiedName~DelegatingMcpServerToolTests|FullyQualifiedName~Sep2106ListToolsBackCompatTests

Tests used --no-build --no-restore --framework net10.0 --configuration Debug, the existing blame/crash handling and 7-minute hang budget, TRX output, and XPlat coverage collection. Coverage includes nested wrappers, wrapper overrides, nullable object and absent schemas, custom tools, both legacy protocols, and modern-client controls.

The complete platform/framework/configuration matrix, npm-backed integrations, packaging/API validation, AOT, documentation, and combined coverage workflows remain for CI. They were not run in this offline validation. A separate independent review is still required before publication.

AI assistance was used in preparing this contribution.

@jeffhandley

Copy link
Copy Markdown
Contributor

On first glance, this looks good. We will take a closer look before merging, but thanks for getting this prep'd, @Oxygen56!

@tarekgh

tarekgh commented Oct 15, 2026

Copy link
Copy Markdown
Contributor

The production fix looks good and addresses a real compatibility issue for delegated tools with older clients.

One issue to fix before merging: the new tests use JsonSerializer.SerializeToElement(...) with anonymous objects, but the test project intentionally disables reflection-based serialization on .NET 9. This causes 15 test failures.

Could you replace those calls in DelegatingToolOutputSchemaTests.cs at lines 94-95, 161, and 172-173 with reflection-free JSON parsing? For example:

using var document = JsonDocument.Parse("""{"type":"integer","minimum":0}""");
var schema = document.RootElement.Clone();

Please keep the reflection-disabled setting unchanged. The focused tests pass on the other three target frameworks, and all 1,333 server/protocol tests pass on .NET 10.

@tarekgh

tarekgh commented Oct 19, 2026

Copy link
Copy Markdown
Contributor

@Oxygen56 do you have a chance to address my feedback so we can proceed? Thanks!

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new tests retain a short discovery timeout instead of the documented controlled-server defaults, risking avoidable CI failures.

1 open issue
What changed in this PR

Fixes #1916 by applying legacy output-schema conversion to SDK-created tools behind delegating wrappers, keeping advertised schemas consistent with structured content for older clients.

Changes:

  • Unwraps delegation chains to identify SDK-created tools while preserving the outer wrapper’s metadata and schema constraints.
  • Leaves custom tools and modern-protocol clients unchanged, without adding public API.
  • Adds regression coverage for nested wrappers, schema overrides, optional schemas, and protocol versions.
File Description
tests/​ModelContextProtocol.Tests/​Server/​DelegatingToolOutputSchemaTests.cs Adds schema and structured-content regression tests.
src/​ModelContextProtocol.Core/​Server/​McpServerImpl.cs Recognizes wrapped SDK tools for legacy schema conversion.
src/​ModelContextProtocol.Core/​Server/​DelegatingMcpServerTool.cs Adds internal access to the wrapped tool.
src/​ModelContextProtocol.Core/​Server/​AIFunctionMcpServerTool.cs Allows legacy conversion of the outer wrapper’s protocol tool.

Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Composition-based wrapper schemas can still cause legacy listings to advertise a result envelope that invocation does not return.

No open issues 🎉

1 previously missed issue

In code that hasn't changed since last review

Medium severity Align advertised and invoked schemas for allOf wrapper overrides

src/​ModelContextProtocol.Core/​Server/​McpServerImpl.cs:1555

For an SDK tool with an object output schema, a delegating wrapper that expresses the equivalent schema as allOf: [innerSchema] now advertises a required result envelope to legacy clients. Invocation still uses the inner schema in CreateStructuredResponse and returns the bare object, so the content fails the advertised schema. Align the listing and invocation wrapping decisions while preserving wrapper constraints, and add a regression test for this override on both legacy protocols.

1 resolved since last review

Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Wrapped tools returning an explicit CallToolResult can advertise a required result envelope while passing through unwrapped structured content, causing a schema mismatch for legacy clients.

1 open issue

Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Raw CallToolResult pass-throughs can return unwrapped objects while legacy tool listings require a result wrapper, breaking schema–response compatibility.

1 open issue
1 resolved since last review

Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Output-schema normalization mishandles nullable tool results and recursive nullable children, causing advertised schemas to reject responses sent to legacy clients.

2 open issues
1 resolved since last review

Review effort: Balanced

@tarekgh
tarekgh requested a balanced review from Copilot October 28, 2026 19:58

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Expanded schema normalization and reflection-based delegation require independent runtime validation of recursive references, Native AOT behavior, and legacy-client compatibility.

No open issues 🎉

2 resolved since last review

Review effort: Balanced

@tarekgh
tarekgh merged commit 9b50c3d into modelcontextprotocol:main Oct 28, 2026
16 of 17 checks passed
@jeffhandley jeffhandley mentioned this pull request Nov 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A tool behind DelegatingMcpServerTool advertises a non-object outputSchema to pre-2026-07-28 clients (regression from 1.4.1)

4 participants