Initial feedback on McpClient NO MERGE #1001

ericstj · 2025-11-20T22:51:31Z

@stephentoub @halter73 @mikekistler @jeffhandley

stephentoub · 2025-11-21T01:35:44Z

docs/McpClient-Feedback.md

+ModelContextProtocol.Protocol.Implementation name is too generic.  Lots of things in this namespace named poorly.  That might be OK if users don't have to deal with them, but it looks like they do.
+
+
+ModelClient.CompleteAsync - naming seems off.  Maybe "GetCompletion" 


FWIW, it's following the naming used by the MCP method: completion/complete

docs/McpClient-Feedback.md

stephentoub · 2025-11-21T01:38:01Z

docs/McpClient-Feedback.md

+
+
+
+ModelClient - Enumerate\* vs List\* - why have these redundant methods that return the same thing just one wrapping.  Can we just return IAE and have folks materialize that?


The List one is the vastly most common need, to actually get back an IList. IIRC, @eiriktsarpalis also thought it would be desirable to have the EnumerateXxAsync ones. I wouldn't want to drop the ListXxAsync ones, as that would be penalizing the 90% case.

I see - do we follow this pattern elsewhere? I just tripped up on it and thought breifly about which was right/better and if they were different. An alternate naming scheme might be

GetAsync and GetAsEnumerableAsync - that makes the less common one less attractive in names.

stephentoub · 2025-11-21T01:40:55Z

docs/McpClient-Feedback.md

+
+
+
+ModelClient.EnumerateToolsAsync - why accept JsonSerializerOptions on this call, it doesn't feel like something tied to enumerating tools.  Would it make more sense to make this a property on the client?


It gets stored into the create McpClientTool instance (McpClientTool : AIFunction), where it's needed for serializing arguments/results when the AIFunction is invoked.

I get it, but why pass on the method? Do folks really provide a different one per call? Would it make more sense as a property on the client to be used as needed by the methods?

stephentoub · 2025-11-21T01:45:05Z

docs/McpClient-Feedback.md

+
+
+
+SetLoggingLevel(LogLevel, CancellationToken) vs SetLoggingLevel(LoggingLevel, CancellationToken) -- WHY?  Don't make these overloads, instead choose one then make conversions on the types.


LoggingLevel is the MCP type, LogLevel is the M.E.Logging type. Why shouldn't these be overloads? LoggingLevel has more values than LogLevel, so anyone with a LogLevel would be forced to call ToLoggingLevel as part of calling this. Why is that better than having it "just work"?

With an enum like this aren't folks directly calling it with a literal? Why make them choose which one to use? The reason I wrote this was because I saw both in intellisense and wondered which I should use and why they were different. Having both sets the precedent for adding more. If we choose one as the currency of this type it establishes a pattern without any promise of more.

stephentoub · 2025-11-21T01:47:23Z

docs/McpClient-Feedback.md

+
+
+
+SubscribeToResourceAsync - how are the notifications delivered?  This method confused me as I don't understand what it's supposed to do.  Might just need to research more.


MCP has a notifications mechanism. The client can set up a notification handler for specific notifications.

csharp-sdk/src/ModelContextProtocol.Core/McpSession.cs

Line 91 in 15139cc

public abstract IAsyncDisposable RegisterNotificationHandler(string method, Func<JsonRpcNotification, CancellationToken, ValueTask> handler);

stephentoub · 2025-11-21T01:48:32Z

docs/McpClient-Feedback.md

+
+TextContentBlock doesn't override ToString -- to get any of the data returned I need to get at protocol types, which feels wrong since those are all "raw" and not designed surface area.
+
+Same is true for TextResourceContents.  The entire content model seems pretty rough - might be better to unify on MEAI content types.


I don't believe relying only on the MEAI types is viable. They're good for passing a 90% representation around, but they will never be full-fidelity with every provider.

Yeah, that's fair. It would put pressure on us to add more types to MEAI if we didn't support the scenario, but it could create a common set of things that the ecosystem might build upon. We can always do that later I suppose. Perhaps we can build conversion extensions now.

Perhaps we can build conversion extensions now.

We have those, e.g.

csharp-sdk/src/ModelContextProtocol.Core/AIContentExtensions.cs

Line 254 in c94cb89

public static AIContent? ToAIContent(this ContentBlock content)

csharp-sdk/src/ModelContextProtocol.Core/AIContentExtensions.cs

Line 297 in c94cb89

public static AIContent ToAIContent(this ResourceContents content)

csharp-sdk/src/ModelContextProtocol.Core/AIContentExtensions.cs

Line 360 in c94cb89

public static ContentBlock ToContentBlock(this AIContent content)

stephentoub · 2025-11-21T01:48:49Z

docs/McpClient-Feedback.md

+
+
+
+Odd that ResourceContents is plural, but  ContentBlock is not.  Why do we even need to different sets of types for these?


They map 1:1 with concepts in the MCP schema / spec.

I see - do you think it makes sense for asking the MCP folks to unify their content types?

stephentoub · 2025-11-21T01:49:22Z

docs/McpClient-Feedback.md

+
+
+
+Similarly ReadResourceResult has Contents, while CallToolResult has Content, but both are ILists.	


You can double-check to confirm, but I believe the naming here is just copying exactly what MCP has (other than capitalization).

stephentoub · 2025-11-21T01:49:55Z

docs/McpClient-Feedback.md

+BlobResourceContents exposes content as a normal string, which means it encoded the UTF-8 to a string, creating work for GC.  Instead it should keep the UTF8 contents, and lazily decode that from base64 UTF8 to bytes (either as stream, or byte array).  The same problem exists with ContentBlock types, where base-64 data is exposed as Unicode strings.
+
+
+Prompts expose arguments as types, whereas tools expose arguments as json schema.


Tools in MCP expose JSON schema. Prompts do not.

Initial feedback on McpClient NO MERGE

fd7ab10

stephentoub reviewed Nov 21, 2025

View reviewed changes

		ModelContextProtocol.Protocol.Implementation name is too generic. Lots of things in this namespace named poorly. That might be OK if users don't have to deal with them, but it looks like they do.


		ModelClient.CompleteAsync - naming seems off. Maybe "GetCompletion"




		ModelClient - Enumerate\* vs List\* - why have these redundant methods that return the same thing just one wrapping. Can we just return IAE and have folks materialize that?




		ModelClient.EnumerateToolsAsync - why accept JsonSerializerOptions on this call, it doesn't feel like something tied to enumerating tools. Would it make more sense to make this a property on the client?




		SetLoggingLevel(LogLevel, CancellationToken) vs SetLoggingLevel(LoggingLevel, CancellationToken) -- WHY? Don't make these overloads, instead choose one then make conversions on the types.




		SubscribeToResourceAsync - how are the notifications delivered? This method confused me as I don't understand what it's supposed to do. Might just need to research more.


		TextContentBlock doesn't override ToString -- to get any of the data returned I need to get at protocol types, which feels wrong since those are all "raw" and not designed surface area.

		Same is true for TextResourceContents. The entire content model seems pretty rough - might be better to unify on MEAI content types.




		Odd that ResourceContents is plural, but ContentBlock is not. Why do we even need to different sets of types for these?




		Similarly ReadResourceResult has Contents, while CallToolResult has Content, but both are ILists.

		BlobResourceContents exposes content as a normal string, which means it encoded the UTF-8 to a string, creating work for GC. Instead it should keep the UTF8 contents, and lazily decode that from base64 UTF8 to bytes (either as stream, or byte array). The same problem exists with ContentBlock types, where base-64 data is exposed as Unicode strings.


		Prompts expose arguments as types, whereas tools expose arguments as json schema.

Initial feedback on McpClient NO MERGE #1001

Are you sure you want to change the base?

Initial feedback on McpClient NO MERGE #1001

Conversation

ericstj commented Nov 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants