-
Notifications
You must be signed in to change notification settings - Fork 3.1k
refactor: replace lowlevel Server decorators with on_* constructor kwargs #1985
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?
Conversation
acea3d7 to
8e1d947
Compare
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
| if handler is not None: | ||
| self._notification_handlers[method] = handler | ||
|
|
||
| def _add_request_handler( |
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.
not as type safe as if we did the Handler approach here.
I don't think we decided for sure whether we wanted to be able to register/remove endpoints at runtime. Thoughts @Kludex ?
|
|
||
| # TODO(maxisbey): remove private access — completion needs post-construction | ||
| # handler registration, find a better pattern for this | ||
| self._lowlevel_server._add_request_handler( # pyright: ignore[reportPrivateUsage] |
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.
This usecase makes sense to me for adding request handlers after the fact... Unless we change it so that the Server object inside MCPServer is only constructed when you actually run the server, not when you construct MCPServer?
Thoughts @Kludex ?
src/mcp/server/lowlevel/server.py
Outdated
| ] = lifespan, | ||
| # Request handlers | ||
| on_list_tools: Callable[ | ||
| [ServerRequestContext[LifespanResultT, Any], types.PaginatedRequestParams | None], |
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.
This should be RequestT, why did you drop it? 🤔
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.
I was thinking that it doesn't make sense to have RequestT be a generic parameter referencing the Transport on the Server class. This is because Server can be instantiated and then used for many different Transports, and you would only know the actual type depending on what you pass in to the run method.
I could be wrong, but it didn't make sense to me. You thinking something else?
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.
But the ServerRequestContext depends on it anyway, so it seems something is wrong here.
This Any shouldn't be here.
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.
I've changed RequestT in the context object to default to Any since it'll be refactored in #2021 anyway.
I've removed the Any, here and in a few other places.
28c7209 to
7221cca
Compare
…args Replace the decorator-based handler registration on the lowlevel Server with direct on_* keyword arguments on the constructor. Handlers are raw callables with a uniform (ctx, params) -> result signature. - Server constructor takes on_list_tools, on_call_tool, etc. - String-keyed dispatch instead of type-keyed - Remove RequestT generic from Server (transport-specific, not bound at construction) - Delete handler.py and func_inspection.py (no longer needed) - Update ExperimentalHandlers to use callback-based registration - Update MCPServer to pass on_* kwargs via _create_handler_kwargs() - Update migration docs and docstrings
- Fix all migration.md examples to use ServerRequestContext instead of RequestContext - Fix all imports to use 'from mcp.server import Server, ServerRequestContext' - Apply ruff formatting to code examples - Add Server constructor calls to examples that were missing them - Re-export ServerRequestContext from mcp.server.__init__ - Add type hints to TaskResultHandler docstring example
…erver Move handler functions from a dict-returning method to private methods on MCPServer, passed directly to the Server constructor by name. This eliminates the **kwargs unpacking pattern and makes the handler registration explicit.
7221cca to
7e9e53f
Compare
Allow users to override default task handlers by passing on_get_task, on_task_result, on_list_tasks, and on_cancel_task to enable_tasks(). User-provided handlers are registered first; defaults fill in the rest.
src/mcp/server/lowlevel/server.py
Outdated
| _task_support=task_support, | ||
| ), | ||
| ) | ||
| await handler(ctx, getattr(notify, "params", None)) |
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.
why do we need to use getattr?
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.
Yea it's not, removed.
src/mcp/server/lowlevel/server.py
Outdated
|
|
||
|
|
||
| async def _ping_handler(request: types.PingRequest) -> types.ServerResult: | ||
| async def _ping_handler(ctx: Any, params: Any) -> types.EmptyResult: |
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.
why Any again? Please check all Anys in this branch.
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.
Yea definitely shouldn't be Any here. I've updated this Any as well as gone through all the other Anys added, I think it's in a better spot now.
The subscribe capability in ResourcesCapability was hardcoded to False, even when on_subscribe_resource handler was provided. Now dynamically checks whether a subscribe handler is registered. Also applies ruff formatting to experimental.py.
Replace the decorator-based handler registration on the lowlevel
Serverwith directon_*keyword arguments on the constructor. Handlers are now raw callables with a uniform(ctx, params) -> resultsignature, dispatched by method string.func_inspection.py(no longer needed without decorator introspection)RequestTgeneric fromServer(transport-specific, never bound at construction)ExperimentalHandlersfrom Server internals via callbacksMCPServerto passon_*kwargs via_create_handler_kwargs()RequestContext.request_idoptional (notification handlers passNone)Tests and examples not yet updated — they still use the old decorator API.
Supersedes #1968.