refactor: Separate ServerTool with HandlerFunc pattern and ToolDependencies #1589

SamMorrowDrums · 2025-12-13T11:17:42Z

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:

pkg/github/dependencies.go - Defines ToolDependencies with proper types
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

Type safety - Tool implementations are fully typed via the helper
No scattered assertions - Single assertion point in NewTool helper
Flexible for remote server - Can define different deps types with their own helpers
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

- Extract ServerTool struct into pkg/toolsets/server_tool.go - Add ToolDependencies struct for passing common dependencies to handlers - HandlerFunc allows lazy handler generation from Tool definitions - NewServerTool for new dependency-based tools - NewServerToolLegacy for backward compatibility with existing handlers - Update toolsets.go to store and pass dependencies - Update all call sites to use NewServerToolLegacy Co-authored-by: Adam Holt <4619+omgitsads@users.noreply.github.com>

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 · 2025-12-13T11:21:53Z

internal/ghmcp/server.go


 		// Register the specified tools (additive to any toolsets already enabled)
-		err = tsg.RegisterSpecificTools(ghServer, enabledTools, cfg.ReadOnly)
+		err = tsg.RegisterSpecificTools(ghServer, enabledTools, cfg.ReadOnly, toolsets.ToolDependencies{})


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:

Remove the deps parameter from RegisterSpecificTools and have it use the dependencies stored in each toolset (via SetDependencies), or

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)

pkg/toolsets/server_tool.go

Copilot · 2025-12-13T11:21:54Z

pkg/toolsets/server_tool.go

+// RegisterFunc registers the tool with the server using the provided dependencies.
+func (st *ServerTool) RegisterFunc(s *mcp.Server, deps ToolDependencies) {


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:

Adding validation to prevent nil handlers from being registered

Documenting when and why HandlerFunc might be nil

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")

}

- Move ToolDependencies to pkg/github/dependencies.go with proper types - Use 'any' in toolsets package to avoid circular dependencies - Add NewTool/NewToolFromHandler helpers that isolate type assertion - Tool implementations will be fully typed with no assertions scattered - Infrastructure ready for incremental tool migration

SamMorrowDrums · 2025-12-13T11:47:43Z

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.

Migrate search.go tools (SearchRepositories, SearchCode, SearchUsers, SearchOrgs) to use the new NewTool helper and ToolDependencies pattern. - Functions now take only TranslationHelperFunc and return ServerTool - Handler generation uses ToolDependencies for typed access to clients - Update tools.go call sites to remove getClient parameter - Update tests to use new Handler(deps) pattern This demonstrates the migration pattern for additional tool files. Co-authored-by: Adam Holt <omgitsads@users.noreply.github.com>

Copilot

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

Copilot AI review requested due to automatic review settings December 13, 2025 11:17

SamMorrowDrums requested a review from a team as a code owner December 13, 2025 11:17

Copilot started reviewing on behalf of SamMorrowDrums December 13, 2025 11:18 View session

Copilot AI reviewed Dec 13, 2025

View reviewed changes

SamMorrowDrums force-pushed the SamMorrowDrums/server-tool-refactor branch from 790b435 to 187a8b6 Compare December 13, 2025 11:38

SamMorrowDrums force-pushed the SamMorrowDrums/server-tool-refactor branch from b29546e to 43acefe Compare December 13, 2025 14:19

SamMorrowDrums mentioned this pull request Dec 13, 2025

Refactor actions.go to use NewTool pattern #1596

Open

Copilot AI reviewed Dec 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor: Separate ServerTool with HandlerFunc pattern and ToolDependencies #1589

refactor: Separate ServerTool with HandlerFunc pattern and ToolDependencies #1589

SamMorrowDrums commented Dec 13, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Dec 13, 2025

Uh oh!

Uh oh!

Copilot AI Dec 13, 2025

Uh oh!

SamMorrowDrums commented Dec 13, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

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

		// RegisterFunc registers the tool with the server using the provided dependencies.
		func (st ServerTool) RegisterFunc(s mcp.Server, deps ToolDependencies) {

refactor: Separate ServerTool with HandlerFunc pattern and ToolDependencies #1589

Are you sure you want to change the base?

refactor: Separate ServerTool with HandlerFunc pattern and ToolDependencies #1589

Conversation

SamMorrowDrums commented Dec 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Architecture

Avoiding Circular Dependencies

Changes

New file: pkg/github/dependencies.go

Updated: pkg/toolsets/server_tool.go

Updated: pkg/toolsets/toolsets.go

Updated: internal/ghmcp/server.go

Benefits

Next Steps

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Dec 13, 2025

Choose a reason for hiding this comment

Uh oh!

SamMorrowDrums commented Dec 13, 2025

Migration Progress Update

Changes in this commit (b29546e)

Migration Pattern Demonstrated

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

SamMorrowDrums commented Dec 13, 2025 •

edited

Loading

New file: `pkg/github/dependencies.go`

Updated: `pkg/toolsets/server_tool.go`

Updated: `pkg/toolsets/toolsets.go`

Updated: `internal/ghmcp/server.go`

Changes in this commit (`b29546e`)