Fix/tool annotations mapping #91
Conversation
Simplifies annotation field validation logic by removing unnecessary conditional checks for specific field names after earlier validation has already filtered them out.
Inverts the conditional logic in priority validation to use early returns for valid cases instead of checking for invalid ranges.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## trunk #91 +/- ##
============================================
+ Coverage 79.74% 81.46% +1.72%
- Complexity 858 917 +59
============================================
Files 46 48 +2
Lines 3080 3162 +82
============================================
+ Hits 2456 2576 +120
+ Misses 624 586 -38
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This pull request implements MCP-compliant annotation handling for WordPress abilities, adding support for converting WordPress-format annotations to MCP specification format and validating them according to their respective schemas (ToolAnnotations for Tools, Annotations for Resources and Prompts).
Key Changes
- Implements annotation mapping from WordPress format (e.g.,
readonly) to MCP format (e.g.,readOnlyHint) for Tools - Adds validation and filtering for MCP-specific annotations (
audience,lastModified,priority) for Resources and Prompts - Introduces comprehensive annotation validation in validators to enforce MCP specification compliance
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
includes/Domain/Tools/RegisterAbilityAsMcpTool.php |
Adds map_annotations_to_mcp() method to convert WordPress format Tool annotations to MCP ToolAnnotations format |
includes/Domain/Tools/McpToolValidator.php |
Adds strict validation for MCP ToolAnnotations fields (readOnlyHint, destructiveHint, idempotentHint, openWorldHint, title) |
includes/Domain/Resources/RegisterAbilityAsMcpResource.php |
Adds map_annotations_to_mcp() and ISO 8601 timestamp validation for Resource annotations |
includes/Domain/Resources/McpResourceValidator.php |
Adds validation for Resource-specific MCP Annotations (audience, lastModified, priority) |
includes/Domain/Prompts/RegisterAbilityAsMcpPrompt.php |
Adds map_annotations_to_mcp() and ISO 8601 timestamp validation for Prompt annotations |
includes/Domain/Prompts/McpPromptValidator.php |
Adds validation for Prompt-specific MCP Annotations (audience, lastModified, priority) |
tests/Unit/Tools/RegisterAbilityAsMcpToolTest.php |
Adds comprehensive test coverage for Tool annotation mapping and filtering |
tests/Unit/Resources/RegisterAbilityAsMcpResourceTest.php |
Adds test coverage for Resource annotation handling and validation |
tests/Unit/Prompts/RegisterAbilityAsMcpPromptTest.php |
Adds test coverage for Prompt annotation handling and validation |
tests/Unit/Domain/Tools/McpToolValidatorTest.php |
Adds tests for MCP ToolAnnotations validation |
tests/Unit/Domain/Resources/McpResourceValidatorTest.php |
Adds tests for MCP Resource Annotations validation |
tests/Unit/Domain/Prompts/McpPromptValidatorTest.php |
Adds tests for MCP Prompt Annotations validation |
tests/Unit/Core/McpAdapterConfigTest.php |
Updates expected resource/prompt lists to include new annotation test fixtures |
tests/Fixtures/DummyAbility.php |
Adds test fixtures for various annotation scenarios (valid, invalid, partial, null values) |
docs/guides/creating-abilities.md |
Extensively updates documentation to explain WordPress vs MCP annotation formats and usage patterns |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Eliminates duplicate annotation mapping code by creating a centralized utility class. The identical mapping logic was previously duplicated across prompt and resource registration classes, violating DRY principles and making maintenance more difficult. Creates a new static utility class that handles the conversion of WordPress ability annotations to MCP-compliant format, including validation and normalization of audience, lastModified, and priority fields. Improves code maintainability by providing a single source of truth for annotation transformation logic.
…to fix/tool-annotations-mapping Resolved conflicts by keeping local refactored code that uses shared utility classes (McpAnnotationMapper and McpValidator) instead of inline methods.
Add is_null() check in annotation field conversion to prevent null values from being cast to false. Null annotations are now properly filtered out as expected.
Remove no-op assertion when no annotations are present. The test already has assertions and the early return is a valid pass condition.
Replace conditional checks with required assertions to prevent false positives. Verify annotations and audience are present, priority is clamped to 0.0, and invalid values are filtered. Update fixture to include mixed valid/invalid audience values.
Ensure test verifies priority is clamped, not removed, by adding explicit assertions for annotations and priority existence.
Replace conditional checks with explicit assertions to prevent false positives. Assert that annotations exist, priority is clamped, and invalid fields are filtered out.
Enhances test coverage to verify that the validator properly detects and reports when the readOnlyHint annotation field contains a non-boolean value. Ensures the validation logic correctly enforces type constraints on annotation fields.
Ensures only the priority field proceeds to numeric validation by adding an explicit field name check before processing. Replaces the comment-only documentation with an actual conditional check that skips any remaining fields that aren't priority, making the validation logic more explicit and maintainable.
…details Refines the comment regarding auto-discovered resources from test fixtures to specify that multiple abilities are involved, enhancing clarity for future reference.
Removed the explicit check for the 'priority' field in the validation logic, allowing all numeric values to be processed. This change simplifies the validation flow and enhances maintainability by ensuring that only numeric values are validated without restricting to a specific field.
JasonTheAdams
left a comment
JasonTheAdams
left a comment
There was a problem hiding this comment.
Great work, @galatanovidiu! Left some comments and questions.
| if ( ! in_array( $field, $valid_fields, true ) ) { | ||
| $errors[] = sprintf( | ||
| /* translators: %s: annotation field name */ | ||
| __( 'Unknown annotation field: %s. Valid MCP annotation fields are: audience, lastModified, priority', 'mcp-adapter' ), | ||
| $field | ||
| ); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
I think we can actually remove $valid_fields and simply handle this as a last condition after 'priority'. We're explicitly checking for each type and validating it, effectively making an if...elseif...else, where the last condition is an invalid field.
…ed validation methods in McpValidator Updated validation logic to call static methods from McpValidator for base64 and MIME type checks. Removed redundant validation methods from McpPromptValidator and McpResourceValidator. Updated unit tests to reflect these changes and ensure proper validation behavior.
- centralize ability→MCP annotation mapping so MCP-native values are preserved and WP-format overrides take precedence - trim ability metadata, normalize resource URIs, and enforce server guards before validating prompts/resources/tools - reuse shared annotation validator logic in tool validator + tests, and add coverage for whitespace URIs and missing MCP server cases
- Trimmed the ability label before assigning it to the tool data title to ensure consistent formatting. - Updated DummyAbility test fixture to include a new test case for handling whitespace in resource URIs.
- Updated annotation keys from 'readOnlyHint', 'destructiveHint', and 'idempotentHint' to 'readonly', 'destructive', and 'idempotent' across multiple ability classes for uniformity. - Ensured that the metadata structure aligns with the new naming conventions.
- Implemented logic to set the annotations.title from the label if annotations exist but the title is not set, ensuring better metadata completeness.
- Replaced 'readOnlyHint', 'destructiveHint', and 'idempotentHint' with 'readonly', 'destructive', and 'idempotent' in the DiscoverAbilitiesAbilityTest, ExecuteAbilityAbilityTest, and GetAbilityInfoAbilityTest for consistency with the updated metadata structure.
…a handling - Adjusted the validation flow for the 'title' field to streamline error handling and ensure non-empty string requirements are enforced. - Cleaned up whitespace handling in the RegisterAbilityAsMcpResource class to enhance URI normalization. - Made minor formatting adjustments in ExecuteAbilityAbility for consistency.
JasonTheAdams
left a comment
JasonTheAdams
left a comment
There was a problem hiding this comment.
Good improvements, @galatanovidiu! I left some more feedback. 😄
| * @var \WP\MCP\Core\McpServer|null | ||
| */ | ||
| private McpServer $mcp_server; | ||
| private ?McpServer $mcp_server = null; |
There was a problem hiding this comment.
I'm confused why it's possible for the McpServer to be nullable. Is there a use case for this? Or should it be a required parameter in the constructor?
There was a problem hiding this comment.
Same question for Tool and Resource.
There was a problem hiding this comment.
This allows flexible component registration, components can be created independently and then associated with a server during registration.
I can modify this and make it more strict if you prefer.
There was a problem hiding this comment.
I see. This actually raises a different question, then: Why are the prompts, tools, and resources coupled to a specific server instance, rather than something ingested by the server?
There was a problem hiding this comment.
hhhh, yes, this creates a circular dependency:
- Components depend on the server (via
get_mcp_server()) - The server contains/registers components
Probably, we can check for uniqueness and validate it inside McpComponentRegistry (or something similar). I need to think about this.
There was a problem hiding this comment.
Yeah, I would think the dependency chain is Server > Registry > Tools/Resources/Prompts. Ideally, the dependency is uni-directional, so the components aren't aware of the registry or server, and the registry isn't aware of the server.
There was a problem hiding this comment.
Should we address this on another issue/PR?
There was a problem hiding this comment.
Yeah, let's resolve this in a subsequent PR. 👍
|
|
||
| case 'string': | ||
| if ( ! is_scalar( $value ) ) { | ||
| return null; |
There was a problem hiding this comment.
I wonder if it would make more sense to return a WP_Error when invalid, instead?
There was a problem hiding this comment.
We can do that, but I do not wnat to validate at this stage. Here I'm just normalizing the data included.
We have the mcp_adapter_validation_enabled setting, which enables component validation if the user wants it.
There was a problem hiding this comment.
Hmm... then shouldn't we be including whatever the value is in the returned results? The fact that we're omitting things from the results means that we are validating — i.e. if it's not valid then we omit it.
There was a problem hiding this comment.
I see what you mean, returning null does effectively exclude invalid values, which could be seen as a form of validation. My intent here, though, is to keep this step focused purely on normalization and avoid introducing validation logic at this stage.
I also don’t want to throw errors or risk breaking the entire MCP functionality if a tool annotation happens to be defined incorrectly. Returning null here ensures the system remains resilient and continues working even when a single annotation is malformed.
The explicit validation path is already available through the mcp_adapter_validation_enabled filter, which users can enable if they prefer stricter behavior.
Anyways, I'm open to discuss a refactor related to normalization/validation functionality, but this should be done on another issue/PR as this one is getting really big :)
| */ | ||
| private static function resolve_annotation_value( array $annotations, string $mcp_field, string $ability_property ): array { | ||
| // WordPress-format overrides take precedence when present. | ||
| if ( '' !== $ability_property && array_key_exists( $ability_property, $annotations ) && ! is_null( $annotations[ $ability_property ] ) ) { |
There was a problem hiding this comment.
Should we be checking ! is_null( $annotations[ $ability_property ] )? If the overriding annotation is null, then shouldn't we still consider that intentional?
There was a problem hiding this comment.
I do not think so. By default thise values are null https://github.com/WordPress/WordPress/blob/master/wp-includes/abilities-api/class-wp-ability.php#L38
So null cannot be intentional as it is the default value, also null annotations have no meaning, they shoul have a value. In MCP we remove them if they are null
There was a problem hiding this comment.
Hmm, so this would be covering the scenario wherein someone ignores the Ability annotation, leaving it null, and uses the MCP alternative instead? That seems like a pretty niche scenario. It also assumes that all built-in annotations in the future will have a default.
There was a problem hiding this comment.
If someone ignores the ability annotation and relies on the MCP alternative instead, then the MCP value will be used. This approach also maintains backward compatibility.
I’m not sure what future ability annotations might look like, but we can address that when the time comes. That said, I do expect them to follow the same format, with the default value remaining null.
As a side note, we should be more careful with backward compatibility moving forward. The previous breaking change caused issues because WooCommerce includes an older version of the mcp-adapter. If someone includes a newer version of the adapter, it could break their site. See issue #87 for details.
| return array( | ||
| 'has_value' => false, | ||
| 'value' => null, | ||
| ); |
There was a problem hiding this comment.
I think we should simply return the value or else return a WP_Error here to clean things up.
There was a problem hiding this comment.
I've simplified the resolve_annotation_value method but it will return null instead of WP_Error. As I was saying, at this point I want to normalize the annotations not validating
| if ( ! $value_info['has_value'] ) { | ||
| continue; | ||
| } | ||
|
|
||
| $normalized = self::normalize_annotation_value( $config['type'], $value_info['value'] ); | ||
| if ( null === $normalized ) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
If either getting the value or normalizing returns a WP_Error, let's use _doing_it_wrong() before continuing.
There was a problem hiding this comment.
Same as above
Update McpAnnotationMapper to use null for ability_property when annotations map directly to MCP field names, improving type safety and code clarity.
Replace if statements with switch...case in get_annotation_validation_errors() and fix priority validation logic.
Replace if statements with switch...case in annotation validation methods and fix priority validation logic.
Return value directly instead of array structure with has_value flag
Why
The adapter was passing annotations through unchanged, causing field name mismatches between WordPress Abilities API format and MCP specification. WordPress uses
readonly,destructive, andidempotent, while MCP requiresreadOnlyHint,destructiveHint, andidempotentHint. This caused protocol compliance issues and could break MCP clients expecting the correct field names.Additionally null annotation values should not be included in MCP responses.
What
Adapts abilities-api annotations format to comply with the MCP ToolAnnotations specification. Adds annotation mapping functions that convert WordPress annotation field names to MCP-compliant format:
readonly→readOnlyHint,destructive→destructiveHint,idempotent→idempotentHintaudience,lastModified, andpriorityfields according to MCP specificationThe implementation includes:
RegisterAbilityAsMcpTool,RegisterAbilityAsMcpResource, andRegisterAbilityAsMcpPromptMcpToolValidator,McpResourceValidator, andMcpPromptValidatorto ensure MCP complianceopenWorldHint,titlefor tools)This change ensures full compliance with the MCP ToolAnnotations specification while maintaining backward compatibility by accepting both WordPress-format and MCP-native annotation fields.
Closes #70