Skip to content

refactor: eliminate three near-duplicate/outlier functions flagged by semantic analysis#3897

Merged
lpcox merged 2 commits intomainfrom
copilot/refactor-semantic-function-clustering
Apr 15, 2026
Merged

refactor: eliminate three near-duplicate/outlier functions flagged by semantic analysis#3897
lpcox merged 2 commits intomainfrom
copilot/refactor-semantic-function-clustering

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 15, 2026

Three recurring code-quality findings from semantic function clustering analysis: a near-duplicate pair across server/proxy, a misplaced config validator, and a trivial one-liner wrapper.

Changes

  • internal/httputil — add shared ParseRateLimitResetHeader(value string) time.Time with strings.TrimSpace normalization, replacing two near-identical private implementations:

    • parseRateLimitResetHeader in internal/server/circuit_breaker.go (had TrimSpace)
    • parseRateLimitReset in internal/proxy/handler.go (lacked TrimSpace)
  • internal/config/validation.go — move validateGuardPolicies here from guard_policy.go; it takes *Config and belongs alongside validateGatewayConfig, validateTrustedBots, validateOpenTelemetryConfig, etc. Replace logGuardPolicylogValidation.

  • internal/proxy/graphql.go — remove truncateForLog (a one-line pass-through to strutil.TruncateRunes); inline strutil.TruncateRunes(s, 80) directly at the two call sites, consistent with the rest of the codebase.

Test changes

  • Added TestParseRateLimitResetHeader to httputil_test.go (includes whitespace-trimming case).
  • Removed TestParseRateLimitResetHeader from circuit_breaker_test.go and TestParseRateLimitReset from rate_limit_test.go — now covered by the shared test.
  • Removed graphql_test.go (only contained TestTruncateForLog; strutil.TruncateRunes is already tested in strutil/truncate_test.go).

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • example.com
    • Triggering command: /tmp/go-build907643046/b514/launcher.test /tmp/go-build907643046/b514/launcher.test -test.testlogfile=/tmp/go-build907643046/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s .cfg�� 6809213/b405/_pk-errorsas 0L32/Gn657Kg8UZO-ifaceassert x_amd64/vet --gdwarf-5 /http2/hpack -o x_amd64/vet .cfg�� lete: awmg" ache/go/1.25.8/x64/src/crypto/tls/alert.go x_amd64/vet . --gdwarf2 --64 x_amd64/vet (dns block)
  • invalid-host-that-does-not-exist-12345.com
    • Triggering command: /tmp/go-build907643046/b496/config.test /tmp/go-build907643046/b496/config.test -test.testlogfile=/tmp/go-build907643046/b496/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build907643046/b395/vet.cfg /testutil/mcptesgo1.25.8 /testutil/mcptes-c=4 x_amd64/vet 6809213/b151/ -imultiarch x86_64-linux-gnu-bool x_amd64/vet 6809�� g_.a olang.org/grpc@v-ifaceassert 64/pkg/tool/linu-nilfunc . ce/tracetest --64 64/pkg/tool/linu-buildtags (dns block)
  • nonexistent.local
    • Triggering command: /tmp/go-build907643046/b514/launcher.test /tmp/go-build907643046/b514/launcher.test -test.testlogfile=/tmp/go-build907643046/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s .cfg�� 6809213/b405/_pk-errorsas 0L32/Gn657Kg8UZO-ifaceassert x_amd64/vet --gdwarf-5 /http2/hpack -o x_amd64/vet .cfg�� lete: awmg" ache/go/1.25.8/x64/src/crypto/tls/alert.go x_amd64/vet . --gdwarf2 --64 x_amd64/vet (dns block)
  • slow.example.com
    • Triggering command: /tmp/go-build907643046/b514/launcher.test /tmp/go-build907643046/b514/launcher.test -test.testlogfile=/tmp/go-build907643046/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s .cfg�� 6809213/b405/_pk-errorsas 0L32/Gn657Kg8UZO-ifaceassert x_amd64/vet --gdwarf-5 /http2/hpack -o x_amd64/vet .cfg�� lete: awmg" ache/go/1.25.8/x64/src/crypto/tls/alert.go x_amd64/vet . --gdwarf2 --64 x_amd64/vet (dns block)
  • this-host-does-not-exist-12345.com
    • Triggering command: /tmp/go-build907643046/b523/mcp.test /tmp/go-build907643046/b523/mcp.test -test.testlogfile=/tmp/go-build907643046/b523/testlog.txt -test.paniconexit0 -test.timeout=10m0s .cfg�� FM1m/LEd7bURSK2u2w4dcFM1m /tmp/go-build1626809213/b288/ x_amd64/vet � Warning: golanrunc --gdwarf2 --64 x_amd64/vet .cfg�� 6184859/b100/vet.cfg -trimpath x_amd64/vet lock.go lock_sort.go -lang=go1.19 x_amd64/vet (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

…ean up outlier functions

- Add shared httputil.ParseRateLimitResetHeader (with TrimSpace); remove
  duplicate parseRateLimitResetHeader (server) and parseRateLimitReset (proxy)
- Move validateGuardPolicies from config/guard_policy.go to config/validation.go
  alongside all other top-level config validators; replace logGuardPolicy with logValidation
- Remove trivial truncateForLog wrapper in proxy/graphql.go; inline
  strutil.TruncateRunes directly at the two call sites
- Update/remove tests accordingly

Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/2e461a0f-7bcc-4bd7-b521-c39a4af9ad09

Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Copilot AI changed the title [WIP] Refactor semantic function clustering analysis for outliers refactor: eliminate three near-duplicate/outlier functions flagged by semantic analysis Apr 15, 2026
Copilot AI requested a review from lpcox April 15, 2026 18:10
@lpcox lpcox marked this pull request as ready for review April 15, 2026 18:17
Copilot AI review requested due to automatic review settings April 15, 2026 18:17
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Refactors a few small utilities to reduce duplication and keep validation/helpers in more appropriate packages, based on semantic analysis findings.

Changes:

  • Added internal/httputil.ParseRateLimitResetHeader and updated proxy rate-limit handling to use it.
  • Moved validateGuardPolicies into internal/config/validation.go to align with other config validation helpers.
  • Inlined strutil.TruncateRunes(..., 80) at GraphQL logging call sites and removed the now-redundant wrapper + its dedicated test file.
Show a summary per file
File Description
internal/server/circuit_breaker_test.go Removes the server-local header parsing test now covered by shared httputil tests.
internal/server/circuit_breaker.go Deletes an unused/duplicated parseRateLimitResetHeader helper.
internal/proxy/rate_limit_test.go Removes proxy-local header parsing test now covered by shared httputil tests.
internal/proxy/handler.go Switches rate-limit reset parsing to httputil.ParseRateLimitResetHeader.
internal/proxy/graphql_test.go Removes a test file that only exercised a trivial wrapper.
internal/proxy/graphql.go Inlines strutil.TruncateRunes and removes truncateForLog.
internal/httputil/httputil_test.go Adds shared tests for the new reset-header parsing helper (including whitespace trimming).
internal/httputil/httputil.go Introduces shared ParseRateLimitResetHeader helper.
internal/config/validation.go Adds validateGuardPolicies alongside other config validation routines.
internal/config/guard_policy.go Removes validateGuardPolicies from guard policy implementation file.

Copilot's findings

Tip

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

  • Files reviewed: 10/10 changed files
  • Comments generated: 0

@lpcox lpcox merged commit 40d8648 into main Apr 15, 2026
17 checks passed
@lpcox lpcox deleted the copilot/refactor-semantic-function-clustering branch April 15, 2026 18:26
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: Two Persistent Outliers + One New Near-Duplicate Pair

3 participants