[refactor] Semantic Function Clustering: Outliers and Organization Improvements

[github-actions]

🔧 Semantic Function Clustering Analysis

Analysis of repository: github/gh-aw-mcpg
Run: §28302052092

Executive Summary

Analyzed 149 non-test Go source files across 24 packages containing 935 functions and methods. Three significant findings were identified, ranging from file organization inconsistencies to package-level mixed concerns. No exact code duplicates were found; the codebase is generally well-structured, but specific areas show patterns where file layout contradicts naming conventions already established in the same package.

---

Identified Issues

1. strutil/strutil.go — Source-Test File Organization Inconsistency

Priority: Medium | Estimated effort: 1-2 hours

Context: The strutil package follows a consistent convention: each focused utility gets its own source file (e.g., truncate.go, random.go, format_duration.go, numbers.go). Each of these also has a matching *_test.go file.

Problem: strutil.go acts as a catch-all containing 6 functions that each already have dedicated test files — but no matching source file exists:

Function	Test File	Missing Source File
DeepCloneJSON	deepclone_test.go	deepclone.go ❌
CopyTrimmedStringIntMap	copy_trimmed_string_int_map_test.go	copy_trimmed_string_int_map.go ❌
DeduplicateStrings	deduplicate_test.go	deduplicate.go ❌
StringsToAny	strings_to_any_test.go	strings_to_any.go ❌
SortedSetKeys	util_test.go	util.go ❌
GetStringFromMap	util_test.go	util.go ❌

Highest-impact outlier — DeepCloneJSON:

• Operates on interface{} JSON values; it is not a string utility
• Already tested in deepclone_test.go, signalling intent to separate it
• Currently lives in strutil.go alongside genuinely string-oriented helpers

// internal/strutil/strutil.go (current — misplaced)
func DeepCloneJSON(v interface{}) interface{} { ... }

// Recommended: move to internal/strutil/deepclone.go

Recommendation:

1. Create internal/strutil/deepclone.go for DeepCloneJSON
2. Create internal/strutil/util.go for SortedSetKeys and GetStringFromMap
3. Optionally extract DeduplicateStrings → deduplicate.go, StringsToAny → strings_to_any.go, CopyTrimmedStringIntMap → copy_trimmed_string_int_map.go
4. Leave strutil.go as a package doc or remove it once all functions are moved

---

2. httputil Package — GitHub-Specific Code Mixed with Generic HTTP Utilities

Priority: Medium | Estimated effort: 2-4 hours

Context: httputil is a generic HTTP utilities package containing WriteJSONResponse, WriteErrorResponse, tls.go (TLS config helpers), and response_writer.go.

Problem: Two files with GitHub-specific domain logic are placed inside this generic package:

internal/httputil/collaborator.go (3 functions):

• ParseCollaboratorPermissionArgs — parses owner/repo/username from args for GitHub collaborator API
• LogAndWrapCollaboratorPermission — formats GitHub collaborator permission response for MCP
• FetchCollaboratorPermission — constructs /repos/{owner}/{repo}/collaborators/{user}/permission URL and calls GitHub API

internal/httputil/github_http.go (4 functions):

• ApplyGitHubAPIHeaders — sets Authorization, Accept: application/vnd.github+json headers
• DoGitHubGET — performs authenticated GitHub API GET
• ParseRateLimitResetHeader — parses X-RateLimit-Reset epoch header
• ComputeRetryAfter — calculates GitHub API retry delay

These 7 functions are GitHub API domain logic (construction of GitHub-specific paths, headers, rate-limit parsing). They are used by both internal/server and internal/proxy, which is why they were co-located in httputil as a shared package — but the semantic mismatch is clear.

Recommendation: Extract these into a dedicated internal/githubhttp/ package:

internal/githubhttp/
    collaborator.go   ← ParseCollaboratorPermissionArgs, FetchCollaboratorPermission, LogAndWrapCollaboratorPermission
    client.go         ← ApplyGitHubAPIHeaders, DoGitHubGET, ParseRateLimitResetHeader, ComputeRetryAfter

Both internal/server and internal/proxy would import githubhttp instead of httputil for these calls.

---

3. config/validation_rules.go — Composite Orchestration Function in an Atomic Rules File

Priority: Low | Estimated effort: 30 min

Context: validation_rules.go contains atomic, single-concern validation rule helpers: PortRange, TimeoutPositive, MountFormat, NonEmptyString, AbsolutePath, etc.

Problem: validateRuleBasedPatterns (line 225) is a composite orchestration function that:

• Iterates ALL server configurations in StdinConfig
• Calls validateGatewayConfig from validation.go
• Validates container image patterns, HTTP URL patterns, and gateway domain patterns

This violates the single-concern principle of the file. It is a mid-level validation orchestrator, not an atomic rule. Compare:

// validation_rules.go — atomic rule (correct placement)
func PortRange(port int, jsonPath string) *ValidationError { ... }

// validation_rules.go — orchestration function (MISPLACED)
func validateRuleBasedPatterns(stdinCfg *StdinConfig) error {
    for name, server := range stdinCfg.MCPServers { // iterates all servers
        ...
        if err := validateGatewayConfig(stdinCfg.Gateway); ...  // calls validation.go
    }
}

Recommendation: Move validateRuleBasedPatterns to validation.go where all other composite validation orchestrators live (validateGatewayConfig, validateGuardPolicies, validateStandardServerConfig, etc.).

---

4. logger/common.go — Non-Descriptive File Name (Minor)

Priority: Low | Estimated effort: 5 min

common.go is a known Go anti-pattern name (packages/files shouldn't need a "common" catch-all). The file actually contains well-documented, cohesive content: generic helpers for managing global logger state with mutex locking (withGlobalLogger, initGlobalLogger, closeGlobalLogger, withMutexLock, CloseAllLoggers, etc.).

Recommendation: Rename logger/common.go → logger/global_state.go or logger/logger_management.go to better communicate its purpose.

---

Function Inventory

By Package (non-test files)

Package	Files	Functions	Notes
server	20	130	Well-organized by feature
config	13	125	7 validation files, clear separation
logger	15	120	15 logger files, comprehensive
difc	11	117	DIFC label/eval logic
guard	9	75	WASM guards, pipeline
mcp	9	70	Protocol types
proxy	6	47	GitHub proxy + DIFC enforcement
launcher	4	39	Backend process management
cmd	10	38	CLI (Cobra)
tracing	6	34	OpenTelemetry
strutil	10	17	String utilities (see Issue #1)
httputil	5	17	HTTP utils (see Issue #2)

Notable Clusters (Well-Organized)

• Validation: config/validation*.go (6 files) cleanly separates concerns: rules, errors, schema, env, tracing, guard policies
• WASM Guard: guard/wasm*.go (4 files) cleanly separates: compilation, parsing, payload, validation
• Logging: logger/*.go (15 files) with specialized loggers per destination (file, server, JSONL, markdown)
• MCP Connection: mcp/connection*.go (3 files) cleanly separates: core state, methods, logging

---

Refactoring Checklist

• Issue Configure as a Go CLI tool #1 (Medium): Create internal/strutil/deepclone.go with DeepCloneJSON and internal/strutil/util.go with SortedSetKeys/GetStringFromMap — match source files to existing test files
• Issue Lpcox/initial implementation #2 (Medium): Create internal/githubhttp/ package and move collaborator.go + github_http.go out of httputil
• Issue Lpcox/initial implementation #3 (Low): Move validateRuleBasedPatterns from validation_rules.go to validation.go
• Issue Lpcox/add difc #4 (Low): Rename logger/common.go → logger/global_state.go

---

Analysis Metadata

• Total Go Files Analyzed: 149 (non-test)
• Total Functions Cataloged: 935
• Packages Analyzed: 24
• Function Clusters Identified: 12 major clusters
• Outliers Found: 3 significant, 1 minor
• Exact Duplicates Detected: 0
• Near-Duplicates: 0 (similar patterns like logWithLevel/logWithLevelAndServer are intentional)
• Detection Method: grep-based function extraction + naming pattern analysis + cross-file usage tracing
• Analysis Date: 2026-06-27

References:

• §28302052092

Generated by Semantic Function Refactoring · 655.8 AIC · ⊞ 36.6K · ◷