From 1348c479bc672b24451dab0617a93d2c5ed6135b Mon Sep 17 00:00:00 2001 From: Kelsey Myers <52179263+kelsey-myers@users.noreply.github.com> Date: Wed, 29 Jul 2026 12:01:20 +0100 Subject: [PATCH 1/6] Bump go-github to pick up SearchType support (#2972) * Bump go-github for search_type support * chore: regenerate license files Auto-generated by license-check workflow --------- Co-authored-by: github-actions[bot] --- go.mod | 2 +- go.sum | 4 ++-- pkg/github/issues.go | 16 +++++++-------- pkg/github/issues_delete_test.go | 2 +- pkg/github/issues_granular.go | 34 ++++++++++++++++---------------- pkg/github/issues_test.go | 30 +++++++++++++--------------- pkg/github/pullrequests.go | 8 ++++---- third-party-licenses.darwin.md | 2 +- third-party-licenses.linux.md | 2 +- third-party-licenses.windows.md | 2 +- 10 files changed, 50 insertions(+), 52 deletions(-) diff --git a/go.mod b/go.mod index 1d8801f5a0..6dba229115 100644 --- a/go.mod +++ b/go.mod @@ -5,7 +5,7 @@ go 1.25.12 require ( github.com/go-chi/chi/v5 v5.3.1 github.com/go-viper/mapstructure/v2 v2.5.0 - github.com/google/go-github/v89 v89.0.0 + github.com/google/go-github/v89 v89.0.1-0.20260728185857-34349a88bac3 github.com/google/jsonschema-go v0.4.3 github.com/josephburnett/jd/v2 v2.5.0 github.com/lithammer/fuzzysearch v1.1.8 diff --git a/go.sum b/go.sum index f6a655d510..db06724eff 100644 --- a/go.sum +++ b/go.sum @@ -16,8 +16,8 @@ github.com/golang-jwt/jwt/v5 v5.3.1/go.mod h1:fxCRLWMO43lRc8nhHWY6LGqRcf+1gQWArs github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= github.com/google/go-cmp v0.7.0 h1:wk8382ETsv4JYUZwIsn6YpYiWiBsYLSJiTsyBybVuN8= github.com/google/go-cmp v0.7.0/go.mod h1:pXiqmnSA92OHEEa9HXL2W4E7lf9JzCmGVUdgjX3N/iU= -github.com/google/go-github/v89 v89.0.0 h1:35bEK5XoEcF3PZrlVbl9XN63f5BcJRA/UGkxeC9xPg0= -github.com/google/go-github/v89 v89.0.0/go.mod h1:QLcbU0ipeAqQuR5KSg8c2lql4Qk1EwJ2dWz/0rP4Nho= +github.com/google/go-github/v89 v89.0.1-0.20260728185857-34349a88bac3 h1:0a/p9KtPso8UBauBD/p9Go1oaZrrEydBNHjaaKkSHJo= +github.com/google/go-github/v89 v89.0.1-0.20260728185857-34349a88bac3/go.mod h1:QLcbU0ipeAqQuR5KSg8c2lql4Qk1EwJ2dWz/0rP4Nho= github.com/google/go-querystring v1.2.0 h1:yhqkPbu2/OH+V9BfpCVPZkNmUXhb2gBxJArfhIxNtP0= github.com/google/go-querystring v1.2.0/go.mod h1:8IFJqpSRITyJ8QhQ13bmbeMBDfmeEJZD5A0egEOmkqU= github.com/google/jsonschema-go v0.4.3 h1:/DBOLZTfDow7pe2GmaJNhltueGTtDKICi8V8p+DQPd0= diff --git a/pkg/github/issues.go b/pkg/github/issues.go index 0f12804a59..ad6c54f262 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -2382,11 +2382,11 @@ func CreateIssue(ctx context.Context, client *github.Client, owner string, repo } // Create the issue request - issueRequest := &github.IssueRequest{ - Title: github.Ptr(title), + issueRequest := github.CreateIssueRequest{ + Title: title, Body: github.Ptr(body), - Assignees: &assignees, - Labels: &labels, + Assignees: assignees, + Labels: labels, IssueFieldValues: issueFieldValues, } @@ -2449,7 +2449,7 @@ func UpdateIssue(ctx context.Context, client *github.Client, gqlClient *githubv4 } // Create the issue request with only provided fields - issueRequest := &github.IssueRequest{} + issueRequest := github.UpdateIssueRequest{} // Set optional parameters if provided if title != "" { @@ -2461,11 +2461,11 @@ func UpdateIssue(ctx context.Context, client *github.Client, gqlClient *githubv4 } if updateOptions.LabelsProvided { - issueRequest.Labels = &labels + issueRequest.Labels = labels } if updateOptions.AssigneesProvided { - issueRequest.Assignees = &assignees + issueRequest.Assignees = assignees } if milestoneNum != 0 { @@ -2519,7 +2519,7 @@ func UpdateIssue(ctx context.Context, client *github.Client, gqlClient *githubv4 } } - updatedIssue, resp, err := client.Issues.Edit(ctx, owner, repo, issueNumber, issueRequest) + updatedIssue, resp, err := client.Issues.Update(ctx, owner, repo, issueNumber, issueRequest) if err != nil { return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to update issue", diff --git a/pkg/github/issues_delete_test.go b/pkg/github/issues_delete_test.go index 54f515ba5c..11239e3a99 100644 --- a/pkg/github/issues_delete_test.go +++ b/pkg/github/issues_delete_test.go @@ -23,7 +23,7 @@ import ( func Test_IssueRequest_EmptyFieldValues_OmittedByJSON(t *testing.T) { t.Parallel() - req := &gogithub.IssueRequest{ + req := &gogithub.UpdateIssueRequest{ Title: gogithub.Ptr("still here"), IssueFieldValues: []*gogithub.IssueRequestFieldValue{}, } diff --git a/pkg/github/issues_granular.go b/pkg/github/issues_granular.go index c1eb556c9c..314ead3eb4 100644 --- a/pkg/github/issues_granular.go +++ b/pkg/github/issues_granular.go @@ -29,7 +29,7 @@ func issueUpdateTool( name, description, title string, extraProps map[string]*jsonschema.Schema, extraRequired []string, - buildRequest func(args map[string]any) (*github.IssueRequest, error), + buildRequest func(args map[string]any) (github.UpdateIssueRequest, error), ) inventory.ServerTool { props := map[string]*jsonschema.Schema{ "owner": { @@ -92,7 +92,7 @@ func issueUpdateTool( return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, nil } - issue, resp, err := client.Issues.Edit(ctx, owner, repo, issueNumber, issueReq) + issue, resp, err := client.Issues.Update(ctx, owner, repo, issueNumber, issueReq) if err != nil { return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to update issue", resp, err), nil, nil } @@ -164,8 +164,8 @@ func GranularCreateIssue(t translations.TranslationHelperFunc) inventory.ServerT } body, _ := OptionalParam[string](args, "body") - issueReq := &github.IssueRequest{ - Title: &title, + issueReq := github.CreateIssueRequest{ + Title: title, } if body != "" { issueReq.Body = &body @@ -206,12 +206,12 @@ func GranularUpdateIssueTitle(t translations.TranslationHelperFunc) inventory.Se "title": {Type: "string", Description: "The new title for the issue"}, }, []string{"title"}, - func(args map[string]any) (*github.IssueRequest, error) { + func(args map[string]any) (github.UpdateIssueRequest, error) { title, err := RequiredParam[string](args, "title") if err != nil { - return nil, err + return github.UpdateIssueRequest{}, err } - return &github.IssueRequest{Title: &title}, nil + return github.UpdateIssueRequest{Title: &title}, nil }, ) } @@ -226,12 +226,12 @@ func GranularUpdateIssueBody(t translations.TranslationHelperFunc) inventory.Ser "body": {Type: "string", Description: "The new body content for the issue"}, }, []string{"body"}, - func(args map[string]any) (*github.IssueRequest, error) { + func(args map[string]any) (github.UpdateIssueRequest, error) { body, err := RequiredParam[string](args, "body") if err != nil { - return nil, err + return github.UpdateIssueRequest{}, err } - return &github.IssueRequest{Body: &body}, nil + return github.UpdateIssueRequest{Body: &body}, nil }, ) } @@ -392,7 +392,7 @@ func GranularUpdateIssueAssignees(t translations.TranslationHelperFunc) inventor for i, p := range payload { logins[i] = p.(string) } - body = &github.IssueRequest{Assignees: &logins} + body = &github.UpdateIssueRequest{Assignees: logins} } apiURL := fmt.Sprintf("repos/%s/%s/issues/%d", owner, repo, issueNumber) @@ -610,7 +610,7 @@ func GranularUpdateIssueLabels(t translations.TranslationHelperFunc) inventory.S for i, p := range payload { names[i] = p.(string) } - body = &github.IssueRequest{Labels: &names} + body = &github.UpdateIssueRequest{Labels: names} } apiURL := fmt.Sprintf("repos/%s/%s/issues/%d", owner, repo, issueNumber) @@ -654,12 +654,12 @@ func GranularUpdateIssueMilestone(t translations.TranslationHelperFunc) inventor }, }, []string{"milestone"}, - func(args map[string]any) (*github.IssueRequest, error) { + func(args map[string]any) (github.UpdateIssueRequest, error) { milestone, err := RequiredInt(args, "milestone") if err != nil { - return nil, err + return github.UpdateIssueRequest{}, err } - return &github.IssueRequest{Milestone: &milestone}, nil + return github.UpdateIssueRequest{Milestone: &milestone}, nil }, ) } @@ -787,7 +787,7 @@ func GranularUpdateIssueType(t translations.TranslationHelperFunc) inventory.Ser }, } } else { - body = &github.IssueRequest{Type: &issueType} + body = &github.UpdateIssueRequest{Type: &issueType} } apiURL := fmt.Sprintf("repos/%s/%s/issues/%d", owner, repo, issueNumber) @@ -981,7 +981,7 @@ func GranularUpdateIssueState(t translations.TranslationHelperFunc) inventory.Se } body = req } else { - req := &github.IssueRequest{State: &state} + req := &github.UpdateIssueRequest{State: &state} if stateReason != "" { req.StateReason = &stateReason } diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index c3ea692464..3af8e4532a 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -1449,7 +1449,7 @@ func Test_CreateIssue(t *testing.T) { State: github.Ptr("open"), HTMLURL: github.Ptr("https://github.com/owner/repo/issues/123"), Assignees: []*github.User{{Login: github.Ptr("user1")}, {Login: github.Ptr("user2")}}, - Labels: []*github.Label{{Name: github.Ptr("bug")}, {Name: github.Ptr("help wanted")}}, + Labels: []*github.Label{{Name: "bug"}, {Name: "help wanted"}}, Milestone: &github.Milestone{Number: github.Ptr(5)}, Type: &github.IssueType{Name: github.Ptr("Bug")}, } @@ -1520,10 +1520,8 @@ func Test_CreateIssue(t *testing.T) { name: "successful issue creation with issue fields reconciled by names", mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ PostReposIssuesByOwnerByRepo: expectRequestBody(t, map[string]any{ - "title": "Issue with fields", - "body": "", - "labels": []any{}, - "assignees": []any{}, + "title": "Issue with fields", + "body": "", "issue_field_values": []any{ map[string]any{"field_id": float64(101), "value": "P1"}, map[string]any{"field_id": float64(102), "value": "Acme"}, @@ -2851,7 +2849,7 @@ func Test_UpdateIssue(t *testing.T) { State: github.Ptr("open"), HTMLURL: github.Ptr("https://github.com/owner/repo/issues/123"), Assignees: []*github.User{{Login: github.Ptr("assignee1")}, {Login: github.Ptr("assignee2")}}, - Labels: []*github.Label{{Name: github.Ptr("bug")}, {Name: github.Ptr("priority")}}, + Labels: []*github.Label{{Name: "bug"}, {Name: "priority"}}, Milestone: &github.Milestone{Number: github.Ptr(5)}, Type: &github.IssueType{Name: github.Ptr("Bug")}, } @@ -2864,7 +2862,7 @@ func Test_UpdateIssue(t *testing.T) { StateReason: github.Ptr("duplicate"), HTMLURL: github.Ptr("https://github.com/owner/repo/issues/123"), Assignees: []*github.User{{Login: github.Ptr("assignee1")}, {Login: github.Ptr("assignee2")}}, - Labels: []*github.Label{{Name: github.Ptr("bug")}, {Name: github.Ptr("priority")}}, + Labels: []*github.Label{{Name: "bug"}, {Name: "priority"}}, Milestone: &github.Milestone{Number: github.Ptr(5)}, Type: &github.IssueType{Name: github.Ptr("Bug")}, } @@ -3265,7 +3263,7 @@ func Test_UpdateIssue(t *testing.T) { Number: github.Ptr(123), Title: github.Ptr("Updated Title"), Body: github.Ptr("Updated Description"), - Labels: []*github.Label{{Name: github.Ptr("bug")}, {Name: github.Ptr("priority")}}, + Labels: []*github.Label{{Name: "bug"}, {Name: "priority"}}, Assignees: []*github.User{{Login: github.Ptr("assignee1")}, {Login: github.Ptr("assignee2")}}, Milestone: &github.Milestone{Number: github.Ptr(5)}, Type: &github.IssueType{Name: github.Ptr("Bug")}, @@ -3970,8 +3968,8 @@ func Test_AddSubIssue(t *testing.T) { }, Labels: []*github.Label{ { - Name: github.Ptr("enhancement"), - Color: github.Ptr("84b6eb"), + Name: "enhancement", + Color: "84b6eb", Description: github.Ptr("New feature or request"), }, }, @@ -4195,8 +4193,8 @@ func Test_GetSubIssues(t *testing.T) { }, Labels: []*github.Label{ { - Name: github.Ptr("bug"), - Color: github.Ptr("d73a4a"), + Name: "bug", + Color: "d73a4a", Description: github.Ptr("Something isn't working"), }, }, @@ -4684,8 +4682,8 @@ func Test_RemoveSubIssue(t *testing.T) { }, Labels: []*github.Label{ { - Name: github.Ptr("enhancement"), - Color: github.Ptr("84b6eb"), + Name: "enhancement", + Color: "84b6eb", Description: github.Ptr("New feature or request"), }, }, @@ -4892,8 +4890,8 @@ func Test_ReprioritizeSubIssue(t *testing.T) { }, Labels: []*github.Label{ { - Name: github.Ptr("enhancement"), - Color: github.Ptr("84b6eb"), + Name: "enhancement", + Color: "84b6eb", Description: github.Ptr("New feature or request"), }, }, diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index daf3b97331..b1cfa00945 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -777,10 +777,10 @@ func CreatePullRequest(t translations.TranslationHelperFunc) inventory.ServerToo return utils.NewToolResultError(err.Error()), nil, nil } - newPR := &github.NewPullRequest{ + newPR := &github.CreatePullRequest{ Title: github.Ptr(title), - Head: github.Ptr(head), - Base: github.Ptr(base), + Head: head, + Base: base, } if body != "" { @@ -794,7 +794,7 @@ func CreatePullRequest(t translations.TranslationHelperFunc) inventory.ServerToo if err != nil { return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, nil } - pr, resp, err := client.PullRequests.Create(ctx, owner, repo, newPR) + pr, resp, err := client.PullRequests.Create(ctx, owner, repo, *newPR) if err != nil { return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to create pull request", diff --git a/third-party-licenses.darwin.md b/third-party-licenses.darwin.md index 2bf5e86eae..6e4581c515 100644 --- a/third-party-licenses.darwin.md +++ b/third-party-licenses.darwin.md @@ -17,7 +17,7 @@ The following packages are included for the amd64, arm64 architectures. - [github.com/github/github-mcp-server](https://pkg.go.dev/github.com/github/github-mcp-server) ([MIT](https://github.com/github/github-mcp-server/blob/HEAD/LICENSE)) - [github.com/go-chi/chi/v5](https://pkg.go.dev/github.com/go-chi/chi/v5) ([MIT](https://github.com/go-chi/chi/blob/v5.3.1/LICENSE)) - [github.com/go-viper/mapstructure/v2](https://pkg.go.dev/github.com/go-viper/mapstructure/v2) ([MIT](https://github.com/go-viper/mapstructure/blob/v2.5.0/LICENSE)) - - [github.com/google/go-github/v89/github](https://pkg.go.dev/github.com/google/go-github/v89/github) ([BSD-3-Clause](https://github.com/google/go-github/blob/v89.0.0/LICENSE)) + - [github.com/google/go-github/v89/github](https://pkg.go.dev/github.com/google/go-github/v89/github) ([BSD-3-Clause](https://github.com/google/go-github/blob/34349a88bac3/LICENSE)) - [github.com/google/go-querystring/query](https://pkg.go.dev/github.com/google/go-querystring/query) ([BSD-3-Clause](https://github.com/google/go-querystring/blob/v1.2.0/LICENSE)) - [github.com/google/jsonschema-go/jsonschema](https://pkg.go.dev/github.com/google/jsonschema-go/jsonschema) ([MIT](https://github.com/google/jsonschema-go/blob/v0.4.3/LICENSE)) - [github.com/gorilla/css/scanner](https://pkg.go.dev/github.com/gorilla/css/scanner) ([BSD-3-Clause](https://github.com/gorilla/css/blob/v1.0.1/LICENSE)) diff --git a/third-party-licenses.linux.md b/third-party-licenses.linux.md index 4caa5f58b2..bdc3cf1fa7 100644 --- a/third-party-licenses.linux.md +++ b/third-party-licenses.linux.md @@ -17,7 +17,7 @@ The following packages are included for the 386, amd64, arm64 architectures. - [github.com/github/github-mcp-server](https://pkg.go.dev/github.com/github/github-mcp-server) ([MIT](https://github.com/github/github-mcp-server/blob/HEAD/LICENSE)) - [github.com/go-chi/chi/v5](https://pkg.go.dev/github.com/go-chi/chi/v5) ([MIT](https://github.com/go-chi/chi/blob/v5.3.1/LICENSE)) - [github.com/go-viper/mapstructure/v2](https://pkg.go.dev/github.com/go-viper/mapstructure/v2) ([MIT](https://github.com/go-viper/mapstructure/blob/v2.5.0/LICENSE)) - - [github.com/google/go-github/v89/github](https://pkg.go.dev/github.com/google/go-github/v89/github) ([BSD-3-Clause](https://github.com/google/go-github/blob/v89.0.0/LICENSE)) + - [github.com/google/go-github/v89/github](https://pkg.go.dev/github.com/google/go-github/v89/github) ([BSD-3-Clause](https://github.com/google/go-github/blob/34349a88bac3/LICENSE)) - [github.com/google/go-querystring/query](https://pkg.go.dev/github.com/google/go-querystring/query) ([BSD-3-Clause](https://github.com/google/go-querystring/blob/v1.2.0/LICENSE)) - [github.com/google/jsonschema-go/jsonschema](https://pkg.go.dev/github.com/google/jsonschema-go/jsonschema) ([MIT](https://github.com/google/jsonschema-go/blob/v0.4.3/LICENSE)) - [github.com/gorilla/css/scanner](https://pkg.go.dev/github.com/gorilla/css/scanner) ([BSD-3-Clause](https://github.com/gorilla/css/blob/v1.0.1/LICENSE)) diff --git a/third-party-licenses.windows.md b/third-party-licenses.windows.md index a7164a2aad..da72cebc03 100644 --- a/third-party-licenses.windows.md +++ b/third-party-licenses.windows.md @@ -17,7 +17,7 @@ The following packages are included for the 386, amd64, arm64 architectures. - [github.com/github/github-mcp-server](https://pkg.go.dev/github.com/github/github-mcp-server) ([MIT](https://github.com/github/github-mcp-server/blob/HEAD/LICENSE)) - [github.com/go-chi/chi/v5](https://pkg.go.dev/github.com/go-chi/chi/v5) ([MIT](https://github.com/go-chi/chi/blob/v5.3.1/LICENSE)) - [github.com/go-viper/mapstructure/v2](https://pkg.go.dev/github.com/go-viper/mapstructure/v2) ([MIT](https://github.com/go-viper/mapstructure/blob/v2.5.0/LICENSE)) - - [github.com/google/go-github/v89/github](https://pkg.go.dev/github.com/google/go-github/v89/github) ([BSD-3-Clause](https://github.com/google/go-github/blob/v89.0.0/LICENSE)) + - [github.com/google/go-github/v89/github](https://pkg.go.dev/github.com/google/go-github/v89/github) ([BSD-3-Clause](https://github.com/google/go-github/blob/34349a88bac3/LICENSE)) - [github.com/google/go-querystring/query](https://pkg.go.dev/github.com/google/go-querystring/query) ([BSD-3-Clause](https://github.com/google/go-querystring/blob/v1.2.0/LICENSE)) - [github.com/google/jsonschema-go/jsonschema](https://pkg.go.dev/github.com/google/jsonschema-go/jsonschema) ([MIT](https://github.com/google/jsonschema-go/blob/v0.4.3/LICENSE)) - [github.com/gorilla/css/scanner](https://pkg.go.dev/github.com/gorilla/css/scanner) ([BSD-3-Clause](https://github.com/gorilla/css/blob/v1.0.1/LICENSE)) From 456fae9d0464944b946a288aed152dbb0c369a76 Mon Sep 17 00:00:00 2001 From: Tommaso Moro <37270480+tommaso-moro@users.noreply.github.com> Date: Wed, 29 Jul 2026 14:06:03 +0100 Subject: [PATCH 2/6] Make fields parameter available by default (#2952) * Promote fields parameter beyond Insiders Keep fields_param as an independently controlled feature flag while removing it from the Insiders expansion. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Make fields parameter available by default Remove the fields_param feature flag and legacy tool variants so selected read tools always advertise and honor fields. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5c43cc70-27b5-47b4-bbd1-99d20f42d61b --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5c43cc70-27b5-47b4-bbd1-99d20f42d61b --- README.md | 8 + docs/feature-flags.md | 90 ---------- docs/insiders-features.md | 90 ---------- .../__toolsnaps__/get_file_contents.snap | 18 ++ .../get_file_contents_ff_fields_param.snap | 57 ------- pkg/github/__toolsnaps__/list_commits.snap | 14 ++ .../list_commits_ff_fields_param.snap | 71 -------- pkg/github/__toolsnaps__/list_issues.snap | 19 +++ .../list_issues_ff_fields_param.snap | 112 ------------- .../__toolsnaps__/list_pull_requests.snap | 34 ++++ .../list_pull_requests_ff_fields_param.snap | 106 ------------ pkg/github/__toolsnaps__/list_releases.snap | 18 ++ .../list_releases_ff_fields_param.snap | 55 ------ pkg/github/__toolsnaps__/search_code.snap | 14 ++ .../search_code_ff_fields_param.snap | 58 ------- pkg/github/__toolsnaps__/search_issues.snap | 33 ++++ .../search_issues_ff_fields_param.snap | 98 ----------- .../__toolsnaps__/search_pull_requests.snap | 31 ++++ .../search_pull_requests_ff_fields_param.snap | 96 ----------- pkg/github/feature_flags.go | 10 -- pkg/github/feature_flags_test.go | 16 -- pkg/github/fields_filtering_test.go | 74 -------- pkg/github/fields_param_gating_test.go | 84 ---------- pkg/github/issues.go | 101 +++-------- pkg/github/issues_test.go | 12 +- pkg/github/pullrequests.go | 101 ++--------- pkg/github/pullrequests_test.go | 12 +- pkg/github/repositories.go | 158 ++++-------------- pkg/github/repositories_test.go | 32 +--- pkg/github/search.go | 53 ++---- pkg/github/search_test.go | 20 +-- pkg/github/tools.go | 8 - 32 files changed, 274 insertions(+), 1429 deletions(-) delete mode 100644 pkg/github/__toolsnaps__/get_file_contents_ff_fields_param.snap delete mode 100644 pkg/github/__toolsnaps__/list_commits_ff_fields_param.snap delete mode 100644 pkg/github/__toolsnaps__/list_issues_ff_fields_param.snap delete mode 100644 pkg/github/__toolsnaps__/list_pull_requests_ff_fields_param.snap delete mode 100644 pkg/github/__toolsnaps__/list_releases_ff_fields_param.snap delete mode 100644 pkg/github/__toolsnaps__/search_code_ff_fields_param.snap delete mode 100644 pkg/github/__toolsnaps__/search_issues_ff_fields_param.snap delete mode 100644 pkg/github/__toolsnaps__/search_pull_requests_ff_fields_param.snap delete mode 100644 pkg/github/fields_param_gating_test.go diff --git a/README.md b/README.md index 1a06c0697d..61021f2950 100644 --- a/README.md +++ b/README.md @@ -960,6 +960,7 @@ The following sets of tools are available: - `after`: Cursor for pagination. Use the cursor from the previous response. (string, optional) - `direction`: Order direction. If provided, the 'orderBy' also needs to be provided. (string, optional) - `field_filters`: Filter by custom issue field values. Each entry takes a field_name and a value; the server looks up the field and coerces the value to its type (single-select option name, text, number, or YYYY-MM-DD date). (object[], optional) + - `fields`: Subset of fields to return for each issue. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body' and 'field_values' in particular drops the largest per-result data. (string[], optional) - `labels`: Filter by labels (string[], optional) - `orderBy`: Order issues by field. If provided, the 'direction' also needs to be provided. (string, optional) - `owner`: Repository owner (string, required) @@ -970,6 +971,7 @@ The following sets of tools are available: - **search_issues** - Search issues - **Required OAuth Scopes**: `repo` + - `fields`: Subset of fields to return for each issue result. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body', 'reactions', and 'labels' in particular drops the largest per-result data. (string[], optional) - `order`: Sort order (string, optional) - `owner`: Optional repository owner. If provided with repo, only issues for this repository are listed. (string, optional) - `page`: Page number for pagination (min 1) (number, optional) @@ -1178,6 +1180,7 @@ The following sets of tools are available: - **Required OAuth Scopes**: `repo` - `base`: Filter by base branch (string, optional) - `direction`: Sort direction (string, optional) + - `fields`: Subset of fields to return for each pull request. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body' in particular drops the largest per-result data. (string[], optional) - `head`: Filter by head user/org and branch (string, optional) - `owner`: Repository owner (string, required) - `page`: Page number for pagination (min 1) (number, optional) @@ -1229,6 +1232,7 @@ The following sets of tools are available: - **search_pull_requests** - Search pull requests - **Required OAuth Scopes**: `repo` + - `fields`: Subset of fields to return for each pull request result. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body', 'reactions', and 'labels' in particular drops the largest per-result data. (string[], optional) - `order`: Sort order (string, optional) - `owner`: Optional repository owner. If provided with repo, only pull requests for this repository are listed. (string, optional) - `page`: Page number for pagination (min 1) (number, optional) @@ -1313,6 +1317,7 @@ The following sets of tools are available: - **get_file_contents** - Get file or directory contents - **Required OAuth Scopes**: `repo` + - `fields`: Subset of fields to return for each entry when the path is a directory. If omitted, all fields are returned. Ignored when the path is a single file. Use this to reduce response size when listing directories and you only need specific fields, e.g. just 'name' and 'type'. (string[], optional) - `owner`: Repository owner (username or organization) (string, required) - `path`: Path to file/directory (string, optional) - `ref`: Accepts optional git refs such as `refs/tags/{tag}`, `refs/heads/{branch}` or `refs/pull/{pr_number}/head` (string, optional) @@ -1346,6 +1351,7 @@ The following sets of tools are available: - **list_commits** - List commits - **Required OAuth Scopes**: `repo` - `author`: Author username or email address to filter commits by (string, optional) + - `fields`: Subset of fields to return for each commit. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields, e.g. just 'sha' and 'html_url'. (string[], optional) - `owner`: Repository owner (string, required) - `page`: Page number for pagination (min 1) (number, optional) - `path`: Only commits containing this file path will be returned (string, optional) @@ -1357,6 +1363,7 @@ The following sets of tools are available: - **list_releases** - List releases - **Required OAuth Scopes**: `repo` + - `fields`: Subset of fields to return for each release. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body' in particular drops the largest per-release data. (string[], optional) - `owner`: Repository owner (string, required) - `page`: Page number for pagination (min 1) (number, optional) - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) @@ -1387,6 +1394,7 @@ The following sets of tools are available: - **search_code** - Search code - **Required OAuth Scopes**: `repo` + - `fields`: Subset of fields to return for each code search result. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'repository' and 'text_matches' in particular drops the largest per-result data. (string[], optional) - `order`: Sort order for results (string, optional) - `page`: Page number for pagination (min 1) (number, optional) - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) diff --git a/docs/feature-flags.md b/docs/feature-flags.md index c83e0c74be..32891af62e 100644 --- a/docs/feature-flags.md +++ b/docs/feature-flags.md @@ -338,94 +338,4 @@ runtime behavior (such as output formatting) won't appear here. - 'blocked_by' - the subject issue is blocked by the related issue. - 'blocking' - the subject issue blocks the related issue. (string, required) -### `fields_param` - -- **get_file_contents** - Get file or directory contents - - **Required OAuth Scopes**: `repo` - - `fields`: Subset of fields to return for each entry when the path is a directory. If omitted, all fields are returned. Ignored when the path is a single file. Use this to reduce response size when listing directories and you only need specific fields, e.g. just 'name' and 'type'. (string[], optional) - - `owner`: Repository owner (username or organization) (string, required) - - `path`: Path to file/directory (string, optional) - - `ref`: Accepts optional git refs such as `refs/tags/{tag}`, `refs/heads/{branch}` or `refs/pull/{pr_number}/head` (string, optional) - - `repo`: Repository name (string, required) - - `sha`: Accepts optional commit SHA. If specified, it will be used instead of ref (string, optional) - -- **list_commits** - List commits - - **Required OAuth Scopes**: `repo` - - `author`: Author username or email address to filter commits by (string, optional) - - `fields`: Subset of fields to return for each commit. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields, e.g. just 'sha' and 'html_url'. (string[], optional) - - `owner`: Repository owner (string, required) - - `page`: Page number for pagination (min 1) (number, optional) - - `path`: Only commits containing this file path will be returned (string, optional) - - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) - - `repo`: Repository name (string, required) - - `sha`: Commit SHA, branch or tag name to list commits of. If not provided, uses the default branch of the repository. If a commit SHA is provided, will list commits up to that SHA. (string, optional) - - `since`: Only commits after this date will be returned (ISO 8601 format: YYYY-MM-DDTHH:MM:SSZ or YYYY-MM-DD) (string, optional) - - `until`: Only commits before this date will be returned (ISO 8601 format: YYYY-MM-DDTHH:MM:SSZ or YYYY-MM-DD) (string, optional) - -- **list_issues** - List issues - - **Required OAuth Scopes**: `repo` - - `after`: Cursor for pagination. Use the cursor from the previous response. (string, optional) - - `direction`: Order direction. If provided, the 'orderBy' also needs to be provided. (string, optional) - - `field_filters`: Filter by custom issue field values. Each entry takes a field_name and a value; the server looks up the field and coerces the value to its type (single-select option name, text, number, or YYYY-MM-DD date). (object[], optional) - - `fields`: Subset of fields to return for each issue. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body' and 'field_values' in particular drops the largest per-result data. (string[], optional) - - `labels`: Filter by labels (string[], optional) - - `orderBy`: Order issues by field. If provided, the 'direction' also needs to be provided. (string, optional) - - `owner`: Repository owner (string, required) - - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) - - `repo`: Repository name (string, required) - - `since`: Filter by date (ISO 8601 timestamp) (string, optional) - - `state`: Filter by state, by default both open and closed issues are returned when not provided (string, optional) - -- **list_pull_requests** - List pull requests - - **Required OAuth Scopes**: `repo` - - `base`: Filter by base branch (string, optional) - - `direction`: Sort direction (string, optional) - - `fields`: Subset of fields to return for each pull request. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body' in particular drops the largest per-result data. (string[], optional) - - `head`: Filter by head user/org and branch (string, optional) - - `owner`: Repository owner (string, required) - - `page`: Page number for pagination (min 1) (number, optional) - - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) - - `repo`: Repository name (string, required) - - `sort`: Sort by (string, optional) - - `state`: Filter by state (string, optional) - -- **list_releases** - List releases - - **Required OAuth Scopes**: `repo` - - `fields`: Subset of fields to return for each release. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body' in particular drops the largest per-release data. (string[], optional) - - `owner`: Repository owner (string, required) - - `page`: Page number for pagination (min 1) (number, optional) - - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) - - `repo`: Repository name (string, required) - -- **search_code** - Search code - - **Required OAuth Scopes**: `repo` - - `fields`: Subset of fields to return for each code search result. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'repository' and 'text_matches' in particular drops the largest per-result data. (string[], optional) - - `order`: Sort order for results (string, optional) - - `page`: Page number for pagination (min 1) (number, optional) - - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) - - `query`: Search query (GitHub code search REST). Implicit AND between terms; supports `OR`, `NOT`, and `"quoted phrase"` for exact match. Qualifiers: `repo:owner/repo`, `org:`, `user:`, `language:`, `path:dir` (prefix match), `filename:exact.ext`, `extension:`, `in:file`, `in:path`, `size:`, `is:archived`, `is:fork`. Max 256 chars. Examples: `WithContext language:go org:github`; `"package main" repo:o/r`; `func extension:go path:cmd repo:o/r`; `NOT TODO language:go repo:o/r`. (string, required) - - `sort`: Sort field ('indexed' only) (string, optional) - -- **search_issues** - Search issues - - **Required OAuth Scopes**: `repo` - - `fields`: Subset of fields to return for each issue result. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body', 'reactions', and 'labels' in particular drops the largest per-result data. (string[], optional) - - `order`: Sort order (string, optional) - - `owner`: Optional repository owner. If provided with repo, only issues for this repository are listed. (string, optional) - - `page`: Page number for pagination (min 1) (number, optional) - - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) - - `query`: Search query using GitHub issues search syntax (string, required) - - `repo`: Optional repository name. If provided with owner, only issues for this repository are listed. (string, optional) - - `sort`: Sort field by number of matches of categories, defaults to best match (string, optional) - -- **search_pull_requests** - Search pull requests - - **Required OAuth Scopes**: `repo` - - `fields`: Subset of fields to return for each pull request result. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body', 'reactions', and 'labels' in particular drops the largest per-result data. (string[], optional) - - `order`: Sort order (string, optional) - - `owner`: Optional repository owner. If provided with repo, only pull requests for this repository are listed. (string, optional) - - `page`: Page number for pagination (min 1) (number, optional) - - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) - - `query`: Search query using GitHub pull request search syntax (string, required) - - `repo`: Optional repository name. If provided with owner, only pull requests for this repository are listed. (string, optional) - - `sort`: Sort field by number of matches of categories, defaults to best match (string, optional) - diff --git a/docs/insiders-features.md b/docs/insiders-features.md index f85870ef20..10df187a91 100644 --- a/docs/insiders-features.md +++ b/docs/insiders-features.md @@ -133,96 +133,6 @@ The list below is generated from the Go source. It covers tool **inventory and s - 'blocked_by' - the subject issue is blocked by the related issue. - 'blocking' - the subject issue blocks the related issue. (string, required) -### `fields_param` - -- **get_file_contents** - Get file or directory contents - - **Required OAuth Scopes**: `repo` - - `fields`: Subset of fields to return for each entry when the path is a directory. If omitted, all fields are returned. Ignored when the path is a single file. Use this to reduce response size when listing directories and you only need specific fields, e.g. just 'name' and 'type'. (string[], optional) - - `owner`: Repository owner (username or organization) (string, required) - - `path`: Path to file/directory (string, optional) - - `ref`: Accepts optional git refs such as `refs/tags/{tag}`, `refs/heads/{branch}` or `refs/pull/{pr_number}/head` (string, optional) - - `repo`: Repository name (string, required) - - `sha`: Accepts optional commit SHA. If specified, it will be used instead of ref (string, optional) - -- **list_commits** - List commits - - **Required OAuth Scopes**: `repo` - - `author`: Author username or email address to filter commits by (string, optional) - - `fields`: Subset of fields to return for each commit. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields, e.g. just 'sha' and 'html_url'. (string[], optional) - - `owner`: Repository owner (string, required) - - `page`: Page number for pagination (min 1) (number, optional) - - `path`: Only commits containing this file path will be returned (string, optional) - - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) - - `repo`: Repository name (string, required) - - `sha`: Commit SHA, branch or tag name to list commits of. If not provided, uses the default branch of the repository. If a commit SHA is provided, will list commits up to that SHA. (string, optional) - - `since`: Only commits after this date will be returned (ISO 8601 format: YYYY-MM-DDTHH:MM:SSZ or YYYY-MM-DD) (string, optional) - - `until`: Only commits before this date will be returned (ISO 8601 format: YYYY-MM-DDTHH:MM:SSZ or YYYY-MM-DD) (string, optional) - -- **list_issues** - List issues - - **Required OAuth Scopes**: `repo` - - `after`: Cursor for pagination. Use the cursor from the previous response. (string, optional) - - `direction`: Order direction. If provided, the 'orderBy' also needs to be provided. (string, optional) - - `field_filters`: Filter by custom issue field values. Each entry takes a field_name and a value; the server looks up the field and coerces the value to its type (single-select option name, text, number, or YYYY-MM-DD date). (object[], optional) - - `fields`: Subset of fields to return for each issue. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body' and 'field_values' in particular drops the largest per-result data. (string[], optional) - - `labels`: Filter by labels (string[], optional) - - `orderBy`: Order issues by field. If provided, the 'direction' also needs to be provided. (string, optional) - - `owner`: Repository owner (string, required) - - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) - - `repo`: Repository name (string, required) - - `since`: Filter by date (ISO 8601 timestamp) (string, optional) - - `state`: Filter by state, by default both open and closed issues are returned when not provided (string, optional) - -- **list_pull_requests** - List pull requests - - **Required OAuth Scopes**: `repo` - - `base`: Filter by base branch (string, optional) - - `direction`: Sort direction (string, optional) - - `fields`: Subset of fields to return for each pull request. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body' in particular drops the largest per-result data. (string[], optional) - - `head`: Filter by head user/org and branch (string, optional) - - `owner`: Repository owner (string, required) - - `page`: Page number for pagination (min 1) (number, optional) - - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) - - `repo`: Repository name (string, required) - - `sort`: Sort by (string, optional) - - `state`: Filter by state (string, optional) - -- **list_releases** - List releases - - **Required OAuth Scopes**: `repo` - - `fields`: Subset of fields to return for each release. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body' in particular drops the largest per-release data. (string[], optional) - - `owner`: Repository owner (string, required) - - `page`: Page number for pagination (min 1) (number, optional) - - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) - - `repo`: Repository name (string, required) - -- **search_code** - Search code - - **Required OAuth Scopes**: `repo` - - `fields`: Subset of fields to return for each code search result. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'repository' and 'text_matches' in particular drops the largest per-result data. (string[], optional) - - `order`: Sort order for results (string, optional) - - `page`: Page number for pagination (min 1) (number, optional) - - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) - - `query`: Search query (GitHub code search REST). Implicit AND between terms; supports `OR`, `NOT`, and `"quoted phrase"` for exact match. Qualifiers: `repo:owner/repo`, `org:`, `user:`, `language:`, `path:dir` (prefix match), `filename:exact.ext`, `extension:`, `in:file`, `in:path`, `size:`, `is:archived`, `is:fork`. Max 256 chars. Examples: `WithContext language:go org:github`; `"package main" repo:o/r`; `func extension:go path:cmd repo:o/r`; `NOT TODO language:go repo:o/r`. (string, required) - - `sort`: Sort field ('indexed' only) (string, optional) - -- **search_issues** - Search issues - - **Required OAuth Scopes**: `repo` - - `fields`: Subset of fields to return for each issue result. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body', 'reactions', and 'labels' in particular drops the largest per-result data. (string[], optional) - - `order`: Sort order (string, optional) - - `owner`: Optional repository owner. If provided with repo, only issues for this repository are listed. (string, optional) - - `page`: Page number for pagination (min 1) (number, optional) - - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) - - `query`: Search query using GitHub issues search syntax (string, required) - - `repo`: Optional repository name. If provided with owner, only issues for this repository are listed. (string, optional) - - `sort`: Sort field by number of matches of categories, defaults to best match (string, optional) - -- **search_pull_requests** - Search pull requests - - **Required OAuth Scopes**: `repo` - - `fields`: Subset of fields to return for each pull request result. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body', 'reactions', and 'labels' in particular drops the largest per-result data. (string[], optional) - - `order`: Sort order (string, optional) - - `owner`: Optional repository owner. If provided with repo, only pull requests for this repository are listed. (string, optional) - - `page`: Page number for pagination (min 1) (number, optional) - - `perPage`: Results per page for pagination (min 1, max 100) (number, optional) - - `query`: Search query using GitHub pull request search syntax (string, required) - - `repo`: Optional repository name. If provided with owner, only pull requests for this repository are listed. (string, optional) - - `sort`: Sort field by number of matches of categories, defaults to best match (string, optional) - --- diff --git a/pkg/github/__toolsnaps__/get_file_contents.snap b/pkg/github/__toolsnaps__/get_file_contents.snap index ea317f6f14..dec933c94d 100644 --- a/pkg/github/__toolsnaps__/get_file_contents.snap +++ b/pkg/github/__toolsnaps__/get_file_contents.snap @@ -7,6 +7,24 @@ "description": "Get the contents of a file or directory from a GitHub repository", "inputSchema": { "properties": { + "fields": { + "description": "Subset of fields to return for each entry when the path is a directory. If omitted, all fields are returned. Ignored when the path is a single file. Use this to reduce response size when listing directories and you only need specific fields, e.g. just 'name' and 'type'.", + "items": { + "enum": [ + "type", + "name", + "path", + "size", + "sha", + "url", + "git_url", + "html_url", + "download_url" + ], + "type": "string" + }, + "type": "array" + }, "owner": { "description": "Repository owner (username or organization)", "type": "string" diff --git a/pkg/github/__toolsnaps__/get_file_contents_ff_fields_param.snap b/pkg/github/__toolsnaps__/get_file_contents_ff_fields_param.snap deleted file mode 100644 index dec933c94d..0000000000 --- a/pkg/github/__toolsnaps__/get_file_contents_ff_fields_param.snap +++ /dev/null @@ -1,57 +0,0 @@ -{ - "annotations": { - "idempotentHint": false, - "readOnlyHint": true, - "title": "Get file or directory contents" - }, - "description": "Get the contents of a file or directory from a GitHub repository", - "inputSchema": { - "properties": { - "fields": { - "description": "Subset of fields to return for each entry when the path is a directory. If omitted, all fields are returned. Ignored when the path is a single file. Use this to reduce response size when listing directories and you only need specific fields, e.g. just 'name' and 'type'.", - "items": { - "enum": [ - "type", - "name", - "path", - "size", - "sha", - "url", - "git_url", - "html_url", - "download_url" - ], - "type": "string" - }, - "type": "array" - }, - "owner": { - "description": "Repository owner (username or organization)", - "type": "string" - }, - "path": { - "default": "/", - "description": "Path to file/directory", - "type": "string" - }, - "ref": { - "description": "Accepts optional git refs such as `refs/tags/{tag}`, `refs/heads/{branch}` or `refs/pull/{pr_number}/head`", - "type": "string" - }, - "repo": { - "description": "Repository name", - "type": "string" - }, - "sha": { - "description": "Accepts optional commit SHA. If specified, it will be used instead of ref", - "type": "string" - } - }, - "required": [ - "owner", - "repo" - ], - "type": "object" - }, - "name": "get_file_contents" -} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/list_commits.snap b/pkg/github/__toolsnaps__/list_commits.snap index 00cce882f1..bc4ffd1753 100644 --- a/pkg/github/__toolsnaps__/list_commits.snap +++ b/pkg/github/__toolsnaps__/list_commits.snap @@ -11,6 +11,20 @@ "description": "Author username or email address to filter commits by", "type": "string" }, + "fields": { + "description": "Subset of fields to return for each commit. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields, e.g. just 'sha' and 'html_url'.", + "items": { + "enum": [ + "sha", + "html_url", + "commit", + "author", + "committer" + ], + "type": "string" + }, + "type": "array" + }, "owner": { "description": "Repository owner", "type": "string" diff --git a/pkg/github/__toolsnaps__/list_commits_ff_fields_param.snap b/pkg/github/__toolsnaps__/list_commits_ff_fields_param.snap deleted file mode 100644 index bc4ffd1753..0000000000 --- a/pkg/github/__toolsnaps__/list_commits_ff_fields_param.snap +++ /dev/null @@ -1,71 +0,0 @@ -{ - "annotations": { - "idempotentHint": false, - "readOnlyHint": true, - "title": "List commits" - }, - "description": "Get list of commits of a branch in a GitHub repository. Returns at least 30 results per page by default, but can return more if specified using the perPage parameter (up to 100).", - "inputSchema": { - "properties": { - "author": { - "description": "Author username or email address to filter commits by", - "type": "string" - }, - "fields": { - "description": "Subset of fields to return for each commit. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields, e.g. just 'sha' and 'html_url'.", - "items": { - "enum": [ - "sha", - "html_url", - "commit", - "author", - "committer" - ], - "type": "string" - }, - "type": "array" - }, - "owner": { - "description": "Repository owner", - "type": "string" - }, - "page": { - "description": "Page number for pagination (min 1)", - "minimum": 1, - "type": "number" - }, - "path": { - "description": "Only commits containing this file path will be returned", - "type": "string" - }, - "perPage": { - "description": "Results per page for pagination (min 1, max 100)", - "maximum": 100, - "minimum": 1, - "type": "number" - }, - "repo": { - "description": "Repository name", - "type": "string" - }, - "sha": { - "description": "Commit SHA, branch or tag name to list commits of. If not provided, uses the default branch of the repository. If a commit SHA is provided, will list commits up to that SHA.", - "type": "string" - }, - "since": { - "description": "Only commits after this date will be returned (ISO 8601 format: YYYY-MM-DDTHH:MM:SSZ or YYYY-MM-DD)", - "type": "string" - }, - "until": { - "description": "Only commits before this date will be returned (ISO 8601 format: YYYY-MM-DDTHH:MM:SSZ or YYYY-MM-DD)", - "type": "string" - } - }, - "required": [ - "owner", - "repo" - ], - "type": "object" - }, - "name": "list_commits" -} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/list_issues.snap b/pkg/github/__toolsnaps__/list_issues.snap index 5c68c01497..1055fe9947 100644 --- a/pkg/github/__toolsnaps__/list_issues.snap +++ b/pkg/github/__toolsnaps__/list_issues.snap @@ -40,6 +40,25 @@ }, "type": "array" }, + "fields": { + "description": "Subset of fields to return for each issue. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body' and 'field_values' in particular drops the largest per-result data.", + "items": { + "enum": [ + "number", + "title", + "body", + "state", + "user", + "labels", + "comments", + "created_at", + "updated_at", + "field_values" + ], + "type": "string" + }, + "type": "array" + }, "labels": { "description": "Filter by labels", "items": { diff --git a/pkg/github/__toolsnaps__/list_issues_ff_fields_param.snap b/pkg/github/__toolsnaps__/list_issues_ff_fields_param.snap deleted file mode 100644 index 1055fe9947..0000000000 --- a/pkg/github/__toolsnaps__/list_issues_ff_fields_param.snap +++ /dev/null @@ -1,112 +0,0 @@ -{ - "annotations": { - "idempotentHint": false, - "readOnlyHint": true, - "title": "List issues" - }, - "description": "List issues in a GitHub repository. For pagination, use the 'endCursor' from the previous response's 'pageInfo' in the 'after' parameter.", - "inputSchema": { - "properties": { - "after": { - "description": "Cursor for pagination. Use the cursor from the previous response.", - "type": "string" - }, - "direction": { - "description": "Order direction. If provided, the 'orderBy' also needs to be provided.", - "enum": [ - "ASC", - "DESC" - ], - "type": "string" - }, - "field_filters": { - "description": "Filter by custom issue field values. Each entry takes a field_name and a value; the server looks up the field and coerces the value to its type (single-select option name, text, number, or YYYY-MM-DD date).", - "items": { - "properties": { - "field_name": { - "description": "Name of the custom field (e.g. \"Priority\"). Case-insensitive.", - "type": "string" - }, - "value": { - "description": "Value to filter on. For single-select fields, the option name (e.g. \"P1\"). For dates, YYYY-MM-DD. For numbers, the numeric value as a string. For text, the text value.", - "type": "string" - } - }, - "required": [ - "field_name", - "value" - ], - "type": "object" - }, - "type": "array" - }, - "fields": { - "description": "Subset of fields to return for each issue. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body' and 'field_values' in particular drops the largest per-result data.", - "items": { - "enum": [ - "number", - "title", - "body", - "state", - "user", - "labels", - "comments", - "created_at", - "updated_at", - "field_values" - ], - "type": "string" - }, - "type": "array" - }, - "labels": { - "description": "Filter by labels", - "items": { - "type": "string" - }, - "type": "array" - }, - "orderBy": { - "description": "Order issues by field. If provided, the 'direction' also needs to be provided.", - "enum": [ - "CREATED_AT", - "UPDATED_AT", - "COMMENTS" - ], - "type": "string" - }, - "owner": { - "description": "Repository owner", - "type": "string" - }, - "perPage": { - "description": "Results per page for pagination (min 1, max 100)", - "maximum": 100, - "minimum": 1, - "type": "number" - }, - "repo": { - "description": "Repository name", - "type": "string" - }, - "since": { - "description": "Filter by date (ISO 8601 timestamp)", - "type": "string" - }, - "state": { - "description": "Filter by state, by default both open and closed issues are returned when not provided", - "enum": [ - "OPEN", - "CLOSED" - ], - "type": "string" - } - }, - "required": [ - "owner", - "repo" - ], - "type": "object" - }, - "name": "list_issues" -} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/list_pull_requests.snap b/pkg/github/__toolsnaps__/list_pull_requests.snap index a94b6eaee1..d37986d529 100644 --- a/pkg/github/__toolsnaps__/list_pull_requests.snap +++ b/pkg/github/__toolsnaps__/list_pull_requests.snap @@ -19,6 +19,40 @@ ], "type": "string" }, + "fields": { + "description": "Subset of fields to return for each pull request. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body' in particular drops the largest per-result data.", + "items": { + "enum": [ + "number", + "title", + "body", + "state", + "draft", + "merged", + "mergeable_state", + "html_url", + "user", + "labels", + "assignees", + "requested_reviewers", + "merged_by", + "head", + "base", + "additions", + "deletions", + "changed_files", + "commits", + "comments", + "created_at", + "updated_at", + "closed_at", + "merged_at", + "milestone" + ], + "type": "string" + }, + "type": "array" + }, "head": { "description": "Filter by head user/org and branch", "type": "string" diff --git a/pkg/github/__toolsnaps__/list_pull_requests_ff_fields_param.snap b/pkg/github/__toolsnaps__/list_pull_requests_ff_fields_param.snap deleted file mode 100644 index d37986d529..0000000000 --- a/pkg/github/__toolsnaps__/list_pull_requests_ff_fields_param.snap +++ /dev/null @@ -1,106 +0,0 @@ -{ - "annotations": { - "idempotentHint": false, - "readOnlyHint": true, - "title": "List pull requests" - }, - "description": "List pull requests in a GitHub repository. If the user specifies an author, then DO NOT use this tool and use the search_pull_requests tool instead.", - "inputSchema": { - "properties": { - "base": { - "description": "Filter by base branch", - "type": "string" - }, - "direction": { - "description": "Sort direction", - "enum": [ - "asc", - "desc" - ], - "type": "string" - }, - "fields": { - "description": "Subset of fields to return for each pull request. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body' in particular drops the largest per-result data.", - "items": { - "enum": [ - "number", - "title", - "body", - "state", - "draft", - "merged", - "mergeable_state", - "html_url", - "user", - "labels", - "assignees", - "requested_reviewers", - "merged_by", - "head", - "base", - "additions", - "deletions", - "changed_files", - "commits", - "comments", - "created_at", - "updated_at", - "closed_at", - "merged_at", - "milestone" - ], - "type": "string" - }, - "type": "array" - }, - "head": { - "description": "Filter by head user/org and branch", - "type": "string" - }, - "owner": { - "description": "Repository owner", - "type": "string" - }, - "page": { - "description": "Page number for pagination (min 1)", - "minimum": 1, - "type": "number" - }, - "perPage": { - "description": "Results per page for pagination (min 1, max 100)", - "maximum": 100, - "minimum": 1, - "type": "number" - }, - "repo": { - "description": "Repository name", - "type": "string" - }, - "sort": { - "description": "Sort by", - "enum": [ - "created", - "updated", - "popularity", - "long-running" - ], - "type": "string" - }, - "state": { - "description": "Filter by state", - "enum": [ - "open", - "closed", - "all" - ], - "type": "string" - } - }, - "required": [ - "owner", - "repo" - ], - "type": "object" - }, - "name": "list_pull_requests" -} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/list_releases.snap b/pkg/github/__toolsnaps__/list_releases.snap index d905f32087..4eeef279e9 100644 --- a/pkg/github/__toolsnaps__/list_releases.snap +++ b/pkg/github/__toolsnaps__/list_releases.snap @@ -7,6 +7,24 @@ "description": "List releases in a GitHub repository", "inputSchema": { "properties": { + "fields": { + "description": "Subset of fields to return for each release. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body' in particular drops the largest per-release data.", + "items": { + "enum": [ + "id", + "tag_name", + "name", + "body", + "html_url", + "published_at", + "prerelease", + "draft", + "author" + ], + "type": "string" + }, + "type": "array" + }, "owner": { "description": "Repository owner", "type": "string" diff --git a/pkg/github/__toolsnaps__/list_releases_ff_fields_param.snap b/pkg/github/__toolsnaps__/list_releases_ff_fields_param.snap deleted file mode 100644 index 4eeef279e9..0000000000 --- a/pkg/github/__toolsnaps__/list_releases_ff_fields_param.snap +++ /dev/null @@ -1,55 +0,0 @@ -{ - "annotations": { - "idempotentHint": false, - "readOnlyHint": true, - "title": "List releases" - }, - "description": "List releases in a GitHub repository", - "inputSchema": { - "properties": { - "fields": { - "description": "Subset of fields to return for each release. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body' in particular drops the largest per-release data.", - "items": { - "enum": [ - "id", - "tag_name", - "name", - "body", - "html_url", - "published_at", - "prerelease", - "draft", - "author" - ], - "type": "string" - }, - "type": "array" - }, - "owner": { - "description": "Repository owner", - "type": "string" - }, - "page": { - "description": "Page number for pagination (min 1)", - "minimum": 1, - "type": "number" - }, - "perPage": { - "description": "Results per page for pagination (min 1, max 100)", - "maximum": 100, - "minimum": 1, - "type": "number" - }, - "repo": { - "description": "Repository name", - "type": "string" - } - }, - "required": [ - "owner", - "repo" - ], - "type": "object" - }, - "name": "list_releases" -} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/search_code.snap b/pkg/github/__toolsnaps__/search_code.snap index 313c2f4c5f..00d4686712 100644 --- a/pkg/github/__toolsnaps__/search_code.snap +++ b/pkg/github/__toolsnaps__/search_code.snap @@ -7,6 +7,20 @@ "description": "Fast and precise code search across ALL GitHub repositories using GitHub's native search engine. Best for finding exact symbols, functions, classes, or specific code patterns.", "inputSchema": { "properties": { + "fields": { + "description": "Subset of fields to return for each code search result. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'repository' and 'text_matches' in particular drops the largest per-result data.", + "items": { + "enum": [ + "name", + "path", + "sha", + "repository", + "text_matches" + ], + "type": "string" + }, + "type": "array" + }, "order": { "description": "Sort order for results", "enum": [ diff --git a/pkg/github/__toolsnaps__/search_code_ff_fields_param.snap b/pkg/github/__toolsnaps__/search_code_ff_fields_param.snap deleted file mode 100644 index 00d4686712..0000000000 --- a/pkg/github/__toolsnaps__/search_code_ff_fields_param.snap +++ /dev/null @@ -1,58 +0,0 @@ -{ - "annotations": { - "idempotentHint": false, - "readOnlyHint": true, - "title": "Search code" - }, - "description": "Fast and precise code search across ALL GitHub repositories using GitHub's native search engine. Best for finding exact symbols, functions, classes, or specific code patterns.", - "inputSchema": { - "properties": { - "fields": { - "description": "Subset of fields to return for each code search result. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'repository' and 'text_matches' in particular drops the largest per-result data.", - "items": { - "enum": [ - "name", - "path", - "sha", - "repository", - "text_matches" - ], - "type": "string" - }, - "type": "array" - }, - "order": { - "description": "Sort order for results", - "enum": [ - "asc", - "desc" - ], - "type": "string" - }, - "page": { - "description": "Page number for pagination (min 1)", - "minimum": 1, - "type": "number" - }, - "perPage": { - "description": "Results per page for pagination (min 1, max 100)", - "maximum": 100, - "minimum": 1, - "type": "number" - }, - "query": { - "description": "Search query (GitHub code search REST). Implicit AND between terms; supports `OR`, `NOT`, and `\"quoted phrase\"` for exact match. Qualifiers: `repo:owner/repo`, `org:`, `user:`, `language:`, `path:dir` (prefix match), `filename:exact.ext`, `extension:`, `in:file`, `in:path`, `size:`, `is:archived`, `is:fork`. Max 256 chars. Examples: `WithContext language:go org:github`; `\"package main\" repo:o/r`; `func extension:go path:cmd repo:o/r`; `NOT TODO language:go repo:o/r`.", - "type": "string" - }, - "sort": { - "description": "Sort field ('indexed' only)", - "type": "string" - } - }, - "required": [ - "query" - ], - "type": "object" - }, - "name": "search_code" -} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/search_issues.snap b/pkg/github/__toolsnaps__/search_issues.snap index a2ec55b911..f705f14725 100644 --- a/pkg/github/__toolsnaps__/search_issues.snap +++ b/pkg/github/__toolsnaps__/search_issues.snap @@ -7,6 +7,39 @@ "description": "Search for issues in GitHub repositories using issues search syntax already scoped to is:issue", "inputSchema": { "properties": { + "fields": { + "description": "Subset of fields to return for each issue result. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body', 'reactions', and 'labels' in particular drops the largest per-result data.", + "items": { + "enum": [ + "number", + "title", + "body", + "state", + "state_reason", + "draft", + "locked", + "html_url", + "user", + "author_association", + "labels", + "assignee", + "assignees", + "milestone", + "comments", + "reactions", + "created_at", + "updated_at", + "closed_at", + "closed_by", + "type", + "repository_url", + "pull_request", + "field_values" + ], + "type": "string" + }, + "type": "array" + }, "order": { "description": "Sort order", "enum": [ diff --git a/pkg/github/__toolsnaps__/search_issues_ff_fields_param.snap b/pkg/github/__toolsnaps__/search_issues_ff_fields_param.snap deleted file mode 100644 index f705f14725..0000000000 --- a/pkg/github/__toolsnaps__/search_issues_ff_fields_param.snap +++ /dev/null @@ -1,98 +0,0 @@ -{ - "annotations": { - "idempotentHint": false, - "readOnlyHint": true, - "title": "Search issues" - }, - "description": "Search for issues in GitHub repositories using issues search syntax already scoped to is:issue", - "inputSchema": { - "properties": { - "fields": { - "description": "Subset of fields to return for each issue result. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body', 'reactions', and 'labels' in particular drops the largest per-result data.", - "items": { - "enum": [ - "number", - "title", - "body", - "state", - "state_reason", - "draft", - "locked", - "html_url", - "user", - "author_association", - "labels", - "assignee", - "assignees", - "milestone", - "comments", - "reactions", - "created_at", - "updated_at", - "closed_at", - "closed_by", - "type", - "repository_url", - "pull_request", - "field_values" - ], - "type": "string" - }, - "type": "array" - }, - "order": { - "description": "Sort order", - "enum": [ - "asc", - "desc" - ], - "type": "string" - }, - "owner": { - "description": "Optional repository owner. If provided with repo, only issues for this repository are listed.", - "type": "string" - }, - "page": { - "description": "Page number for pagination (min 1)", - "minimum": 1, - "type": "number" - }, - "perPage": { - "description": "Results per page for pagination (min 1, max 100)", - "maximum": 100, - "minimum": 1, - "type": "number" - }, - "query": { - "description": "Search query using GitHub issues search syntax", - "type": "string" - }, - "repo": { - "description": "Optional repository name. If provided with owner, only issues for this repository are listed.", - "type": "string" - }, - "sort": { - "description": "Sort field by number of matches of categories, defaults to best match", - "enum": [ - "comments", - "reactions", - "reactions-+1", - "reactions--1", - "reactions-smile", - "reactions-thinking_face", - "reactions-heart", - "reactions-tada", - "interactions", - "created", - "updated" - ], - "type": "string" - } - }, - "required": [ - "query" - ], - "type": "object" - }, - "name": "search_issues" -} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/search_pull_requests.snap b/pkg/github/__toolsnaps__/search_pull_requests.snap index 2e33af03b3..847168b471 100644 --- a/pkg/github/__toolsnaps__/search_pull_requests.snap +++ b/pkg/github/__toolsnaps__/search_pull_requests.snap @@ -7,6 +7,37 @@ "description": "Search for pull requests in GitHub repositories using issues search syntax already scoped to is:pr", "inputSchema": { "properties": { + "fields": { + "description": "Subset of fields to return for each pull request result. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body', 'reactions', and 'labels' in particular drops the largest per-result data.", + "items": { + "enum": [ + "number", + "title", + "body", + "state", + "state_reason", + "draft", + "locked", + "html_url", + "user", + "author_association", + "labels", + "assignee", + "assignees", + "milestone", + "comments", + "reactions", + "created_at", + "updated_at", + "closed_at", + "closed_by", + "pull_request", + "repository_url" + ], + "type": "string" + }, + "type": "array" + }, "order": { "description": "Sort order", "enum": [ diff --git a/pkg/github/__toolsnaps__/search_pull_requests_ff_fields_param.snap b/pkg/github/__toolsnaps__/search_pull_requests_ff_fields_param.snap deleted file mode 100644 index 847168b471..0000000000 --- a/pkg/github/__toolsnaps__/search_pull_requests_ff_fields_param.snap +++ /dev/null @@ -1,96 +0,0 @@ -{ - "annotations": { - "idempotentHint": false, - "readOnlyHint": true, - "title": "Search pull requests" - }, - "description": "Search for pull requests in GitHub repositories using issues search syntax already scoped to is:pr", - "inputSchema": { - "properties": { - "fields": { - "description": "Subset of fields to return for each pull request result. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body', 'reactions', and 'labels' in particular drops the largest per-result data.", - "items": { - "enum": [ - "number", - "title", - "body", - "state", - "state_reason", - "draft", - "locked", - "html_url", - "user", - "author_association", - "labels", - "assignee", - "assignees", - "milestone", - "comments", - "reactions", - "created_at", - "updated_at", - "closed_at", - "closed_by", - "pull_request", - "repository_url" - ], - "type": "string" - }, - "type": "array" - }, - "order": { - "description": "Sort order", - "enum": [ - "asc", - "desc" - ], - "type": "string" - }, - "owner": { - "description": "Optional repository owner. If provided with repo, only pull requests for this repository are listed.", - "type": "string" - }, - "page": { - "description": "Page number for pagination (min 1)", - "minimum": 1, - "type": "number" - }, - "perPage": { - "description": "Results per page for pagination (min 1, max 100)", - "maximum": 100, - "minimum": 1, - "type": "number" - }, - "query": { - "description": "Search query using GitHub pull request search syntax", - "type": "string" - }, - "repo": { - "description": "Optional repository name. If provided with owner, only pull requests for this repository are listed.", - "type": "string" - }, - "sort": { - "description": "Sort field by number of matches of categories, defaults to best match", - "enum": [ - "comments", - "reactions", - "reactions-+1", - "reactions--1", - "reactions-smile", - "reactions-thinking_face", - "reactions-heart", - "reactions-tada", - "interactions", - "created", - "updated" - ], - "type": "string" - } - }, - "required": [ - "query" - ], - "type": "object" - }, - "name": "search_pull_requests" -} \ No newline at end of file diff --git a/pkg/github/feature_flags.go b/pkg/github/feature_flags.go index b0652c3346..abf5de1e95 100644 --- a/pkg/github/feature_flags.go +++ b/pkg/github/feature_flags.go @@ -27,14 +27,6 @@ const FeatureFlagFileBlame = "file_blame" // unless explicitly opted in. const FeatureFlagIssueDependencies = "issue_dependencies" -// FeatureFlagFieldsParam is the feature flag name for the optional `fields` -// parameter on selected read tools (for example search_code and -// get_file_contents). When enabled, those tools advertise `fields` and filter -// each result to the requested subset, reducing response size. It is gated so -// the feature can be rolled out gradually and disabled as a kill switch without -// a redeploy. -const FeatureFlagFieldsParam = "fields_param" - // AllowedFeatureFlags is the allowlist of feature flags that can be enabled // by users via --features CLI flag or X-MCP-Features HTTP header. // Only flags in this list are accepted; unknown flags are silently ignored. @@ -48,7 +40,6 @@ var AllowedFeatureFlags = []string{ FeatureFlagPullRequestsGranular, FeatureFlagFileBlame, FeatureFlagIssueDependencies, - FeatureFlagFieldsParam, } // InsidersFeatureFlags is the list of feature flags that insiders mode enables. @@ -60,7 +51,6 @@ var InsidersFeatureFlags = []string{ FeatureFlagCSVOutput, FeatureFlagFileBlame, FeatureFlagIssueDependencies, - FeatureFlagFieldsParam, } // FeatureFlags defines runtime feature toggles that adjust tool behavior. diff --git a/pkg/github/feature_flags_test.go b/pkg/github/feature_flags_test.go index 30f2b56122..0b73ddeb3b 100644 --- a/pkg/github/feature_flags_test.go +++ b/pkg/github/feature_flags_test.go @@ -160,28 +160,12 @@ func TestResolveFeatureFlags(t *testing.T) { enabledFeatures: []string{MCPAppsDisableFormDeferralFeatureFlag}, expectedFlags: []string{MCPAppsDisableFormDeferralFeatureFlag}, }, - { - name: "fields param is not enabled by default", - enabledFeatures: nil, - unexpectedFlags: []string{FeatureFlagFieldsParam}, - }, - { - name: "fields param can be directly enabled", - enabledFeatures: []string{FeatureFlagFieldsParam}, - expectedFlags: []string{FeatureFlagFieldsParam}, - }, { name: "insiders mode enables insiders flags", enabledFeatures: nil, insidersMode: true, expectedFlags: InsidersFeatureFlags, }, - { - name: "insiders mode enables fields param", - enabledFeatures: nil, - insidersMode: true, - expectedFlags: []string{FeatureFlagFieldsParam}, - }, { name: "insiders mode does not auto-enable ifc labels", enabledFeatures: nil, diff --git a/pkg/github/fields_filtering_test.go b/pkg/github/fields_filtering_test.go index f421fe1ebb..c9dc5de0ee 100644 --- a/pkg/github/fields_filtering_test.go +++ b/pkg/github/fields_filtering_test.go @@ -7,11 +7,9 @@ import ( "testing" "github.com/github/github-mcp-server/internal/githubv4mock" - "github.com/github/github-mcp-server/internal/toolsnaps" "github.com/github/github-mcp-server/pkg/inventory" "github.com/github/github-mcp-server/pkg/translations" "github.com/google/go-github/v89/github" - "github.com/google/jsonschema-go/jsonschema" "github.com/shurcooL/githubv4" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -19,18 +17,6 @@ import ( // --- list_commits --------------------------------------------------------- -func Test_LegacyListCommits_Definition(t *testing.T) { - serverTool := LegacyListCommits(translations.NullTranslationHelper) - tool := serverTool.Tool - require.NoError(t, toolsnaps.Test(tool.Name, tool)) - require.Equal(t, []string{FeatureFlagFieldsParam}, serverTool.FeatureFlagDisable) - - assert.Equal(t, "list_commits", tool.Name) - schema, ok := tool.InputSchema.(*jsonschema.Schema) - require.True(t, ok, "InputSchema should be *jsonschema.Schema") - assert.NotContains(t, schema.Properties, "fields") -} - func mockListCommits() []*github.RepositoryCommit { return []*github.RepositoryCommit{ { @@ -88,18 +74,6 @@ func Test_ListCommits_FieldsTelemetry(t *testing.T) { // --- list_releases -------------------------------------------------------- -func Test_LegacyListReleases_Definition(t *testing.T) { - serverTool := LegacyListReleases(translations.NullTranslationHelper) - tool := serverTool.Tool - require.NoError(t, toolsnaps.Test(tool.Name, tool)) - require.Equal(t, []string{FeatureFlagFieldsParam}, serverTool.FeatureFlagDisable) - - assert.Equal(t, "list_releases", tool.Name) - schema, ok := tool.InputSchema.(*jsonschema.Schema) - require.True(t, ok, "InputSchema should be *jsonschema.Schema") - assert.NotContains(t, schema.Properties, "fields") -} - func mockListReleases() []*github.RepositoryRelease { return []*github.RepositoryRelease{ { @@ -153,18 +127,6 @@ func Test_ListReleases_FieldsTelemetry(t *testing.T) { // --- list_pull_requests --------------------------------------------------- -func Test_LegacyListPullRequests_Definition(t *testing.T) { - serverTool := LegacyListPullRequests(translations.NullTranslationHelper) - tool := serverTool.Tool - require.NoError(t, toolsnaps.Test(tool.Name, tool)) - require.Equal(t, []string{FeatureFlagFieldsParam}, serverTool.FeatureFlagDisable) - - assert.Equal(t, "list_pull_requests", tool.Name) - schema, ok := tool.InputSchema.(*jsonschema.Schema) - require.True(t, ok, "InputSchema should be *jsonschema.Schema") - assert.NotContains(t, schema.Properties, "fields") -} - func mockListPullRequests() []*github.PullRequest { return []*github.PullRequest{ { @@ -219,18 +181,6 @@ func Test_ListPullRequests_FieldsTelemetry(t *testing.T) { // --- search_pull_requests ------------------------------------------------- -func Test_LegacySearchPullRequests_Definition(t *testing.T) { - serverTool := LegacySearchPullRequests(translations.NullTranslationHelper) - tool := serverTool.Tool - require.NoError(t, toolsnaps.Test(tool.Name, tool)) - require.Equal(t, []string{FeatureFlagFieldsParam}, serverTool.FeatureFlagDisable) - - assert.Equal(t, "search_pull_requests", tool.Name) - schema, ok := tool.InputSchema.(*jsonschema.Schema) - require.True(t, ok, "InputSchema should be *jsonschema.Schema") - assert.NotContains(t, schema.Properties, "fields") -} - // mockIssueSearchResult returns a single-item issues search result. It is used // for both search_pull_requests and search_issues since both hit the REST // issues search endpoint. Issues intentionally omit NodeID so search_issues @@ -285,18 +235,6 @@ func Test_SearchPullRequests_FieldsTelemetry(t *testing.T) { // --- search_issues -------------------------------------------------------- -func Test_LegacySearchIssues_Definition(t *testing.T) { - serverTool := LegacySearchIssues(translations.NullTranslationHelper) - tool := serverTool.Tool - require.NoError(t, toolsnaps.Test(tool.Name, tool)) - require.Equal(t, []string{FeatureFlagFieldsParam}, serverTool.FeatureFlagDisable) - - assert.Equal(t, "search_issues", tool.Name) - schema, ok := tool.InputSchema.(*jsonschema.Schema) - require.True(t, ok, "InputSchema should be *jsonschema.Schema") - assert.NotContains(t, schema.Properties, "fields") -} - func Test_SearchIssues_FieldFiltering(t *testing.T) { serverTool := SearchIssues(translations.NullTranslationHelper) client := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ @@ -330,18 +268,6 @@ func Test_SearchIssues_FieldsTelemetry(t *testing.T) { // --- list_issues (GraphQL) ------------------------------------------------ -func Test_LegacyListIssues_Definition(t *testing.T) { - serverTool := LegacyListIssues(translations.NullTranslationHelper) - tool := serverTool.Tool - require.NoError(t, toolsnaps.Test(tool.Name, tool)) - require.Equal(t, []string{FeatureFlagFieldsParam}, serverTool.FeatureFlagDisable) - - assert.Equal(t, "list_issues", tool.Name) - schema, ok := tool.InputSchema.(*jsonschema.Schema) - require.True(t, ok, "InputSchema should be *jsonschema.Schema") - assert.NotContains(t, schema.Properties, "fields") -} - // listIssuesFieldsQuery and listIssuesFieldsVars mirror the exact GraphQL query // and variables list_issues issues for owner/repo with default parameters (no // labels, no since). They must stay in sync with the query built in diff --git a/pkg/github/fields_param_gating_test.go b/pkg/github/fields_param_gating_test.go deleted file mode 100644 index 1e61d4eb81..0000000000 --- a/pkg/github/fields_param_gating_test.go +++ /dev/null @@ -1,84 +0,0 @@ -package github - -import ( - "context" - "testing" - - "github.com/github/github-mcp-server/pkg/translations" - "github.com/google/jsonschema-go/jsonschema" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -// Test_FieldsParamVariants_MutuallyExclusive guards the dual-variant -// registration for the fields_param feature flag. The flag-enabled tools and -// their Legacy* counterparts share a tool name, so exactly one of each pair must -// survive inventory filtering for any flag state. If both ever leaked, a client -// could be offered two tools with the same name. This asserts that each gated -// tool is present exactly once, advertising the `fields` parameter only when -// fields_param is enabled. -func Test_FieldsParamVariants_MutuallyExclusive(t *testing.T) { - gatedTools := []string{ - "search_code", - "get_file_contents", - "list_issues", - "list_releases", - "list_pull_requests", - "search_issues", - "search_pull_requests", - "list_commits", - } - - for _, tc := range []struct { - name string - flagEnabled bool - expectFields bool - featureChecks func(context.Context, string) (bool, error) - }{ - { - name: "flag off registers the legacy variant without fields", - flagEnabled: false, - expectFields: false, - featureChecks: featureCheckerFor(), // fields_param disabled - }, - { - name: "flag on registers the fields variant with fields", - flagEnabled: true, - expectFields: true, - featureChecks: featureCheckerFor(FeatureFlagFieldsParam), - }, - } { - t.Run(tc.name, func(t *testing.T) { - inv, err := NewInventory(translations.NullTranslationHelper). - WithToolsets([]string{"all"}). - WithFeatureChecker(tc.featureChecks). - Build() - require.NoError(t, err) - - available := inv.AvailableTools(context.Background()) - - counts := make(map[string]int, len(available)) - for _, tool := range available { - counts[tool.Tool.Name]++ - } - - // Each gated tool must be present exactly once (never both variants) - // and advertise `fields` only when the flag is enabled. - for _, name := range gatedTools { - require.Equalf(t, 1, counts[name], "expected exactly one %q for flagEnabled=%v; dual variants must be mutually exclusive", name, tc.flagEnabled) - - tool := requireToolByName(t, available, name) - schema, ok := tool.Tool.InputSchema.(*jsonschema.Schema) - require.Truef(t, ok, "%q InputSchema should be *jsonschema.Schema", name) - - if tc.expectFields { - assert.Containsf(t, schema.Properties, "fields", "%q should advertise fields when flag is on", name) - assert.Equalf(t, FeatureFlagFieldsParam, tool.FeatureFlagEnable, "%q should be the flag-enabled variant", name) - } else { - assert.NotContainsf(t, schema.Properties, "fields", "%q must not advertise fields when flag is off", name) - assert.Containsf(t, tool.FeatureFlagDisable, FeatureFlagFieldsParam, "%q should be the legacy (flag-disabled) variant", name) - } - } - }) - } -} diff --git a/pkg/github/issues.go b/pkg/github/issues.go index ad6c54f262..083c5465e0 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -1595,34 +1595,8 @@ func ReprioritizeSubIssue(ctx context.Context, client *github.Client, owner stri return utils.NewToolResultText(string(r)), nil } -// SearchIssues creates a tool to search for issues. It is the -// FeatureFlagFieldsParam-enabled variant: it advertises the optional `fields` -// parameter and filters each result to the requested subset. Both this and -// LegacySearchIssues register under the tool name "search_issues"; exactly one is -// active for any given request thanks to mutually exclusive FeatureFlagEnable / -// FeatureFlagDisable annotations. +// SearchIssues creates a tool to search for issues. func SearchIssues(t translations.TranslationHelperFunc) inventory.ServerTool { - st := searchIssuesTool(t, true) - st.FeatureFlagEnable = FeatureFlagFieldsParam - return st -} - -// LegacySearchIssues is the FeatureFlagFieldsParam-disabled variant of -// search_issues. It exposes the original schema (no `fields` parameter) and never -// filters results, so it acts as the kill switch when the flag is off. It owns -// the canonical search_issues.snap; the flag-enabled variant owns -// search_issues_ff_.snap. Delete this function when the flag is removed. -func LegacySearchIssues(t translations.TranslationHelperFunc) inventory.ServerTool { - st := searchIssuesTool(t, false) - st.FeatureFlagDisable = []string{FeatureFlagFieldsParam} - return st -} - -// searchIssuesTool builds the search_issues tool. When includeFields is true the -// tool advertises the optional `fields` parameter, filters each result to the -// requested subset, and emits fields telemetry. When false it is the original -// tool with no fields parameter and no filtering. -func searchIssuesTool(t translations.TranslationHelperFunc, includeFields bool) inventory.ServerTool { schema := &jsonschema.Schema{ Type: "object", Properties: map[string]*jsonschema.Schema{ @@ -1663,12 +1637,10 @@ func searchIssuesTool(t translations.TranslationHelperFunc, includeFields bool) }, Required: []string{"query"}, } - if includeFields { - schema.Properties["fields"] = fieldsSchemaProperty( - "Subset of fields to return for each issue result. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body', 'reactions', and 'labels' in particular drops the largest per-result data.", - searchIssuesItemFieldEnum, - ) - } + schema.Properties["fields"] = fieldsSchemaProperty( + "Subset of fields to return for each issue result. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body', 'reactions', and 'labels' in particular drops the largest per-result data.", + searchIssuesItemFieldEnum, + ) WithPagination(schema) return NewTool( @@ -1685,13 +1657,11 @@ func searchIssuesTool(t translations.TranslationHelperFunc, includeFields bool) []scopes.Scope{scopes.Repo}, func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { options := []searchOption{ifcSearchPostProcessOption(ctx, deps)} - if includeFields { - fields, err := OptionalStringArrayParam(args, "fields") - if err != nil { - return utils.NewToolResultError(err.Error()), nil, nil - } - options = append(options, withFieldsFiltering(deps, "search_issues", fields)) + fields, err := OptionalStringArrayParam(args, "fields") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil } + options = append(options, withFieldsFiltering(deps, "search_issues", fields)) result, err := searchIssuesHandler(ctx, deps, args, options...) return result, nil, err }) @@ -2652,34 +2622,8 @@ func UpdateIssue(ctx context.Context, client *github.Client, gqlClient *githubv4 return utils.NewToolResultText(string(r)), nil } -// ListIssues creates a tool to list issues in a GitHub repository. It is the -// FeatureFlagFieldsParam-enabled variant: it advertises the optional `fields` -// parameter and filters each issue to the requested subset. Both this and -// LegacyListIssues register under the tool name "list_issues"; exactly one is -// active for any given request thanks to mutually exclusive FeatureFlagEnable / -// FeatureFlagDisable annotations. +// ListIssues creates a tool to list issues in a GitHub repository. func ListIssues(t translations.TranslationHelperFunc) inventory.ServerTool { - st := listIssuesTool(t, true) - st.FeatureFlagEnable = FeatureFlagFieldsParam - return st -} - -// LegacyListIssues is the FeatureFlagFieldsParam-disabled variant of list_issues. -// It exposes the original schema (no `fields` parameter) and never filters -// results, so it acts as the kill switch when the flag is off. It owns the -// canonical list_issues.snap; the flag-enabled variant owns -// list_issues_ff_.snap. Delete this function when the flag is removed. -func LegacyListIssues(t translations.TranslationHelperFunc) inventory.ServerTool { - st := listIssuesTool(t, false) - st.FeatureFlagDisable = []string{FeatureFlagFieldsParam} - return st -} - -// listIssuesTool builds the list_issues tool. When includeFields is true the -// tool advertises the optional `fields` parameter, filters each issue to the -// requested subset, and emits fields telemetry. When false it is the original -// tool with no fields parameter and no filtering. -func listIssuesTool(t translations.TranslationHelperFunc, includeFields bool) inventory.ServerTool { schema := &jsonschema.Schema{ Type: "object", Properties: map[string]*jsonschema.Schema{ @@ -2738,12 +2682,10 @@ func listIssuesTool(t translations.TranslationHelperFunc, includeFields bool) in }, Required: []string{"owner", "repo"}, } - if includeFields { - schema.Properties["fields"] = fieldsSchemaProperty( - "Subset of fields to return for each issue. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body' and 'field_values' in particular drops the largest per-result data.", - listIssuesItemFieldEnum, - ) - } + schema.Properties["fields"] = fieldsSchemaProperty( + "Subset of fields to return for each issue. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body' and 'field_values' in particular drops the largest per-result data.", + listIssuesItemFieldEnum, + ) WithCursorPagination(schema) st := NewTool( @@ -2768,12 +2710,9 @@ func listIssuesTool(t translations.TranslationHelperFunc, includeFields bool) in return utils.NewToolResultError(err.Error()), nil, nil } - var fields []string - if includeFields { - fields, err = OptionalStringArrayParam(args, "fields") - if err != nil { - return utils.NewToolResultError(err.Error()), nil, nil - } + fields, err := OptionalStringArrayParam(args, "fields") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil } // Set optional parameters if provided @@ -2947,7 +2886,7 @@ func listIssuesTool(t translations.TranslationHelperFunc, includeFields bool) in filtered := false var payload any = resp - if includeFields && len(fields) > 0 { + if len(fields) > 0 { filteredIssues, err := filterEachField(resp.Issues, fields) if err != nil { return utils.NewToolResultErrorFromErr("failed to filter issues", err), nil, nil @@ -2965,9 +2904,7 @@ func listIssuesTool(t translations.TranslationHelperFunc, includeFields bool) in return utils.NewToolResultErrorFromErr("failed to marshal response", err), nil, nil } - if includeFields { - recordFieldsUsageFor(ctx, deps, "list_issues", resp, filtered, len(r)) - } + recordFieldsUsageFor(ctx, deps, "list_issues", resp, filtered, len(r)) result := utils.NewToolResultText(string(r)) result = attachStaticIFCLabel(ctx, deps, result, ifc.LabelListIssues(isPrivate)) diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index 3af8e4532a..3e0974862e 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -812,11 +812,7 @@ func Test_SearchIssues(t *testing.T) { // Verify tool definition once serverTool := SearchIssues(translations.NullTranslationHelper) tool := serverTool.Tool - // SearchIssues is the FeatureFlagFieldsParam-enabled variant; it owns the - // _ff_ snapshot. The canonical search_issues.snap is owned by - // LegacySearchIssues (see Test_LegacySearchIssues_Definition). - require.NoError(t, toolsnaps.Test(tool.Name+"_ff_"+FeatureFlagFieldsParam, tool)) - require.Equal(t, FeatureFlagFieldsParam, serverTool.FeatureFlagEnable) + require.NoError(t, toolsnaps.Test(tool.Name, tool)) assert.Equal(t, "search_issues", tool.Name) assert.NotEmpty(t, tool.Description) @@ -1895,11 +1891,7 @@ func Test_ListIssues(t *testing.T) { // Verify tool definition serverTool := ListIssues(translations.NullTranslationHelper) tool := serverTool.Tool - // ListIssues is the FeatureFlagFieldsParam-enabled variant; it owns the - // _ff_ snapshot. The canonical list_issues.snap is owned by - // LegacyListIssues (see Test_LegacyListIssues_Definition). - require.NoError(t, toolsnaps.Test(tool.Name+"_ff_"+FeatureFlagFieldsParam, tool)) - require.Equal(t, FeatureFlagFieldsParam, serverTool.FeatureFlagEnable) + require.NoError(t, toolsnaps.Test(tool.Name, tool)) assert.Equal(t, "list_issues", tool.Name) assert.NotEmpty(t, tool.Description) diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index b1cfa00945..9825ba8845 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -1322,34 +1322,7 @@ func AddReplyToPullRequestComment(t translations.TranslationHelperFunc) inventor } // ListPullRequests creates a tool to list pull requests in a GitHub repository. -// It is the FeatureFlagFieldsParam-enabled variant: it advertises the optional -// `fields` parameter and filters each pull request to the requested subset. Both -// this and LegacyListPullRequests register under the tool name -// "list_pull_requests"; exactly one is active for any given request thanks to -// mutually exclusive FeatureFlagEnable / FeatureFlagDisable annotations. func ListPullRequests(t translations.TranslationHelperFunc) inventory.ServerTool { - st := listPullRequestsTool(t, true) - st.FeatureFlagEnable = FeatureFlagFieldsParam - return st -} - -// LegacyListPullRequests is the FeatureFlagFieldsParam-disabled variant of -// list_pull_requests. It exposes the original schema (no `fields` parameter) and -// never filters results, so it acts as the kill switch when the flag is off. It -// owns the canonical list_pull_requests.snap; the flag-enabled variant owns -// list_pull_requests_ff_.snap. Delete this function when the flag is -// removed. -func LegacyListPullRequests(t translations.TranslationHelperFunc) inventory.ServerTool { - st := listPullRequestsTool(t, false) - st.FeatureFlagDisable = []string{FeatureFlagFieldsParam} - return st -} - -// listPullRequestsTool builds the list_pull_requests tool. When includeFields is -// true the tool advertises the optional `fields` parameter, filters each pull -// request to the requested subset, and emits fields telemetry. When false it is -// the original tool with no fields parameter and no filtering. -func listPullRequestsTool(t translations.TranslationHelperFunc, includeFields bool) inventory.ServerTool { schema := &jsonschema.Schema{ Type: "object", Properties: map[string]*jsonschema.Schema{ @@ -1387,12 +1360,10 @@ func listPullRequestsTool(t translations.TranslationHelperFunc, includeFields bo }, Required: []string{"owner", "repo"}, } - if includeFields { - schema.Properties["fields"] = fieldsSchemaProperty( - "Subset of fields to return for each pull request. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body' in particular drops the largest per-result data.", - listPullRequestsItemFieldEnum, - ) - } + schema.Properties["fields"] = fieldsSchemaProperty( + "Subset of fields to return for each pull request. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body' in particular drops the largest per-result data.", + listPullRequestsItemFieldEnum, + ) WithPagination(schema) return NewTool( @@ -1436,12 +1407,9 @@ func listPullRequestsTool(t translations.TranslationHelperFunc, includeFields bo if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } - var fields []string - if includeFields { - fields, err = OptionalStringArrayParam(args, "fields") - if err != nil { - return utils.NewToolResultError(err.Error()), nil, nil - } + fields, err := OptionalStringArrayParam(args, "fields") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil } pagination, err := OptionalPaginationParams(args) if err != nil { @@ -1504,7 +1472,7 @@ func listPullRequestsTool(t translations.TranslationHelperFunc, includeFields bo filtered := false var payload any = minimalPRs - if includeFields && len(fields) > 0 { + if len(fields) > 0 { filteredPRs, err := filterEachField(minimalPRs, fields) if err != nil { return utils.NewToolResultErrorFromErr("failed to filter pull requests", err), nil, nil @@ -1518,9 +1486,7 @@ func listPullRequestsTool(t translations.TranslationHelperFunc, includeFields bo return utils.NewToolResultErrorFromErr("failed to marshal response", err), nil, nil } - if includeFields { - recordFieldsUsageFor(ctx, deps, "list_pull_requests", minimalPRs, filtered, len(r)) - } + recordFieldsUsageFor(ctx, deps, "list_pull_requests", minimalPRs, filtered, len(r)) result := utils.NewToolResultText(string(r)) // Pull request titles/bodies are user-authored (untrusted); @@ -1639,35 +1605,8 @@ func MergePullRequest(t translations.TranslationHelperFunc) inventory.ServerTool }) } -// SearchPullRequests creates a tool to search for pull requests. It is the -// FeatureFlagFieldsParam-enabled variant: it advertises the optional `fields` -// parameter and filters each result to the requested subset. Both this and -// LegacySearchPullRequests register under the tool name "search_pull_requests"; -// exactly one is active for any given request thanks to mutually exclusive -// FeatureFlagEnable / FeatureFlagDisable annotations. +// SearchPullRequests creates a tool to search for pull requests. func SearchPullRequests(t translations.TranslationHelperFunc) inventory.ServerTool { - st := searchPullRequestsTool(t, true) - st.FeatureFlagEnable = FeatureFlagFieldsParam - return st -} - -// LegacySearchPullRequests is the FeatureFlagFieldsParam-disabled variant of -// search_pull_requests. It exposes the original schema (no `fields` parameter) -// and never filters results, so it acts as the kill switch when the flag is off. -// It owns the canonical search_pull_requests.snap; the flag-enabled variant owns -// search_pull_requests_ff_.snap. Delete this function when the flag is -// removed. -func LegacySearchPullRequests(t translations.TranslationHelperFunc) inventory.ServerTool { - st := searchPullRequestsTool(t, false) - st.FeatureFlagDisable = []string{FeatureFlagFieldsParam} - return st -} - -// searchPullRequestsTool builds the search_pull_requests tool. When -// includeFields is true the tool advertises the optional `fields` parameter, -// filters each result to the requested subset, and emits fields telemetry. When -// false it is the original tool with no fields parameter and no filtering. -func searchPullRequestsTool(t translations.TranslationHelperFunc, includeFields bool) inventory.ServerTool { schema := &jsonschema.Schema{ Type: "object", Properties: map[string]*jsonschema.Schema{ @@ -1708,12 +1647,10 @@ func searchPullRequestsTool(t translations.TranslationHelperFunc, includeFields }, Required: []string{"query"}, } - if includeFields { - schema.Properties["fields"] = fieldsSchemaProperty( - "Subset of fields to return for each pull request result. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body', 'reactions', and 'labels' in particular drops the largest per-result data.", - searchPullRequestsItemFieldEnum, - ) - } + schema.Properties["fields"] = fieldsSchemaProperty( + "Subset of fields to return for each pull request result. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body', 'reactions', and 'labels' in particular drops the largest per-result data.", + searchPullRequestsItemFieldEnum, + ) WithPagination(schema) return NewTool( @@ -1730,13 +1667,11 @@ func searchPullRequestsTool(t translations.TranslationHelperFunc, includeFields []scopes.Scope{scopes.Repo}, func(ctx context.Context, deps ToolDependencies, _ *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { options := []searchOption{ifcSearchPostProcessOption(ctx, deps)} - if includeFields { - fields, err := OptionalStringArrayParam(args, "fields") - if err != nil { - return utils.NewToolResultError(err.Error()), nil, nil - } - options = append(options, withFieldsFiltering(deps, "search_pull_requests", fields)) + fields, err := OptionalStringArrayParam(args, "fields") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil } + options = append(options, withFieldsFiltering(deps, "search_pull_requests", fields)) result, err := searchHandler(ctx, deps.GetClient, args, "pr", "failed to search pull requests", options...) return result, nil, err }) diff --git a/pkg/github/pullrequests_test.go b/pkg/github/pullrequests_test.go index ace47c666b..5fe1229dba 100644 --- a/pkg/github/pullrequests_test.go +++ b/pkg/github/pullrequests_test.go @@ -615,11 +615,7 @@ func Test_ListPullRequests(t *testing.T) { // Verify tool definition once serverTool := ListPullRequests(translations.NullTranslationHelper) tool := serverTool.Tool - // ListPullRequests is the FeatureFlagFieldsParam-enabled variant; it owns - // the _ff_ snapshot. The canonical list_pull_requests.snap is owned by - // LegacyListPullRequests (see Test_LegacyListPullRequests_Definition). - require.NoError(t, toolsnaps.Test(tool.Name+"_ff_"+FeatureFlagFieldsParam, tool)) - require.Equal(t, FeatureFlagFieldsParam, serverTool.FeatureFlagEnable) + require.NoError(t, toolsnaps.Test(tool.Name, tool)) assert.Equal(t, "list_pull_requests", tool.Name) assert.NotEmpty(t, tool.Description) @@ -866,11 +862,7 @@ func Test_MergePullRequest(t *testing.T) { func Test_SearchPullRequests(t *testing.T) { serverTool := SearchPullRequests(translations.NullTranslationHelper) tool := serverTool.Tool - // SearchPullRequests is the FeatureFlagFieldsParam-enabled variant; it owns - // the _ff_ snapshot. The canonical search_pull_requests.snap is owned - // by LegacySearchPullRequests (see Test_LegacySearchPullRequests_Definition). - require.NoError(t, toolsnaps.Test(tool.Name+"_ff_"+FeatureFlagFieldsParam, tool)) - require.Equal(t, FeatureFlagFieldsParam, serverTool.FeatureFlagEnable) + require.NoError(t, toolsnaps.Test(tool.Name, tool)) assert.Equal(t, "search_pull_requests", tool.Name) assert.NotEmpty(t, tool.Description) diff --git a/pkg/github/repositories.go b/pkg/github/repositories.go index aa236cd53a..be7b76edda 100644 --- a/pkg/github/repositories.go +++ b/pkg/github/repositories.go @@ -133,33 +133,8 @@ func GetCommit(t translations.TranslationHelperFunc) inventory.ServerTool { } // ListCommits creates a tool to get the list of commits of a branch in a GitHub -// repository. It is the FeatureFlagFieldsParam-enabled variant: it advertises -// the optional `fields` parameter and filters each commit to the requested -// subset. Both this and LegacyListCommits register under the tool name -// "list_commits"; exactly one is active for any given request thanks to mutually -// exclusive FeatureFlagEnable / FeatureFlagDisable annotations. +// repository. func ListCommits(t translations.TranslationHelperFunc) inventory.ServerTool { - st := listCommitsTool(t, true) - st.FeatureFlagEnable = FeatureFlagFieldsParam - return st -} - -// LegacyListCommits is the FeatureFlagFieldsParam-disabled variant of -// list_commits. It exposes the original schema (no `fields` parameter) and never -// filters results, so it acts as the kill switch when the flag is off. It owns -// the canonical list_commits.snap; the flag-enabled variant owns -// list_commits_ff_.snap. Delete this function when the flag is removed. -func LegacyListCommits(t translations.TranslationHelperFunc) inventory.ServerTool { - st := listCommitsTool(t, false) - st.FeatureFlagDisable = []string{FeatureFlagFieldsParam} - return st -} - -// listCommitsTool builds the list_commits tool. When includeFields is true the -// tool advertises the optional `fields` parameter, filters each commit to the -// requested subset, and emits fields telemetry. When false it is the original -// tool with no fields parameter and no filtering. -func listCommitsTool(t translations.TranslationHelperFunc, includeFields bool) inventory.ServerTool { schema := &jsonschema.Schema{ Type: "object", Properties: map[string]*jsonschema.Schema{ @@ -194,12 +169,10 @@ func listCommitsTool(t translations.TranslationHelperFunc, includeFields bool) i }, Required: []string{"owner", "repo"}, } - if includeFields { - schema.Properties["fields"] = fieldsSchemaProperty( - "Subset of fields to return for each commit. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields, e.g. just 'sha' and 'html_url'.", - listCommitsItemFieldEnum, - ) - } + schema.Properties["fields"] = fieldsSchemaProperty( + "Subset of fields to return for each commit. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields, e.g. just 'sha' and 'html_url'.", + listCommitsItemFieldEnum, + ) WithPagination(schema) return NewTool( @@ -235,12 +208,9 @@ func listCommitsTool(t translations.TranslationHelperFunc, includeFields bool) i if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } - var fields []string - if includeFields { - fields, err = OptionalStringArrayParam(args, "fields") - if err != nil { - return utils.NewToolResultError(err.Error()), nil, nil - } + fields, err := OptionalStringArrayParam(args, "fields") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil } sinceStr, err := OptionalParam[string](args, "since") if err != nil { @@ -313,7 +283,7 @@ func listCommitsTool(t translations.TranslationHelperFunc, includeFields bool) i filtered := false var payload any = minimalCommits - if includeFields && len(fields) > 0 { + if len(fields) > 0 { filteredCommits, err := filterEachField(minimalCommits, fields) if err != nil { return utils.NewToolResultErrorFromErr("failed to filter commits", err), nil, nil @@ -327,9 +297,7 @@ func listCommitsTool(t translations.TranslationHelperFunc, includeFields bool) i return nil, nil, fmt.Errorf("failed to marshal response: %w", err) } - if includeFields { - recordFieldsUsageFor(ctx, deps, "list_commits", minimalCommits, filtered, len(r)) - } + recordFieldsUsageFor(ctx, deps, "list_commits", minimalCommits, filtered, len(r)) result := utils.NewToolResultText(string(r)) // Commit content is reachable from the repo's history; integrity @@ -751,34 +719,8 @@ func FetchRepoIsPrivate(ctx context.Context, client *github.Client, owner, repo } // GetFileContents creates a tool to get the contents of a file or directory from -// a GitHub repository. It is the FeatureFlagFieldsParam-enabled variant: it -// advertises the optional `fields` parameter and filters directory listings to -// the requested subset. Both this and LegacyGetFileContents register under the -// tool name "get_file_contents"; exactly one is active for any given request -// thanks to mutually exclusive FeatureFlagEnable / FeatureFlagDisable annotations. +// a GitHub repository. func GetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool { - st := getFileContentsTool(t, true) - st.FeatureFlagEnable = FeatureFlagFieldsParam - return st -} - -// LegacyGetFileContents is the FeatureFlagFieldsParam-disabled variant of -// get_file_contents. It exposes the original schema (no `fields` parameter) and -// never filters directory listings, so it acts as the kill switch when the flag -// is off. It owns the canonical get_file_contents.snap; the flag-enabled variant -// owns get_file_contents_ff_.snap. Delete this function when the flag is -// removed. -func LegacyGetFileContents(t translations.TranslationHelperFunc) inventory.ServerTool { - st := getFileContentsTool(t, false) - st.FeatureFlagDisable = []string{FeatureFlagFieldsParam} - return st -} - -// getFileContentsTool builds the get_file_contents tool. When includeFields is -// true the tool advertises the optional `fields` parameter, filters directory -// listings to the requested subset, and emits fields telemetry. When false it is -// the original tool with no fields parameter and no filtering. -func getFileContentsTool(t translations.TranslationHelperFunc, includeFields bool) inventory.ServerTool { schema := &jsonschema.Schema{ Type: "object", Properties: map[string]*jsonschema.Schema{ @@ -806,12 +748,10 @@ func getFileContentsTool(t translations.TranslationHelperFunc, includeFields boo }, Required: []string{"owner", "repo"}, } - if includeFields { - schema.Properties["fields"] = fieldsSchemaProperty( - "Subset of fields to return for each entry when the path is a directory. If omitted, all fields are returned. Ignored when the path is a single file. Use this to reduce response size when listing directories and you only need specific fields, e.g. just 'name' and 'type'.", - fileContentFieldEnum, - ) - } + schema.Properties["fields"] = fieldsSchemaProperty( + "Subset of fields to return for each entry when the path is a directory. If omitted, all fields are returned. Ignored when the path is a single file. Use this to reduce response size when listing directories and you only need specific fields, e.g. just 'name' and 'type'.", + fileContentFieldEnum, + ) return NewTool( ToolsetMetadataRepos, @@ -852,12 +792,9 @@ func getFileContentsTool(t translations.TranslationHelperFunc, includeFields boo return utils.NewToolResultError(err.Error()), nil, nil } - var fields []string - if includeFields { - fields, err = OptionalStringArrayParam(args, "fields") - if err != nil { - return utils.NewToolResultError(err.Error()), nil, nil - } + fields, err := OptionalStringArrayParam(args, "fields") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil } client, err := deps.GetClient(ctx) @@ -985,7 +922,7 @@ func getFileContentsTool(t translations.TranslationHelperFunc, includeFields boo // file content or file SHA is nil which means it's a directory filtered := false var payload any = dirContent - if includeFields && len(fields) > 0 { + if len(fields) > 0 { filteredEntries, err := filterEachField(dirContent, fields) if err != nil { return utils.NewToolResultErrorFromErr("failed to filter directory contents", err), nil, nil @@ -997,9 +934,7 @@ func getFileContentsTool(t translations.TranslationHelperFunc, includeFields boo if err != nil { return utils.NewToolResultError("failed to marshal response"), nil, nil } - if includeFields { - recordDirContentsFieldsUsage(ctx, deps, dirContent, filtered, len(r)) - } + recordDirContentsFieldsUsage(ctx, deps, dirContent, filtered, len(r)) return attachIFC(utils.NewToolResultText(string(r))), nil, nil } @@ -1850,34 +1785,8 @@ func GetTag(t translations.TranslationHelperFunc) inventory.ServerTool { ) } -// ListReleases creates a tool to list releases in a GitHub repository. It is the -// FeatureFlagFieldsParam-enabled variant: it advertises the optional `fields` -// parameter and filters each release to the requested subset. Both this and -// LegacyListReleases register under the tool name "list_releases"; exactly one is -// active for any given request thanks to mutually exclusive FeatureFlagEnable / -// FeatureFlagDisable annotations. +// ListReleases creates a tool to list releases in a GitHub repository. func ListReleases(t translations.TranslationHelperFunc) inventory.ServerTool { - st := listReleasesTool(t, true) - st.FeatureFlagEnable = FeatureFlagFieldsParam - return st -} - -// LegacyListReleases is the FeatureFlagFieldsParam-disabled variant of -// list_releases. It exposes the original schema (no `fields` parameter) and never -// filters results, so it acts as the kill switch when the flag is off. It owns -// the canonical list_releases.snap; the flag-enabled variant owns -// list_releases_ff_.snap. Delete this function when the flag is removed. -func LegacyListReleases(t translations.TranslationHelperFunc) inventory.ServerTool { - st := listReleasesTool(t, false) - st.FeatureFlagDisable = []string{FeatureFlagFieldsParam} - return st -} - -// listReleasesTool builds the list_releases tool. When includeFields is true the -// tool advertises the optional `fields` parameter, filters each release to the -// requested subset, and emits fields telemetry. When false it is the original -// tool with no fields parameter and no filtering. -func listReleasesTool(t translations.TranslationHelperFunc, includeFields bool) inventory.ServerTool { schema := &jsonschema.Schema{ Type: "object", Properties: map[string]*jsonschema.Schema{ @@ -1892,12 +1801,10 @@ func listReleasesTool(t translations.TranslationHelperFunc, includeFields bool) }, Required: []string{"owner", "repo"}, } - if includeFields { - schema.Properties["fields"] = fieldsSchemaProperty( - "Subset of fields to return for each release. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body' in particular drops the largest per-release data.", - listReleasesItemFieldEnum, - ) - } + schema.Properties["fields"] = fieldsSchemaProperty( + "Subset of fields to return for each release. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'body' in particular drops the largest per-release data.", + listReleasesItemFieldEnum, + ) WithPagination(schema) return NewTool( @@ -1921,12 +1828,9 @@ func listReleasesTool(t translations.TranslationHelperFunc, includeFields bool) if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } - var fields []string - if includeFields { - fields, err = OptionalStringArrayParam(args, "fields") - if err != nil { - return utils.NewToolResultError(err.Error()), nil, nil - } + fields, err := OptionalStringArrayParam(args, "fields") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil } pagination, err := OptionalPaginationParams(args) if err != nil { @@ -1966,7 +1870,7 @@ func listReleasesTool(t translations.TranslationHelperFunc, includeFields bool) filtered := false var payload any = minimalReleases - if includeFields && len(fields) > 0 { + if len(fields) > 0 { filteredReleases, err := filterEachField(minimalReleases, fields) if err != nil { return utils.NewToolResultErrorFromErr("failed to filter releases", err), nil, nil @@ -1980,9 +1884,7 @@ func listReleasesTool(t translations.TranslationHelperFunc, includeFields bool) return nil, nil, fmt.Errorf("failed to marshal response: %w", err) } - if includeFields { - recordFieldsUsageFor(ctx, deps, "list_releases", minimalReleases, filtered, len(r)) - } + recordFieldsUsageFor(ctx, deps, "list_releases", minimalReleases, filtered, len(r)) result := utils.NewToolResultText(string(r)) // Releases are published by collaborators with push access, so diff --git a/pkg/github/repositories_test.go b/pkg/github/repositories_test.go index fcc5fa0634..332b212a17 100644 --- a/pkg/github/repositories_test.go +++ b/pkg/github/repositories_test.go @@ -27,11 +27,7 @@ func Test_GetFileContents(t *testing.T) { // Verify tool definition once serverTool := GetFileContents(translations.NullTranslationHelper) tool := serverTool.Tool - // GetFileContents is the FeatureFlagFieldsParam-enabled variant; it owns the - // _ff_ snapshot. The canonical get_file_contents.snap is owned by - // LegacyGetFileContents (see Test_LegacyGetFileContents_Definition). - require.NoError(t, toolsnaps.Test(tool.Name+"_ff_"+FeatureFlagFieldsParam, tool)) - require.Equal(t, FeatureFlagFieldsParam, serverTool.FeatureFlagEnable) + require.NoError(t, toolsnaps.Test(tool.Name, tool)) schema, ok := tool.InputSchema.(*jsonschema.Schema) require.True(t, ok, "InputSchema should be *jsonschema.Schema") @@ -548,20 +544,6 @@ func Test_GetFileContents_DirectoryFieldFiltering(t *testing.T) { assert.NotContains(t, textContent.Text, "download_url") } -func Test_LegacyGetFileContents_Definition(t *testing.T) { - serverTool := LegacyGetFileContents(translations.NullTranslationHelper) - tool := serverTool.Tool - // LegacyGetFileContents is the FeatureFlagFieldsParam-disabled variant and - // owns the canonical get_file_contents.snap (no `fields`). - require.NoError(t, toolsnaps.Test(tool.Name, tool)) - require.Equal(t, []string{FeatureFlagFieldsParam}, serverTool.FeatureFlagDisable) - - assert.Equal(t, "get_file_contents", tool.Name) - schema, ok := tool.InputSchema.(*jsonschema.Schema) - require.True(t, ok, "InputSchema should be *jsonschema.Schema") - assert.NotContains(t, schema.Properties, "fields") -} - func Test_GetFileContents_DirectoryFieldsTelemetry(t *testing.T) { mockDirContent := []*github.RepositoryContent{ { @@ -1402,11 +1384,7 @@ func Test_ListCommits(t *testing.T) { // Verify tool definition once serverTool := ListCommits(translations.NullTranslationHelper) tool := serverTool.Tool - // ListCommits is the FeatureFlagFieldsParam-enabled variant; it owns the - // _ff_ snapshot. The canonical list_commits.snap is owned by - // LegacyListCommits (see Test_LegacyListCommits_Definition). - require.NoError(t, toolsnaps.Test(tool.Name+"_ff_"+FeatureFlagFieldsParam, tool)) - require.Equal(t, FeatureFlagFieldsParam, serverTool.FeatureFlagEnable) + require.NoError(t, toolsnaps.Test(tool.Name, tool)) schema, ok := tool.InputSchema.(*jsonschema.Schema) require.True(t, ok, "InputSchema should be *jsonschema.Schema") @@ -3644,11 +3622,7 @@ func Test_GetTag(t *testing.T) { func Test_ListReleases(t *testing.T) { serverTool := ListReleases(translations.NullTranslationHelper) tool := serverTool.Tool - // ListReleases is the FeatureFlagFieldsParam-enabled variant; it owns the - // _ff_ snapshot. The canonical list_releases.snap is owned by - // LegacyListReleases (see Test_LegacyListReleases_Definition). - require.NoError(t, toolsnaps.Test(tool.Name+"_ff_"+FeatureFlagFieldsParam, tool)) - require.Equal(t, FeatureFlagFieldsParam, serverTool.FeatureFlagEnable) + require.NoError(t, toolsnaps.Test(tool.Name, tool)) schema, ok := tool.InputSchema.(*jsonschema.Schema) require.True(t, ok, "InputSchema should be *jsonschema.Schema") diff --git a/pkg/github/search.go b/pkg/github/search.go index 28439e9fb7..3160209318 100644 --- a/pkg/github/search.go +++ b/pkg/github/search.go @@ -191,34 +191,8 @@ func attachSearchRepositoriesIFCLabel(ctx context.Context, deps ToolDependencies setIFCLabel(callResult, ifc.LabelSearchIssues(visibilities)) } -// SearchCode creates a tool to search for code across GitHub repositories. It is -// the FeatureFlagFieldsParam-enabled variant: it advertises the optional -// `fields` parameter and filters each result to the requested subset. Both this -// and LegacySearchCode register under the tool name "search_code"; exactly one -// is active for any given request thanks to mutually exclusive -// FeatureFlagEnable / FeatureFlagDisable annotations. +// SearchCode creates a tool to search for code across GitHub repositories. func SearchCode(t translations.TranslationHelperFunc) inventory.ServerTool { - st := searchCodeTool(t, true) - st.FeatureFlagEnable = FeatureFlagFieldsParam - return st -} - -// LegacySearchCode is the FeatureFlagFieldsParam-disabled variant of -// search_code. It exposes the original schema (no `fields` parameter) and never -// filters results, so it acts as the kill switch when the flag is off. It owns -// the canonical search_code.snap; the flag-enabled variant owns -// search_code_ff_.snap. Delete this function when the flag is removed. -func LegacySearchCode(t translations.TranslationHelperFunc) inventory.ServerTool { - st := searchCodeTool(t, false) - st.FeatureFlagDisable = []string{FeatureFlagFieldsParam} - return st -} - -// searchCodeTool builds the search_code tool. When includeFields is true the -// tool advertises the optional `fields` parameter, filters each result to the -// requested subset, and emits fields telemetry. When false it is the original -// tool with no fields parameter and no filtering. -func searchCodeTool(t translations.TranslationHelperFunc, includeFields bool) inventory.ServerTool { schema := &jsonschema.Schema{ Type: "object", Properties: map[string]*jsonschema.Schema{ @@ -238,12 +212,10 @@ func searchCodeTool(t translations.TranslationHelperFunc, includeFields bool) in }, Required: []string{"query"}, } - if includeFields { - schema.Properties["fields"] = fieldsSchemaProperty( - "Subset of fields to return for each code search result. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'repository' and 'text_matches' in particular drops the largest per-result data.", - codeSearchItemFieldEnum, - ) - } + schema.Properties["fields"] = fieldsSchemaProperty( + "Subset of fields to return for each code search result. If omitted, all fields are returned. Use this to reduce response size when you only need specific fields; omitting 'repository' and 'text_matches' in particular drops the largest per-result data.", + codeSearchItemFieldEnum, + ) WithPagination(schema) return NewTool( @@ -271,12 +243,9 @@ func searchCodeTool(t translations.TranslationHelperFunc, includeFields bool) in if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } - var fields []string - if includeFields { - fields, err = OptionalStringArrayParam(args, "fields") - if err != nil { - return utils.NewToolResultError(err.Error()), nil, nil - } + fields, err := OptionalStringArrayParam(args, "fields") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil } pagination, err := OptionalPaginationParams(args) if err != nil { @@ -338,7 +307,7 @@ func searchCodeTool(t translations.TranslationHelperFunc, includeFields bool) in filtered := false var payload any = minimalResult - if includeFields && len(fields) > 0 { + if len(fields) > 0 { filteredItems, err := filterEachField(minimalItems, fields) if err != nil { return utils.NewToolResultErrorFromErr("failed to filter code search results", err), nil, nil @@ -356,9 +325,7 @@ func searchCodeTool(t translations.TranslationHelperFunc, includeFields bool) in return utils.NewToolResultErrorFromErr("failed to marshal response", err), nil, nil } - if includeFields { - recordSearchCodeFieldsUsage(ctx, deps, minimalResult, filtered, len(r)) - } + recordSearchCodeFieldsUsage(ctx, deps, minimalResult, filtered, len(r)) callResult := utils.NewToolResultText(string(r)) // Code search spans repositories; the IFC label is the conservative diff --git a/pkg/github/search_test.go b/pkg/github/search_test.go index e5e673e74f..52e70b639c 100644 --- a/pkg/github/search_test.go +++ b/pkg/github/search_test.go @@ -342,11 +342,7 @@ func Test_SearchCode(t *testing.T) { // Verify tool definition once serverTool := SearchCode(translations.NullTranslationHelper) tool := serverTool.Tool - // SearchCode is the FeatureFlagFieldsParam-enabled variant; it owns the - // _ff_ snapshot. The canonical search_code.snap is owned by - // LegacySearchCode (see Test_LegacySearchCode_Definition). - require.NoError(t, toolsnaps.Test(tool.Name+"_ff_"+FeatureFlagFieldsParam, tool)) - require.Equal(t, FeatureFlagFieldsParam, serverTool.FeatureFlagEnable) + require.NoError(t, toolsnaps.Test(tool.Name, tool)) assert.Equal(t, "search_code", tool.Name) assert.NotEmpty(t, tool.Description) @@ -571,20 +567,6 @@ func Test_SearchCode_FieldFiltering(t *testing.T) { assert.NotContains(t, textContent.Text, "text_matches") } -func Test_LegacySearchCode_Definition(t *testing.T) { - serverTool := LegacySearchCode(translations.NullTranslationHelper) - tool := serverTool.Tool - // LegacySearchCode is the FeatureFlagFieldsParam-disabled variant and owns - // the canonical search_code.snap (no `fields`). - require.NoError(t, toolsnaps.Test(tool.Name, tool)) - require.Equal(t, []string{FeatureFlagFieldsParam}, serverTool.FeatureFlagDisable) - - assert.Equal(t, "search_code", tool.Name) - schema, ok := tool.InputSchema.(*jsonschema.Schema) - require.True(t, ok, "InputSchema should be *jsonschema.Schema") - assert.NotContains(t, schema.Properties, "fields") -} - func Test_SearchCode_FieldsTelemetry(t *testing.T) { mockSearchResult := &github.CodeSearchResult{ Total: github.Ptr(1), diff --git a/pkg/github/tools.go b/pkg/github/tools.go index 2cfcd3e89b..7bae64d2e8 100644 --- a/pkg/github/tools.go +++ b/pkg/github/tools.go @@ -192,11 +192,8 @@ func AllTools(t translations.TranslationHelperFunc) []inventory.ServerTool { // Repository tools SearchRepositories(t), GetFileContents(t), - LegacyGetFileContents(t), ListCommits(t), - LegacyListCommits(t), SearchCode(t), - LegacySearchCode(t), SearchCommits(t), GetCommit(t), GetFileBlame(t), @@ -204,7 +201,6 @@ func AllTools(t translations.TranslationHelperFunc) []inventory.ServerTool { ListTags(t), GetTag(t), ListReleases(t), - LegacyListReleases(t), GetLatestRelease(t), GetReleaseByTag(t), CreateOrUpdateFile(t), @@ -224,9 +220,7 @@ func AllTools(t translations.TranslationHelperFunc) []inventory.ServerTool { // Issue tools IssueRead(t), SearchIssues(t), - LegacySearchIssues(t), ListIssues(t), - LegacyListIssues(t), ListIssueTypes(t), ListIssueFields(t), IssueWrite(t), @@ -244,9 +238,7 @@ func AllTools(t translations.TranslationHelperFunc) []inventory.ServerTool { // Pull request tools PullRequestRead(t), ListPullRequests(t), - LegacyListPullRequests(t), SearchPullRequests(t), - LegacySearchPullRequests(t), MergePullRequest(t), UpdatePullRequestBranch(t), CreatePullRequest(t), From d080b23f593d153808fc212dc9a69d6e38ef68c9 Mon Sep 17 00:00:00 2001 From: Lizeth Vera <47796851+veralizeth@users.noreply.github.com> Date: Wed, 29 Jul 2026 12:57:26 -0700 Subject: [PATCH 3/6] Add batched update_project_items writes via GraphQL (#2903) * Implement batch project write engine Resolve and validate shared field updates and item references before executing ordered, chunked GraphQL writes with explicit ambiguous outcomes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7ae767ff-c1d0-46a9-b126-2e91403993a0 * Expose update_project_items Add the public projects_write contract, routing, handler coverage, and generated documentation for shared field updates across batches of up to 50 items. Co-authored-by: Lizeth Vera <47796851+veralizeth@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7ae767ff-c1d0-46a9-b126-2e91403993a0 * Classify batch resolution failures Use a neutral code for non-structured lookup failures while preserving structured resolution details. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7ae767ff-c1d0-46a9-b126-2e91403993a0 * Resolve issue references concurrently Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7ae767ff-c1d0-46a9-b126-2e91403993a0 Copilot-Session: 5709a470-df75-43ec-9a9c-98868e6065d2 --------- Co-authored-by: Bryan Zwicker Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7ae767ff-c1d0-46a9-b126-2e91403993a0 Copilot-Session: 5709a470-df75-43ec-9a9c-98868e6065d2 --- README.md | 3 +- pkg/github/__toolsnaps__/projects_write.snap | 99 +- pkg/github/projects.go | 92 +- pkg/github/projects_batch.go | 812 +++++++++ pkg/github/projects_batch_test.go | 1543 ++++++++++++++++++ pkg/github/projects_test.go | 61 +- 6 files changed, 2602 insertions(+), 8 deletions(-) create mode 100644 pkg/github/projects_batch.go create mode 100644 pkg/github/projects_batch_test.go diff --git a/README.md b/README.md index 61021f2950..803ecebaaf 100644 --- a/README.md +++ b/README.md @@ -1123,6 +1123,7 @@ The following sets of tools are available: - `item_owner`: The owner (user or organization) of the repository containing the issue or pull request. Required for 'add_project_item' method. Also accepted by 'update_project_item' when resolving the item by issue number. (string, optional) - `item_repo`: The name of the repository containing the issue or pull request. Required for 'add_project_item' method. Also accepted by 'update_project_item' when resolving the item by issue number. (string, optional) - `item_type`: The item's type, either issue or pull_request. Required for 'add_project_item' method. (string, optional) + - `items`: The items to update with the top-level 'updated_field'. Required for 'update_project_items'; prefer it over calling 'update_project_item' in a loop. Each entry must match exactly one reference variant: 'node_id', numeric 'item_id', or 'item_owner' + 'item_repo' + 'issue_number'. Limit: 50 items per call. (object[], optional) - `iteration_duration`: Duration in days for iterations of the field (e.g. 7 for weekly, 14 for bi-weekly). Required for 'create_iteration_field' method. (number, optional) - `iterations`: Custom iterations for 'create_iteration_field' method. Only set this when you need iterations with varying durations, breaks between them, or specific titles. Otherwise omit it: GitHub auto-creates three iterations of 'iteration_duration' days starting on 'start_date', which is the right choice for most cases. (object[], optional) - `method`: The method to execute (string, required) @@ -1134,7 +1135,7 @@ The following sets of tools are available: - `status`: The status of the project. Used for 'create_project_status_update' method. (string, optional) - `target_date`: The target date of the status update in YYYY-MM-DD format. Used for 'create_project_status_update' method. (string, optional) - `title`: The project title. Required for 'create_project' method. (string, optional) - - `updated_field`: Object describing the field to update and its new value. Required for 'update_project_item'. Two shapes are accepted: (1) by ID — {"id": 123456, "value": "..."}; (2) by name — {"name": "Status", "value": "In Progress"}. For single-select fields, option-name resolution requires the by-name shape; on the by-ID shape, pass the option ID. Set value to null to clear the field. (object, optional) + - `updated_field`: The field/value to apply, using {"id": 123, "value": ...} or {"name": "Status", "value": ...}; null clears the field. Required for 'update_project_item' and 'update_project_items', where one top-level field/value applies to every item in a batch. For 'update_project_item' SINGLE_SELECT fields, the name form accepts option names; the ID form expects an option ID. (object, optional) diff --git a/pkg/github/__toolsnaps__/projects_write.snap b/pkg/github/__toolsnaps__/projects_write.snap index 762ee08c93..d7c5d25eab 100644 --- a/pkg/github/__toolsnaps__/projects_write.snap +++ b/pkg/github/__toolsnaps__/projects_write.snap @@ -5,7 +5,7 @@ "readOnlyHint": false, "title": "Manage GitHub Projects" }, - "description": "Create and manage GitHub Projects: create projects, add/update/delete items, create status updates, and add iteration fields.", + "description": "Create and manage GitHub Projects: create projects, add/update/delete items, bulk-update many items at once, create status updates, and add iteration fields.", "inputSchema": { "properties": { "body": { @@ -40,6 +40,64 @@ ], "type": "string" }, + "items": { + "description": "The items to update with the top-level 'updated_field'. Required for 'update_project_items'; prefer it over calling 'update_project_item' in a loop. Each entry must match exactly one reference variant: 'node_id', numeric 'item_id', or 'item_owner' + 'item_repo' + 'issue_number'. Limit: 50 items per call.", + "items": { + "oneOf": [ + { + "additionalProperties": false, + "properties": { + "node_id": { + "description": "The project item's GraphQL node ID, as returned by 'list_project_items' or 'add_project_item'.", + "type": "string" + } + }, + "required": [ + "node_id" + ], + "type": "object" + }, + { + "additionalProperties": false, + "properties": { + "item_id": { + "description": "The numeric project item ID.", + "type": "integer" + } + }, + "required": [ + "item_id" + ], + "type": "object" + }, + { + "additionalProperties": false, + "properties": { + "issue_number": { + "description": "Issue number used to resolve the project item.", + "type": "integer" + }, + "item_owner": { + "description": "Owner of the repository containing the issue.", + "type": "string" + }, + "item_repo": { + "description": "Repository containing the issue.", + "type": "string" + } + }, + "required": [ + "item_owner", + "item_repo", + "issue_number" + ], + "type": "object" + } + ], + "type": "object" + }, + "type": "array" + }, "iteration_duration": { "description": "Duration in days for iterations of the field (e.g. 7 for weekly, 14 for bi-weekly). Required for 'create_iteration_field' method.", "type": "number" @@ -76,6 +134,7 @@ "enum": [ "add_project_item", "update_project_item", + "update_project_items", "delete_project_item", "create_project_status_update", "create_project", @@ -127,7 +186,43 @@ "type": "string" }, "updated_field": { - "description": "Object describing the field to update and its new value. Required for 'update_project_item'. Two shapes are accepted: (1) by ID — {\"id\": 123456, \"value\": \"...\"}; (2) by name — {\"name\": \"Status\", \"value\": \"In Progress\"}. For single-select fields, option-name resolution requires the by-name shape; on the by-ID shape, pass the option ID. Set value to null to clear the field.", + "description": "The field/value to apply, using {\"id\": 123, \"value\": ...} or {\"name\": \"Status\", \"value\": ...}; null clears the field. Required for 'update_project_item' and 'update_project_items', where one top-level field/value applies to every item in a batch. For 'update_project_item' SINGLE_SELECT fields, the name form accepts option names; the ID form expects an option ID.", + "oneOf": [ + { + "additionalProperties": false, + "properties": { + "id": { + "description": "The numeric project field ID.", + "type": "integer" + }, + "value": { + "description": "The value to apply. Any JSON value is accepted; use null to clear the field." + } + }, + "required": [ + "id", + "value" + ], + "type": "object" + }, + { + "additionalProperties": false, + "properties": { + "name": { + "description": "The project field name. Matching is case-insensitive.", + "type": "string" + }, + "value": { + "description": "The value to apply. Any JSON value is accepted; use null to clear the field." + } + }, + "required": [ + "name", + "value" + ], + "type": "object" + } + ], "type": "object" } }, diff --git a/pkg/github/projects.go b/pkg/github/projects.go index 308c2b87e8..514964be93 100644 --- a/pkg/github/projects.go +++ b/pkg/github/projects.go @@ -32,6 +32,7 @@ const ( ProjectStatusUpdateCreateFailedError = "failed to create project status update" ProjectResolveIDFailedError = "failed to resolve project ID" MaxProjectsPerPage = 50 + maxProjectItemsPerBatch = 50 ) // Method constants for consolidated project tools @@ -44,6 +45,7 @@ const ( projectsMethodGetProjectItem = "get_project_item" projectsMethodAddProjectItem = "add_project_item" projectsMethodUpdateProjectItem = "update_project_item" + projectsMethodUpdateProjectItems = "update_project_items" projectsMethodDeleteProjectItem = "delete_project_item" projectsMethodListProjectStatusUpdates = "list_project_status_updates" projectsMethodGetProjectStatusUpdate = "get_project_status_update" @@ -490,13 +492,90 @@ Use this tool to get details about individual projects, project fields, and proj return tool } +func updateProjectItemsItemSchema() *jsonschema.Schema { + variant := func(required []string, properties map[string]*jsonschema.Schema) *jsonschema.Schema { + return &jsonschema.Schema{ + Type: "object", + AdditionalProperties: &jsonschema.Schema{Not: &jsonschema.Schema{}}, + Properties: properties, + Required: required, + } + } + + return &jsonschema.Schema{ + Type: "object", + OneOf: []*jsonschema.Schema{ + variant([]string{"node_id"}, map[string]*jsonschema.Schema{ + "node_id": { + Type: "string", + Description: "The project item's GraphQL node ID, as returned by 'list_project_items' or 'add_project_item'.", + }, + }), + variant([]string{"item_id"}, map[string]*jsonschema.Schema{ + "item_id": { + Type: "integer", + Description: "The numeric project item ID.", + }, + }), + variant([]string{"item_owner", "item_repo", "issue_number"}, map[string]*jsonschema.Schema{ + "item_owner": { + Type: "string", + Description: "Owner of the repository containing the issue.", + }, + "item_repo": { + Type: "string", + Description: "Repository containing the issue.", + }, + "issue_number": { + Type: "integer", + Description: "Issue number used to resolve the project item.", + }, + }), + }, + } +} + +func projectUpdatedFieldSchema() *jsonschema.Schema { + value := &jsonschema.Schema{ + Description: "The value to apply. Any JSON value is accepted; use null to clear the field.", + } + variant := func(required []string, properties map[string]*jsonschema.Schema) *jsonschema.Schema { + properties["value"] = value + return &jsonschema.Schema{ + Type: "object", + AdditionalProperties: &jsonschema.Schema{Not: &jsonschema.Schema{}}, + Properties: properties, + Required: required, + } + } + + return &jsonschema.Schema{ + Type: "object", + Description: "The field/value to apply, using {\"id\": 123, \"value\": ...} or {\"name\": \"Status\", \"value\": ...}; null clears the field. Required for 'update_project_item' and 'update_project_items', where one top-level field/value applies to every item in a batch. For 'update_project_item' SINGLE_SELECT fields, the name form accepts option names; the ID form expects an option ID.", + OneOf: []*jsonschema.Schema{ + variant([]string{"id", "value"}, map[string]*jsonschema.Schema{ + "id": { + Type: "integer", + Description: "The numeric project field ID.", + }, + }), + variant([]string{"name", "value"}, map[string]*jsonschema.Schema{ + "name": { + Type: "string", + Description: "The project field name. Matching is case-insensitive.", + }, + }), + }, + } +} + // ProjectsWrite returns the tool and handler for modifying GitHub Projects resources. func ProjectsWrite(t translations.TranslationHelperFunc) inventory.ServerTool { tool := NewTool( ToolsetMetadataProjects, mcp.Tool{ Name: "projects_write", - Description: t("TOOL_PROJECTS_WRITE_DESCRIPTION", "Create and manage GitHub Projects: create projects, add/update/delete items, create status updates, and add iteration fields."), + Description: t("TOOL_PROJECTS_WRITE_DESCRIPTION", "Create and manage GitHub Projects: create projects, add/update/delete items, bulk-update many items at once, create status updates, and add iteration fields."), Annotations: &mcp.ToolAnnotations{ Title: t("TOOL_PROJECTS_WRITE_USER_TITLE", "Manage GitHub Projects"), ReadOnlyHint: false, @@ -511,6 +590,7 @@ func ProjectsWrite(t translations.TranslationHelperFunc) inventory.ServerTool { Enum: []any{ projectsMethodAddProjectItem, projectsMethodUpdateProjectItem, + projectsMethodUpdateProjectItems, projectsMethodDeleteProjectItem, projectsMethodCreateProjectStatusUpdate, projectsMethodCreateProject, @@ -559,9 +639,11 @@ func ProjectsWrite(t translations.TranslationHelperFunc) inventory.ServerTool { Type: "number", Description: "The pull request number (use when item_type is 'pull_request' for 'add_project_item' method). Provide either issue_number or pull_request_number.", }, - "updated_field": { - Type: "object", - Description: "Object describing the field to update and its new value. Required for 'update_project_item'. Two shapes are accepted: (1) by ID — {\"id\": 123456, \"value\": \"...\"}; (2) by name — {\"name\": \"Status\", \"value\": \"In Progress\"}. For single-select fields, option-name resolution requires the by-name shape; on the by-ID shape, pass the option ID. Set value to null to clear the field.", + "updated_field": projectUpdatedFieldSchema(), + "items": { + Type: "array", + Description: "The items to update with the top-level 'updated_field'. Required for 'update_project_items'; prefer it over calling 'update_project_item' in a loop. Each entry must match exactly one reference variant: 'node_id', numeric 'item_id', or 'item_owner' + 'item_repo' + 'issue_number'. Limit: " + strconv.Itoa(maxProjectItemsPerBatch) + " items per call.", + Items: updateProjectItemsItemSchema(), }, "body": { Type: "string", @@ -722,6 +804,8 @@ func ProjectsWrite(t translations.TranslationHelperFunc) inventory.ServerTool { return utils.NewToolResultError("updated_field must be an object"), nil, nil } return updateProjectItem(ctx, client, gqlClient, owner, ownerType, projectNumber, itemID, fieldValue) + case projectsMethodUpdateProjectItems: + return updateProjectItemsBatch(ctx, client, gqlClient, owner, ownerType, projectNumber, args) case projectsMethodDeleteProjectItem: itemID, err := RequiredBigInt(args, "item_id") if err != nil { diff --git a/pkg/github/projects_batch.go b/pkg/github/projects_batch.go new file mode 100644 index 0000000000..28493a4bf2 --- /dev/null +++ b/pkg/github/projects_batch.go @@ -0,0 +1,812 @@ +package github + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "math" + "sync" + "time" + + ghErrors "github.com/github/github-mcp-server/pkg/errors" + "github.com/github/github-mcp-server/pkg/utils" + "github.com/google/go-github/v89/github" + "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/shurcooL/githubv4" +) + +// Unknown outcomes cannot be attributed or retried safely because the pinned +// client drops errors[].path. +type batchItemStatus string + +const ( + batchItemSucceeded batchItemStatus = "succeeded" + batchItemFailed batchItemStatus = "failed" + batchItemUnknown batchItemStatus = "unknown" +) + +type batchItemResult struct { + Index int `json:"index"` + Status batchItemStatus `json:"status"` + Item *batchItemIdentity `json:"item,omitempty"` + Error *batchItemError `json:"error,omitempty"` + // Ref preserves the request identity when resolution fails. + Ref map[string]any `json:"ref,omitempty"` +} + +type batchItemIdentity struct { + NodeID string `json:"node_id,omitempty"` + FullDatabaseID string `json:"full_database_id,omitempty"` + ItemID int64 `json:"item_id,omitempty"` +} + +type batchItemError struct { + Code string `json:"code"` + Message string `json:"message"` + Candidates []any `json:"candidates,omitempty"` + Hint string `json:"hint,omitempty"` +} + +type resolvedBatchItem struct { + index int + ref map[string]any + nodeID string + fullDatabaseID int64 +} + +type batchWriteOperation struct { + gqlClient *githubv4.Client + kind batchMutationKind + projectID githubv4.ID + fieldID githubv4.ID + value githubv4.ProjectV2FieldValue +} + +func updateProjectItemsBatch(ctx context.Context, client *github.Client, gqlClient *githubv4.Client, owner, ownerType string, projectNumber int, args map[string]any) (*mcp.CallToolResult, any, error) { + rawItems, exists := args["items"] + if !exists { + return utils.NewToolResultError("missing required parameter: items"), nil, nil + } + itemsRaw, ok := rawItems.([]any) + if !ok { + return utils.NewToolResultError("items must be an array"), nil, nil + } + if len(itemsRaw) == 0 { + return utils.NewToolResultError("items must contain at least one entry"), nil, nil + } + if len(itemsRaw) > maxProjectItemsPerBatch { + return utils.NewToolResultError(fmt.Sprintf("items exceeds maximum of %d entries per call (got %d)", maxProjectItemsPerBatch, len(itemsRaw))), nil, nil + } + + rawField, hasField := args["updated_field"] + if !hasField { + return utils.NewToolResultError("missing required parameter: updated_field"), nil, nil + } + fieldSpec, fieldSpecErr := parseBatchFieldSpec(rawField) + if fieldSpecErr != nil { + return utils.NewToolResultError(fieldSpecErr.Error()), nil, nil + } + + if gqlClient == nil { + return utils.NewToolResultError("internal error: gqlClient is required for update_project_items"), nil, nil + } + + parsed := make([]parsedBatchItem, len(itemsRaw)) + for i, raw := range itemsRaw { + parsed[i] = parseBatchItemEntry(i, raw) + } + + results := make([]batchItemResult, len(itemsRaw)) + pending := 0 + for i, p := range parsed { + if p.err != nil { + results[i] = batchItemResult{Index: i, Status: batchItemFailed, Ref: p.ref, Error: p.err} + } else { + pending++ + } + } + if pending == 0 { + return newUpdateProjectItemsResult(results) + } + + projectID, err := resolveProjectNodeID(ctx, gqlClient, owner, ownerType, projectNumber) + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + field, fieldErr := resolveBatchProjectField(ctx, gqlClient, owner, ownerType, projectNumber, fieldSpec) + if fieldErr != nil { + return batchTopLevelError(fieldErr), nil, nil + } + + kind := batchMutationUpdate + var value githubv4.ProjectV2FieldValue + if fieldSpec.value == nil { + kind = batchMutationClear + } else { + value, fieldErr = convertProjectFieldValue(field, fieldSpec.value) + if fieldErr != nil { + return batchTopLevelError(fieldErr), nil, nil + } + } + + var numericIDs []int64 + for _, p := range parsed { + if p.err == nil && p.refKind == batchRefItemID { + numericIDs = append(numericIDs, p.itemID) + } + } + itemIDLookups := resolveItemNodeIDsByNumericID(ctx, client, owner, ownerType, projectNumber, numericIDs) + + issueLookups := resolveIssueRefs(ctx, gqlClient, projectID, parsed) + + var work []resolvedBatchItem + seenTargets := make(map[string]int) + + for i, p := range parsed { + if p.err != nil { + continue + } + + nodeID, fullDatabaseID, lookupErr := resolveItemReference(p, itemIDLookups, issueLookups) + if lookupErr != nil { + results[i] = batchItemResult{Index: i, Status: batchItemFailed, Ref: p.ref, Error: batchErrorFromResolution(lookupErr)} + continue + } + + if firstIndex, dup := seenTargets[nodeID]; dup { + results[i] = batchItemResult{ + Index: i, Status: batchItemFailed, Ref: p.ref, + Error: &batchItemError{ + Code: "duplicate_target", + Message: fmt.Sprintf("items[%d] targets the same project item as items[%d]; each item may only be written once per call", i, firstIndex), + }, + } + continue + } + + seenTargets[nodeID] = i + work = append(work, resolvedBatchItem{index: i, ref: p.ref, nodeID: nodeID, fullDatabaseID: fullDatabaseID}) + } + + executeBatchWrites(ctx, batchWriteOperation{ + gqlClient: gqlClient, + kind: kind, + projectID: projectID, + fieldID: githubv4.ID(field.NodeID), + value: value, + }, work, results) + + return newUpdateProjectItemsResult(results) +} + +func batchTopLevelError(err error) *mcp.CallToolResult { + var structured *ghErrors.StructuredResolutionError + if errors.As(err, &structured) { + return ghErrors.NewStructuredResolutionErrorResponse(structured) + } + return utils.NewToolResultError(err.Error()) +} + +func newUpdateProjectItemsResult(results []batchItemResult) (*mcp.CallToolResult, any, error) { + succeeded, failed, unknown := 0, 0, 0 + for _, r := range results { + switch r.Status { + case batchItemSucceeded: + succeeded++ + case batchItemUnknown: + unknown++ + default: + failed++ + } + } + + response := map[string]any{ + "total": len(results), + "succeeded": succeeded, + "failed": failed, + "unknown": unknown, + "results": results, + } + r, err := json.Marshal(response) + if err != nil { + return nil, nil, fmt.Errorf("failed to marshal response: %w", err) + } + + result := utils.NewToolResultText(string(r)) + if succeeded == 0 { + result.IsError = true + } + return result, nil, nil +} + +func resolveItemReference(p parsedBatchItem, itemIDLookups map[int64]itemLookupResult, issueLookups map[issueRefKey]itemLookupResult) (nodeID string, fullDatabaseID int64, err error) { + switch p.refKind { + case batchRefNodeID: + return p.nodeID, 0, nil + case batchRefItemID: + lookup := itemIDLookups[p.itemID] + if lookup.err != nil { + return "", 0, lookup.err + } + return lookup.nodeID, p.itemID, nil + case batchRefIssue: + key := issueRefKey{owner: p.issueOwner, repo: p.issueRepo, number: p.issueNumber} + lookup := issueLookups[key] + if lookup.err != nil { + return "", 0, lookup.err + } + return lookup.nodeID, lookup.fullDatabaseID, nil + default: + return "", 0, fmt.Errorf("internal error: unrecognised item reference kind") + } +} + +// Transport, cancellation, or incomplete-data ambiguity stops later chunks; +// GraphQL response errors do not because populated aliases still confirm writes. +func executeBatchWrites(ctx context.Context, operation batchWriteOperation, items []resolvedBatchItem, results []batchItemResult) { + for start := 0; start < len(items); start += batchMutationWireChunkSize { + if ctx.Err() != nil { + markChunkUnknown(items[start:], results, ctx.Err()) + return + } + + end := min(start+batchMutationWireChunkSize, len(items)) + chunk := items[start:end] + + inputs := make([]githubv4.Input, len(chunk)) + for i, item := range chunk { + if operation.kind == batchMutationClear { + inputs[i] = githubv4.ClearProjectV2ItemFieldValueInput{ + ProjectID: operation.projectID, + ItemID: githubv4.ID(item.nodeID), + FieldID: operation.fieldID, + } + } else { + inputs[i] = githubv4.UpdateProjectV2ItemFieldValueInput{ + ProjectID: operation.projectID, + ItemID: githubv4.ID(item.nodeID), + FieldID: operation.fieldID, + Value: operation.value, + } + } + } + + outcomes, mutateErr := executeAliasedMutation(ctx, operation.gqlClient, operation.kind, inputs) + + populated := 0 + for i, oc := range outcomes { + if oc.Populated { + populated++ + results[chunk[i].index] = batchItemResult{ + Index: chunk[i].index, + Status: batchItemSucceeded, + Ref: chunk[i].ref, + Item: &batchItemIdentity{ + NodeID: oc.NodeID, + FullDatabaseID: oc.FullDatabaseID, + ItemID: chunk[i].fullDatabaseID, + }, + } + } + } + + if isGraphQLResponseError(mutateErr) { + markUnpopulatedUnknown(chunk, outcomes, results, mutateErr) + continue + } + + if mutateErr != nil { + markChunkUnknown(items[start:], results, mutateErr) + return + } + + if populated != len(chunk) { + markChunkUnknown(items[start:], results, fmt.Errorf("mutation response did not include every item")) + return + } + } +} + +func markUnpopulatedUnknown(chunk []resolvedBatchItem, outcomes []mutationAliasOutcome, results []batchItemResult, err error) { + for i, oc := range outcomes { + if oc.Populated { + continue + } + results[chunk[i].index] = batchItemResult{ + Index: chunk[i].index, + Status: batchItemUnknown, + Ref: chunk[i].ref, + Error: &batchItemError{Code: "mutation_unconfirmed", Message: err.Error()}, + } + } +} + +func markChunkUnknown(chunk []resolvedBatchItem, results []batchItemResult, err error) { + for _, item := range chunk { + if results[item.index].Status == batchItemSucceeded { + continue + } + results[item.index] = batchItemResult{ + Index: item.index, + Status: batchItemUnknown, + Ref: item.ref, + Error: &batchItemError{Code: "mutation_unconfirmed", Message: err.Error()}, + } + } +} + +const batchItemLookupConcurrency = 5 + +type batchItemRefKind int + +const ( + batchRefNodeID batchItemRefKind = iota + batchRefItemID + batchRefIssue +) + +type parsedBatchItem struct { + index int + ref map[string]any + refKind batchItemRefKind + + nodeID string + itemID int64 + + issueOwner string + issueRepo string + issueNumber int + + err *batchItemError +} + +func parseBatchItemEntry(index int, raw any) parsedBatchItem { + p := parsedBatchItem{index: index} + + entry, ok := raw.(map[string]any) + if !ok || entry == nil { + p.err = &batchItemError{Code: "invalid_item", Message: fmt.Sprintf("items[%d] must be an object", index)} + return p + } + p.ref = itemRefEcho(entry) + + if _, hasUpdatedField := entry["updated_field"]; hasUpdatedField { + p.err = &batchItemError{Code: "invalid_item", Message: fmt.Sprintf("items[%d].updated_field is not supported; use the top-level updated_field", index)} + return p + } + + if refErr := p.parseItemRef(entry); refErr != nil { + p.err = &batchItemError{Code: "invalid_item_ref", Message: refErr.Error()} + } + return p +} + +func (p *parsedBatchItem) parseItemRef(entry map[string]any) error { + _, hasNodeID := entry["node_id"] + _, hasItemID := entry["item_id"] + _, hasOwner := entry["item_owner"] + _, hasRepo := entry["item_repo"] + _, hasIssueNumber := entry["issue_number"] + hasIssueRef := hasOwner || hasRepo || hasIssueNumber + + formsPresent := 0 + if hasNodeID { + formsPresent++ + } + if hasItemID { + formsPresent++ + } + if hasIssueRef { + formsPresent++ + } + + switch { + case formsPresent == 0: + return fmt.Errorf("each item requires exactly one of node_id, item_id, or item_owner + item_repo + issue_number") + case formsPresent > 1: + return fmt.Errorf("each item must set exactly one of node_id, item_id, or item_owner + item_repo + issue_number, not more than one") + } + + switch { + case hasNodeID: + s, ok := entry["node_id"].(string) + if !ok || s == "" { + return fmt.Errorf("node_id must be a non-empty string") + } + p.refKind = batchRefNodeID + p.nodeID = s + case hasItemID: + id, err := validatePositiveInt64(entry["item_id"]) + if err != nil { + return fmt.Errorf("item_id: %w", err) + } + p.refKind = batchRefItemID + p.itemID = id + default: + issueOwner, ownerErr := stringFromEntry(entry, "item_owner") + issueRepo, repoErr := stringFromEntry(entry, "item_repo") + issueNumber, numErr := intFromEntry(entry, "issue_number") + for _, err := range []error{ownerErr, repoErr, numErr} { + if err != nil { + return fmt.Errorf("item_owner, item_repo, and issue_number must all be provided together: %w", err) + } + } + p.refKind = batchRefIssue + p.issueOwner = issueOwner + p.issueRepo = issueRepo + p.issueNumber = issueNumber + } + return nil +} + +func itemRefEcho(entry map[string]any) map[string]any { + ref := map[string]any{} + for _, key := range []string{"node_id", "item_id", "item_owner", "item_repo", "issue_number"} { + if v, ok := entry[key]; ok { + ref[key] = v + } + } + if len(ref) == 0 { + return nil + } + return ref +} + +func stringFromEntry(entry map[string]any, key string) (string, error) { + v, ok := entry[key] + if !ok { + return "", fmt.Errorf("missing %s", key) + } + s, ok := v.(string) + if !ok || s == "" { + return "", fmt.Errorf("%s must be a non-empty string", key) + } + return s, nil +} + +func intFromEntry(entry map[string]any, key string) (int, error) { + v, ok := entry[key] + if !ok { + return 0, fmt.Errorf("missing %s", key) + } + n, err := validatePositiveInt64(v) + if err != nil { + return 0, fmt.Errorf("%s must be a positive integer: %w", key, err) + } + if n > math.MaxInt32 { + return 0, fmt.Errorf("%s exceeds the GraphQL Int maximum of %d", key, int64(math.MaxInt32)) + } + return int(n), nil +} + +func validatePositiveInt64(value any) (int64, error) { + n, err := validateAndConvertToInt64(value) + if err != nil { + return 0, err + } + if n <= 0 { + return 0, fmt.Errorf("value must be greater than zero (got %d)", n) + } + return n, nil +} + +type batchFieldSpec struct { + id int64 + name string + value any +} + +func parseBatchFieldSpec(raw any) (batchFieldSpec, error) { + var spec batchFieldSpec + input, ok := raw.(map[string]any) + if !ok || input == nil { + return spec, fmt.Errorf("updated_field must be an object") + } + + value, hasValue := input["value"] + if !hasValue { + return spec, fmt.Errorf("updated_field.value is required") + } + spec.value = value + + idField, hasID := input["id"] + nameField, hasName := input["name"] + switch { + case hasID && hasName: + return spec, fmt.Errorf("updated_field must set either id or name, not both") + case !hasID && !hasName: + return spec, fmt.Errorf("updated_field requires either id or name") + case hasID: + id, err := validatePositiveInt64(idField) + if err != nil { + return spec, fmt.Errorf("updated_field.id: %w", err) + } + spec.id = id + default: + name, ok := nameField.(string) + if !ok || name == "" { + return spec, fmt.Errorf("updated_field.name must be a non-empty string") + } + spec.name = name + } + return spec, nil +} + +func resolveBatchProjectField(ctx context.Context, gqlClient *githubv4.Client, owner, ownerType string, projectNumber int, spec batchFieldSpec) (*ResolvedField, error) { + if spec.name != "" { + return resolveProjectFieldByName(ctx, gqlClient, owner, ownerType, projectNumber, spec.name, "") + } + + fields, err := listAllProjectFields(ctx, gqlClient, owner, ownerType, projectNumber) + if err != nil { + return nil, err + } + + id := fmt.Sprintf("%d", spec.id) + for _, field := range fields { + if field.ID == id { + return &field, nil + } + } + return nil, ghErrors.NewStructuredResolutionError( + "field_not_found", + id, + fmt.Sprintf("no project field with id %s on project %s#%d; see candidates for available fields", id, owner, projectNumber), + projectFieldCandidates(fields), + ) +} + +func projectFieldCandidates(fields []ResolvedField) []any { + candidates := make([]any, 0, len(fields)) + for _, field := range fields { + candidates = append(candidates, map[string]any{ + "id": field.ID, + "name": field.Name, + "data_type": field.DataType, + }) + } + return candidates +} + +func convertProjectFieldValue(field *ResolvedField, raw any) (githubv4.ProjectV2FieldValue, error) { + var zero githubv4.ProjectV2FieldValue + + switch field.DataType { + case "TEXT": + s, ok := raw.(string) + if !ok { + return zero, fmt.Errorf("field %q is TEXT; value must be a string", field.Name) + } + v := githubv4.String(s) + return githubv4.ProjectV2FieldValue{Text: &v}, nil + + case "NUMBER": + f, ok := toFloat64(raw) + if !ok { + return zero, fmt.Errorf("field %q is NUMBER; value must be a number", field.Name) + } + v := githubv4.Float(f) + return githubv4.ProjectV2FieldValue{Number: &v}, nil + + case "DATE": + s, ok := raw.(string) + if !ok { + return zero, fmt.Errorf("field %q is DATE; value must be a YYYY-MM-DD string", field.Name) + } + t, err := time.Parse("2006-01-02", s) + if err != nil { + return zero, fmt.Errorf("field %q is DATE; value %q is not in YYYY-MM-DD format: %w", field.Name, s, err) + } + return githubv4.ProjectV2FieldValue{Date: &githubv4.Date{Time: t}}, nil + + case "SINGLE_SELECT": + s, ok := raw.(string) + if !ok || s == "" { + return zero, fmt.Errorf("field %q is SINGLE_SELECT; value must be a non-empty string (option name or ID)", field.Name) + } + optID := s + if resolvedID, optErr := resolveSingleSelectOptionByName(field, s); optErr == nil { + optID = resolvedID + } else { + known := false + for _, opt := range field.Options { + if opt.ID == s { + known = true + break + } + } + if !known { + return zero, optErr + } + } + v := githubv4.String(optID) + return githubv4.ProjectV2FieldValue{SingleSelectOptionID: &v}, nil + + case "ITERATION": + s, ok := raw.(string) + if !ok || s == "" { + return zero, fmt.Errorf("field %q is ITERATION; value must be a non-empty iteration ID string", field.Name) + } + v := githubv4.String(s) + return githubv4.ProjectV2FieldValue{IterationID: &v}, nil + + default: + return zero, fmt.Errorf("field %q has unsupported data type %q for update_project_items; use update_project_item instead", field.Name, field.DataType) + } +} + +func toFloat64(raw any) (float64, bool) { + var number float64 + switch v := raw.(type) { + case float64: + number = v + case int: + number = float64(v) + case int64: + number = float64(v) + default: + return 0, false + } + if math.IsNaN(number) || math.IsInf(number, 0) { + return 0, false + } + return number, true +} + +type itemLookupResult struct { + nodeID string + fullDatabaseID int64 + err error +} + +// Numeric lookups are deduplicated and concurrency-bounded; individual failures +// remain isolated while cancellation stops pending work. +func resolveItemNodeIDsByNumericID(ctx context.Context, client *github.Client, owner, ownerType string, projectNumber int, ids []int64) map[int64]itemLookupResult { + seen := make(map[int64]struct{}, len(ids)) + var unique []int64 + for _, id := range ids { + if _, dup := seen[id]; dup { + continue + } + seen[id] = struct{}{} + unique = append(unique, id) + } + + out := make(map[int64]itemLookupResult, len(unique)) + if len(unique) == 0 { + return out + } + + var mu sync.Mutex + var wg sync.WaitGroup + sem := make(chan struct{}, batchItemLookupConcurrency) + + for _, id := range unique { + wg.Add(1) + go func(id int64) { + defer wg.Done() + + select { + case sem <- struct{}{}: + case <-ctx.Done(): + mu.Lock() + out[id] = itemLookupResult{err: ctx.Err()} + mu.Unlock() + return + } + defer func() { <-sem }() + + if ctx.Err() != nil { + mu.Lock() + out[id] = itemLookupResult{err: ctx.Err()} + mu.Unlock() + return + } + + var item *github.ProjectV2Item + var err error + if ownerType == "org" { + item, _, err = client.Projects.GetOrganizationProjectItem(ctx, owner, projectNumber, id, nil) + } else { + item, _, err = client.Projects.GetUserProjectItem(ctx, owner, projectNumber, id, nil) + } + + var res itemLookupResult + switch { + case err != nil: + res = itemLookupResult{err: fmt.Errorf("project item %d: %w", id, err)} + case item == nil || item.NodeID == nil || *item.NodeID == "": + res = itemLookupResult{err: fmt.Errorf("project item %d: response did not include a node id", id)} + default: + res = itemLookupResult{nodeID: *item.NodeID, fullDatabaseID: id} + } + + mu.Lock() + out[id] = res + mu.Unlock() + }(id) + } + wg.Wait() + return out +} + +type issueRefKey struct { + owner string + repo string + number int +} + +func resolveIssueRefs(ctx context.Context, gqlClient *githubv4.Client, projectID githubv4.ID, items []parsedBatchItem) map[issueRefKey]itemLookupResult { + seen := make(map[issueRefKey]struct{}, len(items)) + var unique []issueRefKey + for _, it := range items { + if it.err != nil || it.refKind != batchRefIssue { + continue + } + key := issueRefKey{owner: it.issueOwner, repo: it.issueRepo, number: it.issueNumber} + if _, dup := seen[key]; dup { + continue + } + seen[key] = struct{}{} + unique = append(unique, key) + } + + out := make(map[issueRefKey]itemLookupResult, len(unique)) + if len(unique) == 0 { + return out + } + + var mu sync.Mutex + var wg sync.WaitGroup + sem := make(chan struct{}, batchItemLookupConcurrency) + + for _, key := range unique { + wg.Add(1) + go func(key issueRefKey) { + defer wg.Done() + + select { + case sem <- struct{}{}: + case <-ctx.Done(): + mu.Lock() + out[key] = itemLookupResult{err: ctx.Err()} + mu.Unlock() + return + } + defer func() { <-sem }() + + if ctx.Err() != nil { + mu.Lock() + out[key] = itemLookupResult{err: ctx.Err()} + mu.Unlock() + return + } + + nodeID, itemID, err := resolveProjectItemByIssueNumberWithProjectID(ctx, gqlClient, projectID, key.owner, key.repo, key.number) + + mu.Lock() + out[key] = itemLookupResult{nodeID: nodeID, fullDatabaseID: itemID, err: err} + mu.Unlock() + }(key) + } + wg.Wait() + return out +} + +func batchErrorFromResolution(err error) *batchItemError { + var structured *ghErrors.StructuredResolutionError + if errors.As(err, &structured) { + return &batchItemError{ + Code: structured.Kind, + Message: fmt.Sprintf("%s: %s", structured.Kind, structured.Name), + Hint: structured.Hint, + Candidates: structured.Candidates, + } + } + return &batchItemError{ + Code: "resolution_failed", + Message: err.Error(), + } +} diff --git a/pkg/github/projects_batch_test.go b/pkg/github/projects_batch_test.go new file mode 100644 index 0000000000..985cd5bfc7 --- /dev/null +++ b/pkg/github/projects_batch_test.go @@ -0,0 +1,1543 @@ +package github + +import ( + "context" + "encoding/json" + "fmt" + "io" + "maps" + "math" + "net/http" + "strings" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/github/github-mcp-server/internal/githubv4mock" + ghErrors "github.com/github/github-mcp-server/pkg/errors" + "github.com/github/github-mcp-server/pkg/inventory" + "github.com/github/github-mcp-server/pkg/translations" + "github.com/shurcooL/githubv4" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// fieldNode is a generic project field response node for use in mock data, +// covering data types beyond SINGLE_SELECT (statusFieldNode in +// projects_resolver_test.go is fixed to SINGLE_SELECT). See the comment on +// listAllProjectFields's inline-fragment decoding: the underlying jsonutil +// decoder populates id/databaseId/name/dataType identically across all three +// ProjectV2*Field fragments for a flat node object, so a single flat map +// (with "options" only where relevant) is sufficient regardless of dataType. +func fieldNode(nodeID string, databaseID int, name, dataType string) map[string]any { + return map[string]any{ + "id": nodeID, + "databaseId": databaseID, + "name": name, + "dataType": dataType, + } +} + +// projectIDMatcher returns the githubv4mock matcher for the org project-node-ID +// resolution query issued once per update_project_items call. +func projectIDMatcher(owner string, projectNumber int, projectNodeID string) githubv4mock.Matcher { + return githubv4mock.NewQueryMatcher( + struct { + Organization struct { + ProjectV2 struct { + ID githubv4.ID + } `graphql:"projectV2(number: $projectNumber)"` + } `graphql:"organization(login: $owner)"` + }{}, + map[string]any{ + "owner": githubv4.String(owner), + "projectNumber": githubv4.Int(int32(projectNumber)), //nolint:gosec + }, + githubv4mock.DataResponse(map[string]any{ + "organization": map[string]any{ + "projectV2": map[string]any{"id": projectNodeID}, + }, + }), + ) +} + +// mutationAwareTransport routes GraphQL requests to a fixed query-matcher +// transport (e.g. githubv4mock.NewMockedHTTPClient's Transport) for ordinary +// queries/lookups, and to a sequenced, call-counted responder for mutation +// requests, so end-to-end tests can assert on aliased-mutation call counts and +// per-call variables without needing to hand-construct the exact minified +// mutation query text that reflect.StructOf produces. +type mutationAwareTransport struct { + t *testing.T + queries http.RoundTripper + mutationRespond func(callIndex int, req capturedGraphQLRequest) (status int, body string) + queryCalls []capturedGraphQLRequest + mutationCalls []capturedGraphQLRequest +} + +func (m *mutationAwareTransport) RoundTrip(req *http.Request) (*http.Response, error) { + raw, err := io.ReadAll(req.Body) + if err != nil { + return nil, err + } + _ = req.Body.Close() + + var parsed struct { + Query string `json:"query"` + Variables map[string]any `json:"variables"` + } + if err := json.Unmarshal(raw, &parsed); err != nil { + return nil, err + } + + if !strings.HasPrefix(strings.TrimSpace(parsed.Query), "mutation") { + m.queryCalls = append(m.queryCalls, capturedGraphQLRequest{Query: parsed.Query, Variables: parsed.Variables}) + req.Body = io.NopCloser(strings.NewReader(string(raw))) + return m.queries.RoundTrip(req) + } + + captured := capturedGraphQLRequest{Query: parsed.Query, Variables: parsed.Variables} + idx := len(m.mutationCalls) + m.mutationCalls = append(m.mutationCalls, captured) + if m.mutationRespond == nil { + m.t.Fatalf("unexpected mutation call #%d (query: %s)", idx, parsed.Query) + } + status, body := m.mutationRespond(idx, captured) + return &http.Response{ + StatusCode: status, + Body: io.NopCloser(strings.NewReader(body)), + Header: make(http.Header), + }, nil +} + +type gatedIssueLookupTransport struct { + gate <-chan struct{} + started chan int + projectID string + + mu sync.Mutex + active int + peak int + calls map[int]int +} + +func newGatedIssueLookupTransport(gate <-chan struct{}, projectID string) *gatedIssueLookupTransport { + return &gatedIssueLookupTransport{ + gate: gate, + started: make(chan int, maxProjectItemsPerBatch), + projectID: projectID, + calls: make(map[int]int), + } +} + +func (t *gatedIssueLookupTransport) RoundTrip(req *http.Request) (*http.Response, error) { + raw, err := io.ReadAll(req.Body) + if err != nil { + return nil, err + } + _ = req.Body.Close() + + var parsed struct { + Variables map[string]any `json:"variables"` + } + if err := json.Unmarshal(raw, &parsed); err != nil { + return nil, err + } + rawIssueNumber, ok := parsed.Variables["issueNumber"].(float64) + if !ok { + return nil, fmt.Errorf("issueNumber variable is missing or invalid") + } + issueNumber := int(rawIssueNumber) + + t.mu.Lock() + t.calls[issueNumber]++ + t.active++ + t.peak = max(t.peak, t.active) + t.mu.Unlock() + defer func() { + t.mu.Lock() + t.active-- + t.mu.Unlock() + }() + + t.started <- issueNumber + select { + case <-t.gate: + case <-req.Context().Done(): + return nil, req.Context().Err() + } + + body, err := json.Marshal(map[string]any{ + "data": map[string]any{ + "repository": map[string]any{ + "issue": map[string]any{ + "projectItems": map[string]any{ + "nodes": []any{ + map[string]any{ + "id": fmt.Sprintf("PVTI_item%d", issueNumber), + "fullDatabaseId": fmt.Sprintf("%d", 1000+issueNumber), + "project": map[string]any{"id": t.projectID}, + }, + }, + "pageInfo": map[string]any{ + "hasNextPage": false, "hasPreviousPage": false, + "startCursor": "page-one", "endCursor": "page-one", + }, + }, + }, + }, + }, + }) + if err != nil { + return nil, err + } + return &http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(strings.NewReader(string(body))), + Header: http.Header{"Content-Type": []string{"application/json"}}, + }, nil +} + +func (t *gatedIssueLookupTransport) snapshot() (active int, peak int, calls map[int]int) { + t.mu.Lock() + defer t.mu.Unlock() + + return t.active, t.peak, maps.Clone(t.calls) +} + +func issueBatchItems(issueNumbers ...int) []parsedBatchItem { + items := make([]parsedBatchItem, 0, len(issueNumbers)) + for index, issueNumber := range issueNumbers { + items = append(items, parsedBatchItem{ + index: index, + refKind: batchRefIssue, + issueOwner: "octo-org", + issueRepo: "roadmap", + issueNumber: issueNumber, + }) + } + return items +} + +func waitForIssueLookups(ctx context.Context, t *testing.T, started <-chan int, count int) { + t.Helper() + for range count { + select { + case <-started: + case <-ctx.Done(): + t.Fatalf("timed out waiting for %d issue lookups to start: %v", count, ctx.Err()) + } + } +} + +func waitForIssueLookupResults(ctx context.Context, t *testing.T, results <-chan map[issueRefKey]itemLookupResult) map[issueRefKey]itemLookupResult { + t.Helper() + select { + case resolved := <-results: + return resolved + case <-ctx.Done(): + t.Fatalf("timed out waiting for issue lookups to finish: %v", ctx.Err()) + return nil + } +} + +func Test_UpdateProjectItemsBatch_TopLevelGuards(t *testing.T) { + tooMany := make([]any, maxProjectItemsPerBatch+1) + validItem := map[string]any{"node_id": "PVTI_item1"} + validField := map[string]any{"name": "Notes", "value": "hello"} + tests := []struct { + name string + args map[string]any + wantErr string + }{ + {name: "missing items", args: map[string]any{}, wantErr: "missing required parameter: items"}, + {name: "non-array items", args: map[string]any{"items": "invalid"}, wantErr: "items must be an array"}, + {name: "empty items", args: map[string]any{"items": []any{}}, wantErr: "items must contain at least one entry"}, + {name: "too many items", args: map[string]any{"items": tooMany}, wantErr: "items exceeds maximum of 50 entries"}, + {name: "missing updated field", args: map[string]any{"items": []any{validItem}}, wantErr: "missing required parameter: updated_field"}, + {name: "malformed updated field", args: map[string]any{"items": []any{validItem}, "updated_field": "invalid"}, wantErr: "updated_field must be an object"}, + {name: "missing field value", args: map[string]any{"items": []any{validItem}, "updated_field": map[string]any{"name": "Notes"}}, wantErr: "updated_field.value is required"}, + {name: "missing field reference", args: map[string]any{"items": []any{validItem}, "updated_field": map[string]any{"value": "hello"}}, wantErr: "updated_field requires either id or name"}, + {name: "ambiguous field reference", args: map[string]any{"items": []any{validItem}, "updated_field": map[string]any{"id": float64(1), "name": "Notes", "value": "hello"}}, wantErr: "updated_field must set either id or name"}, + {name: "nil GraphQL client", args: map[string]any{"items": []any{validItem}, "updated_field": validField}, wantErr: "gqlClient is required"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result, structured, err := updateProjectItemsBatch(t.Context(), nil, nil, "octo-org", "org", 1, tt.args) + require.NoError(t, err) + assert.Nil(t, structured) + assert.Contains(t, getErrorResult(t, result).Text, tt.wantErr) + }) + } +} + +func Test_UpdateProjectItemsBatch_InvalidSharedValueIsTopLevelError(t *testing.T) { + queryTransport := githubv4mock.NewMockedHTTPClient( + projectIDMatcher("octo-org", 1, "PVT_project1"), + githubv4mock.NewQueryMatcher( + projectFieldsTestQuery{}, + fieldsQueryVars("octo-org", 1), + githubv4mock.DataResponse(fieldsResponse([]map[string]any{ + statusFieldNode("PVTSSF_status", 101, "Status", []map[string]any{ + {"id": "OPT_todo", "name": "Todo"}, + }), + })), + ), + ) + transport := &mutationAwareTransport{ + t: t, + queries: queryTransport.Transport, + mutationRespond: func(_ int, _ capturedGraphQLRequest) (int, string) { + t.Fatal("invalid shared values must fail before writes") + return http.StatusInternalServerError, "" + }, + } + + result, structured, err := updateProjectItemsBatch( + t.Context(), + nil, + newTestGQLClient(transport), + "octo-org", + "org", + 1, + map[string]any{ + "updated_field": map[string]any{"name": "Status", "value": "Missing"}, + "items": []any{map[string]any{"node_id": "PVTI_item1"}}, + }, + ) + require.NoError(t, err) + assert.Nil(t, structured) + var response map[string]any + require.NoError(t, json.Unmarshal([]byte(getErrorResult(t, result).Text), &response)) + assert.Equal(t, "option_not_found", response["error"]) + assert.Equal(t, "Missing", response["name"]) + assert.Equal(t, []any{map[string]any{"name": "Todo"}}, response["candidates"]) + assert.Empty(t, transport.mutationCalls) +} + +func Test_ProjectsWrite_UpdateProjectItems_NodeIDBypassesRESTLookup(t *testing.T) { + toolDef := ProjectsWrite(translations.NullTranslationHelper) + + queryTransport := githubv4mock.NewMockedHTTPClient( + projectIDMatcher("octo-org", 1, "PVT_project1"), + githubv4mock.NewQueryMatcher( + projectFieldsTestQuery{}, + fieldsQueryVars("octo-org", 1), + githubv4mock.DataResponse(fieldsResponse([]map[string]any{ + fieldNode("PVTF_notes", 101, "Notes", "TEXT"), + })), + ), + ) + + transport := &mutationAwareTransport{ + t: t, + queries: queryTransport.Transport, + mutationRespond: func(_ int, req capturedGraphQLRequest) (int, string) { + assert.Contains(t, req.Query, "updateProjectV2ItemFieldValue") + return http.StatusOK, mutationDataResponse(t, map[int]struct{ NodeID, FullDatabaseID string }{ + 0: {NodeID: "PVTI_item1", FullDatabaseID: "1001"}, + }) + }, + } + gqlClient := newTestGQLClient(transport) + + // No REST handlers registered at all: if the implementation ever fell back + // to a REST lookup for a node_id-addressed item, this would 404. + restClient := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{})) + + deps := BaseDeps{Client: restClient, GQLClient: gqlClient} + handler := toolDef.Handler(deps) + request := createMCPRequest(map[string]any{ + "method": "update_project_items", + "owner": "octo-org", + "owner_type": "org", + "project_number": float64(1), + "updated_field": map[string]any{"name": "Notes", "value": "hello"}, + "items": []any{ + map[string]any{"node_id": "PVTI_item1"}, + }, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError, getTextResult(t, result).Text) + + var response map[string]any + require.NoError(t, json.Unmarshal([]byte(getTextResult(t, result).Text), &response)) + assert.Equal(t, float64(1), response["succeeded"]) + assert.Equal(t, float64(0), response["failed"]) + assert.Equal(t, float64(0), response["unknown"]) +} + +func Test_ProjectsWrite_UpdateProjectItems_NumericItemIDDeduplicatesRESTLookup(t *testing.T) { + toolDef := ProjectsWrite(translations.NullTranslationHelper) + + queryTransport := githubv4mock.NewMockedHTTPClient( + projectIDMatcher("octo-org", 1, "PVT_project1"), + githubv4mock.NewQueryMatcher( + projectFieldsTestQuery{}, + fieldsQueryVars("octo-org", 1), + githubv4mock.DataResponse(fieldsResponse([]map[string]any{ + fieldNode("PVTF_notes", 101, "Notes", "TEXT"), + })), + ), + ) + transport := &mutationAwareTransport{ + t: t, + queries: queryTransport.Transport, + mutationRespond: func(_ int, req capturedGraphQLRequest) (int, string) { + require.Len(t, req.Variables, 1) + assert.Equal(t, "PVTF_notes", req.Variables["input"].(map[string]any)["fieldId"]) + return http.StatusOK, mutationDataResponse(t, map[int]struct{ NodeID, FullDatabaseID string }{ + 0: {NodeID: "PVTI_item1001", FullDatabaseID: "1001"}, + }) + }, + } + gqlClient := newTestGQLClient(transport) + + var restCalls int32 + restClient := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetOrgsProjectsV2ItemsByProjectByItemID: func(w http.ResponseWriter, _ *http.Request) { + atomic.AddInt32(&restCalls, 1) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"id":1001,"node_id":"PVTI_item1001"}`)) + }, + })) + + deps := BaseDeps{Client: restClient, GQLClient: gqlClient} + handler := toolDef.Handler(deps) + request := createMCPRequest(map[string]any{ + "method": "update_project_items", + "owner": "octo-org", + "owner_type": "org", + "project_number": float64(1), + "updated_field": map[string]any{"name": "Notes", "value": "hello"}, + "items": []any{ + map[string]any{"item_id": float64(1001)}, + map[string]any{"item_id": float64(1001)}, + }, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError, getTextResult(t, result).Text) + + var response map[string]any + require.NoError(t, json.Unmarshal([]byte(getTextResult(t, result).Text), &response)) + assert.Equal(t, float64(1), response["succeeded"]) + assert.Equal(t, float64(1), response["failed"]) + assert.Equal(t, int32(1), atomic.LoadInt32(&restCalls), "the same numeric item_id must only be resolved once") + results := response["results"].([]any) + assert.Equal(t, "duplicate_target", results[1].(map[string]any)["error"].(map[string]any)["code"]) +} + +func Test_ProjectsWrite_UpdateProjectItems_IssueRefPaginationIsDeduplicated(t *testing.T) { + toolDef := ProjectsWrite(translations.NullTranslationHelper) + + queryTransport := githubv4mock.NewMockedHTTPClient( + projectIDMatcher("octo-org", 1, "PVT_project1"), + githubv4mock.NewQueryMatcher( + resolveItemByIssueQuery{}, + map[string]any{ + "issueOwner": githubv4.String("github"), + "issueRepo": githubv4.String("planning-tracking"), + "issueNumber": githubv4.Int(123), + }, + githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "issue": map[string]any{ + "projectItems": map[string]any{ + "nodes": []any{ + map[string]any{ + "id": "PVTI_other", + "fullDatabaseId": "9999", + "project": map[string]any{"id": "PVT_other"}, + }, + }, + "pageInfo": map[string]any{ + "hasNextPage": true, "hasPreviousPage": false, + "startCursor": "page-one", "endCursor": "page-one", + }, + }, + }, + }, + }), + ), + githubv4mock.NewQueryMatcher( + resolveItemByIssuePageQuery{}, + map[string]any{ + "issueOwner": githubv4.String("github"), + "issueRepo": githubv4.String("planning-tracking"), + "issueNumber": githubv4.Int(123), + "after": githubv4.String("page-one"), + }, + githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "issue": map[string]any{ + "projectItems": map[string]any{ + "nodes": []any{ + map[string]any{ + "id": "PVTI_item2002", + "fullDatabaseId": "2002", + "project": map[string]any{"id": "PVT_project1"}, + }, + }, + "pageInfo": map[string]any{ + "hasNextPage": false, "hasPreviousPage": true, + "startCursor": "page-two", "endCursor": "page-two", + }, + }, + }, + }, + }), + ), + githubv4mock.NewQueryMatcher( + projectFieldsTestQuery{}, + fieldsQueryVars("octo-org", 1), + githubv4mock.DataResponse(fieldsResponse([]map[string]any{ + fieldNode("PVTF_notes", 101, "Notes", "TEXT"), + })), + ), + ) + transport := &mutationAwareTransport{ + t: t, + queries: queryTransport.Transport, + mutationRespond: func(_ int, req capturedGraphQLRequest) (int, string) { + require.Len(t, req.Variables, 1) + assert.Equal(t, "PVTI_item2002", req.Variables["input"].(map[string]any)["itemId"]) + assert.Equal(t, "PVTF_notes", req.Variables["input"].(map[string]any)["fieldId"]) + return http.StatusOK, mutationDataResponse(t, map[int]struct{ NodeID, FullDatabaseID string }{ + 0: {NodeID: "PVTI_item2002", FullDatabaseID: "2002"}, + }) + }, + } + gqlClient := newTestGQLClient(transport) + restClient := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{})) + + deps := BaseDeps{Client: restClient, GQLClient: gqlClient} + handler := toolDef.Handler(deps) + request := createMCPRequest(map[string]any{ + "method": "update_project_items", + "owner": "octo-org", + "owner_type": "org", + "project_number": float64(1), + "updated_field": map[string]any{"name": "Notes", "value": "hello"}, + "items": []any{ + map[string]any{ + "item_owner": "github", "item_repo": "planning-tracking", "issue_number": float64(123), + }, + map[string]any{ + "item_owner": "github", "item_repo": "planning-tracking", "issue_number": float64(123), + }, + }, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError, getTextResult(t, result).Text) + + var response map[string]any + require.NoError(t, json.Unmarshal([]byte(getTextResult(t, result).Text), &response)) + assert.Equal(t, float64(1), response["succeeded"]) + assert.Equal(t, float64(1), response["failed"]) + results := response["results"].([]any) + item := results[0].(map[string]any)["item"].(map[string]any) + assert.Equal(t, "PVTI_item2002", item["node_id"]) + assert.Equal(t, "2002", item["full_database_id"]) + assert.Equal(t, "duplicate_target", results[1].(map[string]any)["error"].(map[string]any)["code"]) + issueResolutionCalls := 0 + for _, call := range transport.queryCalls { + if strings.Contains(call.Query, "projectItems") { + issueResolutionCalls++ + } + } + assert.Equal(t, 2, issueResolutionCalls, "duplicate issue refs should share one two-page resolution chain") + assert.Len(t, transport.queryCalls, 4, "expected project, fields, and two issue-page queries") +} + +func Test_ProjectsWrite_UpdateProjectItems_DuplicateTargetRejected(t *testing.T) { + toolDef := ProjectsWrite(translations.NullTranslationHelper) + + queryTransport := githubv4mock.NewMockedHTTPClient( + projectIDMatcher("octo-org", 1, "PVT_project1"), + githubv4mock.NewQueryMatcher( + projectFieldsTestQuery{}, + fieldsQueryVars("octo-org", 1), + githubv4mock.DataResponse(fieldsResponse([]map[string]any{ + fieldNode("PVTF_notes", 101, "Notes", "TEXT"), + })), + ), + ) + transport := &mutationAwareTransport{ + t: t, + queries: queryTransport.Transport, + mutationRespond: func(_ int, req capturedGraphQLRequest) (int, string) { + require.Len(t, req.Variables, 1) + assert.Equal(t, 1, strings.Count(req.Query, "updateProjectV2ItemFieldValue")) + assert.Equal(t, "PVTI_item1", req.Variables["input"].(map[string]any)["itemId"]) + return http.StatusOK, mutationDataResponse(t, map[int]struct{ NodeID, FullDatabaseID string }{ + 0: {NodeID: "PVTI_item1", FullDatabaseID: "1001"}, + }) + }, + } + gqlClient := newTestGQLClient(transport) + var restCalls int32 + restClient := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetOrgsProjectsV2ItemsByProjectByItemID: func(w http.ResponseWriter, _ *http.Request) { + atomic.AddInt32(&restCalls, 1) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"id":1001,"node_id":"PVTI_item1"}`)) + }, + })) + + deps := BaseDeps{Client: restClient, GQLClient: gqlClient} + handler := toolDef.Handler(deps) + request := createMCPRequest(map[string]any{ + "method": "update_project_items", + "owner": "octo-org", + "owner_type": "org", + "project_number": float64(1), + "updated_field": map[string]any{"name": "Notes", "value": "hello"}, + "items": []any{ + map[string]any{"node_id": "PVTI_item1"}, + map[string]any{"item_id": float64(1001)}, + }, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError, getTextResult(t, result).Text) + + var response map[string]any + require.NoError(t, json.Unmarshal([]byte(getTextResult(t, result).Text), &response)) + assert.Equal(t, float64(1), response["succeeded"]) + assert.Equal(t, float64(1), response["failed"]) + + results := response["results"].([]any) + second := results[1].(map[string]any) + assert.Equal(t, "failed", second["status"]) + assert.Equal(t, "duplicate_target", second["error"].(map[string]any)["code"]) + assert.Equal(t, int32(1), atomic.LoadInt32(&restCalls)) + assert.Len(t, transport.mutationCalls, 1) +} + +func Test_ProjectsWrite_UpdateProjectItems_TwentyWritesIsOneMutationRequest(t *testing.T) { + toolDef := ProjectsWrite(translations.NullTranslationHelper) + transport := chunkSizeTestRun(t, toolDef, 20) + assert.Len(t, transport.mutationCalls, 1) +} + +func Test_ProjectsWrite_UpdateProjectItems_TwentyOneWritesIsTwoMutationRequests(t *testing.T) { + toolDef := ProjectsWrite(translations.NullTranslationHelper) + transport := chunkSizeTestRun(t, toolDef, 21) + assert.Len(t, transport.mutationCalls, 2) +} + +func Test_ProjectsWrite_UpdateProjectItems_MaximumWritesIsThreeMutationRequests(t *testing.T) { + toolDef := ProjectsWrite(translations.NullTranslationHelper) + transport := chunkSizeTestRun(t, toolDef, maxProjectItemsPerBatch) + assert.Len(t, transport.mutationCalls, 3) +} + +// chunkSizeTestRun runs an update_project_items call with itemCount node_id +// items (all TEXT field updates), returning the mutationAwareTransport so the +// caller can assert on how many aliased-mutation HTTP requests were made. +func chunkSizeTestRun(t *testing.T, toolDef inventory.ServerTool, itemCount int) *mutationAwareTransport { + t.Helper() + + queryTransport := githubv4mock.NewMockedHTTPClient( + projectIDMatcher("octo-org", 1, "PVT_project1"), + githubv4mock.NewQueryMatcher( + projectFieldsTestQuery{}, + fieldsQueryVars("octo-org", 1), + githubv4mock.DataResponse(fieldsResponse([]map[string]any{ + fieldNode("PVTF_notes", 101, "Notes", "TEXT"), + })), + ), + ) + transport := &mutationAwareTransport{ + t: t, + queries: queryTransport.Transport, + mutationRespond: func(_ int, req capturedGraphQLRequest) (int, string) { + // input (index 0) plus inputN for each additional alias in this chunk. + chunkSize := len(req.Variables) + ids := make(map[int]struct{ NodeID, FullDatabaseID string }, chunkSize) + for i := range chunkSize { + ids[i] = struct{ NodeID, FullDatabaseID string }{ + NodeID: "PVTI_chunk", + FullDatabaseID: "1", + } + } + return http.StatusOK, mutationDataResponse(t, ids) + }, + } + gqlClient := newTestGQLClient(transport) + restClient := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{})) + + items := make([]any, itemCount) + for i := range itemCount { + items[i] = map[string]any{"node_id": fmt.Sprintf("PVTI_item%d", i)} + } + + deps := BaseDeps{Client: restClient, GQLClient: gqlClient} + handler := toolDef.Handler(deps) + request := createMCPRequest(map[string]any{ + "method": "update_project_items", + "owner": "octo-org", + "owner_type": "org", + "project_number": float64(1), + "updated_field": map[string]any{"name": "Notes", "value": "hello"}, + "items": items, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError, getTextResult(t, result).Text) + + var response map[string]any + require.NoError(t, json.Unmarshal([]byte(getTextResult(t, result).Text), &response)) + assert.Equal(t, float64(itemCount), response["succeeded"]) + + return transport +} + +func Test_ProjectsWrite_UpdateProjectItems_SharedNullClearsAllItemsInOrder(t *testing.T) { + toolDef := ProjectsWrite(translations.NullTranslationHelper) + + queryTransport := githubv4mock.NewMockedHTTPClient( + projectIDMatcher("octo-org", 1, "PVT_project1"), + githubv4mock.NewQueryMatcher( + projectFieldsTestQuery{}, + fieldsQueryVars("octo-org", 1), + githubv4mock.DataResponse(fieldsResponse([]map[string]any{ + fieldNode("PVTF_notes", 101, "Notes", "TEXT"), + })), + ), + ) + transport := &mutationAwareTransport{ + t: t, + queries: queryTransport.Transport, + mutationRespond: func(_ int, req capturedGraphQLRequest) (int, string) { + assert.Contains(t, req.Query, "clearProjectV2ItemFieldValue") + assert.NotContains(t, req.Query, "updateProjectV2ItemFieldValue") + for _, input := range req.Variables { + assert.NotContains(t, input.(map[string]any), "value") + } + return http.StatusOK, mutationDataResponse(t, map[int]struct{ NodeID, FullDatabaseID string }{ + 0: {NodeID: "PVTI_item0", FullDatabaseID: "1000"}, + 1: {NodeID: "PVTI_item1", FullDatabaseID: "1001"}, + 2: {NodeID: "PVTI_item2", FullDatabaseID: "1002"}, + }) + }, + } + gqlClient := newTestGQLClient(transport) + restClient := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{})) + + deps := BaseDeps{Client: restClient, GQLClient: gqlClient} + handler := toolDef.Handler(deps) + request := createMCPRequest(map[string]any{ + "method": "update_project_items", + "owner": "octo-org", + "owner_type": "org", + "project_number": float64(1), + "updated_field": map[string]any{"name": "Notes", "value": nil}, + "items": []any{ + map[string]any{"node_id": "PVTI_item0"}, + map[string]any{"node_id": "PVTI_item1"}, + map[string]any{"node_id": "PVTI_item2"}, + }, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError, getTextResult(t, result).Text) + + var response map[string]any + require.NoError(t, json.Unmarshal([]byte(getTextResult(t, result).Text), &response)) + assert.Equal(t, float64(3), response["succeeded"]) + + results := response["results"].([]any) + require.Len(t, results, 3) + for i, r := range results { + entry := r.(map[string]any) + assert.Equal(t, float64(i), entry["index"]) + assert.Equal(t, "succeeded", entry["status"]) + assert.Equal(t, fmt.Sprintf("%d", 1000+i), entry["item"].(map[string]any)["full_database_id"]) + } + assert.Len(t, transport.mutationCalls, 1) +} + +func Test_ProjectsWrite_UpdateProjectItems_TransportFailureAbortsLaterChunks(t *testing.T) { + toolDef := ProjectsWrite(translations.NullTranslationHelper) + + queryTransport := githubv4mock.NewMockedHTTPClient( + projectIDMatcher("octo-org", 1, "PVT_project1"), + githubv4mock.NewQueryMatcher( + projectFieldsTestQuery{}, + fieldsQueryVars("octo-org", 1), + githubv4mock.DataResponse(fieldsResponse([]map[string]any{ + fieldNode("PVTF_notes", 101, "Notes", "TEXT"), + })), + ), + ) + transport := &mutationAwareTransport{ + t: t, + queries: queryTransport.Transport, + mutationRespond: func(callIndex int, _ capturedGraphQLRequest) (int, string) { + if callIndex == 0 { + // Systemic transport-level failure: no data at all. + return http.StatusInternalServerError, `{"message":"internal server error"}` + } + t.Fatalf("chunk #%d must not execute after an ambiguous chunk-level failure", callIndex) + return http.StatusInternalServerError, "" + }, + } + gqlClient := newTestGQLClient(transport) + restClient := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{})) + + items := make([]any, 25) + for i := range 25 { + items[i] = map[string]any{"node_id": fmt.Sprintf("PVTI_item%d", i)} + } + + deps := BaseDeps{Client: restClient, GQLClient: gqlClient} + handler := toolDef.Handler(deps) + request := createMCPRequest(map[string]any{ + "method": "update_project_items", + "owner": "octo-org", + "owner_type": "org", + "project_number": float64(1), + "updated_field": map[string]any{"name": "Notes", "value": "x"}, + "items": items, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + // No item succeeded (all unknown after the abort), so IsError is set per + // the "no item succeeded" rule, even though nothing was deterministically + // rejected; the structured result (with unknown statuses) is still available. + assert.True(t, result.IsError) + + var response map[string]any + require.NoError(t, json.Unmarshal([]byte(getTextResult(t, result).Text), &response)) + assert.Equal(t, float64(0), response["succeeded"]) + assert.Equal(t, float64(25), response["unknown"]) + assert.Len(t, transport.mutationCalls, 1, "only the first (failing) chunk should have been sent") + + results := response["results"].([]any) + for _, r := range results { + assert.Equal(t, "unknown", r.(map[string]any)["status"]) + } +} + +func Test_ProjectsWrite_UpdateProjectItems_AllFailedSetsIsError(t *testing.T) { + toolDef := ProjectsWrite(translations.NullTranslationHelper) + restClient := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{})) + mocked := githubv4mock.NewMockedHTTPClient( + projectIDMatcher("octo-org", 1, "PVT_project1"), + ) + countingTransport := &requestCountingTransport{inner: mocked.Transport} + gqlClient := newTestGQLClient(countingTransport) + + deps := BaseDeps{Client: restClient, GQLClient: gqlClient} + handler := toolDef.Handler(deps) + request := createMCPRequest(map[string]any{ + "method": "update_project_items", + "owner": "octo-org", + "owner_type": "org", + "project_number": float64(1), + "updated_field": map[string]any{"name": "Notes", "value": "x"}, + "items": []any{ + map[string]any{}, + map[string]any{"node_id": ""}, + map[string]any{"item_id": float64(0)}, + }, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + assert.True(t, result.IsError, "IsError must be set when no item in the batch succeeds") + + var response map[string]any + require.NoError(t, json.Unmarshal([]byte(getTextResult(t, result).Text), &response)) + assert.Equal(t, float64(0), response["succeeded"]) + assert.Equal(t, float64(3), response["failed"]) + assert.Zero(t, countingTransport.count, "an all-invalid batch should not perform GraphQL resolution") +} + +func Test_ProjectsWrite_UpdateProjectItems_MixedOutcomeKeepsIsErrorFalse(t *testing.T) { + toolDef := ProjectsWrite(translations.NullTranslationHelper) + + queryTransport := githubv4mock.NewMockedHTTPClient( + projectIDMatcher("octo-org", 1, "PVT_project1"), + githubv4mock.NewQueryMatcher( + projectFieldsTestQuery{}, + fieldsQueryVars("octo-org", 1), + githubv4mock.DataResponse(fieldsResponse([]map[string]any{ + fieldNode("PVTF_notes", 101, "Notes", "TEXT"), + })), + ), + ) + transport := &mutationAwareTransport{ + t: t, + queries: queryTransport.Transport, + mutationRespond: func(_ int, _ capturedGraphQLRequest) (int, string) { + return http.StatusOK, mutationDataResponse(t, map[int]struct{ NodeID, FullDatabaseID string }{ + 0: {NodeID: "PVTI_item0", FullDatabaseID: "1000"}, + }) + }, + } + gqlClient := newTestGQLClient(transport) + restClient := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{})) + + deps := BaseDeps{Client: restClient, GQLClient: gqlClient} + handler := toolDef.Handler(deps) + request := createMCPRequest(map[string]any{ + "method": "update_project_items", + "owner": "octo-org", + "owner_type": "org", + "project_number": float64(1), + "updated_field": map[string]any{"name": "Notes", "value": "x"}, + "items": []any{ + map[string]any{"node_id": "PVTI_item0"}, + map[string]any{}, // deterministic failure + }, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + assert.False(t, result.IsError, "mixed outcomes must keep IsError false so the structured result stays available") + + var response map[string]any + require.NoError(t, json.Unmarshal([]byte(getTextResult(t, result).Text), &response)) + assert.Equal(t, float64(1), response["succeeded"]) + assert.Equal(t, float64(1), response["failed"]) +} + +// Test_ProjectsWrite_UpdateProjectItems_EnterpriseClientWiring verifies the +// batch mutation path works unchanged when gqlClient was constructed via +// githubv4.NewEnterpriseClient (GHES), not just githubv4.NewClient: the +// reflection-based mutation logic never assumes a specific endpoint and only +// ever uses the injected client. +func Test_ProjectsWrite_UpdateProjectItems_EnterpriseClientWiring(t *testing.T) { + toolDef := ProjectsWrite(translations.NullTranslationHelper) + + queryTransport := githubv4mock.NewMockedHTTPClient( + projectIDMatcher("octo-org", 1, "PVT_project1"), + githubv4mock.NewQueryMatcher( + projectFieldsTestQuery{}, + fieldsQueryVars("octo-org", 1), + githubv4mock.DataResponse(fieldsResponse([]map[string]any{ + fieldNode("PVTF_notes", 101, "Notes", "TEXT"), + })), + ), + ) + transport := &mutationAwareTransport{ + t: t, + queries: queryTransport.Transport, + mutationRespond: func(_ int, _ capturedGraphQLRequest) (int, string) { + return http.StatusOK, mutationDataResponse(t, map[int]struct{ NodeID, FullDatabaseID string }{ + 0: {NodeID: "PVTI_item0", FullDatabaseID: "1000"}, + }) + }, + } + gqlClient := githubv4.NewEnterpriseClient("https://ghe.example.com/graphql", &http.Client{Transport: transport}) + restClient := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{})) + + deps := BaseDeps{Client: restClient, GQLClient: gqlClient} + handler := toolDef.Handler(deps) + request := createMCPRequest(map[string]any{ + "method": "update_project_items", + "owner": "octo-org", + "owner_type": "org", + "project_number": float64(1), + "updated_field": map[string]any{"name": "Notes", "value": "x"}, + "items": []any{ + map[string]any{"node_id": "PVTI_item0"}, + }, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError, getTextResult(t, result).Text) + + var response map[string]any + require.NoError(t, json.Unmarshal([]byte(getTextResult(t, result).Text), &response)) + assert.Equal(t, float64(1), response["succeeded"]) +} + +func Test_ParseItemRef_ExactlyOneFormRequired(t *testing.T) { + tests := []struct { + name string + entry map[string]any + wantErr string + }{ + { + name: "none provided", + entry: map[string]any{}, + wantErr: "exactly one of", + }, + { + name: "node_id and item_id both provided", + entry: map[string]any{"node_id": "PVTI_x", "item_id": float64(1)}, + wantErr: "not more than one", + }, + { + name: "item_id and issue ref both provided", + entry: map[string]any{"item_id": float64(1), "item_owner": "o", "item_repo": "r", "issue_number": float64(1)}, + wantErr: "not more than one", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + p := parsedBatchItem{} + err := p.parseItemRef(tt.entry) + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + }) + } +} + +func Test_ParseItemRef_NodeIDBypassesLookup(t *testing.T) { + p := parsedBatchItem{} + err := p.parseItemRef(map[string]any{"node_id": "PVTI_abc123"}) + require.NoError(t, err) + assert.Equal(t, batchRefNodeID, p.refKind) + assert.Equal(t, "PVTI_abc123", p.nodeID) +} + +func Test_ParseItemRef_ItemID(t *testing.T) { + p := parsedBatchItem{} + err := p.parseItemRef(map[string]any{"item_id": float64(42)}) + require.NoError(t, err) + assert.Equal(t, batchRefItemID, p.refKind) + assert.Equal(t, int64(42), p.itemID) +} + +func Test_ParseItemRef_IssueRef(t *testing.T) { + p := parsedBatchItem{} + err := p.parseItemRef(map[string]any{"item_owner": "github", "item_repo": "planning-tracking", "issue_number": float64(123)}) + require.NoError(t, err) + assert.Equal(t, batchRefIssue, p.refKind) + assert.Equal(t, "github", p.issueOwner) + assert.Equal(t, "planning-tracking", p.issueRepo) + assert.Equal(t, 123, p.issueNumber) +} + +func Test_ParseItemRef_InvalidNumericReferences(t *testing.T) { + issueRef := func(value any) map[string]any { + return map[string]any{ + "item_owner": "github", + "item_repo": "planning-tracking", + "issue_number": value, + } + } + tests := []struct { + name string + entry map[string]any + }{ + {name: "zero item ID", entry: map[string]any{"item_id": float64(0)}}, + {name: "negative item ID", entry: map[string]any{"item_id": float64(-1)}}, + {name: "fractional item ID", entry: map[string]any{"item_id": float64(1.5)}}, + {name: "NaN item ID", entry: map[string]any{"item_id": math.NaN()}}, + {name: "infinite item ID", entry: map[string]any{"item_id": math.Inf(1)}}, + {name: "overflowing item ID", entry: map[string]any{"item_id": math.MaxFloat64}}, + {name: "zero issue number", entry: issueRef(float64(0))}, + {name: "negative issue number", entry: issueRef(float64(-1))}, + {name: "fractional issue number", entry: issueRef(float64(1.5))}, + {name: "overflowing issue number", entry: issueRef(float64(math.MaxInt32) + 1)}, + {name: "NaN issue number", entry: issueRef(math.NaN())}, + {name: "infinite issue number", entry: issueRef(math.Inf(1))}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + p := parsedBatchItem{} + err := p.parseItemRef(tt.entry) + require.Error(t, err) + }) + } +} + +func Test_ParseItemRef_PartialIssueRefIsError(t *testing.T) { + p := parsedBatchItem{} + err := p.parseItemRef(map[string]any{"item_owner": "github"}) + require.Error(t, err) + assert.Contains(t, err.Error(), "must all be provided together") +} + +func Test_ParseBatchItemEntry_InvalidShape(t *testing.T) { + p := parseBatchItemEntry(0, "not-an-object") + require.NotNil(t, p.err) + assert.Equal(t, "invalid_item", p.err.Code) +} + +func Test_ParseBatchItemEntry_RejectsPerItemUpdatedField(t *testing.T) { + p := parseBatchItemEntry(0, map[string]any{ + "node_id": "PVTI_1", + "updated_field": map[string]any{"name": "Notes", "value": "x"}, + }) + require.NotNil(t, p.err) + assert.Contains(t, p.err.Message, "use the top-level updated_field") +} + +func Test_ConvertProjectFieldValue_Text(t *testing.T) { + field := &ResolvedField{Name: "Notes", DataType: "TEXT"} + v, err := convertProjectFieldValue(field, "hello") + require.NoError(t, err) + require.NotNil(t, v.Text) + assert.Equal(t, "hello", string(*v.Text)) +} + +func Test_ConvertProjectFieldValue_Text_WrongType(t *testing.T) { + field := &ResolvedField{Name: "Notes", DataType: "TEXT"} + _, err := convertProjectFieldValue(field, float64(1)) + require.Error(t, err) +} + +func Test_ConvertProjectFieldValue_Number(t *testing.T) { + field := &ResolvedField{Name: "Estimate", DataType: "NUMBER"} + v, err := convertProjectFieldValue(field, float64(8)) + require.NoError(t, err) + require.NotNil(t, v.Number) + assert.InDelta(t, 8.0, float64(*v.Number), 0.0001) +} + +func Test_ConvertProjectFieldValue_Number_NonFinite(t *testing.T) { + field := &ResolvedField{Name: "Estimate", DataType: "NUMBER"} + for _, value := range []float64{math.NaN(), math.Inf(-1), math.Inf(1)} { + _, err := convertProjectFieldValue(field, value) + require.Error(t, err) + } +} + +func Test_ConvertProjectFieldValue_Date(t *testing.T) { + field := &ResolvedField{Name: "Due", DataType: "DATE"} + v, err := convertProjectFieldValue(field, "2024-01-15") + require.NoError(t, err) + require.NotNil(t, v.Date) + assert.Equal(t, 2024, v.Date.Year()) + assert.Equal(t, 1, int(v.Date.Month())) + assert.Equal(t, 15, v.Date.Day()) +} + +func Test_ConvertProjectFieldValue_Date_BadFormat(t *testing.T) { + field := &ResolvedField{Name: "Due", DataType: "DATE"} + _, err := convertProjectFieldValue(field, "01/15/2024") + require.Error(t, err) +} + +func Test_ConvertProjectFieldValue_SingleSelect_ByName(t *testing.T) { + field := &ResolvedField{ + Name: "Status", + DataType: "SINGLE_SELECT", + Options: []ResolvedFieldOption{{ID: "OPT_1", Name: "In Progress"}}, + } + v, err := convertProjectFieldValue(field, "In Progress") + require.NoError(t, err) + require.NotNil(t, v.SingleSelectOptionID) + assert.Equal(t, "OPT_1", string(*v.SingleSelectOptionID)) +} + +func Test_ConvertProjectFieldValue_SingleSelect_ByOptionID(t *testing.T) { + field := &ResolvedField{ + Name: "Status", + DataType: "SINGLE_SELECT", + Options: []ResolvedFieldOption{{ID: "OPT_1", Name: "In Progress"}}, + } + v, err := convertProjectFieldValue(field, "OPT_1") + require.NoError(t, err) + require.NotNil(t, v.SingleSelectOptionID) + assert.Equal(t, "OPT_1", string(*v.SingleSelectOptionID)) +} + +func Test_ConvertProjectFieldValue_SingleSelect_Unknown(t *testing.T) { + field := &ResolvedField{ + Name: "Status", + DataType: "SINGLE_SELECT", + Options: []ResolvedFieldOption{{ID: "OPT_1", Name: "In Progress"}}, + } + _, err := convertProjectFieldValue(field, "Nonexistent") + require.Error(t, err) +} + +func Test_ConvertProjectFieldValue_Iteration(t *testing.T) { + field := &ResolvedField{Name: "Sprint", DataType: "ITERATION"} + v, err := convertProjectFieldValue(field, "abc123==") + require.NoError(t, err) + require.NotNil(t, v.IterationID) + assert.Equal(t, "abc123==", string(*v.IterationID)) +} + +func Test_ConvertProjectFieldValue_Iteration_EmptyIsError(t *testing.T) { + field := &ResolvedField{Name: "Sprint", DataType: "ITERATION"} + _, err := convertProjectFieldValue(field, "") + require.Error(t, err) +} + +func Test_ConvertProjectFieldValue_UnsupportedDataType(t *testing.T) { + field := &ResolvedField{Name: "Assignees", DataType: "ASSIGNEES"} + _, err := convertProjectFieldValue(field, "someone") + require.Error(t, err) + assert.Contains(t, err.Error(), "unsupported data type") + assert.Contains(t, err.Error(), "update_project_item") +} + +func Test_ResolveBatchProjectField_ByIDAndName(t *testing.T) { + tests := []struct { + name string + spec batchFieldSpec + wantID string + }{ + {name: "numeric ID", spec: batchFieldSpec{id: 101}, wantID: "PVTF_status"}, + {name: "case-insensitive name", spec: batchFieldSpec{name: "priority"}, wantID: "PVTF_priority"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mocked := githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + projectFieldsTestQuery{}, + fieldsQueryVars("octo-org", 7), + githubv4mock.DataResponse(fieldsResponse([]map[string]any{ + statusFieldNode("PVTF_status", 101, "Status", nil), + statusFieldNode("PVTF_priority", 202, "Priority", nil), + })), + ), + ) + + field, err := resolveBatchProjectField(t.Context(), githubv4.NewClient(mocked), "octo-org", "org", 7, tt.spec) + require.NoError(t, err) + assert.Equal(t, tt.wantID, field.NodeID) + }) + } +} + +func Test_ResolveBatchProjectField_AmbiguousName(t *testing.T) { + mocked := githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + projectFieldsTestQuery{}, + fieldsQueryVars("octo-org", 7), + githubv4mock.DataResponse(fieldsResponse([]map[string]any{ + statusFieldNode("PVTSSF_status1", 101, "Status", nil), + statusFieldNode("PVTSSF_status2", 202, "Status", nil), + })), + ), + ) + + _, err := resolveBatchProjectField( + t.Context(), + githubv4.NewClient(mocked), + "octo-org", + "org", + 7, + batchFieldSpec{name: "status"}, + ) + require.Error(t, err) + + var response struct { + Error string `json:"error"` + Candidates []map[string]any `json:"candidates"` + } + require.NoError(t, json.Unmarshal([]byte(err.Error()), &response)) + assert.Equal(t, "field_ambiguous", response.Error) + require.Len(t, response.Candidates, 2) + assert.ElementsMatch(t, []any{"101", "202"}, []any{response.Candidates[0]["id"], response.Candidates[1]["id"]}) +} + +func Test_ResolveItemNodeIDsByNumericID_DeduplicatesOrgAndUserLookups(t *testing.T) { + tests := []struct { + name string + ownerType string + endpoint string + }{ + {name: "organization", ownerType: "org", endpoint: GetOrgsProjectsV2ItemsByProjectByItemID}, + {name: "user", ownerType: "user", endpoint: GetUsersProjectsV2ItemsByUsernameByProjectByItemID}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + calls := 0 + client := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + tt.endpoint: func(w http.ResponseWriter, _ *http.Request) { + calls++ + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"id":1001,"node_id":"PVTI_item1001"}`)) + }, + })) + + resolved := resolveItemNodeIDsByNumericID(t.Context(), client, "octocat", tt.ownerType, 1, []int64{1001, 1001}) + + require.NoError(t, resolved[1001].err) + assert.Equal(t, "PVTI_item1001", resolved[1001].nodeID) + assert.Equal(t, 1, calls) + }) + } +} + +func Test_ResolveIssueRefs_DeduplicatesAndBoundsConcurrency(t *testing.T) { + ctx, cancel := context.WithTimeout(t.Context(), 5*time.Second) + defer cancel() + + gate := make(chan struct{}) + transport := newGatedIssueLookupTransport(gate, "PVT_project") + results := make(chan map[issueRefKey]itemLookupResult, 1) + go func() { + results <- resolveIssueRefs( + ctx, + newTestGQLClient(transport), + githubv4.ID("PVT_project"), + issueBatchItems(1, 2, 3, 4, 5, 6, 1), + ) + }() + + waitForIssueLookups(ctx, t, transport.started, batchItemLookupConcurrency) + active, peak, calls := transport.snapshot() + assert.Equal(t, batchItemLookupConcurrency, active) + assert.Equal(t, batchItemLookupConcurrency, peak) + assert.Len(t, calls, batchItemLookupConcurrency) + + close(gate) + resolved := waitForIssueLookupResults(ctx, t, results) + + require.Len(t, resolved, 6) + for issueNumber := 1; issueNumber <= 6; issueNumber++ { + key := issueRefKey{owner: "octo-org", repo: "roadmap", number: issueNumber} + result, ok := resolved[key] + require.True(t, ok) + require.NoError(t, result.err) + assert.Equal(t, fmt.Sprintf("PVTI_item%d", issueNumber), result.nodeID) + assert.Equal(t, int64(1000+issueNumber), result.fullDatabaseID) + } + + active, peak, calls = transport.snapshot() + assert.Zero(t, active) + assert.Equal(t, batchItemLookupConcurrency, peak) + require.Len(t, calls, 6) + for issueNumber := 1; issueNumber <= 6; issueNumber++ { + assert.Equal(t, 1, calls[issueNumber]) + } +} + +func Test_ResolveIssueRefs_CancellationPopulatesWaitingRefs(t *testing.T) { + testCtx, stop := context.WithTimeout(t.Context(), 5*time.Second) + defer stop() + ctx, cancel := context.WithCancel(testCtx) + defer cancel() + + gate := make(chan struct{}) + defer close(gate) + transport := newGatedIssueLookupTransport(gate, "PVT_project") + results := make(chan map[issueRefKey]itemLookupResult, 1) + go func() { + results <- resolveIssueRefs( + ctx, + newTestGQLClient(transport), + githubv4.ID("PVT_project"), + issueBatchItems(1, 2, 3, 4, 5, 6, 7), + ) + }() + + waitForIssueLookups(testCtx, t, transport.started, batchItemLookupConcurrency) + _, peak, startedCalls := transport.snapshot() + require.Equal(t, batchItemLookupConcurrency, peak) + require.Len(t, startedCalls, batchItemLookupConcurrency) + + cancel() + resolved := waitForIssueLookupResults(testCtx, t, results) + + require.Len(t, resolved, 7) + waiting := 0 + for issueNumber := 1; issueNumber <= 7; issueNumber++ { + key := issueRefKey{owner: "octo-org", repo: "roadmap", number: issueNumber} + result, ok := resolved[key] + require.True(t, ok) + require.ErrorIs(t, result.err, context.Canceled) + if _, started := startedCalls[issueNumber]; !started { + waiting++ + assert.Equal(t, context.Canceled, result.err) + } + } + assert.Equal(t, 2, waiting) + + active, peak, calls := transport.snapshot() + assert.Zero(t, active) + assert.Equal(t, batchItemLookupConcurrency, peak) + assert.Equal(t, startedCalls, calls) +} + +func Test_BatchErrorFromResolution(t *testing.T) { + t.Run("generic wrapped error", func(t *testing.T) { + err := batchErrorFromResolution(fmt.Errorf("item lookup failed: %w", context.DeadlineExceeded)) + + assert.Equal(t, "resolution_failed", err.Code) + assert.Equal(t, "item lookup failed: context deadline exceeded", err.Message) + }) + + t.Run("structured error", func(t *testing.T) { + candidates := []any{map[string]any{"id": "PVTI_1"}} + structured := ghErrors.NewStructuredResolutionError( + "item_not_found", + "octo/repo#42", + "Check that the item belongs to the project.", + candidates, + ) + + err := batchErrorFromResolution(fmt.Errorf("resolve item: %w", structured)) + + assert.Equal(t, structured.Kind, err.Code) + assert.Equal(t, "item_not_found: octo/repo#42", err.Message) + assert.Equal(t, structured.Hint, err.Hint) + assert.Equal(t, candidates, err.Candidates) + }) +} + +func Test_ExecuteBatchWrites_AllAliasGraphQLErrorContinues(t *testing.T) { + transport := &sequencedGraphQLTransport{ + t: t, + responses: []func(capturedGraphQLRequest) (int, string){ + func(_ capturedGraphQLRequest) (int, string) { + return http.StatusOK, mutationErrorResponse(t, map[string]any{}, "all aliases failed") + }, + func(_ capturedGraphQLRequest) (int, string) { + return http.StatusOK, mutationDataResponse(t, map[int]struct{ NodeID, FullDatabaseID string }{ + 0: {NodeID: "PVTI_item20", FullDatabaseID: "1020"}, + }) + }, + }, + } + items, results := batchItemsOfSize(21) + + executeTestBatchWrites(t.Context(), newTestGQLClient(transport), items, results) + + assert.Len(t, transport.calls, 2) + for i := range 20 { + assert.Equal(t, batchItemUnknown, results[i].Status) + } + assert.Equal(t, batchItemSucceeded, results[20].Status) +} + +func Test_ExecuteBatchWrites_PartialGraphQLErrorPreservesSuccess(t *testing.T) { + transport := &sequencedGraphQLTransport{ + t: t, + responses: []func(capturedGraphQLRequest) (int, string){ + func(_ capturedGraphQLRequest) (int, string) { + return http.StatusOK, mutationErrorResponse(t, map[string]any{ + "item0": map[string]any{ + "projectV2Item": map[string]any{"id": "PVTI_item0", "fullDatabaseId": "1000"}, + }, + "item1": nil, + }, "item1 failed") + }, + }, + } + items, results := batchItemsOfSize(2) + + executeTestBatchWrites(t.Context(), newTestGQLClient(transport), items, results) + + assert.Equal(t, batchItemSucceeded, results[0].Status) + assert.Equal(t, items[0].ref, results[0].Ref) + assert.Equal(t, batchItemUnknown, results[1].Status) + assert.Equal(t, items[1].ref, results[1].Ref) +} + +func Test_ExecuteBatchWrites_AmbiguousSuccessResponseAborts(t *testing.T) { + tests := []struct { + name string + body string + confirmedSuccesses int + }{ + { + name: "null data", + body: `{"data":null}`, + }, + { + name: "missing data", + body: `{}`, + }, + { + name: "partial data without errors", + body: mutationDataResponse(t, map[int]struct{ NodeID, FullDatabaseID string }{ + 0: {NodeID: "PVTI_item0", FullDatabaseID: "1000"}, + }), + confirmedSuccesses: 1, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + transport := &sequencedGraphQLTransport{ + t: t, + responses: []func(capturedGraphQLRequest) (int, string){ + func(_ capturedGraphQLRequest) (int, string) { + return http.StatusOK, tt.body + }, + func(_ capturedGraphQLRequest) (int, string) { + return http.StatusOK, mutationDataResponse(t, map[int]struct{ NodeID, FullDatabaseID string }{ + 0: {NodeID: "PVTI_item20", FullDatabaseID: "1020"}, + }) + }, + }, + } + items, results := batchItemsOfSize(21) + + executeTestBatchWrites(t.Context(), newTestGQLClient(transport), items, results) + + assert.Len(t, transport.calls, 1) + for i, result := range results { + if i < tt.confirmedSuccesses { + assert.Equal(t, batchItemSucceeded, result.Status) + continue + } + assert.Equal(t, batchItemUnknown, result.Status) + } + }) + } +} + +func Test_ExecuteBatchWrites_TransportTimeoutAborts(t *testing.T) { + transport := &errorGraphQLTransport{err: context.DeadlineExceeded} + items, results := batchItemsOfSize(21) + + executeTestBatchWrites(t.Context(), newTestGQLClient(transport), items, results) + + assert.Equal(t, 1, transport.calls) + for _, result := range results { + assert.Equal(t, batchItemUnknown, result.Status) + } +} + +func Test_ExecuteBatchWrites_CanceledContextSkipsWrites(t *testing.T) { + ctx, cancel := context.WithCancel(t.Context()) + cancel() + transport := &sequencedGraphQLTransport{t: t} + items, results := batchItemsOfSize(21) + + executeTestBatchWrites(ctx, newTestGQLClient(transport), items, results) + + assert.Empty(t, transport.calls) + for _, result := range results { + assert.Equal(t, batchItemUnknown, result.Status) + } +} + +func executeTestBatchWrites(ctx context.Context, gqlClient *githubv4.Client, items []resolvedBatchItem, results []batchItemResult) { + executeBatchWrites( + ctx, + batchWriteOperation{ + gqlClient: gqlClient, + kind: batchMutationUpdate, + projectID: githubv4.ID("PVT_project"), + fieldID: githubv4.ID("PVTF_field"), + value: githubv4.ProjectV2FieldValue{Text: githubv4.NewString("value")}, + }, + items, + results, + ) +} + +func batchItemsOfSize(n int) ([]resolvedBatchItem, []batchItemResult) { + items := make([]resolvedBatchItem, n) + for i := range n { + nodeID := fmt.Sprintf("PVTI_item%d", i) + items[i] = resolvedBatchItem{ + index: i, + ref: map[string]any{"node_id": nodeID}, + nodeID: nodeID, + } + } + return items, make([]batchItemResult, n) +} diff --git a/pkg/github/projects_test.go b/pkg/github/projects_test.go index 553c2421a4..92de4a5d5f 100644 --- a/pkg/github/projects_test.go +++ b/pkg/github/projects_test.go @@ -866,7 +866,7 @@ func Test_ProjectsWrite(t *testing.T) { require.NoError(t, toolsnaps.Test(toolDef.Tool.Name, toolDef.Tool)) assert.Equal(t, "projects_write", toolDef.Tool.Name) - assert.NotEmpty(t, toolDef.Tool.Description) + assert.Contains(t, toolDef.Tool.Description, "bulk-update many items at once") inputSchema := toolDef.Tool.InputSchema.(*jsonschema.Schema) assert.Contains(t, inputSchema.Properties, "method") assert.Contains(t, inputSchema.Properties, "owner") @@ -879,6 +879,7 @@ func Test_ProjectsWrite(t *testing.T) { assert.Contains(t, inputSchema.Properties, "issue_number") assert.Contains(t, inputSchema.Properties, "pull_request_number") assert.Contains(t, inputSchema.Properties, "updated_field") + assert.Contains(t, inputSchema.Properties, "items") assert.ElementsMatch(t, inputSchema.Required, []string{"method", "owner"}) // Verify DestructiveHint is set @@ -887,6 +888,64 @@ func Test_ProjectsWrite(t *testing.T) { assert.True(t, *toolDef.Tool.Annotations.DestructiveHint) } +func Test_ProjectsWrite_UpdateProjectItemsSchema(t *testing.T) { + inputSchema := ProjectsWrite(translations.NullTranslationHelper).Tool.InputSchema.(*jsonschema.Schema) + assert.Contains(t, inputSchema.Properties["items"].Description, "prefer it over calling 'update_project_item' in a loop") + itemSchema := inputSchema.Properties["items"].Items + + assert.Equal(t, "object", itemSchema.Type) + assert.Empty(t, itemSchema.Properties, "item references should be modeled by oneOf, not flattened properties") + require.Len(t, itemSchema.OneOf, 3) + + expectedRequired := [][]string{ + {"node_id"}, + {"item_id"}, + {"item_owner", "item_repo", "issue_number"}, + } + expectedProperties := [][]string{ + {"node_id"}, + {"item_id"}, + {"item_owner", "item_repo", "issue_number"}, + } + for i, variant := range itemSchema.OneOf { + properties := make([]string, 0, len(variant.Properties)) + for name := range variant.Properties { + properties = append(properties, name) + } + assert.Equal(t, "object", variant.Type) + assert.ElementsMatch(t, expectedRequired[i], variant.Required) + assert.ElementsMatch(t, expectedProperties[i], properties) + for _, property := range variant.Properties { + assert.NotEmpty(t, property.Type) + assert.NotEmpty(t, property.Description) + } + require.NotNil(t, variant.AdditionalProperties) + assert.NotNil(t, variant.AdditionalProperties.Not, "variant must reject additional properties") + } + + fieldSchema := inputSchema.Properties["updated_field"] + assert.Equal(t, "object", fieldSchema.Type) + assert.Contains(t, fieldSchema.Description, "one top-level field/value applies to every item") + require.Len(t, fieldSchema.OneOf, 2) + for i, variant := range fieldSchema.OneOf { + reference := "id" + if i == 1 { + reference = "name" + } + properties := make([]string, 0, len(variant.Properties)) + for name := range variant.Properties { + properties = append(properties, name) + } + assert.ElementsMatch(t, []string{reference, "value"}, variant.Required) + assert.ElementsMatch(t, []string{reference, "value"}, properties) + require.NotNil(t, variant.AdditionalProperties) + assert.NotNil(t, variant.AdditionalProperties.Not) + assert.Empty(t, variant.Properties["value"].Type, "an unconstrained value schema accepts any JSON value, including null") + assert.Empty(t, variant.Properties["value"].Types) + assert.NotEmpty(t, variant.Properties["value"].Description) + } +} + func Test_ProjectsWrite_AddProjectItem(t *testing.T) { toolDef := ProjectsWrite(translations.NullTranslationHelper) From 96a3d782e11598d094ce6ec1e77dbb77fa6b2ac7 Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Tue, 28 Jul 2026 17:21:35 +0200 Subject: [PATCH 4/6] build(deps): bump modelcontextprotocol/go-sdk to v1.7.0 Move from the v1.7.0-pre.3 pre-release to the final v1.7.0 release, which consolidates the pre-releases with no further changes. Regenerate the third-party license files to reflect the new version tag. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 95b8432c-f280-472e-a242-d3ca6dc31f19 --- go.mod | 2 +- go.sum | 4 ++-- third-party-licenses.darwin.md | 4 ++-- third-party-licenses.linux.md | 4 ++-- third-party-licenses.windows.md | 4 ++-- 5 files changed, 9 insertions(+), 9 deletions(-) diff --git a/go.mod b/go.mod index 6dba229115..c96f999428 100644 --- a/go.mod +++ b/go.mod @@ -10,7 +10,7 @@ require ( github.com/josephburnett/jd/v2 v2.5.0 github.com/lithammer/fuzzysearch v1.1.8 github.com/microcosm-cc/bluemonday v1.0.27 - github.com/modelcontextprotocol/go-sdk v1.7.0-pre.3 + github.com/modelcontextprotocol/go-sdk v1.7.0 github.com/muesli/cache2go v0.0.0-20221011235721-518229cd8021 github.com/shurcooL/githubv4 v0.0.0-20240727222349-48295856cce7 github.com/shurcooL/graphql v0.0.0-20230722043721-ed46e5a46466 diff --git a/go.sum b/go.sum index db06724eff..5ddb03aac6 100644 --- a/go.sum +++ b/go.sum @@ -39,8 +39,8 @@ github.com/lithammer/fuzzysearch v1.1.8 h1:/HIuJnjHuXS8bKaiTMeeDlW2/AyIWk2brx1V8 github.com/lithammer/fuzzysearch v1.1.8/go.mod h1:IdqeyBClc3FFqSzYq/MXESsS4S0FsZ5ajtkr5xPLts4= github.com/microcosm-cc/bluemonday v1.0.27 h1:MpEUotklkwCSLeH+Qdx1VJgNqLlpY2KXwXFM08ygZfk= github.com/microcosm-cc/bluemonday v1.0.27/go.mod h1:jFi9vgW+H7c3V0lb6nR74Ib/DIB5OBs92Dimizgw2cA= -github.com/modelcontextprotocol/go-sdk v1.7.0-pre.3 h1:SEAY9IduDif4iApnZgpFkjFIdo3askSGZVbZIYyTy6I= -github.com/modelcontextprotocol/go-sdk v1.7.0-pre.3/go.mod h1:dL7u98E/zjJTGzEq+j30jQ8K2k1mb6LeAH4inEcSGts= +github.com/modelcontextprotocol/go-sdk v1.7.0 h1:yqjY2dsbKAC0LSuWZVBMrHgiG8ukXv6NRo0JiALay44= +github.com/modelcontextprotocol/go-sdk v1.7.0/go.mod h1:dL7u98E/zjJTGzEq+j30jQ8K2k1mb6LeAH4inEcSGts= github.com/muesli/cache2go v0.0.0-20221011235721-518229cd8021 h1:31Y+Yu373ymebRdJN1cWLLooHH8xAr0MhKTEJGV/87g= github.com/muesli/cache2go v0.0.0-20221011235721-518229cd8021/go.mod h1:WERUkUryfUWlrHnFSO/BEUZ+7Ns8aZy7iVOGewxKzcc= github.com/pelletier/go-toml/v2 v2.2.4 h1:mye9XuhQ6gvn5h28+VilKrrPoQVanw5PMw/TB0t5Ec4= diff --git a/third-party-licenses.darwin.md b/third-party-licenses.darwin.md index 6e4581c515..5fb50fdf74 100644 --- a/third-party-licenses.darwin.md +++ b/third-party-licenses.darwin.md @@ -24,8 +24,8 @@ The following packages are included for the amd64, arm64 architectures. - [github.com/josephburnett/jd/v2](https://pkg.go.dev/github.com/josephburnett/jd/v2) ([MIT](https://github.com/josephburnett/jd/blob/v2.5.0/v2/LICENSE)) - [github.com/lithammer/fuzzysearch/fuzzy](https://pkg.go.dev/github.com/lithammer/fuzzysearch/fuzzy) ([MIT](https://github.com/lithammer/fuzzysearch/blob/v1.1.8/LICENSE)) - [github.com/microcosm-cc/bluemonday](https://pkg.go.dev/github.com/microcosm-cc/bluemonday) ([BSD-3-Clause](https://github.com/microcosm-cc/bluemonday/blob/v1.0.27/LICENSE.md)) - - [github.com/modelcontextprotocol/go-sdk](https://pkg.go.dev/github.com/modelcontextprotocol/go-sdk) ([Apache-2.0](https://github.com/modelcontextprotocol/go-sdk/blob/v1.7.0-pre.3/LICENSE)) - - [github.com/modelcontextprotocol/go-sdk](https://pkg.go.dev/github.com/modelcontextprotocol/go-sdk) ([MIT](https://github.com/modelcontextprotocol/go-sdk/blob/v1.7.0-pre.3/LICENSE)) + - [github.com/modelcontextprotocol/go-sdk](https://pkg.go.dev/github.com/modelcontextprotocol/go-sdk) ([Apache-2.0](https://github.com/modelcontextprotocol/go-sdk/blob/v1.7.0/LICENSE)) + - [github.com/modelcontextprotocol/go-sdk](https://pkg.go.dev/github.com/modelcontextprotocol/go-sdk) ([MIT](https://github.com/modelcontextprotocol/go-sdk/blob/v1.7.0/LICENSE)) - [github.com/muesli/cache2go](https://pkg.go.dev/github.com/muesli/cache2go) ([BSD-3-Clause](https://github.com/muesli/cache2go/blob/518229cd8021/LICENSE.txt)) - [github.com/pelletier/go-toml/v2](https://pkg.go.dev/github.com/pelletier/go-toml/v2) ([MIT](https://github.com/pelletier/go-toml/blob/v2.2.4/LICENSE)) - [github.com/sagikazarmark/locafero](https://pkg.go.dev/github.com/sagikazarmark/locafero) ([MIT](https://github.com/sagikazarmark/locafero/blob/v0.11.0/LICENSE)) diff --git a/third-party-licenses.linux.md b/third-party-licenses.linux.md index bdc3cf1fa7..cbb3e5f399 100644 --- a/third-party-licenses.linux.md +++ b/third-party-licenses.linux.md @@ -24,8 +24,8 @@ The following packages are included for the 386, amd64, arm64 architectures. - [github.com/josephburnett/jd/v2](https://pkg.go.dev/github.com/josephburnett/jd/v2) ([MIT](https://github.com/josephburnett/jd/blob/v2.5.0/v2/LICENSE)) - [github.com/lithammer/fuzzysearch/fuzzy](https://pkg.go.dev/github.com/lithammer/fuzzysearch/fuzzy) ([MIT](https://github.com/lithammer/fuzzysearch/blob/v1.1.8/LICENSE)) - [github.com/microcosm-cc/bluemonday](https://pkg.go.dev/github.com/microcosm-cc/bluemonday) ([BSD-3-Clause](https://github.com/microcosm-cc/bluemonday/blob/v1.0.27/LICENSE.md)) - - [github.com/modelcontextprotocol/go-sdk](https://pkg.go.dev/github.com/modelcontextprotocol/go-sdk) ([Apache-2.0](https://github.com/modelcontextprotocol/go-sdk/blob/v1.7.0-pre.3/LICENSE)) - - [github.com/modelcontextprotocol/go-sdk](https://pkg.go.dev/github.com/modelcontextprotocol/go-sdk) ([MIT](https://github.com/modelcontextprotocol/go-sdk/blob/v1.7.0-pre.3/LICENSE)) + - [github.com/modelcontextprotocol/go-sdk](https://pkg.go.dev/github.com/modelcontextprotocol/go-sdk) ([Apache-2.0](https://github.com/modelcontextprotocol/go-sdk/blob/v1.7.0/LICENSE)) + - [github.com/modelcontextprotocol/go-sdk](https://pkg.go.dev/github.com/modelcontextprotocol/go-sdk) ([MIT](https://github.com/modelcontextprotocol/go-sdk/blob/v1.7.0/LICENSE)) - [github.com/muesli/cache2go](https://pkg.go.dev/github.com/muesli/cache2go) ([BSD-3-Clause](https://github.com/muesli/cache2go/blob/518229cd8021/LICENSE.txt)) - [github.com/pelletier/go-toml/v2](https://pkg.go.dev/github.com/pelletier/go-toml/v2) ([MIT](https://github.com/pelletier/go-toml/blob/v2.2.4/LICENSE)) - [github.com/sagikazarmark/locafero](https://pkg.go.dev/github.com/sagikazarmark/locafero) ([MIT](https://github.com/sagikazarmark/locafero/blob/v0.11.0/LICENSE)) diff --git a/third-party-licenses.windows.md b/third-party-licenses.windows.md index da72cebc03..bc7a0f47a4 100644 --- a/third-party-licenses.windows.md +++ b/third-party-licenses.windows.md @@ -25,8 +25,8 @@ The following packages are included for the 386, amd64, arm64 architectures. - [github.com/josephburnett/jd/v2](https://pkg.go.dev/github.com/josephburnett/jd/v2) ([MIT](https://github.com/josephburnett/jd/blob/v2.5.0/v2/LICENSE)) - [github.com/lithammer/fuzzysearch/fuzzy](https://pkg.go.dev/github.com/lithammer/fuzzysearch/fuzzy) ([MIT](https://github.com/lithammer/fuzzysearch/blob/v1.1.8/LICENSE)) - [github.com/microcosm-cc/bluemonday](https://pkg.go.dev/github.com/microcosm-cc/bluemonday) ([BSD-3-Clause](https://github.com/microcosm-cc/bluemonday/blob/v1.0.27/LICENSE.md)) - - [github.com/modelcontextprotocol/go-sdk](https://pkg.go.dev/github.com/modelcontextprotocol/go-sdk) ([Apache-2.0](https://github.com/modelcontextprotocol/go-sdk/blob/v1.7.0-pre.3/LICENSE)) - - [github.com/modelcontextprotocol/go-sdk](https://pkg.go.dev/github.com/modelcontextprotocol/go-sdk) ([MIT](https://github.com/modelcontextprotocol/go-sdk/blob/v1.7.0-pre.3/LICENSE)) + - [github.com/modelcontextprotocol/go-sdk](https://pkg.go.dev/github.com/modelcontextprotocol/go-sdk) ([Apache-2.0](https://github.com/modelcontextprotocol/go-sdk/blob/v1.7.0/LICENSE)) + - [github.com/modelcontextprotocol/go-sdk](https://pkg.go.dev/github.com/modelcontextprotocol/go-sdk) ([MIT](https://github.com/modelcontextprotocol/go-sdk/blob/v1.7.0/LICENSE)) - [github.com/muesli/cache2go](https://pkg.go.dev/github.com/muesli/cache2go) ([BSD-3-Clause](https://github.com/muesli/cache2go/blob/518229cd8021/LICENSE.txt)) - [github.com/pelletier/go-toml/v2](https://pkg.go.dev/github.com/pelletier/go-toml/v2) ([MIT](https://github.com/pelletier/go-toml/blob/v2.2.4/LICENSE)) - [github.com/sagikazarmark/locafero](https://pkg.go.dev/github.com/sagikazarmark/locafero) ([MIT](https://github.com/sagikazarmark/locafero/blob/v0.11.0/LICENSE)) From ea8099d7b29fbb8a8ba6df41f221cc313ccf00c1 Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Tue, 28 Jul 2026 22:25:56 +0200 Subject: [PATCH 5/6] fix: don't advertise unsupported list-changed capabilities The server exposes a static set of tools, prompts, and resources and never mutates them at runtime, so it never emits list_changed notifications. When capabilities are left unset, the go-sdk infers listChanged:true from the presence of items and advertises tools/prompts/resources list-change support we don't actually provide - and the 2026-07-28 spec (subscriptions/listen) tightens expectations around this. Declare empty tools/prompts/resources capabilities in NewMCPServer so both the stdio and remote servers advertise honestly. The remote HTTP handler already set these explicitly; that duplication is now removed in favour of the shared default, leaving only the remote-specific schema cache. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 95b8432c-f280-472e-a242-d3ca6dc31f19 --- pkg/github/server.go | 12 ++++++++++++ pkg/http/handler.go | 8 ++------ 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/pkg/github/server.go b/pkg/github/server.go index 43e0940017..b8f0197889 100644 --- a/pkg/github/server.go +++ b/pkg/github/server.go @@ -89,6 +89,18 @@ func NewMCPServer(ctx context.Context, cfg *MCPServerConfig, deps ToolDependenci Instructions: inv.Instructions(), Logger: cfg.Logger, CompletionHandler: CompletionsHandler(deps.GetClient), + // Advertise tools, prompts, and resources without list-changed + // notifications. The server has a static set of tools/prompts/resources + // and never mutates them at runtime, so it never emits list_changed + // notifications. Left unset, the SDK would infer listChanged:true from + // the presence of items and advertise a capability we don't support - + // which the 2026-07-28 spec (subscriptions/listen) makes stricter still. + // Explicitly declaring these keeps the advertised capabilities honest. + Capabilities: &mcp.ServerCapabilities{ + Tools: &mcp.ToolCapabilities{}, + Prompts: &mcp.PromptCapabilities{}, + Resources: &mcp.ResourceCapabilities{}, + }, } // Apply any additional server options diff --git a/pkg/http/handler.go b/pkg/http/handler.go index eca628a47b..94ee11db04 100644 --- a/pkg/http/handler.go +++ b/pkg/http/handler.go @@ -205,14 +205,10 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { ContentWindowSize: h.config.ContentWindowSize, Logger: h.logger, RepoAccessTTL: h.config.RepoAccessCacheTTL, - // Explicitly set empty capabilities. inv.ForMCPRequest currently returns nothing for Initialize. + // Capabilities (no list-changed advertising) are set by NewMCPServer; + // here we only supply the remote-specific schema cache. ServerOptions: []github.MCPServerOption{ func(so *mcp.ServerOptions) { - so.Capabilities = &mcp.ServerCapabilities{ - Tools: &mcp.ToolCapabilities{}, - Resources: &mcp.ResourceCapabilities{}, - Prompts: &mcp.PromptCapabilities{}, - } so.SchemaCache = h.schemaCache }, }, From ca8ab52dcc45b86fae190398178fd22edb7b1362 Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Thu, 30 Jul 2026 13:37:48 +0200 Subject: [PATCH 6/6] test: assert advertised capabilities omit list-changed Add a regression test locking in the capability contract set by NewMCPServer: tools, prompts, and resources are advertised without list-changed notifications, the deprecated logging capability is not advertised, and the inferred completions capability is preserved. Covers both the stdio path (full inventory, items present) and the HTTP path (inventory emptied for the discovery request), which share the same NewMCPServer entry point. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 95b8432c-f280-472e-a242-d3ca6dc31f19 --- pkg/github/server_test.go | 92 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 92 insertions(+) diff --git a/pkg/github/server_test.go b/pkg/github/server_test.go index bc891fac1f..07cb63c85f 100644 --- a/pkg/github/server_test.go +++ b/pkg/github/server_test.go @@ -11,6 +11,7 @@ import ( "testing" "time" + "github.com/github/github-mcp-server/pkg/inventory" "github.com/github/github-mcp-server/pkg/lockdown" "github.com/github/github-mcp-server/pkg/observability" "github.com/github/github-mcp-server/pkg/observability/metrics" @@ -191,6 +192,97 @@ func TestNewMCPServer_CreatesSuccessfully(t *testing.T) { // is already tested in pkg/github/*_test.go. } +// advertisedServerCapabilities connects an in-memory client to the given server +// and returns the capabilities the server advertised during initialization. +func advertisedServerCapabilities(t *testing.T, server *mcp.Server) *mcp.ServerCapabilities { + t.Helper() + + ctx := context.Background() + clientTransport, serverTransport := mcp.NewInMemoryTransports() + + serverSession, err := server.Connect(ctx, serverTransport, nil) + require.NoError(t, err, "expected server to connect") + t.Cleanup(func() { _ = serverSession.Close() }) + + client := mcp.NewClient(&mcp.Implementation{Name: "test-client", Version: "1.0.0"}, nil) + clientSession, err := client.Connect(ctx, clientTransport, nil) + require.NoError(t, err, "expected client to connect") + t.Cleanup(func() { _ = clientSession.Close() }) + + result := clientSession.InitializeResult() + require.NotNil(t, result, "expected an initialize result") + return result.Capabilities +} + +// TestNewMCPServer_AdvertisedCapabilities locks in the capability contract set by +// NewMCPServer: tools, prompts, and resources are advertised without list-changed +// notifications (the server has a static item set and never emits list_changed), +// the deprecated logging capability is not advertised, and the inferred +// completions capability is preserved. This is asserted for both the stdio path +// (full inventory, items present) and the HTTP path (inventory emptied for the +// discovery/initialize request), which share the same NewMCPServer entry point. +func TestNewMCPServer_AdvertisedCapabilities(t *testing.T) { + t.Parallel() + + cfg := MCPServerConfig{ + Version: "test", + Token: "test-token", + EnabledToolsets: []string{"context"}, + Translator: translations.NullTranslationHelper, + ContentWindowSize: 5000, + } + + deps := stubDeps{obsv: stubExporters()} + + fullInventory, err := NewInventory(cfg.Translator). + WithDeprecatedAliases(DeprecatedToolAliases). + WithToolsets(cfg.EnabledToolsets). + Build() + require.NoError(t, err, "expected inventory build to succeed") + + tests := []struct { + name string + inv *inventory.Inventory + }{ + { + name: "stdio path with registered items", + inv: fullInventory, + }, + { + // The HTTP handler registers only the items relevant to a request; + // for initialize/discover that is nothing, so capabilities must come + // from the explicit declaration rather than being inferred from items. + name: "http path with no registered items for discovery", + inv: fullInventory.ForMCPRequest(inventory.MCPMethodDiscover, ""), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + server, err := NewMCPServer(context.Background(), &cfg, deps, tt.inv) + require.NoError(t, err, "expected server creation to succeed") + + caps := advertisedServerCapabilities(t, server) + + require.NotNil(t, caps.Tools, "tools capability should be advertised") + assert.False(t, caps.Tools.ListChanged, "tools list-changed must not be advertised") + + require.NotNil(t, caps.Prompts, "prompts capability should be advertised") + assert.False(t, caps.Prompts.ListChanged, "prompts list-changed must not be advertised") + + require.NotNil(t, caps.Resources, "resources capability should be advertised") + assert.False(t, caps.Resources.ListChanged, "resources list-changed must not be advertised") + assert.False(t, caps.Resources.Subscribe, "resources subscribe must not be advertised") + + assert.NotNil(t, caps.Completions, "completions capability should be preserved") + // Intentionally asserting the deprecated logging capability is absent. + assert.Nil(t, caps.Logging, "deprecated logging capability should not be advertised") //nolint:staticcheck // SA1019: verifying the deprecated capability is not advertised + }) + } +} + // TestNewServer_NameAndTitleViaTranslation verifies that server name and title // can be overridden via the translation helper (GITHUB_MCP_SERVER_NAME / // GITHUB_MCP_SERVER_TITLE env vars or github-mcp-server-config.json) and