Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR integrates the Smithery platform deployment system by refactoring the server architecture from a global server instance to a factory pattern. The main change replaces the global mcp = FastMCP(...) with a @smithery.server() decorated create_server() function that returns a FastMCP instance.
Key changes:
- Adds smithery dependency (v0.4.2) and applies
@smithery.server()decorator to factory function - Refactors server initialization from global instance to factory pattern with
create_server()function - Updates all test files to mock
create_serverinstead of the globalmcpinstance
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/databeak/server.py | Implements factory pattern with @smithery.server() decorator and moves server initialization into create_server() function |
| tests/unit/test_server.py | Updates all test mocks to use create_server function instead of global mcp instance |
| tests/integration/conftest.py | Modifies fixture to create server instance via factory function and extract underlying FastMCP |
| src/databeak/init.py | Removes mcp from public API exports |
| pyproject.toml | Adds smithery dependency and tool configuration |
| smithery.yaml | Simplifies configuration to use Python runtime |
| .pre-commit-config.yaml | Adds smithery dependency for pre-commit hooks |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| repo_name = "databeak" | ||
|
|
||
| [tool.smithery] | ||
| server = "databeak.server:smithery_server" |
There was a problem hiding this comment.
The server reference 'smithery_server' doesn't exist. Based on the code changes, it should reference the 'create_server' function that is decorated with '@smithery.server()'.
| server = "databeak.server:smithery_server" | |
| server = "databeak.server:create_server" |
| # Extract the underlying FastMCP instance from the Smithery wrapper | ||
| mcp = smithery_server._fastmcp |
There was a problem hiding this comment.
Accessing private attribute '_fastmcp' on smithery_server is fragile and may not exist. The create_server() function returns a FastMCP instance directly, so 'mcp = smithery_server' should be sufficient.
| # Extract the underlying FastMCP instance from the Smithery wrapper | |
| mcp = smithery_server._fastmcp | |
| # Use the FastMCP instance returned directly by create_server | |
| mcp = smithery_server |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #131 +/- ##
==========================================
+ Coverage 87.66% 87.68% +0.01%
==========================================
Files 27 27
Lines 3251 3255 +4
Branches 485 485
==========================================
+ Hits 2850 2854 +4
Misses 281 281
Partials 120 120
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Code Review: Smithery Platform IntegrationSummaryThis PR implements a factory pattern for server initialization to support Smithery platform deployment. The changes are architecturally sound but have one critical configuration issue that needs to be fixed before merge. Critical Issues1. Incorrect pyproject.toml Configuration 🔴Location: [tool.smithery]
server = "databeak.server:smithery_server"Issue: The configuration references Fix Required: [tool.smithery]
server = "databeak.server:create_server"Impact: This will cause Smithery deployments to fail immediately as it cannot find the specified entry point. Code Quality Issues2. Integration Test Anti-PatternLocation: mcp = smithery_server._fastmcpIssue: Accessing private attribute Recommendation:
Architecture Considerations3. Server Lifecycle ManagementLocation: create_server().run(**run_args)Observation: Each call to
Question: Is this the intended behavior for the factory pattern? Consider if the CLI should cache the server instance for the lifetime of the process. Positive Aspects ✓
Security NotesNo security concerns identified. The changes:
Testing RecommendationsBefore merge, verify:
Performance ConsiderationsThe factory pattern adds negligible overhead (~100μs for initialization per server start). For long-running servers this is acceptable. If the CLI is invoked frequently in automation, consider lazy initialization patterns. Action ItemsMust Fix Before Merge:
Consider:
|
Pull Request Review: Smithery Platform Integration✅ Strengths1. Solid Architectural Pattern 2. Comprehensive Test Updates
3. Configuration Completeness
4. API Surface Cleanup
|
| Aspect | Rating | Notes |
|---|---|---|
| Architecture | 🟢 Excellent | Factory pattern is clean and maintainable |
| Test Coverage | 🟢 Excellent | All affected tests updated systematically |
| Type Safety | 🟡 Needs Work | Return type and private access issues |
| Documentation | 🟡 Needs Work | Missing user-facing docs |
| Security | 🟢 Good | No new security concerns |
| Performance | 🟢 Good | Factory overhead is negligible |
Overall: Strong implementation with a few critical fixes needed before merge.
* Smithery configuration * --amend
Summary
create_server()function@smithery.server()decorator for Smithery compatibilitymcpexport from package APITechnical Details
The core architectural change replaces the global
mcpFastMCP instance with a factory function pattern:mcp = FastMCP(...)at module level@smithery.server() def create_server() -> FastMCPThis enables Smithery to manage server lifecycle while maintaining backward compatibility with the existing CLI interface. The
main()function now callscreate_server().run().Test Plan
uv run pytest tests/unit/uv run pytest tests/integration/uv run pre-commit run --all-filesuv run databeak --transport stdiouv run databeak --transport http --host 0.0.0.0 --port 8000[tool.smithery]section🤖 Generated with Claude Code