A future cleanup can't silently re-break the MCP server (WOR-868) - #474
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a Python 3.10-compatible regression test that parses the raw ChangesMCP dependency cap
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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.
Actionable comments posted: 1
🤖 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 `@tests/test_mcp_dependency_cap.py`:
- Around line 32-38: Update the MCP requirement assertion in the dependency-cap
test to require an upper bound below version 2, rather than merely checking for
any “<” comparator. Add a regression fixture using mcp>=1.0,<3 and verify it
fails, while preserving acceptance of valid pre-2 caps.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: adc28029-9e40-4a15-a533-a67ca8fb06e7
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
pyproject.tomltests/test_mcp_dependency_cap.py
|
#473 is now merged ( What changes for you: the What survives, and why it matters: Two things worth knowing before you rebase: 1. 2. There is now a gate on ignore expiry. Grype silently drops the unknown Also still open from CodeRabbit here: the assertion at I deliberately did not push to this branch; it is checked out in an active worktree and that is yours to drive. |
7df39f5 to
f1320b6
Compare
CodeRabbit caught this on review and it was agreed but never applied: the guard asserted `"<" in mcp_spec`, which passes for `mcp>=1.0,<99`. A test whose whole job is to catch the cap being dropped would have sailed through exactly the regression it exists to prevent. Now the upper bound is parsed and compared as a version, so only a real cap at or below 2 passes. Inclusive bounds are handled too — `<=2` admits 2.0.0 itself, which is the broken release. Proven, not asserted. Against the real pyproject.toml: mcp>=1.0,<99 -> FAILS (the old assertion passed this) mcp>=1.0,<3 -> FAILS (the old assertion passed this) mcp>=1.0 -> FAILS Parametrised cases pin both directions: <2, <2.0.0, < 2 and <=1.9 accepted; no-cap, <99, <3, <2.1 and <=2 rejected. The bound checker is split out so those can be exercised without rewriting pyproject.toml. Uses packaging.version.Version so 2.0.0 compares equal to 2 — matching existing precedent in tests/test_install_static.py. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
f1320b6 to
e6205f3
Compare
|



TL;DR
The
mcp<2cap already landed on main via WOR-864. This adds the guard so it can't be dropped again.Features
mcpcapBefore / After
BEFORE — remove the upper bound and nothing complains until users hit it:
AFTER:
Summary
I hit this bug independently while diagnosing an unrelated red check, filed it as WOR-868, and fixed it — then found WOR-864 had landed the identical cap on
mainfirst. The cap is not in this PR anymore; I dropped it during rebase, andpyproject.tomlhere is byte-identical tomain. What remains is the one piecemaindoes not have: a test that fails if the cap is ever removed.Setup
This is worth guarding rather than trusting to memory, because the failure mode is invisible in normal development.
uv.lockpins the dev and CI environment tomcp1.x, so every local run and every CI job passes. Butuvx worthless[mcp]— whatnpx worthless-mcpactually runs — resolves fresh frompyproject.tomland ignores the lockfile. The lockfile hides a broken constraint.The only job that exercises the real wrapper is path-gated on MCP/npm files, so it doesn't run on ordinary pushes to
main. That combination is how a brokenworthless mcpreached PyPI in 0.3.10 and 0.3.11: nothing in the normal path could see it.What
mcpcap without porting to the 2.x API first, a test fails immediately and names the consequence.Why
The cap is the kind of line that looks like stale conservatism to a future reader ("why is this pinned? let's modernise"). Removing it silently re-breaks the MCP server for every fresh install, and no existing test or CI job would notice. The guard makes the reason non-deletable.
How
tests/test_mcp_dependency_cap.pyasserts themcprequirement inpyproject.tomlhas an upper bound, and the failure message explains why and what porting is required first.It matches the requirement line with a regex instead of parsing TOML:
tomllibis 3.11+ and we still support 3.10, and this answers one question about one line — not worth a third-party parser or a version-conditional import.Follow-ups
mcp2.x API (mcp.server.mcpserver) so the cap can be lifted deliberately.WRTLS-199hid the cause — the original crash reported "an internal error occurred" rather than naming the missing module. Worth its own ticket.wrapper-mountonmain, not only when MCP/npm paths change.Tests
tests/test_mcp_dependency_cap.py— passes againstmain's cap. Verified it actually bites: removed the cap locally, test went red with the explanatory message, restored it.Root cause was confirmed against the published wheel in a clean
python:3.12-slimcontainer before any of this:worthless[mcp]==0.3.11→ mcp 2.0.0 →fastmcpimport fails →WRTLS-199; withmcp>=1.0,<2→ mcp 1.29.0 → import OK → server starts.Relates to WOR-868 (duplicate of WOR-864, which shipped the cap).
🤖 Generated with Claude Code
Summary by CodeRabbit