[refactor] Semantic Function Clustering Analysis: Code Organization and Refactoring Opportunities

[github-actions]

Automated semantic analysis of 146 non-test Go source files across 24 packages (~934 functions total). The codebase is generally well-organized with good existing abstractions. This report covers actionable findings confirmed by re-analysis.

---

Full Report

Package Overview

Package	Files	Functions	Purpose
internal/config	16	126	Config loading, validation, expansion
internal/server	20	130	HTTP server, routing, session, guards
internal/logger	15	114	Multi-target structured logging
internal/difc	9	117	Information-flow control labels
internal/mcp	10	70	MCP protocol, connections, transport
internal/guard	11	75	WASM security guards, pipeline
internal/proxy	7	47	GitHub API filtering proxy
internal/launcher	4	39	Backend process management
internal/tracing	6	34	OpenTelemetry tracing
internal/strutil	5	17	Mixed utility functions

---

Finding 1 — PositiveInteger duplicates logic already in TimeoutMinimum

File: internal/config/validation_rules.go | Impact: Medium

TimeoutPositive correctly delegates its min-1 check to TimeoutMinimum, then overrides the suggestion. PositiveInteger re-implements the same < 1 guard independently:

// TimeoutPositive — correctly delegates:
func TimeoutPositive(timeout int, fieldName, jsonPath string) *ValidationError {
    if err := TimeoutMinimum(timeout, 1, fieldName, jsonPath); err != nil {
        err.Suggestion = "Use a positive number of seconds (e.g., 30)"
        return err
    }
    return nil
}

// PositiveInteger — duplicates the same check:
func PositiveInteger(value int, fieldName, jsonPath string) *ValidationError {
    logValidation.Printf(...)
    if value < 1 {   // same logic as TimeoutMinimum(value, 1, ...)
        return &ValidationError{...}
    }
    return nil
}

Recommendation: Refactor PositiveInteger to delegate to TimeoutMinimum and adjust the error message/suggestion, matching the pattern used by TimeoutPositive. The key difference is PositiveInteger uses "must be a positive integer (>= 1)" vs TimeoutMinimum's "must be at least N" — this can be preserved by overriding Message and Suggestion after the delegate call. Estimated effort: 30 min. Risk: very low.

---

Finding 2 — strutil package name doesn't match its contents

File: internal/strutil/ | Impact: Medium (naming/discoverability)

The package is named strutil (suggesting string utilities), but contains utilities across four distinct concerns:

File	Content	String-related?
truncate.go	Truncate, TruncateWithSuffix, TruncateRunes	✅
collections.go	DeduplicateStrings, StringsToAny, CopyTrimmedStringIntMap	✅
util.go	SortedSetKeys, GetStringFromMap, DeepCloneJSON, InterfaceToIntString	⚠️ Mixed
format_duration.go	FormatFutureTime, FormatDuration	⚠️ Time formatting
random.go	RandomBytes, RandomHex, RandomHexWithFallback	⚠️ Cryptographic randomness

DeepCloneJSON deep-clones map[string]interface{}/[]interface{} values (JSON-decoded Go types) — no string logic. FormatDuration formats time durations; RandomBytes/RandomHex produce cryptographic random values. Neither fits the strutil name.

Import sites: internal/auth/header.go, internal/middleware/jqschema.go, internal/tracing/provider.go, internal/sanitize/sanitize.go, internal/proxy/response_transform.go, internal/logger/logger.go, internal/server/circuit_breaker.go (~8 total).

Recommendation: Rename the package to util (or goutil) to reflect its nature as a general-purpose utility package. Estimated effort: 2 hours (rename + update ~8 import sites). Risk: low.

---

Finding 3 — Logger setup*/handle*Error named functions can be inlined

File: internal/logger/{file,markdown,jsonl,tools,observed_url_domains}_logger.go | Impact: Low

Five loggers each define two named functions (setupXLogger, handleXLoggerError) that are only ever referenced once — as values in their factory struct literal. Since loggerFactory[T] already abstracts initialization generically, naming these functions adds surface area without aiding readability.

// Current — 10 named functions (2 per logger × 5 loggers):
func setupFileLogger(file *os.File, logDir, fileName string) (*FileLogger, error) { ... }
func handleFileLoggerError(err error, logDir, fileName string) (*FileLogger, error) { ... }
var fileLoggerFactory = loggerFactory[*FileLogger]{setup: setupFileLogger, onError: handleFileLoggerError}

// Proposed — inline as anonymous functions:
var fileLoggerFactory = loggerFactory[*FileLogger]{
    setup:   func(file *os.File, logDir, fileName string) (*FileLogger, error) { ... },
    onError: func(err error, logDir, fileName string) (*FileLogger, error) { ... },
}

Estimated effort: 1–2 hours. Risk: very low.

---

Finding 4 — proxy/router.go map-key strings are unguarded literals

File: internal/proxy/router.go | Impact: Low

Six small functions (repoArgs, prArgs, issueArgs, repoMethodArgs, repoMethodResourceArgs, repoArgsExtractor) all build map[string]interface{} using the same string keys ("owner", "repo", "pullNumber", etc.) as bare literals repeated ~20 times. No typo protection and no single source of truth.

Recommendation: Define constants for shared map keys:

const (
    argOwner       = "owner"
    argRepo        = "repo"
    argPullNumber  = "pullNumber"
    argIssueNumber = "issue_number"
    argMethod      = "method"
    argResourceID  = "resource_id"
)

Estimated effort: 30 min. Risk: very low.

---

Well-organized patterns (no action needed)

• rejectAuthRequest/rejectHMACRequest → already delegate to rejectRequest → httputil.WriteErrorResponse. Clean layering.
• server/response_writer.go → properly embeds httputil.BaseResponseWriter, adds only body buffering.
• guard/wasm_validate.go:validateIntegrityField → correctly delegates to config.ValidateAndNormalizeIntegrityField.
• logger/global_state.go → withGlobalLogger[T] already eliminates ~40 lines of duplicated mutex locking (documented with before/after comments).
• server/rate_limit.go vs githubhttp/client.go → both parse rate-limit timestamps from different sources (MCP text vs HTTP header); cross-references already in comments.
• proxy/tls.go → delegates to httputil.NewServerTLSConfig, no duplication.
• ContextKey type in guard/context.go and mcp/connection.go → necessary because these packages cannot import each other (would create circular dependencies); this is idiomatic Go.
• RequiredStringField vs NonEmptyString → intentionally distinct: different error message wording ("is required" vs "cannot be empty") and suggestion mechanism (caller-provided vs auto-generated).

---

Implementation Checklist

• Finding 1 (Medium): PositiveInteger in internal/config/validation_rules.go — delegate to TimeoutMinimum
• Finding 2 (Medium): Rename internal/strutil package to util and update ~8 import sites
• Finding 3 (Low): Inline setup*/handle*Error functions in logger factory var declarations
• Finding 4 (Low): Add string constants for map keys in internal/proxy/router.go

Analysis Metadata

Files analyzed	146 non-test .go files in internal/
Functions catalogued	~934
Packages covered	24
Outlier functions (wrong file)	0
Duplicate implementations	1 (PositiveInteger vs TimeoutMinimum)
Analysis date	2026-07-02

Generated by Semantic Function Refactoring · §28622242922

References:

• §28622242922

Generated by Semantic Function Refactoring · 127.8 AIC · ⊞ 9.3K · ◷