refactor(pullrequests): convert PR tools to NewTool pattern #1595

SamMorrowDrums · 2025-12-13T14:13:12Z

Pull Request

Summary

Part of the stacked PR series for tool refactoring. This PR converts all 10 pull request tool functions in pullrequests.go to use the NewTool pattern with ToolDependencies injection.

Converted Functions

PullRequestRead - Read PR details, diff, files, status, comments, reviews
CreatePullRequest - Create new pull requests
UpdatePullRequest - Update existing PRs (title, body, state, draft, reviewers)
ListPullRequests - List PRs in a repository
MergePullRequest - Merge pull requests
SearchPullRequests - Search for PRs
UpdatePullRequestBranch - Update PR branch with base branch changes
PullRequestReviewWrite - Create, submit, and delete PR reviews
AddCommentToPendingReview - Add comments to pending reviews
RequestCopilotReview - Request Copilot code review

Changes

pullrequests.go: All 10 functions now:
- Take only t translations.TranslationHelperFunc parameter
- Return toolsets.ServerTool
- Use ToolDependencies for runtime dependencies (GetClient, GetGQLClient, RepoAccessCache, Flags)
tools.go: Updated to use direct function calls for PR functions (removed NewServerToolLegacy wrappers)
pullrequests_test.go: Updated all tests to use new handler pattern:
- serverTool := FunctionName(translations.NullTranslationHelper)
- deps := ToolDependencies{...}
- handler := serverTool.Handler(deps)
- result, err := handler(ctx, &request) (2 return values)

PR Stack

This is part of a stacked PR series:

✅ refactor: Separate ServerTool with HandlerFunc pattern and ToolDependencies #1589 - Foundation + search.go
✅ Migrate context_tools to new ServerTool pattern #1590 - context_tools.go
✅ refactor(gists): migrate gists.go to NewTool pattern #1591 - gists.go
✅ refactor(notifications): migrate notifications.go to NewTool pattern #1592 - notifications.go
✅ Refactor repositories.go tools to use NewTool pattern with ToolDependencies #1593 - repositories.go
✅ refactor(issues): migrate issues.go to NewTool pattern #1594 - issues.go
🔄 This PR - pullrequests.go

Testing

All existing tests updated and passing
script/lint passes
script/test passes

Convert all 8 tool functions in issues.go to use the new NewTool helper pattern which standardizes dependency injection: - IssueRead: GetClient, GetGQLClient, RepoAccessCache, Flags - ListIssueTypes: GetClient - AddIssueComment: GetClient - SubIssueWrite: GetClient - SearchIssues: GetClient - IssueWrite: GetClient, GetGQLClient - ListIssues: GetGQLClient - AssignCopilotToIssue: GetGQLClient Updated tools.go to use direct function calls instead of NewServerToolLegacy wrappers. Updated all tests in issues_test.go to use the new ToolDependencies pattern and Handler() method. Co-authored-by: Adam Holt <oholt@github.com>

Copilot

Pull request overview

This PR refactors 10 pull request tool functions in pullrequests.go to use the NewTool pattern with ToolDependencies injection, continuing the stacked PR series for tool standardization. The refactoring moves from passing dependencies as function parameters to injecting them at runtime through the ToolDependencies struct.

Key Changes:

All 10 PR tool functions now accept only a translation helper parameter and return toolsets.ServerTool
Runtime dependencies (GetClient, GetGQLClient, RepoAccessCache, Flags) are accessed via the ToolDependencies struct passed to handlers
Test patterns updated to use the new 2-return-value handler signature

Reviewed changes

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

File	Description
pkg/github/tools.go	Removed `NewServerToolLegacy` wrappers for all 10 PR functions, now calling them directly
pkg/github/pullrequests.go	Refactored all 10 PR tool functions to use `NewTool` pattern with `ToolDependencies` injection instead of direct parameter passing
pkg/github/pullrequests_test.go	Updated all test cases to use new handler pattern: create serverTool, build deps struct, get handler, and call with 2 return values

Review Result: The refactoring is clean, consistent, and follows the established pattern from previous PRs in the stack. All changes maintain existing functionality while improving the architecture. No issues found.

Convert all 10 pull request tool functions to use the NewTool pattern with ToolDependencies injection: - PullRequestRead - CreatePullRequest - UpdatePullRequest - ListPullRequests - MergePullRequest - SearchPullRequests - UpdatePullRequestBranch - PullRequestReviewWrite - AddCommentToPendingReview - RequestCopilotReview Update tools.go to use direct function calls (removing NewServerToolLegacy wrappers) for PR functions. Update all tests in pullrequests_test.go to use the new handler pattern with deps and 2-value return. Co-authored-by: Adam Holt <omgitsads@users.noreply.github.com>

t3xash0rnyt0ad777-commits

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

t3xash0rnyt0ad777-commits

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

Convert all 14 tool functions in actions.go to use the NewTool pattern with ToolDependencies for dependency injection. This is part of a broader effort to standardize the tool implementation pattern across the codebase. Changes: - ListWorkflows, ListWorkflowRuns, RunWorkflow, GetWorkflowRun - GetWorkflowRunLogs, ListWorkflowJobs, GetJobLogs - RerunWorkflowRun, RerunFailedJobs, CancelWorkflowRun - ListWorkflowRunArtifacts, DownloadWorkflowRunArtifact - DeleteWorkflowRunLogs, GetWorkflowRunUsage The new pattern: - Takes only translations.TranslationHelperFunc as parameter - Returns toolsets.ServerTool with Tool and Handler - Handler receives ToolDependencies for client access - Enables better testability and consistent interface Co-authored-by: Adam Holt <omgitsads@users.noreply.github.com>

…t to NewTool pattern Co-authored-by: Adam Holt <omgitsads@users.noreply.github.com>

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

Convert 4 functions from NewServerToolLegacy wrapper to NewTool: - ListGlobalSecurityAdvisories - GetGlobalSecurityAdvisory - ListRepositorySecurityAdvisories - ListOrgRepositorySecurityAdvisories Update tools.go toolset registration and tests. Co-authored-by: Adam Holt <omgitsads@users.noreply.github.com>

This PR converts projects.go, labels.go, and dynamic_tools.go from the legacy NewServerToolLegacy wrapper pattern to the new NewTool pattern with proper ToolDependencies. Changes: - projects.go: Convert all 9 project functions to use NewTool with ToolHandlerFor[map[string]any, any] and 3-return-value handlers - projects_test.go: Update tests to use new serverTool.Handler(deps) pattern - labels.go: Convert GetLabel, ListLabels, and LabelWrite to NewTool pattern - labels_test.go: Update tests to use new pattern - dynamic_tools.go: Refactor functions to return ServerTool directly (using NewServerToolLegacy internally since they have special dependencies) - tools.go: Remove NewServerToolLegacy wrappers for dynamic tools registration Co-authored-by: Adam Holt <omgitsads@users.noreply.github.com>

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

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.

- 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

- 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.

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

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>

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

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

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

Copilot AI reviewed Dec 13, 2025

View reviewed changes

SamMorrowDrums force-pushed the SamMorrowDrums/server-tool-refactor-pullrequests branch from 09e7bbd to aa63e07 Compare December 13, 2025 14:16

SamMorrowDrums force-pushed the SamMorrowDrums/server-tool-refactor-issues branch from 53ea837 to 1168315 Compare December 13, 2025 14:17

t3xash0rnyt0ad777-commits reviewed Dec 13, 2025

View reviewed changes

This was referenced Dec 13, 2025

Refactor actions.go to use NewTool pattern #1596

Merged

refactor(git): migrate GetRepositoryTree to NewTool pattern #1597

Merged

SamMorrowDrums and others added 18 commits December 16, 2025 20:03

refactor(git): migrate GetRepositoryTree to NewTool pattern

8fe55ca

refactor(security): migrate code_scanning, secret_scanning, dependabo…

3cd6411

…t to NewTool pattern Co-authored-by: Adam Holt <omgitsads@users.noreply.github.com>

refactor(discussions): migrate to NewTool pattern

894f012

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

refactor: simplify ForMCPRequest switch cases

8786d8e

refactor(generate_docs): hoist success logging to generateAllDocs

70c0305

fix: remove unnecessary type arguments in helper_test.go

10b378f

SamMorrowDrums and others added 15 commits December 16, 2025 20:03

Add conformance-report to .gitignore

3ad56b9

Refactor GenerateToolsetsHelp() to use strings.Builder pattern

6e597cc

Co-authored-by: SamMorrowDrums <4811358+SamMorrowDrums@users.noreply.github.com>

SamMorrowDrums merged commit fb3faa6 into SamMorrowDrums/server-tool-refactor-issues Dec 16, 2025
7 checks passed

SamMorrowDrums deleted the SamMorrowDrums/server-tool-refactor-pullrequests branch December 16, 2025 19:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor(pullrequests): convert PR tools to NewTool pattern #1595

refactor(pullrequests): convert PR tools to NewTool pattern #1595

SamMorrowDrums commented Dec 13, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

t3xash0rnyt0ad777-commits left a comment

Uh oh!

t3xash0rnyt0ad777-commits left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

refactor(pullrequests): convert PR tools to NewTool pattern #1595

refactor(pullrequests): convert PR tools to NewTool pattern #1595

Conversation

SamMorrowDrums commented Dec 13, 2025

Pull Request

Summary

Converted Functions

Changes

PR Stack

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

t3xash0rnyt0ad777-commits left a comment

Choose a reason for hiding this comment

Uh oh!

t3xash0rnyt0ad777-commits left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants