Detect Pong from MCP Client and skip Session ID validation #539
Conversation
WalkthroughDetects and skip empty ping/pong JSON-RPC responses in StreamableHTTPServer.handlePost by adding isPingResponse and isJSONEmpty helpers; refines isSamplingResponse logic; adds tests ensuring empty or omitted result/error pong responses are not treated as sampling responses. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous 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. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 1
🧹 Nitpick comments (2)
server/streamable_http.go (1)
249-251: LGTM with minor edge case consideration.The updated sampling response detection logic correctly differentiates from ping responses. However, consider handling the edge case where both
ResultandErrorare nil - such messages would bypass both ping and sampling detection.Consider adding explicit validation:
isSamplingResponse := jsonMessage.Method == "" && jsonMessage.ID != nil && - (jsonMessage.Result != nil || jsonMessage.Error != nil) + (jsonMessage.Result != nil || jsonMessage.Error != nil) && + !isPingResponseserver/streamable_http_test.go (1)
897-990: Improve test coverage and assertions.The test structure is good and covers the main scenarios, but could be enhanced:
Missing positive assertions: Tests only verify that sampling errors don't occur, but don't confirm that ping responses are actually handled correctly.
Unrealistic scenario: The third test case with empty error may not represent a realistic ping/pong scenario according to the MCP spec.
Consider these improvements:
+ // First verify a real sampling response still triggers the error + t.Run("Real sampling response should still require session ID", func(t *testing.T) { + samplingResponse := map[string]any{ + "jsonrpc": "2.0", + "id": 126, + "result": map[string]any{"content": "test"}, + } + + resp, err := postJSON(server.URL, samplingResponse) + if err != nil { + t.Fatalf("Failed to send sampling response: %v", err) + } + defer resp.Body.Close() + + if resp.StatusCode == http.StatusOK { + t.Error("Expected sampling response to fail without session ID") + } + })Also consider verifying response content more thoroughly:
if resp.StatusCode != http.StatusOK { t.Errorf("Expected status 200 for pong response, got %d. Body: %s", resp.StatusCode, bodyStr) } + + // Verify response body is empty (as expected for ping responses) + if len(bodyBytes) > 0 { + t.Errorf("Expected empty response body for ping, got: %s", bodyStr) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/streamable_http.go(1 hunks)server/streamable_http_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
server/streamable_http.go (1)
mcp/types.go (1)
Result(245-249)
struckoff
force-pushed
the
hotfix/streamable-pong-response
branch
from
2d6f1a2 to
b8977cd
Compare
Description
Fixes #<issue_number> (if applicable)
Type of Change
Checklist
MCP Spec Compliance
Additional Information
This fix ensures compliance with the MCP ping/pong specification:
Previously, when WithHeartbeatInterval was enabled in the HTTP Streamable MCP server, Cursor AI and Claude Desktop got a 400 error - "Missing session ID for sampling response" - because the
Pongresponse was incorrectly interpreted as aSamplingResponse.Without heartbeat support, Cursor disconnects from MCP after a 5-minute timeout.
Summary by CodeRabbit
Bug Fixes
Tests