-
Notifications
You must be signed in to change notification settings - Fork 28
Proposal: Add ability anotations #99
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
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
94fc97d
Proposal: Add ability anotations
gziolo 324d332
Fix linting issues
gziolo a84b388
Update docs and tests
gziolo 47ef053
Increase coverage for annotations
gziolo 9f3a00c
Rename occurences of `read_only` to `readonly` based on feedback
gziolo a6ad2f4
Fix spacing in arrays
gziolo b244bce
Add additional unit tests
gziolo File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 want to suggest we align our internal naming conventions with the established MCP protocol standards to reduce developer cognitive load and maintain consistency across the systems.
Current approach:
read_only
,destructive
,idempotent
readOnlyHint
,destructiveHint
,idempotentHint
Suggested approach:
Since MCP is a well-established protocol with defined conventions, I think we should adopt the MCP naming directly in our abilities API:
readOnlyHint
,destructiveHint
,idempotentHint
internallyAdditional consideration - Instructions handling:
For the
instructions
field, since MCP uses the tool description as instructions, we can handle this in the MCP adapter by appending ourinstructions
content to the description field.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 counterpoint would be that WordPress uses snake case convention for naming properties. REST API is a good reflection of that. It's also inconvenient with JavaScript that uses camel case similarly to the MCP protocol. Sometimes, we even mix the two when functionality spans all layers, as with block types that also support the JSON format, which again uses camel case.
@felixarntz, and @swissspidy, what are your thoughts on that aspect?
Uh oh!
There was an error while loading. Please reload this page.
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.
+1 on aligning with WordPress's snake case convention:
IMO it's perfectly reasonable to have a translation layer in place as part of the MCP adapter, we shouldn't change the Abilities API to tie in particularly well with MCP when it leads to other problems.
Aside: I do agree it's painful to deal with snake_case from the server in the client-side JavaScript world where everything is camelCase. My recommendation for that reason is to use single-word names (like
readonly
,destructive
,idempotent
) wherever possible, as those would be both valid snake_case and valid camelCase. But obviously this is not the problem discussed here - it doesn't allow us to align with the MCP naming.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.
Happy to rename from
read_only
toreadonly
. I'm not even 100% sure whether "read-only" is the ultimate correct English spelling 😄 I definitely saw many timesreadonly
in the context of programming, for example, in TypeScript.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.
Whatever you decide to do is ok with me. Also, adapting those keys to the MCP standards is not a big problem.
I also understand your points on WP coding standards and the abilities-api being decoupled from MCP.
One thing I want to avoid when creating abilities for MCP usage:
This is inconsistent, non-standard, and ugly.
So, what do you think we should do to avoid this situation?
Uh oh!
There was an error while loading. Please reload this page.
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.
Completely agree, we shouldn't have inconsistencies like this.
Where does this example list of identifiers come from? As far as this PR is concerned, I think we can use
readonly
to work around the difference between snake_case and camelCase.If there are terms where we need multi-word identifiers, I'd say we need to use snake_case if their source of truth is on the server (i.e. in PHP).
If in some JavaScript use-cases we need to translate stuff to be camelCase, we could probably do that consistently as well.
Uh oh!
There was an error while loading. Please reload this page.
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 agree with @felixarntz. We shouldn't build anything in Abilities around the MCP standard. The MCP Adapter is a consumer of the Abilities API and should handle any translations and such needed to work. Any parameters added by the MCP Adapter should follow the pattern established here.
As a grammatical note, it kills me every time I see "readonly" in various contexts. We're just lying to ourselves. 😆
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 MCP annotations are documented here:
https://modelcontextprotocol.io/specification/2025-06-18/schema#annotations
https://modelcontextprotocol.io/specification/2025-06-18/schema#toolannotations
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 used the same docs to shape basic needs for abilities based on the discussion in #62.
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.
Renamed
read_only
key toreadonly
, and adjusted docs to use "read-only" form in 9f3a00c.