Migrate code-scanning toolset to modelcontextprotocol/go-sdk #1430
Conversation
Co-authored-by: omgitsads <4619+omgitsads@users.noreply.github.com>
|
|
Ignore lint errors due to unused parameters and types. These will naturally be fixed when all tools are re-enabled. |
There was a problem hiding this comment.
Pull Request Overview
This PR migrates the code-scanning toolset (GetCodeScanningAlert and ListCodeScanningAlerts tools) from mark3labs/mcp-go to modelcontextprotocol/go-sdk. This is part of a larger migration effort tracked in PR #1428.
Key Changes:
- Updated tool constructors to return
(mcp.Tool, mcp.ToolHandlerFor[map[string]any, any]) - Converted handler signatures to accept 3 parameters and return 3 values
- Replaced DSL-based schemas with explicit
jsonschema.Schemastructs - Changed parameter extraction to use
argsmap instead ofrequestobject - Re-enabled the
codeSecuritytoolset intools.go
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/github/code_scanning.go | Migrated tool implementations from mark3labs/mcp-go to modelcontextprotocol/go-sdk with new schemas and handlers |
| pkg/github/code_scanning_test.go | Updated tests to cast InputSchema to *jsonschema.Schema and use new handler signature |
| pkg/github/tools.go | Re-enabled codeSecurity toolset and added nolint directives to DefaultToolsetGroup and InitDynamicToolset |
| pkg/github/toolsnaps/*.snap | Cosmetic updates to field ordering in tool schema snapshots |
| internal/ghmcp/server.go | Removed unused stdioServerLogPrefix constant |
| } | ||
| } | ||
|
|
||
| //nolint:unused |
There was a problem hiding this comment.
The //nolint:unused directive is being added to DefaultToolsetGroup and InitDynamicToolset functions, but these functions appear to be actively used in the codebase. In internal/ghmcp/server.go, DefaultToolsetGroup is called on line 135 and InitDynamicToolset is called on line 154. These directives should only be added if the functions are truly unused, which doesn't appear to be the case here. Consider removing these directives unless they're being added in preparation for future deprecation.
| //nolint:unused |
|
|
||
| // InitDynamicToolset creates a dynamic toolset that can be used to enable other toolsets, and so requires the server and toolset group as arguments | ||
| // | ||
| //nolint:unused |
There was a problem hiding this comment.
The //nolint:unused directive is being added to InitDynamicToolset, but this function is actively used in internal/ghmcp/server.go on line 154. These directives should only be added if the function is truly unused. Consider removing this directive unless it's being added in preparation for future deprecation.
| //nolint:unused |
| client, err := getClient(ctx) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get GitHub client: %w", err) | ||
| return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, nil |
There was a problem hiding this comment.
Inconsistent error handling: When getClient(ctx) fails, the error should be returned as the third return value to match the pattern used in other migrated tools like GetMe in context_tools.go. The return statement should be:
return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, errInstead of:
return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, nil| return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, nil | |
| return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, err |
| resp, | ||
| err, | ||
| ), nil | ||
| ), nil, nil |
There was a problem hiding this comment.
Inconsistent error handling: When GitHub API calls fail, the error should be returned as the third return value to match the pattern used in other migrated tools. The return statement should be:
return ghErrors.NewGitHubAPIErrorResponse(ctx,
"failed to get alert",
resp,
err,
), nil, errInstead of returning nil as the third value.
| ), nil, nil | |
| ), nil, err |
| client, err := getClient(ctx) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get GitHub client: %w", err) | ||
| return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, nil |
There was a problem hiding this comment.
Inconsistent error handling: When getClient(ctx) fails, the error should be returned as the third return value to match the pattern used in other migrated tools. The return statement should be:
return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, errInstead of returning nil as the third value.
| return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, nil | |
| return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, err |
| resp, | ||
| err, | ||
| ), nil | ||
| ), nil, nil |
There was a problem hiding this comment.
Inconsistent error handling: When GitHub API calls fail, the error should be returned as the third return value to match the pattern used in other migrated tools. The return statement should be:
return ghErrors.NewGitHubAPIErrorResponse(ctx,
"failed to list alerts",
resp,
err,
), nil, errInstead of returning nil as the third value.
| ), nil, nil | |
| ), nil, err |
Closes: Part of #1428
Migrates
GetCodeScanningAlertandListCodeScanningAlertstools frommark3labs/mcp-gotomodelcontextprotocol/go-sdk.Changes
(mcp.Tool, mcp.ToolHandlerFor[map[string]any, any])func(context.Context, *mcp.CallToolRequest, map[string]any) (*mcp.CallToolResult, any, error)jsonschema.Schemastructsutils.NewToolResult*functionsInputSchemato*jsonschema.Schemaand pass args map to handlersSchema changes in toolsnaps are cosmetic (field ordering only) - all fields, types, enums, defaults, and descriptions remain logically equivalent.
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.