feat(mcp): enhance MCP server creation with tool discovery and async handling#17
Merged
DavidsonGomes merged 4 commits intoEvolutionAPI:developfrom May 18, 2025
Merged
Conversation
since they are automatically fetched, no need to make them mandatory
- Implement async MCP server tool discovery - Add sync wrapper for tool discovery - Include tool metadata serialization - Add proper file documentation and licensing
Reviewer's GuideThis PR enhances MCP server creation by integrating asynchronous tool discovery (with a synchronous wrapper) when no tools are provided, updating service and API layers for proper async handling, and adjusting the server schema to allow optional tool lists. Sequence Diagram for MCP Server Creation with Tool DiscoverysequenceDiagram
actor Admin
participant APIRoute as "API Route<br>(mcp_server_routes.create_mcp_server)"
participant AsyncHandler as "Async Task Runner<br>(run_in_threadpool)"
participant MCPServerSvc as "MCPServerService<br>(create_mcp_server)"
participant MCPDiscovery as "MCPDiscoveryUtil<br>(discover_mcp_tools)"
participant AsyncDiscover as "AsyncDiscover<br>(_discover_async)"
participant MCPSvc as "MCPService<br>(_connect_to_mcp_server)"
participant ExtMCPServer as "External MCP Server"
Admin->>APIRoute: POST /server (MCPServerCreate data)
APIRoute->>AsyncHandler: run_in_threadpool(MCPServerSvc.create_mcp_server, db, server_data)
AsyncHandler->>MCPServerSvc: create_mcp_server(db, server_data)
MCPServerSvc->>MCPServerSvc: server_data = server.model_dump()
alt Tools NOT provided in server_data
MCPServerSvc->>MCPDiscovery: discover_mcp_tools(config_json)
MCPDiscovery->>AsyncDiscover: asyncio.run(_discover_async(config_json))
AsyncDiscover->>MCPSvc: Create MCPService instance
AsyncDiscover->>MCPSvc: _connect_to_mcp_server(config_json)
MCPSvc->>ExtMCPServer: Connect and fetch tools
ExtMCPServer-->>MCPSvc: Return tools metadata
MCPSvc-->>AsyncDiscover: Raw tools
AsyncDiscover->>AsyncDiscover: Serialize tools
AsyncDiscover-->>MCPDiscovery: Serialized discovered_tools
MCPDiscovery-->>MCPServerSvc: discovered_tools
MCPServerSvc->>MCPServerSvc: server_data["tools"] = discovered_tools
else Tools ARE provided
MCPServerSvc->>MCPServerSvc: server_data["tools"] = [tool.model_dump() for tool in supplied_tools]
end
MCPServerSvc->>MCPServerSvc: db_server = MCPServer(**server_data)
MCPServerSvc->>MCPServerSvc: db.add(db_server)
MCPServerSvc->>MCPServerSvc: db.commit()
MCPServerSvc-->>AsyncHandler: Return db_server
AsyncHandler-->>APIRoute: Return db_server
APIRoute-->>Admin: HTTP 201 Created (MCPServer)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @Danielpeter-99 - I've reviewed your changes - here's some feedback:
- Replace the
printstatement increate_mcp_serverwith a structured logger to avoid cluttering stdout and to support log levels. - Avoid using
asyncio.runinsidediscover_mcp_tools—this can fail if an event loop is already running; consider making your service method fully async or leveraging FastAPI’s background tasks/run_in_threadpoolinstead. - The schema change to
Optional[List[ToolConfig]]with adefault_factory=listis misleading—either acceptNoneand handle it explicitly or just keep it as a non‐optional list.
Here's what I looked at during the review
- 🟡 General issues: 4 issues found
- 🟢 Testing: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| print(f"🔍 Found {len(discovered)} tools.") | ||
| server_data["tools"] = discovered | ||
|
|
||
| else: |
There was a problem hiding this comment.
issue (bug_risk): tool.model_dump() on dict will error; supplied_tools are dicts
Since supplied_tools are already dicts, calling .model_dump() will raise an AttributeError. Either assign supplied_tools directly to server_data["tools"], or loop over BaseModel instances (e.g., server.tools) and call .model_dump() on them.
| supplied_tools = server_data.pop("tools", []) | ||
| if not supplied_tools: | ||
| discovered = discover_mcp_tools(server_data["config_json"]) | ||
| print(f"🔍 Found {len(discovered)} tools.") |
There was a problem hiding this comment.
suggestion: Replace print with structured logging
Use a logger (e.g. logger.info) instead of print so you can control verbosity, include timestamps and context, and integrate with log aggregation.
Comment on lines
+79
to
+82
| if not supplied_tools: | ||
| discovered = discover_mcp_tools(server_data["config_json"]) | ||
| print(f"🔍 Found {len(discovered)} tools.") | ||
| server_data["tools"] = discovered |
There was a problem hiding this comment.
suggestion (bug_risk): Wrap tool discovery in error handling
Use try/except around discover_mcp_tools to catch server errors or unexpected responses and display a clear error message.
Suggested change
| if not supplied_tools: | |
| discovered = discover_mcp_tools(server_data["config_json"]) | |
| print(f"🔍 Found {len(discovered)} tools.") | |
| server_data["tools"] = discovered | |
| if not supplied_tools: | |
| try: | |
| discovered = discover_mcp_tools(server_data["config_json"]) | |
| print(f"🔍 Found {len(discovered)} tools.") | |
| except Exception as e: | |
| logging.error(f"Error discovering MCP tools: {e}") | |
| discovered = [] | |
| server_data["tools"] = discovered |
|
|
||
| def discover_mcp_tools(config_json: Dict[str, Any]) -> List[Dict[str, Any]]: | ||
| """Sync wrapper so we can call it from a sync service function.""" | ||
| return asyncio.run(_discover_async(config_json)) |
There was a problem hiding this comment.
suggestion (bug_risk): asyncio.run may conflict in existing event loops
asyncio.run will raise RuntimeError if called in an active event loop (e.g., FastAPI). Use an async entrypoint (anyio) or run the coroutine in a separate thread/event loop.
| @@ -72,8 +73,16 @@ def create_mcp_server(db: Session, server: MCPServerCreate) -> MCPServer: | |||
| try: | |||
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Extract code out into function (
extract-method) - Use named expression to simplify assignment and conditional (
use-named-expression) - Swap if/else branches (
swap-if-else-branches) - Explicitly raise from a previous error (
raise-from-previous-error)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Enhance MCP server creation by integrating dynamic tool discovery: add an async discovery module with sync wrapper, auto-populate and serialize tools when not provided, adapt the API route to use a threadpool for async handling, and update the schema to accept optional tools.
New Features:
Enhancements:
toolsfield optional in the MCP server schema