-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix fundamental schema issue with required fields defined incorrectly on JsonSchemaType #480
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
Conversation
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.
Found a couple of places where isPropertyRequired helper can be used.
Co-authored-by: Cliff Hall <[email protected]>
Co-authored-by: Cliff Hall <[email protected]>
pcarleton
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.
Auth debugger changes lgtm!
|
Do we have a goto in-project example, in the SDK or servers repo that could be used to exercise the various schema-related stuff like |
I want to put together a little Cloudflare Worker that can take in a generic tool schema and then generate an unauthenticated MCP Server URL that we can share for testing purposes. Just haven't had the time 😢 |
That's cool and useful, and don't want to dissuade you from doing so. But I'm mainly looking for in-project stuff that we maintain with no external dependencies. |
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.
The code looks good and the tests pass. Not going to hold it up for want of a server that has its own tests. This needs to go in.
Fixes a critical schema architecture problem identified by @max-stytch where the
JsonSchemaTypedefinition violated the JSON Schema specification.JsonSchemaTypeincorrectly definedrequiredasbooleanon individual properties instead ofstring[]array on parent object schema, leading to unexpected behavior in some tools testing scenarios and several open issues.Motivation and Context
Related observed issues:
prop.requiredlogic throughout the codebaseHow Has This Been Tested?
Did some basic testing but still working on more methodically testing the different scenarios. Thought it would be worth opening now for visibility's sake.
Breaking Changes
Technically yes due to internal API changes, however these all restore the correct behavior behind the scenes and don't require any changes on the user side:
generateDefaultValue()function signature changed to includepropertyNameandparentSchemaparametersJsonSchemaType.requiredchanged frombooleantostring[]prop.requiredlogic needed updatesTypes of changes
Checklist