Skip to content

Commit f5c4b89

Browse files
lpcoxlpcox
authored
Deduplicate error CallToolResult construction, GitHub token and API URL lookups (#3507)
Three duplication patterns across `internal/server/unified.go`, `internal/cmd/proxy.go`, and `internal/proxy/proxy.go` — an unused helper, divergent token lookup order, and duplicated API URL resolution. ### Inline error CallToolResult → use existing helper 3 inline `&sdk.CallToolResult{IsError: true, ...}` constructions in `unified.go` now call the existing `newErrorCallToolResult()` helper that was being bypassed: ```go // before (repeated 3x with varying field order) return &sdk.CallToolResult{ Content: []sdk.Content{&sdk.TextContent{Text: err.Error()}}, IsError: true, }, nil, err // after return newErrorCallToolResult(deniedErr) ``` ### GitHub token/API URL helpers → `internal/envutil` - **`envutil.LookupGitHubToken()`** — canonical priority: `GITHUB_MCP_SERVER_TOKEN` → `GITHUB_TOKEN` → `GITHUB_PERSONAL_ACCESS_TOKEN` → `GH_TOKEN`. Fixes `cmd/proxy.go` which previously checked `GH_TOKEN` first (inconsistent with `unified.go`). - **`envutil.LookupGitHubAPIURL(defaultURL)`** — shared `GITHUB_API_URL` lookup with trailing-slash trim. - `lookupEnrichmentToken()` and `lookupGitHubAPIBaseURL()` in `unified.go` now delegate to these. - `DeriveGitHubAPIURL()` in `internal/proxy/proxy.go` now delegates the `GITHUB_API_URL` check to `envutil.LookupGitHubAPIURL`, keeping the `GITHUB_SERVER_URL` derivation as-is.
2 parents 62a46aa + ffaa655 commit f5c4b89

File tree

5 files changed

+146
-47
lines changed

5 files changed

+146
-47
lines changed

‎internal/cmd/proxy.go‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import (
1515

1616
"github.com/github/gh-aw-mcpg/internal/config"
1717
"github.com/github/gh-aw-mcpg/internal/difc"
18+
"github.com/github/gh-aw-mcpg/internal/envutil"
1819
"github.com/github/gh-aw-mcpg/internal/logger"
1920
"github.com/github/gh-aw-mcpg/internal/proxy"
2021
"github.com/github/gh-aw-mcpg/internal/tracing"
@@ -177,13 +178,7 @@ func runProxy(cmd *cobra.Command, args []string) error {
177178
// Resolve GitHub token (optional — proxy forwards client auth by default)
178179
token := proxyToken
179180
if token == "" {
180-
token = os.Getenv("GH_TOKEN")
181-
}
182-
if token == "" {
183-
token = os.Getenv("GITHUB_TOKEN")
184-
}
185-
if token == "" {
186-
token = os.Getenv("GITHUB_PERSONAL_ACCESS_TOKEN")
181+
token = envutil.LookupGitHubToken()
187182
}
188183
if token != "" {
189184
logger.LogInfo("startup", "Fallback GitHub token configured from flag/env")

‎internal/envutil/github.go‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
package envutil
2+
3+
import (
4+
"os"
5+
"strings"
6+
)
7+
8+
// LookupGitHubToken searches environment variables for a GitHub token using
9+
// a canonical priority order. It returns the first non-empty (trimmed) value
10+
// found, or an empty string if none is set.
11+
//
12+
// Priority order:
13+
// 1. GITHUB_MCP_SERVER_TOKEN
14+
// 2. GITHUB_TOKEN
15+
// 3. GITHUB_PERSONAL_ACCESS_TOKEN
16+
// 4. GH_TOKEN
17+
func LookupGitHubToken() string {
18+
for _, key := range []string{
19+
"GITHUB_MCP_SERVER_TOKEN",
20+
"GITHUB_TOKEN",
21+
"GITHUB_PERSONAL_ACCESS_TOKEN",
22+
"GH_TOKEN",
23+
} {
24+
if v := strings.TrimSpace(os.Getenv(key)); v != "" {
25+
return v
26+
}
27+
}
28+
return ""
29+
}
30+
31+
// LookupGitHubAPIURL returns the GitHub API base URL from the GITHUB_API_URL
32+
// environment variable. If the variable is not set or empty, it returns
33+
// defaultURL. Any trailing slash is stripped from the result.
34+
func LookupGitHubAPIURL(defaultURL string) string {
35+
if v := strings.TrimSpace(os.Getenv("GITHUB_API_URL")); v != "" {
36+
return strings.TrimRight(v, "/")
37+
}
38+
return strings.TrimRight(strings.TrimSpace(defaultURL), "/")
39+
}

‎internal/envutil/github_test.go‎

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,97 @@
1+
package envutil
2+
3+
import (
4+
"testing"
5+
6+
"github.com/stretchr/testify/assert"
7+
)
8+
9+
func TestLookupGitHubToken(t *testing.T) {
10+
tokenVars := []string{
11+
"GITHUB_MCP_SERVER_TOKEN",
12+
"GITHUB_TOKEN",
13+
"GITHUB_PERSONAL_ACCESS_TOKEN",
14+
"GH_TOKEN",
15+
}
16+
clearAll := func(t *testing.T) {
17+
t.Helper()
18+
for _, k := range tokenVars {
19+
t.Setenv(k, "")
20+
}
21+
}
22+
23+
t.Run("prefers GITHUB_MCP_SERVER_TOKEN", func(t *testing.T) {
24+
clearAll(t)
25+
t.Setenv("GITHUB_MCP_SERVER_TOKEN", "mcp-token")
26+
t.Setenv("GITHUB_TOKEN", "gh-token")
27+
t.Setenv("GH_TOKEN", "gh-cli-token")
28+
assert.Equal(t, "mcp-token", LookupGitHubToken())
29+
})
30+
31+
t.Run("falls back to GITHUB_TOKEN", func(t *testing.T) {
32+
clearAll(t)
33+
t.Setenv("GITHUB_TOKEN", "gh-token")
34+
t.Setenv("GH_TOKEN", "gh-cli-token")
35+
assert.Equal(t, "gh-token", LookupGitHubToken())
36+
})
37+
38+
t.Run("falls back to GITHUB_PERSONAL_ACCESS_TOKEN", func(t *testing.T) {
39+
clearAll(t)
40+
t.Setenv("GITHUB_PERSONAL_ACCESS_TOKEN", "pat-token")
41+
t.Setenv("GH_TOKEN", "gh-cli-token")
42+
assert.Equal(t, "pat-token", LookupGitHubToken())
43+
})
44+
45+
t.Run("falls back to GH_TOKEN", func(t *testing.T) {
46+
clearAll(t)
47+
t.Setenv("GH_TOKEN", "gh-cli-token")
48+
assert.Equal(t, "gh-cli-token", LookupGitHubToken())
49+
})
50+
51+
t.Run("returns empty when no token set", func(t *testing.T) {
52+
clearAll(t)
53+
assert.Equal(t, "", LookupGitHubToken())
54+
})
55+
56+
t.Run("trims whitespace", func(t *testing.T) {
57+
clearAll(t)
58+
t.Setenv("GITHUB_TOKEN", " trimmed ")
59+
assert.Equal(t, "trimmed", LookupGitHubToken())
60+
})
61+
62+
t.Run("skips whitespace-only values", func(t *testing.T) {
63+
clearAll(t)
64+
t.Setenv("GITHUB_MCP_SERVER_TOKEN", " ")
65+
t.Setenv("GITHUB_TOKEN", "actual-token")
66+
assert.Equal(t, "actual-token", LookupGitHubToken())
67+
})
68+
}
69+
70+
func TestLookupGitHubAPIURL(t *testing.T) {
71+
const defaultURL = "https://api.github.com"
72+
73+
t.Run("returns default when env not set", func(t *testing.T) {
74+
t.Setenv("GITHUB_API_URL", "")
75+
assert.Equal(t, defaultURL, LookupGitHubAPIURL(defaultURL))
76+
})
77+
78+
t.Run("returns custom URL from env", func(t *testing.T) {
79+
t.Setenv("GITHUB_API_URL", "https://github.example.com/api/v3")
80+
assert.Equal(t, "https://github.example.com/api/v3", LookupGitHubAPIURL(defaultURL))
81+
})
82+
83+
t.Run("strips trailing slash", func(t *testing.T) {
84+
t.Setenv("GITHUB_API_URL", "https://github.example.com/api/v3/")
85+
assert.Equal(t, "https://github.example.com/api/v3", LookupGitHubAPIURL(defaultURL))
86+
})
87+
88+
t.Run("returns default for whitespace-only env", func(t *testing.T) {
89+
t.Setenv("GITHUB_API_URL", " ")
90+
assert.Equal(t, defaultURL, LookupGitHubAPIURL(defaultURL))
91+
})
92+
93+
t.Run("trims whitespace before stripping trailing slash", func(t *testing.T) {
94+
t.Setenv("GITHUB_API_URL", " https://github.example.com/api/v3/ ")
95+
assert.Equal(t, "https://github.example.com/api/v3", LookupGitHubAPIURL(defaultURL))
96+
})
97+
}

‎internal/proxy/proxy.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import (
1919

2020
"github.com/github/gh-aw-mcpg/internal/config"
2121
"github.com/github/gh-aw-mcpg/internal/difc"
22+
"github.com/github/gh-aw-mcpg/internal/envutil"
2223
"github.com/github/gh-aw-mcpg/internal/guard"
2324
"github.com/github/gh-aw-mcpg/internal/logger"
2425
"github.com/github/gh-aw-mcpg/internal/tracing"
@@ -43,7 +44,7 @@ const (
4344
// - https://github.com → https://api.github.com
4445
// 3. Returns empty string if no env vars are set (caller uses DefaultGitHubAPIBase)
4546
func DeriveGitHubAPIURL() string {
46-
if apiURL := os.Getenv("GITHUB_API_URL"); apiURL != "" {
47+
if apiURL := envutil.LookupGitHubAPIURL(""); apiURL != "" {
4748
logProxy.Printf("GitHub API URL from GITHUB_API_URL: %s", apiURL)
4849
return apiURL
4950
}

‎internal/server/unified.go‎

Lines changed: 6 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,6 @@ import (
77
"io"
88
"log"
99
"net/http"
10-
"os"
11-
"strings"
1210
"sync"
1311
"time"
1412

@@ -18,6 +16,7 @@ import (
1816

1917
"github.com/github/gh-aw-mcpg/internal/config"
2018
"github.com/github/gh-aw-mcpg/internal/difc"
19+
"github.com/github/gh-aw-mcpg/internal/envutil"
2120
"github.com/github/gh-aw-mcpg/internal/guard"
2221
"github.com/github/gh-aw-mcpg/internal/launcher"
2322
"github.com/github/gh-aw-mcpg/internal/logger"
@@ -343,26 +342,13 @@ func (g *guardBackendCaller) callCollaboratorPermission(ctx context.Context, arg
343342
// lookupEnrichmentToken searches environment variables for a GitHub token
344343
// suitable for enrichment API calls.
345344
func lookupEnrichmentToken() string {
346-
for _, key := range []string{
347-
"GITHUB_MCP_SERVER_TOKEN",
348-
"GITHUB_TOKEN",
349-
"GITHUB_PERSONAL_ACCESS_TOKEN",
350-
"GH_TOKEN",
351-
} {
352-
if v := strings.TrimSpace(os.Getenv(key)); v != "" {
353-
return v
354-
}
355-
}
356-
return ""
345+
return envutil.LookupGitHubToken()
357346
}
358347

359348
// lookupGitHubAPIBaseURL returns the GitHub API base URL from environment
360349
// or defaults to https://api.github.com.
361350
func lookupGitHubAPIBaseURL() string {
362-
if v := os.Getenv("GITHUB_API_URL"); v != "" {
363-
return strings.TrimRight(v, "/")
364-
}
365-
return "https://api.github.com"
351+
return envutil.LookupGitHubAPIURL("https://api.github.com")
366352
}
367353

368354
// newErrorCallToolResult creates a standard error CallToolResult with the error message
@@ -462,12 +448,7 @@ func (us *UnifiedServer) callBackendTool(ctx context.Context, serverID, toolName
462448
deniedErr := fmt.Errorf("tool %q is not in the allowed-tools list for this server", toolName)
463449
toolSpan.RecordError(deniedErr)
464450
toolSpan.SetStatus(codes.Error, "tool not allowed")
465-
return &sdk.CallToolResult{
466-
IsError: true,
467-
Content: []sdk.Content{
468-
&sdk.TextContent{Text: deniedErr.Error()},
469-
},
470-
}, nil, deniedErr
451+
return newErrorCallToolResult(deniedErr)
471452
}
472453

473454
// Create backend caller for the guard
@@ -534,14 +515,7 @@ func (us *UnifiedServer) callBackendTool(ctx context.Context, serverID, toolName
534515
toolSpan.RecordError(detailedErr)
535516
toolSpan.SetStatus(codes.Error, "access denied: "+result.Reason)
536517
httpStatusCode = 403
537-
return &sdk.CallToolResult{
538-
Content: []sdk.Content{
539-
&sdk.TextContent{
540-
Text: detailedErr.Error(),
541-
},
542-
},
543-
IsError: true,
544-
}, nil, detailedErr
518+
return newErrorCallToolResult(detailedErr)
545519
}
546520
} else {
547521
log.Printf("[DIFC] Access ALLOWED for agent %s to %s", agentID, resource.Description)
@@ -602,14 +576,7 @@ func (us *UnifiedServer) callBackendTool(ctx context.Context, serverID, toolName
602576
blockErr := fmt.Errorf("DIFC policy violation: %d of %d items in response are not accessible to agent %s",
603577
filtered.GetFilteredCount(), filtered.TotalCount, agentID)
604578
httpStatusCode = 403
605-
return &sdk.CallToolResult{
606-
Content: []sdk.Content{
607-
&sdk.TextContent{
608-
Text: blockErr.Error(),
609-
},
610-
},
611-
IsError: true,
612-
}, nil, blockErr
579+
return newErrorCallToolResult(blockErr)
613580
}
614581

615582
if filtered.GetFilteredCount() > 0 {

0 commit comments

Comments
 (0)