Add support for MCP host session management #466
Conversation
|
""" WalkthroughA new option was introduced to allow initializing MCP clients with a pre-existing session ID, marking them as already initialized. Corresponding option functions were added to both the client and transport layers, and conditional logic was implemented to use the session ID when creating clients. A test was added to verify this behavior. Changes
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Suggested labels
Suggested reviewers
✨ Finishing Touches
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
client/client.go (1)
36-41: Consider adding a comment about the unused sessionID parameter.The
sessionIDparameter is not used in this function, which might be confusing to developers. Consider adding a comment explaining that the session ID is managed by the transport layer and this function only marks the client as initialized.-// WithSession assumes a MCP Session has already been initialized +// WithSession assumes a MCP Session has already been initialized. +// The sessionID is managed by the transport layer; this option only marks the client as initialized. func WithSession(sessionID string) ClientOption { return func(c *Client) { c.initialized = true } }client/http_test.go (1)
84-93: Test looks good but consider avoiding direct access to private fields.The test effectively verifies the session initialization feature. However, it directly accesses the private
client.initializedfield. Consider adding a public method to check initialization status or testing this indirectly through behavior.- if client.initialized != true { - t.Fatalf("Client is not initialized") - } + // Test that client behaves as initialized by attempting a non-initialize request + // This would fail if the client wasn't properly initialized + err = client.Ping(context.Background()) + // Note: This would require the client to be started firstAlternatively, consider adding a public method like
IsInitialized()to the Client struct for better testability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
client/client.go(1 hunks)client/http.go(1 hunks)client/http_test.go(2 hunks)client/transport/streamable_http.go(6 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: ezynda3
PR: mark3labs/mcp-go#461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:16.959Z
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: mark3labs/mcp-go#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/http_test.go (3)
Learnt from: octo
PR: mark3labs/mcp-go#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: mark3labs/mcp-go#461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:16.959Z
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: floatingIce91
PR: mark3labs/mcp-go#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/streamable_http.go (1)
Learnt from: octo
PR: mark3labs/mcp-go#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.
🧬 Code Graph Analysis (4)
client/http_test.go (2)
client/http.go (1)
NewStreamableHttpClient(11-22)client/transport/streamable_http.go (1)
WithSession(78-82)
client/client.go (2)
client/transport/streamable_http.go (1)
WithSession(78-82)client/transport/sse.go (1)
ClientOption(45-45)
client/http.go (2)
client/client.go (3)
ClientOption(27-27)WithSession(37-41)NewClient(51-61)client/transport/streamable_http.go (1)
WithSession(78-82)
client/transport/streamable_http.go (2)
client/client.go (1)
WithSession(37-41)mcp/types.go (1)
MethodInitialize(19-19)
🔇 Additional comments (4)
client/http.go (1)
16-21: LGTM! Clean integration of session handling.The implementation correctly integrates session handling between transport and client layers. The conditional logic ensures the session option is only applied when a session ID is present, which is the expected behavior.
client/transport/streamable_http.go (3)
77-82: LGTM! Clean implementation of session configuration.The
WithSessionfunction properly stores the session ID using atomic operations, which is thread-safe and consistent with the rest of the codebase.
573-574: Good change making error constants public.Making these error constants public enables better error handling by external callers who can now use
errors.Is()to check for specific error conditions.
246-246: All error constant references are consistentI ran a scan across client/transport/streamable_http.go and confirmed that only the public constants
ErrSessionTerminatedandErrGetMethodNotAllowedare used—there are no remaining references to the old lowercase names.
| func WithSession(sessionID string) StreamableHTTPCOption { | ||
| return func(sc *StreamableHTTP) { | ||
| sc.sessionID.Store(sessionID) | ||
| } | ||
| } |
There was a problem hiding this comment.
New constructor option to provide an existing session
| ErrSessionTerminated = fmt.Errorf("session terminated (404). need to re-initialize") | ||
| ErrGetMethodNotAllowed = fmt.Errorf("GET method not allowed") |
There was a problem hiding this comment.
These need to be public to allow MCP hosts to know when the errors are returned, that way a MCP host can re-initialize on a 404
Sounds good, just pushed up your suggestions |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
www/docs/pages/clients/transports.mdx (2)
398-399: Placeholder renders the snippet uncompilable – switch to a real or dummy value
sessionID := // fetch existing session IDis not valid Go syntax and will not evengo vet.
Either comment the entire line or assign a dummy literal / declared variable.- sessionID := // fetch existing session ID + // TODO: fetch existing session ID from persistent storage + var sessionID string
408-413: Re-initialisation path drops error handlingIf
Initializefails a second time (e.g. network outage) the caller silently proceeds with an empty error and an outdatedsessionID. Always bubble the error (or log / return).- c.Initialize(ctx) // Reinitialize if session is terminated + if err := c.Initialize(ctx); err != nil { + log.Fatalf("failed to re-initialise session: %v", err) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
client/client.go(2 hunks)client/http_test.go(2 hunks)www/docs/pages/clients/transports.mdx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- client/http_test.go
- client/client.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:16.959Z
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: mark3labs/mcp-go#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.
www/docs/pages/clients/transports.mdx (1)
Learnt from: leavez
PR: mark3labs/mcp-go#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.
Description
Adds support for a MCP host to provide a stored session for the streamable http transport
Fixes #465
Type of Change
Checklist
MCP Spec Compliance
Additional Information
Summary by CodeRabbit
New Features
Bug Fixes
Tests