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.

MackinnonBuck · 2025-10-16T23:39:57Z

src/ModelContextProtocol.Core/Protocol/CreateMessageRequestParams.cs

    /// </remarks>
    [JsonPropertyName("maxTokens")]
-    public int? MaxTokens { get; init; }
+    public required int MaxTokens { get; set; }


This property was an int? despite the schema declaring it as required. Maybe this was intentional. Happy to change back if so.

We should use the schema as our source of truth when defining these model types.

MackinnonBuck · 2025-10-16T23:42:10Z

src/ModelContextProtocol.Core/Protocol/JsonRpcRequest.cs

-    internal JsonRpcRequest WithId(RequestId id)
-    {
-        return new JsonRpcRequest
-        {
-            JsonRpc = JsonRpc,
-            Id = id,
-            Method = Method,
-            Params = Params,
-            Context = Context,
-        };
-    }


I'm not sure whether this method was a workaround for the fact that the inherited Id property was init-only, or if the type was intentionally designed to be immutable. If the latter, maybe we need to reevaluate the decision to standardize on set.

I added this in #330. At the time I was changing the JsonRpcMessage type hierarchy from interfaces and records to be class-based so I could flow the RelatedTransport. I think my logic at the time was that the records and interfaces that implemented IJsonRpcMessage previously were mostly get or init only with the notable exception of JsonRpcRequest.Id, so I changed it to also be init-only for consistency.

Prior to that, we mutated JsonRpcRequest.Id just like this PR does. https://github.com/modelcontextprotocol/csharp-sdk/pull/330/files#diff-3905e6494bc7bc14c472a02121cd4c7f0e054380966f081731bc55a99f84da58

However, even though that wasn't the reason for my change, I agree with @eiriktsarpalis that we should preserve the cloning behavior. I should have added some sort of regression test then. If we didn't clone, I'd worry that it could be a footgun for someone repeatedly sending a JsonRpcRequest using SendRequestAsync.

MackinnonBuck · 2025-10-16T23:43:59Z

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

        {
            Messages = samplingMessages,
-            MaxTokens = options?.MaxOutputTokens,
+            MaxTokens = options?.MaxOutputTokens ?? int.MaxValue,


We should determine whether int.MaxValue is actually a reasonable default value for MaxTokens. Previously, we allowed null as a valid value, but the schema indicates that it's required.

It's not a reasonable value. LLM apis will fail using maxint.

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

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

src/ModelContextProtocol.Core/McpSessionHandler.cs

        if (request.Id.Id is null)
        {
-            request = request.WithId(new RequestId(Interlocked.Increment(ref _lastRequestId)));
+            request.Id = new RequestId(Interlocked.Increment(ref _lastRequestId));


This now mutates the request parameter directly instead of cloning it. I think that's fine, but please correct me if not. This may have just been a workaround for JsonRpcRequest inheriting init-only properties, but I'm not sure.

In order to avoid potential regressions in the scope of this change, I would suggest preserving the original cloning semantics even if done manually.

eiriktsarpalis · 2025-10-17T09:33:49Z

src/ModelContextProtocol.Core/Protocol/Argument.cs

    /// </summary>
    [JsonPropertyName("name")]
-    public string Name { get; set; } = string.Empty;
+    public required string Name { get; set; }


FWIW introducing required is also considered a breaking change.

Yep. And the same is true for other changes made in this PR (e.g., IReadOnlyList<T> -> IList<T>). We'll have to decide which changes are worth their induced breaks.

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.

eiriktsarpalis · 2025-10-17T09:38:23Z

src/ModelContextProtocol.Core/Protocol/CreateMessageRequestParams.cs

    /// </summary>
    [JsonPropertyName("messages")]
-    public required IReadOnlyList<SamplingMessage> Messages { get; init; }
+    public IList<SamplingMessage> Messages { get; set; } = [];


Why was this changed back to optional? Presumably not required by the schema?

My thinking here was that this property is still effectively "required" in that it's not declared as allowing null values. We just no longer require an explicit value on initialization, since a reasonable, non-null default exists ([]).

However, your comment made me realize that removing required actually does make the property "optional" in that we will no longer throw an exception on deserialization if the property is missing from the JSON payload. Maybe this is OK. We also don't appear to be using RespectNullableAnnotations, so the JSON input could still explicitly provide null for this property anyway. So, required alone still isn't enough to enforce that the JSON property has a specified, non-null value.

Curious about your thoughts - do you think it's fine to have a default initial value as a substitute for required?

Or maybe we should consider enabling RespectNullableAnnotations so that we're able to enforce required-ness to a more complete degree?

eiriktsarpalis · 2025-10-17T09:42:06Z

tests/ModelContextProtocol.Tests/Server/McpServerResourceTests.cs

        {
            Assert.Same(mockServer.Object, server);
-            return new ReadResourceResult { Contents = [new TextResourceContents { Text = "hello" }] };
+            return new ReadResourceResult { Contents = [new TextResourceContents { Text = "hello", Uri = string.Empty }] };


I have a potentially naive question directed to the wider audience of this PR. In general, why do we prefer using string.Empty over ""? They both reference the same instance anyway.

We don't. It's purely style which is chosen.

Why don't we go for the shorter and easier to read alternative then? :-)

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?

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

Audit protocol types #892

Are you sure you want to change the base?

Audit protocol types #892

Uh oh!

Conversation

MackinnonBuck commented Oct 16, 2025

Summary

Description

Property Mutability

Required Properties

Collections

Open Questions

Additional Notes

Uh oh!

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

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!

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

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!

eiriktsarpalis commented Oct 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

halter73 commented Oct 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

MackinnonBuck Oct 16, 2025 •

edited

Loading

MackinnonBuck Oct 16, 2025 •

edited

Loading

MackinnonBuck Oct 17, 2025 •

edited

Loading