From de593a05af9926046a210337b8b7e252f0ac820e Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Wed, 27 May 2026 00:14:43 +0000 Subject: [PATCH 1/2] Improve tests for mcp http_transport Add two missing test cases to http_transport_test.go: 1. TestBuildHTTPClientWithHeaders_NilTransport: covers the base = http.DefaultTransport fallback branch (line 257) when baseClient.Transport is nil, confirming headers are still injected. 2. TestParseHTTPResult/non-200_status_with_JSON-RPC_body_always_synthesises_HTTP_error: documents that parseJSONRPCResponseWithSSE always synthesises a synthetic -32603 HTTP error for non-200 responses, even when the body contains a valid JSON-RPC error field. This clarifies the interaction between the two functions and exercises the non-200 + valid-JSON-body code path in parseHTTPResult. Also adds TestParseHTTPResult/non-200_status_with_JSON-RPC_body as a behaviour-documentation test showing that the defensive rpcResponse.Error == nil guard in parseHTTPResult is currently unreachable (parseJSONRPCResponseWithSSE always sets Error for non-200 responses). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- internal/mcp/http_transport_test.go | 48 +++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/internal/mcp/http_transport_test.go b/internal/mcp/http_transport_test.go index eac6ffb87..f911dfde2 100644 --- a/internal/mcp/http_transport_test.go +++ b/internal/mcp/http_transport_test.go @@ -1545,4 +1545,52 @@ func TestParseHTTPResult(t *testing.T) { assert.Equal(t, -32601, resp.Error.Code) assert.Equal(t, "Method not found", resp.Error.Message) }) + + t.Run("non-200 status with JSON-RPC body always synthesises HTTP error", func(t *testing.T) { + // parseJSONRPCResponseWithSSE always synthesises an HTTP error for non-200 responses, + // even when the body is a valid JSON-RPC error. The error in the body is ignored and + // replaced with the synthetic HTTP error. This documents the actual behaviour. + result := &httpRequestResult{ + StatusCode: http.StatusInternalServerError, + ResponseBody: []byte(`{"jsonrpc":"2.0","id":4,"error":{"code":-32000,"message":"Server overloaded"}}`), + } + resp, err := parseHTTPResult(result) + require.NoError(t, err) + require.NotNil(t, resp) + require.NotNil(t, resp.Error, "non-200 response must always have an error") + // The synthetic HTTP error is used because parseJSONRPCResponseWithSSE + // replaces the response for all non-200 statuses. + assert.Equal(t, -32603, resp.Error.Code, "synthetic HTTP error code should be -32603") + assert.Contains(t, resp.Error.Message, "500", "synthetic error should include HTTP status") + }) +} + +// TestBuildHTTPClientWithHeaders_NilTransport verifies that when the base client has +// a nil Transport, buildHTTPClientWithHeaders falls back to http.DefaultTransport as +// the inner transport for the injecting round-tripper. +func TestBuildHTTPClientWithHeaders_NilTransport(t *testing.T) { + received := make(map[string]string) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + received["X-Test-Header"] = r.Header.Get("X-Test-Header") + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + // base client has no Transport set (nil) — the wrapper must fall back to + // http.DefaultTransport so real network requests still work. + base := &http.Client{} + injected := buildHTTPClientWithHeaders(base, map[string]string{ + "X-Test-Header": "nil-transport-value", + }) + assert.NotSame(t, base, injected, "non-empty headers should return a new client") + + req, err := http.NewRequestWithContext(context.Background(), "GET", srv.URL, nil) + require.NoError(t, err) + + resp, err := injected.Do(req) + require.NoError(t, err) + resp.Body.Close() + + assert.Equal(t, "nil-transport-value", received["X-Test-Header"], + "header should be injected even when base client Transport is nil") } From 0001dfd4dcb903c666b8355a3243832d9620e7e3 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 27 May 2026 01:06:28 +0000 Subject: [PATCH 2/2] Fix data race and soften "always" wording in http_transport tests --- internal/mcp/http_transport_test.go | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/internal/mcp/http_transport_test.go b/internal/mcp/http_transport_test.go index f911dfde2..9a1563dd0 100644 --- a/internal/mcp/http_transport_test.go +++ b/internal/mcp/http_transport_test.go @@ -1546,10 +1546,12 @@ func TestParseHTTPResult(t *testing.T) { assert.Equal(t, "Method not found", resp.Error.Message) }) - t.Run("non-200 status with JSON-RPC body always synthesises HTTP error", func(t *testing.T) { - // parseJSONRPCResponseWithSSE always synthesises an HTTP error for non-200 responses, - // even when the body is a valid JSON-RPC error. The error in the body is ignored and - // replaced with the synthetic HTTP error. This documents the actual behaviour. + t.Run("non-200 status with JSON-RPC body synthesises HTTP error", func(t *testing.T) { + // parseJSONRPCResponseWithSSE synthesises a synthetic HTTP error for non-200 responses. + // In the current implementation this means the JSON-RPC error already present in the body + // is overridden by the synthetic error, so the -32603 code is what callers observe. + // This test documents that current behaviour; if parseJSONRPCResponseWithSSE is changed + // to pass through the body-level error, the assertions below will need updating. result := &httpRequestResult{ StatusCode: http.StatusInternalServerError, ResponseBody: []byte(`{"jsonrpc":"2.0","id":4,"error":{"code":-32000,"message":"Server overloaded"}}`), @@ -1557,9 +1559,8 @@ func TestParseHTTPResult(t *testing.T) { resp, err := parseHTTPResult(result) require.NoError(t, err) require.NotNil(t, resp) - require.NotNil(t, resp.Error, "non-200 response must always have an error") - // The synthetic HTTP error is used because parseJSONRPCResponseWithSSE - // replaces the response for all non-200 statuses. + require.NotNil(t, resp.Error, "non-200 response should have an error set") + // Synthetic HTTP error is produced by parseJSONRPCResponseWithSSE for non-200 statuses. assert.Equal(t, -32603, resp.Error.Code, "synthetic HTTP error code should be -32603") assert.Contains(t, resp.Error.Message, "500", "synthetic error should include HTTP status") }) @@ -1569,9 +1570,11 @@ func TestParseHTTPResult(t *testing.T) { // a nil Transport, buildHTTPClientWithHeaders falls back to http.DefaultTransport as // the inner transport for the injecting round-tripper. func TestBuildHTTPClientWithHeaders_NilTransport(t *testing.T) { - received := make(map[string]string) + // Use a buffered channel to safely pass the observed header value from the + // handler goroutine to the test goroutine without a data race. + receivedHeader := make(chan string, 1) srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - received["X-Test-Header"] = r.Header.Get("X-Test-Header") + receivedHeader <- r.Header.Get("X-Test-Header") w.WriteHeader(http.StatusOK) })) defer srv.Close() @@ -1591,6 +1594,6 @@ func TestBuildHTTPClientWithHeaders_NilTransport(t *testing.T) { require.NoError(t, err) resp.Body.Close() - assert.Equal(t, "nil-transport-value", received["X-Test-Header"], + assert.Equal(t, "nil-transport-value", <-receivedHeader, "header should be injected even when base client Transport is nil") }