refactor: Semantic function clustering — split strutil.go, extract githubhttp package, fix validation.go placement, rename logger/common.go

AGENTS.md

@@ -33,8 +33,9 @@ Quick reference for AI agents working with MCP Gateway (Go-based MCP proxy serve
   - `validation_test.go` - Comprehensive validation tests
 - `internal/difc/` - Decentralized Information Flow Control
 - `internal/envutil/` - Environment variable and Docker env-arg utilities
+- `internal/githubhttp/` - GitHub API-specific HTTP helpers (auth headers, collaborator permission, rate-limit parsing)
 - `internal/guard/` - Security guards (NoopGuard, WasmGuard, WriteSinkGuard)
-- `internal/httputil/` - Shared HTTP helper utilities (server, proxy)
+- `internal/httputil/` - Generic HTTP helper utilities (server, proxy)
 - `internal/launcher/` - Backend process management
 - `internal/logger/` - Debug logging framework (micro logger)
 - `internal/mcp/` - MCP protocol types with enhanced error logging

internal/config/validation.go

@@ -512,3 +512,55 @@ func validateGuardPolicies(cfg *Config) error {
 	}
 	return nil
 }
+
+// validateRuleBasedPatterns validates additional rule-based string constraints that
+// are not handled by schema validation alone.
+func validateRuleBasedPatterns(stdinCfg *StdinConfig) error {
+	logValidation.Printf("Validating string patterns: server_count=%d", len(stdinCfg.MCPServers))
+
+	for name, server := range stdinCfg.MCPServers {
+		jsonPath := fmt.Sprintf("mcpServers.%s", name)
+		logValidation.Printf("Validating server: name=%s, type=%s", name, server.Type)
+
+		if IsStdioServerType(server.Type) {
+			if server.Container != "" && !containerPattern.MatchString(server.Container) {
+				return InvalidPattern("container", server.Container,
+					fmt.Sprintf("%s.container", jsonPath),
+					"Use a valid container image format (e.g., 'ghcr.io/owner/image:tag', 'owner/image:latest', or 'ghcr.io/owner/image:tag@sha256:<digest>')")
+			}
+
+			if server.Entrypoint != "" && len(strings.TrimSpace(server.Entrypoint)) == 0 {
+				return InvalidValue("entrypoint", "entrypoint cannot be empty or whitespace only",
+					fmt.Sprintf("%s.entrypoint", jsonPath),
+					"Provide a valid entrypoint path or remove the field")
+			}
+		}
+
+		if server.Type == "http" {
+			if server.URL != "" && !urlPattern.MatchString(server.URL) {
+				return InvalidPattern("url", server.URL,
+					fmt.Sprintf("%s.url", jsonPath),
+					"Use a valid HTTP or HTTPS URL (e.g., 'https://api.example.com/mcp')")
+			}
+		}
+	}
+
+	if stdinCfg.Gateway != nil {
+		if err := validateGatewayConfig(stdinCfg.Gateway); err != nil {
+			return err
+		}
+
+		if stdinCfg.Gateway.Domain != "" {
+			domain := stdinCfg.Gateway.Domain
+			if domain != "localhost" && domain != "host.docker.internal" &&
+				!domainVarPattern.MatchString(domain) && !domainHostnamePattern.MatchString(domain) {
+				return InvalidValue("domain",
+					fmt.Sprintf("domain '%s' must be 'localhost', 'host.docker.internal', an RFC-1123 hostname label (e.g. 'awmg-mcpg'), or a variable expression", domain),
+					"gateway.domain",
+					"Use 'localhost', 'host.docker.internal', a topology hostname like 'awmg-mcpg', or a variable like '${MCP_GATEWAY_DOMAIN}'")
+			}
+		}
+	}
+
+	return nil
+}

internal/config/validation_rules.go

@@ -219,55 +219,3 @@ func AbsolutePath(value, fieldName, jsonPath string) *ValidationError {
 		Suggestion: "Use an absolute path: Unix paths start with '/' (e.g., '/tmp/payloads'), Windows paths start with a drive letter (e.g., 'C:\\payloads')",
 	}
 }
-
-// validateRuleBasedPatterns validates additional rule-based string constraints that
-// are not handled by schema validation alone.
-func validateRuleBasedPatterns(stdinCfg *StdinConfig) error {
-	logValidation.Printf("Validating string patterns: server_count=%d", len(stdinCfg.MCPServers))
-
-	for name, server := range stdinCfg.MCPServers {
-		jsonPath := fmt.Sprintf("mcpServers.%s", name)
-		logValidation.Printf("Validating server: name=%s, type=%s", name, server.Type)
-
-		if IsStdioServerType(server.Type) {
-			if server.Container != "" && !containerPattern.MatchString(server.Container) {
-				return InvalidPattern("container", server.Container,
-					fmt.Sprintf("%s.container", jsonPath),
-					"Use a valid container image format (e.g., 'ghcr.io/owner/image:tag', 'owner/image:latest', or 'ghcr.io/owner/image:tag@sha256:<digest>')")
-			}
-
-			if server.Entrypoint != "" && len(strings.TrimSpace(server.Entrypoint)) == 0 {
-				return InvalidValue("entrypoint", "entrypoint cannot be empty or whitespace only",
-					fmt.Sprintf("%s.entrypoint", jsonPath),
-					"Provide a valid entrypoint path or remove the field")
-			}
-		}
-
-		if server.Type == "http" {
-			if server.URL != "" && !urlPattern.MatchString(server.URL) {
-				return InvalidPattern("url", server.URL,
-					fmt.Sprintf("%s.url", jsonPath),
-					"Use a valid HTTP or HTTPS URL (e.g., 'https://api.example.com/mcp')")
-			}
-		}
-	}
-
-	if stdinCfg.Gateway != nil {
-		if err := validateGatewayConfig(stdinCfg.Gateway); err != nil {
-			return err
-		}
-
-		if stdinCfg.Gateway.Domain != "" {
-			domain := stdinCfg.Gateway.Domain
-			if domain != "localhost" && domain != "host.docker.internal" &&
-				!domainVarPattern.MatchString(domain) && !domainHostnamePattern.MatchString(domain) {
-				return InvalidValue("domain",
-					fmt.Sprintf("domain '%s' must be 'localhost', 'host.docker.internal', an RFC-1123 hostname label (e.g. 'awmg-mcpg'), or a variable expression", domain),
-					"gateway.domain",
-					"Use 'localhost', 'host.docker.internal', a topology hostname like 'awmg-mcpg', or a variable like '${MCP_GATEWAY_DOMAIN}'")
-			}
-		}
-	}
-
-	return nil
-}

internal/httputil/github_http.go → internal/githubhttp/client.go

@@ -1,4 +1,6 @@
-package httputil
+// Package githubhttp provides GitHub API-specific HTTP helpers shared across
+// multiple packages (server, proxy, etc.).
+package githubhttp
 
 import (
 	"context"
@@ -7,8 +9,12 @@ import (
 	"strconv"
 	"strings"
 	"time"
+
+	"github.com/github/gh-aw-mcpg/internal/logger"
 )
 
+var logHTTP = logger.New("githubhttp:client")
+
 // GitHubUserAgent is the User-Agent header value sent on all GitHub API requests.
 const GitHubUserAgent = "awmg/1.0"
 
@@ -18,7 +24,7 @@ var defaultGitHubHTTPClient = &http.Client{Timeout: 30 * time.Second}
 
 // ApplyGitHubAPIHeaders sets the standard GitHub API request headers on req.
 // authHeader should be the full Authorization header value (e.g. "token xyz" or
-// "Bearer xyz"). When authHeader is empty no Authorization header is set, which
+// "******"). When authHeader is empty no Authorization header is set, which
 // is appropriate when the caller has already decided that no auth is available.
 func ApplyGitHubAPIHeaders(req *http.Request, authHeader string) {
 	path := "<nil>"

internal/githubhttp/client_test.go

@@ -0,0 +1,229 @@
+package githubhttp
+
+import (
+	"context"
+	"net/http"
+	"net/http/httptest"
+	"strconv"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// TestParseRateLimitResetHeader verifies the shared Unix-timestamp header parser.
+func TestParseRateLimitResetHeader(t *testing.T) {
+	t.Parallel()
+
+	now := time.Now()
+	future := now.Add(60 * time.Second)
+
+	tests := []struct {
+		name     string
+		value    string
+		wantZero bool
+		wantTime time.Time
+	}{
+		{
+			name:     "empty",
+			value:    "",
+			wantZero: true,
+		},
+		{
+			name:     "invalid",
+			value:    "not-a-number",
+			wantZero: true,
+		},
+		{
+			name:     "valid unix timestamp",
+			value:    "1000000000",
+			wantZero: false,
+			wantTime: time.Unix(1000000000, 0),
+		},
+		{
+			name:     "future timestamp",
+			value:    strconv.FormatInt(future.Unix(), 10),
+			wantZero: false,
+		},
+		{
+			name:     "value with surrounding whitespace",
+			value:    "  1000000000  ",
+			wantZero: false,
+			wantTime: time.Unix(1000000000, 0),
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			got := ParseRateLimitResetHeader(tt.value)
+			if tt.wantZero {
+				assert.True(t, got.IsZero(), "expected zero time")
+			} else {
+				assert.False(t, got.IsZero(), "expected non-zero time")
+				if !tt.wantTime.IsZero() {
+					assert.Equal(t, tt.wantTime.Unix(), got.Unix())
+				}
+			}
+		})
+	}
+}
+
+// TestApplyGitHubAPIHeaders verifies that ApplyGitHubAPIHeaders sets the
+// expected headers on an HTTP request.
+func TestApplyGitHubAPIHeaders(t *testing.T) {
+	t.Run("sets Authorization when authHeader is non-empty", func(t *testing.T) {
+		req, err := http.NewRequest(http.MethodGet, "http://example.com", nil)
+		require.NoError(t, err)
+
+		ApplyGitHubAPIHeaders(req, "token my-secret")
+
+		assert.Equal(t, "token my-secret", req.Header.Get("Authorization"))
+		assert.Equal(t, "application/vnd.github+json", req.Header.Get("Accept"))
+		assert.Equal(t, GitHubUserAgent, req.Header.Get("User-Agent"))
+	})
+
+	t.Run("does not set Authorization when authHeader is empty", func(t *testing.T) {
+		req, err := http.NewRequest(http.MethodGet, "http://example.com", nil)
+		require.NoError(t, err)
+
+		ApplyGitHubAPIHeaders(req, "")
+
+		assert.Empty(t, req.Header.Get("Authorization"))
+		assert.Equal(t, "application/vnd.github+json", req.Header.Get("Accept"))
+		assert.Equal(t, GitHubUserAgent, req.Header.Get("User-Agent"))
+	})
+
+	t.Run("works with Bearer token scheme", func(t *testing.T) {
+		req, err := http.NewRequest(http.MethodGet, "http://example.com", nil)
+		require.NoError(t, err)
+
+		ApplyGitHubAPIHeaders(req, "Bearer ghp_abc123")
+
+		assert.Equal(t, "Bearer ghp_abc123", req.Header.Get("Authorization"))
+		assert.Equal(t, "application/vnd.github+json", req.Header.Get("Accept"))
+		assert.Equal(t, GitHubUserAgent, req.Header.Get("User-Agent"))
+	})
+
+	t.Run("does not panic when request URL is nil", func(t *testing.T) {
+		req := &http.Request{Method: http.MethodGet, Header: make(http.Header)}
+
+		require.NotPanics(t, func() {
+			ApplyGitHubAPIHeaders(req, "token my-secret")
+		})
+
+		assert.Equal(t, "token my-secret", req.Header.Get("Authorization"))
+		assert.Equal(t, "application/vnd.github+json", req.Header.Get("Accept"))
+		assert.Equal(t, GitHubUserAgent, req.Header.Get("User-Agent"))
+	})
+}
+
+// TestDoGitHubGET verifies that DoGitHubGET sends a GET request with the correct
+// headers and URL to the upstream server.
+func TestDoGitHubGET(t *testing.T) {
+	t.Run("sends GET with GitHub headers", func(t *testing.T) {
+		var capturedMethod, capturedPath, capturedAuth, capturedAccept, capturedUA string
+		upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			capturedMethod = r.Method
+			capturedPath = r.URL.Path
+			capturedAuth = r.Header.Get("Authorization")
+			capturedAccept = r.Header.Get("Accept")
+			capturedUA = r.Header.Get("User-Agent")
+			w.WriteHeader(http.StatusOK)
+		}))
+		defer upstream.Close()
+
+		resp, err := DoGitHubGET(context.Background(), upstream.URL, "/repos/owner/repo", "token ghp_test")
+		require.NoError(t, err)
+		require.NotNil(t, resp)
+		defer resp.Body.Close()
+
+		assert.Equal(t, http.MethodGet, capturedMethod)
+		assert.Equal(t, "/repos/owner/repo", capturedPath)
+		assert.Equal(t, "token ghp_test", capturedAuth)
+		assert.Equal(t, "application/vnd.github+json", capturedAccept)
+		assert.Equal(t, GitHubUserAgent, capturedUA)
+		assert.Equal(t, http.StatusOK, resp.StatusCode)
+	})
+
+	t.Run("returns error on invalid URL", func(t *testing.T) {
+		resp, err := DoGitHubGET(context.Background(), "://bad-url", "/path", "token x")
+		assert.Error(t, err)
+		assert.Nil(t, resp)
+	})
+
+	t.Run("returns error when HTTP transport fails", func(t *testing.T) {
+		// Start a server that immediately closes the connection to force a transport error.
+		upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			// Hijack the connection and close it without sending a response.
+			hj, ok := w.(http.Hijacker)
+			if !ok {
+				http.Error(w, "hijack not supported", http.StatusInternalServerError)
+				return
+			}
+			conn, _, err := hj.Hijack()
+			if err != nil {
+				http.Error(w, "hijack failed", http.StatusInternalServerError)
+				return
+			}
+			conn.Close()
+		}))
+		defer upstream.Close()
+
+		resp, err := DoGitHubGET(context.Background(), upstream.URL, "/path", "token x")
+		assert.Error(t, err)
+		assert.Nil(t, resp)
+	})
+
+	t.Run("returns error on cancelled context", func(t *testing.T) {
+		ctx, cancel := context.WithCancel(context.Background())
+		cancel() // cancel immediately before the request is made
+
+		upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			w.WriteHeader(http.StatusOK)
+		}))
+		defer upstream.Close()
+
+		resp, err := DoGitHubGET(ctx, upstream.URL, "/path", "token x")
+		assert.ErrorIs(t, err, context.Canceled)
+		assert.Nil(t, resp)
+	})
+}
+
+// TestComputeRetryAfter verifies all branches of the retry-delay calculation.
+// The function returns a default 60s for zero or past reset times, applies a
+// 1s safety buffer for future times, and clamps the result to [1, 3600] seconds.
+func TestComputeRetryAfter(t *testing.T) {
+	t.Parallel()
+
+	t.Run("zero time returns default 60s", func(t *testing.T) {
+		t.Parallel()
+		assert.Equal(t, 60, ComputeRetryAfter(time.Time{}))
+	})
+
+	t.Run("time in the past returns default 60s", func(t *testing.T) {
+		t.Parallel()
+		past := time.Now().Add(-5 * time.Minute)
+		assert.Equal(t, 60, ComputeRetryAfter(past))
+	})
+
+	t.Run("future time returns delay with 1s safety buffer", func(t *testing.T) {
+		t.Parallel()
+		// 60s ahead: int(60.0)+1 = 61; allow ±2s for scheduling jitter.
+		future := time.Now().Add(60 * time.Second)
+		got := ComputeRetryAfter(future)
+		assert.InDelta(t, 61, got, 2.0, "expected ~61s for 60s reset with 1s buffer")
+	})
+
+	t.Run("far future is clamped to 3600s maximum", func(t *testing.T) {
+		t.Parallel()
+		assert.Equal(t, 3600, ComputeRetryAfter(time.Now().Add(2*time.Hour)))
+	})
+
+	t.Run("time slightly above max delay is still clamped to 3600s", func(t *testing.T) {
+		t.Parallel()
+		// 3601s ahead: int(3601)+1 = 3602 > 3600 → clamped to 3600.
+		assert.Equal(t, 3600, ComputeRetryAfter(time.Now().Add(3601*time.Second)))
+	})
+}

internal/httputil/collaborator.go → internal/githubhttp/collaborator.go

@@ -1,4 +1,4 @@
-package httputil
+package githubhttp
 
 import (
 	"context"
@@ -11,7 +11,7 @@ import (
 	"github.com/github/gh-aw-mcpg/internal/strutil"
 )
 
-var logCollab = logger.New("httputil:collaborator")
+var logCollab = logger.New("githubhttp:collaborator")
 
 // ParseCollaboratorPermissionArgs extracts and validates the owner, repo, and
 // username fields from an args map for a get_collaborator_permission call.