From 264c9dd90abb402b0106b21467036ffc99cbe020 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B8rnar=20Snoksrud?= Date: Sat, 12 Sep 2026 12:46:10 +0200 Subject: [PATCH] comments: support both mcp majors in the MCP server mcp 2.x moved mcp.server.fastmcp to mcp.server.mcpserver, renamed FastMCP to MCPServer and Image along with it, and renamed Tool.inputSchema to input_schema. Every one of those is a hard import or attribute error for novem.comments.MCP(), and the extra was pinned to <2 to avoid them. Worse, the failure lied about itself. ModuleNotFoundError is an ImportError, so an installed 2.x was reported as 'The "mcp" extra is required' -- telling the user to install a package they already had. _import_mcp() resolves either spelling, so the pin can open to <3. MCPServer is drop-in for what this module uses: the same positional-name constructor, .tool(), .list_tools() and .run(). The example's call_tool handling also needed widening: v1 returns a list or a (list, dict) tuple, while v2 returns a CallToolResult, which is not subscriptable at all. Nothing installed the extra in CI, which is why this went unnoticed, so the new job runs the compat tests against each major. --- .github/workflows/ci.yaml | 25 ++++++++++++++++ examples/mcp_mention_responder.py | 21 ++++++++++++-- novem/comments.py | 30 ++++++++++++++------ pyproject.toml | 8 +++--- tests/test_mcp_compat.py | 47 +++++++++++++++++++++++++++++++ uv.lock | 2 +- 6 files changed, 117 insertions(+), 16 deletions(-) create mode 100644 tests/test_mcp_compat.py diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 1050f0b..8a3f97d 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -40,3 +40,28 @@ jobs: - name: Test run: | uv run pytest tests + + mcp: + # The mcp extra is not part of `uv sync`, so the build job above never + # imports it. Without this, novem.comments.MCP() can break against either + # major and the suite stays green. + runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + mcp-version: ["mcp>=1,<2", "mcp>=2,<3"] + + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + - name: Set up Python + uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 + with: + python-version: "3.12" + - name: Install uv + uses: astral-sh/setup-uv@c771a70e6277c0a99b617c7a806ffedaca235ff9 # v9.0.0 + - name: Install dependencies + run: | + uv sync + - name: Test against ${{ matrix.mcp-version }} + run: | + uv run --with '${{ matrix.mcp-version }}' pytest tests/test_mcp_compat.py -v diff --git a/examples/mcp_mention_responder.py b/examples/mcp_mention_responder.py index e098b47..67e53ad 100644 --- a/examples/mcp_mention_responder.py +++ b/examples/mcp_mention_responder.py @@ -64,6 +64,17 @@ def _log(msg: Any, text: str) -> None: print(f"[{msg.ts}] {text}", file=sys.stderr) +def _tool_content(result: Any) -> Any: + """The content list from a call_tool result, across mcp v1 and v2. + + v1 returns the list itself, or a (list, dict) tuple when the tool declares + structured output. v2 returns a CallToolResult, which is not subscriptable. + """ + if hasattr(result, "content"): + return result.content + return result[0] if isinstance(result, tuple) else result + + async def _handle(msg: Any) -> None: _log(msg, f"event: {msg.event_type} from @{msg.actor} -> {msg.fqnp}") @@ -107,7 +118,7 @@ def on_reply(text: str) -> str: if msg.actor != my_username: _log(msg, "posting 'On it!' acknowledgement") ack_result = await mcp.call_tool("novem_reply", {"text": "On it!"}) - ack_text = ack_result[0][0].text if isinstance(ack_result, tuple) else ack_result[0].text + ack_text = _tool_content(ack_result)[0].text _log(msg, f" ack result: {ack_text}") else: _log(msg, "actor is us, skipping acknowledgement") @@ -170,14 +181,18 @@ def on_reply(text: str) -> str: _log(msg, f" tool {block.name} error: {e}") results.append({"type": "tool_result", "tool_use_id": block.id, "content": str(e), "is_error": True}) continue - content = result[0] if isinstance(result, tuple) else result + content = _tool_content(result) result_content: List[Any] = [] for item in content: if item.type == "image": result_content.append( { "type": "image", - "source": {"type": "base64", "media_type": item.mimeType, "data": item.data}, + "source": { + "type": "base64", + "media_type": getattr(item, "mime_type", None) or item.mimeType, + "data": item.data, + }, } ) else: diff --git a/novem/comments.py b/novem/comments.py index 7186ee1..46453ea 100644 --- a/novem/comments.py +++ b/novem/comments.py @@ -7,6 +7,7 @@ """ import asyncio +import importlib import json import time from dataclasses import dataclass, field @@ -661,13 +662,25 @@ def _do_reply(self, text: str, title: Optional[str] = None) -> str: # --------------------------------------------------------------------------- -def _check_mcp_deps() -> Any: - try: - from mcp.server.fastmcp import FastMCP # type: ignore[import-untyped,import-not-found] +def _import_mcp(symbol: str) -> Any: + """Resolve an mcp symbol across the v1/v2 module split. + + mcp 2.x moved mcp.server.fastmcp to mcp.server.mcpserver and renamed + FastMCP to MCPServer. Both spellings are tried so an installed 2.x is not + reported as a missing dependency, which is what a bare ImportError here + used to claim -- ModuleNotFoundError is an ImportError. + """ + v2_name = {"FastMCP": "MCPServer"}.get(symbol, symbol) + for module, name in (("mcp.server.fastmcp", symbol), ("mcp.server.mcpserver", v2_name)): + try: + return getattr(importlib.import_module(module), name) + except (ImportError, AttributeError): + continue + raise ImportError('The "mcp" extra is required. Install with: pip install novem[mcp]') + - return FastMCP - except ImportError: - raise ImportError('The "mcp" extra is required. Install with: pip install novem[mcp]') +def _check_mcp_deps() -> Any: + return _import_mcp("FastMCP") def _fmt_comment(c: Comment, indent: int = 0) -> str: @@ -805,7 +818,8 @@ async def api_tools() -> List[Dict[str, Any]]: { "name": t.name, "description": t.description or "", - "input_schema": t.inputSchema, + # mcp 2.x renamed the field to input_schema + "input_schema": getattr(t, "input_schema", None) or t.inputSchema, } for t in mcp_tools ] @@ -884,7 +898,7 @@ def novem_get_vis_screenshot() -> Any: are looking at. Only available when the FQNP points to a visualization (plot, grid, mail, …). """ - from mcp.server.fastmcp import Image # type: ignore[import-untyped,import-not-found] + Image = _import_mcp("Image") if not parsed.is_vis: return "This FQNP does not reference a visualization." diff --git a/pyproject.toml b/pyproject.toml index 43bb704..2ea2a5b 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -28,10 +28,10 @@ compute = [ "aiohttp>=3.9", ] mcp = [ - # novem.comments.MCP() targets the v1 API (mcp.server.fastmcp.FastMCP, - # Tool.inputSchema). mcp 2.0 removed both, so keep the extra on 1.x - # until the migration lands -- see the tracking issue. - "mcp>=1.0.0,<2", + # novem.comments.MCP() resolves both spellings: v1's + # mcp.server.fastmcp.FastMCP/Tool.inputSchema and v2's + # mcp.server.mcpserver.MCPServer/Tool.input_schema. + "mcp>=1.0.0,<3", ] [project.urls] diff --git a/tests/test_mcp_compat.py b/tests/test_mcp_compat.py new file mode 100644 index 0000000..d870dc2 --- /dev/null +++ b/tests/test_mcp_compat.py @@ -0,0 +1,47 @@ +"""The mcp extra is optional, so nothing else in the suite imports it. + +mcp 2.x renamed FastMCP to MCPServer and moved it out of mcp.server.fastmcp, +and renamed Tool.inputSchema to input_schema. These pin the surface +novem.comments.MCP() actually touches, against whichever major is installed. +""" + +import asyncio + +import pytest + +pytest.importorskip("mcp", reason="the mcp extra is not installed") + +from novem.comments import _import_mcp # noqa: E402 + + +def test_server_symbol_resolves_on_either_major(): + assert _import_mcp("FastMCP").__name__ in {"FastMCP", "MCPServer"} + + +def test_image_symbol_resolves_on_either_major(): + assert _import_mcp("Image").__name__ == "Image" + + +def test_missing_symbol_reports_the_extra(): + with pytest.raises(ImportError, match="mcp"): + _import_mcp("NoSuchSymbolAnywhere") + + +def test_server_exposes_the_api_the_helper_uses(): + server = _import_mcp("FastMCP")("novem-comments (probe)") + for attr in ("tool", "list_tools", "run"): + assert hasattr(server, attr), f"server has no {attr}()" + + +def test_tool_input_schema_is_reachable(): + """Mirrors the api_tools() lookup in novem/comments.py.""" + server = _import_mcp("FastMCP")("novem-comments (probe)") + + @server.tool() + def hello(name: str) -> str: + """Say hello.""" + return f"hi {name}" + + tool = asyncio.run(server.list_tools())[0] + assert tool.name == "hello" + assert getattr(tool, "input_schema", None) or tool.inputSchema diff --git a/uv.lock b/uv.lock index 496efeb..c38296b 100644 --- a/uv.lock +++ b/uv.lock @@ -1215,7 +1215,7 @@ dev = [ requires-dist = [ { name = "aiohttp", marker = "extra == 'compute'", specifier = ">=3.9" }, { name = "colorama", specifier = ">=0.4.6" }, - { name = "mcp", marker = "extra == 'mcp'", specifier = ">=1.0.0,<2" }, + { name = "mcp", marker = "extra == 'mcp'", specifier = ">=1.0.0,<3" }, { name = "pyreadline3", marker = "os_name == 'nt'", specifier = ">=3.5.4" }, { name = "python-socketio", extras = ["asyncio-client"], marker = "extra == 'events'", specifier = ">=5.11.0" }, { name = "requests", specifier = ">=2.32.4" },