- Notifications
You must be signed in to change notification settings - Fork3.1k
Migrate code-scanning toolset to modelcontextprotocol/go-sdk#1430
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Migrate code-scanning toolset to modelcontextprotocol/go-sdk#1430
Conversation
Co-authored-by: omgitsads <4619+omgitsads@users.noreply.github.com>
LuluBeatson commentedNov 18, 2025
  |
LuluBeatson commentedNov 18, 2025
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.
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 thecode-scanning toolset (GetCodeScanningAlert andListCodeScanningAlerts tools) frommark3labs/mcp-go tomodelcontextprotocol/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 unusedstdioServerLogPrefix constant |
| } | ||
| } | ||
| //nolint:unused |
CopilotAINov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The//nolint:unused directive is being added toDefaultToolsetGroup andInitDynamicToolset functions, but these functions appear to be actively used in the codebase. Ininternal/ghmcp/server.go,DefaultToolsetGroup is called on line 135 andInitDynamicToolset 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 |
CopilotAINov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The//nolint:unused directive is being added toInitDynamicToolset, but this function is actively used ininternal/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) | ||
| iferr!=nil { | ||
| returnnil,fmt.Errorf("failed to get GitHub client: %w",err) | ||
| returnutils.NewToolResultErrorFromErr("failed to get GitHub client",err),nil,nil |
CopilotAINov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Inconsistent error handling: WhengetClient(ctx) fails, the error should be returned as the third return value to match the pattern used in other migrated tools likeGetMe incontext_tools.go. The return statement should be:
returnutils.NewToolResultErrorFromErr("failed to get GitHub client",err),nil,err
Instead of:
returnutils.NewToolResultErrorFromErr("failed to get GitHub client",err),nil,nil
| returnutils.NewToolResultErrorFromErr("failed to get GitHub client",err),nil,nil | |
| returnutils.NewToolResultErrorFromErr("failed to get GitHub client",err),nil,err |
| resp, | ||
| err, | ||
| ),nil | ||
| ),nil,nil |
CopilotAINov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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:
returnghErrors.NewGitHubAPIErrorResponse(ctx,"failed to get alert",resp,err,),nil,err
Instead of returningnil as the third value.
| ),nil,nil | |
| ),nil,err |
| client,err:=getClient(ctx) | ||
| iferr!=nil { | ||
| returnnil,fmt.Errorf("failed to get GitHub client: %w",err) | ||
| returnutils.NewToolResultErrorFromErr("failed to get GitHub client",err),nil,nil |
CopilotAINov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Inconsistent error handling: WhengetClient(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:
returnutils.NewToolResultErrorFromErr("failed to get GitHub client",err),nil,err
Instead of returningnil as the third value.
| returnutils.NewToolResultErrorFromErr("failed to get GitHub client",err),nil,nil | |
| returnutils.NewToolResultErrorFromErr("failed to get GitHub client",err),nil,err |
| resp, | ||
| err, | ||
| ),nil | ||
| ),nil,nil |
CopilotAINov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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:
returnghErrors.NewGitHubAPIErrorResponse(ctx,"failed to list alerts",resp,err,),nil,err
Instead of returningnil as the third value.
| ),nil,nil | |
| ),nil,err |
a405519 intoomgitsads/go-sdkUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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 our2 minute survey.