Drop forked client protocol helpers in favor of the SDK's - #4574
Conversation
Replace FastMCP's copies of _fold_extensions, _evicting_message_handler, and _synthesize_discover with imports from mcp.client.client. The fork had drifted: it was missing validate_extension_identifier, so non-reverse-DNS extension identifiers were silently accepted. Full lifecycle composition over mcp.Client stays blocked upstream — mcp.Client hardcodes ClientSession construction (no session_class hook) and forbids reentry.
marvin-context-protocol
Bot
added
client
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa284cc3da
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| _evicting_message_handler, | ||
| _fold_extensions, |
There was a problem hiding this comment.
These imports are rebound by the module-level _fold_extensions and _evicting_message_handler definitions at lines 226 and 295, so all call sites still use FastMCP's forked copies. In particular, Client(extensions=[...]) continues through the local folding function, which never calls validate_extension_identifier, and therefore still accepts identifiers such as "foo" despite this change's intended validation tightening; remove the local helper definitions (and _FoldedExtensions) so the SDK implementations are actually used.
Useful? React with 👍 / 👎.
1 participant
FastMCP's client forked three protocol helpers from the SDK before those helpers had stabilized: extension folding, the evicting message handler, and discover synthesis. They were near-verbatim copies with no FastMCP-specific behavior, and keeping them meant maintaining a private copy of logic the SDK now owns. The fork had already drifted in a way that mattered — FastMCP's copy of extension folding omitted the SEP-2133 identifier check, so an extension identifier without a reverse-DNS prefix, which the SDK rejects, was silently accepted by
fastmcp.Client.This deletes the forks and imports the SDK's versions, closing that validation gap as a side effect. No public API changes and no behavior change beyond the tightened identifier validation, which affects a path no documented usage exercises.
This is the part of the client-composition work (v4 decision D16) that composes cleanly. The larger goal — rebuilding
fastmcp.Clienton the SDK's high-levelmcp.Client— is blocked upstream:mcp.Clientconstructs itsClientSessionat a single hardcoded site with no injection hook, while FastMCP'ssession_classis load-bearing becauseProxyClientsubstitutes a session that skips result validation so a backend's schema violation surfaces at the end client instead of becoming a proxy error. Delegating session construction would silently re-impose that validation on every proxy. Asession_factory=hook onmcp.Client— the same shape as thenotification_bindings=parameter that was added earlier — would unblock the rest.Label: enhancements