-
Notifications
You must be signed in to change notification settings - Fork 528
feat: add structured output support for MCP tools #357
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- Add JsonSchemaValidator interface and DefaultJsonSchemaValidator implementation - Extend Tool schema to support outputSchema field for defining expected output structure - Add structuredContent field to CallToolResult for validated structured responses - Implement automatic validation of tool outputs against their defined schemas - Add comprehensive test coverage for structured output validation scenarios - Add json-schema-validator and json-unit-assertj dependencies for validation and testing - Update McpServer builders to accept custom JsonSchemaValidator instances - Ensure backward compatibility with existing tools without output schemas This implements the MCP specification requirement that tools with output schemas must provide structured results conforming to those schemas, with automatic validation and error handling for non-conforming outputs. https://modelcontextprotocol.io/specification/2025-06-18/server/tools#structured-content Signed-off-by: Christian Tzolov <[email protected]>
.containsEntry("operation", "2 + 3") | ||
.containsEntry("timestamp", "2024-01-01T10:00:00Z"); | ||
} | ||
else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Didn't quite understand why this is necessary since we are hardcoding the structured content in the above result.
mcp/pom.xml
Outdated
@@ -83,6 +83,15 @@ | |||
<artifactId>reactor-core</artifactId> | |||
</dependency> | |||
|
|||
<!-- Json validator dependency. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment doesn't seem right since this will always be a dependency for the default implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, this comment is incorrect.
I'm conscious of adding new dependencies to the MCP core. Initially I thought we could make it optional, but that would require moving the DefaultJsonSchemaValidator to a separate module or using reflection magic. This feels like overkill for now - we can always refactor later if needed.
// } | ||
// } | ||
return new CallToolResult( | ||
"Tool call with non-empty outputSchema must have a result with structured content", true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Error message could be reworded for the client. Something like "Response missing structured content which is expected when calling tool with non-empty outputSchema"
|
||
// Check if validation passed | ||
if (!validationResult.isEmpty()) { | ||
logger.warn("Validation failed: structuredContent does not match tool outputSchema. " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Since we already have validation failed in the error StructuredOutputCallToolHandler, maybe we can omit it in this message.
// Map<String, Object> structuredOutput = new | ||
// ObjectMapper().readValue( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do implicitly convert here, we should reuse the ObjectMapper
between operations, since ObjectMapper
instances have internal caches and are fairly expensive memory-wise.
// if (!Utils.isEmpty(result.content())) { | ||
// // TODO If the tesult.content() contains json text we can try to | ||
// // convert it into a structured content (Experimental) | ||
// var tc = result.content().stream().filter(c -> c instanceof | ||
// McpSchema.TextContent).findFirst(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ambivalent on this, I think it's surprising behavior with nontrivial overhead - IMO we should just pass through the raw result and allow consumers to migrate on their own.
I assume this is a mirror of converting structuredOutput
into a TextContent
block, but I think it's too expensive to justify.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree. Thats why I commented it out but forgot to remove it.
schemaNode.put("additionalProperties", false); | ||
} | ||
|
||
JsonSchema jsonSchema = JsonSchemaFactory.getInstance(SpecVersion.VersionFlag.V202012) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth peeking inside to see what this is doing, in case it's worth caching this as a field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The topic of schema caching is important.
I will add some basic support.
|
||
private static final Logger logger = LoggerFactory.getLogger(DefaultJsonSchemaValidator.class); | ||
|
||
private ObjectMapper objectMapper = new ObjectMapper(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: assigned value here appears redundant with default constructor
…ependencies - Add schema caching to DefaultJsonSchemaValidator for better performance - Make validator fields final and improve constructor design Signed-off-by: Christian Tzolov <[email protected]>
} | ||
|
||
// Validate the result against the output schema | ||
var validation = this.jsonSchemaValidator.validate(outputSchema, result.structuredContent()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we need to make the sever json schema validation optional?
Would validation enabled by default have some regression impact on existing MCP server implementations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There shouldn't be any regression since none of the existing servers will have tools with output schemas.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see a niche use case for making it optional, for example if you're writing an MCP server proxy and want to force the upstream server to handle validation instead.
Also think it's fine to enable by default.
} | ||
|
||
@Override | ||
public ValidationResponse validate(Map<String, Object> schema, Map<String, Object> structuredContent) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should extend the signature to include an unique schemaName
. Then we can expose a method purgeCache(schemaName)
or similar
private final JsonSchemaFactory schemaFactory; | ||
|
||
// TODO: Implement a strategy to purge the cache (TTL, size limit, etc.) | ||
private final ConcurrentHashMap<String, JsonSchema> schemaCache; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe some simple, interval based purge all strategy would be good enough.
Signed-off-by: Christian Tzolov <[email protected]>
I moved the JsonSchemaValidator classes from server to spec package. So we can reuse them for other validation cases. |
* @throws IllegalArgumentException if jsonSchemaValidator is null | ||
*/ | ||
public AsyncSpecification jsonSchemaValidator(JsonSchemaValidator jsonSchemaValidator) { | ||
Assert.notNull(jsonSchemaValidator, "JsonSchemaValidator must not be null"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: might be nice to add @Nullable
and then use the JetBrains @Contract
annotation in the Assert
class to have control flow-aware null warnings in the IDE. Not urgent or required though.
Part of #285
This PR implements compulsory server-side validation for MCP tools with output schemas, adding support for JSON schema validation of tool outputs according to the MCP specification. Tools can now define output schemas that are automatically validated on the server side, ensuring structured responses conform to expected formats before being sent to clients.
Spec Reference: https://modelcontextprotocol.io/specification/2025-06-18/server/tools#structured-content
Motivation and Context
The MCP specification requires that servers with tools that have output schemas must provide structured results that conform to those schemas. This change implements that mandatory server-side validation requirement by:
How Has This Been Tested?
Test coverage has been added across all server transport implementations:
WebFluxSseIntegrationTests.java
- Added 4 new test methods for structured output validationWebMvcSseIntegrationTests.java
- Added 4 new test methods for structured output validationHttpServletSseServerTransportProviderIntegrationTests.java
- Added 4 new test methods for structured output validationMcpSchemaTests.java
- Added 5 new test methods for Tool schema serialization/deserialization with output schemasBreaking Changes
No breaking changes. This is a backward-compatible addition:
outputSchema
andstructuredContent
fields are optionalJsonSchemaValidator
is provided automatically for server-side validationTypes of changes
Checklist
Additional context
Implementation Details:
JsonSchemaValidator
interface withDefaultJsonSchemaValidator
implementation usingjson-schema-validator
libraryTool
record with optionaloutputSchema
fieldCallToolResult
record with optionalstructuredContent
fieldStructuredOutputCallToolHandler
wrapper that validates outputs automatically on the server sidejson-unit-assertj
for JSON assertionsDesign Decisions:
JsonSchemaValidator
implementations can be provided via builder methodsDependencies Added:
json-schema-validator
(1.5.7) for JSON schema validation on the serverjson-unit-assertj
for enhanced JSON testing capabilitiesScope:
This PR focuses exclusively on server-side validation as required by the MCP specification. Client-side validation is not included in this implementation.