Skip to content

Comments

Fix CreateMessageRequestParams.Metadata type to JsonObject#1354

Merged
stephentoub merged 4 commits intomainfrom
copilot/fix-metadata-jsonobject-type
Feb 23, 2026
Merged

Fix CreateMessageRequestParams.Metadata type to JsonObject#1354
stephentoub merged 4 commits intomainfrom
copilot/fix-metadata-jsonobject-type

Conversation

Copy link
Contributor

Copilot AI commented Feb 22, 2026

  • Confirm MCP spec defines CreateMessageRequest.params.metadata as object type ({ [key: string]: unknown })
  • Confirm all other _meta/Meta properties in C# SDK use JsonObject?
  • Change CreateMessageRequestParams.Metadata from JsonElement? to JsonObject?
  • Add serialization roundtrip test for Metadata in CreateMessageRequestParamsTests
  • Build and test to verify no regressions

🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.

…nObject?

Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
@stephentoub stephentoub added the breaking-change This issue or PR introduces a breaking change label Feb 22, 2026
Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
@stephentoub stephentoub changed the title [WIP] Fix CreateMessageRequestParams.Metadata type to JsonObject Fix CreateMessageRequestParams.Metadata type to JsonObject Feb 22, 2026
Copy link
Contributor

@halter73 halter73 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, I'm still a little unsure when we use JsonObject vs JsonElement vs IDictionary<string, JsonElement>. I remember discussing this back in the "Audit protocol types" PR in #892, but I don't think JsonObject was in the mix then.

This does align with what we did for _meta in #1341, but I don't really know why we did that. I guess this is sorta similar, so this is arguably more consistent. I don't know why _meta was changed to be different than all the other DTO properties though, and I'm not sure if this is more similar to _meta or other untyped collections in JSON DTOs. I feel like @eiriktsarpalis and/or @MackinnonBuck might be better to review this.

Copy link
Contributor

@halter73 halter73 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving based on this explanation from @stephentoub

JsonObject is the only one of the three that requires it be a JSON object: both JsonElement and JsonNode can represent primitives in addition to objects. JsonObject is also mutable, which means you can treat it like a dictionary and easily add things into it. I think both of those make it appropriate for _meta, which is required by spec to be a JSON object and which from a usability perspective benefits from being able to treat like a dictionary.

CreateMessageRequestParams.metadata isn't _meta, but it has the same constraints, e.g. required by schema to be an object

@stephentoub
Copy link
Contributor

FWIW, I also asked the GH copilot agent with Opus 4.6:

"Analyze all use of JsonObject, JsonNode, and JsonElement in public API. Compare each use where relevant against the MCP 11/25/2025 specification's schema. Then discern a pattern for when the C# SDK uses which type, and look for any outliers that should be fixed because they're not following the pattern."

Here's the relevant part of the response:

The Pattern

Category C# Type Spec Schema Rationale
Metadata containers (_meta, Metadata) JsonObject? "type": "object" with "additionalProperties" Mutable — users construct/modify these programmatically (e.g. adding keys)
Data payloads (structuredContent, input, data) JsonElement / JsonElement? "type": "object" or typeless Immutable — deserialized from wire, passed through, not user-constructed
JSON Schema documents (inputSchema, outputSchema) JsonElement / JsonElement? Inline JSON Schema object Immutable — opaque schema documents
Typed dictionaries (arguments, content) IDictionary? "type": "object" with typed additionalProperties Explicit key-value structure with known value types
JSON-RPC wrappers (Params, Result) JsonNode? "type": "object" with "additionalProperties" Mutable — used for constructing/deconstructing RPC message envelopes

The key distinction is mutability and construction intentJsonObject? is used only where the SDK user needs to build an extensible object (metadata), JsonElement/JsonElement? is used for data that flows through immutably, and JsonNode? is reserved exclusively for the JSON-RPC transport layer.

Outliers Found

Only two properties deviate from this pattern:

  1. CallToolResult.StructuredContent — currently JsonNode?, should be JsonElement? (it's a data payload, not a JSON-RPC wrapper)
  2. CreateMessageRequestParams.Metadata — currently JsonElement?, should be JsonObject? (it's a metadata container, not an immutable payload)

All other properties follow the pattern correctly — including ToolResultContentBlock.StructuredContent (JsonElement?), ToolUseContentBlock.Input (JsonElement), LoggingMessageNotificationParams.Data (JsonElement), and all _meta properties (JsonObject?).

@stephentoub stephentoub merged commit ce2a5e4 into main Feb 23, 2026
10 checks passed
@stephentoub stephentoub deleted the copilot/fix-metadata-jsonobject-type branch February 23, 2026 23:08
@eiriktsarpalis
Copy link
Member

Honestly, I'm still a little unsure when we use JsonObject vs JsonElement vs IDictionary<string, JsonElement>. I remember discussing this back in the "Audit protocol types" PR in #892, but I don't think JsonObject was in the mix then.

There are nuanced differences between the three. JsonObject is a subclass of JsonNode and is essentially the same as IDictionary<string, JsonNode?>. It's effectively a JSON-native way of expressing a property bag. I would personally favor IDictionary<string, JsonNode?> over JsonObject only because you can assign the property any instance of that interface (including JsonObject) without forcing a conversion.

When it comes to the other alternatives, JsonElement can be any JSON value and not just a property bag. Being a struct it also poses problems with default(JsonElement) (an invalid instance) showing up in unitialized properties.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking-change This issue or PR introduces a breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants