diff --git a/README.md b/README.md index 46eef3b4b..9e2af2c6d 100644 --- a/README.md +++ b/README.md @@ -591,12 +591,14 @@ The following sets of tools are available (all are on by default): - **update_issue** - Edit issue - `assignees`: New assignees (string[], optional) - `body`: New description (string, optional) + - `duplicate_of`: Issue number that this issue is a duplicate of. Only used when state_reason is 'duplicate'. (number, optional) - `issue_number`: Issue number to update (number, required) - `labels`: New labels (string[], optional) - `milestone`: New 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`: New title (string, optional) - `type`: New issue type (string, optional) diff --git a/pkg/github/__toolsnaps__/update_issue.snap b/pkg/github/__toolsnaps__/update_issue.snap index d95579159..5c3f0e638 100644 --- a/pkg/github/__toolsnaps__/update_issue.snap +++ b/pkg/github/__toolsnaps__/update_issue.snap @@ -17,6 +17,10 @@ "description": "New description", "type": "string" }, + "duplicate_of": { + "description": "Issue number that this issue is a duplicate of. Only used when state_reason is 'duplicate'.", + "type": "number" + }, "issue_number": { "description": "Issue number to update", "type": "number" @@ -48,6 +52,15 @@ ], "type": "string" }, + "state_reason": { + "description": "Reason for the state change. Ignored unless state is changed.", + "enum": [ + "completed", + "not_planned", + "duplicate" + ], + "type": "string" + }, "title": { "description": "New title", "type": "string" diff --git a/pkg/github/issues.go b/pkg/github/issues.go index b5e75b0e6..1c88a9fde 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -18,6 +18,87 @@ import ( "github.com/shurcooL/githubv4" ) +// CloseIssueInput represents the input for closing an issue via the GraphQL API. +// Used to extend the functionality of the githubv4 library to support closing issues as duplicates. +type CloseIssueInput struct { + IssueID githubv4.ID `json:"issueId"` + ClientMutationID *githubv4.String `json:"clientMutationId,omitempty"` + StateReason *IssueClosedStateReason `json:"stateReason,omitempty"` + DuplicateIssueID *githubv4.ID `json:"duplicateIssueId,omitempty"` +} + +// IssueClosedStateReason represents the reason an issue was closed. +// Used to extend the functionality of the githubv4 library to support closing issues as duplicates. +type IssueClosedStateReason string + +const ( + IssueClosedStateReasonCompleted IssueClosedStateReason = "COMPLETED" + IssueClosedStateReasonDuplicate IssueClosedStateReason = "DUPLICATE" + IssueClosedStateReasonNotPlanned IssueClosedStateReason = "NOT_PLANNED" +) + +// fetchIssueIDs retrieves issue IDs via the GraphQL API. +// When duplicateOf is 0, it fetches only the main issue ID. +// When duplicateOf is non-zero, it fetches both the main issue and duplicate issue IDs in a single query. +func fetchIssueIDs(ctx context.Context, gqlClient *githubv4.Client, owner, repo string, issueNumber int, duplicateOf int) (githubv4.ID, githubv4.ID, error) { + // Build query variables common to both cases + vars := map[string]interface{}{ + "owner": githubv4.String(owner), + "repo": githubv4.String(repo), + "issueNumber": githubv4.Int(issueNumber), // #nosec G115 - issue numbers are always small positive integers + } + + if duplicateOf == 0 { + // Only fetch the main issue ID + var query struct { + Repository struct { + Issue struct { + ID githubv4.ID + } `graphql:"issue(number: $issueNumber)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + } + + if err := gqlClient.Query(ctx, &query, vars); err != nil { + return "", "", fmt.Errorf("failed to get issue ID") + } + + return query.Repository.Issue.ID, "", nil + } + + // Fetch both issue IDs in a single query + var query struct { + Repository struct { + Issue struct { + ID githubv4.ID + } `graphql:"issue(number: $issueNumber)"` + DuplicateIssue struct { + ID githubv4.ID + } `graphql:"duplicateIssue: issue(number: $duplicateOf)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + } + + // Add duplicate issue number to variables + vars["duplicateOf"] = githubv4.Int(duplicateOf) // #nosec G115 - issue numbers are always small positive integers + + if err := gqlClient.Query(ctx, &query, vars); err != nil { + return "", "", fmt.Errorf("failed to get issue ID") + } + + return query.Repository.Issue.ID, query.Repository.DuplicateIssue.ID, nil +} + +// getCloseStateReason converts a string state reason to the appropriate enum value +func getCloseStateReason(stateReason string) IssueClosedStateReason { + switch stateReason { + case "not_planned": + return IssueClosedStateReasonNotPlanned + case "duplicate": + return IssueClosedStateReasonDuplicate + default: // Default to "completed" for empty or "completed" values + return IssueClosedStateReasonCompleted + } +} + // IssueFragment represents a fragment of an issue node in the GraphQL API. type IssueFragment struct { Number githubv4.Int @@ -1100,7 +1181,7 @@ func ListIssues(getGQLClient GetGQLClientFn, t translations.TranslationHelperFun } // UpdateIssue creates a tool to update an existing issue in a GitHub repository. -func UpdateIssue(getClient GetClientFn, t translations.TranslationHelperFunc) (tool mcp.Tool, handler server.ToolHandlerFunc) { +func UpdateIssue(getClient GetClientFn, getGQLClient GetGQLClientFn, t translations.TranslationHelperFunc) (tool mcp.Tool, handler server.ToolHandlerFunc) { return mcp.NewTool("update_issue", mcp.WithDescription(t("TOOL_UPDATE_ISSUE_DESCRIPTION", "Update an existing issue in a GitHub repository.")), mcp.WithToolAnnotation(mcp.ToolAnnotation{ @@ -1125,10 +1206,6 @@ func UpdateIssue(getClient GetClientFn, t translations.TranslationHelperFunc) (t mcp.WithString("body", mcp.Description("New description"), ), - mcp.WithString("state", - mcp.Description("New state"), - mcp.Enum("open", "closed"), - ), mcp.WithArray("labels", mcp.Description("New labels"), mcp.Items( @@ -1151,6 +1228,17 @@ func UpdateIssue(getClient GetClientFn, t translations.TranslationHelperFunc) (t mcp.WithString("type", mcp.Description("New issue type"), ), + mcp.WithString("state", + mcp.Description("New state"), + mcp.Enum("open", "closed"), + ), + mcp.WithString("state_reason", + mcp.Description("Reason for the state change. Ignored unless state is changed."), + mcp.Enum("completed", "not_planned", "duplicate"), + ), + mcp.WithNumber("duplicate_of", + mcp.Description("Issue number that this issue is a duplicate of. Only used when state_reason is 'duplicate'."), + ), ), func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { owner, err := RequiredParam[string](request, "owner") @@ -1186,14 +1274,6 @@ func UpdateIssue(getClient GetClientFn, t translations.TranslationHelperFunc) (t issueRequest.Body = github.Ptr(body) } - state, err := OptionalParam[string](request, "state") - if err != nil { - return mcp.NewToolResultError(err.Error()), nil - } - if state != "" { - issueRequest.State = github.Ptr(state) - } - // Get labels labels, err := OptionalStringArrayParam(request, "labels") if err != nil { @@ -1230,13 +1310,38 @@ func UpdateIssue(getClient GetClientFn, t translations.TranslationHelperFunc) (t issueRequest.Type = github.Ptr(issueType) } + // Handle state, state_reason and duplicateOf parameters + state, err := OptionalParam[string](request, "state") + if err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + + stateReason, err := OptionalParam[string](request, "state_reason") + if err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + + duplicateOf, err := OptionalIntParam(request, "duplicate_of") + if err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + if duplicateOf != 0 && stateReason != "duplicate" { + return mcp.NewToolResultError("duplicate_of can only be used when state_reason is 'duplicate'"), nil + } + + // Use REST API for non-state updates client, err := getClient(ctx) if err != nil { return nil, fmt.Errorf("failed to get GitHub client: %w", err) } + updatedIssue, resp, err := client.Issues.Edit(ctx, owner, repo, issueNumber, issueRequest) if err != nil { - return nil, fmt.Errorf("failed to update issue: %w", err) + return ghErrors.NewGitHubAPIErrorResponse(ctx, + "failed to update issue", + resp, + err, + ), nil } defer func() { _ = resp.Body.Close() }() @@ -1248,6 +1353,75 @@ func UpdateIssue(getClient GetClientFn, t translations.TranslationHelperFunc) (t return mcp.NewToolResultError(fmt.Sprintf("failed to update issue: %s", string(body))), nil } + // Use GraphQL API for state updates + if state != "" { + gqlClient, err := getGQLClient(ctx) + if err != nil { + return nil, fmt.Errorf("failed to get GraphQL client: %w", err) + } + + // Mandate specifying duplicateOf when trying to close as duplicate + if state == "closed" && stateReason == "duplicate" && duplicateOf == 0 { + return mcp.NewToolResultError("duplicate_of must be provided when state_reason is 'duplicate'"), nil + } + + // Get target issue ID (and duplicate issue ID if needed) + issueID, duplicateIssueID, err := fetchIssueIDs(ctx, gqlClient, owner, repo, issueNumber, duplicateOf) + if err != nil { + return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "Failed to find issues", err), nil + } + + switch state { + case "open": + // Use ReopenIssue mutation for opening + var mutation struct { + ReopenIssue struct { + Issue struct { + ID githubv4.ID + Number githubv4.Int + URL githubv4.String + State githubv4.String + } + } `graphql:"reopenIssue(input: $input)"` + } + + err = gqlClient.Mutate(ctx, &mutation, githubv4.ReopenIssueInput{ + IssueID: issueID, + }, nil) + if err != nil { + return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "Failed to reopen issue", err), nil + } + case "closed": + // Use CloseIssue mutation for closing + var mutation struct { + CloseIssue struct { + Issue struct { + ID githubv4.ID + Number githubv4.Int + URL githubv4.String + State githubv4.String + } + } `graphql:"closeIssue(input: $input)"` + } + + stateReasonValue := getCloseStateReason(stateReason) + closeInput := CloseIssueInput{ + IssueID: issueID, + StateReason: &stateReasonValue, + } + + // Set duplicate issue ID if needed + if stateReason == "duplicate" { + closeInput.DuplicateIssueID = &duplicateIssueID + } + + err = gqlClient.Mutate(ctx, &mutation, closeInput, nil) + if err != nil { + return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "Failed to close issue", err), nil + } + } + } + // Return minimal response with just essential information minimalResponse := MinimalResponse{ ID: fmt.Sprintf("%d", updatedIssue.GetID()), diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index 5a0d409a6..cc1923df9 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -1033,7 +1033,8 @@ func Test_ListIssues(t *testing.T) { func Test_UpdateIssue(t *testing.T) { // Verify tool definition mockClient := github.NewClient(nil) - tool, _ := UpdateIssue(stubGetClientFn(mockClient), translations.NullTranslationHelper) + mockGQLClient := githubv4.NewClient(nil) + tool, _ := UpdateIssue(stubGetClientFn(mockClient), stubGetGQLClientFn(mockGQLClient), translations.NullTranslationHelper) require.NoError(t, toolsnaps.Test(tool.Name, tool)) assert.Equal(t, "update_issue", tool.Name) @@ -1043,19 +1044,21 @@ func Test_UpdateIssue(t *testing.T) { assert.Contains(t, tool.InputSchema.Properties, "issue_number") assert.Contains(t, tool.InputSchema.Properties, "title") assert.Contains(t, tool.InputSchema.Properties, "body") - assert.Contains(t, tool.InputSchema.Properties, "state") assert.Contains(t, tool.InputSchema.Properties, "labels") assert.Contains(t, tool.InputSchema.Properties, "assignees") assert.Contains(t, tool.InputSchema.Properties, "milestone") assert.Contains(t, tool.InputSchema.Properties, "type") + assert.Contains(t, tool.InputSchema.Properties, "state") + assert.Contains(t, tool.InputSchema.Properties, "state_reason") + assert.Contains(t, tool.InputSchema.Properties, "duplicate_of") assert.ElementsMatch(t, tool.InputSchema.Required, []string{"owner", "repo", "issue_number"}) - // Setup mock issue for success case - mockIssue := &github.Issue{ + // Mock issues for reuse across test cases + mockBaseIssue := &github.Issue{ Number: github.Ptr(123), - Title: github.Ptr("Updated Issue Title"), - Body: github.Ptr("Updated issue description"), - State: github.Ptr("closed"), + Title: github.Ptr("Title"), + Body: github.Ptr("Description"), + State: github.Ptr("open"), HTMLURL: github.Ptr("https://github.com/owner/repo/issues/123"), Assignees: []*github.User{{Login: github.Ptr("assignee1")}, {Login: github.Ptr("assignee2")}}, Labels: []*github.Label{{Name: github.Ptr("bug")}, {Name: github.Ptr("priority")}}, @@ -1063,124 +1066,417 @@ func Test_UpdateIssue(t *testing.T) { Type: &github.IssueType{Name: github.Ptr("Bug")}, } + mockUpdatedIssue := &github.Issue{ + Number: github.Ptr(123), + Title: github.Ptr("Updated Title"), + Body: github.Ptr("Updated Description"), + State: github.Ptr("closed"), + StateReason: github.Ptr("duplicate"), + HTMLURL: github.Ptr("https://github.com/owner/repo/issues/123"), + Assignees: []*github.User{{Login: github.Ptr("assignee1")}, {Login: github.Ptr("assignee2")}}, + Labels: []*github.Label{{Name: github.Ptr("bug")}, {Name: github.Ptr("priority")}}, + Milestone: &github.Milestone{Number: github.Ptr(5)}, + Type: &github.IssueType{Name: github.Ptr("Bug")}, + } + + mockReopenedIssue := &github.Issue{ + Number: github.Ptr(123), + Title: github.Ptr("Title"), + State: github.Ptr("open"), + StateReason: github.Ptr("reopened"), + HTMLURL: github.Ptr("https://github.com/owner/repo/issues/123"), + } + + // Mock GraphQL responses for reuse across test cases + issueIDQueryResponse := githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "issue": map[string]any{ + "id": "I_kwDOA0xdyM50BPaO", + }, + }, + }) + + duplicateIssueIDQueryResponse := githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "issue": map[string]any{ + "id": "I_kwDOA0xdyM50BPaO", + }, + "duplicateIssue": map[string]any{ + "id": "I_kwDOA0xdyM50BPbP", + }, + }, + }) + + closeSuccessResponse := githubv4mock.DataResponse(map[string]any{ + "closeIssue": map[string]any{ + "issue": map[string]any{ + "id": "I_kwDOA0xdyM50BPaO", + "number": 123, + "url": "https://github.com/owner/repo/issues/123", + "state": "CLOSED", + }, + }, + }) + + reopenSuccessResponse := githubv4mock.DataResponse(map[string]any{ + "reopenIssue": map[string]any{ + "issue": map[string]any{ + "id": "I_kwDOA0xdyM50BPaO", + "number": 123, + "url": "https://github.com/owner/repo/issues/123", + "state": "OPEN", + }, + }, + }) + + duplicateStateReason := IssueClosedStateReasonDuplicate + tests := []struct { - name string - mockedClient *http.Client - requestArgs map[string]interface{} - expectError bool - expectedIssue *github.Issue - expectedErrMsg string + name string + mockedRESTClient *http.Client + mockedGQLClient *http.Client + requestArgs map[string]interface{} + expectError bool + expectedIssue *github.Issue + expectedErrMsg string }{ { - name: "update issue with all fields", - mockedClient: mock.NewMockedHTTPClient( + name: "partial update of non-state fields only", + mockedRESTClient: mock.NewMockedHTTPClient( mock.WithRequestMatchHandler( mock.PatchReposIssuesByOwnerByRepoByIssueNumber, - expectRequestBody(t, map[string]any{ - "title": "Updated Issue Title", - "body": "Updated issue description", - "state": "closed", - "labels": []any{"bug", "priority"}, - "assignees": []any{"assignee1", "assignee2"}, - "milestone": float64(5), - "type": "Bug", + expectRequestBody(t, map[string]interface{}{ + "title": "Updated Title", + "body": "Updated Description", }).andThen( - mockResponse(t, http.StatusOK, mockIssue), + mockResponse(t, http.StatusOK, mockUpdatedIssue), ), ), ), + mockedGQLClient: githubv4mock.NewMockedHTTPClient(), requestArgs: map[string]interface{}{ "owner": "owner", "repo": "repo", "issue_number": float64(123), - "title": "Updated Issue Title", - "body": "Updated issue description", - "state": "closed", - "labels": []any{"bug", "priority"}, - "assignees": []any{"assignee1", "assignee2"}, - "milestone": float64(5), - "type": "Bug", + "title": "Updated Title", + "body": "Updated Description", }, expectError: false, - expectedIssue: mockIssue, + expectedIssue: mockUpdatedIssue, }, { - name: "update issue with minimal fields", - mockedClient: mock.NewMockedHTTPClient( + name: "issue not found when updating non-state fields only", + mockedRESTClient: mock.NewMockedHTTPClient( mock.WithRequestMatchHandler( mock.PatchReposIssuesByOwnerByRepoByIssueNumber, - mockResponse(t, http.StatusOK, &github.Issue{ - Number: github.Ptr(123), - Title: github.Ptr("Updated Issue Title"), - HTMLURL: github.Ptr("https://github.com/owner/repo/issues/123"), - State: github.Ptr("open"), - Type: &github.IssueType{Name: github.Ptr("Feature")}, + http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"message": "Not Found"}`)) }), ), ), + mockedGQLClient: githubv4mock.NewMockedHTTPClient(), + requestArgs: map[string]interface{}{ + "owner": "owner", + "repo": "repo", + "issue_number": float64(999), + "title": "Updated Title", + }, + expectError: true, + expectedErrMsg: "failed to update issue", + }, + { + name: "close issue as duplicate", + mockedRESTClient: mock.NewMockedHTTPClient( + mock.WithRequestMatch( + mock.PatchReposIssuesByOwnerByRepoByIssueNumber, + mockBaseIssue, + ), + ), + mockedGQLClient: githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + struct { + Repository struct { + Issue struct { + ID githubv4.ID + } `graphql:"issue(number: $issueNumber)"` + DuplicateIssue struct { + ID githubv4.ID + } `graphql:"duplicateIssue: issue(number: $duplicateOf)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + }{}, + map[string]any{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + "issueNumber": githubv4.Int(123), + "duplicateOf": githubv4.Int(456), + }, + duplicateIssueIDQueryResponse, + ), + githubv4mock.NewMutationMatcher( + struct { + CloseIssue struct { + Issue struct { + ID githubv4.ID + Number githubv4.Int + URL githubv4.String + State githubv4.String + } + } `graphql:"closeIssue(input: $input)"` + }{}, + CloseIssueInput{ + IssueID: "I_kwDOA0xdyM50BPaO", + StateReason: &duplicateStateReason, + DuplicateIssueID: githubv4.NewID("I_kwDOA0xdyM50BPbP"), + }, + nil, + closeSuccessResponse, + ), + ), requestArgs: map[string]interface{}{ "owner": "owner", "repo": "repo", "issue_number": float64(123), - "title": "Updated Issue Title", - "type": "Feature", + "state": "closed", + "state_reason": "duplicate", + "duplicate_of": float64(456), }, - expectError: false, - expectedIssue: &github.Issue{ - Number: github.Ptr(123), - Title: github.Ptr("Updated Issue Title"), - HTMLURL: github.Ptr("https://github.com/owner/repo/issues/123"), - State: github.Ptr("open"), - Type: &github.IssueType{Name: github.Ptr("Feature")}, + expectError: false, + expectedIssue: mockUpdatedIssue, + }, + { + name: "reopen issue", + mockedRESTClient: mock.NewMockedHTTPClient( + mock.WithRequestMatch( + mock.PatchReposIssuesByOwnerByRepoByIssueNumber, + mockBaseIssue, + ), + ), + mockedGQLClient: githubv4mock.NewMockedHTTPClient( + 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(123), + }, + issueIDQueryResponse, + ), + githubv4mock.NewMutationMatcher( + struct { + ReopenIssue struct { + Issue struct { + ID githubv4.ID + Number githubv4.Int + URL githubv4.String + State githubv4.String + } + } `graphql:"reopenIssue(input: $input)"` + }{}, + githubv4.ReopenIssueInput{ + IssueID: "I_kwDOA0xdyM50BPaO", + }, + nil, + reopenSuccessResponse, + ), + ), + requestArgs: map[string]interface{}{ + "owner": "owner", + "repo": "repo", + "issue_number": float64(123), + "state": "open", }, + expectError: false, + expectedIssue: mockReopenedIssue, }, { - name: "update issue fails with not found", - mockedClient: mock.NewMockedHTTPClient( - mock.WithRequestMatchHandler( + name: "main issue not found when trying to close it", + mockedRESTClient: mock.NewMockedHTTPClient( + mock.WithRequestMatch( mock.PatchReposIssuesByOwnerByRepoByIssueNumber, - http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusNotFound) - _, _ = w.Write([]byte(`{"message": "Issue not found"}`)) - }), + mockBaseIssue, + ), + ), + mockedGQLClient: githubv4mock.NewMockedHTTPClient( + 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(999), + }, + githubv4mock.ErrorResponse("Could not resolve to an Issue with the number of 999."), ), ), requestArgs: map[string]interface{}{ "owner": "owner", "repo": "repo", "issue_number": float64(999), - "title": "This issue doesn't exist", + "state": "closed", + "state_reason": "not_planned", }, expectError: true, - expectedErrMsg: "failed to update issue", + expectedErrMsg: "Failed to find issues", }, { - name: "update issue fails with validation error", - mockedClient: mock.NewMockedHTTPClient( + name: "duplicate issue not found when closing as duplicate", + mockedRESTClient: mock.NewMockedHTTPClient( + mock.WithRequestMatch( + mock.PatchReposIssuesByOwnerByRepoByIssueNumber, + mockBaseIssue, + ), + ), + mockedGQLClient: githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + struct { + Repository struct { + Issue struct { + ID githubv4.ID + } `graphql:"issue(number: $issueNumber)"` + DuplicateIssue struct { + ID githubv4.ID + } `graphql:"duplicateIssue: issue(number: $duplicateOf)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + }{}, + map[string]any{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + "issueNumber": githubv4.Int(123), + "duplicateOf": githubv4.Int(999), + }, + githubv4mock.ErrorResponse("Could not resolve to an Issue with the number of 999."), + ), + ), + requestArgs: map[string]interface{}{ + "owner": "owner", + "repo": "repo", + "issue_number": float64(123), + "state": "closed", + "state_reason": "duplicate", + "duplicate_of": float64(999), + }, + expectError: true, + expectedErrMsg: "Failed to find issues", + }, + { + name: "close as duplicate with combined non-state updates", + mockedRESTClient: mock.NewMockedHTTPClient( mock.WithRequestMatchHandler( mock.PatchReposIssuesByOwnerByRepoByIssueNumber, - http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusUnprocessableEntity) - _, _ = w.Write([]byte(`{"message": "Invalid state value"}`)) - }), + expectRequestBody(t, map[string]interface{}{ + "title": "Updated Title", + "body": "Updated Description", + "labels": []any{"bug", "priority"}, + "assignees": []any{"assignee1", "assignee2"}, + "milestone": float64(5), + "type": "Bug", + }).andThen( + mockResponse(t, http.StatusOK, &github.Issue{ + Number: github.Ptr(123), + Title: github.Ptr("Updated Title"), + Body: github.Ptr("Updated Description"), + Labels: []*github.Label{{Name: github.Ptr("bug")}, {Name: github.Ptr("priority")}}, + Assignees: []*github.User{{Login: github.Ptr("assignee1")}, {Login: github.Ptr("assignee2")}}, + Milestone: &github.Milestone{Number: github.Ptr(5)}, + Type: &github.IssueType{Name: github.Ptr("Bug")}, + State: github.Ptr("open"), // Still open after REST update + HTMLURL: github.Ptr("https://github.com/owner/repo/issues/123"), + }), + ), + ), + ), + mockedGQLClient: githubv4mock.NewMockedHTTPClient( + githubv4mock.NewQueryMatcher( + struct { + Repository struct { + Issue struct { + ID githubv4.ID + } `graphql:"issue(number: $issueNumber)"` + DuplicateIssue struct { + ID githubv4.ID + } `graphql:"duplicateIssue: issue(number: $duplicateOf)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + }{}, + map[string]any{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + "issueNumber": githubv4.Int(123), + "duplicateOf": githubv4.Int(456), + }, + duplicateIssueIDQueryResponse, + ), + githubv4mock.NewMutationMatcher( + struct { + CloseIssue struct { + Issue struct { + ID githubv4.ID + Number githubv4.Int + URL githubv4.String + State githubv4.String + } + } `graphql:"closeIssue(input: $input)"` + }{}, + CloseIssueInput{ + IssueID: "I_kwDOA0xdyM50BPaO", + StateReason: &duplicateStateReason, + DuplicateIssueID: githubv4.NewID("I_kwDOA0xdyM50BPbP"), + }, + nil, + closeSuccessResponse, ), ), requestArgs: map[string]interface{}{ "owner": "owner", "repo": "repo", "issue_number": float64(123), - "state": "invalid_state", + "title": "Updated Title", + "body": "Updated Description", + "labels": []any{"bug", "priority"}, + "assignees": []any{"assignee1", "assignee2"}, + "milestone": float64(5), + "type": "Bug", + "state": "closed", + "state_reason": "duplicate", + "duplicate_of": float64(456), + }, + expectError: false, + expectedIssue: mockUpdatedIssue, + }, + { + name: "duplicate_of without duplicate state_reason should fail", + mockedRESTClient: mock.NewMockedHTTPClient(), + mockedGQLClient: githubv4mock.NewMockedHTTPClient(), + requestArgs: map[string]interface{}{ + "owner": "owner", + "repo": "repo", + "issue_number": float64(123), + "state": "closed", + "state_reason": "completed", + "duplicate_of": float64(456), }, expectError: true, - expectedErrMsg: "failed to update issue", + expectedErrMsg: "duplicate_of can only be used when state_reason is 'duplicate'", }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - // Setup client with mock - client := github.NewClient(tc.mockedClient) - _, handler := UpdateIssue(stubGetClientFn(client), translations.NullTranslationHelper) + // Setup clients with mocks + restClient := github.NewClient(tc.mockedRESTClient) + gqlClient := githubv4.NewClient(tc.mockedGQLClient) + _, handler := UpdateIssue(stubGetClientFn(restClient), stubGetGQLClientFn(gqlClient), translations.NullTranslationHelper) // Create call request request := createMCPRequest(tc.requestArgs) @@ -1189,21 +1485,20 @@ func Test_UpdateIssue(t *testing.T) { result, err := handler(context.Background(), request) // Verify results - if tc.expectError { - if err != nil { - assert.Contains(t, err.Error(), tc.expectedErrMsg) - } else { - // For errors returned as part of the result, not as an error - require.NotNil(t, result) - textContent := getTextResult(t, result) - assert.Contains(t, textContent.Text, tc.expectedErrMsg) + if tc.expectError || tc.expectedErrMsg != "" { + require.NoError(t, err) + require.True(t, result.IsError) + errorContent := getErrorResult(t, result) + if tc.expectedErrMsg != "" { + assert.Contains(t, errorContent.Text, tc.expectedErrMsg) } return } require.NoError(t, err) + require.False(t, result.IsError) - // Parse the result and get the text content if no error + // Parse the result and get the text content textContent := getTextResult(t, result) // Unmarshal and verify the minimal result diff --git a/pkg/github/tools.go b/pkg/github/tools.go index 10a3f3eca..baeeb4443 100644 --- a/pkg/github/tools.go +++ b/pkg/github/tools.go @@ -62,7 +62,7 @@ func DefaultToolsetGroup(readOnly bool, getClient GetClientFn, getGQLClient GetG AddWriteTools( toolsets.NewServerTool(CreateIssue(getClient, t)), toolsets.NewServerTool(AddIssueComment(getClient, t)), - toolsets.NewServerTool(UpdateIssue(getClient, t)), + toolsets.NewServerTool(UpdateIssue(getClient, getGQLClient, t)), toolsets.NewServerTool(AssignCopilotToIssue(getGQLClient, t)), toolsets.NewServerTool(AddSubIssue(getClient, t)), toolsets.NewServerTool(RemoveSubIssue(getClient, t)),