Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/mcp/server/mcpserver/prompts/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
from __future__ import annotations

import functools
import inspect
from collections.abc import Awaitable, Callable, Sequence
from typing import TYPE_CHECKING, Any, Literal

Expand All @@ -13,6 +12,7 @@

from mcp.server.mcpserver.utilities.context_injection import find_context_parameter, inject_context
from mcp.server.mcpserver.utilities.func_metadata import func_metadata
from mcp.shared._callable_inspection import is_async_callable
from mcp.types import ContentBlock, Icon, TextContent

if TYPE_CHECKING:
Expand Down Expand Up @@ -157,8 +157,9 @@ async def render(
# Add context to arguments if needed
call_args = inject_context(self.fn, arguments or {}, context, self.context_kwarg)

if inspect.iscoroutinefunction(self.fn):
result = await self.fn(**call_args)
fn = self.fn
if is_async_callable(fn):
result = await fn(**call_args)
else:
result = await anyio.to_thread.run_sync(functools.partial(self.fn, **call_args))

Expand Down
7 changes: 4 additions & 3 deletions src/mcp/server/mcpserver/resources/templates.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
from __future__ import annotations

import functools
import inspect
import re
from collections.abc import Callable
from typing import TYPE_CHECKING, Any
Expand All @@ -15,6 +14,7 @@
from mcp.server.mcpserver.resources.types import FunctionResource, Resource
from mcp.server.mcpserver.utilities.context_injection import find_context_parameter, inject_context
from mcp.server.mcpserver.utilities.func_metadata import func_metadata
from mcp.shared._callable_inspection import is_async_callable
from mcp.types import Annotations, Icon

if TYPE_CHECKING:
Expand Down Expand Up @@ -112,8 +112,9 @@ async def create_resource(
# Add context to params if needed
params = inject_context(self.fn, params, context, self.context_kwarg)

if inspect.iscoroutinefunction(self.fn):
result = await self.fn(**params)
fn = self.fn
if is_async_callable(fn):
result = await fn(**params)
else:
result = await anyio.to_thread.run_sync(functools.partial(self.fn, **params))

Expand Down
11 changes: 7 additions & 4 deletions src/mcp/server/mcpserver/resources/types.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
"""Concrete resource implementations."""

import inspect
from __future__ import annotations

import json
from collections.abc import Callable
from pathlib import Path
Expand All @@ -14,6 +15,7 @@
from pydantic import Field, ValidationInfo, validate_call

from mcp.server.mcpserver.resources.base import Resource
from mcp.shared._callable_inspection import is_async_callable
from mcp.types import Annotations, Icon


Expand Down Expand Up @@ -55,8 +57,9 @@ class FunctionResource(Resource):
async def read(self) -> str | bytes:
"""Read the resource by calling the wrapped function."""
try:
if inspect.iscoroutinefunction(self.fn):
result = await self.fn()
fn = self.fn
if is_async_callable(fn):
result = await fn()
else:
result = await anyio.to_thread.run_sync(self.fn)

Expand All @@ -83,7 +86,7 @@ def from_function(
icons: list[Icon] | None = None,
annotations: Annotations | None = None,
meta: dict[str, Any] | None = None,
) -> "FunctionResource":
) -> FunctionResource:
"""Create a FunctionResource from a function."""
func_name = name or fn.__name__
if func_name == "<lambda>": # pragma: no cover
Expand Down
14 changes: 2 additions & 12 deletions src/mcp/server/mcpserver/tools/base.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
from __future__ import annotations

import functools
import inspect
from collections.abc import Callable
from functools import cached_property
from typing import TYPE_CHECKING, Any
Expand All @@ -11,6 +9,7 @@
from mcp.server.mcpserver.exceptions import ToolError
from mcp.server.mcpserver.utilities.context_injection import find_context_parameter
from mcp.server.mcpserver.utilities.func_metadata import FuncMetadata, func_metadata
from mcp.shared._callable_inspection import is_async_callable
from mcp.shared.exceptions import UrlElicitationRequiredError
from mcp.shared.tool_name_validation import validate_and_warn_tool_name
from mcp.types import Icon, ToolAnnotations
Expand Down Expand Up @@ -63,7 +62,7 @@ def from_function(
raise ValueError("You must provide a name for lambda functions")

func_doc = description or fn.__doc__ or ""
is_async = _is_async_callable(fn)
is_async = is_async_callable(fn)

if context_kwarg is None: # pragma: no branch
context_kwarg = find_context_parameter(fn)
Expand Down Expand Up @@ -118,12 +117,3 @@ async def run(
raise
except Exception as e:
raise ToolError(f"Error executing tool {self.name}: {e}") from e


def _is_async_callable(obj: Any) -> bool:
while isinstance(obj, functools.partial): # pragma: lax no cover
obj = obj.func

return inspect.iscoroutinefunction(obj) or (
callable(obj) and inspect.iscoroutinefunction(getattr(obj, "__call__", None))
)
33 changes: 33 additions & 0 deletions src/mcp/shared/_callable_inspection.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
"""Callable inspection utilities.

Adapted from Starlette's `is_async_callable` implementation.
https://github.com/encode/starlette/blob/main/starlette/_utils.py
"""

from __future__ import annotations

import functools
import inspect
from collections.abc import Awaitable, Callable
from typing import Any, TypeGuard, TypeVar, overload

T = TypeVar("T")

AwaitableCallable = Callable[..., Awaitable[T]]


@overload
def is_async_callable(obj: AwaitableCallable[T]) -> TypeGuard[AwaitableCallable[T]]: ...


@overload
def is_async_callable(obj: Any) -> TypeGuard[AwaitableCallable[Any]]: ...


def is_async_callable(obj: Any) -> Any:
while isinstance(obj, functools.partial): # pragma: lax no cover
obj = obj.func

return inspect.iscoroutinefunction(obj) or (
callable(obj) and inspect.iscoroutinefunction(getattr(obj, "__call__", None))
Comment on lines +26 to +32
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 new src/mcp/shared/_callable_inspection.py module lacks a corresponding tests/shared/test_callable_inspection.py, violating CLAUDE.md's requirements that 'Test files mirror the source tree' and 'New features require tests'. The functools.partial unwrapping path — the key capability this PR advertises — is marked # pragma: lax no cover and is never exercised by any test.

Extended reasoning...

What the bug is and how it manifests

The PR introduces a new public shared module src/mcp/shared/_callable_inspection.py exporting is_async_callable. CLAUDE.md explicitly requires: (1) 'New features require tests' and (2) 'Test files mirror the source tree: src/mcp/client/streamable_http.py -> tests/client/test_streamable_http.py'. The tests/shared/ directory already exists and contains test files for every other shared module (test_auth.py, test_httpx_utils.py, test_otel.py, test_tool_name_validation.py, etc.), making the absence of tests/shared/test_callable_inspection.py a clear CLAUDE.md violation.

The specific code path that is untested

The critical new capability advertised by this PR is support for functools.partial-wrapped coroutines:

The while isinstance(obj, functools.partial) branch is marked # pragma: lax no cover and is never reached by any test. There are zero tests anywhere in the test suite that pass a functools.partial-wrapped coroutine to is_async_callable or to any of the callers (prompts, resources, templates, tools).

Why existing tests do not cover the gap

The only indirect coverage comes from test_tool_manager.py::test_add_async_callable_object, which tests a callable object with an async call for tools only. This exercises the second branch (callable(obj) and inspect.iscoroutinefunction(getattr(obj, 'call', None))) but not the partial-unwrapping loop. The direct-coroutine path for prompts, resources, and templates is also only covered end-to-end, not as unit tests for the shared helper.

Impact

If the partial-unwrapping logic contained a regression (e.g., incorrect unwrapping), no test would catch it. The PR's primary advertised fix — making prompts/resources/templates work with functools.partial-wrapped coroutines — has no test coverage whatsoever.

How to fix

Add tests/shared/test_callable_inspection.py covering at least: (1) a plain coroutine, (2) a sync function, (3) an object with async call, (4) a functools.partial wrapping a coroutine (single and nested), and (5) a functools.partial wrapping a sync function.

Addressing the refutation

The refutation argues this mirrors the original private _is_async_callable in tools/base.py having no dedicated tests. However, that function was private and module-scoped; is_async_callable is now a public shared API used across four modules. The refutation also notes # pragma: lax no cover is legitimate for platform-specific lines — true, but the pragma's presence confirms the path is untested, regardless of whether that is intentional for coverage reporting. CLAUDE.md's mirror-the-tree rule applies to new source files regardless of whether the logic was previously private.

Step-by-step proof of the gap

  1. The PR adds src/mcp/shared/_callable_inspection.py with a functools.partial unwrapping loop.
  2. grep -rn 'functools.partial' tests/ returns zero hits for any test exercising the is_async_callable helper.
  3. The # pragma: lax no cover on the while-loop confirms the loop body is never executed during the test run.
  4. tests/shared/ has no test_callable_inspection.py despite the mirror pattern holding for every other shared module.
  5. The refutation argument that the original had no dedicated tests does not excuse the new public module from CLAUDE.md's stated requirements.

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.

The CLAUDE.md is underspecified. I don't believe in unit testing what is not public facing - it should be reachable by the tests, but we shouldn't test this specifically.

It's hard to refactor code that depends on private implementations.

Comment on lines +26 to +32
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 new is_async_callable function in src/mcp/shared/_callable_inspection.py lacks a function-level docstring, violating CLAUDE.md's rule that public APIs must have docstrings. Since this function is named without an underscore prefix and is imported by 4 modules across the codebase, adding a docstring explaining its parameters, TypeGuard return type, and the functools.partial-unwrapping behavior would clarify its contract.

Extended reasoning...

CLAUDE.md line 16 states explicitly: 'Public APIs must have docstrings'. The is_async_callable function introduced in this PR has no function-level docstring — only the containing module has one.

The refutation argues that because the module is named _callable_inspection.py (underscore prefix), the function should be considered internal. This argument has merit by Python convention: underscore-prefixed modules signal internal implementation. However, the function itself carries no such prefix and is clearly designed to be shared — it lives in mcp.shared and is already imported by 4 consumer modules (prompts/base.py, resources/templates.py, resources/types.py, tools/base.py). The module docstring explicitly notes it is 'Adapted from Starlette's is_async_callable implementation', reinforcing that it is meant to be a shared utility, not a hidden implementation detail.

The function also has non-trivial behavior worth documenting: (1) it unwraps functools.partial objects recursively before checking, (2) it handles callable objects with an async call method, and (3) it returns a TypeGuard[AwaitableCallable[T]] in the overloads, which is significant for static type narrowing. None of these behaviors are explained without a docstring, and new contributors importing the function would only have the module docstring and the overload stubs to go on.

Concrete example: a developer adding a new module that needs to check whether a callable is async would import is_async_callable. Without a docstring, they would need to read the implementation to know that passing a functools.partial(asyncfn) returns True, or that an object with async call is handled. A docstring makes this immediately clear.

The fix is minimal: add a docstring to the def is_async_callable(obj: Any) -> Any implementation overload (the one with a body) explaining the parameter, return value, and the two special cases it handles beyond inspect.iscoroutinefunction.

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.

This is not a public API. Claude doesn't know what's a public API.

)
Loading