Skip to content

Fix static --tools validation fallback #3050

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Mahmoud772122777 wants to merge 1 commit into
base: main
Choose a base branch
from
+37 −8
Open

Fix static --tools validation fallback #3050

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions pkg/http/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ func DefaultGitHubMCPServerFactory(r *http.Request, deps github.ToolDependencies
func DefaultInventoryFactory(cfg *ServerConfig, t translations.TranslationHelperFunc, featureChecker inventory.FeatureFlagChecker, scopeFetcher scopes.FetcherInterface) InventoryFactoryFunc {
// Build the static tool/resource/prompt universe from CLI flags.
// This is done once at startup and captured in the closure.
staticTools, staticResources, staticPrompts := buildStaticInventory(cfg, t)
staticTools, staticResources, staticPrompts, staticErr := buildStaticInventory(cfg, t)
hasStaticFilters := hasStaticConfig(cfg)

// Pre-compute valid tool names for filtering per-request tool headers.
Expand All @@ -256,6 +256,10 @@ func DefaultInventoryFactory(cfg *ServerConfig, t translations.TranslationHelper
}

return func(r *http.Request) (*inventory.Inventory, error) {
if staticErr != nil {
return nil, staticErr
}

b := inventory.NewBuilder().
SetTools(staticTools).
SetResources(staticResources).
Expand Down Expand Up @@ -327,7 +331,7 @@ func hasStaticConfig(cfg *ServerConfig) bool {
// non-granular siblings — must be carried through to the per-request
// inventory, which then installs a checker and resolves the flag before
// registering tools with the MCP server.
func buildStaticInventory(cfg *ServerConfig, t translations.TranslationHelperFunc) ([]inventory.ServerTool, []inventory.ServerResourceTemplate, []inventory.ServerPrompt) {
func buildStaticInventory(cfg *ServerConfig, t translations.TranslationHelperFunc) ([]inventory.ServerTool, []inventory.ServerResourceTemplate, []inventory.ServerPrompt, error) {
// Tools with host-specific capabilities need to know the deployment they
// will talk to. An unparseable host is not fatal here: NewAPIHost rejects
// it later with a clearer error, so fall back to the dotcom default.
Expand All @@ -338,7 +342,7 @@ func buildStaticInventory(cfg *ServerConfig, t translations.TranslationHelperFun
opts := []github.ToolOption{github.WithHost(hostType)}

if !hasStaticConfig(cfg) {
return github.AllTools(t, opts...), github.AllResources(t), github.AllPrompts(t)
return github.AllTools(t, opts...), github.AllResources(t), github.AllPrompts(t), nil
}

b := github.NewInventory(t, opts...).
Expand All @@ -355,13 +359,11 @@ func buildStaticInventory(cfg *ServerConfig, t translations.TranslationHelperFun

inv, err := b.Build()
if err != nil {
// Fall back to all tools if there's an error (e.g. unknown tool names).
// The error will surface again at per-request time if relevant.
return github.AllTools(t, opts...), github.AllResources(t), github.AllPrompts(t)
return nil, nil, nil, err
}

ctx := context.Background()
return inv.AvailableTools(ctx), inv.AvailableResourceTemplates(ctx), inv.AvailablePrompts(ctx)
return inv.AvailableTools(ctx), inv.AvailableResourceTemplates(ctx), inv.AvailablePrompts(ctx), nil
}

// InventoryFiltersForRequest applies filters to the inventory builder
Expand Down
29 changes: 28 additions & 1 deletion pkg/http/handler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -634,6 +634,33 @@ func TestStaticConfigEnforcement(t *testing.T) {
}
}

func TestStaticInventoryInvalidEnabledToolsReturnsBadRequest(t *testing.T) {
apiHost, err := utils.NewAPIHost("https://api.github.com")
require.NoError(t, err)

handler := NewHTTPMcpHandler(
context.Background(),
&ServerConfig{Version: "test", EnabledTools: []string{"nonexistent_tool"}},
nil,
translations.NullTranslationHelper,
slog.Default(),
apiHost,
)

r := chi.NewRouter()
handler.RegisterMiddleware(r)
handler.RegisterRoutes(r)

req := httptest.NewRequest(http.MethodPost, "/", nil)
req.Header.Set(headers.AuthorizationHeader, "Bearer ghp_testtoken")

rr := httptest.NewRecorder()
r.ServeHTTP(rr, req)

assert.Equal(t, http.StatusBadRequest, rr.Code)
assert.Contains(t, rr.Body.String(), "unknown tools specified")
}

func TestStaticInventoryPreservesPerRequestFeatureVariants(t *testing.T) {
tools := []inventory.ServerTool{
mockToolWithFeatureFlag("list_issues", "issues", true, "", github.FeatureFlagCSVOutput),
Expand Down Expand Up @@ -817,7 +844,7 @@ func TestStaticInventoryAppliesHostCapabilities(t *testing.T) {
t.Parallel()

cfg := &ServerConfig{Version: "test", Host: tt.host}
staticTools, _, _ := buildStaticInventory(cfg, translations.NullTranslationHelper)
staticTools, _, _, _ := buildStaticInventory(cfg, translations.NullTranslationHelper)

var found bool
for _, st := range staticTools {
Expand Down