Skip to content

Comments

Address asymmetry in McpServerHandlers/McpClientHandlers and make all filter properties settable#1337

Open
Copilot wants to merge 6 commits intomainfrom
copilot/fix-handler-asymmetry-again
Open

Address asymmetry in McpServerHandlers/McpClientHandlers and make all filter properties settable#1337
Copilot wants to merge 6 commits intomainfrom
copilot/fix-handler-asymmetry-again

Conversation

Copy link
Contributor

Copilot AI commented Feb 20, 2026

Fixes asymmetries between server and client handler/options types and makes all filter properties settable for consistency.

Changes Made

  • Seal McpClientHandlers: Now sealed to match the already-sealed McpServerHandlers, since both are configuration containers that shouldn't be subclassed.
  • Add setter to McpServerOptions.Handlers: Now has get+set with null validation using Throw.IfNull, matching the existing McpClientOptions.Handlers pattern.
  • Add setter to McpServerOptions.Filters: Now has get+set with null validation, consistent with the Handlers property.
  • Add setters to McpServerFilters.Message and McpServerFilters.Request: Sub-container properties now follow the same settable pattern with null validation.
  • Make all IList<T> filter properties settable: All 11 filter list properties in McpRequestFilters and both filter list properties in McpMessageFilters now have get+set with null validation, enabling both { ... } collection initializer syntax and [] collection expression syntax.
  • Revert TestSseServer to use Handlers setter: Reverted TestSseServer/Program.cs to use options.Handlers = new() { ... } object initializer pattern to provide test coverage for completely replacing the McpServerHandlers instance.

All settable properties use the same field-backed lazy initialization pattern (get => field ??= new() or get => field ??= []) already established by McpClientOptions.Handlers.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

…r consistency

- McpClientHandlers is now sealed, matching McpServerHandlers
- McpServerOptions.Handlers now has get+set with null validation, matching McpClientOptions.Handlers

Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
@jeffhandley jeffhandley added the breaking-change This issue or PR introduces a breaking change label Feb 21, 2026
@jeffhandley jeffhandley added this to the Stable public API milestone Feb 21, 2026
jeffhandley
jeffhandley previously approved these changes Feb 21, 2026
…table

- McpServerOptions.Filters is now get+set with null validation
- McpServerFilters.Message and McpServerFilters.Request are now get+set with null validation
- No other get-only/get-init properties found that should be get/set
  (IList<T> filter collections and internal McpServerImpl properties are correctly get-only)

Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
Copy link
Collaborator

@jeffhandley jeffhandley left a comment

Choose a reason for hiding this comment

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

Before we merge the setters all back in, we need to get @halter73's review on this to make sure we don't have to restore the logic from here:

https://github.com/modelcontextprotocol/csharp-sdk/pull/1313/files#diff-dbe35ac96162daee9fdbec4c7b96a70c842ddeff0cd4943bfedcb84ffb0f280dL81

Copilot AI changed the title [WIP] Address asymmetry in McpServerHandlers and McpClientHandlers Address asymmetry in McpServerHandlers/McpClientHandlers and make Filters settable Feb 21, 2026
Copilot AI requested a review from stephentoub February 21, 2026 02:25
@halter73
Copy link
Contributor

I'm fine making these both settable instead for maximum backwards compatibility. I still don't think we need to bring back any of the logic from OverwriteWithSetHandlers though.

This does technically make something order dependent that wasn't previously, but previously configuring McpServerHandlers in a configuration callback had the exact same ordering dependency with WithXXXHandler that configuring McpServerOptions now does. I also don't think this code exists or ever will exist that expect us to merge handlers added via WithXXXHandler and a completely overwritten McpServerOptions.Handler property, so I don't think it's worth adding back the complexity of OverwriteWithSetHandlers.

It might be worth reverting the changes we made to TestSseServer in #1308 though to make sure we still have test coverage for completely replacing the McpServerHandlers instance.

@stephentoub stephentoub marked this pull request as ready for review February 21, 2026 05:39
…sageFilters; revert TestSseServer to use Handlers setter

- All IList<T> filter properties in McpRequestFilters (11 properties) and
  McpMessageFilters (2 properties) now have get+set with null validation
- Reverted TestSseServer to use `options.Handlers = new() { ... }` pattern
  to provide test coverage for the restored Handlers setter

Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
Copilot AI changed the title Address asymmetry in McpServerHandlers/McpClientHandlers and make Filters settable Address asymmetry in McpServerHandlers/McpClientHandlers and make all filter properties settable Feb 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking-change This issue or PR introduces a breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants