Skip to content

refactor(server): extract buildMCPHandler to deduplicate handler construction#5101

Merged
lpcox merged 4 commits intomainfrom
copilot/fix-duplicate-code-streamable-handler
May 4, 2026
Merged

refactor(server): extract buildMCPHandler to deduplicate handler construction#5101
lpcox merged 4 commits intomainfrom
copilot/fix-duplicate-code-streamable-handler

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 4, 2026

Both transport.go (unified mode) and routed.go (routed mode) repeated the same three-step handler stack construction, meaning any middleware chain change required edits in two places.

Changes

  • internal/server/http_helpers.go — adds buildMCPHandler helper and a mcpHandlerConfig options struct that encapsulates the full stack:
    type mcpHandlerConfig struct {
        handlerLog     *logger.Logger
        sessionTimeout time.Duration
        logTag         string
        unifiedServer  *UnifiedServer
        apiKey         string
        hmacSecret     string
    }
    
    func buildMCPHandler(serverFactory func(*http.Request) *sdk.Server, cfg mcpHandlerConfig) http.Handler {
        h := sdk.NewStreamableHTTPHandler(serverFactory, &sdk.StreamableHTTPOptions{
            Stateless:      false,
            Logger:         logger.NewSlogLoggerWithHandler(cfg.handlerLog),
            SessionTimeout: cfg.sessionTimeout,
        })
        return wrapWithMiddleware(WrapWithSessionAutoInit(h), cfg.logTag, cfg.unifiedServer, cfg.apiKey, cfg.hmacSecret)
    }
  • transport.go — replaces inline 3-step construction with a single buildMCPHandler call using mcpHandlerConfig; uses getSessionTimeout() introduced on main
  • routed.go — same, inside the per-backend loop

Copilot AI changed the title [WIP] Fix duplicate code pattern in StreamableHTTP handler refactor(server): extract buildMCPHandler to deduplicate handler construction May 4, 2026
Copilot finished work on behalf of lpcox May 4, 2026 17:20
Copilot AI requested a review from lpcox May 4, 2026 17:20
@lpcox lpcox marked this pull request as ready for review May 4, 2026 17:20
Copilot AI review requested due to automatic review settings May 4, 2026 17:20
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors the internal MCP HTTP server setup by extracting the repeated “StreamableHTTP handler + session auto-init + standard middleware chain” construction into a shared helper, so unified and routed server modes stay in sync when the handler stack changes.

Changes:

  • Added buildMCPHandler helper to construct the standard MCP handler stack in one place.
  • Updated unified mode (transport.go) to use buildMCPHandler instead of inline construction.
  • Updated routed mode (routed.go) to use buildMCPHandler inside the per-backend route loop.
Show a summary per file
File Description
internal/server/http_helpers.go Introduces buildMCPHandler to centralize the StreamableHTTP + auto-init + middleware construction.
internal/server/transport.go Replaces the unified-mode inline handler stack setup with a buildMCPHandler call.
internal/server/routed.go Replaces the routed-mode inline handler stack setup with a buildMCPHandler call per backend route.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 3/3 changed files
  • Comments generated: 2

Comment thread internal/server/http_helpers.go Outdated
Comment on lines +179 to +183
func buildMCPHandler(
serverFactory func(*http.Request) *sdk.Server,
log *logger.Logger,
sessionTimeout time.Duration,
logTag string,
Comment thread internal/server/http_helpers.go Outdated
Comment on lines +183 to +187
logTag string,
unifiedServer *UnifiedServer,
apiKey, hmacSecret string,
) http.Handler {
h := sdk.NewStreamableHTTPHandler(serverFactory, &sdk.StreamableHTTPOptions{
@lpcox
Copy link
Copy Markdown
Collaborator

lpcox commented May 4, 2026

@copilot address the review feedback and fix the failing integration test ci check

Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 4, 2026

@copilot address the review feedback and fix the failing integration test ci check

Addressed both review comments in commit bd62028:

  1. Renamed log parameter to handlerLog in buildMCPHandler to eliminate the shadowing of the stdlib log package.

  2. Introduced mcpHandlerConfig struct — replaces the six positional parameters (including three string fields). Call sites now use named fields, making the intent of each argument immediately clear.

Regarding the CI failures (TestBinaryInvocation_LogFileCreation, TestBinaryInvocation_LogDirEnvironmentVariable, TestDIFCConfigWithGuards): these require Docker on the runner and are also failing on main (e.g. run 25333265199) — they're pre-existing and unrelated to this PR.

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
    • Triggering command: /tmp/go-build3384203968/b513/launcher.test /tmp/go-build3384203968/b513/launcher.test -test.testlogfile=/tmp/go-build3384203968/b513/testlog.txt -test.paniconexit0 -test.timeout=10m0s -I ache/go/1.25.9/x64/src/runtime/cgo k/gh-aw-mcpg/gh-aw-mcpg/internal/guard/guard.go x_amd64/vet --gdwarf-5 --64 -o x_amd64/vet go_.�� 64/src/net -trimpath x_amd64/vet -p google.golang.or--version -lang=go1.24 x_amd64/vet (dns block)
  • invalid-host-that-does-not-exist-12345.com
    • Triggering command: /tmp/go-build3384203968/b495/config.test /tmp/go-build3384203968/b495/config.test -test.testlogfile=/tmp/go-build3384203968/b495/testlog.txt -test.paniconexit0 -test.timeout=10m0s --no�� 4027478/b381/_pkg_.a 64.o x_amd64/vet go g/grpc/internal 64/pkg/tool/linu-o x_amd64/vet -o ache/go/1.25.9/x64/src/runtime/cgo -trimpath .cfg -p go.opentelemetry/tmp/go-build1059822778/b421/vet.cfg -lang=go1.25 2805010/b183/_x001.o (dns block)
  • nonexistent.local
    • Triggering command: /tmp/go-build3384203968/b513/launcher.test /tmp/go-build3384203968/b513/launcher.test -test.testlogfile=/tmp/go-build3384203968/b513/testlog.txt -test.paniconexit0 -test.timeout=10m0s -I ache/go/1.25.9/x64/src/runtime/cgo k/gh-aw-mcpg/gh-aw-mcpg/internal/guard/guard.go x_amd64/vet --gdwarf-5 --64 -o x_amd64/vet go_.�� 64/src/net -trimpath x_amd64/vet -p google.golang.or--version -lang=go1.24 x_amd64/vet (dns block)
  • slow.example.com
    • Triggering command: /tmp/go-build3384203968/b513/launcher.test /tmp/go-build3384203968/b513/launcher.test -test.testlogfile=/tmp/go-build3384203968/b513/testlog.txt -test.paniconexit0 -test.timeout=10m0s -I ache/go/1.25.9/x64/src/runtime/cgo k/gh-aw-mcpg/gh-aw-mcpg/internal/guard/guard.go x_amd64/vet --gdwarf-5 --64 -o x_amd64/vet go_.�� 64/src/net -trimpath x_amd64/vet -p google.golang.or--version -lang=go1.24 x_amd64/vet (dns block)
  • this-host-does-not-exist-12345.com
    • Triggering command: /tmp/go-build3384203968/b522/mcp.test /tmp/go-build3384203968/b522/mcp.test -test.testlogfile=/tmp/go-build3384203968/b522/testlog.txt -test.paniconexit0 -test.timeout=10m0s -o 64/src/net pkg/mod/google.golang.org/grpc@v1.80.0/health/gr--64 x_amd64/vet -p google.golang.or--version -lang=go1.23 x_amd64/vet ap 2805010/b183=/tmp/go-build ap x_amd64/vet -p ache/go/1.25.9/x--version -lang=go1.24 Xw-CfdF/xvP6fpghx9XWVDxnKAFI (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot finished work on behalf of lpcox May 4, 2026 17:35
@lpcox
Copy link
Copy Markdown
Collaborator

lpcox commented May 4, 2026

@copilot merge main

…-code-streamable-handler

# Conflicts:
#	internal/server/transport.go

Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 4, 2026

@copilot merge main

Done — merged main in commit b6d5b453. The only conflict was in transport.go (our mcpHandlerConfig vs. main's inline SDK options); resolved by keeping mcpHandlerConfig and adopting getSessionTimeout() that main introduced.

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
    • Triggering command: /tmp/go-build395042731/b513/launcher.test /tmp/go-build395042731/b513/launcher.test -test.testlogfile=/tmp/go-build395042731/b513/testlog.txt -test.paniconexit0 -test.timeout=10m0s 1521�� 64/src/net -trimpath x_amd64/compile -p google.golang.ordocker-cli-plugin-metadata -lang=go1.24 x_amd64/compile .cfg�� /opt/hostedtoolcache/go/1.25.9/x64/src/runtime/c-m 1521424/b166/ x_amd64/vet --gdwarf-5 --64 -o x_amd64/vet (dns block)
  • invalid-host-that-does-not-exist-12345.com
    • Triggering command: /tmp/go-build395042731/b495/config.test /tmp/go-build395042731/b495/config.test -test.testlogfile=/tmp/go-build395042731/b495/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build395042731/b398/vet.cfg ache/go/1.25.9/x64/src/runtime/cgo -o/tmp/go-build3728664920/b316//_cgo_.o x_amd64/vet -c ions =0 x_amd64/vet go_.�� ache/go/1.25.9/x-errorsas 1521424/b172/ x_amd64/vet internal/gen.go internal/partial-atomic -o x_amd64/vet (dns block)
  • nonexistent.local
    • Triggering command: /tmp/go-build395042731/b513/launcher.test /tmp/go-build395042731/b513/launcher.test -test.testlogfile=/tmp/go-build395042731/b513/testlog.txt -test.paniconexit0 -test.timeout=10m0s 1521�� 64/src/net -trimpath x_amd64/compile -p google.golang.ordocker-cli-plugin-metadata -lang=go1.24 x_amd64/compile .cfg�� /opt/hostedtoolcache/go/1.25.9/x64/src/runtime/c-m 1521424/b166/ x_amd64/vet --gdwarf-5 --64 -o x_amd64/vet (dns block)
  • slow.example.com
    • Triggering command: /tmp/go-build395042731/b513/launcher.test /tmp/go-build395042731/b513/launcher.test -test.testlogfile=/tmp/go-build395042731/b513/testlog.txt -test.paniconexit0 -test.timeout=10m0s 1521�� 64/src/net -trimpath x_amd64/compile -p google.golang.ordocker-cli-plugin-metadata -lang=go1.24 x_amd64/compile .cfg�� /opt/hostedtoolcache/go/1.25.9/x64/src/runtime/c-m 1521424/b166/ x_amd64/vet --gdwarf-5 --64 -o x_amd64/vet (dns block)
  • this-host-does-not-exist-12345.com
    • Triggering command: /tmp/go-build395042731/b522/mcp.test /tmp/go-build395042731/b522/mcp.test -test.testlogfile=/tmp/go-build395042731/b522/testlog.txt -test.paniconexit0 -test.timeout=10m0s buil�� .cfg awmg x_amd64/vet -c=4 -nolocalimports -importcfg x_amd64/vet .cfg�� 1521424/b066/vet.cfg -I x_amd64/vet --gdwarf-5 --64 -o x_amd64/vet (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot finished work on behalf of lpcox May 4, 2026 17:43
@lpcox lpcox merged commit 0d6ef4b into main May 4, 2026
18 checks passed
@lpcox lpcox deleted the copilot/fix-duplicate-code-streamable-handler branch May 4, 2026 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[duplicate-code] Duplicate Code Pattern: StreamableHTTP Handler + Middleware Chain

3 participants