feat: implement protocol version negotiation #502
Conversation
Implement protocol version negotiation following the TypeScript SDK approach: - Update LATEST_PROTOCOL_VERSION to 2025-06-18 - Add client-side validation of server protocol version - Return UnsupportedProtocolVersionError for incompatible versions - Add Mcp-Protocol-Version header support for HTTP transports - Implement SetProtocolVersion method on HTTP connections - Add comprehensive tests for protocol negotiation This ensures both client and server agree on a mutually supported protocol version, preventing compatibility issues. 🤖 Generated with [opencode](https://opencode.ai) Co-Authored-By: opencode <[email protected]>
WalkthroughThis change introduces protocol version negotiation to the MCP client and transport layers. The client now validates the server's protocol version during initialization, stores it, and sets it on HTTP-based transports. New error types and constants are added for protocol version handling. Corresponding updates are made to tests and header handling in both client and server code. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
mcp/errors.go (1)
15-19: Consider using errors.As for more robust type checking.The current type assertion works, but using
errors.Aswould be more robust for wrapped errors and follows modern Go error handling patterns.-// IsUnsupportedProtocolVersion checks if an error is an UnsupportedProtocolVersionError -func IsUnsupportedProtocolVersion(err error) bool { - _, ok := err.(UnsupportedProtocolVersionError) - return ok -} +// IsUnsupportedProtocolVersion checks if an error is an UnsupportedProtocolVersionError +func IsUnsupportedProtocolVersion(err error) bool { + var target UnsupportedProtocolVersionError + return errors.As(err, &target) +}Don't forget to add
"errors"to the imports.client/protocol_negotiation_test.go (1)
149-182: Consider usingt.Errorffor consistency with error handling practices.The test effectively verifies HTTP transport protocol version setting. However, consider using
t.Errorfinstead oft.Fatalfon line 174 to align with the project's preference for giving callers flexibility in error handling (as noted in retrieved learnings).- if err != nil { - t.Fatalf("unexpected error: %v", err) - } + if err != nil { + t.Errorf("unexpected error: %v", err) + return + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
client/client.go(3 hunks)client/protocol_negotiation_test.go(1 hunks)client/stdio_test.go(1 hunks)client/transport/interface.go(1 hunks)client/transport/sse.go(4 hunks)client/transport/streamable_http.go(3 hunks)mcp/errors.go(1 hunks)mcp/types.go(1 hunks)server/streamable_http.go(1 hunks)server/streamable_http_test.go(6 hunks)testdata/mockstdio_server.go(1 hunks)
🧠 Learnings (9)
testdata/mockstdio_server.go (3)
Learnt from: ezynda3
PR: #461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:17.052Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.
Learnt from: octo
PR: #149
File: mcptest/mcptest.go:0-0
Timestamp: 2025-04-21T21:26:32.945Z
Learning: In the mcptest package, prefer returning errors from helper functions rather than calling t.Fatalf() directly, giving callers flexibility in how to handle errors.
Learnt from: lariel-fernandes
PR: #428
File: www/docs/pages/servers/prompts.mdx:218-234
Timestamp: 2025-06-20T20:39:51.870Z
Learning: In the mcp-go library, the GetPromptParams.Arguments field is of type map[string]string, not map[string]interface{}, so direct string access without type assertions is safe and correct.
client/stdio_test.go (2)
Learnt from: octo
PR: #149
File: mcptest/mcptest.go:0-0
Timestamp: 2025-04-21T21:26:32.945Z
Learning: In the mcptest package, prefer returning errors from helper functions rather than calling t.Fatalf() directly, giving callers flexibility in how to handle errors.
Learnt from: ezynda3
PR: #461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:17.052Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.
server/streamable_http.go (1)
Learnt from: ezynda3
PR: #461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:17.052Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.
mcp/errors.go (3)
Learnt from: octo
PR: #149
File: mcptest/mcptest.go:0-0
Timestamp: 2025-04-21T21:26:32.945Z
Learning: In the mcptest package, prefer returning errors from helper functions rather than calling t.Fatalf() directly, giving callers flexibility in how to handle errors.
Learnt from: ezynda3
PR: #461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:17.052Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.
Learnt from: lariel-fernandes
PR: #428
File: www/docs/pages/servers/prompts.mdx:218-234
Timestamp: 2025-06-20T20:39:51.870Z
Learning: In the mcp-go library, the GetPromptParams.Arguments field is of type map[string]string, not map[string]interface{}, so direct string access without type assertions is safe and correct.
server/streamable_http_test.go (6)
Learnt from: ezynda3
PR: #461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:17.052Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.
Learnt from: octo
PR: #149
File: mcptest/mcptest.go:0-0
Timestamp: 2025-04-21T21:26:32.945Z
Learning: In the mcptest package, prefer returning errors from helper functions rather than calling t.Fatalf() directly, giving callers flexibility in how to handle errors.
Learnt from: lariel-fernandes
PR: #428
File: www/docs/pages/servers/prompts.mdx:218-234
Timestamp: 2025-06-20T20:39:51.870Z
Learning: In the mcp-go library, the GetPromptParams.Arguments field is of type map[string]string, not map[string]interface{}, so direct string access without type assertions is safe and correct.
Learnt from: xinwo
PR: #35
File: mcp/tools.go:0-0
Timestamp: 2025-03-04T07:00:57.111Z
Learning: The Tool struct in the mark3labs/mcp-go project should handle both InputSchema and RawInputSchema consistently between MarshalJSON and UnmarshalJSON methods, even though the tools response from MCP server typically doesn't contain rawInputSchema.
Learnt from: xinwo
PR: #35
File: mcp/tools.go:107-137
Timestamp: 2025-03-04T06:59:43.882Z
Learning: Tool responses from the MCP server shouldn't contain RawInputSchema, which is why the UnmarshalJSON method for the Tool struct is implemented to handle only the structured InputSchema format.
Learnt from: floatingIce91
PR: #401
File: server/server.go:1082-1092
Timestamp: 2025-06-23T11:10:42.948Z
Learning: In Go MCP server, ServerTool.Tool field is only used for tool listing and indexing, not for tool execution or middleware. During handleToolCall, only the Handler field is used, so dynamic tools don't need the Tool field populated.
client/transport/sse.go (1)
Learnt from: leavez
PR: #114
File: client/transport/sse.go:137-179
Timestamp: 2025-04-06T10:07:06.685Z
Learning: The SSE client implementation in the MCP-Go project uses a 30-second timeout for reading SSE events to match the behavior of the original implementation before the transport layer refactoring.
client/client.go (2)
Learnt from: ezynda3
PR: #461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:17.052Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.
Learnt from: leavez
PR: #168
File: client/transport/streamable_http.go:183-186
Timestamp: 2025-04-17T15:58:33.813Z
Learning: In Go's atomic.Value, CompareAndSwap compares values using the == operator (value equality), not pointer identity. For strings, this means comparison by content, not memory address, so casting the loaded interface{} value to a string before comparison is unnecessary when using CompareAndSwap.
mcp/types.go (2)
Learnt from: ezynda3
PR: #461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:17.052Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.
Learnt from: lariel-fernandes
PR: #428
File: www/docs/pages/servers/prompts.mdx:218-234
Timestamp: 2025-06-20T20:39:51.870Z
Learning: In the mcp-go library, the GetPromptParams.Arguments field is of type map[string]string, not map[string]interface{}, so direct string access without type assertions is safe and correct.
client/protocol_negotiation_test.go (2)
Learnt from: octo
PR: #149
File: mcptest/mcptest.go:0-0
Timestamp: 2025-04-21T21:26:32.945Z
Learning: In the mcptest package, prefer returning errors from helper functions rather than calling t.Fatalf() directly, giving callers flexibility in how to handle errors.
Learnt from: ezynda3
PR: #461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:17.052Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.
🧬 Code Graph Analysis (3)
testdata/mockstdio_server.go (1)
mcp/types.go (1)
LATEST_PROTOCOL_VERSION(100-100)
client/stdio_test.go (1)
mcp/types.go (1)
LATEST_PROTOCOL_VERSION(100-100)
client/client.go (3)
mcp/types.go (1)
ValidProtocolVersions(103-107)mcp/errors.go (1)
UnsupportedProtocolVersionError(7-9)client/transport/interface.go (1)
HTTPConnection(52-55)
🧰 Additional context used
🧠 Learnings (9)
testdata/mockstdio_server.go (3)
Learnt from: ezynda3
PR: #461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:17.052Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.
Learnt from: octo
PR: #149
File: mcptest/mcptest.go:0-0
Timestamp: 2025-04-21T21:26:32.945Z
Learning: In the mcptest package, prefer returning errors from helper functions rather than calling t.Fatalf() directly, giving callers flexibility in how to handle errors.
Learnt from: lariel-fernandes
PR: #428
File: www/docs/pages/servers/prompts.mdx:218-234
Timestamp: 2025-06-20T20:39:51.870Z
Learning: In the mcp-go library, the GetPromptParams.Arguments field is of type map[string]string, not map[string]interface{}, so direct string access without type assertions is safe and correct.
client/stdio_test.go (2)
Learnt from: octo
PR: #149
File: mcptest/mcptest.go:0-0
Timestamp: 2025-04-21T21:26:32.945Z
Learning: In the mcptest package, prefer returning errors from helper functions rather than calling t.Fatalf() directly, giving callers flexibility in how to handle errors.
Learnt from: ezynda3
PR: #461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:17.052Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.
server/streamable_http.go (1)
Learnt from: ezynda3
PR: #461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:17.052Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.
mcp/errors.go (3)
Learnt from: octo
PR: #149
File: mcptest/mcptest.go:0-0
Timestamp: 2025-04-21T21:26:32.945Z
Learning: In the mcptest package, prefer returning errors from helper functions rather than calling t.Fatalf() directly, giving callers flexibility in how to handle errors.
Learnt from: ezynda3
PR: #461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:17.052Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.
Learnt from: lariel-fernandes
PR: #428
File: www/docs/pages/servers/prompts.mdx:218-234
Timestamp: 2025-06-20T20:39:51.870Z
Learning: In the mcp-go library, the GetPromptParams.Arguments field is of type map[string]string, not map[string]interface{}, so direct string access without type assertions is safe and correct.
server/streamable_http_test.go (6)
Learnt from: ezynda3
PR: #461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:17.052Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.
Learnt from: octo
PR: #149
File: mcptest/mcptest.go:0-0
Timestamp: 2025-04-21T21:26:32.945Z
Learning: In the mcptest package, prefer returning errors from helper functions rather than calling t.Fatalf() directly, giving callers flexibility in how to handle errors.
Learnt from: lariel-fernandes
PR: #428
File: www/docs/pages/servers/prompts.mdx:218-234
Timestamp: 2025-06-20T20:39:51.870Z
Learning: In the mcp-go library, the GetPromptParams.Arguments field is of type map[string]string, not map[string]interface{}, so direct string access without type assertions is safe and correct.
Learnt from: xinwo
PR: #35
File: mcp/tools.go:0-0
Timestamp: 2025-03-04T07:00:57.111Z
Learning: The Tool struct in the mark3labs/mcp-go project should handle both InputSchema and RawInputSchema consistently between MarshalJSON and UnmarshalJSON methods, even though the tools response from MCP server typically doesn't contain rawInputSchema.
Learnt from: xinwo
PR: #35
File: mcp/tools.go:107-137
Timestamp: 2025-03-04T06:59:43.882Z
Learning: Tool responses from the MCP server shouldn't contain RawInputSchema, which is why the UnmarshalJSON method for the Tool struct is implemented to handle only the structured InputSchema format.
Learnt from: floatingIce91
PR: #401
File: server/server.go:1082-1092
Timestamp: 2025-06-23T11:10:42.948Z
Learning: In Go MCP server, ServerTool.Tool field is only used for tool listing and indexing, not for tool execution or middleware. During handleToolCall, only the Handler field is used, so dynamic tools don't need the Tool field populated.
client/transport/sse.go (1)
Learnt from: leavez
PR: #114
File: client/transport/sse.go:137-179
Timestamp: 2025-04-06T10:07:06.685Z
Learning: The SSE client implementation in the MCP-Go project uses a 30-second timeout for reading SSE events to match the behavior of the original implementation before the transport layer refactoring.
client/client.go (2)
Learnt from: ezynda3
PR: #461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:17.052Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.
Learnt from: leavez
PR: #168
File: client/transport/streamable_http.go:183-186
Timestamp: 2025-04-17T15:58:33.813Z
Learning: In Go's atomic.Value, CompareAndSwap compares values using the == operator (value equality), not pointer identity. For strings, this means comparison by content, not memory address, so casting the loaded interface{} value to a string before comparison is unnecessary when using CompareAndSwap.
mcp/types.go (2)
Learnt from: ezynda3
PR: #461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:17.052Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.
Learnt from: lariel-fernandes
PR: #428
File: www/docs/pages/servers/prompts.mdx:218-234
Timestamp: 2025-06-20T20:39:51.870Z
Learning: In the mcp-go library, the GetPromptParams.Arguments field is of type map[string]string, not map[string]interface{}, so direct string access without type assertions is safe and correct.
client/protocol_negotiation_test.go (2)
Learnt from: octo
PR: #149
File: mcptest/mcptest.go:0-0
Timestamp: 2025-04-21T21:26:32.945Z
Learning: In the mcptest package, prefer returning errors from helper functions rather than calling t.Fatalf() directly, giving callers flexibility in how to handle errors.
Learnt from: ezynda3
PR: #461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:17.052Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.
🧬 Code Graph Analysis (3)
testdata/mockstdio_server.go (1)
mcp/types.go (1)
LATEST_PROTOCOL_VERSION(100-100)
client/stdio_test.go (1)
mcp/types.go (1)
LATEST_PROTOCOL_VERSION(100-100)
client/client.go (3)
mcp/types.go (1)
ValidProtocolVersions(103-107)mcp/errors.go (1)
UnsupportedProtocolVersionError(7-9)client/transport/interface.go (1)
HTTPConnection(52-55)
🔇 Additional comments (29)
client/transport/interface.go (1)
50-55: LGTM! Well-designed interface addition.The
HTTPConnectioninterface follows Go best practices by embedding the existingInterfaceand adding a single, focused method. The documentation clearly explains its purpose for HTTP transports that need protocol version header support.mcp/types.go (1)
100-107: Protocol version update looks good.The update to
LATEST_PROTOCOL_VERSIONand the reordering ofValidProtocolVersionsmaintains backward compatibility while promoting the new version. The latest version is correctly placed first in the slice, which is the expected pattern for version preference.mcp/errors.go (1)
5-13: Well-designed error type.The
UnsupportedProtocolVersionErrorfollows Go error conventions with a descriptiveError()method that includes the problematic version for debugging.testdata/mockstdio_server.go (1)
55-55: Good change to use the constant instead of hardcoded version.Replacing the hardcoded protocol version with
mcp.LATEST_PROTOCOL_VERSIONensures the mock server stays consistent with the actual protocol version and automatically updates when the constant changes.server/streamable_http.go (1)
211-212: Good addition of protocol version header constant.The
headerKeyProtocolVersionconstant follows the established pattern and provides a centralized definition for the protocol version header name, preventing inconsistencies and typos.client/stdio_test.go (1)
96-96: LGTM: Protocol version constant usageGood change to use
mcp.LATEST_PROTOCOL_VERSIONinstead of hardcoded string. This ensures test consistency with the protocol version negotiation implementation.server/streamable_http_test.go (6)
31-31: LGTM: Consistent protocol version usageExcellent systematic replacement of hardcoded protocol version with
mcp.LATEST_PROTOCOL_VERSIONconstant. This ensures test consistency with the protocol negotiation implementation.
148-150: LGTM: Protocol version validation consistencyThe assertion correctly uses the same
mcp.LATEST_PROTOCOL_VERSIONconstant for validation, ensuring tests remain synchronized with protocol version updates.
341-343: LGTM: Consistent protocol version validation in stateless testsProper use of the protocol version constant in stateless mode testing maintains consistency across different test scenarios.
567-567: LGTM: Protocol version constant in custom mux testConsistent application of the protocol version constant across all test scenarios, including custom mux configuration.
587-589: LGTM: Protocol version validation in HTTP handler testThe assertion maintains consistency by using the constant for both request and response validation.
845-895: LGTM: Code formatting consistencyGood formatting fix converting spaces to tabs, aligning with Go conventions and the rest of the file's indentation style.
client/transport/sse.go (4)
40-40: LGTM: Thread-safe protocol version storageGood use of
atomic.Valuefor thread-safe storage of the negotiated protocol version string.
328-333: LGTM: Protocol version header implementationCorrect implementation of protocol version header setting with proper nil and empty string checks before adding the header to requests.
444-447: LGTM: HTTPConnection interface implementationClean implementation of the
SetProtocolVersionmethod that completes thetransport.HTTPConnectioninterface for SSE transport.
471-476: LGTM: Consistent header handling in notificationsExcellent consistency in applying the protocol version header to both requests and notifications using the same logic and header constant.
client/transport/streamable_http.go (4)
106-106: LGTM: Thread-safe protocol version storageProper use of
atomic.Valuefor thread-safe storage of the protocol version, consistent with the SSE transport implementation.
211-214: LGTM: HTTPConnection interface implementationClean and simple implementation of the
SetProtocolVersionmethod to satisfy thetransport.HTTPConnectioninterface.
218-218: LGTM: Consistent header constantGood addition of the protocol version header constant, maintaining consistency with other HTTP transports using the same header name.
347-352: LGTM: Protocol version header in shared HTTP methodExcellent placement of the protocol version header logic in the shared
sendHTTPmethod, ensuring both requests and notifications automatically include the header when a protocol version is set.client/client.go (5)
8-8: LGTM: Import for protocol version validationAppropriate addition of the
slicespackage import for theContainsfunction used in protocol version validation.
26-26: LGTM: Protocol version storageGood addition of the
protocolVersionfield to store the negotiated version string on the client.
181-184: LGTM: Protocol version validation logicExcellent validation implementation using
slices.Containsagainstmcp.ValidProtocolVersionsand returning the appropriateUnsupportedProtocolVersionErrorfor unsupported versions.
186-188: LGTM: Storage of negotiated valuesProper storage of both server capabilities and the negotiated protocol version after successful validation.
190-193: LGTM: Transport integration for HTTP connectionsExcellent design using type assertion to check if the transport implements
HTTPConnectioninterface before setting the protocol version. This allows the negotiated version to be propagated to HTTP-based transports while remaining compatible with other transport types.client/protocol_negotiation_test.go (4)
1-12: LGTM! Clean package structure and imports.The package declaration and imports are well-organized and include all necessary dependencies for the protocol negotiation tests.
14-55: LGTM! Well-structured mock transport implementation.The mock transport properly implements the
transport.Interfacewith appropriate state tracking and response mapping. The error handling for unknown methods is good practice for test isolation.
57-137: Excellent comprehensive test coverage for protocol negotiation.The table-driven test approach covers all critical scenarios including supported versions, unsupported past versions, and future versions. The test properly verifies both error conditions and successful protocol version storage in the client. Good use of
t.Errorfovert.Fatalffor better test flexibility.
139-147: LGTM! Clean extension for HTTP transport testing.Good use of embedding to extend the base mock transport with HTTP-specific functionality. The simple
SetProtocolVersionimplementation effectively tracks calls for test verification.
- Move HTTP header constants to common location to avoid duplication - Add errors.Is interface to UnsupportedProtocolVersionError for better Go error handling - Add comprehensive edge case tests for empty and malformed protocol versions - Ensure consistent header constant usage across client and server packages 🤖 Generated with [opencode](https://opencode.ai) Co-Authored-By: opencode <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
client/protocol_negotiation_test.go (1)
155-157: Consider avoiding direct access to private fields in tests.The test is directly accessing
client.protocolVersionwhich appears to be a private field. This creates tight coupling between the test and implementation details. Consider adding a getter method or using a different approach to verify the stored protocol version.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
client/protocol_negotiation_test.go(1 hunks)client/transport/constants.go(1 hunks)client/transport/sse.go(4 hunks)client/transport/streamable_http.go(5 hunks)mcp/errors.go(1 hunks)server/constants.go(1 hunks)server/session_test.go(1 hunks)server/streamable_http.go(4 hunks)server/streamable_http_test.go(14 hunks)
🧠 Learnings (2)
📓 Common learnings
Learnt from: ezynda3
PR: mark3labs/mcp-go#461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:17.052Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.
client/protocol_negotiation_test.go (2)
Learnt from: octo
PR: #149
File: mcptest/mcptest.go:0-0
Timestamp: 2025-04-21T21:26:32.945Z
Learning: In the mcptest package, prefer returning errors from helper functions rather than calling t.Fatalf() directly, giving callers flexibility in how to handle errors.
Learnt from: ezynda3
PR: #461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:17.052Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.
🧬 Code Graph Analysis (1)
client/protocol_negotiation_test.go (4)
mcp/types.go (9)
JSONRPCRequest(312-317)JSONRPCResponse(326-330)Result(233-237)LATEST_PROTOCOL_VERSION(100-100)InitializeRequest(401-405)Params(164-164)InitializeParams(407-413)Implementation(483-486)ClientCapabilities(442-452)client/transport/interface.go (2)
JSONRPCRequest(57-62)JSONRPCResponse(64-73)client/client.go (1)
NewClient(62-72)mcp/errors.go (2)
IsUnsupportedProtocolVersion(22-25)UnsupportedProtocolVersionError(7-9)
✅ Files skipped from review due to trivial changes (5)
- client/transport/constants.go
- server/streamable_http.go
- server/session_test.go
- server/constants.go
- server/streamable_http_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- client/transport/sse.go
- mcp/errors.go
- client/transport/streamable_http.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ezynda3
PR: mark3labs/mcp-go#461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:17.052Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.
client/protocol_negotiation_test.go (2)
Learnt from: octo
PR: #149
File: mcptest/mcptest.go:0-0
Timestamp: 2025-04-21T21:26:32.945Z
Learning: In the mcptest package, prefer returning errors from helper functions rather than calling t.Fatalf() directly, giving callers flexibility in how to handle errors.
Learnt from: ezynda3
PR: #461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:17.052Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.
🧬 Code Graph Analysis (1)
client/protocol_negotiation_test.go (4)
mcp/types.go (9)
JSONRPCRequest(312-317)JSONRPCResponse(326-330)Result(233-237)LATEST_PROTOCOL_VERSION(100-100)InitializeRequest(401-405)Params(164-164)InitializeParams(407-413)Implementation(483-486)ClientCapabilities(442-452)client/transport/interface.go (2)
JSONRPCRequest(57-62)JSONRPCResponse(64-73)client/client.go (1)
NewClient(62-72)mcp/errors.go (2)
IsUnsupportedProtocolVersion(22-25)UnsupportedProtocolVersionError(7-9)
🔇 Additional comments (4)
client/protocol_negotiation_test.go (4)
14-55: LGTM! Well-structured mock transport implementation.The mock transport provides a clean and flexible way to test protocol negotiation scenarios with its method-based response mapping.
163-171: LGTM! Clean extension of mock transport for HTTP testing.The embedded struct pattern is used correctly to add HTTP-specific functionality.
173-206: LGTM! Test correctly verifies HTTP transport protocol version setting.The test properly validates that the protocol version is propagated to HTTP transports after successful initialization.
208-216: Verify type-only comparison in UnsupportedProtocolVersionError.IsThe
Ismethod in mcp/errors.go (16–19) currently does:
- It asserts only that the target error is of type UnsupportedProtocolVersionError, ignoring the
Versionfield.In client/protocol_negotiation_test.go (208–216), the test expects
err1.Is(err2) // true even if err1.Version != err2.Versionto pass.
Please confirm whether we intend to treat all instances of UnsupportedProtocolVersionError as equal regardless of their
Versionvalues.
- If yes, consider adding a comment or updating documentation to clarify that
Isonly checks the error type.- If not, update the implementation of Is to include a comparison of the
Versionfield so that errors with different versions are not considered the same.
| @@ -194,7 +195,7 @@ func (c *StreamableHTTP) Close() error { | |||
| c.logger.Errorf("failed to create close request: %v", err) | |||
| return | |||
| } | |||
| req.Header.Set(headerKeySessionID, sessionId) | |||
| req.Header.Set(HeaderKeySessionID, sessionId) | |||
There was a problem hiding this comment.
🤔, when client sends a DELETE request to server to terminate a session, should we also include "Mcp-Protocol-Version" header if provided?
Ref 1: sepc
If using HTTP, the client MUST include the MCP-Protocol-Version: HTTP header on all subsequent requests to the MCP server
async terminateSession(): Promise<void> {
if (!this._sessionId) {
return; // No session to terminate
}
try {
const headers = await this._commonHeaders();
const init = {
...this._requestInit,
method: "DELETE",
headers,
//....
private async _commonHeaders(): Promise<Headers> {
//...
if (this._sessionId) {
headers["mcp-session-id"] = this._sessionId;
}
if (this._protocolVersion) {
headers["mcp-protocol-version"] = this._protocolVersion;
}
//...
| @@ -97,12 +97,13 @@ func (t *URITemplate) UnmarshalJSON(data []byte) error { | |||
| type JSONRPCMessage any | |||
|
|
|||
| // LATEST_PROTOCOL_VERSION is the most recent version of the MCP protocol. | |||
| const LATEST_PROTOCOL_VERSION = "2025-03-26" | |||
| const LATEST_PROTOCOL_VERSION = "2025-06-18" | |||
|
|
|||
There was a problem hiding this comment.
Upgrading LATEST_PROTOCOL_VERSION from 2025-03-26 to 2025-06-18 is a breaking change for (s *MCPServer) protocolVersion in server.go.
It would be safer to maintain backwards compatibility
There was a problem hiding this comment.
func (s *MCPServer) protocolVersion(clientProtocolVersion string) string {
// For backwards compatibility, if the server does not receive an MCP-Protocol-Version header,
// and has no other way to identify the version - for example, by relying on the protocol version negotiated
// during initialization - the server SHOULD assume protocol version 2025-03-26
// https://modelcontextprotocol.io/specification/2025-06-18/basic/transports#protocol-version-header
if len(clientProtocolVersion) == 0 {
clientProtocolVersion = "2025-03-26"
}
if slices.Contains(mcp.ValidProtocolVersions, clientProtocolVersion) {
return clientProtocolVersion
}
return mcp.LATEST_PROTOCOL_VERSION
}…rmination As per MCP specification, the MCP-Protocol-Version header must be included on all subsequent requests to the MCP server, including DELETE requests for terminating sessions. 🤖 Generated with [opencode](https://opencode.ai) Co-Authored-By: opencode <[email protected]>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
When the server does not receive an MCP-Protocol-Version header, it should assume protocol version 2025-03-26 for backward compatibility as per the MCP specification. 🤖 Generated with [opencode](https://opencode.ai) Co-Authored-By: opencode <[email protected]>
Tests now expect protocol version 2025-03-26 when no protocol version is provided during initialization, as per MCP specification for backward compatibility. 🤖 Generated with [opencode](https://opencode.ai) Co-Authored-By: opencode <[email protected]>
Summary
Changes
LATEST_PROTOCOL_VERSIONto "2025-06-18"UnsupportedProtocolVersionErrorfor incompatible versionsHTTPConnectioninterface withSetProtocolVersion()methodMotivation
The previous implementation didn't validate protocol versions, which could lead to compatibility issues if client and server use incompatible versions. This PR implements the same robust negotiation strategy as the TypeScript SDK.
Test plan
client/protocol_negotiation_test.gowith comprehensive testsgo test -race ./...)🤖 Generated with opencode
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores