Skip to content

Conversation

@SamMorrowDrums
Copy link
Collaborator

Summary

This PR continues the effort to eliminate NewServerToolLegacy wrappers by migrating security scanning tools to the new NewTool pattern with ToolDependencies.

Changes

Converts all functions in:

  • code_scanning.go (2 functions): GetCodeScanningAlert, ListCodeScanningAlerts
  • secret_scanning.go (2 functions): GetSecretScanningAlert, ListSecretScanningAlerts
  • dependabot.go (2 functions): GetDependabotAlert, ListDependabotAlerts

Updates corresponding test files to use the new handler pattern.

Stack

Pattern

Functions are converted from:

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

To:

func FuncName(t translations.TranslationHelperFunc) toolsets.ServerTool {
    return NewTool(mcp.Tool{...}, func(deps ToolDependencies) mcp.ToolHandlerFor[...] {...})
}

Co-authored-by: Adam Holt omgitsads@users.noreply.github.com

…t to NewTool pattern

Co-authored-by: Adam Holt <omgitsads@users.noreply.github.com>
@SamMorrowDrums SamMorrowDrums requested a review from a team as a code owner December 13, 2025 17:35
Copilot AI review requested due to automatic review settings December 13, 2025 17: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 migrates security scanning tools (code_scanning.go, secret_scanning.go, dependabot.go) from the legacy NewServerToolLegacy pattern to the new NewTool pattern with ToolDependencies. This continues the codebase-wide refactoring effort to eliminate legacy wrappers.

Key Changes

  • Function signatures changed from returning (mcp.Tool, mcp.ToolHandlerFor[...]) to returning toolsets.ServerTool
  • Functions no longer accept getClient GetClientFn as a parameter; instead, handlers access dependencies via deps.GetClient
  • Tool registration in tools.go simplified from toolsets.NewServerToolLegacy(GetXxx(getClient, t)) to GetXxx(t)

Reviewed changes

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

Show a summary per file
File Description
pkg/github/tools.go Removed NewServerToolLegacy wrappers for 6 security tools; now directly use returned ServerTool
pkg/github/code_scanning.go Converted 2 functions to NewTool pattern with ToolDependencies
pkg/github/code_scanning_test.go Updated tests to use toolDef.Handler(deps) pattern
pkg/github/secret_scanning.go Converted 2 functions to NewTool pattern with ToolDependencies
pkg/github/secret_scanning_test.go Updated tests to use toolDef.Handler(deps) pattern
pkg/github/dependabot.go Converted 2 functions to NewTool pattern with ToolDependencies
pkg/github/dependabot_test.go Updated tests to use toolDef.Handler(deps) pattern

The refactoring is mechanically sound and maintains the existing behavior of all tools. All files are internally consistent in their error handling patterns, though there are stylistic differences between files that pre-date this PR.

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.

2 participants