[Server] Extend elicitation enum schema compliance#261
[Server] Extend elicitation enum schema compliance#261chr-hertel wants to merge 1 commit intomainfrom
Conversation
chr-hertel
changed the title
There was a problem hiding this comment.
Pull request overview
This PR updates the PHP MCP conformance server and schema layer to support extended elicitation enum schemas (SEP-1330) and defaults (SEP-1034), aligning with upstream conformance expectations.
Changes:
- Add new conformance tools to exercise server-initiated elicitation, defaults, and enum-schema variants.
- Introduce new elicitation schema definition classes for SEP-1330 titled enums and multi-select enums.
- Remove previously-listed elicitation checks from the conformance expected-failures baseline.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Conformance/server.php | Registers new conformance tools for elicitation scenarios. |
| tests/Conformance/Elements.php | Implements tool handlers that send elicitation requests with defaults and enum schemas. |
| tests/Conformance/conformance-baseline.yml | Drops elicitation checks from expected failures (implying they now pass). |
| src/Schema/Elicitation/ElicitationSchema.php | Broadens properties PHPDoc type to AbstractSchemaDefinition. |
| src/Schema/Elicitation/TitledEnumSchemaDefinition.php | Adds SEP-1330 single-select titled enum schema serialization. |
| src/Schema/Elicitation/MultiSelectEnumSchemaDefinition.php | Adds multi-select enum schema serialization. |
| src/Schema/Elicitation/TitledMultiSelectEnumSchemaDefinition.php | Adds SEP-1330 multi-select titled enum schema serialization. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * @param array<string, AbstractSchemaDefinition> $properties Property definitions keyed by name | ||
| * @param string[] $required Array of required property names | ||
| */ |
There was a problem hiding this comment.
The constructor PHPDoc now allows any AbstractSchemaDefinition, but ElicitationSchema::fromArray still delegates to PrimitiveSchemaDefinition::fromArray which currently rejects type: array and cannot recognize oneOf/anyOf enum shapes. This means a client deserializing an elicitation/create request containing SEP-1330 enums (or multi-select arrays) will either throw (array type) or silently downgrade titled enums to a plain StringSchemaDefinition. Consider extending the parsing path (PrimitiveSchemaDefinition + related fromArray implementations) to support these new schema shapes and updating the schema-level tests accordingly.
| /** | ||
| * @param string $title Human-readable title for the field | ||
| * @param list<array{const: string, title: string}> $oneOf Array of const/title pairs | ||
| * @param string|null $description Optional description/help text | ||
| * @param string|null $default Optional default value (must match a const) | ||
| */ | ||
| public function __construct( | ||
| string $title, | ||
| public readonly array $oneOf, | ||
| ?string $description = null, | ||
| public readonly ?string $default = null, | ||
| ) { | ||
| parent::__construct($title, $description); |
There was a problem hiding this comment.
This schema definition doesn’t provide a fromArray() constructor, and the current PrimitiveSchemaDefinition::fromArray logic will parse a {type: "string", oneOf: [...]} schema as a StringSchemaDefinition (losing the enum options). To keep request deserialization symmetric with serialization, add a fromArray() here and update the primitive schema factory to detect the SEP-1330 titled enum shape (e.g., presence of oneOf with const/title) and return this type.
| final class MultiSelectEnumSchemaDefinition extends AbstractSchemaDefinition | ||
| { | ||
| /** | ||
| * @param string $title Human-readable title for the field | ||
| * @param string[] $enum Array of allowed string values | ||
| * @param string|null $description Optional description/help text | ||
| */ | ||
| public function __construct( | ||
| string $title, | ||
| public readonly array $enum, | ||
| ?string $description = null, | ||
| ) { | ||
| parent::__construct($title, $description); | ||
|
|
There was a problem hiding this comment.
This schema definition doesn’t provide a fromArray() constructor, and the current PrimitiveSchemaDefinition::fromArray rejects type: array, so a client deserializing a multi-select enum schema will throw. Consider adding fromArray() here and extending the primitive schema factory to recognize {type: "array", items: {type: "string", enum: [...]}} as a MultiSelectEnumSchemaDefinition.
| final class TitledMultiSelectEnumSchemaDefinition extends AbstractSchemaDefinition | ||
| { | ||
| /** | ||
| * @param string $title Human-readable title for the field | ||
| * @param list<array{const: string, title: string}> $anyOf Array of const/title pairs | ||
| * @param string|null $description Optional description/help text | ||
| */ | ||
| public function __construct( | ||
| string $title, | ||
| public readonly array $anyOf, | ||
| ?string $description = null, | ||
| ) { | ||
| parent::__construct($title, $description); | ||
|
|
||
| if ([] === $anyOf) { | ||
| throw new InvalidArgumentException('anyOf array must not be empty.'); | ||
| } |
There was a problem hiding this comment.
This schema definition doesn’t provide a fromArray() constructor, and the current PrimitiveSchemaDefinition::fromArray rejects type: array, so a client deserializing {type:"array", items:{anyOf:[...]}} will throw. Consider adding fromArray() here and extending the primitive schema factory to detect the SEP-1330 titled multi-select shape (array + items.anyOf with const/title pairs) and return this type.
| * @see https://github.com/modelcontextprotocol/modelcontextprotocol/issues/1330 | ||
| */ | ||
| final class TitledMultiSelectEnumSchemaDefinition extends AbstractSchemaDefinition | ||
| { | ||
| /** |
There was a problem hiding this comment.
There is an established unit test suite for elicitation schema (tests/Unit/Schema/Elicitation/*), but the new SEP-1330 schema definitions introduced here aren’t covered. Adding tests for jsonSerialize() output and for round-tripping via ElicitationSchema::fromArray / PrimitiveSchemaDefinition::fromArray would help prevent regressions and ensure both client and server can handle the new schema shapes consistently.
Tackling extended elicitations schema of modelcontextprotocol/modelcontextprotocol#1330 and conformance tests.
Also fixes #178