fix(server): normalize experimental capability to {} in get_capabilities - #3255
Conversation
An unconfigured server reported experimental differently depending on which
discovery path answered: {} via create_initialization_options() (the legacy
initialize path), None via a direct get_capabilities() call with no
experimental_capabilities argument (what server/discover does internally).
create_initialization_options() was the only caller normalizing None to {}
before passing it down, so get_capabilities() itself fell back to its own
parameter default whenever a caller didn't normalize first. Move the
normalization into get_capabilities(), next to the existing
notification_options fallback, so every caller gets the same value regardless
of what it passes.
Verified with the repro from the issue: legacy and modern now both report
experimental={} for the same server, and "experimental" is present in both
wire dumps instead of only the legacy one.
|
Thanks for the contribution. This repository only keeps pull requests open when they're linked to an issue that a maintainer has assigned to the author — CONTRIBUTING.md explains why and how we work. This PR has been closed for now because you aren't currently assigned to #3254. If a maintainer would like this change as a PR from you, they'll assign you to #3254 and this PR will reopen automatically — there's nothing more you need to do. (If you opened the issue, this PR already shows up on its timeline.) There's no need to open a new PR — this one will be reopened. While it's closed, please push any updates as new commits rather than force-pushing, since GitHub can't reopen a PR whose branch has been rewritten. Maintainers: reopening this PR, removing the |
Fixes #3254
What's broken
An unconfigured
Serverreports itsexperimentalcapability differently depending on which discovery path answers:initialize(viacreate_initialization_options()):experimental == {}, field present on the wire.server/discover(callsget_capabilities()directly):experimental is None, field omitted from the wire dump.That's client-visible:
.get(...)works on the legacy value and raises on the modern one, andis not Nonechecks flip meaning depending on which path answered. Same server, same lack of configuration, two different answers.Why
create_initialization_options()was the only caller that normalized a missingexperimental_capabilitiesargument to{}before handing it toget_capabilities().get_capabilities()itself just used its own parameter default (None) whenever a caller didn't do that normalization first — andserver/discover's internal handler callsget_capabilities(protocol_version=ctx.protocol_version)with nothing forexperimental_capabilities, so it fell straight through toNone.The fix
One line: move the normalization into
get_capabilities()itself, right next to the existingnotification_options = notification_options or NotificationOptions()fallback that already does the same job for a sibling parameter. Now every caller gets{}unless it explicitly passes something else, regardless of which path calls it.How I checked it
Ran the exact repro from the issue against both trees:
Added a test in
tests/server/lowlevel/test_server_discover.py(the file that already exercisesget_capabilities()via the discover path) asserting bothcreate_initialization_options()andserver/discoverreportexperimental == {}for the same unconfigured server. It fails onmainwithassert None == {}and passes with this change.ruff format/ruff check/pyrightare all clean. Ran the fulltests/server/suite (1213 tests) and it passes. I didn't run the complete coverage/strict-no-cover gate across the whole repo, but confirmed the new line is exercised (not incoverage report's missing-lines list) when running the affected test files.Type
🐛 Bug Fix
Disclosure
An AI coding agent helped me write this. I read the SDK's fallback pattern in
get_capabilities(), picked the one-line fix that matches it, reproduced the issue's exact repro on both trees myself, and can walk through any part of this change.