Conversation
…r code Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Analyze and document duplicate code patterns in Go files
refactor: eliminate duplicate StreamableHTTP session callback and SDK list method wrappers
Feb 25, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors duplicated MCP StreamableHTTP session-establishment logic and repeated SDK “list” method wrappers to reduce drift risk and centralize shared behavior in the server and MCP connection layers.
Changes:
- Introduces
setupSessionCallbackininternal/server/http_helpers.goand switches unified/routed StreamableHTTP handlers to use it. - Removes the now-redundant inline session callback code in
internal/server/transport.goandinternal/server/routed.go(including dropping an unused import intransport.go). - Adds
Connection.callListMethodininternal/mcp/connection.goto consolidate therequireSession → SDK call → marshalToResponsepattern for list operations.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| internal/server/transport.go | Replaces inline StreamableHTTP session setup with setupSessionCallback, removes unused sanitize import. |
| internal/server/routed.go | Replaces inline session setup with setupSessionCallback; obtains sessionID from request context for caching. |
| internal/server/http_helpers.go | Adds setupSessionCallback helper to centralize extraction/logging/body logging/context injection for session establishment. |
| internal/mcp/connection.go | Adds callListMethod helper and refactors listTools/listResources/listPrompts to use it. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+112
to
+125
| if backendID != "" { | ||
| logger.LogInfo("client", "New MCP client connection, remote=%s, method=%s, path=%s, backend=%s, session=%s", | ||
| r.RemoteAddr, r.Method, r.URL.Path, backendID, sessionID) | ||
| log.Printf("=== NEW STREAMABLE HTTP CONNECTION (ROUTED) ===") | ||
| } else { | ||
| logger.LogInfo("client", "MCP connection established, remote=%s, method=%s, path=%s, session=%s", | ||
| r.RemoteAddr, r.Method, r.URL.Path, sessionID) | ||
| log.Printf("=== NEW STREAMABLE HTTP CONNECTION ===") | ||
| } | ||
| log.Printf("[%s] %s %s", r.RemoteAddr, r.Method, r.URL.Path) | ||
| if backendID != "" { | ||
| log.Printf("Backend: %s", backendID) | ||
| } | ||
| log.Printf("Authorization (Session ID): %s", sanitize.TruncateSecret(sessionID)) |
There was a problem hiding this comment.
setupSessionCallback logs the raw sessionID in logger.LogInfo (and uses it in formatted messages). Since the session ID is derived from the Authorization header (often an API key), this can leak credentials into production logs. Use a truncated/sanitized form (e.g., sanitize.TruncateSecret(sessionID)) for all log output and avoid emitting the full value at info level.
Suggested change
| if backendID != "" { | |
| logger.LogInfo("client", "New MCP client connection, remote=%s, method=%s, path=%s, backend=%s, session=%s", | |
| r.RemoteAddr, r.Method, r.URL.Path, backendID, sessionID) | |
| log.Printf("=== NEW STREAMABLE HTTP CONNECTION (ROUTED) ===") | |
| } else { | |
| logger.LogInfo("client", "MCP connection established, remote=%s, method=%s, path=%s, session=%s", | |
| r.RemoteAddr, r.Method, r.URL.Path, sessionID) | |
| log.Printf("=== NEW STREAMABLE HTTP CONNECTION ===") | |
| } | |
| log.Printf("[%s] %s %s", r.RemoteAddr, r.Method, r.URL.Path) | |
| if backendID != "" { | |
| log.Printf("Backend: %s", backendID) | |
| } | |
| log.Printf("Authorization (Session ID): %s", sanitize.TruncateSecret(sessionID)) | |
| sanitizedSessionID := sanitize.TruncateSecret(sessionID) | |
| if backendID != "" { | |
| logger.LogInfo("client", "New MCP client connection, remote=%s, method=%s, path=%s, backend=%s, session=%s", | |
| r.RemoteAddr, r.Method, r.URL.Path, backendID, sanitizedSessionID) | |
| log.Printf("=== NEW STREAMABLE HTTP CONNECTION (ROUTED) ===") | |
| } else { | |
| logger.LogInfo("client", "MCP connection established, remote=%s, method=%s, path=%s, session=%s", | |
| r.RemoteAddr, r.Method, r.URL.Path, sanitizedSessionID) | |
| log.Printf("=== NEW STREAMABLE HTTP CONNECTION ===") | |
| } | |
| log.Printf("[%s] %s %s", r.RemoteAddr, r.Method, r.URL.Path) | |
| if backendID != "" { | |
| log.Printf("Backend: %s", backendID) | |
| } | |
| log.Printf("Authorization (Session ID): %s", sanitizedSessionID) |
Comment on lines
108
to
116
| routeHandler := sdk.NewStreamableHTTPHandler(func(r *http.Request) *sdk.Server { | ||
| // Extract and validate session ID from Authorization header | ||
| sessionID := extractAndValidateSession(r) | ||
| if sessionID == "" { | ||
| if _, ok := setupSessionCallback(r, backendID); !ok { | ||
| return nil | ||
| } | ||
|
|
||
| logger.LogInfo("client", "New MCP client connection, remote=%s, method=%s, path=%s, backend=%s, session=%s", | ||
| r.RemoteAddr, r.Method, r.URL.Path, backendID, sessionID) | ||
| log.Printf("=== NEW STREAMABLE HTTP CONNECTION (ROUTED) ===") | ||
| log.Printf("[%s] %s %s", r.RemoteAddr, r.Method, r.URL.Path) | ||
| log.Printf("Backend: %s", backendID) | ||
| log.Printf("Authorization (Session ID): %s", sessionID) | ||
|
|
||
| // Log request body for debugging | ||
| logHTTPRequestBody(r, sessionID, backendID) | ||
|
|
||
| // Store session ID and backend ID in request context | ||
| *r = *injectSessionContext(r, sessionID, backendID) | ||
| log.Printf("✓ Injected session ID and backend ID into context") | ||
| log.Printf("===================================\n") | ||
|
|
||
| // Return a cached filtered proxy server for this backend and session | ||
| // This ensures the same server instance is reused for all requests in a session | ||
| sessionID := r.Context().Value(SessionIDContextKey).(string) | ||
| return serverCache.getOrCreate(backendID, sessionID, func() *sdk.Server { |
There was a problem hiding this comment.
In routed mode, setupSessionCallback already returns the sessionID, but this handler discards it and then re-fetches it from the request context with a type assertion. Capturing the returned sessionID avoids the extra context lookup and removes a potential panic if the context value ever changes or is missing.
See below for a potential fix:
sessionID, ok := setupSessionCallback(r, backendID)
if !ok {
return nil
}
// Return a cached filtered proxy server for this backend and session
// This ensures the same server instance is reused for all requests in a session
Comment on lines
+101
to
+134
| // setupSessionCallback performs common session establishment for StreamableHTTP handlers. | ||
| // It extracts and validates the session ID, logs connection details, logs the request body, | ||
| // and injects session context into the request. The backendID parameter is optional | ||
| // and can be empty for unified mode. Returns false if the request should be rejected | ||
| // (empty session ID). | ||
| func setupSessionCallback(r *http.Request, backendID string) (string, bool) { | ||
| sessionID := extractAndValidateSession(r) | ||
| if sessionID == "" { | ||
| return "", false | ||
| } | ||
|
|
||
| if backendID != "" { | ||
| logger.LogInfo("client", "New MCP client connection, remote=%s, method=%s, path=%s, backend=%s, session=%s", | ||
| r.RemoteAddr, r.Method, r.URL.Path, backendID, sessionID) | ||
| log.Printf("=== NEW STREAMABLE HTTP CONNECTION (ROUTED) ===") | ||
| } else { | ||
| logger.LogInfo("client", "MCP connection established, remote=%s, method=%s, path=%s, session=%s", | ||
| r.RemoteAddr, r.Method, r.URL.Path, sessionID) | ||
| log.Printf("=== NEW STREAMABLE HTTP CONNECTION ===") | ||
| } | ||
| log.Printf("[%s] %s %s", r.RemoteAddr, r.Method, r.URL.Path) | ||
| if backendID != "" { | ||
| log.Printf("Backend: %s", backendID) | ||
| } | ||
| log.Printf("Authorization (Session ID): %s", sanitize.TruncateSecret(sessionID)) | ||
|
|
||
| logHTTPRequestBody(r, sessionID, backendID) | ||
|
|
||
| *r = *injectSessionContext(r, sessionID, backendID) | ||
| log.Printf("✓ Injected session ID into context") | ||
| log.Printf("===================================\n") | ||
|
|
||
| return sessionID, true | ||
| } |
There was a problem hiding this comment.
setupSessionCallback is new behavior centralizing session establishment, logging, request body restoration, and context injection, but internal/server/http_helpers_test.go currently doesn't cover it. Add focused tests for: (1) missing Authorization header returns ok=false and does not modify context/body; (2) valid header returns sessionID and injects SessionIDContextKey (and backend-id when provided); (3) POST body is still readable after the callback.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two near-identical code patterns existed across
transport.go/routed.goandconnection.go, creating a "fix in one place, forget the other" failure mode. The session callback duplication also had a security-relevant inconsistency:routed.gologged raw session IDs whiletransport.gousedsanitize.TruncateSecret.Pattern 1: Session setup callback (
transport.go+routed.go)Re-introduces
setupSessionCallbackinhttp_helpers.go, consolidating ~20 duplicated lines from both StreamableHTTP handler callbacks into a single helper:Also removes the now-unused
sanitizeimport fromtransport.go.Pattern 2: SDK list method wrappers (
connection.go)Extracts a
callListMethodhelper for the repeatedrequireSession → SDK call → marshalToResponsepattern acrosslistTools,listResources, andlistPrompts:Per-method count logging is preserved inside each closure since it references method-specific result fields.
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-build2204598335/b280/launcher.test /tmp/go-build2204598335/b280/launcher.test -test.testlogfile=/tmp/go-build2204598335/b280/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true go riter/tabwriter.--64 64/pkg/tool/linu-o r(dns block)/tmp/go-build391587417/b280/launcher.test /tmp/go-build391587417/b280/launcher.test -test.testlogfile=/tmp/go-build391587417/b280/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true /difc/agent.go /difc/capabilities.go x_amd64/link --with-filename func marshalToResponse /home/REDACTED/work/gh-aw-mcpg/gh-b7Byqgxqkr-cN-7rSOp1/b7Byqgxqkr-cN-7rSOp1 x_amd64/link 4598�� 64/src/net ache/go/1.25.7/x64/src/crypto/in-nolocalimports 4598335/b171/ -p weak -lang=go1.25 OI/f_Y5u0Vsu1Q6lRjIvans/P_v2VhJmRvDLOu0RfjOj(dns block)invalid-host-that-does-not-exist-12345.com/tmp/go-build2204598335/b262/config.test /tmp/go-build2204598335/b262/config.test -test.testlogfile=/tmp/go-build2204598335/b262/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true go(dns block)nonexistent.local/tmp/go-build2204598335/b280/launcher.test /tmp/go-build2204598335/b280/launcher.test -test.testlogfile=/tmp/go-build2204598335/b280/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true go riter/tabwriter.--64 64/pkg/tool/linu-o r(dns block)/tmp/go-build391587417/b280/launcher.test /tmp/go-build391587417/b280/launcher.test -test.testlogfile=/tmp/go-build391587417/b280/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true /difc/agent.go /difc/capabilities.go x_amd64/link --with-filename func marshalToResponse /home/REDACTED/work/gh-aw-mcpg/gh-b7Byqgxqkr-cN-7rSOp1/b7Byqgxqkr-cN-7rSOp1 x_amd64/link 4598�� 64/src/net ache/go/1.25.7/x64/src/crypto/in-nolocalimports 4598335/b171/ -p weak -lang=go1.25 OI/f_Y5u0Vsu1Q6lRjIvans/P_v2VhJmRvDLOu0RfjOj(dns block)slow.example.com/tmp/go-build2204598335/b280/launcher.test /tmp/go-build2204598335/b280/launcher.test -test.testlogfile=/tmp/go-build2204598335/b280/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true go riter/tabwriter.--64 64/pkg/tool/linu-o r(dns block)/tmp/go-build391587417/b280/launcher.test /tmp/go-build391587417/b280/launcher.test -test.testlogfile=/tmp/go-build391587417/b280/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true /difc/agent.go /difc/capabilities.go x_amd64/link --with-filename func marshalToResponse /home/REDACTED/work/gh-aw-mcpg/gh-b7Byqgxqkr-cN-7rSOp1/b7Byqgxqkr-cN-7rSOp1 x_amd64/link 4598�� 64/src/net ache/go/1.25.7/x64/src/crypto/in-nolocalimports 4598335/b171/ -p weak -lang=go1.25 OI/f_Y5u0Vsu1Q6lRjIvans/P_v2VhJmRvDLOu0RfjOj(dns block)this-host-does-not-exist-12345.com/tmp/go-build2204598335/b289/mcp.test /tmp/go-build2204598335/b289/mcp.test -test.testlogfile=/tmp/go-build2204598335/b289/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true -b 64/src/internal/-fno-stack-protector x_amd64/vet --depth 2 REDACTED x_amd64/vet abis�� 64/src/runtime/c-errorsas --global 64/bin/as user.name record(dns block)/tmp/go-build391587417/b289/mcp.test /tmp/go-build391587417/b289/mcp.test -test.testlogfile=/tmp/go-build391587417/b289/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true s iJR4/CmZNTCtTejDoWTZLiJR4 86_64/as(dns block)If you need me to access, download, or install something from one of these locations, you can either:
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.