Skip to content

Conversation

@SamMorrowDrums
Copy link
Collaborator

@SamMorrowDrums SamMorrowDrums commented Dec 13, 2025

Summary

Major refactor of the toolsets package to create an elegant, immutable filtering system with self-describing tools.

Key Changes

Self-Describing Tools:

  • Tools now embed their own ToolsetMetadata (ID and description)
  • Read-only hint is derived from Tool.Annotations.ReadOnlyHint
  • No more separate AddReadTools() / AddWriteTools()

Simplified ToolsetGroup:

  • NewToolsetGroup(tools, resources, prompts) - takes lists directly
  • No more Toolset struct with SetDependencies()
  • Dependencies are only needed at registration time: RegisterAll(ctx, server, deps)

Immutable Filter Chain:

filtered := tsg.
    WithReadOnly(true).
    WithToolsets([]string{"repos", "issues"}).
    WithTools([]string{"get_me"}).  // additive, bypasses toolset filter
    WithFeatureChecker(checker)       // for feature flags

tools := filtered.AvailableTools(ctx)

Feature Flags:

  • FeatureFlagEnable - tool only available when flag is on
  • FeatureFlagDisable - tool excluded when flag is on
  • --features CLI flag for local server
  • Context-based checker for remote server (actor extraction)

Deterministic Ordering:

  • All Available*() methods return items sorted by toolset ID, then name
  • Platform/language independent for consistent docs generation

New Files:

  • pkg/github/toolset_group.go - NewToolsetGroup() factory
  • pkg/github/prompts.go - AllPrompts()
  • pkg/github/resources.go - AllResources()
  • docs/deprecated-tool-aliases.md - alias documentation

Usage

# Enable specific features
github-mcp-server stdio --features=my_feature,another_feature
GITHUB_FEATURES=my_feature github-mcp-server stdio

Stack:

Add CLI flag and config support for feature flags in the local server:

- Add --features flag to main.go (StringSlice, comma-separated)
- Add EnabledFeatures field to StdioServerConfig and MCPServerConfig
- Create createFeatureChecker() that builds a set from enabled features
- Wire WithFeatureChecker() into the toolset group filter chain

This enables tools/resources/prompts that have FeatureFlagEnable set to
a flag name that is passed via --features. The checker uses a simple
set membership test for O(1) lookup.

Usage:
  github-mcp-server stdio --features=my_feature,another_feature
  GITHUB_FEATURES=my_feature github-mcp-server stdio
Copilot AI review requested due to automatic review settings December 13, 2025 22:54
@SamMorrowDrums SamMorrowDrums requested a review from a team as a code owner December 13, 2025 22:54
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 adds feature flag support to the local GitHub MCP server, enabling runtime control over which tools, resources, and prompts are available based on enabled features. The implementation includes a new --features CLI flag, feature flag fields on ServerTool/ServerResource/ServerPrompt, and a filtering mechanism integrated into the toolset group's filter chain.

Key Changes:

  • Added --features CLI flag and GITHUB_FEATURES environment variable support for comma-separated feature flags
  • Implemented FeatureFlagChecker interface and createFeatureChecker() for O(1) flag lookup
  • Added FeatureFlagEnable and FeatureFlagDisable fields to ServerTool, ServerResourceTemplate, and ServerPrompt
  • Integrated feature flag filtering into WithFeatureChecker() method with immutable filter chaining

Reviewed changes

Copilot reviewed 35 out of 35 changed files in this pull request and generated no comments.

Show a summary per file
File Description
pkg/toolsets/toolsets.go Core feature flag filtering logic: FeatureFlagChecker type, WithFeatureChecker() method, isFeatureFlagAllowed() logic integrated into Available* methods
pkg/toolsets/toolsets_test.go Comprehensive test coverage for feature flags (enable/disable/both/errors) across tools, resources, and prompts
pkg/toolsets/server_tool.go Added FeatureFlagEnable and FeatureFlagDisable string fields to ServerTool, ServerResourceTemplate, and ServerPrompt
pkg/github/toolset_group.go New factory function for creating ToolsetGroup with all tools/resources/prompts
internal/ghmcp/server.go Added EnabledFeatures config field, createFeatureChecker() implementation, wired into filter chain via WithFeatureChecker()
cmd/github-mcp-server/main.go Added --features persistent flag with viper binding and GITHUB_FEATURES env var support
Multiple pkg/github/*.go files Updated all tool/resource/prompt constructors to include toolset metadata parameter (structural refactor supporting feature flags)

@SamMorrowDrums SamMorrowDrums changed the title Add --features CLI flag for feature flag support refactor: immutable ToolsetGroup with self-describing tools and feature flags Dec 13, 2025
This commit adds comprehensive validation tests to ensure all MCP items
have required metadata:

- TestAllToolsHaveRequiredMetadata: Validates Toolset.ID and Annotations
- TestAllToolsHaveValidToolsetID: Ensures toolsets are in AvailableToolsets()
- TestAllResourcesHaveRequiredMetadata: Validates resource metadata
- TestAllPromptsHaveRequiredMetadata: Validates prompt metadata
- TestToolReadOnlyHintConsistency: Validates IsReadOnly() matches annotation
- TestNoDuplicate*Names: Ensures unique names across tools/resources/prompts
- TestAllToolsHaveHandlerFunc: Ensures all tools have handlers
- TestDefaultToolsetsAreValid: Validates default toolset IDs
- TestToolsetMetadataConsistency: Ensures consistent descriptions per toolset

Also fixes a bug discovered by these tests: ToolsetMetadataGit was defined
but not added to AvailableToolsets(), causing get_repository_tree to have
an invalid toolset ID.
When no toolsets are specified and dynamic mode is disabled, the server
should use the default toolsets. The bug was introduced when adding
dynamic toolsets support:

1. CleanToolsets(nil) was converting nil to empty slice
2. Empty slice passed to WithToolsets means 'no toolsets'
3. This resulted in zero tools being registered

Fix: Preserve nil for non-dynamic mode (nil = use defaults in WithToolsets)
and only set empty slice when dynamic mode is enabled without explicit
toolsets.
@SamMorrowDrums SamMorrowDrums force-pushed the SamMorrowDrums/server-tool-refactor-toolsets branch from 78c89a3 to 689a040 Compare December 13, 2025 23:13
}

// mockGetRawClient returns a mock raw client for documentation generation
func mockGetRawClient(_ context.Context) (*raw.Client, error) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am sure this can be removed too...

- Rename AddDeprecatedToolAliases to WithDeprecatedToolAliases for
  immutable filter chain consistency (returns new ToolsetGroup)
- Remove unused mockGetRawClient from generate_docs.go (use nil instead)
- Remove legacy ServerTool functions (NewServerToolLegacy and
  NewServerToolFromHandlerLegacy) - no usages
- Add panic in Handler()/RegisterFunc() when HandlerFunc is nil
- Add HasHandler() method for checking if tool has a handler
- Add tests for HasHandler and nil handler panic behavior
- Update all tests to use new WithDeprecatedToolAliases pattern
…lsetGroup

This change applies the same HandlerFunc pattern used by tools to resources,
allowing NewToolsetGroup to be fully stateless (only requiring translations).

Key changes:
- Add ResourceHandlerFunc type to toolsets package
- Update ServerResourceTemplate to use HandlerFunc instead of direct Handler
- Add HasHandler() and Handler(deps) methods to ServerResourceTemplate
- Update RegisterResourceTemplates to take deps parameter
- Refactor repository resource definitions to use HandlerFunc pattern
- Make AllResources(t) stateless (only takes translations)
- Make NewToolsetGroup(t) stateless (only takes translations)
- Update generate_docs.go - no longer needs mock clients
- Update tests to use new patterns

This resolves the concern about mixed concerns in doc generation - the
toolset metadata and resource templates can now be created without any
runtime dependencies, while handlers are generated on-demand when deps
are provided during registration.
- Replace slice joining with strings.Builder for all doc generation
- Iterate AllTools() directly instead of ToolsetIDs()/ToolsForToolset()
- Removes need for special 'dynamic' toolset handling (no tools = no output)
- Context toolset still explicitly handled for custom description
- Consistent pattern across generateToolsetsDoc, generateToolsDoc,
  generateRemoteToolsetsDoc, and generateDeprecatedAliasesTable
- Add AvailableToolsets() method that returns toolsets with actual tools
- Support variadic exclude parameter for filtering out specific toolsets
- Simplifies doc generation by removing manual skip logic
- Naturally excludes empty toolsets (like 'dynamic') without special cases
@SamMorrowDrums SamMorrowDrums force-pushed the SamMorrowDrums/server-tool-refactor-toolsets branch 6 times, most recently from 9ef4da2 to a536c15 Compare December 14, 2025 23:48
- Add Default field to ToolsetMetadata and derive defaults from metadata
- Move toolset validation into WithToolsets (trims whitespace, dedupes, tracks unrecognized)
- Add UnrecognizedToolsets() method for warning about typos
- Add DefaultToolsetIDs() method to derive defaults from metadata
- Remove redundant functions: CleanToolsets, GetValidToolsetIDs, AvailableToolsets, GetDefaultToolsetIDs
- Update DynamicTools to take ToolsetGroup for schema enum generation
- Add stubTranslator for cases needing ToolsetGroup without translations

This eliminates hardcoded toolset lists - everything is now derived from
the actual registered tools and their metadata.
@SamMorrowDrums SamMorrowDrums force-pushed the SamMorrowDrums/server-tool-refactor-toolsets branch from a536c15 to 6b6a874 Compare December 14, 2025 23:50
// copy creates a shallow copy of the Registry for immutable operations.
func (r *Registry) copy() *Registry {
newTG := &Registry{
tools: r.tools, // slices are shared (immutable)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by this comment? I think it's still possible to get a reference to particular ServerTool (for example by calling AvailableTools()), change fields and then tools slice will be changed as well. In order to make it truly immutable we need to make a deep copy

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The immutable copy is for the registry metadata only, the protection is that a function cannot edit the copy of the registry that you hold, and change the filters you think have applied.

We could call it out in comments, but we don't want the expense of deep copy, and it's nice that it always returns a Registry. We could do a builder version, that's an option too.

WDYT?

The only issue with the builder version is if you want Registry in an intermediate state.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i.e. I'm not worried about people mutating tools too much and don't want to copy 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right though, the immutable is in wrong place.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx, that makes sense :)

// When true, write tools are filtered out from Available* methods.
func (r *Registry) WithReadOnly(readOnly bool) *Registry {
newTG := r.copy()
newTG.readOnly = readOnly
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the example from PR description the intended use of this function as as a builder pattern:

filtered := tsg.
    WithReadOnly(true).
    WithToolsets([]string{"repos", "issues"}).
    WithTools([]string{"get_me"}).  // additive, bypasses toolset filter
    WithFeatureChecker(checker)  

In current implementation every builder function is unnecessarily copying the registry. Each Registry.copy() is iterating over tool slices to copy them too.
Maybe we could introduce a struct like RegistryBuilder, then functions as WithReadOnly will just set one field in the builder. Once the user is done with constructing a new object she would call build() on the builder instance, which returns the copied registry object with some fields overridden.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will do the builder pattern. Agree we don't need intermediate state and we couldn't efficiently achieve full immutability anyway! ❤️ thank you.

- 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 and others added 16 commits December 16, 2025 19:53
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)
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.
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.
- Add Enabled field to ServerTool for self-filtering based on context
- Add ToolFilter type and WithFilter method to Builder for cross-cutting filters
- Update isToolEnabled to check Enabled function and builder filters in order:
  1. Tool's Enabled function
  2. Feature flags (FeatureFlagEnable/FeatureFlagDisable)
  3. Read-only filter
  4. Builder filters
  5. Toolset/additional tools check
- Add FilteredTools method to Registry as alias for AvailableTools
- Add comprehensive tests for all new functionality
- All tests pass and linter is clean

Closes #1618

Co-authored-by: SamMorrowDrums <4811358+SamMorrowDrums@users.noreply.github.com>
- Add numbered filter evaluation order to isToolEnabled function doc
- Number inline comments for each filter step (1-5)
- Clarify FilteredTools error return is for future extensibility
- Document that library consumers may need to surface recoverable errors

Addresses review feedback on PR #1620
Co-authored-by: SamMorrowDrums <4811358+SamMorrowDrums@users.noreply.github.com>
@SamMorrowDrums SamMorrowDrums merged commit fae9d5c into SamMorrowDrums/server-tool-refactor-projects Dec 16, 2025
15 checks passed
@SamMorrowDrums SamMorrowDrums deleted the SamMorrowDrums/server-tool-refactor-toolsets branch December 16, 2025 19:01
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