Repository navigation
Carry capabilities.extensions on legacy initialize handshakes - #3776
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
📚 Documentation preview
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked two things in this change and ruled them out.
create_initialization_optionsno longer falls back toself.extensionsitself, butget_capabilitiesstill does (src/mcp/server/lowlevel/server.py:634) and maps an empty map toNone, so the default is preserved.- The schema patch only touches the 2025-11-25 surface, but every pre-2026 version in
mcp_types/methods.pymaps to thev2025models, so the round-trip test's parametrization over allPRE_2026versions is covered.
Extended reasoning...
The change adds an extensions field to the generated 2025-11-25 capability models through a generator schema patch, changes one argument in Server.create_initialization_options, flips three pinned-gap tests and updates one docs page. It touches no auth, crypto or permission code, but it does change what the legacy initialize handshake advertises to clients, which is observable behaviour under the repository's 2.x API stability rule. The inline findings include one that is behavioural, not a nit, so this is not an approval.
One optional note from this repository's REVIEW.md or CLAUDE.md checks was not posted as a comment, over this review's limit for such notes; it is on this commit's check card.
| notification_options or NotificationOptions(), | ||
| experimental_capabilities, | ||
| extensions if extensions is not None else self.extensions, | ||
| extensions, |
There was a problem hiding this comment.
🟡 (optional) Legacy clients are now told a server supports an extension whose only methods that server answers with METHOD_NOT_FOUND on the same connection. The legacy initialize result advertises every entry of self.extensions whatever the version, while _version_gated (src/mcp/server/mcpserver/server.py:1389) still refuses methods pinned with protocol_versions. Fix: on the legacy initialize path, omit identifiers whose contributions are all gated away from handshake versions, mirroring the client's _build_capabilities (src/mcp/client/session.py:621-628). The base hid this because the 2025 sieve dropped the map. [also at: scripts/gen_surface_types.py:80 - Legacy clients are now told a server supports an extension whose methods it refuses on that connection. The initialize result now carries every entry of Server.extensions, whatever version was negotiated.]
Why this was flagged
Trigger: an MCPServer whose extension binds only version-pinned methods, as docs_src/extensions/tutorial004.py:35-40 does with protocol_versions=frozenset({"2026-07-28"}), reached by a client with mode="legacy". _apply_extension writes self._lowlevel_server.extensions[extension.identifier] unconditionally (src/mcp/server/mcpserver/server.py:352), and get_capabilities copies the whole map with no version filter (src/mcp/server/lowlevel/server.py:634). After this change the legacy initialize result therefore carries extensions: {"com.example/search": {}}, yet gated raises METHOD_NOT_FOUND for that method (src/mcp/server/mcpserver/server.py:1389-1390). A client that feature-detects on server_capabilities.extensions now passes its check and gets -32601; on the base branch the map was dropped by the 2025 sieve, so the client saw None and skipped the call. No safeguard applies: the client side drops inactive identifiers on legacy, but the server has no counterpart.
Verification: Triggers when an MCPServer extension binds only methods pinned to a modern version and a client connects with a legacy initialize handshake. src/mcp/server/mcpserver/server.py:1389-1390: if ctx.protocol_version not in versions: raise MCPError(code=METHOD_NOT_FOUND, ...). On the base, the 2025 wire model had no extensions field, so the initialize sieve dropped the map.
Finding ID: f-f019fd81
| client_info = {"name": "C", "version": "1"} | ||
| params = {"protocolVersion": version, "capabilities": {"extensions": extensions}, "clientInfo": client_info} | ||
| request = methods.parse_client_request("initialize", version, params) | ||
| assert isinstance(request, types.InitializeRequest) | ||
| assert request.params.capabilities.extensions == extensions |
There was a problem hiding this comment.
🟡 nit (optional): The client-request half of the new test passes without this PR, so nothing proves the ClientCapabilities schema patch. parse_client_request returns the monolith model parsed from the raw params (src/mcp-types/mcp_types/methods.py:528). The old 2025 surface simply ignored the unknown extensions key. Fix: assert something only the patched surface model can show, e.g. validate the params with the 2025 surface InitializeRequest and check extensions survives its dump, or drop that half of the test.
Why this was flagged
The condition is always present: test_initialize_round_trips_capability_extensions_at_pre_2026_versions asserts request.params.capabilities.extensions == extensions at tests/types/test_methods.py:951. parse_client_request first calls validate_client_request, whose surface model has extra="ignore", then builds the returned object from the untouched body via _monolith_row(monolith, method).model_validate(_body(method, params), by_name=False) (methods.py:527-528). The monolith ClientCapabilities already had extensions (src/mcp-types/mcp_types/_types.py:407), so this assertion holds on the base branch too. Removing the $defs/ClientCapabilities/properties/extensions patch (scripts/gen_surface_types.py:79) would leave the test green, so the patch is unguarded against regression. The repository's test-quality skill asks for the hollow-proof check ("which assertion proves this?") and a provenance docstring, which this test also lacks. No safeguard applies; the server-result half (sieved["capabilities"] == capabilities, line 943) is sound.
Verification: The request half of the new test (tests/types/test_methods.py:947-951) would pass on the base commit, so it does not exercise the ClientCapabilities schema patch. parse_client_request (src/mcp-types/mcp_types/methods.py:527-528) builds the returned object from the raw params, never from the surface model. On the base an unknown extensions key was ignored, not rejected.
Finding ID: f-11055b97
|
|
||
|
|
||
| @pytest.mark.parametrize("version", PRE_2026) | ||
| def test_initialize_round_trips_capability_extensions_at_pre_2026_versions(version: str): |
There was a problem hiding this comment.
🟡 nit (optional): the new test test_initialize_round_trips_capability_extensions_at_pre_2026_versions has no return type hint, which AGENTS.md requires for all code. Fix: add -> None to the signature at tests/types/test_methods.py:938, as the three other tests this PR touches already have. [also at: tests/types/test_methods.py:938 - nit: AGENTS.md requires type hints for all code: the new test_initialize_round_trips_capability_extensions_at_pre_2026_versions(version: str) in tests/types/test_methods.py annotates its parameter but has no return annotation.]
Why this was flagged
No runtime input triggers this; it is a convention check on the added test at tests/types/test_methods.py:938. The signature is def test_initialize_round_trips_capability_extensions_at_pre_2026_versions(version: str): with a typed parameter but no return annotation. AGENTS.md line 50 says type hints are required for all code, and the tests this PR renames in tests/server/test_apps.py:86, tests/server/test_extensions_capability.py:59 and tests/server/mcpserver/test_extension.py:179 all carry -> None. Nothing fails at runtime and the base branch has no such test; neighbouring older tests in the same file (lines 912, 924, 954) also omit the annotation, so no tool enforces it.
Verification: AGENTS.md line 50 reads - Type hints required for all code. The diff adds, at tests/types/test_methods.py:938, def test_initialize_round_trips_capability_extensions_at_pre_2026_versions(version: str): — the parameter is annotated but there is no return annotation. No tooling enforces it, so CI will not fail.
Finding ID: f-a8ecf1a0
|
|
||
|
|
||
| @pytest.mark.parametrize("version", PRE_2026) | ||
| def test_initialize_round_trips_capability_extensions_at_pre_2026_versions(version: str): |
There was a problem hiding this comment.
🟡 nit (optional): The new parametrized test has no docstring, so a later failure gives no hint whether it is a spec regression or an SDK choice. test_initialize_round_trips_capability_extensions_at_pre_2026_versions at tests/types/test_methods.py:938 goes straight into its body. Fix: add a one or two sentence docstring that states the behaviour and its provenance, for example SDK-defined: as the two flipped legacy-handshake tests in this change do.
Why this was flagged
The trigger is a future failure of test_initialize_round_trips_capability_extensions_at_pre_2026_versions (tests/types/test_methods.py:938-951). The test asserts that an extensions map survives serialize_server_result, parse_server_result and parse_client_request for initialize at every PRE_2026 version, but carries no docstring. .claude/skills/test-quality/SKILL.md:16-19, which AGENTS.md:68 requires tests to conform to, asks for "Docstrings: 1–2 sentences of behaviour, honest about provenance — spec-mandated, SDK-defined, or pinning a known gap?" because "provenance is the triage key when the test later fails". The extensions field on the 2025-11-25 capability models is an SDK-added schema patch (scripts/gen_surface_types.py:79-80), not part of the vendored schema, so the provenance is not obvious from the test. Nothing breaks at runtime and the base branch has no such test. No safeguard applies: no lint enforces test docstrings, and the undocumented neighbour at line 924 is older code the skill says not to copy.
Verification: nit — triggered whenever this test later fails and someone has to triage it; nothing breaks at runtime. The skill rule is at .claude/skills/test-quality/SKILL.md:16-19. The added test violates it: tests/types/test_methods.py:937-951 is new in this diff, and line 938 is followed directly at line 939 by extensions = {...} with no docstring.
Finding ID: f-0b6238ad
Closes #3773.
On connections that negotiate
2025-11-25or earlier,serialize_server_result("initialize", ...)sieves the result through the 2025 wire model, which has noextensionsfield, so a server could never advertise an extension (e.g. MCP Apps) to a legacy client.scripts/gen_surface_types.py: add a schema patch that gives the 2025-11-25ClientCapabilities/ServerCapabilitiesan optionalextensionsmap, and regenerate_v2025_11_25.patch_schemanow acceptsold=Noneto mean "key absent".Server.create_initialization_optionsno longer forwardsself.extensionsdirectly;get_capabilitiesalready defaults to it and maps an empty dict toNone, so servers without extensions still send noextensionskey.initializeround-trip test per pre-2026 version, and updatedocs/advanced/extensions.md.AI Disclaimer
This PR was developed with the assistance of either Claude or Codex. I've reviewed and verified the changes.