-
Couldn't load subscription status.
- Fork 32
Add initial core abilities for WordPress 6.9 #108
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
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #108 +/- ##
============================================
+ Coverage 89.17% 90.59% +1.42%
- Complexity 162 177 +15
============================================
Files 19 20 +1
Lines 1238 1489 +251
Branches 117 117
============================================
+ Hits 1104 1349 +245
- Misses 134 140 +6
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, @Jameswlepage! I left a few thoughts and suggestions. I didn't exhaustively review it, yet, but noticed these.
| 'annotations' => array( | ||
| 'type' => 'object', | ||
| 'description' => __( 'Annotations describing ability behavior.', 'abilities-api' ), | ||
| ), | ||
| 'show_in_rest' => array( | ||
| 'type' => 'boolean', | ||
| 'description' => __( 'Whether the ability is exposed in REST.', 'abilities-api' ), | ||
| ), |
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 should keep these inside of meta to be consistent with its structure.
| 'name' => $ability->get_name(), | ||
| 'label' => $ability->get_label(), | ||
| 'description' => $ability->get_description(), | ||
| 'meta' => $ability->get_meta(), | ||
| 'annotations' => $ability->get_meta_item( 'annotations', array() ), | ||
| 'show_in_rest' => $show_in_rest, |
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 think it would be a good idea to make a WP_Ability::to_array() for a more declarative option.
| 'abilities' => array( | ||
| 'type' => 'array', | ||
| 'items' => array( | ||
| 'type' => 'object', |
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'm seriously considering a WP_Ability::to_json_schema() method to generate this structure. I could see this being used in a few places, including JS, and having a declarative way of getting the schema along with ::to_array() would be useful.
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 think it's a consequence of what I explain in #108 (comment). This replicates the existing REST API endpoint. This has far-reaching consequences as noted in #109 (review), too.
|
d52ad8c to
b7f1ae3
Compare
- Add WP_Ability::to_array() method for declarative array export - Add WP_Ability::to_json_schema() method for JSON Schema generation - Add comprehensive test coverage for both methods - Ensure annotations and show_in_rest remain under meta structure The to_array() method returns a complete array representation of the ability including name, label, description, schemas, and metadata, excluding callbacks as they are not serializable. The to_json_schema() method generates a JSON Schema Draft 7 compliant schema describing the ability's structure, useful for validation in both PHP and JavaScript contexts. Implements feedback from PR WordPress#108.
|
Thank you for stating it. I personally would be pretty happy to include these two abilities with some minor tweaks:
The method is pretty old, but its description is more up to date at https://developer.wordpress.org/reference/functions/get_bloginfo/:
To align with that and the other ability name, I propose we go with:
This is shaped nicely, as you could see from my references above. |
| 'description' => __( 'Returns basic information about the current authenticated user.' ), | ||
| 'output_schema' => array( | ||
| 'type' => 'object', | ||
| 'required' => array( 'id', 'display_name', 'locale' ), |
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.
This is a bit surprising, because there are more fields in the response, so it needs some adjustment to meet the actual implementation.
| 'additionalProperties' => false, | ||
| ), | ||
| 'execute_callback' => static function (): array { | ||
| $current_user = wp_get_current_user(); |
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.
This is going to be interesting to see how it plays out in practice with AI clients that use some access tokens. It might happen that this will report back as an agent user 😄
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.
Related: #108 (comment)
Switching to current_user_can() per the above would make this useful for both the agent user, and the user interacting with the agent.
|
|
Implements 4 core abilities to provide a stable foundation: - core/get-bloginfo: Retrieve site information fields - core/get-current-user-info: Get current authenticated user data - core/get-environment-type: Get WordPress environment type - core/find-abilities: Discover available abilities with optional filtering The core abilities are registered via the abilities_api_init hook and can be disabled in test environments or controlled via the abilities_api_register_core_abilities filter. Addresses #105
- Update filter hook name to wp_find_abilities_results for core consistency - Remove text domains from translation functions (core uses default domain) - Fix execute callback signatures to use optional parameters - Inline test environment check in bootstrap.php
|
I think the call to remove the meta ability to find abilities is a good one, and will do this. I like that concept (find/execute), and I know there have been experiments by @galatanovidiu with the MCP adapter. Let's continue to experiment, and understand real world usage there and in the AI experiments plug-in |
b7f1ae3 to
81cc4e2
Compare
This removes the `core/find-abilities` ability to address the chicken-and-egg security issue where the ability could expose non-REST abilities when called via REST API. The ability had a `show_in_rest` parameter that could be set to false, which would bypass the REST visibility controls when the ability itself was marked as exposed in REST. This creates an inconsistent security model. Changes: - Removed register_find_abilities() method from WP_Core_Abilities class - Removed register_find_abilities() call from register() method - Removed test_core_find_abilities_executes() test - Removed core/find-abilities unregister check from test setup
81cc4e2 to
299ecc9
Compare
|
@gziolo, interesting points on the get blog info feedback. I intentionally had it only return one to keep things very atomic/one-shot. But as I think about this further, I don't see any negative in allowing there to be bulk actions. I do wonder if we should name it something that deviates from get blog info.
This too was intentionally as simple as it could get. I approached it considering how it could be a potential of the most basic ability, that also returned helpful information, and could then be layered into something like get site info. But then if you consider a real world use case, I think there's merit to having a full environment info ability. |
|
And the sea of failing tests are reminding me that the category system is now here! Noting that I am registering and categorizing these abilities as |
I would propose:
The reason being that I can definitely see a future wherein there are more abilities tied to interacting with site data (e.g. changing the site title), as well as interacting with user data (e.g. setting their name). I think they're categorically distinct. |
I think it could be useful to make this a touch more generic as
Trying not to get into sensitive data. But I think it's useful for the AI to know details like that. |
|
An interesting consideration when thinking about naming get blog info versus get site info, get site info is not a function in WordPress - Because of that, I could see hallucinations being introduced when using very small LLMs to fill and run abilities. If we mirror the getBlogInfo ability to the schema of getBlogInfo, I would assume that the accuracy is higher, especially on lower parameter models (because that function is in the training data). First, this is only something that applies to TINY LLMS, which almost all users probably won't deal with. However, these are the LLMs that would power on browser, on device, or on server AI. This had me interested, so I generated a basic experiment with ChatGPT and used the OSS Google Gemma 1b and 4b param models (because they power Chrome's built-in AI) to to a very quick, very nonscientific test. https://github.com/Jameswlepage/abilities-llm-benchmarks/blob/main/FINAL_REPORT_wordpress_naming_analysis.md (this is a summary made by Claude Code from the outputs generated with LM Studio and the Gemma models. noting again that this is very quick and messy)
In regards to actually renaming things, I agree with your point here, Greg.
The above consideration is probably just that - larger hosted models won't have trouble with this. So - I've updated the original implementation of get blog info to a site specific implementation accordingly, but these are interesting considerations as we really determine how abilities, AI, and WordPress Core will all interact in the future. |
|
The site/blog info might be a good opportunity to test |
…ent info Major changes based on PR feedback and model testing findings: Namespace Changes: - Renamed core/* to wp/* namespace for clarity - wp/get-site-info (was core/get-bloginfo) - wp/get-current-user-info (namespace change) - wp/get-environment-info (was core/get-environment-type) Category System: - Split from single 'core' category into semantic categories - 'site' category for site-related abilities - 'user' category for user-related abilities Expanded Environment Info: - Added php_version field - Added mysql_version field - Added wp_version field - Added database_type field (mysql/mariadb detection) - Renamed ability to reflect expanded scope Security Improvements: - wp/get-site-info now requires manage_options capability - wp/get-environment-info requires manage_options capability - Protects sensitive site and environment information Tests: - All 193 tests passing with 447 assertions - Updated test fixtures for new names and categories - Added assertions for expanded environment info fields
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.
Good work, @Jameswlepage! Left some suggestions/questions. 👍
| public static function register_category(): void { | ||
| // Site-related capabilities | ||
| wp_register_ability_category( | ||
| 'site', |
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.
Let's use a class constant here, same with user, for reuse in the abilities.
| }, | ||
| 'meta' => array( | ||
| 'annotations' => array( | ||
| 'instructions' => __( 'Retrieves a single site property by passing an allowed field to get_bloginfo().' ), |
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 think I missed this annotation. What's this for? How is it different than the ability description?
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.
This was in response to one of Greg's notes about using annotations to steer models should they get confused by the naming convention (ie; hey AI, the underlying function powering this is actually get_bloginfo()). This description could probably be more helpful. And I actually don't think we should add instructions in the (first) core implementation anymore - folks could extend if they want
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.
| 'environment' => (string) $env, | ||
| 'php_version' => (string) $php_version, | ||
| 'mysql_version' => (string) $db_version, | ||
| 'wp_version' => (string) $wp_version, | ||
| 'database_type' => (string) $type, |
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.
Why are we explicitly casting all of these? Aren't most or all of them strings already?
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.
Good note everything is a string other than the db version, and that "DB version" got me thinking as well. There are some artifacts from previous thoughts here. It shouldn't be the MySQL version because it could be Maria. And that is clarified in the database type. So going to move database type above the database version, and use db_server_info instead of this implementation.
| }, | ||
| 'meta' => array( | ||
| 'annotations' => array( | ||
| 'instructions' => __( 'Retrieves environment information such as environment type, PHP, database, and WordPress versions.' ), |
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.
Last one. Still curious. 😂
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 think we drop instructions in this; was exploring how AI could interact with something like (but this one isn't any diff from the description).
Code improvements: - Add CATEGORY_SITE and CATEGORY_USER class constants for type safety - Use constants instead of magic strings for category references - Improve ability descriptions with more specific use cases - Simplify annotations by removing redundant instructions field - Remove unnecessary type casting in get-site-info value output - Consolidate database info into single db_server_info field Schema changes: - wp/get-environment-info now returns db_server_info instead of separate mysql_version and database_type - Updated test assertions to match new schema - More descriptive field documentation throughout All tests passing: 193 PHP tests (446 assertions), 102 client tests
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.
Nicely done, @Jameswlepage! I have a few more comments, but they're not blockers to merging for me, so I'll go ahead and approve. Your choice if you'd like to implement any of the suggestions.
| 'properties' => array( | ||
| 'environment' => array( | ||
| 'type' => 'string', | ||
| 'description' => __( 'The site\'s runtime environment classification (e.g., production, staging, development).' ), |
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 can use examples here, too.
Co-authored-by: Jason Adams <[email protected]>
- Add example values for environment field in site info schema - Convert short array syntax to WordPress coding standards - Fix spacing alignment for consistency
3324b79 to
93e85df
Compare
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.
Code LGTM 🙌
Left a couple thoughts re robustness/future-compat below. Nonblocking, but I do think making the abilities more generic for 6.9 will be very helpful for both ourselves in the other building blocks and downstream adopters (and aligns with the feedback on WordPress/ai#40 )
| ); | ||
|
|
||
| wp_register_ability( | ||
| 'wp/get-site-info', |
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.
should our prefix be core to align block namespaces? (personally think wp is fine)
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’m biased and prefer core/ based on my familiarity with the Gutenberg project where every aspect uses that convention: block types, block patterns, block bindings, data stores, etc.
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.
It was core/ and I had suggested wp/. I didn't think of the convention already being established elsewhere. It's confusing to me that WP sometimes prefixes with wp_ and other times uses core. 🤷♂️
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.
If Gutenberg uses core as a prefix, we probably should follow that convention because it's the more modern implementation on WordPress. If anybody has strong feelings, let me know.
Will roll it back to core, but not super opinionated here.
| */ | ||
| protected static function register_get_current_user_info(): void { | ||
| wp_register_ability( | ||
| 'wp/get-current-user-info', |
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'm wondering if it makes sense to generalize this to wp/get-user-info and then add an optional id: int input field (which in the future could be expanded with an idType and polymorphism or separate login_name, slug, email fields) that defaults to the current user if not supplied.
I don't think we need both, and implementing it this way will make it easier to handle the "agent user" pattern that @gziolo brought up in #108 (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.
I think it’s perfectly fine to start with supporting only the current user because it has simple permission check model. There isn’t anything sensitive about asking for your info. If any, we could drop the current part from the ability unique name in case we feel it would help making it more extensible to support also fetching details about other users later. Filtering output by field name(s) seems like enhancement worth exploring but I would keep it for later because right now there isn’t much to filter.
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.
However I guess you are comparing it to the site info where such filtering by field is possible. I left my comment earlier, the downside of the shape proposed for site info is that the input and output schema describes more the shape of the response rather what individual fields represent. It’s much richer for the user as every field has description explaining their role.
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 it - good optionality now, but I like starting as direct/upfront/atomic as possible.
| 'additionalProperties' => false, | ||
| ), | ||
| 'execute_callback' => static function (): array { | ||
| $current_user = wp_get_current_user(); |
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.
Related: #108 (comment)
Switching to current_user_can() per the above would make this useful for both the agent user, and the user interacting with the agent.
| 'wp/get-site-info', | ||
| array( | ||
| 'label' => __( 'Get Site Information' ), | ||
| 'description' => __( 'Returns a single site information field configured in WordPress (e.g., site name, URL, version) for display or diagnostics.' ), |
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.
Similarly to #108 (comment), I'm wondering if we'd get more mileage out of this if it returned an entire array of blog_info / site health, and instead used a fields: string[] if a user only wants a subset.
The obvious use case is LLMs/REST where you don't want a bunch of round trips for basic data, but I think it's helpful beyond that.
Semantically, it lets '-info' align better with the other two abilities.
Also, a shaped output_schema with the actual properties and matching types is better for both LLMs, traditional tools, and humans to reason with vs an array{field:string,value:mixed}. (We see the effects of this a lot in GraphQL/headless land)
Thoughts?
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.
Good point. It was the first ability I wrote, so missing consistency and not self documenting which I think is a big unlock. Kind of 'layered' but I think that's OK in this situation.
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.
However, if we make this change, then REST would require a weird empty ?input field given how the current controller works.
GET /run?input # All fields
GET /run?input[fields][]=name # Filtered
This could be a nonbreaking improvement in the future and imo shouldn't block
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 think the problem is that when you provide empty input, then the REST API sanitizes it and sets it to an empty array. Otherwise, execute will use its default value null. So, the solution could be allowing null in the input schema to address it. There might be better alternatives to consider, but I can't think of a simpler approach.
| * @param mixed $input Optional. The raw input provided for the ability. Default `null`. | ||
| * @return mixed The input with the schema default applied when available. | ||
| */ | ||
| public function normalize_input( $input = null ) { |
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 ran out of time to include that in the patch against WordPress trunk. Is there anyone willing to handle the bug fix PR in core just for this change? 😄
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.
WordPress/wordpress-develop#10395 - started working on the patch. Unit tests are still missing at the time of writing.
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 know this was merged, but here are a handful of things to address before a core patch
| * @since 0.3.0 | ||
| */ | ||
| // phpcs:ignore WordPress.NamingConventions.PrefixAllGlobals.NonPrefixedClassFound -- Core class intended for WordPress core. | ||
| final class WP_Core_Abilities { |
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.
What is the value in putting these in a class? It feels like this could all be done in more straightforward functions.
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.
It should be perfectly fine to introduce two functions or static methods and attach them inside src/wp-includes/default-filters.php in WP core, for example:
add_filter( 'wp_abilities_api_categories_init', 'register_core_ability_categories' );
add_filter( 'wp_abilities_api_init', 'register_core_abilities' );| 'version', | ||
| ); | ||
|
|
||
| wp_register_ability( |
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.
This seems to be duplicating the site settings REST endpoint. What is the value of exposing this here when there is already a way to expose this information.
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.
Great Q!
If you're asking from a more philosophical POV ("why do Abilities even exist"), then the answer is similar to "What is the value of having REST endpoints when there's already a php function and ajax?". The tl;dr goal of the Abilities API is to create a sort of "Generic API" abstraction + registry, the idea being that instead of needed to reregister/expose the same functionality to various last-mile APIs (REST, admin-ajax, graphql, PHP in theme/plugins, MCP/A2A, Command Palette, and whatever future tech integrations that come up with). It's standardized and reliable, like hooks but for functionality, so all anybody needs is a generic adapter and it'l work without any domain knowledge of the ability itself.
In fact, I'd hope and expect to see the internals of many of our existing REST endpoints updated to internally use the Abilities API in 7.0 and beyond.
If you're asking from a more practical POV (e.g. regarding the shape of the input/output schemas), I think we're open to evolution here. I believe we want something that it generic to make sense in multiple context, and not just a single one of either REST, MCP, or Command Palette.
| 'type' => 'array', | ||
| 'description' => __( 'The roles assigned to the user.' ), | ||
| 'items' => array( | ||
| 'type' => 'string', |
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.
Roles is an array, not a string. See:
https://core.trac.wordpress.org/browser/trunk/src/wp-includes/class-wp-user.php#L82
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.
Hi @aaronjorbin, what we have seems correct, roles is of type array with each item being a string. Feel free to correct me if I'm missing anything.
| 'properties' => array( | ||
| 'environment' => array( | ||
| 'type' => 'string', | ||
| 'description' => __( 'The site\'s runtime environment classification (e.g., production, staging, development).' ), |
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.
This can only be one of four values. I think it's valueable to make that clearer.
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.
enum would fit perfectly here 👍🏻
Summary
Implements 4 foundational core abilities to provide a stable foundation for WordPress 6.9, addressing issue #105.
Abilities Included
core/get-bloginfo- Retrieve individual site information fields (name, description, url, wpurl, admin_email, charset, language, version)core/get-current-user-info- Get current authenticated user data (id, display_name, user_nicename, user_login, roles, locale)core/get-environment-type- Get WordPress environment type (production, staging, development, local)core/find-abilities- Discover available abilities with optional namespace and show_in_rest filteringDesign Philosophy
These abilities were selected to provide essential foundational operations while following atomic design principles:
core/get-bloginforetrieves one field at a time rather than bulk data)The atomic approach keeps the initial release simple and predictable, with room to add bulk or convenience operations in future releases based on real-world usage patterns.
Implementation Details
metastructure (PRs Moveshow_in_restto meta in the registration process #107, Move newannotationsproperty undermetaand update docs #104)annotationsandshow_in_restproperly nested insidemetareadonly: true,destructive: false,idempotent: trueabilities_api_inithookabilities_api_register_core_abilitiesfilterTesting
All 149 PHPUnit tests pass (345 assertions)
REST API endpoints validated with authentication
All 8 bloginfo fields tested and working
Input validation working correctly
Permission callbacks enforced properly
Architecture Decisions
core/get-bloginfo: Keeps abilities atomic and composable, following Unix philosophy and WordPress REST API patternscore/find-abilities: Requiresreadcapability to prevent anonymous enumeration while still accessible to all authenticated usersAddresses
Closes #105
Checklist
show_in_restto meta in the registration process #107 and Move newannotationsproperty undermetaand update docs #104