-
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
…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
acea3d7 to
8e1d947
Compare
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
| website_url=website_url, | ||
| icons=icons, | ||
| version=version, | ||
| **self._create_handler_kwargs(), |
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 is less nice than the Handler pattern here, but since it's internal not a big issue I suppose?
| 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 ?
| request_id: RequestId | ||
| meta: RequestParamsMeta | None | ||
| session: SessionT | ||
| request_id: RequestId | None = 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.
Maybe worth having a notification specific context object so this doesn't need to be optional
| ) -> CallToolResult: | ||
| if ctx.meta and "progress_token" in ctx.meta: | ||
| await ctx.session.send_progress_notification(ctx.meta["progress_token"], 0.5, 100) | ||
| ... |
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.
Missing to actually pass it to the Server object.
| from mcp.server.lowlevel import Server | ||
| from mcp.shared.context import RequestContext |
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.
| from mcp.server.lowlevel import Server | |
| from mcp.shared.context import RequestContext | |
| from mcp.server import Server, ServerRequestContext |
| return ListToolsResult(tools=[ | ||
| Tool(name="my_tool", description="A tool", inputSchema={}) | ||
| ]) |
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.
use ruff in this code please
| server = Server( | ||
| "my-server", | ||
| on_progress=handle_progress, | ||
| ) |
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.
| server = Server( | |
| "my-server", | |
| on_progress=handle_progress, | |
| ) | |
| server = Server("my-server", on_progress=handle_progress) |
| from mcp.types import CallToolRequestParams, CallToolResult, TextContent | ||
|
|
||
| async def handle_call_tool( | ||
| ctx: RequestContext, params: CallToolRequestParams |
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.
Thinking about this... It's also an option to rename the shared one to be BaseRequestContext, and then have 2 RequestContext that are imported from different modules... 😅
| ctx: RequestContext, params: CallToolRequestParams | |
| ctx: ServerRequestContext, params: CallToolRequestParams |
| ] = 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? 🤔
| _request_handler_map: list[ | ||
| tuple[str, Callable[[ServerRequestContext[LifespanResultT, Any], Any], Awaitable[Any]] | None] | ||
| ] = [ | ||
| ("ping", on_ping), | ||
| ("prompts/list", on_list_prompts), | ||
| ("prompts/get", on_get_prompt), | ||
| ("resources/list", on_list_resources), | ||
| ("resources/templates/list", on_list_resource_templates), | ||
| ("resources/read", on_read_resource), | ||
| ("resources/subscribe", on_subscribe_resource), | ||
| ("resources/unsubscribe", on_unsubscribe_resource), | ||
| ("tools/list", on_list_tools), | ||
| ("tools/call", on_call_tool), | ||
| ("logging/setLevel", on_set_logging_level), | ||
| ("completion/complete", on_completion), | ||
| ] | ||
| for method, handler in _request_handler_map: | ||
| if handler is not None: | ||
| self._request_handlers[method] = 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.
Why a list of tuples instead of a dict?
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.
is this used elsewhere?
| self._lowlevel_server.list_resource_templates()(self.list_resource_templates) | ||
| def _create_handler_kwargs( | ||
| self, | ||
| ) -> dict[str, Callable[[ServerRequestContext[Any, Any], Any], Awaitable[Any]]]: |
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 could be a TypedDict tho, you don't have generics.
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.
and if you do, you now have type safety on the ** usage above.
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.
hmmm... Do you even need this function? Can't you create each handler and pass it above?
| # Transport-specific request context (e.g. starlette Request for HTTP | ||
| # transports, None for stdio). Typed as Any because the server layer is | ||
| # transport-agnostic. | ||
| request_context: Any = 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.
We should find a better solution here...
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.