feat(mcp-hmr): support MCP Python SDK 2.x - #128
Open
CNSeniorious000 wants to merge 14 commits into
Open
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- When
fastmcpis not installed on mcp 1.x,proxy_backend()will raise anImportError; consider handling this explicitly (with a clear error message or fallback) so users see a friendlier failure mode than a hard import error. - The use of
print()to stderr in_mcpserver_kwargsis a bit surprising for a library; consider switching to the standard logging facilities or surfacing this warning through a more structured mechanism.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- When `fastmcp` is not installed on mcp 1.x, `proxy_backend()` will raise an `ImportError`; consider handling this explicitly (with a clear error message or fallback) so users see a friendlier failure mode than a hard import error.
- The use of `print()` to stderr in `_mcpserver_kwargs` is a bit surprising for a library; consider switching to the standard logging facilities or surfacing this warning through a more structured mechanism.
## Individual Comments
### Comment 1
<location path="packages/mcp-hmr/mcp_hmr.py" line_range="193-196" />
<code_context>
lock = Lock()
- async def using(app: FastMCP | mcp.server.FastMCP, stop_event: Event, finish_event: Event):
+ async def using(app, stop_event: Event, finish_event: Event):
async with lock:
with mount(app):
- for session in active_sessions:
- tg.create_task(_notify_list_changed(session))
+ tg.create_task(notify())
await stop_event.wait()
finish_event.set()
</code_context>
<issue_to_address>
**issue (bug_risk):** TaskGroup variable `tg` is used out of scope in `using`, which will raise at runtime.
`tg.create_task(notify())` references `tg` that is only defined in the outer scope (e.g., inside `run_with_patched_session`), so `using` will raise a `NameError` at runtime and notifications will never be scheduled. Please either pass `tg` into `using`, create a new `TaskGroup` inside `using`, or move the notify scheduling into the scope where `tg` is defined.
</issue_to_address>
### Comment 2
<location path="packages/mcp-hmr/mcp_hmr.py" line_range="187" />
<code_context>
+ from reactivity.hmr.core import HMR_CONTEXT, AsyncReloader, _loader
+ from reactivity.hmr.hooks import call_post_reload_hooks, call_pre_reload_hooks
+
+ base_app: Any = ... # base_app / mount / notify are picked from the target itself on first load
+ mount: Any = ...
+ notify: Any = ...
</code_context>
<issue_to_address>
**issue (complexity):** Consider encapsulating the backend state and MCPServer-specific runner logic into small helper abstractions to avoid sentinel variables and reduce cognitive load in `mcp_server` and `run_with_hmr`.
You can reduce most of the added complexity without changing behavior by introducing a tiny `Backend` container and a small runner helper. This keeps your existing `proxy_backend` / `swap_backend` logic but removes the sentinel `...`/`nonlocal` juggling and centralizes backend behavior.
### 1. Replace `base_app/mount/notify` sentinels with a `Backend` object
Right now:
```python
base_app: Any = ... # base_app / mount / notify are picked from the target itself on first load!
mount: Any = ...
notify: Any = ...
...
@async_effect(context=HMR_CONTEXT, call_immediately=False)
async def main():
nonlocal stop_event, finish_event, base_app, mount, notify
...
app = get_app()
if base_app is ...:
base_app, mount, notify = pick_backend(app)
tg.create_task(using(app, stop_event := Event(), finish_event := Event()))
```
You can encapsulate these into a single object and avoid `Any = ...` and multi-variable `nonlocal`:
```python
from dataclasses import dataclass
from typing import Awaitable, Callable, ContextManager
@dataclass
class Backend:
base_app: Any
mount: Callable[[Any], ContextManager[None]]
notify: Callable[[], Awaitable[None]]
```
Change your backend factories to return a `Backend` instead of a triple:
```python
def proxy_backend() -> Backend:
...
return Backend(base_app=base_app, mount=mount, notify=notify)
def swap_backend() -> Backend:
...
return Backend(base_app=base_app, mount=mount, notify=notify)
def pick_backend(app) -> Backend:
...
return swap_backend() if isinstance(app, MCPServer) else proxy_backend()
```
Then `mcp_server` can hold a single `backend` (or `None`) and a single nonlocal:
```python
def mcp_server(target: str):
...
backend: Backend | None = None
lock = Lock()
async def using(app, stop_event: Event, finish_event: Event):
async with lock:
assert backend is not None
with backend.mount(app):
tg.create_task(backend.notify())
await stop_event.wait()
finish_event.set()
...
@async_effect(context=HMR_CONTEXT, call_immediately=False)
async def main():
nonlocal stop_event, finish_event, backend
if stop_event is not None:
stop_event.set()
await finish_event.wait()
app = get_app()
if backend is None:
backend = pick_backend(app)
tg.create_task(using(app, stop_event := Event(), finish_event := Event()))
...
@asynccontextmanager
async def _():
nonlocal tg
async with TaskGroup() as tg, Reloader():
assert backend is not None
yield backend.base_app
return _()
```
This keeps all the existing behavior (including how `mount` and `notify` differ between backends) but:
- removes the `Any = ...` sentinel pattern,
- collapses three separate pieces of shared state into one,
- makes `mcp_server` only aware of a single `Backend` object.
### 2. Extract MCPServer-specific running into a helper
`run_with_hmr` currently mixes MCPServer-specific and FastMCP-specific logic in one large function. You can keep the semantics while delegating the MCPServer branch into a small helper:
```python
def _run_mcpserver(mcp, transport: str, kwargs: dict):
if transport == "stdio":
return mcp.run_stdio_async()
if transport == "sse":
return mcp.run_sse_async(**_mcpserver_kwargs(mcp.run_sse_async, kwargs, "sse_path"))
return mcp.run_streamable_http_async(
**_mcpserver_kwargs(mcp.run_streamable_http_async, kwargs, "streamable_http_path")
)
```
Then `run_with_hmr` becomes:
```python
async def run_with_hmr(target: str, log_level: str | None = None, transport="stdio", **kwargs):
async with mcp_server(target) as mcp:
if not hasattr(mcp, "run_async"): # MCPServer
return await _run_mcpserver(mcp, transport, kwargs)
kwargs |= {"log_level": log_level}
match transport:
case "stdio":
await mcp.run_stdio_async(**_supported(mcp.run_stdio_async, kwargs | {"show_banner": False}))
case "http" | "streamable-http":
await mcp.run_http_async(**_supported(mcp.run_http_async, kwargs))
case "sse":
if hasattr(mcp, "run_sse_async"):
await mcp.run_sse_async(**_supported(mcp.run_sse_async, kwargs))
else:
await mcp.run_http_async(transport="sse", **_supported(mcp.run_http_async, kwargs))
case _:
await mcp.run_async(transport, **_supported(mcp.run_async, kwargs))
```
This keeps all compatibility logic but reduces the cognitive load in your primary entrypoint: `run_with_hmr` no longer has to spell out every MCPServer detail inline.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
packages/mcp-hmr/mcp_hmr.py (3)
79-89: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
providers[-1]assumesmountappends exactly one provider.The code takes the last element as the provider it just mounted. The
lockinmcp_serverserializes mounts, so this holds today. It breaks quietly if a futuremountappends more than one entry, or if another caller mounts onbase_app. Diffing the list before and after is a cheap way to stay exact.♻️ Proposed change
`@contextmanager` def mount(app): + before = len(base_app.providers) # type: ignore base_app.mount(create_proxy(ProxyClient(app))) # type: ignore - provider = base_app.providers[-1] # type: ignore + added = base_app.providers[before:] # type: ignore try: yield finally: # unmount - base_app.providers.remove(provider) # type: ignore + for provider in added: + base_app.providers.remove(provider) # type: ignore🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/mcp-hmr/mcp_hmr.py` around lines 79 - 89, Update the mount context manager to snapshot base_app.providers before create_proxy(...).mount, then identify the newly added provider(s) by diffing the post-mount list against that snapshot; remove exactly the provider entry or entries introduced by this mount during cleanup instead of assuming providers[-1].
280-289: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBackend detection by
hasattrre-derives whatpick_backendalready knew.
pick_backenddecides betweenswap_backendandproxy_backend, then that decision is thrown away and rebuilt here fromhasattr(mcp, "run_async"). The two can disagree ifMCPServerever grows arun_async, and the failure would be a confusing kwargs mismatch rather than a clear error. Exporting the backend kind alongsidebase_appwould keep one source of truth.Non-blocking; the current check works against today's SDKs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/mcp-hmr/mcp_hmr.py` around lines 280 - 289, Update the backend-selection flow around pick_backend and run_with_hmr so pick_backend returns the selected backend kind alongside base_app, then branch in run_with_hmr using that value instead of hasattr(mcp, "run_async"). Preserve the existing runner and transport-specific behavior while keeping backend detection as the single source of truth.
44-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a clearer error when
fastmcpis absent.
fastmcpis not a declared dependency (seepackages/mcp-hmr/pyproject.toml). Onmcp1.x targets, or a non-MCPServertarget on 2.x, line 50 raises a bareModuleNotFoundError: No module named 'fastmcp'. The README explains the requirement, but the CLI user sees only a traceback. Wrap the import and raise a message that names the fix.♻️ Proposed message
- from fastmcp import FastMCP + try: + from fastmcp import FastMCP + except ImportError as e: # fastmcp is not a hard dependency + raise ImportError("mcp-hmr fronts this target with a FastMCP proxy. Run `pip install fastmcp`.") from e from mcp.server.session import ServerSession🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/mcp-hmr/mcp_hmr.py` around lines 44 - 66, Update proxy_backend to catch a missing fastmcp import before its version-specific imports and raise a clear user-facing error that states fastmcp is required for proxying and instructs the user to install it. Preserve the existing fastmcp 3 versus legacy import fallback behavior once the dependency is available.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/mcp-hmr/mcp_hmr.py`:
- Around line 261-268: The _supported function must preserve extra options for
runners that accept arbitrary keyword arguments. Inspect the runner signature
for a VAR_KEYWORD parameter, retain all non-None kwargs when present, and
otherwise continue filtering to declared parameter names so named options remain
supported.
- Around line 220-231: Update the initialization flow around get_app() in the
enclosing reloader callback to handle first-load backend failures explicitly:
provide the existing proxy_backend() fallback, or re-raise the original
exception when get_app() fails, so base_app is never left as Ellipsis before
run_with_hmr invokes the server.
In `@packages/mcp-hmr/README.md`:
- Line 16: Update the README wording to state that FastMCP proxying, and
therefore the requirement for importable fastmcp, applies to all supported mcp
generations whenever the target is not an mcp.server.MCPServer, including
FastMCP targets on mcp 2.x. Also capitalize “Python” in the “official Python
SDK” wording.
---
Nitpick comments:
In `@packages/mcp-hmr/mcp_hmr.py`:
- Around line 79-89: Update the mount context manager to snapshot
base_app.providers before create_proxy(...).mount, then identify the newly added
provider(s) by diffing the post-mount list against that snapshot; remove exactly
the provider entry or entries introduced by this mount during cleanup instead of
assuming providers[-1].
- Around line 280-289: Update the backend-selection flow around pick_backend and
run_with_hmr so pick_backend returns the selected backend kind alongside
base_app, then branch in run_with_hmr using that value instead of hasattr(mcp,
"run_async"). Preserve the existing runner and transport-specific behavior while
keeping backend detection as the single source of truth.
- Around line 44-66: Update proxy_backend to catch a missing fastmcp import
before its version-specific imports and raise a clear user-facing error that
states fastmcp is required for proxying and instructs the user to install it.
Preserve the existing fastmcp 3 versus legacy import fallback behavior once the
dependency is available.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b25fb14f-3966-47d1-a35d-5be4c1dda43f
📒 Files selected for processing (3)
packages/mcp-hmr/README.mdpackages/mcp-hmr/mcp_hmr.pypackages/mcp-hmr/pyproject.toml
💤 Files with no reviewable changes (1)
- packages/mcp-hmr/pyproject.toml
CNSeniorious000
force-pushed
the
mcp-v2-clean
branch
2 times, most recently
from
August 2, 2026 08:06
cfa34c8 to
0dbdfad
Compare
`mcp` 1.x and 2.x are the same distribution and cannot coexist, so the backend is now picked from the target instance at first load instead of from whatever `mcp` is installed: - `MCPServer` targets swap their tool/resource/prompt registries into a stable outer server, since that SDK has neither mount nor proxy, and publish list-changed events on a subscription bus we own. - everything else keeps the FastMCP proxy path, including a `FastMCP` running on top of `mcp` 2.x. Runner kwargs are filtered by signature on every path, as the signatures drift across both SDK generations. The `fastmcp` bound moves from `<3` to `<4`: 2.2.0 pins `mcp<2.0.0`, so `<3` kept 2.x out of reach.
CNSeniorious000
force-pushed
the
mcp-v2-clean
branch
from
August 2, 2026 08:10
0dbdfad to
a410544
Compare
CNSeniorious000
force-pushed
the
mcp-v2-clean
branch
from
August 2, 2026 10:36
e936856 to
eca4e56
Compare
…xts do not share one
CNSeniorious000
force-pushed
the
mcp-v2-clean
branch
from
August 2, 2026 15:27
3cfdae2 to
cbf614b
Compare
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
1 participant
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.
mcp1.x and 2.x are the same distribution and cannot be installed side by side, somcp-hmrnow adapts at runtime to whichever generation is present.What changed
The backend is picked from the target instance at first load, not from whatever
mcphappens to be importable — aFastMCPstill exists onmcp2.x (that is whatfastmcp4 runs on) and must keep the proxy path.MCPServer(mcp2.x)ServerSessionnotificationsMCPServergets a swap rather than a proxy because that SDK has neither mount nor proxy. It talks Python, not MCP, so only the tool/resource/prompt registries are swapped — extensions, custom routes and middleware added on reload won't take effect, and HTTP transport gets no CORS since its runners take no middleware. All documented in the README.Runner kwargs are now filtered by signature on every path, not just the
MCPServerone. The signatures drift a lot:log_levelonly reachedFastMCP.run_stdio_asyncinfastmcp2.13, andMCPServer's runners take a far narrower set and name the route path per transport.fastmcpis dropped as a hard dependency — it is only needed for the proxy path, and it pinsmcp<2, which would have made 2.x unreachable.Verified
End-to-end through the CLI with a real client, editing the server module mid-connection and asserting the tool list updates:
fastmcpmcpfastmcp.FastMCP,mcp.server.fastmcp.FastMCPfastmcp.FastMCP,mcp.server.fastmcp.FastMCPfastmcp.FastMCP,mcp.server.fastmcp.FastMCPMCPServerfastmcp.FastMCPmcp_use.MCPServerNote
The one failure reproduces with plain
fastmcpand nomcp-hmrin the picture —4.0.0b1rejects the2026-07-28envelope on its own proxy connections. Upstream beta bug; worth re-checking at GA.Supersedes #127.