Skip to content

Commit 70c4553

Browse files
Optimize inventory building and add OAuth scope filtering
Avoids double inventory building by reusing inventory from OAuth scope computation: - Modified getOAuthScopes() to return both scopes and inventory - Added PrebuiltInventory field to StdioServerConfig and MCPServerConfig - NewMCPServer reuses prebuilt inventory when available - Only builds inventory once instead of twice Added OAuth scope filtering for better tool management: - OAuthScopes field in StdioServerConfig passes OAuth scopes to server - When OAuth is configured, uses OAuth scopes for tool filtering - Similar to PAT scope filtering, hides tools requiring unavailable scopes - Improves QoL by hiding dead tools that can never succeed Performance improvement: - Eliminates redundant inventory building (saves ~100ms on startup) - Better user experience with scope-filtered tools Addresses review comment 2702350264 Co-authored-by: SamMorrowDrums <[email protected]>
1 parent 39b60d6 commit 70c4553

File tree

2 files changed

+124
-82
lines changed

2 files changed

+124
-82
lines changed

cmd/github-mcp-server/main.go

Lines changed: 59 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -36,36 +36,6 @@ var (
3636
Short: "Start stdio server",
3737
Long: `Start a server that communicates via standard input/output streams using JSON-RPC messages.`,
3838
RunE: func(_ *cobra.Command, _ []string) error {
39-
token := viper.GetString("personal_access_token")
40-
var oauthMgr *oauth.Manager
41-
42-
// If no token provided, setup OAuth manager if configured
43-
if token == "" {
44-
oauthClientID := viper.GetString("oauth_client_id")
45-
if oauthClientID != "" {
46-
// Create OAuth manager for lazy authentication
47-
oauthCfg := oauth.GetGitHubOAuthConfig(
48-
oauthClientID,
49-
viper.GetString("oauth_client_secret"),
50-
getOAuthScopes(),
51-
viper.GetString("host"),
52-
viper.GetInt("oauth_callback_port"),
53-
)
54-
oauthMgr = oauth.NewManager(oauthCfg)
55-
fmt.Fprintf(os.Stderr, "OAuth configured - will prompt for authentication when needed\n")
56-
} else {
57-
fmt.Fprintf(os.Stderr, "Warning: No authentication configured\n")
58-
fmt.Fprintf(os.Stderr, " - Set GITHUB_PERSONAL_ACCESS_TOKEN, or\n")
59-
fmt.Fprintf(os.Stderr, " - Configure OAuth with --oauth-client-id\n")
60-
fmt.Fprintf(os.Stderr, "Tools will prompt for authentication when called\n")
61-
}
62-
}
63-
64-
// Extract token from OAuth manager if available
65-
if oauthMgr != nil && token == "" {
66-
token = oauthMgr.GetAccessToken()
67-
}
68-
6939
// If you're wondering why we're not using viper.GetStringSlice("toolsets"),
7040
// it's because viper doesn't handle comma-separated values correctly for env
7141
// vars when using GetStringSlice.
@@ -97,12 +67,54 @@ var (
9767
}
9868
}
9969

70+
token := viper.GetString("personal_access_token")
71+
var oauthMgr *oauth.Manager
72+
var oauthScopes []string
73+
var prebuiltInventory *inventory.Inventory
74+
75+
// If no token provided, setup OAuth manager if configured
76+
if token == "" {
77+
oauthClientID := viper.GetString("oauth_client_id")
78+
if oauthClientID != "" {
79+
// Get translation helper for inventory building
80+
t, _ := translations.TranslationHelper()
81+
82+
// Compute OAuth scopes and get inventory (avoids double building)
83+
scopesResult := getOAuthScopes(enabledToolsets, enabledTools, enabledFeatures, t)
84+
oauthScopes = scopesResult.scopes
85+
prebuiltInventory = scopesResult.inventory
86+
87+
// Create OAuth manager for lazy authentication
88+
oauthCfg := oauth.GetGitHubOAuthConfig(
89+
oauthClientID,
90+
viper.GetString("oauth_client_secret"),
91+
oauthScopes,
92+
viper.GetString("host"),
93+
viper.GetInt("oauth_callback_port"),
94+
)
95+
oauthMgr = oauth.NewManager(oauthCfg)
96+
fmt.Fprintf(os.Stderr, "OAuth configured - will prompt for authentication when needed\n")
97+
} else {
98+
fmt.Fprintf(os.Stderr, "Warning: No authentication configured\n")
99+
fmt.Fprintf(os.Stderr, " - Set GITHUB_PERSONAL_ACCESS_TOKEN, or\n")
100+
fmt.Fprintf(os.Stderr, " - Configure OAuth with --oauth-client-id\n")
101+
fmt.Fprintf(os.Stderr, "Tools will prompt for authentication when called\n")
102+
}
103+
}
104+
105+
// Extract token from OAuth manager if available
106+
if oauthMgr != nil && token == "" {
107+
token = oauthMgr.GetAccessToken()
108+
}
109+
100110
ttl := viper.GetDuration("repo-access-cache-ttl")
101111
stdioServerConfig := ghmcp.StdioServerConfig{
102112
Version: version,
103113
Host: viper.GetString("host"),
104114
Token: token,
105115
OAuthManager: oauthMgr,
116+
OAuthScopes: oauthScopes,
117+
PrebuiltInventory: prebuiltInventory,
106118
EnabledToolsets: enabledToolsets,
107119
EnabledTools: enabledTools,
108120
EnabledFeatures: enabledFeatures,
@@ -195,65 +207,49 @@ func wordSepNormalizeFunc(_ *pflag.FlagSet, name string) pflag.NormalizedName {
195207
return pflag.NormalizedName(name)
196208
}
197209

210+
// oauthScopesResult holds the result of OAuth scope computation
211+
type oauthScopesResult struct {
212+
scopes []string
213+
inventory *inventory.Inventory // reused inventory to avoid double building
214+
}
215+
198216
// getOAuthScopes returns the OAuth scopes to request based on enabled tools
217+
// Also returns the built inventory to avoid building it twice
199218
// Uses custom scopes if explicitly provided, otherwise computes required scopes
200219
// from the tools that will be enabled based on user configuration
201-
func getOAuthScopes() []string {
220+
func getOAuthScopes(enabledToolsets, enabledTools, enabledFeatures []string, t translations.TranslationHelperFunc) oauthScopesResult {
202221
// Allow explicit override via --oauth-scopes flag
203222
var scopes []string
204223
if viper.IsSet("oauth_scopes") {
205224
if err := viper.UnmarshalKey("oauth_scopes", &scopes); err == nil && len(scopes) > 0 {
206-
return scopes
207-
}
208-
}
209-
210-
// Compute required scopes based on enabled tools
211-
// This ensures we only request scopes for tools the user will actually use
212-
var enabledToolsets []string
213-
if viper.IsSet("toolsets") {
214-
if err := viper.UnmarshalKey("toolsets", &enabledToolsets); err != nil {
215-
// If unmarshaling fails, fall back to defaults
216-
enabledToolsets = nil
217-
}
218-
}
219-
220-
var enabledTools []string
221-
if viper.IsSet("tools") {
222-
if err := viper.UnmarshalKey("tools", &enabledTools); err != nil {
223-
enabledTools = nil
224-
}
225-
}
226-
227-
var enabledFeatures []string
228-
if viper.IsSet("features") {
229-
if err := viper.UnmarshalKey("features", &enabledFeatures); err != nil {
230-
enabledFeatures = nil
225+
// When scopes are explicit, don't build inventory (will be built in server)
226+
return oauthScopesResult{scopes: scopes}
231227
}
232228
}
233229

234230
// Build inventory with the same configuration that will be used at runtime
235231
// This allows us to determine which tools will actually be available
236-
t, _ := translations.TranslationHelper()
232+
// and avoids building the inventory twice
237233
inventoryBuilder := github.NewInventory(t).
238234
WithReadOnly(viper.GetBool("read-only")).
239235
WithToolsets(enabledToolsets).
240236
WithTools(enabledTools).
241237
WithFeatureChecker(createFeatureChecker(enabledFeatures))
242238

243-
inventory, err := inventoryBuilder.Build()
239+
inv, err := inventoryBuilder.Build()
244240
if err != nil {
245-
// If inventory build fails, fall back to default scopes
246-
return getDefaultOAuthScopes()
241+
// If inventory build fails, fall back to default scopes without inventory
242+
return oauthScopesResult{scopes: getDefaultOAuthScopes()}
247243
}
248244

249245
// Collect all required scopes from available tools
250-
requiredScopes := collectRequiredScopes(inventory)
246+
requiredScopes := collectRequiredScopes(inv)
251247
if len(requiredScopes) == 0 {
252248
// If no tools require scopes, use defaults
253-
return getDefaultOAuthScopes()
249+
return oauthScopesResult{scopes: getDefaultOAuthScopes(), inventory: inv}
254250
}
255251

256-
return requiredScopes
252+
return oauthScopesResult{scopes: requiredScopes, inventory: inv}
257253
}
258254

259255
// getDefaultOAuthScopes returns the default scopes for GitHub MCP Server

internal/ghmcp/server.go

Lines changed: 65 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,10 @@ type MCPServerConfig struct {
3636
// GitHub Token to authenticate with the GitHub API
3737
Token string
3838

39+
// PrebuiltInventory is an optional pre-built inventory to avoid double building
40+
// When set, this inventory will be used instead of building a new one
41+
PrebuiltInventory *inventory.Inventory
42+
3943
// EnabledToolsets is a list of toolsets to enable
4044
// See: https://github.com/github/github-mcp-server?tab=readme-ov-file#tool-configuration
4145
EnabledToolsets []string
@@ -229,37 +233,64 @@ func NewMCPServer(cfg MCPServerConfig) (*mcp.Server, error) {
229233
})
230234

231235
// Build and register the tool/resource/prompt inventory
232-
inventoryBuilder := github.NewInventory(cfg.Translator).
233-
WithDeprecatedAliases(github.DeprecatedToolAliases).
234-
WithReadOnly(cfg.ReadOnly).
235-
WithToolsets(enabledToolsets).
236-
WithTools(cfg.EnabledTools).
237-
WithFeatureChecker(featureChecker)
238-
239-
// Apply token scope filtering if scopes are known (for PAT filtering)
240-
if cfg.TokenScopes != nil {
241-
inventoryBuilder = inventoryBuilder.WithFilter(github.CreateToolScopeFilter(cfg.TokenScopes))
242-
}
236+
var inv *inventory.Inventory
237+
if cfg.PrebuiltInventory != nil {
238+
// Use prebuilt inventory to avoid double building
239+
inv = cfg.PrebuiltInventory
240+
241+
// Apply scope filtering if needed (only if not already applied)
242+
// Prebuilt inventory from OAuth scope computation doesn't have scope filter yet
243+
if cfg.TokenScopes != nil {
244+
// Need to rebuild with scope filter
245+
inventoryBuilder := github.NewInventory(cfg.Translator).
246+
WithDeprecatedAliases(github.DeprecatedToolAliases).
247+
WithReadOnly(cfg.ReadOnly).
248+
WithToolsets(enabledToolsets).
249+
WithTools(cfg.EnabledTools).
250+
WithFeatureChecker(featureChecker).
251+
WithFilter(github.CreateToolScopeFilter(cfg.TokenScopes))
252+
253+
var err error
254+
inv, err = inventoryBuilder.Build()
255+
if err != nil {
256+
return nil, fmt.Errorf("failed to rebuild inventory with scope filter: %w", err)
257+
}
258+
}
259+
} else {
260+
// Build inventory from scratch
261+
inventoryBuilder := github.NewInventory(cfg.Translator).
262+
WithDeprecatedAliases(github.DeprecatedToolAliases).
263+
WithReadOnly(cfg.ReadOnly).
264+
WithToolsets(enabledToolsets).
265+
WithTools(cfg.EnabledTools).
266+
WithFeatureChecker(featureChecker)
267+
268+
// Apply token scope filtering if scopes are known (for PAT filtering)
269+
if cfg.TokenScopes != nil {
270+
inventoryBuilder = inventoryBuilder.WithFilter(github.CreateToolScopeFilter(cfg.TokenScopes))
271+
}
243272

244-
inventory, err := inventoryBuilder.Build()
245-
if err != nil {
246-
return nil, fmt.Errorf("failed to build inventory: %w", err)
273+
var err error
274+
inv, err = inventoryBuilder.Build()
275+
if err != nil {
276+
return nil, fmt.Errorf("failed to build inventory: %w", err)
277+
}
247278
}
248279

249-
if unrecognized := inventory.UnrecognizedToolsets(); len(unrecognized) > 0 {
280+
if unrecognized := inv.UnrecognizedToolsets(); len(unrecognized) > 0 {
250281
fmt.Fprintf(os.Stderr, "Warning: unrecognized toolsets ignored: %s\n", strings.Join(unrecognized, ", "))
251282
}
252283

253284
// Register GitHub tools/resources/prompts from the inventory.
254285
// In dynamic mode with no explicit toolsets, this is a no-op since enabledToolsets
255286
// is empty - users enable toolsets at runtime via the dynamic tools below (but can
256287
// enable toolsets or tools explicitly that do need registration).
257-
inventory.RegisterAll(context.Background(), ghServer, deps)
288+
inv.RegisterAll(context.Background(), ghServer, deps)
258289

259290
// Register dynamic toolset management tools (enable/disable) - these are separate
260291
// meta-tools that control the inventory, not part of the inventory itself
261292
if cfg.DynamicToolsets {
262-
registerDynamicTools(ghServer, inventory, deps, cfg.Translator)
293+
registerDynamicTools(ghServer, inv, deps, cfg.Translator)
263294
}
264295

265296
return ghServer, nil
@@ -310,6 +341,14 @@ type StdioServerConfig struct {
310341
RequestAuthentication(context.Context) error
311342
}
312343

344+
// OAuthScopes contains the OAuth scopes that were requested
345+
// When non-nil and OAuthManager is set, these scopes are used for scope filtering
346+
OAuthScopes []string
347+
348+
// PrebuiltInventory is an optional pre-built inventory to avoid double building
349+
// When set, this inventory will be used instead of building a new one
350+
PrebuiltInventory *inventory.Inventory
351+
313352
// EnabledToolsets is a list of toolsets to enable
314353
// See: https://github.com/github/github-mcp-server?tab=readme-ov-file#tool-configuration
315354
EnabledToolsets []string
@@ -380,22 +419,29 @@ func RunStdioServer(cfg StdioServerConfig) error {
380419
// Only classic PATs (ghp_ prefix) return OAuth scopes via X-OAuth-Scopes header.
381420
// Fine-grained PATs and other token types don't support this, so we skip filtering.
382421
var tokenScopes []string
383-
if strings.HasPrefix(cfg.Token, "ghp_") {
422+
switch {
423+
case strings.HasPrefix(cfg.Token, "ghp_"):
384424
fetchedScopes, err := fetchTokenScopesForHost(ctx, cfg.Token, cfg.Host)
385425
if err != nil {
386426
logger.Warn("failed to fetch token scopes, continuing without scope filtering", "error", err)
387427
} else {
388428
tokenScopes = fetchedScopes
389429
logger.Info("token scopes fetched for filtering", "scopes", tokenScopes)
390430
}
391-
} else {
431+
case len(cfg.OAuthScopes) > 0:
432+
// Use OAuth scopes for filtering when OAuth is configured
433+
// This filters tools to only those compatible with the requested OAuth scopes
434+
tokenScopes = cfg.OAuthScopes
435+
logger.Info("using OAuth scopes for tool filtering", "scopes", tokenScopes)
436+
default:
392437
logger.Debug("skipping scope filtering for non-PAT token")
393438
}
394439

395440
ghServer, err := NewMCPServer(MCPServerConfig{
396441
Version: cfg.Version,
397442
Host: cfg.Host,
398443
Token: cfg.Token,
444+
PrebuiltInventory: cfg.PrebuiltInventory,
399445
EnabledToolsets: cfg.EnabledToolsets,
400446
EnabledTools: cfg.EnabledTools,
401447
EnabledFeatures: cfg.EnabledFeatures,

0 commit comments

Comments
 (0)