MCP server instrumentation and repositories interface cleaning #61
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (1)
Tip CodeRabbit can scan for known vulnerabilities in your dependencies using OSV Scanner.OSV Scanner will automatically detect and report security vulnerabilities in your project's dependencies. No additional configuration is required. 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. WalkthroughAdds MCP integration mounted at /mcp with a cached FastMCP builder, refactors application wiring to expose Changes
Sequence Diagram(s)sequenceDiagram
participant Setup as App setup
participant build_mcp as src/mcp/server.build_mcp
participant FastMCP as FastMCP
participant AppState as app.state
participant HTTP as HTTP Server
Note over Setup: During app creation, MCP is built and mounted
Setup->>build_mcp: build_mcp(app)
alt cached instance exists
build_mcp-->>AppState: return app.state.mcp
else first-time creation
build_mcp->>FastMCP: FastMCP.from_fastapi(app, name=app.title)
FastMCP-->>build_mcp: instance
build_mcp->>AppState: store instance in app.state.mcp
end
build_mcp-->>Setup: FastMCP instance
Setup->>HTTP: mount FastMCP at /mcp
Note over HTTP: Request flow
HTTP->>FastMCP: GET /mcp/tools
FastMCP-->>HTTP: list tools (derived from FastAPI routes)
HTTP->>FastMCP: POST /mcp/call
FastMCP->>Setup: dispatch to `rest_app` route handler
Setup-->>FastMCP: handler response
FastMCP-->>HTTP: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested reviewers
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 integrates FastMCP server capabilities into the Infinity API and refactors the MongoDB repository layer to use pymongo's native async client. Key changes include:
- Added FastMCP integration to automatically expose FastAPI routes as MCP tools
- Migrated from
motorlibrary topymongo>=4.15with nativeAsyncMongoClient - Simplified repository singleton pattern by removing reference counting logic
Reviewed Changes
Copilot reviewed 6 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/mcp/server.py |
New module providing FastMCP server builder with caching |
tests/unit/test_mcp/test_mcp_server.py |
Test coverage for MCP server creation and route-to-tool mapping |
src/api.py |
Mounts MCP server at /mcp endpoint and adds app title |
src/repositories/interface.py |
Replaces motor with pymongo AsyncMongoClient, removes RepoInstances class and reference counting, simplifies locking |
requirements.txt |
Removes motor, adds pymongo>=4.15 and fastmcp dependencies |
README.md |
Documents the new MCP server endpoint |
AGENTS.md |
New repository guidelines document for development conventions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/repositories/interface.py (1)
37-61: Initialization must finish before servicing operationsCreating a repository while an event loop is running (typical inside FastAPI handlers or startup hooks) now schedules
_async_init()withloop.create_task(...), but no one awaits it before the first CRUD call. The very firstinsert/find/updateimmediately hitsget_collection(), sees_initialized == False, and raisesRepositoryNotInitializedException, turning the first request into an HTTP 503. Subsequent retries might work only if the background task finishes in time, so this is a deterministic correctness bug. Please gate repository methods on the pending init task (and keep a handle to propagate failures) so users never observe the transient uninitialized state.@@ - async def wrapper(self, *args, **kwargs): + async def wrapper(self, *args, **kwargs): try: + if not getattr(self, "_initialized", False): + init_task = getattr(self, "_init_task", None) + if init_task is not None: + await init_task + else: + await self._async_init() return await method(self, *args, **kwargs) @@ - if not getattr(self, '_initialized', False): + if not getattr(self, '_initialized', False): self.model = model self._max_pool_size = max_pool_size self._initialized_event = asyncio.Event() + self._init_task = None self._initialize() @@ - async with self._init_lock: + async with self._init_lock: if getattr(self, '_initialized', False): return try: self._initialize_connection() except Exception as e: logger.error("Initialization failed: %s", e, exc_info=True) self._initialized = False + self._init_task = None raise self._initialized = True self._initialized_event.set() + self._init_task = None @@ - try: + try: loop = asyncio.get_running_loop() except RuntimeError: asyncio.run(self._async_init()) else: - loop.create_task(self._async_init()) + if getattr(self, "_init_task", None) is None: + self._init_task = loop.create_task(self._async_init())Also applies to: 90-126
🧹 Nitpick comments (1)
tests/unit/test_mcp/test_mcp_server.py (1)
51-52: Consider documenting or making the naming convention more flexible.The tool naming convention
f"{route.name}_{tag}s"is hardcoded and assumes a specific pattern. If route naming conventions change, these tests will break. Consider either:
- Documenting this convention in a comment
- Moving the naming logic to a shared utility that both the MCP server and tests can reference
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
AGENTS.md(1 hunks)README.md(1 hunks)requirements.txt(2 hunks)src/api.py(1 hunks)src/mcp/server.py(1 hunks)src/repositories/interface.py(5 hunks)tests/unit/test_mcp/test_mcp_server.py(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2024-11-15T15:12:21.314Z
Learnt from: GabrielBarberini
PR: RocketPy-Team/Infinity-API#38
File: lib/routes/motor.py:75-75
Timestamp: 2024-11-15T15:12:21.314Z
Learning: When modifying API route paths in `lib/routes/motor.py`, changing from `/rocketpy/{motor_id}` to `/{motor_id}/rocketpy` is acceptable when no external consumers are affected. It's acceptable to introduce this breaking change if the team has decided to adopt the new approach, provided that the `README` and related documentation are updated accordingly.
Applied to files:
requirements.txt
📚 Learning: 2024-12-07T11:50:08.415Z
Learnt from: GabrielBarberini
PR: RocketPy-Team/Infinity-API#41
File: tests/test_routes/test_flights_route.py:122-125
Timestamp: 2024-12-07T11:50:08.415Z
Learning: In the project's pytest test files (e.g., `tests/test_routes/test_flights_route.py`), fixtures like `stub_rocket` and `stub_flight` are function-scoped and can be safely modified within tests without causing side effects.
Applied to files:
tests/unit/test_mcp/test_mcp_server.py
📚 Learning: 2024-06-10T19:08:40.396Z
Learnt from: GabrielBarberini
PR: RocketPy-Team/Infinity-API#20
File: lib/repositories/motor.py:19-27
Timestamp: 2024-06-10T19:08:40.396Z
Learning: The `Repositories` class is a singleton, which makes `MotorRepository` inherit this singleton behavior by transitivity.
Applied to files:
src/repositories/interface.py
🧬 Code graph analysis (3)
src/api.py (1)
src/mcp/server.py (1)
build_mcp(9-20)
tests/unit/test_mcp/test_mcp_server.py (1)
src/mcp/server.py (1)
build_mcp(9-20)
src/repositories/interface.py (1)
src/repositories/motor.py (3)
MotorRepository(9-36)update_motor_by_id(29-32)__init__(17-18)
🪛 Ruff (0.14.2)
src/repositories/interface.py
83-83: Unused static method argument: args
(ARG004)
83-83: Unused static method argument: kwargs
(ARG004)
125-125: Store a reference to the return value of loop.create_task
(RUF006)
⏰ 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). (1)
- GitHub Check: Agent
🔇 Additional comments (3)
tests/unit/test_mcp/test_mcp_server.py (2)
15-21: LGTM: Good test isolation with autouse fixture.The
reset_mcp_statefixture properly ensures test isolation by cleaning up the MCP instance fromapp.statebefore and after each test. The use ofautouse=Trueand defensivehasattrchecks is appropriate.
24-35: LGTM: Well-structured test for MCP server creation and caching.The test correctly validates both the initial creation via
FastMCP.from_fastapiwith the expected parameters and the caching behavior on subsequent calls.AGENTS.md (1)
15-15: Documentation is accurate—79-character line length is correctly configured.The project configuration confirms that both Black and Ruff are set to enforce 79-character line length in
pyproject.toml, and the target version is Python 3.12. The AGENTS.md documentation statement is correct and requires no changes.
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @GabrielBarberini. * #61 (comment) The following files were modified: * `src/mcp/server.py` * `src/repositories/interface.py` * `tests/unit/test_mcp/test_mcp_server.py`
I think its worth this time since I remember to spend a good amount of time to develop the initial abstraction around repositories interface. Anyway this PR is just simplifying everything and giving up the scaffold to enable short living connections with mongo as long living seems to be very stable and initialization has a big overhead. |
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/mcp/server.py(1 hunks)
🔇 Additional comments (1)
src/mcp/server.py (1)
1-7: LGTM! Clean module structure.The imports are minimal and appropriate for the MCP integration helper.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Gabriel Barberini <gabriel.barberini@sumup.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/api.py (1)
106-115: Consider adding type hints for clarity.The
_combine_lifespansfunction would benefit from type hints for its parameters and return type to improve code clarity and IDE support.Apply this diff:
-def _combine_lifespans(rest_lifespan, mcp_lifespan): +from collections.abc import AsyncGenerator +from typing import Callable + +def _combine_lifespans( + rest_lifespan: Callable[[FastAPI], AsyncGenerator], + mcp_lifespan: Callable[[FastAPI], AsyncGenerator] +) -> Callable[[FastAPI], AsyncGenerator]: """Combine FastAPI and MCP lifespans."""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(1 hunks)src/api.py(2 hunks)tests/unit/test_mcp/test_mcp_server.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- README.md
- tests/unit/test_mcp/test_mcp_server.py
🧰 Additional context used
🧬 Code graph analysis (1)
src/api.py (3)
src/__init__.py (1)
parse_error(22-25)src/mcp/server.py (1)
build_mcp(9-21)src/utils.py (1)
RocketPyGZipMiddleware(226-247)
🔇 Additional comments (4)
src/api.py (4)
24-41: LGTM! REST app refactoring is well-structured.The separation of the REST application into
rest_appis clean and consistent. All routers, instrumentation, and middleware are properly configured on the new instance.
44-72: LGTM! OpenAPI configuration updated correctly.All references to
apphave been consistently updated torest_app. The OpenAPI schema generation remains functionally unchanged.
75-99: LGTM! Routes migrated correctly with improved logging.The routes and exception handlers are properly moved to
rest_app. The logging change on line 96 is an improvement—using string formatting instead of f-strings for structured logging and error level for validation errors.
129-129: LGTM! Exports are appropriate.Exporting both
appandrest_appprovides flexibility for consumers who may need direct access to either the combined application or just the REST portion.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
src/mcp/server.py (2)
20-25: Potential race condition in caching pattern.The check-then-set pattern (lines 20-25) could allow multiple FastMCP instances to be created if
build_mcpis called concurrently before the first call completes. While this is likely not an issue if called only during single-threaded startup (as the mounting pattern in src/api.py suggests), consider documenting this constraint.Add a note in the docstring:
def build_mcp(app: FastAPI) -> FastMCP: """ Create or return a cached FastMCP server that mirrors the given FastAPI app. Parameters: app (FastAPI): FastAPI application to mirror; the created FastMCP instance is cached on `app.state.mcp`. Returns: FastMCP: The FastMCP instance corresponding to the provided FastAPI app. + + Note: + This function should be called during single-threaded application startup. + Concurrent calls may result in multiple FastMCP instances being created. """
23-23: Document global settings mutation.Line 23 permanently enables an experimental OpenAPI parser feature globally for the entire fastmcp library. This is a significant side effect that affects all fastmcp behavior in the application and is not obvious from the function signature.
Update the docstring to document this side effect:
def build_mcp(app: FastAPI) -> FastMCP: """ Create or return a cached FastMCP server that mirrors the given FastAPI app. Parameters: app (FastAPI): FastAPI application to mirror; the created FastMCP instance is cached on `app.state.mcp`. Returns: FastMCP: The FastMCP instance corresponding to the provided FastAPI app. + + Side Effects: + Enables `settings.experimental.enable_new_openapi_parser` globally, affecting + all fastmcp behavior in the process. This setting persists for the application lifetime. """tests/unit/test_mcp/test_mcp_server.py (2)
64-64: Avoid using internal FastAPI API.Accessing
route.dependant.path_paramsrelies on FastAPI internal implementation details that may change between versions. Consider usingroute.param_convertors.keys()for a more stable API to extract path parameters.Apply this diff to use the public API:
for tool_name, route in expected.items(): schema = tool_by_name[tool_name].inputSchema or {} required = set(schema.get('required', [])) - path_params = {param.name for param in route.dependant.path_params} + path_params = set(route.param_convertors.keys()) assert path_params.issubset( required ), f"{tool_name} missing path params {path_params - required}"
50-55: Fix potential IndexError when accessing route.tags[0].The code filters routes by
include_in_schemabut doesn't verify thatroute.tagsis non-empty before accessingroute.tags[0]. This will raise anIndexErrorif any route marked for schema inclusion has no tags.Apply this diff to add defensive validation:
expected = {} for route in rest_app.routes: if not isinstance(route, APIRoute) or not route.include_in_schema: continue + if not route.tags: + continue # Skip routes without tags tag = route.tags[0].lower() tool_name = f"{route.name}_{tag}s" expected[tool_name] = routesrc/repositories/interface.py (1)
138-139: Fix race condition in lock initialization.Creating
_init_lockwith a check-then-create pattern outside synchronization allows multiple concurrent calls to_async_init()to create separate lock instances before any can acquire one, defeating the lock's purpose.Move the lock initialization to
__init__to ensure a single lock instance per repository:def __init__(self, model: ApiBaseModel, *, max_pool_size: int = 3): """ Initialize the repository instance for a specific API model and configure its connection pool. Parameters: model (ApiBaseModel): The API model used for validation and to determine the repository's collection. max_pool_size (int, optional): Maximum size of the MongoDB connection pool. Defaults to 3. Notes: If the instance is already initialized, this constructor will not reconfigure it. Initialization of the underlying connection is started asynchronously. """ if not getattr(self, '_initialized', False): self.model = model self._max_pool_size = max_pool_size self._initialized_event = asyncio.Event() + self._init_lock = asyncio.Lock() self._initialize()Then remove the hasattr check in
_async_init:async def _async_init(self): """ Perform idempotent, retry-safe asynchronous initialization of the repository instance. Ensures a per-instance asyncio.Lock exists and acquires it to run initialization exactly once; on success it marks the instance as initialized and sets the internal _initialized_event so awaiters can proceed. If initialization fails, the original exception from _initialize_connection is propagated after logging. """ if getattr(self, '_initialized', False): return - if not hasattr(self, '_init_lock'): - self._init_lock = asyncio.Lock() - async with self._init_lock: if getattr(self, '_initialized', False): return
🧹 Nitpick comments (1)
src/repositories/interface.py (1)
155-166: Consider storing the task reference for observability.The static analysis tool correctly flags that the
create_taskresult is not stored. While the fire-and-forget pattern may be intentional for background initialization, storing the reference would allow tracking initialization failures and provide better observability.Apply this diff to store and optionally track the initialization task:
def _initialize(self): """ Ensure the repository's asynchronous initializer is executed: run it immediately if no event loop is active, otherwise schedule it on the running loop. If there is no running asyncio event loop, this method runs self._async_init() to completion on the current thread, blocking until it finishes. If an event loop is running, it schedules self._async_init() as a background task on that loop and returns immediately. """ try: loop = asyncio.get_running_loop() except RuntimeError: asyncio.run(self._async_init()) else: - loop.create_task(self._async_init()) + self._init_task = loop.create_task(self._async_init())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/mcp/server.py(1 hunks)src/repositories/interface.py(6 hunks)tests/unit/test_mcp/test_mcp_server.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-11-16T00:03:14.224Z
Learnt from: GabrielBarberini
Repo: RocketPy-Team/Infinity-API PR: 38
File: tests/test_routes/test_motors_route.py:42-100
Timestamp: 2024-11-16T00:03:14.224Z
Learning: In `tests/test_routes/test_motors_route.py`, focus on assessing whether the API has the capability of validating the input schema, rather than testing the extension of Pydantic validations (e.g., testing empty request payloads).
Applied to files:
tests/unit/test_mcp/test_mcp_server.py
📚 Learning: 2024-06-10T19:08:40.396Z
Learnt from: GabrielBarberini
Repo: RocketPy-Team/Infinity-API PR: 20
File: lib/repositories/motor.py:19-27
Timestamp: 2024-06-10T19:08:40.396Z
Learning: The `Repositories` class is a singleton, which makes `MotorRepository` inherit this singleton behavior by transitivity.
Applied to files:
src/repositories/interface.py
🧬 Code graph analysis (3)
src/mcp/server.py (1)
src/api.py (1)
custom_openapi(31-56)
tests/unit/test_mcp/test_mcp_server.py (1)
src/mcp/server.py (1)
build_mcp(9-26)
src/repositories/interface.py (2)
src/models/interface.py (1)
ApiBaseModel(10-61)src/secrets.py (2)
Secrets(5-21)get_secret(17-21)
🪛 GitHub Actions: Pylint
src/mcp/server.py
[error] 26-26: pylint: C0304: Final newline missing (missing-final-newline)
tests/unit/test_mcp/test_mcp_server.py
[error] 84-84: pylint: C0304: Final newline missing (missing-final-newline)
src/repositories/interface.py
[error] 287-287: pylint: C0304: Final newline missing (missing-final-newline)
🪛 Ruff (0.14.2)
src/repositories/interface.py
96-96: Unused static method argument: args
(ARG004)
96-96: Unused static method argument: kwargs
(ARG004)
166-166: Store a reference to the return value of loop.create_task
(RUF006)
🔇 Additional comments (6)
src/repositories/interface.py (3)
37-76: LGTM!The enhanced decorator provides comprehensive error handling with clear logging and standardized exception propagation.
96-109: LGTM!The singleton pattern is correctly implemented with thread-safe instantiation. The static analysis warning about unused
argsandkwargsparameters is a false positive—these parameters are required by the__new__signature for proper initialization coordination with__init__.
181-213: LGTM!The migration from AsyncIOMotorClient to pymongo's AsyncMongoClient is correctly implemented with appropriate pool sizing, timeout configuration, and error handling.
tests/unit/test_mcp/test_mcp_server.py (3)
13-24: LGTM!The fixture provides proper test isolation by clearing MCP state before and after each test.
27-37: LGTM!The test correctly verifies both the initial FastMCP creation and the caching behavior on subsequent calls.
70-83: LGTM!The test correctly verifies that REST endpoints remain functional alongside MCP integration.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/api.py (1)
1-106: Add explicit__all__export list for public API clarity.The module exposes
appandrest_appas public entities but lacks an explicit__all__declaration. Adding this improves API clarity and prevents unintended exports.Add this at the end of the file:
__all__ = ["app", "rest_app"]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/api.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/api.py (3)
src/__init__.py (1)
parse_error(22-25)src/mcp/server.py (1)
build_mcp(9-26)src/utils.py (1)
RocketPyGZipMiddleware(226-247)
🔇 Additional comments (5)
src/api.py (5)
1-14: LGTM! Clean import organization.The imports are well-organized and all necessary for the new OpenTelemetry instrumentation and MCP server integration.
17-28: LGTM! Clean REST app initialization.The separation of
rest_appfrom the top-levelappprovides good modularity for mounting the MCP server alongside the REST API.
37-65: LGTM! OpenAPI customization correctly targets rest_app.The custom OpenAPI function properly references
rest_appfor schema generation and caching, consistent with the new app structure.
69-80: LGTM! Route handlers correctly attached to rest_app.Both the main page redirect and health check endpoint are properly configured on
rest_app.
84-92: LGTM! Exception handler properly configured.The validation exception handler is correctly attached to
rest_appand uses appropriate error formatting.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
Refactor