Skip to content

fix(agents): mark MCP tool failures as error results - #528

Draft
rioyu123 wants to merge 1 commit into
anthropics:mainfrom
rioyu123:fix/mcp-error-results
Draft

rioyu123 wants to merge 1 commit into
anthropics:mainfrom
rioyu123:fix/mcp-error-results

Conversation

@rioyu123

Copy link
Copy Markdown

Summary

MCP tool failures currently reach the agent as ordinary tool results. MCPTool.execute() ignores the SDK result's is_error flag, and connection exceptions are returned as strings, so the existing executor never sets tool_result.is_error.

This raises a RuntimeError for either failure, allowing the existing executor to set the error flag and preserve the diagnostic text. Successful output, the no-text fallback, and cancellation propagation stay unchanged. It does not add error-text guessing or change how successful text blocks are selected.

Coordination

This is a draft because #518 changes the same method to preserve multiple text blocks. That work is separate and should remain there. This patch also deliberately changes direct MCPTool.execute() failures from returned strings to exceptions, so #518's exception test would need to follow that contract if both changes land. The combination has not been tested.

Would this error propagation fit the example's intended contract? I can rebase on #518 once the order is settled, without duplicating its text-aggregation changes.

Test plan

  • Added 13 offline tests using real MCP CallToolResult and Anthropic ToolUseBlock objects through the existing execute_tools() path; only the connection call is mocked.
  • Covered serial and parallel execution, error results with and without text, connection errors, mixed success/failure calls, successful text containing the word "Error", the empty-success fallback, and cancellation on the parallel path.
  • Base: 9 failed, 4 passed. Fixed: 13 passed. Ruff check passes for the changed files.
  • Tested on Python 3.12.3 with Anthropic 2.0.0 and MCP 2.3.0, matching the documented dependency range. No live MCP server or Claude API session was used; the existing API-backed example test was not run. These offline tests were run locally, not by the repository's CI workflow.

@BichengWang BichengWang left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Validated b5cac80 offline with Python 3.12.15, Anthropic 2.2.0 and MCP 2.3.0. All 13 tests pass; against current main, the same tests produce 9 failures and 4 passes. A real local stdio MCP server also confirms that explicit errors, empty error results and server exceptions reach execute_tools() with is_error=True in both serial and parallel execution, while successful text containing “Error” remains a success. Additional cancellation probes pass in both modes.

The change preserves exception causes and the existing successful-output behavior; I found no blocking issues in this patch. The test file passes Ruff; the tool file has the same import-order warning on main and this head. I did not run the API-backed example or combine this with #518, so this approval covers the current patch in isolation and leaves the documented landing-order question open.

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.

2 participants