fix(shared): make McpError and UrlElicitationRequiredError pickle-safe on v1.x#2703
Open
NishchayMahor wants to merge 1 commit into
Open
fix(shared): make McpError and UrlElicitationRequiredError pickle-safe on v1.x#2703NishchayMahor wants to merge 1 commit into
NishchayMahor wants to merge 1 commit into
Conversation
…e on v1.x `McpError.__init__(error)` calls `super().__init__(error.message)`, which sets `Exception.args` to `(message_str,)`. The default pickle path then reconstructs by calling `cls(*self.args)`, so unpickling a `McpError` invokes `McpError(message_str)` and crashes with `AttributeError: 'str' object has no attribute 'message'`. `UrlElicitationRequiredError` inherits the same `args` and additionally has a different signature `(elicitations, message=None)`, so its unpickle path also binds the message string to the `elicitations` parameter and errors out. Add `__reduce__` to both classes so pickle reconstructs from the typed `ErrorData` / `_elicitations` fields and the round-trip preserves the high-level payload rather than the raw wire-format args. Existing call sites and the `from_error` constructor are unchanged. A partial fix already landed on `main` for `McpError` (#XXXX) and modelcontextprotocol#2555 is in review for `UrlElicitationRequiredError` on `main`. This change covers the v1.x release branch where both classes are still broken. Github-Issue: modelcontextprotocol#2431
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
`McpError.init(error)` calls `super().init(error.message)`, which sets `Exception.args` to `(message_str,)`. The default pickle path reconstructs an exception by calling `cls(*self.args)`, so unpickling a `McpError` invokes `McpError(message_str)` and crashes with:
```text
AttributeError: 'str' object has no attribute 'message'
```
`UrlElicitationRequiredError` inherits the same `args` and additionally has a different signature `(elicitations, message=None)`, so its unpickle path also binds the message string to the `elicitations` parameter and errors out the same way.
This PR adds `reduce` to both classes so pickle reconstructs from the typed `ErrorData` / `_elicitations` fields, and the round-trip preserves the high-level payload rather than the raw wire-format `args`. Existing call sites and the `from_error` constructor are unchanged.
Repro on `v1.x`
```python
import pickle
from mcp.shared.exceptions import McpError, UrlElicitationRequiredError
from mcp.types import ElicitRequestURLParams, ErrorData
McpError — fails on unpickle
pickle.loads(pickle.dumps(McpError(ErrorData(code=-32600, message="Invalid Request"))))
AttributeError: 'str' object has no attribute 'message'
UrlElicitationRequiredError — fails on unpickle
e = UrlElicitationRequiredError([ElicitRequestURLParams(
mode="url", message="x", url="https://example.com", elicitationId="t-1"
)])
pickle.loads(pickle.dumps(e))
TypeError: takes from 2 to 3 positional arguments but 4 were given
```
Scope vs. existing work
A partial fix already landed on `main` for `McpError` (pickle-safe there now). PR #2555 is in review for `UrlElicitationRequiredError` on `main`. This change covers the `v1.x` release branch where both classes are still broken — per the Anthropic-team comment on #2431 ("both `McpError` and `UrlElicitationRequiredError` fail on v1.x").
If maintainers prefer to land this as a forward-port from main's `McpError` patch plus #2555's approach for `UrlElicitationRequiredError`, I can rebase or close — happy either way.
Issue
Github-Issue: #2431
Tests
Added 4 regression tests in `tests/shared/test_exceptions.py`:
All four fail on `v1.x` without this patch and pass with it. The existing 9 tests in the file stay green.
```
$ uv run pytest tests/shared/test_exceptions.py
========= 13 passed in 0.16s =========
$ uv run ruff check src/mcp/shared/exceptions.py tests/shared/test_exceptions.py
All checks passed!
$ uv run pyright src/mcp/shared/exceptions.py tests/shared/test_exceptions.py
0 errors, 0 warnings, 0 informations
```