Skip to content

Conversation

@SamMorrowDrums
Copy link
Collaborator

Summary

Refactors the pkg/toolsets package to pkg/registry with improved organization and a builder pattern API.

Changes

Package Rename

  • Renamed pkg/toolsetspkg/registry
  • Updated all imports across the codebase (~45 files)

File Organization

Split the monolithic toolsets.go into focused files:

File Purpose
registry.go Core Registry struct, MCP method constants, ForMCPRequest optimization
builder.go Builder pattern for constructing Registry instances
filters.go Tool/resource/prompt filtering logic
server_tool.go ServerTool, ToolsetMetadata, ToolsetID types
resources.go ServerResourceTemplate type
prompts.go ServerPrompt type
errors.go ToolsetDoesNotExistError, ToolDoesNotExistError

Builder Pattern API

New fluent builder API for constructing registries:

registry := registry.NewBuilder().
    SetTools(tools...).
    SetResources(resources...).
    SetPrompts(prompts...).
    WithToolsets(enabledToolsets).
    WithReadOnly(true).
    WithFeatureChecker(checker).
    Build()

Lint Fix

  • Renamed RegistryBuilderBuilder to avoid Go stuttering lint error (registry.RegistryBuilder)

Testing

  • All existing tests pass
  • script/lint passes
  • go build ./... succeeds

Stack

This PR is stacked on #1602

- 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()
@SamMorrowDrums SamMorrowDrums requested a review from a team as a code owner December 15, 2025 14:35
Copilot AI review requested due to automatic review settings December 15, 2025 14:35
Copy link
Contributor

Copilot AI left a 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/toolsetspkg/registry with ~45 file imports updated
  • Builder pattern: New NewBuilder().SetTools().Build() API replacing direct NewRegistry().SetTools()
  • ToolDependencies refactor: Changed from struct to interface with BaseDeps implementation 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)
@SamMorrowDrums SamMorrowDrums force-pushed the SamMorrowDrums/registry-builder-pattern branch 3 times, most recently from 41189d1 to c7dc8fd Compare December 15, 2025 21:58
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.
@SamMorrowDrums SamMorrowDrums force-pushed the SamMorrowDrums/registry-builder-pattern branch from c7dc8fd to 80fd086 Compare December 15, 2025 22:00
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants