Properly return error messages when using unnested error formats in ToolsHandler
#90
Conversation
|
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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## trunk #90 +/- ##
============================================
+ Coverage 79.74% 79.75% +0.01%
- Complexity 858 859 +1
============================================
Files 46 46
Lines 3080 3082 +2
============================================
+ Hits 2456 2458 +2
Misses 624 624
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR enhances error handling in the ToolsHandler by adding support for both string and array error formats. The change provides defensive handling for $result['error'] values, checking if the error is a string before attempting to access it as an array.
Key Changes:
- Added type checking for
$result['error']to handle both string and array formats - Maintains backward compatibility with existing array-based error handling
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if ( is_string( $result['error'] ) ) { | ||
| $error_message = $result['error']; | ||
| } else { | ||
| $error_message = $result['error']['message'] ?? 'An error occurred while executing the tool.'; | ||
| } |
There was a problem hiding this comment.
This change adds handling for $result['error'] as a string, but all error returns in handle_tool_call() and throughout the codebase consistently use array format with ['code' => int, 'message' => string] structure via McpErrorFactory methods.
Could you clarify when $result['error'] would be a string? Based on reviewing:
- Lines 272, 299, 329, 348: All use
McpErrorFactory::*()['error']which returns arrays - Lines 376-379: WP_Error case returns
['message' => ..., 'code' => ...]as an array - Lines 409-411: Exception case returns
['message' => ...]as an array
If this is defensive programming for external tool implementations or future scenarios, consider documenting this with a comment explaining when string errors might occur.
Fixes #89, making
ToolsHandleraccept both nested (as seen inhandle_tool_call) and unnested error formats.However, this fix surfaces an inconsistency in error formatting and raises a bigger question: should
handle_tool_call,ExecuteAbilityAbility,DiscoverAbilitiesAbility, andGetAbilityInfoAbilityuse the same format, either nested or not?