Repository navigation
fix: report MCP isError as ToolFailure on the external URL path - #7643
哈基米 (BlueX888) wants to merge 1 commit into
Conversation
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthrough
ChangesMCP error handling
Sequence Diagram(s)sequenceDiagram
participant MCPToolWrapper
participant MCPServer
participant ToolFailure
MCPToolWrapper->>MCPServer: call_tool
MCPServer-->>MCPToolWrapper: CallToolResult with content and isError
MCPToolWrapper->>ToolFailure: create MCP_ERROR failure when isError is true
ToolFailure-->>MCPToolWrapper: structured failure
Priority: ⬇️ Low 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
0f1d751 to
a3e5334
Compare
|
The |
MCPToolWrapper returned the error text as a plain str when a server answered tools/call with isError=true, so the framework recorded the call as a success: no ToolFailureDetectedEvent, no TaskOutput.has_tool_failures. MCPNativeTool already reported the same server answer as ToolFailure(reason=MCP_ERROR).
a3e5334 to
74585e7
Compare
Summary
MCPToolWrapper(the external HTTPS URL resolution path used bymcp/tool_resolver.py::_resolve_external) returned the response text unconditionally from_execute_tooland never inspected theisErrorflag an MCP server sets on an otherwise successfultools/callresponse. The error text reached the framework as a plainstr, so the call was recorded as a success: noToolFailureDetectedEventandTaskOutput.has_tool_failuresstayed false.MCPNativeToolalready handles the same server answer by returningToolFailure(reason=MCP_ERROR)(lib/crewai/src/crewai/tools/mcp_native_tool.py:136-146).Changes
_execute_toolnow checksresult.isErrorafter extracting the content and returnsToolFailure(message=content, reason=ToolFailureReason.MCP_ERROR, details={"server": ..., "tool": ...}), matchingMCPNativeTool. Signatures along the retry path are widened tostr | ToolFailure. Successful calls are unchanged.Rebased onto main 38db11e. The conflict with #8300 is resolved by keeping that PR's
content_blocks_to_text(result.content)extraction and wrapping it in theisErrorcheck (lib/crewai/src/crewai/tools/mcp_tool_wrapper.py:194-208), so multi-block results and error reporting both hold.Verification
Tests added or updated for the changed behavior
Relevant tests and quality checks pass locally
python -m pytest lib/crewai/tests/tools/test_mcp_tool_wrapper.py lib/crewai/tests/mcp/test_tool_result_content.py -q -p no:randomly— 9 passedpython -m pytest lib/crewai/tests/tools/ lib/crewai/tests/mcp/ -q -p no:randomly— 347 passedWith the source change reverted and the new tests kept,
test_is_error_becomes_a_tool_failurefails withAssertionError: assert False + where False = isinstance('Error: file not found', ToolFailure), confirming the old code reported the server error as a successruff check lib/crewai/src/crewai/tools/mcp_tool_wrapper.py lib/crewai/tests/tools/test_mcp_tool_wrapper.py— All checks passedmypy lib/crewai/src/crewai/tools/mcp_tool_wrapper.py— Success: no issues found in 1 source fileAdditional context
This PR was authored by an AI coding agent. Per
.github/CONTRIBUTING.mdit should carry thellm-generatedlabel; I don't have permission to apply labels, so a maintainer would need to add it.