refactor: eliminate 4 cross-package duplicate-code patterns (server/proxy)#7209
Conversation
…kages Pattern 1: Extract newSDKServer helper in internal/server/http_server.go, replacing the identical sdk.NewServer construction in routed.go and unified.go. Pattern 2: Standardize DIFC parse-error warning in proxy/proxy.go to use logger.LogWarn (matching the style already used in unified.go). Pattern 3: Add WriteSimpleHealthResponse and WriteReflectResponse helpers to internal/httputil/endpoint_helpers.go; update server/reflect.go and the inline health/reflect checks in proxy/handler.go to use them, ensuring both packages share the same response construction path. Pattern 4: Extract writeToResultOrEmpty method on proxyHandler in proxy/handler.go, replacing the two identical ToResult-failure blocks.
- Rename writeToResultOrEmpty -> toResultOrWriteEmpty (the method returns the result on success; it only writes on error, so the name now reads in execution order: try ToResult, or write-empty on failure) - Rename components -> difcComponents in httputil.WriteReflectResponse to match call-site naming conventions
|
@copilot resolve the merge conflicts in this pull request |
…rutil and version)
Merge conflicts resolved in commit |
There was a problem hiding this comment.
Pull request overview
This PR refactors repeated logic across internal/server and internal/proxy to reduce drift between “unified” vs “routed” server modes and proxy endpoints, by centralizing common server construction, endpoint responses, and DIFC result handling.
Changes:
- Consolidates MCP SDK server creation into a shared
newSDKServerhelper used by both unified and routed server modes. - Standardizes proxy DIFC parse-error warnings via
logger.LogWarn(...)to match existing startup logging patterns. - Extracts shared
/healthand/reflectresponse helpers intointernal/httputiland reuses them from both server and proxy. - Removes duplicated Phase-5
ToResulterror handling by introducingtoResultOrWriteEmptyin the proxy handler.
Show a summary per file
| File | Description |
|---|---|
| internal/server/unified.go | Switches MCP SDK server construction to the shared newSDKServer helper. |
| internal/server/routed.go | Switches routed per-backend MCP SDK server construction to newSDKServer. |
| internal/server/reflect.go | Reuses shared httputil.WriteReflectResponse for consistent reflect output. |
| internal/server/http_server.go | Adds newSDKServer helper and centralizes version/logger wiring for MCP SDK server creation. |
| internal/proxy/proxy.go | Uses logger.LogWarn for DIFC parse warnings to align with unified server logging style. |
| internal/proxy/handler.go | Extracts duplicated ToResult failure handling; uses shared health/reflect response helpers. |
| internal/httputil/endpoint_helpers.go | Adds shared health and reflect response helpers used by both server and proxy. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 7/7 changed files
- Comments generated: 0
Four structural duplication patterns across
internal/serverandinternal/proxywere causing diverging implementations that silently drift as the codebase evolves.Pattern 1 —
newSDKServerhelper (internal/server/http_server.go)Extracted the repeated
sdk.NewServer(&sdk.Implementation{...}, &sdk.ServerOptions{...})block into a shared helper used by bothrouted.goandunified.go. Removed now-unusedversionimports from both files.Pattern 2 — Standardized DIFC parse-error warning (
internal/proxy/proxy.go)Replaced
logProxy.Printf("WARNING: ...")withlogger.LogWarn("startup", ...)to match the logging style already used inserver/unified.go.Pattern 3 — Shared health/reflect response helpers (
internal/httputil/endpoint_helpers.go)Added
WriteSimpleHealthResponse(w)andWriteReflectResponse(w, difcComponents)tohttputil— the package bothserverandproxyalready import. The proxy's inline/healthand/reflectbranches now call these helpers;server/reflect.godoes too. This ensures both packages route through identical code asdifc.BuildReflectResponseevolves.Pattern 4 —
toResultOrWriteEmptymethod (internal/proxy/handler.go)Extracted the two identical Phase-5
ToResultfailure blocks into a single method onproxyHandler:Returns
(result, true)on success; logs, writes an empty-shaped response, and returns(nil, false)on error so the caller canreturncleanly.