Document logger Close() pattern and fix import cycle

[Copilot]

Issue #436 identified minimal duplication in logger Close() methods (5-14 lines per type) and recommended documentation over refactoring. The pattern is idiomatic Go: wrapper methods that add context to a shared closeLogFile() helper.

Changes

Documentation (internal/logger/common.go)

• Added 64-line comment block documenting the Close() pattern
• Explains mutex locking, optional cleanup hooks, and delegation to closeLogFile()
• Provides examples for simple (FileLogger) and complex (MarkdownLogger) implementations
• Clarifies why this duplication is intentional: Go idiom, minimal overhead, per-type customization

// Close Pattern for Logger Types
//
// All logger types should implement Close() using this pattern:
//
//   func (l *Logger) Close() error {
//       l.mu.Lock()
//       defer l.mu.Unlock()
//       
//       // Optional: custom cleanup (e.g., MarkdownLogger footer)
//       return closeLogFile(l.logFile, &l.mu, "loggerName")
//   }

Build Fixes

• internal/tty/container.go: Removed logger import to break internal/logger ↔ internal/tty cycle
• internal/config/validation_test.go: Removed duplicate intPtr() declaration

Both were blocking compilation.

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:

• nonexistent.local
  • Triggering command: /tmp/go-build2955798797/b269/launcher.test /tmp/go-build2955798797/b269/launcher.test -test.testlogfile=/tmp/go-build2955798797/b269/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true go pull.rebase /usr/local/.ghcu-lang=go1.25 (dns block)
  • Triggering command: /tmp/go-build1623642188/b269/launcher.test /tmp/go-build1623642188/b269/launcher.test -test.testlogfile=/tmp/go-build1623642188/b269/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build1775386590/b265/vet.cfg go 4hElLqVi8 x_amd64/compile (dns block)
• this-host-does-not-exist-12345.com
  • Triggering command: /tmp/go-build2955798797/b278/mcp.test /tmp/go-build2955798797/b278/mcp.test -test.testlogfile=/tmp/go-build2955798797/b278/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true -b ontext.go 64/pkg/tool/linu-lang=go1.25 --depth 2 REDACTED 64/pkg/tool/linu/tmp/go-build2955798797/b098/vet.cfg conf�� _.a ternal/impl/impl-c=4 64/pkg/tool/linu-nolocalimports user.email (dns block)
  • Triggering command: /tmp/go-build1623642188/b278/mcp.test /tmp/go-build1623642188/b278/mcp.test -test.testlogfile=/tmp/go-build1623642188/b278/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build1775386590/b274/vet.cfg ache/go/1.25.6/x64/src/runtime/cgo1.25.6 rJ3W/LomnKYz78ppfT8whrJ3W 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 Close() Method Duplication</issue_title>
<issue_description># 🔍 Duplicate Code Pattern: Logger Close() Method Duplication

Part of duplicate code analysis: #435

Summary

The three logger types implement nearly identical Close() methods that follow the same pattern: acquire mutex, call closeLogFile() helper, release mutex. While the duplication is minimal (6-8 lines per method), it represents an opportunity for interface-based abstraction.

Duplication Details

Pattern: Logger Close() Methods

• Severity: Medium
• Occurrences: 3 instances (FileLogger, JSONLLogger, MarkdownLogger)
• Locations:
  • internal/logger/file_logger.go (lines 58-62, 5 lines)
  • internal/logger/jsonl_logger.go (lines 59-63, 5 lines)
  • internal/logger/markdown_logger.go (lines 67-80, 14 lines)

Code Comparison:

FileLogger.Close() (5 lines):

func (fl *FileLogger) Close() error {
    fl.mu.Lock()
    defer fl.mu.Unlock()
    return closeLogFile(fl.logFile, &fl.mu, "file")
}

JSONLLogger.Close() (5 lines):

func (jl *JSONLLogger) Close() error {
    jl.mu.Lock()
    defer jl.mu.Unlock()
    return closeLogFile(jl.logFile, &jl.mu, "JSONL")
}

MarkdownLogger.Close() (14 lines - more complex):

func (ml *MarkdownLogger) Close() error {
    ml.mu.Lock()
    defer ml.mu.Unlock()

    if ml.logFile != nil {
        // Write closing details tag before closing
        footer := "\n</details>\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
}

Note: MarkdownLogger is an exception - it needs special cleanup logic to write the footer before closing.

Impact Analysis

Maintainability

• Low impact: Only 3 instances, pattern is clear and well-established
• Common pattern in Go: Wrapper methods around shared helpers are idiomatic
• Already refactored: The closeLogFile() helper in common.go (lines 23-36) consolidates the actual closing logic

Bug Risk

• Very low: The pattern is simple and unlikely to diverge
• Good design: Each logger calls the same closeLogFile() helper, ensuring consistent behavior
• Special cases handled: MarkdownLogger's footer-writing logic is appropriately placed in its Close() method

Code Bloat

• Minimal: ~10 lines total duplication (excluding MarkdownLogger's special logic)
• Not a priority: Given the small size and clear pattern, this is acceptable duplication

Refactoring Recommendations

Option 1: Interface with Default Implementation (Go 1.23+)

Note: This requires Go 1.23+ for interface with methods. Consider carefully before implementing.

// Define a closable logger interface
type ClosableLogger interface {
    GetLogFile() *os.File
    GetMutex() *sync.Mutex
    GetLoggerName() string
    BeforeClose() error  // Hook for custom cleanup
}

// Default Close implementation
func CloseLogger(cl ClosableLogger) error {
    mu := cl.GetMutex()
    mu.Lock()
    defer mu.Unlock()
    
    // Call before-close hook if needed
    if err := cl.BeforeClose(); err != nil {
        return err
    }
    
    return closeLogFile(cl.GetLogFile(), mu, cl.GetLoggerName())
}

// Usage:
func (fl *FileLogger) Close() error {
    return CloseLogger(fl)
}

func (fl *FileLogger) BeforeClose() error {
    return nil  // No special cleanup
}

func (ml *MarkdownLogger) BeforeClose() error {
    if ml.logFile != nil {
        footer := "\n</details>\n"
        _, err := ml.logFile.WriteString(footer)
        return err
    }
    return nil
}

Benefits:

• Eliminates ~10 lines of duplication
• Makes the close pattern explicit and reusable
• Provides clear extension point for logger-specific cleanup

Drawbacks:

• Adds complexity for minimal gain
• Requires additional interface methods
• Less idiomatic than simple wrapper methods in Go

Estimated effort: 2-3 hours (not recommended given low value)

Option 2: Accept the Pattern (Recommended)

Recommendation: Do not refactor - this level of duplication is acceptable and idiomatic in Go.

Rationale:

1. Go idiom: Wrapper methods that add context to shared functions are standard practice
2. Clear and simple: Each Close() method is trivial to understand
3. Low maintenance burden: Unlikely to change or diverge
4. Special cases: MarkdownLogger demonstrates why per-type methods are valuable
5. Already optimized: The shared closeLogFile() helper already eliminates the complex logic
6. Cost-benefit: Refactoring effort outweighs benefits

**What has been do...

• Fixes [duplicate-code] Duplicate Code Pattern: Logger Close() Method Duplication #438

---

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.