Skip to content

fix(mcp): pin mcp<2.0.0 and report fastmcp removal accurately - #355

Closed
erensh27 wants to merge 1 commit into
NVIDIA:mainfrom
erensh27:fix/pin-mcp-under-2
Closed

fix(mcp): pin mcp<2.0.0 and report fastmcp removal accurately#355
erensh27 wants to merge 1 commit into
NVIDIA:mainfrom
erensh27:fix/pin-mcp-under-2

Conversation

@erensh27

@erensh27 erensh27 commented Aug 9, 2026

Copy link
Copy Markdown

Fixes #333.

Two fixes:

  1. pyproject.toml — cap the mcp extra at mcp<2.0.0: mcp 2.0.0 (released 2026-07-28) removed mcp.server.fastmcp, so fresh skillspector[mcp] installs could resolve mcp 2.x and then fail at server init.
  2. src/skillspector/mcp_server.py — on import failure, first check whether mcp itself is importable. If it is, the problem is the missing mcp.server.fastmcp submodule (mcp>=2.0.0), so the error now says so and points at pip install 'skillspector[mcp]' for a compatible pin, instead of the misleading "optional 'mcp' dependency is missing" message.

Reproduction from the issue now yields a truthful error under mcp 2.x, and pip install 'skillspector[mcp]' no longer resolves to mcp 2.x at all.

Comment thread src/skillspector/mcp_server.py Outdated
from mcp.server.fastmcp import FastMCP
except ModuleNotFoundError as exc:
try:
import mcp # noqa: F401

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: this catches every ModuleNotFoundError raised while importing FastMCP, including a missing transitive dependency inside an installed MCP 1.x package. Importing top-level mcp then succeeds and the code falsely reports that MCP 2.x removed FastMCP. Distinguish exc.name == "mcp", exc.name == "mcp.server.fastmcp", and unrelated missing modules, preserving the original error for the last case, and add tests for both incompatible-MCP and transitive-import failures.

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Automated SkillSpector Review]

Requesting changes. The new error branch misdiagnoses missing transitive imports inside installed MCP 1.x as the MCP 2.x FastMCP removal, and the dependency change leaves uv.lock stale (uv lock --check fails). This branch also conflicts with main and substantially overlaps already-approved PR #339, which includes the lock and a focused regression.

mcp 2.0.0 (2026-07-28) removed mcp.server.fastmcp, so an install that
resolved mcp 2.x fails at server init with the misleading 'requires the
optional mcp dependency' error even though mcp is present.

- Distinguish a missing 'mcp' package (missing optional extra) from an
  installed mcp package that lacks 'mcp.server.fastmcp' (incompatible
  mcp>=2.0.0), and re-raise the original error for unrelated missing
  modules such as a missing transitive dependency.
- Add tests covering all three failure modes.
@erensh27
erensh27 force-pushed the fix/pin-mcp-under-2 branch from 47f3abf to 7a055a4 Compare August 11, 2026 09:54
@erensh27

Copy link
Copy Markdown
Author

All review comments addressed:

  • Blocking: the error branch now distinguishes exc.name == "mcp" (missing extra), exc.name == "mcp.server.fastmcp" (installed but incompatible, mcp>=2.0.0), and unrelated missing modules — the original error is preserved for the last case via raise exc.
  • Tests: added coverage for all three failure modes (missing extra, incompatible install, transitive-import failure) in tests/unit/test_mcp_server.py.
  • Conflict/stale lock: rebased on main. main already carries the mcp<2.0.0 pin and matching uv.lock (merged via release snapshots), so this PR no longer touches pyproject.toml/uv.lockuv lock --check passes and the diff is now only the diagnostic fix + tests.

Verified: pytest -m "not integration and not provider" tests/ → 1939 passed; ruff check/format clean. Ready for re-review.

@rng1995

rng1995 commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Thank you for addressing the review feedback, including the improved ModuleNotFoundError.name distinction and the transitive-import regression coverage.

We’re closing this PR to avoid maintaining two competing implementations for the same MCP compatibility work. The supported mcp<2.0.0 bound is already present on main, and #339 remains the selected PR path for the remaining FastMCP compatibility diagnostic and focused test. If that path is updated, the latest distinction implemented here—missing mcp, missing mcp.server.fastmcp, and unrelated transitive import failures—should be preserved.

Thanks again for the quick and careful revision.

@rng1995 rng1995 closed this Aug 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants