Skip to content

Commit 85562b1

Browse files
authored
refactor(cmd): inline trivial getDefault* env-wrapper functions (#3894)
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`: ```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] > > <details> > <summary>Firewall rules blocked me from connecting to one or more addresses (expand for details)</summary> > > #### 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](https://gh.io/copilot/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](https://github.com/github/gh-aw-mcpg/settings/copilot/coding_agent) (admins only) > > </details>
2 parents 40d8648 + ed90ec1 commit 85562b1

File tree

10 files changed

+33
-496
lines changed

10 files changed

+33
-496
lines changed

internal/cmd/flags.go

Lines changed: 9 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -17,41 +17,21 @@
1717
// })
1818
// }
1919
//
20-
// # getDefault*() Flag Helper Pattern
20+
// # Flag Defaults with Environment Variable Overrides
2121
//
22-
// Each flag whose default value can be overridden by an environment variable has a
23-
// corresponding getDefault*() helper function that follows this pattern:
22+
// Flags whose defaults can be overridden by an environment variable use inline
23+
// envutil.GetEnv* calls directly in the RegisterFlag block:
2424
//
25-
// func getDefaultXxx() T {
26-
// return envutil.GetEnvT("MCP_GATEWAY_XXX", defaultXxx)
27-
// }
28-
//
29-
// Current helpers and their environment variables:
25+
// cmd.Flags().StringVar(&myDir, "my-dir", envutil.GetEnvString("MY_DIR_ENV", config.DefaultMyDir), "...")
3026
//
31-
// flags_logging.go getDefaultLogDir() → MCP_GATEWAY_LOG_DIR
32-
// flags_logging.go getDefaultPayloadDir() → MCP_GATEWAY_PAYLOAD_DIR
33-
// flags_logging.go getDefaultPayloadPathPrefix() → MCP_GATEWAY_PAYLOAD_PATH_PREFIX
34-
// flags_logging.go getDefaultPayloadSizeThreshold() → MCP_GATEWAY_PAYLOAD_SIZE_THRESHOLD
35-
// flags_difc.go getDefaultDIFCMode() → MCP_GATEWAY_GUARDS_MODE
36-
// flags_difc.go getDefaultDIFCSinkServerIDs() → MCP_GATEWAY_GUARDS_SINK_SERVER_IDS
37-
// flags_difc.go getDefaultGuardPolicyJSON() → MCP_GATEWAY_GUARD_POLICY_JSON
38-
// flags_difc.go getDefaultAllowOnlyScopePublic() → MCP_GATEWAY_ALLOWONLY_SCOPE_PUBLIC
39-
// flags_difc.go getDefaultAllowOnlyOwner() → MCP_GATEWAY_ALLOWONLY_SCOPE_OWNER
40-
// flags_difc.go getDefaultAllowOnlyRepo() → MCP_GATEWAY_ALLOWONLY_SCOPE_REPO
41-
// flags_difc.go getDefaultAllowOnlyMinIntegrity() → MCP_GATEWAY_ALLOWONLY_MIN_INTEGRITY
42-
// flags_tracing.go getDefaultOTLPEndpoint() → OTEL_EXPORTER_OTLP_ENDPOINT
43-
// flags_tracing.go getDefaultOTLPServiceName() → OTEL_SERVICE_NAME
27+
// This keeps the env-var name co-located with the flag declaration.
4428
//
45-
// This pattern is intentionally kept in individual feature files because:
46-
// - Each helper names the specific environment variable it reads, making the
47-
// coupling between flag and env var explicit and discoverable.
48-
// - The one-liner wrappers are trivial and unlikely to diverge.
49-
// - Go's type system (string/int/bool) prevents a single generic helper without
50-
// sacrificing readability.
29+
// Exception: getDefaultDIFCMode() in flags_difc.go is kept as a named helper
30+
// because it contains validation logic beyond a simple env lookup.
5131
//
5232
// When adding a new flag with an environment variable override:
53-
// 1. Add a defaultXxx constant and a getDefaultXxx() function in the feature file.
54-
// 2. Add the new helper to the table above.
33+
// 1. Use envutil.GetEnv* directly in the RegisterFlag call.
34+
// 2. Document the environment variable in AGENTS.md and README.md.
5535
package cmd
5636

5737
import "github.com/spf13/cobra"

internal/cmd/flags_difc.go

Lines changed: 6 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -13,11 +13,6 @@ import (
1313
"github.com/spf13/cobra"
1414
)
1515

16-
// DIFC flag defaults
17-
const (
18-
defaultAllowOnlyMinIntegrity = ""
19-
)
20-
2116
// DIFC flag variables
2217
var (
2318
difcMode string
@@ -32,12 +27,12 @@ var (
3227
func init() {
3328
RegisterFlag(func(cmd *cobra.Command) {
3429
cmd.Flags().StringVar(&difcMode, "guards-mode", getDefaultDIFCMode(), "Guards enforcement mode: strict (deny violations), filter (remove denied tools), or propagate (auto-adjust agent labels on reads)")
35-
cmd.Flags().StringVar(&difcSinkServerIDs, "guards-sink-server-ids", getDefaultDIFCSinkServerIDs(), "Comma-separated server IDs whose RPC JSONL logs should include agent secrecy/integrity tag snapshots")
36-
cmd.Flags().StringVar(&guardPolicyJSON, "guard-policy-json", getDefaultGuardPolicyJSON(), "Guard policy JSON (e.g. {\"allow-only\":{\"repos\":\"public\",\"min-integrity\":\"none\"}})")
37-
cmd.Flags().BoolVar(&allowOnlyPublic, "allowonly-scope-public", getDefaultAllowOnlyScopePublic(), "Use public AllowOnly scope")
38-
cmd.Flags().StringVar(&allowOnlyOwner, "allowonly-scope-owner", getDefaultAllowOnlyOwner(), "AllowOnly owner scope value")
39-
cmd.Flags().StringVar(&allowOnlyRepo, "allowonly-scope-repo", getDefaultAllowOnlyRepo(), "AllowOnly repo name (requires owner)")
40-
cmd.Flags().StringVar(&allowOnlyMinInt, "allowonly-min-integrity", getDefaultAllowOnlyMinIntegrity(), "AllowOnly integrity: none|unapproved|approved|merged")
30+
cmd.Flags().StringVar(&difcSinkServerIDs, "guards-sink-server-ids", envutil.GetEnvString("MCP_GATEWAY_GUARDS_SINK_SERVER_IDS", ""), "Comma-separated server IDs whose RPC JSONL logs should include agent secrecy/integrity tag snapshots")
31+
cmd.Flags().StringVar(&guardPolicyJSON, "guard-policy-json", envutil.GetEnvString("MCP_GATEWAY_GUARD_POLICY_JSON", ""), "Guard policy JSON (e.g. {\"allow-only\":{\"repos\":\"public\",\"min-integrity\":\"none\"}})")
32+
cmd.Flags().BoolVar(&allowOnlyPublic, "allowonly-scope-public", envutil.GetEnvBool("MCP_GATEWAY_ALLOWONLY_SCOPE_PUBLIC", false), "Use public AllowOnly scope")
33+
cmd.Flags().StringVar(&allowOnlyOwner, "allowonly-scope-owner", envutil.GetEnvString("MCP_GATEWAY_ALLOWONLY_SCOPE_OWNER", ""), "AllowOnly owner scope value")
34+
cmd.Flags().StringVar(&allowOnlyRepo, "allowonly-scope-repo", envutil.GetEnvString("MCP_GATEWAY_ALLOWONLY_SCOPE_REPO", ""), "AllowOnly repo name (requires owner)")
35+
cmd.Flags().StringVar(&allowOnlyMinInt, "allowonly-min-integrity", envutil.GetEnvString("MCP_GATEWAY_ALLOWONLY_MIN_INTEGRITY", ""), "AllowOnly integrity: none|unapproved|approved|merged")
4136
})
4237
}
4338

@@ -55,40 +50,6 @@ func getDefaultDIFCMode() string {
5550
return difc.ModeStrict
5651
}
5752

58-
func getDefaultAllowOnlyScopePublic() bool {
59-
return envutil.GetEnvBool("MCP_GATEWAY_ALLOWONLY_SCOPE_PUBLIC", false)
60-
}
61-
62-
// getDefaultDIFCSinkServerIDs returns the default DIFC sink server IDs string,
63-
// checking MCP_GATEWAY_GUARDS_SINK_SERVER_IDS environment variable.
64-
func getDefaultDIFCSinkServerIDs() string {
65-
return envutil.GetEnvString("MCP_GATEWAY_GUARDS_SINK_SERVER_IDS", "")
66-
}
67-
68-
// getDefaultGuardPolicyJSON returns the default guard policy JSON string,
69-
// checking MCP_GATEWAY_GUARD_POLICY_JSON environment variable.
70-
func getDefaultGuardPolicyJSON() string {
71-
return envutil.GetEnvString("MCP_GATEWAY_GUARD_POLICY_JSON", "")
72-
}
73-
74-
// getDefaultAllowOnlyOwner returns the default AllowOnly owner scope,
75-
// checking MCP_GATEWAY_ALLOWONLY_SCOPE_OWNER environment variable.
76-
func getDefaultAllowOnlyOwner() string {
77-
return envutil.GetEnvString("MCP_GATEWAY_ALLOWONLY_SCOPE_OWNER", "")
78-
}
79-
80-
// getDefaultAllowOnlyRepo returns the default AllowOnly repo name,
81-
// checking MCP_GATEWAY_ALLOWONLY_SCOPE_REPO environment variable.
82-
func getDefaultAllowOnlyRepo() string {
83-
return envutil.GetEnvString("MCP_GATEWAY_ALLOWONLY_SCOPE_REPO", "")
84-
}
85-
86-
// getDefaultAllowOnlyMinIntegrity returns the default AllowOnly minimum integrity level,
87-
// checking MCP_GATEWAY_ALLOWONLY_MIN_INTEGRITY environment variable.
88-
func getDefaultAllowOnlyMinIntegrity() string {
89-
return envutil.GetEnvString("MCP_GATEWAY_ALLOWONLY_MIN_INTEGRITY", defaultAllowOnlyMinIntegrity)
90-
}
91-
9253
func parseDIFCSinkServerIDs(input string) ([]string, error) {
9354
if strings.TrimSpace(input) == "" {
9455
return nil, nil

internal/cmd/flags_difc_test.go

Lines changed: 0 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -147,46 +147,6 @@ func TestValidDIFCModes(t *testing.T) {
147147
require.Len(difc.ValidModes, 3, "should only have 3 valid modes")
148148
}
149149

150-
func TestGetDefaultDIFCSinkServerIDs(t *testing.T) {
151-
tests := []struct {
152-
name string
153-
envValue string
154-
setEnv bool
155-
expected string
156-
}{
157-
{
158-
name: "no env var - returns empty string",
159-
setEnv: false,
160-
expected: "",
161-
},
162-
{
163-
name: "env var set - returns value",
164-
envValue: "safeoutputs,github",
165-
setEnv: true,
166-
expected: "safeoutputs,github",
167-
},
168-
{
169-
name: "empty env var - returns empty string",
170-
envValue: "",
171-
setEnv: true,
172-
expected: "",
173-
},
174-
}
175-
176-
for _, tt := range tests {
177-
t.Run(tt.name, func(t *testing.T) {
178-
if tt.setEnv {
179-
t.Setenv("MCP_GATEWAY_GUARDS_SINK_SERVER_IDS", tt.envValue)
180-
} else {
181-
t.Setenv("MCP_GATEWAY_GUARDS_SINK_SERVER_IDS", "")
182-
}
183-
184-
result := getDefaultDIFCSinkServerIDs()
185-
assert.Equal(t, tt.expected, result)
186-
})
187-
}
188-
}
189-
190150
func TestParseDIFCSinkServerIDs(t *testing.T) {
191151
tests := []struct {
192152
name string
@@ -270,17 +230,3 @@ func TestBuildAllowOnlyPolicy(t *testing.T) {
270230
require.Error(t, err)
271231
})
272232
}
273-
274-
func TestGetDefaultGuardPolicyInputs(t *testing.T) {
275-
t.Setenv("MCP_GATEWAY_GUARD_POLICY_JSON", `{"allow-only":{"repos":"public","min-integrity":"none"}}`)
276-
t.Setenv("MCP_GATEWAY_ALLOWONLY_SCOPE_PUBLIC", "1")
277-
t.Setenv("MCP_GATEWAY_ALLOWONLY_SCOPE_OWNER", "lpcox")
278-
t.Setenv("MCP_GATEWAY_ALLOWONLY_SCOPE_REPO", "gh-aw-mcpg")
279-
t.Setenv("MCP_GATEWAY_ALLOWONLY_MIN_INTEGRITY", "unapproved")
280-
281-
assert.NotEmpty(t, getDefaultGuardPolicyJSON())
282-
assert.True(t, getDefaultAllowOnlyScopePublic())
283-
assert.Equal(t, "lpcox", getDefaultAllowOnlyOwner())
284-
assert.Equal(t, "gh-aw-mcpg", getDefaultAllowOnlyRepo())
285-
assert.Equal(t, "unapproved", getDefaultAllowOnlyMinIntegrity())
286-
}

internal/cmd/flags_logging.go

Lines changed: 4 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,6 @@ import (
88
"github.com/spf13/cobra"
99
)
1010

11-
// Logging flag defaults
12-
const (
13-
defaultPayloadPathPrefix = "" // Empty by default - use actual filesystem path
14-
)
15-
1611
// Logging flag variables
1712
var (
1813
logDir string
@@ -23,33 +18,9 @@ var (
2318

2419
func init() {
2520
RegisterFlag(func(cmd *cobra.Command) {
26-
cmd.Flags().StringVar(&logDir, "log-dir", getDefaultLogDir(), "Directory for log files (falls back to stdout if directory cannot be created)")
27-
cmd.Flags().StringVar(&payloadDir, "payload-dir", getDefaultPayloadDir(), "Directory for storing large payload files (segmented by session ID)")
28-
cmd.Flags().StringVar(&payloadPathPrefix, "payload-path-prefix", getDefaultPayloadPathPrefix(), "Path prefix to use when returning payloadPath to clients (allows remapping host paths to client/agent container paths)")
29-
cmd.Flags().IntVar(&payloadSizeThreshold, "payload-size-threshold", getDefaultPayloadSizeThreshold(), "Size threshold (in bytes) for storing payloads to disk. Payloads larger than this are stored, smaller ones returned inline")
21+
cmd.Flags().StringVar(&logDir, "log-dir", envutil.GetEnvString("MCP_GATEWAY_LOG_DIR", config.DefaultLogDir), "Directory for log files (falls back to stdout if directory cannot be created)")
22+
cmd.Flags().StringVar(&payloadDir, "payload-dir", envutil.GetEnvString("MCP_GATEWAY_PAYLOAD_DIR", config.DefaultPayloadDir), "Directory for storing large payload files (segmented by session ID)")
23+
cmd.Flags().StringVar(&payloadPathPrefix, "payload-path-prefix", envutil.GetEnvString("MCP_GATEWAY_PAYLOAD_PATH_PREFIX", ""), "Path prefix to use when returning payloadPath to clients (allows remapping host paths to client/agent container paths)")
24+
cmd.Flags().IntVar(&payloadSizeThreshold, "payload-size-threshold", envutil.GetEnvInt("MCP_GATEWAY_PAYLOAD_SIZE_THRESHOLD", config.DefaultPayloadSizeThreshold), "Size threshold (in bytes) for storing payloads to disk. Payloads larger than this are stored, smaller ones returned inline")
3025
})
3126
}
32-
33-
// getDefaultLogDir returns the default log directory, checking MCP_GATEWAY_LOG_DIR
34-
// environment variable first, then falling back to the hardcoded default
35-
func getDefaultLogDir() string {
36-
return envutil.GetEnvString("MCP_GATEWAY_LOG_DIR", config.DefaultLogDir)
37-
}
38-
39-
// getDefaultPayloadDir returns the default payload directory, checking MCP_GATEWAY_PAYLOAD_DIR
40-
// environment variable first, then falling back to the hardcoded default
41-
func getDefaultPayloadDir() string {
42-
return envutil.GetEnvString("MCP_GATEWAY_PAYLOAD_DIR", config.DefaultPayloadDir)
43-
}
44-
45-
// getDefaultPayloadPathPrefix returns the default payload path prefix, checking MCP_GATEWAY_PAYLOAD_PATH_PREFIX
46-
// environment variable first, then falling back to the hardcoded default
47-
func getDefaultPayloadPathPrefix() string {
48-
return envutil.GetEnvString("MCP_GATEWAY_PAYLOAD_PATH_PREFIX", defaultPayloadPathPrefix)
49-
}
50-
51-
// getDefaultPayloadSizeThreshold returns the default payload size threshold, checking
52-
// MCP_GATEWAY_PAYLOAD_SIZE_THRESHOLD environment variable first, then falling back to the hardcoded default
53-
func getDefaultPayloadSizeThreshold() int {
54-
return envutil.GetEnvInt("MCP_GATEWAY_PAYLOAD_SIZE_THRESHOLD", config.DefaultPayloadSizeThreshold)
55-
}

0 commit comments

Comments
 (0)