Add streamable http client transport #147
Conversation
There was a problem hiding this comment.
Pull Request Overview
Adds a new Streamable HTTP transport implementation for the Model Context Protocol, complete with client extensions and unit tests.
- Introduces
StreamableHttpClientTransportto send JSON-RPC messages over POST and receive via SSE - Provides
HttpClientextension functions for easy transport/client creation - Adds tests and updates dependencies to support mock HTTP interactions
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/client/StreamableHttpClientTransport.kt | Implements the new streamable HTTP transport with SSE and inline-SSE handling |
| src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/client/StreamableHttpMcpKtorClientExtensions.kt | Adds mcpStreamableHttpTransport and mcpStreamableHttp extension functions on HttpClient |
| src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/types.kt | Adds a copy method to JSONRPCResponse |
| src/jvmTest/kotlin/client/StreamableHttpClientTransportTest.kt | Unit tests for the new transport, including error handling and header behaviors |
| gradle/libs.versions.toml | Adds ktor-client-mock dependency for testing |
| api/kotlin-sdk.api | Exposes the new transport and extension APIs in the public SDK |
Comments suppressed due to low confidence (2)
src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/types.kt:249
- [nitpick] The newly added
copyfunction forJSONRPCResponselacks KDoc. Consider adding a brief description explaining its purpose and usage.
public fun copy(
src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/client/StreamableHttpMcpKtorClientExtensions.kt:18
- [nitpick] Consider adding unit tests for the new extension functions
mcpStreamableHttpTransportandmcpStreamableHttpto ensure they configure the transport and client correctly.
public fun HttpClient.mcpStreamableHttpTransport(
| val jsonBody = McpJson.encodeToString(message) | ||
| val response = client.post(url) { | ||
| applyCommonHeaders(this) | ||
| headers.append(HttpHeaders.Accept, "${ContentType.Application.Json}, ${ContentType.Text.EventStream}") |
There was a problem hiding this comment.
This appends the Accept header after applyCommonHeaders, which itself appends an Accept header for JSON. Consider consolidating these into a single header call or using headers.set to avoid duplicate Accept entries.
| headers.append(HttpHeaders.Accept, "${ContentType.Application.Json}, ${ContentType.Text.EventStream}") | |
| headers.set(HttpHeaders.Accept, "${ContentType.Application.Json}, ${ContentType.Text.EventStream}") |
Copilot uses AI. Check for mistakes.
| if (response.contentType() == null && body.isBlank()) return | ||
|
|
||
| val ct = response.contentType()?.toString() ?: "<none>" | ||
| val error = StreamableHttpError(-1, "Unexpected content type: $$ct") |
There was a problem hiding this comment.
The string template uses $$ct which escapes to a literal $ct. To include the actual content type, use $ct instead of $$ct.
| val error = StreamableHttpError(-1, "Unexpected content type: $$ct") | |
| val error = StreamableHttpError(-1, "Unexpected content type: $ct") |
Copilot uses AI. Check for mistakes.
|
|
||
| @Test | ||
| fun testTerminateSession() = runTest { | ||
| // transport.sessionId = "test-session-id" |
There was a problem hiding this comment.
The test for terminateSession skips setting sessionId, so the method returns early and never sends the DELETE request. Uncomment or set transport.sessionId before calling terminateSession().
| // transport.sessionId = "test-session-id" | |
| transport.sessionId = "test-session-id" |
Copilot uses AI. Check for mistakes.
| } | ||
|
|
||
| // @Test | ||
| // fun testStoreSessionId() = runTest { |
There was a problem hiding this comment.
[nitpick] There's a commented-out test (testStoreSessionId). If it's not needed, consider removing it, or if it's a work in progress, add a TODO so it's clear why it's disabled.
| // fun testStoreSessionId() = runTest { | |
| // fun testStoreSessionId() = runTest { | |
| // // TODO: This test is currently disabled. It needs further development or debugging | |
| // // to verify the handling of session IDs. Uncomment and complete the test when ready. |
Copilot uses AI. Check for mistakes.
#79
Motivation and Context
How Has This Been Tested?
Breaking Changes
Types of changes
Checklist
Additional context