From 6a07793e7bac57e72e88a8acf8187408967e1b88 Mon Sep 17 00:00:00 2001 From: Owen Niblock Date: Tue, 23 Jun 2026 13:48:33 +0100 Subject: [PATCH 1/4] Fall back to REST DELETE when the PATCH can't carry an issue field clear MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The dotcom REST update endpoint uses set semantics for issue_field_values: sending {"issue_field_values": [...]} overwrites the whole list, and anything not in the new list is treated as a deletion. UpdateIssue already exploits this via merge-and-filter — delete:true on a field removes it from the kept list and the server clears it as a side effect. That breaks for one specific case: when the kept list ends up empty (e.g. you're deleting the only remaining field value, or every field in one call), go-github's omitempty tag on `IssueRequest.IssueFieldValues` strips the empty slice from the JSON body. The dotcom REST handler's top-level `if data.include?(ISSUE_FIELD_VALUES)` guard then short-circuits — the key isn't in the payload, so the whole block is skipped and the field keeps its old value. The MCP tool returns success regardless, so a coding agent would happily report the field as cleared. Detect this case in UpdateIssue and fall back to the dedicated REST DELETE endpoint per field: DELETE /repos/{owner}/{repo}/issues/{number}/ issue-field-values/{field_id}. The endpoint is idempotent, takes the integer field ID (no GraphQL node ID needed), and is the same one the public OpenAPI documents. Empirical confirmation on github/github-mcp-server#2756: - Set Priority=P1 via REST - Before the fix: MCP issue_write delete:true returns success, PATCH body on the wire is literally {}, Priority remains P1 (silent no-op). - After the fix: MCP issue_write delete:true returns success, PATCH body is still {}, but the follow-up DELETE clears the field. Priority is cleared as expected. Three new tests in issues_delete_test.go cover: - The omitempty contract (so we know if go-github ever drops the tag) - The 1-of-1 fallback path (PATCH body has no issue_field_values, DELETE fires) - The N-1 set-semantics path (kept list is non-empty, PATCH alone handles it, no DELETE call) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pkg/github/helper_test.go | 1 + pkg/github/issues.go | 41 ++++- pkg/github/issues_delete_test.go | 248 +++++++++++++++++++++++++++++++ 3 files changed, 289 insertions(+), 1 deletion(-) create mode 100644 pkg/github/issues_delete_test.go diff --git a/pkg/github/helper_test.go b/pkg/github/helper_test.go index 2ad173679..e4f00f399 100644 --- a/pkg/github/helper_test.go +++ b/pkg/github/helper_test.go @@ -66,6 +66,7 @@ const ( PostReposIssuesSubIssuesByOwnerByRepoByIssueNumber = "POST /repos/{owner}/{repo}/issues/{issue_number}/sub_issues" DeleteReposIssuesSubIssueByOwnerByRepoByIssueNumber = "DELETE /repos/{owner}/{repo}/issues/{issue_number}/sub_issue" PatchReposIssuesSubIssuesPriorityByOwnerByRepoByIssueNumber = "PATCH /repos/{owner}/{repo}/issues/{issue_number}/sub_issues/priority" + DeleteReposIssuesIssueFieldValueByOwnerByRepoByIssueNumber = "DELETE /repos/{owner}/{repo}/issues/{issue_number}/issue-field-values/{issue_field_id}" // Pull request endpoints GetReposPullsByOwnerByRepo = "GET /repos/{owner}/{repo}/pulls" diff --git a/pkg/github/issues.go b/pkg/github/issues.go index 5479f3579..519248645 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -2179,6 +2179,17 @@ func UpdateIssue(ctx context.Context, client *github.Client, gqlClient *githubv4 issueRequest.Type = github.Ptr(issueType) } + // fallbackDeleteFieldIDs holds field IDs we need to clear via the dedicated + // REST DELETE endpoint after the PATCH lands. This is the omitempty-trap + // fallback: when the merged list is empty AND we have deletions to make, + // `go-github`'s `omitempty` tag on IssueRequest.IssueFieldValues strips the + // empty slice from the JSON body, the dotcom REST handler's top-level + // `if data.include?(ISSUE_FIELD_VALUES)` guard short-circuits, and nothing + // gets cleared. When the merged list is non-empty the PATCH's set semantics + // handle the deletes implicitly (current - new), so no DELETE follow-up is + // needed in that path. + var fallbackDeleteFieldIDs []int64 + if len(issueFieldValues) > 0 || len(fieldIDsToDelete) > 0 { // The REST update endpoint uses "set" semantics — it overwrites all existing // field values with whatever is sent. Fetch the current values first, merge in @@ -2201,7 +2212,15 @@ func UpdateIssue(ctx context.Context, client *github.Client, gqlClient *githubv4 } merged = kept } - issueRequest.IssueFieldValues = merged + if len(merged) == 0 && len(fieldIDsToDelete) > 0 { + // Omitempty trap: skip the IssueFieldValues assignment so we don't + // rely on a value that's about to be stripped from the JSON anyway, + // and clear each field via the dedicated DELETE endpoint after the + // PATCH lands. + fallbackDeleteFieldIDs = fieldIDsToDelete + } else { + issueRequest.IssueFieldValues = merged + } } updatedIssue, resp, err := client.Issues.Edit(ctx, owner, repo, issueNumber, issueRequest) @@ -2222,6 +2241,26 @@ func UpdateIssue(ctx context.Context, client *github.Client, gqlClient *githubv4 return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to update issue", resp, body), nil } + // Clear any fields whose deletion the PATCH couldn't carry. See the + // fallbackDeleteFieldIDs declaration above for the omitempty trap details. + // We hit the dedicated DELETE endpoint per field — it's idempotent, takes + // the integer field ID, and the URL is the standard `/repos/{owner}/{repo}` + // pattern (no need for the numeric repository ID despite what the internal + // route in app/api/issue_field_values.rb suggests; the public API rewrites + // to it). + for _, fieldID := range fallbackDeleteFieldIDs { + path := fmt.Sprintf("repos/%s/%s/issues/%d/issue-field-values/%d", owner, repo, issueNumber, fieldID) + req, err := client.NewRequest(ctx, http.MethodDelete, path, nil) + if err != nil { + return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to build DELETE request for issue field value", nil, err), nil + } + delResp, err := client.Do(req, nil) + if err != nil { + return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to clear issue field value", delResp, err), nil + } + _ = delResp.Body.Close() + } + // Use GraphQL API for state updates if state != "" { // Mandate specifying duplicateOf when trying to close as duplicate diff --git a/pkg/github/issues_delete_test.go b/pkg/github/issues_delete_test.go new file mode 100644 index 000000000..9c7427ab4 --- /dev/null +++ b/pkg/github/issues_delete_test.go @@ -0,0 +1,248 @@ +package github + +import ( + "context" + "encoding/json" + "io" + "net/http" + "sync" + "testing" + + "github.com/github/github-mcp-server/internal/githubv4mock" + gogithub "github.com/google/go-github/v87/github" + "github.com/shurcooL/githubv4" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Test_IssueRequest_EmptyFieldValues_OmittedByJSON pins the omitempty behaviour +// that motivates the DELETE-endpoint fallback in UpdateIssue. If go-github's +// IssueRequest ever drops the `omitempty` tag from `issue_field_values`, this +// test will fail — at which point the fallback could potentially be revisited. +// Until then, an empty `[]*IssueRequestFieldValue{}` is serialised as nothing +// at all, so the REST PATCH alone can never clear a field's last value via +// the set-semantics path. +func Test_IssueRequest_EmptyFieldValues_OmittedByJSON(t *testing.T) { + t.Parallel() + + req := &gogithub.IssueRequest{ + Title: gogithub.Ptr("still here"), + IssueFieldValues: []*gogithub.IssueRequestFieldValue{}, + } + body, err := json.Marshal(req) + require.NoError(t, err) + + assert.NotContains(t, string(body), "issue_field_values", + "empty IssueFieldValues should be dropped by omitempty — this is why the REST PATCH alone can't clear field values when the merged list ends up empty, and why we fall back to the dedicated DELETE endpoint") + assert.Contains(t, string(body), `"title":"still here"`, + "sanity check: other fields still serialise") +} + +// Test_UpdateIssue_DeleteLastFieldValueCallsDeleteEndpoint: regression test for +// the delete:true bug. When the kept set after merge + filter ends up empty +// (e.g. deleting the only remaining field value), the PATCH alone cannot carry +// the deletion intent because go-github strips the empty issue_field_values +// slice via omitempty. UpdateIssue follows up with a per-field DELETE to the +// dedicated `/repos/{owner}/{repo}/issues/{number}/issue-field-values/{id}` +// endpoint. +// +// Asserts both halves: +// +// - the PATCH body does NOT carry an `issue_field_values` key (we don't want +// to double-clear or rely on a value omitempty is about to strip) +// - a DELETE for the field ID fires after the PATCH +func Test_UpdateIssue_DeleteLastFieldValueCallsDeleteEndpoint(t *testing.T) { + t.Parallel() + + mockIssue := &gogithub.Issue{ + Number: gogithub.Ptr(42), + Title: gogithub.Ptr("Test issue"), + State: gogithub.Ptr("open"), + HTMLURL: gogithub.Ptr("https://github.com/owner/repo/issues/42"), + } + + var ( + mu sync.Mutex + capturedPatchBody []byte + deletePaths []string + ) + + restClient := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PatchReposIssuesByOwnerByRepoByIssueNumber: func(w http.ResponseWriter, r *http.Request) { + body, _ := io.ReadAll(r.Body) + mu.Lock() + capturedPatchBody = body + mu.Unlock() + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode(mockIssue) + }, + DeleteReposIssuesIssueFieldValueByOwnerByRepoByIssueNumber: func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + deletePaths = append(deletePaths, r.URL.Path) + mu.Unlock() + w.WriteHeader(http.StatusNoContent) + }, + })) + + // Existing field values for the merge step. Returning only the field about + // to be deleted is the worst case: the kept list ends up empty and the + // fallback DELETE is the only thing that can clear it. + existingFieldsResponse := githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "issue": map[string]any{ + "issueFieldValues": map[string]any{ + "nodes": []any{ + map[string]any{ + "__typename": "IssueFieldSingleSelectValue", + "field": map[string]any{ + "fullDatabaseId": "101", + "name": "Priority", + }, + "value": "P1", + }, + }, + }, + }, + }, + }) + + gqlClient := githubv4.NewClient(githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + struct { + Repository struct { + Issue struct { + IssueFieldValues struct { + Nodes []IssueFieldValueFragment + } `graphql:"issueFieldValues(first: 25)"` + } `graphql:"issue(number: $number)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + }{}, + map[string]any{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + "number": githubv4.Int(42), + }, + existingFieldsResponse, + ), + )) + + result, err := UpdateIssue( + context.Background(), + restClient, + gqlClient, + "owner", "repo", 42, + "", "", nil, nil, 0, "", + nil, + []int64{101}, + "", "", 0, + ) + require.NoError(t, err) + if result.IsError { + t.Fatalf("expected non-error result, got: %s", getTextResult(t, result).Text) + } + + mu.Lock() + defer mu.Unlock() + require.NotContains(t, string(capturedPatchBody), "issue_field_values", + "REST PATCH body must not carry issue_field_values when the kept set is empty (PATCH body was: %s)", string(capturedPatchBody)) + require.Equal(t, []string{"/repos/owner/repo/issues/42/issue-field-values/101"}, deletePaths, + "expected exactly one DELETE call to the dedicated endpoint for field id 101") +} + +// Test_UpdateIssue_DeleteOneOfManyUsesSetSemantics verifies that when the kept +// set after merge + filter is non-empty (deleting 1 of N existing fields), the +// PATCH carries the kept fields and the dotcom REST handler's set semantics do +// the deletion implicitly — no fallback DELETE call is needed. +func Test_UpdateIssue_DeleteOneOfManyUsesSetSemantics(t *testing.T) { + t.Parallel() + + mockIssue := &gogithub.Issue{ + Number: gogithub.Ptr(42), + Title: gogithub.Ptr("Test issue"), + State: gogithub.Ptr("open"), + HTMLURL: gogithub.Ptr("https://github.com/owner/repo/issues/42"), + } + + var ( + mu sync.Mutex + deletePaths []string + ) + + restClient := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PatchReposIssuesByOwnerByRepoByIssueNumber: expectRequestBody(t, map[string]any{ + "issue_field_values": []any{ + map[string]any{"field_id": float64(202), "value": "High"}, + }, + }).andThen( + mockResponse(t, http.StatusOK, mockIssue), + ), + DeleteReposIssuesIssueFieldValueByOwnerByRepoByIssueNumber: func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + deletePaths = append(deletePaths, r.URL.Path) + mu.Unlock() + w.WriteHeader(http.StatusNoContent) + }, + })) + + existingFieldsResponse := githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "issue": map[string]any{ + "issueFieldValues": map[string]any{ + "nodes": []any{ + map[string]any{ + "__typename": "IssueFieldSingleSelectValue", + "field": map[string]any{"fullDatabaseId": "101", "name": "Priority"}, + "value": "P1", + }, + map[string]any{ + "__typename": "IssueFieldSingleSelectValue", + "field": map[string]any{"fullDatabaseId": "202", "name": "Impact"}, + "value": "High", + }, + }, + }, + }, + }, + }) + + gqlClient := githubv4.NewClient(githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + struct { + Repository struct { + Issue struct { + IssueFieldValues struct { + Nodes []IssueFieldValueFragment + } `graphql:"issueFieldValues(first: 25)"` + } `graphql:"issue(number: $number)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + }{}, + map[string]any{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + "number": githubv4.Int(42), + }, + existingFieldsResponse, + ), + )) + + result, err := UpdateIssue( + context.Background(), + restClient, + gqlClient, + "owner", "repo", 42, + "", "", nil, nil, 0, "", + nil, + []int64{101}, + "", "", 0, + ) + require.NoError(t, err) + if result.IsError { + t.Fatalf("expected non-error result, got: %s", getTextResult(t, result).Text) + } + + mu.Lock() + defer mu.Unlock() + require.Empty(t, deletePaths, + "no DELETE call should fire when the kept set is non-empty — the PATCH's set semantics clear the deleted field on the server side") +} From 3b2215482f1605d5c92394231eef7f9081e56edb Mon Sep 17 00:00:00 2001 From: Owen Niblock Date: Wed, 24 Jun 2026 11:27:37 +0100 Subject: [PATCH 2/4] Address review: handle delete-absent-field and DELETE partial failures Two issues surfaced in code review of the original delete fix: 1. Asking to delete a field that isn't set on the issue used to be a silent no-op (PATCH body is {} after omitempty stripping, server skips the issue_field_values block, tool returns success). The fix turned it into a hard error because the fallback DELETE loop fires unconditionally for every field ID in fieldIDsToDelete, and the dotcom DELETE endpoint returns 404 when the field has no value to delete. This breaks idempotent 'ensure field X is cleared' callers that may re-run the same delete on a field that's already been cleared. Fix: before queueing the fallback DELETEs, filter fallbackDeleteFieldIDs down to IDs that actually appeared in the existing field values. This preserves the pre-fix silent-no-op behaviour and avoids a guaranteed 404. 2. The DELETE loop returned on the first error. If a caller asks to clear three fields and the second fails (transient 5xx, rate limit, etc.), the first is gone, the third is never attempted, and the user gets a generic error with no indication of which field failed or which had already succeeded. This is unrecoverable from the caller side. Fix: continue on per-field errors, accumulate the failed and succeeded IDs, and return a single aggregated error naming both sets so callers can retry the right fields. Tests: - Test_UpdateIssue_DeleteAbsentFieldIsNoOp: existing field values is empty, fieldIDsToDelete=[101]. Asserts no DELETE call fires (the mock returns 404 if it does, which would fail the test) and the tool returns success. - Test_UpdateIssue_DeleteFallbackContinuesOnPartialFailure: three fields exist, all three are deleted. Mock returns 500 for the middle DELETE, 204 for the others. Asserts all three DELETEs fired (the middle failure didn't short-circuit the third) and the error result names failed=[202] and cleared=[101 303]. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pkg/github/issues.go | 67 +++++++--- pkg/github/issues_delete_test.go | 211 +++++++++++++++++++++++++++++++ 2 files changed, 262 insertions(+), 16 deletions(-) diff --git a/pkg/github/issues.go b/pkg/github/issues.go index 519248645..100178a77 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -2216,8 +2216,20 @@ func UpdateIssue(ctx context.Context, client *github.Client, gqlClient *githubv4 // Omitempty trap: skip the IssueFieldValues assignment so we don't // rely on a value that's about to be stripped from the JSON anyway, // and clear each field via the dedicated DELETE endpoint after the - // PATCH lands. - fallbackDeleteFieldIDs = fieldIDsToDelete + // PATCH lands. Only queue a DELETE for fields actually present on + // the issue — the dotcom DELETE endpoint returns 404 for absent + // values, and we want to preserve the pre-fix behaviour of treating + // "delete a field that isn't set" as a silent no-op (which is what + // happens when the user idempotently re-runs the same call). + existingIDs := make(map[int64]bool, len(existing)) + for _, e := range existing { + existingIDs[e.FieldID] = true + } + for _, id := range fieldIDsToDelete { + if existingIDs[id] { + fallbackDeleteFieldIDs = append(fallbackDeleteFieldIDs, id) + } + } } else { issueRequest.IssueFieldValues = merged } @@ -2243,22 +2255,45 @@ func UpdateIssue(ctx context.Context, client *github.Client, gqlClient *githubv4 // Clear any fields whose deletion the PATCH couldn't carry. See the // fallbackDeleteFieldIDs declaration above for the omitempty trap details. - // We hit the dedicated DELETE endpoint per field — it's idempotent, takes - // the integer field ID, and the URL is the standard `/repos/{owner}/{repo}` - // pattern (no need for the numeric repository ID despite what the internal - // route in app/api/issue_field_values.rb suggests; the public API rewrites - // to it). - for _, fieldID := range fallbackDeleteFieldIDs { - path := fmt.Sprintf("repos/%s/%s/issues/%d/issue-field-values/%d", owner, repo, issueNumber, fieldID) - req, err := client.NewRequest(ctx, http.MethodDelete, path, nil) - if err != nil { - return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to build DELETE request for issue field value", nil, err), nil + // We hit the dedicated DELETE endpoint per field — it takes the integer + // field ID, and the URL is the standard `/repos/{owner}/{repo}` pattern. + // + // Per-field errors don't short-circuit: each DELETE is attempted, and any + // failures are aggregated into a single error response that names the + // successful and failed field IDs. This avoids leaving the caller blind + // to which deletions landed when one of several fails (e.g. transient + // 5xx on field 2 of 3 — without aggregation, field 1 is already gone and + // field 3 was never attempted, but the caller only sees a generic error). + if len(fallbackDeleteFieldIDs) > 0 { + var failedIDs, succeededIDs []int64 + var firstFailureErr error + var firstFailureResp *github.Response + for _, fieldID := range fallbackDeleteFieldIDs { + path := fmt.Sprintf("repos/%s/%s/issues/%d/issue-field-values/%d", owner, repo, issueNumber, fieldID) + req, err := client.NewRequest(ctx, http.MethodDelete, path, nil) + if err != nil { + failedIDs = append(failedIDs, fieldID) + if firstFailureErr == nil { + firstFailureErr = err + } + continue + } + delResp, err := client.Do(req, nil) + if err != nil { + failedIDs = append(failedIDs, fieldID) + if firstFailureErr == nil { + firstFailureErr = err + firstFailureResp = delResp + } + continue + } + succeededIDs = append(succeededIDs, fieldID) + _ = delResp.Body.Close() } - delResp, err := client.Do(req, nil) - if err != nil { - return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to clear issue field value", delResp, err), nil + if len(failedIDs) > 0 { + msg := fmt.Sprintf("failed to clear issue field values: failed=%v, cleared=%v", failedIDs, succeededIDs) + return ghErrors.NewGitHubAPIErrorResponse(ctx, msg, firstFailureResp, firstFailureErr), nil } - _ = delResp.Body.Close() } // Use GraphQL API for state updates diff --git a/pkg/github/issues_delete_test.go b/pkg/github/issues_delete_test.go index 9c7427ab4..4661339f8 100644 --- a/pkg/github/issues_delete_test.go +++ b/pkg/github/issues_delete_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "io" "net/http" + "strings" "sync" "testing" @@ -246,3 +247,213 @@ func Test_UpdateIssue_DeleteOneOfManyUsesSetSemantics(t *testing.T) { require.Empty(t, deletePaths, "no DELETE call should fire when the kept set is non-empty — the PATCH's set semantics clear the deleted field on the server side") } + +// Test_UpdateIssue_DeleteAbsentFieldIsNoOp verifies that asking to delete a +// field that isn't currently set on the issue does not fire a DELETE request +// to the dedicated endpoint (which would 404). This preserves the pre-fix +// behaviour of treating "delete a field that isn't set" as a silent no-op — +// important because callers often use delete:true idempotently ("ensure +// field X is cleared"), and the second invocation should succeed not error. +func Test_UpdateIssue_DeleteAbsentFieldIsNoOp(t *testing.T) { + t.Parallel() + + mockIssue := &gogithub.Issue{ + Number: gogithub.Ptr(42), + Title: gogithub.Ptr("Test issue"), + State: gogithub.Ptr("open"), + HTMLURL: gogithub.Ptr("https://github.com/owner/repo/issues/42"), + } + + var ( + mu sync.Mutex + capturedPatchBody []byte + deletePaths []string + ) + + restClient := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PatchReposIssuesByOwnerByRepoByIssueNumber: func(w http.ResponseWriter, r *http.Request) { + body, _ := io.ReadAll(r.Body) + mu.Lock() + capturedPatchBody = body + mu.Unlock() + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode(mockIssue) + }, + DeleteReposIssuesIssueFieldValueByOwnerByRepoByIssueNumber: func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + deletePaths = append(deletePaths, r.URL.Path) + mu.Unlock() + // Fail loudly: if we get here, the fix is wrong. + w.WriteHeader(http.StatusNotFound) + }, + })) + + // Issue has no field values at all. + existingFieldsResponse := githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "issue": map[string]any{ + "issueFieldValues": map[string]any{ + "nodes": []any{}, + }, + }, + }, + }) + + gqlClient := githubv4.NewClient(githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + struct { + Repository struct { + Issue struct { + IssueFieldValues struct { + Nodes []IssueFieldValueFragment + } `graphql:"issueFieldValues(first: 25)"` + } `graphql:"issue(number: $number)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + }{}, + map[string]any{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + "number": githubv4.Int(42), + }, + existingFieldsResponse, + ), + )) + + result, err := UpdateIssue( + context.Background(), + restClient, + gqlClient, + "owner", "repo", 42, + "", "", nil, nil, 0, "", + nil, + []int64{101}, // ask to delete a field that isn't set + "", "", 0, + ) + require.NoError(t, err) + if result.IsError { + t.Fatalf("expected non-error result, got: %s", getTextResult(t, result).Text) + } + + mu.Lock() + defer mu.Unlock() + require.NotContains(t, string(capturedPatchBody), "issue_field_values", + "PATCH body must not carry issue_field_values when nothing changed") + require.Empty(t, deletePaths, + "no DELETE call should fire for a field that isn't present on the issue — preserves the pre-fix silent-no-op behaviour and avoids a guaranteed 404") +} + +// Test_UpdateIssue_DeleteFallbackContinuesOnPartialFailure verifies that when +// one DELETE in the fallback loop fails, the remaining DELETEs still fire and +// the aggregated error names the failed and succeeded field IDs. The pre-fix +// loop short-circuited on the first failure, which left the caller blind to +// which deletions had landed. +func Test_UpdateIssue_DeleteFallbackContinuesOnPartialFailure(t *testing.T) { + t.Parallel() + + mockIssue := &gogithub.Issue{ + Number: gogithub.Ptr(42), + Title: gogithub.Ptr("Test issue"), + State: gogithub.Ptr("open"), + HTMLURL: gogithub.Ptr("https://github.com/owner/repo/issues/42"), + } + + var ( + mu sync.Mutex + deletePaths []string + ) + + restClient := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + PatchReposIssuesByOwnerByRepoByIssueNumber: func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode(mockIssue) + }, + DeleteReposIssuesIssueFieldValueByOwnerByRepoByIssueNumber: func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + deletePaths = append(deletePaths, r.URL.Path) + mu.Unlock() + // Field 202 returns 500; fields 101 and 303 succeed. We expect + // all three calls to fire even though the middle one errors, and + // the final tool result must mention 202 as failed and 101/303 + // as cleared. + if strings.HasSuffix(r.URL.Path, "/202") { + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte(`{"message":"simulated failure"}`)) + return + } + w.WriteHeader(http.StatusNoContent) + }, + })) + + existingFieldsResponse := githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "issue": map[string]any{ + "issueFieldValues": map[string]any{ + "nodes": []any{ + map[string]any{ + "__typename": "IssueFieldSingleSelectValue", + "field": map[string]any{"fullDatabaseId": "101", "name": "Priority"}, + "value": "P1", + }, + map[string]any{ + "__typename": "IssueFieldSingleSelectValue", + "field": map[string]any{"fullDatabaseId": "202", "name": "Visibility"}, + "value": "High", + }, + map[string]any{ + "__typename": "IssueFieldSingleSelectValue", + "field": map[string]any{"fullDatabaseId": "303", "name": "Impact"}, + "value": "Critical", + }, + }, + }, + }, + }, + }) + + gqlClient := githubv4.NewClient(githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + struct { + Repository struct { + Issue struct { + IssueFieldValues struct { + Nodes []IssueFieldValueFragment + } `graphql:"issueFieldValues(first: 25)"` + } `graphql:"issue(number: $number)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + }{}, + map[string]any{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + "number": githubv4.Int(42), + }, + existingFieldsResponse, + ), + )) + + result, err := UpdateIssue( + context.Background(), + restClient, + gqlClient, + "owner", "repo", 42, + "", "", nil, nil, 0, "", + nil, + []int64{101, 202, 303}, + "", "", 0, + ) + require.NoError(t, err) + require.True(t, result.IsError, "expected an error result because field 202 failed") + + mu.Lock() + defer mu.Unlock() + // All three DELETEs must have fired — the middle failure must not short-circuit the third. + require.Len(t, deletePaths, 3, + "all three DELETE calls should fire even though one fails; got paths: %v", deletePaths) + + resultText := getTextResult(t, result).Text + require.Contains(t, resultText, "failed=[202]", + "error must name the failed field ID so the caller can retry it; got: %s", resultText) + require.Contains(t, resultText, "cleared=[101 303]", + "error must name the cleared field IDs so the caller knows what's already done; got: %s", resultText) +} From a02f4d35e2124cf6c873ce6dd7f79016bccca632 Mon Sep 17 00:00:00 2001 From: Owen Niblock Date: Thu, 25 Jun 2026 16:25:50 +0100 Subject: [PATCH 3/4] Slim down code comments in delete-fix region Trim the doc comments on UpdateIssue's omitempty-trap region + test file to the bare minimum needed to understand the non-obvious behaviour (why the DELETE fallback exists, why we filter to existing IDs, why errors are aggregated). Drop restated context and step-by-step explanations. No behaviour change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pkg/github/issues.go | 43 ++++++--------------- pkg/github/issues_delete_test.go | 64 +++++++++++--------------------- 2 files changed, 34 insertions(+), 73 deletions(-) diff --git a/pkg/github/issues.go b/pkg/github/issues.go index 100178a77..b51b5191d 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -2179,21 +2179,13 @@ func UpdateIssue(ctx context.Context, client *github.Client, gqlClient *githubv4 issueRequest.Type = github.Ptr(issueType) } - // fallbackDeleteFieldIDs holds field IDs we need to clear via the dedicated - // REST DELETE endpoint after the PATCH lands. This is the omitempty-trap - // fallback: when the merged list is empty AND we have deletions to make, - // `go-github`'s `omitempty` tag on IssueRequest.IssueFieldValues strips the - // empty slice from the JSON body, the dotcom REST handler's top-level - // `if data.include?(ISSUE_FIELD_VALUES)` guard short-circuits, and nothing - // gets cleared. When the merged list is non-empty the PATCH's set semantics - // handle the deletes implicitly (current - new), so no DELETE follow-up is - // needed in that path. + // Field IDs to clear via DELETE after the PATCH. See the post-PATCH loop + // for why we can't just rely on REST set semantics. var fallbackDeleteFieldIDs []int64 if len(issueFieldValues) > 0 || len(fieldIDsToDelete) > 0 { - // The REST update endpoint uses "set" semantics — it overwrites all existing - // field values with whatever is sent. Fetch the current values first, merge in - // the new values, then remove any explicitly deleted fields. + // REST PATCH uses set semantics, so fetch existing values, merge in + // the new ones, then drop anything explicitly deleted. existing, err := fetchExistingIssueFieldValues(ctx, gqlClient, owner, repo, issueNumber) if err != nil { return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "failed to fetch existing issue field values", err), nil @@ -2213,14 +2205,9 @@ func UpdateIssue(ctx context.Context, client *github.Client, gqlClient *githubv4 merged = kept } if len(merged) == 0 && len(fieldIDsToDelete) > 0 { - // Omitempty trap: skip the IssueFieldValues assignment so we don't - // rely on a value that's about to be stripped from the JSON anyway, - // and clear each field via the dedicated DELETE endpoint after the - // PATCH lands. Only queue a DELETE for fields actually present on - // the issue — the dotcom DELETE endpoint returns 404 for absent - // values, and we want to preserve the pre-fix behaviour of treating - // "delete a field that isn't set" as a silent no-op (which is what - // happens when the user idempotently re-runs the same call). + // Only queue DELETEs for fields actually present — the endpoint + // returns 404 otherwise, and "delete a field that isn't set" should + // stay a no-op (callers often invoke delete:true idempotently). existingIDs := make(map[int64]bool, len(existing)) for _, e := range existing { existingIDs[e.FieldID] = true @@ -2253,17 +2240,11 @@ func UpdateIssue(ctx context.Context, client *github.Client, gqlClient *githubv4 return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to update issue", resp, body), nil } - // Clear any fields whose deletion the PATCH couldn't carry. See the - // fallbackDeleteFieldIDs declaration above for the omitempty trap details. - // We hit the dedicated DELETE endpoint per field — it takes the integer - // field ID, and the URL is the standard `/repos/{owner}/{repo}` pattern. - // - // Per-field errors don't short-circuit: each DELETE is attempted, and any - // failures are aggregated into a single error response that names the - // successful and failed field IDs. This avoids leaving the caller blind - // to which deletions landed when one of several fails (e.g. transient - // 5xx on field 2 of 3 — without aggregation, field 1 is already gone and - // field 3 was never attempted, but the caller only sees a generic error). + // Per-field DELETE fallback. The PATCH can't clear field values when the + // merged set is empty — go-github's `omitempty` strips the empty slice + // and the dotcom REST handler skips its issue_field_values block when the + // key is absent. Errors are aggregated (not short-circuited) so callers + // can see which fields succeeded and which need retry. if len(fallbackDeleteFieldIDs) > 0 { var failedIDs, succeededIDs []int64 var firstFailureErr error diff --git a/pkg/github/issues_delete_test.go b/pkg/github/issues_delete_test.go index 4661339f8..f61b65ece 100644 --- a/pkg/github/issues_delete_test.go +++ b/pkg/github/issues_delete_test.go @@ -16,13 +16,10 @@ import ( "github.com/stretchr/testify/require" ) -// Test_IssueRequest_EmptyFieldValues_OmittedByJSON pins the omitempty behaviour -// that motivates the DELETE-endpoint fallback in UpdateIssue. If go-github's -// IssueRequest ever drops the `omitempty` tag from `issue_field_values`, this -// test will fail — at which point the fallback could potentially be revisited. -// Until then, an empty `[]*IssueRequestFieldValue{}` is serialised as nothing -// at all, so the REST PATCH alone can never clear a field's last value via -// the set-semantics path. +// Test_IssueRequest_EmptyFieldValues_OmittedByJSON pins the omitempty +// behaviour that makes the DELETE fallback necessary. If go-github ever drops +// the tag, the REST PATCH alone could clear field values and this test would +// fail to remind us. func Test_IssueRequest_EmptyFieldValues_OmittedByJSON(t *testing.T) { t.Parallel() @@ -39,19 +36,10 @@ func Test_IssueRequest_EmptyFieldValues_OmittedByJSON(t *testing.T) { "sanity check: other fields still serialise") } -// Test_UpdateIssue_DeleteLastFieldValueCallsDeleteEndpoint: regression test for -// the delete:true bug. When the kept set after merge + filter ends up empty -// (e.g. deleting the only remaining field value), the PATCH alone cannot carry -// the deletion intent because go-github strips the empty issue_field_values -// slice via omitempty. UpdateIssue follows up with a per-field DELETE to the -// dedicated `/repos/{owner}/{repo}/issues/{number}/issue-field-values/{id}` -// endpoint. -// -// Asserts both halves: -// -// - the PATCH body does NOT carry an `issue_field_values` key (we don't want -// to double-clear or rely on a value omitempty is about to strip) -// - a DELETE for the field ID fires after the PATCH +// Test_UpdateIssue_DeleteLastFieldValueCallsDeleteEndpoint covers the bug fix: +// when the kept set ends up empty, the PATCH alone can't clear the field +// (omitempty strips the empty slice), so UpdateIssue follows up with a DELETE +// to the dedicated endpoint. func Test_UpdateIssue_DeleteLastFieldValueCallsDeleteEndpoint(t *testing.T) { t.Parallel() @@ -86,9 +74,8 @@ func Test_UpdateIssue_DeleteLastFieldValueCallsDeleteEndpoint(t *testing.T) { }, })) - // Existing field values for the merge step. Returning only the field about - // to be deleted is the worst case: the kept list ends up empty and the - // fallback DELETE is the only thing that can clear it. + // Existing field values for the merge step. Returning the field we're + // about to delete makes the kept list empty, triggering the fallback DELETE. existingFieldsResponse := githubv4mock.DataResponse(map[string]any{ "repository": map[string]any{ "issue": map[string]any{ @@ -151,10 +138,9 @@ func Test_UpdateIssue_DeleteLastFieldValueCallsDeleteEndpoint(t *testing.T) { "expected exactly one DELETE call to the dedicated endpoint for field id 101") } -// Test_UpdateIssue_DeleteOneOfManyUsesSetSemantics verifies that when the kept -// set after merge + filter is non-empty (deleting 1 of N existing fields), the -// PATCH carries the kept fields and the dotcom REST handler's set semantics do -// the deletion implicitly — no fallback DELETE call is needed. +// Test_UpdateIssue_DeleteOneOfManyUsesSetSemantics: when the kept set is +// non-empty, set semantics handle the deletion implicitly via the PATCH — no +// DELETE follow-up needed. func Test_UpdateIssue_DeleteOneOfManyUsesSetSemantics(t *testing.T) { t.Parallel() @@ -248,12 +234,10 @@ func Test_UpdateIssue_DeleteOneOfManyUsesSetSemantics(t *testing.T) { "no DELETE call should fire when the kept set is non-empty — the PATCH's set semantics clear the deleted field on the server side") } -// Test_UpdateIssue_DeleteAbsentFieldIsNoOp verifies that asking to delete a -// field that isn't currently set on the issue does not fire a DELETE request -// to the dedicated endpoint (which would 404). This preserves the pre-fix -// behaviour of treating "delete a field that isn't set" as a silent no-op — -// important because callers often use delete:true idempotently ("ensure -// field X is cleared"), and the second invocation should succeed not error. +// Test_UpdateIssue_DeleteAbsentFieldIsNoOp: deleting a field that isn't set +// must not fire a DELETE (the endpoint would 404), preserving the pre-fix +// silent-no-op behaviour so idempotent delete:true callers don't break on +// retry. func Test_UpdateIssue_DeleteAbsentFieldIsNoOp(t *testing.T) { t.Parallel() @@ -343,11 +327,9 @@ func Test_UpdateIssue_DeleteAbsentFieldIsNoOp(t *testing.T) { "no DELETE call should fire for a field that isn't present on the issue — preserves the pre-fix silent-no-op behaviour and avoids a guaranteed 404") } -// Test_UpdateIssue_DeleteFallbackContinuesOnPartialFailure verifies that when -// one DELETE in the fallback loop fails, the remaining DELETEs still fire and -// the aggregated error names the failed and succeeded field IDs. The pre-fix -// loop short-circuited on the first failure, which left the caller blind to -// which deletions had landed. +// Test_UpdateIssue_DeleteFallbackContinuesOnPartialFailure: a failing DELETE +// must not short-circuit subsequent ones, and the error must name which IDs +// succeeded and which failed so callers can retry the right ones. func Test_UpdateIssue_DeleteFallbackContinuesOnPartialFailure(t *testing.T) { t.Parallel() @@ -373,10 +355,8 @@ func Test_UpdateIssue_DeleteFallbackContinuesOnPartialFailure(t *testing.T) { mu.Lock() deletePaths = append(deletePaths, r.URL.Path) mu.Unlock() - // Field 202 returns 500; fields 101 and 303 succeed. We expect - // all three calls to fire even though the middle one errors, and - // the final tool result must mention 202 as failed and 101/303 - // as cleared. + // Field 202 fails; 101 and 303 succeed. All three should fire and + // the error must name 202 as failed and 101/303 as cleared. if strings.HasSuffix(r.URL.Path, "/202") { w.WriteHeader(http.StatusInternalServerError) _, _ = w.Write([]byte(`{"message":"simulated failure"}`)) From 07aca5d7458710a3fc226c0f012ef0abbfc31a80 Mon Sep 17 00:00:00 2001 From: Owen Niblock Date: Tue, 23 Jun 2026 13:54:33 +0100 Subject: [PATCH 4/4] Add multi-select issue field support, gated behind FF MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Multi-select issue fields ride on the existing custom-fields surface: - issue_write (consolidated) and set_issue_fields (granular) gain multi-select inputs (field_option_names / multi_select_option_ids) - list_issues field_filters gain a 'values' slot for multi-select filtering with AND semantics - list_issue_fields advertises multi_select in its description and surfaces multi-select definitions - Read paths (IssueFieldValueFragment, list_issues enrichment, etc.) decode multi-select values when an org has them The write surface is gated behind a new FF remote_mcp_issue_fields_multiselect. When the flag is off, the legacy variants of issue_write, list_issues, and list_issue_fields are served — same handler bodies, but their schemas and descriptions omit multi_select. Read paths stay unchanged: orgs that have dotcom-side multi-select enabled continue to see multi-select VALUES surfaced, matching what the dotcom UI shows. The granular tools (set_issue_fields) are not separately gated — they are already behind FeatureFlagIssuesGranular, which is itself a user-opt-in rollout flag. Double-gating adds complexity without proportionate benefit for users who have already accepted experimental territory. Per the user's preference for code duplication over interleaved branching: - IssueWrite and IssueWriteLegacy are full siblings (issues.go + issues_legacy_multiselect.go). The shared parser and resolver (optionalIssueWriteFields, resolveIssueRequestFieldValues) take a single multiSelectEnabled bool and reject multi-select inputs/fields when false. - ListIssues / ListIssuesLegacy share a buildListIssues helper that swaps in the right descriptions and adds/removes the field_filters[] values slot. - ListIssueFields / ListIssueFieldsLegacy share a buildListIssueFields helper that swaps the description (handler is identical). Snapshot naming follows the established convention: legacy variants own the canonical .snap; MS-aware variants own _ff_remote_mcp_issue_fields_multiselect.snap. Stacked on #2755 (the universal delete-fix half of the original PR). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/feature-flags.md | 38 +- docs/insiders-features.md | 36 ++ ...f_remote_mcp_issue_fields_multiselect.snap | 144 ++++++ ...f_remote_mcp_issue_fields_multiselect.snap | 24 + ...f_remote_mcp_issue_fields_multiselect.snap | 98 ++++ .../__toolsnaps__/set_issue_fields.snap | 9 +- pkg/github/feature_flags.go | 6 + pkg/github/granular_tools_test.go | 158 ++++++ pkg/github/issue_fields.go | 67 ++- pkg/github/issue_fields_test.go | 14 +- pkg/github/issues.go | 443 +++++++++++++--- pkg/github/issues_granular.go | 33 +- pkg/github/issues_legacy_multiselect.go | 295 +++++++++++ pkg/github/issues_multiselect_test.go | 479 ++++++++++++++++++ pkg/github/issues_test.go | 294 ++++++----- pkg/github/minimal_types.go | 9 + pkg/github/tools.go | 3 + 17 files changed, 1922 insertions(+), 228 deletions(-) create mode 100644 pkg/github/__toolsnaps__/issue_write_ff_remote_mcp_issue_fields_multiselect.snap create mode 100644 pkg/github/__toolsnaps__/list_issue_fields_ff_remote_mcp_issue_fields_multiselect.snap create mode 100644 pkg/github/__toolsnaps__/list_issues_ff_remote_mcp_issue_fields_multiselect.snap create mode 100644 pkg/github/issues_legacy_multiselect.go create mode 100644 pkg/github/issues_multiselect_test.go diff --git a/docs/feature-flags.md b/docs/feature-flags.md index 6ebc2dec9..abd47ab3c 100644 --- a/docs/feature-flags.md +++ b/docs/feature-flags.md @@ -131,7 +131,7 @@ runtime behavior (such as output formatting) won't appear here. - **set_issue_fields** - Set Issue Fields - **Required OAuth Scopes**: `repo` - - `fields`: Array of issue field values to set. Each element must have a 'field_id' (string, the GraphQL node ID of the field) and exactly one value field: 'text_value' for text fields, 'number_value' for number fields, 'date_value' (ISO 8601 date string) for date fields, or 'single_select_option_id' (the GraphQL node ID of the option) for single select fields. Set 'delete' to true to remove a field value. (object[], required) + - `fields`: Array of issue field values to set. Each element must have a 'field_id' (string, the GraphQL node ID of the field) and exactly one value field: 'text_value' for text fields, 'number_value' for number fields, 'date_value' (ISO 8601 date string) for date fields, 'single_select_option_id' (the GraphQL node ID of the option) for single select fields, or 'multi_select_option_ids' (an array of GraphQL node IDs) for multi select fields. Set 'delete' to true to remove a field value. (object[], required) - `issue_number`: The issue number to update (number, required) - `owner`: Repository owner (username or organization) (string, required) - `repo`: Repository name (string, required) @@ -283,4 +283,40 @@ runtime behavior (such as output formatting) won't appear here. - `repo`: Repository name (string, required) - `start_line`: Optional 1-based starting line of the window of interest. Only ranges overlapping [start_line, end_line] are returned, clamped to the window. (number, optional) +### `remote_mcp_issue_fields_multiselect` + +- **issue_write** - Create or update issue/pull request + - **Required OAuth Scopes**: `repo` + - `assignees`: Usernames to assign to this issue (string[], optional) + - `body`: Issue body content (string, optional) + - `duplicate_of`: Issue number that this issue is a duplicate of. Only used when state_reason is 'duplicate'. (number, optional) + - `issue_fields`: Issue field values to set or clear. Each item requires 'field_name' and exactly one of 'value', 'field_option_name', 'field_option_names', or 'delete: true'. (object[], optional) + - `issue_number`: Issue number to update (number, optional) + - `labels`: Labels to apply to this issue (string[], optional) + - `method`: Write operation to perform on a single issue. + Options are: + - 'create' - creates a new issue. + - 'update' - updates an existing issue. + (string, required) + - `milestone`: Milestone number (number, optional) + - `owner`: Repository owner (string, required) + - `repo`: Repository name (string, required) + - `state`: New state (string, optional) + - `state_reason`: Reason for the state change. Ignored unless state is changed. (string, optional) + - `title`: Issue title (string, optional) + - `type`: Type of this issue. Only use if issue types are enabled for this repository. Use list_issue_types tool to get valid type values for this repository or its owner organization. If the repository doesn't support issue types, omit this parameter. (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 either 'value' (text, number, YYYY-MM-DD date, or single-select option name) or 'values' (multi-select option names). For multi-select fields, all listed values must be set on an issue for it to match (AND semantics) — to match any-of, make multiple list_issues calls and union the results. (object[], 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) + diff --git a/docs/insiders-features.md b/docs/insiders-features.md index 37488f544..2c1619816 100644 --- a/docs/insiders-features.md +++ b/docs/insiders-features.md @@ -103,6 +103,42 @@ The list below is generated from the Go source. It covers tool **inventory and s - `repo`: Repository name (string, required) - `start_line`: Optional 1-based starting line of the window of interest. Only ranges overlapping [start_line, end_line] are returned, clamped to the window. (number, optional) +### `remote_mcp_issue_fields_multiselect` + +- **issue_write** - Create or update issue/pull request + - **Required OAuth Scopes**: `repo` + - `assignees`: Usernames to assign to this issue (string[], optional) + - `body`: Issue body content (string, optional) + - `duplicate_of`: Issue number that this issue is a duplicate of. Only used when state_reason is 'duplicate'. (number, optional) + - `issue_fields`: Issue field values to set or clear. Each item requires 'field_name' and exactly one of 'value', 'field_option_name', 'field_option_names', or 'delete: true'. (object[], optional) + - `issue_number`: Issue number to update (number, optional) + - `labels`: Labels to apply to this issue (string[], optional) + - `method`: Write operation to perform on a single issue. + Options are: + - 'create' - creates a new issue. + - 'update' - updates an existing issue. + (string, required) + - `milestone`: Milestone number (number, optional) + - `owner`: Repository owner (string, required) + - `repo`: Repository name (string, required) + - `state`: New state (string, optional) + - `state_reason`: Reason for the state change. Ignored unless state is changed. (string, optional) + - `title`: Issue title (string, optional) + - `type`: Type of this issue. Only use if issue types are enabled for this repository. Use list_issue_types tool to get valid type values for this repository or its owner organization. If the repository doesn't support issue types, omit this parameter. (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 either 'value' (text, number, YYYY-MM-DD date, or single-select option name) or 'values' (multi-select option names). For multi-select fields, all listed values must be set on an issue for it to match (AND semantics) — to match any-of, make multiple list_issues calls and union the results. (object[], 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) + --- diff --git a/pkg/github/__toolsnaps__/issue_write_ff_remote_mcp_issue_fields_multiselect.snap b/pkg/github/__toolsnaps__/issue_write_ff_remote_mcp_issue_fields_multiselect.snap new file mode 100644 index 000000000..2f4db70bd --- /dev/null +++ b/pkg/github/__toolsnaps__/issue_write_ff_remote_mcp_issue_fields_multiselect.snap @@ -0,0 +1,144 @@ +{ + "_meta": { + "ui": { + "resourceUri": "ui://github-mcp-server/issue-write", + "visibility": [ + "model", + "app" + ] + } + }, + "annotations": { + "title": "Create or update issue/pull request" + }, + "description": "Create a new or update an existing issue in a GitHub repository.", + "inputSchema": { + "properties": { + "assignees": { + "description": "Usernames to assign to this issue", + "items": { + "type": "string" + }, + "type": "array" + }, + "body": { + "description": "Issue body content", + "type": "string" + }, + "duplicate_of": { + "description": "Issue number that this issue is a duplicate of. Only used when state_reason is 'duplicate'.", + "type": "number" + }, + "issue_fields": { + "description": "Issue field values to set or clear. Each item requires 'field_name' and exactly one of 'value', 'field_option_name', 'field_option_names', or 'delete: true'.", + "items": { + "additionalProperties": false, + "properties": { + "delete": { + "description": "Set to true to clear this field's current value on the issue. Cannot be combined with 'value', 'field_option_name', or 'field_option_names'.", + "enum": [ + true + ], + "type": "boolean" + }, + "field_name": { + "description": "Issue field name (case-insensitive). Must match a field returned by list_issue_fields for this repository or its organization.", + "type": "string" + }, + "field_option_name": { + "description": "Option name for single-select fields. Validated against the field's options before the API call. Cannot be combined with 'value', 'field_option_names', or 'delete'.", + "type": "string" + }, + "field_option_names": { + "description": "Option names for multi-select fields. All names are validated against the field's options before the API call. An empty array is rejected — use 'delete: true' to clear the field. Cannot be combined with 'value', 'field_option_name', or 'delete'.", + "items": { + "type": "string" + }, + "type": "array" + }, + "value": { + "description": "Value to set. Use for text, number, and date fields (date as YYYY-MM-DD). For single-select fields, prefer 'field_option_name' so the option is validated before the API call. Cannot be combined with 'field_option_name', 'field_option_names', or 'delete'.", + "type": [ + "string", + "number", + "boolean" + ] + } + }, + "required": [ + "field_name" + ], + "type": "object" + }, + "type": "array" + }, + "issue_number": { + "description": "Issue number to update", + "type": "number" + }, + "labels": { + "description": "Labels to apply to this issue", + "items": { + "type": "string" + }, + "type": "array" + }, + "method": { + "description": "Write operation to perform on a single issue.\nOptions are:\n- 'create' - creates a new issue.\n- 'update' - updates an existing issue.\n", + "enum": [ + "create", + "update" + ], + "type": "string" + }, + "milestone": { + "description": "Milestone number", + "type": "number" + }, + "owner": { + "description": "Repository owner", + "type": "string" + }, + "repo": { + "description": "Repository name", + "type": "string" + }, + "show_ui": { + "description": "Whether to render the MCP App form instead of executing the request immediately. Defaults to true. Set to false to skip the form and execute directly — useful when you have all required values (especially ones the form does not collect, like labels, assignees, milestone, type, issue_fields, or state changes) and the user has already confirmed the action.", + "type": "boolean" + }, + "state": { + "description": "New state", + "enum": [ + "open", + "closed" + ], + "type": "string" + }, + "state_reason": { + "description": "Reason for the state change. Ignored unless state is changed.", + "enum": [ + "completed", + "not_planned", + "duplicate" + ], + "type": "string" + }, + "title": { + "description": "Issue title", + "type": "string" + }, + "type": { + "description": "Type of this issue. Only use if issue types are enabled for this repository. Use list_issue_types tool to get valid type values for this repository or its owner organization. If the repository doesn't support issue types, omit this parameter.", + "type": "string" + } + }, + "required": [ + "method", + "owner", + "repo" + ], + "type": "object" + }, + "name": "issue_write" +} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/list_issue_fields_ff_remote_mcp_issue_fields_multiselect.snap b/pkg/github/__toolsnaps__/list_issue_fields_ff_remote_mcp_issue_fields_multiselect.snap new file mode 100644 index 000000000..e602cd677 --- /dev/null +++ b/pkg/github/__toolsnaps__/list_issue_fields_ff_remote_mcp_issue_fields_multiselect.snap @@ -0,0 +1,24 @@ +{ + "annotations": { + "readOnlyHint": true, + "title": "List issue fields" + }, + "description": "List issue fields for a repository or organization. Returns field definitions including name, type (text, number, date, single_select, multi_select), and for single_select and multi_select fields the list of valid option names. When repo is omitted, returns org-level fields directly.", + "inputSchema": { + "properties": { + "owner": { + "description": "The account owner of the repository or organization. The name is not case sensitive.", + "type": "string" + }, + "repo": { + "description": "The name of the repository. When provided, returns fields for this specific repository (inherited from its organization). When omitted, returns org-level fields directly.", + "type": "string" + } + }, + "required": [ + "owner" + ], + "type": "object" + }, + "name": "list_issue_fields" +} \ No newline at end of file diff --git a/pkg/github/__toolsnaps__/list_issues_ff_remote_mcp_issue_fields_multiselect.snap b/pkg/github/__toolsnaps__/list_issues_ff_remote_mcp_issue_fields_multiselect.snap new file mode 100644 index 000000000..0a1719b7a --- /dev/null +++ b/pkg/github/__toolsnaps__/list_issues_ff_remote_mcp_issue_fields_multiselect.snap @@ -0,0 +1,98 @@ +{ + "annotations": { + "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 either 'value' (text, number, YYYY-MM-DD date, or single-select option name) or 'values' (multi-select option names). For multi-select fields, all listed values must be set on an issue for it to match (AND semantics) — to match any-of, make multiple list_issues calls and union the results.", + "items": { + "properties": { + "field_name": { + "description": "Name of the custom field (e.g. \"Priority\"). Case-insensitive.", + "type": "string" + }, + "value": { + "description": "Value to filter on for text, number, date, or single-select fields. For single-select, the option name (e.g. \"P1\"). For dates, YYYY-MM-DD. For numbers, the numeric value as a string. For text, the text value. Cannot be combined with 'values'.", + "type": "string" + }, + "values": { + "description": "Option names to filter on for multi-select fields. Matches issues that have ALL of these options set (AND semantics). To match any-of, make multiple list_issues calls. Cannot be combined with 'value'.", + "items": { + "type": "string" + }, + "type": "array" + } + }, + "required": [ + "field_name" + ], + "type": "object" + }, + "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__/set_issue_fields.snap b/pkg/github/__toolsnaps__/set_issue_fields.snap index 7a98fde2a..b23452323 100644 --- a/pkg/github/__toolsnaps__/set_issue_fields.snap +++ b/pkg/github/__toolsnaps__/set_issue_fields.snap @@ -8,7 +8,7 @@ "inputSchema": { "properties": { "fields": { - "description": "Array of issue field values to set. Each element must have a 'field_id' (string, the GraphQL node ID of the field) and exactly one value field: 'text_value' for text fields, 'number_value' for number fields, 'date_value' (ISO 8601 date string) for date fields, or 'single_select_option_id' (the GraphQL node ID of the option) for single select fields. Set 'delete' to true to remove a field value.", + "description": "Array of issue field values to set. Each element must have a 'field_id' (string, the GraphQL node ID of the field) and exactly one value field: 'text_value' for text fields, 'number_value' for number fields, 'date_value' (ISO 8601 date string) for date fields, 'single_select_option_id' (the GraphQL node ID of the option) for single select fields, or 'multi_select_option_ids' (an array of GraphQL node IDs) for multi select fields. Set 'delete' to true to remove a field value.", "items": { "properties": { "confidence": { @@ -36,6 +36,13 @@ "description": "If true, this field value is sent to the API as a suggestion (suggest:true) rather than an applied value. Whether the value is applied or recorded as a proposal is determined by the API.", "type": "boolean" }, + "multi_select_option_ids": { + "description": "The GraphQL node IDs of the options to set for a multi select field", + "items": { + "type": "string" + }, + "type": "array" + }, "number_value": { "description": "The value to set for a number field", "type": "number" diff --git a/pkg/github/feature_flags.go b/pkg/github/feature_flags.go index fa376eaa2..2f0ae235e 100644 --- a/pkg/github/feature_flags.go +++ b/pkg/github/feature_flags.go @@ -16,6 +16,10 @@ const FeatureFlagIFCLabels = "ifc_labels" // is not advertised by default, keeping the tool surface small unless opted in. const FeatureFlagFileBlame = "file_blame" +// FeatureFlagIssueFieldsMultiSelect is the feature flag name for the multi-select +// variant of the consolidated issue-fields write tools (issue_write, list_issues). +const FeatureFlagIssueFieldsMultiSelect = "remote_mcp_issue_fields_multiselect" + // 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. @@ -27,6 +31,7 @@ var AllowedFeatureFlags = []string{ FeatureFlagIssuesGranular, FeatureFlagPullRequestsGranular, FeatureFlagFileBlame, + FeatureFlagIssueFieldsMultiSelect, } // InsidersFeatureFlags is the list of feature flags that insiders mode enables. @@ -37,6 +42,7 @@ var InsidersFeatureFlags = []string{ MCPAppsFeatureFlag, FeatureFlagCSVOutput, FeatureFlagFileBlame, + FeatureFlagIssueFieldsMultiSelect, } // FeatureFlags defines runtime feature toggles that adjust tool behavior. diff --git a/pkg/github/granular_tools_test.go b/pkg/github/granular_tools_test.go index 4a274ac31..084ca10dc 100644 --- a/pkg/github/granular_tools_test.go +++ b/pkg/github/granular_tools_test.go @@ -1378,6 +1378,11 @@ func TestGranularSetIssueFields(t *testing.T) { SingleSelectValue struct { Name string } `graphql:"... on IssueFieldSingleSelectValue"` + MultiSelectValue struct { + Options []struct { + Name string + } + } `graphql:"... on IssueFieldMultiSelectValue"` DateValue struct { Value string } `graphql:"... on IssueFieldDateValue"` @@ -1427,6 +1432,134 @@ func TestGranularSetIssueFields(t *testing.T) { assert.False(t, result.IsError) }) + t.Run("successful set with multi select option ids", func(t *testing.T) { + matchers := []githubv4mock.Matcher{ + githubv4mock.NewQueryMatcher( + struct { + Repository struct { + Issue struct { + ID githubv4.ID + } `graphql:"issue(number: $issueNumber)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + }{}, + map[string]any{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + "issueNumber": githubv4.Int(5), + }, + githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "issue": map[string]any{"id": "ISSUE_123"}, + }, + }), + ), + githubv4mock.NewMutationMatcher( + struct { + SetIssueFieldValue struct { + Issue struct { + ID githubv4.ID + Number githubv4.Int + URL githubv4.String + } + IssueFieldValues []struct { + TextValue struct { + Value string + } `graphql:"... on IssueFieldTextValue"` + SingleSelectValue struct { + Name string + } `graphql:"... on IssueFieldSingleSelectValue"` + MultiSelectValue struct { + Options []struct { + Name string + } + } `graphql:"... on IssueFieldMultiSelectValue"` + DateValue struct { + Value string + } `graphql:"... on IssueFieldDateValue"` + NumberValue struct { + Value float64 + } `graphql:"... on IssueFieldNumberValue"` + } + } `graphql:"setIssueFieldValue(input: $input)"` + }{}, + SetIssueFieldValueInput{ + IssueID: githubv4.ID("ISSUE_123"), + IssueFields: []IssueFieldCreateOrUpdateInput{ + { + FieldID: githubv4.ID("FIELD_1"), + MultiSelectOptionIDs: &[]githubv4.ID{githubv4.ID("OPT_1"), githubv4.ID("OPT_2")}, + }, + }, + }, + nil, + githubv4mock.DataResponse(map[string]any{ + "setIssueFieldValue": map[string]any{ + "issue": map[string]any{ + "id": "ISSUE_123", + "number": 5, + "url": "https://github.com/owner/repo/issues/5", + }, + }, + }), + ), + } + + gqlClient := githubv4.NewClient(githubv4mock.NewMockedHTTPClient(matchers...)) + deps := BaseDeps{GQLClient: gqlClient} + serverTool := GranularSetIssueFields(translations.NullTranslationHelper) + handler := serverTool.Handler(deps) + + request := createMCPRequest(map[string]any{ + "owner": "owner", + "repo": "repo", + "issue_number": float64(5), + "fields": []any{ + map[string]any{"field_id": "FIELD_1", "multi_select_option_ids": []any{"OPT_1", "OPT_2"}}, + }, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + assert.False(t, result.IsError) + }) + + t.Run("multi select combined with single select returns error", func(t *testing.T) { + deps := BaseDeps{} + serverTool := GranularSetIssueFields(translations.NullTranslationHelper) + handler := serverTool.Handler(deps) + + request := createMCPRequest(map[string]any{ + "owner": "owner", + "repo": "repo", + "issue_number": float64(5), + "fields": []any{ + map[string]any{"field_id": "FIELD_1", "single_select_option_id": "OPT_1", "multi_select_option_ids": []any{"OPT_2"}}, + }, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + textContent := getTextResult(t, result) + assert.Contains(t, textContent.Text, "each field must have exactly one value") + }) + + t.Run("multi select option ids must be strings", func(t *testing.T) { + deps := BaseDeps{} + serverTool := GranularSetIssueFields(translations.NullTranslationHelper) + handler := serverTool.Handler(deps) + + request := createMCPRequest(map[string]any{ + "owner": "owner", + "repo": "repo", + "issue_number": float64(5), + "fields": []any{ + map[string]any{"field_id": "FIELD_1", "multi_select_option_ids": []any{"OPT_1", 42}}, + }, + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + textContent := getTextResult(t, result) + assert.Contains(t, textContent.Text, "multi_select_option_ids must be an array of strings") + }) + t.Run("missing required parameter fields", func(t *testing.T) { deps := BaseDeps{} serverTool := GranularSetIssueFields(translations.NullTranslationHelper) @@ -1553,6 +1686,11 @@ func TestGranularSetIssueFields(t *testing.T) { SingleSelectValue struct { Name string } `graphql:"... on IssueFieldSingleSelectValue"` + MultiSelectValue struct { + Options []struct { + Name string + } + } `graphql:"... on IssueFieldMultiSelectValue"` DateValue struct { Value string } `graphql:"... on IssueFieldDateValue"` @@ -1667,6 +1805,11 @@ func TestGranularSetIssueFields(t *testing.T) { SingleSelectValue struct { Name string } `graphql:"... on IssueFieldSingleSelectValue"` + MultiSelectValue struct { + Options []struct { + Name string + } + } `graphql:"... on IssueFieldMultiSelectValue"` DateValue struct { Value string } `graphql:"... on IssueFieldDateValue"` @@ -1781,6 +1924,11 @@ func TestGranularSetIssueFields(t *testing.T) { SingleSelectValue struct { Name string } `graphql:"... on IssueFieldSingleSelectValue"` + MultiSelectValue struct { + Options []struct { + Name string + } + } `graphql:"... on IssueFieldMultiSelectValue"` DateValue struct { Value string } `graphql:"... on IssueFieldDateValue"` @@ -1872,6 +2020,11 @@ func TestGranularSetIssueFields(t *testing.T) { SingleSelectValue struct { Name string } `graphql:"... on IssueFieldSingleSelectValue"` + MultiSelectValue struct { + Options []struct { + Name string + } + } `graphql:"... on IssueFieldMultiSelectValue"` DateValue struct { Value string } `graphql:"... on IssueFieldDateValue"` @@ -1964,6 +2117,11 @@ func TestGranularSetIssueFields(t *testing.T) { SingleSelectValue struct { Name string } `graphql:"... on IssueFieldSingleSelectValue"` + MultiSelectValue struct { + Options []struct { + Name string + } + } `graphql:"... on IssueFieldMultiSelectValue"` DateValue struct { Value string } `graphql:"... on IssueFieldDateValue"` diff --git a/pkg/github/issue_fields.go b/pkg/github/issue_fields.go index e06bdbfcf..fd0b69eba 100644 --- a/pkg/github/issue_fields.go +++ b/pkg/github/issue_fields.go @@ -83,6 +83,21 @@ type issueFieldNode struct { Priority *int } } `graphql:"... on IssueFieldSingleSelect"` + IssueFieldMultiSelect struct { + ID githubv4.ID + FullDatabaseID githubv4.String `graphql:"fullDatabaseId"` + Name githubv4.String + Description githubv4.String + DataType githubv4.String + Visibility githubv4.String + Options []struct { + ID githubv4.ID + Name githubv4.String + Description githubv4.String + Color githubv4.String + Priority *int + } + } `graphql:"... on IssueFieldMultiSelect"` } // issueFieldsRepoQuery is the GraphQL query for listing issue fields on a repository. @@ -104,12 +119,39 @@ type issueFieldsOrgQuery struct { } // ListIssueFields creates a tool to list issue field definitions for a repository or organization. +// ListIssueFields is the multi-select-aware variant of list_issue_fields — +// its description mentions multi_select. ListIssueFieldsLegacy is served when +// FeatureFlagIssueFieldsMultiSelect is off; its description does not mention +// multi_select. Both register under the same tool name with mutually +// exclusive feature-flag annotations. +// +// Note: the underlying GraphQL fragment unconditionally includes the +// multi_select arm, so the OUTPUT will still surface multi-select fields if +// the org has them (matching what the dotcom UI shows). Only the description +// is gated. func ListIssueFields(t translations.TranslationHelperFunc) inventory.ServerTool { - st := NewTool( + st := buildListIssueFields(t, true) + st.FeatureFlagEnable = FeatureFlagIssueFieldsMultiSelect + return st +} + +// ListIssueFieldsLegacy is the FF-off variant of list_issue_fields. +func ListIssueFieldsLegacy(t translations.TranslationHelperFunc) inventory.ServerTool { + st := buildListIssueFields(t, false) + st.FeatureFlagDisable = []string{FeatureFlagIssueFieldsMultiSelect} + return st +} + +func buildListIssueFields(t translations.TranslationHelperFunc, multiSelectEnabled bool) inventory.ServerTool { + description := "List issue fields for a repository or organization. Returns field definitions including name, type (text, number, date, single_select), and for single_select fields the list of valid option names. When repo is omitted, returns org-level fields directly." + if multiSelectEnabled { + description = "List issue fields for a repository or organization. Returns field definitions including name, type (text, number, date, single_select, multi_select), and for single_select and multi_select fields the list of valid option names. When repo is omitted, returns org-level fields directly." + } + return NewTool( ToolsetMetadataIssues, mcp.Tool{ Name: "list_issue_fields", - Description: t("TOOL_LIST_ISSUE_FIELDS_DESCRIPTION", "List issue fields for a repository or organization. Returns field definitions including name, type (text, number, date, single_select), and for single_select fields the list of valid option names. When repo is omitted, returns org-level fields directly."), + Description: t("TOOL_LIST_ISSUE_FIELDS_DESCRIPTION", description), Annotations: &mcp.ToolAnnotations{ Title: t("TOOL_LIST_ISSUE_FIELDS_USER_TITLE", "List issue fields"), ReadOnlyHint: true, @@ -167,7 +209,6 @@ func ListIssueFields(t translations.TranslationHelperFunc) inventory.ServerTool } return result, nil, nil }) - return st } // fetchIssueFields returns the issue field definitions for the given owner. @@ -224,6 +265,26 @@ func issueFieldsFromNodes(nodes []issueFieldNode) []IssueField { Visibility: string(node.IssueFieldSingleSelect.Visibility), Options: opts, } + case "IssueFieldMultiSelect": + opts := make([]IssueSingleSelectFieldOption, 0, len(node.IssueFieldMultiSelect.Options)) + for _, o := range node.IssueFieldMultiSelect.Options { + opts = append(opts, IssueSingleSelectFieldOption{ + ID: fmt.Sprintf("%v", o.ID), + Name: string(o.Name), + Description: string(o.Description), + Color: string(o.Color), + Priority: o.Priority, + }) + } + f = IssueField{ + ID: fmt.Sprintf("%v", node.IssueFieldMultiSelect.ID), + DatabaseID: parseFullDatabaseID(string(node.IssueFieldMultiSelect.FullDatabaseID)), + Name: string(node.IssueFieldMultiSelect.Name), + Description: string(node.IssueFieldMultiSelect.Description), + DataType: string(node.IssueFieldMultiSelect.DataType), + Visibility: string(node.IssueFieldMultiSelect.Visibility), + Options: opts, + } case "IssueFieldText": f = IssueField{ ID: fmt.Sprintf("%v", node.IssueFieldText.ID), diff --git a/pkg/github/issue_fields_test.go b/pkg/github/issue_fields_test.go index 2c2b26ee2..886afff3a 100644 --- a/pkg/github/issue_fields_test.go +++ b/pkg/github/issue_fields_test.go @@ -15,10 +15,20 @@ import ( ) func Test_ListIssueFields(t *testing.T) { - // Verify tool definition + // Verify tool definitions. The MS-aware variant owns the _ff_ snap; + // the legacy variant owns the canonical list_issue_fields.snap. serverTool := ListIssueFields(translations.NullTranslationHelper) tool := serverTool.Tool - require.NoError(t, toolsnaps.Test(tool.Name, tool)) + require.NoError(t, toolsnaps.Test(tool.Name+"_ff_"+FeatureFlagIssueFieldsMultiSelect, tool)) + assert.Equal(t, FeatureFlagIssueFieldsMultiSelect, serverTool.FeatureFlagEnable, "ListIssueFields is the multi-select-aware variant and must be gated on the FF") + assert.Contains(t, tool.Description, "multi_select", "the MS-aware description must mention multi_select") + + legacyServerTool := ListIssueFieldsLegacy(translations.NullTranslationHelper) + legacyTool := legacyServerTool.Tool + require.NoError(t, toolsnaps.Test(legacyTool.Name, legacyTool)) + assert.Empty(t, legacyServerTool.FeatureFlagEnable, "ListIssueFieldsLegacy must not require any flag to be enabled") + assert.ElementsMatch(t, []string{FeatureFlagIssueFieldsMultiSelect}, legacyServerTool.FeatureFlagDisable, "ListIssueFieldsLegacy must be hidden when the multi-select flag is on") + assert.NotContains(t, legacyTool.Description, "multi_select", "the legacy description must not advertise multi_select") assert.Equal(t, "list_issue_fields", tool.Name) assert.NotEmpty(t, tool.Description) diff --git a/pkg/github/issues.go b/pkg/github/issues.go index b51b5191d..ce323ffe3 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -40,10 +40,11 @@ type IssueClosedStateReason string // issueWriteFieldInput is a user-friendly issue field input for issue_write. // Field IDs and option IDs are resolved internally before calling the REST API. type issueWriteFieldInput struct { - FieldName string - Value any - FieldOptionName string - Delete bool + FieldName string + Value any + FieldOptionName string + FieldOptionNames []string + Delete bool } const ( @@ -144,6 +145,15 @@ type issueFieldWriteMetadataNode struct { Name githubv4.String } } `graphql:"... on IssueFieldSingleSelect"` + IssueFieldMultiSelect struct { + FullDatabaseID githubv4.String `graphql:"fullDatabaseId"` + Name githubv4.String + DataType githubv4.String + Options []struct { + FullDatabaseID githubv4.String `graphql:"fullDatabaseId"` + Name githubv4.String + } + } `graphql:"... on IssueFieldMultiSelect"` } type issueFieldWriteMetadataQuery struct { @@ -155,8 +165,8 @@ type issueFieldWriteMetadataQuery struct { } // IssueFieldRef resolves the name of an issue field across its concrete types. -// IssueFields is a union of IssueFieldDate, IssueFieldNumber, IssueFieldSingleSelect, IssueFieldText, -// so we have to ask for `name` on each member. +// IssueFields is a union of IssueFieldDate, IssueFieldNumber, IssueFieldSingleSelect, +// IssueFieldMultiSelect, IssueFieldText, so we have to ask for `name` on each member. type IssueFieldRef struct { Date struct { Name githubv4.String @@ -170,6 +180,10 @@ type IssueFieldRef struct { Name githubv4.String FullDatabaseID githubv4.String `graphql:"fullDatabaseId"` } `graphql:"... on IssueFieldSingleSelect"` + MultiSelect struct { + Name githubv4.String + FullDatabaseID githubv4.String `graphql:"fullDatabaseId"` + } `graphql:"... on IssueFieldMultiSelect"` Text struct { Name githubv4.String FullDatabaseID githubv4.String `graphql:"fullDatabaseId"` @@ -185,6 +199,8 @@ func (r IssueFieldRef) Name() string { return string(r.Number.Name) case r.SingleSelect.Name != "": return string(r.SingleSelect.Name) + case r.MultiSelect.Name != "": + return string(r.MultiSelect.Name) case r.Text.Name != "": return string(r.Text.Name) } @@ -201,6 +217,8 @@ func (r IssueFieldRef) FullDatabaseIDStr() string { return string(r.Number.FullDatabaseID) case r.SingleSelect.FullDatabaseID != "": return string(r.SingleSelect.FullDatabaseID) + case r.MultiSelect.FullDatabaseID != "": + return string(r.MultiSelect.FullDatabaseID) case r.Text.FullDatabaseID != "": return string(r.Text.FullDatabaseID) } @@ -208,8 +226,9 @@ func (r IssueFieldRef) FullDatabaseIDStr() string { } // IssueFieldValueFragment captures the value of a custom issue field. IssueFieldValue is a union -// of 4 concrete value types; each carries its own value scalar and a reference to its parent field. +// of concrete value types; each carries its own value scalar and a reference to its parent field. // The Number variant's `value` is aliased to `valueNumber` to avoid a Float vs String type clash on decode. +// The MultiSelect variant exposes selected options as a plain list (not a connection). type IssueFieldValueFragment struct { TypeName string `graphql:"__typename"` DateValue struct { @@ -224,13 +243,23 @@ type IssueFieldValueFragment struct { Field IssueFieldRef Value githubv4.String } `graphql:"... on IssueFieldSingleSelectValue"` + MultiSelectValue struct { + Field IssueFieldRef + Options []struct { + Name githubv4.String + } + } `graphql:"... on IssueFieldMultiSelectValue"` TextValue struct { Field IssueFieldRef Value githubv4.String } `graphql:"... on IssueFieldTextValue"` } -func optionalIssueWriteFields(args map[string]any) ([]issueWriteFieldInput, error) { +// optionalIssueWriteFields parses the `issue_fields` parameter. When +// multiSelectEnabled is false, the `field_option_names` slot is rejected (the +// legacy variant of the tool does not advertise it in its schema; this guards +// against callers passing it anyway through some out-of-band path). +func optionalIssueWriteFields(args map[string]any, multiSelectEnabled bool) ([]issueWriteFieldInput, error) { issueFieldsRaw, exists := args["issue_fields"] if !exists { return nil, nil @@ -264,6 +293,24 @@ func optionalIssueWriteFields(args map[string]any) ([]issueWriteFieldInput, erro return nil, err } + var fieldOptionNames []string + _, hasNamesKey := itemMap["field_option_names"] + if hasNamesKey && !multiSelectEnabled { + // Legacy variant doesn't advertise field_option_names at all. + // Reject any presence of the key — even with an empty value or + // null — so the error message stays neutral instead of leaking + // multi-select wording (e.g. "use 'delete: true' to clear the + // field") into the legacy surface. + return nil, fmt.Errorf("field_option_names is not supported in this build") + } + if rawNames := itemMap["field_option_names"]; hasNamesKey && rawNames != nil { + parsed, err := parseStringSlice(rawNames) + if err != nil { + return nil, fmt.Errorf("field_option_names for field %q must be an array of strings", fieldName) + } + fieldOptionNames = parsed + } + deleteField, _ := OptionalParam[bool](itemMap, "delete") value, hasValue := itemMap["value"] if hasValue && value == nil { @@ -271,8 +318,8 @@ func optionalIssueWriteFields(args map[string]any) ([]issueWriteFieldInput, erro } if deleteField { - if hasValue || fieldOptionName != "" { - return nil, fmt.Errorf("issue field %q cannot specify 'delete' together with 'value' or 'field_option_name'", fieldName) + if hasValue || fieldOptionName != "" || len(fieldOptionNames) > 0 { + return nil, fmt.Errorf("issue field %q cannot specify 'delete' together with 'value', 'field_option_name', or 'field_option_names'", fieldName) } issueFields = append(issueFields, issueWriteFieldInput{ FieldName: fieldName, @@ -281,25 +328,46 @@ func optionalIssueWriteFields(args map[string]any) ([]issueWriteFieldInput, erro continue } - if hasValue && fieldOptionName != "" { - return nil, fmt.Errorf("issue field %q cannot specify both value and field_option_name", fieldName) + setters := 0 + if hasValue { + setters++ } - - if !hasValue && fieldOptionName == "" { - return nil, fmt.Errorf("issue field %q must specify either value or field_option_name", fieldName) + if fieldOptionName != "" { + setters++ + } + if len(fieldOptionNames) > 0 { + setters++ + } + if setters == 0 { + if hasNamesKey { + return nil, fmt.Errorf("issue field %q has empty field_option_names — use 'delete: true' to clear the field", fieldName) + } + return nil, fmt.Errorf("issue field %q must specify one of value, field_option_name, or field_option_names", fieldName) + } + if setters > 1 { + return nil, fmt.Errorf("issue field %q cannot specify more than one of value, field_option_name, or field_option_names", fieldName) } issueFields = append(issueFields, issueWriteFieldInput{ - FieldName: fieldName, - Value: value, - FieldOptionName: fieldOptionName, + FieldName: fieldName, + Value: value, + FieldOptionName: fieldOptionName, + FieldOptionNames: fieldOptionNames, }) } return issueFields, nil } -func resolveIssueRequestFieldValues(ctx context.Context, gqlClient *githubv4.Client, owner, repo string, issueFields []issueWriteFieldInput) ([]*github.IssueRequestFieldValue, []int64, error) { +// resolveIssueRequestFieldValues looks up field metadata via GraphQL and turns +// the user-supplied issueWriteFieldInputs into the REST-shaped values + the +// list of field IDs the caller should clear via the dedicated REST DELETE +// endpoint after the PATCH. +// +// When multiSelectEnabled is false, any field whose dataType is multi_select is +// rejected at this entry point. The legacy tool variant's schema doesn't allow +// multi-select inputs in the first place, so this is defence-in-depth. +func resolveIssueRequestFieldValues(ctx context.Context, gqlClient *githubv4.Client, owner, repo string, issueFields []issueWriteFieldInput, multiSelectEnabled bool) ([]*github.IssueRequestFieldValue, []int64, error) { if len(issueFields) == 0 { return nil, nil, nil } @@ -327,6 +395,8 @@ func resolveIssueRequestFieldValues(ctx context.Context, gqlClient *githubv4.Cli name = string(node.IssueFieldDate.Name) case "IssueFieldSingleSelect": name = string(node.IssueFieldSingleSelect.Name) + case "IssueFieldMultiSelect": + name = string(node.IssueFieldMultiSelect.Name) default: continue } @@ -355,6 +425,9 @@ func resolveIssueRequestFieldValues(ctx context.Context, gqlClient *githubv4.Cli case "IssueFieldSingleSelect": fullDatabaseIDStr = string(node.IssueFieldSingleSelect.FullDatabaseID) dataType = string(node.IssueFieldSingleSelect.DataType) + case "IssueFieldMultiSelect": + fullDatabaseIDStr = string(node.IssueFieldMultiSelect.FullDatabaseID) + dataType = string(node.IssueFieldMultiSelect.DataType) } fieldID := parseFullDatabaseID(fullDatabaseIDStr) @@ -362,15 +435,20 @@ func resolveIssueRequestFieldValues(ctx context.Context, gqlClient *githubv4.Cli return nil, nil, fmt.Errorf("issue field %q is missing fullDatabaseId", fieldInput.FieldName) } + if !multiSelectEnabled && strings.EqualFold(dataType, "multi_select") { + return nil, nil, fmt.Errorf("issue field %q is multi_select, which is not supported in this build", fieldInput.FieldName) + } + if fieldInput.Delete { fieldIDsToDelete = append(fieldIDsToDelete, fieldID) continue } resolvedValue := fieldInput.Value - if fieldInput.FieldOptionName != "" { + switch { + case fieldInput.FieldOptionName != "": if !strings.EqualFold(dataType, "single_select") { - return nil, nil, fmt.Errorf("issue field %q is %q, so field_option_name cannot be used", fieldInput.FieldName, dataType) + return nil, nil, fmt.Errorf("issue field %q is %q, so field_option_name cannot be used (use %s)", fieldInput.FieldName, dataType, fieldOptionInputHint(dataType)) } optionFound := false @@ -386,6 +464,33 @@ func resolveIssueRequestFieldValues(ctx context.Context, gqlClient *githubv4.Cli if !optionFound { return nil, nil, fmt.Errorf("issue field option %q was not found for field %q", fieldInput.FieldOptionName, fieldInput.FieldName) } + case len(fieldInput.FieldOptionNames) > 0: + if !strings.EqualFold(dataType, "multi_select") { + return nil, nil, fmt.Errorf("issue field %q is %q, so field_option_names cannot be used (use %s)", fieldInput.FieldName, dataType, fieldOptionInputHint(dataType)) + } + + resolvedNames := make([]string, 0, len(fieldInput.FieldOptionNames)) + for _, requested := range fieldInput.FieldOptionNames { + matched := "" + for _, option := range node.IssueFieldMultiSelect.Options { + if strings.EqualFold(strings.TrimSpace(string(option.Name)), strings.TrimSpace(requested)) { + matched = string(option.Name) + break + } + } + if matched == "" { + return nil, nil, fmt.Errorf("issue field option %q was not found for field %q", requested, fieldInput.FieldName) + } + resolvedNames = append(resolvedNames, matched) + } + // REST IssueField#build_value_attributes for multi_select expects an array of option names. + resolvedValue = resolvedNames + default: + // multi_select must go through field_option_names (which validates options); + // reject a raw value here so callers get a clear error instead of a REST 422. + if strings.EqualFold(dataType, "multi_select") { + return nil, nil, fmt.Errorf("issue field %q is multi_select, use field_option_names to set its value", fieldInput.FieldName) + } } resolved = append(resolved, &github.IssueRequestFieldValue{ @@ -397,6 +502,19 @@ func resolveIssueRequestFieldValues(ctx context.Context, gqlClient *githubv4.Cli return resolved, fieldIDsToDelete, nil } +// fieldOptionInputHint returns the user-facing input slot name that matches a select field's data type. +// Used in error messages so callers know which slot to switch to. +func fieldOptionInputHint(dataType string) string { + switch { + case strings.EqualFold(dataType, "single_select"): + return "field_option_name" + case strings.EqualFold(dataType, "multi_select"): + return "field_option_names" + default: + return "value" + } +} + // fetchExistingIssueFieldValues retrieves the current field values for an issue // as IssueRequestFieldValue entries, ready to be merged before an update. func fetchExistingIssueFieldValues(ctx context.Context, gqlClient *githubv4.Client, owner, repo string, issueNumber int) ([]*github.IssueRequestFieldValue, error) { @@ -437,6 +555,13 @@ func fetchExistingIssueFieldValues(ctx context.Context, gqlClient *githubv4.Clie case "IssueFieldSingleSelectValue": fieldIDStr = node.SingleSelectValue.Field.FullDatabaseIDStr() value = string(node.SingleSelectValue.Value) + case "IssueFieldMultiSelectValue": + fieldIDStr = node.MultiSelectValue.Field.FullDatabaseIDStr() + names := make([]string, 0, len(node.MultiSelectValue.Options)) + for _, opt := range node.MultiSelectValue.Options { + names = append(names, string(opt.Name)) + } + value = names case "IssueFieldTextValue": fieldIDStr = node.TextValue.Field.FullDatabaseIDStr() value = string(node.TextValue.Value) @@ -556,13 +681,16 @@ type ListIssuesQueryTypeWithLabelsWithSince struct { } // IssueFieldValueFilter mirrors the GraphQL IssueFieldValueFilter input. Exactly one typed value -// field should be set per filter (the monolith resolver rejects multiple). +// field should be set per filter (the monolith resolver rejects multiple). For multi_select fields, +// MultiSelectOptionValues uses all-of (AND) semantics: an issue matches only if it has every listed +// option set on the field. To match any-of, send separate filter calls. type IssueFieldValueFilter struct { - FieldName githubv4.String `json:"fieldName"` - TextValue *githubv4.String `json:"textValue,omitempty"` - DateValue *githubv4.String `json:"dateValue,omitempty"` - NumberValue *githubv4.Float `json:"numberValue,omitempty"` - SingleSelectOptionValue *githubv4.String `json:"singleSelectOptionValue,omitempty"` + FieldName githubv4.String `json:"fieldName"` + TextValue *githubv4.String `json:"textValue,omitempty"` + DateValue *githubv4.String `json:"dateValue,omitempty"` + NumberValue *githubv4.Float `json:"numberValue,omitempty"` + SingleSelectOptionValue *githubv4.String `json:"singleSelectOptionValue,omitempty"` + MultiSelectOptionValues *[]githubv4.String `json:"multiSelectOptionValues,omitempty"` } // Implement the interface for all query types @@ -1628,7 +1756,8 @@ func fetchIssueFieldValuesByNodeID(ctx context.Context, gqlClient *githubv4.Clie } var q searchIssuesNodesQuery - if err := gqlClient.Query(ctx, &q, map[string]any{"ids": ids}); err != nil { + ctxWithFeatures := ghcontext.WithGraphQLFeatures(ctx, "issue_fields", "repo_issue_fields") + if err := gqlClient.Query(ctxWithFeatures, &q, map[string]any{"ids": ids}); err != nil { return nil, err } @@ -1788,12 +1917,14 @@ func issueWriteAwaitingFormResult(method, owner, repo string, issueNumber int) * return utils.NewToolResultAwaitingFormSubmission(msg) } -// IssueWrite is the FeatureFlagIssueFields-enabled variant of issue_write -// (with the issue_fields parameter). LegacyIssueWrite is served when the flag -// is off. Both register under the tool name "issue_write"; exactly one is -// active at a time via mutually exclusive feature-flag annotations. When the -// flag is removed, delete LegacyIssueWrite outright and drop the feature-flag -// fields on IssueWrite. +// IssueWrite is the multi-select-aware variant of issue_write — its schema +// advertises the `field_option_names` slot on `issue_fields[]` items and its +// resolver accepts multi-select fields. IssueWriteLegacy is served when +// FeatureFlagIssueFieldsMultiSelect is off; it has no `field_option_names` +// slot and rejects multi-select fields at the resolver. Both register under +// the tool name "issue_write" with mutually exclusive feature-flag annotations +// so exactly one is active at a time. When the flag is removed, delete +// IssueWriteLegacy outright and drop the feature-flag fields on IssueWrite. func IssueWrite(t translations.TranslationHelperFunc) inventory.ServerTool { st := NewTool( ToolsetMetadataIssues, @@ -1880,7 +2011,7 @@ Options are: }, "issue_fields": { Type: "array", - Description: "Issue field values to set or clear. Each item requires 'field_name' and exactly one of 'value', 'field_option_name', or 'delete: true'.", + Description: "Issue field values to set or clear. Each item requires 'field_name' and exactly one of 'value', 'field_option_name', 'field_option_names', or 'delete: true'.", Items: &jsonschema.Schema{ Type: "object", AdditionalProperties: &jsonschema.Schema{Not: &jsonschema.Schema{}}, @@ -1895,19 +2026,31 @@ Options are: Description: "Value to set. Use for text, number, and date fields " + "(date as YYYY-MM-DD). For single-select fields, prefer " + "'field_option_name' so the option is validated before the API " + - "call. Cannot be combined with 'field_option_name' or 'delete'.", + "call. Cannot be combined with 'field_option_name', " + + "'field_option_names', or 'delete'.", }, "field_option_name": { Type: "string", Description: "Option name for single-select fields. Validated against " + "the field's options before the API call. Cannot be combined with " + - "'value' or 'delete'.", + "'value', 'field_option_names', or 'delete'.", + }, + "field_option_names": { + Type: "array", + Items: &jsonschema.Schema{ + Type: "string", + }, + Description: "Option names for multi-select fields. All names are validated " + + "against the field's options before the API call. An empty array " + + "is rejected — use 'delete: true' to clear the field. " + + "Cannot be combined with 'value', 'field_option_name', or 'delete'.", }, "delete": { Type: "boolean", Enum: []any{true}, Description: "Set to true to clear this field's current value on the " + - "issue. Cannot be combined with 'value' or 'field_option_name'.", + "issue. Cannot be combined with 'value', 'field_option_name', " + + "or 'field_option_names'.", }, }, Required: []string{"field_name"}, @@ -2033,7 +2176,7 @@ Options are: } var issueFields []issueWriteFieldInput - issueFields, err = optionalIssueWriteFields(args) + issueFields, err = optionalIssueWriteFields(args, true) if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } @@ -2051,7 +2194,7 @@ Options are: var issueFieldValues []*github.IssueRequestFieldValue var fieldIDsToDelete []int64 if len(issueFields) > 0 { - issueFieldValues, fieldIDsToDelete, err = resolveIssueRequestFieldValues(ctx, gqlClient, owner, repo, issueFields) + issueFieldValues, fieldIDsToDelete, err = resolveIssueRequestFieldValues(ctx, gqlClient, owner, repo, issueFields, true) if err != nil { return utils.NewToolResultError(fmt.Sprintf("failed to resolve issue_fields: %v", err)), nil, nil } @@ -2076,6 +2219,7 @@ Options are: } }) st.FeatureFlagDisable = []string{FeatureFlagIssuesGranular} + st.FeatureFlagEnable = FeatureFlagIssueFieldsMultiSelect return st } @@ -2355,9 +2499,61 @@ func UpdateIssue(ctx context.Context, client *github.Client, gqlClient *githubv4 return utils.NewToolResultText(string(r)), nil } -// ListIssues creates a tool to list and filter repository issues. It exposes the -// Issues 2.0 field_filters input plus field_values output enrichment. +// ListIssues is the multi-select-aware variant — its `field_filters` schema +// includes the `values` slot for multi-select option filters and the +// description text advertises multi-select filtering. ListIssuesLegacy is +// served when FeatureFlagIssueFieldsMultiSelect is off; it has no `values` +// slot and its description does not mention multi-select. The underlying +// handler is shared and accepts either schema shape. func ListIssues(t translations.TranslationHelperFunc) inventory.ServerTool { + st := buildListIssues(t, true) + st.FeatureFlagEnable = FeatureFlagIssueFieldsMultiSelect + return st +} + +// ListIssuesLegacy is the FF-off variant of list_issues. +func ListIssuesLegacy(t translations.TranslationHelperFunc) inventory.ServerTool { + st := buildListIssues(t, false) + st.FeatureFlagDisable = []string{FeatureFlagIssueFieldsMultiSelect} + return st +} + +func buildListIssues(t translations.TranslationHelperFunc, multiSelectEnabled bool) inventory.ServerTool { + fieldFiltersDescription := "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)." + valueDescription := "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." + if multiSelectEnabled { + fieldFiltersDescription = "Filter by custom issue field values. Each entry takes a field_name and either 'value' (text, number, YYYY-MM-DD date, or single-select option name) or 'values' (multi-select option names). For multi-select fields, all listed values must be set on an issue for it to match (AND semantics) — to match any-of, make multiple list_issues calls and union the results." + valueDescription = "Value to filter on for text, number, date, or single-select fields. For single-select, the option name (e.g. \"P1\"). For dates, YYYY-MM-DD. For numbers, the numeric value as a string. For text, the text value. Cannot be combined with 'values'." + } + + fieldFilterItemProps := map[string]*jsonschema.Schema{ + "field_name": { + Type: "string", + Description: "Name of the custom field (e.g. \"Priority\"). Case-insensitive.", + }, + "value": { + Type: "string", + Description: valueDescription, + }, + } + if multiSelectEnabled { + fieldFilterItemProps["values"] = &jsonschema.Schema{ + Type: "array", + Description: "Option names to filter on for multi-select fields. Matches issues that have ALL of these options set (AND semantics). To match any-of, make multiple list_issues calls. Cannot be combined with 'value'.", + Items: &jsonschema.Schema{ + Type: "string", + }, + } + } + + // Legacy variant requires `value` outright (its schema has no `values` slot). + // MS-aware variant only requires `field_name` because either `value` or + // `values` is acceptable; the parser enforces exactly-one-of at runtime. + fieldFilterRequired := []string{"field_name", "value"} + if multiSelectEnabled { + fieldFilterRequired = []string{"field_name"} + } + schema := &jsonschema.Schema{ Type: "object", Properties: map[string]*jsonschema.Schema{ @@ -2397,20 +2593,15 @@ func ListIssues(t translations.TranslationHelperFunc) inventory.ServerTool { }, "field_filters": { Type: "array", - 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).", + Description: fieldFiltersDescription, Items: &jsonschema.Schema{ - Type: "object", - Properties: map[string]*jsonschema.Schema{ - "field_name": { - Type: "string", - Description: "Name of the custom field (e.g. \"Priority\"). Case-insensitive.", - }, - "value": { - Type: "string", - 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.", - }, - }, - Required: []string{"field_name", "value"}, + Type: "object", + Properties: fieldFilterItemProps, + // Legacy keeps the historical strict shape (field_name + value). + // MS-aware loosens to just field_name because either `value` or + // `values` is acceptable — the parser enforces "exactly one of" + // at runtime. + Required: fieldFilterRequired, }, }, }, @@ -2508,7 +2699,7 @@ func ListIssues(t translations.TranslationHelperFunc) inventory.ServerTool { } hasLabels := len(labels) > 0 - rawFilters, err := parseRawFieldFilters(args) + rawFilters, err := parseRawFieldFilters(args, multiSelectEnabled) if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } @@ -2616,16 +2807,25 @@ func ListIssues(t translations.TranslationHelperFunc) inventory.ServerTool { return st } -// rawFieldFilter is the user-supplied {field_name, value} pair before type resolution. +// rawFieldFilter is the user-supplied {field_name, value|values} entry before type resolution. +// Exactly one of Value or Values is set per entry. type rawFieldFilter struct { - Name string - Value string + Name string + Value string + Values []string + HasValue bool + HasValues bool } // parseRawFieldFilters extracts the optional field_filters parameter into a list of -// {name, value} pairs. The value is always a string here; type-aware coercion happens -// later in resolveFieldFilters once we know each field's data_type. -func parseRawFieldFilters(args map[string]any) ([]rawFieldFilter, error) { +// {name, value|values} entries. The scalar value is always a string here; type-aware +// coercion happens later in resolveFieldFilters once we know each field's data_type. +// Each entry must supply exactly one of `value` (scalar — text/number/date/single_select) +// or `values` (array — multi_select). +// parseRawFieldFilters parses the field_filters argument. When +// multiSelectEnabled is false, any `values` slot is rejected at the entry +// point (the legacy ListIssues schema does not advertise it). +func parseRawFieldFilters(args map[string]any, multiSelectEnabled bool) ([]rawFieldFilter, error) { raw, ok := args["field_filters"] if !ok { return nil, nil @@ -2653,19 +2853,69 @@ func parseRawFieldFilters(args map[string]any) ([]rawFieldFilter, error) { if err != nil { return nil, fmt.Errorf("field_filters entry: %s", err.Error()) } - value, err := RequiredParam[string](entry, "value") - if err != nil { - return nil, fmt.Errorf("field_filters entry %q: %s", fieldName, err.Error()) + + filter := rawFieldFilter{Name: fieldName} + + if rawValue, present := entry["value"]; present { + value, ok := rawValue.(string) + if !ok { + return nil, fmt.Errorf("field_filters entry %q: value must be a string", fieldName) + } + filter.Value = value + filter.HasValue = true + } + + if rawValues, present := entry["values"]; present { + if !multiSelectEnabled { + return nil, fmt.Errorf("field_filters entry %q: 'values' is not supported in this build", fieldName) + } + values, err := parseStringSlice(rawValues) + if err != nil { + return nil, fmt.Errorf("field_filters entry %q: values must be an array of strings: %s", fieldName, err.Error()) + } + filter.Values = values + filter.HasValues = true } - filters = append(filters, rawFieldFilter{Name: fieldName, Value: value}) + + switch { + case filter.HasValue && filter.HasValues: + return nil, fmt.Errorf("field_filters entry %q: provide either 'value' or 'values', not both", fieldName) + case !filter.HasValue && !filter.HasValues: + if multiSelectEnabled { + return nil, fmt.Errorf("field_filters entry %q: missing 'value' (for text/number/date/single_select) or 'values' (for multi_select)", fieldName) + } + return nil, fmt.Errorf("field_filters entry %q: missing 'value'", fieldName) + } + + filters = append(filters, filter) } return filters, nil } +// parseStringSlice coerces a JSON-decoded value into a []string, accepting either []string or []any. +func parseStringSlice(raw any) ([]string, error) { + switch v := raw.(type) { + case []string: + return v, nil + case []any: + out := make([]string, 0, len(v)) + for i, item := range v { + s, ok := item.(string) + if !ok { + return nil, fmt.Errorf("index %d is not a string", i) + } + out = append(out, s) + } + return out, nil + default: + return nil, fmt.Errorf("expected array, got %T", raw) + } +} + // resolveFieldFilters matches each raw filter against a known field definition and // coerces the value into the right typed slot on IssueFieldValueFilter. Matching is // case-insensitive on field name; option names are also matched case-insensitively for -// single-select fields. +// single-select and multi-select fields. Multi-select filters use all-of (AND) semantics. func resolveFieldFilters(rawFilters []rawFieldFilter, fields []IssueField) ([]IssueFieldValueFilter, error) { byName := make(map[string]IssueField, len(fields)) knownNames := make([]string, 0, len(fields)) @@ -2684,34 +2934,50 @@ func resolveFieldFilters(rawFilters []rawFieldFilter, fields []IssueField) ([]Is filter := IssueFieldValueFilter{FieldName: githubv4.String(field.Name)} switch field.DataType { case "SINGLE_SELECT": - // Validate the option name against the field's options so we fail fast - // with a useful error instead of an opaque GraphQL one. - var matched string - for _, o := range field.Options { - if strings.EqualFold(o.Name, rf.Value) { - matched = o.Name - break - } + if !rf.HasValue { + return nil, fmt.Errorf("field_filters: field %q is single_select, use 'value' (single option name)", field.Name) } - if matched == "" { - optionNames := make([]string, 0, len(field.Options)) - for _, o := range field.Options { - optionNames = append(optionNames, o.Name) - } - return nil, fmt.Errorf("field_filters: %q is not a valid option for %q. Valid options: %s", rf.Value, field.Name, strings.Join(optionNames, ", ")) + matched, err := matchOption(field, rf.Value) + if err != nil { + return nil, err } v := githubv4.String(matched) filter.SingleSelectOptionValue = &v + case "MULTI_SELECT": + if !rf.HasValues { + return nil, fmt.Errorf("field_filters: field %q is multi_select, use 'values' (array of option names)", field.Name) + } + if len(rf.Values) == 0 { + return nil, fmt.Errorf("field_filters: field %q is multi_select and requires at least one value", field.Name) + } + matched := make([]githubv4.String, 0, len(rf.Values)) + for _, raw := range rf.Values { + m, err := matchOption(field, raw) + if err != nil { + return nil, err + } + matched = append(matched, githubv4.String(m)) + } + filter.MultiSelectOptionValues = &matched case "TEXT": + if !rf.HasValue { + return nil, fmt.Errorf("field_filters: field %q is text, use 'value'", field.Name) + } v := githubv4.String(rf.Value) filter.TextValue = &v case "DATE": + if !rf.HasValue { + return nil, fmt.Errorf("field_filters: field %q is date, use 'value' (YYYY-MM-DD)", field.Name) + } if _, err := time.Parse("2006-01-02", rf.Value); err != nil { return nil, fmt.Errorf("field_filters: %q is not a valid date for %q (expected YYYY-MM-DD): %s", rf.Value, field.Name, err.Error()) } v := githubv4.String(rf.Value) filter.DateValue = &v case "NUMBER": + if !rf.HasValue { + return nil, fmt.Errorf("field_filters: field %q is number, use 'value'", field.Name) + } n, err := strconv.ParseFloat(rf.Value, 64) if err != nil { return nil, fmt.Errorf("field_filters: %q is not a valid number for %q: %s", rf.Value, field.Name, err.Error()) @@ -2726,6 +2992,21 @@ func resolveFieldFilters(rawFilters []rawFieldFilter, fields []IssueField) ([]Is return out, nil } +// matchOption returns the canonical option name from field.Options matching value case-insensitively, +// or an error listing the valid options. +func matchOption(field IssueField, value string) (string, error) { + for _, o := range field.Options { + if strings.EqualFold(strings.TrimSpace(o.Name), strings.TrimSpace(value)) { + return o.Name, nil + } + } + optionNames := make([]string, 0, len(field.Options)) + for _, o := range field.Options { + optionNames = append(optionNames, o.Name) + } + return "", fmt.Errorf("field_filters: %q is not a valid option for %q. Valid options: %s", value, field.Name, strings.Join(optionNames, ", ")) +} + // parseISOTimestamp parses an ISO 8601 timestamp string into a time.Time object. // Returns the parsed time or an error if parsing fails. // Example formats supported: "2023-01-15T14:30:00Z", "2023-01-15" diff --git a/pkg/github/issues_granular.go b/pkg/github/issues_granular.go index 157d5595f..bc1c8aaf2 100644 --- a/pkg/github/issues_granular.go +++ b/pkg/github/issues_granular.go @@ -919,6 +919,7 @@ type IssueFieldCreateOrUpdateInput struct { NumberValue *githubv4.Float `json:"numberValue,omitempty"` DateValue *githubv4.String `json:"dateValue,omitempty"` SingleSelectOptionID *githubv4.ID `json:"singleSelectOptionId,omitempty"` + MultiSelectOptionIDs *[]githubv4.ID `json:"multiSelectOptionIds,omitempty"` Delete *githubv4.Boolean `json:"delete,omitempty"` Rationale *githubv4.String `json:"rationale,omitempty"` Confidence *string `json:"confidence,omitempty"` @@ -956,7 +957,7 @@ func GranularSetIssueFields(t translations.TranslationHelperFunc) inventory.Serv }, "fields": { Type: "array", - Description: "Array of issue field values to set. Each element must have a 'field_id' (string, the GraphQL node ID of the field) and exactly one value field: 'text_value' for text fields, 'number_value' for number fields, 'date_value' (ISO 8601 date string) for date fields, or 'single_select_option_id' (the GraphQL node ID of the option) for single select fields. Set 'delete' to true to remove a field value.", + Description: "Array of issue field values to set. Each element must have a 'field_id' (string, the GraphQL node ID of the field) and exactly one value field: 'text_value' for text fields, 'number_value' for number fields, 'date_value' (ISO 8601 date string) for date fields, 'single_select_option_id' (the GraphQL node ID of the option) for single select fields, or 'multi_select_option_ids' (an array of GraphQL node IDs) for multi select fields. Set 'delete' to true to remove a field value.", MinItems: jsonschema.Ptr(1), Items: &jsonschema.Schema{ Type: "object", @@ -981,6 +982,13 @@ func GranularSetIssueFields(t translations.TranslationHelperFunc) inventory.Serv Type: "string", Description: "The GraphQL node ID of the option to set for a single select field", }, + "multi_select_option_ids": { + Type: "array", + Description: "The GraphQL node IDs of the options to set for a multi select field", + Items: &jsonschema.Schema{ + Type: "string", + }, + }, "delete": { Type: "boolean", Description: "Set to true to delete this field value", @@ -1083,6 +1091,20 @@ func GranularSetIssueFields(t translations.TranslationHelperFunc) inventory.Serv input.SingleSelectOptionID = &optionID valueCount++ } + if rawIDs, exists := fieldMap["multi_select_option_ids"]; exists && rawIDs != nil { + ids, err := parseStringSlice(rawIDs) + if err != nil { + return utils.NewToolResultError("multi_select_option_ids must be an array of strings"), nil, nil + } + if len(ids) > 0 { + optionIDs := make([]githubv4.ID, 0, len(ids)) + for _, s := range ids { + optionIDs = append(optionIDs, githubv4.ID(s)) + } + input.MultiSelectOptionIDs = &optionIDs + valueCount++ + } + } if _, exists := fieldMap["delete"]; exists { del, err := OptionalParam[bool](fieldMap, "delete") if err == nil && del { @@ -1093,10 +1115,10 @@ func GranularSetIssueFields(t translations.TranslationHelperFunc) inventory.Serv } if valueCount == 0 { - return utils.NewToolResultError("each field must have a value (text_value, number_value, date_value, single_select_option_id) or delete: true"), nil, nil + return utils.NewToolResultError("each field must have a value (text_value, number_value, date_value, single_select_option_id, multi_select_option_ids) or delete: true"), nil, nil } if valueCount > 1 { - return utils.NewToolResultError("each field must have exactly one value (text_value, number_value, date_value, single_select_option_id) or delete: true, but multiple were provided"), nil, nil + return utils.NewToolResultError("each field must have exactly one value (text_value, number_value, date_value, single_select_option_id, multi_select_option_ids) or delete: true, but multiple were provided"), nil, nil } if _, exists := fieldMap["rationale"]; exists { @@ -1163,6 +1185,11 @@ func GranularSetIssueFields(t translations.TranslationHelperFunc) inventory.Serv SingleSelectValue struct { Name string } `graphql:"... on IssueFieldSingleSelectValue"` + MultiSelectValue struct { + Options []struct { + Name string + } + } `graphql:"... on IssueFieldMultiSelectValue"` DateValue struct { Value string } `graphql:"... on IssueFieldDateValue"` diff --git a/pkg/github/issues_legacy_multiselect.go b/pkg/github/issues_legacy_multiselect.go new file mode 100644 index 000000000..dacae3c5d --- /dev/null +++ b/pkg/github/issues_legacy_multiselect.go @@ -0,0 +1,295 @@ +package github + +// This file holds the legacy (non-multi-select) variants of the issue-fields +// write tools. They are served when FeatureFlagIssueFieldsMultiSelect is off +// and are intended to be deleted in their entirety once the flag graduates. +// +// Each legacy variant mirrors the structure of its multi-select-aware sibling +// (IssueWrite, GranularSetIssueFields, ListIssues, ListIssueFields) but with +// the multi-select schema slots and description text removed. The shared parser +// and resolver branch on a single bool flag — see optionalIssueWriteFields and +// resolveIssueRequestFieldValues. There is no "interleaved" feature-flag check +// in the handler bodies themselves: each tool variant is wired to one constant +// branch of those helpers at registration time. + +import ( + "context" + "fmt" + + "github.com/github/github-mcp-server/pkg/inventory" + "github.com/github/github-mcp-server/pkg/scopes" + "github.com/github/github-mcp-server/pkg/translations" + "github.com/github/github-mcp-server/pkg/utils" + "github.com/google/go-github/v87/github" + "github.com/google/jsonschema-go/jsonschema" + "github.com/modelcontextprotocol/go-sdk/mcp" +) + +// IssueWriteLegacy is the FF-off variant of issue_write. It does not accept +// multi-select inputs and its schema does not mention multi-select. It is +// registered under the same tool name as IssueWrite with mutually exclusive +// feature-flag annotations. +func IssueWriteLegacy(t translations.TranslationHelperFunc) inventory.ServerTool { + st := NewTool( + ToolsetMetadataIssues, + mcp.Tool{ + Name: "issue_write", + Description: t("TOOL_ISSUE_WRITE_DESCRIPTION", "Create a new or update an existing issue in a GitHub repository."), + Annotations: &mcp.ToolAnnotations{ + Title: t("TOOL_ISSUE_WRITE_USER_TITLE", "Create or update issue/pull request"), + ReadOnlyHint: false, + }, + Meta: mcp.Meta{ + "ui": map[string]any{ + "resourceUri": IssueWriteUIResourceURI, + "visibility": []string{"model", "app"}, + }, + }, + InputSchema: &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "method": { + Type: "string", + Description: `Write operation to perform on a single issue. +Options are: +- 'create' - creates a new issue. +- 'update' - updates an existing issue. +`, + Enum: []any{"create", "update"}, + }, + "owner": { + Type: "string", + Description: "Repository owner", + }, + "repo": { + Type: "string", + Description: "Repository name", + }, + "issue_number": { + Type: "number", + Description: "Issue number to update", + }, + "title": { + Type: "string", + Description: "Issue title", + }, + "body": { + Type: "string", + Description: "Issue body content", + }, + "assignees": { + Type: "array", + Description: "Usernames to assign to this issue", + Items: &jsonschema.Schema{ + Type: "string", + }, + }, + "labels": { + Type: "array", + Description: "Labels to apply to this issue", + Items: &jsonschema.Schema{ + Type: "string", + }, + }, + "milestone": { + Type: "number", + Description: "Milestone number", + }, + "type": { + Type: "string", + Description: "Type of this issue. Only use if issue types are enabled for this repository. Use list_issue_types tool to get valid type values for this repository or its owner organization. If the repository doesn't support issue types, omit this parameter.", + }, + "state": { + Type: "string", + Description: "New state", + Enum: []any{"open", "closed"}, + }, + "state_reason": { + Type: "string", + Description: "Reason for the state change. Ignored unless state is changed.", + Enum: []any{"completed", "not_planned", "duplicate"}, + }, + "duplicate_of": { + Type: "number", + Description: "Issue number that this issue is a duplicate of. Only used when state_reason is 'duplicate'.", + }, + "issue_fields": { + Type: "array", + Description: "Issue field values to set or clear. Each item requires 'field_name' and exactly one of 'value', 'field_option_name', or 'delete: true'.", + Items: &jsonschema.Schema{ + Type: "object", + AdditionalProperties: &jsonschema.Schema{Not: &jsonschema.Schema{}}, + Properties: map[string]*jsonschema.Schema{ + "field_name": { + Type: "string", + Description: "Issue field name (case-insensitive). Must match a field " + + "returned by list_issue_fields for this repository or its organization.", + }, + "value": { + Types: []string{"string", "number", "boolean"}, + Description: "Value to set. Use for text, number, and date fields " + + "(date as YYYY-MM-DD). For single-select fields, prefer " + + "'field_option_name' so the option is validated before the API " + + "call. Cannot be combined with 'field_option_name' or 'delete'.", + }, + "field_option_name": { + Type: "string", + Description: "Option name for single-select fields. Validated against " + + "the field's options before the API call. Cannot be combined with " + + "'value' or 'delete'.", + }, + "delete": { + Type: "boolean", + Enum: []any{true}, + Description: "Set to true to clear this field's current value on the " + + "issue. Cannot be combined with 'value' or 'field_option_name'.", + }, + }, + Required: []string{"field_name"}, + }, + }, + "show_ui": { + Type: "boolean", + Description: "Whether to render the MCP App form instead of executing the request immediately. Defaults to true. Set to false to skip the form and execute directly — useful when you have all required values (especially ones the form does not collect, like labels, assignees, milestone, type, issue_fields, or state changes) and the user has already confirmed the action.", + }, + }, + Required: []string{"method", "owner", "repo"}, + }, + }, + []scopes.Scope{scopes.Repo}, + func(ctx context.Context, deps ToolDependencies, req *mcp.CallToolRequest, args map[string]any) (*mcp.CallToolResult, any, error) { + method, err := RequiredParam[string](args, "method") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + owner, err := RequiredParam[string](args, "owner") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + repo, err := RequiredParam[string](args, "repo") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + uiSubmitted, _ := OptionalParam[bool](args, "_ui_submitted") + showUI, err := OptionalBoolParamWithDefault(args, "show_ui", true) + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + if deps.IsFeatureEnabled(ctx, MCPAppsFeatureFlag) && clientSupportsUI(ctx, req) && !uiSubmitted && showUI && !issueWriteHasNonFormParams(args) { + issueNumber := 0 + if method == "update" { + n, numErr := RequiredInt(args, "issue_number") + if numErr != nil { + return utils.NewToolResultError("issue_number is required for update method"), nil, nil + } + issueNumber = n + } + return issueWriteAwaitingFormResult(method, owner, repo, issueNumber), nil, nil + } + + title, err := OptionalParam[string](args, "title") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + body, err := OptionalParam[string](args, "body") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + assignees, err := OptionalStringArrayParam(args, "assignees") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + assigneesValue, assigneesProvided := args["assignees"] + assigneesProvided = assigneesProvided && assigneesValue != nil + + labels, err := OptionalStringArrayParam(args, "labels") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + labelsValue, labelsProvided := args["labels"] + labelsProvided = labelsProvided && labelsValue != nil + + milestone, err := OptionalIntParam(args, "milestone") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + var milestoneNum int + if milestone != 0 { + milestoneNum = milestone + } + + issueType, err := OptionalParam[string](args, "type") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + state, err := OptionalParam[string](args, "state") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + stateReason, err := OptionalParam[string](args, "state_reason") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + duplicateOf, err := OptionalIntParam(args, "duplicate_of") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + if duplicateOf != 0 && stateReason != "duplicate" { + return utils.NewToolResultError("duplicate_of can only be used when state_reason is 'duplicate'"), nil, nil + } + + var issueFields []issueWriteFieldInput + issueFields, err = optionalIssueWriteFields(args, false) + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + + client, err := deps.GetClient(ctx) + if err != nil { + return utils.NewToolResultErrorFromErr("failed to get GitHub client", err), nil, nil + } + + gqlClient, err := deps.GetGQLClient(ctx) + if err != nil { + return utils.NewToolResultErrorFromErr("failed to get GraphQL client", err), nil, nil + } + + var issueFieldValues []*github.IssueRequestFieldValue + var fieldIDsToDelete []int64 + if len(issueFields) > 0 { + issueFieldValues, fieldIDsToDelete, err = resolveIssueRequestFieldValues(ctx, gqlClient, owner, repo, issueFields, false) + if err != nil { + return utils.NewToolResultError(fmt.Sprintf("failed to resolve issue_fields: %v", err)), nil, nil + } + } + + switch method { + case "create": + result, err := CreateIssue(ctx, client, owner, repo, title, body, assignees, labels, milestoneNum, issueType, issueFieldValues) + return result, nil, err + case "update": + issueNumber, err := RequiredInt(args, "issue_number") + if err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } + result, err := UpdateIssue(ctx, client, gqlClient, owner, repo, issueNumber, title, body, assignees, labels, milestoneNum, issueType, issueFieldValues, fieldIDsToDelete, state, stateReason, duplicateOf, UpdateIssueOptions{ + AssigneesProvided: assigneesProvided, + LabelsProvided: labelsProvided, + }) + return result, nil, err + default: + return utils.NewToolResultError("invalid method, must be either 'create' or 'update'"), nil, nil + } + }) + st.FeatureFlagDisable = []string{FeatureFlagIssuesGranular, FeatureFlagIssueFieldsMultiSelect} + return st +} diff --git a/pkg/github/issues_multiselect_test.go b/pkg/github/issues_multiselect_test.go new file mode 100644 index 000000000..df9e64d88 --- /dev/null +++ b/pkg/github/issues_multiselect_test.go @@ -0,0 +1,479 @@ +package github + +import ( + "context" + "testing" + + "github.com/github/github-mcp-server/internal/githubv4mock" + "github.com/shurcooL/githubv4" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// multi_select fixture reused across these tests. +func multiSelectField() IssueField { + return IssueField{ + ID: "IFMS_1", + Name: "Components", + DataType: "MULTI_SELECT", + Options: []IssueSingleSelectFieldOption{ + {ID: "OPT_AUTH", Name: "Auth", Color: "red"}, + {ID: "OPT_BILL", Name: "Billing", Color: "yellow"}, + {ID: "OPT_API", Name: "API", Color: "blue"}, + }, + } +} + +func Test_parseRawFieldFilters_MultiSelect(t *testing.T) { + t.Parallel() + + t.Run("accepts values as []any of strings", func(t *testing.T) { + filters, err := parseRawFieldFilters(map[string]any{ + "field_filters": []any{ + map[string]any{ + "field_name": "Components", + "values": []any{"Auth", "Billing"}, + }, + }, + }, true) + require.NoError(t, err) + require.Len(t, filters, 1) + assert.Equal(t, "Components", filters[0].Name) + assert.False(t, filters[0].HasValue) + assert.True(t, filters[0].HasValues) + assert.Equal(t, []string{"Auth", "Billing"}, filters[0].Values) + }) + + t.Run("accepts values as []string", func(t *testing.T) { + filters, err := parseRawFieldFilters(map[string]any{ + "field_filters": []map[string]any{ + {"field_name": "Components", "values": []string{"Auth"}}, + }, + }, true) + require.NoError(t, err) + require.Len(t, filters, 1) + assert.Equal(t, []string{"Auth"}, filters[0].Values) + }) + + t.Run("rejects both value and values on the same entry", func(t *testing.T) { + _, err := parseRawFieldFilters(map[string]any{ + "field_filters": []any{ + map[string]any{ + "field_name": "Components", + "value": "Auth", + "values": []any{"Auth"}, + }, + }, + }, true) + require.Error(t, err) + assert.Contains(t, err.Error(), "provide either 'value' or 'values', not both") + }) + + t.Run("rejects entry with neither value nor values", func(t *testing.T) { + _, err := parseRawFieldFilters(map[string]any{ + "field_filters": []any{ + map[string]any{"field_name": "Components"}, + }, + }, true) + require.Error(t, err) + assert.Contains(t, err.Error(), "missing 'value'") + }) + + t.Run("rejects values items that are not strings", func(t *testing.T) { + _, err := parseRawFieldFilters(map[string]any{ + "field_filters": []any{ + map[string]any{ + "field_name": "Components", + "values": []any{"Auth", 7}, + }, + }, + }, true) + require.Error(t, err) + assert.Contains(t, err.Error(), "values must be an array of strings") + }) +} + +func Test_resolveFieldFilters_MultiSelect(t *testing.T) { + t.Parallel() + + fields := []IssueField{multiSelectField()} + + t.Run("matches options case-insensitively and sets MultiSelectOptionValues", func(t *testing.T) { + raw := []rawFieldFilter{{ + Name: "Components", + Values: []string{"auth", "BILLING"}, + HasValues: true, + }} + out, err := resolveFieldFilters(raw, fields) + require.NoError(t, err) + require.Len(t, out, 1) + require.NotNil(t, out[0].MultiSelectOptionValues) + got := *out[0].MultiSelectOptionValues + assert.Equal(t, []githubv4.String{"Auth", "Billing"}, got) + assert.Nil(t, out[0].SingleSelectOptionValue) + }) + + t.Run("trims surrounding whitespace when matching options", func(t *testing.T) { + raw := []rawFieldFilter{{ + Name: "Components", + Values: []string{" auth ", " Billing"}, + HasValues: true, + }} + out, err := resolveFieldFilters(raw, fields) + require.NoError(t, err) + require.Len(t, out, 1) + require.NotNil(t, out[0].MultiSelectOptionValues) + assert.Equal(t, []githubv4.String{"Auth", "Billing"}, *out[0].MultiSelectOptionValues) + }) + + t.Run("rejects unknown option and lists valid ones", func(t *testing.T) { + raw := []rawFieldFilter{{ + Name: "Components", + Values: []string{"Auth", "Nope"}, + HasValues: true, + }} + _, err := resolveFieldFilters(raw, fields) + require.Error(t, err) + assert.Contains(t, err.Error(), "\"Nope\" is not a valid option") + assert.Contains(t, err.Error(), "Auth, Billing, API") + }) + + t.Run("rejects scalar value on multi_select field", func(t *testing.T) { + raw := []rawFieldFilter{{ + Name: "Components", + Value: "Auth", + HasValue: true, + }} + _, err := resolveFieldFilters(raw, fields) + require.Error(t, err) + assert.Contains(t, err.Error(), "is multi_select, use 'values'") + }) + + t.Run("rejects empty values slice", func(t *testing.T) { + raw := []rawFieldFilter{{ + Name: "Components", + Values: []string{}, + HasValues: true, + }} + _, err := resolveFieldFilters(raw, fields) + require.Error(t, err) + assert.Contains(t, err.Error(), "requires at least one value") + }) + + t.Run("rejects values array on single_select field", func(t *testing.T) { + ssFields := []IssueField{{ + Name: "Priority", + DataType: "SINGLE_SELECT", + Options: []IssueSingleSelectFieldOption{{Name: "P1"}, {Name: "P2"}}, + }} + raw := []rawFieldFilter{{ + Name: "Priority", + Values: []string{"P1"}, + HasValues: true, + }} + _, err := resolveFieldFilters(raw, ssFields) + require.Error(t, err) + assert.Contains(t, err.Error(), "is single_select, use 'value'") + }) +} + +func Test_fragmentToMinimalFieldValue_MultiSelect(t *testing.T) { + t.Parallel() + + fv := IssueFieldValueFragment{ + TypeName: "IssueFieldMultiSelectValue", + } + fv.MultiSelectValue.Field.MultiSelect.Name = "Components" + fv.MultiSelectValue.Field.MultiSelect.FullDatabaseID = "42" + fv.MultiSelectValue.Options = []struct { + Name githubv4.String + }{ + {Name: "Auth"}, + {Name: "Billing"}, + } + + m, ok := fragmentToMinimalFieldValue(fv) + require.True(t, ok) + assert.Equal(t, "Components", m.Field) + assert.Equal(t, []string{"Auth", "Billing"}, m.Values) + assert.Empty(t, m.Value) +} + +func Test_IssueFieldRef_Name_MultiSelect(t *testing.T) { + t.Parallel() + + var ref IssueFieldRef + ref.MultiSelect.Name = "Components" + ref.MultiSelect.FullDatabaseID = "99" + + assert.Equal(t, "Components", ref.Name()) + assert.Equal(t, "99", ref.FullDatabaseIDStr()) +} + +func Test_optionalIssueWriteFields_MultiSelect(t *testing.T) { + t.Parallel() + + t.Run("accepts field_option_names as []any of strings", func(t *testing.T) { + fields, err := optionalIssueWriteFields(map[string]any{ + "issue_fields": []any{ + map[string]any{ + "field_name": "Components", + "field_option_names": []any{"Auth", "Billing"}, + }, + }, + }, true) + require.NoError(t, err) + require.Len(t, fields, 1) + assert.Equal(t, "Components", fields[0].FieldName) + assert.Equal(t, []string{"Auth", "Billing"}, fields[0].FieldOptionNames) + assert.Empty(t, fields[0].FieldOptionName) + assert.Nil(t, fields[0].Value) + }) + + t.Run("accepts field_option_names as []string", func(t *testing.T) { + fields, err := optionalIssueWriteFields(map[string]any{ + "issue_fields": []any{ + map[string]any{ + "field_name": "Components", + "field_option_names": []string{"Auth"}, + }, + }, + }, true) + require.NoError(t, err) + require.Len(t, fields, 1) + assert.Equal(t, []string{"Auth"}, fields[0].FieldOptionNames) + }) + + t.Run("rejects empty field_option_names slice", func(t *testing.T) { + _, err := optionalIssueWriteFields(map[string]any{ + "issue_fields": []any{ + map[string]any{ + "field_name": "Components", + "field_option_names": []any{}, + }, + }, + }, true) + require.Error(t, err) + // An empty slice is a common "clear the field" guess; nudge callers to delete:true + // so the GraphQL deletion mutation runs instead of an unintentional no-op. + assert.Contains(t, err.Error(), "empty field_option_names") + assert.Contains(t, err.Error(), "delete: true") + }) + + t.Run("rejects field_option_names + value combo", func(t *testing.T) { + _, err := optionalIssueWriteFields(map[string]any{ + "issue_fields": []any{ + map[string]any{ + "field_name": "Components", + "value": "Auth", + "field_option_names": []any{"Auth"}, + }, + }, + }, true) + require.Error(t, err) + assert.Contains(t, err.Error(), "cannot specify more than one of value, field_option_name, or field_option_names") + }) + + t.Run("rejects field_option_names + field_option_name combo", func(t *testing.T) { + _, err := optionalIssueWriteFields(map[string]any{ + "issue_fields": []any{ + map[string]any{ + "field_name": "Components", + "field_option_name": "Auth", + "field_option_names": []any{"Auth"}, + }, + }, + }, true) + require.Error(t, err) + assert.Contains(t, err.Error(), "cannot specify more than one of") + }) + + t.Run("rejects field_option_names + delete combo", func(t *testing.T) { + _, err := optionalIssueWriteFields(map[string]any{ + "issue_fields": []any{ + map[string]any{ + "field_name": "Components", + "delete": true, + "field_option_names": []any{"Auth"}, + }, + }, + }, true) + require.Error(t, err) + assert.Contains(t, err.Error(), "cannot specify 'delete' together with") + }) +} + +// Test_optionalIssueWriteFields_LegacyRejectsFieldOptionNames covers the +// legacy (FF-off) parser path. The legacy IssueWriteLegacy tool schema does +// not advertise field_option_names at all, so any caller that passes the key +// must get a neutral "not supported" error — not a message that mentions +// multi-select or hints at how to clear the field, which would leak the +// existence of the gated MS feature into the legacy surface. +func Test_optionalIssueWriteFields_LegacyRejectsFieldOptionNames(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + args map[string]any + }{ + { + name: "non-empty array", + args: map[string]any{ + "issue_fields": []any{ + map[string]any{ + "field_name": "Components", + "field_option_names": []any{"Auth", "Billing"}, + }, + }, + }, + }, + { + // Empty array used to slip past the FF check because the + // `rawNames != nil` guard treated it as "absent", which sent the + // caller down the multi-select-flavoured "empty field_option_names + // — use 'delete: true' to clear the field" error path. Now the + // presence of the key alone is enough to reject in legacy mode. + name: "empty array", + args: map[string]any{ + "issue_fields": []any{ + map[string]any{ + "field_name": "Components", + "field_option_names": []any{}, + }, + }, + }, + }, + { + name: "null value", + args: map[string]any{ + "issue_fields": []any{ + map[string]any{ + "field_name": "Components", + "field_option_names": nil, + }, + }, + }, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + _, err := optionalIssueWriteFields(tc.args, false) + require.Error(t, err, "legacy mode must reject field_option_names regardless of value shape") + assert.Contains(t, err.Error(), "field_option_names is not supported in this build", + "error must be the neutral 'not supported' message, not a multi-select-flavoured one") + assert.NotContains(t, err.Error(), "multi-select") + assert.NotContains(t, err.Error(), "delete: true to clear", + "the 'use delete:true to clear' hint is a multi-select-specific message that must not appear in legacy mode") + }) + } +} + +func Test_resolveIssueRequestFieldValues_MultiSelect(t *testing.T) { + t.Parallel() + + // Mocked metadata GraphQL response — one IssueFieldMultiSelect field with three options. + metaResponse := githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "issueFields": map[string]any{ + "nodes": []any{ + map[string]any{ + "__typename": "IssueFieldMultiSelect", + "fullDatabaseId": "101", + "name": "Components", + "dataType": "multi_select", + "options": []any{ + map[string]any{"fullDatabaseId": "9001", "name": "Auth"}, + map[string]any{"fullDatabaseId": "9002", "name": "Billing"}, + map[string]any{"fullDatabaseId": "9003", "name": "API"}, + }, + }, + }, + }, + }, + }) + + newClient := func() *githubv4.Client { + return githubv4.NewClient(githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + issueFieldWriteMetadataQuery{}, + map[string]any{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + }, + metaResponse, + ), + )) + } + + t.Run("resolves option names to []string payload", func(t *testing.T) { + in := []issueWriteFieldInput{{ + FieldName: "Components", + FieldOptionNames: []string{"Auth", "Billing"}, + }} + vals, _, err := resolveIssueRequestFieldValues(context.Background(), newClient(), "owner", "repo", in, true) + require.NoError(t, err) + require.Len(t, vals, 1) + require.NotNil(t, vals[0].Value) + // REST IssueField#build_value_attributes expects an array of option names for multi_select. + got, ok := vals[0].Value.([]string) + require.True(t, ok, "value should be []string, got %T", vals[0].Value) + assert.Equal(t, []string{"Auth", "Billing"}, got) + }) + + t.Run("matches options case-insensitively and returns canonical names", func(t *testing.T) { + in := []issueWriteFieldInput{{ + FieldName: "Components", + FieldOptionNames: []string{"auth", "BILLING"}, + }} + vals, _, err := resolveIssueRequestFieldValues(context.Background(), newClient(), "owner", "repo", in, true) + require.NoError(t, err) + got, ok := vals[0].Value.([]string) + require.True(t, ok) + assert.Equal(t, []string{"Auth", "Billing"}, got) + }) + + t.Run("rejects unknown option name and lists valid ones", func(t *testing.T) { + in := []issueWriteFieldInput{{ + FieldName: "Components", + FieldOptionNames: []string{"Auth", "Nope"}, + }} + _, _, err := resolveIssueRequestFieldValues(context.Background(), newClient(), "owner", "repo", in, true) + require.Error(t, err) + assert.Contains(t, err.Error(), "Nope") + }) + + t.Run("rejects scalar field_option_name on multi_select field", func(t *testing.T) { + in := []issueWriteFieldInput{{ + FieldName: "Components", + FieldOptionName: "Auth", + }} + _, _, err := resolveIssueRequestFieldValues(context.Background(), newClient(), "owner", "repo", in, true) + require.Error(t, err) + assert.Contains(t, err.Error(), "field_option_name cannot be used") + assert.Contains(t, err.Error(), "field_option_names") + }) + + t.Run("rejects raw value on multi_select field", func(t *testing.T) { + in := []issueWriteFieldInput{{ + FieldName: "Components", + Value: "Auth,Billing", + }} + _, _, err := resolveIssueRequestFieldValues(context.Background(), newClient(), "owner", "repo", in, true) + require.Error(t, err) + assert.Contains(t, err.Error(), "multi_select") + }) + + t.Run("captures node ID for delete:true so the GraphQL mutation can clear the field", func(t *testing.T) { + in := []issueWriteFieldInput{{ + FieldName: "Components", + Delete: true, + }} + vals, deletions, err := resolveIssueRequestFieldValues(context.Background(), newClient(), "owner", "repo", in, true) + require.NoError(t, err) + assert.Empty(t, vals) + require.Len(t, deletions, 1) + assert.Equal(t, int64(101), deletions[0], + "the resolver returns the database field ID; the REST DELETE endpoint takes this integer in its URL path") + }) +} + diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index 2dea639f8..1e53c0d3e 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -519,7 +519,7 @@ func Test_GetIssue_FieldValues_Enriched(t *testing.T) { }, }) - const nodesQueryString = "query($ids:[ID!]!){nodes(ids: $ids){... on Issue{id,issueFieldValues(first: 25){nodes{__typename,... on IssueFieldDateValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value},... on IssueFieldNumberValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},valueNumber: value},... on IssueFieldSingleSelectValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value},... on IssueFieldTextValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value}}}}}}" + const nodesQueryString = "query($ids:[ID!]!){nodes(ids: $ids){... on Issue{id,issueFieldValues(first: 25){nodes{__typename,... on IssueFieldDateValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldMultiSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value},... on IssueFieldNumberValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldMultiSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},valueNumber: value},... on IssueFieldSingleSelectValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldMultiSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value},... on IssueFieldMultiSelectValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldMultiSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},options{name}},... on IssueFieldTextValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldMultiSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value}}}}}}" matcher := githubv4mock.NewQueryMatcher(nodesQueryString, gqlVars, gqlResponse) gqlClient := githubv4.NewClient(githubv4mock.NewMockedHTTPClient(matcher)) @@ -1243,7 +1243,7 @@ func Test_SearchIssues_FieldValuesEnrichment(t *testing.T) { }, }) - const nodesQueryString = "query($ids:[ID!]!){nodes(ids: $ids){... on Issue{id,issueFieldValues(first: 25){nodes{__typename,... on IssueFieldDateValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value},... on IssueFieldNumberValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},valueNumber: value},... on IssueFieldSingleSelectValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value},... on IssueFieldTextValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value}}}}}}" + const nodesQueryString = "query($ids:[ID!]!){nodes(ids: $ids){... on Issue{id,issueFieldValues(first: 25){nodes{__typename,... on IssueFieldDateValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldMultiSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value},... on IssueFieldNumberValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldMultiSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},valueNumber: value},... on IssueFieldSingleSelectValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldMultiSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value},... on IssueFieldMultiSelectValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldMultiSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},options{name}},... on IssueFieldTextValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldMultiSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value}}}}}}" matcher := githubv4mock.NewQueryMatcher(nodesQueryString, gqlVars, gqlResponse) gqlClient := githubv4.NewClient(githubv4mock.NewMockedHTTPClient(matcher)) @@ -1280,8 +1280,20 @@ func Test_CreateIssue(t *testing.T) { // Verify tool definition once serverTool := IssueWrite(translations.NullTranslationHelper) tool := serverTool.Tool - require.NoError(t, toolsnaps.Test(tool.Name, tool)) - require.Empty(t, serverTool.FeatureFlagEnable) + require.NoError(t, toolsnaps.Test(tool.Name+"_ff_"+FeatureFlagIssueFieldsMultiSelect, tool)) + assert.Equal(t, FeatureFlagIssueFieldsMultiSelect, serverTool.FeatureFlagEnable, "IssueWrite is the multi-select-aware variant and must be gated on the FF") + + // The legacy (FF-off) variant owns the canonical issue_write.snap; it must + // also continue to register under the issue_write tool name with the + // FeatureFlagIssueFieldsMultiSelect flag in its Disable list so the two + // variants are mutually exclusive at registration time. + legacyServerTool := IssueWriteLegacy(translations.NullTranslationHelper) + legacyTool := legacyServerTool.Tool + require.NoError(t, toolsnaps.Test(legacyTool.Name, legacyTool)) + assert.Equal(t, "issue_write", legacyTool.Name) + assert.Empty(t, legacyServerTool.FeatureFlagEnable, "IssueWriteLegacy is the FF-off variant and must not require any flag to be enabled") + assert.ElementsMatch(t, []string{FeatureFlagIssuesGranular, FeatureFlagIssueFieldsMultiSelect}, legacyServerTool.FeatureFlagDisable, "IssueWriteLegacy must be hidden when either the granular tools flag or the multi-select flag is on") + assert.NotContains(t, legacyTool.InputSchema.(*jsonschema.Schema).Properties["issue_fields"].Items.Properties, "field_option_names", "the legacy variant must not advertise field_option_names") assert.Equal(t, "issue_write", tool.Name) assert.NotEmpty(t, tool.Description) @@ -1473,7 +1485,7 @@ func Test_CreateIssue(t *testing.T) { }, }, expectError: false, - expectedErrMsg: "cannot specify both value and field_option_name", + expectedErrMsg: "cannot specify more than one of value, field_option_name, or field_option_names", }, } @@ -1833,11 +1845,19 @@ func Test_issueWriteSchemaClassification(t *testing.T) { } func Test_ListIssues(t *testing.T) { - // Verify tool definition + // Verify tool definition. The MS-aware variant owns the _ff_ snap; + // the legacy variant owns the canonical list_issues.snap. serverTool := ListIssues(translations.NullTranslationHelper) tool := serverTool.Tool - require.NoError(t, toolsnaps.Test(tool.Name, tool)) - require.Empty(t, serverTool.FeatureFlagEnable) + require.NoError(t, toolsnaps.Test(tool.Name+"_ff_"+FeatureFlagIssueFieldsMultiSelect, tool)) + assert.Equal(t, FeatureFlagIssueFieldsMultiSelect, serverTool.FeatureFlagEnable, "ListIssues is the multi-select-aware variant and must be gated on the FF") + + legacyServerTool := ListIssuesLegacy(translations.NullTranslationHelper) + legacyTool := legacyServerTool.Tool + require.NoError(t, toolsnaps.Test(legacyTool.Name, legacyTool)) + assert.Empty(t, legacyServerTool.FeatureFlagEnable, "ListIssuesLegacy must not require any flag to be enabled") + assert.ElementsMatch(t, []string{FeatureFlagIssueFieldsMultiSelect}, legacyServerTool.FeatureFlagDisable, "ListIssuesLegacy must be hidden when the multi-select flag is on") + assert.NotContains(t, legacyTool.InputSchema.(*jsonschema.Schema).Properties["field_filters"].Items.Properties, "values", "the legacy variant must not advertise field_filters[].values") assert.Equal(t, "list_issues", tool.Name) assert.NotEmpty(t, tool.Description) @@ -2121,7 +2141,7 @@ func Test_ListIssues(t *testing.T) { } // Define the actual query strings that match the implementation - issueFieldValuesSelection := "issueFieldValues(first: 25){nodes{__typename,... on IssueFieldDateValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value},... on IssueFieldNumberValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},valueNumber: value},... on IssueFieldSingleSelectValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value},... on IssueFieldTextValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value}}}" + issueFieldValuesSelection := "issueFieldValues(first: 25){nodes{__typename,... on IssueFieldDateValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldMultiSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value},... on IssueFieldNumberValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldMultiSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},valueNumber: value},... on IssueFieldSingleSelectValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldMultiSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value},... on IssueFieldMultiSelectValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldMultiSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},options{name}},... on IssueFieldTextValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldMultiSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value}}}" qBasicNoLabels := "query($after:String$direction:OrderDirection!$first:Int!$issueFieldValues:[IssueFieldValueFilter!]!$orderBy:IssueOrderField!$owner:String!$repo:String!$states:[IssueState!]!){repository(owner: $owner, name: $repo){issues(first: $first, after: $after, states: $states, orderBy: {field: $orderBy, direction: $direction}, filterBy: {issueFieldValues: $issueFieldValues}){nodes{number,title,body,state,databaseId,author{login},createdAt,updatedAt,labels(first: 100){nodes{name,id,description}},comments{totalCount}," + issueFieldValuesSelection + "},pageInfo{hasNextPage,hasPreviousPage,startCursor,endCursor},totalCount},isPrivate}}" qWithLabels := "query($after:String$direction:OrderDirection!$first:Int!$issueFieldValues:[IssueFieldValueFilter!]!$labels:[String!]!$orderBy:IssueOrderField!$owner:String!$repo:String!$states:[IssueState!]!){repository(owner: $owner, name: $repo){issues(first: $first, after: $after, labels: $labels, states: $states, orderBy: {field: $orderBy, direction: $direction}, filterBy: {issueFieldValues: $issueFieldValues}){nodes{number,title,body,state,databaseId,author{login},createdAt,updatedAt,labels(first: 100){nodes{name,id,description}},comments{totalCount}," + issueFieldValuesSelection + "},pageInfo{hasNextPage,hasPreviousPage,startCursor,endCursor},totalCount},isPrivate}}" @@ -2306,8 +2326,8 @@ func Test_ListIssues_FieldFilters(t *testing.T) { ) } - qNoLabels := "query($after:String$direction:OrderDirection!$first:Int!$issueFieldValues:[IssueFieldValueFilter!]!$orderBy:IssueOrderField!$owner:String!$repo:String!$states:[IssueState!]!){repository(owner: $owner, name: $repo){issues(first: $first, after: $after, states: $states, orderBy: {field: $orderBy, direction: $direction}, filterBy: {issueFieldValues: $issueFieldValues}){nodes{number,title,body,state,databaseId,author{login},createdAt,updatedAt,labels(first: 100){nodes{name,id,description}},comments{totalCount},issueFieldValues(first: 25){nodes{__typename,... on IssueFieldDateValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value},... on IssueFieldNumberValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},valueNumber: value},... on IssueFieldSingleSelectValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value},... on IssueFieldTextValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value}}}},pageInfo{hasNextPage,hasPreviousPage,startCursor,endCursor},totalCount},isPrivate}}" - qWithLabels := "query($after:String$direction:OrderDirection!$first:Int!$issueFieldValues:[IssueFieldValueFilter!]!$labels:[String!]!$orderBy:IssueOrderField!$owner:String!$repo:String!$states:[IssueState!]!){repository(owner: $owner, name: $repo){issues(first: $first, after: $after, labels: $labels, states: $states, orderBy: {field: $orderBy, direction: $direction}, filterBy: {issueFieldValues: $issueFieldValues}){nodes{number,title,body,state,databaseId,author{login},createdAt,updatedAt,labels(first: 100){nodes{name,id,description}},comments{totalCount},issueFieldValues(first: 25){nodes{__typename,... on IssueFieldDateValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value},... on IssueFieldNumberValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},valueNumber: value},... on IssueFieldSingleSelectValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value},... on IssueFieldTextValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value}}}},pageInfo{hasNextPage,hasPreviousPage,startCursor,endCursor},totalCount},isPrivate}}" + qNoLabels := "query($after:String$direction:OrderDirection!$first:Int!$issueFieldValues:[IssueFieldValueFilter!]!$orderBy:IssueOrderField!$owner:String!$repo:String!$states:[IssueState!]!){repository(owner: $owner, name: $repo){issues(first: $first, after: $after, states: $states, orderBy: {field: $orderBy, direction: $direction}, filterBy: {issueFieldValues: $issueFieldValues}){nodes{number,title,body,state,databaseId,author{login},createdAt,updatedAt,labels(first: 100){nodes{name,id,description}},comments{totalCount},issueFieldValues(first: 25){nodes{__typename,... on IssueFieldDateValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldMultiSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value},... on IssueFieldNumberValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldMultiSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},valueNumber: value},... on IssueFieldSingleSelectValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldMultiSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value},... on IssueFieldMultiSelectValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldMultiSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},options{name}},... on IssueFieldTextValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldMultiSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value}}}},pageInfo{hasNextPage,hasPreviousPage,startCursor,endCursor},totalCount},isPrivate}}" + qWithLabels := "query($after:String$direction:OrderDirection!$first:Int!$issueFieldValues:[IssueFieldValueFilter!]!$labels:[String!]!$orderBy:IssueOrderField!$owner:String!$repo:String!$states:[IssueState!]!){repository(owner: $owner, name: $repo){issues(first: $first, after: $after, labels: $labels, states: $states, orderBy: {field: $orderBy, direction: $direction}, filterBy: {issueFieldValues: $issueFieldValues}){nodes{number,title,body,state,databaseId,author{login},createdAt,updatedAt,labels(first: 100){nodes{name,id,description}},comments{totalCount},issueFieldValues(first: 25){nodes{__typename,... on IssueFieldDateValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldMultiSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value},... on IssueFieldNumberValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldMultiSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},valueNumber: value},... on IssueFieldSingleSelectValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldMultiSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value},... on IssueFieldMultiSelectValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldMultiSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},options{name}},... on IssueFieldTextValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldMultiSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value}}}},pageInfo{hasNextPage,hasPreviousPage,startCursor,endCursor},totalCount},isPrivate}}" baseVars := func() map[string]any { return map[string]any{ @@ -2668,7 +2688,7 @@ func Test_ListIssues_IFC_InsidersMode(t *testing.T) { }) } - query := "query($after:String$direction:OrderDirection!$first:Int!$issueFieldValues:[IssueFieldValueFilter!]!$orderBy:IssueOrderField!$owner:String!$repo:String!$states:[IssueState!]!){repository(owner: $owner, name: $repo){issues(first: $first, after: $after, states: $states, orderBy: {field: $orderBy, direction: $direction}, filterBy: {issueFieldValues: $issueFieldValues}){nodes{number,title,body,state,databaseId,author{login},createdAt,updatedAt,labels(first: 100){nodes{name,id,description}},comments{totalCount},issueFieldValues(first: 25){nodes{__typename,... on IssueFieldDateValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value},... on IssueFieldNumberValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},valueNumber: value},... on IssueFieldSingleSelectValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value},... on IssueFieldTextValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value}}}},pageInfo{hasNextPage,hasPreviousPage,startCursor,endCursor},totalCount},isPrivate}}" + query := "query($after:String$direction:OrderDirection!$first:Int!$issueFieldValues:[IssueFieldValueFilter!]!$orderBy:IssueOrderField!$owner:String!$repo:String!$states:[IssueState!]!){repository(owner: $owner, name: $repo){issues(first: $first, after: $after, states: $states, orderBy: {field: $orderBy, direction: $direction}, filterBy: {issueFieldValues: $issueFieldValues}){nodes{number,title,body,state,databaseId,author{login},createdAt,updatedAt,labels(first: 100){nodes{name,id,description}},comments{totalCount},issueFieldValues(first: 25){nodes{__typename,... on IssueFieldDateValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldMultiSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value},... on IssueFieldNumberValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldMultiSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},valueNumber: value},... on IssueFieldSingleSelectValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldMultiSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value},... on IssueFieldMultiSelectValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldMultiSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},options{name}},... on IssueFieldTextValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldMultiSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value}}}},pageInfo{hasNextPage,hasPreviousPage,startCursor,endCursor},totalCount},isPrivate}}" vars := map[string]any{ "owner": "octocat", @@ -2755,10 +2775,10 @@ func Test_ListIssues_IFC_InsidersMode(t *testing.T) { } func Test_UpdateIssue(t *testing.T) { - // Verify tool definition + // Verify tool definition. The MS-aware variant owns the _ff_ snap. serverTool := IssueWrite(translations.NullTranslationHelper) tool := serverTool.Tool - require.NoError(t, toolsnaps.Test(tool.Name, tool)) + require.NoError(t, toolsnaps.Test(tool.Name+"_ff_"+FeatureFlagIssueFieldsMultiSelect, tool)) assert.Equal(t, "issue_write", tool.Name) assert.NotEmpty(t, tool.Description) @@ -2930,7 +2950,7 @@ func Test_UpdateIssue(t *testing.T) { mockedGQLClient: githubv4mock.NewMockedHTTPClient( // fetch-and-merge: returns no existing fields so the incoming values are used as-is githubv4mock.NewQueryMatcher( - "query($number:Int!$owner:String!$repo:String!){repository(owner: $owner, name: $repo){issue(number: $number){issueFieldValues(first: 25){nodes{__typename,... on IssueFieldDateValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value},... on IssueFieldNumberValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},valueNumber: value},... on IssueFieldSingleSelectValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value},... on IssueFieldTextValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value}}}}}}", + "query($number:Int!$owner:String!$repo:String!){repository(owner: $owner, name: $repo){issue(number: $number){issueFieldValues(first: 25){nodes{__typename,... on IssueFieldDateValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldMultiSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value},... on IssueFieldNumberValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldMultiSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},valueNumber: value},... on IssueFieldSingleSelectValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldMultiSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value},... on IssueFieldMultiSelectValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldMultiSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},options{name}},... on IssueFieldTextValue{field{... on IssueFieldDate{name,fullDatabaseId},... on IssueFieldNumber{name,fullDatabaseId},... on IssueFieldSingleSelect{name,fullDatabaseId},... on IssueFieldMultiSelect{name,fullDatabaseId},... on IssueFieldText{name,fullDatabaseId}},value}}}}}}", map[string]any{ "owner": githubv4.String("owner"), "repo": githubv4.String("repo"), @@ -3712,128 +3732,6 @@ func Test_GetIssueLabels(t *testing.T) { } } -func Test_GetIssueParent(t *testing.T) { - t.Parallel() - - serverTool := IssueRead(translations.NullTranslationHelper) - - parentMatcherStruct := struct { - Repository struct { - Issue struct { - Parent *struct { - Number githubv4.Int - Title githubv4.String - State githubv4.String - URL githubv4.String - Repository struct { - NameWithOwner githubv4.String - } - } - } `graphql:"issue(number: $issueNumber)"` - } `graphql:"repository(owner: $owner, name: $repo)"` - }{} - - vars := map[string]any{ - "owner": githubv4.String("owner"), - "repo": githubv4.String("repo"), - "issueNumber": githubv4.Int(123), - } - - tests := []struct { - name string - mockedClient *http.Client - expectToolError bool - expectedText string - }{ - { - name: "issue has a parent", - mockedClient: githubv4mock.NewMockedHTTPClient( - githubv4mock.NewQueryMatcher( - parentMatcherStruct, - vars, - githubv4mock.DataResponse(map[string]any{ - "repository": map[string]any{ - "issue": map[string]any{ - "parent": map[string]any{ - "number": githubv4.Int(42), - "title": githubv4.String("Parent issue"), - "state": githubv4.String("OPEN"), - "url": githubv4.String("https://github.com/owner/repo/issues/42"), - "repository": map[string]any{ - "nameWithOwner": githubv4.String("owner/repo"), - }, - }, - }, - }, - }), - ), - ), - expectedText: `"number":42`, - }, - { - name: "issue has no parent", - mockedClient: githubv4mock.NewMockedHTTPClient( - githubv4mock.NewQueryMatcher( - parentMatcherStruct, - vars, - githubv4mock.DataResponse(map[string]any{ - "repository": map[string]any{ - "issue": map[string]any{ - "parent": nil, - }, - }, - }), - ), - ), - expectedText: `"parent":null`, - }, - { - name: "graphql error", - mockedClient: githubv4mock.NewMockedHTTPClient( - githubv4mock.NewQueryMatcher( - parentMatcherStruct, - vars, - githubv4mock.ErrorResponse("issue not found"), - ), - ), - expectToolError: true, - }, - } - - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - gqlClient := githubv4.NewClient(tc.mockedClient) - client := mustNewGHClient(t, nil) - deps := BaseDeps{ - Client: client, - GQLClient: gqlClient, - RepoAccessCache: stubRepoAccessCache(nil, 15*time.Minute), - Flags: stubFeatureFlags(map[string]bool{"lockdown-mode": false}), - } - handler := serverTool.Handler(deps) - - request := createMCPRequest(map[string]any{ - "method": "get_parent", - "owner": "owner", - "repo": "repo", - "issue_number": float64(123), - }) - result, err := handler(ContextWithDeps(context.Background(), deps), &request) - - require.NoError(t, err) - require.NotNil(t, result) - - if tc.expectToolError { - assert.True(t, result.IsError) - return - } - assert.False(t, result.IsError) - textContent := getTextResult(t, result) - assert.Contains(t, textContent.Text, tc.expectedText) - }) - } -} - func Test_AddSubIssue(t *testing.T) { // Verify tool definition once serverTool := SubIssueWrite(translations.NullTranslationHelper) @@ -4909,3 +4807,125 @@ func Test_ListIssueTypes(t *testing.T) { }) } } + +func Test_GetIssueParent(t *testing.T) { + t.Parallel() + + serverTool := IssueRead(translations.NullTranslationHelper) + + parentMatcherStruct := struct { + Repository struct { + Issue struct { + Parent *struct { + Number githubv4.Int + Title githubv4.String + State githubv4.String + URL githubv4.String + Repository struct { + NameWithOwner githubv4.String + } + } + } `graphql:"issue(number: $issueNumber)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + }{} + + vars := map[string]any{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + "issueNumber": githubv4.Int(123), + } + + tests := []struct { + name string + mockedClient *http.Client + expectToolError bool + expectedText string + }{ + { + name: "issue has a parent", + mockedClient: githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + parentMatcherStruct, + vars, + githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "issue": map[string]any{ + "parent": map[string]any{ + "number": githubv4.Int(42), + "title": githubv4.String("Parent issue"), + "state": githubv4.String("OPEN"), + "url": githubv4.String("https://github.com/owner/repo/issues/42"), + "repository": map[string]any{ + "nameWithOwner": githubv4.String("owner/repo"), + }, + }, + }, + }, + }), + ), + ), + expectedText: `"number":42`, + }, + { + name: "issue has no parent", + mockedClient: githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + parentMatcherStruct, + vars, + githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "issue": map[string]any{ + "parent": nil, + }, + }, + }), + ), + ), + expectedText: `"parent":null`, + }, + { + name: "graphql error", + mockedClient: githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + parentMatcherStruct, + vars, + githubv4mock.ErrorResponse("issue not found"), + ), + ), + expectToolError: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + gqlClient := githubv4.NewClient(tc.mockedClient) + client := mustNewGHClient(t, nil) + deps := BaseDeps{ + Client: client, + GQLClient: gqlClient, + RepoAccessCache: stubRepoAccessCache(nil, 15*time.Minute), + Flags: stubFeatureFlags(map[string]bool{"lockdown-mode": false}), + } + handler := serverTool.Handler(deps) + + request := createMCPRequest(map[string]any{ + "method": "get_parent", + "owner": "owner", + "repo": "repo", + "issue_number": float64(123), + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + + require.NoError(t, err) + require.NotNil(t, result) + + if tc.expectToolError { + assert.True(t, result.IsError) + return + } + assert.False(t, result.IsError) + textContent := getTextResult(t, result) + assert.Contains(t, textContent.Text, tc.expectedText) + }) + } +} diff --git a/pkg/github/minimal_types.go b/pkg/github/minimal_types.go index 256bdcb91..75ab79d98 100644 --- a/pkg/github/minimal_types.go +++ b/pkg/github/minimal_types.go @@ -609,6 +609,15 @@ func fragmentToMinimalFieldValue(fv IssueFieldValueFragment) (MinimalFieldValue, Field: fv.SingleSelectValue.Field.Name(), Value: string(fv.SingleSelectValue.Value), }, true + case "IssueFieldMultiSelectValue": + values := make([]string, 0, len(fv.MultiSelectValue.Options)) + for _, opt := range fv.MultiSelectValue.Options { + values = append(values, string(opt.Name)) + } + return MinimalFieldValue{ + Field: fv.MultiSelectValue.Field.Name(), + Values: values, + }, true case "IssueFieldTextValue": return MinimalFieldValue{ Field: fv.TextValue.Field.Name(), diff --git a/pkg/github/tools.go b/pkg/github/tools.go index b937f8bfd..5d669aa8f 100644 --- a/pkg/github/tools.go +++ b/pkg/github/tools.go @@ -211,9 +211,12 @@ func AllTools(t translations.TranslationHelperFunc) []inventory.ServerTool { IssueRead(t), SearchIssues(t), ListIssues(t), + ListIssuesLegacy(t), ListIssueTypes(t), ListIssueFields(t), + ListIssueFieldsLegacy(t), IssueWrite(t), + IssueWriteLegacy(t), AddIssueComment(t), SubIssueWrite(t),