Skip to content

Commit d4dc29c

Browse files
lpcoxlpcox
authored
Extract duplicate error handling patterns into reusable helpers (#811)
Analysis identified 18 instances of duplicated error handling logic across `internal/server/unified.go` and `internal/mcp/connection.go`. This consolidates them into tested helper functions. ## Changes ### SDK CallToolResult Error Returns (16 instances) - Extracted `newErrorCallToolResult(err error) (*sdk.CallToolResult, interface{}, error)` helper - Replaces repeated pattern: `return &sdk.CallToolResult{IsError: true}, nil, err` - Affected: tool handlers, session initialization, backend communication ```go // Before (repeated 16 times) if err != nil { return &sdk.CallToolResult{IsError: true}, nil, err } // After if err != nil { return newErrorCallToolResult(err) } ``` ### HTTP Connection Error Detection (2 instances) - Extracted `isHTTPConnectionError(err error) bool` helper - Consolidates network error classification (connection refused, no such host, network unreachable) - Affected: HTTP initialize and request paths ```go // Before (repeated 2 times) if strings.Contains(err.Error(), "connection refused") || strings.Contains(err.Error(), "no such host") || strings.Contains(err.Error(), "network is unreachable") { // handle connection error } // After if isHTTPConnectionError(err) { // handle connection error } ``` ### Test Coverage - Added 8 test cases covering both helpers - Tests validate error propagation and proper nil handling ## Notes - Third pattern (command error detection) had only 1 occurrence - not duplicated, no action taken - Added TODO for future improvement: replace string matching with `*net.OpError` type assertions > [!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-build3296312196/b271/launcher.test /tmp/go-build3296312196/b271/launcher.test -test.testlogfile=/tmp/go-build3296312196/b271/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true ache/go/1.25.6/x/tmp/tmp.7JmQLDmIdb/golangci-lint-2.8.0-linux-amd64.tar.gz nto helper functREDACTED x_amd64/vet -pthread -Wl,--no-gc-sect-o -fmessage-length/tmp/go-build1663340372/b266/_pkg_.a x_amd64/vet -I ache/go/1.25.6/x-p -fPIC x_amd64/vet -pthread -Wl,--no-gc-sect-o -fmessage-length/tmp/go-build3296312196/b259/_pkg_.a x_amd64/vet` (dns block) > - Triggering command: `/tmp/go-build3756687988/b271/launcher.test /tmp/go-build3756687988/b271/launcher.test -test.testlogfile=/tmp/go-build3756687988/b271/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true` (dns block) > - `invalid-host-that-does-not-exist-12345.com` > - Triggering command: `/tmp/go-build3296312196/b259/config.test /tmp/go-build3296312196/b259/config.test -test.testlogfile=/tmp/go-build3296312196/b259/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true -c=4 -nolocalimports -importcfg /tmp/go-build1663340372/b283/importcfg -pack /home/REDACTED/work/gh-aw-mcpg/gh-aw-mcpg/internal/testutil/mcptest/example_test.go /home/REDACTED/work/gh-aw-mcpg/gh-aw-mcpg/internal/testutil/mcptest/gateway_integration_test.go 64/s�� ache/go/1.25.6/x64/src/net nto helper functions - Pattern 2 (High Priority): Extract SDK CallToolResult error returns - test x_amd64/vet --gdwarf-5 --64 -o x_amd64/vet` (dns block) > - `nonexistent.local` > - Triggering command: `/tmp/go-build3296312196/b271/launcher.test /tmp/go-build3296312196/b271/launcher.test -test.testlogfile=/tmp/go-build3296312196/b271/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true ache/go/1.25.6/x/tmp/tmp.7JmQLDmIdb/golangci-lint-2.8.0-linux-amd64.tar.gz nto helper functREDACTED x_amd64/vet -pthread -Wl,--no-gc-sect-o -fmessage-length/tmp/go-build1663340372/b266/_pkg_.a x_amd64/vet -I ache/go/1.25.6/x-p -fPIC x_amd64/vet -pthread -Wl,--no-gc-sect-o -fmessage-length/tmp/go-build3296312196/b259/_pkg_.a x_amd64/vet` (dns block) > - Triggering command: `/tmp/go-build3756687988/b271/launcher.test /tmp/go-build3756687988/b271/launcher.test -test.testlogfile=/tmp/go-build3756687988/b271/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true` (dns block) > - `slow.example.com` > - Triggering command: `/tmp/go-build3296312196/b271/launcher.test /tmp/go-build3296312196/b271/launcher.test -test.testlogfile=/tmp/go-build3296312196/b271/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true ache/go/1.25.6/x/tmp/tmp.7JmQLDmIdb/golangci-lint-2.8.0-linux-amd64.tar.gz nto helper functREDACTED x_amd64/vet -pthread -Wl,--no-gc-sect-o -fmessage-length/tmp/go-build1663340372/b266/_pkg_.a x_amd64/vet -I ache/go/1.25.6/x-p -fPIC x_amd64/vet -pthread -Wl,--no-gc-sect-o -fmessage-length/tmp/go-build3296312196/b259/_pkg_.a x_amd64/vet` (dns block) > - Triggering command: `/tmp/go-build3756687988/b271/launcher.test /tmp/go-build3756687988/b271/launcher.test -test.testlogfile=/tmp/go-build3756687988/b271/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true` (dns block) > - `this-host-does-not-exist-12345.com` > - Triggering command: `/tmp/go-build668030957/b001/mcp.test /tmp/go-build668030957/b001/mcp.test -test.testlogfile=/tmp/go-build668030957/b001/testlog.txt -test.paniconexit0 -test.timeout=10m0s conf�� 64/src/runtime/c-c=4 credential.usern-nolocalimports .12/x64/bin/as` (dns block) > - Triggering command: `/tmp/go-build3296312196/b280/mcp.test /tmp/go-build3296312196/b280/mcp.test -test.testlogfile=/tmp/go-build3296312196/b280/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true -c=4 -nolocalimports -importcfg /tmp/go-build3296312196/b282/importcfg -pack /home/REDACTED/work/gh-aw-mcpg/gh-aw-mcpg/internal/middleware/jqschema.go /home/REDACTED/work/gh-aw-mcpg/gh-aw-mcpg/internal/middleware/jqschema_bench_test.go` (dns block) > - Triggering command: `/tmp/go-build3756687988/b280/mcp.test /tmp/go-build3756687988/b280/mcp.test -test.testlogfile=/tmp/go-build3756687988/b280/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true` (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 Analysis Report - 3 Patterns Detected</issue_title> > <issue_description># 🔍 Duplicate Code Analysis Report > > *Analysis of commit 06ff18a* > > ## Summary > > This analysis identified **3 significant duplication patterns** across the Go codebase, primarily in error handling and HTTP connection logic. While the project has already refactored some areas (e.g., logger initialization with generics), several high-impact patterns remain. > > ## Detected Patterns > > This analysis found 3 significant duplication patterns: > > 1. **HTTP Connection Error Checking** - Severity: Medium - See sub-issue #740 > 2. **SDK CallToolResult Error Returns** - Severity: High - See sub-issue #741 > 3. **Command Error Detection Logic** - Severity: Low - See sub-issue #742 > > ## Overall Impact > > - **Total Duplicated Lines**: ~45 lines of duplicated logic > - **Affected Files**: 2 files (`internal/mcp/connection.go`, `internal/server/unified.go`) > - **Maintainability Risk**: Medium - Error handling duplication increases bug risk > - **Refactoring Priority**: **High** for pattern #2, Medium for pattern #1, Low for pattern #3 > > ## Next Steps > > 1. Review individual pattern sub-issues for detailed analysis > 2. Prioritize refactoring based on severity and impact > 3. Create implementation plan for highest priority patterns > > ## Analysis Metadata > > - **Analyzed Files**: 60+ Go files in `internal/` directory > - **Detection Method**: Serena semantic code analysis + pattern matching > - **Commit**: 06ff18a > - **Analysis Date**: 2026-02-06</issue_description> > > ## Comments on the Issue (you are @copilot in this section) > > <comments> > </comments> > </details> > **Custom agent used: agentic-workflows** > GitHub Agentic Workflows (gh-aw) - Create, debug, and upgrade AI-powered workflows with intelligent prompt routing <!-- START COPILOT CODING AGENT SUFFIX --> - Fixes #739 <!-- START COPILOT CODING AGENT TIPS --> --- 💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more [Copilot coding agent tips](https://gh.io/copilot-coding-agent-tips) in the docs.
2 parents de26188 + bdf88a6 commit d4dc29c

4 files changed

Lines changed: 125 additions & 22 deletions

File tree

‎internal/mcp/connection.go‎

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,20 @@ import (
2323

2424
var logConn = logger.New("mcp:connection")
2525

26+
// isHTTPConnectionError checks if an error is a network connection error
27+
// This helper reduces code duplication for checking common connection error patterns.
28+
// Note: Uses string matching which is fragile but consistent with existing patterns in the codebase.
29+
// TODO: Consider using errors.Is() or type assertions (*net.OpError) for more robust error classification.
30+
func isHTTPConnectionError(err error) bool {
31+
if err == nil {
32+
return false
33+
}
34+
errMsg := err.Error()
35+
return strings.Contains(errMsg, "connection refused") ||
36+
strings.Contains(errMsg, "no such host") ||
37+
strings.Contains(errMsg, "network is unreachable")
38+
}
39+
2640
// parseSSEResponse extracts JSON data from SSE-formatted response
2741
// SSE format: "event: message\ndata: {json}\n\n"
2842
func parseSSEResponse(body []byte) ([]byte, error) {
@@ -584,9 +598,7 @@ func (c *Connection) initializeHTTPSession() (string, error) {
584598
httpResp, err := c.httpClient.Do(httpReq)
585599
if err != nil {
586600
// Check if it's a connection error (cannot connect at all)
587-
if strings.Contains(err.Error(), "connection refused") ||
588-
strings.Contains(err.Error(), "no such host") ||
589-
strings.Contains(err.Error(), "network is unreachable") {
601+
if isHTTPConnectionError(err) {
590602
return "", fmt.Errorf("cannot connect to HTTP backend at %s: %w", c.httpURL, err)
591603
}
592604
return "", fmt.Errorf("failed to send initialize request to %s: %w", c.httpURL, err)
@@ -698,9 +710,7 @@ func (c *Connection) sendHTTPRequest(ctx context.Context, method string, params
698710
httpResp, err := c.httpClient.Do(httpReq)
699711
if err != nil {
700712
// Check if it's a connection error (cannot connect at all)
701-
if strings.Contains(err.Error(), "connection refused") ||
702-
strings.Contains(err.Error(), "no such host") ||
703-
strings.Contains(err.Error(), "network is unreachable") {
713+
if isHTTPConnectionError(err) {
704714
return nil, fmt.Errorf("cannot connect to HTTP backend at %s: %w", c.httpURL, err)
705715
}
706716
return nil, fmt.Errorf("failed to send HTTP request to %s: %w", c.httpURL, err)

‎internal/mcp/connection_test.go‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -703,3 +703,50 @@ func TestNewHTTPConnection(t *testing.T) {
703703
assert.Equal(t, httpClient, conn.httpClient, "HTTP client should match")
704704
assert.Equal(t, HTTPTransportStreamable, conn.httpTransportType, "Transport type should match")
705705
}
706+
707+
// TestIsHTTPConnectionError tests the HTTP connection error detection helper
708+
func TestIsHTTPConnectionError(t *testing.T) {
709+
tests := []struct {
710+
name string
711+
err error
712+
expected bool
713+
}{
714+
{
715+
name: "nil error",
716+
err: nil,
717+
expected: false,
718+
},
719+
{
720+
name: "connection refused error",
721+
err: fmt.Errorf("dial tcp: connection refused"),
722+
expected: true,
723+
},
724+
{
725+
name: "no such host error",
726+
err: fmt.Errorf("dial tcp: lookup example.invalid: no such host"),
727+
expected: true,
728+
},
729+
{
730+
name: "network is unreachable error",
731+
err: fmt.Errorf("dial tcp: network is unreachable"),
732+
expected: true,
733+
},
734+
{
735+
name: "other error",
736+
err: fmt.Errorf("some other error"),
737+
expected: false,
738+
},
739+
{
740+
name: "timeout error",
741+
err: fmt.Errorf("context deadline exceeded"),
742+
expected: false,
743+
},
744+
}
745+
746+
for _, tt := range tests {
747+
t.Run(tt.name, func(t *testing.T) {
748+
result := isHTTPConnectionError(tt.err)
749+
assert.Equal(t, tt.expected, result, "isHTTPConnectionError should return %v for %v", tt.expected, tt.err)
750+
})
751+
}
752+
}

‎internal/server/call_tool_result_test.go‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package server
22

33
import (
4+
"fmt"
45
"testing"
56

67
sdk "github.com/modelcontextprotocol/go-sdk/mcp"
@@ -151,3 +152,41 @@ func TestConvertToCallToolResult_NilCheck(t *testing.T) {
151152

152153
t.Log("✓ CallToolResult is properly non-nil and structured")
153154
}
155+
156+
// TestNewErrorCallToolResult tests the error CallToolResult helper
157+
func TestNewErrorCallToolResult(t *testing.T) {
158+
tests := []struct {
159+
name string
160+
err error
161+
expectError bool
162+
}{
163+
{
164+
name: "simple error",
165+
err: fmt.Errorf("test error"),
166+
expectError: true,
167+
},
168+
{
169+
name: "formatted error",
170+
err: fmt.Errorf("formatted error: %s", "test"),
171+
expectError: true,
172+
},
173+
}
174+
175+
for _, tt := range tests {
176+
t.Run(tt.name, func(t *testing.T) {
177+
result, data, err := newErrorCallToolResult(tt.err)
178+
179+
// Verify the error is returned
180+
assert.Equal(t, tt.err, err, "Error should be returned as-is")
181+
182+
// Verify data is nil
183+
assert.Nil(t, data, "Data should be nil for error results")
184+
185+
// Verify CallToolResult is properly structured
186+
require.NotNil(t, result, "CallToolResult should not be nil")
187+
assert.True(t, result.IsError, "IsError should be true")
188+
189+
t.Logf("✓ Error CallToolResult properly created with IsError=%v", result.IsError)
190+
})
191+
}
192+
}

‎internal/server/unified.go‎

Lines changed: 23 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -320,7 +320,7 @@ func (us *UnifiedServer) registerToolsFromBackend(serverID string) error {
320320
toolArgs, err := parseToolArguments(req)
321321
if err != nil {
322322
logger.LogError("client", "Failed to unmarshal tool arguments, tool=%s, error=%v", toolNameCopy, err)
323-
return &sdk.CallToolResult{IsError: true}, nil, err
323+
return newErrorCallToolResult(err)
324324
}
325325

326326
// Log the MCP tool call request
@@ -331,7 +331,7 @@ func (us *UnifiedServer) registerToolsFromBackend(serverID string) error {
331331
// Check session is initialized
332332
if err := us.requireSession(ctx); err != nil {
333333
logger.LogError("client", "MCP tool call failed: session not initialized, session=%s, tool=%s", sessionID, toolNameCopy)
334-
return &sdk.CallToolResult{IsError: true}, nil, err
334+
return newErrorCallToolResult(err)
335335
}
336336

337337
result, data, err := us.callBackendTool(ctx, serverIDCopy, toolNameCopy, toolArgs)
@@ -395,7 +395,7 @@ func (us *UnifiedServer) registerSysTools() error {
395395
toolArgs, err := parseToolArguments(req)
396396
if err != nil {
397397
logger.LogError("client", "Failed to unmarshal sys_init arguments, error=%v", err)
398-
return &sdk.CallToolResult{IsError: true}, nil, err
398+
return newErrorCallToolResult(err)
399399
}
400400

401401
// Extract token from args
@@ -408,7 +408,7 @@ func (us *UnifiedServer) registerSysTools() error {
408408
sessionID := us.getSessionID(ctx)
409409
if sessionID == "" {
410410
logger.LogError("client", "MCP session initialization failed: no session ID provided")
411-
return &sdk.CallToolResult{IsError: true}, nil, fmt.Errorf("no session ID provided")
411+
return newErrorCallToolResult(fmt.Errorf("no session ID provided"))
412412
}
413413

414414
logger.LogInfo("client", "MCP session initialization started, session=%s, has_token=%v", sessionID, token != "")
@@ -436,7 +436,7 @@ func (us *UnifiedServer) registerSysTools() error {
436436
result, err := us.sysServer.HandleRequest("tools/call", json.RawMessage(params))
437437
if err != nil {
438438
logger.LogError("client", "MCP session initialization: sys_init call failed, session=%s, error=%v", sessionID, err)
439-
return &sdk.CallToolResult{IsError: true}, nil, err
439+
return newErrorCallToolResult(err)
440440
}
441441

442442
resultJSON, _ := json.Marshal(result)
@@ -486,7 +486,7 @@ func (us *UnifiedServer) registerSysTools() error {
486486
// Check session is initialized
487487
if err := us.requireSession(ctx); err != nil {
488488
logger.LogError("client", "MCP sys_list_servers failed: session not initialized, session=%s", sessionID)
489-
return &sdk.CallToolResult{IsError: true}, nil, err
489+
return newErrorCallToolResult(err)
490490
}
491491

492492
params, _ := json.Marshal(map[string]interface{}{
@@ -496,7 +496,7 @@ func (us *UnifiedServer) registerSysTools() error {
496496
result, err := us.sysServer.HandleRequest("tools/call", json.RawMessage(params))
497497
if err != nil {
498498
logger.LogError("client", "MCP sys_list_servers error, session=%s, error=%v", sessionID, err)
499-
return &sdk.CallToolResult{IsError: true}, nil, err
499+
return newErrorCallToolResult(err)
500500
}
501501

502502
resultJSON, _ := json.Marshal(result)
@@ -657,7 +657,7 @@ func (us *UnifiedServer) callBackendTool(ctx context.Context, serverID, toolName
657657
resource, operation, err := g.LabelResource(ctx, toolName, args, backendCaller, us.capabilities)
658658
if err != nil {
659659
log.Printf("[DIFC] Guard labeling failed: %v", err)
660-
return &sdk.CallToolResult{IsError: true}, nil, fmt.Errorf("guard labeling failed: %w", err)
660+
return newErrorCallToolResult(fmt.Errorf("guard labeling failed: %w", err))
661661
}
662662

663663
log.Printf("[DIFC] Resource: %s | Operation: %s | Secrecy: %v | Integrity: %v",
@@ -688,33 +688,33 @@ func (us *UnifiedServer) callBackendTool(ctx context.Context, serverID, toolName
688688
sessionID := us.getSessionID(ctx)
689689
conn, err := launcher.GetOrLaunchForSession(us.launcher, serverID, sessionID)
690690
if err != nil {
691-
return &sdk.CallToolResult{IsError: true}, nil, fmt.Errorf("failed to connect: %w", err)
691+
return newErrorCallToolResult(fmt.Errorf("failed to connect: %w", err))
692692
}
693693

694694
response, err := conn.SendRequestWithServerID(ctx, "tools/call", map[string]interface{}{
695695
"name": toolName,
696696
"arguments": args,
697697
}, serverID)
698698
if err != nil {
699-
return &sdk.CallToolResult{IsError: true}, nil, err
699+
return newErrorCallToolResult(err)
700700
}
701701

702702
// Check if the backend returned an error
703703
if response.Error != nil {
704-
return &sdk.CallToolResult{IsError: true}, nil, fmt.Errorf("backend error: code=%d, message=%s", response.Error.Code, response.Error.Message)
704+
return newErrorCallToolResult(fmt.Errorf("backend error: code=%d, message=%s", response.Error.Code, response.Error.Message))
705705
}
706706

707707
// Parse the backend result
708708
var backendResult interface{}
709709
if err := json.Unmarshal(response.Result, &backendResult); err != nil {
710-
return &sdk.CallToolResult{IsError: true}, nil, fmt.Errorf("failed to parse result: %w", err)
710+
return newErrorCallToolResult(fmt.Errorf("failed to parse result: %w", err))
711711
}
712712

713713
// **Phase 4: Guard labels the response data (for fine-grained filtering)**
714714
labeledData, err := g.LabelResponse(ctx, toolName, backendResult, backendCaller, us.capabilities)
715715
if err != nil {
716716
log.Printf("[DIFC] Response labeling failed: %v", err)
717-
return &sdk.CallToolResult{IsError: true}, nil, fmt.Errorf("response labeling failed: %w", err)
717+
return newErrorCallToolResult(fmt.Errorf("response labeling failed: %w", err))
718718
}
719719

720720
// **Phase 5: Reference Monitor performs fine-grained filtering (if applicable)**
@@ -735,13 +735,13 @@ func (us *UnifiedServer) callBackendTool(ctx context.Context, serverID, toolName
735735
// Convert filtered data to result
736736
finalResult, err = filtered.ToResult()
737737
if err != nil {
738-
return &sdk.CallToolResult{IsError: true}, nil, fmt.Errorf("failed to convert filtered data: %w", err)
738+
return newErrorCallToolResult(fmt.Errorf("failed to convert filtered data: %w", err))
739739
}
740740
} else {
741741
// Simple labeled data - already passed coarse-grained check
742742
finalResult, err = labeledData.ToResult()
743743
if err != nil {
744-
return &sdk.CallToolResult{IsError: true}, nil, fmt.Errorf("failed to convert labeled data: %w", err)
744+
return newErrorCallToolResult(fmt.Errorf("failed to convert labeled data: %w", err))
745745
}
746746
}
747747

@@ -767,7 +767,7 @@ func (us *UnifiedServer) callBackendTool(ctx context.Context, serverID, toolName
767767
// Convert finalResult to SDK CallToolResult format
768768
callResult, err := convertToCallToolResult(finalResult)
769769
if err != nil {
770-
return &sdk.CallToolResult{IsError: true}, nil, fmt.Errorf("failed to convert result: %w", err)
770+
return newErrorCallToolResult(fmt.Errorf("failed to convert result: %w", err))
771771
}
772772

773773
return callResult, finalResult, nil
@@ -779,6 +779,13 @@ func (us *UnifiedServer) Run(transport sdk.Transport) error {
779779
return us.server.Run(us.ctx, transport)
780780
}
781781

782+
// newErrorCallToolResult creates a standard error CallToolResult
783+
// This helper reduces code duplication for error returns following the pattern:
784+
// return &sdk.CallToolResult{IsError: true}, nil, err
785+
func newErrorCallToolResult(err error) (*sdk.CallToolResult, interface{}, error) {
786+
return &sdk.CallToolResult{IsError: true}, nil, err
787+
}
788+
782789
// parseToolArguments extracts and unmarshals tool arguments from a CallToolRequest
783790
// Returns the parsed arguments as a map, or an error if parsing fails
784791
func parseToolArguments(req *sdk.CallToolRequest) (map[string]interface{}, error) {

0 commit comments

Comments
 (0)