refactor logger to zerolog

[cytown]

📝 Description

refactor logger to zerolog.

🗣️ Type of Change

• 🐞 Bug fix (non-breaking change which fixes an issue)
• ✨ New feature (non-breaking change which adds functionality)
• 📖 Documentation update
• ⚡ Code refactoring (no functional changes, no api changes)

🤖 AI Code Generation

• 🤖 Fully AI-generated (100% AI, 0% Human)
• 🛠️ Mostly AI-generated (AI draft, Human verified/modified)
• 👨‍💻 Mostly Human-written (Human lead, AI assisted or none)

🔗 Related Issue

📚 Technical Context (Skip for Docs)

• Reference URL:
• Reasoning:

🧪 Test Environment

• Hardware: PC
• OS: macOS 13.6
• Model/Provider:
• Channels:

📸 Evidence (Optional)

Click to view Logs/Screenshots

☑️ Checklist

• My code/docs follow the style of this project.
• I have performed a self-review of my own changes.
• I have updated the documentation accordingly.

[mengzhuo]

Also you should update go.mod if zerolog is explicity required.

[cytown]

Also you should update go.mod if zerolog is explicity required.

done.

[yinwm]

Code review

Found 2 issues:

1. File handle leak in EnableFileLogging - The function opens a new file but does not close the previous file handle when called multiple times. The original code had if logger.file != nil { logger.file.Close() } before opening a new file, but this was removed in the refactor.

picoclaw/pkg/logger/logger.go

Lines 67 to 83 in 4a8a3ae

	func EnableFileLogging(filePath string) error {
	mu.Lock()
	defer mu.Unlock()
	if err := os.MkdirAll(filepath.Dir(filePath), 0o755); err != nil {
	return fmt.Errorf("failed to create log directory: %w", err)
	}
	file, err := os.OpenFile(filePath, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0o644)
	if err != nil {
	return fmt.Errorf("failed to open log file: %w", err)
	}
	fileLogger = zerolog.New(file).With().Timestamp().Caller().Logger()
	return nil
	}

2. File handle leak in DisableFileLogging - The function resets fileLogger to zero value but does not close the underlying file handle. The original code had if logger.file != nil { logger.file.Close() } which was removed.

picoclaw/pkg/logger/logger.go

Lines 84 to 88 in 4a8a3ae

	func DisableFileLogging() {
	mu.Lock()
	defer mu.Unlock()
	fileLogger = zerolog.Logger{}
	}

Suggested fix: Store the file handle in a global variable (e.g., logFile *os.File) and close it properly in both functions:

var (
    logger     zerolog.Logger
    fileLogger zerolog.Logger
    logFile    *os.File  // Add this
    // ...
)

func EnableFileLogging(filePath string) error {
    mu.Lock()
    defer mu.Unlock()

    if err := os.MkdirAll(filepath.Dir(filePath), 0o755); err != nil {
        return fmt.Errorf("failed to create log directory: %w", err)
    }

    newFile, err := os.OpenFile(filePath, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0o644)
    if err != nil {
        return fmt.Errorf("failed to open log file: %w", err)
    }

    // Close old file if exists
    if logFile != nil {
        logFile.Close()
    }

    logFile = newFile
    fileLogger = zerolog.New(logFile).With().Timestamp().Caller().Logger()
    return nil
}

func DisableFileLogging() {
    mu.Lock()
    defer mu.Unlock()

    if logFile != nil {
        logFile.Close()
        logFile = nil
    }
    fileLogger = zerolog.Logger{}
}

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[cytown]

Code review

Found 2 issues:

1. File handle leak in EnableFileLogging - The function opens a new file but does not close the previous file handle when called multiple times. The original code had if logger.file != nil { logger.file.Close() } before opening a new file, but this was removed in the refactor.

picoclaw/pkg/logger/logger.go

Lines 67 to 83 in 4a8a3ae

	func EnableFileLogging(filePath string) error {
	mu.Lock()
	defer mu.Unlock()
	if err := os.MkdirAll(filepath.Dir(filePath), 0o755); err != nil {
	return fmt.Errorf("failed to create log directory: %w", err)
	}
	file, err := os.OpenFile(filePath, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0o644)
	if err != nil {
	return fmt.Errorf("failed to open log file: %w", err)
	}
	fileLogger = zerolog.New(file).With().Timestamp().Caller().Logger()
	return nil
	}

2. File handle leak in DisableFileLogging - The function resets fileLogger to zero value but does not close the underlying file handle. The original code had if logger.file != nil { logger.file.Close() } which was removed.

picoclaw/pkg/logger/logger.go

Lines 84 to 88 in 4a8a3ae

	func DisableFileLogging() {
	mu.Lock()
	defer mu.Unlock()
	fileLogger = zerolog.Logger{}
	}

Suggested fix: Store the file handle in a global variable (e.g., logFile *os.File) and close it properly in both functions:

var (
    logger     zerolog.Logger
    fileLogger zerolog.Logger
    logFile    *os.File  // Add this
    // ...
)

func EnableFileLogging(filePath string) error {
    mu.Lock()
    defer mu.Unlock()

    if err := os.MkdirAll(filepath.Dir(filePath), 0o755); err != nil {
        return fmt.Errorf("failed to create log directory: %w", err)
    }

    newFile, err := os.OpenFile(filePath, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0o644)
    if err != nil {
        return fmt.Errorf("failed to open log file: %w", err)
    }

    // Close old file if exists
    if logFile != nil {
        logFile.Close()
    }

    logFile = newFile
    fileLogger = zerolog.New(logFile).With().Timestamp().Caller().Logger()
    return nil
}

func DisableFileLogging() {
    mu.Lock()
    defer mu.Unlock()

    if logFile != nil {
        logFile.Close()
        logFile = nil
    }
    fileLogger = zerolog.Logger{}
}

🤖 Generated with Claude Code

• If this code review was useful, please react with 👍. Otherwise, react with 👎.

done,

[mengzhuo]

@yinwm Could you take a look?

[yinwm]

Code Review

Found 4 issues that should be addressed before merging:

1. FATAL level uses wrong zerolog method

At pkg/logger/logger.go#L119-L120:

case zerolog.FatalLevel:
    event = logger.Error()  // Should use logger.Fatal()

Then manually calling os.Exit(1) at the end. This bypasses zerolog's Fatal() mechanism and skips OnFatal hooks. Should use logger.Fatal() directly and remove the manual exit.

2. DRY violation - duplicate switch-case

The level-to-event switch logic is duplicated for console logger and file logger in logMessage(). Extract to a helper function like getEvent(logger zerolog.Logger, level LogLevel) *zerolog.Event.

3. API inconsistency

Only Fatalf was added, but there's no corresponding Debugf, Infof, Warnf, Errorf. Either add all of them or remove Fatalf for consistency.

4. TODO comment should not be merged

TimeFormat: "15:04:05", // TODO: make it configurable???

The three question marks indicate uncertainty. Either implement the configuration or remove the TODO entirely. Don't merge unresolved TODOs.

---

Good aspects

• Using mature zerolog library is the right direction
• Maintained backward API compatibility
• Auto-creating log directory is a nice improvement
• Third-party SDK logger integration looks correct

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[cytown]

Code Review

Found 4 issues that should be addressed before merging:

1. FATAL level uses wrong zerolog method

At pkg/logger/logger.go#L119-L120:

case zerolog.FatalLevel:
    event = logger.Error()  // Should use logger.Fatal()

Then manually calling os.Exit(1) at the end. This bypasses zerolog's Fatal() mechanism and skips OnFatal hooks. Should use logger.Fatal() directly and remove the manual exit.

2. DRY violation - duplicate switch-case

The level-to-event switch logic is duplicated for console logger and file logger in logMessage(). Extract to a helper function like getEvent(logger zerolog.Logger, level LogLevel) *zerolog.Event.

3. API inconsistency

Only Fatalf was added, but there's no corresponding Debugf, Infof, Warnf, Errorf. Either add all of them or remove Fatalf for consistency.

4. TODO comment should not be merged

TimeFormat: "15:04:05", // TODO: make it configurable???

The three question marks indicate uncertainty. Either implement the configuration or remove the TODO entirely. Don't merge unresolved TODOs.

Good aspects

• Using mature zerolog library is the right direction
• Maintained backward API compatibility
• Auto-creating log directory is a nice improvement
• Third-party SDK logger integration looks correct

🤖 Generated with Claude Code

• If this code review was useful, please react with 👍. Otherwise, react with 👎.

1/2 fixed. 3 it's for 3rd party logger usage, no need to fix. 4 it's a feature need negotiate later, so make it TODO, please check the previous review comment.