Skip to content

chore(deps): update mcp requirement from <2,>=1.28.1 to >=2.2.0,<3 - #2334

Merged
JSv4 merged 6 commits into
mainfrom
dependabot/pip/mcp-gte-2.2.0-and-lt-3
Sep 14, 2026
Merged

JSv4 merged 6 commits into
mainfrom
dependabot/pip/mcp-gte-2.2.0-and-lt-3

Conversation

@dependabot

@dependabot dependabot Bot commented on behalf of github Sep 10, 2026

Copy link
Copy Markdown
Contributor

Updates the requirements on mcp to permit the latest version.

Release notes

Sourced from mcp's releases.

v2.2.0

pip install -U mcp. Docs: https://py.sdk.modelcontextprotocol.io/

A few defaults changed in this release. If you run a server or client on 2.x, skim these first:

Behaviour changes

HTTP client redirects are only followed within the endpoint's origin (#3397)

  • Client("https://..."), streamable_http_client and sse_client follow a redirect only if it stays on the same scheme, host and port (or upgrades http to https on the same host).
  • A redirect anywhere else is not followed: the call fails with MCPError and the session stays usable (an SSE connect fails with httpx2.HTTPStatusError). If that other URL is the server you meant, use it as the endpoint URL.
  • The follow_redirects setting on an httpx2.AsyncClient you pass in is no longer used for MCP requests, so you don't need it for the trailing-slash redirect any more.
  • The OAuth providers apply the same rule to their own requests.

Idle Streamable HTTP sessions now expire (legacy <=2025-11-25 spec( (#3395)

  • A stateful session with nothing in flight for 30 minutes is closed. The client's next request gets a 404 and it has to initialize again.
  • Clients that keep the GET stream open (the SDK's Client does) are not affected. Neither are stateless servers or 2026-07-28 connections.
  • A server also holds at most 10 000 sessions at once; beyond that, new sessions get a 503.
  • To turn either off: mcp.run(transport="streamable-http", session_idle_timeout=None, max_sessions=None) (also on streamable_http_app() and run_streamable_http_async()).

The OAuth client checks the authorization server's issuer on the legacy path too (#3398)

  • For servers without protected resource metadata, authorization server metadata whose issuer isn't the server's own origin is now rejected with OAuthFlowError: Authorization server metadata issuer mismatch. The protected-resource-metadata path has done this since 2.0.
  • A 403 that isn't an insufficient_scope challenge is returned to the caller instead of retried.
  • If protected resource metadata can't be fetched because of a 5xx/429, the flow now stops instead of falling back to the legacy endpoints.

Two new MCPDeprecationWarnings (#3435, #3447)

  • ClientCredentialsOAuthProvider / PrivateKeyJWTOAuthProvider without issuer=. Pass your authorization server's issuer URL; 3.0 will require it.
  • AuthSettings with resource_server_url set but validate_token_resource unset. Set it to True or False; 3.0 defaults it to True.
  • Both keep working as before in 2.x; this mostly matters if your tests turn warnings into errors.

New

  • AuthSettings.validate_token_resource: only accept tokens your TokenVerifier reports as issued for this server (#3447).
  • issuer= on ClientCredentialsOAuthProvider and PrivateKeyJWTOAuthProvider (#3398).
  • session_idle_timeout= and max_sessions= on the Streamable HTTP server entry points (#3395).

Fixes

  • A client DELETE frees its session immediately, and a refused opening request no longer leaves a session behind (#2455, #3228, #3300).
  • $refs in a tool's outputSchema resolve within that schema only; an unresolvable one surfaces as RuntimeError: Invalid schema for tool ... (#3394).

Known gaps

The tasks extension (SEP-2663), DPoP (SEP-1932) and the jwt-bearer grant are not implemented yet; https://github.com/modelcontextprotocol/python-sdk/blob/main/ROADMAP.md tracks them.

What's Changed

... (truncated)

Commits
  • 9972c21 Replace RootModel wrappers with type aliases and TypeAdapter validation (#3470)
  • fd66270 docs: refresh translations, and translate pages in parallel (#3458)
  • 08a3bc8 docs: ask for AI disclosure on comments too (#3459)
  • 7bb486a docs: stop presenting the in-memory client as the way to connect (#3443)
  • 0c91368 Add AuthSettings.validate_token_resource to check a bearer token's resource (...
  • 9771e6b Keep following a relative redirect when the endpoint URL carries userinfo (#3...
  • a925e55 Bump the locked versions of eight dev and test dependencies (#3449)
  • e8b9486 Bump pymdown-extensions from 11.0 to 11.0.1 (#3285)
  • c6762e8 Follow redirects only within the MCP endpoint's origin (#3397)
  • 5fd3abc Skip automatic docs previews for fork PRs and drop the setup-uv retry steps (...
  • Additional commits viewable in compare view

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase.


Dependabot commands and options

You can trigger Dependabot actions by commenting on this PR:

  • @dependabot rebase will rebase this PR
  • @dependabot recreate will recreate this PR, overwriting any edits that have been made to it
  • @dependabot show <dependency name> ignore conditions will show all of the ignore conditions of the specified dependency
  • @dependabot ignore this major version will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
  • @dependabot ignore this minor version will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
  • @dependabot ignore this dependency will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)

Updates the requirements on [mcp](https://github.com/modelcontextprotocol/python-sdk) to permit the latest version.
- [Release notes](https://github.com/modelcontextprotocol/python-sdk/releases)
- [Changelog](https://github.com/modelcontextprotocol/python-sdk/blob/main/RELEASE.md)
- [Commits](modelcontextprotocol/python-sdk@v1.28.1...v2.2.0)

---
updated-dependencies:
- dependency-name: mcp
  dependency-version: 2.2.0
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>
@dependabot dependabot Bot added dependencies Pull requests that update a dependency file python Pull requests that update Python code labels Sep 10, 2026
@claude

claude Bot commented Sep 10, 2026

Copy link
Copy Markdown

Review

This is an automated Dependabot bump of mcp from >=1.28.1,<2 to >=2.2.0,<3. This should not be merged as-is — it directly contradicts a pre-existing pin comment in the same file (requirements/base.txt:84-90, unchanged by this PR):

pydantic-ai-slim[mcp] -> fastmcp-slim caps mcp<2.0 across its whole published range as of 2026-08; mcp 2.0 is a breaking rewrite (decorator-based handler registration on mcp.server.lowlevel.Server removed in favor of on_*= constructor kwargs) that opencontractserver/mcp/server.py does not yet speak. Bump this pin only alongside a migration of that file, once fastmcp-slim ships v2 support...

Findings

  1. Breaking API usage confirmed in-repo. opencontractserver/mcp/server.py uses exactly the decorator-based registration pattern the comment warns is removed in mcp 2.0:

    • @mcp_server.list_resources() / @mcp_server.list_tools() (lines ~758, 798, 1397, 1413)
    • mcp_server.call_tool()(call_tool_handler) (line 1025)
    • @scoped_server.call_tool() (line 1418)

    Both the module-level server (mcp_server = Server("opencontracts")) and the per-corpus scoped server (scoped_server = Server(f"opencontracts-corpus-{corpus_slug}")) rely on this API. If mcp 2.0's Server.list_tools()/call_tool() decorators are indeed gone, this file will fail at import/registration time, taking down both the stdio MCP entrypoint and the scoped-per-corpus HTTP/SSE servers.

  2. Likely unresolvable as a standalone bump anyway. requirements/base.txt:59 pins pydantic-ai-slim[openai,anthropic,google,mcp]>=1.107.5,<2, which pulls in fastmcp-slim. Per the existing comment, fastmcp-slim caps mcp<2.0 across its whole published range, so this PR's mcp>=2.2.0 may not even resolve without a corresponding fastmcp-slim/pydantic-ai-slim update — worth confirming pip-compile/pip install actually succeeds with this change before merging.

  3. No corresponding code migration in this PR. The diff touches only requirements/base.txt (1 line). Per the comment's own instructions ("bump this pin only alongside a migration of that file"), a version bump this large needs a companion migration of opencontractserver/mcp/server.py to the new on_*= constructor-kwarg API, plus test coverage for both the module-level and scoped MCP servers.

Recommendation

Close or hold this PR until opencontractserver/mcp/server.py is migrated to the mcp 2.x API (and fastmcp-slim/pydantic-ai-slim are confirmed compatible). Merging as-is risks breaking the MCP server entrypoints in production. Consider adding this dependency to Dependabot's ignore list for major-version bumps (ignore: dependency-name: mcp, versions: [">=2"]) until the migration lands, to avoid repeated auto-generated PRs against a documented blocker.

python-sdk 2.0 removed decorator-based handler registration on
mcp.server.Server in favour of on_*= constructor kwargs whose handlers take
(ctx, params) and return typed result models. opencontractserver/mcp/server.py
still used the 1.x decorators, so the dependabot bump alone would have broken
both MCP entrypoints at import time (see the review on #2334).

- server.py: build the global and corpus-scoped servers through shared
  adapters (_build_on_call_tool, _build_on_list_tools,
  _build_on_list_resource_templates, _on_read_resource). Argument validation
  against inputSchema and isError wrapping of dispatcher exceptions -- which
  the 1.x call_tool decorator did implicitly -- are now explicit. Tool and
  template catalogues move to get_tool_definitions() /
  get_resource_template_definitions(). SDK types use their 2.x snake_case
  fields; resource URIs are plain str.
- resources/read payloads are stamped application/json
  (MCP_RESOURCE_MIME_TYPE) to match the advertised templates; caller-side
  read failures return JSON-RPC INVALID_PARAMS with the message.
- tests: replace the removed server.request_handlers seam with
  mcp.client.Client in-memory sessions and add MCPSdkClientRoundTripTest,
  which drives both servers through the SDK runtime (list/call/read,
  validation errors, permission propagation) plus a real stateless
  Streamable HTTP JSON-RPC request through StreamableHTTPSessionManager.
- requirements/base.txt + .pre-commit-config.yaml: mcp>=2.2.0,<3 with the
  stale "fastmcp-slim caps mcp<2" note replaced (fastmcp-slim resolves
  against 2.x; pip check is clean).
- docs/mcp/README.md: SDK integration section; changelog fragment added.

JSv4 commented Sep 14, 2026

Copy link
Copy Markdown
Collaborator

Pushed the companion migration the review asked for (3fc6fde), so this bump no longer lands as a bare pin change. Addressing the three findings:

  1. Breaking API usage in opencontractserver/mcp/server.py — migrated. Both the global server (create_mcp_server) and the per-corpus scoped server (create_scoped_mcp_server) are now built with the 2.x on_*= constructor kwargs through a shared set of adapters (_build_on_call_tool, _build_on_list_tools, _build_on_list_resource_templates, _on_read_resource). The two behaviours the 1.x call_tool() decorator provided implicitly — jsonschema validation of arguments against the advertised inputSchema, and wrapping dispatcher exceptions into isError results — are now explicit in the adapter. SDK types use their 2.x snake_case fields (input_schema, uri_template, mime_type; URIs are plain str).

  2. Resolvability — confirmed. mcp>=2.2.0,<3 resolves with the existing pydantic-ai-slim[...,mcp]>=1.107.5,<2 pin (fastmcp-slim 3.4.7/4.0.x both accept mcp 2.x), and pip check on the resolved tree is clean. The stale "fastmcp-slim caps mcp<2" comment in requirements/base.txt is replaced, and the pre-commit mypy hook's mcp pin is moved to the same >=2.2.0,<3 range so the hook env and runtime can't diverge again.

  3. Test coverage — the removed server.request_handlers[...] seam is replaced by mcp.client.Client in-memory sessions, and a new MCPSdkClientRoundTripTest drives both servers through the SDK runtime (tools/list, tools/call, resources/read, argument-validation errors, unknown tool, _mcp_user permission propagation) plus a real stateless Streamable HTTP JSON-RPC request through StreamableHTTPSessionManager and the ASGI app. Verified locally: the full opencontractserver/mcp/tests/test_mcp.py and test_mcp_extended.py suites, the rate-limiting/discovery/architecture suites that import the server, mypy, and all pre-commit hooks pass against mcp 2.2.0.

Two intentional wire-level changes, noted in the changelog fragment: resources/read contents are stamped application/json (matching the advertised templates; 1.x's deprecated str-return path stamped text/plain), and a resource read that fails on the caller's side (unrecognised URI, invisible corpus) now returns JSON-RPC INVALID_PARAMS with the message rather than a generic internal error.


Generated by Claude Code

@claude

claude Bot commented Sep 14, 2026

Copy link
Copy Markdown

Review: mcp python-sdk 2.x migration

This is well beyond a routine dependabot bump — opencontractserver/mcp/server.py was ported from the 1.x decorator API (@server.list_tools(), etc.) to the 2.x on_*= constructor-kwarg API, with a shared set of adapters for the global and corpus-scoped servers. Overall this is a solid piece of engineering: the _build_on_call_tool / _build_on_list_tools / _on_read_resource adapters cleanly consolidate what used to be duplicated per-server logic, requirements/base.txt and .pre-commit-config.yaml are kept in lockstep with explanatory comments (avoiding the exact "main goes red with no commit touching mcp" failure mode called out in the old comment), the changelog fragment is detailed and follows the repo's changelog.d/ convention, and docs/mcp/README.md documents the new SDK integration contract. The new MCPSdkClientRoundTripTest class is a real improvement — it drives both servers through mcp.client.Client and a live StreamableHTTPSessionManager/ASGI request instead of reaching into server.request_handlers[...], so it will actually catch a future SDK contract drift.

A few things worth a look before merge:

1. Schema-validation short-circuit skips per-tool rate limiting and telemetry (moderate)

In _build_on_call_tool (opencontractserver/mcp/server.py:819-836), argument validation against tool.input_schema now happens in the SDK adapter, before dispatch() (i.e. call_tool_handler / the scoped call_tool closure) is ever called:

```python
async def on_call_tool(ctx, params) -> CallToolResult:
arguments = params.arguments or {}
tool = tools_by_name.get(params.name)
if tool is not None:
try:
jsonschema.validate(instance=arguments, schema=tool.input_schema)
except jsonschema.ValidationError as e:
return _tool_error_result(f"Input validation error: {e.message}")
try:
content = await dispatch(params.name, arguments)
...
```

But _check_per_tool_rate_limit(name) and the arecord_mcp_tool_call(...) telemetry calls only happen inside dispatch() (server.py:715 for call_tool_handler, server.py:1575 for the scoped closure). This is new: pre-PR there was no jsonschema-based pre-validation at all (confirmed against the pre-migration file), so every call — including ones with mistyped arguments — used to reach the dispatcher and get rate-limited/recorded before failing.

Net effect after this PR: a caller can send a known tool name with a deliberately wrong-typed argument (e.g. list_public_corpuses with limit: "ten") repeatedly, and each call bypasses the per-tool rate-limit bucket and leaves no telemetry row, because it never reaches _check_per_tool_rate_limit/arecord_mcp_tool_call. The ASGI-level global rate limit (skip_global=False path) still applies as a backstop, so this isn't unbounded, but it does defeat the per-tool bucket specifically — which matters most for the one write tool, create_thread_message (the code's own comment at server.py:415-421 notes it doesn't yet have a dedicated stricter bucket and currently relies on the generic per-tool limit). It's also a monitoring blind spot: dashboards built on arecord_mcp_tool_call will silently undercount malformed-argument traffic.

test_global_server_rejects_mistyped_arguments (new in this PR) only asserts the isError shape, not that rate-limiting/telemetry still fire — so this gap isn't covered either way. Worth either moving the rate-limit check ahead of schema validation, or recording telemetry/rate-limit consumption for schema failures too, so the failure mode is consistent regardless of which validation layer rejects the call.

2. jsonschema.SchemaError isn't caught (minor)

Same block: except jsonschema.ValidationError doesn't cover jsonschema.exceptions.SchemaError, which jsonschema.validate raises if the schema itself is malformed rather than the instance. Low risk today since all input_schema dicts are static and hand-written, but if a future tool definition has a typo in its schema, the failure mode changes from a friendly isError to an unhandled exception escaping on_call_tool (bypassing the "any exception becomes isError" guarantee the rest of this PR is careful to preserve). Might be worth broadening the except clause, or adding a test that validates every entry from get_tool_definitions()/get_scoped_tool_definitions() is a syntactically valid JSON Schema.

3. New direct jsonschema import isn't pinned in requirements/base.txt (nit)

server.py:31 adds import jsonschema as a production-code dependency, but requirements/base.txt doesn't list it — every other line in that file gets its own explicit pin + source comment (including the mcp line touched by this same PR). If jsonschema is only present transitively today (via mcp/fastmcp-slim), that's an implicit dependency on another package's dependency tree; a future bump elsewhere could drop it without anything in this repo's manifests noticing. Worth adding an explicit pinned line for it if it's meant to be a first-class dependency now.


Nothing above blocks the migration itself — the adapter design, error-wrapping parity with 1.x, and resource/URI field renames (inputSchemainput_schema, uriTemplateuri_template, mimeTypemime_type, AnyUrlstr) all look correctly and thoroughly handled and tested.

Follow-up to the python-sdk 2.x migration, addressing the automated review
on #2334:

- A call rejected by inputSchema validation never reached the dispatcher,
  so it consumed no per-tool rate-limit budget and left no telemetry row.
  _reject_malformed_arguments now runs _check_per_tool_rate_limit and
  records an InputValidationError event before returning the isError
  result; scoped servers pass their URL-bound corpus_slug for attribution.
  (1.x's call_tool decorator had the same pre-validation gap; this closes
  it rather than merely restoring parity.)
- The adapter's catch-all now also covers a jsonschema.SchemaError from a
  malformed tool definition, and MCPToolSchemaValidityTest checks every
  advertised inputSchema against the JSON Schema meta-schema so such a
  typo fails in CI instead of at runtime.
- jsonschema is imported directly by server.py, so it gets an explicit pin
  in requirements/base.txt instead of riding on mcp's dependency tree.

JSv4 commented Sep 14, 2026

Copy link
Copy Markdown
Collaborator

Addressed all three findings in e9da3dc.

1. Schema-rejected calls bypass per-tool rate limiting / telemetry — fixed. One correction to the premise: this was not new in this PR. The 1.x @server.call_tool() decorator ran the same jsonschema.validate(arguments, tool.inputSchema) (validate_input=True by default) before invoking our dispatcher and returned isError on failure, so malformed calls never reached _check_per_tool_rate_limit / arecord_mcp_tool_call pre-migration either. The gap is real regardless, so rather than restore parity I closed it: _reject_malformed_arguments now runs _check_per_tool_rate_limit(name) (an exhausted bucket yields the rate-limit isError, never the validation message) and records an InputValidationError telemetry event, with scoped servers passing their URL-bound corpus_slug for attribution. Covered by test_schema_rejection_still_rate_limits_and_records_telemetry, test_schema_rejection_honors_per_tool_rate_limit, and test_scoped_schema_rejection_records_url_bound_corpus. The check is not duplicated for valid calls: the dispatcher remains the single place that bills those.

2. jsonschema.SchemaError uncaught — fixed both ways. Validation now sits inside the adapter's catch-all so a malformed schema degrades to an isError result rather than escaping, and the new MCPToolSchemaValidityTest checks every advertised inputSchema (global + scoped) against the JSON Schema meta-schema so a typo fails in CI instead of at runtime.

3. Explicit jsonschema pin — added jsonschema>=4.20.0,<5 to requirements/base.txt (the floor matches mcp's own requirement) with a comment saying why it is first-class now.

Verified locally against mcp 2.2.0: full opencontractserver/mcp/tests/test_mcp.py + test_mcp_extended.py + test_unified_rate_limiting.py, mypy, and pre-commit all pass.


Generated by Claude Code

@claude

claude Bot commented Sep 14, 2026

Copy link
Copy Markdown

Review

This is a substantial, well-executed migration of the MCP server to python-sdk 2.x, well beyond a typical dependabot bump. The on_*= adapter approach (_build_on_call_tool, _build_on_list_tools, _build_on_list_resource_templates, _on_read_resource) is a clean way to centralize argument validation, error wrapping, and resource serialization for both the global and scoped servers. The new MCPSdkClientRoundTripTest suite driving real mcp.client.Client sessions and a live StreamableHTTPSessionManager request is a big upgrade over the old server.request_handlers[...] seam - it actually exercises the SDK wiring that changed. The docstrings/comments (e.g. the "do not double-record telemetry" note on the scoped call_tool, the _format_tool_error_text identity-check comment) read like real lessons learned, not boilerplate. The .pre-commit-config.yaml mcp pin is correctly kept in lockstep with requirements/base.txt, and the changelog fragment follows the changelog.d/ convention.

A couple of minor things worth a look, nothing blocking:

  1. MCP_RESOURCE_MIME_TYPE constant is not used everywhere it applies.
    opencontractserver/constants/mcp.py adds MCP_RESOURCE_MIME_TYPE = "application/json" with a docstring saying it should match the mime_type advertised on their ResourceTemplate entries. It is used in _on_read_resource (server.py:926), but every Resource/ResourceTemplate definition that advertises mime_type="application/json" still hardcodes the literal string instead of referencing the constant: get_resource_template_definitions() (server.py:939,945,951,957), get_scoped_resource_definitions() (server.py:1475,1489,1512), and get_scoped_resource_template_definitions() (server.py:1536,1542,1548). Per CLAUDE.md guidance on avoiding magic values and using constants files, it would be worth sweeping those ~10 call sites to use MCP_RESOURCE_MIME_TYPE too, otherwise the constant and the literals can silently drift apart.

  2. Stale comment in tests.
    opencontractserver/mcp/tests/test_mcp.py line 3289 still says "Compare as strings since Resource.uri is an AnyUrl type", but the whole point of this migration is that Resource.uri is now a plain str (the AnyUrl import was removed from server.py). The str(...) wrapping is now a no-op; either drop the cast or update the comment so it does not mislead the next reader into thinking AnyUrl is still in play.

Other things checked that look correct:

  • Error-message exposure: _build_on_call_tool outer except Exception as e: return _tool_error_result(str(e)) will forward arbitrary internal exception text as tool-call content to any caller, including anonymous ones on the public endpoint. This mirrors the 1.x SDK own decorator behavior and the PR is explicit about preserving that contract, so it is not a regression - just flagging for awareness (contrast with _on_read_resource, where only ValueError/PermissionDenied/ValidationError/ObjectDoesNotExist get their message surfaced via INVALID_PARAMS, and anything else falls through to the SDK generic internal error).
  • Rate-limit/telemetry double-counting: _reject_malformed_arguments accounts for schema-rejected calls itself (since dispatch is never reached), and the scoped call_tool docstring explicitly warns against re-adding a pre-raise arecord_mcp_tool_call for the unknown-tool/corpus-visibility checks. Tests (test_schema_rejection_still_rate_limits_and_records_telemetry, test_scoped_schema_rejection_records_url_bound_corpus) cover this correctly.
  • content = await dispatch(...) control flow in _build_on_call_tool.on_call_tool correctly runs for both the unknown-tool path (skips schema check, dispatch raises ValueError) and the schema-valid path, with no unbound-variable risk.
  • Global vs. scoped Tool/ResourceTemplate catalogues consistently switched from camelCase (inputSchema, uriTemplate, mimeType) to the 2.x snake_case fields, and AnyUrl was fully removed from server.py in favor of plain str URIs.

Test coverage looks strong - schema-validity pinning (MCPToolSchemaValidityTest), rate-limit/telemetry edge cases, resource-read error mapping, and the full SDK round trip are all covered.

…nyUrl test comment

Follow-up nits from the automated review on #2334: the constant added for
resources/read payloads now also backs every Resource/ResourceTemplate
mime_type so the advertised and served types cannot drift, and the scoped
resource test no longer claims Resource.uri is an AnyUrl (mcp 2.x types it
as str).

JSv4 commented Sep 14, 2026

Copy link
Copy Markdown
Collaborator

Both nits taken in 29f5f5f: every Resource / ResourceTemplate mime_type now references MCP_RESOURCE_MIME_TYPE (the literal is gone from server.py), and the scoped-resource test no longer claims Resource.uri is an AnyUrl. No behaviour change; the resource, scoped-server and SDK round-trip tests plus mypy pass locally.

On the error-exposure note: agreed that _build_on_call_tool forwarding str(e) for unexpected exceptions is inherited 1.x behaviour rather than a regression, so it stays as is in this PR.


Generated by Claude Code

@codecov

codecov Bot commented Sep 14, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 99.65398% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
opencontractserver/mcp/tests/test_mcp.py 99.56% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

JSv4 commented Sep 14, 2026

Copy link
Copy Markdown
Collaborator

Status on 29f5f5f: every code check is green (linter, pytest, backend-ci-gate, redis-integration, CodeQL, codecov patch). The one red check, claude-review, is the review action itself failing internally (Claude result reported subtype success with is_error:true in the job log) before it posted anything; the diff does not touch that workflow and the same job succeeded on the two previous heads. Re-running it once; nothing to fix in the PR for it.

The Codecov note (one uncovered line in test_mcp.py) is informational: codecov/patch reports success and the line is the unreachable return None fallthrough of a test helper.


Generated by Claude Code

JSv4 commented Sep 14, 2026

Copy link
Copy Markdown
Collaborator

Re-running claude-review from here was refused (403, the session token cannot re-run workflow jobs). A maintainer can re-run the failed job on run 34824842468 from the Actions tab; the PR itself needs no change for it. Still watching this PR.


Generated by Claude Code

@claude

claude Bot commented Sep 14, 2026

Copy link
Copy Markdown

Review: mcp 2.x migration

This is a substantial hand-authored migration riding on top of the dependabot version bump (not a mechanical dependency update), so I reviewed opencontractserver/mcp/server.py and the test suite in full rather than just the diff stats.

Strengths

  • Behavior parity is explicit and tested. The two things the 1.x decorator API gave "for free" — inputSchema validation before dispatch, and exceptions-to-isError conversion — are reimplemented in _build_on_call_tool/_reject_malformed_arguments and covered by dedicated tests (test_global_server_rejects_mistyped_arguments, test_schema_rejection_still_rate_limits_and_records_telemetry, test_schema_rejection_honors_per_tool_rate_limit). Good catch that a schema-rejected call still needs to consume the per-tool rate-limit bucket and hit telemetry — that's an easy thing to miss when moving validation out of the dispatcher.
  • DRY consolidation. _build_on_call_tool / _build_on_list_tools / _build_on_list_resource_templates / _on_read_resource are shared between the global and scoped servers, replacing what would otherwise be two near-duplicate on_*= wiring blocks.
  • Real SDK round-trip coverage, not just handler-function unit tests: MCPSdkClientRoundTripTest drives both servers through mcp.client.Client in-memory sessions, and one test goes further to exercise a real stateless Streamable HTTP JSON-RPC request through StreamableHTTPSessionManager — that's exactly the seam that changed (on_*= kwargs, typed result envelopes), so it's the right level to pin.
  • MCPToolSchemaValidityTest guards against a hand-written inputSchema typo surfacing as a runtime jsonschema.SchemaError instead of failing CI — a nice defensive addition given schema validation is now done manually rather than by the framework.
  • Version pins kept in lockstep. .pre-commit-config.yaml's mcp range was updated to match requirements/base.txt, with a comment explaining why drift there silently breaks main (matches the kind of failure mode already called out in this repo's CLAUDE.md pitfalls list). jsonschema is now pinned explicitly since server.py takes a direct runtime dependency on it rather than relying on it arriving transitively via mcp.
  • Changelog fragment and docs/mcp/README.md updates are present and point at the code (adapter section, test names) rather than duplicating logic in prose, matching the repo's docs convention.

Minor notes (non-blocking)

  • from mcp.server.lowlevel.server import ServerRequestContext reaches into a lowlevel submodule for a type. If that isn't part of the SDK's stable public surface, a future patch release could move/rename it silently (this codebase has already been bitten by similar upstream-internal-seam drift — see CLAUDE.md pitfall Bump django-debug-toolbar from 3.2.2 to 3.7.0 #21 re: pydantic-ai). It's only used for type hints on the adapter closures, so the blast radius of a rename is small, but worth a quick check of whether mcp.server or mcp.types re-exports it instead.
  • In _build_on_call_tool, a jsonschema.SchemaError from a malformed tool schema (as opposed to a user's bad arguments) falls through to the generic except Exception and becomes an opaque isError string with no server-side log line. MCPToolSchemaValidityTest should catch this class of bug before it ships, so the risk is low, but if it ever did regress in production it'd be silent until someone reads a client-side error message.
  • CallToolResult(content=list(content))content from dispatch() is already typed list[TextContent], so the list(...) wrap is redundant (harmless).

Verified

  • All CI checks green (pytest, backend-ci-gate, linter, CodeQL, redis-integration, codecov).
  • No other call sites in the codebase import the mcp SDK directly outside opencontractserver/mcp/ (checked via grep), so this migration is self-contained and doesn't leave a partially-migrated surface elsewhere.
  • No pydantic-ai/fastmcp MCP-client usage elsewhere in opencontractserver/llms that would also need to track this bump.

Nice work — this is a clean, well-tested migration of a non-trivial breaking SDK upgrade.

…failures

Follow-up notes from the automated review on #2334: ServerRequestContext is
part of mcp.server's public __all__, so import it from there instead of the
lowlevel submodule (one less internal seam to drift). The adapter's catch-all
now also emits a server-side warning so a failure that surfaces to the client
as an isError result is visible in logs too. The list() copy on the result
content stays: list is invariant and mypy needs the copy to widen
list[TextContent] to the SDK's content-block union (comment added).

JSv4 commented Sep 14, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the re-review. The three minor notes, in a7a1b81:

  • ServerRequestContext import — good call: it is in mcp.server.__all__ in 2.2.0, so it is now imported from mcp.server alongside Server instead of the lowlevel submodule.
  • Silent SchemaError / unexpected failures — the adapter's catch-all now logs a server-side warning (MCP tool <name> failed: <type>: <msg>) before returning the isError result, so nothing in that branch is visible only to the client anymore.
  • list(content) — kept, with a comment: list is invariant, so mypy rejects passing a list[TextContent] where the SDK expects list[ContentBlock]; the copy is what lets it infer the wider element type. Removing it fails the mypy hook.

Round-trip and adapter tests, mypy and lint pass locally.


Generated by Claude Code

@claude

claude Bot commented Sep 14, 2026

Copy link
Copy Markdown

Review: MCP python-sdk 2.x migration

This started as a routine Dependabot bump (mcp<2 to mcp>=2.2.0,<3) but correctly grew into a full migration of opencontractserver/mcp/server.py off the removed 1.x decorator API (@server.list_tools() etc.) onto the 2.x on_*= constructor-kwarg handlers -- a plain dependency bump here would have broken both MCP entrypoints at import time, so good catch bundling that in.

Correctness

  • The adapter layer (_build_on_call_tool, _build_on_list_tools, _build_on_list_resource_templates, _on_read_resource) cleanly consolidates duplicate logic between the global and scoped servers -- a real DRY improvement over the old per-server decorator bodies.
  • Traced the rate-limit/telemetry path for schema-rejected calls (_reject_malformed_arguments): it calls _check_per_tool_rate_limit once, and only records InputValidationError telemetry if that doesn't already raise/record RateLimitExceeded -- no double-counting. test_schema_rejection_still_rate_limits_and_records_telemetry and test_schema_rejection_honors_per_tool_rate_limit cover both branches.
  • jsonschema.SchemaError from a malformed hand-written tool schema isn't caught by the inner except jsonschema.ValidationError, so it falls through to the outer catch-all in on_call_tool and becomes a generic isError -- consistent with the docstring, and MCPToolSchemaValidityTest guards against a bad schema ever shipping by validating every advertised inputSchema against the JSON Schema meta-schema in CI.
  • Confirmed no leftover 1.x surface anywhere in the module (grep for inputSchema=/uriTemplate=/mimeType=/AnyUrl/.request_handlers[ turned up nothing outside docstrings/comments referencing the wire-protocol field names).
  • MCP_RESOURCE_MIME_TYPE is now used consistently for every Resource/ResourceTemplate definition and the actual resources/read response, so the advertised and served MIME types can't drift -- nice follow-through from the earlier review round.

Minor / non-blocking observations

  • _on_read_resource narrows caught exceptions to (ValueError, PermissionDenied, ValidationError, ObjectDoesNotExist) to INVALID_PARAMS. That matches what resources.py's getters actually raise today, but it's an implicit contract between two modules with no shared marker/base exception -- worth a one-line comment in resources.py (or a docstring cross-reference) so a future resource getter that raises something else doesn't silently degrade to an opaque SDK internal-error instead of a structured INVALID_PARAMS.
  • The scoped call_tool closure in create_scoped_mcp_server and the module-level call_tool_handler still duplicate a fair amount of dispatch/telemetry logic (pre-existing, not introduced here) -- not a blocker, but might be worth a follow-up now that the SDK-facing side has been unified.
  • Nice touch keeping .pre-commit-config.yaml's mcp pin in exact lockstep with requirements/base.txt, with a comment explaining why a hook/runtime major-version mismatch would silently redden CI.

Test coverage
Coverage is thorough: MCPSdkClientRoundTripTest exercises both servers through the real SDK client (not just the internal dispatchers), including a genuine stateless Streamable HTTP JSON-RPC round trip through StreamableHTTPSessionManager -- that's the transport layer most migrations of this kind leave untested. Schema validation, rate-limit interaction, telemetry attribution for scoped vs. global tools, and permission-based resource visibility are all covered.

Security
No IDOR/permission concerns spotted -- the migration preserves the existing _mcp_user context-var propagation and the MIN(document, corpus)-style visibility checks in the unchanged resource/tool dispatchers; only the SDK registration shape changed. PermissionDenied/ValidationError/ObjectDoesNotExist still surface as structured payloads rather than raw tracebacks.

Overall this is a careful, well-tested migration with good documentation (README section + changelog fragment) tying the change together. I'd merge once CI is green (couldn't verify checks status directly from this session).

JSv4 commented Sep 14, 2026

Copy link
Copy Markdown
Collaborator

Thanks. On the two non-blocking notes:

  • Implicit exception contract between resources.py and _on_read_resource — agreed it deserves a cross-reference; it is a comment-only change, so rather than spend another full CI cycle on it alone I'll fold it into the next code push on this PR if one comes, otherwise it can land with the follow-up below.
  • Dispatch/telemetry duplication between the scoped call_tool closure and call_tool_handler — pre-existing, and worth unifying now that the SDK-facing side is shared, but it touches the telemetry/error-recording semantics both dispatchers pin with tests, so it belongs in its own PR rather than in a dependency bump.

Nothing else outstanding from my side; the PR is ready to merge once CI on a7a1b81 finishes.


Generated by Claude Code

@JSv4
JSv4 merged commit aedfc5f into main Sep 14, 2026
16 checks passed
@dependabot
dependabot Bot deleted the dependabot/pip/mcp-gte-2.2.0-and-lt-3 branch September 14, 2026 23:29
@github-actions github-actions Bot locked and limited conversation to collaborators Sep 14, 2026
@JSv4 JSv4 linked an issue Sep 15, 2026 that may be closed by this pull request
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

dependencies Pull requests that update a dependency file python Pull requests that update Python code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Migrate opencontractserver/mcp/server.py to the mcp>=2.0.0 API

2 participants