[DRAFT] Restructure IMcpEndpoint
, IMcpClient
, and IMcpServer
#723
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Note: This PR is currently a work in progress. Future changes are expected.
Summary
Makes the following changes:
McpEndpoint
abstract base classIMcpClient
interfaceIMcpEndpoint
->IMcpSession
IMcpServer
->IMcpServerSession
McpClient
->McpClientSession
McpSession
(internal) ->McpSessionHandler
(still internal)McpClientSession
publicIMcpClient
gets replaced withMcpClientSession
McpClientSession
andMcpServerSession
to function without relying on a common base class implementing shared functionalityNote: The "session" naming is not final. We might end up keeping it, or replacing it with "connection", or dropping the suffix altogether and keeping the original
McpClient
andMcpServer
naming.Description
The original issue comment suggested we remove the
IMcpEndpoint
,IMcpClient
, andIMcpServer
interfaces and make each corresponding classpublic
.This PR currently only removes the
IMcpClient
interface but keeps the other two (albeit with different names). However, I'd still like to pursue removing the other interfaces. Some notes about this:McpServerSession
is not the only implementation ofIMcpServerSession
. There's alsoDestinationBoundMcpServer
, which wraps the underlyingIMcpServerSession
and augments it with the destination transport. This pattern wouldn't be possible if we were to removeIMcpServerSession
. I need to think about and evaluate any alternative ways to achieve similar functionality for this specific case, without relying on an interface.McpSession
class would look different than theMcpEndpoint
abstract base class that exists today. This is because the currentMcpEndpoint
class primarily serves as a way to share functionality among the existingMcpClient
andMcpServer
types. Simply makingMcpEndpoint
public would drastically expand the public API surface, since each internal type exposed by a protected member would have to be madepublic
(e.g.,RequestHandlers
,NotificationHandlers
, andMcpSessionHandler
). What we'd end up doing instead is converting theIMcpEndpoint
interface "directly" into anMcpSession
abstract class, replacing the currentMcpEndpoint
abstract class. This PR already removesMcpEndpoint
and refactorsMcpServerSession
andMcpClientSession
to not rely on a base class providing shared functionality, so it should be fairly staightforward to add back a new abstract base class whose purpose is more interface-like.To Do
IMcpServerSession
and makeMcpServerSession
publicMcpServerFactory
and favor a publicMcpServerSession
constructorMcpClientFactory
in favor of a non-staticMcpClient
class, which:McpClientSession
s, such as:ConnectAsync(string url)
LaunchAsync(params string[] commandLineArgs)
McpClientSessions
to enable disposing them all at onceMcpClientOptions
that configures eachMcpClientSession
Anticipated usage examples
Following are some examples for how I expect the usage of
McpClient
APIs to look:Before
After
This resembles the changes proposed by @halter73 in #355 (comment)
Alternatively, we could rename
McpClientFactory
toMcpClient
and keep the existing factory approach. This would also work and is an approach worth considering. However, a non-staticMcpClient
provides a convenient way to share configuration between sessions and dispose them all at once.Fixes #355