-
Notifications
You must be signed in to change notification settings - Fork 3
Description
Analysis of repository: githubnext/gh-aw-mcpg - Workflow run §21333425601
Executive Summary
Analyzed 50 non-test Go files across 15 packages containing approximately 176 functions (87 exported, 89 private). The codebase demonstrates excellent organization with strong use of Go generics and clear separation of concerns. Identified 3 actionable refactoring opportunities that can be resolved in ~1-2 hours.
Key Findings:
- ✅ Well-organized areas: Logger package (exemplary generics usage), Config validation (clear separation), DIFC package (modular design)
⚠️ 1 critical duplicate: Shutdown middleware duplicated across 2 files (~95% identical)- 🔧 2 organizational improvements: Misplaced session utility, deprecated function still in use
Function Inventory Summary
By Package
| Package | Files | Functions | Primary Purpose |
|---|---|---|---|
internal/auth |
1 | 4 | Authentication header parsing |
internal/cmd |
2 | 11 | CLI commands (Cobra) |
internal/config |
5 | 35 | Configuration parsing & validation |
internal/difc |
5 | 30 | DIFC security labels |
internal/guard |
4 | 9 | Security guards |
internal/launcher |
2 | 5 | Backend process management |
internal/logger |
11 | 40 | Logging framework |
internal/mcp |
2 | 15 | MCP protocol types |
internal/middleware |
1 | 6 | Request middleware |
internal/server |
9 | 30 | HTTP server |
internal/sys |
1 | 1 | System utilities |
internal/testutil/mcptest |
4 | 4 | Testing utilities |
internal/timeutil |
1 | 1 | Time formatting |
internal/tty |
2 | 3 | TTY detection |
Total: 50 files, 176 functions
Semantic Clustering Results
Major Function Patterns Identified:
| Pattern | Count | Observation |
|---|---|---|
New* constructors |
20+ | Consistent constructor naming ✓ |
| Validation functions | 10 | Well-organized in config package ✓ |
Close() methods |
8 | Standard interface pattern ✓ |
| Middleware functions | 6 | In server package, some duplication |
| Parse functions | 3 | Appropriately distributed |
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 logger variable 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 (
logRoutedvslogTransport) - Both functions: Check shutdown state, return HTTP 503, use same error JSON
Recommendation:
- Consolidate into single function with logger parameter or use same logger
- Remove one function and update call sites
- Optional: Create
internal/server/middleware.gofor shared middleware functions
Implementation Approach:
// Unified version in routed.go or new middleware.go
func rejectIfShutdown(unifiedServer *UnifiedServer, next http.Handler, logPrefix string) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if unifiedServer.IsShutdown() {
// Use appropriate logger based on context
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)
})
}
Estimated Impact:
- Eliminates 13 lines of duplicate code
- Improves maintainability (single point of change)
- Effort: 30-45 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()
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
Recommendation:
- Move to
internal/auth/header.goas public functionTruncateSessionID(sessionID string) string - Update import in
sdk_logging.goto useauth.TruncateSessionID() - Make it exportable for reuse across packages
Implementation:
// In internal/auth/header.go
// 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 usage:
sessionID := auth.TruncateSessionID(auth.ExtractSessionID(authHeader))
Benefits:
- Centralizes 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()
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:
internal/server/sdk_logging.go:48-sessionID := extractSessionFromAuth(authHeader)internal/server/routed.go:112-sessionID := extractSessionFromAuth(authHeader)internal/server/transport.go:94-sessionID := extractSessionFromAuth(authHeader)
Recommendation:
- Replace all 3 usages with direct calls to
auth.ExtractSessionID(authHeader) - Remove deprecated wrapper function from
auth.go - Verify no other usages with:
grep -r "extractSessionFromAuth" internal/
Implementation:
// Replace in sdk_logging.go, routed.go, transport.go:
- sessionID := extractSessionFromAuth(authHeader)
+ sessionID := auth.ExtractSessionID(authHeader)
// Then delete the deprecated function from server/auth.go
Benefits:
- Removes technical debt
- Simplifies codebase (one less 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 ✅
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)
Organization: Well-structured with clear boundaries between general, environment, and schema validation ✅
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 identify duplication vs. appropriate interface usage:
Standard Go Interface Methods (Expected) ✅
| Method | Occurrences | Explanation |
|---|---|---|
Close() |
8 | Standard io.Closer interface - proper pattern |
New* |
20+ | Standard constructor pattern - proper pattern |
| HTTP handlers | 6 | Standard http.Handler interface - proper pattern |
Conclusion: These are appropriate uses of Go's type system and interface patterns, not problematic duplication.
Refactoring Recommendations
Priority 1: Quick Wins (1-2 hours total)
-
✅ Consolidate Duplicate Shutdown Middleware (30-45 min)
- Merge
rejectIfShutdownandrejectIfShutdownUnifiedinto single function - Update call sites in
routed.goandtransport.go - Optional: Create
internal/server/middleware.gofor middleware functions - Benefits: Eliminates 13 lines of duplicate code, easier maintenance
- Merge
-
✅ Move Session Truncation to Auth Package (15-20 min)
- Move
truncateSessiontointernal/auth/header.goasTruncateSessionID() - Update import in
sdk_logging.go - Benefits: Better organization, reusable utility
- Move
-
✅ Remove Deprecated Wrapper (10-15 min)
- Replace 3 usages of
extractSessionFromAuthwith directauth.ExtractSessionIDcalls - Remove deprecated function from
auth.go - Benefits: Removes technical debt, simplifies code
- Replace 3 usages of
Total Estimated Effort: 1-1.5 hours
Priority 2: Optional Enhancements (2-4 hours)
- Consider Creating Server Middleware File (optional)
- Extract middleware functions to
internal/server/middleware.go:authMiddlewarerejectIfShutdown(consolidated)withResponseLogging
- Benefits: Clearer organization, all middleware in one place
- Extract middleware functions to
Implementation Checklist
Priority 1 Tasks (Required)
-
Issue Configure as a Go CLI tool #1: Consolidate
rejectIfShutdownandrejectIfShutdownUnified- Create unified function
- Update
routed.gocall site - Update
transport.gocall site - Remove duplicate function
- Run
make testto verify
-
Issue Lpcox/initial implementation #2: Move
truncateSessionto auth package- Add
TruncateSessionID()tointernal/auth/header.go - Update usage in
sdk_logging.go - Remove old private function
- Run
make testto verify
- Add
-
Issue Lpcox/initial implementation #3: Remove deprecated
extractSessionFromAuth- 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 - Run
make testto verify
- Replace usage in
Verification
- Run full test suite:
make test-all - Verify no functionality broken
- Run linting:
make lint - Check formatting:
make format
Priority 2 Tasks (Optional)
- Consider creating
internal/server/middleware.go - Review server package organization for additional improvements
Analysis Metadata
- Repository: githubnext/gh-aw-mcpg
- Total Go Files Analyzed: 50 (excluding test files)
- Total Functions Cataloged: 176 (87 exported, 89 private)
- Duplicate Code Issues: 1 (shutdown middleware)
- Organizational Issues: 2 (session truncation placement, deprecated wrapper)
- Well-Organized Packages: Logger (excellent generics), Config (clear separation), DIFC (modular)
- Detection Method: Serena semantic analysis + static code analysis + manual review
- Analysis Date: 2026-01-25
- Overall Assessment: 🟢 Excellent - Minor improvements available, codebase is well-structured
- Estimated Total Refactoring Time: 1-2 hours 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)
- ✅ Appropriate interface patterns (Close, constructor methods)
The identified refactoring opportunities are minor organizational improvements rather than critical issues. The suggested changes will improve code maintainability and reduce duplication, but the code is already in good shape.
Recommendation: Implement Priority 1 items (1-2 hours effort) for immediate improvements. Priority 2 items are optional enhancements for future consideration.
References:
- Workflow Run: §21333425601
AI generated by Semantic Function Refactoring