Skip to content

Commit b6d6e07

Browse files
Kludexmaxisbey
authored andcommitted
Worker-stable wire keys; restore typed model for unannotated eliciting resolvers
Review follow-ups on #2986: - _state_key carries no id(...): it is module:qualname (a callable object uses its type's), so request_state round-trips and resumes on any worker (stateless HTTP). Two resolvers sharing that base (method instances, factory closures) are already disambiguated deterministically at registration (#N), so dropping the id is safe. - An eliciting resolver whose annotation lacks an Elicit[T] arm has elicit_schema None; its answer restored from request_state is now re-validated against the live Elicit.schema (via _elicit consulting res.state) instead of injecting a raw dict. - Move the datetime import to module top (AGENTS.md). Add regression tests: an unannotated eliciting resolver in a multi-round flow, and worker-stable wire keys for method instances and callable objects.
1 parent fad97fd commit b6d6e07

2 files changed

Lines changed: 124 additions & 17 deletions

File tree

src/mcp/server/mcpserver/resolve.py

Lines changed: 41 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,13 @@ def find_resolved_parameters(fn: Callable[..., Any]) -> dict[str, tuple[Resolve,
155155
for name in inspect.signature(fn).parameters:
156156
annotation = hints.get(name)
157157
if get_origin(annotation) is not Annotated:
158+
# A `Resolve` marker is only honored at the top level; flag (rather than
159+
# silently drop) one buried in a union, e.g. `Annotated[T, Resolve(f)] | None`.
160+
if _contains_resolve(annotation):
161+
raise InvalidSignature(
162+
f"Parameter {name!r} of {_resolver_name(fn)!r} wraps `Resolve(...)` in a "
163+
"union; annotate the parameter directly as `Annotated[T, Resolve(...)]`"
164+
)
158165
continue
159166
type_arg, *metadata = get_args(annotation)
160167
marker = next((m for m in metadata if isinstance(m, Resolve)), None)
@@ -163,6 +170,13 @@ def find_resolved_parameters(fn: Callable[..., Any]) -> dict[str, tuple[Resolve,
163170
return resolved
164171

165172

173+
def _contains_resolve(annotation: Any) -> bool:
174+
"""True when a `Resolve` marker is nested inside `annotation` (e.g. a union member)."""
175+
if get_origin(annotation) is Annotated:
176+
return any(isinstance(m, Resolve) for m in get_args(annotation)[1:])
177+
return any(_contains_resolve(arg) for arg in get_args(annotation))
178+
179+
166180
def _elicit_return_schema(return_annotation: Any) -> type[BaseModel] | None:
167181
"""Extract `T` from a resolver return type's `Elicit[T]` arm, if present.
168182
@@ -187,12 +201,13 @@ def _is_union(annotation: Any) -> bool:
187201
def _wants_union(type_arg: Any) -> bool:
188202
"""True when `type_arg` is an `ElicitationResult` member (or a union of them).
189203
190-
Handles the bare `ElicitationResult[T]` alias (a `TypeAliasType` carrying the
191-
union on `__value__`), an explicit `AcceptedElicitation[T] | ... ` union, and a
192-
single member.
204+
Handles the subscripted `ElicitationResult[T]` alias (a `TypeAliasType` whose
205+
union is on the origin's `__value__`), the bare `ElicitationResult` alias (the
206+
`__value__` is on `type_arg` itself), an explicit `AcceptedElicitation[T] | ...`
207+
union, and a single member.
193208
"""
194-
origin = get_origin(type_arg)
195-
value = getattr(origin, "__value__", None)
209+
# Unwrap the `ElicitationResult` alias whether it is bare or subscripted.
210+
value = getattr(type_arg, "__value__", None) or getattr(get_origin(type_arg), "__value__", None)
196211
if value is not None:
197212
type_arg = value
198213
members = get_args(type_arg) if get_origin(type_arg) is not None else (type_arg,)
@@ -330,16 +345,17 @@ def __init__(
330345

331346

332347
def _state_key(fn: Callable[..., Any]) -> str:
333-
"""Process-stable wire key for a resolver's elicitation.
348+
"""Worker-stable base wire key for a resolver, derived only from registration data.
334349
335-
`id(fn)` isn't stable across `input_required` rounds, so key `input_requests` /
336-
`request_state` by `module:qualname`. Bound methods add their `__self__` id so
337-
two instances of the same method get distinct questions and stored outcomes
338-
(the registered `Resolve(...)` holds the instance for the call's lifetime).
350+
`input_requests`/`request_state` must round-trip through the client and resume on
351+
any worker (stateless HTTP), so the key carries no `id(...)`: it is the resolver's
352+
`module:qualname` (a callable object uses its type's). Distinct resolvers that
353+
share this base - two instances of one method, two closures from one factory - are
354+
disambiguated deterministically by `build_resolver_plans` (`base`, `base#1`, ...).
339355
"""
340-
base = f"{getattr(fn, '__module__', '')}:{getattr(fn, '__qualname__', fn)!s}"
341-
bound_self = getattr(fn, "__self__", None)
342-
return f"{base}#{id(bound_self)}" if bound_self is not None else base
356+
qualname = getattr(fn, "__qualname__", None) or type(fn).__qualname__
357+
module = getattr(fn, "__module__", None) or type(fn).__module__
358+
return f"{module}:{qualname}"
343359

344360

345361
async def resolve_arguments(
@@ -396,7 +412,11 @@ async def _resolve(fn: Callable[..., Any], res: _Resolution) -> ElicitationResul
396412
if wire_key in res.pending:
397413
# Already asked this round by another consumer; don't run the resolver again.
398414
raise _Pending
399-
if wire_key in res.state:
415+
# Restore a prior round's outcome directly only when its model is known from the
416+
# `Elicit[T]` return arm. Without that (a resolver that elicits but isn't annotated
417+
# `-> ... Elicit[T]`), fall through and re-run the resolver so `_elicit` can
418+
# re-validate the stored answer against the live `Elicit.schema`.
419+
if wire_key in res.state and (plan.elicit_schema is not None or res.state[wire_key].action != "accept"):
400420
outcome = _outcome_from_state(res.state[wire_key], plan.elicit_schema)
401421
res.cache[cache_key] = outcome
402422
# Carry the restored answer forward: if a later resolver is still pending,
@@ -449,6 +469,13 @@ async def _elicit(elicit: Elicit[Any], key: str, res: _Resolution) -> Elicitatio
449469
if not res.input_required:
450470
return await res.context.elicit(elicit.message, elicit.schema)
451471

472+
# Answered in a prior round (restored without a known schema, e.g. an unannotated
473+
# resolver): re-validate the stored entry against the live `Elicit.schema`.
474+
if key in res.state and key not in res.answers:
475+
outcome = _outcome_from_state(res.state[key], elicit.schema)
476+
res.elicited[key] = outcome
477+
return outcome
478+
452479
answer = res.answers.get(key)
453480
if answer is None:
454481
res.pending[key] = _elicit_request(elicit)

tests/server/mcpserver/test_resolve.py

Lines changed: 83 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
11
"""Tests for resolver dependency injection (MRTR) on MCPServer tools."""
22

33
from collections.abc import Callable
4-
from typing import Annotated, Literal
4+
from datetime import datetime
5+
from typing import Annotated, Any, Literal
56

67
import pytest
78
from mcp_types import (
@@ -340,6 +341,32 @@ async def tool(login: Annotated[Login, Resolve(login)]) -> str:
340341
Tool.from_function(tool)
341342

342343

344+
def test_resolve_marker_inside_a_union_raises_at_registration():
345+
async def login(ctx: Context) -> Login:
346+
return Login(username="x") # pragma: no cover
347+
348+
async def tool(login: Annotated[Login, Resolve(login)] | None = None) -> str:
349+
return login.username if login else "" # pragma: no cover
350+
351+
with pytest.raises(InvalidSignature, match="wraps `Resolve"):
352+
Tool.from_function(tool)
353+
354+
355+
def test_bare_elicitation_result_alias_wants_the_outcome_union():
356+
# The bare `ElicitationResult` alias (no `[T]` subscription) must still opt into
357+
# the result union, not be treated as wanting the unwrapped model.
358+
async def login(ctx: Context) -> Login:
359+
return Login(username="x") # pragma: no cover
360+
361+
async def tool(login: object) -> str:
362+
return "x" # pragma: no cover
363+
364+
bare_alias: Any = ElicitationResult
365+
tool.__annotations__["login"] = Annotated[bare_alias, Resolve(login)]
366+
(_, wants_union) = find_resolved_parameters(tool)["login"]
367+
assert wants_union is True
368+
369+
343370
def test_resolve_marker_on_return_annotation_is_ignored():
344371
async def login(ctx: Context) -> Login:
345372
return Login(username="x") # pragma: no cover
@@ -853,8 +880,6 @@ async def both(
853880

854881
@pytest.mark.anyio
855882
async def test_non_serializable_sibling_resolver_does_not_break_rounds():
856-
from datetime import datetime
857-
858883
mcp = MCPServer(name="NonSerializable")
859884

860885
async def clock(ctx: Context) -> datetime:
@@ -1052,3 +1077,58 @@ def answer(key: str, params: ElicitRequestFormParams) -> ElicitResult:
10521077
result = await _drive_mrtr(client, "tool", {}, answer)
10531078
assert isinstance(result.content[0], TextContent)
10541079
assert result.content[0].text == "A,B"
1080+
1081+
1082+
@pytest.mark.anyio
1083+
async def test_eliciting_resolver_without_elicit_arm_restores_a_typed_model():
1084+
# A resolver annotated `-> Login` that actually returns `Elicit(...)` has no
1085+
# `Elicit[T]` return arm, so `elicit_schema` is None. Its answer, restored from
1086+
# request_state in a 3+ round flow, must still come back as a Login model (not a
1087+
# raw dict) so a dependent resolver/tool can use its attributes.
1088+
mcp = MCPServer(name="LyingAnnotation")
1089+
1090+
# Annotated without an `Elicit[T]` return arm, so `elicit_schema` is None.
1091+
async def login(ctx: Context) -> object:
1092+
return Elicit("user?", Login)
1093+
1094+
async def confirm(login: Annotated[Login, Resolve(login)]) -> Elicit[Confirm]:
1095+
return Elicit(f"as {login.username}?", Confirm)
1096+
1097+
@mcp.tool()
1098+
async def act(
1099+
login: Annotated[Login, Resolve(login)],
1100+
confirm: Annotated[Confirm, Resolve(confirm)],
1101+
) -> str:
1102+
return f"{login.username}:{confirm.ok}"
1103+
1104+
def answer(key: str, params: ElicitRequestFormParams) -> ElicitResult:
1105+
if "user" in params.message:
1106+
return ElicitResult(action="accept", content={"username": "octocat"})
1107+
assert "as octocat?" in params.message # login restored as a real model
1108+
return ElicitResult(action="accept", content={"ok": True})
1109+
1110+
async with Client(mcp) as client:
1111+
result = await _drive_mrtr(client, "act", {}, answer)
1112+
assert isinstance(result.content[0], TextContent)
1113+
assert result.content[0].text == "octocat:True"
1114+
1115+
1116+
def test_wire_key_is_worker_stable_for_methods_and_callable_objects():
1117+
from mcp.server.mcpserver.resolve import _state_key
1118+
1119+
class Service:
1120+
async def token(self, ctx: Context) -> Login:
1121+
return Login(username="x") # pragma: no cover
1122+
1123+
class CallableResolver:
1124+
async def __call__(self, ctx: Context) -> Login:
1125+
return Login(username="x") # pragma: no cover
1126+
1127+
a, b = Service(), Service()
1128+
# No id(...) in the key: two instances of one method get the same base (they are
1129+
# disambiguated at registration, not here), and the key carries no memory address.
1130+
assert _state_key(a.token) == _state_key(b.token)
1131+
assert "#" not in _state_key(a.token)
1132+
assert _state_key(a.token).endswith("Service.token")
1133+
# Callable objects key by their type's qualname (they have no `__qualname__`).
1134+
assert _state_key(CallableResolver()).endswith("CallableResolver")

0 commit comments

Comments
 (0)