Semantic refactoring: eliminate code duplication and fix misplaced functions (8 findings)

[Copilot]

Automated semantic analysis identified 8 actionable code organization issues across internal/. This PR resolves all of them.

P1 — Quick Wins

• Remove duplicate JSON-RPC types (server/sdk_logging.go): JSONRPCRequest, JSONRPCResponse, JSONRPCError were structurally identical to mcp.Request, mcp.Response, mcp.ResponseError. Replaced with the canonical mcp types and merged sdk_logging.go into http_helpers.go (now a single-function file after the type removal).

• Relocate isEffectivelyEmpty (logger/rpc_helpers.go → logger/rpc_formatter.go): Function had one caller in rpc_formatter.go; moved to co-locate it.

• Consistent duration formatting (server/circuit_breaker.go): formatResetAt used raw time.Duration.String(); replaced with strutil.FormatDuration to match the rest of the codebase.

• Fix stdlib log leakage (proxy/proxy.go): Two log.Printf calls bypassed the project's structured logProxy logger and DEBUG=proxy:* filtering. Replaced and removed the "log" import.

P2 — Medium Effort

• Relocate Docker helpers (config/docker_helpers.go → sys/docker.go): Six runtime Docker inspection functions (CheckDockerAccessible, ValidateContainerID, CheckPortMapping, CheckStdinInteractive, CheckLogDirMounted, runDockerInspect) belonged in internal/sys alongside container detection, not in the config parser. Exported the functions, updated callers in config/validation_env.go, and migrated tests to sys/docker_test.go.

• Promote sessionSuffix to strutil: launcher/log_helpers.go defined a private sessionSuffix helper; mcp/errors.go inlined the same logic. Extracted as strutil.SessionSuffix and updated both callers.

• Deduplicate OTel HTTP wrapping (server.WithOTELTracing): ~80% of its implementation duplicated tracing.WrapHTTPHandler. Refactored to delegate W3C extraction and span creation to WrapHTTPHandler, with a post-handler closure that enriches the span with session ID:

func WithOTELTracing(next http.Handler, tag string) http.Handler {
    enriched := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
        next.ServeHTTP(w, r)
        // r.Context() reflects session ID after setupSessionCallback mutates *r in-place
        span := oteltrace.SpanFromContext(r.Context())
        span.SetAttributes(attribute.String("session.id", auth.TruncateSessionID(SessionIDFromContext(r.Context()))))
    })
    return tracing.WrapHTTPHandler(enriched, "gateway.request", attribute.String("gateway.tag", tag))
}

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-build227273503/b509/launcher.test /tmp/go-build227273503/b509/launcher.test -test.testlogfile=/tmp/go-build227273503/b509/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true ect/protoreflect/methods.go ect/protoreflect/proto.go x_amd64/compile (dns block)
  • Triggering command: /tmp/go-build2433055712/b491/launcher.test /tmp/go-build2433055712/b491/launcher.test -test.testlogfile=/tmp/go-build2433055712/b491/testlog.txt -test.paniconexit0 -test.timeout=10m0s ortc�� g_.a 64/src/net/http/internal/httpcom-ifaceassert x_amd64/vet --gdwarf-5 ls -o x_amd64/vet -W -L1q213H7 /tmp/go-build2392850310/b151/ x_amd64/vet . telabs/wazero/indocker-cli-plugin-metadata --64 x_amd64/vet (dns block)
• invalid-host-that-does-not-exist-12345.com
  • Triggering command: /tmp/go-build2433055712/b224/config.test /tmp/go-build2433055712/b224/config.test -test.testlogfile=/tmp/go-build2433055712/b224/testlog.txt -test.paniconexit0 -test.timeout=10m0s ortc�� /tmp/go-build239-errorsas 64/src/vendor/go-ifaceassert x_amd64/vet -p crypto/internal/-o -lang=go1.25 x_amd64/vet 2850�� 1.80.0/internal/-p -I x_amd64/vet 64/src/os/user/gsleep --64 -o x_amd64/vet (dns block)
  • Triggering command: /tmp/go-build1042049869/b491/config.test /tmp/go-build1042049869/b491/config.test -test.testlogfile=/tmp/go-build1042049869/b491/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true -guard/target/de--irreversible-delete -guard/target/de-U0 -guard/target/de449d321381cc393f6a76bce0f87da0469da1f43b -guard/target/degit -guard/target/deshow -guard/target/de449d321381cc393f6a76bce0f87da0469da1f43b:internal/server/http_helpers.go -guard/target/debug/deps/github_guard-57d41235e07a5585.1yg4dgf4ofc88gtczrpthgg1u.035gz5i.rcgu.o -gua�� elpers.go -guard/target/debug/deps/github_guard-57d41235e07a5585.2z8afzdm9zucrirrh7hnf4z1l.035gz5i.rcgu.o -guard/target/debug/deps/github_guard-57d41235e07a5585.34w8f3apoo4qlefxtp7qruodt.035gz5i.rcgu.o -guard/target/de/bin/sh -guard/target/de-c -guard/target/deecho " test-rust - Run Rust guard unit tests (requires cargo)" -guard/target/debug/deps/github_guard-57d41235e07a5585.3o0f2kbgbq7jvxzdnkys32nfz.035gz5i.rcgu.o (dns block)
• nonexistent.local
  • Triggering command: /tmp/go-build227273503/b509/launcher.test /tmp/go-build227273503/b509/launcher.test -test.testlogfile=/tmp/go-build227273503/b509/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true ect/protoreflect/methods.go ect/protoreflect/proto.go x_amd64/compile (dns block)
  • Triggering command: /tmp/go-build2433055712/b491/launcher.test /tmp/go-build2433055712/b491/launcher.test -test.testlogfile=/tmp/go-build2433055712/b491/testlog.txt -test.paniconexit0 -test.timeout=10m0s ortc�� g_.a 64/src/net/http/internal/httpcom-ifaceassert x_amd64/vet --gdwarf-5 ls -o x_amd64/vet -W -L1q213H7 /tmp/go-build2392850310/b151/ x_amd64/vet . telabs/wazero/indocker-cli-plugin-metadata --64 x_amd64/vet (dns block)
• slow.example.com
  • Triggering command: /tmp/go-build227273503/b509/launcher.test /tmp/go-build227273503/b509/launcher.test -test.testlogfile=/tmp/go-build227273503/b509/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true ect/protoreflect/methods.go ect/protoreflect/proto.go x_amd64/compile (dns block)
  • Triggering command: /tmp/go-build2433055712/b491/launcher.test /tmp/go-build2433055712/b491/launcher.test -test.testlogfile=/tmp/go-build2433055712/b491/testlog.txt -test.paniconexit0 -test.timeout=10m0s ortc�� g_.a 64/src/net/http/internal/httpcom-ifaceassert x_amd64/vet --gdwarf-5 ls -o x_amd64/vet -W -L1q213H7 /tmp/go-build2392850310/b151/ x_amd64/vet . telabs/wazero/indocker-cli-plugin-metadata --64 x_amd64/vet (dns block)
• this-host-does-not-exist-12345.com
  • Triggering command: /tmp/go-build227273503/b518/mcp.test /tmp/go-build227273503/b518/mcp.test -test.testlogfile=/tmp/go-build227273503/b518/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true rnal/filedesc/build.go rnal/filedesc/desc.go x_amd64/compile -nxv 2850310/b130/ 64/bin/git x_amd64/compile go_.�� h/v2@v2.3.0/xxha-errorsas ache/go/1.25.9/x-ifaceassert (dns block)
  • Triggering command: /tmp/go-build2433055712/b494/mcp.test /tmp/go-build2433055712/b494/mcp.test -test.testlogfile=/tmp/go-build2433055712/b494/testlog.txt -test.paniconexit0 -test.timeout=10m0s 2850�� g_.a om/tetratelabs/wazero@v1.11.0/in-ifaceassert x_amd64/vet --gdwarf-5 --64 -o x_amd64/vet -W 2850310/b246/_pkg_.a 5oDDV1CJn x_amd64/vet . telabs/wazero/in/usr/bin/runc --64 x_amd64/vet (dns block)

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

• Configure Actions setup steps to set up my environment, which run before the firewall is enabled
• Add the appropriate URLs or hosts to the custom allowlist in this repository's Copilot coding agent settings (admins only)

[Copilot]

Pull request overview

Refactors and consolidates several helpers under internal/ to eliminate duplication, improve code organization, and align logging/tracing and formatting behavior across components.

Changes:

• Moved Docker inspection helpers into internal/sys and updated config callers/tests accordingly.
• Removed duplicate JSON-RPC structs and consolidated SDK logging into server/http_helpers.go using canonical mcp types.
• Extracted shared session suffix formatting into strutil.SessionSuffix and updated call sites.

Show a summary per file

File	Description
internal/sys/docker.go	Relocated Docker helpers into sys, exported entrypoints, and standardized logging category.
internal/sys/docker_test.go	Added tests for Docker helpers after relocation.
internal/config/validation_env.go	Updated env validation to call Docker helpers from sys.
internal/config/validation_env_test.go	Removed Docker-helper tests that were moved to internal/sys.
internal/config/docker_helpers_and_env_test.go	Removed container ID validation tests that were moved to internal/sys.
internal/server/http_helpers.go	Consolidated SDK logging here; refactored WithOTELTracing to delegate to tracing.WrapHTTPHandler.
internal/server/sdk_logging.go	Removed (SDK logging merged into http_helpers.go; duplicate JSON-RPC types eliminated).
internal/tracing/http.go	(Context reference) Provides WrapHTTPHandler used by the new WithOTELTracing implementation.
internal/server/circuit_breaker.go	Uses strutil.FormatDuration for consistent “reset at” formatting.
internal/proxy/proxy.go	Replaced stdlib log.Printf usage with structured logProxy logger.
internal/strutil/session_suffix.go	Introduced shared SessionSuffix helper.
internal/strutil/session_suffix_test.go	Added unit tests for SessionSuffix.
internal/mcp/errors.go	Replaced inline session suffix formatting with strutil.SessionSuffix.
internal/launcher/log_helpers.go	Replaced local sessionSuffix helper with strutil.SessionSuffix and removed unused import.
internal/launcher/log_helpers_test.go	Removed launcher-local sessionSuffix test (now covered in strutil).
internal/logger/rpc_helpers.go	Removed isEffectivelyEmpty after relocating it.
internal/logger/rpc_formatter.go	Added isEffectivelyEmpty colocated with its only caller.

Copilot's findings

Tip

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

Comments suppressed due to low confidence (4)

internal/sys/docker.go:105

• CheckPortMapping determines mapping via strings.Contains(output, portKey) which can produce false positives (e.g., port 80 matches 8080/tcp) and doesn't validate empty/invalid port values. Consider unmarshaling the {{json .NetworkSettings.Ports}} output into a map and checking for an exact "<port>/tcp" key with at least one host binding; also return an error for empty/non-numeric ports.
  internal/sys/docker_test.go:236
• This case assumes abcdef123456 refers to a nonexistent container and asserts an error. On systems where Docker is running and that ID prefix exists, the test could unexpectedly pass/fail depending on local state. Use a randomly generated valid-form ID (and/or explicitly skip when Docker is accessible) to keep the test deterministic.

			name:        "valid container ID format - nonexistent container",
			containerID: "abcdef123456",
			port:        "8080",
			shouldError: true, // Will fail because container doesn't exist
		},

internal/sys/docker_test.go:279

• This test assumes a valid-form container ID (abcdef123456) is nonexistent and therefore CheckStdinInteractive returns false. If that ID prefix exists locally, the result could be true and the test becomes environment-dependent. Consider using a per-run random ID or focusing only on invalid-ID cases for unit tests.

			name:        "valid container ID format - nonexistent container",
			containerID: "abcdef123456",
			expected:    false, // Will fail because container doesn't exist
		},

internal/sys/docker_test.go:314

• This test assumes abcdef123456 does not exist and therefore CheckLogDirMounted returns false. If a container with that ID prefix exists in the test environment, this becomes flaky. Prefer a randomly generated valid-form ID (or skip when Docker is accessible) to avoid coupling unit tests to host Docker state.

			name:        "valid container ID format - nonexistent container",
			containerID: "abcdef123456",
			logDir:      "/tmp/gh-aw/mcp-logs",
			expected:    false, // Will fail because container doesn't exist
		},

• Files reviewed: 16/16 changed files
• Comments generated: 1

[Copilot]

This test hard-codes a syntactically valid container ID (abcdef123456) and expects runDockerInspect to fail because the container doesn't exist. If Docker is available and a container happens to have that ID prefix, this becomes flaky. Prefer generating a random (but valid-form) ID per test run, or remove the "valid format" case and restrict this unit test to validation-only paths (invalid IDs) to avoid depending on the host Docker state.

This issue also appears in the following locations of the same file:

• line 232
• line 276
• line 310

[lpcox]

@copilot address this review feedback #4516 (review)

[Copilot]

@copilot address this review feedback #4516 (review)

Removed the four Docker-state-dependent test cases (lines 148–152, 232, 276, 310) from TestRunDockerInspect, TestCheckPortMapping, TestCheckStdinInteractive, and TestCheckLogDirMounted. Each function now only tests invalid-ID paths (empty string, special chars) that exercise the pure validation logic without touching Docker state. Committed in eb43432.