-
Notifications
You must be signed in to change notification settings - Fork 15
Description
Overview
Analysis of 74 non-test Go source files (~541 functions) in internal/ confirms that internal/server/unified.go (1,679 lines, 49+ functions) continues to house several free functions whose natural home is another package. Additionally, internal/auth/header.go contains truncation logic that partially duplicates internal/strutil/truncate.go, and internal/dockerutil/ remains a single-function package that could be absorbed. No exact cross-package duplicates were found beyond these cases.
All findings below are verified against the current codebase (HEAD main branch) and represent genuinely actionable improvements — not style nits.
Full Report
Analysis Metadata
- Total Go files analyzed: 74 (excluding test files)
- Total functions cataloged: ~541 top-level functions and methods
- Files with most functions:
internal/server/unified.go(49),internal/guard/wasm.go(33),internal/difc/agent.go(27) - Outliers found: 7 free functions in wrong packages
- Near-duplicates found: 1 (
TruncateSessionIDvsstrutil.TruncateWithSuffix) - Thin single-function packages: 1 (
dockerutil) - Detection method: Naming-pattern analysis + cross-reference of package boundaries + usage tracing
Finding 1 — Policy-parsing free functions in server/unified.go (High Impact)
File: internal/server/unified.go:597, 1185, 1269, 1309
Four free (non-method) functions that parse and normalize guard policies live in the server package despite operating purely on config types and raw map[string]interface{} data:
| Function | Line | What it does |
|---|---|---|
hasServerGuardPolicies(cfg *config.Config) bool |
597 | Reports whether any server has per-server guard policies |
normalizeScopeKind(policy map[string]interface{}) |
1185 | Lowercases / trims the scope_kind field |
parseServerGuardPolicy(serverID string, raw map[string]interface{}) |
1269 | Resolves a guard policy from a raw JSON map |
parsePolicyMap(raw map[string]interface{}) |
1309 | Parses allow-only / write-sink policy structure |
These are called only from unified.go's guard-registration pipeline, but they import and return *config.GuardPolicy / *config.Config types and have no dependency on UnifiedServer state. Moving them to internal/config/guard_policy.go (alongside ParseGuardPolicyJSON and ValidateGuardPolicy) or a new internal/config/guard_policy_parse.go would:
- Co-locate all policy-parsing logic in one package
- Make
unified.gosolely responsible for orchestration - Allow these helpers to be reused by other consumers of
configwithout importingserver
Example of the coupling mismatch:
// internal/server/unified.go:1309 — parses config types, belongs in config package
func parsePolicyMap(raw map[string]interface{}) (*config.GuardPolicy, error) {
// ...calls config.ParseGuardPolicyJSON and config.ValidateGuardPolicy...
}Recommendation: Move parseServerGuardPolicy, parsePolicyMap, normalizeScopeKind, and hasServerGuardPolicies to internal/config/guard_policy.go. Export them (or keep as package-private) as appropriate for the config package's API surface.
Estimated effort: ~1 hour. No logic changes required.
Finding 2 — DIFC tag conversion utilities in server/unified.go (Medium Impact)
File: internal/server/unified.go:1166–1183
Two free functions convert between []string and []difc.Tag:
func toDIFCTags(values []string) []difc.Tag { ... } // line 1166
func tagsToStrings(tags []difc.Tag) []string { ... } // line 1177These operate exclusively on difc.Tag (a type alias for string) and have zero dependency on UnifiedServer or any server-package type. They belong in internal/difc/ as peer utilities alongside NewSecrecyLabel, NewIntegrityLabel, etc.
Currently both functions are package-private and used only in unified.go, but they would be natural exports from the difc package (e.g., difc.TagsFromStrings / difc.TagsToStrings).
Recommendation: Move to internal/difc/labels.go or a new internal/difc/convert.go, exporting them.
Estimated effort: ~30 min.
Finding 3 — findServerWASMGuardFile in server/unified.go (Medium Impact)
File: internal/server/unified.go:728–755
func findServerWASMGuardFile(serverID string) (string, bool, error) {
// scans MCP_GATEWAY_WASM_GUARDS_DIR/(serverID)/ for .wasm files
}This function performs filesystem discovery of WASM guard binaries. The internal/guard/ package already owns WASM guard creation (wasm.go) and the guard registry (registry.go). Guard file discovery is a guard concern, not a server concern.
Recommendation: Move to internal/guard/wasm.go or a new internal/guard/discovery.go.
Estimated effort: ~30 min.
Finding 4 — TruncateSessionID partially duplicates strutil.TruncateWithSuffix (Low Impact)
Files: internal/auth/header.go:172–182, internal/strutil/truncate.go:26–34
TruncateSessionID in the auth package implements its own 8-char truncation logic:
func TruncateSessionID(sessionID string) string {
if sessionID == "" { return "(none)" }
if len(sessionID) <= 8 { return sessionID }
return sessionID[:8] + "..."
}strutil.TruncateWithSuffix(s, 8, "...") already covers the truncation part. The only difference is the "(none)" empty-string sentinel, which is a logging presentation detail.
The function could be simplified to:
func TruncateSessionID(sessionID string) string {
if sessionID == "" { return "(none)" }
return strutil.TruncateWithSuffix(sessionID, 8, "...")
}This removes duplicated slicing logic and makes auth consistently use the strutil utility.
Recommendation: Refactor TruncateSessionID to delegate truncation to strutil.TruncateWithSuffix.
Estimated effort: ~10 min.
Finding 5 — internal/dockerutil/ is a single-function package (Low Impact)
File: internal/dockerutil/env.go
The entire dockerutil package contains one exported function:
func ExpandEnvArgs(args []string) []string { ... }It is imported only by internal/mcp/connection.go. The function expands Docker -e KEY style argument pairs from environment variables — which is an environment-variable utility concern, not a Docker-specific concern.
Options:
- Merge into
internal/envutil/asenvutil.ExpandDockerEnvArgs - Rename the package to
internal/launcher/dockerenv/to better signal its scope - Keep as-is if future Docker utilities are planned
Recommendation: Merge into internal/envutil/envutil.go since the function is purely about env-var expansion and has no Docker API dependencies.
Estimated effort: ~20 min.
Summary Table
| Finding | File | Functions | Package Mismatch | Priority |
|---|---|---|---|---|
| Policy-parsing free functions | server/unified.go |
parseServerGuardPolicy, parsePolicyMap, normalizeScopeKind, hasServerGuardPolicies |
Should be in config |
High |
| DIFC tag conversions | server/unified.go |
toDIFCTags, tagsToStrings |
Should be in difc |
Medium |
| WASM guard file discovery | server/unified.go |
findServerWASMGuardFile |
Should be in guard |
Medium |
| Truncation duplication | auth/header.go |
TruncateSessionID |
Should use strutil |
Low |
| Single-function package | dockerutil/env.go |
ExpandEnvArgs |
Should merge into envutil |
Low |
What Was NOT Flagged
- The split between
config/docker_helpers.go(config-time Docker validation) anddockerutil/env.go(runtime env expansion) is intentional and appropriate — different concerns. logger/rpc_helpers.go:truncateAndSanitizeis a private thin wrapper that is co-located with its callers — acceptable.mcp/connection.go:SetDIFCSinkServerIDs/isDIFCSinkServerID— global state management tied to the connection lifecycle. Moving is possible but lower value.- All
init()functions acrosscmd/flags_*.go— standard Go flag-registration pattern. - Free functions in
server/routed.go(rejectIfShutdown,newFilteredServerCache,CreateHTTPServerForRoutedMode,createFilteredServer) — these all operate on server types and belong in the server package.
Implementation Checklist
- Move
hasServerGuardPolicies,normalizeScopeKind,parseServerGuardPolicy,parsePolicyMapfromserver/unified.gotoconfig/guard_policy.go - Move
toDIFCTags,tagsToStringsfromserver/unified.gotodifc/labels.goordifc/convert.go(exported) - Move
findServerWASMGuardFilefromserver/unified.gotoguard/wasm.goorguard/discovery.go - Refactor
TruncateSessionIDinauth/header.goto delegate tostrutil.TruncateWithSuffix - Merge
dockerutil.ExpandEnvArgsintoenvutiland remove thedockerutilpackage - Update all import paths and callers after each move
- Run
make agent-finishedto verify no regressions
References:
Generated by Semantic Function Refactoring · ◷