refactor: rename pkg/toolsets to pkg/registry with builder pattern #1607
Conversation
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
- Rename pkg/toolsets to pkg/registry (better reflects its purpose)
- Split monolithic toolsets.go into focused files:
- registry.go: Core Registry struct and MCP methods
- builder.go: Builder pattern for creating Registry instances
- filters.go: All filtering logic (toolsets, read-only, feature flags)
- resources.go: ServerResourceTemplate type
- prompts.go: ServerPrompt type
- errors.go: Error types
- server_tool.go: ServerTool and ToolsetMetadata (existing)
- Fix lint: Rename RegistryBuilder to Builder (avoid stuttering)
- Update all imports across ~45 files
This refactoring improves code organization and makes the registry's
purpose clearer. The builder pattern provides a clean API:
reg := registry.NewBuilder().
SetTools(tools).
WithReadOnly(true).
WithToolsets([]string{"repos"}).
Build()
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the pkg/toolsets package to pkg/registry with improved organization and a builder pattern API. The main changes include renaming the package, splitting the monolithic toolsets.go into focused files (registry.go, builder.go, filters.go, etc.), introducing a builder pattern for constructing Registry instances, and converting ToolDependencies from a struct to an interface with a BaseDeps implementation.
Key Changes
- Package rename:
pkg/toolsets→pkg/registrywith ~45 file imports updated - Builder pattern: New
NewBuilder().SetTools().Build()API replacing directNewRegistry().SetTools() - ToolDependencies refactor: Changed from struct to interface with
BaseDepsimplementation for better flexibility
Reviewed changes
Copilot reviewed 56 out of 56 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/registry/registry.go | Core Registry struct, MCP constants, and main registry operations |
| pkg/registry/builder.go | Builder pattern implementation with fluent API |
| pkg/registry/filters.go | Tool/resource/prompt filtering logic extraction |
| pkg/registry/server_tool.go | ServerTool type (package declaration change only) |
| pkg/registry/resources.go | ServerResourceTemplate and related types |
| pkg/registry/prompts.go | ServerPrompt type definitions |
| pkg/registry/errors.go | Error type definitions |
| pkg/registry/registry_test.go | Test updates for builder pattern |
| pkg/github/dependencies.go | ToolDependencies struct→interface, BaseDeps implementation |
| pkg/github/*_test.go | Test updates to use BaseDeps |
| internal/ghmcp/server.go | Server setup refactoring with new builder pattern |
| e2e/e2e_test.go | E2E test updates for builder pattern |
| cmd/github-mcp-server/generate_docs.go | Documentation generation updates |
JoannaaKL
reviewed
Dec 15, 2025
| // - Runtime toolset enabling for dynamic toolsets mode | ||
| type Registry struct { | ||
| // tools holds all tools in this group | ||
| tools []ServerTool |
Contributor
There was a problem hiding this comment.
Can each of these collections be a map already?
JoannaaKL
reviewed
Dec 15, 2025
| func (r *Registry) ToolsetIDs() []ToolsetID { | ||
| seen := make(map[ToolsetID]bool) | ||
| for i := range r.tools { | ||
| seen[r.tools[i].Toolset.ID] = true |
Contributor
There was a problem hiding this comment.
It doesn't need to be a map, we can use a set here and just check if the id is already there. (Although in go it's not such a big difference, set is anyway a map ;__;)
Two behavioral regressions were fixed in resolveEnabledToolsets(): 1. When --tools=X is used without --toolsets, the server should only register the specified tools, not the default toolsets. Now returns an empty slice instead of nil when EnabledTools is set. 2. When --toolsets=all --dynamic-toolsets is used, the 'all' and 'default' pseudo-toolsets should be removed so only the dynamic management tools are registered. This matches the original pre-refactor behavior.
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.
Summary
Refactors the
pkg/toolsetspackage topkg/registrywith improved organization and a builder pattern API.Changes
Package Rename
pkg/toolsets→pkg/registryFile Organization
Split the monolithic
toolsets.gointo focused files:registry.gobuilder.gofilters.goserver_tool.goresources.goprompts.goerrors.goBuilder Pattern API
New fluent builder API for constructing registries:
Lint Fix
RegistryBuilder→Builderto avoid Go stuttering lint error (registry.RegistryBuilder)Testing
script/lintpassesgo build ./...succeedsStack
This PR is stacked on #1602