fix: implement SessionWithClientInfo for streamableHttpSession #640
Conversation
- Add clientInfo and clientCapabilities fields to streamableHttpSession struct - Implement GetClientInfo, SetClientInfo, GetClientCapabilities, SetClientCapabilities methods - Add SessionWithClientInfo interface declaration - Enable client capability checking for StreamableHTTP transport in stateful mode Fixes mark3labs#639
WalkthroughAdded client information and capabilities storage to Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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. Comment |
![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 (1)
server/streamable_http_client_info_test.go (1)
9-60: LGTM! Comprehensive test coverage.The test properly verifies that
streamableHttpSessionimplementsSessionWithClientInfo, checks initial empty values, and validates the Set/Get roundtrips for both client info and capabilities.As per coding guidelines, consider using
testify/assertfor more concise assertions:+import ( + "github.com/stretchr/testify/assert" +) func TestStreamableHttpSessionImplementsSessionWithClientInfo(t *testing.T) { // Create the session stores toolStore := newSessionToolsStore() resourceStore := newSessionResourcesStore() templatesStore := newSessionResourceTemplatesStore() logStore := newSessionLogLevelsStore() // Create a streamable HTTP session session := newStreamableHttpSession("test-session", toolStore, resourceStore, templatesStore, logStore) // Verify it implements SessionWithClientInfo var clientSession ClientSession = session clientInfoSession, ok := clientSession.(SessionWithClientInfo) - if !ok { - t.Fatal("streamableHttpSession should implement SessionWithClientInfo") - } + assert.True(t, ok, "streamableHttpSession should implement SessionWithClientInfo") // Test GetClientInfo with no data set (should return empty) clientInfo := clientInfoSession.GetClientInfo() - if clientInfo.Name != "" || clientInfo.Version != "" { - t.Errorf("expected empty client info, got %+v", clientInfo) - } + assert.Empty(t, clientInfo.Name) + assert.Empty(t, clientInfo.Version) // Test SetClientInfo and GetClientInfo expectedClientInfo := mcp.Implementation{ Name: "test-client", Version: "1.0.0", } clientInfoSession.SetClientInfo(expectedClientInfo) actualClientInfo := clientInfoSession.GetClientInfo() - if actualClientInfo.Name != expectedClientInfo.Name || actualClientInfo.Version != expectedClientInfo.Version { - t.Errorf("expected client info %+v, got %+v", expectedClientInfo, actualClientInfo) - } + assert.Equal(t, expectedClientInfo.Name, actualClientInfo.Name) + assert.Equal(t, expectedClientInfo.Version, actualClientInfo.Version) // Test GetClientCapabilities with no data set (should return empty) capabilities := clientInfoSession.GetClientCapabilities() - if capabilities.Sampling != nil || capabilities.Roots != nil { - t.Errorf("expected empty client capabilities, got %+v", capabilities) - } + assert.Nil(t, capabilities.Sampling) + assert.Nil(t, capabilities.Roots) // Test SetClientCapabilities and GetClientCapabilities expectedCapabilities := mcp.ClientCapabilities{ Sampling: &struct{}{}, } clientInfoSession.SetClientCapabilities(expectedCapabilities) actualCapabilities := clientInfoSession.GetClientCapabilities() - if actualCapabilities.Sampling == nil { - t.Errorf("expected sampling capability to be set") - } + assert.NotNil(t, actualCapabilities.Sampling) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/streamable_http.go(2 hunks)server/streamable_http_client_info_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Order imports: standard library first, then third-party, then local packages (goimports enforces this)
Follow Go naming conventions: exported identifiers in PascalCase; unexported in camelCase; acronyms uppercase (HTTP, JSON, MCP)
Error handling: return sentinel errors, wrap with fmt.Errorf("context: %w", err), and check with errors.Is/As
Prefer explicit types and strongly-typed structs; avoid using any except where protocol flexibility is required (e.g., Arguments any)
All exported types and functions must have GoDoc comments starting with the identifier name; avoid inline comments unless necessary
Functions that are handlers or long-running must accept context.Context as the first parameter
Ensure thread safety for shared state using sync.Mutex and document thread-safety requirements in comments
For JSON: use json struct tags with omitempty for optional fields; use json.RawMessage for flexible/deferred parsing
Files:
server/streamable_http_client_info_test.goserver/streamable_http.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Testing: use testify/assert and testify/require
Write table-driven tests using a tests := []struct{ name, ... } pattern
Go test files must end with _test.go
Files:
server/streamable_http_client_info_test.go
🧠 Learnings (2)
📓 Common learnings
Learnt from: ezynda3
Repo: mark3labs/mcp-go 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.
📚 Learning: 2025-06-30T07:13:17.052Z
Learnt from: ezynda3
Repo: mark3labs/mcp-go 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.
Applied to files:
server/streamable_http_client_info_test.go
🧬 Code graph analysis (2)
server/streamable_http_client_info_test.go (2)
server/session.go (2)
ClientSession(12-21)SessionWithClientInfo(66-76)mcp/types.go (2)
Implementation(531-535)ClientCapabilities(482-494)
server/streamable_http.go (2)
mcp/types.go (2)
Implementation(531-535)ClientCapabilities(482-494)server/session.go (1)
SessionWithClientInfo(66-76)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: coverage
🔇 Additional comments (3)
server/streamable_http.go (3)
979-980: LGTM! Thread-safe storage for client metadata.The use of
atomic.ValueforclientInfoandclientCapabilitiesensures thread-safe access across concurrent requests, which is essential for session data in HTTP transport handlers.
1058-1082: LGTM! Correct implementation of SessionWithClientInfo methods.The getter methods properly handle uninitialized state by returning empty structs, and the use of type assertions provides defensive programming against potential type mismatches. The setter methods correctly use
atomic.Value.Store()for thread-safe updates.
1089-1089: LGTM! Compile-time interface validation.The interface assertion ensures that
streamableHttpSessioncorrectly implementsSessionWithClientInfoat compile time, preventing accidental breakage of the interface contract.
2 participants
Description
This PR implements the missing
SessionWithClientInfointerface forstreamableHttpSession, enabling servers using StreamableHTTP transport to check client capabilities before performing operations like sampling.The
streamableHttpSessionstruct was missing the implementation of theSessionWithClientInfointerface, creating an inconsistency withsseSessionand preventing servers from accessing client capabilities in stateful mode.Fixes #639
Type of Change
Checklist
MCP Spec Compliance
Additional Information
Problem
The original issue occurred when servers using StreamableHTTP transport with stateful sessions (
WithStateLess(false)) could not access client capabilities that were set during initialization. This prevented proper capability checking before operations likeRequestSampling().Solution
The implementation follows the exact same pattern as
sseSessionfor consistency:atomic.Valuefor thread-safe storageSessionWithClientInfoTesting
This enables the use case described in the original issue: