-
Notifications
You must be signed in to change notification settings - Fork 1.4k
SEP-1034: Default values for Elicitation Schemas #1096
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
SEP-1034: Default values for Elicitation Schemas #1096
Conversation
commit: |
|
Discussed offline. I think in this approach we're doing the "assume defaults were used" on the server side. The spec says though that "All primitive types support optional default values to provide sensible starting points. Clients that support defaults SHOULD pre-populate form fields with these values." See https://github.com/modelcontextprotocol/modelcontextprotocol/pull/1035/files We should likely move this to the client side instead. |
src/server/elicitation.test.ts
Outdated
| const client = new Client({ name: 'test-client', version: '1.0.0' }, { capabilities: { elicitation: {} } }); | ||
| const client = new Client( | ||
| { name: 'test-client', version: '1.0.0' }, | ||
| { capabilities: { elicitation: {} }, jsonSchemaValidator: validatorProvider } |
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.
we don't need this anymore
src/client/index.ts
Outdated
| // Handle arrays | ||
| if (schema.type === 'array' && Array.isArray(data) && schema.items) { | ||
| const itemsSchema = schema.items as JsonSchemaType | JsonSchemaType[]; | ||
| if (Array.isArray(itemsSchema)) { | ||
| for (let i = 0; i < data.length && i < itemsSchema.length; i++) { | ||
| applyElicitationDefaults(itemsSchema[i], data[i]); | ||
| } | ||
| } else { | ||
| for (const item of data) { | ||
| applyElicitationDefaults(itemsSchema, item); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Combine schemas | ||
| if (Array.isArray(schema.allOf)) { | ||
| for (const sub of schema.allOf) { | ||
| applyElicitationDefaults(sub, data); | ||
| } | ||
| } | ||
| if (Array.isArray(schema.anyOf)) { | ||
| for (const sub of schema.anyOf) { | ||
| applyElicitationDefaults(sub, data); | ||
| } | ||
| } | ||
| if (Array.isArray(schema.oneOf)) { | ||
| for (const sub of schema.oneOf) { | ||
| applyElicitationDefaults(sub, data); | ||
| } | ||
| } |
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.
remove
|
Tested a checkout of this PR with conformance test created in modelcontextprotocol/conformance#19 and everything passes:
|


Draft PR for implementing SEP-1034.
#1054
modelcontextprotocol/modelcontextprotocol#1034
Motivation and Context
How Has This Been Tested?
Breaking Changes
Types of changes
Checklist
Additional context