Skip to content

Conversation

@LuluBeatson
Copy link
Contributor

@LuluBeatson LuluBeatson commented Nov 21, 2025

Closes: Part of #1428

Migrates the 4 tools found in search.go:

  • Part of repos toolset:
    • search_repositories
    • search_code
  • users toolset:
    • search_users
  • orgs toolset:
    • search_orgs

@LuluBeatson LuluBeatson requested a review from a team as a code owner November 21, 2025 16:52
Copilot AI review requested due to automatic review settings November 21, 2025 16:52
@LuluBeatson LuluBeatson marked this pull request as draft November 21, 2025 16:53
Copilot finished reviewing on behalf of LuluBeatson November 21, 2025 16: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 migrates 4 search tools (search_repositories, search_code, search_users, search_orgs) from the legacy MCP framework to the new Go SDK, enabling them in the default toolset configuration.

Key changes:

  • Migrated from mark3labs/mcp-go to modelcontextprotocol/go-sdk/mcp
  • Updated function signatures to return (mcp.Tool, mcp.ToolHandlerFor[map[string]any, any])
  • Replaced builder-pattern tool definitions with direct struct initialization using jsonschema.Schema

Reviewed changes

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

File Description
pkg/github/search.go Migrated 4 search tool implementations to Go SDK pattern, updated imports, changed error handling to use utils package, replaced status code literals with http.StatusOK constant
pkg/github/search_test.go Updated tests to work with new SDK - removed build ignore directive, added jsonschema import, updated schema assertions to cast InputSchema to *jsonschema.Schema, updated handler calls to match new 3-return signature
pkg/github/tools.go Re-enabled search tools by uncommenting SearchRepositories, SearchCode, SearchUsers, and SearchOrgs registrations, and adding users and orgs toolsets to the toolset group
pkg/github/toolsnaps/search_*.snap Updated tool schema snapshots to reflect new JSON schema serialization format with reordered fields (type/required moved, properties field ordering changed)

@LuluBeatson
Copy link
Contributor Author

image image

@LuluBeatson LuluBeatson marked this pull request as ready for review November 21, 2025 17:03
@stasyu2009-ux
Copy link

I've already fixed the error there, I just need to remove the function that's not available to me, I don't have it, so my work is done.

Copy link

@harshul-narvar harshul-narvar left a comment

Choose a reason for hiding this comment

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

Overall Review

This PR successfully migrates 4 search tools from the old MCP SDK (mark3labs/mcp-go) to the new Go SDK (modelcontextprotocol/go-sdk). The migration follows the expected patterns and the code changes look correct. Here are my observations:

✅ Positive Aspects

  1. Correct SDK Migration: The migration from builder pattern to JSON Schema-based tool definitions is properly implemented.
  2. Handler Signature Updates: All handler functions correctly use the new signature func(ctx context.Context, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error).
  3. Error Handling: Consistent use of utils.NewToolResultError and utils.NewToolResultErrorFromErr throughout.
  4. Status Code Check: Good change from magic number 200 to http.StatusOK.
  5. Test Updates: Tests are properly updated to match the new handler signature and schema structure.
  6. Tool Registration: Tools are correctly re-enabled in tools.go.

⚠️ Potential Issues

  1. WithPagination Function: The PR calls WithPagination(schema) with a schema parameter, but I don't see this function signature in the current codebase. The existing WithPagination() returns mcp.ToolOption for the old SDK. Please verify that the base branch (omgitsads/go-sdk) has a WithPagination(*jsonschema.Schema) function that adds pagination properties to the schema.

  2. OptionalPaginationParams Function: The PR calls OptionalPaginationParams(args) where args is map[string]any, but the current implementation takes mcp.CallToolRequest. Please confirm that the base branch has an overloaded version that accepts map[string]any or that the helper functions (RequiredParam, OptionalParam, etc.) have been updated to work with map[string]any.

  3. Helper Functions: The PR uses RequiredParam[string](args, "query") and similar functions with args (which is map[string]any). These functions currently expect mcp.CallToolRequest. Please verify these helper functions have been updated in the base branch to accept map[string]any.

📝 Minor Suggestions

  1. Consistency: The PR consistently uses utils.NewToolResultError and utils.NewToolResultErrorFromErr, which is good. Make sure the utils package exists and these functions are available.

  2. Test Coverage: The tests look comprehensive and properly updated. The addition of toolsnaps.Test for Test_SearchOrgs is a good catch.

  3. Code Organization: The migration is clean and well-organized. The removal of //go:build ignore is correct.

✅ Ready to Merge (with caveats)

Assuming the base branch has:

  • WithPagination(*jsonschema.Schema) function
  • Updated helper functions that accept map[string]any
  • The utils package with the result helper functions

This PR looks ready to merge. The migration is thorough and follows the expected patterns for the Go SDK migration.


Recommendation: Please verify that all the helper functions and WithPagination exist in the base branch with the expected signatures before merging.

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.

5 participants