-
Notifications
You must be signed in to change notification settings - Fork 981
Fix $ref resolution in request schema properties before validation #902
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?
Fix $ref resolution in request schema properties before validation #902
Conversation
|
Hi @vincent0426 thanks for this, but we just merged a change #889 last week that addresses this specific issue. |
|
Hi @cliffhall, thanks for the response, but I think they address different issues. This PR fixes request handling (e.g., schemas sent by elicitation), while #889 addresses tool rendering. I’m still seeing the error on the latest version which fails before the handler, during validation against
|
cliffhall
left a comment
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.
Hoping we can avoid adding even more logic to useConnection by extracting the bulk of the change to schemaUtils.ts
37b99b3 to
b000fc2
Compare
cliffhall
left a comment
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.
Hoping we could extend this to cover refs in $defs
| ...message.params, | ||
| requestedSchema: { | ||
| ...requestedSchema, | ||
| properties: Object.fromEntries( |
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.
Could this also resolve $refs that point to $defs or definitions? Seems like a perfect place to do so.
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 added a test resolves $ref references to $defs in requestedSchema to verify that resolveRef already handles this. I believe it uses a generic path that resolves defs as well.
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.
That test seems to be failing at the moment. https://github.com/modelcontextprotocol/inspector/actions/runs/19337854835/job/55324774030?pr=902
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.
ah yeah fixed
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.
But is there a test that shows we can resolve $defs? Was this working before?
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 there is a test resolves $ref references to $defs in requestedSchema
...
$ref: "#/$defs/UserInput",
...
43fa657 to
c1f18a8
Compare

Added
$refresolution forrequestedSchema.propertiesbefore SDK validation. When incoming requests contain$refreferences in their schema properties, they are automatically resolved before reaching the SDK's validation layer.Before the fix
After

Motivation and Context
While playing around with python enum elicitation requests, when MCP servers send elicitation requests (or any requests) with
$refreferences inrequestedSchema.properties(e.g.,{ $ref: "#/properties/nameDef" }), the SDK attempts to validate these schemas before our handlers can process them. Unresolved$refreferences can cause validation failures.This change intercepts incoming messages at the transport level (before validation) and resolves all
$refreferences inrequestedSchema.properties, ensuring the SDK receives fully resolved schemas that validate correctly.How Has This Been Tested?
Added a new unit test:
$refinrequestedSchema.propertiesBreaking Changes
Types of changes
Checklist
Additional context