Feedback on McpClient NO MERGE #1001

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

@stephentoub @halter73 @mikekistler @jeffhandley

docs/McpClient-Feedback.md

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.

With an enum like this aren't folks directly calling it with a literal?

I'd expect it would more commonly be read from configuration or otherwise be dynamic rather than being hardcoded to a literal at the call site.

Will scan for any other public surface that uses MCP primitives where we might have an ME/MEAI primitive as well.

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);

This probably needs better docs to connect the two methods at least. I wonder if naming could be improved to make it more obvious that the methods are complimentary.

I think I understand what to pass SubscribeToResourceAsync - that's the resource "path", but no clue what I should pass for method to RegisterNotificationHandler based on those docs. Is it just any of these

csharp-sdk/src/ModelContextProtocol.Core/Protocol/RequestMethods.cs

Line 6 in e6db415

public static class RequestMethods

and the the combination of methods subscribed + resource subscribed to determines when the handler is invoked?

It's unusual that this method has subscribe+unsubscribe, yet RegisterNotificationHandler just returns an IAsyncDisposable. Seems inconsistent.

I also see tests that are specifying Handlers when creating the client.

This probably needs better docs to connect the two methods at least.

We need lots more docs but it does seem this should be high on the list.

Could make Subscribe return an IAsyncDisposable. That could unsubscribe.
Could add an overload to Subscribe that takes a handler and registers it. The dispose from that would both unsubscribe and unregister.
@ericstj to file an issue

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)

I see - do we want to tell folks this is the preferred way to deal with content? I presume we still want the MCP content types to work (without using protocol types).

I see - do we want to tell folks this is the preferred way to deal with content?

In what context? If you're just coding against the MCP library directly, you'd typically use the MCP types. You'd use the conversion utilities when needing to interoperate across library boundaries. For example, the McpClientTool's AIFunction.InvokeAsync override converts the MCP types to AIContent so that other components in an IChatClient pipeline can consume the content readily.

docs/McpClient-Feedback.md

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.

Sure, but both are describing typing of arguments on the same McpClient - it feels unusual that they have different technologies for the same/similar task.

Can you elaborate? What do you mean by "Prompts expose arguments as types"? There's no type information for prompts in the MCP specification, e.g.

csharp-sdk/src/ModelContextProtocol.Core/Protocol/PromptArgument.cs

Lines 18 to 47 in 136755f

public sealed class PromptArgument : IBaseMetadata

{

/// <inheritdoc />

[JsonPropertyName("name")]

public required string Name { get; set; }

/// <inheritdoc />

[JsonPropertyName("title")]

public string? Title { get; set; }

/// <summary>

/// Gets or sets a human-readable description of the argument's purpose and expected values.

/// </summary>

/// <remarks>

/// This description helps developers understand what information should be provided

/// for this argument and how it will affect the generated prompt.

/// </remarks>

[JsonPropertyName("description")]

public string? Description { get; set; }

/// <summary>

/// Gets or sets a value that indicates whether this argument must be provided when requesting the prompt.

/// </summary>

/// <remarks>

/// When set to <see langword="true"/>, the client must include this argument when making a <see cref="RequestMethods.PromptsGet"/> request.

/// If a required argument is missing, the server should respond with an error.

/// </remarks>

[JsonPropertyName("required")]

public bool? Required { get; set; }

}

. In contrast, tools in the MCP specification expose an input schema for all parameters, e.g.

csharp-sdk/src/ModelContextProtocol.Core/Protocol/Tool.cs

Line 56 in 136755f

public JsonElement InputSchema

. They're different in the MCP library because they're different in the MCP spec/protocol.

ericstj · 2025-11-25T16:48:49Z

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.
+
+
+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.


@jeffhandley -- here's an example of one of the "need to use Protocols" to do a basic thing. I didn't do a deep scrub of everything, but this one was a very basic scenario

What is this proposing? Not exposing any of the of the protocol types in various APIs?

To use these we have to downcast. There is no DebuggerDisplay to help folks understand what they have in the IDE. We could make these more closely match the pattern used in AIContent. @ericstj to file issue - DebuggerDisplay and ToString.

ericstj · 2025-11-25T16:50:38Z

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.


This seems like something we control and ought to resolve before going stable.

@ericstj to open an issue for properties that don't force folks to decode a string. Expose raw, and lazy byte[]

docs/McpClient-Feedback.md

Initial feedback on McpClient NO MERGE

fd7ab10

stephentoub reviewed Nov 21, 2025

View reviewed changes

ericstj commented Nov 25, 2025

View reviewed changes

docs/McpClient-Feedback.md Show resolved Hide resolved

ericstj changed the title ~~Initial feedback on McpClient NO MERGE~~ Feedback on McpClient NO MERGE Dec 2, 2025

jeffhandley assigned ericstj Dec 2, 2025

jeffhandley requested review from halter73 and mikekistler December 2, 2025 18:44

jeffhandley added this to the Stable public API milestone Dec 2, 2025




		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.

		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.

	public sealed class PromptArgument : IBaseMetadata
	{
	/// <inheritdoc />
	[JsonPropertyName("name")]
	public required string Name { get; set; }

	/// <inheritdoc />
	[JsonPropertyName("title")]
	public string? Title { get; set; }

	/// <summary>
	/// Gets or sets a human-readable description of the argument's purpose and expected values.
	/// </summary>
	/// <remarks>
	/// This description helps developers understand what information should be provided
	/// for this argument and how it will affect the generated prompt.
	/// </remarks>
	[JsonPropertyName("description")]
	public string? Description { get; set; }

	/// <summary>
	/// Gets or sets a value that indicates whether this argument must be provided when requesting the prompt.
	/// </summary>
	/// <remarks>
	/// When set to <see langword="true"/>, the client must include this argument when making a <see cref="RequestMethods.PromptsGet"/> request.
	/// If a required argument is missing, the server should respond with an error.
	/// </remarks>
	[JsonPropertyName("required")]
	public bool? Required { get; set; }
	}

Feedback on McpClient NO MERGE #1001

Are you sure you want to change the base?

Feedback on McpClient NO MERGE #1001

Conversation

ericstj commented Nov 20, 2025

Uh oh!

Uh oh!

Uh oh!

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!

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants