- Notifications
You must be signed in to change notification settings - Fork3.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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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>
…ngAgentPromptCo-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 theissues toolset frommark3labs/mcp-go tomodelcontextprotocol/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 castInputSchema to*jsonschema.Schema and added third nil return value to handler calls |
| pkg/github/search_utils.go | Migrated search helper function to acceptargs 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 | MovedparseISOTimestamp 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) | ||
| iferr!=nil { | ||
| returnnil,err | ||
| returnnil,nil,err |
CopilotAINov 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. Whenclient.Query() fails, the function returnsnil, 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:
iferr:=client.Query(ctx,&query,variables);err!=nil {returnutils.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.
| returnnil,nil,err | |
| pageInfo:="" | |
| ifvariables["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 { | ||
| returnnil,fmt.Errorf("failed to replace actors for assignable: %w",err) | ||
| returnnil,nil,fmt.Errorf("failed to replace actors for assignable: %w",err) |
CopilotAINov 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. Whenclient.Mutate() fails, the function returnsnil, 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:
iferr:= client.Mutate(...);err!=nil {returnutils.NewToolResultErrorFromErr("failed to assign copilot to issue",err),nil,nil}
| returnnil,nil,fmt.Errorf("failed toreplace actors for assignable: %w",err) | |
| returnutils.NewToolResultErrorFromErr("failed toassign copilot to issue",err),nil,nil |
| client,err:=getGQLClient(ctx) | ||
| iferr!=nil { | ||
| returnnil,fmt.Errorf("failed to get GitHub client: %w",err) | ||
| returnnil,nil,fmt.Errorf("failed to get GitHub client: %w",err) |
CopilotAINov 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 returnsnil, 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:
iferr!=nil {returnutils.NewToolResultErrorFromErr("failed to get GitHub client",err),nil,nil}
This same pattern appears at line 1571 and should be fixed there as well.
| returnnil,nil,fmt.Errorf("failed to get GitHub client: %w",err) | |
| returnutils.NewToolResultErrorFromErr("failed to get GitHub client",err),nil,nil |
| paginationParams,err:=pagination.ToGraphQLParams() | ||
| iferr!=nil { | ||
| returnnil,err | ||
| returnnil,nil,err |
CopilotAINov 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. WhenToGraphQLParams() fails, the function returnsnil, 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:
iferr!=nil {returnutils.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.
| returnnil,nil,err | |
| returnutils.NewToolResultErrorFromErr("failed to convert pagination parameters",err),nil,nil |
| out,err:=json.Marshal(response) | ||
| iferr!=nil { | ||
| returnnil,fmt.Errorf("failed to marshal issues: %w",err) | ||
| returnnil,nil,fmt.Errorf("failed to marshal issues: %w",err) |
CopilotAINov 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. Whenclient.Query() fails, the function returnsnil, 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:
iferr:=client.Query(ctx,issueQuery,vars);err!=nil {returnutils.NewToolResultErrorFromErr("failed to list issues",err),nil,nil}
Compare with line 1450 where query errors are correctly wrapped as tool result errors usingutils.NewToolResultError(err.Error()).
| returnnil,nil,fmt.Errorf("failed to marshal issues: %w",err) | |
| returnutils.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!
726c683 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 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 moreCopilot coding agent tips in the docs.