Commit cce3e24
authored
Refactor HTTP request initialization to eliminate duplicate code pattern (#937)
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):
```go
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):
```go
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]
>
> <details>
> <summary>Firewall rules blocked me from connecting to one or more
addresses (expand for details)</summary>
>
> #### 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](https://gh.io/copilot/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](https://github.com/github/gh-aw-mcpg/settings/copilot/coding_agent)
(admins only)
>
> </details>
<!-- START COPILOT ORIGINAL PROMPT -->
<details>
<summary>Original prompt</summary>
----
*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**:
``````go
// 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`:
``````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:
``````go
// 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...
</details>
<!-- START COPILOT CODING AGENT SUFFIX -->
- Fixes #8932 files changed
Lines changed: 106 additions & 100 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
170 | 170 | | |
171 | 171 | | |
172 | 172 | | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
| 184 | + | |
| 185 | + | |
| 186 | + | |
| 187 | + | |
| 188 | + | |
| 189 | + | |
| 190 | + | |
| 191 | + | |
| 192 | + | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
| 197 | + | |
| 198 | + | |
| 199 | + | |
| 200 | + | |
| 201 | + | |
| 202 | + | |
| 203 | + | |
| 204 | + | |
| 205 | + | |
| 206 | + | |
| 207 | + | |
| 208 | + | |
| 209 | + | |
| 210 | + | |
| 211 | + | |
| 212 | + | |
| 213 | + | |
| 214 | + | |
| 215 | + | |
| 216 | + | |
| 217 | + | |
| 218 | + | |
| 219 | + | |
| 220 | + | |
| 221 | + | |
| 222 | + | |
| 223 | + | |
| 224 | + | |
| 225 | + | |
| 226 | + | |
| 227 | + | |
| 228 | + | |
173 | 229 | | |
174 | 230 | | |
175 | 231 | | |
| |||
573 | 629 | | |
574 | 630 | | |
575 | 631 | | |
576 | | - | |
577 | | - | |
578 | | - | |
579 | | - | |
580 | | - | |
581 | | - | |
582 | | - | |
583 | | - | |
584 | | - | |
585 | | - | |
586 | | - | |
587 | | - | |
588 | | - | |
589 | | - | |
| 632 | + | |
590 | 633 | | |
591 | 634 | | |
592 | 635 | | |
593 | 636 | | |
594 | | - | |
595 | | - | |
596 | 637 | | |
597 | | - | |
598 | | - | |
599 | | - | |
600 | | - | |
| 638 | + | |
| 639 | + | |
| 640 | + | |
| 641 | + | |
| 642 | + | |
601 | 643 | | |
602 | | - | |
603 | | - | |
604 | | - | |
605 | | - | |
606 | | - | |
| 644 | + | |
607 | 645 | | |
608 | | - | |
609 | 646 | | |
610 | 647 | | |
611 | | - | |
| 648 | + | |
612 | 649 | | |
613 | 650 | | |
614 | 651 | | |
| |||
618 | 655 | | |
619 | 656 | | |
620 | 657 | | |
621 | | - | |
622 | | - | |
623 | | - | |
624 | | - | |
625 | | - | |
626 | | - | |
627 | | - | |
| 658 | + | |
628 | 659 | | |
629 | 660 | | |
630 | | - | |
631 | | - | |
| 661 | + | |
| 662 | + | |
632 | 663 | | |
633 | 664 | | |
634 | 665 | | |
635 | 666 | | |
636 | 667 | | |
637 | 668 | | |
638 | 669 | | |
639 | | - | |
| 670 | + | |
640 | 671 | | |
641 | 672 | | |
642 | | - | |
| 673 | + | |
643 | 674 | | |
644 | 675 | | |
645 | | - | |
| 676 | + | |
646 | 677 | | |
647 | 678 | | |
648 | 679 | | |
| |||
674 | 705 | | |
675 | 706 | | |
676 | 707 | | |
677 | | - | |
678 | | - | |
679 | | - | |
680 | | - | |
681 | | - | |
682 | | - | |
683 | | - | |
684 | | - | |
685 | | - | |
686 | | - | |
687 | | - | |
688 | | - | |
689 | | - | |
690 | | - | |
691 | | - | |
692 | | - | |
693 | | - | |
694 | | - | |
695 | | - | |
696 | | - | |
697 | | - | |
698 | | - | |
699 | | - | |
700 | | - | |
701 | | - | |
702 | | - | |
703 | | - | |
704 | | - | |
705 | | - | |
706 | | - | |
707 | | - | |
708 | | - | |
709 | 708 | | |
710 | 709 | | |
711 | | - | |
712 | | - | |
713 | | - | |
714 | | - | |
715 | | - | |
716 | | - | |
| 710 | + | |
| 711 | + | |
| 712 | + | |
| 713 | + | |
| 714 | + | |
| 715 | + | |
| 716 | + | |
| 717 | + | |
| 718 | + | |
| 719 | + | |
| 720 | + | |
| 721 | + | |
717 | 722 | | |
718 | | - | |
719 | | - | |
720 | | - | |
721 | 723 | | |
722 | | - | |
723 | | - | |
| 724 | + | |
| 725 | + | |
| 726 | + | |
| 727 | + | |
| 728 | + | |
| 729 | + | |
724 | 730 | | |
725 | | - | |
| 731 | + | |
726 | 732 | | |
727 | 733 | | |
728 | | - | |
| 734 | + | |
729 | 735 | | |
730 | 736 | | |
731 | 737 | | |
732 | 738 | | |
733 | 739 | | |
734 | 740 | | |
735 | | - | |
| 741 | + | |
736 | 742 | | |
737 | 743 | | |
738 | | - | |
| 744 | + | |
739 | 745 | | |
740 | 746 | | |
741 | 747 | | |
742 | | - | |
743 | | - | |
| 748 | + | |
| 749 | + | |
744 | 750 | | |
745 | 751 | | |
746 | 752 | | |
747 | 753 | | |
748 | | - | |
749 | | - | |
| 754 | + | |
| 755 | + | |
750 | 756 | | |
751 | 757 | | |
752 | 758 | | |
753 | 759 | | |
754 | | - | |
| 760 | + | |
755 | 761 | | |
756 | 762 | | |
757 | 763 | | |
| |||
762 | 768 | | |
763 | 769 | | |
764 | 770 | | |
765 | | - | |
766 | | - | |
| 771 | + | |
| 772 | + | |
767 | 773 | | |
768 | 774 | | |
769 | 775 | | |
770 | 776 | | |
771 | | - | |
772 | | - | |
| 777 | + | |
| 778 | + | |
773 | 779 | | |
774 | 780 | | |
775 | 781 | | |
| |||
781 | 787 | | |
782 | 788 | | |
783 | 789 | | |
784 | | - | |
785 | | - | |
| 790 | + | |
| 791 | + | |
786 | 792 | | |
787 | 793 | | |
788 | 794 | | |
789 | 795 | | |
790 | | - | |
791 | | - | |
| 796 | + | |
| 797 | + | |
792 | 798 | | |
793 | 799 | | |
794 | 800 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
13 | 13 | | |
14 | 14 | | |
15 | 15 | | |
16 | | - | |
| 16 | + | |
17 | 17 | | |
18 | 18 | | |
19 | 19 | | |
| |||
0 commit comments