Skip to content

Comments

Refactor: split mcp/connection.go by concern, relocate misplaced server helpers#1373

Merged
lpcox merged 2 commits intomainfrom
copilot/refactor-semantic-function-clustering-again
Feb 25, 2026
Merged

Refactor: split mcp/connection.go by concern, relocate misplaced server helpers#1373
lpcox merged 2 commits intomainfrom
copilot/refactor-semantic-function-clustering-again

Conversation

Copy link
Contributor

Copilot AI commented Feb 25, 2026

connection.go (973 lines) mixed core MCP session management with HTTP transport negotiation. Two helpers in server/transport.go belonged in other files.

Changes

internal/mcp/ — split by concern

  • Extracted HTTP transport layer to new http_transport.go (~496 lines): transport type negotiation (tryStreamableHTTPTransport, trySSETransport, tryPlainJSONTransport, trySDKTransport), request lifecycle helpers (setupHTTPRequest, executeHTTPRequest, initializeHTTPSession, sendHTTPRequest), parsing utilities (parseSSEResponse, parseJSONRPCResponseWithSSE), and supporting types/vars (HTTPTransportType, httpRequestResult, transportConnector, requestIDCounter)
  • connection.go reduced from 973 → 492 lines; now contains only Connection struct, NewConnection/NewHTTPConnection, SendRequest*, SDK method dispatch, and Close

internal/server/ — relocate misplaced helpers

  • Moved applyAuthIfConfigured from transport.goauth.go (alongside authMiddleware)
  • Moved withResponseLogging from transport.gohttp_helpers.go (alongside wrapWithMiddleware, logHTTPRequestBody)
  • transport.go now contains only HTTPTransport SDK adapter and CreateHTTPServerForMCP

All moves are within-package reorganizations; no logic changes.

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-build80772430/b284/launcher.test /tmp/go-build80772430/b284/launcher.test -test.testlogfile=/tmp/go-build80772430/b284/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true g_.a MQ_U/XgBWM_timxeqS5QdMQ_U x_amd64/vet OUTPUT -d 168.63.129.16 x_amd64/vet 6728�� lang.org/x/sys/cpu/cpu_gc_x86.s ache/go/1.25.6/x64/src/internal/testlog/exit.go x_amd64/vet INVALID,NEW -j DROP x_amd64/vet (dns block)
  • invalid-host-that-does-not-exist-12345.com
    • Triggering command: /tmp/go-build80772430/b266/config.test /tmp/go-build80772430/b266/config.test -test.testlogfile=/tmp/go-build80772430/b266/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true aCJj/o1rujB_p3NGFKnssaCJj 83FPnLSVe x_amd64/vet (dns block)
  • nonexistent.local
    • Triggering command: /tmp/go-build80772430/b284/launcher.test /tmp/go-build80772430/b284/launcher.test -test.testlogfile=/tmp/go-build80772430/b284/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true g_.a MQ_U/XgBWM_timxeqS5QdMQ_U x_amd64/vet OUTPUT -d 168.63.129.16 x_amd64/vet 6728�� lang.org/x/sys/cpu/cpu_gc_x86.s ache/go/1.25.6/x64/src/internal/testlog/exit.go x_amd64/vet INVALID,NEW -j DROP x_amd64/vet (dns block)
  • slow.example.com
    • Triggering command: /tmp/go-build80772430/b284/launcher.test /tmp/go-build80772430/b284/launcher.test -test.testlogfile=/tmp/go-build80772430/b284/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true g_.a MQ_U/XgBWM_timxeqS5QdMQ_U x_amd64/vet OUTPUT -d 168.63.129.16 x_amd64/vet 6728�� lang.org/x/sys/cpu/cpu_gc_x86.s ache/go/1.25.6/x64/src/internal/testlog/exit.go x_amd64/vet INVALID,NEW -j DROP x_amd64/vet (dns block)
  • this-host-does-not-exist-12345.com
    • Triggering command: /tmp/go-build2273372946/b225/mcp.test /tmp/go-build2273372946/b225/mcp.test -test.testlogfile=/tmp/go-build2273372946/b225/testlog.txt -test.paniconexit0 -test.timeout=10m0s conf�� go user.name bin/git (dns block)
    • Triggering command: /tmp/go-build80772430/b293/mcp.test /tmp/go-build80772430/b293/mcp.test -test.testlogfile=/tmp/go-build80772430/b293/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 64/src/net ternal/fips140/hkdf/cast.go x_amd64/vet (dns block)

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

Original prompt

This section details on the original issue you should resolve

<issue_title>[refactor] Semantic Function Clustering Analysis: Refactoring Opportunities</issue_title>
<issue_description>## Overview

Automated semantic function clustering analysis of 67 non-test Go source files (428 functions) across 16 packages in internal/. The analysis identified two primary refactoring opportunities with medium-to-high impact: functions placed in files where they don't logically belong, and a large file with mixed concerns that warrants a split. Additionally, two cases of semantically similar (near-duplicate) functions were found.

Overall code organization is healthy — packages are well-structured, naming is consistent, and the logger package's documented intentional patterns are recognized as such.

Full Report

Function Inventory

Package Files Functions Notes
auth 1 5 Well-organized
cmd 7 22 Well-organized
config 9 60 Well-organized
difc 5 67 Well-organized
dockerutil 1 1 Very thin — may grow
envutil 1 3 Appropriate size
guard 4 18 Well-organized
launcher 3 25 Well-organized
logger 14 91 Large but intentionally structured
mcp 3 34 connection.go is oversized — see below
middleware 1 6 Appropriate size
server 10 60 Two outliers — see below
strutil 1 2 Appropriate size
sys 1 6 Appropriate size
tty 2 3 Appropriate size
version 1 2 Appropriate size

Identified Issues

1. 🔴 internal/mcp/connection.go Has Two Distinct Concerns (973 lines, 33 functions)

Issue: connection.go conflates MCP session/operation management with HTTP transport negotiation. The file has grown to 973 lines because it handles two separate responsibilities:

Concern A — MCP session operations (correct location):

  • NewConnection, NewHTTPConnection
  • SendRequest, SendRequestWithServerID
  • listTools, callTool, listResources, readResource, listPrompts, getPrompt
  • requireSession, Close, IsHTTP, GetHTTPURL, GetHTTPHeaders

Concern B — HTTP transport negotiation (candidates for extraction):

  • trySDKTransport, tryStreamableHTTPTransport, trySSETransport, tryPlainJSONTransport
  • setupHTTPRequest, executeHTTPRequest, initializeHTTPSession, sendHTTPRequest
  • parseSSEResponse, parseJSONRPCResponseWithSSE
  • newMCPClient, newHTTPConnection, isHTTPConnectionError

Recommendation: Extract the HTTP transport negotiation functions into internal/mcp/http_transport.go. This follows the same pattern already established in internal/server/ where transport concerns live in transport.go.

internal/mcp/
  connection.go       ← core MCP operations (reduce from 973 → ~450 lines)
  http_transport.go   ← HTTP transport probing/negotiation (new, ~520 lines)
  schema.go           ← unchanged
  types.go            ← unchanged

Estimated effort: 1–2 hours (pure file split, no logic changes)
Estimated impact: Significantly improved navigability; connection.go becomes focused on what developers expect to find there


2. 🟡 internal/server/transport.go Contains Auth and Logging Helpers That Belong Elsewhere

Issue: transport.go is primarily responsible for the HTTPTransport SDK adapter struct and CreateHTTPServerForMCP. However, it also defines two middleware helpers that are out of place:

applyAuthIfConfigured (line 61 in transport.go)

func applyAuthIfConfigured(apiKey string, handler http.HandlerFunc) http.HandlerFunc {
    if apiKey != "" {
        return authMiddleware(apiKey, handler)
    }
    return handler
}

This is an auth helper that wraps authMiddleware from auth.go. It is called from handlers.go and http_helpers.go — not from anything transport-specific. It belongs in auth.go next to authMiddleware.

withResponseLogging (line 48 in transport.go)

func withResponseLogging(handler http.Handler) http.Handler {
    // wraps handler to log response bodies
}

This is an HTTP response logging middleware used in handlers.go (for /health, /close, OAuth discovery endpoints). It belongs in http_helpers.go alongside wrapWithMiddleware and logHTTPRequestBody, or in handlers.go since it's only used there.

Recommendation:

  • Move applyAuthIfConfiguredinternal/server/auth.go
  • Move withResponseLogginginternal/server/http_helpers.go

Estimated effort: 30 minutes (two function moves, same package — no import changes needed)
Estimated impact: transport.go becomes purely about the HTTPTransport SDK adapter; auth functions are consolidated in auth.go


3. 🟢 Near-Duplicate: auth.TruncateSessionID vs sanitize.TruncateSecret

Issue: Two similar truncation functions exist for se...

Custom agent used: agentic-workflows
GitHub Agentic Workflows (gh-aw) - Create, debug, and upgrade AI-powered workflows with intelligent prompt routing


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

… misplaced server helpers

Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Copilot AI changed the title [WIP] Refactor semantic function clustering analysis based on findings Refactor: split mcp/connection.go by concern, relocate misplaced server helpers Feb 25, 2026
@lpcox lpcox marked this pull request as ready for review February 25, 2026 01:22
Copilot AI review requested due to automatic review settings February 25, 2026 01:22
@lpcox lpcox merged commit 387f45d into main Feb 25, 2026
12 checks passed
@lpcox lpcox deleted the copilot/refactor-semantic-function-clustering-again branch February 25, 2026 01:22
Copy link
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 performs a clean refactoring to improve code organization by splitting files based on functional concerns and relocating misplaced helper functions. The refactoring reduces internal/mcp/connection.go from 973 to 492 lines by extracting HTTP transport logic into a new dedicated file, and consolidates authentication and logging helpers in internal/server/ into their semantically appropriate locations.

Changes:

  • Split internal/mcp/connection.go by extracting HTTP transport negotiation (~496 lines) into new internal/mcp/http_transport.go
  • Moved applyAuthIfConfigured from server/transport.go to server/auth.go (alongside authMiddleware)
  • Moved withResponseLogging from server/transport.go to server/http_helpers.go (alongside other HTTP helpers)

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
internal/mcp/http_transport.go New file containing HTTP transport layer: transport type negotiation, request lifecycle helpers, SSE parsing, and supporting types (HTTPTransportType, httpRequestResult, etc.)
internal/mcp/connection.go Reduced from 973→492 lines; now focused on core MCP session management (Connection struct, NewConnection, SendRequest methods, SDK dispatch)
internal/server/transport.go Removed misplaced helpers; now contains only HTTPTransport SDK adapter and CreateHTTPServerForMCP
internal/server/http_helpers.go Added withResponseLogging function (moved from transport.go) alongside other HTTP helper functions
internal/server/auth.go Added applyAuthIfConfigured function (moved from transport.go) alongside authMiddleware

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

@github-actions
Copy link
Contributor

GitHub MCP: ✅
Refactor: split mcp/connection.go by concern, relocate misplaced server helpers
[test] Add tests for difc.labels (CheckFlow, ViolationError, Clone, Union)
Serena activate_project: ✅
Playwright github title: ✅
File write: ✅
Bash cat: ✅
Overall: PASS

🔮 The oracle has spoken through Smoke Codex

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[refactor] Semantic Function Clustering Analysis: Refactoring Opportunities

2 participants