refactor: extract duplicate code patterns into helpers (#1855)

lpcox · web-flow · commit 8269bae9fcf7 · 2026-03-13T09:39:07.000-07:00
Addresses duplicate code analysis identifying ~50 lines of structural duplication across two patterns in `internal/server/unified.go`. ## Changes ### Pattern 1: Backend Tool Call Execution (~40 lines eliminated) Extracted `executeBackendToolCall` helper consolidating: - Connection management via `launcher.GetOrLaunchForSession` - `tools/call` request dispatch - Backend error checking - Result unmarshaling **Before:** ```go // guardBackendCaller.CallTool and callBackendTool Phase 3 each had: conn, err := launcher.GetOrLaunchForSession(us.launcher, serverID, sessionID) if err != nil { ... } response, err := conn.SendRequestWithServerID(ctx, "tools/call", ...) if err != nil { ... } if response.Error != nil { ... } var result interface{} json.Unmarshal(response.Result, &result) ``` **After:** ```go result, err := executeBackendToolCall(ctx, us.launcher, serverID, sessionID, toolName, args) ``` ### Pattern 2: Sys Tool Delegation (~10 lines eliminated) Extracted `callSysServer` helper consolidating: - Tool name marshaling - `sysServer.HandleRequest` delegation **Before:** ```go // sysInitHandler and sysListHandler each had: params, _ := json.Marshal(map[string]interface{}{ "name": toolName, "arguments": map[string]interface{}{}, }) result, err := us.sysServer.HandleRequest("tools/call", json.RawMessage(params)) ``` **After:** ```go result, err := us.callSysServer("sys_init") ``` ## Impact - Eliminates copy-paste error risk when modifying backend call or sys server logic - Follows existing helper patterns (`newErrorCallToolResult`, `parseToolArguments`) - No functional changes—pure refactoring > [!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-build4242760615/b336/launcher.test /tmp/go-build4242760615/b336/launcher.test -test.testlogfile=/tmp/go-build4242760615/b336/testlog.txt -test.paniconexit0 -test.timeout=10m0s -uns�� ache/go/1.25.7/x64/src/net /tmp/go-build1249286257/b024/vet.cfg ache/go/1.25.7/x64/pkg/tool/linux_amd64/vet unset position.go .12/x64/bin/git ache/go/1.25.7/x64/pkg/tool/linuHEAD -W -I /tmp/go-build1249286257/b165/ ache/go/1.25.7/x64/pkg/tool/linux_amd64/compile . --gdwarf2 --64 ache/go/1.25.7/x64/pkg/tool/linux_amd64/compile` (dns block) > - Triggering command: `/tmp/go-build2320097585/b332/launcher.test /tmp/go-build2320097585/b332/launcher.test -test.testlogfile=/tmp/go-build2320097585/b332/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 64/bin/git ortcfg /usr/sbin/bash submodules | heanode 9286257/b132/vet-v ndor/bin/git bash /usr�� runtime-runc/moby ortcfg csi/net-interface-handler aw-mcpg/internal/opt/hostedtoolcache/go/1.25.7/x64/pkg/tool/linux_amd64/asm aw-mcpg/internal-V=full` (dns block) > - `invalid-host-that-does-not-exist-12345.com` > - Triggering command: `/tmp/go-build4242760615/b318/config.test /tmp/go-build4242760615/b318/config.test -test.testlogfile=/tmp/go-build4242760615/b318/testlog.txt -test.paniconexit0 -test.timeout=10m0s --de�� c.test --debug-prefix-map 64/bin/git -I /opt/hostedtoolc-unsafeptr=false ut-1969789729.c VF78F_aidvBPXKXaKR/S4HB-3OSyH3DKHEAD` (dns block) > - Triggering command: `/tmp/go-build2320097585/b314/config.test /tmp/go-build2320097585/b314/config.test -test.testlogfile=/tmp/go-build2320097585/b314/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 17fb9960acf3cdef -importcfg ine by/b2276789a3c14/usr/bin/chronyc -w -buildmode=exe ine --ve�� 85e4fa5c5748c5dd -extld=gcc /opt/hostedtoolcache/go/1.25.7/xjson aw-mcpg/internalbase64 9286257/b151/vet-d ns /opt/hostedtoolcache/go/1.25.7/x64/pkg/tool/linu--listen` (dns block) > - `nonexistent.local` > - Triggering command: `/tmp/go-build4242760615/b336/launcher.test /tmp/go-build4242760615/b336/launcher.test -test.testlogfile=/tmp/go-build4242760615/b336/testlog.txt -test.paniconexit0 -test.timeout=10m0s -uns�� ache/go/1.25.7/x64/src/net /tmp/go-build1249286257/b024/vet.cfg ache/go/1.25.7/x64/pkg/tool/linux_amd64/vet unset position.go .12/x64/bin/git ache/go/1.25.7/x64/pkg/tool/linuHEAD -W -I /tmp/go-build1249286257/b165/ ache/go/1.25.7/x64/pkg/tool/linux_amd64/compile . --gdwarf2 --64 ache/go/1.25.7/x64/pkg/tool/linux_amd64/compile` (dns block) > - Triggering command: `/tmp/go-build2320097585/b332/launcher.test /tmp/go-build2320097585/b332/launcher.test -test.testlogfile=/tmp/go-build2320097585/b332/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 64/bin/git ortcfg /usr/sbin/bash submodules | heanode 9286257/b132/vet-v ndor/bin/git bash /usr�� runtime-runc/moby ortcfg csi/net-interface-handler aw-mcpg/internal/opt/hostedtoolcache/go/1.25.7/x64/pkg/tool/linux_amd64/asm aw-mcpg/internal-V=full` (dns block) > - `slow.example.com` > - Triggering command: `/tmp/go-build4242760615/b336/launcher.test /tmp/go-build4242760615/b336/launcher.test -test.testlogfile=/tmp/go-build4242760615/b336/testlog.txt -test.paniconexit0 -test.timeout=10m0s -uns�� ache/go/1.25.7/x64/src/net /tmp/go-build1249286257/b024/vet.cfg ache/go/1.25.7/x64/pkg/tool/linux_amd64/vet unset position.go .12/x64/bin/git ache/go/1.25.7/x64/pkg/tool/linuHEAD -W -I /tmp/go-build1249286257/b165/ ache/go/1.25.7/x64/pkg/tool/linux_amd64/compile . --gdwarf2 --64 ache/go/1.25.7/x64/pkg/tool/linux_amd64/compile` (dns block) > - Triggering command: `/tmp/go-build2320097585/b332/launcher.test /tmp/go-build2320097585/b332/launcher.test -test.testlogfile=/tmp/go-build2320097585/b332/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 64/bin/git ortcfg /usr/sbin/bash submodules | heanode 9286257/b132/vet-v ndor/bin/git bash /usr�� runtime-runc/moby ortcfg csi/net-interface-handler aw-mcpg/internal/opt/hostedtoolcache/go/1.25.7/x64/pkg/tool/linux_amd64/asm aw-mcpg/internal-V=full` (dns block) > - `this-host-does-not-exist-12345.com` > - Triggering command: `/tmp/go-build4242760615/b345/mcp.test /tmp/go-build4242760615/b345/mcp.test -test.testlogfile=/tmp/go-build4242760615/b345/testlog.txt -test.paniconexit0 -test.timeout=10m0s -I ache/go/1.25.7/x64/src/net -I if (a[i] &amp;&amp; a[i] !~ /^[0-9]&#43;$/) exit(2); if (a[i] &lt; b[i]) exit(3); else if (a[i] &gt; b[i]) exit(0) --gdwarf-5 --64 -o ache/go/1.25.7/x64/pkg/tool/linuHEAD -o /tmp/go-build1249286257/b210/_pkg_.a -trimpath ache/go/1.25.7/x64/pkg/tool/linux_amd64/vet -p github.com/githu/tmp/go-build3632611954/b228/vet.cfg -lang=go1.25 ache/go/1.25.7/x64/pkg/tool/linux_amd64/vet` (dns block) > - Triggering command: `/tmp/go-build2320097585/b341/mcp.test /tmp/go-build2320097585/b341/mcp.test -test.testlogfile=/tmp/go-build2320097585/b341/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true /home/REDACTED/work/gh-aw-mcpg/gh-aw-mcpg/internal/strutil/truncat6662819a7d6931901c1593745131476bbash y CURL_CA_BUNDLE=/etc/ssl/certs/ca-certificates.crt&#34;, &#34;REQUESTS_CA_BUNDLE=/etc/ssl/c--version by/bd0209c110101/opt/hostedtoolcache/go/1.25.7/x64/pkg/tool/linux_amd64/link aw-mcpg/internal-o ache/go/1.25.7/x/tmp/go-build2320097585/b314/config.test docker insp�� 551622d230bb31fc-s by/bd0209c110101-w rtificates.crt&#34;,-buildmode=exe by/a501e7528e5b9bash e330148d0ce51d5e/usr/bin/runc` (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>
diff --git a/internal/server/unified.go b/internal/server/unified.go
@@ -469,6 +469,21 @@ func (us *UnifiedServer) registerSysTool(name, description string, inputSchema m
 	us.toolsMu.Unlock()
 }
 
+// callSysServer is a helper that calls a sys tool by marshaling the tool name,
+// delegating to sysServer.HandleRequest, and returning the result.
+// This consolidates the common pattern used by sys tool handlers.
+func (us *UnifiedServer) callSysServer(toolName string) (interface{}, error) {
+	params, _ := json.Marshal(map[string]interface{}{
+		"name":      toolName,
+		"arguments": map[string]interface{}{},
+	})
+	result, err := us.sysServer.HandleRequest("tools/call", json.RawMessage(params))
+	if err != nil {
+		return nil, err
+	}
+	return result, nil
+}
+
 // registerSysTools registers built-in sys tools
 func (us *UnifiedServer) registerSysTools() error {
 	// Create sys_init handler
@@ -511,11 +526,7 @@ func (us *UnifiedServer) registerSysTools() error {
 		log.Printf("Initialized session: %s", sessionID)
 
 		// Call sys_init
-		params, _ := json.Marshal(map[string]interface{}{
-			"name":      "sys_init",
-			"arguments": map[string]interface{}{},
-		})
-		result, err := us.sysServer.HandleRequest("tools/call", json.RawMessage(params))
+		result, err := us.callSysServer("sys_init")
 		if err != nil {
 			logger.LogError("client", "MCP session initialization: sys_init call failed, session=%s, error=%v", sessionID, err)
 			return newErrorCallToolResult(err)
@@ -554,11 +565,7 @@ func (us *UnifiedServer) registerSysTools() error {
 			return newErrorCallToolResult(err)
 		}
 
-		params, _ := json.Marshal(map[string]interface{}{
-			"name":      "sys_list_servers",
-			"arguments": map[string]interface{}{},
-		})
-		result, err := us.sysServer.HandleRequest("tools/call", json.RawMessage(params))
+		result, err := us.callSysServer("sys_list_servers")
 		if err != nil {
 			logger.LogError("client", "MCP sys_list_servers error, session=%s, error=%v", sessionID, err)
 			return newErrorCallToolResult(err)
@@ -783,32 +790,25 @@ func (us *UnifiedServer) createGuardFromConfig(name string, cfg *config.GuardCon
 	}
 }
 
-// guardBackendCaller implements guard.BackendCaller for guards to query backend metadata
-type guardBackendCaller struct {
-	server   *UnifiedServer
-	serverID string
-	ctx      context.Context
-}
-
-func (g *guardBackendCaller) CallTool(ctx context.Context, toolName string, args interface{}) (interface{}, error) {
-	// Make a read-only call to the backend for metadata
-	// This bypasses DIFC checks since it's internal to the guard
-	log.Printf("[DIFC] Guard calling backend %s tool %s for metadata", g.serverID, toolName)
-
-	// Get or launch backend connection (use session-aware connection for stateful backends)
-	sessionID := g.ctx.Value(SessionIDContextKey)
-	if sessionID == nil {
-		sessionID = "default"
-	}
-	conn, err := launcher.GetOrLaunchForSession(g.server.launcher, g.serverID, sessionID.(string))
+// executeBackendToolCall executes a backend MCP tool call and returns the raw result.
+// This helper consolidates the common pattern of:
+// 1. Get or launch backend connection
+// 2. Send tools/call request
+// 3. Check for backend error
+// 4. Unmarshal and return result
+//
+// Callers are responsible for adapting the result to their specific return types
+// and wrapping errors as needed.
+func executeBackendToolCall(ctx context.Context, l *launcher.Launcher, serverID, sessionID, toolName string, args interface{}) (interface{}, error) {
+	conn, err := launcher.GetOrLaunchForSession(l, serverID, sessionID)
 	if err != nil {
 		return nil, fmt.Errorf("failed to connect: %w", err)
 	}
 
-	response, err := conn.SendRequestWithServerID(g.ctx, "tools/call", map[string]interface{}{
+	response, err := conn.SendRequestWithServerID(ctx, "tools/call", map[string]interface{}{
 		"name":      toolName,
 		"arguments": args,
-	}, g.serverID)
+	}, serverID)
 	if err != nil {
 		return nil, err
 	}
@@ -827,6 +827,27 @@ func (g *guardBackendCaller) CallTool(ctx context.Context, toolName string, args
 	return result, nil
 }
 
+// guardBackendCaller implements guard.BackendCaller for guards to query backend metadata
+type guardBackendCaller struct {
+	server   *UnifiedServer
+	serverID string
+	ctx      context.Context
+}
+
+func (g *guardBackendCaller) CallTool(ctx context.Context, toolName string, args interface{}) (interface{}, error) {
+	// Make a read-only call to the backend for metadata
+	// This bypasses DIFC checks since it's internal to the guard
+	log.Printf("[DIFC] Guard calling backend %s tool %s for metadata", g.serverID, toolName)
+
+	// Get or launch backend connection (use session-aware connection for stateful backends)
+	sessionID := g.ctx.Value(SessionIDContextKey)
+	if sessionID == nil {
+		sessionID = "default"
+	}
+
+	return executeBackendToolCall(g.ctx, g.server.launcher, g.serverID, sessionID.(string), toolName, args)
+}
+
 // convertToCallToolResult converts backend result data to SDK CallToolResult format
 // The backend returns a JSON object with a "content" field containing an array of content items
 func convertToCallToolResult(data interface{}) (*sdk.CallToolResult, error) {
@@ -1004,31 +1025,11 @@ func (us *UnifiedServer) callBackendTool(ctx context.Context, serverID, toolName
 	}
 
 	// **Phase 3: Execute the backend call**
-	// Get or launch backend connection (use session-aware connection for stateful backends)
-	conn, err := launcher.GetOrLaunchForSession(us.launcher, serverID, sessionID)
-	if err != nil {
-		return newErrorCallToolResult(fmt.Errorf("failed to connect: %w", err))
-	}
-
-	response, err := conn.SendRequestWithServerID(ctx, "tools/call", map[string]interface{}{
-		"name":      toolName,
-		"arguments": args,
-	}, serverID)
+	backendResult, err := executeBackendToolCall(ctx, us.launcher, serverID, sessionID, toolName, args)
 	if err != nil {
 		return newErrorCallToolResult(err)
 	}
 
-	// Check if the backend returned an error
-	if response.Error != nil {
-		return newErrorCallToolResult(fmt.Errorf("backend error: code=%d, message=%s", response.Error.Code, response.Error.Message))
-	}
-
-	// Parse the backend result
-	var backendResult interface{}
-	if err := json.Unmarshal(response.Result, &backendResult); err != nil {
-		return newErrorCallToolResult(fmt.Errorf("failed to parse result: %w", err))
-	}
-
 	// **Phase 4: Guard labels the response data (for fine-grained filtering)**
 	// Per spec: LabelResponse() is only called for read operations in all modes,
 	// and for read-write operations in filter/propagate modes.
