[refactor] Semantic Function Clustering: Outliers, Duplication, and Organization Improvements

[github-actions]

Analysis of repository: github/gh-aw-mcpg — §23388696494

Overview

Analysis of 84 non-test Go files (~572 functions) across internal/ confirmed six actionable refactoring issues carried over from prior analysis (#2242, now closed). The codebase is generally well-structured; these are targeted, low-effort improvements — the total estimated effort is under 90 minutes.

Full Report

Function Inventory Summary

Package	Files	Functions (approx)
internal/difc	8	~90
internal/config	10	~70
internal/logger	11	~65
internal/server	11	~65
internal/cmd	8	~40
internal/guard	6	~40
internal/mcp	6	~35
internal/proxy	5	~30
internal/launcher	3	~25
Other packages	16	~30
Root (main.go)	1	~5
Total	85	~572

---

Identified Issues

1. Redundant Private Wrapper Function

File: internal/mcp/connection.go:40–42

// REDUNDANT — unexported wrapper that only delegates to the exported function
func getAgentTagsSnapshotFromContext(ctx context.Context) (*AgentTagsSnapshot, bool) {
    return GetAgentTagsSnapshotFromContext(ctx)
}

This unexported function is called exactly once (line 296) and provides zero abstraction — it solely delegates to its exported counterpart GetAgentTagsSnapshotFromContext. The call site can directly call the exported function.

Recommendation: Remove the private wrapper. Update line 296 to call GetAgentTagsSnapshotFromContext directly.

Estimated effort: 5 minutes
Impact: Eliminates unnecessary indirection; removes confusion about why two functions share identical signatures.

---

2. Missing Named Constant for "backend-id" Context Key

File: internal/server/http_helpers.go:105

// INCONSISTENT — magic string cast, no named constant
ctx = context.WithValue(ctx, mcp.ContextKey("backend-id"), backendID)

The other context keys in the mcp package have proper named constants:

• mcp.SessionIDContextKey = "awmg-session-id" (mcp/connection.go:29)
• mcp.AgentTagsSnapshotContextKey = "awmg-agent-tags-snapshot" (mcp/connection.go:32)

But "backend-id" has no named constant and lacks the awmg- namespace prefix, creating inconsistency.

Recommendation: Add a named constant in internal/mcp/connection.go:

// BackendIDContextKey stores the routed server backend ID in context
const BackendIDContextKey ContextKey = "awmg-backend-id"

Then update http_helpers.go:105 to use mcp.BackendIDContextKey.

Estimated effort: 10 minutes
Impact: Consistent with the existing context key pattern; improves discoverability and searchability.

---

3. authMiddleware Bypasses auth.ValidateAPIKey()

File: internal/server/auth.go:57

The authMiddleware docstring explicitly references auth.ValidateAPIKey() as the validation function to use, but the implementation performs a raw string comparison:

// Documented as using auth.ValidateAPIKey() for key validation
if authHeader != apiKey {   // ← bypasses the dedicated function

The auth.ValidateAPIKey() function in internal/auth/header.go:97 encodes the "auth disabled when expected=empty" semantic. While applyAuthIfConfigured guards against an empty key, the raw comparison bypasses the package's intended API and creates an inconsistency between documentation and implementation.

Recommendation: Replace the direct comparison in authMiddleware:

if !auth.ValidateAPIKey(authHeader, apiKey) {
    // ... error handling
}

Estimated effort: 15 minutes
Impact: Aligns implementation with documentation; uses the auth package's intended public API.

---

4. RUNNING_IN_CONTAINER Env Var Not Checked in config/validation_env.go

Files:

• internal/tty/container.go:30 — checks RUNNING_IN_CONTAINER
• internal/config/validation_env.go — detectContainerized() does not check it

Check	tty/container.go	config/validation_env.go:detectContainerized()
/.dockerenv file	✓	✓
/proc/*/cgroup for docker/containerd	✓	✓
RUNNING_IN_CONTAINER env var	✓	✗ missing

RUNNING_IN_CONTAINER=true is documented as the override to force container detection when file-based signals are unavailable. However, detectContainerized() ignores this env var, so --validate-env fails to detect containerized execution in environments where the documented override is used.

Recommendation: Add the env var check to detectContainerized() in config/validation_env.go:

if os.Getenv("RUNNING_IN_CONTAINER") == "true" {
    return true, ""
}

Estimated effort: 20 minutes
Impact: Fixes a behavioral gap in --validate-env; aligns both container detectors.

---

5. FilteredCollectionLabeledData.FilterReason Field Is Never Read in Production Code

Files: internal/difc/resource.go:143 and internal/difc/evaluator.go:429

The FilteredCollectionLabeledData struct has a FilterReason field that is hardcoded to "DIFC policy" on construction:

// internal/difc/evaluator.go:429
filtered := &FilteredCollectionLabeledData{
    ...,
    FilterReason: "DIFC policy",  // ← always this string, never read in production
}

FilterReason is never accessed in any non-test production code. The per-item reason is already available via FilteredItemDetail.Reason (actively used in difc_log.go and buildDIFCFilteredNotice).

Recommendation: Either remove FilterReason from the struct or promote it to a typed constant:

// Option A: Remove unused field entirely
type FilteredCollectionLabeledData struct {
    Accessible []LabeledItem
    Filtered   []FilteredItemDetail
    TotalCount int
    // FilterReason removed
}

// Option B: Named constant (if collection-level reason is needed in future)
const filterReasonDIFCPolicy = "DIFC policy"

Estimated effort: 10 minutes
Impact: Removes dead code; eliminates the implicit contract that FilterReason must always be "DIFC policy".

---

6. Server Certificate Validity Extends Past CA Certificate Validity in TLS Generator

File: internal/proxy/tls.go:121–122

The CA certificate correctly reuses computed variables:

// Lines 75–76: variables computed once
notBefore := time.Now().Add(-1 * time.Hour)
notAfter := notBefore.Add(24 * time.Hour)  // = now + 23h

caTemplate := &x509.Certificate{
    ...,
    NotBefore: notBefore,  // now - 1h
    NotAfter:  notAfter,   // now + 23h  ← correct
}

But the server certificate template repeats time.Now() inline instead of reusing the variables:

serverTemplate := &x509.Certificate{
    ...,
    NotBefore: time.Now().Add(-1 * time.Hour),  // ← separate time.Now() call
    NotAfter:  time.Now().Add(24 * time.Hour),  // = now + 24h ← OUTLIVES CA
}

Problem: The server cert's NotAfter is now + 24h, while the CA cert's NotAfter is now + 23h. The server certificate is valid for ~1 hour after its signing CA expires. Per RFC 5280 §4.1.2.5, a leaf cert's validity period should not extend beyond the CA's validity period — strict TLS verifiers may reject such a chain.

Recommendation: Reuse the pre-computed variables:

serverTemplate := &x509.Certificate{
    ...,
    NotBefore: notBefore,  // reuse variable — now - 1h
    NotAfter:  notAfter,   // reuse variable — now + 23h, same as CA
}

Estimated effort: 5 minutes
Impact: Fixes an X.509 correctness issue; server and CA certificates share the same validity window.

---

Semantic Clusters (Well-Organized Areas)

The following areas require no changes:

✓ proxy/ package — proxy.go, handler.go, router.go, graphql.go, tls.go each have clear single responsibilities with consistent pkg:filename logger naming.

✓ difc_log.go — logFilteredItems, buildDIFCFilteredNotice and helpers are cleanly self-contained.

✓ String utilities — internal/strutil/truncate.go and deduplicate.go are clean central locations; all consumers correctly delegate to them.

✓ Config validation split — validation.go, validation_env.go, validation_schema.go have clear single responsibilities.

✓ Logger generics — internal/logger/global_helpers.go uses Go generics to avoid duplication across logger types.

✓ config/rules/ — Validation error constructors are cleanly centralized.

---

Refactoring Recommendations

Priority 1 — Correctness fix (5 min)

1. Fix server cert validity window in proxy/tls.go:121–122: replace inline time.Now() calls with notBefore/notAfter variables.

Priority 2 — Low effort, high clarity (< 30 min total)

2. Remove getAgentTagsSnapshotFromContext private wrapper (mcp/connection.go:40–42); update call site at line 296.
3. Add BackendIDContextKey constant to mcp/connection.go; update server/http_helpers.go:105.
4. Use auth.ValidateAPIKey() in authMiddleware instead of raw authHeader != apiKey.

Priority 3 — Behavioral fix and dead code (< 30 min total)

5. Add RUNNING_IN_CONTAINER env var check to config/validation_env.go:detectContainerized().
6. Remove or promote FilterReason field from difc/resource.go (update evaluator.go accordingly).

---

Implementation Checklist

• Fix server cert NotBefore/NotAfter in proxy/tls.go:121–122 to reuse notBefore/notAfter variables
• Remove getAgentTagsSnapshotFromContext private wrapper (mcp/connection.go:40–42); update call at line 296
• Add BackendIDContextKey constant to mcp/connection.go; update server/http_helpers.go:105
• Replace authHeader != apiKey with !auth.ValidateAPIKey(authHeader, apiKey) in server/auth.go
• Add RUNNING_IN_CONTAINER env var check in config/validation_env.go:detectContainerized()
• Remove or promote FilterReason field in difc/resource.go (update evaluator.go)
• Run make agent-finished to verify all changes pass lint, build, and tests

---

Analysis Metadata

• Total Go files analyzed: 84 (excluding _test.go)
• Total functions cataloged: ~572
• Issues found: 6 (all carried over — no regressions, no new issues in latest commit)
• Detection method: Naming pattern analysis + cross-package function comparison + implementation review
• Analysis date: 2026-03-21

References:

• §23388696494
• #2242 (previous run, now closed)

Generated by Semantic Function Refactoring · ◷