fix(streamable_http): preserve error context for client responses - #822
Open
lawrence3699 wants to merge 1 commit into
Open
fix(streamable_http): preserve error context for client responses#822lawrence3699 wants to merge 1 commit into
lawrence3699 wants to merge 1 commit into
Conversation
|
Connected to Huly®: MCP_G-387 |
Contributor
WalkthroughThis pull request refactors error message handling in the streamable HTTP server's response path. It introduces a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
This PR fixes misleading error labeling in the Streamable HTTP transport by preserving the original pending request method when wrapping client-returned JSON-RPC errors, so elicitation and roots/list failures no longer appear as “sampling error …”.
Changes:
- Store pending request method alongside the per-request response channel (sampling, elicitation/create, roots/list).
- Use the stored method context when wrapping JSON-RPC errors in
handleSamplingResponse. - Add a regression test ensuring error labels match the originating pending request method.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
server/streamable_http.go |
Tracks pending request method + channel and uses it to label client-returned JSON-RPC errors correctly. |
server/streamable_http_sampling_test.go |
Adds test coverage to assert error labels reflect pending request context (sampling vs elicitation vs roots/list). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
892
to
893
| http.Error(w, "No pending sampling request found for the given request ID", http.StatusBadRequest) | ||
| return fmt.Errorf("no pending request found for session %s, request %d", sessionID, response.requestID) |
Comment on lines
+811
to
+816
| // Look up the pending request so we can preserve the original request context | ||
| responseContext, publicMessage, statusCode, err := s.pendingResponseContext(sessionID, requestID) | ||
| if err != nil { | ||
| http.Error(w, publicMessage, statusCode) | ||
| return err | ||
| } |
Comment on lines
+860
to
+863
| responseContextInterface, exists := session.samplingRequests.Load(requestID) | ||
| if !exists { | ||
| return pendingResponseItem{}, "No pending sampling request found for the given request ID", http.StatusBadRequest, fmt.Errorf("no pending request found for session %s, request %d", sessionID, requestID) | ||
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
server/streamable_http.go (1)
914-922: Avoid defaulting unknown pending methods back tosampling.If another pending request method is added later, the default branch will silently mislabel it as a sampling error—the same failure mode this patch fixes. Prefer falling back to the actual method string when available.
♻️ Proposed fallback tweak
func pendingResponseErrorLabel(method mcp.MCPMethod) string { switch method { + case mcp.MethodSamplingCreateMessage: + return "sampling" case mcp.MethodElicitationCreate: return "elicitation" case mcp.MethodListRoots: - return "roots/list" + return string(mcp.MethodListRoots) default: + if method != "" { + return string(method) + } return "sampling" } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/streamable_http.go` around lines 914 - 922, The helper pendingResponseErrorLabel currently maps known mcp.MCPMethod values but returns the hardcoded "sampling" in the default branch, which can mislabel new/unknown methods; update pendingResponseErrorLabel (and its switch over mcp.MCPMethod) so the default branch returns the actual method string (e.g., method.String() or equivalent human-readable representation) instead of "sampling", keeping the existing explicit cases for MethodElicitationCreate and MethodListRoots.server/streamable_http_sampling_test.go (1)
99-115: Use testify assertions in the new test.This test currently uses
t.Fatalf/t.Fatalinstead oftestify/assertandtestify/require, which the repo guideline requires for all test files.♻️ Proposed assertion cleanup
import ( "bytes" "context" "encoding/json" "net/http" "net/http/httptest" "strconv" "strings" "testing" "time" "github.com/mark3labs/mcp-go/mcp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" )err := httpServer.handleSamplingResponse(recorder, req, struct { ID json.RawMessage `json:"id"` Result json.RawMessage `json:"result,omitempty"` Error json.RawMessage `json:"error,omitempty"` Method mcp.MCPMethod `json:"method,omitempty"` }{ ID: []byte(strconv.FormatInt(requestID, 10)), Error: []byte(`{"code":-32601,"message":"Method not found"}`), }) - if err != nil { - t.Fatalf("handleSamplingResponse returned error: %v", err) - } - if recorder.Code != http.StatusAccepted { - t.Fatalf("expected status %d, got %d", http.StatusAccepted, recorder.Code) - } + require.NoError(t, err) + assert.Equal(t, http.StatusAccepted, recorder.Code) select { case requestErr := <-errCh: - if requestErr == nil { - t.Fatal("expected request error, got nil") - } - if requestErr.Error() != tt.expectedError { - t.Fatalf("expected error %q, got %q", tt.expectedError, requestErr.Error()) - } + require.Error(t, requestErr) + assert.EqualError(t, requestErr, tt.expectedError) case <-time.After(time.Second): - t.Fatal("timed out waiting for request error") + require.Fail(t, "timed out waiting for request error") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/streamable_http_sampling_test.go` around lines 99 - 115, Replace t.Fatalf/t.Fatal checks in the handleSamplingResponse test with testify assertions: use require.NoError(t, err) for the initial error check (referencing handleSamplingResponse), require.Equal(t, http.StatusAccepted, recorder.Code) for the status code, then for the errCh select use require.Eventually or assert.Eventually to wait for a value and assert.Error(t, requestErr) and assert.Equal(t, tt.expectedError, requestErr.Error()) (referencing errCh and tt.expectedError); ensure you import "github.com/stretchr/testify/assert" and/or "github.com/stretchr/testify/require" and remove the direct t.Fatal/t.Fatalf calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/streamable_http_sampling_test.go`:
- Around line 99-115: Replace t.Fatalf/t.Fatal checks in the
handleSamplingResponse test with testify assertions: use require.NoError(t, err)
for the initial error check (referencing handleSamplingResponse),
require.Equal(t, http.StatusAccepted, recorder.Code) for the status code, then
for the errCh select use require.Eventually or assert.Eventually to wait for a
value and assert.Error(t, requestErr) and assert.Equal(t, tt.expectedError,
requestErr.Error()) (referencing errCh and tt.expectedError); ensure you import
"github.com/stretchr/testify/assert" and/or
"github.com/stretchr/testify/require" and remove the direct t.Fatal/t.Fatalf
calls.
In `@server/streamable_http.go`:
- Around line 914-922: The helper pendingResponseErrorLabel currently maps known
mcp.MCPMethod values but returns the hardcoded "sampling" in the default branch,
which can mislabel new/unknown methods; update pendingResponseErrorLabel (and
its switch over mcp.MCPMethod) so the default branch returns the actual method
string (e.g., method.String() or equivalent human-readable representation)
instead of "sampling", keeping the existing explicit cases for
MethodElicitationCreate and MethodListRoots.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4e1dc35e-3a10-4f79-8bd7-14cf47d743af
📒 Files selected for processing (2)
server/streamable_http.goserver/streamable_http_sampling_test.go
Contributor
|
@lawrence3699 please have a look at and address the coderabbit comments |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
handleSamplingResponsecurrently wraps every client-returned JSON-RPC error assampling error ..., even when the pending server request waselicitation/createorroots/list.This patch stores the pending request method alongside the response channel and uses that context when wrapping client errors. Sampling keeps its existing
sampling error ...label, while elicitation and roots responses now surface the correct request context.Before:
RequestElicitation:sampling error -32601: Method not foundListRoots:sampling error -32601: Method not foundAfter:
RequestElicitation:elicitation error -32601: Method not foundListRoots:roots/list error -32601: Method not foundFixes #817.
Validation
go test ./server -run TestStreamableHTTPServer_ResponseErrorsUsePendingRequestContext -vgo test ./...go test ./... -raceSummary by CodeRabbit