-
Notifications
You must be signed in to change notification settings - Fork 3.2k
refactor: rename pkg/toolsets to pkg/registry with builder pattern #1607
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.
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
base: SamMorrowDrums/server-tool-refactor-toolsets
Are you sure you want to change the base?
refactor: rename pkg/toolsets to pkg/registry with builder pattern #1607
Conversation
- 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()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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.
Labels are closely related to issues - you add labels to issues, search issues by label, etc. Keeping them in a separate toolset required users to explicitly enable 'labels' to get this functionality. Moving to issues toolset makes labels available by default since issues is a default toolset.
This restores conformance with the original behavior where: - get_label is in issues toolset (read-only label access for issue workflows) - get_label, list_label, label_write are in labels toolset (full management) The duplicate get_label registration is intentional - it was in both toolsets in the original implementation. Added test exception to allow this case.
- Expand nil toolsets to default IDs before GenerateInstructions (nil means 'use defaults' in registry but instructions need actual names) - Remove unconditional HasTools/HasResources/HasPrompts=true in NewServer (let SDK determine capabilities based on registered items, matching main)
Tests cover: - list_available_toolsets: verifies toolsets are listed with enabled status - get_toolset_tools: verifies tools can be retrieved for a toolset - enable_toolset: verifies toolset can be enabled and marked as enabled - enable_toolset invalid: verifies proper error for non-existent toolset - toolsets enum: verifies tools have proper enum values in schema
In dynamic mode, explicitly set HasTools/HasResources/HasPrompts=true since toolsets with those capabilities can be enabled at runtime. This ensures clients know the server supports these features even when no tools/resources/prompts are initially registered.
- Add dynamic tool call testing (list_available_toolsets, get_toolset_tools, enable_toolset) - Parse and sort embedded JSON in text fields for proper comparison - Separate progress output (stderr) from summary (stdout) for CI - Add test type field to distinguish standard vs dynamic tests
- Runs on pull requests to main - Compares PR branch against merge-base with origin/main - Outputs full conformance report to GitHub Actions Job Summary - Uploads detailed report as artifact for deeper investigation - Does not fail the build on differences (may be intentional)
41189d1 to
c7dc8fd
Compare
Address review feedback to use maps for collections. Added lookup maps (toolsByName, resourcesByURI, promptsByName) while keeping slices for ordered iteration. This provides O(1) lookup for: - FindToolByName - filterToolsByName (used by ForMCPRequest) - filterResourcesByURI - filterPromptsByName Maps are built once during Build() and shared in ForMCPRequest copies.
c7dc8fd to
80fd086
Compare
Add toolsetIDSet (map[ToolsetID]bool) to Registry for O(1) HasToolset lookups. Previously HasToolset iterated through all tools, resourceTemplates, and prompts to check if any belonged to the given toolset. Now it's a simple map lookup. The set is populated during the single-pass processToolsets() call, which already collected all valid toolset IDs. This adds zero new iteration - just returns the existing validIDs map. processToolsets now returns 6 values: - enabledToolsets, unrecognized, toolsetIDs, toolsetIDSet, defaultToolsetIDs, descriptions
FindToolByName() is only called once per request at most (to find toolset ID for dynamic enablement). The SDK handles tool dispatch after registration. A simple linear scan over ~90 tools is trivially fast and avoids: - sync.Once complexity - Map allocation - Premature optimization for non-existent 'repeated lookups' The pre-computed maps we keep (toolsetIDSet, etc.) are justified because they're used for filtering logic that runs on every request.
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