From b0b9656294e1611ac84aa0b178ef929d0e0a791f Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Fri, 3 Apr 2026 12:41:12 +0000 Subject: [PATCH] refactor(server): move logRuntimeError from auth.go to http_helpers.go MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit logRuntimeError has no conceptual tie to authentication — its only caller is rejectRequest in http_helpers.go. Moving it co-locates the function with both its caller and the other HTTP utility helpers (writeErrorResponse, rejectRequest, withResponseLogging). Also removes the unused 'log' and 'time' imports from auth.go, and moves TestLogRuntimeError (plus the stringPtr helper) to http_helpers_test.go for consistent test-to-source colocation. Follows the recommendation in #3113 (semantic function clustering analysis). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- internal/server/auth.go | 22 ---------- internal/server/auth_test.go | 62 ---------------------------- internal/server/http_helpers.go | 21 ++++++++++ internal/server/http_helpers_test.go | 58 ++++++++++++++++++++++++++ 4 files changed, 79 insertions(+), 84 deletions(-) diff --git a/internal/server/auth.go b/internal/server/auth.go index 92bd05f94..03e3f0830 100644 --- a/internal/server/auth.go +++ b/internal/server/auth.go @@ -1,35 +1,13 @@ package server import ( - "log" "net/http" - "time" "github.com/github/gh-aw-mcpg/internal/logger" ) var logAuth = logger.New("server:auth") -// logRuntimeError logs runtime errors to stdout per spec section 9.2 -func logRuntimeError(errorType, detail string, r *http.Request, serverName *string) { - logAuth.Printf("Logging runtime error: type=%s, detail=%s", errorType, detail) - - timestamp := time.Now().UTC().Format(time.RFC3339) - requestID := r.Header.Get("X-Request-ID") - if requestID == "" { - requestID = "unknown" - } - - server := "gateway" - if serverName != nil { - server = *serverName - } - - // Spec 9.2: Log to stdout with timestamp, server name, request ID, error details - log.Printf("[ERROR] timestamp=%s server=%s request_id=%s error_type=%s detail=%s path=%s method=%s", - timestamp, server, requestID, errorType, detail, r.URL.Path, r.Method) -} - // isMalformedAuthHeader returns true if the header value contains characters // that are not valid in HTTP header values per RFC 7230: null bytes, control // characters below 0x20 (except horizontal tab 0x09), or DEL (0x7F). diff --git a/internal/server/auth_test.go b/internal/server/auth_test.go index acf5cf2f0..a24a33509 100644 --- a/internal/server/auth_test.go +++ b/internal/server/auth_test.go @@ -292,68 +292,6 @@ func TestAuthMiddleware_ConcurrentRequests(t *testing.T) { } } -// TestLogRuntimeError tests the logRuntimeError function -func TestLogRuntimeError(t *testing.T) { - tests := []struct { - name string - errorType string - detail string - requestID string - serverName *string - path string - method string - }{ - { - name: "BasicError", - errorType: "authentication_failed", - detail: "missing_auth_header", - requestID: "req-123", - serverName: nil, - path: "/api/test", - method: "GET", - }, - { - name: "ErrorWithServerName", - errorType: "backend_error", - detail: "connection_failed", - requestID: "req-456", - serverName: stringPtr("test-server"), - path: "/mcp/test", - method: "POST", - }, - { - name: "ErrorWithoutRequestID", - errorType: "validation_error", - detail: "invalid_input", - requestID: "", - serverName: nil, - path: "/health", - method: "GET", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - // Create a test request - req := httptest.NewRequest(tt.method, tt.path, nil) - if tt.requestID != "" { - req.Header.Set("X-Request-ID", tt.requestID) - } - - // Call logRuntimeError (it logs to stdout via log.Printf) - // We can't easily capture the log output in tests, but we can verify it doesn't panic - assert.NotPanics(t, func() { - logRuntimeError(tt.errorType, tt.detail, req, tt.serverName) - }, "logRuntimeError should not panic") - }) - } -} - -// Helper function to create string pointer -func stringPtr(s string) *string { - return &s -} - // TestIsMalformedAuthHeader tests the isMalformedAuthHeader helper. func TestIsMalformedAuthHeader(t *testing.T) { tests := []struct { diff --git a/internal/server/http_helpers.go b/internal/server/http_helpers.go index b77875b2c..b919520b0 100644 --- a/internal/server/http_helpers.go +++ b/internal/server/http_helpers.go @@ -6,6 +6,7 @@ import ( "io" "log" "net/http" + "time" "github.com/github/gh-aw-mcpg/internal/auth" "github.com/github/gh-aw-mcpg/internal/guard" @@ -17,6 +18,26 @@ import ( var logHelpers = logger.New("server:helpers") +// logRuntimeError logs runtime errors to stdout per spec section 9.2 +func logRuntimeError(errorType, detail string, r *http.Request, serverName *string) { + logHelpers.Printf("Logging runtime error: type=%s, detail=%s", errorType, detail) + + timestamp := time.Now().UTC().Format(time.RFC3339) + requestID := r.Header.Get("X-Request-ID") + if requestID == "" { + requestID = "unknown" + } + + server := "gateway" + if serverName != nil { + server = *serverName + } + + // Spec 9.2: Log to stdout with timestamp, server name, request ID, error details + log.Printf("[ERROR] timestamp=%s server=%s request_id=%s error_type=%s detail=%s path=%s method=%s", + timestamp, server, requestID, errorType, detail, r.URL.Path, r.Method) +} + // writeErrorResponse writes a JSON error response with a consistent shape. // All HTTP error paths in the server package should use this helper to ensure // clients always receive application/json rather than text/plain. diff --git a/internal/server/http_helpers_test.go b/internal/server/http_helpers_test.go index 037111552..ceb561bd0 100644 --- a/internal/server/http_helpers_test.go +++ b/internal/server/http_helpers_test.go @@ -672,3 +672,61 @@ func TestWrapWithMiddleware_LogTagVariations(t *testing.T) { }) } } + +// TestLogRuntimeError tests the logRuntimeError function. +func TestLogRuntimeError(t *testing.T) { + tests := []struct { + name string + errorType string + detail string + requestID string + serverName *string + path string + method string + }{ + { + name: "BasicError", + errorType: "authentication_failed", + detail: "missing_auth_header", + requestID: "req-123", + serverName: nil, + path: "/api/test", + method: "GET", + }, + { + name: "ErrorWithServerName", + errorType: "backend_error", + detail: "connection_failed", + requestID: "req-456", + serverName: stringPtr("test-server"), + path: "/mcp/test", + method: "POST", + }, + { + name: "ErrorWithoutRequestID", + errorType: "validation_error", + detail: "invalid_input", + requestID: "", + serverName: nil, + path: "/health", + method: "GET", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + req := httptest.NewRequest(tt.method, tt.path, nil) + if tt.requestID != "" { + req.Header.Set("X-Request-ID", tt.requestID) + } + assert.NotPanics(t, func() { + logRuntimeError(tt.errorType, tt.detail, req, tt.serverName) + }, "logRuntimeError should not panic") + }) + } +} + +// stringPtr returns a pointer to s. Used as a helper in tests. +func stringPtr(s string) *string { + return &s +}