Skip to content

Conversation

galatanovidiu
Copy link
Contributor

@galatanovidiu galatanovidiu commented Sep 17, 2025

Summary

This PR represents a comprehensive architectural restructuring of the WordPress MCP Adapter, addressing significant technical debt while introducing three major feature sets.

While I understand the importance of small, atomic PRs for easier review, the scope and interconnected nature of the issues made it impractical and time-consuming.

I hope this work unblocks a cleaner, more modular workflow moving forward.

What's New

1. HttpTransport - Modern Transport Infrastructure

The new HttpTransport class replaces the existing RestTransport and StreamableTransport implementations, providing a unified, more robust transport layer.

Key Features:

  • Unified HTTP handling with dedicated request processing services (HttpRequestHandler, RequestRouter)
  • Session management with user-based authentication (SessionManager, HttpSessionValidator)
  • Standardized error responses via JsonRpcResponseBuilder with proper HTTP status code mapping
  • Improved request context handling through HttpRequestContext and McpTransportContext
  • Better separation of concerns with specialized infrastructure components

Technical Benefits:

  • Provides consistent error handling across all HTTP-based transports
  • Implements proper session lifecycle management for stateful operations
  • Improves security through built-in authentication validation

2. Default Server with Layered Tools Architecture

A new default MCP server that automatically registers with the adapter, featuring a layered approach to exposing WordPress abilities as MCP tools.

Components:

  • DefaultServerFactory: Automated server creation and registration
  • Layered Tools System: Three specialized tools for ability discovery and execution:
    • DiscoverAbilitiesTool: Discovers available WordPress abilities
    • GetAbilityInfoTool: Retrieves detailed information about specific abilities
    • ExecuteAbilityTool: Executes abilities with proper parameter handling

Technical Benefits:

  • Implements dynamic ability discovery without hard-coding tool definitions
  • Provides a flexible execution model that adapts to WordPress capabilities
  • Establishes a standardized interface for WordPress functionality access

3. STDIO Server Bridge for CLI Integration

Complete WP-CLI integration enabling MCP servers to be exposed through STDIO transport, making them accessible from command-line tools and external applications.

Features:

  • WP-CLI Command: New wp mcp-adapter command for server management
  • StdioServerBridge: Bridges MCP servers to STDIO for command-line communication
  • Universal Server Access: Any registered MCP server can be accessed via CLI

Usage:

# Start an MCP server via STDIO
wp mcp-adapter serve --server=mcp-adapter-default-server --user=admin

# List available servers
wp mcp-adapter list

Technical Benefits:

  • Enables STDIO transport protocol

4. MCP Component Type System & Auto-Discovery

Introduction of a type-based system for categorizing and automatically discovering MCP components (tools, resources, and prompts).

Key Features:

  • Type Classification: Abilities can be explicitly marked as tool, resource, or prompt via meta.mcp.type
  • Public Access Control: Fine-grained control over which abilities are exposed through MCP via meta.mcp.public
  • Auto-Discovery: The default server automatically discovers resources and prompts based on their mcp.type without manual registration
  • Default Behavior: Abilities default to type tool if not specified

Example:

wp_register_ability('my-plugin/post-resource', [
    'label' => 'Post Resource',
    'description' => 'Access WordPress posts',
    'meta' => [
        'mcp' => [
            'public' => true,     // Expose via MCP
            'type'   => 'resource' // Auto-discovered as resource
        ],
        'uri' => 'site://posts/{id}' // Resource-specific metadata
    ]
]);

Technical Benefits:

  • Eliminates the need for manual registration of resources and prompts in the default server
  • Provides clear component type distinction for better organization
  • Enables the default server to automatically configure itself based on ability metadata

Architecture Improvements

Enhanced Core Infrastructure

  • Component Registry (McpComponentRegistry): Centralized management of MCP components with improved lifecycle handling
  • Transport Factory (McpTransportFactory): Standardized transport creation with dependency injection
  • Handler Standardization (HandlerHelperTrait): Consistent implementation patterns across all handlers
  • Improved Separation of Concerns: Clear boundaries between transport, server, and handler layers
  • Better Dependency Management: Reduced coupling between components through interface-based design

Error Handling Improvements

  • Standardized Error Codes: Consistent JSON-RPC error code mapping to HTTP status codes
  • Structured Error Responses: JsonRpcResponseBuilder ensures uniform error format across all endpoints
  • Enhanced Validation: Comprehensive validation for tool execution, request processing, and parameter handling
  • Better Error Context: More informative error messages with proper error chain propagation

Code Quality Enhancements

  • Type Safety: Improved type hints and generic specifications throughout the codebase
  • Static Analysis: PHPStan compliance improvements with proper type annotations
  • Code Standards: Full WordPress coding standards compliance
  • Dependency Management: Updated to the latest versions of development dependencies
  • Configuration Consistency: All hooks and filters use standardized mcp_adapter_ prefix

Testing & Quality Assurance

Comprehensive Test Coverage (26 test files added/modified):

  • Unit tests for all new infrastructure components
  • Integration tests for HTTP transport layer and request routing
  • Session management and authentication flow tests
  • Error handling consistency validation tests
  • Handler trait behavior tests
  • Transport infrastructure validation tests

Breaking Changes

Removed Components

  • RestTransport: Removed and replaced by HttpTransport - all functionality preserved in the new implementation
  • StreamableTransport: Removed and replaced by HttpTransport - enhanced streaming capabilities included
  • Migration guide provided below for updating existing implementations

Filter Name Standardization

All WordPress filter names now use the mcp_adapter_ prefix for consistency:

  • mcp_rest_transport_*mcp_adapter_rest_transport_*
  • mcp_streamable_transport_*mcp_adapter_streamable_transport_*
  • Custom filter implementations require prefix updates

Migration Guide

Transport Migration

See the migration guide for detailed migration instructions.

Filter Migration

// Old filter names
add_filter('mcp_validation_enabled', $callback);

// New standardized names
add_filter('mcp_adapter_validation_enabled', $callback);

Implementation Details

New Infrastructure Components

  • HttpTransport - Primary transport implementation with HTTP/1.1 and HTTP/2 support
  • HttpRequestHandler - Request processing with proper header management
  • HttpSessionValidator - Session validation with WordPress user integration
  • SessionManager - Stateful session management with cleanup procedures
  • RequestRouter - Intelligent request routing based on HTTP method and content type
  • JsonRpcResponseBuilder - JSON-RPC compliant response formatting
  • McpTransportContext - Enhanced transport context with better state management
  • McpComponentRegistry - Component lifecycle management with proper initialization
  • McpTransportFactory - Transport instantiation with configuration injection

CLI Implementation Components

  • McpCommand - WP-CLI command implementation with subcommand support
  • StdioServerBridge - STDIO transport bridge with proper stream handling

Default Server Tools

  • DiscoverAbilitiesTool - WordPress ability discovery with metadata extraction
  • ExecuteAbilityTool - Ability execution with parameter validation and error handling
  • GetAbilityInfoTool - Detailed ability information retrieval
  • DefaultServerFactory - Automated server creation with configuration management

Technical Debt Resolution

Resolved Issues

  • Eliminated circular dependencies in the transport layer
  • Fixed inconsistent error response formats
  • Resolved session state management problems
  • Addressed missing type hints and annotations

Architecture Improvements

  • Implemented proper dependency injection patterns
  • Established clear interface contracts
  • Improved separation of concerns across layers
  • Standardized configuration management
  • Enhanced error propagation mechanisms

Future Technical Considerations

  • Enhanced CLI commands for advanced server management scenarios
  • Performance optimizations for high-concurrency environments
  • Additional metadata-driven features for component discovery and management

Introduces a new transport layer that allows clients to communicate with an MCP server over HTTP, adhering to the MCP 2025-06-18 Streamable HTTP specification.

The transport establishes a single WordPress REST API endpoint that intelligently handles different HTTP methods:
- `POST`: Processes single or batch JSON-RPC messages from the client.
- `GET`: Establishes a connection for Server-Sent Events (SSE), with full streaming functionality to be implemented later.
- `DELETE`: Terminates the client session.
- `OPTIONS`: Handles CORS preflight requests.

A unified session management system is included, using WordPress transients to track client sessions. This ensures consistent state handling for all requests after the initial handshake. A comprehensive suite of integration tests is also added to validate the transport's functionality and compliance.
…nsport

Replaced `__CLASS__` with `self::class` in the constructor of both RestTransport and StreamableTransport to comply with the coding standards
Ensures the `rest_post_dispatch` filter closure always returns the response object, preventing potential breaks in the filter chain.

Additionally, this change converts the closure to a static function for a minor performance improvement and applies other code style enhancements, such as using Yoda conditions and cleaning up whitespace.
- Fix useless conditions in McpPromptValidator and McpToolValidator
- Simplify validation logic to return regex results directly
- Add generic type specifications for WP_REST_Request parameters
- Fix WP_REST_Request<array<string, mixed>> in HttpTransport and StreamableTransport

Fixes GitHub PHPStan errors:
- Method get_cors_headers() generic type specification
- Method get_cors_origin() generic type specification
Implements a fallback to the `NullMcpObservabilityHandler` if a custom observability handler is not provided or the specified class does not exist. This makes the feature optional and prevents potential fatal errors from invalid configurations.

Additionally, removes a redundant `is_callable` check for the transport permission callback, as this is already enforced by the method's type hint.
The `wp_json_encode` function can return `false` on failure, which violates the `string` return type hint.

This change handles potential encoding failures by returning an empty JSON object string (`'{}'`) instead of `false`, preventing type errors and improving robustness.
Removes the redundant `is_array()` check when handling error formats. The `isset($result['error'])` check is sufficient, as it will only evaluate to true if `$result` is an array containing an 'error' key.
Simplifies the instantiation of the `McpTransportContext` by moving the `McpRequestRouter` creation logic into its constructor.

Previously, a complex two-step process was required to work around a circular dependency between the context and the router. The context now automatically creates its own router instance if one is not explicitly provided.

This change significantly cleans up the setup logic in the main server class and in tests.
Removes `WP_Error` from the PHPDoc for the request handling method.

This change aligns the documentation with the actual implementation, which exclusively returns a `WP_REST_Response` object.
The SystemHandler class was instantiated with an McpServer instance, but this dependency was not utilized by any of its methods.
Removes several unused `use` statements across the codebase for improved hygiene.

The transport context is also simplified by no longer automatically instantiating a request router. This change clarifies responsibilities and enforces that the router must be explicitly provided.
…tubs

- Added `php-stubs/wp-cli-stubs` version 2.12 to `composer.json`.
- Updated `phpstan/phpstan` to version 2.1.26.
- Updated various other dependencies in `composer.lock` to their latest versions, including `phpunit/phpunit`, `slevomat/coding-standard`, and `squizlabs/php_codesniffer`.
- Updated `phpstan.neon.dist` to include the new `wp-cli-stubs.php` bootstrap file for static analysis.
Updated package versions for PHPCompatibility, phpcompatibility-paragonie, phpstan/php-8-stubs, phpstan/phpstan, phpunit, and sebastian/exporter. Adjusted references and distribution URLs to reflect the latest commits. This ensures compatibility with the latest features and fixes from these dependencies.
@galatanovidiu
Copy link
Contributor Author

Does this supersede #46 and incorporate feedback from there?

Yes, it does. I'm keeping #46 open for now until we reach consensus on the direction forward.

I'm really not a fan of massive PRs like this that make huge architectural decisions. Were there any discussions about them that I missed? Can the PR be split up?

I'm also not a fan of these massive PRs, especially this one was really intensive and time-consuming.
More details here:
https://wordpress.slack.com/archives/C08TJ8BPULS/p1758214390610409

For instance, the WP-CLI changes can easily be extracted, or better yet reverted. Why a stdio server when there's http? Not to mention that WP-CLI is user-less, things should work without simulating a specific user.

I'm not pro or against exposing a server through STDIO transport. Personally, I will most likely never use this one, but this was stipulated on https://make.wordpress.org/ai/2025/07/17/mcp-adapter/.

Copy link
Member

@JasonTheAdams JasonTheAdams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing documentation here, @galatanovidiu! I haven't gone through the code yet, but the docs helped a lot in inferring how things work. Hopefully this helps with some conversation at a high level.

Comment on lines 122 to 123
- **RestTransport**: Deprecated
- **StreamableTransport**: Deprecated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be in favor of just removing these. It feels strange to include deprecated classes in the first release in WP 6.9.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mcp-adapter will not be included in WordPress core, and there are no current plans to do so.
For now, it can be used either as a standalone plugin or as a package, depending on the use case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noting - It will be publicized alongside the 6. 9 release as a Core package offered by this team. So I would agree in removing these as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I can do this.
It will be a breaking change, but we might have more breaking changes. See my other comment here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

### Event Emission Pattern

#### Error Response vs Error Logging
The system emits events rather than storing counters:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by "storing counters" here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, this is a leftover from a previous version of the docs.
There used to be an observer that counted events, but I removed it since it wasn’t particularly useful, and it was not working properly anyway.
I’ll revise this section to avoid confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed that description as it was unclear and unnecessary.

public static function instance(): self {
if (!isset(self::$instance)) {
self::$instance = new self();
add_action('rest_api_init', [self::$instance, 'init'], 15);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish we could use the Service Provider pattern to reduce instantiation side-effects like this.

What would you think about adding a separate initialize() method, so it's:

McpAdapter::instance()->initialize()

That way we can at least instantiate without side-effects.

Comment on lines 249 to 252
All components support annotations in `meta.annotations`:
- **Universal**: `priority`, `readOnlyHint`, `destructiveHint`, `idempotentHint`, `openWorldHint`
- **Resources**: `audience`, `lastModified` (MCP specification)
- **Prompts**: Support both template-level and message content annotations
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be useful to link out to the MCP docs here, such as https://modelcontextprotocol.io/specification/2025-06-18/server/resources#annotations

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've redone this section.
I've removed the list as it will be best to get the details on what annotations are available and how to use them directly from the MCP specifications.

Comment on lines 113 to +125
public function create_mcp_server( $adapter ) {
// Your server creation code here
$adapter->create_server(
'my-plugin-server',
'my-plugin',
'mcp',
'My Plugin MCP Server',
'Custom MCP server for my plugin',
'1.0.0',
[ \WP\MCP\Transport\HttpTransport::class ],
\WP\MCP\Infrastructure\ErrorHandling\ErrorLogMcpErrorHandler::class,
[ 'my-plugin/get-posts' ]
);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The presence of this here makes it seem like creating a per-plugin server is recommended, but my understanding is that it's really not. I'm not personally aware of any benefits of having many MCP servers running over just one, especially with tool layering.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should keep this functionality available, but we shouldn’t encourage creating a server per plugin as the default approach. In most cases, a single MCP server is sufficient and avoids unnecessary complexity. That said, there may still be edge cases where bootstrapping your own server makes sense.

@JamesLepage @swissspidy @gziolo @justlevine — curious to hear your thoughts.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to instantiate a shared global instance of the default MCP server? This way we would promote a simplified usage that is going to be sufficient to most cases. The existing example for a plugin-specific server could be included in the Advanced Usage section.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that the default implementation and basic usage should Have one MCP server, especially because they will solve the grouping question at some point with future protocol versions.

But at the same time, I don't think we should bury the fact that there can be multiple MCP implementations. I'm thinking specifically in the WooCommerce context.

It makes a lot of sense for Woo to have its own server, specifically aimed at agentic commerce, as an example. In fact, that's almost a necessity, and I could see that being the case with other plugins as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the approach of a global out of the box server. I mentioned using the layered approach as that fallback server somewhere, and still think that's a great thing to pursue.

## Creating Resources

Create consistent error responses that AI agents can understand:
Resources provide access to data or content. They require a `uri` in the meta field:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm mildly concerned that the way a tool/resource/prompt is determined is by inferring it from the parameters such as uri. I wonder if it would be better to have an mcp_component_type meta parameter that explicitly sets the type. This would be more clearly polymorphic and provide better validation for the intended component type.

But maybe I'm missing something. I'm just not seeing how the component type is clearly determined in the examples below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you’re absolutely right.

Currently, the default server exposes all capabilities as MCP tools, which is not correct. We definitely need a way to differentiate between component types (tools, resources, prompts, etc.).

While implementing this isn’t a huge technical task, there are a few ongoing discussions around the abilities API and filtering/built-in metas, so I’ve been holding off until those are settled.

The only MCP-related meta I'm currently checking is the public_mcp; However, I think we need a more structured approach for MCP-related metadata. Example:

{
...
"meta": {
    "show_in_rest": true,
    "mcp": {
      "public": true,
      "type": "tool",
      ...
    }
  }
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels like we need to resolve the existing issue in the Abilities API that will help to reason about resources vs tools in a more general way:

For the rest of potential meta annotations, it would be helpful to finish a filtering capability for ability registration so developers can inject some of these settings:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another solution will be to remove the concept of resources for now (both on abilities-api and on the MCP adaptor) and implement it in a future version.

The resources, in my opinion, are a nice-to-have feature, but not a deal breaker.

In MCP, the resources are not things that an agent will query/execute, or choose when to use them. They are static content that a user can attach to the prompt if and when they want. At least this is how it is implemented on all the MCP clients I've tried.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, starting only with tools is also a viable strategy to start with 👍🏻

Copy link
Contributor

@Jameswlepage Jameswlepage Oct 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gziolo @galatanovidiu What's the most current thinking here?

The resources, in my opinion, are a nice-to-have feature, but not a deal breaker.

I'd agree with this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My current thinking here is to add the meta.mcp parameters, even if we introduce breaking changes.

For those who already use mcp-adapter v0.1.0, we can create migration documentation covering:

  • How to update existing abilities with the new meta.mcp.type structure
  • Changes needed for custom server definitions
  • Migration path from inference-based type determination to explicit type declaration

For abilities-api default abilities, this is also very important if we want them exposed as MCP features.

If these abilities will be included in WP 6.9, it's critical to build them the right way from the beginning. Once they're in core, changing the metadata structure becomes much more difficult due to backward compatibility requirements.

Since we're still at v0.1.0, now is the right time to make this architectural improvement rather than being stuck with parameter inference forever. The explicit type declaration will make the codebase much clearer and more maintainable going forward.

Comment on lines 9 to 10
-**`RestTransport`** - Deprecated, use `HttpTransport`
-**`StreamableTransport`** - Deprecated, use `HttpTransport`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As said above, I think these should be outright removed, not deprecated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment on lines 87 to 93
### Error Codes

The system uses standard JSON-RPC error codes:
Standard JSON-RPC and MCP-specific error codes:

```php
// Standard JSON-RPC codes (-32768 to -32000)
// Standard JSON-RPC codes
const PARSE_ERROR = -32700;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where are these constants defined to be referenced? McpErrorFactory?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, they are defined on McpErrorFactory

Revise the observability guide to provide practical implementation details for using custom handlers.

The previous high-level overview of the event emission pattern is replaced with concrete code examples. The new documentation demonstrates how to configure a custom handler for both the default server (via a filter) and for custom-created servers.

This change also corrects missing closing fences on several code blocks.
Revised the description of the event emission pattern in the observability documentation. The MCP Adapter now specifies that it emits individual events as they happen to handlers, enhancing clarity on the event flow.
…prove value handling

Updated the McpObservabilityHelperTrait to allow a longer key length of 64 characters and modified the value processing to handle non-scalar values by encoding them as JSON. This change aims to improve logging efficiency while ensuring sensitive information is redacted.
Updated the visibility of the request_handler property in the HttpTransport class from private to protected. This change allows for better extensibility and access in subclasses.
Updated the ToolsHandler class to differentiate between protocol errors and tool execution errors. Protocol errors are now returned directly as JSON-RPC errors, while tool execution errors are handled as successful responses with an 'isError' flag set to true, following the MCP specification.
…rsion

Modified the mcp_error_to_http_status method to accept both integer and string error codes, enhancing flexibility in error handling. This change ensures that the method can accommodate a wider range of MCP/JSON-RPC error codes while maintaining existing logic for integer codes.
Added a check to handle cases where wp_json_encode may return false, ensuring that the value is set to an empty string in such scenarios.
…S error code

Modified comments and variable names in the ErrorResponseConsistencyTest to clarify that the tests are focused on parameter validation errors (INVALID_PARAMS).
Introduced new test methods for the convenience wrappers in McpErrorFactory, including tests for missing parameters, invalid parameters, and disabled MCP errors. Updated comments to clarify the purpose of each test and their relation to standard error codes.
…rom being added

Updated the add_session_header_to_response method to use a static flag, ensuring that the session header filter is only added once per request or when the session ID changes.
@JasonTheAdams
Copy link
Member

@galatanovidiu Given all the testing that's been done here and since we want to view 0.x as an iterative alpha, I'm going to suggest that we remove the deprecated code here to remove technical debt and merge this. Let me know once you've cleaned up the deprecated things and I'll give it a list quick review and approve. 👍

This guide documents the transport layer refactor introduced in version 0.3.0.

It explains the removal of `RestTransport` and `StreamableTransport` and the consolidation into a single, unified `HttpTransport`. The document provides code examples for using the new transport, implementing custom authentication, and creating custom transport layers for advanced use cases.
@galatanovidiu
Copy link
Contributor Author

galatanovidiu commented Oct 1, 2025

@galatanovidiu Given all the testing that's been done here and since we want to view 0.x as an iterative alpha, I'm going to suggest that we remove the deprecated code here to remove technical debt and merge this. Let me know once you've cleaned up the deprecated things and I'll give it a list quick review and approve. 👍

@JasonTheAdams Done

@JasonTheAdams JasonTheAdams self-requested a review October 1, 2025 21:25
Copy link
Member

@JasonTheAdams JasonTheAdams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I glanced through a bunch of the code with the mind that we'll iterate more later. I have a few questions for you before proceeding, @galatanovidiu!

Comment on lines +222 to +225
'mcp' => [
'public' => true, // Expose this ability via MCP
'type' => 'prompt' // Mark as prompt for auto-discovery
]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we relying on this structure in this PR? Or just documenting the intended structure?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'm relying on this structure. This resolves the "tool/resource/prompt is determined by inferring it from the parameters" problem.

  • The public parameter will determine if the ability will be used on the default MCP server, or not.
  • The type parameter defines the kind of ability being registered, in this case, 'prompt' enables auto-discovery as a prompt-type ability.

This makes the behavior explicit instead of relying on implicit inference.

@gziolo what do you think about this

Copy link
Member

@gziolo gziolo Oct 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For MCP specific behaviors, this structure makes perfect sense 👍🏻

In the Abilities API, there is already a basic handling that helps to distinguish resource from tool based on meta.type:

https://github.com/WordPress/abilities-api/blob/80f47ee41bc8c92bde11b59941c001c617513cf6/packages/client/src/api.ts#L179-L180

https://github.com/WordPress/abilities-api/blob/80f47ee41bc8c92bde11b59941c001c617513cf6/includes/rest-api/endpoints/class-wp-rest-abilities-run-controller.php#L95

It's mostly to choose between GET vs POST request method. My current thinking is that we should generalize that differently using hints as discussed in:

This way, a prompt would be marked as hints.readOnly, which would resolve to GET method internally for handling abilities with REST API, and mpc.type set to prompt would provide additional details for the MCP adapter. That ensures there is no duplication between Abilities API and MCP for deciding on tool vs resource vs prompt which matters mostly for MCP today.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened a PR to better illustrate my feedback:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great to leverage the same metadata in MCP, but we need to address a fundamental terminology mismatch first.

The core issue: WordPress Abilities API and MCP use "tools" and "resources" to mean completely different things:

WordPress Abilities API:

Tools = callable functions that modify data
Resources = callable functions that read data

MCP:

Tools = callable functions that the AI invokes automatically when needed
Resources = static context that users manually attach to conversations

So WordPress's "tool vs resource" distinction doesn't map to MCP's "tool vs resource"

Concrete example: A site-info ability could be implemented as:

MCP tool: AI calls it when it needs current site data during conversation
MCP resource: User pre-attaches site info as reference context

The choice depends on the usage pattern (AI-driven vs. user-driven), not the type of action.

Copy link
Member

@gziolo gziolo Oct 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's yet another reason to stop using tools and resources nomenclature inside the Abilities API and leave it for MCP. In fact, it's what the PR I shared is trying to achieve using annotations instead.


The default server includes three core abilities that provide MCP functionality:

### Layered Tooling Architecture
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a note, we'll want to update this doc in a subsequent PR that adds support for categories into layering.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh. Do we actually want to register the layering tools as abilities? It feels like we shouldn't touch the abilities with something that's exclusively for how we structure the MCP server. I figured we'd manually register these tools apart from the Abilities API.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had this discussion with @Jameswlepage
My first implementation utilized custom tools, but I've since modified the functionality to use the abilities.
This makes the implementation cleaner.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a comment https://github.com/WordPress/mcp-adapter/pull/48/files#r2368119081 suggesting that we consider creating a local abilities registry for these 3 top-layer tools, and keep it referenced locally in MCP. Nearly the same way as in unit tests:

$registry = new WP_Abilities_Registry();
$registry->register( 'mcp-adapter/discover-abilities', $args );
// etc..

However, I'm still not entirely sure how it fits in the current architecture.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will give this a try

Comment on lines 55 to 59
$enable_serve = apply_filters( 'mcp_adapter_enable_stdio_transport', true );

if ( ! $enable_serve ) {
\WP_CLI::error( 'The STDIO transport is disabled. Enable it by setting the "mcp_adapter_enable_stdio_transport" filter to true.' );
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be examined within the transport itself? Or at another layer? Feels like this check shouldn't be exclusive to WP CLI.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're absolutely right! The check should be in StdioServerBridge itself, not exclusive to WP-CLI. This would match the pattern we use in HttpTransport, where permission checks are part of the transport layer.

I will look into this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes done. Please check

$this->mcp_server = $mcp_server;
$this->error_handler = $error_handler;
/** @var class-string<\WP\MCP\Infrastructure\Observability\Contracts\McpObservabilityHandlerInterface> $observability_handler */
$this->observability_handler = $observability_handler;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious why we're passing the FQCN instead of an instance of the handler?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The observability handler is passed as an FQCN because it implements only static methods record_event(), record_timing().

We can review both observability and error handlers and modify them in subsequent PR's.

Refactors the validation for the STDIO transport by moving the check from the WP-CLI command to the `StdioServerBridge`.

This change improves separation of concerns, as the bridge is now responsible for ensuring its own preconditions are met. The bridge throws a `RuntimeException` if the transport is disabled, which is then caught by the command and reported to the user as a WP-CLI error.

Unit tests are updated to reflect this new division of responsibility.
This update introduces a metadata-driven observability system, consolidating event naming and status tagging for improved consistency. Key changes include:

- Refactored event recording methods to use instance methods instead of static methods.
- Unified event names with a `status` tag for easier filtering and aggregation.
- Enhanced documentation to reflect the new architecture and best practices for querying events.
- Updated various handlers and tests to align with the new observability structure.

These changes streamline observability tracking and improve the overall clarity of event data.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants