refactor: semantic function clustering — TLS relocation, util consolidation, and documentation

internal/cmd/proxy.go

@@ -10,29 +10,21 @@ import (
 	"os"
 	"os/signal"
 	"path/filepath"
-	"strings"
 	"syscall"
 
 	"github.com/github/gh-aw-mcpg/internal/config"
 	"github.com/github/gh-aw-mcpg/internal/difc"
 	"github.com/github/gh-aw-mcpg/internal/envutil"
 	"github.com/github/gh-aw-mcpg/internal/githubhttp"
 	"github.com/github/gh-aw-mcpg/internal/guard"
+	"github.com/github/gh-aw-mcpg/internal/httputil"
 	"github.com/github/gh-aw-mcpg/internal/logger"
 	"github.com/github/gh-aw-mcpg/internal/proxy"
 	"github.com/spf13/cobra"
 )
 
 var logProxyCmd = logger.New("cmd:proxy")
 
-var tlsTrustEnvKeys = []string{
-	"NODE_EXTRA_CA_CERTS",
-	"SSL_CERT_FILE",
-	"GIT_SSL_CAINFO",
-	"CURL_CA_BUNDLE",
-	"REQUESTS_CA_BUNDLE",
-}
-
 // Proxy subcommand flag variables
 var (
 	proxyGuardWasm       string
@@ -261,7 +253,7 @@ func runProxy(cmd *cobra.Command, args []string) error {
 		if err != nil {
 			return fmt.Errorf("failed to generate TLS certificates: %w", err)
 		}
-		if err := configureTLSTrustEnvironment(tlsCfg.CACertPath); err != nil {
+		if err := httputil.ConfigureTLSTrustEnvironment(tlsCfg.CACertPath); err != nil {
 			return err
 		}
 		logger.LogInfo("startup", "TLS certificates generated: ca=%s", tlsCfg.CACertPath)
@@ -336,21 +328,6 @@ func runProxy(cmd *cobra.Command, args []string) error {
 	return nil
 }
 
-func configureTLSTrustEnvironment(caCertPath string) error {
-	if strings.ContainsAny(caCertPath, "\r\n") {
-		return fmt.Errorf("invalid TLS CA cert path contains newline")
-	}
-
-	logProxyCmd.Printf("Configuring TLS trust environment: caCertPath=%s, envVars=%v", caCertPath, tlsTrustEnvKeys)
-	for _, key := range tlsTrustEnvKeys {
-		if err := os.Setenv(key, caCertPath); err != nil {
-			return fmt.Errorf("failed to set %s: %w", key, err)
-		}
-	}
-	logProxyCmd.Printf("TLS trust environment configured successfully: %d env vars set", len(tlsTrustEnvKeys))
-	return nil
-}
-
 // clientAddr returns a client-friendly address from a listener address.
 // When the host is a wildcard (0.0.0.0, ::, or empty), it substitutes
 // "localhost" so the printed GH_HOST value is usable from a client.

internal/cmd/proxy_test.go

@@ -440,49 +440,3 @@ func TestClientAddr(t *testing.T) {
 		})
 	}
 }
-
-func TestConfigureTLSTrustEnvironment(t *testing.T) {
-	caPath := "/tmp/proxy-tls/ca.crt"
-
-	t.Run("sets trust environment variables in process", func(t *testing.T) {
-		assert := assert.New(t)
-		t.Setenv("GITHUB_ENV", "")
-		for _, key := range tlsTrustEnvKeys {
-			t.Setenv(key, "")
-		}
-
-		err := configureTLSTrustEnvironment(caPath)
-		require.NoError(t, err)
-
-		for _, key := range tlsTrustEnvKeys {
-			assert.Equal(caPath, os.Getenv(key), "expected %s to be set", key)
-		}
-	})
-
-	t.Run("does not rely on GITHUB_ENV", func(t *testing.T) {
-		assert := assert.New(t)
-		githubEnvFile := t.TempDir() + "/github_env"
-		const original = "UNCHANGED=1\n"
-		require.NoError(t, os.WriteFile(githubEnvFile, []byte(original), 0o644))
-		t.Setenv("GITHUB_ENV", githubEnvFile)
-		for _, key := range tlsTrustEnvKeys {
-			t.Setenv(key, "")
-		}
-
-		require.NoError(t, configureTLSTrustEnvironment(caPath))
-
-		for _, key := range tlsTrustEnvKeys {
-			assert.Equal(caPath, os.Getenv(key), "expected %s to be set", key)
-		}
-
-		content, err := os.ReadFile(githubEnvFile)
-		require.NoError(t, err)
-		assert.Equal(original, string(content))
-	})
-
-	t.Run("rejects CA cert path with newline", func(t *testing.T) {
-		err := configureTLSTrustEnvironment("/tmp/ca.crt\nMALICIOUS=1")
-		require.Error(t, err)
-		assert.ErrorContains(t, err, "contains newline")
-	})
-}

internal/config/config_env.go

@@ -1,5 +1,21 @@
 package config
 
+// config_env.go — gateway-specific environment variable helpers.
+//
+// This file intentionally layers on top of internal/envutil rather than
+// calling os.Getenv directly. The layering is deliberate:
+//
+//   - internal/envutil provides generic, typed environment-variable accessors
+//     (GetEnvString, GetEnvIntRaw, …) with no knowledge of MCP Gateway semantics.
+//
+//   - This file adds gateway-specific validation rules (port ranges, timeout
+//     bounds, feature-flag defaults) on top of those primitives, keeping the
+//     higher-level policy separate from the lower-level reading mechanism.
+//
+// Keep this file focused on shared accessor/validation helpers used by
+// gateway config loading. Feature-specific policy may live in its own package
+// (for example, guard-policy parsing and command flag wiring).
+
 import (
 	"fmt"
 	"time"

internal/httputil/tls.go

@@ -1,14 +1,52 @@
+// Package httputil — generic HTTP helper utilities.
+//
+// TLS helpers are split across two packages:
+//
+//   - internal/httputil (this package): protocol-level helpers that apply to
+//     all TLS listeners and clients (MinTLSVersion, NewServerTLSConfig,
+//     NewClientTLSConfig, ConfigureTLSTrustEnvironment).
+//
+//   - internal/proxy: certificate *generation* (GenerateSelfSignedTLS) lives
+//     there because it is only needed when the proxy runs in self-signed mode.
+//     It calls httputil.NewServerTLSConfig to assemble the final *tls.Config.
 package httputil
 
 import (
 	"crypto/tls"
+	"fmt"
+	"os"
+	"strings"
+
+	"github.com/github/gh-aw-mcpg/internal/logger"
 )
 
+var logTLS = logger.New("httputil:tls")
+
 // MinTLSVersion is the minimum TLS version enforced across all gateway listeners
 // and clients. Centralizing this constant ensures a single point of change if
 // the policy is tightened (e.g., to tls.VersionTLS13).
 const MinTLSVersion = tls.VersionTLS12
 
+// tlsTrustEnvKeys is the canonical set of well-known environment variables
+// that point HTTP/TLS clients (Node.js, curl, git, Python requests) to a CA
+// bundle.
+var tlsTrustEnvKeys = []string{
+	"NODE_EXTRA_CA_CERTS",
+	"SSL_CERT_FILE",
+	"GIT_SSL_CAINFO",
+	"CURL_CA_BUNDLE",
+	"REQUESTS_CA_BUNDLE",
+}
+
+// TLSTrustEnvKeys returns the set of trust-environment variable names used by
+// ConfigureTLSTrustEnvironment.
+//
+// The returned slice is a defensive copy and can be safely modified by callers
+// without affecting package behavior.
+func TLSTrustEnvKeys() []string {
+	return append([]string(nil), tlsTrustEnvKeys...)
+}
+
 // NewServerTLSConfig returns a *tls.Config for TLS server listeners carrying
 // the provided certificate and the gateway-wide minimum TLS version.
 func NewServerTLSConfig(cert tls.Certificate) *tls.Config {
@@ -23,3 +61,22 @@ func NewServerTLSConfig(cert tls.Certificate) *tls.Config {
 func NewClientTLSConfig() *tls.Config {
 	return &tls.Config{MinVersion: MinTLSVersion}
 }
+
+// ConfigureTLSTrustEnvironment sets all trust-environment variables to
+// caCertPath so that
+// child processes (Node.js, curl, git, Python) automatically trust the
+// self-signed CA certificate generated by internal/proxy.GenerateSelfSignedTLS.
+func ConfigureTLSTrustEnvironment(caCertPath string) error {
+	if strings.ContainsAny(caCertPath, "\r\n") {
+		return fmt.Errorf("invalid TLS CA cert path contains newline")
+	}
+
+	logTLS.Printf("Configuring TLS trust environment: caCertPath=%s, envVars=%v", caCertPath, tlsTrustEnvKeys)
+	for _, key := range tlsTrustEnvKeys {
+		if err := os.Setenv(key, caCertPath); err != nil {
+			return fmt.Errorf("failed to set %s: %w", key, err)
+		}
+	}
+	logTLS.Printf("TLS trust environment configured successfully: %d env vars set", len(tlsTrustEnvKeys))
+	return nil
+}

internal/httputil/tls_test.go

@@ -2,9 +2,11 @@ package httputil
 
 import (
 	"crypto/tls"
+	"os"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestNewServerTLSConfig(t *testing.T) {
@@ -25,3 +27,62 @@ func TestNewClientTLSConfig(t *testing.T) {
 	assert.EqualValues(t, MinTLSVersion, cfg.MinVersion)
 	assert.Empty(t, cfg.Certificates)
 }
+
+func TestConfigureTLSTrustEnvironment(t *testing.T) {
+	t.Run("sets all trust env vars to the given path", func(t *testing.T) {
+		// Unset all keys before the test so we start from a clean state.
+		for _, key := range TLSTrustEnvKeys() {
+			t.Setenv(key, "")
+		}
+
+		const caPath = "/tmp/ca.crt"
+		err := ConfigureTLSTrustEnvironment(caPath)
+		require.NoError(t, err)
+
+		for _, key := range TLSTrustEnvKeys() {
+			assert.Equal(t, caPath, os.Getenv(key), "expected %s to be set to %s", key, caPath)
+		}
+	})
+
+	t.Run("does not rely on GITHUB_ENV file writes", func(t *testing.T) {
+		assert := assert.New(t)
+		githubEnvFile := t.TempDir() + "/github_env"
+		const original = "UNCHANGED=1\n"
+		require.NoError(t, os.WriteFile(githubEnvFile, []byte(original), 0o644))
+		t.Setenv("GITHUB_ENV", githubEnvFile)
+		for _, key := range TLSTrustEnvKeys() {
+			t.Setenv(key, "")
+		}
+
+		const caPath = "/tmp/ca.crt"
+		require.NoError(t, ConfigureTLSTrustEnvironment(caPath))
+
+		for _, key := range TLSTrustEnvKeys() {
+			assert.Equal(caPath, os.Getenv(key), "expected %s to be set", key)
+		}
+
+		content, err := os.ReadFile(githubEnvFile)
+		require.NoError(t, err)
+		assert.Equal(original, string(content))
+	})
+
+	t.Run("returns a defensive copy of trust env keys", func(t *testing.T) {
+		keys := TLSTrustEnvKeys()
+		require.NotEmpty(t, keys)
+		originalFirst := keys[0]
+		keys[0] = "MODIFIED_KEY"
+
+		keysAfter := TLSTrustEnvKeys()
+		assert.Equal(t, originalFirst, keysAfter[0])
+	})
+
+	t.Run("rejects path with embedded newline", func(t *testing.T) {
+		err := ConfigureTLSTrustEnvironment("/tmp/ca\n.crt")
+		assert.ErrorContains(t, err, "invalid TLS CA cert path contains newline")
+	})
+
+	t.Run("rejects path with embedded carriage return", func(t *testing.T) {
+		err := ConfigureTLSTrustEnvironment("/tmp/ca\r.crt")
+		assert.ErrorContains(t, err, "invalid TLS CA cert path contains newline")
+	})
+}

internal/logger/fileutil.go

@@ -68,6 +68,9 @@ func initLogFile(logDir, fileName string, flags int) (*os.File, error) {
 // atomicWriteFile writes data to filePath atomically using a temp-file + rename strategy.
 // On rename failure the temp file is removed; a removal error that is not os.IsNotExist
 // is logged as a warning but does not mask the primary rename error.
+//
+// TODO: export as internal/util.AtomicWriteFile (or internal/fileutil) when a
+// second consumer outside the logger package emerges.
 func atomicWriteFile(filePath string, data []byte, perm os.FileMode) error {
 	tempPath := filePath + ".tmp"
 	if err := os.WriteFile(tempPath, data, perm); err != nil {

internal/proxy/tls.go

@@ -8,6 +8,15 @@
 //
 // The CA certificate is written to a file so callers can inject it into their
 // trust store (e.g., via NODE_EXTRA_CA_CERTS or update-ca-certificates).
+//
+// TLS helpers are split across two packages:
+//
+//   - internal/proxy (this file): certificate *generation* (GenerateSelfSignedTLS).
+//     This is the only place self-signed certs are created.
+//
+//   - internal/httputil: protocol-level helpers that apply to all TLS listeners
+//     and clients (MinTLSVersion, NewServerTLSConfig, NewClientTLSConfig,
+//     ConfigureTLSTrustEnvironment).
 package proxy
 
 import (

internal/tracing/http.go

@@ -20,6 +20,12 @@ import (
 // status code. It embeds httputil.BaseResponseWriter which provides WriteHeader,
 // Write (with implicit-200 capture), and Unwrap for transparent interface
 // delegation (e.g. http.Flusher, http.Hijacker).
+//
+// statusResponseWriter is a pure embed — it adds no methods of its own.
+// All status-capture logic lives in httputil.BaseResponseWriter, which is the
+// canonical embed base for package-specific ResponseWriter wrappers across the
+// codebase. See internal/server/response_writer.go for the server-side variant
+// that additionally buffers the response body for debug logging.
 type statusResponseWriter struct {
 	httputil.BaseResponseWriter
 }

internal/util/format_duration.go

@@ -5,8 +5,20 @@ import (
 	"time"
 )
 
-// Time and duration helpers live in the util package because their output is
-// string formatting used by logging and user-visible status messages.
+// String and time/duration formatting helpers live in the util package because
+// their output is used by logging and user-visible status messages throughout
+// the codebase.
+
+// FormatSessionIDForLog returns a log-safe session ID representation.
+// Empty session IDs are rendered as "(none)"; non-empty IDs are truncated to
+// the first 8 bytes with an ellipsis when needed.
+func FormatSessionIDForLog(sessionID string) string {
+	const sessionIDLogMaxLen = 8
+	if sessionID == "" {
+		return "(none)"
+	}
+	return Truncate(sessionID, sessionIDLogMaxLen)
+}
 
 // FormatFutureTime returns a human-readable representation of a future time,
 // combining an RFC3339 timestamp with a relative countdown (e.g. "2026-05-03T12:00:00Z (in 5.0m)").

internal/util/format_duration_test.go

@@ -179,3 +179,23 @@ func TestFormatFutureTime(t *testing.T) {
 		assert.Contains(t, result, " (in ")
 	})
 }
+
+func TestFormatSessionIDForLog(t *testing.T) {
+	tests := []struct {
+		name      string
+		sessionID string
+		expected  string
+	}{
+		{name: "empty session ID returns none", sessionID: "", expected: "(none)"},
+		{name: "short session ID returned as-is", sessionID: "abc123", expected: "abc123"},
+		{name: "exactly 8 chars returned as-is", sessionID: "abcd1234", expected: "abcd1234"},
+		{name: "long session ID truncated", sessionID: "abcdefgh-1234-5678-abcd-ef1234567890", expected: "abcdefgh..."},
+		{name: "unicode is truncated by bytes", sessionID: "session-émojis-🔑", expected: "session-..."},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equal(t, tt.expected, FormatSessionIDForLog(tt.sessionID))
+		})
+	}
+}