From 9479f50c8701659d9ddf463b9e6c1f6a8a3661cf Mon Sep 17 00:00:00 2001 From: fahimreza-dev Date: Tue, 25 Aug 2026 13:53:07 +0600 Subject: [PATCH] fix(mcp): answer a notification 202 on whichever transport served it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- includes/class-saddle-mcp.php | 79 ++++++++++ languages/saddle.pot | 32 ++-- saddle.php | 6 + tests/mcp-notification-normalize-test.php | 173 ++++++++++++++++++++++ 4 files changed, 274 insertions(+), 16 deletions(-) create mode 100644 tests/mcp-notification-normalize-test.php diff --git a/includes/class-saddle-mcp.php b/includes/class-saddle-mcp.php index 6ae3bc3..ca62ed5 100644 --- a/includes/class-saddle-mcp.php +++ b/includes/class-saddle-mcp.php @@ -300,10 +300,89 @@ public static function register_routes() { ), ) ); + } + /** + * Make the notification acknowledgement true whichever transport answered. + * + * THE FIX FOR #155, and the reason it is separate from register_routes(): + * that method runs only when Saddle serves MCP itself. When the official + * MCP Adapter plugin is installed, saddle.php hands the whole request to + * it, and the answer to `notifications/initialized` is then that plugin's + * code at whatever version the site has — not ours. A tester on Codex hit + * exactly that: 200 with an empty body, then `EOF while parsing a value`, + * which is what a strict client does when it tries to JSON-parse nothing. + * + * Saddle should not depend on a third-party plugin's spec compliance for + * the one step that sits between "connected" and `tools/list`. Both filters + * are our own and neither touches the vendored library. They are safe on + * both paths because the adapter registers under Saddle's OWN namespace and + * route ({@see self::register_adapter_server()}), so owns_route() matches + * either way — and everything here is scoped to that route. + */ + public static function register_spec_guards() { + // Before WP_REST_Server sets the status from the response object + // (rest_post_dispatch fires first), so correcting it here is what + // actually reaches the wire. + add_filter( 'rest_post_dispatch', array( __CLASS__, 'normalize_notification_status' ), 10, 3 ); add_filter( 'rest_pre_serve_request', array( __CLASS__, 'serve_empty_acknowledgement' ), 10, 3 ); } + /** + * Force 202 on a JSON-RPC notification aimed at Saddle's MCP route. + * + * Narrow twice over: our route only, and only when the body really is a + * notification — no `id`, and a `notifications/` method. A batch qualifies + * only if EVERY member does, because one real call in it expects a real + * response. An unparseable body is left alone: that is a 400 the transport + * already answered correctly, and guessing at it would hide the error. + * + * @param WP_HTTP_Response $response The dispatched response. + * @param mixed $server The REST server (unused). + * @param WP_REST_Request $request The request. + * @return mixed + */ + public static function normalize_notification_status( $response, $server = null, $request = null ) { + if ( ! $response instanceof WP_HTTP_Response || ! $request instanceof WP_REST_Request ) { + return $response; + } + + if ( ! self::owns_route( $request ) || ! self::is_notification_request( $request ) ) { + return $response; + } + + $response->set_status( 202 ); + return $response; + } + + /** + * Whether a request body is a JSON-RPC notification, or a batch of nothing + * but notifications. + * + * @param WP_REST_Request $request The request. + * @return bool + */ + private static function is_notification_request( WP_REST_Request $request ) { + $body = json_decode( (string) $request->get_body(), true ); + if ( ! is_array( $body ) || array() === $body ) { + return false; + } + + $messages = isset( $body['jsonrpc'] ) || isset( $body['method'] ) ? array( $body ) : $body; + + foreach ( $messages as $message ) { + if ( ! is_array( $message ) ) { + return false; + } + $method = isset( $message['method'] ) ? (string) $message['method'] : ''; + if ( isset( $message['id'] ) || 0 !== strpos( $method, 'notifications/' ) ) { + return false; + } + } + + return true; + } + /** * Answer GET and DELETE with the 405 the spec asks for. * diff --git a/languages/saddle.pot b/languages/saddle.pot index edec6ca..7495c65 100644 --- a/languages/saddle.pot +++ b/languages/saddle.pot @@ -9,7 +9,7 @@ msgstr "" "MIME-Version: 1.0\n" "Content-Type: text/plain; charset=UTF-8\n" "Content-Transfer-Encoding: 8bit\n" -"POT-Creation-Date: 2026-08-25T04:45:33+00:00\n" +"POT-Creation-Date: 2026-08-25T07:53:05+00:00\n" "PO-Revision-Date: YEAR-MO-DA HO:MI+ZONE\n" "X-Generator: WP-CLI 2.12.0\n" "X-Domain: saddle\n" @@ -2117,7 +2117,7 @@ msgid "Saddle sign-in keys cannot be used over XML-RPC." msgstr "" #: includes/class-saddle-connection.php:386 -#: includes/class-saddle-mcp.php:399 +#: includes/class-saddle-mcp.php:478 msgid "Your sign-in key was rejected — it was most likely revoked or removed. Reconnect the app from Saddle to issue a fresh key." msgstr "" @@ -2500,65 +2500,65 @@ msgstr "" msgid "Tiered, default-safe, approval-gated MCP access to posts, pages, and media." msgstr "" -#: includes/class-saddle-mcp.php:336 +#: includes/class-saddle-mcp.php:415 msgid "This MCP endpoint does not offer an event stream. Send JSON-RPC over POST instead." msgstr "" -#: includes/class-saddle-mcp.php:337 +#: includes/class-saddle-mcp.php:416 msgid "This MCP endpoint is stateless and issues no session to terminate." msgstr "" -#: includes/class-saddle-mcp.php:388 +#: includes/class-saddle-mcp.php:467 msgid "The access token was rejected — it has expired, been revoked, or was issued for a different site. Refresh it, or reconnect the app to authorize a new one." msgstr "" -#: includes/class-saddle-mcp.php:409 +#: includes/class-saddle-mcp.php:488 msgid "The request arrived without a sign-in key. If you did connect one, your host may be stripping the Authorization header — open Saddle and run the connection check to confirm and fix it." msgstr "" -#: includes/class-saddle-mcp.php:442 +#: includes/class-saddle-mcp.php:521 msgid "Parse error: request body is not valid JSON-RPC." msgstr "" -#: includes/class-saddle-mcp.php:451 +#: includes/class-saddle-mcp.php:530 msgid "Invalid request." msgstr "" #. translators: 1: protocol version the client asked for, 2: comma-separated list of supported versions. -#: includes/class-saddle-mcp.php:601 +#: includes/class-saddle-mcp.php:680 #, php-format msgid "Unsupported MCP protocol version \"%1$s\". This server speaks %2$s." msgstr "" #. translators: %s: JSON-RPC method name. #. translators: %s: method name -#: includes/class-saddle-mcp.php:679 +#: includes/class-saddle-mcp.php:758 #: includes/lib/wp-mcp/includes/Infrastructure/ErrorHandling/McpErrorFactory.php:117 #, php-format msgid "Method not found: %s" msgstr "" -#: includes/class-saddle-mcp.php:738 +#: includes/class-saddle-mcp.php:817 msgid "Saddle is paused on this site, so no tools will run. The site owner can resume it in Saddle → Settings. Do not retry until they do." msgstr "" -#: includes/class-saddle-mcp.php:750 +#: includes/class-saddle-mcp.php:829 msgid "Saddle exposes tiered, approval-gated access to this site's posts, pages, and media. Call saddle/get-instructions for the current scope and safety rules before acting." msgstr "" -#: includes/class-saddle-mcp.php:943 +#: includes/class-saddle-mcp.php:1022 msgid "Invalid params: a Saddle tool name is required." msgstr "" #. translators: %s: tool name. #. translators: %s: tool name -#: includes/class-saddle-mcp.php:953 +#: includes/class-saddle-mcp.php:1032 #: includes/lib/wp-mcp/includes/Infrastructure/ErrorHandling/McpErrorFactory.php:246 #, php-format msgid "Tool not found: %s" msgstr "" -#: includes/class-saddle-mcp.php:983 +#: includes/class-saddle-mcp.php:1062 msgid "None of Saddle’s site-wide gates (pause, access level, per-tool toggles) blocked this — the connected WordPress account likely lacks a capability this tool requires, or the specific item is protected. Do not retry the same call." msgstr "" @@ -4290,7 +4290,7 @@ msgid "This persisted attribute does nothing — rewrite it on a path the block msgstr "" #. translators: %s: minimum WordPress version. -#: saddle.php:345 +#: saddle.php:351 #, php-format msgid "Saddle requires WordPress %s or later (the core Abilities API). The MCP server is disabled until you upgrade." msgstr "" diff --git a/saddle.php b/saddle.php index 3f4766e..4741bc4 100644 --- a/saddle.php +++ b/saddle.php @@ -284,6 +284,12 @@ public static function setup_mcp_transport() { // the build most likely to need it is the one without the adapter. Saddle_MCP_Diagnostics::register(); + // Spec conformance Saddle owns on BOTH transports: a JSON-RPC + // notification on our route must be answered 202 with no body, and on + // the adapter path that answer would otherwise be the MCP Adapter + // plugin's, at whatever version the site happens to have (#155). + add_action( 'rest_api_init', array( 'Saddle_MCP', 'register_spec_guards' ) ); + if ( self::adapter_available() ) { // Third-party hook, owned by the MCP Adapter plugin — its name is // theirs, and this is the documented way to register a server with diff --git a/tests/mcp-notification-normalize-test.php b/tests/mcp-notification-normalize-test.php new file mode 100644 index 0000000..bd5cf1a --- /dev/null +++ b/tests/mcp-notification-normalize-test.php @@ -0,0 +1,173 @@ +set_header( 'content-type', 'application/json' ); + $req->set_body( wp_json_encode( $body ) ); + return $req; + } + + /** + * Stands in for a transport that answered the notification wrongly — the + * adapter's job on that path, and the exact shape the tester reported. + */ + private function dispatched( WP_REST_Request $request, $status = 200, $data = null ) { + return apply_filters( + 'rest_post_dispatch', + new WP_REST_Response( $data, $status ), + rest_get_server(), + $request + ); + } + + /* -------- the bug -------- */ + + public function test_a_notification_answered_200_is_corrected_to_202() { + $request = $this->request( + array( + 'jsonrpc' => '2.0', + 'method' => 'notifications/initialized', + ) + ); + $response = $this->dispatched( $request, 200 ); + + $this->assertSame( + 202, + $response->get_status(), + 'A notification must leave Saddle as 202 whichever transport answered it.' + ); + } + + public function test_an_all_notification_batch_answered_200_is_corrected() { + $request = $this->request( + array( + array( 'jsonrpc' => '2.0', 'method' => 'notifications/initialized' ), + array( 'jsonrpc' => '2.0', 'method' => 'notifications/cancelled' ), + ) + ); + $response = $this->dispatched( $request, 200 ); + + $this->assertSame( 202, $response->get_status() ); + } + + public function test_the_corrected_202_carries_no_body() { + $request = $this->request( + array( + 'jsonrpc' => '2.0', + 'method' => 'notifications/initialized', + ) + ); + $response = $this->dispatched( $request, 200 ); + + $served = apply_filters( 'rest_pre_serve_request', false, $response, $request, rest_get_server() ); + + $this->assertTrue( + $served, + 'The acknowledgement must be reported as already served, so WordPress prints nothing.' + ); + } + + /* -------- and nothing else moves -------- */ + + public function test_a_real_request_on_our_route_is_left_alone() { + $request = $this->request( + array( + 'jsonrpc' => '2.0', + 'id' => 1, + 'method' => 'tools/list', + ) + ); + $response = $this->dispatched( $request, 200, array( 'result' => array() ) ); + + $this->assertSame( 200, $response->get_status(), 'A call with an id is not a notification.' ); + $this->assertNotNull( $response->get_data(), 'A real response keeps its envelope.' ); + } + + public function test_a_batch_containing_a_real_call_is_left_alone() { + $request = $this->request( + array( + array( 'jsonrpc' => '2.0', 'method' => 'notifications/initialized' ), + array( 'jsonrpc' => '2.0', 'id' => 1, 'method' => 'tools/list' ), + ) + ); + $response = $this->dispatched( $request, 200 ); + + $this->assertSame( + 200, + $response->get_status(), + 'A batch that contains one real call expects a real response.' + ); + } + + public function test_a_notification_shaped_body_on_someone_elses_route_is_left_alone() { + $request = $this->request( + array( + 'jsonrpc' => '2.0', + 'method' => 'notifications/initialized', + ), + '/wp/v2/posts' + ); + $response = $this->dispatched( $request, 200 ); + + $this->assertSame( + 200, + $response->get_status(), + 'Saddle must only ever touch its own MCP route.' + ); + } + + public function test_a_non_json_body_on_our_route_is_left_alone() { + $req = new WP_REST_Request( 'POST', '/saddle/v1/mcp' ); + $req->set_header( 'content-type', 'application/json' ); + $req->set_body( 'not json at all' ); + + $response = $this->dispatched( $req, 400 ); + + $this->assertSame( 400, $response->get_status(), 'An unparseable body keeps its error status.' ); + } + + /** + * The guard that stops this emptying the next response in the same + * process — the failure mode the own-transport version was careful about. + */ + public function test_a_non_202_response_is_never_emptied() { + $request = $this->request( array( 'jsonrpc' => '2.0', 'id' => 1, 'method' => 'ping' ) ); + $response = $this->dispatched( $request, 200, array( 'result' => 'pong' ) ); + + $served = apply_filters( 'rest_pre_serve_request', false, $response, $request, rest_get_server() ); + + $this->assertFalse( $served, 'Only the 202 acknowledgement may be short-circuited.' ); + } +}