diff --git a/internal/config/env_validation.go b/internal/config/validation_env.go similarity index 100% rename from internal/config/env_validation.go rename to internal/config/validation_env.go diff --git a/internal/config/env_validation_test.go b/internal/config/validation_env_test.go similarity index 100% rename from internal/config/env_validation_test.go rename to internal/config/validation_env_test.go diff --git a/internal/config/schema_validation.go b/internal/config/validation_schema.go similarity index 100% rename from internal/config/schema_validation.go rename to internal/config/validation_schema.go diff --git a/internal/config/schema_validation_test.go b/internal/config/validation_schema_test.go similarity index 100% rename from internal/config/schema_validation_test.go rename to internal/config/validation_schema_test.go diff --git a/internal/logger/error_formatting.go b/internal/logger/error_formatting.go deleted file mode 100644 index 6ba2d550..00000000 --- a/internal/logger/error_formatting.go +++ /dev/null @@ -1,47 +0,0 @@ -package logger - -import ( - "regexp" - "strings" -) - -// Pre-compiled regexes for performance (avoid recompiling in hot paths). -var ( - // Timestamp patterns for log cleanup - // Pattern 1: ISO 8601 with T or space separator (e.g., "2024-01-01T12:00:00.123Z " or "2024-01-01 12:00:00 "). - timestampPattern1 = regexp.MustCompile(`^\d{4}-\d{2}-\d{2}[T\s]\d{2}:\d{2}:\d{2}(\.\d+)?([+-]\d{2}:\d{2}|Z)?\s*`) - // Pattern 2: Bracketed date-time (e.g., "[2024-01-01 12:00:00] "). - timestampPattern2 = regexp.MustCompile(`^\[\d{4}-\d{2}-\d{2}\s+\d{2}:\d{2}:\d{2}\]\s*`) - // Pattern 3: Bracketed time only (e.g., "[12:00:00] "). - timestampPattern3 = regexp.MustCompile(`^\[\d{2}:\d{2}:\d{2}\]\s+`) - // Pattern 4: Time only with optional milliseconds (e.g., "12:00:00.123 "). - timestampPattern4 = regexp.MustCompile(`^\d{2}:\d{2}:\d{2}(\.\d+)?\s+`) - - // Log level pattern for message cleanup (case-insensitive). - logLevelPattern = regexp.MustCompile(`(?i)^\[?(ERROR|WARNING|WARN|INFO|DEBUG)\]?\s*[:-]?\s*`) -) - -// ExtractErrorMessage extracts a clean error message from a log line. -// It removes timestamps, log level prefixes, and other common noise. -// If the message is longer than 200 characters, it will be truncated. -func ExtractErrorMessage(line string) string { - // Remove common timestamp patterns using pre-compiled regexes - cleanedLine := line - cleanedLine = timestampPattern1.ReplaceAllString(cleanedLine, "") - cleanedLine = timestampPattern2.ReplaceAllString(cleanedLine, "") - cleanedLine = timestampPattern3.ReplaceAllString(cleanedLine, "") - cleanedLine = timestampPattern4.ReplaceAllString(cleanedLine, "") - - // Remove common log level prefixes using pre-compiled regex - cleanedLine = logLevelPattern.ReplaceAllString(cleanedLine, "") - - // Trim whitespace - cleanedLine = strings.TrimSpace(cleanedLine) - - // If the line is too long (>200 chars), truncate it - if len(cleanedLine) > 200 { - cleanedLine = cleanedLine[:197] + "..." - } - - return cleanedLine -} diff --git a/internal/logger/global_helpers.go b/internal/logger/global_helpers.go index fcb73dc5..4595daac 100644 --- a/internal/logger/global_helpers.go +++ b/internal/logger/global_helpers.go @@ -1,6 +1,6 @@ // Package logger provides structured logging for the MCP Gateway. // -// This file contains helper functions for managing global logger state with proper +// This file contains generic helper functions for managing global logger state with proper // mutex handling. These helpers encapsulate common patterns for initializing and // closing global loggers (FileLogger, JSONLLogger, MarkdownLogger) to reduce code // duplication while maintaining thread safety. @@ -14,84 +14,102 @@ // directly by external code. Use the public Init* and Close* functions instead. package logger -// This file contains helper functions that encapsulate the common patterns -// for global logger initialization and cleanup. These helpers reduce code -// duplication while maintaining type safety. +import "sync" -// initGlobalFileLogger is a helper that encapsulates the common pattern for -// initializing a global FileLogger with proper mutex handling. -func initGlobalFileLogger(logger *FileLogger) { - globalLoggerMu.Lock() - defer globalLoggerMu.Unlock() +// closableLogger is a constraint for types that have a Close method. +// This is satisfied by *FileLogger, *JSONLLogger, and *MarkdownLogger. +type closableLogger interface { + *FileLogger | *JSONLLogger | *MarkdownLogger + Close() error +} + +// initGlobalLogger is a generic helper that encapsulates the common pattern for +// initializing a global logger with proper mutex handling. +// +// Type parameters: +// - T: Any pointer type that satisfies closableLogger constraint +// +// Parameters: +// - mu: Mutex to protect access to the global logger +// - current: Pointer to the current global logger instance +// - newLogger: New logger instance to set as the global logger +// +// This function: +// 1. Acquires the mutex lock +// 2. Closes any existing logger if present +// 3. Sets the new logger as the global instance +// 4. Releases the mutex lock +func initGlobalLogger[T closableLogger](mu *sync.RWMutex, current *T, newLogger T) { + mu.Lock() + defer mu.Unlock() - if globalFileLogger != nil { - globalFileLogger.Close() + if *current != nil { + (*current).Close() } - globalFileLogger = logger + *current = newLogger } -// closeGlobalFileLogger is a helper that encapsulates the common pattern for -// closing and clearing a global FileLogger with proper mutex handling. -func closeGlobalFileLogger() error { - globalLoggerMu.Lock() - defer globalLoggerMu.Unlock() +// closeGlobalLogger is a generic helper that encapsulates the common pattern for +// closing and clearing a global logger with proper mutex handling. +// +// Type parameters: +// - T: Any pointer type that satisfies closableLogger constraint +// +// Parameters: +// - mu: Mutex to protect access to the global logger +// - logger: Pointer to the global logger instance to close +// +// Returns: +// - error: Any error returned by the logger's Close() method +// +// This function: +// 1. Acquires the mutex lock +// 2. Closes the logger if it exists +// 3. Sets the logger pointer to nil +// 4. Releases the mutex lock +// 5. Returns any error from the Close() operation +func closeGlobalLogger[T closableLogger](mu *sync.RWMutex, logger *T) error { + mu.Lock() + defer mu.Unlock() - if globalFileLogger != nil { - err := globalFileLogger.Close() - globalFileLogger = nil + if *logger != nil { + err := (*logger).Close() + var zero T + *logger = zero return err } return nil } -// initGlobalJSONLLogger is a helper that encapsulates the common pattern for -// initializing a global JSONLLogger with proper mutex handling. -func initGlobalJSONLLogger(logger *JSONLLogger) { - globalJSONLMu.Lock() - defer globalJSONLMu.Unlock() +// Type-specific helper functions that use the generic implementations above. +// These maintain backward compatibility with existing code. - if globalJSONLLogger != nil { - globalJSONLLogger.Close() - } - globalJSONLLogger = logger +// initGlobalFileLogger initializes the global FileLogger using the generic helper. +func initGlobalFileLogger(logger *FileLogger) { + initGlobalLogger(&globalLoggerMu, &globalFileLogger, logger) } -// closeGlobalJSONLLogger is a helper that encapsulates the common pattern for -// closing and clearing a global JSONLLogger with proper mutex handling. -func closeGlobalJSONLLogger() error { - globalJSONLMu.Lock() - defer globalJSONLMu.Unlock() +// closeGlobalFileLogger closes the global FileLogger using the generic helper. +func closeGlobalFileLogger() error { + return closeGlobalLogger(&globalLoggerMu, &globalFileLogger) +} - if globalJSONLLogger != nil { - err := globalJSONLLogger.Close() - globalJSONLLogger = nil - return err - } - return nil +// initGlobalJSONLLogger initializes the global JSONLLogger using the generic helper. +func initGlobalJSONLLogger(logger *JSONLLogger) { + initGlobalLogger(&globalJSONLMu, &globalJSONLLogger, logger) } -// initGlobalMarkdownLogger is a helper that encapsulates the common pattern for -// initializing a global MarkdownLogger with proper mutex handling. -func initGlobalMarkdownLogger(logger *MarkdownLogger) { - globalMarkdownMu.Lock() - defer globalMarkdownMu.Unlock() +// closeGlobalJSONLLogger closes the global JSONLLogger using the generic helper. +func closeGlobalJSONLLogger() error { + return closeGlobalLogger(&globalJSONLMu, &globalJSONLLogger) +} - if globalMarkdownLogger != nil { - globalMarkdownLogger.Close() - } - globalMarkdownLogger = logger +// initGlobalMarkdownLogger initializes the global MarkdownLogger using the generic helper. +func initGlobalMarkdownLogger(logger *MarkdownLogger) { + initGlobalLogger(&globalMarkdownMu, &globalMarkdownLogger, logger) } -// closeGlobalMarkdownLogger is a helper that encapsulates the common pattern for -// closing and clearing a global MarkdownLogger with proper mutex handling. +// closeGlobalMarkdownLogger closes the global MarkdownLogger using the generic helper. func closeGlobalMarkdownLogger() error { - globalMarkdownMu.Lock() - defer globalMarkdownMu.Unlock() - - if globalMarkdownLogger != nil { - err := globalMarkdownLogger.Close() - globalMarkdownLogger = nil - return err - } - return nil + return closeGlobalLogger(&globalMarkdownMu, &globalMarkdownLogger) } diff --git a/internal/logger/rpc_helpers.go b/internal/logger/rpc_helpers.go index 7df77cff..91b6e650 100644 --- a/internal/logger/rpc_helpers.go +++ b/internal/logger/rpc_helpers.go @@ -8,6 +8,7 @@ // - extractEssentialFields: Extracts key JSON-RPC fields for compact logging // - getMapKeys: Utility for extracting map keys without values // - isEffectivelyEmpty: Checks if data is effectively empty (e.g., only params: null) +// - ExtractErrorMessage: Extracts clean error messages from log lines // // These helpers are used by the RPC logging system to safely and efficiently // process message payloads before logging them. @@ -15,10 +16,28 @@ package logger import ( "encoding/json" + "regexp" + "strings" "github.com/githubnext/gh-aw-mcpg/internal/logger/sanitize" ) +// Pre-compiled regexes for performance (avoid recompiling in hot paths). +var ( + // Timestamp patterns for log cleanup + // Pattern 1: ISO 8601 with T or space separator (e.g., "2024-01-01T12:00:00.123Z " or "2024-01-01 12:00:00 "). + timestampPattern1 = regexp.MustCompile(`^\d{4}-\d{2}-\d{2}[T\s]\d{2}:\d{2}:\d{2}(\.\d+)?([+-]\d{2}:\d{2}|Z)?\s*`) + // Pattern 2: Bracketed date-time (e.g., "[2024-01-01 12:00:00] "). + timestampPattern2 = regexp.MustCompile(`^\[\d{4}-\d{2}-\d{2}\s+\d{2}:\d{2}:\d{2}\]\s*`) + // Pattern 3: Bracketed time only (e.g., "[12:00:00] "). + timestampPattern3 = regexp.MustCompile(`^\[\d{2}:\d{2}:\d{2}\]\s+`) + // Pattern 4: Time only with optional milliseconds (e.g., "12:00:00.123 "). + timestampPattern4 = regexp.MustCompile(`^\d{2}:\d{2}:\d{2}(\.\d+)?\s+`) + + // Log level pattern for message cleanup (case-insensitive). + logLevelPattern = regexp.MustCompile(`(?i)^\[?(ERROR|WARNING|WARN|INFO|DEBUG)\]?\s*[:-]?\s*`) +) + // truncateAndSanitize truncates the payload to max length and sanitizes secrets func truncateAndSanitize(payload string, maxLength int) string { // First sanitize secrets @@ -93,3 +112,28 @@ func isEffectivelyEmpty(data map[string]interface{}) bool { return false } + +// ExtractErrorMessage extracts a clean error message from a log line. +// It removes timestamps, log level prefixes, and other common noise. +// If the message is longer than 200 characters, it will be truncated. +func ExtractErrorMessage(line string) string { + // Remove common timestamp patterns using pre-compiled regexes + cleanedLine := line + cleanedLine = timestampPattern1.ReplaceAllString(cleanedLine, "") + cleanedLine = timestampPattern2.ReplaceAllString(cleanedLine, "") + cleanedLine = timestampPattern3.ReplaceAllString(cleanedLine, "") + cleanedLine = timestampPattern4.ReplaceAllString(cleanedLine, "") + + // Remove common log level prefixes using pre-compiled regex + cleanedLine = logLevelPattern.ReplaceAllString(cleanedLine, "") + + // Trim whitespace + cleanedLine = strings.TrimSpace(cleanedLine) + + // If the line is too long (>200 chars), truncate it + if len(cleanedLine) > 200 { + cleanedLine = cleanedLine[:197] + "..." + } + + return cleanedLine +} diff --git a/internal/logger/error_formatting_test.go b/internal/logger/rpc_helpers_test.go similarity index 100% rename from internal/logger/error_formatting_test.go rename to internal/logger/rpc_helpers_test.go diff --git a/internal/mcp/schema_normalizer.go b/internal/mcp/schema_normalizer.go deleted file mode 100644 index 261914fb..00000000 --- a/internal/mcp/schema_normalizer.go +++ /dev/null @@ -1,59 +0,0 @@ -package mcp - -import ( - "github.com/githubnext/gh-aw-mcpg/internal/logger" -) - -// NormalizeInputSchema fixes common schema validation issues in tool definitions -// that can cause downstream validation errors. -// -// Known issues fixed: -// 1. Missing schema: When a backend returns no inputSchema (nil), we provide -// a default empty object schema that accepts any properties. This is required -// by the MCP SDK's Server.AddTool method. -// 2. Object schemas without properties: When a schema declares "type": "object" -// but is missing the required "properties" field, we add an empty properties -// object to make it valid per JSON Schema standards. -func NormalizeInputSchema(schema map[string]interface{}, toolName string) map[string]interface{} { - // If backend didn't provide a schema, use a default empty object schema - // This allows the tool to be registered and clients will see it accepts any parameters - if schema == nil { - logger.LogWarn("backend", "Tool schema normalized: %s - backend provided no inputSchema, using default empty object schema", toolName) - return map[string]interface{}{ - "type": "object", - "properties": map[string]interface{}{}, - } - } - - // Check if this is an object type schema - typeVal, hasType := schema["type"] - if !hasType { - return schema - } - - typeStr, isString := typeVal.(string) - if !isString || typeStr != "object" { - return schema - } - - // Check if properties field exists - _, hasProperties := schema["properties"] - _, hasAdditionalProperties := schema["additionalProperties"] - - // If it's an object type but missing both properties and additionalProperties, - // add an empty properties object to make it valid - if !hasProperties && !hasAdditionalProperties { - logger.LogWarn("backend", "Tool schema normalized: %s - added empty properties to object type schema", toolName) - - // Create a copy of the schema to avoid modifying the original - normalized := make(map[string]interface{}) - for k, v := range schema { - normalized[k] = v - } - normalized["properties"] = map[string]interface{}{} - - return normalized - } - - return schema -} diff --git a/internal/mcp/types.go b/internal/mcp/types.go index b283e0c9..36f0b95f 100644 --- a/internal/mcp/types.go +++ b/internal/mcp/types.go @@ -1,6 +1,10 @@ package mcp -import "encoding/json" +import ( + "encoding/json" + + "github.com/githubnext/gh-aw-mcpg/internal/logger" +) // Request represents a JSON-RPC 2.0 request type Request struct { @@ -43,3 +47,57 @@ type ContentItem struct { Type string `json:"type"` Text string `json:"text,omitempty"` } + +// NormalizeInputSchema fixes common schema validation issues in tool definitions +// that can cause downstream validation errors. +// +// Known issues fixed: +// 1. Missing schema: When a backend returns no inputSchema (nil), we provide +// a default empty object schema that accepts any properties. This is required +// by the MCP SDK's Server.AddTool method. +// 2. Object schemas without properties: When a schema declares "type": "object" +// but is missing the required "properties" field, we add an empty properties +// object to make it valid per JSON Schema standards. +func NormalizeInputSchema(schema map[string]interface{}, toolName string) map[string]interface{} { + // If backend didn't provide a schema, use a default empty object schema + // This allows the tool to be registered and clients will see it accepts any parameters + if schema == nil { + logger.LogWarn("backend", "Tool schema normalized: %s - backend provided no inputSchema, using default empty object schema", toolName) + return map[string]interface{}{ + "type": "object", + "properties": map[string]interface{}{}, + } + } + + // Check if this is an object type schema + typeVal, hasType := schema["type"] + if !hasType { + return schema + } + + typeStr, isString := typeVal.(string) + if !isString || typeStr != "object" { + return schema + } + + // Check if properties field exists + _, hasProperties := schema["properties"] + _, hasAdditionalProperties := schema["additionalProperties"] + + // If it's an object type but missing both properties and additionalProperties, + // add an empty properties object to make it valid + if !hasProperties && !hasAdditionalProperties { + logger.LogWarn("backend", "Tool schema normalized: %s - added empty properties to object type schema", toolName) + + // Create a copy of the schema to avoid modifying the original + normalized := make(map[string]interface{}) + for k, v := range schema { + normalized[k] = v + } + normalized["properties"] = map[string]interface{}{} + + return normalized + } + + return schema +} diff --git a/internal/mcp/schema_normalizer_test.go b/internal/mcp/types_test.go similarity index 100% rename from internal/mcp/schema_normalizer_test.go rename to internal/mcp/types_test.go