Fix go-sdk integration: content types, error messages, pagination, tool annotations#2647
Conversation
… pagination, and tool annotations Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/12ad1499-dedf-449f-906b-7ec1b9be9ada Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
…llocation Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/12ad1499-dedf-449f-906b-7ec1b9be9ada Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes several MCP gateway integration gaps with modelcontextprotocol/go-sdk, focusing on preserving non-text tool content, improving client-visible tool errors, correctly handling paginated list endpoints, and forwarding tool annotations from backend servers.
Changes:
- Fix
ConvertToCallToolResultto correctly map backendimage,audio, andresourcecontent items into SDK content types (instead of silently producing empty text). - Include error messages as
TextContentin errorCallToolResults so MCP clients receive actionable failure details. - Implement cursor-based pagination loops for tools/resources/prompts listing, aggregating all pages.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/server/unified.go | Adds ToolInfo annotations field and includes error message text content in error CallToolResult helper. |
| internal/server/tool_registry.go | Parses backend tool annotations and forwards them into stored ToolInfo and registered sdk.Tool objects. |
| internal/server/call_tool_result_test.go | Extends tests to assert error message content is present in error CallToolResult. |
| internal/mcp/tool_result.go | Adds conversion support for image/audio/resource content items in backend tool results. |
| internal/mcp/tool_result_test.go | Adds coverage for image/audio/resource conversion and adjusts unknown-type behavior test. |
| internal/mcp/connection.go | Fixes silent truncation by fetching all pages for listTools/listResources/listPrompts via NextCursor. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func newErrorCallToolResult(err error) (*sdk.CallToolResult, interface{}, error) { | ||
| return &sdk.CallToolResult{IsError: true}, nil, err | ||
| return &sdk.CallToolResult{ | ||
| IsError: true, | ||
| Content: []sdk.Content{ | ||
| &sdk.TextContent{Text: err.Error()}, | ||
| }, | ||
| }, nil, err |
There was a problem hiding this comment.
newErrorCallToolResult dereferences err via err.Error() without guarding against a nil error. If this helper is ever called with nil (even accidentally), it will panic. Consider enforcing non-nil (e.g., return a generic error/result when err==nil) or changing the signature/contract to guarantee err is non-nil.
| Name string | ||
| Description string | ||
| InputSchema map[string]interface{} | ||
| Annotations *sdk.ToolAnnotations | ||
| BackendID string // Which backend this tool belongs to |
There was a problem hiding this comment.
ToolInfo now stores Annotations, but routed mode builds per-backend servers from ToolInfo (see createFilteredServer in internal/server/routed.go) and currently does not set sdk.Tool.Annotations when registering those tools. As a result, clients using routed mode still won't receive tool annotations in tools/list. Please plumb ToolInfo.Annotations through when creating the filtered server's sdk.Tool.
| cursor := first.NextCursor | ||
| for cursor != "" { | ||
| result, err := c.getSDKSession().ListTools(c.ctx, &sdk.ListToolsParams{Cursor: cursor}) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return result, err | ||
| }) | ||
| allTools = append(allTools, result.Tools...) | ||
| logConn.Printf("listTools: received page of %d tools (total so far: %d) from serverID=%s", len(result.Tools), len(allTools), c.serverID) | ||
| cursor = result.NextCursor | ||
| } |
There was a problem hiding this comment.
Pagination logic for tools/resources/prompts now loops on NextCursor, but there are no tests asserting that multi-page results are fully aggregated. Given connection.go already has extensive transport/method-dispatch tests, please add coverage that stubs a session returning multiple pages and verifies listTools/listResources/listPrompts return the full combined set.
Four issues identified in the Go Fan audit of
modelcontextprotocol/go-sdkusage — one bug causing silent data loss, three enhancements.Bug: Non-text content silently dropped (
internal/mcp/tool_result.go)ConvertToCallToolResultonly parsedtype+textfields, soimage,audio, andresourcecontent items were replaced with emptyTextContent. Now properly converts tosdk.ImageContent,sdk.AudioContent, andsdk.EmbeddedResource:Data []byteauto-decodes from the base64-encoded JSON field via Go's standard JSON unmarshaler.Error results now include message (
internal/server/unified.go)newErrorCallToolResultreturnedIsError: truewith no content, leaving MCP clients with no explanation. Now includes the error asTextContent.Paginated list methods (
internal/mcp/connection.go)listTools,listResources, andlistPromptseach made a single call, silently dropping tools/resources/prompts beyond the first page. All three now loop onNextCursoruntil exhausted. First-page result used as initial slice capacity to reduce allocations.Forward tool annotations (
internal/server/tool_registry.go)Backend tool
Annotations(readOnlyHint,destructiveHint,openWorldHint, etc.) were not captured duringtools/listparsing. Now forwarded through to the registeredsdk.Tooland stored inToolInfo.Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
example.com/tmp/go-build1361167601/b330/launcher.test /tmp/go-build1361167601/b330/launcher.test -test.testlogfile=/tmp/go-build1361167601/b330/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true -c=4 -nolocalimports -importcfg /tmp/go-build1361167601/b311/importcfg -pack /home/REDACTED/work/gh-aw-mcpg/gh-aw-mcpg/internal/middleware/jqschema.go _linux_amd64.o 64/s�� 64/src/runtime/c-p lzHvcSTIL ache/go/1.25.8/x-lang=go1.25(dns block)/tmp/go-build1147003510/b330/launcher.test /tmp/go-build1147003510/b330/launcher.test -test.testlogfile=/tmp/go-build1147003510/b330/testlog.txt -test.paniconexit0 -test.timeout=10m0s -I ache/go/1.25.8/x64/src/net -I docker-buildx --gdwarf-5 --64 -o docker-buildx(dns block)/tmp/go-build1286029736/b330/launcher.test /tmp/go-build1286029736/b330/launcher.test -test.testlogfile=/tmp/go-build1286029736/b330/testlog.txt -test.paniconexit0 -test.timeout=10m0s -V=f��(dns block)invalid-host-that-does-not-exist-12345.com/tmp/go-build1361167601/b315/config.test /tmp/go-build1361167601/b315/config.test -test.testlogfile=/tmp/go-build1361167601/b315/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true go ternal/fips140/r--64 64/pkg/tool/linu-o(dns block)/tmp/go-build3971546875/b315/config.test /tmp/go-build3971546875/b315/config.test -test.testlogfile=/tmp/go-build3971546875/b315/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true ports/wasi_snapshot_preview1/args.go ports/wasi_snapshot_preview1/clock.go x_amd64/compile -p encoding -lang=go1.25 x_amd64/compile go_.�� ache/go/1.25.8/x64/src/runtime/c-errorsas ache/go/1.25.8/x64/src/crypto/de-ifaceassert x_amd64/vet -I ions =0 x_amd64/vet(dns block)/tmp/go-build1147003510/b315/config.test /tmp/go-build1147003510/b315/config.test -test.testlogfile=/tmp/go-build1147003510/b315/testlog.txt -test.paniconexit0 -test.timeout=10m0s 1167�� /opt/hostedtoolcache/go/1.25.8/x64/src/runtime/c-p 1167601/b165/ .cfg --gdwarf-5 --64 -o as estl�� ache/go/1.25.8/x64/src/net --debug-prefix-map .cfg -I /opt/hostedtoolc--norc -I ache/go/1.25.8/x64/pkg/tool/linu-buildtags(dns block)nonexistent.local/tmp/go-build1361167601/b330/launcher.test /tmp/go-build1361167601/b330/launcher.test -test.testlogfile=/tmp/go-build1361167601/b330/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true -c=4 -nolocalimports -importcfg /tmp/go-build1361167601/b311/importcfg -pack /home/REDACTED/work/gh-aw-mcpg/gh-aw-mcpg/internal/middleware/jqschema.go _linux_amd64.o 64/s�� 64/src/runtime/c-p lzHvcSTIL ache/go/1.25.8/x-lang=go1.25(dns block)/tmp/go-build1147003510/b330/launcher.test /tmp/go-build1147003510/b330/launcher.test -test.testlogfile=/tmp/go-build1147003510/b330/testlog.txt -test.paniconexit0 -test.timeout=10m0s -I ache/go/1.25.8/x64/src/net -I docker-buildx --gdwarf-5 --64 -o docker-buildx(dns block)/tmp/go-build1286029736/b330/launcher.test /tmp/go-build1286029736/b330/launcher.test -test.testlogfile=/tmp/go-build1286029736/b330/testlog.txt -test.paniconexit0 -test.timeout=10m0s -V=f��(dns block)slow.example.com/tmp/go-build1361167601/b330/launcher.test /tmp/go-build1361167601/b330/launcher.test -test.testlogfile=/tmp/go-build1361167601/b330/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true -c=4 -nolocalimports -importcfg /tmp/go-build1361167601/b311/importcfg -pack /home/REDACTED/work/gh-aw-mcpg/gh-aw-mcpg/internal/middleware/jqschema.go _linux_amd64.o 64/s�� 64/src/runtime/c-p lzHvcSTIL ache/go/1.25.8/x-lang=go1.25(dns block)/tmp/go-build1147003510/b330/launcher.test /tmp/go-build1147003510/b330/launcher.test -test.testlogfile=/tmp/go-build1147003510/b330/testlog.txt -test.paniconexit0 -test.timeout=10m0s -I ache/go/1.25.8/x64/src/net -I docker-buildx --gdwarf-5 --64 -o docker-buildx(dns block)/tmp/go-build1286029736/b330/launcher.test /tmp/go-build1286029736/b330/launcher.test -test.testlogfile=/tmp/go-build1286029736/b330/testlog.txt -test.paniconexit0 -test.timeout=10m0s -V=f��(dns block)this-host-does-not-exist-12345.com/tmp/go-build1361167601/b339/mcp.test /tmp/go-build1361167601/b339/mcp.test -test.testlogfile=/tmp/go-build1361167601/b339/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 64/src/runtime/cgo 64/src/vendor/golang.org/x/text/--64 64/pkg/tool/linux_amd64/vet(dns block)/tmp/go-build1147003510/b339/mcp.test /tmp/go-build1147003510/b339/mcp.test -test.testlogfile=/tmp/go-build1147003510/b339/testlog.txt -test.paniconexit0 -test.timeout=10m0s(dns block)/tmp/go-build1286029736/b339/mcp.test /tmp/go-build1286029736/b339/mcp.test -test.testlogfile=/tmp/go-build1286029736/b339/testlog.txt -test.paniconexit0 -test.timeout=10m0s -uns�� -unreachable=false /tmp/go-build1361167601/b221/vet.cfg ache/go/1.25.8/x64/pkg/tool/linux_amd64/vet go ternal/fips140/a10 64/pkg/tool/linu--64 ache/go/1.25.8/x64/pkg/tool/linux_amd64/vet -qE (create|run) 64/pkg/tool/linu-I ker/cli-plugins/docker-compose go lang.org/x/text/cat-file x_amd64/vet ker/cli-plugins/blob(dns block)If you need me to access, download, or install something from one of these locations, you can either:
💬 Send tasks to Copilot coding agent from Slack and Teams to turn conversations into code. Copilot posts an update in your thread when it's finished.