From 4935b0bd7ce32495664f5bfaa08a57588976fa94 Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Thu, 5 Jun 2025 01:50:11 +0000 Subject: [PATCH 1/2] Refactor transport layer abstraction This commit addresses concerns regarding the utility of `AbstractTransport` and `TransportInterface`. Key changes: - I removed unused utility methods (`encodeMessage`, `decodeMessage`) and constant (`MAX_MESSAGE_SIZE`) from `AbstractTransport`. - I removed the default `log()` implementation from `AbstractTransport`. - Based on your feedback and the minimal remaining functionality in `AbstractTransport` (only a default `preferSseStream` method), `AbstractTransport.php` has been removed entirely. - `StdioTransport` and `HttpTransport` now directly implement `TransportInterface`. - `StdioTransport` implements `preferSseStream` as a no-op. Its specific `log()` method for STDERR output is now a direct public method, not part of the interface. - `HttpTransport` implements `preferSseStream`. - The `log()` method declaration was removed from `TransportInterface` as it was not called polymorphically by the `Server` and `HttpTransport` did not require it. This simplifies the interface for `HttpTransport`. These changes streamline the transport hierarchy, remove dead code, and clarify the roles of the transport components, while ensuring all existing tests and linting checks pass. --- src/Transport/AbstractTransport.php | 87 ---------------------------- src/Transport/HttpTransport.php | 18 +++++- src/Transport/StdioTransport.php | 13 ++++- src/Transport/TransportInterface.php | 10 ---- 4 files changed, 29 insertions(+), 99 deletions(-) delete mode 100644 src/Transport/AbstractTransport.php diff --git a/src/Transport/AbstractTransport.php b/src/Transport/AbstractTransport.php deleted file mode 100644 index d565cc0..0000000 --- a/src/Transport/AbstractTransport.php +++ /dev/null @@ -1,87 +0,0 @@ -toJson(); - - if (strlen($json) > static::MAX_MESSAGE_SIZE) { - throw new TransportException("Encoded message exceeds size limit of " . static::MAX_MESSAGE_SIZE . " bytes."); - } - - return $json; - } - - /** - * Decodes and validates a received JSON string into a JsonRpcMessage. - * - * @param string $data The raw JSON data received from the transport. - * @return JsonRpcMessage|null The decoded message, or null if decoding fails (e.g., invalid JSON). - * @throws TransportException If the received data string exceeds MAX_MESSAGE_SIZE before decoding. - */ - protected function decodeMessage(string $data): ?JsonRpcMessage - { - if (strlen($data) > static::MAX_MESSAGE_SIZE) { - throw new TransportException("Received data exceeds size limit of " . static::MAX_MESSAGE_SIZE . " bytes."); - } - - try { - return JsonRpcMessage::fromJson($data); - } catch (\Exception $e) { - // Log but don't throw - allow server to handle invalid messages - $this->log("Error parsing received JSON message: " . $e->getMessage() . ". Data: " . substr($data, 0, 200) . "..."); - return null; - } - } - - /** - * Logs a message related to the transport's operation. - * - * This default implementation writes messages to PHP's error_log. - * Subclasses can override this to use a more sophisticated logging mechanism. - * - * @param string $message The message to log. - */ - public function log(string $message): void - { - error_log(get_class($this) . ": " . $message); - } - - /** - * Indicates a preference for using Server-Sent Events (SSE) for streaming responses, if applicable. - * - * Transports that support SSE (like HttpTransport) can use this hint to switch - * their response mode. Other transports can ignore this. - * - * @param bool $prefer True to prefer SSE, false otherwise. - */ - public function preferSseStream(bool $prefer = true): void - { - // Default implementation does nothing, to be overridden by transports that support SSE. - } -} diff --git a/src/Transport/HttpTransport.php b/src/Transport/HttpTransport.php index 98c1945..7a7b21a 100644 --- a/src/Transport/HttpTransport.php +++ b/src/Transport/HttpTransport.php @@ -24,7 +24,7 @@ * Origin checks, complex ACK logic, and related session properties have been removed. * It primarily focuses on basic JSON-RPC via POST. */ -class HttpTransport extends AbstractTransport +class HttpTransport implements TransportInterface { // Kept essential properties for basic POST JSON-RPC. /** @var ResponseInterface|null The PSR-7 response object being prepared. */ @@ -266,4 +266,20 @@ public function getResponse(): ResponseInterface } return $this->response; } + + /** + * Indicates a preference for using Server-Sent Events (SSE) for streaming responses, if applicable. + * + * Transports that support SSE (like HttpTransport) can use this hint to switch + * their response mode. Other transports can ignore this. + * + * @param bool $prefer True to prefer SSE, false otherwise. + */ + public function preferSseStream(bool $prefer = true): void + { + // Default implementation does nothing, to be overridden by transports that support SSE. + // In a real HttpTransport that supports SSE, this method would set a flag + // to change response content-type and formatting in the send() method. + // For this simplified version, it remains a no-op. + } } diff --git a/src/Transport/StdioTransport.php b/src/Transport/StdioTransport.php index 3d30a57..c96a73f 100644 --- a/src/Transport/StdioTransport.php +++ b/src/Transport/StdioTransport.php @@ -13,7 +13,7 @@ * Implements a transport using STDIN, STDOUT, and STDERR for line-based JSON messaging. * Each JSON-RPC message (or batch of messages) is expected to be on a single line. */ -class StdioTransport extends AbstractTransport +class StdioTransport implements TransportInterface { /** @var resource The standard input stream. */ private $stdin; @@ -203,4 +203,15 @@ public function isStreamOpen(): bool { return false; } + + /** + * Indicates a preference for using Server-Sent Events (SSE) for streaming responses, if applicable. + * StdioTransport does not support SSE, so this is a no-op. + * + * @param bool $prefer True to prefer SSE, false otherwise. + */ + public function preferSseStream(bool $prefer = true): void + { + // StdioTransport does not support SSE. + } } diff --git a/src/Transport/TransportInterface.php b/src/Transport/TransportInterface.php index 6cd95fc..b922e9d 100644 --- a/src/Transport/TransportInterface.php +++ b/src/Transport/TransportInterface.php @@ -50,16 +50,6 @@ public function receive(): ?array; */ public function send(JsonRpcMessage|array $message): void; - /** - * Logs a message specific to the transport's operation. - * - * Example: For StdioTransport, this might write to STDERR. - * For HttpTransport, it might use a PSR-3 logger if integrated. - * - * @param string $message The message to log. - */ - public function log(string $message): void; - /** * Checks if the transport connection is considered closed. * From 4dc50dd38a99a24637d063fdd3051c50f4a356a0 Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Thu, 5 Jun 2025 02:13:07 +0000 Subject: [PATCH 2/2] Refactor: Remove unused preferSseStream method from TransportInterface The preferSseStream method was defined in TransportInterface and implemented as a no-op in HttpTransport, StdioTransport, and the MockTransport. Codebase analysis confirmed that this method was not called anywhere, nor was it part of the documented external API in README.md or examples. The HttpTransport currently does not support Server-Sent Events (SSE), and the MCP specification (2025-03-26) makes SSE an optional feature for servers. As the method provided no functionality towards current or planned SSE capabilities and was unused, it has been removed to simplify the interface and reduce potential confusion. This change involved: - Removing the method definition from TransportInterface. - Removing the implementations from HttpTransport, StdioTransport, and MockTransport. - Verifying that all tests and linters pass after the removal. --- src/Transport/HttpTransport.php | 16 ---------------- src/Transport/StdioTransport.php | 11 ----------- src/Transport/TransportInterface.php | 10 ---------- tests/Util/MockTransport.php | 6 ------ 4 files changed, 43 deletions(-) diff --git a/src/Transport/HttpTransport.php b/src/Transport/HttpTransport.php index 7a7b21a..97e1387 100644 --- a/src/Transport/HttpTransport.php +++ b/src/Transport/HttpTransport.php @@ -266,20 +266,4 @@ public function getResponse(): ResponseInterface } return $this->response; } - - /** - * Indicates a preference for using Server-Sent Events (SSE) for streaming responses, if applicable. - * - * Transports that support SSE (like HttpTransport) can use this hint to switch - * their response mode. Other transports can ignore this. - * - * @param bool $prefer True to prefer SSE, false otherwise. - */ - public function preferSseStream(bool $prefer = true): void - { - // Default implementation does nothing, to be overridden by transports that support SSE. - // In a real HttpTransport that supports SSE, this method would set a flag - // to change response content-type and formatting in the send() method. - // For this simplified version, it remains a no-op. - } } diff --git a/src/Transport/StdioTransport.php b/src/Transport/StdioTransport.php index c96a73f..8d07c4d 100644 --- a/src/Transport/StdioTransport.php +++ b/src/Transport/StdioTransport.php @@ -203,15 +203,4 @@ public function isStreamOpen(): bool { return false; } - - /** - * Indicates a preference for using Server-Sent Events (SSE) for streaming responses, if applicable. - * StdioTransport does not support SSE, so this is a no-op. - * - * @param bool $prefer True to prefer SSE, false otherwise. - */ - public function preferSseStream(bool $prefer = true): void - { - // StdioTransport does not support SSE. - } } diff --git a/src/Transport/TransportInterface.php b/src/Transport/TransportInterface.php index b922e9d..696c7ca 100644 --- a/src/Transport/TransportInterface.php +++ b/src/Transport/TransportInterface.php @@ -69,14 +69,4 @@ public function isClosed(): bool; * @return bool True if a stream is actively open, false otherwise. */ public function isStreamOpen(): bool; - - /** - * Hints to the transport that Server-Sent Events (SSE) are preferred for the response stream, if applicable. - * - * Transports that support SSE (e.g., HttpTransport) can use this to modify - * how they format and send responses. Transports that do not support SSE can ignore this hint. - * - * @param bool $prefer True to indicate a preference for SSE streaming, false otherwise. - */ - public function preferSseStream(bool $prefer = true): void; } diff --git a/tests/Util/MockTransport.php b/tests/Util/MockTransport.php index 1543cd2..e54862b 100644 --- a/tests/Util/MockTransport.php +++ b/tests/Util/MockTransport.php @@ -80,12 +80,6 @@ public function log(string $message): void // error_log("MockTransport Log: " . $message); // For debugging tests } - public function preferSseStream(bool $prefer = true): void - { - // No-op for this mock implementation. - // Could store the preference if tests need to assert it was called. - } - /** * Checks if the transport stream is currently open. * For this mock, defaults to false. Can be made configurable if needed.