Skip to content

Commit 3906216

Browse files
authored
Refactor URL derivation and helper ownership across envutil/config/mcp (#3968)
The codebase had duplicated GitHub API URL resolution logic with divergent behavior between proxy and unified server paths, plus helper placement that blurred package boundaries. This refactor consolidates URL derivation, separates config expansion from validation, and moves MCP result construction to the MCP layer. - **GitHub API URL derivation is now single-source (`envutil`)** - Added `envutil.DeriveGitHubAPIURL(defaultURL string)` and internal server-URL derivation logic. - `server/unified` now derives from `GITHUB_API_URL` **or** `GITHUB_SERVER_URL` (instead of only `GITHUB_API_URL`), aligning behavior with proxy mode. - Removed duplicate derivation functions from `internal/proxy/proxy.go`. - Updated call sites: - `internal/cmd/proxy.go` → `envutil.DeriveGitHubAPIURL("")` - `internal/server/unified.go` → `envutil.DeriveGitHubAPIURL("https://api.github.com")` - **Config variable expansion extracted from validation** - Moved expansion concerns from `internal/config/validation.go` to new `internal/config/expand.go`: - `expandVariablesCore`, `expandVariables`, `ExpandRawJSONVariables`, `expandEnvVariables`, `expandTracingVariables`, `varExprPattern` - `validation.go` now remains focused on validation rules and flow. - **MCP error `CallToolResult` helper moved to `mcp` package** - Added `mcp.NewErrorCallToolResult(err)` in `internal/mcp/tool_result.go`. - Replaced server-local helper usage in `internal/server/unified.go` and `internal/server/tool_registry.go`. - Updated corresponding tests to use the new helper location. ```go // unified/server path now uses centralized derivation logic apiURL := envutil.DeriveGitHubAPIURL("https://api.github.com") // shared MCP-layer error result helper return mcp.NewErrorCallToolResult(err) ``` > [!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-build1057253636/b514/launcher.test /tmp/go-build1057253636/b514/launcher.test -test.testlogfile=/tmp/go-build1057253636/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build1057253636/b467/vet.cfg apic.go decode.go x_amd64/vet -p roundrobin lcache/go/1.25.8-bool x_amd64/vet -p g_.a 1227851/b288/ x_amd64/vet -I /tmp/go-build278-atomic` (dns block) > - Triggering command: `/tmp/go-build900982912/b514/launcher.test /tmp/go-build900982912/b514/launcher.test -test.testlogfile=/tmp/go-build900982912/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s -d -lang=go1.25 64/pkg/tool/linu./internal/config b2799acda4848aff53b399dfb07b4159 /tmp/go-build278bash 0a4 64/pkg/tool/linu--noprofile ctl star�� r/runc-log.json 64/pkg/tool/linu/var/run/docker/runtime-runc/mob--log-format .13/x64/git auHmSssMaaom2Ek4bash -goversion r/6j32koixnhwjnj--noprofile /usr/bin/runc.original` (dns block) > - `invalid-host-that-does-not-exist-12345.com` > - Triggering command: `/tmp/go-build1057253636/b496/config.test /tmp/go-build1057253636/b496/config.test -test.testlogfile=/tmp/go-build1057253636/b496/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build1057253636/b387/vet.cfg @v1.1.3/cpu/cpu.-errorsas 1227851/b151/ x_amd64/vet --gdwarf-5 ternal/engine/in-unsafeptr=false lcache/go/1.25.8-unreachable=false x_amd64/vet 1227�� 8RQNG6Nox -I x_amd64/vet --gdwarf-5 --64 -o x_amd64/vet` (dns block) > - Triggering command: `/tmp/go-build2556833836/b492/config.test /tmp/go-build2556833836/b492/config.test -test.testlogfile=/tmp/go-build2556833836/b492/testlog.txt -test.paniconexit0 -test.timeout=10m0s /usr�� 7253636/b560/tty--log-format ntime.v2.task/mojson 7253636/b560/impdelete ntime.v2.task/mobash 062439bb528087fb/usr/bin/runc UNDLE=/etc/ssl/c--root uKbOHgHoxAqgN/t5/var/run/docker/runtime-runc/moby k/_t�� tr7/ca-certifica/run/containerd/io.containerd.runtime.v2.task/moby/d15ea4acb1ef69f4d1e1e6e2b19b0bash test:latest bash .cfg olang.org/protob--norc x_amd64/vet 95d45854b162103b` (dns block) > - Triggering command: `/tmp/go-build76351485/b001/config.test /tmp/go-build76351485/b001/config.test -test.testlogfile=/tmp/go-build76351485/b001/testlog.txt -test.paniconexit0 -test.timeout=10m0s -m 2e5d631e9ae2084a/run/containerd/io.containerd.runtime.v2.task/moby/b6f36c5d7e9d4--size-limit y ctl b04263944d57c5b7git d94d1b364afb75b8show 5d0/log.json ctl dock�� ion.go y ash ntime.v2.task/mo/opt/hostedtoolcache/CodeQL/2.25.1/x64/codeql/tools/linux64/REDACTED-linux 0b5929f8b23b298f/opt/hostedtoolcache/CodeQL/2.25.1/x64/codeql/go/tools/autobuild.sh e-handler 2a9e172d1927c35b435/log.json` (dns block) > - `nonexistent.local` > - Triggering command: `/tmp/go-build1057253636/b514/launcher.test /tmp/go-build1057253636/b514/launcher.test -test.testlogfile=/tmp/go-build1057253636/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build1057253636/b467/vet.cfg apic.go decode.go x_amd64/vet -p roundrobin lcache/go/1.25.8-bool x_amd64/vet -p g_.a 1227851/b288/ x_amd64/vet -I /tmp/go-build278-atomic` (dns block) > - Triggering command: `/tmp/go-build900982912/b514/launcher.test /tmp/go-build900982912/b514/launcher.test -test.testlogfile=/tmp/go-build900982912/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s -d -lang=go1.25 64/pkg/tool/linu./internal/config b2799acda4848aff53b399dfb07b4159 /tmp/go-build278bash 0a4 64/pkg/tool/linu--noprofile ctl star�� r/runc-log.json 64/pkg/tool/linu/var/run/docker/runtime-runc/mob--log-format .13/x64/git auHmSssMaaom2Ek4bash -goversion r/6j32koixnhwjnj--noprofile /usr/bin/runc.original` (dns block) > - `slow.example.com` > - Triggering command: `/tmp/go-build1057253636/b514/launcher.test /tmp/go-build1057253636/b514/launcher.test -test.testlogfile=/tmp/go-build1057253636/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build1057253636/b467/vet.cfg apic.go decode.go x_amd64/vet -p roundrobin lcache/go/1.25.8-bool x_amd64/vet -p g_.a 1227851/b288/ x_amd64/vet -I /tmp/go-build278-atomic` (dns block) > - Triggering command: `/tmp/go-build900982912/b514/launcher.test /tmp/go-build900982912/b514/launcher.test -test.testlogfile=/tmp/go-build900982912/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s -d -lang=go1.25 64/pkg/tool/linu./internal/config b2799acda4848aff53b399dfb07b4159 /tmp/go-build278bash 0a4 64/pkg/tool/linu--noprofile ctl star�� r/runc-log.json 64/pkg/tool/linu/var/run/docker/runtime-runc/mob--log-format .13/x64/git auHmSssMaaom2Ek4bash -goversion r/6j32koixnhwjnj--noprofile /usr/bin/runc.original` (dns block) > - `this-host-does-not-exist-12345.com` > - Triggering command: `/tmp/go-build1057253636/b523/mcp.test /tmp/go-build1057253636/b523/mcp.test -test.testlogfile=/tmp/go-build1057253636/b523/testlog.txt -test.paniconexit0 -test.timeout=10m0s -I .cfg olang.org/grpc@v1.80.0/balancer_wrapper.go x_amd64/vet ute.go mentation.go -o x_amd64/vet .cfg�� J5E4/LjHfXKvhsjfAdZsIJ5E4 -I x_amd64/vet -I 1227851/b468/ -imultiarch x_amd64/vet` (dns block) > - Triggering command: `/tmp/go-build2556833836/b489/mcp.test /tmp/go-build2556833836/b489/mcp.test -test.testlogfile=/tmp/go-build2556833836/b489/testlog.txt -test.paniconexit0 -test.timeout=10m0s` (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 1b1af3a + 265ad34 commit 3906216

13 files changed

Lines changed: 358 additions & 545 deletions

internal/cmd/proxy.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -189,7 +189,7 @@ func runProxy(cmd *cobra.Command, args []string) error {
189189
// Resolve GitHub API URL: flag → env vars → default
190190
apiURL := proxyAPIURL
191191
if apiURL == "" {
192-
apiURL = proxy.DeriveGitHubAPIURL()
192+
apiURL = envutil.DeriveGitHubAPIURL("")
193193
}
194194
if apiURL == "" {
195195
apiURL = proxy.DefaultGitHubAPIBase

internal/config/expand.go

Lines changed: 133 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,133 @@
1+
package config
2+
3+
import (
4+
"fmt"
5+
"os"
6+
"regexp"
7+
8+
"github.com/github/gh-aw-mcpg/internal/config/rules"
9+
)
10+
11+
// Variable expression pattern: ${VARIABLE_NAME}
12+
var varExprPattern = regexp.MustCompile(`\$\{([A-Za-z_][A-Za-z0-9_]*)\}`)
13+
14+
// expandVariablesCore is the shared implementation for variable expansion.
15+
// It works with byte slices and handles the core expansion logic, tracking undefined variables.
16+
// This eliminates code duplication between expandVariables and ExpandRawJSONVariables.
17+
// It returns the expanded bytes, a slice of undefined variable names, and an error
18+
// (currently always nil).
19+
func expandVariablesCore(data []byte, contextDesc string) ([]byte, []string, error) {
20+
logValidation.Printf("Expanding variables: context=%s", contextDesc)
21+
var undefinedVars []string
22+
23+
result := varExprPattern.ReplaceAllFunc(data, func(match []byte) []byte {
24+
// Extract variable name (remove ${ and })
25+
varName := string(match[2 : len(match)-1])
26+
27+
if envValue, exists := os.LookupEnv(varName); exists {
28+
logValidation.Printf("Expanded variable: %s (found in environment)", varName)
29+
return []byte(envValue)
30+
}
31+
32+
// Track undefined variable
33+
undefinedVars = append(undefinedVars, varName)
34+
logValidation.Printf("Undefined variable: %s", varName)
35+
return match // Keep original if undefined
36+
})
37+
38+
logValidation.Printf("Variable expansion completed: context=%s, undefined_count=%d", contextDesc, len(undefinedVars))
39+
return result, undefinedVars, nil
40+
}
41+
42+
// expandVariables expands variable expressions in a string.
43+
// value is the source string and jsonPath identifies the config location for errors.
44+
// It returns the expanded string and an error if any variable is undefined.
45+
func expandVariables(value, jsonPath string) (string, error) {
46+
result, undefinedVars, _ := expandVariablesCore([]byte(value), fmt.Sprintf("jsonPath=%s", jsonPath))
47+
48+
if len(undefinedVars) > 0 {
49+
logValidation.Printf("Variable expansion failed: undefined variables=%v", undefinedVars)
50+
return "", rules.UndefinedVariable(undefinedVars[0], jsonPath)
51+
}
52+
53+
return string(result), nil
54+
}
55+
56+
// ExpandRawJSONVariables expands all ${VAR} expressions in JSON data before schema validation.
57+
// This ensures the schema validates the expanded values, not the variable syntax.
58+
// It collects undefined variables and reports the first undefined variable as an error.
59+
func ExpandRawJSONVariables(data []byte) ([]byte, error) {
60+
result, undefinedVars, _ := expandVariablesCore(data, "raw JSON data")
61+
62+
if len(undefinedVars) > 0 {
63+
logValidation.Printf("Variable expansion failed: undefined variables=%v", undefinedVars)
64+
return nil, rules.UndefinedVariable(undefinedVars[0], "configuration")
65+
}
66+
67+
return result, nil
68+
}
69+
70+
// expandEnvVariables expands all variable expressions in an env map.
71+
// env is the map to expand and serverName is used for config-path error context.
72+
// It returns a new map with expanded values or an error if any variable is undefined.
73+
func expandEnvVariables(env map[string]string, serverName string) (map[string]string, error) {
74+
logValidation.Printf("Expanding env variables for server: %s, count=%d", serverName, len(env))
75+
result := make(map[string]string, len(env))
76+
77+
for key, value := range env {
78+
jsonPath := fmt.Sprintf("mcpServers.%s.env.%s", serverName, key)
79+
80+
expanded, err := expandVariables(value, jsonPath)
81+
if err != nil {
82+
return nil, err
83+
}
84+
85+
result[key] = expanded
86+
}
87+
88+
logValidation.Printf("Env variable expansion completed for server: %s", serverName)
89+
return result, nil
90+
}
91+
92+
// expandTracingVariables expands ${VAR} expressions in TracingConfig fields.
93+
// This is called for TOML-loaded configs before validation, mirroring the
94+
// stdin JSON path where ExpandRawJSONVariables handles expansion.
95+
func expandTracingVariables(cfg *TracingConfig) error {
96+
if cfg == nil {
97+
return nil
98+
}
99+
100+
if cfg.Endpoint != "" {
101+
expanded, err := expandVariables(cfg.Endpoint, "gateway.opentelemetry.endpoint")
102+
if err != nil {
103+
return err
104+
}
105+
cfg.Endpoint = expanded
106+
}
107+
108+
if cfg.TraceID != "" {
109+
expanded, err := expandVariables(cfg.TraceID, "gateway.opentelemetry.traceId")
110+
if err != nil {
111+
return err
112+
}
113+
cfg.TraceID = expanded
114+
}
115+
116+
if cfg.SpanID != "" {
117+
expanded, err := expandVariables(cfg.SpanID, "gateway.opentelemetry.spanId")
118+
if err != nil {
119+
return err
120+
}
121+
cfg.SpanID = expanded
122+
}
123+
124+
if cfg.Headers != "" {
125+
expanded, err := expandVariables(cfg.Headers, "gateway.opentelemetry.headers")
126+
if err != nil {
127+
return err
128+
}
129+
cfg.Headers = expanded
130+
}
131+
132+
return nil
133+
}

internal/config/validation.go

Lines changed: 0 additions & 119 deletions
Original file line numberDiff line numberDiff line change
@@ -16,9 +16,6 @@ import (
1616
// ValidationError is an alias for rules.ValidationError for backward compatibility
1717
type ValidationError = rules.ValidationError
1818

19-
// Variable expression pattern: ${VARIABLE_NAME}
20-
var varExprPattern = regexp.MustCompile(`\$\{([A-Za-z_][A-Za-z0-9_]*)\}`)
21-
2219
// W3C trace context patterns (spec §4.1.3.6)
2320
var (
2421
traceIDPattern = regexp.MustCompile(`^[0-9a-f]{32}$`)
@@ -45,122 +42,6 @@ func logValidateServerFailed(name, reason string) {
4542
logValidation.Printf("Validation failed: %s, name=%s", reason, name)
4643
}
4744

48-
// expandVariablesCore is the shared implementation for variable expansion.
49-
// It works with byte slices and handles the core expansion logic, tracking undefined variables.
50-
// This eliminates code duplication between expandVariables and ExpandRawJSONVariables.
51-
func expandVariablesCore(data []byte, contextDesc string) ([]byte, []string, error) {
52-
logValidation.Printf("Expanding variables: context=%s", contextDesc)
53-
var undefinedVars []string
54-
55-
result := varExprPattern.ReplaceAllFunc(data, func(match []byte) []byte {
56-
// Extract variable name (remove ${ and })
57-
varName := string(match[2 : len(match)-1])
58-
59-
if envValue, exists := os.LookupEnv(varName); exists {
60-
logValidation.Printf("Expanded variable: %s (found in environment)", varName)
61-
return []byte(envValue)
62-
}
63-
64-
// Track undefined variable
65-
undefinedVars = append(undefinedVars, varName)
66-
logValidation.Printf("Undefined variable: %s", varName)
67-
return match // Keep original if undefined
68-
})
69-
70-
logValidation.Printf("Variable expansion completed: context=%s, undefined_count=%d", contextDesc, len(undefinedVars))
71-
return result, undefinedVars, nil
72-
}
73-
74-
// expandVariables expands variable expressions in a string
75-
// Returns the expanded string and error if any variable is undefined
76-
func expandVariables(value, jsonPath string) (string, error) {
77-
result, undefinedVars, _ := expandVariablesCore([]byte(value), fmt.Sprintf("jsonPath=%s", jsonPath))
78-
79-
if len(undefinedVars) > 0 {
80-
logValidation.Printf("Variable expansion failed: undefined variables=%v", undefinedVars)
81-
return "", rules.UndefinedVariable(undefinedVars[0], jsonPath)
82-
}
83-
84-
return string(result), nil
85-
}
86-
87-
// ExpandRawJSONVariables expands all ${VAR} expressions in JSON data before schema validation.
88-
// This ensures the schema validates the expanded values, not the variable syntax.
89-
// It collects all undefined variables and reports them in a single error.
90-
func ExpandRawJSONVariables(data []byte) ([]byte, error) {
91-
result, undefinedVars, _ := expandVariablesCore(data, "raw JSON data")
92-
93-
if len(undefinedVars) > 0 {
94-
logValidation.Printf("Variable expansion failed: undefined variables=%v", undefinedVars)
95-
return nil, rules.UndefinedVariable(undefinedVars[0], "configuration")
96-
}
97-
98-
return result, nil
99-
}
100-
101-
// expandEnvVariables expands all variable expressions in an env map
102-
func expandEnvVariables(env map[string]string, serverName string) (map[string]string, error) {
103-
logValidation.Printf("Expanding env variables for server: %s, count=%d", serverName, len(env))
104-
result := make(map[string]string, len(env))
105-
106-
for key, value := range env {
107-
jsonPath := fmt.Sprintf("mcpServers.%s.env.%s", serverName, key)
108-
109-
expanded, err := expandVariables(value, jsonPath)
110-
if err != nil {
111-
return nil, err
112-
}
113-
114-
result[key] = expanded
115-
}
116-
117-
logValidation.Printf("Env variable expansion completed for server: %s", serverName)
118-
return result, nil
119-
}
120-
121-
// expandTracingVariables expands ${VAR} expressions in TracingConfig fields.
122-
// This is called for TOML-loaded configs before validation, mirroring the
123-
// stdin JSON path where ExpandRawJSONVariables handles expansion.
124-
func expandTracingVariables(cfg *TracingConfig) error {
125-
if cfg == nil {
126-
return nil
127-
}
128-
129-
if cfg.Endpoint != "" {
130-
expanded, err := expandVariables(cfg.Endpoint, "gateway.opentelemetry.endpoint")
131-
if err != nil {
132-
return err
133-
}
134-
cfg.Endpoint = expanded
135-
}
136-
137-
if cfg.TraceID != "" {
138-
expanded, err := expandVariables(cfg.TraceID, "gateway.opentelemetry.traceId")
139-
if err != nil {
140-
return err
141-
}
142-
cfg.TraceID = expanded
143-
}
144-
145-
if cfg.SpanID != "" {
146-
expanded, err := expandVariables(cfg.SpanID, "gateway.opentelemetry.spanId")
147-
if err != nil {
148-
return err
149-
}
150-
cfg.SpanID = expanded
151-
}
152-
153-
if cfg.Headers != "" {
154-
expanded, err := expandVariables(cfg.Headers, "gateway.opentelemetry.headers")
155-
if err != nil {
156-
return err
157-
}
158-
cfg.Headers = expanded
159-
}
160-
161-
return nil
162-
}
163-
16445
// validateMounts validates mount specifications using centralized rules
16546
func validateMounts(mounts []string, jsonPath string) error {
16647
for i, mount := range mounts {

internal/envutil/github.go

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
package envutil
22

33
import (
4+
"fmt"
5+
"net/url"
46
"os"
57
"strings"
68

@@ -9,6 +11,9 @@ import (
911

1012
var logGitHub = logger.New("envutil:github")
1113

14+
// DefaultGitHubAPIBaseURL is the default GitHub API base URL.
15+
const DefaultGitHubAPIBaseURL = "https://api.github.com"
16+
1217
// LookupGitHubToken searches environment variables for a GitHub token using
1318
// a canonical priority order. It returns the first non-empty (trimmed) value
1419
// found, or an empty string if none is set.
@@ -49,3 +54,49 @@ func LookupGitHubAPIURL(defaultURL string) string {
4954
logGitHub.Printf("GitHub API URL using default: %s", url)
5055
return url
5156
}
57+
58+
// DeriveGitHubAPIURL resolves the GitHub API URL from environment variables:
59+
// 1. GITHUB_API_URL
60+
// 2. GITHUB_SERVER_URL (derived)
61+
// 3. defaultURL
62+
func DeriveGitHubAPIURL(defaultURL string) string {
63+
if apiURL := LookupGitHubAPIURL(""); apiURL != "" {
64+
return apiURL
65+
}
66+
if serverURL := strings.TrimSpace(os.Getenv("GITHUB_SERVER_URL")); serverURL != "" {
67+
derived := deriveAPIFromServerURL(serverURL)
68+
if derived != "" {
69+
logGitHub.Printf("GitHub API URL derived from GITHUB_SERVER_URL=%s: %s", serverURL, derived)
70+
return derived
71+
}
72+
}
73+
return strings.TrimRight(strings.TrimSpace(defaultURL), "/")
74+
}
75+
76+
// deriveAPIFromServerURL converts a GITHUB_SERVER_URL to the corresponding API endpoint.
77+
// GHEC tenants (*.ghe.com): https://tenant.ghe.com → https://copilot-api.tenant.ghe.com
78+
// GitHub.com: https://github.com → https://api.github.com
79+
// GHES (all others): https://github.example.com → https://github.example.com/api/v3
80+
func deriveAPIFromServerURL(serverURL string) string {
81+
parsed, err := url.Parse(strings.TrimRight(serverURL, "/"))
82+
if err != nil || parsed.Host == "" {
83+
return ""
84+
}
85+
if parsed.Scheme != "http" && parsed.Scheme != "https" {
86+
return ""
87+
}
88+
89+
hostname := strings.ToLower(parsed.Hostname())
90+
91+
switch {
92+
case hostname == "github.com" || hostname == "www.github.com":
93+
return DefaultGitHubAPIBaseURL
94+
case strings.HasSuffix(hostname, ".ghe.com"):
95+
if port := parsed.Port(); port != "" {
96+
return fmt.Sprintf("%s://copilot-api.%s:%s", parsed.Scheme, hostname, port)
97+
}
98+
return fmt.Sprintf("%s://copilot-api.%s", parsed.Scheme, hostname)
99+
default:
100+
return fmt.Sprintf("%s://%s/api/v3", parsed.Scheme, parsed.Host)
101+
}
102+
}

0 commit comments

Comments
 (0)