Skip to content

[Repo Assist] refactor(server): move logRuntimeError from auth.go to http_helpers.go #3119

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lpcox merged 1 commit into from
Apr 4, 2026
Merged

[Repo Assist] refactor(server): move logRuntimeError from auth.go to http_helpers.go #3119

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 0 additions & 22 deletions internal/server/auth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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).
Expand Down
62 changes: 0 additions & 62 deletions internal/server/auth_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
21 changes: 21 additions & 0 deletions internal/server/http_helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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)
Comment on lines +36 to +38
Copy link

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logRuntimeError claims to log "to stdout per spec", but it uses the package-level standard logger (log.Printf). By default, the standard logger writes to stderr and also prepends its own timestamp/prefix unless log.SetFlags(0) is called, which can make the emitted line diverge from the intended spec format. Consider using a dedicated logger configured with os.Stdout and flags=0 (or update the comment if stdout/format is not required).

Copilot uses AI. Check for mistakes.
}

// 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.
Expand Down
58 changes: 58 additions & 0 deletions internal/server/http_helpers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Loading