Conversation
… in mcptest, add filteredServerCache max-size - Extract registerToolWithoutValidation helper in tool_registry.go to deduplicate the server.AddTool bypass pattern (with long comment) from tool_registry.go and routed.go - Use mcp.ParseToolArguments in mcptest/server.go instead of inline json.Unmarshal - Add maxSize field (default 1000) to filteredServerCache with LRU eviction to prevent unbounded memory growth - Add tests: TestFilteredServerCache_MaxSize, TestFilteredServerCache_TTLEviction, TestRegisterToolWithoutValidation Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/8fb7f3cd-ebb2-4acd-b60e-77bb0297dc9f Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR tightens up the Go SDK integration by deduplicating the “schema-validation bypass” tool registration pattern, standardizing tool argument parsing in test utilities, and adding a safety bound to routed-mode’s per-session filtered server cache to prevent unbounded growth.
Changes:
- Extract
registerToolWithoutValidationhelper and reuse it for tool registration in unified and routed flows. - Switch mcptest tool handler argument parsing to
mcp.ParseToolArgumentsfor consistency and safer nil handling. - Cap
filteredServerCachesize (default 1000) with LRU eviction; add unit tests for size cap, TTL eviction, and helper behavior.
Show a summary per file
| File | Description |
|---|---|
| internal/testutil/mcptest/server.go | Uses shared mcp.ParseToolArguments helper instead of inline JSON unmarshal. |
| internal/server/tool_registry.go | Adds registerToolWithoutValidation helper and uses it during backend tool registration. |
| internal/server/routed.go | Adds filteredServerCache max-size cap with LRU eviction; uses the new tool-registration helper. |
| internal/server/routed_test.go | Adds tests for cache max-size/LRU behavior, TTL eviction, and helper forwarding via in-memory transport. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 4
| IsError: true, | ||
| Content: []sdk.Content{ | ||
| &sdk.TextContent{ | ||
| Text: fmt.Sprintf("Failed to parse arguments: %v", err), |
There was a problem hiding this comment.
The error text here will likely read "Failed to parse arguments: failed to parse arguments: ..." because mcp.ParseToolArguments already wraps errors with "failed to parse arguments". Consider changing this message to avoid repeating the same phrase (e.g., "Failed to parse tool arguments" or just surface err.Error()).
| Text: fmt.Sprintf("Failed to parse arguments: %v", err), | |
| Text: fmt.Sprintf("Failed to parse tool arguments: %v", err), |
| // Enforce max-size limit: evict the least-recently-used entry when at capacity. | ||
| if len(c.servers) >= c.maxSize { | ||
| var lruKey string | ||
| var lruTime time.Time | ||
| for k, entry := range c.servers { | ||
| if lruKey == "" || entry.lastUsed.Before(lruTime) { | ||
| lruKey = k | ||
| lruTime = entry.lastUsed | ||
| } | ||
| } | ||
| logRouted.Printf("[CACHE] Max size reached (%d), evicting LRU entry: key=%s", c.maxSize, lruKey) | ||
| delete(c.servers, lruKey) |
There was a problem hiding this comment.
Max-size eviction can remove entries even if they haven't reached the TTL, meaning a still-valid (backend, session) may get a new *sdk.Server instance mid-session. Since routed mode comments indicate the same server instance is reused per session, consider confirming that recreating the filtered server early is safe for StreamableHTTP session semantics (or only evict entries that are already expired / explicitly close the evicted session/server).
| // Enforce max-size limit: evict the least-recently-used entry when at capacity. | |
| if len(c.servers) >= c.maxSize { | |
| var lruKey string | |
| var lruTime time.Time | |
| for k, entry := range c.servers { | |
| if lruKey == "" || entry.lastUsed.Before(lruTime) { | |
| lruKey = k | |
| lruTime = entry.lastUsed | |
| } | |
| } | |
| logRouted.Printf("[CACHE] Max size reached (%d), evicting LRU entry: key=%s", c.maxSize, lruKey) | |
| delete(c.servers, lruKey) | |
| // Do not evict non-expired entries just to satisfy maxSize. Routed mode relies on | |
| // reusing the same filtered server instance for a given (backend, session), and | |
| // evicting an active entry here could recreate that server mid-session. | |
| if len(c.servers) >= c.maxSize { | |
| logRouted.Printf("[CACHE] Max size reached (%d), retaining active entries and allowing cache growth until TTL eviction", c.maxSize) |
| ttl := 10 * time.Millisecond | ||
| cache := newFilteredServerCache(ttl) | ||
|
|
||
| callCount := 0 | ||
| creator := func() *sdk.Server { | ||
| callCount++ | ||
| return sdk.NewServer(&sdk.Implementation{Name: "test", Version: "1.0"}, &sdk.ServerOptions{}) | ||
| } | ||
|
|
||
| // Add an entry | ||
| cache.getOrCreate("backend", "session1", creator) | ||
| assert.Equal(1, callCount) | ||
| assert.Equal(1, len(cache.servers)) | ||
|
|
||
| // Wait for TTL to expire | ||
| time.Sleep(20 * time.Millisecond) | ||
|
|
||
| // Next call should evict the expired entry and create a new one | ||
| cache.getOrCreate("backend", "session2", creator) | ||
| assert.Equal(2, callCount, "Should have created a new server after TTL eviction") | ||
|
|
There was a problem hiding this comment.
This TTL eviction test relies on wall-clock timing (10ms TTL + 20ms sleep), which can be flaky under load/slow CI. Consider using a fake clock / injecting time.Now into filteredServerCache, or increasing the TTL/sleep margins to make the test more robust.
| logRouted.Printf("[CACHE] Max size reached (%d), evicting LRU entry: key=%s", c.maxSize, lruKey) | ||
| delete(c.servers, lruKey) |
There was a problem hiding this comment.
This log line includes the full cache key, which embeds the raw session ID (often derived from Authorization / API key). To avoid leaking secrets into logs, consider truncating/redacting the session portion (e.g., using internal/auth.TruncateSessionID) or logging only the backend ID + a hashed/truncated session identifier.
- Fix duplicate error wrapping in mcptest: change 'Failed to parse arguments' to 'Failed to parse tool arguments' since ParseToolArguments already wraps with 'failed to parse arguments' - Remove LRU eviction of active cache entries: routed mode relies on reusing the same filtered server instance per (backend, session), so evicting non-expired entries could break StreamableHTTP session semantics. Instead log a warning and allow cache growth until TTL eviction. - Truncate session IDs in cache log messages using auth.TruncateSessionID to avoid leaking secrets into debug logs - Increase TTL test margins (10ms→100ms TTL, 20ms→200ms sleep) to reduce flakiness under CI load - Fix search_repositories URL encoding (url.QueryEscape) for query param Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Three quick-win improvements identified in the go-sdk module review: duplicate
server.AddToolbypass pattern, inconsistent argument parsing in mcptest, and unboundedfilteredServerCachegrowth.Changes
registerToolWithoutValidationhelper (tool_registry.go): Extracts the schema-validation-bypass pattern that was duplicated—with identical long comments—in bothtool_registry.goandrouted.gointo a single documented helper:mcp.ParseToolArgumentsin mcptest (mcptest/server.go): Replaces inlinejson.Unmarshalwith the existing helper, consistent with the rest of the codebase. The helper also handlesnilparams correctly, which the inline code didn't.filteredServerCachemax-size cap (routed.go): Adds amaxSizefield (default 1000) with LRU eviction on insert when at capacity. The existing TTL eviction handles the normal case; this is a safety bound against session-explosion scenarios.Tests
New unit tests in
routed_test.go:TestFilteredServerCache_MaxSize(verifies LRU eviction at capacity),TestFilteredServerCache_TTLEviction, andTestRegisterToolWithoutValidation(end-to-end via in-memory transports).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-build1930381889/b510/launcher.test /tmp/go-build1930381889/b510/launcher.test -test.testlogfile=/tmp/go-build1930381889/b510/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true ache/go/1.25.8/x-errorsas ache/go/1.25.8/x-ifaceassert ache/go/1.25.8/x-nilfunc -p google.golang.or-atomic -lang=go1.24 ache/go/1.25.8/x-buildtags go_.�� 64/src/net 0381889/b008/vet-ifaceassert x_amd64/compile -p path/filepath -lang=go1.25 x_amd64/compile(dns block)/tmp/go-build891657139/b514/launcher.test /tmp/go-build891657139/b514/launcher.test -test.testlogfile=/tmp/go-build891657139/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s n-me�� -bool -buildtags ache/go/1.25.8/x64/pkg/tool/linux_amd64/vet -errorsas -ifaceassert -nilfunc gYHi0jJAQUNV /usr�� --version -tests 2fdf923968a1d42a-d ternal/engine/wa/tmp/go-build891657139/b502/difc.test ternal/engine/wa-test.testlogfile=/tmp/go-build891657139/b502/testlog.txt x_amd64/compile x_amd64/vet(dns block)invalid-host-that-does-not-exist-12345.com/tmp/go-build1930381889/b492/config.test /tmp/go-build1930381889/b492/config.test -test.testlogfile=/tmp/go-build1930381889/b492/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 1024825-9d38bb4040a9/httpbody/httpbody.pb.go .cfg x_amd64/compile(dns block)/tmp/go-build891657139/b496/config.test /tmp/go-build891657139/b496/config.test -test.testlogfile=/tmp/go-build891657139/b496/testlog.txt -test.paniconexit0 -test.timeout=10m0s -ato�� -bool -buildtags 64/pkg/tool/linux_amd64/vet -errorsas -ifaceassert -nilfunc /testutil/mcptest -ato�� -bool -buildtags /usr/sbin/bash -errorsas -ifaceassert -nilfunc bash(dns block)nonexistent.local/tmp/go-build1930381889/b510/launcher.test /tmp/go-build1930381889/b510/launcher.test -test.testlogfile=/tmp/go-build1930381889/b510/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true ache/go/1.25.8/x-errorsas ache/go/1.25.8/x-ifaceassert ache/go/1.25.8/x-nilfunc -p google.golang.or-atomic -lang=go1.24 ache/go/1.25.8/x-buildtags go_.�� 64/src/net 0381889/b008/vet-ifaceassert x_amd64/compile -p path/filepath -lang=go1.25 x_amd64/compile(dns block)/tmp/go-build891657139/b514/launcher.test /tmp/go-build891657139/b514/launcher.test -test.testlogfile=/tmp/go-build891657139/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s n-me�� -bool -buildtags ache/go/1.25.8/x64/pkg/tool/linux_amd64/vet -errorsas -ifaceassert -nilfunc gYHi0jJAQUNV /usr�� --version -tests 2fdf923968a1d42a-d ternal/engine/wa/tmp/go-build891657139/b502/difc.test ternal/engine/wa-test.testlogfile=/tmp/go-build891657139/b502/testlog.txt x_amd64/compile x_amd64/vet(dns block)slow.example.com/tmp/go-build1930381889/b510/launcher.test /tmp/go-build1930381889/b510/launcher.test -test.testlogfile=/tmp/go-build1930381889/b510/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true ache/go/1.25.8/x-errorsas ache/go/1.25.8/x-ifaceassert ache/go/1.25.8/x-nilfunc -p google.golang.or-atomic -lang=go1.24 ache/go/1.25.8/x-buildtags go_.�� 64/src/net 0381889/b008/vet-ifaceassert x_amd64/compile -p path/filepath -lang=go1.25 x_amd64/compile(dns block)/tmp/go-build891657139/b514/launcher.test /tmp/go-build891657139/b514/launcher.test -test.testlogfile=/tmp/go-build891657139/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s n-me�� -bool -buildtags ache/go/1.25.8/x64/pkg/tool/linux_amd64/vet -errorsas -ifaceassert -nilfunc gYHi0jJAQUNV /usr�� --version -tests 2fdf923968a1d42a-d ternal/engine/wa/tmp/go-build891657139/b502/difc.test ternal/engine/wa-test.testlogfile=/tmp/go-build891657139/b502/testlog.txt x_amd64/compile x_amd64/vet(dns block)this-host-does-not-exist-12345.com/tmp/go-build1930381889/b519/mcp.test /tmp/go-build1930381889/b519/mcp.test -test.testlogfile=/tmp/go-build1930381889/b519/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true @v1.43.0/attribute/doc.go @v1.43.0/attribute/encoder.go x_amd64/asm -o /dev/null /tmp/ccoRgC1w.s x_amd64/asm -I g_.a -I x_amd64/vet --gdwarf-5 nternal/telemetrdocker-cli-plugin-metadata -o x_amd64/vet(dns block)/tmp/go-build891657139/b523/mcp.test /tmp/go-build891657139/b523/mcp.test -test.testlogfile=/tmp/go-build891657139/b523/testlog.txt -test.paniconexit0 -test.timeout=10m0s insp�� 0381889/b557/_pkg_.a {{json .Mounts}} x_amd64/vet ache/go/1.25.8/xbash TOnX/oKI-cIQvxuH/usr/bin/runc 0381889/b171/ x_amd64/vet /usr�� 9fZi/McNztBWEc4wDV21h9fZi 0381889/b171/_x0-buildtags x_amd64/vet 64/src/net ache/go/1.25.8/x/usr/bin/runc cal/bin/as x_amd64/vet(dns block)If you need me to access, download, or install something from one of these locations, you can either: