Use shared is_async_callable instead of inspect.iscoroutinefunction#2389
Use shared is_async_callable instead of inspect.iscoroutinefunction#2389
is_async_callable instead of inspect.iscoroutinefunction#2389Conversation
Extract `_is_async_callable` from `tools/base.py` into `mcp.shared._callable_inspection` and replace all raw `inspect.iscoroutinefunction` calls across prompts, resources, and templates. The shared helper also handles `functools.partial` wrappers and callable objects with an async `__call__`.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — clean extraction of the existing helper into a shared module; inline nits noted.
Extended reasoning...
Overview
This PR extracts the private _is_async_callable helper from tools/base.py into a new shared module mcp/shared/_callable_inspection.py, then replaces inspect.iscoroutinefunction calls in prompts, resources, and templates with the shared helper. The logic itself is unchanged — only its location and visibility change.
Security Risks
None. The change is a pure refactor with no new trust boundaries, no auth or crypto paths, and no user-controlled input handling.
Level of Scrutiny
Low. The code being moved is identical to the private function that was already in tools/base.py. The four call sites are mechanical one-line replacements. The PR is self-contained and pyright/ruff clean.
Other Factors
Both flagged bugs are nits: missing dedicated unit tests for the new shared module (the partial-unwrapping path carries # pragma: lax no cover) and a missing docstring on the public function. Neither affects correctness — 333 existing tests pass and the logic is a faithful transplant of the original. The inline comments capture these adequately for the author to address in a follow-up.
|
|
||
| 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)) |
There was a problem hiding this comment.
🟡 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
- The PR adds src/mcp/shared/_callable_inspection.py with a functools.partial unwrapping loop.
- grep -rn 'functools.partial' tests/ returns zero hits for any test exercising the is_async_callable helper.
- The # pragma: lax no cover on the while-loop confirms the loop body is never executed during the test run.
- tests/shared/ has no test_callable_inspection.py despite the mirror pattern holding for every other shared module.
- The refutation argument that the original had no dedicated tests does not excuse the new public module from CLAUDE.md's stated requirements.
There was a problem hiding this comment.
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.
|
|
||
| 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)) |
There was a problem hiding this comment.
🟡 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.
There was a problem hiding this comment.
This is not a public API. Claude doesn't know what's a public API.
1 participant
Summary
inspect.iscoroutinefunctionmisses callable objects with an async__call__andfunctools.partial-wrapped coroutines. Thetools/base.pymodule already had a local_is_async_callablehelper that handles these cases, but the prompts, resources, and templates modules were using the rawinspect.iscoroutinefunction.This PR extracts the helper into
mcp.shared._callable_inspection.is_async_callable(adapted from Starlette's implementation) and replaces allinspect.iscoroutinefunctionusage across the codebase.Test plan