From e3e01bb46235c78c0503a12aa63ddc6558dcdee8 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 10 Jul 2026 11:19:35 +0000 Subject: [PATCH 1/2] Initial plan From 06e57de408b05fea6eeccaf247efd8bcaebb3125 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 10 Jul 2026 11:26:36 +0000 Subject: [PATCH 2/2] refactor(logger): use initLogFile and serverFileLoggerFactory in ServerFileLogger - Replace manual os.OpenFile call in getOrCreateLogger with initLogFile helper, which handles MkdirAll + OpenFile in one call and avoids duplicating file-init logic - Add serverFileLoggerFactory variable using loggerFactory[*ServerFileLogger], consistent with the factory pattern used by all other logger types - Refactor InitServerFileLogger to delegate setup/onError to the factory, moving fallback logic into the onError field - Remove now-unused path/filepath import - Update global_state.go comment to reflect the new factory usage --- internal/logger/global_state.go | 5 ++-- internal/logger/server_file_logger.go | 35 ++++++++++++++++++--------- 2 files changed, 26 insertions(+), 14 deletions(-) diff --git a/internal/logger/global_state.go b/internal/logger/global_state.go index 15ab645fb..e422bb671 100644 --- a/internal/logger/global_state.go +++ b/internal/logger/global_state.go @@ -149,8 +149,9 @@ import ( // - Error: Returns nil (never fails, falls back to unified logging) // - Use case: Per-server logs are helpful but not required // -// Note: ServerFileLogger doesn't use initLogger() because it creates -// files on-demand, but follows the same fallback philosophy. +// Note: ServerFileLogger uses serverFileLoggerFactory but not initLogger() +// because it creates per-serverID files on demand rather than opening +// a single file at initialization. The factory setup receives a nil file. // // Global Logger Management: // diff --git a/internal/logger/server_file_logger.go b/internal/logger/server_file_logger.go index a3b97a819..41081178b 100644 --- a/internal/logger/server_file_logger.go +++ b/internal/logger/server_file_logger.go @@ -4,7 +4,6 @@ import ( "fmt" "log" "os" - "path/filepath" "sync" "github.com/github/gh-aw-mcpg/internal/syncutil" @@ -24,6 +23,20 @@ var ( globalServerLoggerMu sync.RWMutex ) +// serverFileLoggerFactory bundles the setup and error-handler for ServerFileLogger. +// Unlike other factories, setup receives a nil file because ServerFileLogger creates +// per-serverID files on demand rather than opening a single file at initialization. +var serverFileLoggerFactory = loggerFactory[*ServerFileLogger]{ + setup: func(_ *os.File, logDir, _ string) (*ServerFileLogger, error) { + log.Printf("Initialized per-serverID logging in directory: %s", logDir) + return newServerFileLogger(logDir, false), nil + }, + onError: func(err error, logDir, _ string) (*ServerFileLogger, error) { + return fallbackLoggerOnInitError(err, "Failed to create log directory for server logs", + "Falling back to unified logging only", newServerFileLogger(logDir, true)) + }, +} + func newServerFileLogger(logDir string, useFallback bool) *ServerFileLogger { return &ServerFileLogger{ logDir: logDir, @@ -35,17 +48,16 @@ func newServerFileLogger(logDir string, useFallback bool) *ServerFileLogger { // InitServerFileLogger initializes the global server file logger func InitServerFileLogger(logDir string) error { - // Create log directory if it doesn't exist + var sfl *ServerFileLogger + var initErr error if err := os.MkdirAll(logDir, 0755); err != nil { - logFallbackWarnings(err, "Failed to create log directory for server logs", "Falling back to unified logging only") - sfl := newServerFileLogger(logDir, true) - initGlobalLogger(&globalServerLoggerMu, &globalServerFileLogger, sfl) - return nil + sfl, initErr = serverFileLoggerFactory.onError(err, logDir, "") + } else { + sfl, initErr = serverFileLoggerFactory.setup(nil, logDir, "") + } + if initErr != nil { + return initErr } - - sfl := newServerFileLogger(logDir, false) - - log.Printf("Initialized per-serverID logging in directory: %s", logDir) initGlobalLogger(&globalServerLoggerMu, &globalServerFileLogger, sfl) return nil } @@ -60,8 +72,7 @@ func (sfl *ServerFileLogger) getOrCreateLogger(serverID string) (*log.Logger, er // Create log file for this serverID fileName := fmt.Sprintf("%s.log", serverID) - logPath := filepath.Join(sfl.logDir, fileName) - file, err := os.OpenFile(logPath, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644) + file, err := initLogFile(sfl.logDir, fileName, os.O_APPEND) if err != nil { return nil, fmt.Errorf("failed to open log file for server %s: %w", serverID, err) }