[refactor] Semantic Function Clustering Analysis: Refactoring Opportunities

[github-actions]

Overview

Analysis of 107 non-test Go files containing 729 functions across the internal/ directory. The codebase is well-organized overall, but there are a few concrete, actionable improvements identified through semantic clustering and pattern analysis.

Summary of findings:

• 2 medium-impact issues (duplicate logic, misplaced shared utility)
• 2 low-impact issues (unnecessary indirection, minor helper overlap)
• No critical structural problems detected

---

Identified Issues

1. 🔴 Duplicate getTracer() Method — Medium Impact

Issue: Identical method implemented independently in two separate packages with the same comment.

Location	Receiver	Comment
internal/proxy/handler.go:42	(h *proxyHandler)	"getTracer returns the cached tracer if set, otherwise falls back to the global tracer."
internal/server/unified.go:126	(us *UnifiedServer)	"getTracer returns the cached tracer if set, otherwise falls back to the global tracer."

Current duplicated code (identical in both):

// getTracer returns the cached tracer if set, otherwise falls back to the global tracer.
func (h *proxyHandler) getTracer() oteltrace.Tracer {
    if h.tracer != nil {
        return h.tracer
    }
    return tracing.Tracer()
}

Recommendation: Add a package-level helper to internal/tracing:

// GetCachedOrGlobal returns cached if non-nil, otherwise falls back to the global tracer.
func GetCachedOrGlobal(cached oteltrace.Tracer) oteltrace.Tracer {
    if cached != nil {
        return cached
    }
    return Tracer()
}

Both structs then become one-liners:

func (h *proxyHandler) getTracer() oteltrace.Tracer   { return tracing.GetCachedOrGlobal(h.tracer) }
func (us *UnifiedServer) getTracer() oteltrace.Tracer { return tracing.GetCachedOrGlobal(us.tracer) }

Estimated effort: 30 min
Benefits: Single source of truth, future-proof if caching logic ever changes

---

2. 🟡 logFuncs Map Defined in Wrong File — Medium Impact

Issue: logFuncs is a cross-cutting utility shared by three files (markdown_logger.go, server_file_logger.go, and file_logger.go). Its documentation lives in common.go but its definition lives in file_logger.go, creating a split between where the concept is explained and where it's defined.

• Defined: internal/logger/file_logger.go:141
• Documented: internal/logger/common.go:205–210
• Used: internal/logger/markdown_logger.go:187, internal/logger/server_file_logger.go:153

From common.go:

"The shared logFuncs map (file_logger.go) centralises the LogLevel → log-function mapping..."
"When adding a new LogLevel constant (e.g., LogLevelTrace): 1. Add a new entry to the logFuncs map in file_logger.go."

This means common.go documents a contract implemented elsewhere—a developer reading common.go needs to navigate to file_logger.go to make the required change.

Recommendation: Move the logFuncs map variable (currently file_logger.go:136–148) to common.go alongside its documentation. Update the common.go instruction to say "add a new entry to the logFuncs map below".

Estimated effort: 15 min
Benefits: Documentation and implementation co-located; reduced cognitive overhead when adding new log levels

---

3. 🟢 logWithMarkdown Unnecessary Indirection — Low Impact

Issue: logWithMarkdown in markdown_logger.go accepts a regularLogFunc callback parameter, but its only caller (logWithMarkdownLevel) always passes logFuncs[level]. The extra layer of indirection makes the call chain slightly harder to follow:

// logWithMarkdown - takes a func parameter
func logWithMarkdown(level LogLevel, regularLogFunc func(string, string, ...interface{}), ...) { ... }

// logWithMarkdownLevel - always passes logFuncs[level]
func logWithMarkdownLevel(level LogLevel, ...) {
    logWithMarkdown(level, logFuncs[level], ...)
}

Recommendation: Inline the logFuncs lookup into logWithMarkdown directly (removing the regularLogFunc parameter), eliminating logWithMarkdownLevel as a separate function:

func logWithMarkdown(level LogLevel, category, format string, args ...interface{}) {
    logFuncs[level](category, format, args...)
    withGlobalLogger(&globalMarkdownMu, &globalMarkdownLogger, func(logger *MarkdownLogger) {
        logger.Log(level, category, format, args...)
    })
}

Estimated effort: 20 min
Benefits: Simpler call chain, one fewer function, easier to understand

---

4. 🟢 marshalAndSanitizeForLog vs logger.LogMarshaledForDebug — Low Impact

Issue: Two helpers serve similar purposes but with different patterns:

Location	Function	Pattern
internal/server/tool_registry.go:352	marshalAndSanitizeForLog(value interface{}) string	Returns sanitized JSON string
internal/logger/marshal_debug.go	LogMarshaledForDebug(value, onSuccess, onFailure)	Callback-based, marshal errors go to onFailure

marshalAndSanitizeForLog silently ignores marshal errors (json.Marshal result discarded on error), while LogMarshaledForDebug explicitly handles them via callbacks.

Recommendation: Not a blocking refactor, but the marshalAndSanitizeForLog helper should either handle errors explicitly (like LogMarshaledForDebug) or add a comment explaining why silent error discard is intentional here.

Estimated effort: 10 min
Benefits: Consistent error handling philosophy

---

Analysis: Well-Organized Patterns (No Action Needed)

The following patterns were initially flagged but are intentionally designed and should not be changed:

• Parallel setup*/Init*/Close*Logger functions across 4 logger types: Already unified by initLogger[T] generic + closeGlobalLogger[T]. The common.go comment explicitly acknowledges why this duplication is acceptable.
• validateServerAuth → validateAuthConfig two-level call: validateServerAuth adds a server-type guard before delegating—a clean layered validation pattern.
• config_env.go vs internal/envutil/: Gateway-specific env parsing (GetGatewayPortFromEnv, etc.) belongs in the config package; generic env access belongs in envutil. Appropriate separation.
• filteredServerCache.getOrCreate vs syncutil.GetOrCreate: The server cache adds TTL, LRU eviction, and max-size capping—a substantially richer implementation than the generic helper.

---

Function Inventory

Package	Files	Functions	Notes
internal/logger	14	~100	Well-structured, intentional parallel patterns
internal/config	11	~80	Good validation layering
internal/difc	8	~60	Clear domain boundary
internal/server	17	~90	Large but well-decomposed by concern
internal/mcp	6	~50	Focused on protocol/connection
internal/proxy	6	~40	Clear HTTP proxy separation
internal/launcher	4	~35	Well-focused
Others	31	~175	Generally well-organized

---

Implementation Checklist

• Issue 1: Add tracing.GetCachedOrGlobal() helper and update both getTracer() methods
• Issue 2: Move logFuncs map from file_logger.go to common.go, co-locating with its documentation
• Issue 3: Inline logFuncs[level] into logWithMarkdown, remove logWithMarkdownLevel
• Issue 4: Add comment to marshalAndSanitizeForLog explaining silent error discard, or handle errors

---

Analysis Metadata

• Total Go Files Analyzed: 107 (excluding test files)
• Total Functions Cataloged: 729
• Packages Analyzed: 18
• Outliers Found: 1 (logFuncs placement)
• Duplicates Detected: 1 (identical getTracer() in two packages)
• Low-impact improvements: 2
• Detection Method: Function signature extraction + naming pattern clustering + implementation comparison
• Analysis Date: 2026-04-20
• Workflow Run: §24662472869

References:

• §24662472869

Generated by Semantic Function Refactoring · ● 1.5M · ◷