Refactor codebase and configuration capabilities#9
Conversation
|
Warning Rate limit exceeded@Agent-Hellboy has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 38 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughA middleware configuration system was introduced for the MCP server, including a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Uvicorn
participant run_server.py
participant config.py
participant pymcp.applications
participant pymcp.middleware
participant FastAPIApp
User->>Uvicorn: Start server
Uvicorn->>run_server.py: Execute main block
run_server.py->>config.py: Import middleware_config
run_server.py->>pymcp.applications: Call create_app(middleware_config)
pymcp.applications->>pymcp.middleware: setup_middleware(app, middleware_config)
pymcp.middleware->>FastAPIApp: Add middleware (CORS, Logging, etc.)
pymcp.applications->>run_server.py: Return configured app
run_server.py->>Uvicorn: Run app
Assessment against linked issues
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #9 +/- ##
===========================================
+ Coverage 74.80% 88.11% +13.30%
===========================================
Files 2 5 +3
Lines 127 143 +16
===========================================
+ Hits 95 126 +31
+ Misses 32 17 -15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
pymcp/server.py (1)
74-108: Consider refactoring to reduce code duplication.The tools/call handling logic is duplicated between this file and
pymcp/utils.py. Consider creating a shared function to handle tool execution.Create a shared function in utils.py:
async def execute_tool(tool_name, args, rpc_id): """Execute a tool and return the JSON-RPC response.""" tools = tool_registry.get_tools() if tool_name not in tools: return { "jsonrpc": "2.0", "id": rpc_id, "error": {"code": -32601, "message": f"No such tool '{tool_name}'"}, } try: result_text = tools[tool_name]["function"](**args) return { "jsonrpc": "2.0", "id": rpc_id, "result": {"content": [{"type": "text", "text": result_text}]}, } except Exception as e: logging.error(f"Error executing tool '{tool_name}': {str(e)}", exc_info=True) return { "jsonrpc": "2.0", "id": rpc_id, "error": { "code": -32603, "message": f"Error executing tool '{tool_name}': {str(e)}", }, }Then use it in both places to avoid duplication.
🧹 Nitpick comments (7)
pymcp/middleware.py (1)
55-55: Complete the error handling implementation.The comment suggests error handling should be implemented but provides no implementation.
Would you like me to generate a basic error handling middleware implementation or create an issue to track this task?
tests/test_server.py (1)
14-14: Fix import order issue.The static analysis tool flagged that module-level imports should be at the top of the file. The import on line 14 should be moved up with other imports.
Move the import to the top of the file:
import os import sys import pytest import requests +from pymcp.applications import create_app sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), ".."))) import time from threading import Thread import uvicorn from pymcp.registry import tool_registry -from pymcp.applications import create_apppymcp/applications.py (1)
13-24: Well-designed application factory with excellent separation of concerns.The factory function correctly implements the application creation pattern and provides good flexibility through optional configuration or kwargs.
Consider adding return type annotation for better type safety:
-def create_app(middleware_config: Optional[MiddlewareConfig] = None, **kwargs): +def create_app(middleware_config: Optional[MiddlewareConfig] = None, **kwargs) -> FastAPI:example/web-based/run_mcp_with_flask_call.py (2)
11-11: Consider making the Flask API URL configurable.The hardcoded URL could be extracted to a configuration variable or environment variable for better flexibility.
+FLASK_API_URL = os.environ.get("FLASK_API_URL", "http://127.0.0.1:5005") + @tool_registry.register def callFlaskHelloTool() -> str: """Calls the /hello endpoint of the Flask API server running on port 5005 and returns the message.""" try: - resp = requests.get("http://127.0.0.1:5005/hello", timeout=2) + resp = requests.get(f"{FLASK_API_URL}/hello", timeout=2)Don't forget to import
osat the top of the file.
25-25: Security consideration: Server is exposed on all interfaces.Running on
0.0.0.0exposes the server to all network interfaces. Consider using127.0.0.1for local development or make it configurable.- uvicorn.run(create_app(middleware_config=None), host="0.0.0.0", port=8088) + uvicorn.run(create_app(middleware_config=None), host="127.0.0.1", port=8088)pymcp/utils.py (2)
52-69: Enhance error logging for better debugging.When tool execution fails, logging the full exception traceback would help with debugging.
except Exception as e: + logging.error(f"Error executing tool '{tool_name}': {str(e)}", exc_info=True) error = { "jsonrpc": "2.0", "id": rpc_id, "error": { "code": -32603, "message": f"Error executing tool '{tool_name}': {str(e)}", }, }
10-14: Consider adding session validation.The function assumes the session exists but doesn't validate it. While the caller should ensure this, defensive programming would add a check.
async def handle_rpc_method(method, data, session_id, rpc_id, sessions): """ Handle JSON-RPC methods for the MCP server. """ + if session_id not in sessions: + logging.error(f"Session {session_id} not found") + return queue = sessions[session_id]["queue"]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
README.md(1 hunks)example/config.py(1 hunks)example/run_server.py(2 hunks)example/web-based/run_mcp_with_flask_call.py(1 hunks)guide.md(1 hunks)pymcp/applications.py(1 hunks)pymcp/middleware.py(1 hunks)pymcp/server.py(2 hunks)pymcp/utils.py(1 hunks)tests/test_server.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (7)
example/config.py (1)
pymcp/middleware.py (1)
MiddlewareConfig(11-32)
example/run_server.py (1)
pymcp/applications.py (1)
create_app(13-24)
pymcp/utils.py (1)
pymcp/registry.py (1)
get_tools(34-35)
tests/test_server.py (1)
pymcp/applications.py (1)
create_app(13-24)
pymcp/applications.py (1)
pymcp/middleware.py (2)
MiddlewareConfig(11-32)setup_middleware(35-55)
example/web-based/run_mcp_with_flask_call.py (2)
pymcp/applications.py (1)
create_app(13-24)pymcp/registry.py (1)
register(8-32)
pymcp/server.py (1)
pymcp/utils.py (1)
handle_rpc_method(10-83)
🪛 Flake8 (7.2.0)
tests/test_server.py
[error] 14-14: module level import not at top of file
(E402)
pymcp/server.py
[error] 16-16: expected 2 blank lines, found 1
(E302)
[error] 21-21: expected 2 blank lines, found 1
(E302)
[error] 25-25: expected 2 blank lines, found 1
(E302)
[error] 49-49: expected 2 blank lines, found 1
(E302)
🔇 Additional comments (3)
example/config.py (1)
1-20: Well-structured middleware configuration.The configuration demonstrates good security practices by restricting CORS to specific origins and provides a clear example of how to use the middleware system. The structure is clean and well-documented.
tests/test_server.py (1)
24-24: Good adoption of the new application factory pattern.The change to use
create_app(middleware_config=None)correctly adopts the new application factory pattern and maintains test functionality.example/run_server.py (1)
44-44: Excellent adoption of the new application factory pattern.The use of
create_app(middleware_config=middleware_config)correctly implements the new architecture and provides clean separation of concerns.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pymcp/middleware.py (1)
24-29: Acknowledge past security discussion.I see the CORS security concern was previously discussed and you mentioned exploring environment-aware configuration later. The current permissive defaults remain a security consideration for production use.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
README.md(1 hunks)example/config.py(1 hunks)example/run_server.py(2 hunks)example/web-based/run_mcp_with_api_call.py(2 hunks)guide.md(1 hunks)pymcp/applications.py(1 hunks)pymcp/middleware.py(1 hunks)pymcp/server.py(3 hunks)pymcp/utils.py(1 hunks)tests/test_server.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- example/config.py
- example/run_server.py
- pymcp/applications.py
- README.md
- pymcp/utils.py
- guide.md
- pymcp/server.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/test_server.py (1)
pymcp/applications.py (1)
create_app(14-25)
🪛 Ruff (0.11.9)
pymcp/middleware.py
57-57: Using hasattr(x, "__call__") to test if x is callable is unreliable. Use callable(x) for consistent results.
Replace with callable()
(B004)
🪛 Flake8 (7.2.0)
tests/test_server.py
[error] 14-14: module level import not at top of file
(E402)
🔇 Additional comments (5)
pymcp/middleware.py (1)
39-60: LGTM - Clean middleware setup implementation.The setup function properly applies middleware configurations to the FastAPI app. The CORS configuration is applied correctly, compression is conditionally enabled, and custom middleware validation (once fixed) provides good error handling.
example/web-based/run_mcp_with_api_call.py (2)
3-3: LGTM - Correct import for new factory pattern.The import change from
pymcp.servertopymcp.applications.create_appaligns with the new application factory pattern introduced in the refactor.
25-25: LGTM - Proper app instantiation using factory.The uvicorn.run call correctly uses
create_app(middleware_config=None)to create the app instance dynamically, which is consistent with the new architecture.tests/test_server.py (2)
14-14: Import placement is intentional and correct.The import is placed after the
sys.pathmanipulation, which is the correct pattern for adding local modules to the path before importing them. The static analysis flag is a false positive in this context.
24-24: LGTM - Test server correctly uses new factory pattern.The server thread properly uses
create_app(middleware_config=None)to create the app instance, ensuring tests use the same application factory pattern as the main application.
2 participants
fixes #8
Summary by CodeRabbit
New Features
Documentation
Refactor
Tests