Refactor HTTP request initialization to eliminate duplicate code pattern

[Claude]

The internal/mcp/connection.go file contained duplicate HTTP request initialization workflow across initializeHTTPSession and sendHTTPRequest methods, differing only in header customization logic. This made the codebase harder to maintain and inconsistent for future HTTP-related changes.

Changes

• Extracted executeHTTPRequest helper function that consolidates the common pattern: create JSON-RPC request → marshal → setup HTTP → execute → read response

  • Returns httpRequestResult struct containing status code, response body, and headers
  • Accepts optional headerModifier callback for caller-specific header customization
  • Provides consistent connection error handling with method-specific error messages

• Refactored initializeHTTPSession to use the helper with session ID header management

• Refactored sendHTTPRequest to use the helper with context-based session ID priority handling

Example

Before (duplicated in both methods):

request := createJSONRPCRequest(requestID, method, params)
requestBody, err := json.Marshal(request)
// ... marshal error handling
httpReq, err := setupHTTPRequest(ctx, c.httpURL, requestBody, c.headers)
// ... setup error handling
httpResp, err := c.httpClient.Do(httpReq)
// ... connection error handling
defer httpResp.Body.Close()
responseBody, err := io.ReadAll(httpResp.Body)
// ... read error handling

After (single location):

result, err := c.executeHTTPRequest(ctx, method, params, requestID, func(httpReq *http.Request) {
    // Caller-specific header customization
    httpReq.Header.Set("Mcp-Session-Id", sessionID)
})

Impact

• Future HTTP changes (timeouts, retries, logging) require updates in one location
• Consistent error messages across all HTTP operations
• Net change: +105 lines, -99 lines (new struct and documentation)

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-build3466960917/b275/launcher.test /tmp/go-build3466960917/b275/launcher.test -test.testlogfile=/tmp/go-build3466960917/b275/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 64/src/net /bin/sh (dns block)
  • Triggering command: /tmp/go-build3643262344/b275/launcher.test /tmp/go-build3643262344/b275/launcher.test -test.testlogfile=/tmp/go-build3643262344/b275/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true /opt/hostedtoolcache/go/1.25.7/x64/src/net ache/go/1.25.7/x64/src/crypto/internal/fips140hash/hash.go /usr/bin/gcc --gdwarf-5 --64 -o /usr/bin/gcc -I /opt/hostedtoolcache/go/1.25.7/x64/src/net -fPIC 64/pkg/tool/linux_amd64/vet -pthread -Wl,--no-gc-sect-d -fmessage-length=0 64/pkg/tool/linux_amd64/vet (dns block)
  • Triggering command: /tmp/go-build3690481232/b279/launcher.test /tmp/go-build3690481232/b279/launcher.test -test.testlogfile=/tmp/go-build3690481232/b279/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true /home/REDACTED/work/gh-aw-mcpg/gh-aw-mcpg/internal/envutil/envutil.go /home/REDACTED/work/gh-aw-mcpg/gh-aw-mcpg/internal/envutil/envutil_test.go ache/uv/0.10.2/x86_64/git elect.go nix.go 64/pkg/tool/linu/tmp/go-build4145022949/b166/vet.cfg bash /usr�� --version T.md d ache/go/1.25.7/x/opt/hostedtoolcache/go/1.25.7/x64/pkg/tool/linux_amd64/vet -trimpath /x86_64-linux-gn/tmp/go-build4145022949/b183/vet.cfg /usr/bin/runc.original (dns block)
• invalid-host-that-does-not-exist-12345.com
  • Triggering command: /tmp/go-build3466960917/b260/config.test /tmp/go-build3466960917/b260/config.test -test.testlogfile=/tmp/go-build3466960917/b260/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 64/src/net node cal/bin/as --output-format stream-json cal/bin/which 01.o -p 64/src/net client.go 64/pkg/tool/linux_amd64/compile -I /tmp/go-build271-unsafeptr=false -I 64/pkg/tool/linu/tmp/go-build3466960917/b144/vet.cfg (dns block)
• nonexistent.local
  • Triggering command: /tmp/go-build3466960917/b275/launcher.test /tmp/go-build3466960917/b275/launcher.test -test.testlogfile=/tmp/go-build3466960917/b275/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 64/src/net /bin/sh (dns block)
  • Triggering command: /tmp/go-build3643262344/b275/launcher.test /tmp/go-build3643262344/b275/launcher.test -test.testlogfile=/tmp/go-build3643262344/b275/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true /opt/hostedtoolcache/go/1.25.7/x64/src/net ache/go/1.25.7/x64/src/crypto/internal/fips140hash/hash.go /usr/bin/gcc --gdwarf-5 --64 -o /usr/bin/gcc -I /opt/hostedtoolcache/go/1.25.7/x64/src/net -fPIC 64/pkg/tool/linux_amd64/vet -pthread -Wl,--no-gc-sect-d -fmessage-length=0 64/pkg/tool/linux_amd64/vet (dns block)
  • Triggering command: /tmp/go-build3690481232/b279/launcher.test /tmp/go-build3690481232/b279/launcher.test -test.testlogfile=/tmp/go-build3690481232/b279/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true /home/REDACTED/work/gh-aw-mcpg/gh-aw-mcpg/internal/envutil/envutil.go /home/REDACTED/work/gh-aw-mcpg/gh-aw-mcpg/internal/envutil/envutil_test.go ache/uv/0.10.2/x86_64/git elect.go nix.go 64/pkg/tool/linu/tmp/go-build4145022949/b166/vet.cfg bash /usr�� --version T.md d ache/go/1.25.7/x/opt/hostedtoolcache/go/1.25.7/x64/pkg/tool/linux_amd64/vet -trimpath /x86_64-linux-gn/tmp/go-build4145022949/b183/vet.cfg /usr/bin/runc.original (dns block)
• slow.example.com
  • Triggering command: /tmp/go-build3466960917/b275/launcher.test /tmp/go-build3466960917/b275/launcher.test -test.testlogfile=/tmp/go-build3466960917/b275/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 64/src/net /bin/sh (dns block)
  • Triggering command: /tmp/go-build3643262344/b275/launcher.test /tmp/go-build3643262344/b275/launcher.test -test.testlogfile=/tmp/go-build3643262344/b275/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true /opt/hostedtoolcache/go/1.25.7/x64/src/net ache/go/1.25.7/x64/src/crypto/internal/fips140hash/hash.go /usr/bin/gcc --gdwarf-5 --64 -o /usr/bin/gcc -I /opt/hostedtoolcache/go/1.25.7/x64/src/net -fPIC 64/pkg/tool/linux_amd64/vet -pthread -Wl,--no-gc-sect-d -fmessage-length=0 64/pkg/tool/linux_amd64/vet (dns block)
  • Triggering command: /tmp/go-build3690481232/b279/launcher.test /tmp/go-build3690481232/b279/launcher.test -test.testlogfile=/tmp/go-build3690481232/b279/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true /home/REDACTED/work/gh-aw-mcpg/gh-aw-mcpg/internal/envutil/envutil.go /home/REDACTED/work/gh-aw-mcpg/gh-aw-mcpg/internal/envutil/envutil_test.go ache/uv/0.10.2/x86_64/git elect.go nix.go 64/pkg/tool/linu/tmp/go-build4145022949/b166/vet.cfg bash /usr�� --version T.md d ache/go/1.25.7/x/opt/hostedtoolcache/go/1.25.7/x64/pkg/tool/linux_amd64/vet -trimpath /x86_64-linux-gn/tmp/go-build4145022949/b183/vet.cfg /usr/bin/runc.original (dns block)
• this-host-does-not-exist-12345.com
  • Triggering command: /tmp/go-build3466960917/b284/mcp.test /tmp/go-build3466960917/b284/mcp.test -test.testlogfile=/tmp/go-build3466960917/b284/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true g_.a -trimpath ache/go/1.25.7/x64/pkg/tool/linux_amd64/asm 2690816/b098/ strings ctor ache/go/1.25.7/x/tmp/go-build3466960917/b205/vet.cfg -I fips140/sha3 gkWDTCvzb 2690816/b130=> --gdwarf-5 2690816/b130/ -o ache/go/1.25.7/x64/pkg/include (dns block)
  • Triggering command: /tmp/go-build330381427/b001/mcp.test /tmp/go-build330381427/b001/mcp.test -test.testlogfile=/tmp/go-build330381427/b001/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true -test.run=TestNewHTTPConnection|TestSendHTTPRequest|Test.*HTTP .cfg 64/pkg/tool/linux_amd64/vet -D GOAMD64_v1 -gensymabis 64/pkg/tool/linux_amd64/vet s#\.�� 64/src/runtime/trace/annotation.-s .cfg ache/go/1.25.7/x64/pkg/tool/linu-buildmode=exe rsions$# d; base64 HEAD 64/pkg/tool/linu-o ache/go/1.25.7/x64/pkg/tool/linuHEAD (dns block)
  • Triggering command: /tmp/go-build2307564073/b001/mcp.test /tmp/go-build2307564073/b001/mcp.test -test.testlogfile=/tmp/go-build2307564073/b001/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true -test.run=TestNewHTTPConnection.* 64/pkg/tool/linu-plugin-opt=/usr/libexec/gcc/x86_64-linux-gnu/13/lto-wrapper ache/go/1.25.7/x64/pkg/tool/linux_amd64/vet go syscall/unix/at.-d x_amd64/compile ache/go/1.25.7/x64/pkg/tool/linux_amd64/vet (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: HTTP Request Initialization in MCP Connection</issue_title>
<issue_description># 🔍 Duplicate Code Pattern: HTTP Request Initialization

Part of duplicate code analysis: #892

Summary

The internal/mcp/connection.go file contains 2 nearly identical instances of HTTP request initialization workflow, differing only in the RPC method name. This pattern appears in both initializeHTTPSession and sendHTTPRequest methods.

Duplication Details

Pattern: HTTP Request Creation Workflow

• Severity: Medium

• Occurrences: 2 instances in connection.go

• Locations:

  • internal/mcp/connection.go (lines 576-625) - initializeHTTPSession method
  • internal/mcp/connection.go (lines 678-726) - sendHTTPRequest method

• Code Sample:

// Duplicated pattern (appears 2 times with minor variations):

// 1. Create JSON-RPC request
request := createJSONRPCRequest(requestID, method, params)

// 2. Marshal request body
requestBody, err := json.Marshal(request)
if err != nil {
    return ..., fmt.Errorf("failed to marshal request: %w", err)
}

// 3. Create HTTP request
httpReq, err := setupHTTPRequest(ctx, c.httpURL, requestBody, c.headers)
if err != nil {
    return ..., err
}

// 4. Execute HTTP request
httpResp, err := c.httpClient.Do(httpReq)
if err != nil {
    if isHTTPConnectionError(err) {
        return ..., fmt.Errorf("cannot connect to HTTP backend at %s: %w", c.httpURL, err)
    }
    return ..., fmt.Errorf("HTTP request failed: %w", err)
}
defer httpResp.Body.Close()

// 5. Read response body
responseBody, err := io.ReadAll(httpResp.Body)
if err != nil {
    return ..., fmt.Errorf("failed to read response: %w", err)
}

// 6. Parse response (method-specific)
...

Impact Analysis

• Maintainability: Any change to HTTP request handling (e.g., timeout logic, retry mechanism, logging) requires updating 2 locations
• Bug Risk: Medium - If HTTP error handling is updated in one method but not the other, behavior becomes inconsistent
• Code Bloat: ~35 lines of duplicated code (steps 1-5 above)
• Extensibility: Adding new HTTP-based MCP methods would require copying this entire pattern again

Refactoring Recommendations

1. Extract Helper Function: executeHTTPRequest

• Create a shared helper function in internal/mcp/connection.go:

// executeHTTPRequest executes an HTTP JSON-RPC request and returns the raw response body
func (c *Connection) executeHTTPRequest(ctx context.Context, method string, params interface{}, requestID int) ([]byte, error) {
    // Create JSON-RPC request
    request := createJSONRPCRequest(requestID, method, params)
    
    // Marshal request body
    requestBody, err := json.Marshal(request)
    if err != nil {
        return nil, fmt.Errorf("failed to marshal %s request: %w", method, err)
    }
    
    // Create HTTP request
    httpReq, err := setupHTTPRequest(ctx, c.httpURL, requestBody, c.headers)
    if err != nil {
        return nil, err
    }
    
    // Execute HTTP request
    httpResp, err := c.httpClient.Do(httpReq)
    if err != nil {
        if isHTTPConnectionError(err) {
            return nil, fmt.Errorf("cannot connect to HTTP backend at %s: %w", c.httpURL, err)
        }
        return nil, fmt.Errorf("%s HTTP request failed: %w", method, err)
    }
    defer httpResp.Body.Close()
    
    // Read response body
    responseBody, err := io.ReadAll(httpResp.Body)
    if err != nil {
        return nil, fmt.Errorf("failed to read %s response: %w", method, err)
    }
    
    return responseBody, nil
}

• Update both methods:

// In initializeHTTPSession:
responseBody, err := c.executeHTTPRequest(context.Background(), "initialize", initParams, requestID)
if err != nil {
    return "", err
}
// ... parse response ...

// In sendHTTPRequest:
responseBody, err := c.executeHTTPRequest(ctx, method, params, requestID)
if err != nil {
    return nil, err
}
// ... parse response ...

• Estimated effort: 2-3 hours
• Benefits:
  • Single location for HTTP request execution logic
  • Easier to add retry logic, request/response logging, or timeouts
  • Consistent error messages across all HTTP operations
  • Reduces file from 1,011 lines to ~976 lines

2. Future Enhancement: Add Request/Response Logging

• Once extracted, the helper function becomes an ideal place to add:
  • Debug logging for request/response payloads
  • Request timing metrics
  • Retry logic with exponential backoff
• These enhancements would automatically apply to all HTTP operations

Implementation Checklist

• Review duplication findings with team
• Create h...

• Fixes [duplicate-code] Duplicate Code Pattern: HTTP Request Initialization in MCP Connection #893

[Copilot]

Pull request overview

This pull request successfully refactors duplicate HTTP request initialization code in internal/mcp/connection.go by extracting a common helper function. The refactoring eliminates ~90 lines of duplicated logic while maintaining existing functionality and test coverage.

Changes:

• Introduced executeHTTPRequest helper function with callback-based header customization to consolidate JSON-RPC request creation, marshaling, HTTP setup, execution, and response reading
• Refactored initializeHTTPSession and sendHTTPRequest to use the new helper, reducing code duplication and providing a single location for future HTTP-related enhancements
• Minor whitespace cleanup in main.go

Reviewed changes

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

File	Description
internal/mcp/connection.go	Added httpRequestResult struct and executeHTTPRequest helper function; refactored initializeHTTPSession and sendHTTPRequest to use the helper with callback-based header modification
main.go	Removed trailing whitespace on line 16

---

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