diff --git a/AGENTS.md b/AGENTS.md index 2ee11972b..0766d9357 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -295,6 +295,8 @@ if log.Enabled() { import "github.com/github/gh-aw-mcpg/internal/logger" // Log operational events (written to mcp-gateway.log) +// Un-suffixed Log* functions target the unified file logger sink. +// Use Log*ToMarkdown or Log*ToServer when you need those explicit destinations. logger.LogInfo("category", "Operation completed successfully") logger.LogWarn("category", "Potential issue detected: %s", issue) logger.LogError("category", "Operation failed: %v", err) diff --git a/internal/config/config_env.go b/internal/config/config_env.go index f4a50a488..e92edd7c5 100644 --- a/internal/config/config_env.go +++ b/internal/config/config_env.go @@ -2,7 +2,6 @@ package config import ( "fmt" - "strconv" "time" "github.com/github/gh-aw-mcpg/internal/config/rules" @@ -13,16 +12,15 @@ import ( const ToolTimeoutMin = 10 func parseAndValidateIntEnv(envKey string, validate func(int) *rules.ValidationError) (int, bool, error) { - raw := envutil.GetEnvString(envKey, "") - if raw == "" { + value, present, err := envutil.GetEnvIntRaw(envKey) + if !present { logConfig.Printf("%s not set in environment", envKey) return 0, false, nil } - value, err := strconv.Atoi(raw) if err != nil { - logConfig.Printf("%s=%q is not a valid integer: %v", envKey, raw, err) - return 0, false, fmt.Errorf("invalid %s value: %s", envKey, raw) + logConfig.Printf("%s is not a valid integer: %v", envKey, err) + return 0, false, fmt.Errorf("invalid %s value", envKey) } if validationErr := validate(value); validationErr != nil { diff --git a/internal/envutil/envutil.go b/internal/envutil/envutil.go index 9a85dedd1..b86beb6cd 100644 --- a/internal/envutil/envutil.go +++ b/internal/envutil/envutil.go @@ -28,17 +28,36 @@ func GetEnvString(envKey, defaultValue string) string { return defaultValue } +// GetEnvIntRaw returns the raw integer value of envKey without applying defaults +// or positivity constraints. +// It returns (0, false, nil) when envKey is unset or empty. +// It returns (0, true, err) when envKey is set but cannot be parsed as an integer. +func GetEnvIntRaw(envKey string) (int, bool, error) { + envValue := os.Getenv(envKey) + if envValue == "" { + return 0, false, nil + } + + value, err := strconv.Atoi(envValue) + if err != nil { + return 0, true, err + } + return value, true, nil +} + // GetEnvInt returns the integer value of the environment variable specified by envKey. // If the environment variable is not set, is empty, cannot be parsed as an integer, // or is not positive (> 0), it returns the defaultValue. // This function validates that the value is a positive integer. func GetEnvInt(envKey string, defaultValue int) int { - if envValue := os.Getenv(envKey); envValue != "" { - if value, err := strconv.Atoi(envValue); err == nil && value > 0 { - return value - } - logEnvUtil.Printf("GetEnvInt: %s=%q is not a valid positive integer, using default=%d", envKey, sanitize.TruncateSecret(envValue), defaultValue) + value, ok, err := GetEnvIntRaw(envKey) + if !ok { + return defaultValue + } + if err == nil && value > 0 { + return value } + logEnvUtil.Printf("GetEnvInt: %s=%q is not a valid positive integer, using default=%d", envKey, sanitize.TruncateSecret(os.Getenv(envKey)), defaultValue) return defaultValue } diff --git a/internal/envutil/envutil_test.go b/internal/envutil/envutil_test.go index 4131eb529..5e4cbd7dd 100644 --- a/internal/envutil/envutil_test.go +++ b/internal/envutil/envutil_test.go @@ -289,6 +289,78 @@ func TestGetEnvInt(t *testing.T) { } } +func TestGetEnvIntRaw(t *testing.T) { + tests := []struct { + name string + envKey string + envValue string + setEnv bool + wantValue int + wantOK bool + wantErr bool + }{ + { + name: "valid integer", + envKey: "TEST_INT_RAW_VAR", + envValue: "42", + setEnv: true, + wantValue: 42, + wantOK: true, + wantErr: false, + }, + { + name: "not set", + envKey: "TEST_INT_RAW_VAR", + setEnv: false, + wantOK: false, + wantErr: false, + }, + { + name: "empty", + envKey: "TEST_INT_RAW_VAR", + envValue: "", + setEnv: true, + wantOK: false, + wantErr: false, + }, + { + name: "invalid integer", + envKey: "TEST_INT_RAW_VAR", + envValue: "not-a-number", + setEnv: true, + wantOK: true, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.setEnv { + t.Setenv(tt.envKey, tt.envValue) + } else { + previousValue, hadPreviousValue := os.LookupEnv(tt.envKey) + t.Cleanup(func() { + if hadPreviousValue { + _ = os.Setenv(tt.envKey, previousValue) + } else { + _ = os.Unsetenv(tt.envKey) + } + }) + _ = os.Unsetenv(tt.envKey) + } + + got, ok, err := GetEnvIntRaw(tt.envKey) + assert.Equal(t, tt.wantValue, got) + assert.Equal(t, tt.wantOK, ok) + if tt.wantErr { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) + } +} + func TestGetEnvBool(t *testing.T) { tests := []struct { name string diff --git a/internal/logger/file_logger.go b/internal/logger/file_logger.go index 2ddc6d01e..9bcfff06b 100644 --- a/internal/logger/file_logger.go +++ b/internal/logger/file_logger.go @@ -131,22 +131,30 @@ var ( logDebug = makeLevelLogger(logWithLevel, LogLevelDebug) ) -// LogInfo logs an informational message. +// LogInfo logs an informational message to the unified file logger sink. +// The underlying filename depends on logger initialization. For +// destination-specific logging use LogInfoToMarkdown or LogInfoToServer. func LogInfo(category, format string, args ...interface{}) { logInfo(category, format, args...) } -// LogWarn logs a warning message. +// LogWarn logs a warning message to the unified file logger sink. +// The underlying filename depends on logger initialization. For +// destination-specific logging use LogWarnToMarkdown or LogWarnToServer. func LogWarn(category, format string, args ...interface{}) { logWarn(category, format, args...) } -// LogError logs an error message. +// LogError logs an error message to the unified file logger sink. +// The underlying filename depends on logger initialization. For +// destination-specific logging use LogErrorToMarkdown or LogErrorToServer. func LogError(category, format string, args ...interface{}) { logError(category, format, args...) } -// LogDebug logs a debug message. +// LogDebug logs a debug message to the unified file logger sink. +// The underlying filename depends on logger initialization. For +// destination-specific logging use LogDebugToMarkdown or LogDebugToServer. func LogDebug(category, format string, args ...interface{}) { logDebug(category, format, args...) } diff --git a/internal/server/difc_log.go b/internal/server/difc_log.go index 697c11ec9..bb3150838 100644 --- a/internal/server/difc_log.go +++ b/internal/server/difc_log.go @@ -61,6 +61,8 @@ func buildFilteredItemLogEntry(serverID, toolName string, detail difc.FilteredIt // getFilteredItemStringField returns the first non-empty string value from the map // matching any of the given field names. +// NOTE: This helper is intentionally generic; if similar untyped JSON map extraction +// patterns appear elsewhere, consider moving it to a shared utility package. func getFilteredItemStringField(m map[string]interface{}, fields ...string) string { for _, f := range fields { if v, ok := m[f]; ok {