fix: propagate is_error flag from SDK MCP tool results#717
Conversation
The call_tool handler in create_sdk_mcp_server() was discarding the is_error field from tool result dicts, always returning content as a bare list which the MCP framework wrapped with isError=False. Return a CallToolResult directly so the is_error flag is preserved. The MCP framework already checks isinstance(results, CallToolResult) and passes it through as-is. Fixes #564
…ror in JSONRPC bridge Widen the content list type annotation in create_sdk_mcp_server to match CallToolResult's expected parameter type, fixing a mypy arg-type error caused by list invariance. Also fix the JSONRPC bridge in query.py to use camelCase 'isError' instead of snake_case 'is_error' when checking and setting the error flag on CallToolResult, which uses camelCase Pydantic field names.
…ror in JSONRPC bridge Widen the content list type annotation in create_sdk_mcp_server to match CallToolResult's expected parameter type, fixing a mypy arg-type error caused by list invariance. Also fix the JSONRPC bridge in query.py to use camelCase 'isError' instead of snake_case 'is_error', matching the Pydantic field name on CallToolResult.
There was a problem hiding this comment.
LGTM — the isError propagation fix and casing correction in query.py both look correct.
Extended reasoning...
Overview
This PR fixes is_error flag propagation from SDK MCP tool results across two code paths: (1) __init__.py now returns a CallToolResult directly instead of a bare content list, preserving the isError flag, and (2) query.py fixes the casing from is_error to isError in the JSONRPC bridge hasattr check and response dict key. A new test verifies the direct handler path.
Security risks
No security concerns. The changes are limited to error flag propagation in internal MCP tool handling — no auth, crypto, or permissions code is affected.
Level of scrutiny
This is a small, focused bug fix touching well-scoped internal plumbing. The pattern is straightforward: return a typed result object instead of a bare list, and fix an attribute name casing mismatch. The author addressed the casing bug I flagged in a prior review.
Other factors
The remaining bug report is a minor test coverage nit — the new test exercises the direct handler path but not the JSONRPC bridge path. This is a valid observation but does not indicate a code correctness issue; the actual code changes in both paths are correct. The PR includes 312 passing existing tests.
There was a problem hiding this comment.
LGTM — the camelCase fix in query.py addresses my earlier feedback, and the core fix in init.py is straightforward.
Extended reasoning...
Overview
This PR touches three files to fix is_error propagation from SDK MCP tool results:
__init__.py: ReturnsCallToolResult(content=content, isError=...)instead of a bare content list, so the MCP framework preserves the error flag.query.py: Fixes the JSONRPC bridge to use camelCaseisError(matching the Pydantic field name) instead of snake_caseis_error— this was the bug I reported in a previous review, now resolved.test_sdk_mcp_integration.py: Addstest_is_error_flag_propagatedcovering the direct handler path.
Security risks
None. The change only affects error flag propagation in tool results — no auth, crypto, or permissions code is touched.
Level of scrutiny
Low-to-medium. This is a small, focused bug fix with clear intent. The content type annotation expansion (AudioContent, ResourceLink, EmbeddedResource) aligns with what CallToolResult accepts. The query.py fix is a mechanical camelCase correction.
Other factors
My previous review flagged the snake_case bug in query.py lines 503-504, which has been addressed in the current diff (inline comment resolved). The remaining suggestion about adding a JSONRPC bridge path test is a minor coverage gap, not a correctness issue — both code paths are correct. No outstanding reviewer comments remain unaddressed.
There was a problem hiding this comment.
LGTM — the isError propagation fix is correct in both code paths, and the prior snake_case bug in query.py has been addressed.
Extended reasoning...
Overview
This PR fixes is_error flag propagation across two code paths: the direct call_tool handler in __init__.py (now returns CallToolResult directly instead of a bare content list) and the JSONRPC bridge in query.py (fixed snake_case is_error to camelCase isError to match the Pydantic field name). A new test covers the direct handler path.
Security risks
No security concerns. The change is about error flag propagation for MCP tool results — no auth, crypto, or permissions code is touched.
Level of scrutiny
This is a straightforward bug fix with clear intent. The scope is small (3 files, ~30 lines of meaningful change), the fix follows existing patterns, and the correctness is easy to verify by inspection. The content type annotation broadening (AudioContent, ResourceLink, EmbeddedResource) aligns with MCP library types and is a safe improvement.
Other factors
My previous red bug report (snake_case is_error in query.py) has been addressed in this revision. Both inline comments are marked resolved. The test coverage suggestion (testing the JSONRPC bridge path) was a minor improvement suggestion, not a blocker. The existing test_error_handling and new test_is_error_flag_propagated tests provide reasonable coverage for the fix.
E2E Test Results for PR #717SummaryTested the Unit TestsAll 9 SDK MCP integration tests pass, including the new E2E TestCreated an SDK MCP server with two tools:
Asked Claude to call both tools and summarize results. Claude correctly distinguished between them:
What the fix doesTwo changes make this work:
Previously, VerdictPASS -- both unit tests and e2e test confirm the fix works as intended. |
create_sdk_mcp_server tool handlers return CallToolResult objects since v0.1.51 (PR #717). mcp<1.19.0 cannot handle this return type from @server.call_tool() decorated functions — it falls through the iterable branch, fails pydantic validation, and silently swallows the tool output into an error result. Users upgrading claude-agent-sdk in an existing env with old mcp would see in-process SDK MCP tools run but return validation error blobs instead of actual results.
## Problem `create_sdk_mcp_server` tool handlers have returned `CallToolResult` objects since v0.1.51 (PR #717). The `mcp` package's `@server.call_tool()` decorator only accepts `CallToolResult` returns from `mcp>=1.19.0`. With older versions it falls through the iterable branch (pydantic models iterate as `(field, value)` tuples), fails validation, and swallows the result into `_make_error_result(str(e))`. Effect: in-process SDK MCP tools run their handler, but the model receives a ~5KB pydantic "20 validation errors for CallToolResult" blob with `is_error=True` instead of the actual output. Silent data loss. This only bites users who `pip install -U claude-agent-sdk` in an existing env — fresh installs pull latest `mcp` and work fine. stdio/HTTP/SSE MCP servers are unaffected. ## Fix Bump the floor: `mcp>=0.1.0` → `mcp>=1.19.0`. ## Verification Boundary-tested with a direct `request_handlers[CallToolRequest]` repro: | mcp version | result | |---|---| | 1.12.4 | ❌ `isError: True`, validation error blob | | 1.18.0 | ❌ `isError: True`, validation error blob | | **1.19.0** | ✅ `isError: False`, tool output returned | | 1.27.0 | ✅ `isError: False`, tool output returned | - `pip install -e . 'mcp==1.12.4'` → `ResolutionImpossible` (pin enforced) - Full `ClaudeSDKClient` e2e with `create_sdk_mcp_server`: tool result reaches the model - `pytest`: 755 passed, 3 skipped · `mypy`: clean · `ruff`: clean Reported internally.
Summary
The
call_toolhandler increate_sdk_mcp_server()was discarding theis_errorfield from tool result dicts. It returned content as a bare list, which the MCP framework then wrapped in aCallToolResult(isError=False)regardless of what the tool returned.This meant tools that returned
{"content": [...], "is_error": True}would have their error status silently dropped — Claude would see the error text but treat it as a successful result, breaking error recovery logic.Fix
Return a
CallToolResultdirectly instead of a bare content list. The MCP framework already checksisinstance(results, CallToolResult)and passes it through as-is (confirmed in MCP server source).Before:
After:
The companion code path in
_internal/query.py(lines 503-504) already correctly propagatesis_error, confirming this was an oversight in the MCP server handler path.Test
Added
test_is_error_flag_propagatedtotests/test_sdk_mcp_integration.py:is_error: Trueis propagated through the full handler chainisError=FalseAll 312 existing tests continue to pass.
Fixes #564