refactor: remove dynamic toolsets and deprecated closure constructor - #2512
Merged
Conversation
SamMorrowDrums
force-pushed
the
sammorrowdrums/remove-dynamic-toolsets
branch
from
May 20, 2026 08:35
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>
SamMorrowDrums
force-pushed
the
sammorrowdrums/remove-dynamic-toolsets
branch
from
May 20, 2026 08:46
20e3c5f to
3c39808
Compare
This was referenced Aug 10, 2026
katayama8000
added a commit
to nulab/backlog-mcp-server
that referenced
this pull request
Aug 10, 2026
`--dynamic-toolsets` / `ENABLE_DYNAMIC_TOOLSETS` and the three meta-tools `enable_toolset`, `list_available_toolsets` and `get_toolset_tools` are gone, along with the plumbing that existed only for them. #179 argued the feature is unsound on the HTTP transport: MCP 2026-07-28 removed protocol sessions, so `enable_toolset` cannot keep its promise that a toolset stays enabled *for the caller* — the state lives on the shared toolset group, one client's call rewrites what every other client of the process sees, and there is no disable_toolset. The reference implementation reached the same conclusion first, and went further. GitHub removed the identical feature — same flag name, same three tool names — in github/github-mcp-server#2512 (May 2026, +51/-942), citing reasons #179 arrived at independently: local-only, never offered over the remote transport, and a parallel registration path plus config plumbing and four inventory methods kept alive for it. Confirmed against their current main: no `dynamic-toolsets` flag, and code search for `DynamicToolset` returns nothing. Experimental behaviour moved under `--features`, a different mechanism. So there is no ecosystem convention left to stay consistent with, and this was already marked Experimental here. Removed: src/tools/dynamicTools/toolsets.ts the three meta-tools src/utils/toolRegistrar.ts createToolRegistrar src/dynamicToolsetPersistence.test.ts pinned the shared-group mutation ToolRegistrar from types/tool.ts and lib.ts getToolset / enableToolset from utils/toolsetUtils.ts `registerDynamicTools` stays: `list_organizations` uses it, along with `DynamicToolDefinition` and `DynamicToolsetGroup`. Two things follow from nothing mutating the toolset group any more: the `tools/list` cache hint is always published now, and the shared group is shared only to avoid rebuilding the tool tree per request. Breaking twice over. `ToolRegistrar` was exported from lib.ts, so library consumers are affected as well as CLI users. And since yargs ignores unknown flags, an existing setup would otherwise have started with a quietly different tool list: the flag used to drop `all` from the enabled toolsets, so passing only it went from no toolsets plus three meta-tools to every toolset enabled. Both the flag and the environment variable now print a notice rather than failing, so a config carrying `ENABLE_DYNAMIC_TOOLSETS: "1"` keeps starting. toolsetUtils.test.ts covered only the two removed helpers and would have become empty; rewritten to cover `buildToolsetGroup`, which had no direct tests. Part of #179. The other half of that issue — the `fields` description payload and whether --optimize-response should exist — is untouched, so it stays open. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.