Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (2)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds an MCP subpackage: public exports (including new MCP result types), dataclasses for tools/results and resource templates, decorators to attach MCP metadata, a JSON‑RPC MCPService with HTTP routing and logging middleware, utility helpers, an example MCP server, and an "mcp" extra dependency. (49 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTP as HTTP Handler
participant RPC as JSON‑RPC Server
participant Svc as MCPService
participant Registry as Tool/Resource Registry
Client->>HTTP: POST /mcp (JSON‑RPC)
HTTP->>RPC: deliver request
RPC->>Svc: invoke RPC method
alt tools/list
Svc->>Registry: enumerate tools
Registry-->>Svc: tools metadata
else tools/call
Svc->>Registry: lookup tool by name
Registry-->>Svc: tool function
Svc->>Svc: execute(tool, arguments)
Svc->>Svc: transform MCPToolResult -> MCP content (text/resource_link)
else resources/list/read
Svc->>Registry: lookup templates/lists
alt found
Svc->>Registry: call resource handler
Registry-->>Svc: resource data
else not found
Svc-->>RPC: error response
end
end
Svc-->>RPC: RPC result
RPC-->>HTTP: JSON‑RPC response
HTTP-->>Client: HTTP response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces MCP (Model Context Protocol) server support to the ASAB ecosystem, enabling the framework to serve as an MCP server for AI assistants and other MCP clients.
Key Changes:
- Added a new
asab.mcpmodule with service, decorators, and utility functions - Implemented MCP protocol support including tools, resources, and resource templates
- Added
aiohttp-rpcdependency for JSON-RPC server functionality
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| setup.py | Added 'mcp' extras with aiohttp-rpc dependency and removed extra blank line |
| asab/mcp/init.py | Module initialization exposing MCP service, decorators, and data classes |
| asab/mcp/service.py | Core MCP service implementing protocol methods for tools, resources, and templates |
| asab/mcp/decorators.py | Decorators for defining MCP tools and resource templates |
| asab/mcp/datacls.py | Data classes for MCP tool info, results, and resource templates |
| asab/mcp/utils.py | Utility functions including ping, null pruning, and URI template matching |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
asab/mcp/utils.py
Outdated
| ''' | ||
|
|
||
| # URI template is ie "note://{path}", uri is e.g. "note://notes/mynote.md" | ||
| return pattern == uri |
There was a problem hiding this comment.
This function is supposed to perform RFC 6570 URI Template matching according to the docstring, but it only performs a simple equality check. This will not correctly match URI templates with variables (e.g., 'note://{path}' will never match 'note://notes/mynote.md').
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
asab/mcp/service.py
Outdated
|
|
||
| import aiohttp_rpc | ||
|
|
||
| from .utils import rcpcall_ping, prune_nulls |
There was a problem hiding this comment.
Corrected spelling of 'rcpcall_ping' to 'rpccall_ping'.
asab/mcp/utils.py
Outdated
| # TODO: This is a placeholder implementation, more complete implementation of RFC 6570 URI Template Matching is needed | ||
|
|
||
| # URI template is ie "note://{path}", uri is e.g. "note://notes/mynote.md" | ||
| return pattern == uri |
There was a problem hiding this comment.
The placeholder implementation returns a boolean instead of the documented dictionary of variables. This will cause type errors for callers expecting a dict return value.
| return pattern == uri | |
| if pattern == uri: | |
| return {} | |
| else: | |
| return None |
asab/mcp/service.py
Outdated
|
|
||
| self.RPCServer.add_method(aiohttp_rpc.JsonRpcMethod(self._rcpcall_mcp_initialize, name="initialize")) | ||
| self.RPCServer.add_method(aiohttp_rpc.JsonRpcMethod(self._rcpcall_notifications_initialized, name="notifications/initialized")) | ||
| self.RPCServer.add_method(aiohttp_rpc.JsonRpcMethod(rcpcall_ping, name="ping")) |
There was a problem hiding this comment.
Corrected spelling of 'rcpcall_ping' to 'rpccall_ping'.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
setup.py (1)
90-96: MCP extra wiring looks good; consider version bounding foraiohttp-rpc.The new
mcpextra cleanly isolates the optional MCP dependency. You may want to add a lower/upper version bound foraiohttp-rpconce you know which versions are tested, to reduce the risk of future breaking changes.asab/mcp/__init__.py (1)
1-11: Align__all__ordering with Ruff’s RUF022 (sorted exports).The public exports look right. Ruff is complaining only about ordering; to keep CI happy you can sort
__all__alphabetically, e.g.:-__all__ = [ - "MCPService", - "mcp_tool", - "mcp_resource_template", - "MCPToolInfo", - "MCPResourceTemplateInfo", -] +__all__ = [ + "MCPResourceTemplateInfo", + "MCPService", + "MCPToolInfo", + "mcp_resource_template", + "mcp_tool", +]asab/mcp/utils.py (1)
2-9: Silence Ruff ARG001 by underscoring unused RPC parameters.
rcpcall_pingintentionally accepts arbitrary args/kwargs for JSON‑RPC compatibility; you can avoid Ruff’s unused-arg warnings by underscoring them:-def rcpcall_ping(*args, **kwargs): +def rcpcall_ping(*_args, **_kwargs):No behavioral change, just linter noise reduction.
asab/mcp/datacls.py (1)
1-46: Align type hints with actualNoneusage for schemas andtitle.
MCPToolInfo.inputSchema/outputSchemaandMCPToolResultResourceLink.titleare annotated as non‑optional types, but you pass/allowNonein practice (via decorator defaults and the= Nonedefault ontitle). To keep type-checkers and IDEs happy, it’s worth marking these as optional and giving defaults where appropriate, e.g.:-import dataclasses +import dataclasses +from typing import Optional @@ @dataclasses.dataclass class MCPToolInfo: name: str title: str description: str - inputSchema: dict - outputSchema: dict + inputSchema: Optional[dict] = None + outputSchema: Optional[dict] = None @@ class MCPToolResultResourceLink(MCPToolResult): @@ - mimeType: str - title: str = None # For resource listing + mimeType: str + title: Optional[str] = None # For resource listingThis matches how the decorators and service already use these fields without changing behavior.
asab/mcp/service.py (4)
69-69: Rename unused*args, **kwargsparameters to quiet Ruff ARG002.Several RPC handlers take
*args, **kwargspurely to satisfy the JSON‑RPC method signature but never use them. To keep Ruff happy without changing behavior, you can underscore them:- async def _rcpcall_mcp_initialize(self, capabilities=None, clientInfo=None, *args, **kwargs): + async def _rcpcall_mcp_initialize(self, capabilities=None, clientInfo=None, *_args, **_kwargs): - async def _rcpcall_tools_list(self, *args, **kwargs): + async def _rcpcall_tools_list(self, *_args, **_kwargs): - async def _rcpcall_tools_call(self, name, arguments, *args, **kwargs): + async def _rcpcall_tools_call(self, name, arguments, *_args, **_kwargs): - async def _rcpcall_resources_list(self, *args, **kwargs): + async def _rcpcall_resources_list(self, *_args, **_kwargs): - async def _rcpcall_resources_read(self, uri, *args, **kwargs): + async def _rcpcall_resources_read(self, uri, *_args, **_kwargs): - async def _rcpcall_resource_templates_list(self, *args, **kwargs): + async def _rcpcall_resource_templates_list(self, *_args, **_kwargs): - async def _rcpcall_notifications_initialized(self, *args, **kwargs): + async def _rcpcall_notifications_initialized(self, *_args, **_kwargs):Also applies to: 103-103, 118-118, 177-177, 206-206, 241-241, 256-256
69-100: Avoid reusingcapabilitiesfor both input and output in_rcpcall_mcp_initialize.You first normalize the incoming
capabilitiesargument, then immediately overwrite it with a new dict for server capabilities. That’s slightly confusing and makes it impossible to inspect client‑provided capabilities later.Consider using a separate variable for the server side, e.g.:
clientInfo = clientInfo or {} L.log(...) server_capabilities = {} if self.Tools: server_capabilities["tools"] = {"listChanged": True} if self.ResourceTemplates or self.ResourceLists: server_capabilities["resources"] = {"listChanged": True} return { ..., "capabilities": server_capabilities, }This keeps the shape of the response unchanged but clarifies intent and leaves room to use client capabilities in the future if needed.
206-238:resources/readrouting ignoresuriTemplateand the URI matcher util (forward-looking note).Right now
_rcpcall_resources_readpicks the first template whose_uriPrefixis a prefix ofuri, and then delegatesfnct(uri). This is fine for simple prefix-based routing, but:
- The
MCPResourceTemplateInfo.uriTemplatefield is never used here.- The helper
uri_template_match_fromutils.pyalso isn’t used.- Overlapping prefixes rely solely on registration order.
None of this is wrong for a first cut, but once you start using real URI templates per the spec, you’ll probably want to route via
uriTemplateand the matcher (possibly returning extracted variables, not just prefix matches) to avoid ambiguity.
266-275: Logging middleware is straightforward; just ensure it matchesaiohttp_rpc’s request/response API.The middleware’s structure (await handler, then log based on
response.errorandrequest.method_name) is clean and unobtrusive. The only caveat is that it relies onaiohttp_rpcproviding those attributes on its request/response objects.If you haven’t already, please double-check this against the
aiohttp-rpcmiddleware examples. I can help tweak the middleware shape if their API differs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
asab/mcp/__init__.py(1 hunks)asab/mcp/datacls.py(1 hunks)asab/mcp/decorators.py(1 hunks)asab/mcp/service.py(1 hunks)asab/mcp/utils.py(1 hunks)setup.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
asab/mcp/__init__.py (3)
asab/mcp/service.py (1)
MCPService(15-263)asab/mcp/decorators.py (2)
mcp_tool(4-14)mcp_resource_template(17-28)asab/mcp/datacls.py (2)
MCPToolInfo(5-10)MCPResourceTemplateInfo(40-46)
asab/mcp/decorators.py (1)
asab/mcp/datacls.py (2)
MCPToolInfo(5-10)MCPResourceTemplateInfo(40-46)
asab/mcp/service.py (3)
asab/mcp/utils.py (2)
rcpcall_ping(2-9)prune_nulls(12-45)asab/mcp/datacls.py (2)
MCPToolResultTextContent(19-23)MCPToolResultResourceLink(27-36)asab/abc/service.py (1)
Service(4-81)
🪛 Ruff (0.14.5)
asab/mcp/__init__.py
5-11: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
asab/mcp/utils.py
2-2: Unused function argument: args
(ARG001)
2-2: Unused function argument: kwargs
(ARG001)
asab/mcp/service.py
45-45: Avoid specifying long messages outside the exception class
(TRY003)
55-55: Avoid specifying long messages outside the exception class
(TRY003)
69-69: Unused method argument: args
(ARG002)
69-69: Unused method argument: kwargs
(ARG002)
103-103: Unused method argument: args
(ARG002)
103-103: Unused method argument: kwargs
(ARG002)
118-118: Unused method argument: args
(ARG002)
118-118: Unused method argument: kwargs
(ARG002)
128-128: Avoid specifying long messages outside the exception class
(TRY003)
134-134: Do not catch blind exception: Exception
(BLE001)
169-169: Prefer TypeError exception for invalid type
(TRY004)
169-169: Avoid specifying long messages outside the exception class
(TRY003)
177-177: Unused method argument: args
(ARG002)
177-177: Unused method argument: kwargs
(ARG002)
199-199: Prefer TypeError exception for invalid type
(TRY004)
199-199: Avoid specifying long messages outside the exception class
(TRY003)
206-206: Unused method argument: args
(ARG002)
206-206: Unused method argument: kwargs
(ARG002)
225-225: Avoid specifying long messages outside the exception class
(TRY003)
241-241: Unused method argument: args
(ARG002)
241-241: Unused method argument: kwargs
(ARG002)
256-256: Unused method argument: args
(ARG002)
256-256: Unused method argument: kwargs
(ARG002)
🔇 Additional comments (3)
asab/mcp/utils.py (1)
12-45:prune_nullslogic is solid and matches intended MCP usage.The recursive pruning correctly:
- drops
Nonevalues,- removes private keys starting with
_,- eliminates empty dicts/lists produced by pruning,
which fits how it’s used ondataclasses.asdict(...)results in the service.asab/mcp/decorators.py (1)
1-28: Decorators align cleanly with MCPService registration.Both
mcp_toolandmcp_resource_templatecorrectly attach the expected metadata (_mcp_tool_info/_mcp_resource_template_info), matching howMCPService.add_toolandadd_resource_templateconsume it. Stripping name/title/description is a nice touch to avoid accidental whitespace bugs.asab/mcp/service.py (1)
177-203: Resources/list path is clear; type guard is strict by design.
_rcpcall_resources_listcorrectly:
- aggregates all resource lists,
- enforces that each item is an
MCPToolResultResourceLink, and- uses
prune_nullsso_uriPrefix/Nonefields don’t leak into the response.The strict
ValueErroron unsupported resource types is a reasonable early guard, just be aware it will surface as a server error if a list function returns anything else.If you later need to support additional resource representations, we can adjust the mapping and error type accordingly.
asab/mcp/service.py
Outdated
| async def _rcpcall_tools_call(self, name, arguments, *args, **kwargs): | ||
| ''' | ||
| To invoke a tool, clients send a tools/call request. | ||
|
|
||
| https://modelcontextprotocol.io/specification/2025-06-18/server/tools#invoking-tools | ||
| ''' | ||
|
|
||
| x = self.Tools.get(name) | ||
| if x is None: | ||
| L.warning("Tool not found", struct_data={"name": name}) | ||
| raise KeyError(f"Tool {name} not found") | ||
|
|
||
| tool_function, _ = x | ||
|
|
||
| try: | ||
| result = await tool_function(**arguments) | ||
| except Exception as e: | ||
| L.exception("Tool failed", struct_data={"name": name, "error": str(e)}) | ||
| return { | ||
| "content": [{ | ||
| "type": "text", | ||
| "text": "General error occured." | ||
| }], | ||
| "isError": True, | ||
| } | ||
|
|
||
| if not isinstance(result, list): | ||
| result = [result] | ||
|
|
||
| transformed_result = [] | ||
| for item in result: | ||
| if isinstance(item, MCPToolResultTextContent): | ||
| transformed_result.append({ | ||
| "type": "text", | ||
| "text": item.text, | ||
| }) | ||
| elif isinstance(item, str): | ||
| # A shotcut for Text content. | ||
| transformed_result.append({ | ||
| "type": "text", | ||
| "text": item, | ||
| }) | ||
| elif isinstance(item, MCPToolResultResourceLink): | ||
| transformed_result.append({ | ||
| "type": "resource_link", | ||
| "uri": item.uri, | ||
| "name": item.name, | ||
| "description": item.description, | ||
| "mimeType": item.mimeType, | ||
| }) | ||
| else: | ||
| raise ValueError(f"Unsupported result type: {type(item)}") | ||
|
|
||
| return { | ||
| "content": transformed_result, | ||
| "isError": False, | ||
| } |
There was a problem hiding this comment.
Tighten up tools/call result handling (typo fix + minor ergonomics).
Two small points in _rcpcall_tools_call:
- The generic error message has a typo:
-
"text": "General error occured."
-
"text": "General error occurred."
- For unsupported result types you raise
ValueError; conceptually this is closer to aTypeError(wrong result type), but that’s mostly a naming/detail choice and not a blocker.
Otherwise the flow (lookup, await tool, normalize to list, map MCPToolResultTextContent / str / MCPToolResultResourceLink) looks good and matches the MCP expectations.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async def _rcpcall_tools_call(self, name, arguments, *args, **kwargs): | |
| ''' | |
| To invoke a tool, clients send a tools/call request. | |
| https://modelcontextprotocol.io/specification/2025-06-18/server/tools#invoking-tools | |
| ''' | |
| x = self.Tools.get(name) | |
| if x is None: | |
| L.warning("Tool not found", struct_data={"name": name}) | |
| raise KeyError(f"Tool {name} not found") | |
| tool_function, _ = x | |
| try: | |
| result = await tool_function(**arguments) | |
| except Exception as e: | |
| L.exception("Tool failed", struct_data={"name": name, "error": str(e)}) | |
| return { | |
| "content": [{ | |
| "type": "text", | |
| "text": "General error occured." | |
| }], | |
| "isError": True, | |
| } | |
| if not isinstance(result, list): | |
| result = [result] | |
| transformed_result = [] | |
| for item in result: | |
| if isinstance(item, MCPToolResultTextContent): | |
| transformed_result.append({ | |
| "type": "text", | |
| "text": item.text, | |
| }) | |
| elif isinstance(item, str): | |
| # A shotcut for Text content. | |
| transformed_result.append({ | |
| "type": "text", | |
| "text": item, | |
| }) | |
| elif isinstance(item, MCPToolResultResourceLink): | |
| transformed_result.append({ | |
| "type": "resource_link", | |
| "uri": item.uri, | |
| "name": item.name, | |
| "description": item.description, | |
| "mimeType": item.mimeType, | |
| }) | |
| else: | |
| raise ValueError(f"Unsupported result type: {type(item)}") | |
| return { | |
| "content": transformed_result, | |
| "isError": False, | |
| } | |
| async def _rcpcall_tools_call(self, name, arguments, *args, **kwargs): | |
| ''' | |
| To invoke a tool, clients send a tools/call request. | |
| https://modelcontextprotocol.io/specification/2025-06-18/server/tools#invoking-tools | |
| ''' | |
| x = self.Tools.get(name) | |
| if x is None: | |
| L.warning("Tool not found", struct_data={"name": name}) | |
| raise KeyError(f"Tool {name} not found") | |
| tool_function, _ = x | |
| try: | |
| result = await tool_function(**arguments) | |
| except Exception as e: | |
| L.exception("Tool failed", struct_data={"name": name, "error": str(e)}) | |
| return { | |
| "content": [{ | |
| "type": "text", | |
| "text": "General error occurred." | |
| }], | |
| "isError": True, | |
| } | |
| if not isinstance(result, list): | |
| result = [result] | |
| transformed_result = [] | |
| for item in result: | |
| if isinstance(item, MCPToolResultTextContent): | |
| transformed_result.append({ | |
| "type": "text", | |
| "text": item.text, | |
| }) | |
| elif isinstance(item, str): | |
| # A shotcut for Text content. | |
| transformed_result.append({ | |
| "type": "text", | |
| "text": item, | |
| }) | |
| elif isinstance(item, MCPToolResultResourceLink): | |
| transformed_result.append({ | |
| "type": "resource_link", | |
| "uri": item.uri, | |
| "name": item.name, | |
| "description": item.description, | |
| "mimeType": item.mimeType, | |
| }) | |
| else: | |
| raise ValueError(f"Unsupported result type: {type(item)}") | |
| return { | |
| "content": transformed_result, | |
| "isError": False, | |
| } |
🧰 Tools
🪛 Ruff (0.14.5)
118-118: Unused method argument: args
(ARG002)
118-118: Unused method argument: kwargs
(ARG002)
128-128: Avoid specifying long messages outside the exception class
(TRY003)
134-134: Do not catch blind exception: Exception
(BLE001)
169-169: Prefer TypeError exception for invalid type
(TRY004)
169-169: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In asab/mcp/service.py around lines 118 to 174, fix two small issues in
_rcpcall_tools_call: correct the typo in the error response text from "General
error occured." to "General error occurred." and replace the raised ValueError
for unsupported result types with a TypeError (e.g. raise
TypeError(f"Unsupported result type: {type(item)}")) so the exception better
reflects a wrong-type condition.
asab/mcp/utils.py
Outdated
| def uri_template_match_(pattern, uri: str) -> dict: | ||
| ''' | ||
| Check if the URI matches the URI template. | ||
| Returns a dictionary of the variables in the URI template or None | ||
|
|
||
| According to RFC 6570 URI Template Matching | ||
| ''' | ||
|
|
||
| # URI template is ie "note://{path}", uri is e.g. "note://notes/mynote.md" | ||
| return pattern == uri |
There was a problem hiding this comment.
uri_template_match_ return type/docs don’t match the implementation.
The function is annotated to return dict and the docstring talks about returning a dictionary (or None), but it currently returns a bool (pattern == uri). That mismatch can be surprising to callers.
Either make it a boolean helper explicitly, or adjust it to return mappings as documented. For a minimal, behavior-preserving fix you could align the signature and docstring with the current boolean behavior:
-def uri_template_match_(pattern, uri: str) -> dict:
- '''
- Check if the URI matches the URI template.
- Returns a dictionary of the variables in the URI template or None
-
- According to RFC 6570 URI Template Matching
- '''
-
- # URI template is ie "note://{path}", uri is e.g. "note://notes/mynote.md"
- return pattern == uri
+def uri_template_match_(pattern: str, uri: str) -> bool:
+ '''
+ Temporary placeholder matcher.
+
+ Returns True if the URI matches the template pattern exactly, False otherwise.
+ '''
+ # URI template is ie "note://{path}", uri is e.g. "note://notes/mynote.md"
+ return pattern == uriYou can later evolve this into a real RFC 6570 matcher that returns extracted variables.
🤖 Prompt for AI Agents
In asab/mcp/utils.py around lines 49-58 the function uri_template_match_ is
annotated to return dict and its docstring describes returning a dict or None,
but the implementation returns a boolean (pattern == uri); align the API by
changing the return annotation to bool and updating the docstring to state it
returns True/False whether the URI exactly matches the template (or, if you
prefer the original contract, implement RFC6570 extraction instead); for a
minimal, behavior-preserving fix, update the function signature to -> bool and
replace the docstring accordingly so callers and type checkers match the actual
behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
asab/mcp/utils.py (1)
49-60: Return type mismatch remains unresolved.This issue was flagged in previous reviews. The function signature indicates it returns
dict, but the implementation returnsbool(pattern == uri). The TODO acknowledges this is a placeholder for RFC 6570 URI Template Matching.Since this is acknowledged as incomplete and has been flagged before, consider either:
- Implementing basic template variable extraction to match the contract, or
- Updating the signature to
-> booland the docstring to reflect the current exact-match behaviorasab/mcp/service.py (1)
8-8: Critical: Import name mismatch will cause ImportError.The import statement uses
rcpcall_pingbut the function inutils.pyis defined asrpccall_ping(with two 'p's). This was flagged in previous reviews but remains unfixed.Apply this diff to fix the import:
-from .utils import rcpcall_ping, prune_nulls +from .utils import rpccall_ping, prune_nulls
🧹 Nitpick comments (2)
asab/mcp/service.py (2)
118-174: Consider using TypeError for type validation.The error handling and result transformation logic is solid. However, line 169 raises
ValueErrorfor unsupported result types whenTypeErrorwould be more semantically correct for a type mismatch.Apply this diff:
else: - raise ValueError(f"Unsupported result type: {type(item)}") + raise TypeError(f"Unsupported result type: {type(item)}")The bare
Exceptioncatch on line 134 is a trade-off—it prevents tool errors from breaking the RPC response but may hide bugs. This is acceptable for an RPC error boundary, but consider logging the full exception context (which you already do on line 135).
177-203: Consider using TypeError for type validation.The resource listing logic is correct. Similar to the tools handler, line 199 should raise
TypeErrorinstead ofValueErrorfor unsupported resource types.Apply this diff:
else: - raise ValueError(f"Unsupported resource type: {type(resource)}") + raise TypeError(f"Unsupported resource type: {type(resource)}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
asab/mcp/service.py(1 hunks)asab/mcp/utils.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
asab/mcp/service.py (2)
asab/mcp/utils.py (1)
prune_nulls(12-45)asab/mcp/datacls.py (2)
MCPToolResultTextContent(19-23)MCPToolResultResourceLink(27-36)
🪛 Ruff (0.14.5)
asab/mcp/service.py
45-45: Avoid specifying long messages outside the exception class
(TRY003)
55-55: Avoid specifying long messages outside the exception class
(TRY003)
69-69: Unused method argument: args
(ARG002)
69-69: Unused method argument: kwargs
(ARG002)
103-103: Unused method argument: args
(ARG002)
103-103: Unused method argument: kwargs
(ARG002)
118-118: Unused method argument: args
(ARG002)
118-118: Unused method argument: kwargs
(ARG002)
128-128: Avoid specifying long messages outside the exception class
(TRY003)
134-134: Do not catch blind exception: Exception
(BLE001)
169-169: Prefer TypeError exception for invalid type
(TRY004)
169-169: Avoid specifying long messages outside the exception class
(TRY003)
177-177: Unused method argument: args
(ARG002)
177-177: Unused method argument: kwargs
(ARG002)
199-199: Prefer TypeError exception for invalid type
(TRY004)
199-199: Avoid specifying long messages outside the exception class
(TRY003)
206-206: Unused method argument: args
(ARG002)
206-206: Unused method argument: kwargs
(ARG002)
225-225: Avoid specifying long messages outside the exception class
(TRY003)
241-241: Unused method argument: args
(ARG002)
241-241: Unused method argument: kwargs
(ARG002)
256-256: Unused method argument: args
(ARG002)
256-256: Unused method argument: kwargs
(ARG002)
asab/mcp/utils.py
2-2: Unused function argument: args
(ARG001)
2-2: Unused function argument: kwargs
(ARG001)
🔇 Additional comments (11)
asab/mcp/utils.py (2)
2-9: LGTM! Intentional parameter pattern for JSON-RPC handler.The unused
*args, **kwargsparameters are intentional for JSON-RPC handlers to accept any additional parameters without raising errors. This is a common pattern for health-check endpoints.
12-45: LGTM! Clean recursive pruning logic.The function correctly handles nested structures and removes None values, private keys (starting with
_), and empty containers. The implementation is clear and correct.asab/mcp/service.py (9)
17-37: Initialization logic looks good once import is fixed.The MCPService initialization properly sets up registries, the RPC server with middleware, and registers all required MCP protocol methods. Line 29 will work correctly once the import on line 8 is fixed.
40-61: LGTM! Clear registration methods.The registration methods properly check for metadata (either as parameter or function attribute) and raise appropriate errors if missing. The pattern is consistent and clear.
64-66: TODO noted for future enhancement.The passthrough to RPCServer is appropriate for now. The TODO about tenant and authorization handling is noted for future work.
69-100: LGTM! Proper MCP initialization response.The initialization handler correctly builds capabilities based on registered tools and resources. The unused
*args, **kwargsparameters are intentional for JSON-RPC handler compatibility.
103-115: LGTM! Clean tool listing implementation.The method properly converts tool metadata to dict format and prunes null values for clean JSON responses. Pagination can be added later as needed.
206-238: LGTM! Resource reading logic is functional.The URI prefix matching approach is simple and effective. The TODO comments appropriately mark areas for future enhancement (direct resource handling and more MCP-compliant error responses).
241-253: LGTM! Consistent with tool listing pattern.The resource template listing follows the same clean pattern as tool listing, with proper null pruning and dict conversion.
256-263: LGTM! Appropriate notification handler.The initialized notification handler correctly acknowledges the client's initialization completion with logging and an empty response.
266-275: LGTM! Clean logging middleware.The middleware properly logs both successful and failed JSON-RPC requests with appropriate log levels and structured data.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
asab/mcp/utils.py (2)
2-9:rpccall_pingimplementation is fine; you can optionally silence unused-arg warnings.The handler correctly matches a generic MCP
pingand returns an empty dict. If Ruff’s ARG001 is noisy, consider renaming the parameters to make the intent explicit:-def rpccall_ping(*args, **kwargs): +def rpccall_ping(*_, **__):
12-45:prune_nullslogic looks good; align the docstring with the actual behavior.The recursive pruning does what you need and works well with
dataclasses.asdict, but it also drops dict keys starting with"_", which the docstring doesn’t mention. Consider documenting that “private” keys are removed too:- Recursively remove keys with value None, list items that are None, - and any empty dicts/lists that become empty as a result. + Recursively remove: + - dict keys whose value is None + - dict keys starting with "_" (treated as private/internal) + - list items that are None + - any empty dicts/lists that become empty as a result.asab/mcp/service.py (7)
15-37: RPC server wiring is clear; consider adding aiohttp-rpc’s exception middleware to harden error handling.Tools/resources registration and route setup look good. To ensure unhandled exceptions in RPC methods are consistently converted into JSON-RPC errors (rather than bubbling as generic 500s), you may want to include
aiohttp_rpc.middlewares.exception_middlewareahead of your custom logging middleware:- self.RPCServer = aiohttp_rpc.JsonRpcServer(middlewares=[logging_middleware]) + self.RPCServer = aiohttp_rpc.JsonRpcServer( + middlewares=[aiohttp_rpc.middlewares.exception_middleware, logging_middleware], + )(Adjust for the exact server/middleware names of the aiohttp-rpc version you pin.)
64-67: HTTP handler delegation is fine; keep tenant/auth TODO in mind.Forwarding directly to
self.RPCServer.handle_http_requestis straightforward, but you currently ignore the{tenant}path parameter and any authorization context. At some point you’ll likely want to surfacerequest.match_info["tenant"](and auth data) into RPC context so tools can enforce per-tenant access.
69-100:_rcpcall_mcp_initializeworks, but thecapabilitiesargument is effectively unused.You set
capabilities = capabilities or {}and then immediately overwrite it with{}at Line 79, so any client-provided capabilities are ignored. Either remove the parameter/first assignment or start incorporating client capabilities (e.g., for negotiated features) to avoid confusion for callers.
118-174:tools/callflow is sound; consider unifying error shapes for missing tools vs tool failures.Normalizing
resultto a list and mappingMCPToolResultTextContent/str/MCPToolResultResourceLinkto MCP content objects is well-structured. The only behavioral nit is that “tool not found” raisesKeyError(surfacing as an RPC error), whereas tool execution failures return{content: [...], isError: True}; if you want clients to handle a single uniform result shape, you might return anisError=Truepayload for the “missing tool” case as well instead of raising.
177-203: Resource listing pipeline looks good; ready for future pagination/filtering.Aggregating from all
ResourceListsand normalizingMCPToolResultResourceLinkinstances viaprune_nullsyields a cleanresourcesarray. When you later add cursor/URI filters from the MCP request parameters, you can extend this handler to honor them while keeping the current transformation logic.
206-238:resources/readprefix matching is a reasonable first cut; template matching and error contract could be refined.Selecting a template via
uri.startswith(mcp_resource_template_info._uriPrefix)is a simple and workable heuristic, but it won’t handle full URI templates (e.g., RFC 6570-style patterns) or overlapping prefixes. Longer term you may want a dedicated URI-template matcher and/or explicit template fields rather than relying on a private_uriPrefix. Also, for “not found” you currently raiseKeyError; if you want a uniform response shape, consider returning an emptycontentslist or a structured error payload instead.
266-275:logging_middlewareis useful; be careful with the method attribute name on the request.The overall pattern (run handler, then log success vs failure based on
response.error) is good. To avoid AttributeError across aiohttp-rpc versions, it may be safer to resolve the method name defensively, since some versions expose it asmethodrather thanmethod_name:- if response.error is None: - L.log(asab.LOG_NOTICE, "JSON-RPC request completed", struct_data={"method": request.method_name}) - else: - L.warning("JSON-RPC request failed", struct_data={ - "method": request.method_name, - "error": response.error.message, - }) + method = getattr(request, "method_name", getattr(request, "method", None)) + if response.error is None: + L.log(asab.LOG_NOTICE, "JSON-RPC request completed", struct_data={"method": method}) + else: + L.warning("JSON-RPC request failed", struct_data={ + "method": method, + "error": getattr(response.error, "message", str(response.error)), + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
asab/mcp/service.py(1 hunks)asab/mcp/utils.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
asab/mcp/service.py (3)
asab/mcp/utils.py (2)
rpccall_ping(2-9)prune_nulls(12-45)asab/mcp/datacls.py (2)
MCPToolResultTextContent(19-23)MCPToolResultResourceLink(27-36)asab/abc/service.py (1)
Service(4-81)
🪛 Ruff (0.14.5)
asab/mcp/utils.py
2-2: Unused function argument: args
(ARG001)
2-2: Unused function argument: kwargs
(ARG001)
asab/mcp/service.py
45-45: Avoid specifying long messages outside the exception class
(TRY003)
55-55: Avoid specifying long messages outside the exception class
(TRY003)
69-69: Unused method argument: args
(ARG002)
69-69: Unused method argument: kwargs
(ARG002)
103-103: Unused method argument: args
(ARG002)
103-103: Unused method argument: kwargs
(ARG002)
118-118: Unused method argument: args
(ARG002)
118-118: Unused method argument: kwargs
(ARG002)
128-128: Avoid specifying long messages outside the exception class
(TRY003)
134-134: Do not catch blind exception: Exception
(BLE001)
169-169: Prefer TypeError exception for invalid type
(TRY004)
169-169: Avoid specifying long messages outside the exception class
(TRY003)
177-177: Unused method argument: args
(ARG002)
177-177: Unused method argument: kwargs
(ARG002)
199-199: Prefer TypeError exception for invalid type
(TRY004)
199-199: Avoid specifying long messages outside the exception class
(TRY003)
206-206: Unused method argument: args
(ARG002)
206-206: Unused method argument: kwargs
(ARG002)
225-225: Avoid specifying long messages outside the exception class
(TRY003)
241-241: Unused method argument: args
(ARG002)
241-241: Unused method argument: kwargs
(ARG002)
256-256: Unused method argument: args
(ARG002)
256-256: Unused method argument: kwargs
(ARG002)
🔇 Additional comments (4)
asab/mcp/service.py (4)
40-61: Tool/resource registration helpers enforce metadata nicely.Using
*_mcp_*_infoattached by decorators, and failing fast withValueErrorwhen metadata is missing, gives a clean contract for contributors wiring new tools/templates/resource lists. No functional issues from this block.
103-115: Tool listing via dataclasses +prune_nullsis a solid approach.Serializing
mcp_tool_infowithdataclasses.asdictand then cleaning it withprune_nullsgives a clean MCP-facing schema while hiding internal/None fields. This looks correct and consistent with the spec links in the docstring.
241-253: Resource template listing mirrors tools listing correctly.Using
dataclasses.asdictplusprune_nullsforresourceTemplateskeeps the wire format aligned with tools/list and hides internal/None fields. No issues here.
256-263:notifications/initializedhandler is appropriately minimal.Logging a NOTICE and returning an empty object is sufficient for this notification; it keeps the server’s side-effects explicit without complicating the response schema.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
examples/mcp_server.py (1)
18-20: Prefer late binding for tool registration.Registering
self.tool_hello_worldfrom__init__grabs the bound coroutine object; when the event loop later imports the module, that binding persists even if the method is overridden in subclasses/tests. Consider deferringadd_toolto initialization hooks (e.g.,async def initialize) or registering the function object directly to keep overriding flexible.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
asab/mcp/service.py(1 hunks)asab/mcp/utils.py(1 hunks)examples/mcp_server.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
asab/mcp/service.py (3)
asab/mcp/utils.py (2)
rpc_ping(2-9)prune_nulls(12-45)asab/mcp/datacls.py (2)
MCPToolResultTextContent(19-23)MCPToolResultResourceLink(27-36)asab/abc/service.py (1)
Service(4-81)
examples/mcp_server.py (3)
asab/web/__init__.py (1)
create_web_server(22-59)asab/mcp/service.py (2)
MCPService(15-263)add_tool(40-47)asab/mcp/decorators.py (1)
mcp_tool(4-14)
🪛 Ruff (0.14.5)
asab/mcp/utils.py
2-2: Unused function argument: args
(ARG001)
2-2: Unused function argument: kwargs
(ARG001)
asab/mcp/service.py
45-45: Avoid specifying long messages outside the exception class
(TRY003)
55-55: Avoid specifying long messages outside the exception class
(TRY003)
69-69: Unused method argument: args
(ARG002)
69-69: Unused method argument: kwargs
(ARG002)
103-103: Unused method argument: args
(ARG002)
103-103: Unused method argument: kwargs
(ARG002)
118-118: Unused method argument: args
(ARG002)
118-118: Unused method argument: kwargs
(ARG002)
128-128: Avoid specifying long messages outside the exception class
(TRY003)
134-134: Do not catch blind exception: Exception
(BLE001)
169-169: Prefer TypeError exception for invalid type
(TRY004)
169-169: Avoid specifying long messages outside the exception class
(TRY003)
177-177: Unused method argument: args
(ARG002)
177-177: Unused method argument: kwargs
(ARG002)
199-199: Prefer TypeError exception for invalid type
(TRY004)
199-199: Avoid specifying long messages outside the exception class
(TRY003)
206-206: Unused method argument: args
(ARG002)
206-206: Unused method argument: kwargs
(ARG002)
225-225: Avoid specifying long messages outside the exception class
(TRY003)
241-241: Unused method argument: args
(ARG002)
241-241: Unused method argument: kwargs
(ARG002)
256-256: Unused method argument: args
(ARG002)
256-256: Unused method argument: kwargs
(ARG002)
examples/mcp_server.py
1-1: Shebang is present but file is not executable
(EXE001)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
asab/mcp/__init__.py (1)
5-13: Consider sorting__all__alphabetically.Static analysis suggests applying isort-style alphabetical sorting to the
__all__list for consistency.Apply this diff to sort the exports:
__all__ = [ - "MCPService", - "mcp_tool", - "mcp_resource_template", "MCPToolInfo", "MCPResourceTemplateInfo", "MCPToolResultResourceLink", "MCPToolResultTextContent", + "MCPService", + "mcp_resource_template", + "mcp_tool", ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
asab/mcp/__init__.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
asab/mcp/__init__.py (3)
asab/mcp/service.py (1)
MCPService(15-263)asab/mcp/decorators.py (2)
mcp_tool(4-14)mcp_resource_template(17-28)asab/mcp/datacls.py (4)
MCPToolInfo(5-10)MCPResourceTemplateInfo(40-46)MCPToolResultTextContent(19-23)MCPToolResultResourceLink(27-36)
🪛 Ruff (0.14.5)
asab/mcp/__init__.py
5-13: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build (3.9)
- GitHub Check: build (3.11)
- GitHub Check: build (3.10)
- GitHub Check: build (3.13)
- GitHub Check: build (3.12)
🔇 Additional comments (1)
asab/mcp/__init__.py (1)
1-3: LGTM!The imports correctly re-export the public API from the MCP submodules, establishing a clean package interface.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
asab/mcp/service.py (5)
35-35: Fix method name typo:_rcp_tools_list→_rpc_tools_list.The method name has a typo where "rpc" is misspelled as "rcp". This typo is also present in the method definition at line 113.
Apply this diff:
- self.RPCServer.add_method(aiohttp_rpc.JsonRpcMethod(self._rcp_tools_list, name="tools/list")) + self.RPCServer.add_method(aiohttp_rpc.JsonRpcMethod(self._rpc_tools_list, name="tools/list"))
77-110: Client capabilities are being discarded.The
capabilitiesparameter received at line 77 is overwritten at line 91 with an empty dict, losing any capabilities the client declared (e.g., experimental features, roots, sampling). This prevents proper client-server capability negotiation.Apply this diff to preserve client capabilities:
async def _rpc_mcp_initialize(self, capabilities=None, clientInfo=None, *args, **kwargs): - capabilities = capabilities or {} + client_capabilities = capabilities or {} clientInfo = clientInfo or {} L.log(asab.LOG_NOTICE, "MCP Client initializing", struct_data={ "name": clientInfo.get('name', 'unknown'), "version": clientInfo.get('version', 'unknown'), }) instructions = "" for instruction in self.Instructions.values(): instructions += instruction + "\n" - capabilities = {} + server_capabilities = {} if len(self.Tools) > 0: - capabilities['tools'] = { + server_capabilities['tools'] = { 'listChanged': True, } if len(self.ResourceTemplates) > 0 or len(self.ResourceLists) > 0: - capabilities['resources'] = { + server_capabilities['resources'] = { 'listChanged': True, } return { "protocolVersion": "2024-11-05", "serverInfo": { "name": self.Name, "version": self.Version, }, "instructions": instructions, - "capabilities": capabilities, + "capabilities": server_capabilities, }
113-113: Fix method name typo:_rcp_tools_list→_rpc_tools_list.The method name has a typo where "rpc" is misspelled as "rcp".
Apply this diff:
- async def _rcp_tools_list(self, *args, **kwargs): + async def _rpc_tools_list(self, *args, **kwargs):
128-184: Default missing tool arguments to an empty dict.When the client omits the
argumentsparameter, aiohttp-rpc passesNone, causing line 143tool_function(**arguments)to raise "argument after ** must be a mapping". This breaks zero-argument tools.Apply this diff:
- async def _rpc_tools_call(self, name, arguments, *args, **kwargs): + async def _rpc_tools_call(self, name, arguments=None, *args, **kwargs): ''' To invoke a tool, clients send a tools/call request. https://modelcontextprotocol.io/specification/2025-06-18/server/tools#invoking-tools ''' + if arguments is None: + arguments = {} + if not isinstance(arguments, dict): + raise TypeError("MCP tool arguments must be an object") + x = self.Tools.get(name)Additionally, fix the typo at line 149 and improve exception types:
- "text": "General error occured." + "text": "General error occurred."- raise ValueError(f"Unsupported result type: {type(item)}") + raise TypeError(f"Unsupported result type: {type(item)}")
276-285: Guardresponse.erroraccess for non-JsonRpc responses.Line 278 directly accesses
response.error, which will raiseAttributeErrorif the response is a rawaiohttp.web.Response(e.g., on protocol errors). The middleware needs to handle both JsonRpc and non-JsonRpc response types.Apply this diff:
async def logging_middleware(request, handler): response = await handler(request) - if response.error is None: + error = getattr(response, "error", None) + if error is None: L.log(asab.LOG_NOTICE, "JSON-RPC request completed", struct_data={"method": request.method_name}) else: L.warning("JSON-RPC request failed", struct_data={ "method": request.method_name, - "error": response.error.message, + "error": getattr(error, "message", str(error)), }) return response
🧹 Nitpick comments (1)
asab/mcp/service.py (1)
187-213: Consider usingTypeErrorfor type validation.The implementation correctly iterates resource lists and transforms results. However, line 209 raises
ValueErrorfor type mismatches;TypeErrorwould be more semantically appropriate.Apply this diff if you prefer more precise exception types:
- raise ValueError(f"Unsupported resource type: {type(resource)}") + raise TypeError(f"Unsupported resource type: {type(resource)}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
asab/mcp/service.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
asab/mcp/service.py (3)
asab/mcp/utils.py (2)
rpc_ping(2-9)prune_nulls(12-45)asab/mcp/datacls.py (2)
MCPToolResultTextContent(19-23)MCPToolResultResourceLink(27-36)asab/abc/service.py (1)
Service(4-81)
🪛 Ruff (0.14.5)
asab/mcp/service.py
49-49: Avoid specifying long messages outside the exception class
(TRY003)
59-59: Avoid specifying long messages outside the exception class
(TRY003)
77-77: Unused method argument: args
(ARG002)
77-77: Unused method argument: kwargs
(ARG002)
113-113: Unused method argument: args
(ARG002)
113-113: Unused method argument: kwargs
(ARG002)
128-128: Unused method argument: args
(ARG002)
128-128: Unused method argument: kwargs
(ARG002)
138-138: Avoid specifying long messages outside the exception class
(TRY003)
144-144: Do not catch blind exception: Exception
(BLE001)
179-179: Prefer TypeError exception for invalid type
(TRY004)
179-179: Avoid specifying long messages outside the exception class
(TRY003)
187-187: Unused method argument: args
(ARG002)
187-187: Unused method argument: kwargs
(ARG002)
209-209: Prefer TypeError exception for invalid type
(TRY004)
209-209: Avoid specifying long messages outside the exception class
(TRY003)
216-216: Unused method argument: args
(ARG002)
216-216: Unused method argument: kwargs
(ARG002)
235-235: Avoid specifying long messages outside the exception class
(TRY003)
251-251: Unused method argument: args
(ARG002)
251-251: Unused method argument: kwargs
(ARG002)
266-266: Unused method argument: args
(ARG002)
266-266: Unused method argument: kwargs
(ARG002)
🔇 Additional comments (6)
asab/mcp/service.py (6)
1-13: LGTM!Imports are clean and appropriate for the MCP service implementation.
44-69: LGTM!The registration methods follow a clean, consistent pattern. The approach of checking for decorator-attached metadata (
_mcp_tool_info,_mcp_resource_template_info) before falling back to explicit parameters provides good ergonomics.
72-74: Ensure tenant and authorization are addressed before production use.The TODO comment indicates that tenant handling and authorization are not yet implemented. The route includes a
{tenant}parameter that is currently ignored.Verify that this is intentional for the current development phase and that tenant/authorization will be implemented before production deployment.
114-125: LGTM!The tools list implementation correctly iterates over registered tools, converts them to dictionaries, and prunes null values for clean JSON-RPC responses.
251-263: LGTM!The resource templates list handler follows the same clean pattern as
_rcp_tools_list, correctly transforming registered templates into MCP-compliant responses.
266-273: LGTM!The initialized notification handler correctly acknowledges client initialization per the MCP specification.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
asab/mcp/service.py (2)
127-185: Harden tools/call argument handling and result-type error.Two small issues in
_rpc_tools_call:
- Arguments normalization & type check
arguments = arguments or {}means:
Nonebecomes{}(good for zero-arg tools),- but any falsy value (
[],"",0) also becomes{}, which can silently drop client-sent data.Also,
tool_function(**arguments)assumes a mapping; if a non-dict slips through, Python will raise at the**expansion.To make behavior explicit and safer, you can:
- async def _rpc_tools_call(self, name, arguments, *args, **kwargs): + async def _rpc_tools_call(self, name, arguments=None, *args, **kwargs): @@ - arguments = arguments or {} + if arguments is None: + arguments = {} + elif not isinstance(arguments, dict): + raise TypeError("MCP tool arguments must be an object")That aligns better with the MCP expectation that
argumentsis an object, and gives a clear error if the client sends the wrong shape.
- Unsupported result type exception
For the final
elsebranch:else: raise ValueError(f"Unsupported result type: {type(item)}")this is conceptually a type mismatch, so
TypeErroris a more precise signal and also matches the linter suggestion:- else: - raise ValueError(f"Unsupported result type: {type(item)}") + else: + raise TypeError(f"Unsupported result type: {type(item)}")The rest of the flow (logging on failure, mapping known result types to MCP content, and normalizing non-list results to a list) looks good.
276-285: Guard against responses without.errorin logging middleware.
logging_middlewareassumesresponse.errorexists:response = await handler(request) if response.error is None: ... else: ... response.error.message ...On protocol or HTTP-level errors, the handler may return a plain
aiohttp.web.Response(or other object) without an.errorattribute, which would raiseAttributeErrorhere and mask the real problem. A safer pattern:async def logging_middleware(request, handler): response = await handler(request) - if response.error is None: - L.log(asab.LOG_NOTICE, "JSON-RPC request completed", struct_data={"method": request.method_name}) - else: - L.warning("JSON-RPC request failed", struct_data={ - "method": request.method_name, - "error": response.error.message, - }) + error = getattr(response, "error", None) + method = getattr(request, "method_name", getattr(request, "method", None)) + if error is None: + L.log(asab.LOG_NOTICE, "JSON-RPC request completed", struct_data={"method": method}) + else: + L.warning("JSON-RPC request failed", struct_data={ + "method": method, + "error": getattr(error, "message", str(error)), + }) return responseThis preserves your structured logging for JSON-RPC responses while avoiding crashes when the response object doesn’t expose
.error.
🧹 Nitpick comments (5)
asab/mcp/service.py (5)
64-69:resource_uri_prefixis stored but never used.
add_resource_listkeysself.ResourceListsbyresource_uri_prefix, but_rpc_resources_listonly iterates over the values and ignores the key. If the prefix isn’t needed elsewhere, consider dropping it from the API; if it’s meant for future routing, a short comment or TODO would clarify intent.
72-75: HTTP handler currently ignores tenant and authorization.
_handle_http_requestdrops the{tenant}path param and performs no auth, as noted by the TODO. Before exposing this on shared or multi-tenant deployments, make sure tenant resolution and authorization are wired in (router, middleware, or here) so MCP isn’t effectively world-writeable.
77-273: Tidy unused*args, **kwargsacross RPC handlers.Several RPC handlers accept
*args, **kwargsbut don’t use them (_rpc_mcp_initialize,_rcp_tools_list,_rpc_tools_call,_rpc_resources_list,_rpc_resources_read,_rpc_resource_templates_list,_rpc_notifications_initialized), which Ruff reports as ARG002. If they’re only present to match the RPC framework’s call signature, consider renaming them to_args,_kwargsor explicitlydel args, kwargsat the top of the function to quiet the linter without changing behavior.
112-124: Consider renaming_rcp_tools_list→_rpc_tools_listfor consistency.Minor naming nit: this handler is registered under
"tools/list"but the function is spelled_rcp_tools_list(missing the second “p”), while other RPC handlers use_rpc_*. Renaming would make the API surface more uniform (remember to update the registration call).
187-213: Unsupported resource type should likely be aTypeError.In
_rpc_resources_list, theelsebranch raisesValueErrorforUnsupported resource type. Semantically this is also a type mismatch, soTypeErrorwould be more accurate and consistent with_rpc_tools_callif you adopt the same change there:- else: - raise ValueError(f"Unsupported resource type: {type(resource)}") + else: + raise TypeError(f"Unsupported resource type: {type(resource)}")Functionally it’s fine as-is; this is just a tightening of the error semantics.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
asab/mcp/service.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
asab/mcp/service.py (3)
asab/mcp/utils.py (2)
rpc_ping(2-9)prune_nulls(12-45)asab/mcp/datacls.py (2)
MCPToolResultTextContent(19-23)MCPToolResultResourceLink(27-36)asab/abc/service.py (1)
Service(4-81)
🪛 Ruff (0.14.5)
asab/mcp/service.py
49-49: Avoid specifying long messages outside the exception class
(TRY003)
59-59: Avoid specifying long messages outside the exception class
(TRY003)
77-77: Unused method argument: args
(ARG002)
77-77: Unused method argument: kwargs
(ARG002)
112-112: Unused method argument: args
(ARG002)
112-112: Unused method argument: kwargs
(ARG002)
127-127: Unused method argument: args
(ARG002)
127-127: Unused method argument: kwargs
(ARG002)
138-138: Avoid specifying long messages outside the exception class
(TRY003)
144-144: Do not catch blind exception: Exception
(BLE001)
179-179: Prefer TypeError exception for invalid type
(TRY004)
179-179: Avoid specifying long messages outside the exception class
(TRY003)
187-187: Unused method argument: args
(ARG002)
187-187: Unused method argument: kwargs
(ARG002)
209-209: Prefer TypeError exception for invalid type
(TRY004)
209-209: Avoid specifying long messages outside the exception class
(TRY003)
216-216: Unused method argument: args
(ARG002)
216-216: Unused method argument: kwargs
(ARG002)
235-235: Avoid specifying long messages outside the exception class
(TRY003)
251-251: Unused method argument: args
(ARG002)
251-251: Unused method argument: kwargs
(ARG002)
266-266: Unused method argument: args
(ARG002)
266-266: Unused method argument: kwargs
(ARG002)
🔇 Additional comments (4)
asab/mcp/service.py (4)
17-42: MCPService initialization and RPC wiring look solid.Service registration, internal registries, JSON-RPC server setup, and method registrations are coherent and align well with the MCP flows; I don’t see functional issues here.
44-62: add_tool / add_resource_template behavior is reasonable.Auto-pulling metadata from decorators when explicit info isn’t passed is a nice DX touch, and the validation on missing metadata is appropriate; no changes needed from my side here.
251-263: Resource templates listing matches the tools listing pattern.
_rpc_resource_templates_listmirrors_rcp_tools_listby usingdataclasses.asdict+prune_nullsover the registered templates; that’s consistent and should serialize cleanly.
266-273: notifications/initialized handler is straightforward.Lightweight logging of the notification and returning
{}is aligned with the spec and looks fine as an endpoint implementation.
| async def _rpc_mcp_initialize(self, capabilities=None, clientInfo=None, *args, **kwargs): | ||
| capabilities = capabilities.copy() or {} | ||
| clientInfo = clientInfo or {} | ||
|
|
||
| L.log(asab.LOG_NOTICE, "MCP Client initializing", struct_data={ | ||
| "name": clientInfo.get('name', 'unknown'), | ||
| "version": clientInfo.get('version', 'unknown'), | ||
|
|
||
| }) | ||
|
|
||
| instructions = "" | ||
| for instruction in self.Instructions.values(): | ||
| instructions += instruction + "\n" | ||
|
|
||
| if len(self.Tools) > 0: | ||
| capabilities['tools'] = { | ||
| 'listChanged': True, | ||
| } | ||
|
|
||
| if len(self.ResourceTemplates) > 0 or len(self.ResourceLists) > 0: | ||
| capabilities['resources'] = { | ||
| 'listChanged': True, | ||
| } | ||
|
|
||
| return { | ||
| "protocolVersion": "2024-11-05", | ||
| "serverInfo": { | ||
| "name": self.Name, | ||
| "version": self.Version, | ||
| }, | ||
| "instructions": instructions, | ||
| "capabilities": capabilities, | ||
| } |
There was a problem hiding this comment.
Fix capabilities.copy() when capabilities is None.
With the MCP initialize request, capabilities is optional; when omitted you’ll hit AttributeError on capabilities.copy(). Initializing via the or {} branch needs to happen before calling .copy().
You can do:
- async def _rpc_mcp_initialize(self, capabilities=None, clientInfo=None, *args, **kwargs):
- capabilities = capabilities.copy() or {}
+ async def _rpc_mcp_initialize(self, capabilities=None, clientInfo=None, *args, **kwargs):
+ if capabilities is None:
+ capabilities = {}
+ else:
+ capabilities = capabilities.copy()The rest of the capability augmentation logic looks fine.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async def _rpc_mcp_initialize(self, capabilities=None, clientInfo=None, *args, **kwargs): | |
| capabilities = capabilities.copy() or {} | |
| clientInfo = clientInfo or {} | |
| L.log(asab.LOG_NOTICE, "MCP Client initializing", struct_data={ | |
| "name": clientInfo.get('name', 'unknown'), | |
| "version": clientInfo.get('version', 'unknown'), | |
| }) | |
| instructions = "" | |
| for instruction in self.Instructions.values(): | |
| instructions += instruction + "\n" | |
| if len(self.Tools) > 0: | |
| capabilities['tools'] = { | |
| 'listChanged': True, | |
| } | |
| if len(self.ResourceTemplates) > 0 or len(self.ResourceLists) > 0: | |
| capabilities['resources'] = { | |
| 'listChanged': True, | |
| } | |
| return { | |
| "protocolVersion": "2024-11-05", | |
| "serverInfo": { | |
| "name": self.Name, | |
| "version": self.Version, | |
| }, | |
| "instructions": instructions, | |
| "capabilities": capabilities, | |
| } | |
| async def _rpc_mcp_initialize(self, capabilities=None, clientInfo=None, *args, **kwargs): | |
| if capabilities is None: | |
| capabilities = {} | |
| else: | |
| capabilities = capabilities.copy() | |
| clientInfo = clientInfo or {} | |
| L.log(asab.LOG_NOTICE, "MCP Client initializing", struct_data={ | |
| "name": clientInfo.get('name', 'unknown'), | |
| "version": clientInfo.get('version', 'unknown'), | |
| }) | |
| instructions = "" | |
| for instruction in self.Instructions.values(): | |
| instructions += instruction + "\n" | |
| if len(self.Tools) > 0: | |
| capabilities['tools'] = { | |
| 'listChanged': True, | |
| } | |
| if len(self.ResourceTemplates) > 0 or len(self.ResourceLists) > 0: | |
| capabilities['resources'] = { | |
| 'listChanged': True, | |
| } | |
| return { | |
| "protocolVersion": "2024-11-05", | |
| "serverInfo": { | |
| "name": self.Name, | |
| "version": self.Version, | |
| }, | |
| "instructions": instructions, | |
| "capabilities": capabilities, | |
| } |
🧰 Tools
🪛 Ruff (0.14.5)
77-77: Unused method argument: args
(ARG002)
77-77: Unused method argument: kwargs
(ARG002)
🤖 Prompt for AI Agents
In asab/mcp/service.py around lines 77 to 109, capabilities.copy() is called
when capabilities may be None which raises AttributeError; fix by ensuring
capabilities is defaulted before copying — e.g. evaluate (capabilities or {})
first and then call .copy() (capabilities = (capabilities or {}).copy()),
leaving the rest of the augmentation logic unchanged.
| async def _rpc_resources_read(self, uri, *args, **kwargs): | ||
| ''' | ||
| To read a resource, clients send a resources/read request. | ||
|
|
||
| https://modelcontextprotocol.io/specification/2025-06-18/server/resources#reading-resources | ||
| ''' | ||
| fnct = None | ||
|
|
||
| # TODO: Check the "direct" | ||
|
|
||
| # Find the resource template function that matches the URI | ||
| if fnct is None: | ||
| for resource_template_function, mcp_resource_template_info in self.ResourceTemplates.values(): | ||
| if uri.startswith(mcp_resource_template_info._uriPrefix): | ||
| fnct = resource_template_function | ||
| break | ||
|
|
||
| if fnct is None: | ||
| # TODO: Find a more compliant way to handle this, but for now we'll just raise an error. | ||
| raise KeyError(f"Resource template {uri} not found") | ||
|
|
||
| result = await fnct(uri) | ||
| if result is None: | ||
| return { | ||
| "contents": [], | ||
| } | ||
|
|
||
| if not isinstance(result, list): | ||
| result = [result] | ||
|
|
||
| return { | ||
| "contents": result, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Ensure resources/read returns JSON-serializable content, not dataclass instances.
_rpc_resources_read returns:
result = await fnct(uri)
...
if not isinstance(result, list):
result = [result]
return {
"contents": result,
}Unlike tools listing and resource templates listing (which use dataclasses.asdict + prune_nulls) and tools/call (which maps to plain dicts), this handler forwards result verbatim. If fnct returns dataclass instances or other non-JSON-serializable objects, the JSON-RPC layer will fail to encode the response.
Two options:
- Require
fnctimplementations to return fully JSON-serializable content structures ({"type": "text", "text": ...}, etc.), and document that contract clearly; or - Mirror the tools handling by transforming known MCP content dataclasses into dicts (or
dataclasses.asdict(...)plusprune_nulls) before returning.
I’d recommend picking one contract and enforcing it here so resources/read can’t break at serialization time.
I'll verify the review comment by examining the codebase to confirm the serialization concerns and compare with other handlers.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find the other handlers mentioned in the review comment
rg -n "def _rpc_tools" asab/mcp/service.py -A 30
Length of output: 908
🏁 Script executed:
#!/bin/bash
# Search for dataclasses.asdict usage in the codebase
rg -n "asdict" asab/ -B 2 -A 2Length of output: 757
🏁 Script executed:
#!/bin/bash
# Look for prune_nulls usage
rg -n "prune_nulls" asab/ -B 2 -A 2Length of output: 1739
🏁 Script executed:
#!/bin/bash
# Check for ResourceTemplates definition and usage
rg -n "ResourceTemplates" asab/mcp/service.py -B 2 -A 2Length of output: 963
🏁 Script executed:
#!/bin/bash
# Search for JSON encoding/serialization mechanisms
rg -n "json\.dumps\|JSONEncoder\|__dict__" asab/mcp/service.py -B 1 -A 1Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for the complete _rpc_resources_read method and surrounding context
sed -n '210,260p' asab/mcp/service.pyLength of output: 1362
🏁 Script executed:
#!/bin/bash
# Search for other _rpc_ methods to compare patterns
rg -n "async def _rpc_" asab/mcp/service.pyLength of output: 426
I need to see the complete _rpc_tools_call method and understand how responses are handled. Let me gather more information.
---
🏁 Script executed:
```shell
#!/bin/bash
# Get the full _rpc_tools_call method
sed -n '127,180p' asab/mcp/service.py
Length of output: 1396
🏁 Script executed:
#!/bin/bash
# Search for how aiohttp_rpc handles responses and JSON encoding
rg -n "aiohttp_rpc" asab/mcp/service.py -B 2 -A 5Length of output: 1407
🏁 Script executed:
#!/bin/bash
# Look for any test files or examples showing resource template function returns
find asab -name "*test*" -o -name "*example*" | head -20Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check the custom JSON encoder in web/rest/json.py
cat asab/web/rest/json.pyLength of output: 7978
🏁 Script executed:
#!/bin/bash
# Look for where _rpc_ methods are registered/called to understand response flow
rg -n "setattr\|getattr\|RpcHandler" asab/mcp/service.py -B 1 -A 1Length of output: 0
I need to understand how the HTTP responses are serialized for RPC calls.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find _handle_http_request implementation
rg -n "_handle_http_request" asab/mcp/service.py -A 20
Length of output: 1928
🏁 Script executed:
#!/bin/bash
# Search for imports and see if JSONDumper or json_response is used
rg -n "from\|import" asab/mcp/service.py | head -30Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look at the file structure to find if there are more details
wc -l asab/mcp/service.pyLength of output: 23
🏁 Script executed:
#!/bin/bash
# Check if there's any serialization setup in the service init or elsewhere
rg -n "json\|dumps\|JSONDumper" asab/mcp/service.pyLength of output: 0
🏁 Script executed:
#!/bin/bash
# Get info about aiohttp_rpc to see if it has built-in JSON encoder handling
rg -n "JsonRpcServer" asab/mcp/service.py -B 2 -A 10Length of output: 806
Let me search for information about aiohttp_rpc's JSON handling and any examples of resource template functions.
🏁 Script executed:
#!/bin/bash
# Look for any examples or tests of resource template usage
find asab -type f -name "*.py" | xargs grep -l "add_resource_template\|resource_template_function" 2>/dev/null | head -5Length of output: 19
🏁 Script executed:
#!/bin/bash
# Search entire codebase for any resource template function definitions
rg -n "add_resource_template" asab/ -B 2 -A 10Length of output: 893
🏁 Script executed:
#!/bin/bash
# Check MCPToolResultTextContent and related dataclasses to understand structure
rg -n "class MCPToolResult\|class MCPResourceContent" asab/mcp/Length of output: 0
🌐 Web query:
aiohttp_rpc JsonRpcServer JSON encoder dataclass handling
💡 Result:
Use the JSONRPCServer's json_serialize hook and pass a json.dumps that knows how to convert dataclasses (and nested non‑JSON types). Example:
- install/see docs: aiohttp-rpc exposes JSONRPCServer with a json_serialize argument. [1][2]
Minimal example:
from aiohttp import web
import aiohttp_rpc
import json
from dataclasses import dataclass, asdict, is_dataclass
from functools import partial
import uuid
@DataClass
class User:
id: uuid.UUID
name: str
async def get_user(user_id) -> User:
return User(id=uuid.UUID(user_id), name="alice")
def default_json(o):
# dataclass -> dict, UUID -> str, fallback to repr
if is_dataclass(o):
return asdict(o)
if isinstance(o, uuid.UUID):
return str(o)
return repr(o)
rpc_server = aiohttp_rpc.JSONRPCServer(
json_serialize=partial(json.dumps, default=default_json),
)
rpc_server.add_method(get_user)
app = web.Application()
mount rpc_server.handle_http_request(...) as your handler
Notes:
- asdict handles nested dataclasses.
- If you need type/field control or validation, consider a dedicated dataclass serializer (e.g., serpyco, dataclasses-serialization) and call that from default_json. [1][3][4]
Sources:
[1] aiohttp-rpc (integration / json_serialize example). (pypi.org)
[2] aiohttp-json-rpc / related projects (implementation patterns). (pypi.org)
[3] serpyco-rs (dataclass serializer alternative). (libraries.io)
[4] dataclasses-serialization (JSON serializer for dataclasses). (pypi.org)
Citations:
- 1: https://pypi.org/project/aiohttp-rpc/2.0.0a1/?utm_source=openai
- 2: https://pypi.org/project/aiohttp-json-rpc/?utm_source=openai
- 3: https://libraries.io/pypi/serpyco-rs?utm_source=openai
- 4: https://pypi.org/project/dataclasses-serialization/?utm_source=openai
Verify the contract: resource template functions may return dataclass instances, and _rpc_resources_read lacks serialization.
The review comment is correct and well-founded. Here's what verification confirmed:
-
Inconsistent handling is real:
_rpc_tools_callexplicitly transforms results to plain dicts (lines 157–176), while_rpc_resources_readreturns result verbatim (line 245). -
aiohttp_rpc has no custom JSON serializer configured: The JsonRpcServer is created without a
json_serializeparameter (line 28), meaning it uses standardjson.dumps(), which cannot serialize dataclass instances. -
Established pattern elsewhere: Other listing handlers use
prune_nulls(dataclasses.asdict(...))(lines 121, 260), suggesting the codebase expects dataclass handling. -
Actual runtime risk: If resource template functions return dataclass instances (a reasonable expectation given the pattern), the JSON-RPC layer will fail at serialization time with
TypeError: Object of type X is not JSON serializable.
The two approaches from the review comment remain valid:
- Explicitly transform known MCP content dataclasses (mirror
_rpc_tools_call), or - Enforce a documentation contract requiring fully JSON-serializable structures.
🧰 Tools
🪛 Ruff (0.14.5)
216-216: Unused method argument: args
(ARG002)
216-216: Unused method argument: kwargs
(ARG002)
235-235: Avoid specifying long messages outside the exception class
(TRY003)
This enables straightforward build of MCP servers in the ASAB microservices ecosystem.
Summary by CodeRabbit
New Features
Chores
Utilities
✏️ Tip: You can customize this high-level summary in your review settings.