Audit protocol types #892

MackinnonBuck · 2025-10-16T23:38:21Z

Summary

Audits and standardizes MCP protocol types for consistency.

Fixes #519

Description

There are several inconsistencies across protocol types, including:

init vs. set
required vs. default property values
Read-only vs. mutable collection properties
enum vs. string
JsonNode vs. JsonElement vs. IDictionary<string, JsonElement> vs. IDictionary<string, object>

This PR applies the following set of conventions to improve consistency:

Property Mutability

Guideline: Always use set - no init-only properties.

Rationale:

If we're going to standardize on either set or init, it's easier to standardize on set because doing so is non-breaking
Using init means that if we do want to mutate a property after initialization, we need to clone the its containing object, which can be error-prone and isn't great for perf
It's not clear (to me at least) that we actually rely on the immutability of any properties in protocol types

Required Properties

Guideline: Use required when the spec indicates a property is required, unless a clear, safe default exists.

Rationale:

Reduces initialization errors when mapping to/from protocol types
Aligns better with the MCP schema

Safe defaults include:

Empty collections
The value returned by calling a type's parameterless constructor

Avoid defaults such as:

The empty string ("") - this is rarely the expected value for a "required" property
An enum value we arbitrary decide should be the default

Collections

Guideline: Prefer IList<T> / IDictionary<TKey, TValue> over IReadOnlyList<T> / IReadOnlyDictionary<TKey, TValue>.

Rationale:

Reduces unnecessary cloning if mutation is needed
It's not clear that we actually rely on collections in any protocol types being read-only

Open Questions

The following inconsistencies remain for further discussion:

enum vs. string: properties

The MCP schema defines some properties as unions of string values.

enum offers strong typing
string is more future-proof (e.g., if new values are added)

Existing examples of each:

enum: ContextInclusion, Role
string: ElicitResult.Action

JSON-like Structures:

The use of JsonNode, JsonElement, IDictionary<string, JsonElement>, and IDictionary<string, object> varies.

These each have different pros and cons, and I'm not sure we necessarily want or need to standardize on a single one.
We should define principles for when each is appropriate and align existing types accordingly.

Additional Notes

None of these need to be hard and fast rules. If there's a compelling reason to deviate, it should be fine to do so.
This PR also addresses a few smaller inconsistencies not listed above.

src/ModelContextProtocol.Core/Protocol/CreateMessageRequestParams.cs

src/ModelContextProtocol.Core/Protocol/JsonRpcRequest.cs

src/ModelContextProtocol.Core/Server/McpServer.Methods.cs

MackinnonBuck · 2025-10-16T23:45:52Z

src/ModelContextProtocol.Core/AIContentExtensions.cs

                {
                    Blob = dataContent.Base64Data.ToString(),
                    MimeType = dataContent.MediaType,
+                    Uri = string.Empty,


This was implicitly string.Empty before, but now it's explicit. This PR doesn't introduce a behavioral change here, but we should decide whether the empty string should really be used here, as it's not actually a valid URI. It would also be strange to use dataContent.Uri, because then the data is specified in two places (once in Blob and once in Uri).

then the data is specified in two places

It doesn't sound bad, DataContent.Uri is just $"data:{MediaType};base64,{base64}" while MimeType and Blob are there to access those parts without having to parse the Uri. It would also align with DataCotent.

Realizing that there's also TextResourceContents, I wonder if Uri should be abstract and Blob and Text types should provide an implementation.

public sealed class TextContent { public override string Uri => $"data:text/plain;base64,{Base64Url.EncodeToString(Encoding.UTF8.GetBytes(Text))}" }

then the data is specified in two places

It doesn't sound bad, DataContent.Uri is just $"data:{MediaType};base64,{base64}" while MimeType and Blob are there to access those parts without having to parse the Uri. It would also align with DataCotent.

The DataContent type in Microsoft.Extensions.AI is different in that the Uri can only be a data URI, and the Data property is [JsonIgnore]d, so the duplication only exists in-memory. However, the MCP BlobResourceContents type has both uri and blob as separate, required properties. This means that if you treat BlobResourceContents.uri as a data URI, you'll be including the data twice in JSON, which would be unfortunate IMO.

The spec also doesn't seem to acknowledge the possibility of the uri property being a data URI (although it doesn't explicitly disallow it). The examples all show file://, and only https://, file://, and git:// are listed as examples of common URI schemes.

So, the purpose of BlobResourceContents.uri seems to be identifying the data, which is different than DataContent.Uri, whose purposes is containing the data itself. This is what creates the dilemma when mapping between these two types.

Realizing that there's also TextResourceContents, I wonder if Uri should be abstract and Blob and Text types should provide an implementation

This would disallow TextResourceContents and BlobResourceContents from having anything other than a data URI, right?

src/ModelContextProtocol.Core/McpSessionHandler.cs

src/ModelContextProtocol.Core/Protocol/Argument.cs

eiriktsarpalis · 2025-10-17T09:35:52Z

Guideline: Always use set - no init-only properties.

Rationale:

If we're going to standardize on either set or init, it's easier to standardize on set because doing so is non-breaking

Using init means that if we do want to mutate a property after initialization, we need to clone the its containing object, which can be error-prone and isn't great for perf

It's not clear (to me at least) that we actually rely on the immutability of any properties in protocol types

For context, the presence of init properties is a holdover from when most of the models were record types, where the language supports persistable updates using with expressions. Since we decided to move away from records going from init to set sounds reasonable.

src/ModelContextProtocol.Core/Protocol/CreateMessageRequestParams.cs

tests/ModelContextProtocol.Tests/Server/McpServerResourceTests.cs

halter73 · 2025-10-17T22:44:47Z

The use of JsonNode, JsonElement, IDictionary<string, JsonElement>, and IDictionary<string, object> varies.

These each have different pros and cons, and I'm not sure we necessarily want or need to standardize on a single one.

We should define principles for when each is appropriate and align existing types accordingly.

What are our principles around this? I know JsonNode is mutable and JsonElement is not, but why is JsonNode use for params and results and JsonElement or IDictionary<string, JsonElement> seemingly used everywhere else particularly considering these properties are settable? And what's up with IDictionary<string, object> just for experimental capabilities?

MackinnonBuck · 2025-10-20T16:31:41Z

What are our principles around this? I know JsonNode is mutable and JsonElement is not, but why is JsonNode use for params and results and JsonElement or IDictionary<string, JsonElement> seemingly used everywhere else particularly considering these properties are settable? And what's up with IDictionary<string, object> just for experimental capabilities?

Exactly what I'm wondering too 🙂 Maybe we could get an STJ expert to weigh in on this?

Edit: @eiriktsarpalis , do you have any thoughts about this?

Copilot · 2025-10-29T17:55:18Z

@MackinnonBuck I've opened a new pull request, #923, to work on those changes. Once the pull request is ready, I'll request review from you.

eiriktsarpalis · 2025-10-29T18:31:27Z

What are our principles around this? I know JsonNode is mutable and JsonElement is not, but why is JsonNode use for params and results and JsonElement or IDictionary<string, JsonElement> seemingly used everywhere else particularly considering these properties are settable? And what's up with IDictionary<string, object> just for experimental capabilities?

Exactly what I'm wondering too 🙂 Maybe we could get an STJ expert to weigh in on this?

Edit: @eiriktsarpalis , do you have any thoughts about this?

JsonElement is a struct with default(JsonElement) representing an invalid value (similar to ImmutableArray<T>). This makes it awkward to use in properties that are optional, typically requiring JsonElement? which adds its own complexities into the mix. This is why I originally opted for JsonNode? to encode parameters, which is a class with null naturally mapping to JSON null.
When such concerns aren't present JsonElement is the natural choice since it's more efficient (it's basically just a annotated buffer)..
IDictionary<string, object> is used typically in hybrid situations where you want values to both be JsonElement or potentially other .NET values that we are able to serialize into JSON. This is how AIFunction has been designed to work.

* Initial plan * Add DefaultSamplingMaxTokens property to McpServerOptions Co-authored-by: MackinnonBuck <[email protected]> * Add test to verify DefaultSamplingMaxTokens is respected Co-authored-by: MackinnonBuck <[email protected]> * Merge test into existing SampleAsync_Messages_Forwards_To_McpServer_SendRequestAsync Co-authored-by: MackinnonBuck <[email protected]> * Update src/ModelContextProtocol.Core/Server/McpServerOptions.cs --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: MackinnonBuck <[email protected]> Co-authored-by: Mackinnon Buck <[email protected]>

MackinnonBuck · 2025-10-31T23:06:15Z

Thanks for the clarification, @eiriktsarpalis!

I'll leave the properties representing JSON objects as-is for this PR, then. Nothing jumps out at me as obviously using the wrong type.

Audit

fd5d22b

MackinnonBuck requested review from halter73 and stephentoub October 16, 2025 23:38

MackinnonBuck commented Oct 16, 2025

View reviewed changes

Merge branch 'main' into mbuck/audit-protocol-types

9843088

eiriktsarpalis reviewed Oct 17, 2025

View reviewed changes

src/ModelContextProtocol.Core/Protocol/Argument.cs Show resolved Hide resolved

eiriktsarpalis reviewed Oct 17, 2025

View reviewed changes

src/ModelContextProtocol.Core/Protocol/CreateMessageRequestParams.cs Show resolved Hide resolved

eiriktsarpalis reviewed Oct 17, 2025

View reviewed changes

tests/ModelContextProtocol.Tests/Server/McpServerResourceTests.cs Outdated Show resolved Hide resolved

PR feedback

1722cb5

Fix StreamableHttpServerConformanceTests

f4cbc98

Copilot AI mentioned this pull request Oct 29, 2025

Add configurable default for sampling MaxTokens #923

Merged

5 tasks

jozkee mentioned this pull request Oct 30, 2025

Allow ChatOptions.ConversationId to be an OpenAI conversation ID with Responses dotnet/extensions#6960

Merged

Audit protocol types #892

Are you sure you want to change the base?

Audit protocol types #892

Conversation

MackinnonBuck commented Oct 16, 2025

Summary

Description

Property Mutability

Required Properties

Collections

Open Questions

Additional Notes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MackinnonBuck Oct 16, 2025

Choose a reason for hiding this comment

Uh oh!

jozkee Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

jozkee Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

MackinnonBuck Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

eiriktsarpalis commented Oct 17, 2025

Uh oh!

Uh oh!

Uh oh!

halter73 commented Oct 17, 2025

Uh oh!

MackinnonBuck commented Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI commented Oct 29, 2025

Uh oh!

eiriktsarpalis commented Oct 29, 2025

Uh oh!

MackinnonBuck commented Oct 31, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

MackinnonBuck commented Oct 20, 2025 •

edited

Loading