Consolidate logger initialization to use generic helpers

[Copilot]

ServerFileLogger was manually implementing global state management that other loggers already handled via generic helpers in global_helpers.go. This created 23 lines of duplicate code and made fallback behavior strategies unclear.

Changes

Extended generic helpers to ServerFileLogger

• Added *ServerFileLogger to closableLogger constraint
• Replaced manual initGlobalServerFileLogger() and CloseServerFileLogger() with calls to generic initGlobalLogger() and closeGlobalLogger()
• Eliminates 23 lines of duplicate mutex handling and cleanup logic

Documented fallback strategies
Added 130-line "Initialization Pattern for Logger Types" section to common.go explaining when each logger uses fallbacks:

• FileLogger: stdout fallback for critical operational logs
• MarkdownLogger: silent fallback for optional preview logs
• JSONLLogger: strict error mode for structured data
• ServerFileLogger: unified logger fallback for per-server logs

Example

Before (manual implementation):

func initGlobalServerFileLogger(logger *ServerFileLogger) {
    globalServerLoggerMu.Lock()
    defer globalServerLoggerMu.Unlock()
    if globalServerFileLogger != nil {
        globalServerFileLogger.Close()
    }
    globalServerFileLogger = logger
}

After (using generic helper):

func initGlobalServerFileLogger(logger *ServerFileLogger) {
    initGlobalLogger(&globalServerLoggerMu, &globalServerFileLogger, logger)
}

All loggers now use the same thread-safe initialization pattern. Documentation provides clear guidance for adding new logger types.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

• example.com
  • Triggering command: /tmp/go-build2696183341/b271/launcher.test /tmp/go-build2696183341/b271/launcher.test -test.testlogfile=/tmp/go-build2696183341/b271/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 1636032/b166/_pkg_.a go x_amd64/compile (dns block)
• invalid-host-that-does-not-exist-12345.com
  • Triggering command: /tmp/go-build2696183341/b259/config.test /tmp/go-build2696183341/b259/config.test -test.testlogfile=/tmp/go-build2696183341/b259/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 1636032/b153/_pkg_.a .cfg x_amd64/vet (dns block)
• nonexistent.local
  • Triggering command: /tmp/go-build2696183341/b271/launcher.test /tmp/go-build2696183341/b271/launcher.test -test.testlogfile=/tmp/go-build2696183341/b271/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 1636032/b166/_pkg_.a go x_amd64/compile (dns block)
• slow.example.com
  • Triggering command: /tmp/go-build2696183341/b271/launcher.test /tmp/go-build2696183341/b271/launcher.test -test.testlogfile=/tmp/go-build2696183341/b271/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 1636032/b166/_pkg_.a go x_amd64/compile (dns block)
• this-host-does-not-exist-12345.com
  • Triggering command: /tmp/go-build2696183341/b280/mcp.test /tmp/go-build2696183341/b280/mcp.test -test.testlogfile=/tmp/go-build2696183341/b280/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true ache/go/1.25.6/x64/src/runtime/c-g .cfg x_amd64/vet (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

• Configure Actions setup steps to set up my environment, which run before the firewall is enabled
• Add the appropriate URLs or hosts to the custom allowlist in this repository's Copilot coding agent settings (admins only)

Original prompt

---

This section details on the original issue you should resolve

<issue_title>[duplicate-code] Duplicate Code Pattern: Logger Initialization Boilerplate</issue_title>
<issue_description># 🔍 Duplicate Code Pattern: Logger Initialization Boilerplate

Part of duplicate code analysis: #797

Summary

Four logger files (file_logger.go, jsonl_logger.go, markdown_logger.go, server_file_logger.go) share nearly identical initialization patterns for file handling, error management, and global state setup. This creates significant maintenance overhead and increases the risk of inconsistent behavior across loggers.

Duplication Details

Pattern: Logger Initialization Structure

• Severity: High
• Occurrences: 4 instances (FileLogger, JSONLLogger, MarkdownLogger, ServerFileLogger)
• Locations:
  • internal/logger/file_logger.go (lines 28-60, 63-68)
  • internal/logger/jsonl_logger.go (lines 38-66, 69-74)
  • internal/logger/markdown_logger.go (lines 28-54, 72-89)
  • internal/logger/server_file_logger.go (lines 70-114)

Duplicated Components

1. File Initialization Pattern (~40 lines per logger)
Each logger implements nearly identical logic:

func Init*Logger(logDir, fileName string) error {
    logger, err := initLogger(
        logDir, fileName, os.O_APPEND, // or os.O_TRUNC
        // Setup function
        func(file *os.File, logDir, fileName string) (*Logger, error) {
            // Create logger struct
            // Initialize fields
            // Return logger
        },
        // Error handler
        func(err error, logDir, fileName string) (*Logger, error) {
            // Fallback logic or error return
        },
    )
    // Initialize global logger
    return err
}

2. Close Method Pattern (~8 lines per logger)

func (l *Logger) Close() error {
    l.mu.Lock()
    defer l.mu.Unlock()
    return closeLogFile(l.logFile, &l.mu, "loggerType")
}

3. Global Logger State Pattern (~12 lines per logger)

var (
    global*Logger **Logger
    global*Mu     sync.RWMutex
)

func initGlobal*Logger(logger **Logger) {
    initGlobalLogger(&global*Mu, &global*Logger, logger)
}

func closeGlobal*Logger() error {
    return closeGlobalLogger(&global*Mu, &global*Logger)
}

Code Sample: File Logger

// InitFileLogger initializes the global file logger
func InitFileLogger(logDir, fileName string) error {
    logger, err := initLogger(
        logDir, fileName, os.O_APPEND,
        func(file *os.File, logDir, fileName string) (*FileLogger, error) {
            fl := &FileLogger{
                logDir:   logDir,
                fileName: fileName,
                logFile:  file,
                logger:   log.New(file, "", 0),
            }
            log.Printf("Logging to file: %s", filepath.Join(logDir, fileName))
            return fl, nil
        },
        func(err error, logDir, fileName string) (*FileLogger, error) {
            // Fallback to stdout...
            return fl, nil
        },
    )
    initGlobalFileLogger(logger)
    return err
}

Similar structures exist in all 4 logger files with only minor variations.

Impact Analysis

Maintainability

• High Risk: Changes to initialization logic must be replicated across 4 files
• Inconsistency: Different error handling strategies across loggers (some fallback to stdout, others return errors)
• Code Bloat: ~120+ lines of similar initialization code

Bug Risk

• Medium Risk: Bug fixes must be applied to all 4 loggers independently
• Historical Issues: The codebase already recognized this pattern and created common.go with initLogger() helper, but only partial consolidation occurred
• Fallback Inconsistency: FileLogger/MarkdownLogger fallback to stdout, JSONLLogger doesn't - easy to miss

Code Bloat

• Total Lines: ~120+ lines of duplicated initialization patterns
• Percentage: ~16% of logger package code (727 total lines across 4 files)

Refactoring Recommendations

1. Extract Logger Factory Pattern

Effort: Medium (4-6 hours)

Create a generic logger factory that handles common initialization:

// LoggerConfig defines common logger configuration
type LoggerConfig struct {
    LogDir        string
    FileName      string
    FileFlags     int  // os.O_APPEND, os.O_TRUNC, etc.
    AllowFallback bool // Whether to fallback to stdout on error
}

// LoggerFactory creates and initializes loggers with common patterns
type LoggerFactory[T closableLogger] struct {
    config LoggerConfig
}

func (f *LoggerFactory[T]) Create(
    setupFunc func(*os.File, string, string) (T, error),
    fallbackFunc func(error, string, string) (T, error),
) (T, error) {
    // Common initialization logic
    // Replaces duplicate initLogger calls
}

Benefits:

• Centralized file handling and error management
• Consistent fallback behavior
• Reduces initialization code by ~80 lines
• Single source of trut...

Custom agent used: agentic-workflows
GitHub Agentic Workflows (gh-aw) - Create, debug, and upgrade AI-powered workflows with intelligent prompt routing

• Fixes [duplicate-code] Duplicate Code Pattern: Logger Initialization Boilerplate #798

---

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

[Copilot]

Pull request overview

This PR consolidates ServerFileLogger global lifecycle management to use the same generic init/close helpers as the other logger types, and adds/expands documentation describing logger initialization and fallback strategies.

Changes:

• Extend closableLogger to include *ServerFileLogger and add generic-helper-backed initGlobalServerFileLogger / closeGlobalServerFileLogger.
• Replace ServerFileLogger’s manual global close logic with the generic closeGlobalLogger-based helper.
• Add a detailed “Initialization Pattern for Logger Types” documentation section describing fallback behaviors.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
internal/logger/server_file_logger.go	Swaps manual global close logic for a helper (closeGlobalServerFileLogger).
internal/logger/global_helpers.go	Extends the generic global logger helpers to cover ServerFileLogger and adds type-specific wrapper helpers.
internal/logger/common.go	Adds long-form documentation for initialization patterns and fallback strategy guidance.

Comments suppressed due to low confidence (2)

internal/logger/common.go:185

• In "Adding a New Logger Type", step 3 mandates using initLogger(), but this isn't applicable for logger types that don't follow the open-on-init pattern (as noted for ServerFileLogger). Consider adjusting the guidance to say "use initLogger() when the logger opens a file during initialization" and describe alternatives for on-demand loggers.

//  1. Implement Close() method following the Close Pattern (above)
//  2. Add type to closableLogger constraint in global_helpers.go
//  3. Use initLogger() for initialization with appropriate fallback strategy
//  4. Add initGlobal*Logger() and closeGlobal*Logger() helpers

internal/logger/common.go:303

• File handle may be writable as a result of data flow from a call to OpenFile and closing it may result in data loss upon failure, which is not handled explicitly.

		file.Close()

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The file header comment still describes these helpers as managing global state for FileLogger/JSONLLogger/MarkdownLogger only, but this PR extends them to ServerFileLogger as well. Update the header comment to include ServerFileLogger so documentation matches current behavior.

[Copilot]

The documentation says "All logger types use the initLogger() generic helper" but later in the same section notes that ServerFileLogger does not use initLogger() (it creates files on-demand). Please reword this to avoid the contradiction (e.g., specify which logger types use initLogger()).

This issue also appears on line 182 of the same file.