Addressing PR comments

[Claude]

Original prompt

---

This section details on the original issue you should resolve

<issue_title>[duplicate-code] Duplicate Code Pattern: Logger Functions with Repeated Mutex Lock Patterns</issue_title>
<issue_description># 🔍 Duplicate Code Pattern: Logger Functions with Mutex Lock Patterns

Part of duplicate code analysis: #970

Summary

The logger package contains 12+ nearly identical functions with repeated mutex lock/unlock patterns across three files. Each set of 4 functions (Info/Warn/Error/Debug) follows the exact same structure, differing only in the log level parameter.

Duplication Details

Pattern: Repeated Logger Function Structure

• Severity: High
• Occurrences: 12+ instances across 3 files
• Locations:
  • internal/logger/file_logger.go - Lines 114-151 (4 functions: LogInfo, LogWarn, LogError, LogDebug)
  • internal/logger/server_file_logger.go - Lines 162-211 (4 functions: LogInfoWithServer, LogWarnWithServer, LogErrorWithServer, LogDebugWithServer)
  • internal/logger/markdown_logger.go - Lines 187-204 (4 functions: LogInfoMd, LogWarnMd, LogErrorMd, LogDebugMd)

Code Sample - file_logger.go Pattern:

func LogInfo(category, format string, args ...interface{}) {
	globalLoggerMu.RLock()
	defer globalLoggerMu.RUnlock()

	if globalFileLogger != nil {
		globalFileLogger.Log(LogLevelInfo, category, format, args...)
	}
}

func LogWarn(category, format string, args ...interface{}) {
	globalLoggerMu.RLock()
	defer globalLoggerMu.RUnlock()

	if globalFileLogger != nil {
		globalFileLogger.Log(LogLevelWarn, category, format, args...)
	}
}
// ... LogError and LogDebug follow the same pattern

Code Sample - server_file_logger.go Pattern:

func LogInfoWithServer(serverID, category, format string, args ...interface{}) {
	globalServerLoggerMu.RLock()
	defer globalServerLoggerMu.RUnlock()

	if globalServerFileLogger != nil {
		globalServerFileLogger.Log(serverID, LogLevelInfo, category, format, args...)
	}

	// Also log to the main log file for unified view
	LogInfo(category, "[%s] %s", serverID, fmt.Sprintf(format, args...))
}
// ... LogWarnWithServer, LogErrorWithServer, LogDebugWithServer follow the same pattern

Impact Analysis

• Maintainability: High Impact - Any change to logging behavior (e.g., adding tracing, modifying mutex strategy) requires updating 12+ functions identically
• Bug Risk: High - If one function is updated but others are missed, inconsistent logging behavior results
• Code Bloat: ~120+ lines of repetitive code (12 functions × ~10 lines each)
• Testing Complexity: Each function requires separate testing, increasing test maintenance

Refactoring Recommendations

1. Extract Generic Logger Helper Function (Recommended)

Extract common mutex lock/unlock and nil-check pattern into a generic helper:

// In file_logger.go
func logWithLevel(level LogLevel, category, format string, args ...interface{}) {
	globalLoggerMu.RLock()
	defer globalLoggerMu.RUnlock()

	if globalFileLogger != nil {
		globalFileLogger.Log(level, category, format, args...)
	}
}

func LogInfo(category, format string, args ...interface{}) {
	logWithLevel(LogLevelInfo, category, format, args...)
}

func LogWarn(category, format string, args ...interface{}) {
	logWithLevel(LogLevelWarn, category, format, args...)
}

func LogError(category, format string, args ...interface{}) {
	logWithLevel(LogLevelError, category, format, args...)
}

func LogDebug(category, format string, args ...interface{}) {
	logWithLevel(LogLevelDebug, category, format, args...)
}

• Location: internal/logger/file_logger.go (add logWithLevel helper)
• Estimated Effort: 2-3 hours
• Benefits:
  • Reduces code from ~40 lines to ~20 lines per file
  • Centralizes mutex handling logic
  • Makes future changes safer (single point of modification)

2. Apply Same Pattern to server_file_logger.go

// In server_file_logger.go
func logWithLevelAndServer(serverID string, level LogLevel, category, format string, args ...interface{}) {
	globalServerLoggerMu.RLock()
	defer globalServerLoggerMu.RUnlock()

	if globalServerFileLogger != nil {
		globalServerFileLogger.Log(serverID, level, category, format, args...)
	}

	// Also log to the main log file for unified view
	switch level {
	case LogLevelInfo:
		LogInfo(category, "[%s] %s", serverID, fmt.Sprintf(format, args...))
	case LogLevelWarn:
		LogWarn(category, "[%s] %s", serverID, fmt.Sprintf(format, args...))
	// ... other levels
	}
}

• Location: internal/logger/server_file_logger.go
• Estimated Effort: 2-3 hours
• Benefits: Same as above, plus eliminates redundant dual-logging logic

3. Apply Same Pattern to markdown_logger.go

Similar refactoring for LogInfoMd, LogWarnMd, LogErrorMd, LogDebugMd:

// In markdown_logger.go
func logWithMarkdownLevel(level LogLevel, category, format string, args ...interface{}) ...

</details>



<!-- START COPILOT CODING AGENT SUFFIX -->

- Fixes github/gh-aw-mcpg#971

[lpcox]

@claude[agent] please fix these linting errors

make lint
shell: /usr/bin/bash -e {0}
env:
GOTOOLCHAIN: local
Running linters...
Running gofmt check...
The following files are not formatted:
internal/logger/helper_functions_test.go
make: *** [Makefile:26: lint] Error 1

[Claude]

@claude[agent] please fix these linting errors

make lint
...

Fixed in commit 2aa8f03. The file has been formatted with gofmt and all linting checks now pass.

[Copilot]

Pull request overview

This PR addresses duplicate code patterns in the logger package by introducing inline helper functions that consolidate repeated mutex lock/unlock and level-specific dispatching patterns. The refactoring reduces code duplication by approximately 25% across three logger files while maintaining identical behavior and thread-safety guarantees.

Changes:

• Extracted logWithLevel helper in file_logger.go to eliminate duplicate mutex patterns across Info/Warn/Error/Debug functions
• Extracted logWithLevelAndServer helper in server_file_logger.go to consolidate dual logging (server-specific + unified) logic
• Extracted logWithMarkdownLevel helper in markdown_logger.go to reduce duplication in markdown logging functions
• Added comprehensive test coverage in helper_functions_test.go covering all helper functions, formatting, and concurrency

Reviewed changes

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

File	Description
internal/logger/file_logger.go	Introduces logWithLevel helper to eliminate duplicate mutex locking and nil-checking patterns across 4 logging functions (Info/Warn/Error/Debug)
internal/logger/server_file_logger.go	Introduces logWithLevelAndServer helper to consolidate dual logging logic (server-specific + unified) with switch-based level dispatching
internal/logger/markdown_logger.go	Introduces logWithMarkdownLevel helper to map log levels to their corresponding regular log functions, reducing duplication
internal/logger/helper_functions_test.go	Adds comprehensive test coverage for all three helper functions, including concurrency tests and formatting validation

---

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

[Copilot]

The logWithMarkdownLevel helper function doesn't handle the case where the log level doesn't match any of the expected values. If regularLogFunc remains nil after the switch statement, calling it on line 203 will cause a nil pointer panic. Consider adding a default case that either uses a fallback logging function or returns early to prevent potential panics. While this shouldn't happen in practice with the current usage, defensive programming would make this more robust.

Suggested change

	// If no matching log function was found for the provided level,
	// return early to avoid calling a nil function and causing a panic.
	if regularLogFunc == nil {
	return
	}

[Copilot]

The logWithLevelAndServer helper function doesn't handle the case where the log level doesn't match any of the expected values in the switch statement. If an unexpected log level is passed, the server-specific logging will still occur (line 169), but the unified logging will be silently skipped. Consider adding a default case that logs a warning or falls back to LogInfo to ensure logs are never silently dropped due to an unexpected log level value.

Suggested change

	LogDebug(category, "[%s] %s", serverID, formattedMessage)
	LogDebug(category, "[%s] %s", serverID, formattedMessage)
	default:
	// Fallback to a warning to avoid silently dropping unified logs on unexpected levels
	LogWarn(category, "[%s] %s (unexpected log level: %v)", serverID, formattedMessage, level)