Skip to content

Commit 170b8ad

Browse files
refactor: consolidate toolset validation into ToolsetGroup
- Add Default field to ToolsetMetadata and derive defaults from metadata - Move toolset validation into WithToolsets (trims whitespace, dedupes, tracks unrecognized) - Add UnrecognizedToolsets() method for warning about typos - Add DefaultToolsetIDs() method to derive defaults from metadata - Remove redundant functions: CleanToolsets, GetValidToolsetIDs, AvailableToolsets, GetDefaultToolsetIDs - Update DynamicTools to take ToolsetGroup for schema enum generation - Add stubTranslator for cases needing ToolsetGroup without translations This eliminates hardcoded toolset lists - everything is now derived from the actual registered tools and their metadata.
1 parent 2142b02 commit 170b8ad

File tree

10 files changed

+224
-298
lines changed

10 files changed

+224
-298
lines changed

e2e/e2e_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -178,7 +178,7 @@ func setupMCPClient(t *testing.T, options ...clientOption) *mcp.ClientSession {
178178
// so that there is a shared setup mechanism, but let's wait till we feel more friction.
179179
enabledToolsets := opts.enabledToolsets
180180
if enabledToolsets == nil {
181-
enabledToolsets = github.GetDefaultToolsetIDs()
181+
enabledToolsets = github.NewToolsetGroup(translations.NullTranslationHelper).DefaultToolsetIDs()
182182
}
183183

184184
ghServer, err := ghmcp.NewMCPServer(ghmcp.MCPServerConfig{

internal/ghmcp/server.go

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -109,12 +109,7 @@ func NewMCPServer(cfg MCPServerConfig) (*mcp.Server, error) {
109109
// - explicit list means "use these toolsets"
110110
var enabledToolsets []string
111111
if cfg.EnabledToolsets != nil {
112-
// Clean up explicitly passed toolsets (removes duplicates, whitespace)
113-
var invalidToolsets []string
114-
enabledToolsets, invalidToolsets = github.CleanToolsets(cfg.EnabledToolsets)
115-
if len(invalidToolsets) > 0 {
116-
fmt.Fprintf(os.Stderr, "Invalid toolsets ignored: %s\n", strings.Join(invalidToolsets, ", "))
117-
}
112+
enabledToolsets = cfg.EnabledToolsets
118113
} else if cfg.DynamicToolsets {
119114
// Dynamic mode with no toolsets specified: start with no toolsets enabled
120115
// so users can enable them on demand via the dynamic tools
@@ -181,6 +176,11 @@ func NewMCPServer(cfg MCPServerConfig) (*mcp.Server, error) {
181176
WithTools(enabledTools).
182177
WithFeatureChecker(createFeatureChecker(cfg.EnabledFeatures))
183178

179+
// Warn about unrecognized toolset names (likely typos)
180+
if unrecognized := filteredTsg.UnrecognizedToolsets(); len(unrecognized) > 0 {
181+
fmt.Fprintf(os.Stderr, "Warning: unrecognized toolsets ignored: %s\n", strings.Join(unrecognized, ", "))
182+
}
183+
184184
// Register all mcp functionality with the server
185185
// Use background context for local server (no per-request actor context)
186186
filteredTsg.RegisterAll(context.Background(), ghServer, deps)
@@ -196,7 +196,7 @@ func NewMCPServer(cfg MCPServerConfig) (*mcp.Server, error) {
196196
ToolDeps: deps,
197197
T: cfg.Translator,
198198
}
199-
dynamicTools := github.DynamicTools()
199+
dynamicTools := github.DynamicTools(filteredTsg)
200200
for _, tool := range dynamicTools {
201201
tool.RegisterFunc(ghServer, dynamicDeps)
202202
}

pkg/github/dynamic_tools.go

Lines changed: 11 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -33,20 +33,9 @@ func NewDynamicTool(toolset toolsets.ToolsetMetadata, tool mcp.Tool, handler fun
3333
})
3434
}
3535

36-
// AllToolsetIDsEnum returns all available toolset IDs as an enum for JSON Schema.
37-
func AllToolsetIDsEnum() []any {
38-
toolsets := AvailableToolsets()
39-
result := make([]any, len(toolsets))
40-
for i, ts := range toolsets {
41-
result[i] = ts.ID
42-
}
43-
return result
44-
}
45-
46-
// ToolsetEnum returns the list of toolset IDs as an enum for JSON Schema.
47-
// Deprecated: Use AllToolsetIDsEnum() instead.
48-
func ToolsetEnum(toolsetGroup *toolsets.ToolsetGroup) []any {
49-
toolsetIDs := toolsetGroup.ToolsetIDs()
36+
// toolsetIDsEnum returns the list of toolset IDs as an enum for JSON Schema.
37+
func toolsetIDsEnum(tsg *toolsets.ToolsetGroup) []any {
38+
toolsetIDs := tsg.ToolsetIDs()
5039
result := make([]any, len(toolsetIDs))
5140
for i, id := range toolsetIDs {
5241
result[i] = id
@@ -56,16 +45,17 @@ func ToolsetEnum(toolsetGroup *toolsets.ToolsetGroup) []any {
5645

5746
// DynamicTools returns the tools for dynamic toolset management.
5847
// These tools allow runtime discovery and enablement of toolsets.
59-
func DynamicTools() []toolsets.ServerTool {
48+
// The tsg parameter provides the available toolset IDs for JSON Schema enums.
49+
func DynamicTools(tsg *toolsets.ToolsetGroup) []toolsets.ServerTool {
6050
return []toolsets.ServerTool{
6151
ListAvailableToolsets(),
62-
GetToolsetsTools(),
63-
EnableToolset(),
52+
GetToolsetsTools(tsg),
53+
EnableToolset(tsg),
6454
}
6555
}
6656

6757
// EnableToolset creates a tool that enables a toolset at runtime.
68-
func EnableToolset() toolsets.ServerTool {
58+
func EnableToolset(tsg *toolsets.ToolsetGroup) toolsets.ServerTool {
6959
return NewDynamicTool(
7060
ToolsetMetadataDynamic,
7161
mcp.Tool{
@@ -81,7 +71,7 @@ func EnableToolset() toolsets.ServerTool {
8171
"toolset": {
8272
Type: "string",
8373
Description: "The name of the toolset to enable",
84-
Enum: AllToolsetIDsEnum(),
74+
Enum: toolsetIDsEnum(tsg),
8575
},
8676
},
8777
Required: []string{"toolset"},
@@ -163,7 +153,7 @@ func ListAvailableToolsets() toolsets.ServerTool {
163153
}
164154

165155
// GetToolsetsTools creates a tool that lists all tools in a specific toolset.
166-
func GetToolsetsTools() toolsets.ServerTool {
156+
func GetToolsetsTools(tsg *toolsets.ToolsetGroup) toolsets.ServerTool {
167157
return NewDynamicTool(
168158
ToolsetMetadataDynamic,
169159
mcp.Tool{
@@ -179,7 +169,7 @@ func GetToolsetsTools() toolsets.ServerTool {
179169
"toolset": {
180170
Type: "string",
181171
Description: "The name of the toolset you want to get the tools for",
182-
Enum: AllToolsetIDsEnum(),
172+
Enum: toolsetIDsEnum(tsg),
183173
},
184174
},
185175
Required: []string{"toolset"},

pkg/github/tools.go

Lines changed: 21 additions & 83 deletions
Original file line numberDiff line numberDiff line change
@@ -28,10 +28,12 @@ var (
2828
ToolsetMetadataContext = toolsets.ToolsetMetadata{
2929
ID: "context",
3030
Description: "Tools that provide context about the current user and GitHub context you are operating in",
31+
Default: true,
3132
}
3233
ToolsetMetadataRepos = toolsets.ToolsetMetadata{
3334
ID: "repos",
3435
Description: "GitHub Repository related tools",
36+
Default: true,
3537
}
3638
ToolsetMetadataGit = toolsets.ToolsetMetadata{
3739
ID: "git",
@@ -40,14 +42,17 @@ var (
4042
ToolsetMetadataIssues = toolsets.ToolsetMetadata{
4143
ID: "issues",
4244
Description: "GitHub Issues related tools",
45+
Default: true,
4346
}
4447
ToolsetMetadataPullRequests = toolsets.ToolsetMetadata{
4548
ID: "pull_requests",
4649
Description: "GitHub Pull Request related tools",
50+
Default: true,
4751
}
4852
ToolsetMetadataUsers = toolsets.ToolsetMetadata{
4953
ID: "users",
5054
Description: "GitHub User related tools",
55+
Default: true,
5156
}
5257
ToolsetMetadataOrgs = toolsets.ToolsetMetadata{
5358
ID: "orgs",
@@ -107,53 +112,6 @@ var (
107112
}
108113
)
109114

110-
func AvailableToolsets() []toolsets.ToolsetMetadata {
111-
return []toolsets.ToolsetMetadata{
112-
ToolsetMetadataContext,
113-
ToolsetMetadataRepos,
114-
ToolsetMetadataGit,
115-
ToolsetMetadataIssues,
116-
ToolsetMetadataPullRequests,
117-
ToolsetMetadataUsers,
118-
ToolsetMetadataOrgs,
119-
ToolsetMetadataActions,
120-
ToolsetMetadataCodeSecurity,
121-
ToolsetMetadataSecretProtection,
122-
ToolsetMetadataDependabot,
123-
ToolsetMetadataNotifications,
124-
ToolsetMetadataExperiments,
125-
ToolsetMetadataDiscussions,
126-
ToolsetMetadataGists,
127-
ToolsetMetadataSecurityAdvisories,
128-
ToolsetMetadataProjects,
129-
ToolsetMetadataStargazers,
130-
ToolsetMetadataDynamic,
131-
ToolsetLabels,
132-
}
133-
}
134-
135-
// GetValidToolsetIDs returns a map of all valid toolset IDs for quick lookup
136-
func GetValidToolsetIDs() map[toolsets.ToolsetID]bool {
137-
validIDs := make(map[toolsets.ToolsetID]bool)
138-
for _, toolset := range AvailableToolsets() {
139-
validIDs[toolset.ID] = true
140-
}
141-
// Add special keywords
142-
validIDs[ToolsetMetadataAll.ID] = true
143-
validIDs[ToolsetMetadataDefault.ID] = true
144-
return validIDs
145-
}
146-
147-
func GetDefaultToolsetIDs() []toolsets.ToolsetID {
148-
return []toolsets.ToolsetID{
149-
ToolsetMetadataContext.ID,
150-
ToolsetMetadataRepos.ID,
151-
ToolsetMetadataIssues.ID,
152-
ToolsetMetadataPullRequests.ID,
153-
ToolsetMetadataUsers.ID,
154-
}
155-
}
156-
157115
// AllTools returns all tools with their embedded toolset metadata.
158116
// Tool functions return ServerTool directly with toolset info.
159117
func AllTools(t translations.TranslationHelperFunc) []toolsets.ServerTool {
@@ -304,16 +262,19 @@ func ToStringPtr(s string) *string {
304262

305263
// GenerateToolsetsHelp generates the help text for the toolsets flag
306264
func GenerateToolsetsHelp() string {
307-
// Format default tools
308-
defaultIDs := GetDefaultToolsetIDs()
265+
// Get toolset group to derive defaults and available toolsets
266+
tsg := NewToolsetGroup(stubTranslator)
267+
268+
// Format default tools from metadata
269+
defaultIDs := tsg.DefaultToolsetIDs()
309270
defaultStrings := make([]string, len(defaultIDs))
310271
for i, id := range defaultIDs {
311272
defaultStrings[i] = string(id)
312273
}
313274
defaultTools := strings.Join(defaultStrings, ", ")
314275

315-
// Format available tools with line breaks for better readability
316-
allToolsets := AvailableToolsets()
276+
// Get all available toolsets (excludes context and dynamic for display)
277+
allToolsets := tsg.AvailableToolsets("context", "dynamic")
317278
var availableToolsLines []string
318279
const maxLineLength = 70
319280
currentLine := ""
@@ -349,6 +310,10 @@ func GenerateToolsetsHelp() string {
349310
return toolsetsHelp
350311
}
351312

313+
// stubTranslator is a passthrough translator for cases where we need a ToolsetGroup
314+
// but don't need actual translations (e.g., getting toolset IDs for CLI help).
315+
func stubTranslator(_, fallback string) string { return fallback }
316+
352317
// AddDefaultToolset removes the default toolset and expands it to the actual default toolset IDs
353318
func AddDefaultToolset(result []string) []string {
354319
hasDefault := false
@@ -367,43 +332,16 @@ func AddDefaultToolset(result []string) []string {
367332

368333
result = RemoveToolset(result, string(ToolsetMetadataDefault.ID))
369334

370-
for _, defaultToolset := range GetDefaultToolsetIDs() {
371-
if !seen[string(defaultToolset)] {
372-
result = append(result, string(defaultToolset))
335+
// Get default toolset IDs from the ToolsetGroup
336+
tsg := NewToolsetGroup(stubTranslator)
337+
for _, id := range tsg.DefaultToolsetIDs() {
338+
if !seen[string(id)] {
339+
result = append(result, string(id))
373340
}
374341
}
375342
return result
376343
}
377344

378-
// cleanToolsets cleans and handles special toolset keywords:
379-
// - Duplicates are removed from the result
380-
// - Removes whitespaces
381-
// - Validates toolset names and returns invalid ones separately - for warning reporting
382-
// Returns: (toolsets, invalidToolsets)
383-
func CleanToolsets(enabledToolsets []string) ([]string, []string) {
384-
seen := make(map[string]bool)
385-
result := make([]string, 0, len(enabledToolsets))
386-
invalid := make([]string, 0)
387-
validIDs := GetValidToolsetIDs()
388-
389-
// Add non-default toolsets, removing duplicates and trimming whitespace
390-
for _, toolset := range enabledToolsets {
391-
trimmed := strings.TrimSpace(toolset)
392-
if trimmed == "" {
393-
continue
394-
}
395-
if !seen[trimmed] {
396-
seen[trimmed] = true
397-
result = append(result, trimmed)
398-
if !validIDs[toolsets.ToolsetID(trimmed)] {
399-
invalid = append(invalid, trimmed)
400-
}
401-
}
402-
}
403-
404-
return result, invalid
405-
}
406-
407345
func RemoveToolset(tools []string, toRemove string) []string {
408346
result := make([]string, 0, len(tools))
409347
for _, tool := range tools {

0 commit comments

Comments
 (0)