Add resolver dependency injection for MCPServer tools#2969
Conversation
A tool parameter annotated `Annotated[T, Resolve(fn)]` is filled by running the resolver `fn` before the tool body, instead of by the calling LLM. Resolvers form a dependency graph: a resolver may declare its own `Resolve(...)` dependencies, read the `Context` (including the new `Context.headers`), and receive the tool's own arguments by name. A resolver may return `Elicit[T]` to ask the client; the SDK runs the elicitation and injects the answer. Each resolver runs at most once per `tools/call`. The injected type follows the consumer's annotation: the unwrapped model aborts the call on decline/cancel, while the elicitation result union lets the consumer branch on the outcome. Resolved parameters are omitted from the tool's input schema; unclassifiable resolver parameters and cyclic resolver dependencies raise at registration time.
The headers property's request-present branch and the schema-inspection helpers in the resolver tests were not exercised, breaking the 100% coverage gate. Add direct Context.headers tests and mark the never-run helper bodies.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docs/migration.md">
<violation number="1" location="docs/migration.md:1408">
P2: Example uses DeclinedElicitation/CancelledElicitation without importing them, so the migration snippet is not runnable.</violation>
</file>
<file name="src/mcp/server/mcpserver/tools/base.py">
<violation number="1" location="src/mcp/server/mcpserver/tools/base.py:83">
P2: Tool registration can leak raw NameError for unresolved type hints. This breaks the existing InvalidSignature error path and returns less actionable failures.</violation>
<violation number="2" location="src/mcp/server/mcpserver/tools/base.py:95">
P2: Resolver by-name classification allows argument names that cannot be injected at runtime. This defers a resolvable-signature error into a runtime failure.</violation>
</file>
<file name="src/mcp/server/mcpserver/resolve.py">
<violation number="1" location="src/mcp/server/mcpserver/resolve.py:97">
P3: `find_resolved_parameters` iterates `hints.items()` which includes `'return'` from `get_type_hints`. Should filter to actual parameters (e.g., using `inspect.signature`).</violation>
<violation number="2" location="src/mcp/server/mcpserver/resolve.py:110">
P1: `_wants_union` misclassifies `AcceptedElicitation[T]` annotations, so tools requesting a specific accepted-result member get unwrapped model data instead of the elicitation result object.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| if context_kwarg is None: # pragma: no branch | ||
| context_kwarg = find_context_parameter(fn) | ||
|
|
||
| resolved_params = find_resolved_parameters(fn) |
There was a problem hiding this comment.
P2: Tool registration can leak raw NameError for unresolved type hints. This breaks the existing InvalidSignature error path and returns less actionable failures.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/mcp/server/mcpserver/tools/base.py, line 83:
<comment>Tool registration can leak raw NameError for unresolved type hints. This breaks the existing InvalidSignature error path and returns less actionable failures.</comment>
<file context>
@@ -67,13 +80,23 @@ def from_function(
if context_kwarg is None: # pragma: no branch
context_kwarg = find_context_parameter(fn)
+ resolved_params = find_resolved_parameters(fn)
+
+ skip_names = [context_kwarg] if context_kwarg is not None else []
</file context>
There was a problem hiding this comment.
Not an issue. find_resolved_parameters resolves hints via _type_hints, which catches the failure and returns {} (no raw NameError). func_metadata is then called and raises a proper InvalidSignature for unresolvable annotations, so the existing error path is preserved. Verified.
There was a problem hiding this comment.
The parent comment was wrong here: find_resolved_parameters already swallows unresolved hints and func_metadata still raises InvalidSignature, so there’s no raw NameError leak in this path.
find_resolved_parameters called typing.get_type_hints on the callable directly, which raises for a callable instance (an object with __call__), breaking tool registration for callable objects. Resolve hints off __call__ and tolerate unresolvable hints, mirroring find_context_parameter.
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/mcp/server/mcpserver/tools/base.py">
<violation number="1" location="src/mcp/server/mcpserver/tools/base.py:83">
P2: Tool registration can leak raw NameError for unresolved type hints. This breaks the existing InvalidSignature error path and returns less actionable failures.</violation>
</file>
<file name="src/mcp/server/mcpserver/resolve.py">
<violation number="1" location="src/mcp/server/mcpserver/resolve.py:110">
P1: `_wants_union` misclassifies `AcceptedElicitation[T]` annotations, so tools requesting a specific accepted-result member get unwrapped model data instead of the elicitation result object.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
After merging main, LATEST_PROTOCOL_VERSION is 2026-07-28, which defines no server-to-client requests, so elicitation/create is unavailable at the default negotiated version. Pin these tests to mode='legacy' (negotiates 2025-11-25) where elicitation is supported, matching test_elicitation.py.
…esolver naming - tools/base.py: build tool_arg_names as 'alias or field_name' to match the runtime kwarg keys, so a by-name resolver param on an aliased field resolves instead of raising KeyError at call time. - resolve.py: iterate inspect.signature params (not get_type_hints items, which include 'return') so a Resolve marker on a return annotation is ignored; add _resolver_name so callable-object resolvers raise InvalidSignature instead of AttributeError in error messages. - migration.md: import DeclinedElicitation/CancelledElicitation used in the branching example so the snippet is runnable. Add regression tests for each.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs in this implementation, but this PR introduces a new public API surface (Resolve/Elicit dependency injection) and modifies the tool-call execution path, so it warrants a maintainer's review of the design and API choices.
Extended reasoning...
Overview
This PR adds a resolver dependency-injection framework for @mcp.tool() functions: a new ~280-line resolve.py module (resolver detection, DAG analysis with cycle detection, per-call memoized execution, elicitation handling), new public exports from mcp.server.mcpserver (Resolve, Elicit, ElicitationResult and the elicitation result types), a new transport-agnostic Context.headers property, integration into Tool.from_function/Tool.run, and a small refactor of FuncMetadata argument validation. It includes 11 new tests for the resolver module plus two for Context.headers, and a migration-guide section.
Security risks
The feature itself is server-side and additive, but it does introduce two things worth a human's eye: Context.headers exposes raw transport headers to tool/resolver code (the documented pattern derives identity from a header like x-github-user, which is only safe behind a trusted proxy/auth layer), and resolvers bypass the tool's JSON-schema argument validation by design (like Context injection today). Neither is an injection or auth-bypass bug in the SDK, but the API encourages patterns whose security depends on deployment context.
Level of scrutiny
High. This is a new public API with non-trivial design decisions (decline/cancel semantics driven by the consumer's annotation, function-identity memoization, by-name tool-arg injection including alias handling, sync resolvers running in a thread) and it touches the tool-call execution path used by every MCPServer tool. API-surface additions to an SDK are the kind of decision maintainers should explicitly own, regardless of implementation correctness.
Other factors
The implementation is well-tested (the new test_resolve.py covers accept/decline, nesting, memoization, schema exclusion, registration-time errors, aliasing, and callable-object resolvers), and the most recent commits already address the earlier cubic reviewer findings (by-name aliasing, return-annotation handling, callable-resolver naming). My review and the bug-hunting pass found no concrete bugs; the deferral is purely about scope and API design ownership, not correctness concerns.
…nd-method memoization bughunter findings on #2969: - Resolvers may return any type, not just BaseModel. Wrapping the return in AcceptedElicitation(data=...) validated it against the schema bound, so e.g. Annotated[str, Resolve(get_token)] failed every call with a cryptic ValidationError. Use model_construct to wrap the value without validation (the Elicit[T] path still validates via ctx.elicit). - _is_context_annotation now unwraps unions, so a resolver param typed Context | None is accepted, matching find_context_parameter on tools. - Memoize resolvers by the callable itself (hash/eq) instead of id(fn), so a bound-method resolver referenced as auth.login in two places runs at most once and participates in cycle detection. Fresh bound-method objects share identity by (__func__, __self__). Add regression tests for each.
There was a problem hiding this comment.
2 issues found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/mcp/server/mcpserver/tools/base.py">
<violation number="1" location="src/mcp/server/mcpserver/tools/base.py:83">
P2: Tool registration can leak raw NameError for unresolved type hints. This breaks the existing InvalidSignature error path and returns less actionable failures.</violation>
</file>
<file name="src/mcp/server/mcpserver/resolve.py">
<violation number="1" location="src/mcp/server/mcpserver/resolve.py:110">
P1: `_wants_union` misclassifies `AcceptedElicitation[T]` annotations, so tools requesting a specific accepted-result member get unwrapped model data instead of the elicitation result object.</violation>
<violation number="2" location="src/mcp/server/mcpserver/resolve.py:210">
P2: Context detection is over-broad: any generic containing `Context` is treated as a context parameter, not just union/optional context annotations.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| candidates = get_args(annotation) if get_origin(annotation) is not None else (annotation,) | ||
| return any(isinstance(c, type) and issubclass(c, Context) for c in candidates) |
There was a problem hiding this comment.
P2: Context detection is over-broad: any generic containing Context is treated as a context parameter, not just union/optional context annotations.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/mcp/server/mcpserver/resolve.py, line 210:
<comment>Context detection is over-broad: any generic containing `Context` is treated as a context parameter, not just union/optional context annotations.</comment>
<file context>
@@ -193,12 +207,13 @@ def _resolve_marker(annotation: Any) -> tuple[Resolve | None, bool]:
if get_origin(annotation) is Annotated:
annotation = get_args(annotation)[0]
- return isinstance(annotation, type) and issubclass(annotation, Context)
+ candidates = get_args(annotation) if get_origin(annotation) is not None else (annotation,)
+ return any(isinstance(c, type) and issubclass(c, Context) for c in candidates)
</file context>
| candidates = get_args(annotation) if get_origin(annotation) is not None else (annotation,) | |
| return any(isinstance(c, type) and issubclass(c, Context) for c in candidates) | |
| origin = get_origin(annotation) | |
| is_union = origin is typing.Union or ( | |
| origin is not None and origin.__module__ == "types" and origin.__name__ == "UnionType" | |
| ) | |
| candidates = get_args(annotation) if is_union else (annotation,) | |
| return any(isinstance(c, type) and issubclass(c, Context) for c in candidates) |
There was a problem hiding this comment.
Intentional - this mirrors find_context_parameter exactly, which also matches any generic containing Context (e.g. it returns a param for list[Context] too). Restricting resolvers to unions-only would re-introduce the tool/resolver asymmetry that the earlier review asked me to fix. If the broad match is wrong it should be fixed in both places at once; keeping them in sync here.
There was a problem hiding this comment.
The parent comment was too broad here: this resolver check intentionally mirrors find_context_parameter, so narrowing it to unions-only would reintroduce the tool/resolver asymmetry. If the broad match is wrong, both paths should be changed together.
|
Addressed the three bughunter findings in aac86dc:
Each has a regression test. 100% coverage, CI green. |
Review follow-ups on #2969: - Tool.run validated arguments twice when resolvers were present (once to feed resolvers, once in call_fn_with_arg_validation). A field with default_factory or a stateful validator could hand a by-name resolver a different value than the tool body. Validate once and pass it through via a new pre_validated argument so both observe the same value. - Key the resolver cache/plans by (id(__func__), id(__self__)) for bound methods and id(fn) otherwise, instead of the callable's equality, so two distinct callables that compare equal can no longer share a plan/cache entry while bound-method memoization still works. Add regression tests.
There was a problem hiding this comment.
2 issues found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/mcp/server/mcpserver/tools/base.py">
<violation number="1" location="src/mcp/server/mcpserver/tools/base.py:83">
P2: Tool registration can leak raw NameError for unresolved type hints. This breaks the existing InvalidSignature error path and returns less actionable failures.</violation>
</file>
<file name="src/mcp/server/mcpserver/resolve.py">
<violation number="1" location="src/mcp/server/mcpserver/resolve.py:210">
P2: Context detection is over-broad: any generic containing `Context` is treated as a context parameter, not just union/optional context annotations.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
The earlier double-validation concern is resolved in 37c038c (single validation pass threaded through as pre_validated, with a regression test) and I found no new issues — but this introduces a new public DI API (Resolve/Elicit), new resolver-graph machinery, and changes to the tool call path, so it warrants a maintainer's review of the API design rather than auto-approval.
Extended reasoning...
Overview
This PR adds resolver dependency injection for MCPServer tools: a new ~290-line resolve.py module (Resolve/Elicit markers, resolver DAG analysis, per-call memoized resolution, elicitation integration), new public exports from mcp.server.mcpserver, a transport-agnostic Context.headers property, changes to Tool.from_function/Tool.run, a new validate_arguments/pre_validated seam in func_metadata, plus extensive tests (19 in test_resolve.py) and migration docs.
Security risks
Resolvers run server-defined callables only — the calling client cannot inject or select resolvers, and resolved parameters are stripped from the input schema so clients cannot supply them. Context.headers exposes request headers to tool/resolver code, which is intentional and transport-scoped (None on stdio); no auth or crypto code is touched. The AcceptedElicitation.model_construct path skips validation by design for non-elicited resolver returns, which is internal data, not client-supplied. I see no concrete injection or bypass risk, but the elicitation-driven flow does change how user-confirmation steps are wired, which deserves a human eye.
Level of scrutiny
High. This is a feature-level addition with new public API surface and design decisions (annotation-driven unwrap-vs-union semantics, resolver identity/memoization keys, by-name argument injection, registration-time cycle detection) that a maintainer should deliberately sign off on — it is not a mechanical or pattern-following change. It also modifies the hot path of every tool call (Tool.run, call_fn_with_arg_validation), even though the non-resolver path is behaviorally unchanged.
Other factors
The author has been responsive: all cubic findings were either fixed (with regression tests) or rebutted with verification, and my prior inline finding about double argument validation was fixed in 37c038c by threading a single validated pass through pre_validated — I checked the current code and the fix looks correct. Test coverage of the new module is thorough (accept/decline/cancel, nesting, memoization, aliasing, sync resolvers, registration-time errors). The remaining considerations are design-level (public API shape, scoping to tools only, default-mode interaction with the 2026-07-28 elicitation changes), which is exactly what human review is for.
Review follow-ups on #2969: - _resolver_key now keys any bound method (pure-python or built-in) by its underlying function/name plus __self__ identity, so a built-in bound method (no __func__, fresh object each access) referenced twice still memoizes to one call. - call_fn_with_arg_validation copies the validated args before merging the injected kwargs, so a caller-provided pre_validated dict is never mutated. Add regression tests.
…form works Codex review caught that the documented Annotated[ElicitationResult[Login], Resolve(login)] form silently dropped the resolver: ElicitationResult was a collapsed union alias (not subscriptable), so under 'from __future__ import annotations' get_type_hints raised, _type_hints swallowed it, and the parameter stayed client-supplied with the resolver never running. Redefine ElicitationResult via TypeAliasType so ElicitationResult[T] is genuinely subscriptable, and teach _wants_union to unwrap the alias. Update the migration doc to use the clean ElicitationResult[T] form. Add a regression test exercising the postponed-annotations path.
Replace the GitHub star example with a delete_folder tool: the confirm_delete resolver lists the folder by reading the tool's own path argument and only elicits when the folder is non-empty (an empty folder resolves to ok=True with no round-trip). delete_folder annotates ElicitationResult[Confirm] and handles every outcome - accept-and-delete, accept-but-keep, decline, and cancel. Add end-to-end tests covering all five paths; the cancel path now exercises elicit_with_validation's cancel branch (pragma removed).
Reviewer's guideThis is the base of a two-PR stack. It adds the Where to look
Design decisions worth a second look
Not in scope (deliberate): resolvers on resources/prompts (tools only); cross-call idempotency. Reviewed by cubic + Claude + a bughunter pass; all findings fixed. CI green, 100% coverage. The example in |
- A `Resolve(...)` marker nested in a union (e.g. Annotated[T, Resolve(f)] | None) was silently dropped, leaving the param in the LLM schema and never running the resolver. Raise InvalidSignature at registration instead. - The bare `ElicitationResult` alias (no [T]) now opts into the result union like the subscripted form, rather than being treated as wanting the unwrapped model. Add regression tests for both.
…tx.headers in context Add a tested docs_src/elicitation/tutorial004.py (the delete_folder resolver flow) and an 'Ask before the tool runs' section to docs/tutorial/elicitation.md, plus a ctx.headers bullet to docs/tutorial/context.md. Tests prove the resolver asks only for a non-empty folder, hides the resolved param from the schema, and branches on every outcome.
There was a problem hiding this comment.
All the issues I raised on earlier revisions have been addressed and I found no new bugs in the latest commits, but this PR introduces a sizable new public API (Resolve/Elicit resolver injection, Context.headers, the ElicitationResult alias change) with real design decisions, so it should still get a human review before merging.
Extended reasoning...
Overview
This PR adds resolver dependency injection for @mcp.tool() functions: a new ~320-line src/mcp/server/mcpserver/resolve.py module (the Resolve/Elicit markers, registration-time DAG analysis with cycle detection, and the runtime resolver walk), wiring in tools/base.py (schema exclusion via skip_names, injection via arguments_to_pass_directly), a validate_arguments/pre_validated refactor in func_metadata.py, a new transport-agnostic Context.headers property, the ElicitationResult alias rewrapped as a TypeAliasType, new public re-exports on mcp.server.mcpserver, plus docs (migration guide, elicitation/context tutorials, a tested tutorial004) and extensive tests.
Security risks
No direct injection or auth-bypass surface. The most security-relevant aspects are: (1) resolved parameters are removed from the LLM-facing input schema, so trust-sensitive values (logins, confirmations) are server-supplied rather than model-supplied — the registration-time InvalidSignature error for a Resolve marker buried in a union closes the earlier silent-fallback footgun; (2) Context.headers exposes raw request headers to tool/resolver code, which is intentional and transport-gated (None on stdio). Nothing here touches auth, crypto, or permissions code paths.
Level of scrutiny
High. This is a substantial new feature in the production server SDK with deliberate design choices (annotation-driven unwrap-vs-branch semantics, per-call memoization keyed by callable identity, the ElicitationResult isinstance behavior change, the choice to mirror find_context_parameter's broad Context matching). Those are exactly the kind of API-surface decisions a maintainer should sign off on, regardless of implementation correctness. It is also the base of a stacked pair (#2986), so its API shape constrains follow-up work.
Other factors
The review history is healthy: cubic and my prior passes raised a number of issues (double validation, bare-alias misclassification, union-wrapped Resolve markers, bound-method memoization, docs gaps, the isinstance behavior note) and each was either fixed with a regression test or explicitly justified. The latest revision has 100% coverage of resolve.py, runnable doc examples exercised by the docs tests, and CI is reported green. I found no new defects in the current head, so my role here is to confirm the prior findings are resolved rather than to block — but the size and public-API scope put this outside what I should shadow-approve.
A custom request type on the request context is not required to expose headers; reading them through a blind cast raised AttributeError. Use getattr with a None fallback and drop the now-unused protocol.
The only docs for Resolve/Elicit were a subsection of the elicitation tutorial, which never shows the non-asking uses. The new page covers the mechanism on its own terms: declaring a resolver, the hidden input schema, dependencies of dependencies with per-call memoization, and resolvers that elicit, with runnable docs_src snippets proven by tests. The migration guide entry now covers only the actual behavior change: ElicitationResult is a TypeAliasType, so isinstance against the alias raises TypeError (check the member classes instead). The feature description moved to the tutorial where it belongs. A pre-parse pragma in func_metadata.py is now exercised by the new client-supplied-value test, so the marker comes off.
…chema A back-office refund server where the amount is computed by resolvers from the order record and never appears in the tool's input schema, so the model cannot supply it. The story exercises the resolver DAG (load_order -> refund_scope -> refund_amount / ask_restock), the no-round-trip fast path, per-call memoization observable from the client, validation of elicited free text, and both decline semantics: an unwrapped dependency aborts the call, the ElicitationResult union lets the tool branch.
| for param_name in sig.parameters: | ||
| annotation = hints.get(param_name) | ||
| if annotation is not None and _is_context_annotation(annotation): | ||
| params[param_name] = _ParamPlan("context") | ||
| continue | ||
| marker, wants_union = _resolve_marker(annotation) | ||
| if marker is not None: | ||
| params[param_name] = _ParamPlan("resolve", marker, wants_union) | ||
| nested.append(marker.fn) | ||
| continue | ||
| if param_name in tool_arg_names: | ||
| params[param_name] = _ParamPlan("by_name") | ||
| continue | ||
| raise InvalidSignature( | ||
| f"Resolver {_resolver_name(fn)!r} parameter {param_name!r} cannot be resolved: " | ||
| "expected a Context, an Annotated[_, Resolve(...)], or a tool argument by name" | ||
| ) |
There was a problem hiding this comment.
🟡 When a resolver dependency parameter wraps its Resolve marker in a union (e.g. repo: Annotated[Login, Resolve(get_login)] | None = None), build_resolver_plans/_resolve_marker never see the marker: if the parameter name happens to collide with a tool argument it is silently classified as by_name, so the resolver never runs and the function receives the raw client/LLM-supplied value instead of the server-resolved one; if it doesn't collide, registration fails with a misleading 'cannot be resolved' message. Consider extending the _contains_resolve guard you already added for tool parameters (in find_resolved_parameters) to analyze() here, or unwrapping unions in _resolve_marker.
Extended reasoning...
What the bug is
The PR's registration-time guard for a Resolve marker buried in a union (_contains_resolve, added in c3ea531 with the regression test test_resolve_marker_inside_a_union_raises_at_registration) is only invoked from find_resolved_parameters, i.e. for tool parameters. The analogous classification path for resolver dependency parameters — build_resolver_plans.analyze() together with _resolve_marker() (src/mcp/server/mcpserver/resolve.py:202-218 and 229-234) — has no such check, so the same spelling on a resolver's own parameter is handled inconsistently and, in the worst case, silently does the wrong thing.
The code path
analyze() classifies each resolver parameter in order: (1) _is_context_annotation, (2) _resolve_marker, (3) name in tool_arg_names, else InvalidSignature. _resolve_marker() returns (None, False) whenever get_origin(annotation) is not Annotated — for Annotated[Login, Resolve(get_login)] | None the top-level origin is UnionType, so the marker is invisible. _is_context_annotation is also False (the union members are an Annotated alias and NoneType). If the parameter name matches a tool argument, step (3) silently wins: the param becomes kind='by_name', get_login is never added to nested, never analyzed, and never executed.
Step-by-step proof (reproduced empirically against the PR head, PYTHONPATH=src):
- Define a resolver dependency:
async def get_login(ctx: Context) -> Login: ... async def confirm(repo: Annotated[Login, Resolve(get_login)] | None = None) -> str: return f"confirm got: {type(repo).__name__}={repo!r}" @mcp.tool() async def star_repo(repo: str, c: Annotated[str, Resolve(confirm)]) -> str: return c
- Registration succeeds with no error or warning. The resolver plan for
confirmclassifiesrepoasby_name;get_loginappears in no plan at all. - Calling
star_repowith{"repo": "modelcontextprotocol/python-sdk"}returnsconfirm got: str='modelcontextprotocol/python-sdk'— the resolver author wroteResolve(get_login)expecting a server-derivedLogin, but received the raw, client/LLM-controlled string. - When the parameter name does not collide with a tool argument, registration at least fails — but the
InvalidSignaturemessage ('cannot be resolved: expected a Context, an Annotated[_, Resolve(...)], or a tool argument by name') is misleading, since the parameter does carry a Resolve marker.
Why existing safeguards don't catch it
The _contains_resolve guard lives only in find_resolved_parameters; build_resolver_plans never calls it, and the test suite only covers the tool-parameter variant. The PR also explicitly tolerates Context | None on resolver parameters (_is_context_annotation unwraps unions, with a dedicated test), which makes the optional-union spelling a plausible thing for users to try on Resolve-annotated params too.
Impact and why it's a nit
Resolved values are typically trust-sensitive (login/confirmation in the PR's own examples), so silently substituting a client-supplied value where the author expected a server-resolved one is the bad failure mode here — but it requires both the undocumented union spelling and a name collision with a tool argument, and the non-colliding case fails loudly (just with a confusing message). All documented spellings work correctly. This is the same class of issue as the earlier tool-parameter finding that was fixed; flagging it for consistency, not as a blocker.
How to fix
In analyze(), before falling back to the by_name / InvalidSignature branches, check _contains_resolve(annotation) and raise the same 'wraps Resolve(...) in a union' InvalidSignature used for tool parameters — or unwrap union members in _resolve_marker, mirroring _is_context_annotation. Either is a small change plus one regression test.
| def _wants_union(type_arg: Any) -> bool: | ||
| """True when `type_arg` is an `ElicitationResult` member (or a union of them). | ||
|
|
||
| Handles the subscripted `ElicitationResult[T]` alias (a `TypeAliasType` whose | ||
| union is on the origin's `__value__`), the bare `ElicitationResult` alias (the | ||
| `__value__` is on `type_arg` itself), an explicit `AcceptedElicitation[T] | ...` | ||
| union, and a single member. | ||
| """ | ||
| # Unwrap the `ElicitationResult` alias whether it is bare or subscripted. | ||
| value = getattr(type_arg, "__value__", None) or getattr(get_origin(type_arg), "__value__", None) | ||
| if value is not None: | ||
| type_arg = value | ||
| members = get_args(type_arg) if get_origin(type_arg) is not None else (type_arg,) | ||
| return any(isinstance(m, type) and issubclass(m, _ELICITATION_RESULT_MEMBERS) for m in members) |
There was a problem hiding this comment.
🟡 When the documented ElicitationResult[T] alias appears as a union member — e.g. Annotated[ElicitationResult[Login] | None, Resolve(login)] or Optional[ElicitationResult[Confirm]] — _wants_union returns False, so the consumer silently receives the unwrapped model on accept and the call aborts with a ToolError on decline/cancel, the opposite of the branch-on-outcome intent the annotation expresses (the explicit-member spelling AcceptedElicitation[Login] | None is detected). Same family as the bare-alias and Resolve-in-union gaps already fixed; the fix is to unwrap each union member's __value__ (or recurse) before the issubclass check.
Extended reasoning...
What the bug is
_wants_union (src/mcp/server/mcpserver/resolve.py:142-155) decides whether a Resolve consumer receives the full elicitation outcome or the unwrapped model. It unwraps the ElicitationResult TypeAliasType only when the alias is the whole annotation — via getattr(type_arg, "__value__", None) (the bare-alias fix) or getattr(get_origin(type_arg), "__value__", None) (the subscripted form). When the alias is one member of a union, neither branch fires:
- For
type_arg = ElicitationResult[Login] | None,get_origin(type_arg)istypes.UnionType, which has no__value__, so the unwrapping line is a no-op. members = get_args(type_arg)is(ElicitationResult[Login], NoneType). The subscripted alias member is aTypeAliasTypesubscription, not a class, soisinstance(m, type)is False and theissubclasscheck skips it;NoneTypeis not an elicitation member.- Result:
wants_union = False.
This was verified empirically against the PR head (Python 3.11, pydantic 2.12): _wants_union(ElicitationResult[Login] | None) → False, _wants_union(Optional[ElicitationResult[Login]]) → False, while _wants_union(ElicitationResult[Login]) → True and _wants_union(AcceptedElicitation[Login] | None) → True. The inconsistency lives inside the same function: the explicit-member spelling unioned with extra arms is detected, but the documented ElicitationResult[T] spelling unioned with anything is not.
Step-by-step proof
- Define
async def login(ctx: Context) -> Login | Elicit[Login]: return Elicit("GitHub username?", Login). - Register a tool with
login: Annotated[ElicitationResult[Login] | None, Resolve(login)] = Nonewhose body doesmatch login: case AcceptedElicitation(data=data): ...intending to branch on accept/decline/cancel. find_resolved_parametersrecords(Resolve(login), wants_union=False)— registration succeeds, the param is correctly hidden from the input schema, and no error or warning is raised.- Client accepts the elicitation →
resolve_argumentscalls_unwrap(outcome, name)(resolve.py:261-263, 308-311) and injects the bareLogin. Thecase AcceptedElicitation(data=data)arm never matches; the fallback branch silently runs instead. - Client declines →
_unwrapraisesToolError("... elicitation was decline")and the whole call returns an error result — exactly the abort behavior the author opted out of by annotating the result union.
Why existing safeguards don't catch it
The two fixes already made in this PR each cover a different shape: the bare-alias fix reads __value__ off type_arg itself (only when the alias is the whole annotation), and the _contains_resolve guard fires only when the Resolve marker itself is buried in a union. Here the marker is at the top level (Annotated wraps the union), so _contains_resolve never runs, and the alias is one union member deep, where _wants_union neither recurses nor unwraps. Type checkers accept the spelling, so nothing flags it statically either.
Impact and severity
Silent wrong behavior, but only for an undocumented spelling — a resolved parameter is always supplied by the framework, so | None has limited motivation, and all documented spellings (ElicitationResult[T], the explicit member union, the unwrapped model) work correctly. Hence a nit, not a blocker — the same family as the bare-alias and Resolve-in-union gaps already accepted and fixed in this PR, flagged for consistency with that design intent.
How to fix
In _wants_union, unwrap each union member's __value__ before the issubclass check, or simply recurse: after the alias unwrap, return any(_wants_union(m) for m in members) when type_arg is a union. One line plus a regression test, mirroring the earlier bare-alias fix (test_bare_elicitation_result_alias_wants_the_outcome_union).
2 participants
Summary
Adds server-side dependency injection for
@mcp.tool()functions. A tool parameter annotatedAnnotated[T, Resolve(fn)]is filled by running the resolverfnbefore the tool body, instead of by the calling LLM. Resolvers form a dependency graph: a resolver may declare its ownResolve(...)dependencies, read theContext(including the newContext.headers), and receive the tool's own arguments by name. A resolver may returnElicit[T]to ask the client; the SDK runs the elicitation and injects the answer. Each resolver runs at most once pertools/call.Design
Context-injection seam: resolvers are detected at registration (Tool.from_function), excluded from the tool's JSON input schema viaskip_names, and supplied at call time througharguments_to_pass_directly- so they bypass argument validation exactly likeContextdoes today. A client that sends a value for a resolved parameter anyway is ignored: the resolver's value is the only one the tool can receive.Context, not a StarletteRequest: a new transport-agnosticContext.headersproperty (Noneon stdio, or when the transport's request object carries no headers). Headers are client-supplied input - fine for a locale or a feature flag; an identity still comes from the authorization layer, never from a header the caller can set. The docs say this explicitly.Annotated[Login, Resolve(login)]) injects the model on accept and aborts the call with an error result on decline/cancel. AnnotatingElicitationResult[Login](or the explicit union) lets the consumermatchon accept/decline/cancel instead:InvalidSignaturethere.Docs & examples
docs/tutorial/dependencies.md), with complete runnable snippets underdocs_src/dependencies/proven bytests/docs_src/test_dependencies.py(8 tests). The elicitation tutorial gained an "Ask before the tool runs" section for the eliciting form.examples/stories/refund_desk/: a back-office refund server where the amount is resolver-computed from the order record and never appears in the input schema, so the model cannot supply it. Runs in the stories matrix (uv run python -m stories.refund_desk.client).docs/migration.mddocuments the one behavioral change (below).Notes
ElicitationResultis now a subscriptableTypeAliasType(soElicitationResult[T]works in annotations) instead of a plain union, which makes a runtimeisinstance(result, ElicitationResult)raiseTypeError- check against the member classes (AcceptedElicitationetc.) instead. Everything else is additive. New public symbols (Resolve,Elicit, and the re-exported elicitation result types) live inmcp.server.mcpserver, not top-levelmcp.typing.get_type_hints, matching the existing limitation for tool parameter types (locally-scoped types underfrom __future__ import annotationsare not resolvable - this is pre-existing, not introduced here).Tests
tests/server/mcpserver/test_resolve.py(26 tests, 100% coverage ofresolve.py): direct value vsElicit, accept/decline for both unwrapped and union consumers, nested resolvers, exactly-once memoization (including bound-method resolvers), by-name tool-arg injection incl. aliased fields, schema exclusion, sync resolvers, non-BaseModelresolver returns, single-validation-pass semantics, and registration-time cycle/unresolvable-param errors. Plus the docs-snippet tests and therefund_deskstory legs above.AI Disclaimer