refactor: remove dynamic toolsets and deprecated closure constructor#2512
Merged
Conversation
b962757 to
20e3c5f
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Removes the local-only dynamic toolset discovery feature and its supporting plumbing (meta-tools, config/flags, inventory APIs), and deletes the deprecated closure-based NewServerToolWithDeps constructor to simplify the tool registration surface area.
Changes:
- Removed dynamic toolset discovery mode end-to-end (CLI/env/config wiring, server registration path, docs, and conformance script coverage).
- Simplified inventory/tool registration by removing dynamic toolset management APIs and the deprecated closure-based tool constructor.
- Updated docs/help generation to exclude only the
contexttoolset (sincedynamicno longer exists).
Show a summary per file
| File | Description |
|---|---|
| script/conformance-test | Removed dynamic-mode test paths and configs from conformance runner. |
| README.md | Removed dynamic tool discovery documentation and updated tool configuration guidance. |
| pkg/inventory/server_tool.go | Removed deprecated NewServerToolWithDeps constructor. |
| pkg/inventory/server_tool_test.go | Dropped tests for the removed constructor; kept/updated context-handler tests. |
| pkg/inventory/registry.go | Updated inventory doc comment to remove dynamic enabling reference. |
| pkg/inventory/registry_test.go | Removed tests for deleted dynamic toolset inventory APIs. |
| pkg/inventory/filters.go | Removed dynamic toolset management methods (ToolsForToolset, EnableToolset, etc.). |
| pkg/inventory/builder.go | Updated toolset config docs now that “dynamic empty toolsets” mode is gone. |
| pkg/http/server.go | Removed DynamicToolsets from HTTP server config. |
| pkg/http/handler.go | Removed dynamic-toolsets branching and updated toolset resolution call signature. |
| pkg/http/handler_test.go | Updated tests for new ResolvedEnabledToolsets signature. |
| pkg/github/tools.go | Removed ToolsetMetadataDynamic; updated toolsets help generation exclusion list. |
| pkg/github/server.go | Removed DynamicToolsets config and dynamic meta-tool registration; simplified toolset resolution. |
| pkg/github/server_test.go | Updated ResolvedEnabledToolsets tests to match new behavior/signature. |
| pkg/github/dynamic_tools.go | Deleted dynamic toolset management tools implementation. |
| pkg/github/dynamic_tools_test.go | Deleted tests for removed dynamic toolset management tools. |
| internal/ghmcp/server.go | Removed dynamic-toolsets config wiring and logging in stdio server setup. |
| docs/toolsets-and-icons.md | Removed the “Dynamic” toolset icon entry. |
| docs/server-configuration.md | Removed dynamic mode documentation and references. |
| docs/installation-guides/README.md | Removed mention of dynamic tool discovery. |
| cmd/github-mcp-server/main.go | Removed --dynamic-toolsets flag and associated viper binding/config propagation. |
| cmd/github-mcp-server/generate_docs.go | Updated toolset documentation generation to stop excluding dynamic. |
| .github/copilot-instructions.md | Removed dynamic toolsets env var/flag references from contributor guidance. |
Copilot's findings
- Files reviewed: 24/24 changed files
- Comments generated: 1
Dynamic toolset discovery (the meta-tools enable_toolset, list_available_toolsets, get_toolset_tools and the --dynamic-toolsets / GITHUB_DYNAMIC_TOOLSETS switch) was a local-only feature never offered by the remote server. Removing it deletes a meaningful chunk of branching, configuration surface and tests for a path no longer in active use. The deprecated closure-based NewServerToolWithDeps generic constructor was only kept around for the dynamic tool registration path and is removed together with it. Going forward there are exactly two constructors: - NewServerTool — raw mcp.ToolHandler, no closure, no unmarshalling - NewServerToolWithContextHandler[In, Out] — typed handler, deps via context Inventory methods that only existed for the dynamic path (ToolsForToolset, IsToolsetEnabled, EnableToolset, EnabledToolsetIDs) are removed. ResolvedEnabledToolsets loses its dynamic flag. Also strips dynamic references from the README, server configuration docs, copilot-instructions, mcp-diff workflow, and conformance-test script. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
20e3c5f to
3c39808
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
Removes the dynamic toolset discovery feature (the
--dynamic-toolsets/GITHUB_DYNAMIC_TOOLSETSswitch and the meta-toolsenable_toolset,list_available_toolsets,get_toolset_tools) along with the deprecated closure-basedNewServerToolWithDepsconstructor that only existed to support it.This is tech debt cleanup:
ToolsForToolset,IsToolsetEnabled,EnableToolset,EnabledToolsetIDs), an extra branch inResolvedEnabledToolsets, three meta-tools, and a chunk of conformance/CI matrix.NewServerTool— rawmcp.ToolHandler, no closure, no unmarshallingNewServerToolWithContextHandler[In, Out]— typed handler, deps via contextDiff highlights
pkg/github/dynamic_tools.go+ test → deletedpkg/inventory/server_tool.go—NewServerToolWithDepsremovedpkg/inventory/filters.go—ToolsForToolset,IsToolsetEnabled,EnableToolset,EnabledToolsetIDsremovedResolvedEnabledToolsets(dynamic, toolsets, tools)→ResolvedEnabledToolsets(toolsets, tools)DynamicToolsetsfield removed fromMCPServerConfig,StdioServerConfig, httpServerConfig--dynamic-toolsetsflag removedToolsetMetadataDynamicremoved;AvailableToolsets("context", "dynamic")→AvailableToolsets("context").github/workflows/mcp-diff.ymlchange that drops the dynamic test matrix isn't included in this PR (OAuth scope) — please apply it as a follow-upVerification
script/lintcleanscript/testpassesscript/generate-docsproduces no extra diff after this commitStacks on (now merged) #2511.