refactor: Separate ServerTool with HandlerFunc pattern and ToolDependencies

[SamMorrowDrums]

Summary

This PR refactors the ServerTool type to separate tool definitions from handler generation, enabling:

• Static tool definitions that can be passed around without handlers set up
• Lazy handler generation via HandlerFunc when tools are registered
• Dependency injection through a typed ToolDependencies struct

Architecture

ServerTool {
    Tool: mcp.Tool           // Static definition (name, description, schema)
    HandlerFunc: func(any) -> handler  // Generates handler on-demand
}

Avoiding Circular Dependencies

The toolsets package uses any for dependencies to stay generic and avoid importing pkg/github. The type safety is achieved through:

1. pkg/github/dependencies.go - Defines ToolDependencies with proper types
2. NewTool / NewToolFromHandler helpers - Isolate the single type assertion

// Tool implementations are fully typed - no assertions in tool code
func SearchRepositories(t TranslationHelperFunc) toolsets.ServerTool {
    return NewTool(tool, func(deps ToolDependencies) mcp.ToolHandlerFor[...] {
        client, _ := deps.GetClient(ctx)  // fully typed!
        // ...
    })
}

Changes

New file: pkg/github/dependencies.go

• ToolDependencies struct with properly typed fields
• NewTool[In, Out] - typed helper for standard tool handlers
• NewToolFromHandler - typed helper for raw handlers

Updated: pkg/toolsets/server_tool.go

• HandlerFunc uses any for deps to avoid circular imports
• NewServerTool / NewServerToolFromHandler accept any
• NewServerToolLegacy for backward compatibility with existing handlers

Updated: pkg/toolsets/toolsets.go

• deps field typed as any
• SetDependencies(any) method

Updated: internal/ghmcp/server.go

• Creates github.ToolDependencies and passes to toolsets

Benefits

1. Type safety - Tool implementations are fully typed via the helper
2. No scattered assertions - Single assertion point in NewTool helper
3. Flexible for remote server - Can define different deps types with their own helpers
4. Toolsets stays generic - Reusable without GitHub-specific knowledge

Next Steps

Individual tool files can be incrementally migrated from NewServerToolLegacy to NewTool in stacked PRs.

Testing

• All existing tests pass
• No functional changes to tool behavior

[Copilot]

Pull request overview

This PR refactors the ServerTool architecture to separate tool definitions from handler generation, introducing a dependency injection pattern through ToolDependencies and a HandlerFunc type. This enables static tool definitions that can be passed around before dependencies are available, with handlers generated on-demand during registration.

Key Changes:

• New ToolDependencies struct centralizes all shared dependencies (GitHub clients, translation functions, caches, flags)
• HandlerFunc pattern allows lazy handler generation with dependencies injected at registration time
• NewServerToolLegacy provides backward compatibility during the migration
• SetDependencies() method on Toolset enables fluent API for dependency configuration

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
pkg/toolsets/server_tool.go	New file defining ToolDependencies, HandlerFunc, and refactored ServerTool struct with constructor functions
pkg/toolsets/toolsets.go	Adds deps field to Toolset, implements SetDependencies() method, updates RegisterTools to use stored deps, modifies RegisterSpecificTools signature
pkg/toolsets/toolsets_test.go	Updates mockTool to use new API (NewServerToolFromHandler), adds InputSchema to mock tools, passes deps to RegisterSpecificTools
pkg/github/tools.go	Creates ToolDependencies struct and calls SetDependencies() on all toolsets, converts all tools to use NewServerToolLegacy
pkg/github/dynamic_tools.go	Simplifies EnableToolset to use toolset.RegisterTools(s) instead of manual loop
internal/ghmcp/server.go	Updates RegisterSpecificTools call to include ToolDependencies{} parameter and imports toolsets package

[Copilot]

An empty ToolDependencies{} is being passed to RegisterSpecificTools, which means the registered tools won't have access to the GitHub clients, translation functions, or other dependencies they need. The dependencies were already created and set on the toolsets at lines 167-180 via SetDependencies(deps), but RegisterSpecificTools bypasses the toolset's stored dependencies and uses the deps parameter instead.

The solution is to either:

1. Remove the deps parameter from RegisterSpecificTools and have it use the dependencies stored in each toolset (via SetDependencies), or
2. Pass the actual deps struct here instead of an empty one

Option 1 would be more consistent with how RegisterAll works (which calls toolset.RegisterTools(s) that uses the stored deps), while option 2 would require exposing the deps or reconstructing it here.

Suggested change

	err = tsg.RegisterSpecificTools(ghServer, enabledTools, cfg.ReadOnly, toolsets.ToolDependencies{})
	err = tsg.RegisterSpecificTools(ghServer, enabledTools, cfg.ReadOnly)

[Copilot]

The RegisterFunc method registers a tool with a potentially nil handler (if HandlerFunc is nil, Handler() returns nil). While this may be intentional to support partial construction, it could lead to runtime errors when the MCP server tries to call the handler. Consider either:

1. Adding validation to prevent nil handlers from being registered
2. Documenting when and why HandlerFunc might be nil
3. Making HandlerFunc non-optional in the ServerTool struct

Suggested change

	// RegisterFunc registers the tool with the server using the provided dependencies.
	func (st *ServerTool) RegisterFunc(s *mcp.Server, deps ToolDependencies) {
	// RegisterFunc registers the tool with the server using the provided dependencies.
	// RegisterFunc registers the tool with the server using the provided dependencies.
	// Panics if HandlerFunc is nil, as registering a tool with a nil handler is a programming error.
	func (st *ServerTool) RegisterFunc(s *mcp.Server, deps ToolDependencies) {
	if st.HandlerFunc == nil {
	panic("toolsets: cannot register tool '" + st.Tool.Name + "' with nil HandlerFunc")
	}

[SamMorrowDrums]

Migration Progress Update

Added the first tool file migration: search.go

Changes in this commit (b29546e)

• Migrated SearchRepositories, SearchCode, SearchUsers, SearchOrgs to use the new NewTool helper pattern
• Functions now take only TranslationHelperFunc and return toolsets.ServerTool directly
• Handler generation uses ToolDependencies for typed access to clients (deps.GetClient(ctx))
• Updated tools.go call sites to remove getClient parameter and use the tools directly (no NewServerToolLegacy wrapper needed)
• Updated tests to use the new Handler(deps) pattern

Migration Pattern Demonstrated

Old signature:

func SearchRepositories(getClient GetClientFn, t translations.TranslationHelperFunc) (mcp.Tool, mcp.ToolHandlerFor[...])

New signature:

func SearchRepositories(t translations.TranslationHelperFunc) toolsets.ServerTool

The key benefit: no type assertions in tool code. The single type assertion is isolated in the NewTool helper.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.