refactor: move outlier functions, eliminate thin wrappers, centralize security policy

internal/cmd/proxy.go

@@ -19,6 +19,7 @@ import (
 	"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/github/gh-aw-mcpg/internal/util"
 	"github.com/spf13/cobra"
 )
 
@@ -304,7 +305,7 @@ func runProxy(cmd *cobra.Command, args []string) error {
 			if tlsCfg != nil {
 				fmt.Fprintf(stderr, "  CA cert:   %s\n", tlsCfg.CACertPath)
 				fmt.Fprintf(stderr, "\nConnect with:\n")
-				fmt.Fprintf(stderr, "  export GH_HOST=%s\n", clientAddr(actualAddr))
+				fmt.Fprintf(stderr, "  export GH_HOST=%s\n", util.ClientAddr(actualAddr))
 				fmt.Fprintf(stderr, "  export NODE_EXTRA_CA_CERTS=%s\n", tlsCfg.CACertPath)
 				fmt.Fprintf(stderr, "  export SSL_CERT_FILE=%s\n", tlsCfg.CACertPath)
 				fmt.Fprintf(stderr, "  export GIT_SSL_CAINFO=%s\n", tlsCfg.CACertPath)
@@ -326,25 +327,6 @@ func runProxy(cmd *cobra.Command, args []string) error {
 	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.
-//
-// Note: output.go uses "127.0.0.1" for the same wildcard substitution in
-// the gateway config output, while this function uses "localhost" because
-// GH_HOST must be a resolvable hostname for the gh CLI.
-func clientAddr(addr string) string {
-	host, port, err := net.SplitHostPort(addr)
-	if err != nil {
-		return addr
-	}
-	switch host {
-	case "", "0.0.0.0", "::", "[::]":
-		return net.JoinHostPort("localhost", port)
-	}
-	return addr
-}
-
 // proxyForcePublicReposIfNeeded checks if GITHUB_REPOSITORY is public and, if so,
 // overrides the allow-only policy's repos field to "public". This prevents agents
 // in public-repo workflows from reading private repos through the proxy.

internal/cmd/proxy_test.go

@@ -13,6 +13,7 @@ import (
 	"github.com/stretchr/testify/require"
 
 	"github.com/github/gh-aw-mcpg/internal/config"
+	"github.com/github/gh-aw-mcpg/internal/util"
 )
 
 // TestDetectGuardWasm_FileNotFound tests that detectGuardWasm returns empty string
@@ -527,7 +528,7 @@ func TestClientAddr(t *testing.T) {
 
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
-			assert.Equal(t, tc.expected, clientAddr(tc.input))
+			assert.Equal(t, tc.expected, util.ClientAddr(tc.input))
 		})
 	}
 }

internal/config/error_format.go

@@ -0,0 +1,39 @@
+package config
+
+import (
+	"errors"
+	"strings"
+
+	"github.com/BurntSushi/toml"
+)
+
+// AppendConfigDocsFooter appends standard documentation links to an error message.
+func AppendConfigDocsFooter(sb *strings.Builder) {
+	sb.WriteString("\n\nPlease check your configuration against the MCP Gateway specification at:")
+	sb.WriteString("\n" + ConfigSpecURL)
+	sb.WriteString("\n\nJSON Schema reference:")
+	sb.WriteString("\n" + SchemaURL)
+}
+
+// FormatConfigError returns a rich diagnostic message for TOML parse errors.
+// When err wraps a toml.ParseError, it returns ParseError.ErrorWithUsage() which
+// includes a source-code snippet and column pointer, e.g.:
+//
+//	toml: line 5 (field command): expected "=", got "[" instead
+//
+//	  3 | [servers.github]
+//	  4 | command = "docker"
+//	  5 | [servers.github
+//	      | ^
+//
+// For all other error types, it falls back to err.Error().
+func FormatConfigError(err error) string {
+	if err == nil {
+		return ""
+	}
+	var perr toml.ParseError
+	if errors.As(err, &perr) {
+		return perr.ErrorWithUsage()
+	}
+	return err.Error()
+}

internal/config/validation_errors.go

@@ -1,11 +1,8 @@
 package config
 
 import (
-	"errors"
 	"fmt"
 	"strings"
-
-	"github.com/BurntSushi/toml"
 )
 
 // Documentation URL constants
@@ -85,14 +82,6 @@ func UnsupportedField(fieldName, message, jsonPath, suggestion string) *Validati
 	return InvalidValue(fieldName, message, jsonPath, suggestion)
 }
 
-// AppendConfigDocsFooter appends standard documentation links to an error message
-func AppendConfigDocsFooter(sb *strings.Builder) {
-	sb.WriteString("\n\nPlease check your configuration against the MCP Gateway specification at:")
-	sb.WriteString("\n" + ConfigSpecURL)
-	sb.WriteString("\n\nJSON Schema reference:")
-	sb.WriteString("\n" + SchemaURL)
-}
-
 // InvalidPattern creates a ValidationError for values that don't match a required pattern.
 // Used by validation_schema.go for container, mount, URL, and other pattern validations.
 func InvalidPattern(fieldName, value, jsonPath, suggestion string) *ValidationError {
@@ -128,26 +117,3 @@ func SchemaValidationError(serverType, message, jsonPath, suggestion string) *Va
 		suggestion,
 	)
 }
-
-// FormatConfigError returns a rich diagnostic message for TOML parse errors.
-// When err wraps a toml.ParseError, it returns ParseError.ErrorWithUsage() which
-// includes a source-code snippet and column pointer, e.g.:
-//
-//	toml: line 5 (field command): expected "=", got "[" instead
-//
-//	  3 | [servers.github]
-//	  4 | command = "docker"
-//	  5 | [servers.github
-//	      | ^
-//
-// For all other error types, it falls back to err.Error().
-func FormatConfigError(err error) string {
-	if err == nil {
-		return ""
-	}
-	var perr toml.ParseError
-	if errors.As(err, &perr) {
-		return perr.ErrorWithUsage()
-	}
-	return err.Error()
-}

internal/guard/guard.go

@@ -62,3 +62,11 @@ type AgentLabelsPayload struct {
 // RequestState represents any state that the guard needs to pass from request to response
 // This is useful when the guard needs to carry information from LabelResource to LabelResponse
 type RequestState interface{}
+
+// IsSafeOutputsServer reports whether serverID identifies a safe-outputs server.
+// Safe-outputs servers use write-sink guards and are subject to special DIFC
+// sink-visibility policy (see internal/server/guard_init.go).
+// Matches the canonical "safe-outputs" ID and the legacy "safeoutputs" alias.
+func IsSafeOutputsServer(serverID string) bool {
+	return serverID == "safe-outputs" || serverID == "safeoutputs"
+}

internal/httputil/httputil.go

@@ -6,7 +6,6 @@ import (
 	"encoding/json"
 	"net/http"
 
-	"github.com/github/gh-aw-mcpg/internal/difc"
 	"github.com/github/gh-aw-mcpg/internal/logger"
 )
 
@@ -55,17 +54,3 @@ func IsTransientHTTPError(statusCode int) bool {
 	}
 	return transient
 }
-
-// WriteSimpleHealthResponse writes a minimal {"status":"ok"} JSON health response.
-// This is the standard health response shape for endpoints that do not have access
-// to detailed server status (e.g. the proxy), keeping them consistent with each other.
-func WriteSimpleHealthResponse(w http.ResponseWriter) {
-	WriteJSONResponse(w, http.StatusOK, map[string]string{"status": "ok"})
-}
-
-// WriteReflectResponse writes the DIFC label snapshot JSON response for /reflect endpoints.
-// Both the server and the proxy expose /reflect; sharing this helper ensures the output
-// format evolves consistently as difc.BuildReflectResponse changes.
-func WriteReflectResponse(w http.ResponseWriter, difcComponents difc.DIFCComponents) {
-	WriteJSONResponse(w, http.StatusOK, difc.BuildReflectResponse(difcComponents))
-}

internal/httputil/httputil_test.go

@@ -6,12 +6,9 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"testing"
-	"time"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
-
-	"github.com/github/gh-aw-mcpg/internal/difc"
 )
 
 // errorResponseWriter is a minimal http.ResponseWriter whose Write method
@@ -244,58 +241,3 @@ func TestWriteErrorResponse(t *testing.T) {
 		})
 	}
 }
-
-// TestWriteSimpleHealthResponse verifies that the helper writes a {"status":"ok"} health response.
-func TestWriteSimpleHealthResponse(t *testing.T) {
-	rec := httptest.NewRecorder()
-	WriteSimpleHealthResponse(rec)
-
-	assert.Equal(t, http.StatusOK, rec.Code)
-	assert.Equal(t, "application/json", rec.Header().Get("Content-Type"))
-
-	var body map[string]string
-	require.NoError(t, json.NewDecoder(rec.Body).Decode(&body))
-	assert.Equal(t, "ok", body["status"])
-	assert.Len(t, body, 1, "response body should contain only the 'status' field")
-}
-
-// TestWriteReflectResponse verifies that the helper serialises a DIFC label snapshot correctly.
-func TestWriteReflectResponse(t *testing.T) {
-	t.Run("empty components returns valid response shape", func(t *testing.T) {
-		rec := httptest.NewRecorder()
-		WriteReflectResponse(rec, difc.DIFCComponents{})
-
-		assert.Equal(t, http.StatusOK, rec.Code)
-		assert.Equal(t, "application/json", rec.Header().Get("Content-Type"))
-
-		var body difc.ReflectResponse
-		require.NoError(t, json.NewDecoder(rec.Body).Decode(&body))
-		assert.NotNil(t, body.Agents)
-		assert.NotEmpty(t, body.Timestamp)
-	})
-
-	t.Run("strict mode is reflected in response body", func(t *testing.T) {
-		components, err := difc.NewComponents("strict", difc.EnforcementStrict)
-		require.NoError(t, err)
-
-		rec := httptest.NewRecorder()
-		WriteReflectResponse(rec, components)
-
-		assert.Equal(t, http.StatusOK, rec.Code)
-
-		var body difc.ReflectResponse
-		require.NoError(t, json.NewDecoder(rec.Body).Decode(&body))
-		assert.Equal(t, "strict", body.Mode)
-		assert.NotNil(t, body.Agents)
-	})
-
-	t.Run("timestamp is an RFC3339 string", func(t *testing.T) {
-		rec := httptest.NewRecorder()
-		WriteReflectResponse(rec, difc.DIFCComponents{})
-
-		var body difc.ReflectResponse
-		require.NoError(t, json.NewDecoder(rec.Body).Decode(&body))
-		_, parseErr := time.Parse(time.RFC3339, body.Timestamp)
-		assert.NoError(t, parseErr, "Timestamp should be a valid RFC3339 string")
-	})
-}

internal/mcp/helpers.go

@@ -3,7 +3,6 @@ package mcp
 import (
 	"encoding/json"
 	"fmt"
-	"strings"
 
 	"github.com/github/gh-aw-mcpg/internal/logger"
 )
@@ -66,10 +65,3 @@ func callParamMethod[P any](c *Connection, rawParams interface{}, fn func(P) (in
 	}
 	return marshalToResponse(result)
 }
-
-// IsSingularReadTool returns true when toolName refers to a tool expected to
-// return a single resource (e.g. get_*, *_read). List/search tools are treated
-// as collection tools even if they happen to return one item.
-func IsSingularReadTool(toolName string) bool {
-	return !strings.HasPrefix(toolName, "list_") && !strings.HasPrefix(toolName, "search_")
-}

internal/mcpresult/mcpresult.go

@@ -19,8 +19,18 @@ func IsErrorResult(result map[string]interface{}) bool {
 // maps. It supports both []interface{} values produced by json.Unmarshal and
 // []map[string]interface{} values produced by helper constructors.
 //
-// Non-map items in []interface{} are skipped so callers can decide whether to
-// ignore them or treat them as an error.
+// Semantics: lenient — non-map items in []interface{} are silently skipped.
+// This contrasts with the strict path in internal/mcp/tool_result.go
+// (convertMapToCallToolResult) which returns an error for non-map content items.
+// Use this function in contexts where partial results are acceptable (e.g.
+// DIFC filtering, rate-limit detection); use the strict path when producing
+// SDK-valid CallToolResult values where malformed items must surface as errors.
+//
+// Both normalization paths handle the same wire format variations:
+//   - []interface{}      — produced by json.Unmarshal on the raw backend response
+//   - []map[string]interface{} — produced by helper constructors (e.g. BuildMCPTextResponse)
+//
+// When adding new content type variants, update both paths consistently.
 func NormalizeContentItems(contentVal interface{}) ([]map[string]interface{}, bool) {
 	logMCPResult.Printf("Normalizing MCP content items from type %T", contentVal)
 

internal/proxy/handler.go

@@ -64,13 +64,13 @@ func (h *proxyHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 
 	// Health check endpoint
 	if rawPath == "/health" || rawPath == "/healthz" {
-		httputil.WriteSimpleHealthResponse(w)
+		httputil.WriteJSONResponse(w, http.StatusOK, map[string]string{"status": "ok"})
 		return
 	}
 
 	// Reflect endpoint exposes a live DIFC label snapshot.
 	if r.Method == http.MethodGet && rawPath == "/reflect" {
-		httputil.WriteReflectResponse(w, h.server.DIFCComponents)
+		httputil.WriteJSONResponse(w, http.StatusOK, difc.BuildReflectResponse(h.server.DIFCComponents))
 		return
 	}
 

internal/proxy/tls.go

@@ -36,6 +36,7 @@ import (
 
 	"github.com/github/gh-aw-mcpg/internal/httputil"
 	"github.com/github/gh-aw-mcpg/internal/logger"
+	"github.com/github/gh-aw-mcpg/internal/util"
 )
 
 var logTLS = logger.ForFile()
@@ -183,13 +184,10 @@ func GenerateSelfSignedTLS(dir string) (*TLSConfig, error) {
 }
 
 func randomSerial() (*big.Int, error) {
-	serial, err := rand.Int(rand.Reader, new(big.Int).Lsh(big.NewInt(1), 128))
+	serial, err := util.RandomBigInt(128)
 	if err != nil {
 		return nil, fmt.Errorf("failed to generate serial number: %w", err)
 	}
-	if serial.Sign() == 0 {
-		serial = new(big.Int).Add(serial, big.NewInt(1))
-	}
 	logTLS.Printf("generated random serial number: %s", serial)
 	return serial, nil
 }

internal/server/difc_log_test.go

@@ -10,7 +10,6 @@ import (
 
 	"github.com/github/gh-aw-mcpg/internal/difc"
 	"github.com/github/gh-aw-mcpg/internal/logger"
-	"github.com/github/gh-aw-mcpg/internal/mcp"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -658,34 +657,3 @@ func TestBuildDIFCSingleItemFilteredError_NoReason(t *testing.T) {
 	// No trailing "()" should appear when reason is empty.
 	assert.NotContains(t, err.Error(), "()")
 }
-
-// TestIsSingularReadTool verifies the heuristic that distinguishes singular-read tools
-// (get_*, *_read) from collection tools (list_*, search_*).
-func TestIsSingularReadTool(t *testing.T) {
-	tests := []struct {
-		toolName string
-		singular bool
-	}{
-		{"issue_read", true},
-		{"pull_request_read", true},
-		{"get_issue", true},
-		{"get_pull_request", true},
-		{"get_file_contents", true},
-		{"get_repository", true},
-		{"get_commit", true},
-		{"list_issues", false},
-		{"list_pull_requests", false},
-		{"list_commits", false},
-		{"list_branches", false},
-		{"search_issues", false},
-		{"search_pull_requests", false},
-		{"search_code", false},
-		{"search_repositories", false},
-	}
-	for _, tc := range tests {
-		t.Run(tc.toolName, func(t *testing.T) {
-			assert.Equal(t, tc.singular, mcp.IsSingularReadTool(tc.toolName),
-				"mcp.IsSingularReadTool(%q) should be %v", tc.toolName, tc.singular)
-		})
-	}
-}