Add InProcessTransport
#189
Conversation
WalkthroughThis change introduces an in-process transport mechanism for the MCP client-server architecture in Go. A new Changes
✨ 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:
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: 2
🧹 Nitpick comments (4)
client/inprocess.go (1)
8-12: Implementation looks good but consider return type adjustment.The function signature returns an error, but the implementation always returns nil for the error. Consider either:
- Adjusting the return type to just
*Clientsince this function doesn't currently fail, or- Adding a comment explaining why the error is part of the return value (e.g., for future-proofing)
-// NewInProcessClient connect directly to a mcp server object in the same process -func NewInProcessClient(server *server.MCPServer) (*Client, error) { +// NewInProcessClient connects directly to a mcp server object in the same process +func NewInProcessClient(server *server.MCPServer) *Client { inProcessTransport := transport.NewInProcessTransport(server) - return NewClient(inProcessTransport), nil + return NewClient(inProcessTransport) }client/transport/inprocess.go (2)
30-49: Consider optimizing the marshaling/unmarshaling flow.The current implementation marshals the request to JSON, passes it to the server's
HandleMessage, then marshals and unmarshals the response again. This approach works but introduces potential inefficiencies and error points through multiple serialization steps.If
server.MCPServerprovides a method that can accept structured requests directly (or could be extended to do so), consider using that approach to eliminate the redundant marshaling/unmarshaling steps:func (c *InProcessTransport) SendRequest(ctx context.Context, request JSONRPCRequest) (*JSONRPCResponse, error) { - requestBytes, err := json.Marshal(request) - if err != nil { - return nil, fmt.Errorf("failed to marshal request: %w", err) - } - requestBytes = append(requestBytes, '\n') - - respMessage := c.server.HandleMessage(ctx, requestBytes) - respByte, err := json.Marshal(respMessage) - if err != nil { - return nil, fmt.Errorf("failed to marshal response message: %w", err) - } - rpcResp := JSONRPCResponse{} - err = json.Unmarshal(respByte, &rpcResp) - if err != nil { - return nil, fmt.Errorf("failed to unmarshal response message: %w", err) - } - - return &rpcResp, nil + // If server could be extended with a method like HandleStructuredRequest + respMessage, err := c.server.HandleStructuredRequest(ctx, request) + if err != nil { + return nil, fmt.Errorf("failed to handle request: %w", err) + } + + return respMessage, nil }If modifying the server interface isn't possible, the current implementation is correct but less efficient.
51-60: Notification response is discarded without checks.The server's response to the notification is discarded without any verification. While this follows the JSON-RPC specification (notifications don't expect responses), it might be worth adding a comment explaining this behavior to avoid confusion.
func (c *InProcessTransport) SendNotification(ctx context.Context, notification mcp.JSONRPCNotification) error { notificationBytes, err := json.Marshal(notification) if err != nil { return fmt.Errorf("failed to marshal notification: %w", err) } notificationBytes = append(notificationBytes, '\n') - c.server.HandleMessage(ctx, notificationBytes) + // Per JSON-RPC spec, notifications don't expect responses, so we discard the result + _ = c.server.HandleMessage(ctx, notificationBytes) return nil }client/inprocess_test.go (1)
160-406: Consider reducing test duplication with setup helper.Each test repeats similar setup code for creating, starting, and initializing the client. This leads to significant code duplication across the test functions.
Consider refactoring to use a helper function that handles the common setup and returns an initialized client:
+func setupInitializedClient(t *testing.T, mcpServer *server.MCPServer) *Client { + client, err := NewInProcessClient(mcpServer) + if err != nil { + t.Fatalf("Failed to create client: %v", err) + } + + if err := client.Start(context.Background()); err != nil { + t.Fatalf("Failed to start client: %v", err) + } + + // Initialize + initRequest := mcp.InitializeRequest{} + initRequest.Params.ProtocolVersion = mcp.LATEST_PROTOCOL_VERSION + initRequest.Params.ClientInfo = mcp.Implementation{ + Name: "test-client", + Version: "1.0.0", + } + + _, err = client.Initialize(context.Background(), initRequest) + if err != nil { + t.Fatalf("Failed to initialize: %v", err) + } + + return client +}Then use this helper in each test:
t.Run("ListTools", func(t *testing.T) { - client, err := NewInProcessClient(mcpServer) - if err != nil { - t.Fatalf("Failed to create client: %v", err) - } - defer client.Close() - - if err := client.Start(context.Background()); err != nil { - t.Fatalf("Failed to start client: %v", err) - } - - // Initialize - initRequest := mcp.InitializeRequest{} - initRequest.Params.ProtocolVersion = mcp.LATEST_PROTOCOL_VERSION - initRequest.Params.ClientInfo = mcp.Implementation{ - Name: "test-client", - Version: "1.0.0", - } - - _, err = client.Initialize(context.Background(), initRequest) - if err != nil { - t.Fatalf("Failed to initialize: %v", err) - } + client := setupInitializedClient(t, mcpServer) + defer client.Close() request := mcp.ListToolsRequest{} result, err := client.ListTools(context.Background(), request) if err != nil { t.Errorf("ListTools failed: %v", err) } if len(result.Tools) != 1 { t.Errorf("Expected 1 tool, got %d", len(result.Tools)) } })This would significantly reduce code duplication and make the tests more maintainable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
client/inprocess.go(1 hunks)client/inprocess_test.go(1 hunks)client/transport/inprocess.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
client/inprocess.go (2)
client/client.go (2)
Client(16-24)NewClient(34-38)client/transport/inprocess.go (1)
NewInProcessTransport(20-24)
client/transport/inprocess.go (1)
mcp/types.go (1)
JSONRPCNotification(192-195)
🔇 Additional comments (3)
client/inprocess_test.go (3)
11-83: Good test setup with comprehensive server configuration.The test setup is thorough and well-structured, establishing a complete test environment with tools, resources, and prompts. This provides excellent coverage for testing the in-process client's integration with the server.
85-139: Comprehensive initialization and basic functionality test.This test effectively verifies the client initialization and basic functionality, including the verification of tool annotations.
141-158: Good error case testing.The test properly verifies that the client behaves as expected when trying to make requests before initialization. Testing error cases is an important aspect of robust test coverage.
Summary by CodeRabbit
More details in #185