Fix WP_Error handling in PromptsHandler and ResourcesHandler #74
Conversation
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
Add proper WP_Error detection after ability execution in PromptsHandler and ResourcesHandler to prevent crashes and return graceful error responses.
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR adds proper WP_Error handling to PromptsHandler and ResourcesHandler to prevent type errors when WordPress abilities return error objects. Previously, both handlers assumed ability->execute() always returned arrays, causing crashes when WP_Error objects were returned during validation failures or operational errors.
Key Changes:
- Added
WP_Errordetection and graceful error handling in both handlers afterability->execute()calls - Implemented MCP-compliant error responses with detailed metadata for debugging
- Aligned error handling pattern with existing
ToolsHandlerimplementation
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| includes/Handlers/Resources/ResourcesHandler.php | Added WP_Error detection after ability execution with logging and MCP error response |
| includes/Handlers/Prompts/PromptsHandler.php | Added WP_Error detection after ability execution with logging and MCP error response |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
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 #74 +/- ##
============================================
- Coverage 68.94% 68.11% -0.84%
- Complexity 821 823 +2
============================================
Files 46 46
Lines 3037 3077 +40
============================================
+ Hits 2094 2096 +2
- Misses 943 981 +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:
|
galatanovidiu
added a commit
that referenced
this pull request
Oct 22, 2025
Add proper WP_Error detection after ability execution in PromptsHandler and ResourcesHandler to prevent crashes and return graceful error responses.
galatanovidiu
added a commit
that referenced
this pull request
Nov 5, 2025
* Refactor Core layer to use WP_Error instead of exceptions Replace exception-based error handling with WordPress-native WP_Error pattern in the Core layer components. Changes: - McpAdapter::create_server() now returns self|WP_Error instead of throwing exceptions for validation failures (invalid error handler, observability handler, timing, and duplicate server IDs) - DefaultServerFactory::create() adds is_wp_error() check and logs errors - McpComponentRegistry removes 4 try-catch blocks from register_tools(), add_tool(), register_resources(), and register_prompts() - McpTransportFactory logs errors and continues processing other transports instead of throwing exceptions - Update McpTransportFactoryTest to expect graceful error handling Error codes introduced: - invalid_error_handler - invalid_observability_handler - invalid_timing - duplicate_server_id All 357 tests passing. Related to #24 * Refactor Domain layer validators to use WP_Error instead of exceptions Convert all domain validators and validate() methods to return WP_Error instead of throwing exceptions, following WordPress conventions. Changes: - All validator methods (validate_tool_data, validate_tool_instance, validate_tool_uniqueness, and equivalents for Resources and Prompts) now return bool|WP_Error instead of void - McpTool::get_ability() returns WP_Ability|WP_Error instead of throwing InvalidArgumentException - McpTool::validate(), McpResource::validate(), and McpPrompt::validate() return self|WP_Error instead of throwing - ToolsHandler::handle_tool_call() adds is_wp_error() check after calling get_ability() - Add assertWPError() and assertNotWPError() helper methods to TestCase - Update 37 validator tests across 3 test files to expect WP_Error instead of exceptions Error codes introduced: - tool_validation_failed, tool_not_unique - resource_validation_failed, resource_not_unique - prompt_validation_failed, prompt_not_unique - ability_not_found All 357 tests passing with 1521 assertions. Related to #24 * Refactor registration classes to propagate WP_Error instead of throwing exceptions Convert all RegisterAbilityAs* classes to return WP_Error instead of throwing exceptions when converting WordPress abilities to MCP components. Changes: - RegisterAbilityAsMcpTool::make() returns McpTool|WP_Error - RegisterAbilityAsMcpResource::make() returns McpResource|WP_Error - RegisterAbilityAsMcpResource::get_uri() returns string|WP_Error instead of throwing InvalidArgumentException - RegisterAbilityAsMcpResource::get_data() propagates WP_Error from get_uri() - RegisterAbilityAsMcpPrompt::make() returns McpPrompt|WP_Error - Remove all @throws annotations from registration classes Error propagation pattern: - Check is_wp_error() at each method in the chain - Return WP_Error immediately if found - Allow from_array() validation errors to bubble up naturally This completes the elimination of all exception throws in the registration layer, with errors now properly propagated as WP_Error objects. All 357 tests passing with 1521 assertions. Related to #24 * Add WP_Error handling in Prompts and Resources handlers Add is_wp_error() checks in handlers after get_ability() calls to properly handle errors propagated from the registration and domain layers. Changes: - PromptsHandler::get_prompt() checks is_wp_error() after get_ability() for non-builder prompts - ResourcesHandler::read_resource() checks is_wp_error() after get_ability() - Both handlers log errors and return proper MCP error responses with metadata including error_code and failure_reason - Error responses include 'ability_retrieval_failed' as failure_reason This completes the refactoring to eliminate all exception-based error handling in favor of WordPress's native WP_Error pattern. The codebase now has: - Zero exception throws in production code (7 eliminated) - Zero try-catch blocks for InvalidArgumentException (4 removed) - Consistent WP_Error propagation from Core → Domain → Registration → Handlers All 357 tests passing with 1521 assertions. Fixes #24 * Fix PHPStan errors by making all get_ability() methods return WP_Error Ensure consistency across all MCP component types by making get_ability() return WP_Ability|WP_Error instead of nullable WP_Ability. Changes: - McpResource::get_ability() now returns WP_Ability|WP_Error instead of ?WP_Ability (null) - McpPrompt::get_ability() now returns WP_Ability|WP_Error instead of ?WP_Ability (null) - McpPromptBuilder anonymous class get_ability() returns WP_Error with 'builder_has_no_ability' error code - Update 2 tests to expect WP_Error instead of null from builder prompts This ensures PHPStan type checking passes and all get_ability() methods across the codebase follow the same error handling pattern. All 357 tests passing with 1523 assertions. PHPStan Level 8 analysis passing with no errors. Related to #24 * Refactor error logging in DefaultServerFactory to use _doing_it_wrong. * Remove unused WP_Ability imports from McpPrompt, McpPromptBuilder, and McpResource files. Update error logging in DefaultServerFactory to escape error messages and codes. * Update error logging in DefaultServerFactory to ensure error codes are cast to string. * Adds unit tests for handlers and component registry Introduces comprehensive unit test suites for the `ToolsHandler` and `ResourcesHandler` to verify their core functionality. These tests cover success paths, error conditions like missing parameters or components not being found, and permission handling. Additionally, enhances the `McpComponentRegistry` tests to ensure that non-string inputs are correctly skipped during registration and that no observability events are recorded when the feature is disabled. * Fix tool error handling to comply with MCP specification Implement proper distinction between protocol errors and tool execution errors according to MCP spec (https://modelcontextprotocol.io/specification/2025-06-18/server/tools#error-handling): Protocol errors (JSON-RPC format with 'error' key): - Tool not found - Missing required parameters - Internal errors (ability retrieval failures) Tool execution errors (isError: true format): - Permission denied - Permission check failures - Execution failures - WP_Error from ability execution Changes: - ToolsHandler::call_tool() now distinguishes error types based on failure_reason metadata - Permission denied, permission check failures, and execution errors now return {content: [{type: 'text', text: '...'}], isError: true} - Protocol errors still return JSON-RPC error format - Update 7 tests to expect isError format for tool execution errors - Remove impossible is_wp_error() check (PHPStan was correct) All 357 tests passing with 1527 assertions. PHPStan Level 8 analysis passing with no errors. Fixes #69 * Fix WP_Error handling in Prompts and Resources handlers (#74) Add proper WP_Error detection after ability execution in PromptsHandler and ResourcesHandler to prevent crashes and return graceful error responses. * Fix permission error tests to align with MCP specification Updates test expectations for permission-related errors in ToolsHandler to use isError format instead of JSON-RPC error format. Per MCP spec: "Any errors that originate from the tool SHOULD be reported inside the result object, with isError set to true, not as an MCP protocol-level error response. Otherwise, the LLM would not be able to see that an error occurred and self-correct." Permission errors originate from the tool's permission check phase, so they should be returned as tool execution errors (isError: true) rather than protocol-level errors. This allows LLMs to observe and potentially handle permission failures. Changes: - test_call_tool_permission_exception_returns_error: Now expects isError - test_call_tool_permission_denied_returns_error: Now expects isError - Added spec-based comments explaining the error handling approach * Fixed backwards conditional logic in TestCase::set_up_before_class() * Removes a comment * Adds assertions for registered abilities Improves test robustness by adding assertions to ensure abilities are registered before being used in tests. This prevents unexpected errors and provides more informative feedback when abilities are not correctly registered. * Guards prompt building against exceptions Improves the stability of prompt registration by wrapping the prompt builder instantiation and build process in a try-catch block. This prevents a single prompt builder exception from halting the entire component registration process. When an exception occurs, it logs the error and records a failure event, allowing the system to continue registering other components. * Adds return type declaration Adds a return type declaration to the `get_ability` method for better type hinting. * Improves developer error handling and testing Enhances developer experience by providing more informative error messages and adding new tests to ensure proper error handling in various scenarios. Adds a mechanism to capture `_doing_it_wrong` calls for more robust testing of developer-facing errors. This allows for assertions that specific functions trigger `_doing_it_wrong` with the expected messages, leading to more reliable detection of incorrect API usage. Specifically, the changes cover: - Creating servers outside of the `mcp_adapter_init` hook. - Using duplicate server IDs. - Using non-existent or invalid transport classes. - Cloning and waking up the Plugin singleton. These improvements help developers identify and fix integration issues more easily, resulting in a more stable and maintainable system. * Improves error handling in prompt builder Ensures that errors during prompt building are caught more broadly by catching `\Throwable` instead of just `\Exception`. This allows the system to handle a wider range of potential issues that might arise when constructing prompts, improving stability and providing more informative error logs. * Registers abilities during the API init hook Ensures abilities are registered within the `wp_abilities_api_init` hook. This is necessary as the WordPress abilities API checks if the hook is currently running. Adds a helper function `register_ability_in_hook` to facilitate registering abilities correctly. * Refactors ability registration in tests Refactors the way abilities are registered in tests to ensure proper cleanup and prevent potential duplicate registrations when the `wp_abilities_api_init` hook is fired multiple times. It achieves this by using a static function for the callback and registers the ability using the `register_ability_in_hook` method. * Enhances ability registration tests Improves the testing framework for ability registration by adding assertions to verify that abilities are correctly registered before use. This change aims to prevent unexpected errors during tests and provides clearer feedback when registration issues occur, contributing to a more reliable testing environment. * Enables MCP component validation during registration Implements validation checks for MCP components (tools and prompts) during registration, ensuring that components meet the required criteria before being added to the system. This change ensures that the validation flag on the McpComponentRegistry is respected by directly calling the validators, and that components are validated during registration and when added individually. This improves the reliability and consistency of the MCP system by preventing the registration of invalid components. It also records component registration failures for observability purposes. Adds the ability to set the MCP server on the McpResource. * Enhances error handling in MCP component tests Implements additional tests for error scenarios in MCP components, including handling WP_Error and exceptions during ability execution, resource reading, and tool calls. This change ensures that the system correctly identifies and reports errors, improving the reliability of the MCP framework. It also verifies that appropriate error messages and metadata are returned, contributing to better observability and debugging capabilities.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Why
WordPress abilities can legitimately return
WP_Errorobjects when validation fails or operations encounter errors. Currently,PromptsHandlerandResourcesHandlerassumeability->execute()always returns arrays, causing type errors that crash the MCP server instead of returning graceful error responses.What
This PR adds proper
WP_Errordetection and handling in both handlers afterability->execute()calls:WP_Errorafter execution and return MCP-compliant error responseWP_Errorafter execution and return MCP-compliant error responseBoth handlers now:
WP_Errorobjects usingis_wp_error()This matches the existing pattern already implemented in
ToolsHandler.Fixes #39