fix(mcp): answer the handshake the way strict clients require (#97) - #98
Merged
Conversation
Mark Roach ran the experiment that isolates this. Same endpoint, same site, same credentials: Claude Desktop through mcp-remote discovered the tools and called them against live data, while ChatGPT completed OAuth, was recognised, read resources — and then reported the site "exposes no callable WordPress actions or data access in this session". So the abilities, the tiers, OAuth and the endpoint were all fine. What was broken was the handshake, against a client strict enough to care. Every client performs initialize -> notifications/initialized -> tools/list. The middle step is a notification, and the spec is unambiguous: "If the input is a JSON-RPC response or notification … the server MUST return HTTP status code 202 Accepted with no body." Saddle answered 200 with the JSON literal `null`. A lenient client shrugs and carries on — which is exactly what mcp-remote does, and the entire reason Claude worked. A strict one treats the handshake as unfinished and never asks what tools exist. Three MUSTs were being missed, all in this transport: - notifications now get 202 with a genuinely empty body. WordPress serializes a null body as the four characters `null`, so emptiness takes a deliberate rest_pre_serve_request short-circuit. It is registered once at route registration rather than per-request: a filter added mid-request only unhooks itself if it fires, and a response that never reaches the serving stage would leave it armed to swallow whatever came next. - GET now returns 405 rather than 404, which is the spec's sanctioned way for a server with no SSE stream to say so, and DELETE the same, since this transport is stateless and issues no session to terminate. Both keep the permission callback, so an anonymous probe still gets 401 rather than confirmation that Saddle lives here. - An unsupported MCP-Protocol-Version now gets 400, and the negotiated version is echoed back. A missing header stays fine: the spec says assume 2025-03-26, and every Application Password client in the field omits it. Also: an unparseable body returns 400 instead of 200 — a parse failure was being recorded in the traffic trace as a success — and resources/list, resources/templates/list and prompts/list answer with empty lists rather than Method-not-found. Saddle advertises only the tools capability so a conformant client would not ask, but ChatGPT demonstrably probes all three, and the vendored adapter already answers two of them this way; there is no reason for Saddle's two transports to differ on the same call. This is not only Mark's problem. Since bff1a99 stopped shipping the vendored adapter to WordPress.org, this transport is the ONLY one a .org install will ever have — and Saddle_MCP_Compat, which fixed the same user-visible symptom for the adapter in #80, is registered only when the adapter is present. A note for whoever touches this next, because it cost real time here: leaving GET unregistered LOOKS conformant, because WP_REST_Server::dispatch() answers a method mismatch with 405 — so a test written against dispatch() passes while real clients get 404 rest_no_route. That false negative was caught only by curling a Playground install running the built wporg zip. The suite cannot route to this transport at all (the dev tree carries the adapter, so register_routes() never runs), which is why the handler is unit-tested directly and the routing is verified on the wire. Verified on the wire against dist/saddle built through the wporg channel: initialize 200, notifications/initialized 202 with 0 bytes, tools/list 200 with 66 tools, GET 405, DELETE 405, anonymous GET 401. Adds OAuth-bearer cover for the tier filter merged in 06277d3, whose tests were Application-Password only — the path Mark is actually on, and one whose failure mode would look exactly like this bug. Closes #97 Refs #80 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GkZr73cqaSesHRDG89Yy8S
4 tasks
ifahimreza
added a commit
that referenced
this pull request
Aug 25, 2026
A tester on Codex could not connect: initialize came back 200, then notifications/initialized came back 200 with an empty body, and the client died with "EOF while parsing a value" — which is what a strict client does when it tries to JSON-parse nothing. It is not Saddle's own transport. That returns 202 with no body and has since #97/#98, and core sets the status from the response object at class-wp-rest-server.php:477, before the rest_pre_serve_request that empties the body at :516 — so the 202 is on the wire. That path is fine. It is the other one. saddle.php hands the whole request to the official MCP Adapter plugin when it is installed, and serve_empty_acknowledgement was hooked inside register_routes() — the else branch. So on the adapter path none of Saddle's spec handling ran, and the answer to the one step between "connected" and tools/list came from a plugin we neither ship nor version. Both zips exclude includes/lib/**, so adapter_available() in the field means exactly "the customer installed that plugin". Saddle should not depend on someone else's spec compliance there. Two filters, both ours, neither touching the vendored library: - rest_post_dispatch forces 202 when the request targets Saddle's MCP route and the body really is a notification. It fires at :464, before the status is read at :477, which is why correcting it there is what reaches the wire rather than just the object. - serve_empty_acknowledgement moves out of register_routes() so it is registered on both paths. Safe on both because the adapter registers under Saddle's OWN namespace and route, so owns_route() already matches either way, and everything is scoped to that route. Narrow twice over: our route only, and only a real notification — no id, a notifications/ method, and for a batch only when EVERY member qualifies, because one real call in it expects a real response. An unparseable body is left alone; that is a 400 the transport already got right, and guessing at it would hide the error. Eight tests, five of which are the "nothing else moves" half: a call with an id, a mixed batch, a notification-shaped body on someone else's route, an unparseable body, and a non-202 response never being emptied. Verified red first — the three behaviour tests failed naming the exact symptom. The set_up() re-registration is not ceremony: the guards hook on rest_api_init, which fires once per process because rest_get_server() memoizes, while WP_UnitTestCase restores $wp_filter after every test — so without it only the first test would have them. 645 tests (was 637), 0 lint errors. Closes #155
4 tasks
ifahimreza
added a commit
that referenced
this pull request
Aug 25, 2026
…156) A tester on Codex could not connect: initialize came back 200, then notifications/initialized came back 200 with an empty body, and the client died with "EOF while parsing a value" — which is what a strict client does when it tries to JSON-parse nothing. It is not Saddle's own transport. That returns 202 with no body and has since #97/#98, and core sets the status from the response object at class-wp-rest-server.php:477, before the rest_pre_serve_request that empties the body at :516 — so the 202 is on the wire. That path is fine. It is the other one. saddle.php hands the whole request to the official MCP Adapter plugin when it is installed, and serve_empty_acknowledgement was hooked inside register_routes() — the else branch. So on the adapter path none of Saddle's spec handling ran, and the answer to the one step between "connected" and tools/list came from a plugin we neither ship nor version. Both zips exclude includes/lib/**, so adapter_available() in the field means exactly "the customer installed that plugin". Saddle should not depend on someone else's spec compliance there. Two filters, both ours, neither touching the vendored library: - rest_post_dispatch forces 202 when the request targets Saddle's MCP route and the body really is a notification. It fires at :464, before the status is read at :477, which is why correcting it there is what reaches the wire rather than just the object. - serve_empty_acknowledgement moves out of register_routes() so it is registered on both paths. Safe on both because the adapter registers under Saddle's OWN namespace and route, so owns_route() already matches either way, and everything is scoped to that route. Narrow twice over: our route only, and only a real notification — no id, a notifications/ method, and for a batch only when EVERY member qualifies, because one real call in it expects a real response. An unparseable body is left alone; that is a 400 the transport already got right, and guessing at it would hide the error. Eight tests, five of which are the "nothing else moves" half: a call with an id, a mixed batch, a notification-shaped body on someone else's route, an unparseable body, and a non-202 response never being emptied. Verified red first — the three behaviour tests failed naming the exact symptom. The set_up() re-registration is not ceremony: the guards hook on rest_api_init, which fires once per process because rest_get_server() memoizes, while WP_UnitTestCase restores $wp_filter after every test — so without it only the first test would have them. 645 tests (was 637), 0 lint errors. Closes #155
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #97
Refs #80
What Mark's testing proved
Same endpoint, same site, same credentials, on
staging.kesuk.net:mcp-remotediscovered the tools and called them —saddle-list-pagesreturned 90 pages,saddle-search-contentreturned 135 matches for "Microsoft 365".So abilities, tiers, OAuth and the endpoint were all fine. The handshake was broken, against a client strict enough to care.
Root cause
Every client performs
initialize→notifications/initialized→tools/list. The middle step is a notification, and the spec is unambiguous:Saddle answered 200 with the JSON literal
null. A lenient client shrugs and carries on — exactly whatmcp-remotedoes, and the whole reason Claude worked. A strict one treats the handshake as unfinished and never asks what tools exist.Three MUSTs were being missed, all in this transport.
This is not only Mark's problem
Since
bff1a99stopped shipping the vendored adapter to WordPress.org, this transport is the only one a.orginstall will ever have.Saddle_MCP_Compat— which fixed the same user-visible symptom for the adapter in #80 — is registered only when the adapter is present, correctly, since it exists to work around the adapter's strictness. Nobody had since checked the transport.orgusers actually get.The fixes
202, genuinely empty body. WordPress serializes a null body as the four charactersnull, so emptiness takes a deliberaterest_pre_serve_requestshort-circuit. Registered once at route registration, not per-request: a filter added mid-request only unhooks itself if it fires, and a response that never reaches the serving stage would leave it armed to swallow whatever came next.GET→405(was 404), the spec's sanctioned way for a server with no SSE stream to say so;DELETE→405(was 400), since this transport is stateless and issues no session to terminate. Both keep the permission callback, so an anonymous probe still gets 401 rather than confirmation Saddle lives here.MCP-Protocol-Version→400, negotiated version echoed back. A missing header stays fine — the spec says assume2025-03-26, and every Application Password client in the field omits it.400(was 200 — a parse failure was recorded in the traffic trace as a success).resources/list,resources/templates/list,prompts/list→ empty lists rather than-32601. Saddle advertises only thetoolscapability so a conformant client would not ask, but ChatGPT demonstrably probes all three, and the vendored adapter already answers two of them this way.A warning for whoever touches this next
Leaving
GETunregistered looks conformant, becauseWP_REST_Server::dispatch()answers a method mismatch with 405 — so a test written againstdispatch()passes while real clients get404 rest_no_route. I wrote exactly that test, concluded "already correct, nothing to do", and only caught it by curling a real install.The suite cannot route to this transport at all: the dev tree carries the vendored adapter, so
Saddle_MCP::register_routes()never runs anddispatch()reaches the adapter's route. Hence the handler is unit-tested directly and the routing is verified on the wire. There is a test asserting that assumption still holds, so this note cannot silently go stale.Testing
composer test— 560 tests, 1892 assertions, green (1 pre-existing skip)composer lint— 0 errorsdist/saddlebuilt through the wporg channel (adapter genuinely absent — confirmed: noincludes/lib, no updater), served by WP Playground on PHP 8.2 / WP 6.9:initialize→notifications/initialized→tools/listsequence as one test — the thing that broke, which nothing exercised end to end06277d3, whose tests were Application-Password only. That is the path Mark is on, and its failure mode would look identical to this bugWP_DEBUGonNo CI in this repo, so "green" means the commands above were run locally.
Worth knowing
tools/listis 58KB at the admin tier. The tier filter narrows that to the read surface on a default install, but it is a real number to keep an eye on.If ChatGPT still shows nothing after this, the next step is the Client traffic panel: a
tools/listrow with status 200 and a non-zero count would mean Saddle is conformant and the tools are being dropped inside ChatGPT — a documented class of connector bug.🤖 Generated with Claude Code
https://claude.ai/code/session_01GkZr73cqaSesHRDG89Yy8S