Refactor: Consolidate function clustering — Docker utilities, stdlib reimplementation, and string truncation (#1350)

Analysis of 67 non-test Go files identified 5 refactoring opportunities:
misplaced Docker logic, near-duplicate truncation utilities, repeated
logger boilerplate, and a trivial stdlib reimplementation. This PR
addresses 3 high-impact opportunities with minimal code changes.

## Changes

- **Removed `containsEqual()` helper** — Replaced with
`strings.Contains(s, "=")` (stdlib)
  - Function manually iterated through string to find `'='` character
  - Already used `strings.Contains()` elsewhere in same file

- **Extracted Docker utilities to `internal/dockerutil`**
  - Moved `expandDockerEnvArgs()` from `internal/mcp/connection.go`
  - Avoids circular dependency between `mcp` and `launcher` packages
  - Tests moved to `internal/dockerutil/env_test.go`

- **Created `internal/strutil` package for string truncation**
  - Consolidates 4 duplicate truncation patterns across codebase:
- Inline truncation in `connection.go` (500 chars + "... (truncated)")
    - `truncateAndSanitize()` in `logger/rpc_helpers.go`
- `TruncateSecret()` in `logger/sanitize` (special case, not modified)
- `TruncateSessionID()` in `auth/header` (special case, not modified)
- Provides `Truncate()` and `TruncateWithSuffix()` with consistent edge
case handling

```go
// Before: manual truncation with inconsistent patterns
bodyPreview := string(body)
if len(bodyPreview) > 500 {
    bodyPreview = bodyPreview[:500] + "... (truncated)"
}

// After: unified utility
bodyPreview := strutil.TruncateWithSuffix(string(body), 500, "... (truncated)")
```

## Out of Scope

**Logger boilerplate consolidation** deferred — ~86 wrapper functions
across 6 logger types follow consistent patterns for type safety.
Refactoring would require architectural changes (generics/reflection)
with marginal benefit vs risk.

> [!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-build3509919732/b284/launcher.test
/tmp/go-build3509919732/b284/launcher.test
-test.testlogfile=/tmp/go-build3509919732/b284/testlog.txt
-test.paniconexit0 -test.timeout=10m0s -test.v=true --abbrev-ref HEAD
ache/uv/0.10.5/x86_64/git
64/pkg/tool/linu/opt/hostedtoolcache/go/1.25.6/x64/pkg/tool/linux_amd64/vet
base64 ache/Python/3.12/tmp/go-build2383556780/b045/vet.cfg base64 -d`
(dns block)
> - `invalid-host-that-does-not-exist-12345.com`
> - Triggering command: `/tmp/go-build3509919732/b266/config.test
/tmp/go-build3509919732/b266/config.test
-test.testlogfile=/tmp/go-build3509919732/b266/testlog.txt
-test.paniconexit0 -test.timeout=10m0s -test.v=true
home/REDACTED/.nvm-n1 /sys/fs/cgroup 64/bin/git
64/pkg/tool/linu/opt/hostedtoolcache/go/1.25.6/x64/pkg/tool/linux_amd64/vet
base64 ache/uv/0.10.5/x/tmp/go-build2383556780/b066/vet.cfg base64 -d
rgo/bin/git iptables /home/REDACTED/.cargo/bin/git -t security
ache/Python/3.12/tmp/go-build2383556780/b228/vet.cfg git` (dns block)
> - `nonexistent.local`
> - Triggering command: `/tmp/go-build3509919732/b284/launcher.test
/tmp/go-build3509919732/b284/launcher.test
-test.testlogfile=/tmp/go-build3509919732/b284/testlog.txt
-test.paniconexit0 -test.timeout=10m0s -test.v=true --abbrev-ref HEAD
ache/uv/0.10.5/x86_64/git
64/pkg/tool/linu/opt/hostedtoolcache/go/1.25.6/x64/pkg/tool/linux_amd64/vet
base64 ache/Python/3.12/tmp/go-build2383556780/b045/vet.cfg base64 -d`
(dns block)
> - `slow.example.com`
> - Triggering command: `/tmp/go-build3509919732/b284/launcher.test
/tmp/go-build3509919732/b284/launcher.test
-test.testlogfile=/tmp/go-build3509919732/b284/testlog.txt
-test.paniconexit0 -test.timeout=10m0s -test.v=true --abbrev-ref HEAD
ache/uv/0.10.5/x86_64/git
64/pkg/tool/linu/opt/hostedtoolcache/go/1.25.6/x64/pkg/tool/linux_amd64/vet
base64 ache/Python/3.12/tmp/go-build2383556780/b045/vet.cfg base64 -d`
(dns block)
> - `this-host-does-not-exist-12345.com`
> - Triggering command: `/tmp/go-build3663849786/b001/mcp.test
/tmp/go-build3663849786/b001/mcp.test
-test.testlogfile=/tmp/go-build3663849786/b001/testlog.txt
-test.paniconexit0 -test.timeout=10m0s -test.v=true --abbrev-ref HEAD
p/bin/git --abbrev-ref HEAD /usr/bin/base64 base64 -d 64/bin/as base64
86_64/git /opt/hostedtoolcbase64 ache/go/1.25.6/x-d /usr/bin/git git`
(dns block)
> - Triggering command: `/tmp/go-build3509919732/b293/mcp.test
/tmp/go-build3509919732/b293/mcp.test
-test.testlogfile=/tmp/go-build3509919732/b293/testlog.txt
-test.paniconexit0 -test.timeout=10m0s -test.v=true ./internal/mcp
./internal/logge-nolocalimports /usr/bin/dirname-importcfg [:upper:] git
nfig/composer/ve-unreachable=false dirname /hom�� @v5.3.1/compiler.go
@v5.3.1/content.go x_amd64/compile s#\(.*\)\.\([^\.grep -e
/opt/hostedtoolc(create|run) x_amd64/compile` (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>[refactor] Semantic Function Clustering Analysis —
Outliers, Near-Duplicates, and Consolidation Opportunities</issue_title>
<issue_description>## Overview

Analysis of **67 non-test Go source files** across `internal/`
catalogued **426 top-level functions** and identified 5 actionable
refactoring opportunities. The findings span misplaced functions (wrong
package), near-duplicate string-truncation utilities, repeated
boilerplate logger wrappers, and a trivial helper reimplementing stdlib.

The `internal/logger/` package is the most complex area (~120 functions
across 13 files) and contains the most structural repetition. The
`internal/mcp/connection.go` file contains Docker-specific logic that
belongs in `internal/launcher/`.

<details><summary><b>Full Report</b></summary>

## Analysis Metadata

| Metric | Value |
|--------|-------|
| Go files analyzed | 67 (non-test, `internal/` only) |
| Functions catalogued | 426 |
| Packages analysed | 16 |
| Outliers found | 2 |
| Near-duplicates detected | 3 clusters |
| Analysis date | 2026-02-23 |

---

## Identified Issues

### 1. 🗂️ Outlier: Docker-specific functions in MCP connection file

**Severity**: Medium  
**File**: `internal/mcp/connection.go` (lines 968–1005)  
**Functions**: `expandDockerEnvArgs`, `containsEqual`

`connection.go` is responsible for MCP protocol transport (stdio, HTTP,
SSE). However it contains Docker-specific argument-expansion logic that
belongs conceptually in `internal/launcher/`:

```go
// internal/mcp/connection.go — Docker logic in an MCP transport file
func expandDockerEnvArgs(args []string) []string { ... }
func containsEqual(s string) bool { ... }
```

**Additional problem**: `containsEqual` reimplements
`strings.Contains(s, "=")` from the standard library. The entire
function body can be replaced with a one-liner.

**Recommendation**:  
- Move `expandDockerEnvArgs` to `internal/launcher/launcher.go` (or a
new `internal/launcher/docker_args.go`), where Docker process arguments
are already assembled
- Remove `containsEqual` entirely and inline `strings.Contains(arg,
"=")` at the single call site
- **Estimated effort**: 30 minutes  
- **Benefits**: `connection.go` stays protocol-focused; Docker
pre-processing stays near Docker process launch

---

### 2. 🗂️ Outlier: `applyAuthIfConfigured` in transport file instead of
auth file

**Severity**: Low  
**File**: `internal/server/transport.go` (lines 59–65)  
**Function**: `applyAuthIfConfigured`

The project already has a dedicated `internal/server/auth.go` that owns
`authMiddleware`. `applyAuthIfConfigured` is a thin conditional wrapper
around it:

```go
// internal/server/transport.go — auth helper split from auth.go
func applyAuthIfConfigured(apiKey string, handler http.HandlerFunc) http.HandlerFunc {
    if apiKey != "" {
        return authMiddleware(apiKey, handler)
    }
    return handler
}
```

`applyAuthIfConfigured` is called from `http_helpers.go` and
`handlers.go` — neither of which is `transport.go`. The function's
natural home is `auth.go` alongside `authMiddleware`.

**Recommendation**:  
- Move `applyAuthIfConfigured` from `transport.go` to `auth.go`  
- **Estimated effort**: 15 minutes  
- **Benefits**: All auth-gating logic lives in one file; `transport.go`
becomes purely about HTTP server/transport construction

---

### 3. 🔁 Near-duplicate: `TruncateSessionID` vs `TruncateSecret`

**Severity**: Low  
**Files**: `internal/auth/header.go:172`,
`internal/logger/sanitize/sanitize.go:75`

Both functions truncate a string for safe logging, but with slightly
different semantics:

```go
// internal/auth/header.go
func TruncateSessionID(sessionID string) string {
    if sessionID == ""  { return "(none)" }
    if len(sessionID) <= 8 { return sessionID }
    return sessionID[:8] + "..."
}

// internal/logger/sanitize/sanitize.go
func TruncateSecret(input string) string {
    if len(input) > 4  { return input[:4] + "..." }
    else if len(input) > 0 { return "..." }
    return ""
}
```

The two functions differ in: prefix length (8 vs 4), empty-string return
value (`"(none)"` vs `""`), and short-input handling. Despite different
semantics, both exist to prevent sensitive data from appearing in logs.

**Recommendation**:  
- Consider whether `TruncateSessionID` could call `TruncateSecret` (or a
parameterised version) to reduce divergence
- At minimum, add a cross-reference comment in each function pointing to
the other
- A longer-term option: add a `TruncateWithLength(s string, n int,
fallback string) string` helper in `sanitize/` and have both functions
delegate to it
- **Estimated effort**: 1–2 hours  
- **Benefits**: Single source of truth for truncation semantics; easier
to change policy (e.g., 8 chars → 12 chars) in one place

---

### 4. 🔁 Structural repetition: `LogX` / `LogXMd` / `LogXWithServer`
quadruplets

**Severity**: Medium  
**Files**: `internal/logger/file_logger....

</details>



<!-- START COPILOT CODING AGENT SUFFIX -->

- Fixes #1336

Author: lpcox

internal/dockerutil/env.go

@@ -0,0 +1,33 @@
+package dockerutil
+
+import (
+	"fmt"
+	"os"
+	"strings"
+)
+
+// ExpandEnvArgs expands Docker -e flags that reference environment variables
+// Converts "-e VAR_NAME" to "-e VAR_NAME=value" by reading from the process environment
+func ExpandEnvArgs(args []string) []string {
+	result := make([]string, 0, len(args))
+	for i := 0; i < len(args); i++ {
+		arg := args[i]
+
+		// Check if this is a -e flag
+		if arg == "-e" && i+1 < len(args) {
+			nextArg := args[i+1]
+			// If next arg doesn't contain '=', it's a variable reference
+			if len(nextArg) > 0 && !strings.Contains(nextArg, "=") {
+				// Look up the variable in the environment
+				if value, exists := os.LookupEnv(nextArg); exists {
+					result = append(result, "-e")
+					result = append(result, fmt.Sprintf("%s=%s", nextArg, value))
+					i++ // Skip the next arg since we processed it
+					continue
+				}
+			}
+		}
+		result = append(result, arg)
+	}
+	return result
+}

internal/dockerutil/env_test.go

@@ -0,0 +1,91 @@
+package dockerutil
+
+import (
+	"os"
+	"testing"
+)
+
+func TestExpandEnvArgs(t *testing.T) {
+	tests := []struct {
+		name     string
+		args     []string
+		envVars  map[string]string
+		expected []string
+	}{
+		{
+			name:     "no -e flags",
+			args:     []string{"run", "--rm", "image"},
+			envVars:  map[string]string{},
+			expected: []string{"run", "--rm", "image"},
+		},
+		{
+			name:     "expand single env variable",
+			args:     []string{"run", "-e", "VAR_NAME", "image"},
+			envVars:  map[string]string{"VAR_NAME": "value1"},
+			expected: []string{"run", "-e", "VAR_NAME=value1", "image"},
+		},
+		{
+			name:     "expand multiple env variables",
+			args:     []string{"run", "-e", "VAR1", "-e", "VAR2", "image"},
+			envVars:  map[string]string{"VAR1": "value1", "VAR2": "value2"},
+			expected: []string{"run", "-e", "VAR1=value1", "-e", "VAR2=value2", "image"},
+		},
+		{
+			name:     "preserve existing key=value format",
+			args:     []string{"run", "-e", "VAR=predefined", "image"},
+			envVars:  map[string]string{},
+			expected: []string{"run", "-e", "VAR=predefined", "image"},
+		},
+		{
+			name:     "mixed: expand and preserve",
+			args:     []string{"run", "-e", "VAR1", "-e", "VAR2=fixed", "image"},
+			envVars:  map[string]string{"VAR1": "value1"},
+			expected: []string{"run", "-e", "VAR1=value1", "-e", "VAR2=fixed", "image"},
+		},
+		{
+			name:     "undefined env variable",
+			args:     []string{"run", "-e", "UNDEFINED_VAR", "image"},
+			envVars:  map[string]string{},
+			expected: []string{"run", "-e", "UNDEFINED_VAR", "image"},
+		},
+		{
+			name:     "empty env variable value",
+			args:     []string{"run", "-e", "EMPTY_VAR", "image"},
+			envVars:  map[string]string{"EMPTY_VAR": ""},
+			expected: []string{"run", "-e", "EMPTY_VAR=", "image"},
+		},
+		{
+			name:     "-e at end of args (no following arg)",
+			args:     []string{"run", "image", "-e"},
+			envVars:  map[string]string{},
+			expected: []string{"run", "image", "-e"},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			// Set up environment variables for test
+			for k, v := range tt.envVars {
+				os.Setenv(k, v)
+			}
+			// Clean up after test
+			t.Cleanup(func() {
+				for k := range tt.envVars {
+					os.Unsetenv(k)
+				}
+			})
+
+			result := ExpandEnvArgs(tt.args)
+
+			if len(result) != len(tt.expected) {
+				t.Fatalf("Expected %d args, got %d: %v", len(tt.expected), len(result), result)
+			}
+
+			for i := range result {
+				if result[i] != tt.expected[i] {
+					t.Errorf("Arg %d: expected '%s', got '%s'", i, tt.expected[i], result[i])
+				}
+			}
+		})
+	}
+}

internal/logger/rpc_helpers.go

@@ -20,6 +20,7 @@ import (
 	"strings"
 
 	"github.com/github/gh-aw-mcpg/internal/logger/sanitize"
+	"github.com/github/gh-aw-mcpg/internal/strutil"
 )
 
 // Pre-compiled regexes for performance (avoid recompiling in hot paths).
@@ -44,10 +45,7 @@ func truncateAndSanitize(payload string, maxLength int) string {
 	sanitized := sanitize.SanitizeString(payload)
 
 	// Then truncate if needed
-	if len(sanitized) > maxLength {
-		return sanitized[:maxLength] + "..."
-	}
-	return sanitized
+	return strutil.Truncate(sanitized, maxLength)
 }
 
 // extractEssentialFields extracts key fields from the payload for logging

internal/mcp/connection.go

@@ -10,14 +10,15 @@ import (
 	"log"
 	"log/slog"
 	"net/http"
-	"os"
 	"os/exec"
 	"strings"
 	"sync/atomic"
 	"time"
 
+	"github.com/github/gh-aw-mcpg/internal/dockerutil"
 	"github.com/github/gh-aw-mcpg/internal/logger"
 	"github.com/github/gh-aw-mcpg/internal/logger/sanitize"
+	"github.com/github/gh-aw-mcpg/internal/strutil"
 	"github.com/github/gh-aw-mcpg/internal/version"
 	sdk "github.com/modelcontextprotocol/go-sdk/mcp"
 )
@@ -93,10 +94,7 @@ func parseJSONRPCResponseWithSSE(body []byte, statusCode int, contextDesc string
 				return httpErrorResponse(), nil
 			}
 			// Include the response body to help debug what the server actually returned
-			bodyPreview := string(body)
-			if len(bodyPreview) > 500 {
-				bodyPreview = bodyPreview[:500] + "... (truncated)"
-			}
+			bodyPreview := strutil.TruncateWithSuffix(string(body), 500, "... (truncated)")
 			return nil, fmt.Errorf("failed to parse %s (received non-JSON or malformed response): %w\nResponse body: %s", contextDesc, sseErr, bodyPreview)
 		}
 
@@ -314,7 +312,7 @@ func NewConnection(ctx context.Context, serverID, command string, args []string,
 
 	// Expand Docker -e flags that reference environment variables
 	// Docker's `-e VAR_NAME` expects VAR_NAME to be in the environment
-	expandedArgs := expandDockerEnvArgs(args)
+	expandedArgs := dockerutil.ExpandEnvArgs(args)
 	logConn.Printf("Expanded args for Docker env: %v", sanitize.SanitizeArgs(expandedArgs))
 
 	// Create command transport
@@ -965,41 +963,6 @@ func (c *Connection) getPrompt(params interface{}) (*Response, error) {
 	return marshalToResponse(result)
 }
 
-// expandDockerEnvArgs expands Docker -e flags that reference environment variables
-// Converts "-e VAR_NAME" to "-e VAR_NAME=value" by reading from the process environment
-func expandDockerEnvArgs(args []string) []string {
-	result := make([]string, 0, len(args))
-	for i := 0; i < len(args); i++ {
-		arg := args[i]
-
-		// Check if this is a -e flag
-		if arg == "-e" && i+1 < len(args) {
-			nextArg := args[i+1]
-			// If next arg doesn't contain '=', it's a variable reference
-			if len(nextArg) > 0 && !containsEqual(nextArg) {
-				// Look up the variable in the environment
-				if value, exists := os.LookupEnv(nextArg); exists {
-					result = append(result, "-e")
-					result = append(result, fmt.Sprintf("%s=%s", nextArg, value))
-					i++ // Skip the next arg since we processed it
-					continue
-				}
-			}
-		}
-		result = append(result, arg)
-	}
-	return result
-}
-
-func containsEqual(s string) bool {
-	for _, c := range s {
-		if c == '=' {
-			return true
-		}
-	}
-	return false
-}
-
 // Close closes the connection
 func (c *Connection) Close() error {
 	c.cancel()

internal/mcp/connection_test.go

@@ -7,7 +7,6 @@ import (
 	"io"
 	"net/http"
 	"net/http/httptest"
-	"os"
 	"strings"
 	"testing"
 
@@ -131,92 +130,6 @@ func TestHTTPRequest_ConfiguredHeaders(t *testing.T) {
 	assert.Equal(t, sessionID, receivedSessionID)
 }
 
-// TestExpandDockerEnvArgs tests the Docker environment variable expansion function
-func TestExpandDockerEnvArgs(t *testing.T) {
-	tests := []struct {
-		name     string
-		args     []string
-		envVars  map[string]string
-		expected []string
-	}{
-		{
-			name:     "no -e flags",
-			args:     []string{"run", "--rm", "image"},
-			envVars:  map[string]string{},
-			expected: []string{"run", "--rm", "image"},
-		},
-		{
-			name:     "expand single env variable",
-			args:     []string{"run", "-e", "VAR_NAME", "image"},
-			envVars:  map[string]string{"VAR_NAME": "value1"},
-			expected: []string{"run", "-e", "VAR_NAME=value1", "image"},
-		},
-		{
-			name:     "expand multiple env variables",
-			args:     []string{"run", "-e", "VAR1", "-e", "VAR2", "image"},
-			envVars:  map[string]string{"VAR1": "value1", "VAR2": "value2"},
-			expected: []string{"run", "-e", "VAR1=value1", "-e", "VAR2=value2", "image"},
-		},
-		{
-			name:     "preserve existing key=value format",
-			args:     []string{"run", "-e", "VAR=predefined", "image"},
-			envVars:  map[string]string{},
-			expected: []string{"run", "-e", "VAR=predefined", "image"},
-		},
-		{
-			name:     "mixed: expand and preserve",
-			args:     []string{"run", "-e", "VAR1", "-e", "VAR2=fixed", "image"},
-			envVars:  map[string]string{"VAR1": "value1"},
-			expected: []string{"run", "-e", "VAR1=value1", "-e", "VAR2=fixed", "image"},
-		},
-		{
-			name:     "undefined env variable",
-			args:     []string{"run", "-e", "UNDEFINED_VAR", "image"},
-			envVars:  map[string]string{},
-			expected: []string{"run", "-e", "UNDEFINED_VAR", "image"},
-		},
-		{
-			name:     "empty env variable value",
-			args:     []string{"run", "-e", "EMPTY_VAR", "image"},
-			envVars:  map[string]string{"EMPTY_VAR": ""},
-			expected: []string{"run", "-e", "EMPTY_VAR=", "image"},
-		},
-		{
-			name:     "-e at end of args (no following arg)",
-			args:     []string{"run", "image", "-e"},
-			envVars:  map[string]string{},
-			expected: []string{"run", "image", "-e"},
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			// Set up environment variables for test
-			for k, v := range tt.envVars {
-				os.Setenv(k, v)
-			}
-			// Clean up after test
-			t.Cleanup(func() {
-				for k := range tt.envVars {
-					os.Unsetenv(k)
-				}
-			})
-
-			result := expandDockerEnvArgs(tt.args)
-
-			if len(result) != len(tt.expected) {
-				t.Fatalf("Expected %d args, got %d: %v", len(tt.expected), len(result), result)
-			}
-
-			for i := range result {
-				if result[i] != tt.expected[i] {
-					t.Errorf("Arg %d: expected '%s', got '%s'", i, tt.expected[i], result[i])
-				}
-			}
-		})
-	}
-}
-
 // TestHTTPRequest_ErrorResponses tests handling of various error conditions
 func TestHTTPRequest_ErrorResponses(t *testing.T) {
 	tests := []struct {

internal/strutil/truncate.go

@@ -0,0 +1,31 @@
+package strutil
+
+// Truncate truncates a string to the specified maximum length.
+// If the string is longer than maxLen, it's truncated and "..." is appended.
+// If maxLen is 0, returns "..." for non-empty strings, empty string for empty strings.
+// If maxLen is negative, the original string is returned.
+func Truncate(s string, maxLen int) string {
+	if maxLen < 0 {
+		return s
+	}
+	if maxLen == 0 {
+		if len(s) > 0 {
+			return "..."
+		}
+		return ""
+	}
+	if len(s) <= maxLen {
+		return s
+	}
+	return s[:maxLen] + "..."
+}
+
+// TruncateWithSuffix truncates a string to the specified maximum length with a custom suffix.
+// If the string is longer than maxLen, it's truncated and suffix is appended.
+// If maxLen is 0 or negative, the original string is returned.
+func TruncateWithSuffix(s string, maxLen int, suffix string) string {
+	if maxLen <= 0 || len(s) <= maxLen {
+		return s
+	}
+	return s[:maxLen] + suffix
+}