Skip to content

Commit a536c15

Browse files
SamMorrowDrumsSamMorrowDrums
committed
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 a536c15

File tree

11 files changed

+551
-543
lines changed

11 files changed

+551
-543
lines changed

‎cmd/github-mcp-server/generate_docs.go‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -51,13 +51,13 @@ func generateReadmeDocs(readmePath string) error {
5151
t, _ := translations.TranslationHelper()
5252

5353
// Create toolset group - stateless, no dependencies needed for doc generation
54-
tsg := github.NewToolsetGroup(t)
54+
r := github.NewRegistry(t)
5555

5656
// Generate toolsets documentation
57-
toolsetsDoc := generateToolsetsDoc(tsg)
57+
toolsetsDoc := generateToolsetsDoc(r)
5858

5959
// Generate tools documentation
60-
toolsDoc := generateToolsDoc(tsg)
60+
toolsDoc := generateToolsDoc(r)
6161

6262
// Read the current README.md
6363
// #nosec G304 - readmePath is controlled by command line flag, not user input
@@ -104,7 +104,7 @@ func generateRemoteServerDocs(docsPath string) error {
104104
return os.WriteFile(docsPath, []byte(updatedContent), 0600) //#nosec G306
105105
}
106106

107-
func generateToolsetsDoc(tsg *toolsets.ToolsetGroup) string {
107+
func generateToolsetsDoc(r *toolsets.Registry) string {
108108
var buf strings.Builder
109109

110110
// Add table header and separator
@@ -116,17 +116,17 @@ func generateToolsetsDoc(tsg *toolsets.ToolsetGroup) string {
116116

117117
// AvailableToolsets() returns toolsets that have tools, sorted by ID
118118
// Exclude context (custom description above) and dynamic (internal only)
119-
for _, ts := range tsg.AvailableToolsets("context", "dynamic") {
119+
for _, ts := range r.AvailableToolsets("context", "dynamic") {
120120
fmt.Fprintf(&buf, "| `%s` | %s |\n", ts.ID, ts.Description)
121121
}
122122

123123
return strings.TrimSuffix(buf.String(), "\n")
124124
}
125125

126-
func generateToolsDoc(tsg *toolsets.ToolsetGroup) string {
126+
func generateToolsDoc(r *toolsets.Registry) string {
127127
// AllTools() returns tools sorted by toolset ID then tool name.
128128
// We iterate once, grouping by toolset as we encounter them.
129-
tools := tsg.AllTools()
129+
tools := r.AllTools()
130130
if len(tools) == 0 {
131131
return ""
132132
}
@@ -300,7 +300,7 @@ func generateRemoteToolsetsDoc() string {
300300
t, _ := translations.TranslationHelper()
301301

302302
// Create toolset group - stateless
303-
tsg := github.NewToolsetGroup(t)
303+
r := github.NewRegistry(t)
304304

305305
// Generate table header
306306
buf.WriteString("| Name | Description | API URL | 1-Click Install (VS Code) | Read-only Link | 1-Click Read-only Install (VS Code) |\n")
@@ -311,7 +311,7 @@ func generateRemoteToolsetsDoc() string {
311311

312312
// AvailableToolsets() returns toolsets that have tools, sorted by ID
313313
// Exclude context (handled separately) and dynamic (internal only)
314-
for _, ts := range tsg.AvailableToolsets("context", "dynamic") {
314+
for _, ts := range r.AvailableToolsets("context", "dynamic") {
315315
idStr := string(ts.ID)
316316

317317
formattedName := formatToolsetName(idStr)

‎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.NewRegistry(translations.NullTranslationHelper).DefaultToolsetIDs()
182182
}
183183

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

‎internal/ghmcp/server.go‎

Lines changed: 13 additions & 13 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
@@ -163,7 +158,7 @@ func NewMCPServer(cfg MCPServerConfig) (*mcp.Server, error) {
163158
}
164159

165160
// Create toolset group with all tools, resources, and prompts (stateless)
166-
tsg := github.NewToolsetGroup(cfg.Translator)
161+
r := github.NewRegistry(cfg.Translator)
167162

168163
// Clean tool names (WithTools will resolve any deprecated aliases)
169164
enabledTools := github.CleanTools(cfg.EnabledTools)
@@ -174,13 +169,18 @@ func NewMCPServer(cfg MCPServerConfig) (*mcp.Server, error) {
174169
// - WithToolsets: nil=defaults, empty=none, handles "all"/"default" keywords
175170
// - WithTools: additional tools that bypass toolset filtering (additive, resolves aliases)
176171
// - WithFeatureChecker: filters based on feature flags
177-
filteredTsg := tsg.
172+
filteredTsg := r.
178173
WithDeprecatedToolAliases(github.DeprecatedToolAliases).
179174
WithReadOnly(cfg.ReadOnly).
180175
WithToolsets(enabledToolsets).
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)
@@ -191,12 +191,12 @@ func NewMCPServer(cfg MCPServerConfig) (*mcp.Server, error) {
191191
// so dynamic tools can enable any toolset at runtime.
192192
if cfg.DynamicToolsets {
193193
dynamicDeps := github.DynamicToolDependencies{
194-
Server: ghServer,
195-
ToolsetGroup: filteredTsg,
196-
ToolDeps: deps,
197-
T: cfg.Translator,
194+
Server: ghServer,
195+
Registry: filteredTsg,
196+
ToolDeps: deps,
197+
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: 23 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -13,13 +13,13 @@ import (
1313
)
1414

1515
// DynamicToolDependencies contains dependencies for dynamic toolset management tools.
16-
// It includes the managed ToolsetGroup, the server for registration, and the deps
16+
// It includes the managed Registry, the server for registration, and the deps
1717
// that will be passed to tools when they are dynamically enabled.
1818
type DynamicToolDependencies struct {
1919
// Server is the MCP server to register tools with
2020
Server *mcp.Server
21-
// ToolsetGroup contains all available tools that can be enabled dynamically
22-
ToolsetGroup *toolsets.ToolsetGroup
21+
// Registry contains all available tools that can be enabled dynamically
22+
Registry *toolsets.Registry
2323
// ToolDeps are the dependencies passed to tools when they are registered
2424
ToolDeps any
2525
// T is the translation helper function
@@ -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(r *toolsets.Registry) []any {
38+
toolsetIDs := r.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 r parameter provides the available toolset IDs for JSON Schema enums.
49+
func DynamicTools(r *toolsets.Registry) []toolsets.ServerTool {
6050
return []toolsets.ServerTool{
6151
ListAvailableToolsets(),
62-
GetToolsetsTools(),
63-
EnableToolset(),
52+
GetToolsetsTools(r),
53+
EnableToolset(r),
6454
}
6555
}
6656

6757
// EnableToolset creates a tool that enables a toolset at runtime.
68-
func EnableToolset() toolsets.ServerTool {
58+
func EnableToolset(r *toolsets.Registry) 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(r),
8575
},
8676
},
8777
Required: []string{"toolset"},
@@ -96,19 +86,19 @@ func EnableToolset() toolsets.ServerTool {
9686

9787
toolsetID := toolsets.ToolsetID(toolsetName)
9888

99-
if !deps.ToolsetGroup.HasToolset(toolsetID) {
89+
if !deps.Registry.HasToolset(toolsetID) {
10090
return utils.NewToolResultError(fmt.Sprintf("Toolset %s not found", toolsetName)), nil, nil
10191
}
10292

103-
if deps.ToolsetGroup.IsToolsetEnabled(toolsetID) {
93+
if deps.Registry.IsToolsetEnabled(toolsetID) {
10494
return utils.NewToolResultText(fmt.Sprintf("Toolset %s is already enabled", toolsetName)), nil, nil
10595
}
10696

10797
// Mark the toolset as enabled so IsToolsetEnabled returns true
108-
deps.ToolsetGroup.EnableToolset(toolsetID)
98+
deps.Registry.EnableToolset(toolsetID)
10999

110100
// Get tools for this toolset and register them with the managed deps
111-
toolsForToolset := deps.ToolsetGroup.ToolsForToolset(toolsetID)
101+
toolsForToolset := deps.Registry.ToolsForToolset(toolsetID)
112102
for _, st := range toolsForToolset {
113103
st.RegisterFunc(deps.Server, deps.ToolDeps)
114104
}
@@ -137,16 +127,16 @@ func ListAvailableToolsets() toolsets.ServerTool {
137127
},
138128
func(deps DynamicToolDependencies) mcp.ToolHandlerFor[map[string]any, any] {
139129
return func(_ context.Context, _ *mcp.CallToolRequest, _ map[string]any) (*mcp.CallToolResult, any, error) {
140-
toolsetIDs := deps.ToolsetGroup.ToolsetIDs()
141-
descriptions := deps.ToolsetGroup.ToolsetDescriptions()
130+
toolsetIDs := deps.Registry.ToolsetIDs()
131+
descriptions := deps.Registry.ToolsetDescriptions()
142132

143133
payload := make([]map[string]string, 0, len(toolsetIDs))
144134
for _, id := range toolsetIDs {
145135
t := map[string]string{
146136
"name": string(id),
147137
"description": descriptions[id],
148138
"can_enable": "true",
149-
"currently_enabled": fmt.Sprintf("%t", deps.ToolsetGroup.IsToolsetEnabled(id)),
139+
"currently_enabled": fmt.Sprintf("%t", deps.Registry.IsToolsetEnabled(id)),
150140
}
151141
payload = append(payload, t)
152142
}
@@ -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(r *toolsets.Registry) 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(r),
183173
},
184174
},
185175
Required: []string{"toolset"},
@@ -194,12 +184,12 @@ func GetToolsetsTools() toolsets.ServerTool {
194184

195185
toolsetID := toolsets.ToolsetID(toolsetName)
196186

197-
if !deps.ToolsetGroup.HasToolset(toolsetID) {
187+
if !deps.Registry.HasToolset(toolsetID) {
198188
return utils.NewToolResultError(fmt.Sprintf("Toolset %s not found", toolsetName)), nil, nil
199189
}
200190

201191
// Get all tools for this toolset (ignoring current filters for discovery)
202-
toolsInToolset := deps.ToolsetGroup.ToolsForToolset(toolsetID)
192+
toolsInToolset := deps.Registry.ToolsForToolset(toolsetID)
203193
payload := make([]map[string]string, 0, len(toolsInToolset))
204194

205195
for _, st := range toolsInToolset {

0 commit comments

Comments
 (0)