Skip to content

refactor: semantic function clustering — TLS relocation, util consolidation, and documentation#9205

Open
lpcox with Copilot wants to merge 2 commits into
mainfrom
copilot/refactor-semantic-function-clustering
Open

refactor: semantic function clustering — TLS relocation, util consolidation, and documentation#9205
lpcox with Copilot wants to merge 2 commits into
mainfrom
copilot/refactor-semantic-function-clustering

Conversation

Copilot AI commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Six housekeeping items from the semantic function clustering analysis: misplaced TLS helper, scattered TLS documentation, undocumented ResponseWriter embed pattern, single-function util file, missing layering comment, and an unexported-but-relocatable atomic write helper.

Priority 1 — Move configureTLSTrustEnvironment to httputil

configureTLSTrustEnvironment and tlsTrustEnvKeys were buried in cmd/proxy.go despite being reusable TLS infrastructure. Now exported from internal/httputil:

// Before: private to cmd/proxy.go
configureTLSTrustEnvironment(tlsCfg.CACertPath)

// After: in httputil, callable from anywhere
httputil.ConfigureTLSTrustEnvironment(tlsCfg.CACertPath)
httputil.TLSTrustEnvKeys // ["NODE_EXTRA_CA_CERTS", "SSL_CERT_FILE", ...]

Priority 1 — Cross-package TLS comments

Both httputil/tls.go and proxy/tls.go now carry package-level doc comments explaining the split: httputil owns protocol-level TLS helpers (version policy, config constructors, trust env); proxy owns cert generation (GenerateSelfSignedTLS).

Priority 2 — Document statusResponseWriter as pure embed

tracing.statusResponseWriter is a struct with no methods of its own — all behaviour comes from httputil.BaseResponseWriter. The doc comment now makes this explicit and cross-references the body-buffering server.responseWriter variant.

Priority 3 — Consolidate util/session.go into util/format_duration.go

session.go contained exactly one function (FormatSessionIDForLog). Merged into format_duration.go (renamed to "String and time/duration formatting helpers") and deleted session.go + session_test.go.

Priority 3 — Layering comment in config/config_env.go

Added a package-level comment explaining why config_env.go wraps envutil rather than calling os.Getenv directly: envutil provides generic typed accessors; this file adds gateway-specific validation policy on top.

Priority 3 — Flag atomicWriteFile for future relocation

Added a TODO comment noting that logger.atomicWriteFile should be exported to internal/util or a new internal/fileutil package once a second consumer outside logger appears.

Copilot AI linked an issue Jul 12, 2026 that may be closed by this pull request
6 tasks
- Move configureTLSTrustEnvironment + tlsTrustEnvKeys from cmd/proxy.go
  to httputil/tls.go as ConfigureTLSTrustEnvironment + TLSTrustEnvKeys
- Add cross-package TLS comments in httputil/tls.go and proxy/tls.go
- Document statusResponseWriter in tracing/http.go as pure BaseResponseWriter embed
- Merge util/session.go (FormatSessionIDForLog) into util/format_duration.go,
  delete util/session.go and util/session_test.go
- Add layering comment to config/config_env.go explaining intentional envutil layering
- Flag atomicWriteFile in logger/fileutil.go as future export candidate
- Update cmd/proxy_test.go to use httputil.ConfigureTLSTrustEnvironment/TLSTrustEnvKeys
- Add tests for ConfigureTLSTrustEnvironment in httputil/tls_test.go
Copilot AI changed the title [WIP] Refactor semantic function clustering analysis refactor: semantic function clustering — TLS relocation, util consolidation, and documentation Jul 12, 2026
Copilot AI requested a review from lpcox July 12, 2026 23:40
Copilot finished work on behalf of lpcox July 12, 2026 23:40
@lpcox lpcox marked this pull request as ready for review July 13, 2026 00:41
Copilot AI review requested due to automatic review settings July 13, 2026 00:41

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors shared TLS and formatting utilities while improving architectural documentation.

Changes:

  • Relocates TLS trust environment configuration into httputil.
  • Consolidates session formatting and its tests.
  • Documents TLS ownership, response-writer embedding, environment layering, and future file utility extraction.
Show a summary per file
File Description
internal/util/session.go Removes consolidated helper file.
internal/util/session_test.go Removes consolidated test file.
internal/util/format_duration.go Adds session ID formatting helper.
internal/util/format_duration_test.go Moves session formatting tests.
internal/tracing/http.go Documents response-writer embedding.
internal/proxy/tls.go Documents TLS package responsibilities.
internal/logger/fileutil.go Notes potential atomic-write relocation.
internal/httputil/tls.go Adds shared TLS trust configuration.
internal/httputil/tls_test.go Tests TLS trust configuration.
internal/config/config_env.go Documents environment-helper layering.
internal/cmd/proxy.go Uses the relocated TLS helper.
internal/cmd/proxy_test.go Updates tests for the relocated helper.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 12/12 changed files
  • Comments generated: 3
  • Review effort level: Medium

Comment thread internal/httputil/tls.go
// HTTP/TLS clients (Node.js, curl, git, Python requests) to a CA bundle.
// ConfigureTLSTrustEnvironment sets all of them to the path of the generated
// CA certificate so that child processes automatically trust the self-signed cert.
var TLSTrustEnvKeys = []string{
Comment on lines +15 to +17
// New MCP_GATEWAY_* environment variables should be added here rather than
// in envutil, unless the accessor would be genuinely reusable outside the
// gateway (e.g., a generic typed getter with no domain knowledge).
}

err := configureTLSTrustEnvironment(caPath)
err := httputil.ConfigureTLSTrustEnvironment(caPath)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[refactor] Semantic Function Clustering Analysis

3 participants