-
Notifications
You must be signed in to change notification settings - Fork 3.3k
Use shared is_async_callable instead of inspect.iscoroutinefunction
#2389
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
| ) | ||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.