go-sdk review: schema bypass canary test, ServerInfo logging, doc comments (#3531)

lpcox · web-flow · commit d4f63f8caaa9 · 2026-04-10T10:47:03.000-07:00
Addresses high-priority items from the go-sdk module review (#748): SDK upgrade safety canary, backend diagnostics, and documentation for maintainers. ### Schema bypass canary test - `TestSchemaBypassCanary` registers tools with draft-07 features (`$ref`, `definitions`, `patternProperties`) via `Server.AddTool` and asserts no panic. Breaks loudly if a future SDK version tightens validation on the instance method path that `registerToolWithoutValidation` relies on. ### Surface backend server info during registration - New `Connection.ServerInfo()` extracts name/version from `session.InitializeResult()`. - `registerToolsFromBackend` logs backend identity on connect, or notes its absence — useful for debugging version mismatches. ```go if name, version := conn.ServerInfo(); name != "" { logger.LogInfoWithServer(serverID, "backend", "Backend server info: name=%s, version=%s", name, version) } ``` ### Documentation for future maintainers - **`marshalToResponse` ID placeholder**: explains why `ID: 1` is safe (SDK resolves correlation internally; this ID is never used for matching). - **Local `CallToolParams` type**: explains why `mcp/types.go` maintains its own type with `map[string]interface{}` Arguments instead of reusing the SDK's `json.RawMessage`-based params. > [!WARNING] > > <details> > <summary>Firewall rules blocked me from connecting to one or more addresses (expand for details)</summary> > > #### I tried to connect to the following addresses, but was blocked by firewall rules: > > - `example.com` > - Triggering command: `/tmp/go-build2372329102/b510/launcher.test /tmp/go-build2372329102/b510/launcher.test -test.testlogfile=/tmp/go-build2372329102/b510/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true g_.a ache/go/1.25.8/x-ifaceassert u/13/cc1 -I ernal/logger -I u/13/cc1 ache�� SaYeuEbQ8 64/src/net 64/pkg/tool/linu-nilfunc -p crypto/internal/-o -lang=go1.25 64/pkg/tool/linu-trimpath` (dns block) > - Triggering command: `/tmp/go-build3813880827/b514/launcher.test /tmp/go-build3813880827/b514/launcher.test -test.testlogfile=/tmp/go-build3813880827/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s -ato�� -bool -buildtags x_amd64/vet -errorsas -ifaceassert -nilfunc x_amd64/vet -ato�� -bool -buildtags x_amd64/vet -errorsas -ifaceassert -nilfunc x_amd64/vet` (dns block) > - `invalid-host-that-does-not-exist-12345.com` > - Triggering command: `/tmp/go-build2372329102/b492/config.test /tmp/go-build2372329102/b492/config.test -test.testlogfile=/tmp/go-build2372329102/b492/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true ternal/ieee754/i-p _cgo_.o x_amd64/compile` (dns block) > - Triggering command: `/tmp/go-build3813880827/b496/config.test /tmp/go-build3813880827/b496/config.test -test.testlogfile=/tmp/go-build3813880827/b496/testlog.txt -test.paniconexit0 -test.timeout=10m0s -o /tmp/go-build2372329102/b390/_pkg_.a -trimpath .test -p google.golang.or/tmp/go-build3275255162/b301/vet.cfg -lang=go1.24 .test 3723�� /tmp/go-build2372329102/b422/_pkg_.a -trimpath de/node/bin/git -p google.golang.or/tmp/go-build3275255162/b430/vet.cfg -lang=go1.24 /opt/hostedtoolcache/go/1.25.8/x64/pkg/tool/linu/tmp/go-build2372329102/b537/_testmain.go` (dns block) > - `nonexistent.local` > - Triggering command: `/tmp/go-build2372329102/b510/launcher.test /tmp/go-build2372329102/b510/launcher.test -test.testlogfile=/tmp/go-build2372329102/b510/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true g_.a ache/go/1.25.8/x-ifaceassert u/13/cc1 -I ernal/logger -I u/13/cc1 ache�� SaYeuEbQ8 64/src/net 64/pkg/tool/linu-nilfunc -p crypto/internal/-o -lang=go1.25 64/pkg/tool/linu-trimpath` (dns block) > - Triggering command: `/tmp/go-build3813880827/b514/launcher.test /tmp/go-build3813880827/b514/launcher.test -test.testlogfile=/tmp/go-build3813880827/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s -ato�� -bool -buildtags x_amd64/vet -errorsas -ifaceassert -nilfunc x_amd64/vet -ato�� -bool -buildtags x_amd64/vet -errorsas -ifaceassert -nilfunc x_amd64/vet` (dns block) > - `slow.example.com` > - Triggering command: `/tmp/go-build2372329102/b510/launcher.test /tmp/go-build2372329102/b510/launcher.test -test.testlogfile=/tmp/go-build2372329102/b510/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true g_.a ache/go/1.25.8/x-ifaceassert u/13/cc1 -I ernal/logger -I u/13/cc1 ache�� SaYeuEbQ8 64/src/net 64/pkg/tool/linu-nilfunc -p crypto/internal/-o -lang=go1.25 64/pkg/tool/linu-trimpath` (dns block) > - Triggering command: `/tmp/go-build3813880827/b514/launcher.test /tmp/go-build3813880827/b514/launcher.test -test.testlogfile=/tmp/go-build3813880827/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s -ato�� -bool -buildtags x_amd64/vet -errorsas -ifaceassert -nilfunc x_amd64/vet -ato�� -bool -buildtags x_amd64/vet -errorsas -ifaceassert -nilfunc x_amd64/vet` (dns block) > - `this-host-does-not-exist-12345.com` > - Triggering command: `/tmp/go-build2372329102/b519/mcp.test /tmp/go-build2372329102/b519/mcp.test -test.testlogfile=/tmp/go-build2372329102/b519/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true s/known/wrapperspb/wrappers.pb.go .cfg x_amd64/compile --gdwarf-5 --64 -o x_amd64/compile -o g_.a 2329102/b165/ x_amd64/vet -p` (dns block) > - Triggering command: `/tmp/go-build3813880827/b523/mcp.test /tmp/go-build3813880827/b523/mcp.test -test.testlogfile=/tmp/go-build3813880827/b523/testlog.txt -test.paniconexit0 -test.timeout=10m0s -ato�� 2329102/b519/mcp-errorsas -buildtags x_amd64/vet -errorsas -ifaceassert -nilfunc x_amd64/vet n-me�� ry=1 {{json .Mounts}} ache/Python/3.12.13/x64/bin/sh ache/go/1.25.8/xbash .cfg ache/go/1.25.8/x--version /usr/bin/runc.original` (dns block) > > If you need me to access, download, or install something from one of these locations, you can either: > > - Configure [Actions setup steps](https://gh.io/copilot/actions-setup-steps) to set up my environment, which run before the firewall is enabled > - Add the appropriate URLs or hosts to the custom allowlist in this repository's [Copilot coding agent settings](https://github.com/github/gh-aw-mcpg/settings/copilot/coding_agent) (admins only) > > </details>
diff --git a/internal/mcp/connection.go b/internal/mcp/connection.go
@@ -287,6 +287,20 @@ func (c *Connection) GetHTTPHeaders() map[string]string {
 	return c.headers
 }
 
+// ServerInfo returns the backend's name and version from the MCP initialize handshake.
+// Returns ("", "") when no SDK session is available (plain JSON-RPC transport).
+func (c *Connection) ServerInfo() (name, version string) {
+	sess := c.getSDKSession()
+	if sess == nil {
+		return "", ""
+	}
+	initResult := sess.InitializeResult()
+	if initResult == nil || initResult.ServerInfo == nil {
+		return "", ""
+	}
+	return initResult.ServerInfo.Name, initResult.ServerInfo.Version
+}
+
 // reconnectPlainJSON re-initialises the plain JSON-RPC session with the HTTP backend.
 // It is safe for concurrent callers: only one reconnect runs at a time, and the updated
 // session ID is available to all callers once the lock is released.
@@ -482,6 +496,11 @@ func (c *Connection) callSDKMethod(method string, params interface{}) (*Response
 
 // marshalToResponse marshals an SDK result into a Response object.
 // This helper reduces code duplication across all MCP method wrappers.
+//
+// The ID field is set to a static placeholder (1) because this Response is only
+// constructed after the SDK's session.XXX() call has already resolved the
+// request–response correlation internally. The gateway never uses this ID for
+// matching; it is present solely to satisfy the JSON-RPC 2.0 structure.
 func marshalToResponse(result interface{}) (*Response, error) {
 	resultJSON, err := json.Marshal(result)
 	if err != nil {
@@ -490,7 +509,7 @@ func marshalToResponse(result interface{}) (*Response, error) {
 
 	return &Response{
 		JSONRPC: "2.0",
-		ID:      1, // Placeholder ID
+		ID:      1, // Placeholder – see function comment for safety rationale
 		Result:  resultJSON,
 	}, nil
 }
diff --git a/internal/mcp/types.go b/internal/mcp/types.go
@@ -34,7 +34,14 @@ type Tool struct {
 	InputSchema map[string]interface{} `json:"inputSchema"`
 }
 
-// CallToolParams represents parameters for calling a tool
+// CallToolParams represents parameters for calling a tool.
+//
+// This is a local type rather than an alias for the SDK's CallToolParams because the
+// gateway's plain JSON-RPC transport path needs to marshal/unmarshal tool call parameters
+// from raw JSON without importing the SDK's typed params (which use json.RawMessage for
+// Arguments). The local type uses map[string]interface{} for Arguments, matching the
+// gateway's internal representation and simplifying the marshal/unmarshal roundtrip in
+// callParamMethod. See callTool in connection.go for the bridge to the SDK type.
 type CallToolParams struct {
 	Name      string                 `json:"name"`
 	Arguments map[string]interface{} `json:"arguments,omitempty"`
diff --git a/internal/server/tool_registry.go b/internal/server/tool_registry.go
@@ -153,6 +153,14 @@ func (us *UnifiedServer) registerToolsFromBackend(serverID string) error {
 		return fmt.Errorf("failed to connect: %w", err)
 	}
 
+	// Surface backend server info from the MCP initialize handshake for diagnostics.
+	// This helps debug compatibility issues between the gateway and specific backends.
+	if name, version := conn.ServerInfo(); name != "" {
+		logger.LogInfoWithServer(serverID, "backend", "Backend server info: name=%s, version=%s", name, version)
+	} else {
+		logger.LogInfoWithServer(serverID, "backend", "Backend server info unavailable (no SDK session or server omitted serverInfo)")
+	}
+
 	// List tools from backend
 	result, err := conn.SendRequestWithServerID(context.Background(), "tools/list", nil, serverID)
 	if err != nil {
diff --git a/internal/server/tool_registry_test.go b/internal/server/tool_registry_test.go
@@ -8,6 +8,7 @@ import (
 	"testing"
 
 	"github.com/github/gh-aw-mcpg/internal/config"
+	sdk "github.com/modelcontextprotocol/go-sdk/mcp"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -591,3 +592,73 @@ func TestRegisterToolsFromBackend_EmptyAllowedList(t *testing.T) {
 	assert.Contains(us.tools, "s___tool_b")
 	assert.Contains(us.tools, "s___tool_c")
 }
+
+// TestSchemaBypassCanary is a canary test for SDK upgrades.
+//
+// The gateway relies on Server.AddTool (instance method) to register backend tools
+// WITHOUT full JSON Schema validation, because backends may emit schemas using
+// draft-07 features (e.g., "$ref", "definitions", "if/then/else") that the SDK's
+// stricter package-level AddTool function would reject.
+//
+// This test verifies that Server.AddTool still accepts such schemas. If it panics
+// or rejects them after an SDK upgrade, the gateway's tool registration will break
+// and this test serves as the early warning.
+//
+// See also: registerToolWithoutValidation in tool_registry.go.
+func TestSchemaBypassCanary(t *testing.T) {
+	assert := assert.New(t)
+
+	server := sdk.NewServer(&sdk.Implementation{Name: "canary", Version: "1.0"}, &sdk.ServerOptions{})
+	noop := func(ctx context.Context, req *sdk.CallToolRequest) (*sdk.CallToolResult, error) {
+		return &sdk.CallToolResult{}, nil
+	}
+
+	// Draft-07 schema with $ref and definitions — valid JSON Schema but uses
+	// features beyond what the SDK's stricter path validates.
+	draft07Schema := map[string]interface{}{
+		"type": "object",
+		"properties": map[string]interface{}{
+			"repo": map[string]interface{}{
+				"$ref": "#/definitions/repoName",
+			},
+		},
+		"definitions": map[string]interface{}{
+			"repoName": map[string]interface{}{
+				"type":      "string",
+				"minLength": 1,
+			},
+		},
+	}
+
+	// Server.AddTool (instance method) must not panic — this is the code path
+	// that registerToolWithoutValidation uses.
+	assert.NotPanics(func() {
+		server.AddTool(&sdk.Tool{
+			Name:        "draft07_tool",
+			Description: "Tool with draft-07 schema features",
+			InputSchema: draft07Schema,
+		}, noop)
+	}, "Server.AddTool must accept draft-07 schemas; if this fails after an SDK upgrade, "+
+		"registerToolWithoutValidation needs to be updated")
+
+	// Schema with additionalProperties and patternProperties — another draft-07
+	// feature set that backends commonly use.
+	extendedSchema := map[string]interface{}{
+		"type": "object",
+		"properties": map[string]interface{}{
+			"query": map[string]interface{}{"type": "string"},
+		},
+		"additionalProperties": false,
+		"patternProperties": map[string]interface{}{
+			"^x-": map[string]interface{}{"type": "string"},
+		},
+	}
+
+	assert.NotPanics(func() {
+		server.AddTool(&sdk.Tool{
+			Name:        "extended_schema_tool",
+			Description: "Tool with extended schema features",
+			InputSchema: extendedSchema,
+		}, noop)
+	}, "Server.AddTool must accept schemas with patternProperties/additionalProperties")
+}
