Harden MCP pagination against cursor cycles and add go-sdk registration canaries (#4302)

This updates the go-sdk integration points highlighted in the review:
production pagination now fails fast on cyclical cursors, and
upgrade-sensitive behavior around tool registration is covered by a
stronger canary. It also clarifies transport-specific protocol/session
handling to reduce future maintenance ambiguity.

- **Pagination safety in `internal/mcp/connection.go`**
- Added cursor-cycle detection to `paginateAll` using a `seenCursors`
set.
- Returns an explicit error when a backend repeats a cursor instead of
continuing until the page cap.

- **Pagination behavior coverage in `internal/mcp/connection_test.go`**
- Added a cycle-focused test case to `TestPaginateAll` that verifies
early termination on repeated cursors.
- Adjusted the max-pages test to use unique cursors so it continues
validating the cap path independently.

- **Transport-scope clarity in `internal/mcp/http_transport.go`**
- Clarified that `MCPProtocolVersion` applies to the plain JSON-RPC
fallback path only.
- Added inline rationale in `isSessionNotFoundError` for string fallback
matching when SDK typed errors are unavailable.

- **SDK-upgrade canary for schema-validation bypass in
`internal/server/routed_test.go`**
- Extended `TestRegisterToolWithoutValidation` with a strict-schema tool
and intentionally schema-invalid arguments.
- Verifies the `registerToolWithoutValidation`/`Server.AddTool` path
still invokes handlers without SDK argument-value validation
regressions.

```go
seenCursors := make(map[string]struct{})
for pageCount := 1; cursor != ""; pageCount++ {
    if _, seen := seenCursors[cursor]; seen {
        return nil, fmt.Errorf("list%s: backend serverID=%s returned cyclical cursor %q", itemKind, serverID, cursor)
    }
    seenCursors[cursor] = struct{}{}
    // fetch next page...
}
```

> [!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-build2010847725/b513/launcher.test
/tmp/go-build2010847725/b513/launcher.test
-test.testlogfile=/tmp/go-build2010847725/b513/testlog.txt
-test.paniconexit0 -test.timeout=10m0s
/tmp/go-build2010847725/b423/vet.cfg
rotocol/go-sdk@v1.5.0/jsonrpc/jsonrpc.go -trimpath x_amd64/vet gci-lint
failed /opt/hostedtoolcache/go/1.25.9/x64/pkg/tool/linux_amd64/vet
7457537/b209/sym-atomic -lang=go1.25 x_amd64/vet -p g_.a
477457537/b287//-ifaceassert x_amd64/vet -I go-sdk/mcp 7457537/b287/
x_amd64/vet` (dns block)
> - Triggering command: `/tmp/go-build81877103/b513/launcher.test
/tmp/go-build81877103/b513/launcher.test
-test.testlogfile=/tmp/go-build81877103/b513/testlog.txt
-test.paniconexit0 -test.timeout=10m0s rds/��
lib/rustlib/x86_/home/REDACTED/.rustup/toolchains/stable-x86_64-REDACTED-linux-gnu/lib/rustlib/x86_git
lib/rustlib/x86_/home/REDACTED/.rustup/toolchains/stable-x86_64-REDACTED-linux-gnu/lib/rustlib/x86_commit
-guard/target/debug/deps/rustco0oSqi/symbols.o -guard/target/debash b0d7
-guard/target/de--noprofile
-guard/target/debug/deps/serde_derive-bdc7cd22a58a5141.serde_derive.12123747d8da05ed-cgu.03.rcgu.o
-gua��
-guard/target/debug/deps/serde_derive-bdc7cd22a58a5141.serde_derive.12123747d8da05ed-cgu.05.rcgu/opt/hostedtoolcache/go/1.25.9/x64/pkg/tool/linux_amd64/vet
-guard/target/debug/deps/serde_derive-bdc7cd22a58a5141.serde_derive.12123747d8da05ed-cgu.06.rcgu/tmp/go-build3709670566/b506/vet.cfg
-guard/target/debug/deps/serde_derive-bdc7cd22a58a5141.serde_derive.12123747d8da05ed-cgu.07.rcgu.o
-guard/target/debash -guard/target/de--norc -guard/target/de--noprofile
-guard/target/debug/deps/serde_derive-bdc7cd22a58a5141.serde_derive.12123747d8da05ed-cgu.11.rcgu.o`
(dns block)
> - `invalid-host-that-does-not-exist-12345.com`
> - Triggering command: `/tmp/go-build2010847725/b495/config.test
/tmp/go-build2010847725/b495/config.test
-test.testlogfile=/tmp/go-build2010847725/b495/testlog.txt
-test.paniconexit0 -test.timeout=10m0s
/tmp/go-build2010847725/b397/vet.cfg @v1.1.3/cpu/x86/-c=4
ggXp/9qGZEHgmklo-nolocalimports x_amd64/vet --gdwarf-5
nal/encoding/tag-atomic -o x_amd64/vet 7457�� g_.a
ache/go/1.25.9/x-ifaceassert x_amd64/vet -I /opt/hostedtoolc-atomic -I
x_amd64/vet` (dns block)
> - `nonexistent.local`
> - Triggering command: `/tmp/go-build2010847725/b513/launcher.test
/tmp/go-build2010847725/b513/launcher.test
-test.testlogfile=/tmp/go-build2010847725/b513/testlog.txt
-test.paniconexit0 -test.timeout=10m0s
/tmp/go-build2010847725/b423/vet.cfg
rotocol/go-sdk@v1.5.0/jsonrpc/jsonrpc.go -trimpath x_amd64/vet gci-lint
failed /opt/hostedtoolcache/go/1.25.9/x64/pkg/tool/linux_amd64/vet
7457537/b209/sym-atomic -lang=go1.25 x_amd64/vet -p g_.a
477457537/b287//-ifaceassert x_amd64/vet -I go-sdk/mcp 7457537/b287/
x_amd64/vet` (dns block)
> - Triggering command: `/tmp/go-build81877103/b513/launcher.test
/tmp/go-build81877103/b513/launcher.test
-test.testlogfile=/tmp/go-build81877103/b513/testlog.txt
-test.paniconexit0 -test.timeout=10m0s rds/��
lib/rustlib/x86_/home/REDACTED/.rustup/toolchains/stable-x86_64-REDACTED-linux-gnu/lib/rustlib/x86_git
lib/rustlib/x86_/home/REDACTED/.rustup/toolchains/stable-x86_64-REDACTED-linux-gnu/lib/rustlib/x86_commit
-guard/target/debug/deps/rustco0oSqi/symbols.o -guard/target/debash b0d7
-guard/target/de--noprofile
-guard/target/debug/deps/serde_derive-bdc7cd22a58a5141.serde_derive.12123747d8da05ed-cgu.03.rcgu.o
-gua��
-guard/target/debug/deps/serde_derive-bdc7cd22a58a5141.serde_derive.12123747d8da05ed-cgu.05.rcgu/opt/hostedtoolcache/go/1.25.9/x64/pkg/tool/linux_amd64/vet
-guard/target/debug/deps/serde_derive-bdc7cd22a58a5141.serde_derive.12123747d8da05ed-cgu.06.rcgu/tmp/go-build3709670566/b506/vet.cfg
-guard/target/debug/deps/serde_derive-bdc7cd22a58a5141.serde_derive.12123747d8da05ed-cgu.07.rcgu.o
-guard/target/debash -guard/target/de--norc -guard/target/de--noprofile
-guard/target/debug/deps/serde_derive-bdc7cd22a58a5141.serde_derive.12123747d8da05ed-cgu.11.rcgu.o`
(dns block)
> - `slow.example.com`
> - Triggering command: `/tmp/go-build2010847725/b513/launcher.test
/tmp/go-build2010847725/b513/launcher.test
-test.testlogfile=/tmp/go-build2010847725/b513/testlog.txt
-test.paniconexit0 -test.timeout=10m0s
/tmp/go-build2010847725/b423/vet.cfg
rotocol/go-sdk@v1.5.0/jsonrpc/jsonrpc.go -trimpath x_amd64/vet gci-lint
failed /opt/hostedtoolcache/go/1.25.9/x64/pkg/tool/linux_amd64/vet
7457537/b209/sym-atomic -lang=go1.25 x_amd64/vet -p g_.a
477457537/b287//-ifaceassert x_amd64/vet -I go-sdk/mcp 7457537/b287/
x_amd64/vet` (dns block)
> - Triggering command: `/tmp/go-build81877103/b513/launcher.test
/tmp/go-build81877103/b513/launcher.test
-test.testlogfile=/tmp/go-build81877103/b513/testlog.txt
-test.paniconexit0 -test.timeout=10m0s rds/��
lib/rustlib/x86_/home/REDACTED/.rustup/toolchains/stable-x86_64-REDACTED-linux-gnu/lib/rustlib/x86_git
lib/rustlib/x86_/home/REDACTED/.rustup/toolchains/stable-x86_64-REDACTED-linux-gnu/lib/rustlib/x86_commit
-guard/target/debug/deps/rustco0oSqi/symbols.o -guard/target/debash b0d7
-guard/target/de--noprofile
-guard/target/debug/deps/serde_derive-bdc7cd22a58a5141.serde_derive.12123747d8da05ed-cgu.03.rcgu.o
-gua��
-guard/target/debug/deps/serde_derive-bdc7cd22a58a5141.serde_derive.12123747d8da05ed-cgu.05.rcgu/opt/hostedtoolcache/go/1.25.9/x64/pkg/tool/linux_amd64/vet
-guard/target/debug/deps/serde_derive-bdc7cd22a58a5141.serde_derive.12123747d8da05ed-cgu.06.rcgu/tmp/go-build3709670566/b506/vet.cfg
-guard/target/debug/deps/serde_derive-bdc7cd22a58a5141.serde_derive.12123747d8da05ed-cgu.07.rcgu.o
-guard/target/debash -guard/target/de--norc -guard/target/de--noprofile
-guard/target/debug/deps/serde_derive-bdc7cd22a58a5141.serde_derive.12123747d8da05ed-cgu.11.rcgu.o`
(dns block)
> - `this-host-does-not-exist-12345.com`
> - Triggering command: `/tmp/go-build2010847725/b522/mcp.test
/tmp/go-build2010847725/b522/mcp.test
-test.testlogfile=/tmp/go-build2010847725/b522/testlog.txt
-test.paniconexit0 -test.timeout=10m0s abis�� cfg -I x_amd64/vet
--gdwarf-5 ce -o x_amd64/vet cfg 7457537/b449/_pkg_.a
nLJk/a4JpKmHiBmr_E514nLJk x_amd64/vet --gdwarf-5 ntio/asm/keyset -o
x_amd64/vet` (dns block)
> - Triggering command: `/tmp/go-build81877103/b522/mcp.test
/tmp/go-build81877103/b522/mcp.test
-test.testlogfile=/tmp/go-build81877103/b522/testlog.txt
-test.paniconexit0 -test.timeout=10m0s lib/��
lib/rustlib/x86_/home/REDACTED/.rustup/toolchains/stable-x86_64-REDACTED-linux-gnu/lib/rustlib/x86_git
lib/rustlib/x86_/home/REDACTED/.rustup/toolchains/stable-x86_64-REDACTED-linux-gnu/lib/rustlib/x86_commit
lib/rustlib/x86_/home/REDACTED/.rustup/toolchains/stable-x86_64-REDACTED-linux-gnu/lib/rustlib/x86_-m
lib/rustlib/x86_bash b0d7 lib/rustlib/x86_--noprofile
lib/rustlib/x86_/home/REDACTED/.rustup/toolchains/stable-x86_64-REDACTED-linux-gnu/lib/rustlib/x86_64-REDACTED-linux-gnu/lib/libminiz_oxide-2b6a8d2f6e1dc71b.rlib
lib/��
lib/rustlib/x86_64-REDACTED-linux-gnu/lib/librustc_std_workspace_alloc-76b5fe9328c1063f.rlib
lib/rustlib/x86_64-REDACTED-linux-gnu/lib/libminiz_oxide-2b6a8d2f6e1dc71b.rlib
iginal -f &#34;$GOPATH/bin/bash -I yload_StoredInPa--noprofile
-guard/target/debug/deps/libserde_derive-bdc7cd22a58a5141.so` (dns
block)
> - Triggering command: `/tmp/go-build1985989918/b522/mcp.test
/tmp/go-build1985989918/b522/mcp.test
-test.testlogfile=/tmp/go-build1985989918/b522/testlog.txt
-test.paniconexit0 -test.timeout=10m0s /tmp�� /run/containerd/-p
herFiles,CFiles,main by/31b594e5b708d-lang=go1.25 ntime.v2.task/mogit
-guard/target/deshow
-guard/target/de7291de63ecded2ae0119c7bba93a1d428d68106a:internal/server/routed_test.go
296/log.json b2f7�� 081a786f99487854go1.25.9 y er-linux
ntime.v2.task/mo/opt/hostedtoolcache/CodeQL/2.25.1/x64/codeql/tools/linux64/java/lib/jspawnhelper
/tmp/awmg-log-te21.0.9&#43;10-LTS --routed er-linux` (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>

Author: lpcox

internal/mcp/connection.go

@@ -610,10 +610,15 @@ func paginateAll[T any](
 	logConn.Printf("list%s: received page of %d %s from serverID=%s", itemKind, len(first.Items), itemKind, serverID)
 
 	cursor := first.NextCursor
+	seenCursors := make(map[string]struct{})
 	for pageCount := 1; cursor != ""; pageCount++ {
 		if pageCount >= paginateAllMaxPages {
 			return nil, fmt.Errorf("list%s: backend serverID=%s returned more than %d pages; aborting to prevent unbounded memory growth", itemKind, serverID, paginateAllMaxPages)
 		}
+		if _, seen := seenCursors[cursor]; seen {
+			return nil, fmt.Errorf("list%s: backend serverID=%s returned cyclical cursor %q", itemKind, serverID, cursor)
+		}
+		seenCursors[cursor] = struct{}{}
 		page, err := fetch(cursor)
 		if err != nil {
 			return nil, err

internal/mcp/connection_test.go

@@ -978,16 +978,43 @@ func TestPaginateAll(t *testing.T) {
 	})
 
 	t.Run("exceeding max pages returns error", func(t *testing.T) {
-		// Each call returns a cursor so the loop never ends naturally.
+		// Each call returns a unique cursor so the loop never ends naturally.
 		callCount := 0
 		_, err := paginateAll("server1", "tools", func(cursor string) (paginatedPage[string], error) {
 			callCount++
-			return paginatedPage[string]{Items: []string{"x"}, NextCursor: "next"}, nil
+			nextCursor := "next"
+			if cursor != "" {
+				nextCursor = cursor + "next"
+			}
+			return paginatedPage[string]{Items: []string{"x"}, NextCursor: nextCursor}, nil
 		})
 		require.Error(t, err)
 		assert.Contains(t, err.Error(), "more than")
 		assert.Contains(t, err.Error(), "pages")
 		// Must stop at the page limit, not run forever.
 		assert.Equal(t, paginateAllMaxPages, callCount)
 	})
+
+	t.Run("cyclical cursor returns error", func(t *testing.T) {
+		callCount := 0
+		_, err := paginateAll("server1", "tools", func(cursor string) (paginatedPage[string], error) {
+			callCount++
+			switch cursor {
+			case "":
+				return paginatedPage[string]{Items: []string{"a"}, NextCursor: "page2"}, nil
+			case "page2":
+				return paginatedPage[string]{Items: []string{"b"}, NextCursor: "page3"}, nil
+			case "page3":
+				return paginatedPage[string]{Items: []string{"c"}, NextCursor: "page2"}, nil
+			default:
+				return paginatedPage[string]{Items: nil, NextCursor: ""}, nil
+			}
+		})
+
+		require.Error(t, err)
+		assert.Contains(t, err.Error(), "cyclical cursor")
+		assert.Contains(t, err.Error(), "page2")
+		// Initial page + 2 unique cursor fetches, then cycle detected before another fetch.
+		assert.Equal(t, 3, callCount)
+	})
 }

internal/mcp/http_transport.go

@@ -33,7 +33,9 @@ const (
 	HTTPTransportPlainJSON HTTPTransportType = "plain-json"
 )
 
-// MCPProtocolVersion is the MCP protocol version used in initialization requests.
+// MCPProtocolVersion is the MCP protocol version used only by the plain JSON-RPC
+// fallback path in this package. Streamable and SSE transports are SDK-managed
+// and negotiate protocol versions internally.
 const MCPProtocolVersion = "2025-11-25"
 
 // requestIDCounter is used to generate unique request IDs for HTTP requests
@@ -78,6 +80,8 @@ func isSessionNotFoundError(err error) bool {
 	if errors.Is(err, sdk.ErrSessionMissing) {
 		return true
 	}
+	// Plain JSON-RPC fallback requests bypass SDK session types, so they cannot
+	// return sdk.ErrSessionMissing and are matched by backend error text instead.
 	return strings.Contains(strings.ToLower(err.Error()), "session not found")
 }
 

internal/server/routed_test.go

@@ -703,6 +703,24 @@ func TestRegisterToolWithoutValidation(t *testing.T) {
 		},
 	}, handler)
 
+	// This canary verifies the key behavior relied on by registerToolWithoutValidation:
+	// tool calls are not rejected by SDK argument-value validation.
+	var strictHandlerCalled bool
+	registerToolWithoutValidation(server, &sdk.Tool{
+		Name:        "strict_tool",
+		Description: "A strict-schema tool",
+		InputSchema: map[string]interface{}{
+			"type": "object",
+			"properties": map[string]interface{}{
+				"count": map[string]interface{}{"type": "integer"},
+			},
+			"required": []interface{}{"count"},
+		},
+	}, func(ctx context.Context, req *sdk.CallToolRequest, state interface{}) (*sdk.CallToolResult, interface{}, error) {
+		strictHandlerCalled = true
+		return &sdk.CallToolResult{IsError: false}, state, nil
+	})
+
 	// Use in-memory transports to connect a client to the server and invoke the tool
 	serverTransport, clientTransport := sdk.NewInMemoryTransports()
 	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
@@ -721,6 +739,16 @@ func TestRegisterToolWithoutValidation(t *testing.T) {
 	require.NoError(err)
 	assert.False(result.IsError)
 	assert.True(handlerCalled, "Handler should have been called")
+
+	// Provide an intentionally invalid value for the strict schema ("count" must be integer).
+	// If SDK starts validating argument values on this registration path, this call will fail.
+	strictResult, err := clientSession.CallTool(ctx, &sdk.CallToolParams{
+		Name:      "strict_tool",
+		Arguments: map[string]interface{}{"count": "not-an-integer"},
+	})
+	require.NoError(err)
+	assert.False(strictResult.IsError)
+	assert.True(strictHandlerCalled, "Strict handler should be called even with schema-invalid arguments")
 }
 
 // TestCreateHTTPServerForRoutedMode_OAuth tests OAuth discovery endpoint in routed mode