[refactor] Semantic Function Clustering Analysis - Minor Code Organization Improvements #475

Analysis of repository: githubnext/gh-aw-mcpg

Executive Summary

Analyzed 48 non-test Go files across 14 packages containing approximately 362 functions/methods (both exported and unexported). The codebase demonstrates excellent organization with strong use of Go generics, clear separation of concerns, and well-structured packages. Identified 3 actionable refactoring opportunities that can improve code organization and reduce duplication.

Key Findings:

• ✅ Well-organized areas: Logger package (exemplary generics usage), Config package (clear validation separation), DIFC package (modular design)
• ⚠️ 1 critical duplicate: Shutdown middleware duplicated across 2 files (~95% identical, 13 duplicate lines)
• 🔧 2 organizational improvements: Session truncation utility in wrong package, deprecated function still in use (3 call sites)

Function Inventory Summary

By Package

Total: 48 files, 362 functions/methods

Semantic Clustering Results

Major Function Patterns Identified:

Identified Issues

1. 🔴 Duplicate Shutdown Middleware (Critical - High Impact)

Issue: Two nearly identical functions for shutdown rejection middleware

Occurrences:

• File 1: internal/server/routed.go:21-33 - rejectIfShutdown()
• File 2: internal/server/transport.go:18-30 - rejectIfShutdownUnified()

Code Comparison:

// routed.go:21
func rejectIfShutdown(unifiedServer *UnifiedServer, next http.Handler) http.Handler {
    return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
        if unifiedServer.IsShutdown() {
            logRouted.Printf("Rejecting request during shutdown: remote=%s, method=%s, path=%s", 
                r.RemoteAddr, r.Method, r.URL.Path)
            logger.LogWarn("shutdown", "Request rejected during shutdown, remote=%s, path=%s", 
                r.RemoteAddr, r.URL.Path)
            w.Header().Set("Content-Type", "application/json")
            w.WriteHeader(http.StatusServiceUnavailable)
            w.Write([]byte(shutdownErrorJSON))
            return
        }
        next.ServeHTTP(w, r)
    })
}

// transport.go:18 - NEARLY IDENTICAL (only debug logger differs: logTransport vs logRouted)
func rejectIfShutdownUnified(unifiedServer *UnifiedServer, next http.Handler) http.Handler {
    return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
        if unifiedServer.IsShutdown() {
            logTransport.Printf("Rejecting request during shutdown: remote=%s, method=%s, path=%s", 
                r.RemoteAddr, r.Method, r.URL.Path)
            logger.LogWarn("shutdown", "Request rejected during shutdown, remote=%s, path=%s", 
                r.RemoteAddr, r.URL.Path)
            w.Header().Set("Content-Type", "application/json")
            w.WriteHeader(http.StatusServiceUnavailable)
            w.Write([]byte(shutdownErrorJSON))
            return
        }
        next.ServeHTTP(w, r)
    })
}

Analysis:

• Similarity: ~95% identical (13 duplicate lines)
• Only difference: Debug logger variable name (logRouted vs logTransport)
• Both functions: Check shutdown state, return HTTP 503, use same error JSON
• Impact: Code duplication, double maintenance burden

Recommendation:

1. Consolidate into single function with logger namespace parameter
2. Remove one function and update call sites
3. Optional: Create internal/server/middleware.go for shared middleware

Implementation Approach:

// Option 1: Single function with logger parameter in routed.go
func rejectIfShutdown(unifiedServer *UnifiedServer, next http.Handler, logNamespace string) http.Handler {
    log := logger.New(logNamespace)
    return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
        if unifiedServer.IsShutdown() {
            log.Printf("Rejecting request during shutdown: remote=%s, method=%s, path=%s", 
                r.RemoteAddr, r.Method, r.URL.Path)
            logger.LogWarn("shutdown", "Request rejected during shutdown, remote=%s, path=%s", 
                r.RemoteAddr, r.URL.Path)
            w.Header().Set("Content-Type", "application/json")
            w.WriteHeader(http.StatusServiceUnavailable)
            w.Write([]byte(shutdownErrorJSON))
            return
        }
        next.ServeHTTP(w, r)
    })
}

// Update call sites:
// In routed.go: rejectIfShutdown(unifiedServer, handler, "server:routed")
// In transport.go: rejectIfShutdown(unifiedServer, handler, "server:transport")

Estimated Impact:

• Eliminates 13 lines of duplicate code
• Single point of maintenance for shutdown logic
• Effort: 20-30 minutes

2. 🟡 Session Truncation Utility in Wrong Package (Medium Priority)

Issue: Session ID truncation utility is in server package but should be in auth package

Occurrence:

• File: internal/server/sdk_logging.go:144-153
• Function: truncateSession()
• Current usage: Only used within sdk_logging.go (7 call sites)

Code:

// truncateSession returns a truncated session ID for logging (first 8 chars)
func truncateSession(s string) string {
    if s == "" {
        return "(none)"
    }
    if len(s) <= 8 {
        return s
    }
    return s[:8] + "..."
}

Problem:

• Session manipulation utility is in server/logging file
• Not reusable by other packages
• Auth package already contains session handling (ExtractSessionID, ExtractAgentID)
• Violates "one file per feature" principle - auth concerns in server package

Recommendation:

1. Move to internal/auth/header.go as public function TruncateSessionID(sessionID string) string
2. Update import in sdk_logging.go to use auth.TruncateSessionID()
3. Make it exportable for reuse across packages

Implementation:

// In internal/auth/header.go - add new exported function
// TruncateSessionID returns a truncated session ID for safe logging (first 8 chars).
// Returns "(none)" for empty session IDs, and appends "..." for truncated values.
func TruncateSessionID(sessionID string) string {
    if sessionID == "" {
        return "(none)"
    }
    if len(sessionID) <= 8 {
        return sessionID
    }
    return sessionID[:8] + "..."
}

// In internal/server/sdk_logging.go - update all 7 usages:
// Before: truncateSession(sessionID)
// After:  auth.TruncateSessionID(sessionID)

Benefits:

• Centralizes all session-related operations in auth package
• Makes utility discoverable and reusable
• Follows Go best practice of grouping related functionality
• Effort: 15-20 minutes

3. 🟢 Deprecated Function Still In Use (Low Priority - Technical Debt)

Issue: Function marked as deprecated but still actively used in 3 locations

Deprecated Function:

• File: internal/server/auth.go:45-53
• Function: extractSessionFromAuth()
• Status: Marked deprecated, delegating to auth.ExtractSessionID()

Code:

// extractSessionFromAuth extracts session ID from Authorization header.
// This function delegates to auth.ExtractSessionID for consistent session ID extraction.
// Per spec 7.1: When API key is configured, Authorization contains plain API key.
// When API key is not configured, supports Bearer token for backward compatibility.
//
// Deprecated: Use auth.ExtractSessionID directly instead.
func extractSessionFromAuth(authHeader string) string {
    return auth.ExtractSessionID(authHeader)
}

Current Usages:

1. internal/server/sdk_logging.go:48 - sessionID := extractSessionFromAuth(authHeader)
2. internal/server/routed.go:112 - sessionID := extractSessionFromAuth(authHeader)
3. internal/server/transport.go:94 - sessionID := extractSessionFromAuth(authHeader)

Recommendation:

1. Replace all 3 usages with direct calls to auth.ExtractSessionID(authHeader)
2. Remove deprecated wrapper function from auth.go
3. Verify no other usages exist

Implementation:

// Replace in sdk_logging.go:48, routed.go:112, transport.go:94:
- sessionID := extractSessionFromAuth(authHeader)
+ sessionID := auth.ExtractSessionID(authHeader)

// Then delete the deprecated function from internal/server/auth.go:45-53

Benefits:

• Removes technical debt
• Simplifies codebase (eliminates unnecessary indirection)
• Follows deprecation best practice
• Effort: 10-15 minutes

Well-Organized Areas ✅

Logger Package - Exemplary Use of Go Generics

Files: internal/logger/global_helpers.go, internal/logger/common.go

The logger package demonstrates excellent use of Go 1.18+ generics to eliminate code duplication:

// Generic helper for initializing any logger type
func initGlobalLogger[T closableLogger](mu *sync.RWMutex, current *T, newLogger T) {
    mu.Lock()
    defer mu.Unlock()
    if *current != nil {
        (*current).Close()
    }
    *current = newLogger
}

// Generic helper for closing any logger type
func closeGlobalLogger[T closableLogger](mu *sync.RWMutex, logger *T) error {
    mu.Lock()
    defer mu.Unlock()
    if *logger != nil {
        err := (*logger).Close()
        var zero T
        *logger = zero
        return err
    }
    return nil
}

// Constraint that works with FileLogger, JSONLLogger, MarkdownLogger
type closableLogger interface {
    *FileLogger | *JSONLLogger | *MarkdownLogger
    Close() error
}

Impact: Eliminates duplication across 3 logger types with type-safe generics ✅

Note: This is an excellent example of modern Go best practices and should serve as a model for similar patterns elsewhere in the codebase.

Config Package - Clear Separation of Concerns

Files:

• internal/config/validation.go - General validation logic (10 functions)
• internal/config/validation_env.go - Environment-specific validation (13 functions)
• internal/config/validation_schema.go - JSON schema validation (8 functions)
• internal/config/rules/rules.go - Validation error rules (8 functions)

Organization: Well-structured with clear boundaries between:

• General validation (server config, mounts, custom schemas)
• Environment validation (Docker, container detection, port mapping)
• Schema validation (JSON schema, string patterns)
• Error formatting (consistent error messages with JSON paths)

Impact: Clear file organization makes validation logic easy to find and maintain ✅

DIFC Package - Excellent Modular Design

Files:

• internal/difc/agent.go - Agent label management (4 functions)
• internal/difc/capabilities.go - Capability operations (1 function)
• internal/difc/evaluator.go - Flow evaluation (2 functions)
• internal/difc/labels.go - Label operations (5 functions)
• internal/difc/resource.go - Resource labeling (3 functions)

Organization: Clear separation by responsibility, each file has a single, well-defined purpose ✅

Analysis of Function Name Patterns

Analyzed common function name patterns to distinguish between appropriate interface usage vs. problematic duplication:

Standard Go Interface Methods (Expected) ✅

Conclusion: These are appropriate uses of Go's type system and interface patterns, not problematic duplication. The codebase correctly implements Go idioms.

Additional Observations

Positive Patterns Identified

1. Consistent Constructor Naming: All packages use New* pattern for constructors (25+ instances)
2. Proper Interface Implementation: Standard Go interfaces (Close(), http.Handler) correctly implemented
3. Clear Package Boundaries: Each package has a well-defined responsibility
4. Modern Go Features: Excellent use of generics in logger package
5. Test Coverage: Comprehensive test files for all packages

No Issues Found

• ❌ No scattered helper functions - utilities are properly centralized
• ❌ No misplaced validation logic - all validation in config package
• ❌ No format/parse duplication - each serves a specific purpose
• ❌ No generic opportunities missed - already using generics effectively in logger

Refactoring Recommendations

Priority 1: Quick Wins (45-70 minutes total)

1. ✅ Consolidate Duplicate Shutdown Middleware (20-30 min)

   • Merge rejectIfShutdown and rejectIfShutdownUnified into single function
   • Add logger namespace parameter to distinguish call sites
   • Update 2 call sites in routed.go and transport.go
   • Benefits: Eliminates 13 lines of duplicate code, single maintenance point
   • Risk: Low - pure refactoring with no logic changes

2. ✅ Move Session Truncation to Auth Package (15-20 min)

   • Move truncateSession to internal/auth/header.go as TruncateSessionID()
   • Update 7 call sites in sdk_logging.go
   • Add proper godoc comment
   • Benefits: Better organization, reusable utility, follows Go conventions
   • Risk: Low - simple function move

3. ✅ Remove Deprecated Wrapper (10-15 min)

   • Replace 3 usages of extractSessionFromAuth with direct auth.ExtractSessionID calls
   • Remove deprecated function from auth.go
   • Benefits: Removes technical debt, simplifies code
   • Risk: Low - already marked deprecated

Total Estimated Effort: 45-70 minutes

Priority 2: Optional Enhancements (Future Consideration)

4. Consider Creating Dedicated Middleware File (optional, 1-2 hours)
   • If more middleware functions are added in the future, consider creating internal/server/middleware.go
   • Would contain: rejectIfShutdown, authMiddleware, withResponseLogging
   • Benefits: Clearer organization, all middleware in one place
   • Note: Not necessary at current scale (only 3 middleware functions)

Implementation Checklist

Priority 1 Tasks (Recommended)

• Issue Configure as a Go CLI tool #1: Consolidate shutdown middleware

  • Create unified rejectIfShutdown function with logger parameter
  • Update call site in routed.go:CreateHTTPServerForRoutedMode
  • Update call site in transport.go:CreateHTTPServerForMCP
  • Remove duplicate rejectIfShutdownUnified function
  • Run make test-all to verify

• Issue Lpcox/initial implementation #2: Move session truncation to auth package

  • Add TruncateSessionID() to internal/auth/header.go
  • Update 7 usages in sdk_logging.go
  • Remove private truncateSession function
  • Run make test-all to verify

• Issue Lpcox/initial implementation #3: Remove deprecated wrapper

  • Replace usage in sdk_logging.go:48
  • Replace usage in routed.go:112
  • Replace usage in transport.go:94
  • Delete deprecated function from auth.go:45-53
  • Run make test-all to verify

Verification

• Run full test suite: make test-all
• Verify no functionality broken
• Run linting: make lint
• Check formatting: make format
• Final verification: make agent-finished

Priority 2 Tasks (Optional - Future)

• Monitor for additional middleware functions
• Consider creating middleware.go if 5+ middleware functions exist

Analysis Metadata

• Repository: githubnext/gh-aw-mcpg
• Total Go Files Analyzed: 48 (excluding test files)
• Total Functions Cataloged: 362 (both exported and unexported)
• Packages Analyzed: 14 (auth, cmd, config, difc, guard, launcher, logger, mcp, middleware, server, sys, testutil, timeutil, tty)
• Duplicate Code Issues: 1 (shutdown middleware - 13 duplicate lines)
• Organizational Issues: 2 (session truncation placement, deprecated wrapper usage)
• Well-Organized Packages: Logger (exemplary generics), Config (clear separation), DIFC (modular)
• Detection Method: Static code analysis + manual semantic review + function inventory
• Analysis Date: 2026-01-26
• Overall Assessment: 🟢 Excellent - Minor improvements available, codebase is well-structured
• Estimated Total Refactoring Time: 45-70 minutes for Priority 1 items

Conclusion

This codebase demonstrates strong engineering practices with:

• ✅ Excellent use of Go generics to eliminate duplication (logger package)
• ✅ Clear separation of concerns (config validation, DIFC security)
• ✅ Appropriate interface patterns (Close methods, constructors, HTTP handlers)
• ✅ Well-organized package structure (14 packages with clear responsibilities)
• ✅ Consistent naming conventions (New*, validate*, Log*)

The identified refactoring opportunities are minor organizational improvements rather than critical issues. All 3 issues are low-risk, high-value changes that can be completed in under an hour. The code is already in excellent shape.

Recommendation: Implement Priority 1 items (45-70 minutes total effort) for immediate code quality improvements. Priority 2 items are optional and only needed if the codebase grows significantly.

References:

• Workflow Run: §21359589137
• Configuration Reference: MCP Gateway Configuration

AI generated by Semantic Function Refactoring