Share one JSON-RPC error writer across HTTP denial paths

.claude/rules/go-style.md

@@ -200,3 +200,18 @@ Before implementing any non-trivial functionality from scratch:
 3. **Look for existing Go packages** — search for well-maintained OSS libraries that solve the problem before writing custom implementations.
 
 Implementing from scratch should be a last resort, justified by a specific gap no existing solution fills.
+
+## JSON-RPC Envelopes
+
+Never `json.Marshal` (or `json.NewEncoder().Encode`) a `jsonrpc2` type.
+`jsonrpc2.Response` carries no json tags and `jsonrpc2.ID`'s only field is
+unexported, so reflection emits Go-capitalized keys, drops the mandatory
+`"jsonrpc":"2.0"` tag, and renders every id — valid or not — as `{}` (#5950).
+Serialize through `jsonrpc2.EncodeMessage`; for HTTP error responses use
+`pkg/mcp.EncodeJSONRPCError` / `pkg/mcp.WriteJSONRPCError`, which add
+encode-before-header ordering and a conformant fallback body.
+
+When an envelope must be hand-built as a map (only where no `jsonrpc2.ID` is
+in hand), omit the `"id"` key when the id is absent — never emit `"id":null`.
+MCP's `RequestId` is `string | number`, so null is not representable, and the
+reference TypeScript SDK client throws on it inside its transport (#6038).

pkg/authz/middleware.go

@@ -158,21 +158,10 @@ func handleUnauthorized(w http.ResponseWriter, msgID interface{}, err error) {
 		Error: jsonrpc2.NewError(mcp.JSONRPCCodeDenied, "Unauthorized"),
 	}
 
-	// Encode before writing any header, so a marshal failure never leaves a
-	// half-written response (e.g. a 403 header followed by a second 500 write).
-	body, encErr := jsonrpc2.EncodeMessage(errorResponse)
-	if encErr != nil {
-		// Unreachable in practice: errorResponse is always a well-formed
-		// jsonrpc2.Response built from strings/ints above. Fall back to a
-		// hardcoded valid JSON-RPC error body rather than writing nothing.
-		slog.Error("failed to encode JSON-RPC unauthorized response", "error", encErr)
-		body = fmt.Appendf(nil, `{"jsonrpc":"2.0","error":{"code":%d,"message":"Unauthorized"}}`, mcp.JSONRPCCodeDenied)
-	}
-
-	w.Header().Set("Content-Type", "application/json")
-	w.WriteHeader(http.StatusForbidden)
-	//nolint:gosec // G104: writing the JSON-RPC denial body to an HTTP client; nothing to do on error
-	_, _ = w.Write(body)
+	// The helper encodes before writing any header, so a marshal failure never
+	// leaves a half-written response (e.g. a 403 header followed by a second
+	// 500 write). Nothing to do on a write error for a denial body.
+	_ = mcp.WriteJSONRPCError(w, http.StatusForbidden, errorResponse)
 }
 
 // Middleware creates an HTTP middleware that authorizes MCP requests.

pkg/mcp/jsonrpc_write.go

@@ -0,0 +1,53 @@
+// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc.
+// SPDX-License-Identifier: Apache-2.0
+
+package mcp
+
+import (
+	"log/slog"
+	"net/http"
+
+	"golang.org/x/exp/jsonrpc2"
+)
+
+// EncodeJSONRPCError returns the wire encoding of a jsonrpc2 error response.
+//
+// This is the single sanctioned way to serialize a jsonrpc2 envelope in this
+// codebase. NEVER json.Marshal (or json.NewEncoder().Encode) a jsonrpc2 type:
+// jsonrpc2.Response carries no json tags and jsonrpc2.ID's only field is
+// unexported, so reflection produces Go-capitalized keys, drops the mandatory
+// "jsonrpc":"2.0" tag, and renders every id — valid or not — as "{}" (#5950).
+// jsonrpc2.EncodeMessage marshals through the library's tagged wire struct:
+// lowercase keys, the version tag stamped unconditionally, and an id that is
+// omitted when absent (never null, which MCP cannot express and the reference
+// TypeScript SDK client rejects, #6038) while a legitimate id of 0 survives.
+//
+// EncodeMessage cannot fail for a Response built from an ID and a
+// jsonrpc2.NewError; if it somehow does, this logs and falls back to a
+// hardcoded, conformant Internal Error body rather than returning nothing —
+// the original code and id are not trustworthy at that point.
+func EncodeJSONRPCError(resp *jsonrpc2.Response) []byte {
+	body, err := jsonrpc2.EncodeMessage(resp)
+	if err != nil {
+		slog.Error("failed to encode JSON-RPC error response", "error", err)
+		return []byte(`{"jsonrpc":"2.0","error":{"code":-32603,"message":"Internal error"}}`)
+	}
+	return body
+}
+
+// WriteJSONRPCError writes resp as an HTTP response: the JSON-RPC error body
+// under Content-Type application/json with the given HTTP status. The body is
+// encoded before any header is written, so an encode failure never leaves a
+// half-written response (see EncodeJSONRPCError for the fallback). Returns
+// the body write error; callers with no recovery path may discard it.
+//
+// Do not use this to write into an SSE stream — a bare JSON object in a
+// text/event-stream parses as an unrecognized field and is silently
+// discarded (#6037); SSE paths must frame the body as an event instead.
+func WriteJSONRPCError(w http.ResponseWriter, httpStatus int, resp *jsonrpc2.Response) error {
+	body := EncodeJSONRPCError(resp)
+	w.Header().Set("Content-Type", "application/json")
+	w.WriteHeader(httpStatus)
+	_, err := w.Write(body)
+	return err
+}

pkg/mcp/jsonrpc_write_test.go

@@ -0,0 +1,88 @@
+// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc.
+// SPDX-License-Identifier: Apache-2.0
+
+package mcp
+
+import (
+	"encoding/json"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"golang.org/x/exp/jsonrpc2"
+)
+
+// TestEncodeJSONRPCError pins the wire properties the helper exists to
+// guarantee: lowercase keys with the version tag stamped (the #5950 bug was
+// reflection over the untagged jsonrpc2.Response), an absent id omitted
+// rather than rendered null (#6038), and a legitimate id of 0 surviving —
+// the omitempty on the library's wire struct tests IsNil, not zero-ness.
+func TestEncodeJSONRPCError(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name string
+		id   jsonrpc2.ID
+		want string
+	}{
+		{
+			name: "int64 id",
+			id:   jsonrpc2.Int64ID(42),
+			want: `{"jsonrpc":"2.0","id":42,"error":{"code":403,"message":"denied"}}`,
+		},
+		{
+			name: "string id",
+			id:   jsonrpc2.StringID("abc"),
+			want: `{"jsonrpc":"2.0","id":"abc","error":{"code":403,"message":"denied"}}`,
+		},
+		{
+			name: "zero id survives",
+			id:   jsonrpc2.Int64ID(0),
+			want: `{"jsonrpc":"2.0","id":0,"error":{"code":403,"message":"denied"}}`,
+		},
+		{
+			name: "empty id omits the key",
+			id:   jsonrpc2.ID{},
+			want: `{"jsonrpc":"2.0","error":{"code":403,"message":"denied"}}`,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			body := EncodeJSONRPCError(&jsonrpc2.Response{
+				ID:    tt.id,
+				Error: jsonrpc2.NewError(JSONRPCCodeDenied, "denied"),
+			})
+
+			// assert.JSONEq is key-case-sensitive, so this single assertion
+			// catches "Error"/"ID" substituted for "error"/"id" and a missing
+			// "jsonrpc" tag; key presence is asserted separately because a
+			// decoded nil is indistinguishable from a decoded null.
+			assert.JSONEq(t, tt.want, string(body))
+
+			var decoded map[string]json.RawMessage
+			require.NoError(t, json.Unmarshal(body, &decoded))
+			_, hasID := decoded["id"]
+			assert.Equal(t, tt.id != jsonrpc2.ID{}, hasID, "id key presence mismatch")
+		})
+	}
+}
+
+func TestWriteJSONRPCError(t *testing.T) {
+	t.Parallel()
+
+	rr := httptest.NewRecorder()
+	err := WriteJSONRPCError(rr, http.StatusForbidden, &jsonrpc2.Response{
+		ID:    jsonrpc2.Int64ID(7),
+		Error: jsonrpc2.NewError(JSONRPCCodeDenied, "denied"),
+	})
+	require.NoError(t, err)
+
+	assert.Equal(t, http.StatusForbidden, rr.Code)
+	assert.Equal(t, "application/json", rr.Header().Get("Content-Type"))
+	assert.JSONEq(t, `{"jsonrpc":"2.0","id":7,"error":{"code":403,"message":"denied"}}`, rr.Body.String())
+}

pkg/webhook/mutating/middleware.go

@@ -328,19 +328,8 @@ func sendErrorResponse(w http.ResponseWriter, statusCode int, message string, ms
 		Error: jsonrpc2.NewError(code, message),
 	}
 
-	// Encode before writing any header, so an encode failure never leaves a
-	// half-written response (e.g. a status header followed by a second write).
-	body, encErr := jsonrpc2.EncodeMessage(errResp)
-	if encErr != nil {
-		// Unreachable in practice: errResp is always a well-formed jsonrpc2.Response
-		// built from strings/ints above. Fall back to a hardcoded valid JSON-RPC
-		// error body rather than writing nothing.
-		slog.Error("failed to encode JSON-RPC error response", "error", encErr)
-		body = fmt.Appendf(nil, `{"jsonrpc":"2.0","error":{"code":%d,"message":"Internal error"}}`, code)
-	}
-
-	w.Header().Set("Content-Type", "application/json")
-	w.WriteHeader(statusCode)
-	//nolint:gosec // G104: writing the JSON-RPC denial body to an HTTP client; nothing to do on error
-	_, _ = w.Write(body)
+	// The helper encodes before writing any header, so an encode failure never
+	// leaves a half-written response (e.g. a status header followed by a
+	// second write). Nothing to do on a write error for a denial body.
+	_ = mcp.WriteJSONRPCError(w, statusCode, errResp)
 }

pkg/webhook/validating/middleware.go

@@ -194,19 +194,8 @@ func sendErrorResponse(w http.ResponseWriter, statusCode int, message string, ms
 		Error: jsonrpc2.NewError(code, message),
 	}
 
-	// Encode before writing any header, so an encode failure never leaves a
-	// half-written response (e.g. a status header followed by a second write).
-	body, encErr := jsonrpc2.EncodeMessage(errResp)
-	if encErr != nil {
-		// Unreachable in practice: errResp is always a well-formed jsonrpc2.Response
-		// built from strings/ints above. Fall back to a hardcoded valid JSON-RPC
-		// error body rather than writing nothing.
-		slog.Error("failed to encode JSON-RPC error response", "error", encErr)
-		body = fmt.Appendf(nil, `{"jsonrpc":"2.0","error":{"code":%d,"message":"Internal error"}}`, code)
-	}
-
-	w.Header().Set("Content-Type", "application/json")
-	w.WriteHeader(statusCode)
-	//nolint:gosec // G104: writing the JSON-RPC denial body to an HTTP client; nothing to do on error
-	_, _ = w.Write(body)
+	// The helper encodes before writing any header, so an encode failure never
+	// leaves a half-written response (e.g. a status header followed by a
+	// second write). Nothing to do on a write error for a denial body.
+	_ = mcp.WriteJSONRPCError(w, statusCode, errResp)
 }