refactor(cmd): inline trivial getDefault* env-wrapper functions

[Copilot]

13 getDefault* functions across flags_logging.go, flags_difc.go, and flags_tracing.go were pure one-liner delegates to envutil.GetEnv{String,Int,Bool} — ~30 lines of boilerplate with no abstraction value.

Changes

• Inlined 12 wrapper functions directly at their single call sites in RegisterFlag blocks and proxy.go/root.go:

  // Before
  cmd.Flags().StringVar(&logDir, "log-dir", getDefaultLogDir(), "...")
  
  // After
  cmd.Flags().StringVar(&logDir, "log-dir", envutil.GetEnvString("MCP_GATEWAY_LOG_DIR", config.DefaultLogDir), "...")

• Preserved getDefaultDIFCMode() — retains validation logic (case normalisation + difc.ParseEnforcementMode check) that exceeds a simple env lookup
• Removed unused constants defaultPayloadPathPrefix and defaultAllowOnlyMinIntegrity (both were "")
• Added envutil import to root.go to support the inlined GetEnvBool call
• Updated flags.go package comment to document the new inline convention and note the getDefaultDIFCMode exception
• Removed/trimmed test functions that exclusively tested the deleted helpers (flags_logging_test.go deleted entirely; flags_tracing_test.go, flags_difc_test.go, proxy_test.go trimmed)

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-build3838746122/b514/launcher.test /tmp/go-build3838746122/b514/launcher.test -test.testlogfile=/tmp/go-build3838746122/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s -I .cfg qaJP/faPO1YLymyA-ifaceassert x_amd64/vet --gdwarf-5 l -o x_amd64/vet .cfg�� -I ylov/l0IgnnYtDUbmqPCsylov x_amd64/vet . --gdwarf2 --64 x_amd64/vet (dns block)
• invalid-host-that-does-not-exist-12345.com
  • Triggering command: /tmp/go-build3838746122/b496/config.test /tmp/go-build3838746122/b496/config.test -test.testlogfile=/tmp/go-build3838746122/b496/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build3838746122/b398/vet.cfg g_.a 1866238/b288/ x_amd64/vet -I ernal/launcher -imultiarch x_amd64/vet 1866�� .cfg /tmp/go-build131-ifaceassert 64/pkg/tool/linu-nilfunc . --gdwarf2 --64 64/pkg/tool/linu-buildtags (dns block)
• nonexistent.local
  • Triggering command: /tmp/go-build3838746122/b514/launcher.test /tmp/go-build3838746122/b514/launcher.test -test.testlogfile=/tmp/go-build3838746122/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s -I .cfg qaJP/faPO1YLymyA-ifaceassert x_amd64/vet --gdwarf-5 l -o x_amd64/vet .cfg�� -I ylov/l0IgnnYtDUbmqPCsylov x_amd64/vet . --gdwarf2 --64 x_amd64/vet (dns block)
• slow.example.com
  • Triggering command: /tmp/go-build3838746122/b514/launcher.test /tmp/go-build3838746122/b514/launcher.test -test.testlogfile=/tmp/go-build3838746122/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s -I .cfg qaJP/faPO1YLymyA-ifaceassert x_amd64/vet --gdwarf-5 l -o x_amd64/vet .cfg�� -I ylov/l0IgnnYtDUbmqPCsylov x_amd64/vet . --gdwarf2 --64 x_amd64/vet (dns block)
• this-host-does-not-exist-12345.com
  • Triggering command: /tmp/go-build3838746122/b523/mcp.test /tmp/go-build3838746122/b523/mcp.test -test.testlogfile=/tmp/go-build3838746122/b523/testlog.txt -test.paniconexit0 -test.timeout=10m0s .cfg�� akF0/nz7G04-4df3VsO6SakF0 /tmp/go-build1311866238/b288/ 64/pkg/tool/linux_amd64/vet . -imultiarch x86_64-linux-gnu(create|run) 64/pkg/tool/linux_amd64/vet .cfg�� pkg/mod/go.opentelemetry.io/prot-errorsas -plugin-opt=/usr/libexec/gcc/x86-ifaceassert x_amd64/vet -plugin-opt=-pas/usr/libexec/docker/docker-init -plugin-opt=-pas--version -plugin-opt=-pass-through=-lpthr-bool 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 cmd flag default handling by inlining trivial getDefault* env-wrapper helpers into flag registrations and keeping only the non-trivial DIFC mode default helper.

Changes:

• Inlined env default lookups (envutil.GetEnv{String,Int,Bool}) at flag declaration call sites across logging/tracing/DIFC flags and proxy/root usage.
• Removed now-unused default constants and deleted/trimmed tests that only covered the removed helpers.
• Updated flags.go package comment to document the new inline convention and the getDefaultDIFCMode() exception.

Show a summary per file

File	Description
internal/cmd/root.go	Imports envutil and inlines allow-only env default lookup; simplifies payload path prefix override condition.
internal/cmd/proxy.go	Inlines envutil-based defaults for log dir and OTLP flags.
internal/cmd/proxy_test.go	Updates log-dir default test to assert against config.DefaultLogDir.
internal/cmd/flags_tracing.go	Inlines envutil defaults for OTLP endpoint/service name; removes wrapper helpers.
internal/cmd/flags_tracing_test.go	Removes tests for deleted helpers; retains constant-default assertion test.
internal/cmd/flags_logging.go	Inlines envutil defaults for logging/payload-related flags; removes wrapper helpers and unused default const.
internal/cmd/flags_logging_test.go	Deletes tests that only exercised removed wrapper helpers.
internal/cmd/flags_difc.go	Inlines envutil defaults for DIFC-related env-driven flags; keeps getDefaultDIFCMode() for validation.
internal/cmd/flags_difc_test.go	Removes tests that only exercised removed wrapper helpers.
internal/cmd/flags.go	Updates package-level documentation to describe the new inline env-default pattern.

Copilot's findings

Tip

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

Comments suppressed due to low confidence (1)

internal/cmd/flags_tracing_test.go:14

• This test name refers to getDefaultOTLPServiceName, which was removed. Rename the test to reflect what it actually validates (e.g., the config.DefaultTracingServiceName constant), so the intent stays accurate after the refactor.

func TestGetDefaultOTLPServiceName_DefaultIsCorrect(t *testing.T) {
	// Verify the default constant value hasn't changed unexpectedly.
	// "mcp-gateway" is the canonical service name used in OTLP traces.
	assert.Equal(t, "mcp-gateway", config.DefaultTracingServiceName,
		"DefaultTracingServiceName constant should remain 'mcp-gateway'")

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

[Copilot]

The payload-path-prefix override block infers “environment variable was set” solely from payloadPathPrefix != "", and the preceding comment says “if different from default, it was explicitly set” even though the code checks Flags().Changed. Consider using os.LookupEnv("MCP_GATEWAY_PAYLOAD_PATH_PREFIX") (similar to other env detection in this file) to decide whether to apply the env override, and update the comment to match the actual behavior (CLI Changed vs env override). This avoids coupling correctness to the empty-string default.

See below for a potential fix:

	// Apply payload path prefix from CLI when the flag was explicitly provided;
	// otherwise, apply the environment-derived value only if the env var is set.
	if cmd.Flags().Changed("payload-path-prefix") {
		cfg.Gateway.PayloadPathPrefix = payloadPathPrefix
	} else if _, ok := os.LookupEnv("MCP_GATEWAY_PAYLOAD_PATH_PREFIX"); ok {

[lpcox]

Review Summary

Copilot PR Reviewer Feedback Assessment

1. Rename TestGetDefaultOTLPServiceName_DefaultIsCorrect (low-confidence, suppressed) — ✅ Valid. The test name references the deleted getDefaultOTLPServiceName function. Renamed to TestDefaultTracingServiceName_IsCorrect to match what it actually validates.

2. Use os.LookupEnv for payload-path-prefix override (root.go:250) — ❌ Would introduce a bug. The payloadPathPrefix variable gets its default from envutil.GetEnvString("MCP_GATEWAY_PAYLOAD_PATH_PREFIX", ""), which treats empty env vars as unset. Using os.LookupEnv would detect ="" as "set" while the flag default remains "" — creating a mismatch where an explicitly-empty env var would override config file settings with an empty string. The payloadPathPrefix != "" check correctly matches the envutil semantics. Updated the comment to be more precise about what the check does.

Own Review

The refactor is clean and well-motivated:

• 12 trivial one-liner getDefault* wrappers inlined at call sites — reduces indirection with no loss of functionality
• getDefaultDIFCMode() correctly preserved — it has validation logic beyond a simple env lookup
• Test deletions are appropriate — the removed tests only verified envutil.GetEnv* pass-through behavior, which is already tested in internal/envutil/
• Updated flags.go doc comment accurately describes the new convention

Commits Pushed

1. Renamed stale test function name
2. Clarified payload-path-prefix comment to describe the actual check semantics