[refactor] Semantic Function Clustering Analysis — Refactoring Opportunities

[github-actions]

🔧 Semantic Function Clustering Analysis

Analysis of repository: github/gh-aw-mcpg
Analysis run: 2026-07-29 | Workflow run §30491488955

---

Executive Summary

A full semantic analysis was performed on all 166 non-test Go source files across 26 packages, cataloguing 1,010 functions and methods. The codebase is generally well-structured with clear separation of concerns. However, several refactoring opportunities were identified:

1. Two parallel responseWriter implementations — httputil.BaseResponseWriter and server.responseWriter correctly use composition, but the server layer re-declares WriteHeader/Write wrappers that only add logging, creating an incomplete abstraction boundary.
2. Rate-limit parsing logic split across githubhttp and server — The detection pipeline spans two packages without a clear boundary contract.
3. internal/util contains heterogeneous concerns — format.go, netutil.go, toolname.go, json.go, truncate.go, collections.go, and random.go all live together; network utilities sit beside string truncation and JSON deep-clone.
4. internal/config/validation_shared.go is empty of functions — It only holds package-level vars; this is unusual for a file named _shared.
5. internal/server/difc_log.go is a server-layer file entirely dedicated to DIFC presentation logic — It could sit closer to the difc package, but its dependency on logger.FilteredItemLogEntry makes it a genuine presentation layer, not a true outlier.

---

Identified Issues

1. internal/util — Heterogeneous Utility File Scatter

Pattern: The util package mixes seven unrelated responsibilities into one flat directory with no sub-grouping. Each file is individually coherent, but the aggregate package is a catch-all that will grow unbounded.

File	Responsibility
format.go	String/time formatting for logging
truncate.go	String truncation utilities
collections.go	Generic collection helpers
random.go	Cryptographic random generation
json.go	JSON deep-clone
netutil.go	Network address normalization
toolname.go	MCP tool name parsing

Recommendation: The files themselves are well-written and individually small. No immediate merge is needed, but the package should be documented as a utility staging area with a policy that functions graduate to more specific packages when callers extend beyond one package. Specifically:

• netutil.go (ClientAddr, ClientHost) is only used by the server/cmd layer — consider internal/netutil or moving into internal/httputil.
• toolname.go (ParseServerIDFromToolName) encodes server tool-name protocol — consider moving to internal/mcp or internal/server where the separator constant is most relevant.
• json.go (DeepCloneJSON) is used in middleware/guard — consider moving to internal/middleware if callers narrow.

Estimated effort: Low — documentation + deferred migration on next related change.

---

2. Rate-Limit Detection Split Between githubhttp and server

Pattern: Rate-limit detection logic is divided between two packages without a clear contract:

internal/githubhttp/rate_limit.go
  - ParseRateLimitResetHeader(value string) time.Time
  - ParseRateLimitResetFromText(text string) time.Time
  - IsRateLimitText(text string) bool
  - RateLimitSignal(resp *http.Response) (bool, string, string)

internal/server/rate_limit.go
  - extractRateLimitErrorText(result interface{}) string
  - isRateLimitToolResult(result interface{}) (bool, time.Time)

server/rate_limit.go calls githubhttp.IsRateLimitText and githubhttp.ParseRateLimitResetFromText — which is fine. However, extractRateLimitErrorText extracts text from a raw interface{} tool result (a different input shape than an HTTP response), making it a distinct concern that belongs exclusively in the server circuit-breaker layer. The split is acceptable but the file names are misleading.

Recommendation: Rename internal/server/rate_limit.go to internal/server/circuit_breaker_rate_limit.go or move its two private functions directly into circuit_breaker.go, which is the only caller. This makes the ownership boundary explicit.

Estimated effort: 30 minutes — file rename or content merge.

---

3. internal/config/validation_shared.go — Empty Function File

Pattern: validation_shared.go contains zero functions — only package-level variable declarations (logValidation and customSchemaCache).

// validation_shared.go — current contents
package config

var logValidation = logger.ForFile()
var customSchemaCache sync.Map

Issue: A file named _shared.go with no functions creates a misleading navigation target. Developers opening this file expect shared helper functions, not just package-scoped state variables.

Recommendation: Either:

1. Rename to validation_state.go or validation_vars.go to signal it holds shared state, OR
2. Move the two vars into validation_rules.go or validation_server.go (whichever owns the most validation logic) and delete the file.

Estimated effort: 15 minutes — rename or inline.

---

4. internal/httputil/response_writer.go vs internal/server/response_writer.go

Pattern: The codebase deliberately uses httputil.BaseResponseWriter as an embedded type in server.responseWriter. This is good composition. However, the server layer re-declares WriteHeader and Write wrappers that only add logResponseWriter.Printf(...) calls:

// server/response_writer.go
func (w *responseWriter) WriteHeader(statusCode int) {
    logResponseWriter.Printf("Setting response status code: %d", statusCode)
    w.BaseResponseWriter.WriteHeader(statusCode)  // just calls through
}

func (w *responseWriter) Write(b []byte) (int, error) {
    logResponseWriter.Printf("Writing response body: %d bytes", len(b))
    w.body.Write(b)
    return w.BaseResponseWriter.Write(b)  // calls through after buffering
}

The Write override is necessary (adds body buffering). The WriteHeader override is only for a debug log line, which is unnecessary noise.

Recommendation: Remove the WriteHeader override from server.responseWriter — the base class already logs internally. This reduces the override surface area and makes the intent clearer (responseWriter adds body buffering, not status-code behavior).

Estimated effort: 5 minutes — delete 4 lines.

---

5. internal/mcp/connection_logging.go — Thin Wrapper File

Pattern: connection_logging.go contains 3 small private functions that wrap the logger package:

func logOutboundRPCRequest(serverID, method string, payload []byte, snapshot *AgentTagsSnapshot)
func logInboundRPCResponse(serverID string, payload []byte, err error, snapshot *AgentTagsSnapshot)
func logInboundRPCResponseFromResult(serverID string, result *Response, err error, snapshot *AgentTagsSnapshot) (*Response, error)

These are thin wrappers with 3-10 lines each. They exist in their own file but are only called from connection.go and connection_methods.go.

Recommendation: Merge into connection.go or connection_methods.go (whichever uses them most) to reduce file count in the mcp package. The mcp package currently has 11 files; consolidating logging helpers would reduce it to 10.

Estimated effort: 15 minutes — inline 3 small functions.

---

Well-Organized Patterns (No Action Needed)

The following patterns show exemplary organization worth noting:

Pattern	Location	Quality
TLS split: generation vs protocol/file-loading	proxy/tls.go + httputil/tls.go	✅ Clearly documented split with architectural rule comments
Validation split by concern	config/validation_*.go (7 files)	✅ Each file has a single coherent responsibility
Guard split by lifecycle	guard/wasm_lifecycle.go, wasm_exec.go, wasm_parse.go, etc.	✅ Clean functional decomposition
Sanitize vs Redact	sanitize/sanitize.go + sanitize/redact.go	✅ Pattern-based sanitization vs known-secret redaction are correctly separated
envutil.ExpandEnvArgs vs config.expandVariables	Different packages	✅ Comment in ExpandEnvArgs explicitly notes the behavioral difference (soft vs hard env expansion)

---

Function Clustering Summary

Cluster	Files	Functions	Organization Quality
Validation	config/validation_*.go (7 files)	~75	✅ Excellent — single-purpose files
Logging	logger/*.go (16 files)	~100	✅ Good — each logger type has its own file
Guard execution	guard/wasm_*.go (5 files)	~35	✅ Good — lifecycle/exec/parse/labels/payload separated
DIFC	difc/*.go (11 files)	~110	✅ Good — evaluator, labels, agent, pipeline, resource separated
Server	server/*.go (22 files)	~130	✅ Good — feature-per-file pattern
Config	config/*.go (20 files)	~140	⚠️ One empty function file (validation_shared.go)
Utility	util/*.go (7 files)	~35	⚠️ Mixed concerns — acceptable now, policy needed to prevent drift
MCP client	mcp/*.go (11 files)	~85	⚠️ connection_logging.go could be merged
Rate limit	server/rate_limit.go + githubhttp/rate_limit.go	6	⚠️ Boundary is acceptable but file naming could be clearer

---

Implementation Checklist

• P1 (15 min): Rename internal/config/validation_shared.go → validation_state.go
• P1 (5 min): Remove unnecessary WriteHeader override in internal/server/response_writer.go
• P2 (30 min): Rename internal/server/rate_limit.go → circuit_breaker_rate_limit.go or merge into circuit_breaker.go
• P2 (15 min): Merge internal/mcp/connection_logging.go into connection.go or connection_methods.go
• P3 (deferred): Add package doc to internal/util establishing graduation policy; track netutil.go and toolname.go for future migration

---

Analysis Metadata

Metric	Value
Total Go files analyzed	166
Total functions catalogued	1,010
Packages analyzed	26
Function clusters identified	9
True outliers found	0 (no functions clearly in wrong files)
Near-outliers / boundary issues	4
Exact code duplicates	0
Analysis method	Static grep-based function inventory + semantic naming pattern analysis

References:

• §30491488955

Generated by Semantic Function Refactoring · sonnet46 · 81.3 AIC · ⊞ 9.9K · ◷