Validate and filter inputSchema for McpServerTool so it is spec compliant

[PederHP]

Fixes #342

Tools were returned like this when AIFunction jsonSchema was used directly:

{
  "name": "Echo",
  "description": "Echoes the message back to the client.",
  "inputSchema": {
    "type": "object",
    "properties": {
      "message": {
        "type": "string"
      },      
      "title": "Echo",
      "description": "Echoes the message back to the client.",
    },
    "required": ["message"]
  }
}

But the spec looks like this:

  /**
   * A JSON Schema object defining the expected parameters for the tool.
   */
  inputSchema: {
    type: "object";
    properties?: { [key: string]: object };
    required?: string[];
  };

Which made some clients fail to list and/or call the tools.

Changes are validate the json according to the schema, instead of accepting additional properties, and to filter the AIFunction jsonSchema in the Create.

The above tool will now look like this:

{
  "name": "Echo",
  "description": "Echoes the message back to the client.",
  "inputSchema": {
    "type": "object",
    "properties": {
      "message": {
        "type": "string"
      }
    },
    "required": ["message"]
  }
}

Edit: See my comments below. This is likely not the source of the GitHub Copilot error I encountered. But it's still a valid topic. Even if the MCP schema is open / permissible, it might be confusing to developers that the title and description properties are duplicated in this manner. In other words, I might not be last not person to end up on a wild goose chase when something else causes an error related to the schema, when seeing this discrepancy from other SDKs. So I still think we should consider an elegant way of cleaning up the AIFunction based JSON. But I'll await the resolution of the open questions, and then rework the PR or close it depending on the outcome.

[Copilot]

Pull Request Overview

This PR ensures that the inputSchema for McpServerTool adheres strictly to the spec by filtering out disallowed properties (such as "title" and "description").

• Updated test cases to validate that only allowed properties are present.
• Modified McpJsonUtilities to disallow extra properties.
• Implemented FilterJsonSchema to produce a spec-compliant schema.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
tests/ModelContextProtocol.Tests/Protocol/ProtocolTypeTests.cs	Adjusted inline test cases to confirm that extra properties cause schema rejection.
src/ModelContextProtocol/Utils/Json/McpJsonUtilities.cs	Updated property validation to allow only "type", "properties", and "required".
src/ModelContextProtocol/Server/AIFunctionMcpServerTool.cs	Added FilterJsonSchema to filter out non-allowed properties from the JSON schema.

Comments suppressed due to low confidence (2)

tests/ModelContextProtocol.Tests/Protocol/ProtocolTypeTests.cs:33

• Consider adding additional test cases that cover scenarios with multiple extra properties to ensure that filtering behaves as expected in all cases.

[InlineData("{"type":"object", "title": "MyAwesomeTool", "description": "It's awesome!", "properties": {}, "required" : ["NotAParam"] }")]

src/ModelContextProtocol/Server/AIFunctionMcpServerTool.cs:224

• Consider expanding unit test coverage for the FilterJsonSchema method with diverse JSON inputs to verify that all unwanted properties are effectively removed.

/// Filters a JsonElement containing a schema to only include allowed properties: "type", "properties", and "required".

[halter73]

I see that a lot of tests are failing due to our client now rejecting invalid schemas sent over the wire. I think we should probably update most of those tests to have the server generate valid schemas, but I also think it'd make sense to be a little more tolerant in what we accept. Otherwise, our clients will break with our own servers from just one preview ago. The fact that most other clients don't blow up with what our server is doing probably shows this is the right approach.

[halter73]

Now we need to update the return false; at the end of the loop to return true if the "type" property was found and know unknown properties were found.

[halter73]

I think it would probably be better to use JsonObject to build up the JSON DOM and then convert that to a JsonElement using jsonNode.Deserialize<JsonElement>(typeInfo).

https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/use-dom#create-a-jsonnode-dom-with-object-initializers-and-make-changes

@eiriktsarpalis is probably the best person to ask about the right approach here though.

Edit: Could we also use AIFunctionFactoryOptions.JsonSchemaCreateOptions.TransformSchemaNode for the AIFunctions we create ourselves from Delegates and the like and avoid generating the intermediate schema in that case?

[eiriktsarpalis]

Edit: Could we also use AIFunctionFactoryOptions.JsonSchemaCreateOptions.TransformSchemaNode for the AIFunctions we create ourselves from Delegates and the like and avoid generating the intermediate schema in that case?

Yep, that's the best way to achieve that. Although per my remarks in #343 (comment) this should most likely be left to individual implementers as I think the metaschema is not meant to be interpreted literally.

[eiriktsarpalis]

But the spec looks like this:

  /**
   * A JSON Schema object defining the expected parameters for the tool.
   */
  inputSchema: {
    type: "object";
    properties?: { [key: string]: object };
    required?: string[];
  };

I believe that the intention of the spec authors here is to provide a concise proxy for the metaschema as opposed to mandating this shape for every input schema. I recall there being a discussion about this in the spec repo (which I unfortunately can't locate right now) where it was said that any valid JSON schema is also valid under MCP. @dsp-ant or @jspahrsummers can maybe provide clarification on this.

[PederHP]

I just discovered the issue I encountered with GitHub Copilot might not be caused by the lack of spec compliance:

https://github.com/microsoft/vscode/blob/8d4b26f8379177c7fd383adbfee40a73406d505d/src/vs/workbench/contrib/mcp/common/mcpServer.ts#L383

https://github.com/microsoft/vscode/blob/8d4b26f8379177c7fd383adbfee40a73406d505d/src/vs/workbench/contrib/mcp/common/mcpServer.ts#L337

This is the error I have in my repro (GitHub Copilot output):

Tool `GetLocation` has invalid JSON parameters:
	- Unable to load schema from 'https://json-schema.org/draft-07/schema': . (at /)
	- Schema: {"title":"GetLocation","description":"Get the location of the user.","type":"object","properties":{}}

(same for other tools)

It might be caused by not being able to load the schema due to a proxy issue or similar. I just noticed the schema and fixated on that.

Still relevant to figure out if we're allowed to have additional properties in the inputSchema.

[PederHP]

I see that a lot of tests are failing due to our client now rejecting invalid schemas sent over the wire. I think we should probably update most of those tests to have the server generate valid schemas, but I also think it'd make sense to be a little more tolerant in what we accept. Otherwise, our clients will break with our own servers from just one preview ago. The fact that most other clients don't blow up with what our server is doing probably shows this is the right approach.

Yeah I didn't intend to fail on the client side from validating the schemas. Only on the server side - as a guard against custom implementations.

And as per below discussion and comments by @eiriktsarpalis it all depends on whether the inputSchema is permissible or not. In hindsight I recall seeing the discussion about the current MCP schema.ts schema being a compromise because expressing a full JSON schema isn't really feasible. But perhaps a comment should be made in the specification repo schema.ts that this is intended to allow a full JSON schema - and clients must not fail because of strict validation.

[jspahrsummers]

I believe that the intention of the spec authors here is to provide a concise proxy for the metaschema as opposed to mandating this shape for every input schema. I recall there being a discussion about this in the spec repo (which I unfortunately can't locate right now) where it was said that any valid JSON schema is also valid under MCP. @dsp-ant or @jspahrsummers can maybe provide clarification on this.

Yes, this is meant to support full JSON Schema. It's not a principled stance that we avoid defining it here, just not sure it's easy to do practically. Totally open to ideas here.

[eiriktsarpalis]

Should we update the comment in the spec to say this explicitly? Seems we're being impacted by clients who are treating the current schema a bit too literally.