Fix StreamableHttpServerTransport review comments

[michaellatman]

Summary

This PR addresses all review comments from PR #87:

• Added comprehensive KDoc documentation for all public APIs
• Improved Accept header validation to use proper matching instead of contains
• Fixed ContentType header key to use HttpHeaders.ContentType
• Corrected parseBody error message for invalid JSON format
• Added unit tests for StreamableHttpServerTransport
• Did NOT add callMapping.clear() in close() method after checking TypeScript SDK

Changes

Documentation

• Added KDoc comments for the class constructor parameters
• Added KDoc comments for the sessionId property
• Added detailed KDoc comments for all public methods

Header Validation

• Changed Accept header validation to properly parse and match media types
• Now correctly handles headers with parameters (e.g., text/event-stream;q=0.9)

Bug Fixes

• Fixed ContentType header to use HttpHeaders.ContentType instead of ContentType.toString()
• Fixed error message in parseBody method from "Server already initialized" to "Body must be a JSON object or array"

CallMapping Handling

• After reviewing the TypeScript SDK implementation, I determined that callMapping should NOT be cleared in close()
• The entries are already properly cleaned up after each response is sent (line 108)
• This matches the TypeScript implementation pattern

Tests

• Added unit tests covering basic functionality (but more probably wanted)
• Tests verify proper initialization, start/close behavior, and configuration options

Disclaimer

Note: I have not yet tried this library with a full application - I just wanted to help kick the stale review from PR #87. I can come back with more thorough testing around June 12 if you need additional help.

Test Results

All tests are passing successfully.

[michaellatman]

Definitely, I’ll need to revisit this. If no one else gets to it, I’ll probably come work on the client project as well.

[Copilot]

Pull Request Overview

This pull request refactors the StreamableHttpServerTransport implementation by addressing review comments, improving API documentation, and refining header validation and error handling.

• Improved KDoc documentation for public APIs
• Enhanced header validation logic and updated error messages
• Added comprehensive unit tests for the transport’s behavior

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
src/jvmTest/kotlin/server/StreamableHttpServerTransportTest.kt	Added unit tests to verify start/close behavior, message and error callbacks, and flag handling
src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/types.kt	Made the response id field nullable to support error responses without valid ids
src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/shared/Constants.kt	Introduced a constant for the MCP session header
src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/StreamableHttpServerTransport.kt	Updated header validation, session management, JSON response handling, and documented API methods

Comments suppressed due to low confidence (2)

src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/StreamableHttpServerTransport.kt:109

• In the non-JSON response branch (lines 109–112), the callMapping entry is not removed after sending a message. Consider also cleaning up the callMapping (e.g., callMapping.remove(streamId)) to avoid potential resource leaks.

        } else {

src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/StreamableHttpServerTransport.kt:288

• [nitpick] Consider either implementing the flush of headers as indicated by the TODO comment or, if deferred intentionally, add a comment clarifying why this behavior is not required to improve clarity for future maintenance.

        // TODO: Equivalent of typescript res.writeHead(200, headers).flushHeaders();

[devcrocod]

Obsolete