Skip to content

Commit a82e67c

Browse files
authored
chore: upgrade go-sdk to v1.5.0 and address go-fan review items (#3610)
The project was on `go-sdk` v1.4.1 while v1.5.0 was available, and two fragility concerns were identified: session expiry detection via string matching and SSE deprecation warnings bypassing the structured logger. ## Changes - **`go.mod`/`go.sum`** — upgrade `github.com/modelcontextprotocol/go-sdk` v1.4.1 → v1.5.0 - **`internal/mcp/http_transport.go`** — replace string-matching session error detection with the typed sentinel introduced in v1.5.0, retaining string-match as fallback for non-SDK (plain JSON-RPC) transports: ```go // Before (fragile): return strings.Contains(strings.ToLower(err.Error()), "session not found") // After (robust, with fallback): if errors.Is(err, sdk.ErrSessionMissing) { return true } return strings.Contains(strings.ToLower(err.Error()), "session not found") ``` - **`internal/mcp/connection.go`** — route SSE deprecation `log.Printf` calls through `logger.LogWarn` so they appear in `mcp-gateway.log` instead of bypassing the structured logging pipeline - **`internal/server/tool_registry.go`** — update `registerToolWithoutValidation` comment to accurately reflect v1.5.0 `Server.AddTool` behaviour: now enforces `type: "object"` on the input schema but still does not validate argument values at call time; notes verification against v1.5.0 source > [!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-build347415617/b510/launcher.test /tmp/go-build347415617/b510/launcher.test -test.testlogfile=/tmp/go-build347415617/b510/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 64/src/net -trimpath de/node/bin/as -p io -lang=go1.25 o_main.o -I uf@v1.36.11/refl-errorsas uf@v1.36.11/refl-ifaceassert x_amd64/vet --gdwarf-5 --64 -o x_amd64/vet` (dns block) > - Triggering command: `/tmp/go-build2356109318/b510/launcher.test /tmp/go-build2356109318/b510/launcher.test -test.testlogfile=/tmp/go-build2356109318/b510/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true eFSy5PFyu cfg 64/pkg/tool/linux_amd64/vet --gdwarf-5 --64 -o 64/pkg/tool/linu--others pkg/�� om/davecgh/go-spew@v1.1.1/spew/bypass.go cfg 64/pkg/tool/linux_amd64/vet -plugin-opt=-pasbash g/grpc/internal/--norc -plugin-opt=-pas--noprofile 64/pkg/tool/linux_amd64/vet` (dns block) > - Triggering command: `/tmp/go-build2885812085/b514/launcher.test /tmp/go-build2885812085/b514/launcher.test -test.testlogfile=/tmp/go-build2885812085/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s -ato�� -bool -buildtags iginal -errorsas -ifaceassert -nilfunc iginal -ato�� /launcher/connection_pool.go /launcher/health_monitor.go x_amd64/vet -errorsas -ifaceassert -nilfunc x_amd64/vet` (dns block) > - `invalid-host-that-does-not-exist-12345.com` > - Triggering command: `/tmp/go-build347415617/b492/config.test /tmp/go-build347415617/b492/config.test -test.testlogfile=/tmp/go-build347415617/b492/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true o /home/REDACTED/go/pkg/mod/github.c--64 x_amd64/compile /home/REDACTED/go//opt/hostedtoolcache/go/1.25.8/x64/pkg/tool/linux_amd64/vet fips140/ecdsa /home/REDACTED/go/-unreachable=false x_amd64/compile ual_�� azero@v1.11.0/internal/wasmdebug-p azero@v1.11.0/internal/wasmdebuggolang.org/x/net/http2 x_amd64/asm -p crypto/internal/-atomic -lang=go1.25 x_amd64/asm` (dns block) > - Triggering command: `/tmp/go-build2885812085/b496/config.test /tmp/go-build2885812085/b496/config.test -test.testlogfile=/tmp/go-build2885812085/b496/testlog.txt -test.paniconexit0 -test.timeout=10m0s -uns�� -unreachable=false /tmp/go-build347415617/b075/vet.cfg ash g_.a gmYLA_3tW .13/x64/as /opt/hostedtoolcache/go/1.25.8/x64/pkg/tool/linu--others -ato�� -bool -buildtags 86_64/git -errorsas -ifaceassert -nilfunc iginal` (dns block) > - `nonexistent.local` > - Triggering command: `/tmp/go-build347415617/b510/launcher.test /tmp/go-build347415617/b510/launcher.test -test.testlogfile=/tmp/go-build347415617/b510/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 64/src/net -trimpath de/node/bin/as -p io -lang=go1.25 o_main.o -I uf@v1.36.11/refl-errorsas uf@v1.36.11/refl-ifaceassert x_amd64/vet --gdwarf-5 --64 -o x_amd64/vet` (dns block) > - Triggering command: `/tmp/go-build2356109318/b510/launcher.test /tmp/go-build2356109318/b510/launcher.test -test.testlogfile=/tmp/go-build2356109318/b510/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true eFSy5PFyu cfg 64/pkg/tool/linux_amd64/vet --gdwarf-5 --64 -o 64/pkg/tool/linu--others pkg/�� om/davecgh/go-spew@v1.1.1/spew/bypass.go cfg 64/pkg/tool/linux_amd64/vet -plugin-opt=-pasbash g/grpc/internal/--norc -plugin-opt=-pas--noprofile 64/pkg/tool/linux_amd64/vet` (dns block) > - Triggering command: `/tmp/go-build2885812085/b514/launcher.test /tmp/go-build2885812085/b514/launcher.test -test.testlogfile=/tmp/go-build2885812085/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s -ato�� -bool -buildtags iginal -errorsas -ifaceassert -nilfunc iginal -ato�� /launcher/connection_pool.go /launcher/health_monitor.go x_amd64/vet -errorsas -ifaceassert -nilfunc x_amd64/vet` (dns block) > - `slow.example.com` > - Triggering command: `/tmp/go-build347415617/b510/launcher.test /tmp/go-build347415617/b510/launcher.test -test.testlogfile=/tmp/go-build347415617/b510/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 64/src/net -trimpath de/node/bin/as -p io -lang=go1.25 o_main.o -I uf@v1.36.11/refl-errorsas uf@v1.36.11/refl-ifaceassert x_amd64/vet --gdwarf-5 --64 -o x_amd64/vet` (dns block) > - Triggering command: `/tmp/go-build2356109318/b510/launcher.test /tmp/go-build2356109318/b510/launcher.test -test.testlogfile=/tmp/go-build2356109318/b510/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true eFSy5PFyu cfg 64/pkg/tool/linux_amd64/vet --gdwarf-5 --64 -o 64/pkg/tool/linu--others pkg/�� om/davecgh/go-spew@v1.1.1/spew/bypass.go cfg 64/pkg/tool/linux_amd64/vet -plugin-opt=-pasbash g/grpc/internal/--norc -plugin-opt=-pas--noprofile 64/pkg/tool/linux_amd64/vet` (dns block) > - Triggering command: `/tmp/go-build2885812085/b514/launcher.test /tmp/go-build2885812085/b514/launcher.test -test.testlogfile=/tmp/go-build2885812085/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s -ato�� -bool -buildtags iginal -errorsas -ifaceassert -nilfunc iginal -ato�� /launcher/connection_pool.go /launcher/health_monitor.go x_amd64/vet -errorsas -ifaceassert -nilfunc x_amd64/vet` (dns block) > - `this-host-does-not-exist-12345.com` > - Triggering command: `/tmp/go-build347415617/b519/mcp.test /tmp/go-build347415617/b519/mcp.test -test.testlogfile=/tmp/go-build347415617/b519/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 1.80.0/internal/channelz/channel.go 1.80.0/internal/channelz/channelmap.go x_amd64/vet 305_amd64.s --64 -o x_amd64/vet 0880�� m/grpc-gateway/v2@v2.28.0/runtim-errorsas m/grpc-gateway/v2@v2.28.0/runtim-ifaceassert x_amd64/vet --gdwarf-5 --64 -o x_amd64/vet` (dns block) > - Triggering command: `/tmp/go-build2356109318/b519/mcp.test /tmp/go-build2356109318/b519/mcp.test -test.testlogfile=/tmp/go-build2356109318/b519/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true olang.org/grpc@v-p cfg 64/pkg/tool/linu-lang=go1.25 0880581/b288/ --64` (dns block) > - Triggering command: `/tmp/go-build2885812085/b523/mcp.test /tmp/go-build2885812085/b523/mcp.test -test.testlogfile=/tmp/go-build2885812085/b523/testlog.txt -test.paniconexit0 -test.timeout=10m0s -ato�� -bool -buildtags x_amd64/vet -errorsas -ifaceassert -nilfunc x_amd64/vet /usr�� --version -tests x_amd64/vet balancer/weight/grep` (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>
2 parents 4c7872d + 53b7390 commit a82e67c

File tree

6 files changed

+24
-13
lines changed

6 files changed

+24
-13
lines changed

go.mod

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ go 1.25.0
44

55
require (
66
github.com/BurntSushi/toml v1.6.0
7-
github.com/modelcontextprotocol/go-sdk v1.4.1
7+
github.com/modelcontextprotocol/go-sdk v1.5.0
88
github.com/spf13/cobra v1.10.2
99
golang.org/x/term v0.41.0
1010
)

go.sum

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,8 @@ github.com/go-logr/logr v1.4.3 h1:CjnDlHq8ikf6E492q6eKboGOC0T8CDaOvkHCIg8idEI=
1212
github.com/go-logr/logr v1.4.3/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY=
1313
github.com/go-logr/stdr v1.2.2 h1:hSWxHoqTgW2S2qGc0LTAI563KZ5YKYRhT3MFKZMbjag=
1414
github.com/go-logr/stdr v1.2.2/go.mod h1:mMo/vtBO5dYbehREoey6XUKy/eSumjCCveDpRre4VKE=
15-
github.com/golang-jwt/jwt/v5 v5.3.0 h1:pv4AsKCKKZuqlgs5sUmn4x8UlGa0kEVt/puTpKx9vvo=
16-
github.com/golang-jwt/jwt/v5 v5.3.0/go.mod h1:fxCRLWMO43lRc8nhHWY6LGqRcf+1gQWArsqaEUEa5bE=
15+
github.com/golang-jwt/jwt/v5 v5.3.1 h1:kYf81DTWFe7t+1VvL7eS+jKFVWaUnK9cB1qbwn63YCY=
16+
github.com/golang-jwt/jwt/v5 v5.3.1/go.mod h1:fxCRLWMO43lRc8nhHWY6LGqRcf+1gQWArsqaEUEa5bE=
1717
github.com/golang/protobuf v1.5.4 h1:i7eJL8qZTpSEXOPTxNKhASYpMn+8e5Q6AdndVa1dWek=
1818
github.com/golang/protobuf v1.5.4/go.mod h1:lnTiLA8Wa4RWRcIUkrtSVa5nRhsEGBg48fD6rSs7xps=
1919
github.com/google/go-cmp v0.7.0 h1:wk8382ETsv4JYUZwIsn6YpYiWiBsYLSJiTsyBybVuN8=
@@ -34,8 +34,8 @@ github.com/kr/pretty v0.3.1 h1:flRD4NNwYAUpkphVc1HcthR4KEIFJ65n8Mw5qdRn3LE=
3434
github.com/kr/pretty v0.3.1/go.mod h1:hoEshYVHaxMs3cyo3Yncou5ZscifuDolrwPKZanG3xk=
3535
github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY=
3636
github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE=
37-
github.com/modelcontextprotocol/go-sdk v1.4.1 h1:M4x9GyIPj+HoIlHNGpK2hq5o3BFhC+78PkEaldQRphc=
38-
github.com/modelcontextprotocol/go-sdk v1.4.1/go.mod h1:Bo/mS87hPQqHSRkMv4dQq1XCu6zv4INdXnFZabkNU6s=
37+
github.com/modelcontextprotocol/go-sdk v1.5.0 h1:CHU0FIX9kpueNkxuYtfYQn1Z0slhFzBZuq+x6IiblIU=
38+
github.com/modelcontextprotocol/go-sdk v1.5.0/go.mod h1:gggDIhoemhWs3BGkGwd1umzEXCEMMvAnhTrnbXJKKKA=
3939
github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
4040
github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
4141
github.com/rogpeppe/go-internal v1.14.1 h1:UQB4HGPB6osV0SQTLymcB4TgvyWu6ZyliaW0tI/otEQ=

internal/mcp/connection.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -248,10 +248,10 @@ func NewHTTPConnection(ctx context.Context, serverID, url string, headers map[st
248248
conn, err = trySSETransport(ctx, cancel, serverID, url, headers, headerClient, keepAlive)
249249
if err == nil {
250250
logger.LogWarn("backend", "⚠️ MCP over SSE has been deprecated. Connected using SSE transport for url=%s. Please migrate to streamable HTTP transport (2025-03-26 spec).", url)
251-
log.Printf("⚠️ WARNING: MCP over SSE (2024-11-05 spec) has been DEPRECATED")
252-
log.Printf("⚠️ The server at %s is using the deprecated SSE transport", url)
253-
log.Printf("⚠️ Please migrate to streamable HTTP transport (2025-03-26 spec)")
254-
log.Printf("Configured HTTP MCP server with SSE transport: %s", url)
251+
logger.LogWarn("backend", "⚠️ WARNING: MCP over SSE (2024-11-05 spec) has been DEPRECATED")
252+
logger.LogWarn("backend", "⚠️ The server at %s is using the deprecated SSE transport", url)
253+
logger.LogWarn("backend", "⚠️ Please migrate to streamable HTTP transport (2025-03-26 spec)")
254+
logger.LogWarn("backend", "Configured HTTP MCP server with SSE transport: %s", url)
255255
return conn, nil
256256
}
257257
logConn.Printf("SSE transport failed: %v", err)

internal/mcp/http_transport.go

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,12 +67,17 @@ func isHTTPConnectionError(err error) bool {
6767
return false
6868
}
6969

70-
// isSessionNotFoundError checks if an error message indicates a backend MCP session has expired
70+
// isSessionNotFoundError checks if an error indicates a backend MCP session has expired
7171
// or is not found. This is used to detect when automatic reconnection to the backend is needed.
72+
// It uses errors.Is to check for sdk.ErrSessionMissing (the typed sentinel introduced in v1.5.0),
73+
// and also falls back to string-matching for non-SDK transports that return plain error messages.
7274
func isSessionNotFoundError(err error) bool {
7375
if err == nil {
7476
return false
7577
}
78+
if errors.Is(err, sdk.ErrSessionMissing) {
79+
return true
80+
}
7681
return strings.Contains(strings.ToLower(err.Error()), "session not found")
7782
}
7883

internal/mcp/http_transport_test.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import (
1212
"time"
1313

1414
"github.com/github/gh-aw-mcpg/internal/oidc"
15+
sdk "github.com/modelcontextprotocol/go-sdk/mcp"
1516
"github.com/stretchr/testify/assert"
1617
"github.com/stretchr/testify/require"
1718
)
@@ -875,6 +876,8 @@ func TestIsSessionNotFoundError(t *testing.T) {
875876
{name: "uppercase returns true", err: fmt.Errorf("Session Not Found"), want: true},
876877
{name: "embedded in longer message returns true", err: fmt.Errorf("Streamable HTTP error: Error POSTing to endpoint: session not found"), want: true},
877878
{name: "session expired message returns false", err: fmt.Errorf("session expired"), want: false},
879+
{name: "sdk ErrSessionMissing sentinel returns true", err: sdk.ErrSessionMissing, want: true},
880+
{name: "wrapped sdk ErrSessionMissing returns true", err: fmt.Errorf("transport failure: %w", sdk.ErrSessionMissing), want: true},
878881
}
879882
for _, tt := range tests {
880883
t.Run(tt.name, func(t *testing.T) {

internal/server/tool_registry.go

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -44,9 +44,12 @@ type launchResult struct {
4444
// The wrapper in this function adapts the three-argument form back to the SDK's two-argument
4545
// form when registering with the SDK server.
4646
//
47-
// NOTE: The Server.AddTool method (used here) skips JSON Schema validation whereas the
48-
// sdk.AddTool function validates the schema. This distinction relies on internal SDK
49-
// behaviour and must be re-verified on every SDK upgrade.
47+
// NOTE: The Server.AddTool method (used here) does not validate tool arguments against the
48+
// input schema at call time, whereas the package-level sdk.AddTool function does. The method
49+
// does require that InputSchema is non-nil and has type "object" (enforced since v1.5.0), but
50+
// it does not validate the argument values — that responsibility belongs to the caller.
51+
// This distinction relies on internal SDK behaviour and must be re-verified on every SDK upgrade.
52+
// Verified correct for go-sdk v1.5.0 (see server.go:Server.AddTool vs AddTool[In,Out]).
5053
func registerToolWithoutValidation(server *sdk.Server, tool *sdk.Tool, handler func(context.Context, *sdk.CallToolRequest, interface{}) (*sdk.CallToolResult, interface{}, error)) {
5154
server.AddTool(tool, func(ctx context.Context, req *sdk.CallToolRequest) (*sdk.CallToolResult, error) {
5255
result, _, err := handler(ctx, req, nil)

0 commit comments

Comments
 (0)