Skip to content

Commit 19beb33

Browse files
Bug fix: invalid tool should error out (#1776)
* get it working * clean up approach by moving cleantools inside builder, this simplifies remote server too * add tests for trimming and deduplication * error out in the builder if there are unrecognized tools --------- Co-authored-by: Sam Morrow <info@sam-morrow.com>
1 parent 3ea9bfc commit 19beb33

File tree

9 files changed

+292
-140
lines changed

9 files changed

+292
-140
lines changed

cmd/github-mcp-server/generate_docs.go

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

5353
// (not available to regular users) while including tools with FeatureFlagDisable.
54-
r := github.NewInventory(t).WithToolsets([]string{"all"}).Build()
54+
// Build() can only fail if WithTools specifies invalid tools - not used here
55+
r, _ := github.NewInventory(t).WithToolsets([]string{"all"}).Build()
5556

5657
// Generate toolsets documentation
5758
toolsetsDoc := generateToolsetsDoc(r)
@@ -341,7 +342,8 @@ func generateRemoteToolsetsDoc() string {
341342
t, _ := translations.TranslationHelper()
342343

343344
// Build inventory - stateless
344-
r := github.NewInventory(t).Build()
345+
// Build() can only fail if WithTools specifies invalid tools - not used here
346+
r, _ := github.NewInventory(t).Build()
345347

346348
// Generate table header (icon is combined with Name column)
347349
buf.WriteString("| Name | Description | API URL | 1-Click Install (VS Code) | Read-only Link | 1-Click Read-only Install (VS Code) |\n")

cmd/github-mcp-server/list_scopes.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,10 @@ func runListScopes() error {
121121
inventoryBuilder = inventoryBuilder.WithTools(enabledTools)
122122
}
123123

124-
inv := inventoryBuilder.Build()
124+
inv, err := inventoryBuilder.Build()
125+
if err != nil {
126+
return fmt.Errorf("failed to build inventory: %w", err)
127+
}
125128

126129
// Collect all tools and their scopes
127130
output := collectToolScopes(inv, readOnly)

internal/ghmcp/server.go

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -221,15 +221,18 @@ func NewMCPServer(cfg MCPServerConfig) (*mcp.Server, error) {
221221
WithDeprecatedAliases(github.DeprecatedToolAliases).
222222
WithReadOnly(cfg.ReadOnly).
223223
WithToolsets(enabledToolsets).
224-
WithTools(github.CleanTools(cfg.EnabledTools)).
224+
WithTools(cfg.EnabledTools).
225225
WithFeatureChecker(createFeatureChecker(cfg.EnabledFeatures))
226226

227227
// Apply token scope filtering if scopes are known (for PAT filtering)
228228
if cfg.TokenScopes != nil {
229229
inventoryBuilder = inventoryBuilder.WithFilter(github.CreateToolScopeFilter(cfg.TokenScopes))
230230
}
231231

232-
inventory := inventoryBuilder.Build()
232+
inventory, err := inventoryBuilder.Build()
233+
if err != nil {
234+
return nil, fmt.Errorf("failed to build inventory: %w", err)
235+
}
233236

234237
if unrecognized := inventory.UnrecognizedToolsets(); len(unrecognized) > 0 {
235238
fmt.Fprintf(os.Stderr, "Warning: unrecognized toolsets ignored: %s\n", strings.Join(unrecognized, ", "))

pkg/github/dynamic_tools_test.go

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -25,9 +25,10 @@ func createDynamicRequest(args map[string]any) *mcp.CallToolRequest {
2525

2626
func TestDynamicTools_ListAvailableToolsets(t *testing.T) {
2727
// Build a registry with no toolsets enabled (dynamic mode)
28-
reg := NewInventory(translations.NullTranslationHelper).
28+
reg, err := NewInventory(translations.NullTranslationHelper).
2929
WithToolsets([]string{}).
3030
Build()
31+
require.NoError(t, err)
3132

3233
// Create a mock server
3334
server := mcp.NewServer(&mcp.Implementation{Name: "test"}, nil)
@@ -73,9 +74,10 @@ func TestDynamicTools_ListAvailableToolsets(t *testing.T) {
7374

7475
func TestDynamicTools_GetToolsetTools(t *testing.T) {
7576
// Build a registry with no toolsets enabled (dynamic mode)
76-
reg := NewInventory(translations.NullTranslationHelper).
77+
reg, err := NewInventory(translations.NullTranslationHelper).
7778
WithToolsets([]string{}).
7879
Build()
80+
require.NoError(t, err)
7981

8082
// Create a mock server
8183
server := mcp.NewServer(&mcp.Implementation{Name: "test"}, nil)
@@ -122,9 +124,10 @@ func TestDynamicTools_GetToolsetTools(t *testing.T) {
122124

123125
func TestDynamicTools_EnableToolset(t *testing.T) {
124126
// Build a registry with no toolsets enabled (dynamic mode)
125-
reg := NewInventory(translations.NullTranslationHelper).
127+
reg, err := NewInventory(translations.NullTranslationHelper).
126128
WithToolsets([]string{}).
127129
Build()
130+
require.NoError(t, err)
128131

129132
// Create a mock server
130133
server := mcp.NewServer(&mcp.Implementation{Name: "test"}, nil)
@@ -170,9 +173,10 @@ func TestDynamicTools_EnableToolset(t *testing.T) {
170173

171174
func TestDynamicTools_EnableToolset_InvalidToolset(t *testing.T) {
172175
// Build a registry with no toolsets enabled (dynamic mode)
173-
reg := NewInventory(translations.NullTranslationHelper).
176+
reg, err := NewInventory(translations.NullTranslationHelper).
174177
WithToolsets([]string{}).
175178
Build()
179+
require.NoError(t, err)
176180

177181
// Create a mock server
178182
server := mcp.NewServer(&mcp.Implementation{Name: "test"}, nil)
@@ -203,7 +207,8 @@ func TestDynamicTools_EnableToolset_InvalidToolset(t *testing.T) {
203207

204208
func TestDynamicTools_ToolsetsEnum(t *testing.T) {
205209
// Build a registry
206-
reg := NewInventory(translations.NullTranslationHelper).Build()
210+
reg, err := NewInventory(translations.NullTranslationHelper).Build()
211+
require.NoError(t, err)
207212

208213
// Get tools to verify they have proper enum values
209214
tools := DynamicTools(reg)

pkg/github/scope_filter_test.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -167,11 +167,12 @@ func TestCreateToolScopeFilter_Integration(t *testing.T) {
167167
filter := CreateToolScopeFilter([]string{"repo"})
168168

169169
// Build inventory with the filter
170-
inv := inventory.NewBuilder().
170+
inv, err := inventory.NewBuilder().
171171
SetTools(tools).
172172
WithToolsets([]string{"test"}).
173173
WithFilter(filter).
174174
Build()
175+
require.NoError(t, err)
175176

176177
// Get available tools
177178
availableTools := inv.AvailableTools(context.Background())

pkg/github/tools.go

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -309,7 +309,8 @@ func ToStringPtr(s string) *string {
309309
// GenerateToolsetsHelp generates the help text for the toolsets flag
310310
func GenerateToolsetsHelp() string {
311311
// Get toolset group to derive defaults and available toolsets
312-
r := NewInventory(stubTranslator).Build()
312+
// Build() can only fail if WithTools specifies invalid tools - not used here
313+
r, _ := NewInventory(stubTranslator).Build()
313314

314315
// Format default tools from metadata using strings.Builder
315316
var defaultBuf strings.Builder
@@ -391,7 +392,8 @@ func AddDefaultToolset(result []string) []string {
391392
result = RemoveToolset(result, string(ToolsetMetadataDefault.ID))
392393

393394
// Get default toolset IDs from the Inventory
394-
r := NewInventory(stubTranslator).Build()
395+
// Build() can only fail if WithTools specifies invalid tools - not used here
396+
r, _ := NewInventory(stubTranslator).Build()
395397
for _, id := range r.DefaultToolsetIDs() {
396398
if !seen[string(id)] {
397399
result = append(result, string(id))
@@ -443,7 +445,8 @@ func CleanTools(toolNames []string) []string {
443445
// GetDefaultToolsetIDs returns the IDs of toolsets marked as Default.
444446
// This is a convenience function that builds an inventory to determine defaults.
445447
func GetDefaultToolsetIDs() []string {
446-
r := NewInventory(stubTranslator).Build()
448+
// Build() can only fail if WithTools specifies invalid tools - not used here
449+
r, _ := NewInventory(stubTranslator).Build()
447450
ids := r.DefaultToolsetIDs()
448451
result := make([]string, len(ids))
449452
for i, id := range ids {

pkg/github/toolset_icons_test.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,8 @@ import (
1313
// This prevents broken icon references from being merged.
1414
func TestAllToolsetIconsExist(t *testing.T) {
1515
// Get all available toolsets from the inventory
16-
inv := NewInventory(stubTranslator).Build()
16+
inv, err := NewInventory(stubTranslator).Build()
17+
require.NoError(t, err)
1718
toolsets := inv.AvailableToolsets()
1819

1920
// Also test remote-only toolsets
@@ -72,7 +73,8 @@ func TestToolsetMetadataHasIcons(t *testing.T) {
7273
"default": true, // Meta-toolset
7374
}
7475

75-
inv := NewInventory(stubTranslator).Build()
76+
inv, err := NewInventory(stubTranslator).Build()
77+
require.NoError(t, err)
7678
toolsets := inv.AvailableToolsets()
7779

7880
for _, ts := range toolsets {

pkg/inventory/builder.go

Lines changed: 46 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package inventory
22

33
import (
44
"context"
5+
"fmt"
56
"sort"
67
"strings"
78
)
@@ -101,6 +102,7 @@ func (b *Builder) WithToolsets(toolsetIDs []string) *Builder {
101102
// WithTools specifies additional tools that bypass toolset filtering.
102103
// These tools are additive - they will be included even if their toolset is not enabled.
103104
// Read-only filtering still applies to these tools.
105+
// Input is cleaned (trimmed, deduplicated) during Build().
104106
// Deprecated tool aliases are automatically resolved to their canonical names during Build().
105107
// Returns self for chaining.
106108
func (b *Builder) WithTools(toolNames []string) *Builder {
@@ -127,11 +129,33 @@ func (b *Builder) WithFilter(filter ToolFilter) *Builder {
127129
return b
128130
}
129131

132+
// cleanTools trims whitespace and removes duplicates from tool names.
133+
// Empty strings after trimming are excluded.
134+
func cleanTools(tools []string) []string {
135+
seen := make(map[string]bool)
136+
var cleaned []string
137+
for _, name := range tools {
138+
trimmed := strings.TrimSpace(name)
139+
if trimmed == "" {
140+
continue
141+
}
142+
if !seen[trimmed] {
143+
seen[trimmed] = true
144+
cleaned = append(cleaned, trimmed)
145+
}
146+
}
147+
return cleaned
148+
}
149+
130150
// Build creates the final Inventory with all configuration applied.
131151
// This processes toolset filtering, tool name resolution, and sets up
132152
// the inventory for use. The returned Inventory is ready for use with
133153
// AvailableTools(), RegisterAll(), etc.
134-
func (b *Builder) Build() *Inventory {
154+
//
155+
// Build returns an error if any tools specified via WithTools() are not recognized
156+
// (i.e., they don't exist in the tool set and are not deprecated aliases).
157+
// This ensures invalid tool configurations fail fast at build time.
158+
func (b *Builder) Build() (*Inventory, error) {
135159
r := &Inventory{
136160
tools: b.tools,
137161
resourceTemplates: b.resourceTemplates,
@@ -145,10 +169,19 @@ func (b *Builder) Build() *Inventory {
145169
// Process toolsets and pre-compute metadata in a single pass
146170
r.enabledToolsets, r.unrecognizedToolsets, r.toolsetIDs, r.toolsetIDSet, r.defaultToolsetIDs, r.toolsetDescriptions = b.processToolsets()
147171

148-
// Process additional tools (resolve aliases)
172+
// Build set of valid tool names for validation
173+
validToolNames := make(map[string]bool, len(b.tools))
174+
for i := range b.tools {
175+
validToolNames[b.tools[i].Tool.Name] = true
176+
}
177+
178+
// Process additional tools (clean, resolve aliases, and track unrecognized)
149179
if len(b.additionalTools) > 0 {
150-
r.additionalTools = make(map[string]bool, len(b.additionalTools))
151-
for _, name := range b.additionalTools {
180+
cleanedTools := cleanTools(b.additionalTools)
181+
182+
r.additionalTools = make(map[string]bool, len(cleanedTools))
183+
var unrecognizedTools []string
184+
for _, name := range cleanedTools {
152185
// Always include the original name - this handles the case where
153186
// the tool exists but is controlled by a feature flag that's OFF.
154187
r.additionalTools[name] = true
@@ -157,11 +190,19 @@ func (b *Builder) Build() *Inventory {
157190
// the new consolidated tool is available.
158191
if canonical, isAlias := b.deprecatedAliases[name]; isAlias {
159192
r.additionalTools[canonical] = true
193+
} else if !validToolNames[name] {
194+
// Not a valid tool and not a deprecated alias - track as unrecognized
195+
unrecognizedTools = append(unrecognizedTools, name)
160196
}
161197
}
198+
199+
// Error out if there are unrecognized tools
200+
if len(unrecognizedTools) > 0 {
201+
return nil, fmt.Errorf("unrecognized tools: %s", strings.Join(unrecognizedTools, ", "))
202+
}
162203
}
163204

164-
return r
205+
return r, nil
165206
}
166207

167208
// processToolsets processes the toolsetIDs configuration and returns:

0 commit comments

Comments
 (0)