fix(deps): cap mcp[cli] below 2.0 to unbreak fresh installs - #290
Conversation
Upstream mcp 2.0.0 renamed mcp.server.fastmcp.FastMCP to mcp.server.mcpserver.MCPServer with no back-compat shim. The unbounded ">=1.28.1" spec let pip resolve 2.0.0, crashing mcpg at import before the server could even start. Pins to the last compatible 1.x line; uv.lock regenerated (resolves mcp==1.28.1). Also fixes two unrelated, pre-existing bugs surfaced while verifying this on a Windows dev box: - CPython 3.14 privatized asyncio.WindowsSelectorEventLoopPolicy to _WindowsSelectorEventLoopPolicy (confirmed against typeshed's windows_events.pyi). The two Windows-only call sites that pin the selector policy for psycopg now fall back to the private name when the public one is gone, instead of raising AttributeError on 3.14. - Two tests hardcoded POSIX-only assumptions that only fail when actually run on Windows: a subprocess-allowlist test asserted "/usr/bin"-style paths are absolute (os.path.isabs disagrees on Windows), and the http_runtime uvicorn test's fake run() didn't accept the loop kwarg the win32 code path has always passed. Full unit + contract suite green (2867 passed, 3 skipped). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The Windows event loop policy fallback relies on the private
asyncio._WindowsSelectorEventLoopPolicy; consider guarding this withhasattr/try-except and a clear failure path so future asyncio internals changes don’t turn it into an obscureAttributeError. - The Windows-compat test changes now anchor
/usr/binviaabspath, which resolves to a drive-relative path on Windows; consider usingPathwith platform-appropriate absolute paths (e.g.,Path.cwd()-based) to make the intention of the allowlist tests clearer and less tied to Unix-style paths.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The Windows event loop policy fallback relies on the private `asyncio._WindowsSelectorEventLoopPolicy`; consider guarding this with `hasattr`/try-except and a clear failure path so future asyncio internals changes don’t turn it into an obscure `AttributeError`.
- The Windows-compat test changes now anchor `/usr/bin` via `abspath`, which resolves to a drive-relative path on Windows; consider using `Path` with platform-appropriate absolute paths (e.g., `Path.cwd()`-based) to make the intention of the allowlist tests clearer and less tied to Unix-style paths.
## Individual Comments
### Comment 1
<location path="tests/unit/test_http_runtime.py" line_range="825-826" />
<code_context>
captured: dict[str, object] = {}
- def _fake_run(app: object, *, host: str, port: int) -> None:
+ def _fake_run(app: object, *, host: str, port: int, **kwargs: object) -> None:
+ # **kwargs swallows the win32-only ``loop="none"`` kwarg
+ # (see http_runtime.run_http) without coupling this test to
+ # the host platform.
</code_context>
<issue_to_address>
**suggestion (testing):** Add assertions around the `loop="none"` kwarg to ensure the Windows-specific behavior is covered by tests.
Currently the test ignores the win32-only `loop="none"` kwarg rather than asserting on it, so the Windows-specific behavior of `run_http` isn’t actually covered. Please extend this test (or add a dedicated one) that patches `sys.platform` to `'win32'`, calls `run_http`, and asserts that `_fake_run` receives `kwargs["loop"] == "none"`, so the Windows loop policy integration is verified and protected against regressions.
Suggested implementation:
```python
captured: dict[str, object] = {}
def _fake_run(app: object, *, host: str, port: int, **kwargs: object) -> None:
# **kwargs swallows the win32-only ``loop="none"`` kwarg
# (see http_runtime.run_http) without coupling this test to
# the host platform.
captured["app"] = app
captured["host"] = host
captured["port"] = port
captured["kwargs"] = kwargs
def test_run_http_win32_passes_none_loop(monkeypatch) -> None:
"""
Ensure that on win32 platforms, http_runtime.run_http passes loop="none"
through to the underlying run function.
"""
import sys
# Simulate Windows platform
monkeypatch.setattr(sys, "platform", "win32")
# Ensure we start from a clean capture state
captured.clear()
# Patch the underlying run function used by http_runtime.run_http
# so that we can inspect the kwargs it receives.
#
# NOTE: The exact attribute name on http_runtime that needs patching
# may differ (e.g. http_runtime._run, http_runtime._server.run, etc.).
# Adjust the monkeypatch target to match the implementation.
import http_runtime # adjust to the actual import used in this test module
monkeypatch.setattr(http_runtime, "run", _fake_run)
# Call run_http; on win32 this should pass loop="none" to _fake_run.
http_runtime.run_http(app=object(), host="127.0.0.1", port=8000)
assert "kwargs" in captured
assert captured["kwargs"].get("loop") == "none"
```
To fully integrate this change with the existing test suite, you may need to:
1. Adjust the `import http_runtime` inside `test_run_http_win32_passes_none_loop` to match how `http_runtime` is imported elsewhere in `test_http_runtime.py` (e.g. `from fastapi_cli import http_runtime` or similar), or remove the import if `http_runtime` is already imported at module level.
2. Update the `monkeypatch.setattr(http_runtime, "run", _fake_run)` line to target the correct callable that `run_http` delegates to (for example, `http_runtime._run`, `http_runtime._server.run`, or whichever function is actually called inside `run_http`).
3. If the signature of `run_http` differs (e.g. additional parameters or different names), adjust the `http_runtime.run_http(...)` call accordingly so the test matches the real function interface.
4. If `sys` is already imported at the top of the file, you can remove the local `import sys` inside the test and rely on the existing import instead.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| def _fake_run(app: object, *, host: str, port: int, **kwargs: object) -> None: | ||
| # **kwargs swallows the win32-only ``loop="none"`` kwarg |
There was a problem hiding this comment.
suggestion (testing): Add assertions around the loop="none" kwarg to ensure the Windows-specific behavior is covered by tests.
Currently the test ignores the win32-only loop="none" kwarg rather than asserting on it, so the Windows-specific behavior of run_http isn’t actually covered. Please extend this test (or add a dedicated one) that patches sys.platform to 'win32', calls run_http, and asserts that _fake_run receives kwargs["loop"] == "none", so the Windows loop policy integration is verified and protected against regressions.
Suggested implementation:
captured: dict[str, object] = {}
def _fake_run(app: object, *, host: str, port: int, **kwargs: object) -> None:
# **kwargs swallows the win32-only ``loop="none"`` kwarg
# (see http_runtime.run_http) without coupling this test to
# the host platform.
captured["app"] = app
captured["host"] = host
captured["port"] = port
captured["kwargs"] = kwargs
def test_run_http_win32_passes_none_loop(monkeypatch) -> None:
"""
Ensure that on win32 platforms, http_runtime.run_http passes loop="none"
through to the underlying run function.
"""
import sys
# Simulate Windows platform
monkeypatch.setattr(sys, "platform", "win32")
# Ensure we start from a clean capture state
captured.clear()
# Patch the underlying run function used by http_runtime.run_http
# so that we can inspect the kwargs it receives.
#
# NOTE: The exact attribute name on http_runtime that needs patching
# may differ (e.g. http_runtime._run, http_runtime._server.run, etc.).
# Adjust the monkeypatch target to match the implementation.
import http_runtime # adjust to the actual import used in this test module
monkeypatch.setattr(http_runtime, "run", _fake_run)
# Call run_http; on win32 this should pass loop="none" to _fake_run.
http_runtime.run_http(app=object(), host="127.0.0.1", port=8000)
assert "kwargs" in captured
assert captured["kwargs"].get("loop") == "none"To fully integrate this change with the existing test suite, you may need to:
- Adjust the
import http_runtimeinsidetest_run_http_win32_passes_none_loopto match howhttp_runtimeis imported elsewhere intest_http_runtime.py(e.g.from fastapi_cli import http_runtimeor similar), or remove the import ifhttp_runtimeis already imported at module level. - Update the
monkeypatch.setattr(http_runtime, "run", _fake_run)line to target the correct callable thatrun_httpdelegates to (for example,http_runtime._run,http_runtime._server.run, or whichever function is actually called insiderun_http). - If the signature of
run_httpdiffers (e.g. additional parameters or different names), adjust thehttp_runtime.run_http(...)call accordingly so the test matches the real function interface. - If
sysis already imported at the top of the file, you can remove the localimport sysinside the test and rely on the existing import instead.
There was a problem hiding this comment.
Code Review
This pull request caps the mcp[cli] dependency below version 2.0 to prevent import crashes, introduces forward-compatibility for CPython 3.14 on Windows by falling back to the privatized event loop policy, and resolves several Windows-specific test failures. The code review feedback recommends safely retrieving the private _WindowsSelectorEventLoopPolicy attribute using getattr to avoid potential AttributeError exceptions on alternative Python implementations or future releases.
| # CPython 3.14 privatized WindowsSelectorEventLoopPolicy to | ||
| # _WindowsSelectorEventLoopPolicy; fall back to it when the public | ||
| # name is gone so this keeps working on 3.12-3.14. | ||
| _selector_policy_cls = ( | ||
| getattr(asyncio, "WindowsSelectorEventLoopPolicy", None) or asyncio._WindowsSelectorEventLoopPolicy | ||
| ) | ||
| asyncio.set_event_loop_policy(_selector_policy_cls()) |
There was a problem hiding this comment.
Directly accessing the private attribute asyncio._WindowsSelectorEventLoopPolicy can raise an AttributeError at import/evaluation time if the private attribute is renamed, removed, or not present in alternative Python implementations (e.g., PyPy, GraalPy, or future Python versions like 3.15+). Using getattr to safely retrieve the private attribute and checking if it is not None before calling it prevents runtime crashes and is more robust.
| # CPython 3.14 privatized WindowsSelectorEventLoopPolicy to | |
| # _WindowsSelectorEventLoopPolicy; fall back to it when the public | |
| # name is gone so this keeps working on 3.12-3.14. | |
| _selector_policy_cls = ( | |
| getattr(asyncio, "WindowsSelectorEventLoopPolicy", None) or asyncio._WindowsSelectorEventLoopPolicy | |
| ) | |
| asyncio.set_event_loop_policy(_selector_policy_cls()) | |
| # CPython 3.14 privatized WindowsSelectorEventLoopPolicy to | |
| # _WindowsSelectorEventLoopPolicy; fall back to it when the public | |
| # name is gone so this keeps working on 3.12-3.14. | |
| _selector_policy_cls = ( | |
| getattr(asyncio, "WindowsSelectorEventLoopPolicy", None) | |
| or getattr(asyncio, "_WindowsSelectorEventLoopPolicy", None) | |
| ) | |
| if _selector_policy_cls is not None: | |
| asyncio.set_event_loop_policy(_selector_policy_cls()) |
| # CPython 3.14 privatized WindowsSelectorEventLoopPolicy to | ||
| # _WindowsSelectorEventLoopPolicy; fall back to it when the public | ||
| # name is gone so this keeps working on 3.12-3.14. | ||
| selector_policy_cls = ( | ||
| getattr(asyncio, "WindowsSelectorEventLoopPolicy", None) or asyncio._WindowsSelectorEventLoopPolicy | ||
| ) | ||
| asyncio.set_event_loop_policy(selector_policy_cls()) |
There was a problem hiding this comment.
Directly accessing the private attribute asyncio._WindowsSelectorEventLoopPolicy can raise an AttributeError at runtime if the private attribute is renamed, removed, or not present in alternative Python implementations (e.g., PyPy, GraalPy, or future Python versions like 3.15+). Using getattr to safely retrieve the private attribute and checking if it is not None before calling it prevents runtime crashes and is more robust.
| # CPython 3.14 privatized WindowsSelectorEventLoopPolicy to | |
| # _WindowsSelectorEventLoopPolicy; fall back to it when the public | |
| # name is gone so this keeps working on 3.12-3.14. | |
| selector_policy_cls = ( | |
| getattr(asyncio, "WindowsSelectorEventLoopPolicy", None) or asyncio._WindowsSelectorEventLoopPolicy | |
| ) | |
| asyncio.set_event_loop_policy(selector_policy_cls()) | |
| # CPython 3.14 privatized WindowsSelectorEventLoopPolicy to | |
| # _WindowsSelectorEventLoopPolicy; fall back to it when the public | |
| # name is gone so this keeps working on 3.12-3.14. | |
| selector_policy_cls = ( | |
| getattr(asyncio, "WindowsSelectorEventLoopPolicy", None) | |
| or getattr(asyncio, "_WindowsSelectorEventLoopPolicy", None) | |
| ) | |
| if selector_policy_cls is not None: | |
| asyncio.set_event_loop_policy(selector_policy_cls()) |
…iew feedback) Sourcery and gemini-code-assist both flagged the private _WindowsSelectorEventLoopPolicy fallback on PR #290 as needing a safer lookup. Turns out the pre-existing (untouched by this PR) test_run_http_pins_selector_loop_on_windows already documented why: CPython 3.14's own deprecation shim can raise NameError reading the removed public name, which a bare getattr(asyncio, name, default) does not catch (only AttributeError). Switched both call sites to try/except (AttributeError, NameError) and added a test that simulates the exact shim to prove the fallback still resolves the private name. Also adds a clear RuntimeError if neither name exists, per review feedback, instead of letting a future rename surface as an opaque crash. Full unit + contract suite green (2868 passed, 3 skipped). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Summary
mcpSDK just published2.0.0, which renamesmcp.server.fastmcp.FastMCPtomcp.server.mcpserver.MCPServerwith no back-compat shim. MCPg's dependency spec (mcp[cli]>=1.28.1) had no upper bound, so a freshpip install mcpgtoday resolvesmcp==2.0.0and crashes at import (ModuleNotFoundError: No module named 'mcp.server.fastmcp') before the server can even start.>=1.28.1,<2and regeneratesuv.lock(resolvesmcp==1.28.1). Migrating MCPg itself onto the newMCPServerAPI is tracked as separate follow-up work — this PR is the hotfix that unbreaks installs in the meantime.mypy/test run (both reproduce identically on unmodifiedmain, confirmed viagit stash):asyncio.WindowsSelectorEventLoopPolicyto_WindowsSelectorEventLoopPolicy(confirmed against typeshed'swindows_events.pyi). The two Windows-only call sites that pin the selector policy for psycopg (__main__.py,http_runtime.run_http) now fall back to the private name when the public one is gone./usr/bin-style paths are absolute (os.path.isabsdisagrees on Windows), and thehttp_runtimeuvicorn test's fakerun()didn't accept theloopkwarg the win32 code path has always passed.Roadmap linkage
Advances roadmap row: N/A — dependency hotfix + pre-existing Windows compatibility fixes, no feature-shortlist row.
Test plan
uv run mypy src/mcpg— clean (0 errors, down from 2 pre-existing)uv run pytest tests/unit tests/contract -q— 2867 passed, 3 skipped, 0 faileduv run ruff check/uv run ruff format --check— cleanmcpg --version/mcpg --helpmanually verified against the regenerated lockfile🤖 Generated with Claude Code
Summary by Sourcery
Cap the MCP CLI dependency below 2.0 and add Windows compatibility fixes to keep installs and runtime working across supported Python versions.
Bug Fixes:
Tests: