diff --git a/internal/config/validation_test.go b/internal/config/validation_test.go index 9e0aa64e..1b39da7b 100644 --- a/internal/config/validation_test.go +++ b/internal/config/validation_test.go @@ -691,7 +691,4 @@ func TestLoadFromStdin_ValidationErrors(t *testing.T) { } } -// Helper function -func intPtr(i int) *int { - return &i -} +// Helper function - defined in validation_string_patterns_test.go diff --git a/internal/logger/common.go b/internal/logger/common.go index cf8fa75b..16f0b719 100644 --- a/internal/logger/common.go +++ b/internal/logger/common.go @@ -8,6 +8,70 @@ import ( "sync" ) +// Close Pattern for Logger Types +// +// All logger types in this package should implement their Close() method using this pattern: +// +// func (l *Logger) Close() error { +// l.mu.Lock() +// defer l.mu.Unlock() +// +// // Optional: Perform cleanup before closing (e.g., write footer) +// // if l.logFile != nil { +// // if err := writeCleanup(); err != nil { +// // return closeLogFile(l.logFile, &l.mu, "loggerName") +// // } +// // } +// +// return closeLogFile(l.logFile, &l.mu, "loggerName") +// } +// +// Why this pattern? +// +// 1. Mutex protection: Acquire lock at method entry to ensure thread-safe cleanup +// 2. Deferred unlock: Use defer to release lock even if errors occur +// 3. Optional cleanup: Logger-specific cleanup (like MarkdownLogger's footer) goes before closeLogFile +// 4. Shared helper: Always delegate to closeLogFile() for consistent sync and close behavior +// 5. Error handling: Return errors from closeLogFile to indicate serious issues +// +// Examples: +// +// Simple Close() with no cleanup (FileLogger, JSONLLogger): +// +// func (fl *FileLogger) Close() error { +// fl.mu.Lock() +// defer fl.mu.Unlock() +// return closeLogFile(fl.logFile, &fl.mu, "file") +// } +// +// Close() with custom cleanup (MarkdownLogger): +// +// func (ml *MarkdownLogger) Close() error { +// ml.mu.Lock() +// defer ml.mu.Unlock() +// +// if ml.logFile != nil { +// // Write closing details tag before closing +// footer := "\n\n" +// if _, err := ml.logFile.WriteString(footer); err != nil { +// // Even if footer write fails, try to close the file properly +// return closeLogFile(ml.logFile, &ml.mu, "markdown") +// } +// +// // Footer written successfully, now close +// return closeLogFile(ml.logFile, &ml.mu, "markdown") +// } +// return nil +// } +// +// This pattern is intentionally duplicated across logger types rather than abstracted: +// - It's a standard Go idiom for wrapper methods +// - The duplication is minimal (5-14 lines per type) +// - Each logger can customize cleanup as needed +// - The shared closeLogFile() helper eliminates complex logic duplication +// +// When adding a new logger type, follow this pattern to ensure consistent behavior. + // closeLogFile is a common helper for closing log files with consistent error handling. // It syncs buffered data before closing and handles errors appropriately. // The mutex should already be held by the caller. diff --git a/internal/tty/container.go b/internal/tty/container.go index bab8812b..e1f6f316 100644 --- a/internal/tty/container.go +++ b/internal/tty/container.go @@ -3,19 +3,12 @@ package tty import ( "os" "strings" - - "github.com/githubnext/gh-aw-mcpg/internal/logger" ) -var log = logger.New("tty:container") - // IsRunningInContainer detects if the current process is running inside a container func IsRunningInContainer() bool { - log.Print("Detecting container environment") - // Method 1: Check for /.dockerenv file (Docker-specific) if _, err := os.Stat("/.dockerenv"); err == nil { - log.Print("Container detected: /.dockerenv file exists") return true } @@ -27,17 +20,14 @@ func IsRunningInContainer() bool { strings.Contains(content, "containerd") || strings.Contains(content, "kubepods") || strings.Contains(content, "lxc") { - log.Printf("Container detected: found container indicator in /proc/1/cgroup") return true } } // Method 3: Check environment variable (set by Dockerfile) if os.Getenv("RUNNING_IN_CONTAINER") == "true" { - log.Print("Container detected: RUNNING_IN_CONTAINER=true") return true } - log.Print("No container environment detected") return false }