Refactor: Extract wrapWithMiddleware helper to eliminate duplicate middleware chaining code

[Claude]

Summary

Consolidates duplicate middleware chaining pattern (WithSDKLogging → rejectIfShutdown → applyAuthIfConfigured) that was replicated verbatim between CreateHTTPServerForMCP (transport.go) and CreateHTTPServerForRoutedMode (routed.go).

Changes

• Added wrapWithMiddleware helper in http_helpers.go that encapsulates the standard middleware stack with documented spec references (5.1.3 for shutdown, 7.1 for auth)
• Updated both server factories to use the helper, reducing 9 lines of duplicate code per location to a single function call
• Added comprehensive tests covering auth scenarios, shutdown behavior, middleware ordering verification, and log tag variations

Example

Before:

// Duplicated in both transport.go and routed.go
loggedHandler := WithSDKLogging(handler, "unified")
shutdownHandler := rejectIfShutdown(unifiedServer, loggedHandler, "server:transport")
finalHandler := applyAuthIfConfigured(apiKey, shutdownHandler.ServeHTTP)

After:

// Single call, consistent across both modes
finalHandler := wrapWithMiddleware(handler, "unified", unifiedServer, apiKey)

Impact

Adding new middleware layers (rate limiting, tracing, etc.) now requires changes in one location instead of two, with spec references and ordering documented centrally.

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-build1187723181/b275/launcher.test /tmp/go-build1187723181/b275/launcher.test -test.testlogfile=/tmp/go-build1187723181/b275/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true go HEAD ndor/bin/as --global r = get && echo "--abbrev-ref ortcfg rev-�� 64/src/runtime/c-c=4 64/src/os/dir_un-nolocalimports x_amd64/vet get --global ache/Python/3.12/home/REDACTED/go/pkg/mod/github.com/google/jsonschema-go@v0.4.2/jsonschema/annotations.go x_amd64/vet (dns block)
• invalid-host-that-does-not-exist-12345.com
  • Triggering command: /tmp/go-build1187723181/b260/config.test /tmp/go-build1187723181/b260/config.test -test.testlogfile=/tmp/go-build1187723181/b260/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true ortcfg s.go 64/pkg/tool/linu-o --global go x86-64.so.2 64/pkg/tool/linu-buildtags ortc�� 1554416/b013/_pk-errorsas 64/src/crypto/in-ifaceassert 64/pkg/tool/linu-nilfunc unset --global 86_64/uname 64/pkg/tool/linu-tests (dns block)
• nonexistent.local
  • Triggering command: /tmp/go-build1187723181/b275/launcher.test /tmp/go-build1187723181/b275/launcher.test -test.testlogfile=/tmp/go-build1187723181/b275/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true go HEAD ndor/bin/as --global r = get && echo "--abbrev-ref ortcfg rev-�� 64/src/runtime/c-c=4 64/src/os/dir_un-nolocalimports x_amd64/vet get --global ache/Python/3.12/home/REDACTED/go/pkg/mod/github.com/google/jsonschema-go@v0.4.2/jsonschema/annotations.go x_amd64/vet (dns block)
• slow.example.com
  • Triggering command: /tmp/go-build1187723181/b275/launcher.test /tmp/go-build1187723181/b275/launcher.test -test.testlogfile=/tmp/go-build1187723181/b275/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true go HEAD ndor/bin/as --global r = get && echo "--abbrev-ref ortcfg rev-�� 64/src/runtime/c-c=4 64/src/os/dir_un-nolocalimports x_amd64/vet get --global ache/Python/3.12/home/REDACTED/go/pkg/mod/github.com/google/jsonschema-go@v0.4.2/jsonschema/annotations.go x_amd64/vet (dns block)
• this-host-does-not-exist-12345.com
  • Triggering command: /tmp/go-build1187723181/b284/mcp.test /tmp/go-build1187723181/b284/mcp.test -test.testlogfile=/tmp/go-build1187723181/b284/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true go 64/src/internal/-ifaceassert 64/pkg/tool/linu-nilfunc --global user.email ache/Python/3.12--abbrev-ref 64/pkg/tool/linuHEAD abis�� toml@v1.6.0/decode.go toml@v1.6.0/deprecated.go u/13/cc1 -tree /cgroup k/_temp/ghcca-node/node/bin/node/opt/hostedtoolcache/go/1.25.6/x64/src/net u/13/cc1 (dns block)

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

• Configure 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 (admins only)

Original prompt

---

This section details on the original issue you should resolve

<issue_title>[duplicate-code] Duplicate Code Pattern: Middleware Chaining and HTTP Server Construction</issue_title>
<issue_description>Part of duplicate code analysis: #1283

Summary

The three-step middleware wrapping chain (WithSDKLogging → rejectIfShutdown → applyAuthIfConfigured) and the final http.Server construction are duplicated verbatim between CreateHTTPServerForMCP (transport.go) and CreateHTTPServerForRoutedMode (routed.go), along with identical comments.

Duplication Details

Pattern: Middleware chain + http.Server construction

• Severity: Low
• Occurrences: 2 (one per HTTP server factory function)
• Locations:
  • internal/server/transport.go (lines 118–135)
  • internal/server/routed.go (lines 140–162, inside per-backend loop)

transport.go:

// Wrap SDK handler with detailed logging for JSON-RPC translation debugging
loggedHandler := WithSDKLogging(streamableHandler, "unified")

// Apply shutdown check middleware (spec 5.1.3)
// This must come before auth to ensure shutdown takes precedence
shutdownHandler := rejectIfShutdown(unifiedServer, loggedHandler, "server:transport")

// Apply auth middleware if API key is configured (spec 7.1)
finalHandler := applyAuthIfConfigured(apiKey, shutdownHandler.ServeHTTP)

mux.Handle("/mcp/", finalHandler)
mux.Handle("/mcp", finalHandler)

return &http.Server{Addr: addr, Handler: mux}

routed.go (inside per-backend loop, then after):

loggedHandler := WithSDKLogging(routeHandler, "routed:"+backendID)
shutdownHandler := rejectIfShutdown(unifiedServer, loggedHandler, "server:routed")
finalHandler := applyAuthIfConfigured(apiKey, shutdownHandler.ServeHTTP)

mux.Handle(route+"/", finalHandler)
mux.Handle(route, finalHandler)

// ... (outside loop)
return &http.Server{Addr: addr, Handler: mux}

Impact Analysis

• Maintainability: Adding a new middleware layer (e.g., rate limiting, tracing) requires changes in both files. The spec references in comments (5.1.3, 7.1) would also need to be kept in sync.
• Bug Risk: Low, since the middleware order is mechanically enforced, but adding middleware only in one place is a realistic mistake.
• Code Bloat: Minor (~6 lines duplicated).

Refactoring Recommendations

1. Extract a wrapWithMiddleware helper in transport.go or http_helpers.go:

   // wrapWithMiddleware applies the standard middleware stack to an SDK handler.
   // Order: SDK logging → shutdown check (spec 5.1.3) → auth (spec 7.1)
   func wrapWithMiddleware(handler http.Handler, logTag string, unifiedServer *UnifiedServer, apiKey string) http.HandlerFunc {
       loggedHandler := WithSDKLogging(handler, logTag)
       shutdownHandler := rejectIfShutdown(unifiedServer, loggedHandler, "server:"+logTag)
       return applyAuthIfConfigured(apiKey, shutdownHandler.ServeHTTP)
   }

2. Optionally extract buildHTTPServer:

   func buildHTTPServer(addr string, mux *http.ServeMux) *http.Server {
       return &http.Server{Addr: addr, Handler: mux}
   }

3. Estimated effort: ~20 minutes

4. Benefits: Middleware order documented and enforced in one place; easier to add new middleware globally

Implementation Checklist

• Review duplication findings
• Extract wrapWithMiddleware helper
• Update transport.go and routed.go to use the helper
• Verify middleware order is preserved
• Verify no functionality broken

Parent Issue

See parent analysis report: #1283
Related to #1283

Generated by Duplicate Code Detector

• expires on Mar 1, 2026, 3:02 AM UTC

Comments on the Issue (you are @claude[agent] in this section)

• Fixes [duplicate-code] Duplicate Code Pattern: Middleware Chaining and HTTP Server Construction #1284

[Copilot]

Pull request overview

Refactors the HTTP server factories to eliminate duplicated middleware chaining by centralizing the standard middleware stack into a single helper, with accompanying unit tests to validate behavior.

Changes:

• Added wrapWithMiddleware() helper to encapsulate SDK logging, shutdown rejection, and optional auth.
• Updated unified and routed HTTP server creation to use the helper instead of inline chaining.
• Added tests for wrapWithMiddleware() across auth/shutdown scenarios and log tag variants.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
internal/server/http_helpers.go	Introduces wrapWithMiddleware() helper to consolidate standard middleware wrapping.
internal/server/transport.go	Uses wrapWithMiddleware() for unified-mode handler wiring.
internal/server/routed.go	Uses wrapWithMiddleware() for per-backend routed-mode handler wiring.
internal/server/http_helpers_test.go	Adds new unit tests targeting wrapWithMiddleware() behavior.

---

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

[Copilot]

This test claims to validate that “shutdown check happens before auth”, but it sets a valid Authorization header, so auth will always pass and the observed 503 only demonstrates shutdown rejection after successful auth. If the intended precedence is “shutdown takes precedence even when auth is missing/invalid”, add a case with apiKey configured + missing/invalid Authorization while shutdown=true and assert 503 (or rename/adjust comments if 401 is intended).

[Copilot]

wrapWithMiddleware’s docstring and inline comment say the shutdown check “must come before auth to ensure shutdown takes precedence”, but the composed handler currently wraps auth outside rejectIfShutdown (applyAuthIfConfigured(apiKey, shutdownHandler.ServeHTTP)). Given authMiddleware returns 401 before calling next when Authorization is missing/invalid, shutdown will not take precedence in those cases (you’ll return 401 instead of 503 during shutdown when auth is enabled). Either change the wrapping so rejectIfShutdown is the outermost middleware (or make authMiddleware aware of shutdown), or update the comments/spec claims if 401 is the intended precedence.

[Copilot]

rejectIfShutdown’s logNamespace used to be a stable call-site identifier (e.g. "server:transport" / "server:routed"), but wrapWithMiddleware now derives it from logTag ("server:"+logTag). In routed mode this creates per-backend logger namespaces like "server:routed:", which can explode cardinality and makes log filtering/alerting less predictable. Consider passing an explicit shutdown log namespace into wrapWithMiddleware (or keep the previous constants) instead of deriving it from logTag.