Skip to content

Commit 0f16742

Browse files
authored
Refactor env int parsing and clarify logger sink intent (#6019)
The semantic clustering review found three refactor opportunities: ambiguous logger sink naming, duplicated int env parsing logic, and a generic JSON extraction helper hidden in DIFC logging code. This PR takes the low-risk path: consolidate parsing behavior and improve discoverability/intent via docs and comments without API churn. - **Env parsing consolidation** - Added `envutil.GetEnvIntRaw(envKey) (int, bool, error)` to centralize raw integer env parsing (present/not-present/parse-error semantics). - Refactored `config.parseAndValidateIntEnv` to delegate parsing to `GetEnvIntRaw`, keeping domain validation in `config`. - Updated `GetEnvInt` to reuse `GetEnvIntRaw` internally, removing duplicate parse logic. - **Logger sink clarity (no rename)** - Clarified godoc on `LogInfo/LogWarn/LogError/LogDebug` that unsuffixed functions target the unified file sink (`mcp-gateway.log`). - Added matching guidance in `AGENTS.md` to direct destination-specific usage toward `Log*ToMarkdown` / `Log*ToServer`. - **DIFC helper discoverability** - Added a targeted note on `getFilteredItemStringField` in `internal/server/difc_log.go` indicating it is intentionally generic and a candidate for future extraction if reuse grows. - **Focused coverage update** - Added tests for the new `GetEnvIntRaw` behavior in `internal/envutil/envutil_test.go`. ```go // config now delegates parse to envutil, then applies domain validation. value, present, err := envutil.GetEnvIntRaw(envKey) if !present { return 0, false, nil } if err != nil { return 0, false, fmt.Errorf("invalid %s value", envKey) } if validationErr := validate(value); validationErr != nil { return 0, false, fmt.Errorf("%s", validationErr.Message) } ```
2 parents dad5178 + 2f1769a commit 0f16742

6 files changed

Lines changed: 116 additions & 15 deletions

File tree

AGENTS.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -297,6 +297,8 @@ if log.Enabled() {
297297
import "github.com/github/gh-aw-mcpg/internal/logger"
298298

299299
// Log operational events (written to mcp-gateway.log)
300+
// Un-suffixed Log* functions target the unified file logger sink.
301+
// Use Log*ToMarkdown or Log*ToServer when you need those explicit destinations.
300302
logger.LogInfo("category", "Operation completed successfully")
301303
logger.LogWarn("category", "Potential issue detected: %s", issue)
302304
logger.LogError("category", "Operation failed: %v", err)

internal/config/config_env.go

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ package config
22

33
import (
44
"fmt"
5-
"strconv"
65
"time"
76

87
"github.com/github/gh-aw-mcpg/internal/config/rules"
@@ -13,16 +12,15 @@ import (
1312
const ToolTimeoutMin = 10
1413

1514
func parseAndValidateIntEnv(envKey string, validate func(int) *rules.ValidationError) (int, bool, error) {
16-
raw := envutil.GetEnvString(envKey, "")
17-
if raw == "" {
15+
value, present, err := envutil.GetEnvIntRaw(envKey)
16+
if !present {
1817
logConfig.Printf("%s not set in environment", envKey)
1918
return 0, false, nil
2019
}
2120

22-
value, err := strconv.Atoi(raw)
2321
if err != nil {
24-
logConfig.Printf("%s=%q is not a valid integer: %v", envKey, raw, err)
25-
return 0, false, fmt.Errorf("invalid %s value: %s", envKey, raw)
22+
logConfig.Printf("%s is not a valid integer: %v", envKey, err)
23+
return 0, false, fmt.Errorf("invalid %s value", envKey)
2624
}
2725

2826
if validationErr := validate(value); validationErr != nil {

internal/envutil/envutil.go

Lines changed: 24 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -28,17 +28,36 @@ func GetEnvString(envKey, defaultValue string) string {
2828
return defaultValue
2929
}
3030

31+
// GetEnvIntRaw returns the raw integer value of envKey without applying defaults
32+
// or positivity constraints.
33+
// It returns (0, false, nil) when envKey is unset or empty.
34+
// It returns (0, true, err) when envKey is set but cannot be parsed as an integer.
35+
func GetEnvIntRaw(envKey string) (int, bool, error) {
36+
envValue := os.Getenv(envKey)
37+
if envValue == "" {
38+
return 0, false, nil
39+
}
40+
41+
value, err := strconv.Atoi(envValue)
42+
if err != nil {
43+
return 0, true, err
44+
}
45+
return value, true, nil
46+
}
47+
3148
// GetEnvInt returns the integer value of the environment variable specified by envKey.
3249
// If the environment variable is not set, is empty, cannot be parsed as an integer,
3350
// or is not positive (> 0), it returns the defaultValue.
3451
// This function validates that the value is a positive integer.
3552
func GetEnvInt(envKey string, defaultValue int) int {
36-
if envValue := os.Getenv(envKey); envValue != "" {
37-
if value, err := strconv.Atoi(envValue); err == nil && value > 0 {
38-
return value
39-
}
40-
logEnvUtil.Printf("GetEnvInt: %s=%q is not a valid positive integer, using default=%d", envKey, sanitize.TruncateSecret(envValue), defaultValue)
53+
value, ok, err := GetEnvIntRaw(envKey)
54+
if !ok {
55+
return defaultValue
56+
}
57+
if err == nil && value > 0 {
58+
return value
4159
}
60+
logEnvUtil.Printf("GetEnvInt: %s=%q is not a valid positive integer, using default=%d", envKey, sanitize.TruncateSecret(os.Getenv(envKey)), defaultValue)
4261
return defaultValue
4362
}
4463

internal/envutil/envutil_test.go

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -289,6 +289,78 @@ func TestGetEnvInt(t *testing.T) {
289289
}
290290
}
291291

292+
func TestGetEnvIntRaw(t *testing.T) {
293+
tests := []struct {
294+
name string
295+
envKey string
296+
envValue string
297+
setEnv bool
298+
wantValue int
299+
wantOK bool
300+
wantErr bool
301+
}{
302+
{
303+
name: "valid integer",
304+
envKey: "TEST_INT_RAW_VAR",
305+
envValue: "42",
306+
setEnv: true,
307+
wantValue: 42,
308+
wantOK: true,
309+
wantErr: false,
310+
},
311+
{
312+
name: "not set",
313+
envKey: "TEST_INT_RAW_VAR",
314+
setEnv: false,
315+
wantOK: false,
316+
wantErr: false,
317+
},
318+
{
319+
name: "empty",
320+
envKey: "TEST_INT_RAW_VAR",
321+
envValue: "",
322+
setEnv: true,
323+
wantOK: false,
324+
wantErr: false,
325+
},
326+
{
327+
name: "invalid integer",
328+
envKey: "TEST_INT_RAW_VAR",
329+
envValue: "not-a-number",
330+
setEnv: true,
331+
wantOK: true,
332+
wantErr: true,
333+
},
334+
}
335+
336+
for _, tt := range tests {
337+
t.Run(tt.name, func(t *testing.T) {
338+
if tt.setEnv {
339+
t.Setenv(tt.envKey, tt.envValue)
340+
} else {
341+
previousValue, hadPreviousValue := os.LookupEnv(tt.envKey)
342+
t.Cleanup(func() {
343+
if hadPreviousValue {
344+
_ = os.Setenv(tt.envKey, previousValue)
345+
} else {
346+
_ = os.Unsetenv(tt.envKey)
347+
}
348+
})
349+
_ = os.Unsetenv(tt.envKey)
350+
}
351+
352+
got, ok, err := GetEnvIntRaw(tt.envKey)
353+
assert.Equal(t, tt.wantValue, got)
354+
assert.Equal(t, tt.wantOK, ok)
355+
if tt.wantErr {
356+
assert.Error(t, err)
357+
} else {
358+
assert.NoError(t, err)
359+
}
360+
})
361+
}
362+
}
363+
292364
func TestGetEnvBool(t *testing.T) {
293365
tests := []struct {
294366
name string

internal/logger/file_logger.go

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -131,22 +131,30 @@ var (
131131
logDebug = makeLevelLogger(logWithLevel, LogLevelDebug)
132132
)
133133

134-
// LogInfo logs an informational message.
134+
// LogInfo logs an informational message to the unified file logger sink.
135+
// The underlying filename depends on logger initialization. For
136+
// destination-specific logging use LogInfoToMarkdown or LogInfoToServer.
135137
func LogInfo(category, format string, args ...interface{}) {
136138
logInfo(category, format, args...)
137139
}
138140

139-
// LogWarn logs a warning message.
141+
// LogWarn logs a warning message to the unified file logger sink.
142+
// The underlying filename depends on logger initialization. For
143+
// destination-specific logging use LogWarnToMarkdown or LogWarnToServer.
140144
func LogWarn(category, format string, args ...interface{}) {
141145
logWarn(category, format, args...)
142146
}
143147

144-
// LogError logs an error message.
148+
// LogError logs an error message to the unified file logger sink.
149+
// The underlying filename depends on logger initialization. For
150+
// destination-specific logging use LogErrorToMarkdown or LogErrorToServer.
145151
func LogError(category, format string, args ...interface{}) {
146152
logError(category, format, args...)
147153
}
148154

149-
// LogDebug logs a debug message.
155+
// LogDebug logs a debug message to the unified file logger sink.
156+
// The underlying filename depends on logger initialization. For
157+
// destination-specific logging use LogDebugToMarkdown or LogDebugToServer.
150158
func LogDebug(category, format string, args ...interface{}) {
151159
logDebug(category, format, args...)
152160
}

internal/server/difc_log.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,8 @@ func buildFilteredItemLogEntry(serverID, toolName string, detail difc.FilteredIt
6161

6262
// getFilteredItemStringField returns the first non-empty string value from the map
6363
// matching any of the given field names.
64+
// NOTE: This helper is intentionally generic; if similar untyped JSON map extraction
65+
// patterns appear elsewhere, consider moving it to a shared utility package.
6466
func getFilteredItemStringField(m map[string]interface{}, fields ...string) string {
6567
for _, f := range fields {
6668
if v, ok := m[f]; ok {

0 commit comments

Comments
 (0)