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 81.87% 81.88% +0.01%
- Complexity 933 934 +1
============================================
Files 49 49
Lines 3216 3218 +2
============================================
+ Hits 2633 2635 +2
Misses 583 583
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.
Resolved conflict in tests/Unit/Handlers/ToolsHandlerTest.php by keeping both new tests: test_call_tool_with_string_error_from_execute (from this branch) and test_call_tool_wraps_scalar_return_values (from trunk).
galatanovidiu
left a comment
galatanovidiu
left a comment
There was a problem hiding this comment.
LGTM! The fix correctly handles the inconsistent error formats between ability implementations. The type check elegantly handles both:
- String errors (from ExecuteAbilityAbility)
- Array errors with message key (from other abilities)
Good test coverage with test_call_tool_with_string_error_from_execute. Resolved the merge conflict by keeping both this PR's test and test_call_tool_wraps_scalar_return_values from trunk. All 587 tests pass.
Note: This is a targeted hotfix for 0.4.1. A more comprehensive solution addressing the underlying error format inconsistency will land in 0.5.0 as part of the architecture improvements.
2 participants
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?