removed stdio transport support#15
Conversation
|
/gemini-review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| # Default fallback for non-HTTP request formats | ||
| { headers: {}, params: {} } | ||
| end | ||
| end |
There was a problem hiding this comment.
The pull request is titled "removed stdio transport support", but the only change present is a modification to a comment. If functionality was indeed removed, the corresponding code changes appear to be missing. Could you please clarify if the title is accurate or if other changes should be included in this PR?
| require_relative "session" | ||
| require_relative "errors" | ||
| require_relative "transport/stdio" # Default transport | ||
| # require_relative "transport/sse" # Load on demand to avoid async dependencies |
There was a problem hiding this comment.
This change removes the require for the stdio transport, which the comment identifies as the default. This is likely a breaking change.
If the code that instantiates or references the stdio transport still exists elsewhere, this will cause a LoadError or NameError at runtime. Is the transport's implementation file and its usage being removed in this PR as well?
For a significant change like this, the pull request description should be updated to explain the motivation and impact.
| # to spin up an MCP-compatible server—including: | ||
| # | ||
| # * **Transport adapters** (synchronous `stdio` or HTTP + SSE) | ||
| # * **Transport adapters** (streamable HTTP and SSE) |
There was a problem hiding this comment.
The phrasing (streamable HTTP and SSE) is slightly ambiguous, as Server-Sent Events (SSE) is itself a streaming technology that runs over HTTP, making the description a bit redundant.
For improved clarity, could you consider one of the following?
If other streaming HTTP methods are supported in addition to SSE:
| # * **Transport adapters** (streamable HTTP and SSE) | |
| # * **Transport adapters** (streaming HTTP, including Server-Sent Events) |
If SSE is the primary streaming mechanism, the previous wording (without stdio) is very clear:
| # * **Transport adapters** (streamable HTTP and SSE) | |
| # * **Transport adapters** (HTTP and SSE) |
| # | ||
| # server.run # => starts the stdio transport and begins processing JSON-RPC messages | ||
| # server.run # => starts the HTTP stream transport and begins processing JSON-RPC messages | ||
| # ``` |
There was a problem hiding this comment.
To improve clarity, could we be more specific about the type of 'HTTP stream transport' used by default? The following line mentions SSE as a production option, which might cause some confusion. If the default is also SSE, it would be great to state that explicitly.
1 participant
No description provided.