Skip to content

Commit ad8da50

Browse files
SamMorrowDrumsCopilot
SamMorrowDrums
and
Copilot
committed
fix: address review feedback on granular toolsets
- Fix REST response handling: capture resp, close body, use ghErrors helpers in issueUpdateTool, prUpdateTool, GranularCreateIssue, and GranularRequestPullRequestReviewers - Add FeatureFlagDisable on consolidated tools (IssueWrite, SubIssueWrite, UpdatePullRequest, PullRequestReviewWrite, AddCommentToPendingReview) so they are hidden when granular variants are active - Use OptionalStringArrayParam for assignees, labels, reviewers instead of manual loop that silently dropped non-string elements - Fix side/startSide empty string leak: pass nil pointer when absent instead of pointer to empty string in GraphQL mutations - Fix milestone minimum from 0 to 1 to match RequiredInt rejection of 0 - Return MinimalResponse {id, url} instead of full JSON objects - Fix RequiredParam[bool] rejecting draft=false by using presence check - Add handler tests for update_pull_request_draft_state (draft + ready) and add_pull_request_review_comment with full GraphQL mocking Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent ee1fb9f commit ad8da50

6 files changed

Lines changed: 255 additions & 46 deletions

File tree

‎pkg/github/__toolsnaps__/update_issue_milestone.snap‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@
1414
},
1515
"milestone": {
1616
"description": "The milestone number to set on the issue",
17-
"minimum": 0,
17+
"minimum": 1,
1818
"type": "integer"
1919
},
2020
"owner": {

‎pkg/github/granular_tools_test.go‎

Lines changed: 178 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -522,3 +522,181 @@ func TestGranularCreatePullRequestReview(t *testing.T) {
522522
require.NoError(t, err)
523523
assert.False(t, result.IsError)
524524
}
525+
526+
func TestGranularUpdatePullRequestDraftState(t *testing.T) {
527+
tests := []struct {
528+
name string
529+
draft bool
530+
}{
531+
{name: "convert to draft", draft: true},
532+
{name: "mark ready for review", draft: false},
533+
}
534+
535+
for _, tc := range tests {
536+
t.Run(tc.name, func(t *testing.T) {
537+
var matchers []githubv4mock.Matcher
538+
539+
matchers = append(matchers, githubv4mock.NewQueryMatcher(
540+
struct {
541+
Repository struct {
542+
PullRequest struct {
543+
ID githubv4.ID
544+
} `graphql:"pullRequest(number: $number)"`
545+
} `graphql:"repository(owner: $owner, name: $name)"`
546+
}{},
547+
map[string]any{
548+
"owner": githubv4.String("owner"),
549+
"name": githubv4.String("repo"),
550+
"number": githubv4.Int(1),
551+
},
552+
githubv4mock.DataResponse(map[string]any{
553+
"repository": map[string]any{
554+
"pullRequest": map[string]any{"id": "PR_123"},
555+
},
556+
}),
557+
))
558+
559+
if tc.draft {
560+
matchers = append(matchers, githubv4mock.NewMutationMatcher(
561+
struct {
562+
ConvertPullRequestToDraft struct {
563+
PullRequest struct {
564+
ID githubv4.ID
565+
IsDraft githubv4.Boolean
566+
}
567+
} `graphql:"convertPullRequestToDraft(input: $input)"`
568+
}{},
569+
githubv4.ConvertPullRequestToDraftInput{PullRequestID: githubv4.ID("PR_123")},
570+
nil,
571+
githubv4mock.DataResponse(map[string]any{
572+
"convertPullRequestToDraft": map[string]any{
573+
"pullRequest": map[string]any{"id": "PR_123", "isDraft": true},
574+
},
575+
}),
576+
))
577+
} else {
578+
matchers = append(matchers, githubv4mock.NewMutationMatcher(
579+
struct {
580+
MarkPullRequestReadyForReview struct {
581+
PullRequest struct {
582+
ID githubv4.ID
583+
IsDraft githubv4.Boolean
584+
}
585+
} `graphql:"markPullRequestReadyForReview(input: $input)"`
586+
}{},
587+
githubv4.MarkPullRequestReadyForReviewInput{PullRequestID: githubv4.ID("PR_123")},
588+
nil,
589+
githubv4mock.DataResponse(map[string]any{
590+
"markPullRequestReadyForReview": map[string]any{
591+
"pullRequest": map[string]any{"id": "PR_123", "isDraft": false},
592+
},
593+
}),
594+
))
595+
}
596+
597+
gqlClient := githubv4.NewClient(githubv4mock.NewMockedHTTPClient(matchers...))
598+
deps := BaseDeps{GQLClient: gqlClient}
599+
serverTool := GranularUpdatePullRequestDraftState(translations.NullTranslationHelper)
600+
handler := serverTool.Handler(deps)
601+
602+
request := createMCPRequest(map[string]any{
603+
"owner": "owner",
604+
"repo": "repo",
605+
"pullNumber": float64(1),
606+
"draft": tc.draft,
607+
})
608+
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
609+
require.NoError(t, err)
610+
assert.False(t, result.IsError)
611+
})
612+
}
613+
}
614+
615+
func TestGranularAddPullRequestReviewComment(t *testing.T) {
616+
mockedClient := githubv4mock.NewMockedHTTPClient(
617+
githubv4mock.NewQueryMatcher(
618+
struct {
619+
Viewer struct {
620+
Login githubv4.String
621+
}
622+
}{},
623+
nil,
624+
githubv4mock.DataResponse(map[string]any{
625+
"viewer": map[string]any{"login": "testuser"},
626+
}),
627+
),
628+
githubv4mock.NewQueryMatcher(
629+
struct {
630+
Repository struct {
631+
PullRequest struct {
632+
Reviews struct {
633+
Nodes []struct {
634+
ID githubv4.ID
635+
State githubv4.PullRequestReviewState
636+
URL githubv4.URI
637+
}
638+
} `graphql:"reviews(first: 1, author: $author)"`
639+
} `graphql:"pullRequest(number: $prNum)"`
640+
} `graphql:"repository(owner: $owner, name: $name)"`
641+
}{},
642+
map[string]any{
643+
"author": githubv4.String("testuser"),
644+
"owner": githubv4.String("owner"),
645+
"name": githubv4.String("repo"),
646+
"prNum": githubv4.Int(1),
647+
},
648+
githubv4mock.DataResponse(map[string]any{
649+
"repository": map[string]any{
650+
"pullRequest": map[string]any{
651+
"reviews": map[string]any{
652+
"nodes": []map[string]any{
653+
{"id": "PRR_123", "state": "PENDING", "url": "https://github.com/owner/repo/pull/1#pullrequestreview-123"},
654+
},
655+
},
656+
},
657+
},
658+
}),
659+
),
660+
githubv4mock.NewMutationMatcher(
661+
struct {
662+
AddPullRequestReviewThread struct {
663+
Thread struct {
664+
ID githubv4.ID
665+
}
666+
} `graphql:"addPullRequestReviewThread(input: $input)"`
667+
}{},
668+
githubv4.AddPullRequestReviewThreadInput{
669+
Path: githubv4.String("src/main.go"),
670+
Body: githubv4.String("This needs a fix"),
671+
SubjectType: githubv4mock.Ptr(githubv4.PullRequestReviewThreadSubjectTypeLine),
672+
Line: githubv4mock.Ptr(githubv4.Int(42)),
673+
Side: githubv4mock.Ptr(githubv4.DiffSideRight),
674+
PullRequestReviewID: githubv4mock.Ptr(githubv4.ID("PRR_123")),
675+
},
676+
nil,
677+
githubv4mock.DataResponse(map[string]any{
678+
"addPullRequestReviewThread": map[string]any{
679+
"thread": map[string]any{"id": "PRRT_456"},
680+
},
681+
}),
682+
),
683+
)
684+
gqlClient := githubv4.NewClient(mockedClient)
685+
deps := BaseDeps{GQLClient: gqlClient}
686+
serverTool := GranularAddPullRequestReviewComment(translations.NullTranslationHelper)
687+
handler := serverTool.Handler(deps)
688+
689+
request := createMCPRequest(map[string]any{
690+
"owner": "owner",
691+
"repo": "repo",
692+
"pullNumber": float64(1),
693+
"path": "src/main.go",
694+
"body": "This needs a fix",
695+
"subjectType": "LINE",
696+
"line": float64(42),
697+
"side": "RIGHT",
698+
})
699+
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
700+
require.NoError(t, err)
701+
assert.False(t, result.IsError)
702+
}

‎pkg/github/issues.go‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -677,7 +677,7 @@ func AddIssueComment(t translations.TranslationHelperFunc) inventory.ServerTool
677677

678678
// SubIssueWrite creates a tool to add a sub-issue to a parent issue.
679679
func SubIssueWrite(t translations.TranslationHelperFunc) inventory.ServerTool {
680-
return NewTool(
680+
st := NewTool(
681681
ToolsetMetadataIssues,
682682
mcp.Tool{
683683
Name: "sub_issue_write",
@@ -787,6 +787,8 @@ Options are:
787787
return utils.NewToolResultError(fmt.Sprintf("unknown method: %s", method)), nil, nil
788788
}
789789
})
790+
st.FeatureFlagDisable = FeatureFlagIssuesGranular
791+
return st
790792
}
791793

792794
func AddSubIssue(ctx context.Context, client *github.Client, owner string, repo string, issueNumber int, subIssueID int, replaceParent bool) (*mcp.CallToolResult, error) {
@@ -970,7 +972,7 @@ func SearchIssues(t translations.TranslationHelperFunc) inventory.ServerTool {
970972
const IssueWriteUIResourceURI = "ui://github-mcp-server/issue-write"
971973

972974
func IssueWrite(t translations.TranslationHelperFunc) inventory.ServerTool {
973-
return NewTool(
975+
st := NewTool(
974976
ToolsetMetadataIssues,
975977
mcp.Tool{
976978
Name: "issue_write",
@@ -1179,6 +1181,8 @@ Options are:
11791181
return utils.NewToolResultError("invalid method, must be either 'create' or 'update'"), nil, nil
11801182
}
11811183
})
1184+
st.FeatureFlagDisable = FeatureFlagIssuesGranular
1185+
return st
11821186
}
11831187

11841188
func CreateIssue(ctx context.Context, client *github.Client, owner string, repo string, title string, body string, assignees []string, labels []string, milestoneNum int, issueType string) (*mcp.CallToolResult, error) {

‎pkg/github/issues_granular.go‎

Lines changed: 26 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"fmt"
77
"maps"
88

9+
ghErrors "github.com/github/github-mcp-server/pkg/errors"
910
"github.com/github/github-mcp-server/pkg/inventory"
1011
"github.com/github/github-mcp-server/pkg/scopes"
1112
"github.com/github/github-mcp-server/pkg/translations"
@@ -84,12 +85,16 @@ func issueUpdateTool(
8485
return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, nil
8586
}
8687

87-
issue, _, err := client.Issues.Edit(ctx, owner, repo, issueNumber, issueReq)
88+
issue, resp, err := client.Issues.Edit(ctx, owner, repo, issueNumber, issueReq)
8889
if err != nil {
89-
return utils.NewToolResultErrorFromErr("failed to update issue", err), nil, nil
90+
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to update issue", resp, err), nil, nil
9091
}
92+
defer func() { _ = resp.Body.Close() }()
9193

92-
r, err := json.Marshal(issue)
94+
r, err := json.Marshal(MinimalResponse{
95+
ID: fmt.Sprintf("%d", issue.GetID()),
96+
URL: issue.GetHTMLURL(),
97+
})
9398
if err != nil {
9499
return utils.NewToolResultErrorFromErr("failed to marshal response", err), nil, nil
95100
}
@@ -164,12 +169,16 @@ func GranularCreateIssue(t translations.TranslationHelperFunc) inventory.ServerT
164169
return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, nil
165170
}
166171

167-
issue, _, err := client.Issues.Create(ctx, owner, repo, issueReq)
172+
issue, resp, err := client.Issues.Create(ctx, owner, repo, issueReq)
168173
if err != nil {
169-
return utils.NewToolResultErrorFromErr("failed to create issue", err), nil, nil
174+
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to create issue", resp, err), nil, nil
170175
}
176+
defer func() { _ = resp.Body.Close() }()
171177

172-
r, err := json.Marshal(issue)
178+
r, err := json.Marshal(MinimalResponse{
179+
ID: fmt.Sprintf("%d", issue.GetID()),
180+
URL: issue.GetHTMLURL(),
181+
})
173182
if err != nil {
174183
return utils.NewToolResultErrorFromErr("failed to marshal response", err), nil, nil
175184
}
@@ -235,15 +244,12 @@ func GranularUpdateIssueAssignees(t translations.TranslationHelperFunc) inventor
235244
},
236245
[]string{"assignees"},
237246
func(args map[string]any) (*github.IssueRequest, error) {
238-
raw, _ := OptionalParam[[]any](args, "assignees")
239-
if len(raw) == 0 {
240-
return nil, fmt.Errorf("missing required parameter: assignees")
247+
assignees, err := OptionalStringArrayParam(args, "assignees")
248+
if err != nil {
249+
return nil, err
241250
}
242-
assignees := make([]string, 0, len(raw))
243-
for _, v := range raw {
244-
if s, ok := v.(string); ok {
245-
assignees = append(assignees, s)
246-
}
251+
if len(assignees) == 0 {
252+
return nil, fmt.Errorf("missing required parameter: assignees")
247253
}
248254
return &github.IssueRequest{Assignees: &assignees}, nil
249255
},
@@ -265,15 +271,12 @@ func GranularUpdateIssueLabels(t translations.TranslationHelperFunc) inventory.S
265271
},
266272
[]string{"labels"},
267273
func(args map[string]any) (*github.IssueRequest, error) {
268-
raw, _ := OptionalParam[[]any](args, "labels")
269-
if len(raw) == 0 {
270-
return nil, fmt.Errorf("missing required parameter: labels")
274+
labels, err := OptionalStringArrayParam(args, "labels")
275+
if err != nil {
276+
return nil, err
271277
}
272-
labels := make([]string, 0, len(raw))
273-
for _, v := range raw {
274-
if s, ok := v.(string); ok {
275-
labels = append(labels, s)
276-
}
278+
if len(labels) == 0 {
279+
return nil, fmt.Errorf("missing required parameter: labels")
277280
}
278281
return &github.IssueRequest{Labels: &labels}, nil
279282
},
@@ -290,7 +293,7 @@ func GranularUpdateIssueMilestone(t translations.TranslationHelperFunc) inventor
290293
"milestone": {
291294
Type: "integer",
292295
Description: "The milestone number to set on the issue",
293-
Minimum: jsonschema.Ptr(0.0),
296+
Minimum: jsonschema.Ptr(1.0),
294297
},
295298
},
296299
[]string{"milestone"},

‎pkg/github/pullrequests.go‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -751,7 +751,7 @@ func UpdatePullRequest(t translations.TranslationHelperFunc) inventory.ServerToo
751751
Required: []string{"owner", "repo", "pullNumber"},
752752
}
753753

754-
return NewTool(
754+
st := NewTool(
755755
ToolsetMetadataPullRequests,
756756
mcp.Tool{
757757
Name: "update_pull_request",
@@ -990,6 +990,8 @@ func UpdatePullRequest(t translations.TranslationHelperFunc) inventory.ServerToo
990990

991991
return utils.NewToolResultText(string(r)), nil, nil
992992
})
993+
st.FeatureFlagDisable = FeatureFlagPullRequestsGranular
994+
return st
993995
}
994996

995997
// AddReplyToPullRequestComment creates a tool to add a reply to an existing pull request comment.
@@ -1555,7 +1557,7 @@ func PullRequestReviewWrite(t translations.TranslationHelperFunc) inventory.Serv
15551557
Required: []string{"method", "owner", "repo", "pullNumber"},
15561558
}
15571559

1558-
return NewTool(
1560+
st := NewTool(
15591561
ToolsetMetadataPullRequests,
15601562
mcp.Tool{
15611563
Name: "pull_request_review_write",
@@ -1607,6 +1609,8 @@ Available methods:
16071609
return utils.NewToolResultError(fmt.Sprintf("unknown method: %s", params.Method)), nil, nil
16081610
}
16091611
})
1612+
st.FeatureFlagDisable = FeatureFlagPullRequestsGranular
1613+
return st
16101614
}
16111615

16121616
func CreatePullRequestReview(ctx context.Context, client *githubv4.Client, params PullRequestReviewWriteParams) (*mcp.CallToolResult, error) {
@@ -1947,7 +1951,7 @@ func AddCommentToPendingReview(t translations.TranslationHelperFunc) inventory.S
19471951
Required: []string{"owner", "repo", "pullNumber", "path", "body", "subjectType"},
19481952
}
19491953

1950-
return NewTool(
1954+
st := NewTool(
19511955
ToolsetMetadataPullRequests,
19521956
mcp.Tool{
19531957
Name: "add_comment_to_pending_review",
@@ -2074,6 +2078,8 @@ func AddCommentToPendingReview(t translations.TranslationHelperFunc) inventory.S
20742078
// API implementation details to the LLM.
20752079
return utils.NewToolResultText("pull request review comment successfully added to pending review"), nil, nil
20762080
})
2081+
st.FeatureFlagDisable = FeatureFlagPullRequestsGranular
2082+
return st
20772083
}
20782084

20792085
// newGQLString like takes something that approximates a string (of which there are many types in shurcooL/githubv4)

0 commit comments

Comments
 (0)