Skip to content

Add resolver dependency injection for MCPServer tools#2969

Open
Kludex wants to merge 13 commits into
mainfrom
resolver-dependency-injection
Open

Add resolver dependency injection for MCPServer tools#2969
Kludex wants to merge 13 commits into
mainfrom
resolver-dependency-injection

Conversation

@Kludex

@Kludex Kludex commented Jun 25, 2026

Copy link
Copy Markdown
Member

Summary

Adds server-side dependency injection for @mcp.tool() functions. 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.

from typing import Annotated

from pydantic import BaseModel

from mcp.server.mcpserver import Context, Elicit, MCPServer, Resolve

mcp = MCPServer(name="github")


class Login(BaseModel):
    username: str


class Confirm(BaseModel):
    ok: bool


async def login(ctx: Context) -> Login | Elicit[Login]:
    if username := (ctx.headers or {}).get("x-github-user"):
        return Login(username=username)        # resolved from context, no question
    return Elicit("GitHub username?", Login)    # must ask


async def confirm(repo: str, login: Annotated[Login, Resolve(login)]) -> Elicit[Confirm]:
    return Elicit(f"Star {repo} as {login.username}?", Confirm)


@mcp.tool()
async def star_repo(
    repo: str,
    login: Annotated[Login, Resolve(login)],
    confirm: Annotated[Confirm, Resolve(confirm)],
) -> str:
    """Star a GitHub repo."""
    return f"starred {repo} as {login.username}" if confirm.ok else "cancelled"

Design

  • Mechanism reuses the existing Context-injection seam: resolvers are detected at registration (Tool.from_function), excluded from the tool's JSON input schema via skip_names, and supplied at call time through arguments_to_pass_directly - so they bypass argument validation exactly like Context does today.
  • Headers come from the Context, not a Starlette Request: a new transport-agnostic Context.headers property (None on stdio).
  • Exactly-once is per-call resolver memoization (keyed by function identity); cross-call idempotency is intentionally out of scope.
  • Decline/cancel handling is driven by the consumer's annotation. Annotating the unwrapped model (Annotated[Login, Resolve(login)]) injects the model on accept and aborts the call with an error result on decline/cancel. Annotating the elicitation result union lets the consumer match on accept/decline/cancel instead:
@mcp.tool()
async def whoami(
    login: Annotated[AcceptedElicitation[Login] | DeclinedElicitation | CancelledElicitation, Resolve(login)],
) -> str:
    match login:
        case AcceptedElicitation(data=data):
            return f"hi {data.username}"
        case _:
            return "no username provided"
  • The resolver dependency graph is analyzed once at registration: unclassifiable resolver parameters and cyclic dependencies raise InvalidSignature there.

Notes

  • This is purely additive - no breaking changes. New public symbols (Resolve, Elicit, and the re-exported elicitation result types) live in mcp.server.mcpserver, not top-level mcp.
  • Resolvers and their referenced types must be resolvable by typing.get_type_hints, matching the existing limitation for tool parameter types (locally-scoped types under from __future__ import annotations are not resolvable - this is pre-existing, not introduced here).
  • Scoped to tools; resources/prompts use a different injection path and can follow.

Tests

tests/server/mcpserver/test_resolve.py (11 tests, 100% coverage of resolve.py): direct value vs Elicit, accept/decline for both unwrapped and union consumers, nested resolvers, exactly-once memoization, by-name tool-arg injection, schema exclusion, sync resolvers, and registration-time cycle/unresolvable-param errors.

AI Disclaimer

This PR was developed with the assistance of either Claude or Codex. I've reviewed and verified the changes.

Kludex added 2 commits June 25, 2026 15:44
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 cubic-dev-ai Bot 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.

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

Comment thread src/mcp/server/mcpserver/resolve.py
Comment thread docs/migration.md Outdated
Comment thread src/mcp/server/mcpserver/tools/base.py Outdated
if context_kwarg is None: # pragma: no branch
context_kwarg = find_context_parameter(fn)

resolved_params = find_resolved_parameters(fn)

@cubic-dev-ai cubic-dev-ai Bot Jun 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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>
Fix with cubic

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

Comment thread src/mcp/server/mcpserver/resolve.py Outdated
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.

@cubic-dev-ai cubic-dev-ai Bot 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.

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

Comment thread src/mcp/server/mcpserver/resolve.py
Comment thread src/mcp/server/mcpserver/resolve.py
Kludex added 3 commits June 25, 2026 17:20
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 claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@cubic-dev-ai cubic-dev-ai Bot 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.

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

Comment on lines +210 to +211
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)

@cubic-dev-ai cubic-dev-ai Bot Jun 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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>
Suggested change
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)
Fix with cubic

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

Comment thread src/mcp/server/mcpserver/resolve.py Outdated
@Kludex

Kludex commented Jun 25, 2026

Copy link
Copy Markdown
Member Author

Addressed the three bughunter findings in aac86dc:

  1. Non-BaseModel resolver returns — wrapping the return in AcceptedElicitation(data=...) validated it against the BaseModel bound, so Annotated[str, Resolve(get_token)] failed every call. Now wrapped with model_construct (no validation); the Elicit[T] path still validates via ctx.elicit.
  2. Context | None on resolvers_is_context_annotation now unwraps unions, matching find_context_parameter on tools.
  3. Bound-method memoization — resolvers are keyed by the callable (hash/eq by (__func__, __self__)) instead of id(fn), so auth.login referenced twice runs at most once and participates in cycle detection.

Each has a regression test. 100% coverage, CI green.

Comment thread src/mcp/server/mcpserver/tools/base.py
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.

@cubic-dev-ai cubic-dev-ai Bot 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.

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

Comment thread src/mcp/server/mcpserver/resolve.py
Comment thread src/mcp/server/mcpserver/utilities/func_metadata.py

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.
Comment thread src/mcp/server/mcpserver/resolve.py
Kludex added 3 commits June 26, 2026 09:49
…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.
Comment on lines +128 to +140
def _wants_union(type_arg: Any) -> bool:
"""True when `type_arg` is an `ElicitationResult` member (or a union of them).

Handles the bare `ElicitationResult[T]` alias (a `TypeAliasType` carrying the
union on `__value__`), an explicit `AcceptedElicitation[T] | ... ` union, and a
single member.
"""
origin = get_origin(type_arg)
value = getattr(origin, "__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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Annotating a resolved parameter with the bare alias — Annotated[ElicitationResult, Resolve(login)] (no [T] subscription, which type checkers accept as the implicit-Any form) — is silently treated as wanting the unwrapped model: _wants_union returns False for the bare TypeAliasType, so the consumer gets unwrapped data on accept and the call aborts on decline/cancel, the opposite of the branch-on-outcome intent the annotation expresses. Consider also reading __value__ off type_arg itself for the bare alias, or raising InvalidSignature for the unsubscripted form at registration.

Extended reasoning...

What the bug is

_wants_union decides whether a Resolve-annotated consumer receives the full elicitation outcome (the ElicitationResult union) or the unwrapped model. It handles the subscripted alias ElicitationResult[T] via the origin.__value__ branch, and explicit member unions via get_args + issubclass. But the bare, unsubscripted alias ElicitationResult (now a TypeAliasType after the subscriptability fix in b7b8967) falls through both paths and is classified as wants_union=False.

The code path

For type_arg = ElicitationResult (bare):

  1. get_origin(ElicitationResult) is None for an unsubscripted TypeAliasType, so origin is None and getattr(origin, "__value__", None) is None — the unwrapping branch is skipped, even though the alias itself carries the union on its own __value__ attribute.
  2. members becomes (ElicitationResult,) — a single TypeAliasType instance.
  3. isinstance(ElicitationResult, type) is False (a TypeAliasType is not a class), so the any(...) is False and _wants_union returns False.

By contrast, _wants_union(ElicitationResult[Login]) returns True (origin is the alias, which has __value__), and the explicit AcceptedElicitation[Login] | DeclinedElicitation | CancelledElicitation union returns True.

Why nothing prevents it

Type checkers accept an unsubscripted generic alias as the implicitly-Any-parameterized form, so Annotated[ElicitationResult, Resolve(login)] produces no static warning. Registration succeeds (the parameter is found and excluded from the schema), and nothing at runtime flags the misclassification — the consumer just silently gets the unwrapped behavior.

Step-by-step proof

  1. Define async def login(ctx: Context) -> Login | Elicit[Login]: return Elicit("GitHub username?", Login).
  2. Register a tool: async def whoami(login: Annotated[ElicitationResult, Resolve(login)]) -> str with a match login: case AcceptedElicitation(data=data): ... body intended to branch on the outcome.
  3. find_resolved_parameters records (Resolve(login), wants_union=False) because of the path above.
  4. Client accepts the elicitation → resolve_arguments calls _unwrap(outcome, name) and injects the raw Login model. The user's case AcceptedElicitation(data=data) never matches; their fallback branch runs instead.
  5. Client declines → _unwrap raises ToolError("... elicitation was decline") and the whole tool call returns an error result — exactly the abort behavior the user opted out of by annotating the result union.

Impact and fix

The impact is silent wrong behavior, but only for the undocumented bare-alias spelling — docs/migration.md, the PR description, and all tests use ElicitationResult[T] or the explicit member union, which work correctly. The fix is a one-liner: also consult getattr(type_arg, "__value__", None) when origin is None (treating the bare alias as wanting the union, with the union members then checked as today), or alternatively raise InvalidSignature for the unsubscripted alias at registration so the user is told to subscribe it. Either keeps the annotation's intent and the runtime behavior in agreement.

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).
Comment on lines +117 to +125
for name in inspect.signature(fn).parameters:
annotation = hints.get(name)
if get_origin(annotation) is not Annotated:
continue
type_arg, *metadata = get_args(annotation)
marker = next((m for m in metadata if isinstance(m, Resolve)), None)
if marker is not None:
resolved[name] = (marker, _wants_union(type_arg))
return resolved

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 The Resolve marker is silently ignored when the annotation wraps it in a union — e.g. login: Annotated[Login, Resolve(login)] | None = None — because find_resolved_parameters only checks for a top-level Annotated origin. The parameter then stays in the LLM-facing input schema and the resolver never runs, with no error or warning. Consider unwrapping union members when looking for the marker (mirroring the Context | None handling), or raising InvalidSignature at registration when a Resolve marker is present but cannot be honored.

Extended reasoning...

What the bug is

find_resolved_parameters() (src/mcp/server/mcpserver/resolve.py:117-125) classifies a parameter as resolver-filled only when get_origin(annotation) is Annotated. If a user spells an optional resolved parameter as Annotated[Login, Resolve(login)] | None = None (or Optional[Annotated[Login, Resolve(login)]]), the top-level origin is types.UnionType / typing.Union, so the loop continues and the Resolve marker buried inside the union arm is never seen.

The code path and consequences

Because the marker is dropped, the parameter is not added to skip_names in Tool.from_function (tools/base.py:84-87), so it remains in the tool's JSON input schema (parameters['properties'] contains login) and is presented to the calling LLM. resolved_params is empty, so resolve_arguments never runs the resolver, and the tool body receives whatever the model supplied (or None) instead of the server-resolved/elicited value. No error or warning is raised at registration or call time — the dependency-injection contract is silently dropped.

Step-by-step proof (verified empirically against the PR head, PYTHONPATH=src:src/mcp-types):

  1. Define async def login_resolver(ctx: Context) -> Login: ... and a tool async def star_repo(repo: str, login: Annotated[Login, Resolve(login_resolver)] | None = None) -> str.
  2. find_resolved_parameters(star_repo) returns {}get_origin of the login annotation is UnionType, not Annotated.
  3. Tool.from_function(star_repo).parameters['properties'] contains both repo and login; resolved_params is {}.
  4. At call time the LLM is asked to fill login; the resolver never executes; the tool body sees the model-supplied value (or None).

Why this spelling is plausible, not contrived

The SDK deliberately accepts the analogous union form for Context: find_context_parameter unwraps unions on tools (ctx: Context | None is detected and excluded from the schema), and this PR explicitly added the same tolerance for resolver Context params via _is_context_annotation (with a dedicated test, test_resolver_accepts_optional_context_annotation). A user who knows Context | None works will reasonably write the optional Resolve form and get silent misbehavior. The working spelling is Annotated[Login | None, Resolve(login)] (union inside Annotated), but nothing tells the user that. The failure mode matters because resolved params often carry trust-sensitive values (login/confirmation in the PR's own examples) — silently letting the LLM fill them is worse than a crash, and it contradicts the PR's own design of raising InvalidSignature at registration for unclassifiable resolver setups.

Secondary effect: the same union form on a resolver dependency parameter at least fails loudly — _resolve_marker also misses the marker and build_resolver_plans raises InvalidSignature 'parameter ... cannot be resolved' — but the message is misleading because the parameter does carry a Resolve annotation.

How to fix

Either unwrap union members when searching for the Resolve marker (mirroring how find_context_parameter / _is_context_annotation handle Context | None), or raise InvalidSignature at registration when a Resolve marker is present anywhere in the annotation but cannot be honored. Either keeps the developer's intent and the runtime behavior in agreement.

This is a nit — all documented and tested spellings work correctly, the broken form is undocumented and somewhat unmotivated (a resolved param is always filled by the framework, so | None = None has little purpose) — so it should not block the PR, but it is a real silent footgun worth a registration-time error or union unwrapping.

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.

1 participant