-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Migrate issues toolset to modelcontextprotocol/go-sdk #1440
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…functions) Co-authored-by: omgitsads <4619+omgitsads@users.noreply.github.com>
Co-authored-by: omgitsads <4619+omgitsads@users.noreply.github.com>
Co-authored-by: omgitsads <4619+omgitsads@users.noreply.github.com>
Co-authored-by: omgitsads <4619+omgitsads@users.noreply.github.com>
…ngAgentPrompt Co-authored-by: omgitsads <4619+omgitsads@users.noreply.github.com>
Co-authored-by: omgitsads <4619+omgitsads@users.noreply.github.com>
Co-authored-by: omgitsads <4619+omgitsads@users.noreply.github.com>
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 the issues toolset from mark3labs/mcp-go to modelcontextprotocol/go-sdk, completing part of the broader migration effort (#1428).
Key changes:
- Updated 8 tools with new schema definitions and handler signatures (3-value returns)
- Migrated 1 prompt (
AssignCodingAgentPrompt) to new struct format - Changed handler parameter extraction from
requestobject toargs map[string]any - Updated all error handling to use
utils.NewToolResult*()constructors - Regenerated tool snapshots reflecting schema structure changes
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/github/issues.go | Migrated 8 issue tools to new SDK with schema structs and 3-value handler returns |
| pkg/github/issues_test.go | Updated test assertions to cast InputSchema to *jsonschema.Schema and added third nil return value to handler calls |
| pkg/github/search_utils.go | Migrated search helper function to accept args map[string]any instead of request object |
| pkg/github/workflow_prompts.go | Converted 2 prompts from builder pattern to struct format with new message content wrapping |
| pkg/github/tools.go | Re-enabled issues toolset registration (uncommented lines) |
| pkg/github/minimal_types.go | Moved parseISOTimestamp function to issues.go (only used there) |
| pkg/github/toolsnaps/*.snap | Regenerated snapshots with new JSON schema structure (type/required/properties ordering) |
| err := client.Query(ctx, &query, variables) | ||
| if err != nil { | ||
| return nil, err | ||
| return nil, nil, err |
Copilot
AI
Nov 19, 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.
Incorrect error handling in GraphQL query. When client.Query() fails, the function returns nil, nil, err which will return a Go error to the handler caller. This is inconsistent with the SDK pattern where API errors should be returned as tool result errors.
This should be:
if err := client.Query(ctx, &query, variables); err != nil {
return utils.NewToolResultErrorFromErr("failed to query suggested actors", err), nil, nil
}This error is within the pagination loop, so the error context should clarify which page failed if relevant.
| return nil, nil, err | |
| pageInfo := "" | |
| if variables["endCursor"] != nil { | |
| pageInfo = fmt.Sprintf(" (endCursor: %v)", variables["endCursor"]) | |
| } | |
| return utils.NewToolResultErrorFromErr( | |
| fmt.Sprintf("failed to query suggested actors%s", pageInfo), | |
| err, | |
| ), nil, nil |
| nil, | ||
| ); err != nil { | ||
| return nil, fmt.Errorf("failed to replace actors for assignable: %w", err) | ||
| return nil, nil, fmt.Errorf("failed to replace actors for assignable: %w", err) |
Copilot
AI
Nov 19, 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.
Incorrect error handling in GraphQL mutation. When client.Mutate() fails, the function returns nil, nil, err which will return a Go error to the handler caller. This is inconsistent with the SDK pattern where API errors should be returned as tool result errors.
This should be:
if err := client.Mutate(...); err != nil {
return utils.NewToolResultErrorFromErr("failed to assign copilot to issue", err), nil, nil
}| return nil, nil, fmt.Errorf("failed to replace actors for assignable: %w", err) | |
| return utils.NewToolResultErrorFromErr("failed to assign copilot to issue", err), nil, nil |
| client, err := getGQLClient(ctx) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get GitHub client: %w", err) | ||
| return nil, nil, fmt.Errorf("failed to get GitHub client: %w", err) |
Copilot
AI
Nov 19, 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.
Incorrect error handling when getting GitHub client. The function returns nil, nil, fmt.Errorf(...) which will return a Go error to the handler caller. This is inconsistent with the SDK migration pattern where all errors should be returned as tool result errors.
This should be:
if err != nil {
return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, nil
}This same pattern appears at line 1571 and should be fixed there as well.
| return nil, nil, fmt.Errorf("failed to get GitHub client: %w", err) | |
| return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, nil |
| paginationParams, err := pagination.ToGraphQLParams() | ||
| if err != nil { | ||
| return nil, err | ||
| return nil, nil, err |
Copilot
AI
Nov 19, 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.
Incorrect error handling in pagination parameter conversion. When ToGraphQLParams() fails, the function returns nil, nil, err which will return a Go error to the handler caller. This is inconsistent with the SDK pattern where validation/client errors should be returned as tool result errors.
This should be:
if err != nil {
return utils.NewToolResultErrorFromErr("failed to convert pagination parameters", err), nil, nil
}The same pattern is used correctly throughout the rest of the function for other client errors.
| return nil, nil, err | |
| return utils.NewToolResultErrorFromErr("failed to convert pagination parameters", err), nil, nil |
| out, err := json.Marshal(response) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to marshal issues: %w", err) | ||
| return nil, nil, fmt.Errorf("failed to marshal issues: %w", err) |
Copilot
AI
Nov 19, 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.
Incorrect error handling in query execution. When client.Query() fails, the function returns nil, nil, err which will return a Go error to the handler caller. This is inconsistent with the SDK pattern where API errors should be returned as tool result errors.
This should be:
if err := client.Query(ctx, issueQuery, vars); err != nil {
return utils.NewToolResultErrorFromErr("failed to list issues", err), nil, nil
}Compare with line 1450 where query errors are correctly wrapped as tool result errors using utils.NewToolResultError(err.Error()).
| return nil, nil, fmt.Errorf("failed to marshal issues: %w", err) | |
| return utils.NewToolResultErrorFromErr("failed to marshal issues", err), nil, nil |
SamMorrowDrums
left a comment
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.
Still generally looking great. Didn't check the lint fail, but the tool and prompt refactoring looks correct.
| "description": "Repository owner", | ||
| "type": "string" | ||
| "type": "string", | ||
| "description": "Repository owner" |
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.
Are these toolsnap ordering changes deterministic? As long as this is a one-off churn it's all good.
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.
Will take a look and confirm!
Closes: Part of #1428
Migrates the
issuestoolset frommark3labs/mcp-gotomodelcontextprotocol/go-sdk.Changes
Tools migrated (8):
IssueRead- Read issue details, comments, sub-issues, or labelsListIssueTypes- List available issue types for an organizationAddIssueComment- Add comments to issuesSubIssueWrite- Add, remove, or reprioritize sub-issuesSearchIssues- Search issues with GitHub search syntaxIssueWrite- Create or update issuesListIssues- List and filter repository issuesAssignCopilotToIssue- Assign Copilot to issuesPrompts migrated (1):
AssignCodingAgentPrompt- Multi-issue Copilot assignment workflowKey schema changes:
mcp.NewTool()) → struct format withjsonschema.Schema(ctx, request) (*CallToolResult, error)→(ctx, *request, args) (*CallToolResult, any, error)requestobject →args map[string]anymcp.NewToolResult*()→utils.NewToolResult*()Argumentsnowmap[string]stringBefore/After:
Files modified:
pkg/github/issues.go- Tool implementationspkg/github/issues_test.go- Test assertions updated for new signaturespkg/github/search_utils.go- Search helper migratedpkg/github/__toolsnaps__/*.snap- Schema snapshots regeneratedOriginal prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.