Skip to content

refactor: relocate misplaced functions to their natural homes#5006

Merged
lpcox merged 5 commits into
mainfrom
copilot/refactor-session-helpers
May 2, 2026
Merged

refactor: relocate misplaced functions to their natural homes#5006
lpcox merged 5 commits into
mainfrom
copilot/refactor-session-helpers

Conversation

Copilot AI commented May 2, 2026

Copy link
Copy Markdown
Contributor

Four findings from semantic function clustering analysis: session lifecycle helpers in the wrong file, feature-specific constructors buried in unified.go, a zero-logic passthrough wrapper, and two tiny tracing files that should be one.

Changes

  • Finding 1 — Session helpers → session.go
    Moved extractAndValidateSession, injectSessionContext, setupSessionCallback, logHTTPRequestBody out of http_helpers.go into session.go. http_helpers.go shrinks ~100 lines; all session lifecycle logic is now in one file.

  • Finding 2 — Feature constructors → their owning files

    • buildCircuitBreakers: unified.gocircuit_breaker.go
    • buildAllowedToolSets + hasWildcard: unified.gotool_registry.go
      Added config import to both destination files.
  • Finding 3 — Remove writeErrorResponse wrapper
    Deleted the one-line passthrough and replaced all call sites with httputil.WriteErrorResponse directly:

    // before
    func writeErrorResponse(w http.ResponseWriter, statusCode int, code, message string) {
        httputil.WriteErrorResponse(w, statusCode, code, message)
    }
    
    // after — call sites use httputil directly
    httputil.WriteErrorResponse(w, status, code, msg)
  • Finding 4 — Merge tracing CLI files
    cmd/tracing_helpers.go + cmd/flags_tracing.go (75 lines combined) → cmd/tracing.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-build3872797525/b513/launcher.test /tmp/go-build3872797525/b513/launcher.test -test.testlogfile=/tmp/go-build3872797525/b513/testlog.txt -test.paniconexit0 -test.timeout=10m0s 8043�� olang.org/grpc@v1.80.0/internal/idle/idle.go cfg x_amd64/vet . --gdwarf2 --64 x_amd64/vet -I 8043057/b418/_pkg_.a -I x_amd64/vet --gdwarf-5 g/grpc/internal//usr/bin/runc -o x_amd64/vet (dns block)
    • Triggering command: /tmp/go-build3399420040/b509/launcher.test /tmp/go-build3399420040/b509/launcher.test -test.testlogfile=/tmp/go-build3399420040/b509/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true /tmp/go-build2509960937/b173/vet/run/containerd/io.containerd.runtime.v2.task/moby/db2aa2a60d29ftail 64/pkg/tool/linu-buildtags bash g_.a -trimpath "REQUESTS_CA_B--root /opt/hostedtoolc/var/run/docker/runtime-runc/moby -uns�� y x_amd64/vet e-handler @v1.43.0/propaga/usr/libexec/docker/cli-plugins/docker-compose @v1.43.0/propagadocker-cli-plugin-metadata x_amd64/vet e-handler (dns block)
  • invalid-host-that-does-not-exist-12345.com
    • Triggering command: /tmp/go-build3872797525/b495/config.test /tmp/go-build3872797525/b495/config.test -test.testlogfile=/tmp/go-build3872797525/b495/testlog.txt -test.paniconexit0 -test.timeout=10m0s -I g_.a NbAn/6jVna5QQyS18CrolNbAn x_amd64/vet --gdwarf-5 resolver/delegat/tmp/go-build2509960937/b300/vet.cfg -o x_amd64/vet 8043�� qzhGZjM5u cfg 64/pkg/tool/linux_amd64/vet (compatibility issue with Go 1.25.0). Continuing with other checks..."; \ elif command -v golan -imultiarch (dns block)
    • Triggering command: /tmp/go-build3399420040/b491/config.test /tmp/go-build3399420040/b491/config.test -test.testlogfile=/tmp/go-build3399420040/b491/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 88635552ec0a846ca0962f57bea95f938934c2423396c1fa129 x_amd64/compile ateway_with_pipe.sh cdc8770c07c851b4aae2b041e8136bb0e9e/log.json pc_health_v1/hea--prefix=/net/ipv4/conf/vethe00a9b0 x_amd64/compile ateway_with_pipe--prefix=/net/ipv6/conf/vethe00a9b0 -uns�� 3c2c91bb777a34fa x_amd64/compile by/db2aa2a60d29f83ac692768d36051d3e54950f8b5453b528136881dc8dce1cff/log.json g_.a -trimpath x_amd64/vet dCQFD9yY_0oeD/ar--prefix=/net/ipv6/conf/vethdaded9f (dns block)
  • nonexistent.local
    • Triggering command: /tmp/go-build3872797525/b513/launcher.test /tmp/go-build3872797525/b513/launcher.test -test.testlogfile=/tmp/go-build3872797525/b513/testlog.txt -test.paniconexit0 -test.timeout=10m0s 8043�� olang.org/grpc@v1.80.0/internal/idle/idle.go cfg x_amd64/vet . --gdwarf2 --64 x_amd64/vet -I 8043057/b418/_pkg_.a -I x_amd64/vet --gdwarf-5 g/grpc/internal//usr/bin/runc -o x_amd64/vet (dns block)
    • Triggering command: /tmp/go-build3399420040/b509/launcher.test /tmp/go-build3399420040/b509/launcher.test -test.testlogfile=/tmp/go-build3399420040/b509/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true /tmp/go-build2509960937/b173/vet/run/containerd/io.containerd.runtime.v2.task/moby/db2aa2a60d29ftail 64/pkg/tool/linu-buildtags bash g_.a -trimpath "REQUESTS_CA_B--root /opt/hostedtoolc/var/run/docker/runtime-runc/moby -uns�� y x_amd64/vet e-handler @v1.43.0/propaga/usr/libexec/docker/cli-plugins/docker-compose @v1.43.0/propagadocker-cli-plugin-metadata x_amd64/vet e-handler (dns block)
  • slow.example.com
    • Triggering command: /tmp/go-build3872797525/b513/launcher.test /tmp/go-build3872797525/b513/launcher.test -test.testlogfile=/tmp/go-build3872797525/b513/testlog.txt -test.paniconexit0 -test.timeout=10m0s 8043�� olang.org/grpc@v1.80.0/internal/idle/idle.go cfg x_amd64/vet . --gdwarf2 --64 x_amd64/vet -I 8043057/b418/_pkg_.a -I x_amd64/vet --gdwarf-5 g/grpc/internal//usr/bin/runc -o x_amd64/vet (dns block)
    • Triggering command: /tmp/go-build3399420040/b509/launcher.test /tmp/go-build3399420040/b509/launcher.test -test.testlogfile=/tmp/go-build3399420040/b509/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true /tmp/go-build2509960937/b173/vet/run/containerd/io.containerd.runtime.v2.task/moby/db2aa2a60d29ftail 64/pkg/tool/linu-buildtags bash g_.a -trimpath "REQUESTS_CA_B--root /opt/hostedtoolc/var/run/docker/runtime-runc/moby -uns�� y x_amd64/vet e-handler @v1.43.0/propaga/usr/libexec/docker/cli-plugins/docker-compose @v1.43.0/propagadocker-cli-plugin-metadata x_amd64/vet e-handler (dns block)
  • this-host-does-not-exist-12345.com
    • Triggering command: /tmp/go-build3872797525/b522/mcp.test /tmp/go-build3872797525/b522/mcp.test -test.testlogfile=/tmp/go-build3872797525/b522/testlog.txt -test.paniconexit0 -test.timeout=10m0s -I 8043057/b414/_pkg_.a -fPIC x_amd64/vet -pthread g/grpc/internal//usr/bin/runc -fmessage-length--version x_amd64/vet (dns block)
    • Triggering command: /tmp/go-build3399420040/b518/mcp.test /tmp/go-build3399420040/b518/mcp.test -test.testlogfile=/tmp/go-build3399420040/b518/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true -I by/eca09d10600f3--log-format ker/docker-init by/eca09d10600f3/usr/libexec/docker/cli-plugins/docker-compose b1a7041eaee94398docker-cli-plugin-metadata x_amd64/vet 91e/log.json /tmp�� tes.crt x_amd64/vet /usr/sbin/bash by/b7068f2c73a9cbash /interface.go x_amd64/vet bash (dns block)

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

Copilot AI and others added 2 commits May 2, 2026 15:28
- Move extractAndValidateSession, injectSessionContext, setupSessionCallback,
  logHTTPRequestBody from http_helpers.go to session.go (Finding 1)
- Move buildCircuitBreakers from unified.go to circuit_breaker.go (Finding 2)
- Move buildAllowedToolSets + hasWildcard from unified.go to tool_registry.go (Finding 2)
- Remove writeErrorResponse wrapper; replace call sites with httputil.WriteErrorResponse (Finding 3)
- Merge cmd/tracing_helpers.go + cmd/flags_tracing.go into cmd/tracing.go (Finding 4)

Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/22f5de96-f134-4f57-b6e9-419f884d918b

Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
@lpcox lpcox marked this pull request as ready for review May 2, 2026 15:29
Copilot AI review requested due to automatic review settings May 2, 2026 15:29
Copilot AI changed the title [WIP] Refactor misplaced session helpers in HTTP helpers file refactor: relocate misplaced functions to their natural homes May 2, 2026
Copilot finished work on behalf of lpcox May 2, 2026 15:29
Copilot AI requested a review from lpcox May 2, 2026 15:29

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 internal server and CLI code by relocating several helper functions into more appropriate files, reducing duplication and tightening package boundaries (notably around session handling, circuit breakers, allowed-tools filtering, and tracing flags).

Changes:

  • Moved session establishment helpers (extractAndValidateSession, injectSessionContext, setupSessionCallback, logHTTPRequestBody) from http_helpers.go to session.go.
  • Moved buildCircuitBreakers into circuit_breaker.go and allowed-tools helpers into tool_registry.go.
  • Removed the writeErrorResponse wrapper in favor of direct httputil.WriteErrorResponse usage and merged tracing flag/helper files into internal/cmd/tracing.go.
Show a summary per file
File Description
internal/server/unified.go Removes misplaced helper implementations now housed in dedicated files.
internal/server/tool_registry.go Hosts the allowed-tools set builder and wildcard helper used by unified server initialization/tests.
internal/server/session.go Centralizes session extraction/context injection and request body debug logging helpers.
internal/server/http_helpers_test.go Updates error response test to call httputil.WriteErrorResponse directly.
internal/server/http_helpers.go Removes wrapper and updates rejection path to use httputil.WriteErrorResponse.
internal/server/handlers.go Updates close-handler error path to call httputil.WriteErrorResponse directly.
internal/server/circuit_breaker.go Hosts the circuit breaker builder previously in unified.go.
internal/cmd/tracing.go Merges tracing flags + helpers into one file and registers flags via init().
internal/cmd/flags_tracing.go Deletes the old tracing flags file after consolidation.

Copilot's findings

Tip

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

  • Files reviewed: 9/9 changed files
  • Comments generated: 2

Comment thread internal/server/session.go Outdated
Comment thread internal/server/session.go Outdated
lpcox and others added 2 commits May 2, 2026 08:34
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
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 — Refactoring Opportunities

3 participants