diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index e0d6873d10..975df2a633 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -243,7 +243,6 @@ All workflows run on push/PR unless noted. Located in `.github/workflows/`: - **GITHUB_HOST** - For GitHub Enterprise Server (prefix with `https://`) - **GITHUB_TOOLSETS** - Comma-separated toolset list (overrides --toolsets flag) - **GITHUB_READ_ONLY** - Set to "1" for read-only mode -- **GITHUB_DYNAMIC_TOOLSETS** - Set to "1" for dynamic toolset discovery - **UPDATE_TOOLSNAPS** - Set to "true" when running tests to update snapshots - **GITHUB_MCP_SERVER_E2E_TOKEN** - Token for e2e tests - **GITHUB_MCP_SERVER_E2E_DEBUG** - Set to "true" for in-process e2e debugging @@ -273,7 +272,7 @@ server.json - MCP server registry metadata `cmd/github-mcp-server/main.go` - Uses cobra for CLI, viper for config, supports: - `stdio` command (default) - MCP stdio transport - `generate-docs` command - Documentation generation -- Flags: --toolsets, --read-only, --dynamic-toolsets, --gh-host, --log-file +- Flags: --toolsets, --read-only, --gh-host, --log-file ## Important Reminders diff --git a/.github/workflows/mcp-diff.yml b/.github/workflows/mcp-diff.yml index 56f3500811..bb6341c096 100644 --- a/.github/workflows/mcp-diff.yml +++ b/.github/workflows/mcp-diff.yml @@ -34,8 +34,6 @@ jobs: [ {"name": "default", "args": ""}, {"name": "read-only", "args": "--read-only"}, - {"name": "dynamic-toolsets", "args": "--dynamic-toolsets"}, - {"name": "read-only+dynamic", "args": "--read-only --dynamic-toolsets"}, {"name": "toolsets-repos", "args": "--toolsets=repos"}, {"name": "toolsets-issues", "args": "--toolsets=issues"}, {"name": "toolsets-context", "args": "--toolsets=context"}, @@ -45,20 +43,7 @@ jobs: {"name": "toolsets-all", "args": "--toolsets=all"}, {"name": "tools-get_me", "args": "--tools=get_me"}, {"name": "tools-get_me,list_issues", "args": "--tools=get_me,list_issues"}, - {"name": "toolsets-repos+read-only", "args": "--toolsets=repos --read-only"}, - {"name": "toolsets-all+dynamic", "args": "--toolsets=all --dynamic-toolsets"}, - {"name": "toolsets-repos+dynamic", "args": "--toolsets=repos --dynamic-toolsets"}, - {"name": "toolsets-repos,issues+dynamic", "args": "--toolsets=repos,issues --dynamic-toolsets"}, - { - "name": "dynamic-tool-calls", - "args": "--dynamic-toolsets", - "custom_messages": [ - {"id": 10, "name": "list_toolsets_before", "message": {"jsonrpc": "2.0", "id": 10, "method": "tools/call", "params": {"name": "list_available_toolsets", "arguments": {}}}}, - {"id": 11, "name": "get_toolset_tools", "message": {"jsonrpc": "2.0", "id": 11, "method": "tools/call", "params": {"name": "get_toolset_tools", "arguments": {"toolset": "repos"}}}}, - {"id": 12, "name": "enable_toolset", "message": {"jsonrpc": "2.0", "id": 12, "method": "tools/call", "params": {"name": "enable_toolset", "arguments": {"toolset": "repos"}}}}, - {"id": 13, "name": "list_toolsets_after", "message": {"jsonrpc": "2.0", "id": 13, "method": "tools/call", "params": {"name": "list_available_toolsets", "arguments": {}}}} - ] - } + {"name": "toolsets-repos+read-only", "args": "--toolsets=repos --read-only"} ] - name: Add interpretation note diff --git a/README.md b/README.md index b291af0e66..e4f70b6222 100644 --- a/README.md +++ b/README.md @@ -424,7 +424,7 @@ The environment variable `GITHUB_TOOLSETS` takes precedence over the command lin #### Specifying Individual Tools -You can also configure specific tools using the `--tools` flag. Tools can be used independently or combined with toolsets and dynamic toolsets discovery for fine-grained control. +You can also configure specific tools using the `--tools` flag. Tools can be used independently or combined with toolsets for fine-grained control. 1. **Using Command Line Argument**: @@ -446,17 +446,9 @@ You can also configure specific tools using the `--tools` flag. Tools can be use This registers all tools from `repos` and `issues` toolsets, plus `get_gist`. -4. **Combining with Dynamic Toolsets** (additive): - - ```bash - github-mcp-server --tools get_file_contents --dynamic-toolsets - ``` - - This registers `get_file_contents` plus the dynamic toolset tools (`enable_toolset`, `list_available_toolsets`, `get_toolset_tools`). - **Important Notes:** -- Tools, toolsets, and dynamic toolsets can all be used together +- Tools and toolsets can be used together - Read-only mode takes priority: write tools are skipped if `--read-only` is set, even if explicitly requested via `--tools` - Tool names must match exactly (e.g., `get_file_contents`, not `getFileContents`). Invalid tool names will cause the server to fail at startup with an error message - When tools are renamed, old names are preserved as aliases for backward compatibility. See [Tool Renaming](docs/tool-renaming.md) for details. @@ -1462,29 +1454,6 @@ The following sets of tools are available: -## Dynamic Tool Discovery - -**Note**: This feature is currently in beta and is not available in the Remote GitHub MCP Server. Please test it out and let us know if you encounter any issues. - -Instead of starting with all tools enabled, you can turn on dynamic toolset discovery. Dynamic toolsets allow the MCP host to list and enable toolsets in response to a user prompt. This should help to avoid situations where the model gets confused by the sheer number of tools available. - -### Using Dynamic Tool Discovery - -When using the binary, you can pass the `--dynamic-toolsets` flag. - -```bash -./github-mcp-server --dynamic-toolsets -``` - -When using Docker, you can pass the toolsets as environment variables: - -```bash -docker run -i --rm \ - -e GITHUB_PERSONAL_ACCESS_TOKEN= \ - -e GITHUB_DYNAMIC_TOOLSETS=1 \ - ghcr.io/github/github-mcp-server -``` - ## Read-Only Mode To run the server in read-only mode, you can use the `--read-only` flag. This will only offer read-only tools, preventing any modifications to repositories, issues, pull requests, etc. diff --git a/cmd/github-mcp-server/generate_docs.go b/cmd/github-mcp-server/generate_docs.go index 7d7b1f6ab3..7a97e4f661 100644 --- a/cmd/github-mcp-server/generate_docs.go +++ b/cmd/github-mcp-server/generate_docs.go @@ -145,8 +145,8 @@ func generateToolsetsDoc(i *inventory.Inventory) string { fmt.Fprintf(&buf, "| %s | `context` | **Strongly recommended**: Tools that provide context about the current user and GitHub context you are operating in |\n", contextIcon) // AvailableToolsets() returns toolsets that have tools, sorted by ID - // Exclude context (custom description above) and dynamic (internal only) - for _, ts := range i.AvailableToolsets("context", "dynamic") { + // Exclude context (custom description above) + for _, ts := range i.AvailableToolsets("context") { icon := octiconImg(ts.Icon) fmt.Fprintf(&buf, "| %s | `%s` | %s |\n", icon, ts.ID, ts.Description) } @@ -346,8 +346,8 @@ func generateRemoteToolsetsDoc() string { fmt.Fprintf(&buf, "| %s
`all` | All available GitHub MCP tools | https://api.githubcopilot.com/mcp/ | [Install](https://insiders.vscode.dev/redirect/mcp/install?name=github&config=%%7B%%22type%%22%%3A%%20%%22http%%22%%2C%%22url%%22%%3A%%20%%22https%%3A%%2F%%2Fapi.githubcopilot.com%%2Fmcp%%2F%%22%%7D) | [read-only](https://api.githubcopilot.com/mcp/readonly) | [Install read-only](https://insiders.vscode.dev/redirect/mcp/install?name=github&config=%%7B%%22type%%22%%3A%%20%%22http%%22%%2C%%22url%%22%%3A%%20%%22https%%3A%%2F%%2Fapi.githubcopilot.com%%2Fmcp%%2Freadonly%%22%%7D) |\n", allIcon) // AvailableToolsets() returns toolsets that have tools, sorted by ID - // Exclude context (handled separately) and dynamic (internal only) - for _, ts := range r.AvailableToolsets("context", "dynamic") { + // Exclude context (handled separately) + for _, ts := range r.AvailableToolsets("context") { idStr := string(ts.ID) apiURL := fmt.Sprintf("https://api.githubcopilot.com/mcp/x/%s", idStr) diff --git a/cmd/github-mcp-server/main.go b/cmd/github-mcp-server/main.go index 8f2ae58525..ec948ab6e0 100644 --- a/cmd/github-mcp-server/main.go +++ b/cmd/github-mcp-server/main.go @@ -85,7 +85,6 @@ var ( EnabledToolsets: enabledToolsets, EnabledTools: enabledTools, EnabledFeatures: enabledFeatures, - DynamicToolsets: viper.GetBool("dynamic_toolsets"), ReadOnly: viper.GetBool("read-only"), ExportTranslations: viper.GetBool("export-translations"), EnableCommandLogging: viper.GetBool("enable-command-logging"), @@ -144,7 +143,6 @@ var ( ReadOnly: viper.GetBool("read-only"), EnabledToolsets: enabledToolsets, EnabledTools: enabledTools, - DynamicToolsets: viper.GetBool("dynamic_toolsets"), ExcludeTools: excludeTools, InsidersMode: viper.GetBool("insiders"), } @@ -165,7 +163,6 @@ func init() { rootCmd.PersistentFlags().StringSlice("tools", nil, "Comma-separated list of specific tools to enable") rootCmd.PersistentFlags().StringSlice("exclude-tools", nil, "Comma-separated list of tool names to disable regardless of other settings") rootCmd.PersistentFlags().StringSlice("features", nil, "Comma-separated list of feature flags to enable") - rootCmd.PersistentFlags().Bool("dynamic-toolsets", false, "Enable dynamic toolsets") rootCmd.PersistentFlags().Bool("read-only", false, "Restrict the server to read-only operations") rootCmd.PersistentFlags().String("log-file", "", "Path to log file") rootCmd.PersistentFlags().Bool("enable-command-logging", false, "When enabled, the server will log all command requests and responses to the log file") @@ -187,7 +184,6 @@ func init() { _ = viper.BindPFlag("tools", rootCmd.PersistentFlags().Lookup("tools")) _ = viper.BindPFlag("exclude_tools", rootCmd.PersistentFlags().Lookup("exclude-tools")) _ = viper.BindPFlag("features", rootCmd.PersistentFlags().Lookup("features")) - _ = viper.BindPFlag("dynamic_toolsets", rootCmd.PersistentFlags().Lookup("dynamic-toolsets")) _ = viper.BindPFlag("read-only", rootCmd.PersistentFlags().Lookup("read-only")) _ = viper.BindPFlag("log-file", rootCmd.PersistentFlags().Lookup("log-file")) _ = viper.BindPFlag("enable-command-logging", rootCmd.PersistentFlags().Lookup("enable-command-logging")) diff --git a/docs/installation-guides/README.md b/docs/installation-guides/README.md index aadfa6a04f..0c0f7840ef 100644 --- a/docs/installation-guides/README.md +++ b/docs/installation-guides/README.md @@ -104,6 +104,5 @@ If you encounter issues: After installation, you may want to explore: - **Toolsets**: Enable/disable specific GitHub API capabilities - **Read-Only Mode**: Restrict to read-only operations -- **Dynamic Tool Discovery**: Enable tools on-demand - **Lockdown Mode**: Hide public issue details created by users without push access diff --git a/docs/server-configuration.md b/docs/server-configuration.md index 693c096a1b..2342664c3a 100644 --- a/docs/server-configuration.md +++ b/docs/server-configuration.md @@ -11,7 +11,6 @@ We currently support the following ways in which the GitHub MCP Server can be co | Individual Tools | `X-MCP-Tools` header | `--tools` flag or `GITHUB_TOOLS` env var | | Exclude Tools | `X-MCP-Exclude-Tools` header | `--exclude-tools` flag or `GITHUB_EXCLUDE_TOOLS` env var | | Read-Only Mode | `X-MCP-Readonly` header or `/readonly` URL | `--read-only` flag or `GITHUB_READ_ONLY` env var | -| Dynamic Mode | Not available | `--dynamic-toolsets` flag or `GITHUB_DYNAMIC_TOOLSETS` env var | | Lockdown Mode | `X-MCP-Lockdown` header | `--lockdown-mode` flag or `GITHUB_LOCKDOWN_MODE` env var | | Insiders Mode | `X-MCP-Insiders` header or `/insiders` URL | `--insiders` flag or `GITHUB_INSIDERS` env var | | Feature Flags | `X-MCP-Features` header | `--features` flag | @@ -24,7 +23,7 @@ We currently support the following ways in which the GitHub MCP Server can be co ## How Configuration Works -All configuration options are **composable**: you can combine toolsets, individual tools, excluded tools, dynamic discovery, read-only mode and lockdown mode in any way that suits your workflow. +All configuration options are **composable**: you can combine toolsets, individual tools, excluded tools, read-only mode and lockdown mode in any way that suits your workflow. Note: **read-only** mode acts as a strict security filter that takes precedence over any other configuration, by disabling write tools even when explicitly requested. @@ -287,59 +286,6 @@ When active, this mode will disable all tools that are not read-only even if the --- -### Dynamic Discovery (Local Only) - -**Best for:** Letting the LLM discover and enable toolsets as needed. - -Starts with only discovery tools (`enable_toolset`, `list_available_toolsets`, `get_toolset_tools`), then expands on demand. - - - - - - -
Local Server Only
- -```json -{ - "type": "stdio", - "command": "go", - "args": [ - "run", - "./cmd/github-mcp-server", - "stdio", - "--dynamic-toolsets" - ], - "env": { - "GITHUB_PERSONAL_ACCESS_TOKEN": "${input:github_token}" - } -} -``` - -**With some tools pre-enabled:** -```json -{ - "type": "stdio", - "command": "go", - "args": [ - "run", - "./cmd/github-mcp-server", - "stdio", - "--dynamic-toolsets", - "--tools=get_me,search_code" - ], - "env": { - "GITHUB_PERSONAL_ACCESS_TOKEN": "${input:github_token}" - } -} -``` - -
- -When both dynamic mode and specific tools are enabled in the server configuration, the server will start with the 3 dynamic tools + the specified tools. - ---- - ### Lockdown Mode **Best for:** Public repositories where you want to limit content from users without push access. @@ -521,7 +467,6 @@ See [Scope Filtering](./scope-filtering.md) for details on how filtering works w | Server fails to start | Invalid tool name in `--tools` or `X-MCP-Tools` | Check tool name spelling; use exact names from [Tools list](../README.md#tools) | | Write tools not working | Read-only mode enabled | Remove `--read-only` flag or `X-MCP-Readonly` header | | Tools missing | Toolset not enabled | Add the required toolset or specific tool | -| Dynamic tools not available | Using remote server | Dynamic mode is available in the local MCP server only | --- diff --git a/docs/toolsets-and-icons.md b/docs/toolsets-and-icons.md index 9c26b4aa10..9228248ecb 100644 --- a/docs/toolsets-and-icons.md +++ b/docs/toolsets-and-icons.md @@ -161,7 +161,6 @@ icons := octicons.Icons("repo") | Labels | `tag` | | Stargazers | `star` | | Notifications | `bell` | -| Dynamic | `tools` | | Copilot | `copilot` | | Support Search | `book` | diff --git a/internal/ghmcp/server.go b/internal/ghmcp/server.go index 6c8c3934d5..3ca249dd17 100644 --- a/internal/ghmcp/server.go +++ b/internal/ghmcp/server.go @@ -153,7 +153,7 @@ func NewStdioMCPServer(ctx context.Context, cfg github.MCPServerConfig) (*mcp.Se inventoryBuilder := github.NewInventory(cfg.Translator). WithDeprecatedAliases(github.DeprecatedToolAliases). WithReadOnly(cfg.ReadOnly). - WithToolsets(github.ResolvedEnabledToolsets(cfg.DynamicToolsets, cfg.EnabledToolsets, cfg.EnabledTools)). + WithToolsets(github.ResolvedEnabledToolsets(cfg.EnabledToolsets, cfg.EnabledTools)). WithTools(github.CleanTools(cfg.EnabledTools)). WithExcludeTools(cfg.ExcludeTools). WithServerInstructions(). @@ -210,10 +210,6 @@ type StdioServerConfig struct { // Items with FeatureFlagEnable matching an entry in this list will be available EnabledFeatures []string - // Whether to enable dynamic toolsets - // See: https://github.com/github/github-mcp-server?tab=readme-ov-file#dynamic-tool-discovery - DynamicToolsets bool - // ReadOnly indicates if we should only register read-only tools ReadOnly bool @@ -267,7 +263,7 @@ func RunStdioServer(cfg StdioServerConfig) error { slogHandler = slog.NewTextHandler(logOutput, &slog.HandlerOptions{Level: slog.LevelInfo}) } logger := slog.New(slogHandler) - logger.Info("starting server", "version", cfg.Version, "host", cfg.Host, "dynamicToolsets", cfg.DynamicToolsets, "readOnly", cfg.ReadOnly, "lockdownEnabled", cfg.LockdownMode) + logger.Info("starting server", "version", cfg.Version, "host", cfg.Host, "readOnly", cfg.ReadOnly, "lockdownEnabled", cfg.LockdownMode) // Fetch token scopes for scope-based tool filtering (PAT tokens only) // Only classic PATs (ghp_ prefix) return OAuth scopes via X-OAuth-Scopes header. @@ -292,7 +288,6 @@ func RunStdioServer(cfg StdioServerConfig) error { EnabledToolsets: cfg.EnabledToolsets, EnabledTools: cfg.EnabledTools, EnabledFeatures: cfg.EnabledFeatures, - DynamicToolsets: cfg.DynamicToolsets, ReadOnly: cfg.ReadOnly, Translator: t, ContentWindowSize: cfg.ContentWindowSize, diff --git a/pkg/github/dynamic_tools.go b/pkg/github/dynamic_tools.go deleted file mode 100644 index 1106616fa0..0000000000 --- a/pkg/github/dynamic_tools.go +++ /dev/null @@ -1,217 +0,0 @@ -package github - -import ( - "context" - "encoding/json" - "fmt" - - "github.com/github/github-mcp-server/pkg/inventory" - "github.com/github/github-mcp-server/pkg/translations" - "github.com/github/github-mcp-server/pkg/utils" - "github.com/google/jsonschema-go/jsonschema" - "github.com/modelcontextprotocol/go-sdk/mcp" -) - -// DynamicToolDependencies contains dependencies for dynamic toolset management tools. -// It includes the managed Inventory, the server for registration, and the deps -// that will be passed to tools when they are dynamically enabled. -type DynamicToolDependencies struct { - // Server is the MCP server to register tools with - Server *mcp.Server - // Inventory contains all available tools, resources and prompts that can be enabled dynamically - Inventory *inventory.Inventory - // ToolDeps are the dependencies passed to tools when they are registered - ToolDeps any - // T is the translation helper function - T translations.TranslationHelperFunc -} - -// NewDynamicTool creates a ServerTool with fully-typed DynamicToolDependencies. -// Dynamic tools use a different dependency structure (DynamicToolDependencies) than regular -// tools (ToolDependencies), so they intentionally use the closure pattern. -func NewDynamicTool(toolset inventory.ToolsetMetadata, tool mcp.Tool, handler func(deps DynamicToolDependencies) mcp.ToolHandlerFor[map[string]any, any]) inventory.ServerTool { - //nolint:staticcheck // SA1019: Dynamic tools use a different deps structure, closure pattern is intentional - return inventory.NewServerToolWithDeps(tool, toolset, func(d any) mcp.ToolHandlerFor[map[string]any, any] { - return handler(d.(DynamicToolDependencies)) - }) -} - -// toolsetIDsEnum returns the list of toolset IDs as an enum for JSON Schema. -func toolsetIDsEnum(r *inventory.Inventory) []any { - toolsetIDs := r.ToolsetIDs() - result := make([]any, len(toolsetIDs)) - for i, id := range toolsetIDs { - result[i] = id - } - return result -} - -// DynamicTools returns the tools for dynamic toolset management. -// These tools allow runtime discovery and enablement of inventory. -// The r parameter provides the available toolset IDs for JSON Schema enums. -func DynamicTools(r *inventory.Inventory) []inventory.ServerTool { - return []inventory.ServerTool{ - ListAvailableToolsets(), - GetToolsetsTools(r), - EnableToolset(r), - } -} - -// EnableToolset creates a tool that enables a toolset at runtime. -func EnableToolset(r *inventory.Inventory) inventory.ServerTool { - return NewDynamicTool( - ToolsetMetadataDynamic, - mcp.Tool{ - Name: "enable_toolset", - Description: "Enable one of the sets of tools the GitHub MCP server provides, use get_toolset_tools and list_available_toolsets first to see what this will enable", - Annotations: &mcp.ToolAnnotations{ - Title: "Enable a toolset", - ReadOnlyHint: true, - }, - InputSchema: &jsonschema.Schema{ - Type: "object", - Properties: map[string]*jsonschema.Schema{ - "toolset": { - Type: "string", - Description: "The name of the toolset to enable", - Enum: toolsetIDsEnum(r), - }, - }, - Required: []string{"toolset"}, - }, - }, - func(deps DynamicToolDependencies) mcp.ToolHandlerFor[map[string]any, any] { - return func(_ context.Context, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { - toolsetName, err := RequiredParam[string](args, "toolset") - if err != nil { - return utils.NewToolResultError(err.Error()), nil, nil - } - - toolsetID := inventory.ToolsetID(toolsetName) - - if !deps.Inventory.HasToolset(toolsetID) { - return utils.NewToolResultError(fmt.Sprintf("Toolset %s not found", toolsetName)), nil, nil - } - - if deps.Inventory.IsToolsetEnabled(toolsetID) { - return utils.NewToolResultText(fmt.Sprintf("Toolset %s is already enabled", toolsetName)), nil, nil - } - - // Mark the toolset as enabled so IsToolsetEnabled returns true - deps.Inventory.EnableToolset(toolsetID) - - // Get tools for this toolset and register them with the managed deps - toolsForToolset := deps.Inventory.ToolsForToolset(toolsetID) - for _, st := range toolsForToolset { - st.RegisterFunc(deps.Server, deps.ToolDeps) - } - - return utils.NewToolResultText(fmt.Sprintf("Toolset %s enabled with %d tools", toolsetName, len(toolsForToolset))), nil, nil - } - }, - ) -} - -// ListAvailableToolsets creates a tool that lists all available inventory. -func ListAvailableToolsets() inventory.ServerTool { - return NewDynamicTool( - ToolsetMetadataDynamic, - mcp.Tool{ - Name: "list_available_toolsets", - Description: "List all available toolsets this GitHub MCP server can offer, providing the enabled status of each. Use this when a task could be achieved with a GitHub tool and the currently available tools aren't enough. Call get_toolset_tools with these toolset names to discover specific tools you can call", - Annotations: &mcp.ToolAnnotations{ - Title: "List available toolsets", - ReadOnlyHint: true, - }, - InputSchema: &jsonschema.Schema{ - Type: "object", - Properties: map[string]*jsonschema.Schema{}, - }, - }, - func(deps DynamicToolDependencies) mcp.ToolHandlerFor[map[string]any, any] { - return func(_ context.Context, _ *mcp.CallToolRequest, _ map[string]any) (*mcp.CallToolResult, any, error) { - toolsetIDs := deps.Inventory.ToolsetIDs() - descriptions := deps.Inventory.ToolsetDescriptions() - - payload := make([]map[string]string, 0, len(toolsetIDs)) - for _, id := range toolsetIDs { - t := map[string]string{ - "name": string(id), - "description": descriptions[id], - "can_enable": "true", - "currently_enabled": fmt.Sprintf("%t", deps.Inventory.IsToolsetEnabled(id)), - } - payload = append(payload, t) - } - - r, err := json.Marshal(payload) - if err != nil { - return nil, nil, fmt.Errorf("failed to marshal features: %w", err) - } - - return utils.NewToolResultText(string(r)), nil, nil - } - }, - ) -} - -// GetToolsetsTools creates a tool that lists all tools in a specific toolset. -func GetToolsetsTools(r *inventory.Inventory) inventory.ServerTool { - return NewDynamicTool( - ToolsetMetadataDynamic, - mcp.Tool{ - Name: "get_toolset_tools", - Description: "Lists all the capabilities that are enabled with the specified toolset, use this to get clarity on whether enabling a toolset would help you to complete a task", - Annotations: &mcp.ToolAnnotations{ - Title: "List all tools in a toolset", - ReadOnlyHint: true, - }, - InputSchema: &jsonschema.Schema{ - Type: "object", - Properties: map[string]*jsonschema.Schema{ - "toolset": { - Type: "string", - Description: "The name of the toolset you want to get the tools for", - Enum: toolsetIDsEnum(r), - }, - }, - Required: []string{"toolset"}, - }, - }, - func(deps DynamicToolDependencies) mcp.ToolHandlerFor[map[string]any, any] { - return func(_ context.Context, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { - toolsetName, err := RequiredParam[string](args, "toolset") - if err != nil { - return utils.NewToolResultError(err.Error()), nil, nil - } - - toolsetID := inventory.ToolsetID(toolsetName) - - if !deps.Inventory.HasToolset(toolsetID) { - return utils.NewToolResultError(fmt.Sprintf("Toolset %s not found", toolsetName)), nil, nil - } - - // Get all tools for this toolset (ignoring current filters for discovery) - toolsInToolset := deps.Inventory.ToolsForToolset(toolsetID) - payload := make([]map[string]string, 0, len(toolsInToolset)) - - for _, st := range toolsInToolset { - tool := map[string]string{ - "name": st.Tool.Name, - "description": st.Tool.Description, - "can_enable": "true", - "toolset": toolsetName, - } - payload = append(payload, tool) - } - - r, err := json.Marshal(payload) - if err != nil { - return nil, nil, fmt.Errorf("failed to marshal features: %w", err) - } - - return utils.NewToolResultText(string(r)), nil, nil - } - }, - ) -} diff --git a/pkg/github/dynamic_tools_test.go b/pkg/github/dynamic_tools_test.go deleted file mode 100644 index ec559099ef..0000000000 --- a/pkg/github/dynamic_tools_test.go +++ /dev/null @@ -1,236 +0,0 @@ -package github - -import ( - "context" - "encoding/json" - "testing" - - "github.com/github/github-mcp-server/pkg/inventory" - "github.com/github/github-mcp-server/pkg/translations" - "github.com/google/jsonschema-go/jsonschema" - "github.com/modelcontextprotocol/go-sdk/mcp" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -// createDynamicRequest creates an MCP request with the given arguments for dynamic tools. -func createDynamicRequest(args map[string]any) *mcp.CallToolRequest { - argsJSON, _ := json.Marshal(args) - return &mcp.CallToolRequest{ - Params: &mcp.CallToolParamsRaw{ - Arguments: json.RawMessage(argsJSON), - }, - } -} - -func TestDynamicTools_ListAvailableToolsets(t *testing.T) { - // Build a registry with no toolsets enabled (dynamic mode) - reg, err := NewInventory(translations.NullTranslationHelper). - WithToolsets([]string{}). - Build() - require.NoError(t, err) - - // Create a mock server - server := mcp.NewServer(&mcp.Implementation{Name: "test"}, nil) - - // Create dynamic tool dependencies - deps := DynamicToolDependencies{ - Server: server, - Inventory: reg, - ToolDeps: nil, - T: translations.NullTranslationHelper, - } - - // Get the list_available_toolsets tool - tool := ListAvailableToolsets() - handler := tool.Handler(deps) - - // Call the handler - result, err := handler(context.Background(), createDynamicRequest(map[string]any{})) - require.NoError(t, err) - require.NotNil(t, result) - require.Len(t, result.Content, 1) - - // Parse the result - var toolsets []map[string]string - textContent := result.Content[0].(*mcp.TextContent) - err = json.Unmarshal([]byte(textContent.Text), &toolsets) - require.NoError(t, err) - - // Verify we got toolsets - assert.NotEmpty(t, toolsets, "should have available toolsets") - - // Find the repos toolset and verify it's not enabled - var reposToolset map[string]string - for _, ts := range toolsets { - if ts["name"] == "repos" { - reposToolset = ts - break - } - } - require.NotNil(t, reposToolset, "repos toolset should exist") - assert.Equal(t, "false", reposToolset["currently_enabled"], "repos should not be enabled initially") -} - -func TestDynamicTools_GetToolsetTools(t *testing.T) { - // Build a registry with no toolsets enabled (dynamic mode) - reg, err := NewInventory(translations.NullTranslationHelper). - WithToolsets([]string{}). - Build() - require.NoError(t, err) - - // Create a mock server - server := mcp.NewServer(&mcp.Implementation{Name: "test"}, nil) - - // Create dynamic tool dependencies - deps := DynamicToolDependencies{ - Server: server, - Inventory: reg, - ToolDeps: nil, - T: translations.NullTranslationHelper, - } - - // Get the get_toolset_tools tool - tool := GetToolsetsTools(reg) - handler := tool.Handler(deps) - - // Call the handler for repos toolset - result, err := handler(context.Background(), createDynamicRequest(map[string]any{ - "toolset": "repos", - })) - require.NoError(t, err) - require.NotNil(t, result) - require.Len(t, result.Content, 1) - - // Parse the result - var tools []map[string]string - textContent := result.Content[0].(*mcp.TextContent) - err = json.Unmarshal([]byte(textContent.Text), &tools) - require.NoError(t, err) - - // Verify we got tools for the repos toolset - assert.NotEmpty(t, tools, "repos toolset should have tools") - - // Verify at least get_commit is there (a repos toolset tool) - var foundGetCommit bool - for _, tool := range tools { - if tool["name"] == "get_commit" { - foundGetCommit = true - break - } - } - assert.True(t, foundGetCommit, "get_commit should be in repos toolset") -} - -func TestDynamicTools_EnableToolset(t *testing.T) { - // Build a registry with no toolsets enabled (dynamic mode) - reg, err := NewInventory(translations.NullTranslationHelper). - WithToolsets([]string{}). - Build() - require.NoError(t, err) - - // Create a mock server - server := mcp.NewServer(&mcp.Implementation{Name: "test"}, nil) - - // Create dynamic tool dependencies - deps := DynamicToolDependencies{ - Server: server, - Inventory: reg, - ToolDeps: NewBaseDeps(nil, nil, nil, nil, translations.NullTranslationHelper, FeatureFlags{}, 0, nil, stubExporters()), - T: translations.NullTranslationHelper, - } - - // Verify repos is not enabled initially - assert.False(t, reg.IsToolsetEnabled(inventory.ToolsetID("repos"))) - - // Get the enable_toolset tool - tool := EnableToolset(reg) - handler := tool.Handler(deps) - - // Enable the repos toolset - result, err := handler(context.Background(), createDynamicRequest(map[string]any{ - "toolset": "repos", - })) - require.NoError(t, err) - require.NotNil(t, result) - require.Len(t, result.Content, 1) - - // Verify the toolset is now enabled - assert.True(t, reg.IsToolsetEnabled(inventory.ToolsetID("repos")), "repos should be enabled after enable_toolset") - - // Verify the success message - textContent := result.Content[0].(*mcp.TextContent) - assert.Contains(t, textContent.Text, "enabled") - - // Try enabling again - should say already enabled - result2, err := handler(context.Background(), createDynamicRequest(map[string]any{ - "toolset": "repos", - })) - require.NoError(t, err) - textContent2 := result2.Content[0].(*mcp.TextContent) - assert.Contains(t, textContent2.Text, "already enabled") -} - -func TestDynamicTools_EnableToolset_InvalidToolset(t *testing.T) { - // Build a registry with no toolsets enabled (dynamic mode) - reg, err := NewInventory(translations.NullTranslationHelper). - WithToolsets([]string{}). - Build() - require.NoError(t, err) - - // Create a mock server - server := mcp.NewServer(&mcp.Implementation{Name: "test"}, nil) - - // Create dynamic tool dependencies - deps := DynamicToolDependencies{ - Server: server, - Inventory: reg, - ToolDeps: nil, - T: translations.NullTranslationHelper, - } - - // Get the enable_toolset tool - tool := EnableToolset(reg) - handler := tool.Handler(deps) - - // Try to enable a non-existent toolset - result, err := handler(context.Background(), createDynamicRequest(map[string]any{ - "toolset": "nonexistent", - })) - require.NoError(t, err) - require.NotNil(t, result) - - // Should be an error result - textContent := result.Content[0].(*mcp.TextContent) - assert.Contains(t, textContent.Text, "not found") -} - -func TestDynamicTools_ToolsetsEnum(t *testing.T) { - // Build a registry - reg, err := NewInventory(translations.NullTranslationHelper).Build() - require.NoError(t, err) - - // Get tools to verify they have proper enum values - tools := DynamicTools(reg) - - // Find enable_toolset and get_toolset_tools - for _, tool := range tools { - if tool.Tool.Name == "enable_toolset" || tool.Tool.Name == "get_toolset_tools" { - // Verify the toolset property has an enum - schema := tool.Tool.InputSchema.(*jsonschema.Schema) - toolsetProp := schema.Properties["toolset"] - require.NotNil(t, toolsetProp, "toolset property should exist") - assert.NotEmpty(t, toolsetProp.Enum, "toolset property should have enum values") - - // Verify repos is in the enum - var foundRepos bool - for _, v := range toolsetProp.Enum { - if v == inventory.ToolsetID("repos") { - foundRepos = true - break - } - } - assert.True(t, foundRepos, "repos should be in toolset enum for %s", tool.Tool.Name) - } - } -} diff --git a/pkg/github/server.go b/pkg/github/server.go index a9a75642f8..41e502db3c 100644 --- a/pkg/github/server.go +++ b/pkg/github/server.go @@ -38,10 +38,6 @@ type MCPServerConfig struct { // Items with FeatureFlagEnable matching an entry in this list will be available EnabledFeatures []string - // Whether to enable dynamic toolsets - // See: https://github.com/github/github-mcp-server?tab=readme-ov-file#dynamic-tool-discovery - DynamicToolsets bool - // ReadOnly indicates if we should only offer read-only tools ReadOnly bool @@ -91,16 +87,6 @@ func NewMCPServer(ctx context.Context, cfg *MCPServerConfig, deps ToolDependenci o(serverOpts) } - // In dynamic mode, explicitly advertise capabilities since tools/resources/prompts - // may be enabled at runtime even if none are registered initially. - if cfg.DynamicToolsets { - serverOpts.Capabilities = &mcp.ServerCapabilities{ - Tools: &mcp.ToolCapabilities{}, - Resources: &mcp.ResourceCapabilities{}, - Prompts: &mcp.PromptCapabilities{}, - } - } - ghServer := NewServer(cfg.Version, cfg.Translator("SERVER_NAME", "github-mcp-server"), cfg.Translator("SERVER_TITLE", "GitHub MCP Server"), serverOpts) // Add middlewares. Order matters - for example, the error context middleware should be applied last so that it runs FIRST (closest to the handler) to ensure all errors are captured, @@ -114,49 +100,17 @@ func NewMCPServer(ctx context.Context, cfg *MCPServerConfig, deps ToolDependenci } // Register GitHub tools/resources/prompts from the inventory. - // In dynamic mode with no explicit toolsets, this is a no-op since enabledToolsets - // is empty - users enable toolsets at runtime via the dynamic tools below (but can - // enable toolsets or tools explicitly that do need registration). inv.RegisterAll(ctx, ghServer, deps) - // Register dynamic toolset management tools (enable/disable) - these are separate - // meta-tools that control the inventory, not part of the inventory itself - if cfg.DynamicToolsets { - registerDynamicTools(ghServer, inv, deps, cfg.Translator) - } - return ghServer, nil } -// registerDynamicTools adds the dynamic toolset enable/disable tools to the server. -func registerDynamicTools(server *mcp.Server, inventory *inventory.Inventory, deps ToolDependencies, t translations.TranslationHelperFunc) { - dynamicDeps := DynamicToolDependencies{ - Server: server, - Inventory: inventory, - ToolDeps: deps, - T: t, - } - for _, tool := range DynamicTools(inventory) { - tool.RegisterFunc(server, dynamicDeps) - } -} - // ResolvedEnabledToolsets determines which toolsets should be enabled based on config. // Returns nil for "use defaults", empty slice for "none", or explicit list. -func ResolvedEnabledToolsets(dynamicToolsets bool, enabledToolsets []string, enabledTools []string) []string { - // In dynamic mode, remove "all" and "default" since users enable toolsets on demand - if dynamicToolsets && enabledToolsets != nil { - enabledToolsets = RemoveToolset(enabledToolsets, string(ToolsetMetadataAll.ID)) - enabledToolsets = RemoveToolset(enabledToolsets, string(ToolsetMetadataDefault.ID)) - } - +func ResolvedEnabledToolsets(enabledToolsets []string, enabledTools []string) []string { if enabledToolsets != nil { return enabledToolsets } - if dynamicToolsets { - // Dynamic mode with no toolsets specified: start empty so users enable on demand - return []string{} - } if len(enabledTools) > 0 { // When specific tools are requested but no toolsets, don't use default toolsets // This matches the original behavior: --tools=X alone registers only X diff --git a/pkg/github/server_test.go b/pkg/github/server_test.go index be078d3603..be37ca949d 100644 --- a/pkg/github/server_test.go +++ b/pkg/github/server_test.go @@ -290,28 +290,17 @@ func TestResolveEnabledToolsets(t *testing.T) { expectedResult []string }{ { - name: "nil toolsets without dynamic mode and no tools - use defaults", + name: "nil toolsets and no tools - use defaults", cfg: MCPServerConfig{ EnabledToolsets: nil, - DynamicToolsets: false, EnabledTools: nil, }, expectedResult: nil, // nil means "use defaults" }, - { - name: "nil toolsets with dynamic mode - start empty", - cfg: MCPServerConfig{ - EnabledToolsets: nil, - DynamicToolsets: true, - EnabledTools: nil, - }, - expectedResult: []string{}, // empty slice means no toolsets - }, { name: "explicit toolsets", cfg: MCPServerConfig{ EnabledToolsets: []string{"repos", "issues"}, - DynamicToolsets: false, }, expectedResult: []string{"repos", "issues"}, }, @@ -319,32 +308,22 @@ func TestResolveEnabledToolsets(t *testing.T) { name: "empty toolsets - disable all", cfg: MCPServerConfig{ EnabledToolsets: []string{}, - DynamicToolsets: false, }, - expectedResult: []string{}, // empty slice means no toolsets + expectedResult: []string{}, }, { name: "specific tools without toolsets - no default toolsets", cfg: MCPServerConfig{ EnabledToolsets: nil, - DynamicToolsets: false, EnabledTools: []string{"get_me"}, }, expectedResult: []string{}, // empty slice when tools specified but no toolsets }, - { - name: "dynamic mode with explicit toolsets removes all and default", - cfg: MCPServerConfig{ - EnabledToolsets: []string{"all", "repos"}, - DynamicToolsets: true, - }, - expectedResult: []string{"repos"}, // "all" is removed in dynamic mode - }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - result := ResolvedEnabledToolsets(tc.cfg.DynamicToolsets, tc.cfg.EnabledToolsets, tc.cfg.EnabledTools) + result := ResolvedEnabledToolsets(tc.cfg.EnabledToolsets, tc.cfg.EnabledTools) assert.Equal(t, tc.expectedResult, result) }) } diff --git a/pkg/github/tools.go b/pkg/github/tools.go index f4c653bf8d..c7f5abf3bd 100644 --- a/pkg/github/tools.go +++ b/pkg/github/tools.go @@ -123,11 +123,6 @@ var ( Description: "GitHub Stargazers related tools", Icon: "star", } - ToolsetMetadataDynamic = inventory.ToolsetMetadata{ - ID: "dynamic", - Description: "Discover GitHub MCP tools that can help achieve tasks by enabling additional sets of tools, you can control the enablement of any toolset to access its tools when this toolset is enabled.", - Icon: "tools", - } ToolsetLabels = inventory.ToolsetMetadata{ ID: "labels", Description: "GitHub Labels related tools", @@ -350,8 +345,8 @@ func GenerateToolsetsHelp() string { defaultBuf.WriteString(string(id)) } - // Get all available toolsets (excludes context and dynamic for display) - allToolsets := r.AvailableToolsets("context", "dynamic") + // Get all available toolsets (excludes context for display) + allToolsets := r.AvailableToolsets("context") var availableBuf strings.Builder const maxLineLength = 70 currentLine := "" diff --git a/pkg/http/handler.go b/pkg/http/handler.go index 1ae4713216..90423d93cc 100644 --- a/pkg/http/handler.go +++ b/pkg/http/handler.go @@ -321,7 +321,6 @@ func hasStaticConfig(cfg *ServerConfig) bool { return cfg.ReadOnly || cfg.EnabledToolsets != nil || cfg.EnabledTools != nil || - cfg.DynamicToolsets || len(cfg.ExcludeTools) > 0 || cfg.InsidersMode } @@ -337,7 +336,7 @@ func buildStaticInventory(cfg *ServerConfig, t translations.TranslationHelperFun b := github.NewInventory(t). WithFeatureChecker(featureChecker). WithReadOnly(cfg.ReadOnly). - WithToolsets(github.ResolvedEnabledToolsets(cfg.DynamicToolsets, cfg.EnabledToolsets, cfg.EnabledTools)) + WithToolsets(github.ResolvedEnabledToolsets(cfg.EnabledToolsets, cfg.EnabledTools)) if len(cfg.EnabledTools) > 0 { b = b.WithTools(github.CleanTools(cfg.EnabledTools)) @@ -373,7 +372,7 @@ func InventoryFiltersForRequest(r *http.Request, builder *inventory.Builder) *in tools := ghcontext.GetTools(ctx) if len(toolsets) > 0 { - builder = builder.WithToolsets(github.ResolvedEnabledToolsets(false, toolsets, tools)) // No dynamic toolsets in HTTP mode + builder = builder.WithToolsets(github.ResolvedEnabledToolsets(toolsets, tools)) } if len(tools) > 0 { diff --git a/pkg/http/handler_test.go b/pkg/http/handler_test.go index 9887ff1f3b..fd2966fd05 100644 --- a/pkg/http/handler_test.go +++ b/pkg/http/handler_test.go @@ -738,7 +738,7 @@ func buildStaticInventoryFromTools(cfg *ServerConfig, tools []inventory.ServerTo SetTools(tools). WithFeatureChecker(featureChecker). WithReadOnly(cfg.ReadOnly). - WithToolsets(github.ResolvedEnabledToolsets(cfg.DynamicToolsets, cfg.EnabledToolsets, cfg.EnabledTools)) + WithToolsets(github.ResolvedEnabledToolsets(cfg.EnabledToolsets, cfg.EnabledTools)) if len(cfg.EnabledTools) > 0 { b = b.WithTools(github.CleanTools(cfg.EnabledTools)) diff --git a/pkg/http/server.go b/pkg/http/server.go index f7cdaf9093..b8c419ea04 100644 --- a/pkg/http/server.go +++ b/pkg/http/server.go @@ -78,9 +78,6 @@ type ServerConfig struct { // EnabledTools is a list of specific tools to enable (additive to toolsets). EnabledTools []string - // DynamicToolsets enables dynamic toolset discovery mode. - DynamicToolsets bool - // ExcludeTools is a list of tool names to disable regardless of other settings. // When set via CLI flag, per-request headers cannot re-include these tools. ExcludeTools []string diff --git a/pkg/inventory/builder.go b/pkg/inventory/builder.go index d656359bb6..2642c6127a 100644 --- a/pkg/inventory/builder.go +++ b/pkg/inventory/builder.go @@ -106,8 +106,7 @@ func (b *Builder) WithServerInstructions() *Builder { // - "default": expands to toolsets marked with Default: true in their metadata // // Input strings are trimmed of whitespace and duplicates are removed. -// Pass nil to use default toolsets. Pass an empty slice to disable all toolsets -// (useful for dynamic toolsets mode where tools are enabled on demand). +// Pass nil to use default toolsets. Pass an empty slice to disable all toolsets. // Returns self for chaining. func (b *Builder) WithToolsets(toolsetIDs []string) *Builder { b.toolsetIDs = toolsetIDs diff --git a/pkg/inventory/filters.go b/pkg/inventory/filters.go index 707457853c..604aa1000d 100644 --- a/pkg/inventory/filters.go +++ b/pkg/inventory/filters.go @@ -4,7 +4,6 @@ import ( "context" "fmt" "os" - "slices" "sort" ) @@ -215,62 +214,6 @@ func (r *Inventory) filterPromptsByName(name string) []ServerPrompt { return []ServerPrompt{} } -// ToolsForToolset returns all tools belonging to a specific toolset. -// This method bypasses the toolset enabled filter (for dynamic toolset registration), -// but still respects the read-only filter. -func (r *Inventory) ToolsForToolset(toolsetID ToolsetID) []ServerTool { - var result []ServerTool - for i := range r.tools { - tool := &r.tools[i] - // Only check read-only filter, not toolset enabled filter - if tool.Toolset.ID == toolsetID { - if r.readOnly && !tool.IsReadOnly() { - continue - } - result = append(result, *tool) - } - } - - // Sort by tool name for deterministic order - sort.Slice(result, func(i, j int) bool { - return result[i].Tool.Name < result[j].Tool.Name - }) - - return result -} - -// IsToolsetEnabled checks if a toolset is currently enabled based on filters. -func (r *Inventory) IsToolsetEnabled(toolsetID ToolsetID) bool { - return r.isToolsetEnabled(toolsetID) -} - -// EnableToolset marks a toolset as enabled in this group. -// This is used by dynamic toolset management to track which toolsets have been enabled. -func (r *Inventory) EnableToolset(toolsetID ToolsetID) { - if r.enabledToolsets == nil { - // nil means all enabled, so nothing to do - return - } - r.enabledToolsets[toolsetID] = true -} - -// EnabledToolsetIDs returns the list of enabled toolset IDs based on current filters. -// Returns all toolset IDs if no filter is set. -func (r *Inventory) EnabledToolsetIDs() []ToolsetID { - if r.enabledToolsets == nil { - return r.ToolsetIDs() - } - - ids := make([]ToolsetID, 0, len(r.enabledToolsets)) - for id := range r.enabledToolsets { - if r.HasToolset(id) { - ids = append(ids, id) - } - } - slices.Sort(ids) - return ids -} - // FilteredTools returns tools filtered by the Enabled function and builder filters. // This provides an explicit API for accessing filtered tools, currently implemented // as an alias for AvailableTools. diff --git a/pkg/inventory/registry.go b/pkg/inventory/registry.go index a0bbc7a550..d54b3f12d5 100644 --- a/pkg/inventory/registry.go +++ b/pkg/inventory/registry.go @@ -23,7 +23,6 @@ import ( // - Filtered access to tools/resources/prompts via Available* methods // - Deterministic ordering for documentation generation // - Lazy dependency injection during registration via RegisterAll() -// - Runtime toolset enabling for dynamic toolsets mode type Inventory struct { // tools holds all tools in this group (ordered for iteration) tools []ServerTool diff --git a/pkg/inventory/registry_test.go b/pkg/inventory/registry_test.go index e6aedc620c..8e35861f15 100644 --- a/pkg/inventory/registry_test.go +++ b/pkg/inventory/registry_test.go @@ -462,21 +462,6 @@ func TestToolsetDescriptions(t *testing.T) { } } -func TestToolsForToolset(t *testing.T) { - tools := []ServerTool{ - mockTool("tool1", "toolset1", true), - mockTool("tool2", "toolset1", true), - mockTool("tool3", "toolset2", true), - } - - reg := mustBuild(t, NewBuilder().SetTools(tools)) - toolset1Tools := reg.ToolsForToolset("toolset1") - - if len(toolset1Tools) != 2 { - t.Fatalf("Expected 2 tools for toolset1, got %d", len(toolset1Tools)) - } -} - func TestWithDeprecatedAliases(t *testing.T) { tools := []ServerTool{ mockTool("new_name", "toolset1", true), @@ -638,30 +623,6 @@ func TestHasToolset(t *testing.T) { } } -func TestEnabledToolsetIDs(t *testing.T) { - tools := []ServerTool{ - mockTool("tool1", "toolset1", true), - mockTool("tool2", "toolset2", true), - } - - // Without filter, all toolsets are enabled - reg := mustBuild(t, NewBuilder().SetTools(tools).WithToolsets([]string{"all"})) - ids := reg.EnabledToolsetIDs() - if len(ids) != 2 { - t.Fatalf("Expected 2 enabled toolset IDs, got %d", len(ids)) - } - - // With filter - filtered := mustBuild(t, NewBuilder().SetTools(tools).WithToolsets([]string{"toolset1"})) - filteredIDs := filtered.EnabledToolsetIDs() - if len(filteredIDs) != 1 { - t.Fatalf("Expected 1 enabled toolset ID, got %d", len(filteredIDs)) - } - if filteredIDs[0] != "toolset1" { - t.Errorf("Expected toolset1, got %s", filteredIDs[0]) - } -} - func TestAllTools(t *testing.T) { tools := []ServerTool{ mockTool("read_tool", "toolset1", true), diff --git a/pkg/inventory/server_tool.go b/pkg/inventory/server_tool.go index 316fffaa91..41d38b7ec2 100644 --- a/pkg/inventory/server_tool.go +++ b/pkg/inventory/server_tool.go @@ -119,35 +119,6 @@ func (st *ServerTool) RegisterFunc(s *mcp.Server, deps any) { s.AddTool(&toolCopy, handler) } -// NewServerToolWithDeps creates a ServerTool from a tool definition, toolset metadata, and a typed handler function. -// The handler function takes dependencies (as any) and returns a typed handler. -// Callers should type-assert deps to their typed dependencies struct. -// -// Deprecated: This creates closures at registration time. For better performance in -// per-request server scenarios, use NewServerToolWithContextHandler instead. -func NewServerToolWithDeps[In any, Out any](tool mcp.Tool, toolset ToolsetMetadata, handlerFn func(deps any) mcp.ToolHandlerFor[In, Out]) ServerTool { - return ServerTool{ - Tool: tool, - Toolset: toolset, - HandlerFunc: func(deps any) mcp.ToolHandler { - typedHandler := handlerFn(deps) - return func(ctx context.Context, req *mcp.CallToolRequest) (*mcp.CallToolResult, error) { - var arguments In - if err := json.Unmarshal(req.Params.Arguments, &arguments); err != nil { - return &mcp.CallToolResult{ - Content: []mcp.Content{ - &mcp.TextContent{Text: fmt.Sprintf("invalid arguments: %s", err)}, - }, - IsError: true, - }, nil - } - resp, _, err := typedHandler(ctx, req, arguments) - return resp, err - } - }, - } -} - // NewServerToolWithContextHandler creates a ServerTool with a handler that receives deps via context. // This is the preferred approach for tools because it doesn't create closures at registration time, // which is critical for performance in servers that create a new instance per request. diff --git a/pkg/inventory/server_tool_test.go b/pkg/inventory/server_tool_test.go index 0263857c93..69cee94af0 100644 --- a/pkg/inventory/server_tool_test.go +++ b/pkg/inventory/server_tool_test.go @@ -10,42 +10,6 @@ import ( "github.com/stretchr/testify/require" ) -func TestNewServerToolWithDeps_InvalidArguments_ReturnsIsError(t *testing.T) { - type expectedArgs struct { - Owner string `json:"owner"` - Repo string `json:"repo"` - } - - tool := NewServerToolWithDeps( - mcp.Tool{Name: "test_tool"}, - testToolsetMetadata("test"), - func(_ any) mcp.ToolHandlerFor[expectedArgs, *mcp.CallToolResult] { - return func(_ context.Context, _ *mcp.CallToolRequest, _ expectedArgs) (*mcp.CallToolResult, *mcp.CallToolResult, error) { - t.Fatal("handler should not be called with invalid arguments") - return nil, nil, nil - } - }, - ) - - handler := tool.HandlerFunc(nil) - - badArgs, _ := json.Marshal(map[string]any{"owner": 12345, "repo": true}) - result, err := handler(context.Background(), &mcp.CallToolRequest{ - Params: &mcp.CallToolParamsRaw{ - Name: "test_tool", - Arguments: badArgs, - }, - }) - - require.NoError(t, err) - require.NotNil(t, result) - assert.True(t, result.IsError) - assert.Len(t, result.Content, 1) - textContent, ok := result.Content[0].(*mcp.TextContent) - require.True(t, ok) - assert.Contains(t, textContent.Text, "invalid arguments") -} - func TestNewServerToolWithContextHandler_InvalidArguments_ReturnsIsError(t *testing.T) { type expectedArgs struct { Query string `json:"query"` @@ -79,23 +43,21 @@ func TestNewServerToolWithContextHandler_InvalidArguments_ReturnsIsError(t *test assert.Contains(t, textContent.Text, "invalid arguments") } -func TestNewServerToolWithDeps_ValidArguments_Succeeds(t *testing.T) { +func TestNewServerToolWithContextHandler_ValidArguments_Succeeds(t *testing.T) { type expectedArgs struct { Owner string `json:"owner"` Repo string `json:"repo"` } - tool := NewServerToolWithDeps( + tool := NewServerToolWithContextHandler( mcp.Tool{Name: "test_tool"}, testToolsetMetadata("test"), - func(_ any) mcp.ToolHandlerFor[expectedArgs, *mcp.CallToolResult] { - return func(_ context.Context, _ *mcp.CallToolRequest, args expectedArgs) (*mcp.CallToolResult, *mcp.CallToolResult, error) { - return &mcp.CallToolResult{ - Content: []mcp.Content{ - &mcp.TextContent{Text: "success: " + args.Owner + "/" + args.Repo}, - }, - }, nil, nil - } + func(_ context.Context, _ *mcp.CallToolRequest, args expectedArgs) (*mcp.CallToolResult, any, error) { + return &mcp.CallToolResult{ + Content: []mcp.Content{ + &mcp.TextContent{Text: "success: " + args.Owner + "/" + args.Repo}, + }, + }, nil, nil }, ) diff --git a/script/conformance-test b/script/conformance-test index 3ff0a55c27..549ced271f 100755 --- a/script/conformance-test +++ b/script/conformance-test @@ -68,12 +68,6 @@ LIST_TOOLS_MSG='{"jsonrpc":"2.0","id":2,"method":"tools/list","params":{}}' LIST_RESOURCES_MSG='{"jsonrpc":"2.0","id":3,"method":"resources/listTemplates","params":{}}' LIST_PROMPTS_MSG='{"jsonrpc":"2.0","id":4,"method":"prompts/list","params":{}}' -# Dynamic toolset management tool calls (for dynamic mode testing) -LIST_TOOLSETS_MSG='{"jsonrpc":"2.0","id":10,"method":"tools/call","params":{"name":"list_available_toolsets","arguments":{}}}' -GET_TOOLSET_TOOLS_MSG='{"jsonrpc":"2.0","id":11,"method":"tools/call","params":{"name":"get_toolset_tools","arguments":{"toolset":"repos"}}}' -ENABLE_TOOLSET_MSG='{"jsonrpc":"2.0","id":12,"method":"tools/call","params":{"name":"enable_toolset","arguments":{"toolset":"repos"}}}' -LIST_TOOLSETS_AFTER_MSG='{"jsonrpc":"2.0","id":13,"method":"tools/call","params":{"name":"list_available_toolsets","arguments":{}}}' - # Function to normalize JSON for comparison # Sorts all arrays (including nested ones) and formats consistently # Also handles embedded JSON strings in "text" fields (from tool call responses) @@ -154,84 +148,18 @@ run_mcp_test() { echo "$duration" } -# Function to run MCP server with dynamic tool calls (for dynamic mode testing) -run_mcp_dynamic_test() { - local binary="$1" - local name="$2" - local flags="$3" - local output_prefix="$4" - - local start_time end_time duration - start_time=$(date +%s.%N) - - # Run the server with dynamic tool calls in sequence: - # 1. Initialize - # 2. List available toolsets (before enable) - # 3. Get tools for repos toolset - # 4. Enable repos toolset - # 5. List available toolsets (after enable - should show repos as enabled) - output=$( - ( - echo "$INIT_MSG" - echo "$INITIALIZED_MSG" - echo "$LIST_TOOLSETS_MSG" - sleep 0.1 - echo "$GET_TOOLSET_TOOLS_MSG" - sleep 0.1 - echo "$ENABLE_TOOLSET_MSG" - sleep 0.1 - echo "$LIST_TOOLSETS_AFTER_MSG" - sleep 0.3 - ) | GITHUB_PERSONAL_ACCESS_TOKEN=1 $binary stdio $flags 2>/dev/null - ) - - end_time=$(date +%s.%N) - duration=$(echo "$end_time - $start_time" | bc) - - # Parse and save each response by matching JSON-RPC id - echo "$output" | while IFS= read -r line; do - id=$(echo "$line" | jq -r '.id // empty' 2>/dev/null) - case "$id" in - 1) echo "$line" | jq -S '.' > "${output_prefix}_initialize.json" 2>/dev/null ;; - 10) echo "$line" | jq -S '.' > "${output_prefix}_list_toolsets_before.json" 2>/dev/null ;; - 11) echo "$line" | jq -S '.' > "${output_prefix}_get_toolset_tools.json" 2>/dev/null ;; - 12) echo "$line" | jq -S '.' > "${output_prefix}_enable_toolset.json" 2>/dev/null ;; - 13) echo "$line" | jq -S '.' > "${output_prefix}_list_toolsets_after.json" 2>/dev/null ;; - esac - done - - # Create empty files if not created - touch "${output_prefix}_initialize.json" "${output_prefix}_list_toolsets_before.json" \ - "${output_prefix}_get_toolset_tools.json" "${output_prefix}_enable_toolset.json" \ - "${output_prefix}_list_toolsets_after.json" - - # Normalize all JSON files - for endpoint in initialize list_toolsets_before get_toolset_tools enable_toolset list_toolsets_after; do - normalize_json "${output_prefix}_${endpoint}.json" - done - - echo "$duration" -} - -# Test configurations - array of "name|flags|type" -# type can be "standard" or "dynamic" (for dynamic tool call testing) +# Test configurations - array of "name|flags" declare -a TEST_CONFIGS=( - "default||standard" - "read-only|--read-only|standard" - "dynamic-toolsets|--dynamic-toolsets|standard" - "read-only+dynamic|--read-only --dynamic-toolsets|standard" - "toolsets-repos|--toolsets=repos|standard" - "toolsets-issues|--toolsets=issues|standard" - "toolsets-pull_requests|--toolsets=pull_requests|standard" - "toolsets-repos,issues|--toolsets=repos,issues|standard" - "toolsets-all|--toolsets=all|standard" - "tools-get_me|--tools=get_me|standard" - "tools-get_me,list_issues|--tools=get_me,list_issues|standard" - "toolsets-repos+read-only|--toolsets=repos --read-only|standard" - "toolsets-all+dynamic|--toolsets=all --dynamic-toolsets|standard" - "toolsets-repos+dynamic|--toolsets=repos --dynamic-toolsets|standard" - "toolsets-repos,issues+dynamic|--toolsets=repos,issues --dynamic-toolsets|standard" - "dynamic-tool-calls|--dynamic-toolsets|dynamic" + "default|" + "read-only|--read-only" + "toolsets-repos|--toolsets=repos" + "toolsets-issues|--toolsets=issues" + "toolsets-pull_requests|--toolsets=pull_requests" + "toolsets-repos,issues|--toolsets=repos,issues" + "toolsets-all|--toolsets=all" + "tools-get_me|--tools=get_me" + "tools-get_me,list_issues|--tools=get_me,list_issues" + "toolsets-repos+read-only|--toolsets=repos --read-only" ) # Summary arrays @@ -244,36 +172,24 @@ log "${YELLOW}Running conformance tests...${NC}" log "" for config in "${TEST_CONFIGS[@]}"; do - IFS='|' read -r test_name flags test_type <<< "$config" - + IFS='|' read -r test_name flags <<< "$config" + log "${BLUE}Test: ${test_name}${NC}" log " Flags: ${flags:-}" - log " Type: ${test_type}" # Create output directories mkdir -p "$REPORT_DIR/main/$test_name" mkdir -p "$REPORT_DIR/branch/$test_name" mkdir -p "$REPORT_DIR/diffs/$test_name" - if [ "$test_type" = "dynamic" ]; then - # Run dynamic tool call test - main_time=$(run_mcp_dynamic_test "$REPORT_DIR/main/github-mcp-server" "main" "$flags" "$REPORT_DIR/main/$test_name/output") - log " Main: ${main_time}s" - - branch_time=$(run_mcp_dynamic_test "$REPORT_DIR/branch/github-mcp-server" "branch" "$flags" "$REPORT_DIR/branch/$test_name/output") - log " Branch: ${branch_time}s" - - endpoints="initialize list_toolsets_before get_toolset_tools enable_toolset list_toolsets_after" - else - # Run standard test - main_time=$(run_mcp_test "$REPORT_DIR/main/github-mcp-server" "main" "$flags" "$REPORT_DIR/main/$test_name/output") - log " Main: ${main_time}s" - - branch_time=$(run_mcp_test "$REPORT_DIR/branch/github-mcp-server" "branch" "$flags" "$REPORT_DIR/branch/$test_name/output") - log " Branch: ${branch_time}s" - - endpoints="initialize tools resources prompts" - fi + # Run standard test + main_time=$(run_mcp_test "$REPORT_DIR/main/github-mcp-server" "main" "$flags" "$REPORT_DIR/main/$test_name/output") + log " Main: ${main_time}s" + + branch_time=$(run_mcp_test "$REPORT_DIR/branch/github-mcp-server" "branch" "$flags" "$REPORT_DIR/branch/$test_name/output") + log " Branch: ${branch_time}s" + + endpoints="initialize tools resources prompts" # Calculate time difference time_diff=$(echo "$branch_time - $main_time" | bc) @@ -393,7 +309,7 @@ for i in "${!TEST_NAMES[@]}"; do echo "" >> "$REPORT_FILE" # Check all possible endpoints - for endpoint in initialize tools resources prompts list_toolsets_before get_toolset_tools enable_toolset list_toolsets_after; do + for endpoint in initialize tools resources prompts; do diff_file="$REPORT_DIR/diffs/$name/${endpoint}.diff" if [ -f "$diff_file" ] && [ -s "$diff_file" ]; then echo "#### ${endpoint}" >> "$REPORT_FILE"