From 23a3257059709566045e643671377197e94ca2e5 Mon Sep 17 00:00:00 2001 From: kerobbi Date: Tue, 9 Sep 2025 09:57:29 +0100 Subject: [PATCH 1/7] add state_reason param --- README.md | 1 + pkg/github/__toolsnaps__/update_issue.snap | 10 ++++++ pkg/github/issues.go | 12 ++++++++ pkg/github/issues_test.go | 36 ++++++++++++++++++++++ 4 files changed, 59 insertions(+) diff --git a/README.md b/README.md index d1ce061da..1655318c5 100644 --- a/README.md +++ b/README.md @@ -597,6 +597,7 @@ The following sets of tools are available (all are on by default): - `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..031ceb9fe 100644 --- a/pkg/github/__toolsnaps__/update_issue.snap +++ b/pkg/github/__toolsnaps__/update_issue.snap @@ -48,6 +48,16 @@ ], "type": "string" }, + "state_reason": { + "description": "Reason for the state change, ignored unless state is changed.", + "enum": [ + "completed", + "not_planned", + "duplicate", + "reopened" + ], + "type": "string" + }, "title": { "description": "New title", "type": "string" diff --git a/pkg/github/issues.go b/pkg/github/issues.go index 01ce7b42e..a551e8975 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -1128,6 +1128,10 @@ func UpdateIssue(getClient GetClientFn, t translations.TranslationHelperFunc) (t 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", "reopened"), + ), mcp.WithArray("labels", mcp.Description("New labels"), mcp.Items( @@ -1193,6 +1197,14 @@ func UpdateIssue(getClient GetClientFn, t translations.TranslationHelperFunc) (t issueRequest.State = github.Ptr(state) } + stateReason, err := OptionalParam[string](request, "state_reason") + if err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + if stateReason != "" { + issueRequest.StateReason = github.Ptr(stateReason) + } + // Get labels labels, err := OptionalStringArrayParam(request, "labels") if err != nil { diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index 5a0d409a6..29b97583c 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -1044,6 +1044,7 @@ func Test_UpdateIssue(t *testing.T) { 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, "state_reason") assert.Contains(t, tool.InputSchema.Properties, "labels") assert.Contains(t, tool.InputSchema.Properties, "assignees") assert.Contains(t, tool.InputSchema.Properties, "milestone") @@ -1174,6 +1175,41 @@ func Test_UpdateIssue(t *testing.T) { expectError: true, expectedErrMsg: "failed to update issue", }, + { + name: "close issue as not planned", + mockedClient: mock.NewMockedHTTPClient( + mock.WithRequestMatchHandler( + mock.PatchReposIssuesByOwnerByRepoByIssueNumber, + expectRequestBody(t, map[string]any{ + "state": "closed", + "state_reason": "not_planned", + }).andThen( + mockResponse(t, http.StatusOK, &github.Issue{ + Number: github.Ptr(123), + Title: github.Ptr("Test Issue"), + HTMLURL: github.Ptr("https://github.com/owner/repo/issues/123"), + State: github.Ptr("closed"), + StateReason: github.Ptr("not_planned"), + }), + ), + ), + ), + requestArgs: map[string]interface{}{ + "owner": "owner", + "repo": "repo", + "issue_number": float64(123), + "state": "closed", + "state_reason": "not_planned", + }, + expectError: false, + expectedIssue: &github.Issue{ + Number: github.Ptr(123), + Title: github.Ptr("Test Issue"), + HTMLURL: github.Ptr("https://github.com/owner/repo/issues/123"), + State: github.Ptr("closed"), + StateReason: github.Ptr("not_planned"), + }, + }, } for _, tc := range tests { From e58c822b38325c00413b4f3f2c76a3f2778da168 Mon Sep 17 00:00:00 2001 From: kerobbi Date: Tue, 9 Sep 2025 18:28:45 +0100 Subject: [PATCH 2/7] add close as duplicate functionality --- README.md | 3 +- pkg/github/__toolsnaps__/update_issue.snap | 6 +- pkg/github/issues.go | 166 ++++++++++++++++++--- pkg/github/tools.go | 2 +- 4 files changed, 154 insertions(+), 23 deletions(-) diff --git a/README.md b/README.md index 1655318c5..43bf8be15 100644 --- a/README.md +++ b/README.md @@ -591,13 +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) + - `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 031ceb9fe..381d32979 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" @@ -49,7 +53,7 @@ "type": "string" }, "state_reason": { - "description": "Reason for the state change, ignored unless state is changed.", + "description": "Reason for the state change. Ignored unless state is changed.", "enum": [ "completed", "not_planned", diff --git a/pkg/github/issues.go b/pkg/github/issues.go index a551e8975..287a85d73 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -18,6 +18,25 @@ import ( "github.com/shurcooL/githubv4" ) +// Custom GraphQL structs for closing issues as duplicate +// These extend the githubv4 library which is missing this functionality + +// Local definition matching GitHub's GraphQL schema for closing issues +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"` +} + +type IssueClosedStateReason string + +const ( + IssueClosedStateReasonCompleted IssueClosedStateReason = "COMPLETED" + IssueClosedStateReasonDuplicate IssueClosedStateReason = "DUPLICATE" + IssueClosedStateReasonNotPlanned IssueClosedStateReason = "NOT_PLANNED" +) + // IssueFragment represents a fragment of an issue node in the GraphQL API. type IssueFragment struct { Number githubv4.Int @@ -1099,7 +1118,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{ @@ -1129,9 +1148,12 @@ func UpdateIssue(getClient GetClientFn, t translations.TranslationHelperFunc) (t mcp.Enum("open", "closed"), ), mcp.WithString("state_reason", - mcp.Description("Reason for the state change, ignored unless state is changed."), + mcp.Description("Reason for the state change. Ignored unless state is changed."), mcp.Enum("completed", "not_planned", "duplicate", "reopened"), ), + mcp.WithNumber("duplicate_of", + mcp.Description("Issue number that this issue is a duplicate of. Only used when state_reason is 'duplicate'."), + ), mcp.WithArray("labels", mcp.Description("New labels"), mcp.Items( @@ -1171,6 +1193,8 @@ func UpdateIssue(getClient GetClientFn, t translations.TranslationHelperFunc) (t // Create the issue request with only provided fields issueRequest := &github.IssueRequest{} + restUpdateNeeded := false + gqlUpdateNeeded := false // Set optional parameters if provided title, err := OptionalParam[string](request, "title") @@ -1179,6 +1203,7 @@ func UpdateIssue(getClient GetClientFn, t translations.TranslationHelperFunc) (t } if title != "" { issueRequest.Title = github.Ptr(title) + restUpdateNeeded = true } body, err := OptionalParam[string](request, "body") @@ -1187,22 +1212,37 @@ func UpdateIssue(getClient GetClientFn, t translations.TranslationHelperFunc) (t } if body != "" { issueRequest.Body = github.Ptr(body) + restUpdateNeeded = true } + // Handle state and state_reason parameters state, err := OptionalParam[string](request, "state") if err != nil { return mcp.NewToolResultError(err.Error()), nil } - if state != "" { - issueRequest.State = github.Ptr(state) - } stateReason, err := OptionalParam[string](request, "state_reason") if err != nil { return mcp.NewToolResultError(err.Error()), nil } - if stateReason != "" { - issueRequest.StateReason = github.Ptr(stateReason) + + // If closing as duplicate, use GraphQL API, otherwise use REST API for state changes + if stateReason == "duplicate" { + gqlUpdateNeeded = true + } else { + if state != "" { + issueRequest.State = github.Ptr(state) + restUpdateNeeded = true + } + if stateReason != "" { + issueRequest.StateReason = github.Ptr(stateReason) + restUpdateNeeded = true + } + } + + duplicateOf, err := OptionalIntParam(request, "duplicate_of") + if err != nil { + return mcp.NewToolResultError(err.Error()), nil } // Get labels @@ -1212,6 +1252,7 @@ func UpdateIssue(getClient GetClientFn, t translations.TranslationHelperFunc) (t } if len(labels) > 0 { issueRequest.Labels = &labels + restUpdateNeeded = true } // Get assignees @@ -1221,6 +1262,7 @@ func UpdateIssue(getClient GetClientFn, t translations.TranslationHelperFunc) (t } if len(assignees) > 0 { issueRequest.Assignees = &assignees + restUpdateNeeded = true } milestone, err := OptionalIntParam(request, "milestone") @@ -1230,6 +1272,7 @@ func UpdateIssue(getClient GetClientFn, t translations.TranslationHelperFunc) (t if milestone != 0 { milestoneNum := milestone issueRequest.Milestone = &milestoneNum + restUpdateNeeded = true } // Get issue type @@ -1239,34 +1282,117 @@ func UpdateIssue(getClient GetClientFn, t translations.TranslationHelperFunc) (t } if issueType != "" { issueRequest.Type = github.Ptr(issueType) + restUpdateNeeded = true } - client, err := getClient(ctx) - if err != nil { - return nil, fmt.Errorf("failed to get GitHub client: %w", err) + if !restUpdateNeeded && !gqlUpdateNeeded { + return mcp.NewToolResultError("No update parameters provided."), nil } - updatedIssue, resp, err := client.Issues.Edit(ctx, owner, repo, issueNumber, issueRequest) - if err != nil { - return nil, fmt.Errorf("failed to update issue: %w", err) + + // Handle REST API updates (title, body, state, labels, assignees, milestone, type, state_reason except "duplicate") + if restUpdateNeeded { + client, err := getClient(ctx) + if err != nil { + return nil, fmt.Errorf("failed to get GitHub client: %w", err) + } + + _, resp, err := client.Issues.Edit(ctx, owner, repo, issueNumber, issueRequest) + if err != nil { + return ghErrors.NewGitHubAPIErrorResponse(ctx, + "failed to update issue", + resp, + err, + ), nil + } + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != http.StatusOK { + body, err := io.ReadAll(resp.Body) + if err != nil { + return nil, fmt.Errorf("failed to read response body: %w", err) + } + return mcp.NewToolResultError(fmt.Sprintf("failed to update issue: %s", string(body))), nil + } } - defer func() { _ = resp.Body.Close() }() - if resp.StatusCode != http.StatusOK { - body, err := io.ReadAll(resp.Body) + // Handle GraphQL API updates (state_reason = "duplicate") + if gqlUpdateNeeded { + if duplicateOf == 0 { + return mcp.NewToolResultError("duplicate_of must be provided when state_reason is 'duplicate'"), nil + } + + gqlClient, err := getGQLClient(ctx) if err != nil { - return nil, fmt.Errorf("failed to read response body: %w", err) + return nil, fmt.Errorf("failed to get GraphQL client: %w", err) + } + + var issueQuery struct { + Repository struct { + Issue struct { + ID githubv4.ID + } `graphql:"issue(number: $issueNumber)"` + DuplicateIssue struct { + ID githubv4.ID + } `graphql:"duplicateIssue: issue(number: $duplicateNumber)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + } + + err = gqlClient.Query(ctx, &issueQuery, map[string]interface{}{ + "owner": githubv4.String(owner), + "repo": githubv4.String(repo), + "issueNumber": githubv4.Int(issueNumber), // #nosec G115 - issue numbers are always small positive integers + "duplicateNumber": githubv4.Int(duplicateOf), // #nosec G115 - issue numbers are always small positive integers + }) + if err != nil { + return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "Failed to find issues", err), nil + } + + var mutation struct { + CloseIssue struct { + Issue struct { + ID githubv4.ID + Number githubv4.Int + URL githubv4.String + State githubv4.String + } + } `graphql:"closeIssue(input: $input)"` + } + + duplicateStateReason := IssueClosedStateReasonDuplicate + err = gqlClient.Mutate(ctx, &mutation, CloseIssueInput{ + IssueID: issueQuery.Repository.Issue.ID, + StateReason: &duplicateStateReason, + DuplicateIssueID: &issueQuery.Repository.DuplicateIssue.ID, + }, nil) + if err != nil { + return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "Failed to close issue as duplicate", err), nil } - return mcp.NewToolResultError(fmt.Sprintf("failed to update issue: %s", string(body))), nil } + // Get the final state of the issue to return + client, err := getClient(ctx) + if err != nil { + return nil, err + } + + finalIssue, resp, err := client.Issues.Get(ctx, owner, repo, issueNumber) + if err != nil { + return ghErrors.NewGitHubAPIErrorResponse(ctx, "Failed to get issue", resp, err), nil + } + defer func() { + if resp != nil && resp.Body != nil { + _ = resp.Body.Close() + } + }() + // Return minimal response with just essential information minimalResponse := MinimalResponse{ - URL: updatedIssue.GetHTMLURL(), + URL: finalIssue.GetHTMLURL(), } r, err := json.Marshal(minimalResponse) if err != nil { - return nil, fmt.Errorf("failed to marshal response: %w", err) + return mcp.NewToolResultError(fmt.Sprintf("Failed to marshal response: %v", err)), nil } return mcp.NewToolResultText(string(r)), nil diff --git a/pkg/github/tools.go b/pkg/github/tools.go index 728d78097..c76690ce9 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)), From fc200581647e4304a74b89130392fb5a1cf30253 Mon Sep 17 00:00:00 2001 From: kerobbi Date: Wed, 10 Sep 2025 10:30:49 +0100 Subject: [PATCH 3/7] refactor and improve tests --- pkg/github/issues.go | 8 +- pkg/github/issues_test.go | 367 ++++++++++++++++++++++++++++++-------- 2 files changed, 294 insertions(+), 81 deletions(-) diff --git a/pkg/github/issues.go b/pkg/github/issues.go index 287a85d73..21d32c666 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -18,10 +18,8 @@ import ( "github.com/shurcooL/githubv4" ) -// Custom GraphQL structs for closing issues as duplicate -// These extend the githubv4 library which is missing this functionality - -// Local definition matching GitHub's GraphQL schema for closing issues +// 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"` @@ -29,6 +27,8 @@ type CloseIssueInput struct { 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 ( diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index 29b97583c..9da6c6c41 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) @@ -1045,23 +1046,40 @@ func Test_UpdateIssue(t *testing.T) { assert.Contains(t, tool.InputSchema.Properties, "body") assert.Contains(t, tool.InputSchema.Properties, "state") assert.Contains(t, tool.InputSchema.Properties, "state_reason") + assert.Contains(t, tool.InputSchema.Properties, "duplicate_of") 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.ElementsMatch(t, tool.InputSchema.Required, []string{"owner", "repo", "issue_number"}) - // Setup mock issue for success case - mockIssue := &github.Issue{ - Number: github.Ptr(123), - Title: github.Ptr("Updated Issue Title"), - Body: github.Ptr("Updated issue description"), - State: github.Ptr("closed"), - 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")}, + // Setup mock issues for various test scenarios + mockUpdatedIssue := &github.Issue{ + Number: github.Ptr(123), + Title: github.Ptr("Updated Issue Title"), + Body: github.Ptr("Updated issue description"), + State: github.Ptr("closed"), + StateReason: github.Ptr("completed"), + 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")}, + } + + mockClosedNotPlannedIssue := &github.Issue{ + Number: github.Ptr(123), + Title: github.Ptr("Issue Title"), + State: github.Ptr("closed"), + StateReason: github.Ptr("not_planned"), + HTMLURL: github.Ptr("https://github.com/owner/repo/issues/123"), + } + + mockClosedIssue := &github.Issue{ + Number: github.Ptr(123), + Title: github.Ptr("Issue Title"), + State: github.Ptr("closed"), + HTMLURL: github.Ptr("https://github.com/owner/repo/issues/123"), } tests := []struct { @@ -1077,18 +1095,23 @@ func Test_UpdateIssue(t *testing.T) { mockedClient: 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 Issue Title", + "body": "Updated issue description", + "state": "closed", + "state_reason": "completed", + "labels": []any{"bug", "priority"}, + "assignees": []any{"assignee1", "assignee2"}, + "milestone": float64(5), + "type": "Bug", }).andThen( - mockResponse(t, http.StatusOK, mockIssue), + mockResponse(t, http.StatusOK, mockUpdatedIssue), ), ), + mock.WithRequestMatch( + mock.GetReposIssuesByOwnerByRepoByIssueNumber, + mockUpdatedIssue, + ), ), requestArgs: map[string]interface{}{ "owner": "owner", @@ -1097,43 +1120,14 @@ func Test_UpdateIssue(t *testing.T) { "title": "Updated Issue Title", "body": "Updated issue description", "state": "closed", + "state_reason": "completed", "labels": []any{"bug", "priority"}, "assignees": []any{"assignee1", "assignee2"}, "milestone": float64(5), "type": "Bug", }, expectError: false, - expectedIssue: mockIssue, - }, - { - name: "update issue with minimal fields", - mockedClient: 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")}, - }), - ), - ), - requestArgs: map[string]interface{}{ - "owner": "owner", - "repo": "repo", - "issue_number": float64(123), - "title": "Updated Issue Title", - "type": "Feature", - }, - 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")}, - }, + expectedIssue: mockUpdatedIssue, }, { name: "update issue fails with not found", @@ -1180,19 +1174,17 @@ func Test_UpdateIssue(t *testing.T) { mockedClient: mock.NewMockedHTTPClient( mock.WithRequestMatchHandler( mock.PatchReposIssuesByOwnerByRepoByIssueNumber, - expectRequestBody(t, map[string]any{ + expectRequestBody(t, map[string]interface{}{ "state": "closed", "state_reason": "not_planned", }).andThen( - mockResponse(t, http.StatusOK, &github.Issue{ - Number: github.Ptr(123), - Title: github.Ptr("Test Issue"), - HTMLURL: github.Ptr("https://github.com/owner/repo/issues/123"), - State: github.Ptr("closed"), - StateReason: github.Ptr("not_planned"), - }), + mockResponse(t, http.StatusOK, mockClosedNotPlannedIssue), ), ), + mock.WithRequestMatch( + mock.GetReposIssuesByOwnerByRepoByIssueNumber, + mockClosedNotPlannedIssue, + ), ), requestArgs: map[string]interface{}{ "owner": "owner", @@ -1201,14 +1193,46 @@ func Test_UpdateIssue(t *testing.T) { "state": "closed", "state_reason": "not_planned", }, - expectError: false, - expectedIssue: &github.Issue{ - Number: github.Ptr(123), - Title: github.Ptr("Test Issue"), - HTMLURL: github.Ptr("https://github.com/owner/repo/issues/123"), - State: github.Ptr("closed"), - StateReason: github.Ptr("not_planned"), + expectError: false, + expectedIssue: mockClosedNotPlannedIssue, + }, + { + name: "close issue without state_reason", + mockedClient: mock.NewMockedHTTPClient( + mock.WithRequestMatchHandler( + mock.PatchReposIssuesByOwnerByRepoByIssueNumber, + expectRequestBody(t, map[string]interface{}{ + "state": "closed", + }).andThen( + mockResponse(t, http.StatusOK, mockClosedIssue), + ), + ), + mock.WithRequestMatch( + mock.GetReposIssuesByOwnerByRepoByIssueNumber, + mockClosedIssue, + ), + ), + requestArgs: map[string]interface{}{ + "owner": "owner", + "repo": "repo", + "issue_number": float64(123), + "state": "closed", + // No state_reason provided }, + expectError: false, + expectedIssue: mockClosedIssue, + }, + { + name: "no update parameters provided", + mockedClient: mock.NewMockedHTTPClient(), + requestArgs: map[string]interface{}{ + "owner": "owner", + "repo": "repo", + "issue_number": float64(123), + // No update fields + }, + expectError: false, // Error is returned in the result, not as Go error + expectedErrMsg: "No update parameters provided.", }, } @@ -1216,7 +1240,7 @@ func Test_UpdateIssue(t *testing.T) { t.Run(tc.name, func(t *testing.T) { // Setup client with mock client := github.NewClient(tc.mockedClient) - _, handler := UpdateIssue(stubGetClientFn(client), translations.NullTranslationHelper) + _, handler := UpdateIssue(stubGetClientFn(client), stubGetGQLClientFn(githubv4.NewClient(nil)), translations.NullTranslationHelper) // Create call request request := createMCPRequest(tc.requestArgs) @@ -1225,21 +1249,210 @@ 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 + var updateResp MinimalResponse + err = json.Unmarshal([]byte(textContent.Text), &updateResp) + require.NoError(t, err) + + assert.Equal(t, tc.expectedIssue.GetHTMLURL(), updateResp.URL) + }) + } +} + +func Test_UpdateIssue_CloseAsDuplicate(t *testing.T) { + // Setup mock issue for success case + mockClosedDuplicateIssue := &github.Issue{ + Number: github.Ptr(123), + Title: github.Ptr("Issue Title"), + Body: github.Ptr("Issue description"), + State: github.Ptr("closed"), + StateReason: github.Ptr("duplicate"), + HTMLURL: github.Ptr("https://github.com/owner/repo/issues/123"), + } + + // GraphQL response data for successful queries + successfulQueryResponse := githubv4mock.DataResponse(map[string]any{ + "repository": map[string]any{ + "issue": map[string]any{ + "id": "I_kwDOA0xdyM50BPaO", + }, + "duplicateIssue": map[string]any{ + "id": "I_kwDOA0xdyM50BPbP", + }, + }, + }) + + successfulMutationResponse := 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", + }, + }, + }) + + duplicateStateReason := IssueClosedStateReasonDuplicate + + tests := []struct { + name string + mockedClient *http.Client + requestArgs map[string]interface{} + expectError bool + expectedIssue *github.Issue + expectedErrMsg string + }{ + { + name: "successful close as duplicate", + mockedClient: 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: $duplicateNumber)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + }{}, + map[string]any{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + "issueNumber": githubv4.Int(123), + "duplicateNumber": githubv4.Int(456), + }, + successfulQueryResponse, + ), + 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, + successfulMutationResponse, + ), + ), + requestArgs: map[string]interface{}{ + "owner": "owner", + "repo": "repo", + "issue_number": float64(123), + "state": "closed", + "state_reason": "duplicate", + "duplicate_of": float64(456), + }, + expectError: false, + expectedIssue: mockClosedDuplicateIssue, + }, + { + name: "fail to close as duplicate - duplicate_of missing", + mockedClient: mock.NewMockedHTTPClient(), // No API call expected + requestArgs: map[string]interface{}{ + "owner": "owner", + "repo": "repo", + "issue_number": float64(123), + "state_reason": "duplicate", + // Missing duplicate_of + }, + expectError: true, + expectedErrMsg: "duplicate_of must be provided when state_reason is 'duplicate'", + }, + { + name: "fail to close as duplicate - duplicate issue not found", + mockedClient: 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: $duplicateNumber)"` + } `graphql:"repository(owner: $owner, name: $repo)"` + }{}, + map[string]any{ + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + "issueNumber": githubv4.Int(123), + "duplicateNumber": 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_reason": "duplicate", + "duplicate_of": float64(999), + }, + expectError: true, + expectedErrMsg: "Failed to find issues", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + // Mock both GraphQL and the final REST API GET call + restClient := github.NewClient(mock.NewMockedHTTPClient( + mock.WithRequestMatch( + mock.GetReposIssuesByOwnerByRepoByIssueNumber, + mockClosedDuplicateIssue, + ), + )) + mockGQLClient := githubv4.NewClient(tc.mockedClient) + _, handler := UpdateIssue(stubGetClientFn(restClient), stubGetGQLClientFn(mockGQLClient), translations.NullTranslationHelper) + + // Create call request + request := createMCPRequest(tc.requestArgs) + + // Call handler + result, err := handler(context.Background(), request) + + // Verify results + 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 textContent := getTextResult(t, result) // Unmarshal and verify the minimal result From 0702d8eea057bbb2044a8dcbc9a072fb703f1f5f Mon Sep 17 00:00:00 2001 From: kerobbi Date: Wed, 10 Sep 2025 11:39:29 +0100 Subject: [PATCH 4/7] fix state reason validation logic and update tests --- pkg/github/issues.go | 24 ++++++++++++++++-------- pkg/github/issues_test.go | 15 +++++++++++++++ 2 files changed, 31 insertions(+), 8 deletions(-) diff --git a/pkg/github/issues.go b/pkg/github/issues.go index 21d32c666..ce4d0c395 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -1226,18 +1226,26 @@ func UpdateIssue(getClient GetClientFn, getGQLClient GetGQLClientFn, t translati return mcp.NewToolResultError(err.Error()), nil } - // If closing as duplicate, use GraphQL API, otherwise use REST API for state changes - if stateReason == "duplicate" { + // Validate state_reason usage + if stateReason != "" && state == "" { + return mcp.NewToolResultError("state_reason can only be used when state is also provided"), nil + } + if state == "open" && stateReason != "" && stateReason != "reopened" { + return mcp.NewToolResultError("when state is 'open', state_reason can only be 'reopened'"), nil + } + if state == "closed" && stateReason != "" && stateReason != "completed" && stateReason != "not_planned" && stateReason != "duplicate" { + return mcp.NewToolResultError("when state is 'closed', state_reason can only be 'completed', 'not_planned', or 'duplicate'"), nil + } + + // Use GraphQL for duplicate closure, REST for everything else + if state == "closed" && stateReason == "duplicate" { gqlUpdateNeeded = true - } else { - if state != "" { - issueRequest.State = github.Ptr(state) - restUpdateNeeded = true - } + } else if state != "" { + issueRequest.State = github.Ptr(state) if stateReason != "" { issueRequest.StateReason = github.Ptr(stateReason) - restUpdateNeeded = true } + restUpdateNeeded = true } duplicateOf, err := OptionalIntParam(request, "duplicate_of") diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index 9da6c6c41..1c63b223e 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -1234,6 +1234,19 @@ func Test_UpdateIssue(t *testing.T) { expectError: false, // Error is returned in the result, not as Go error expectedErrMsg: "No update parameters provided.", }, + { + name: "state_reason without state should fail", + mockedClient: mock.NewMockedHTTPClient(), // No API calls expected + requestArgs: map[string]interface{}{ + "owner": "owner", + "repo": "repo", + "issue_number": float64(123), + // No state provided + "state_reason": "not_planned", + }, + expectError: true, + expectedErrMsg: "state_reason can only be used when state is also provided", + }, } for _, tc := range tests { @@ -1379,6 +1392,7 @@ func Test_UpdateIssue_CloseAsDuplicate(t *testing.T) { "owner": "owner", "repo": "repo", "issue_number": float64(123), + "state": "closed", "state_reason": "duplicate", // Missing duplicate_of }, @@ -1412,6 +1426,7 @@ func Test_UpdateIssue_CloseAsDuplicate(t *testing.T) { "owner": "owner", "repo": "repo", "issue_number": float64(123), + "state": "closed", "state_reason": "duplicate", "duplicate_of": float64(999), }, From 2328eb8596562854f26d0391717ab6fe33adaf21 Mon Sep 17 00:00:00 2001 From: kerobbi Date: Thu, 11 Sep 2025 15:12:10 +0100 Subject: [PATCH 5/7] move state and state reason handling to gql --- pkg/github/__toolsnaps__/update_issue.snap | 3 +- pkg/github/issues.go | 262 +++++++---- pkg/github/issues_test.go | 517 +++++++++++---------- 3 files changed, 443 insertions(+), 339 deletions(-) diff --git a/pkg/github/__toolsnaps__/update_issue.snap b/pkg/github/__toolsnaps__/update_issue.snap index 381d32979..5c3f0e638 100644 --- a/pkg/github/__toolsnaps__/update_issue.snap +++ b/pkg/github/__toolsnaps__/update_issue.snap @@ -57,8 +57,7 @@ "enum": [ "completed", "not_planned", - "duplicate", - "reopened" + "duplicate" ], "type": "string" }, diff --git a/pkg/github/issues.go b/pkg/github/issues.go index 5bbc93373..31091944d 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -37,6 +37,71 @@ const ( 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": + reason := IssueClosedStateReasonNotPlanned + return &reason + case "duplicate": + reason := IssueClosedStateReasonDuplicate + return &reason + default: // Default to "completed" for empty or "completed" values + reason := IssueClosedStateReasonCompleted + return &reason + } +} + // IssueFragment represents a fragment of an issue node in the GraphQL API. type IssueFragment struct { Number githubv4.Int @@ -1144,17 +1209,6 @@ func UpdateIssue(getClient GetClientFn, getGQLClient GetGQLClientFn, t translati mcp.WithString("body", mcp.Description("New description"), ), - 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", "reopened"), - ), - mcp.WithNumber("duplicate_of", - mcp.Description("Issue number that this issue is a duplicate of. Only used when state_reason is 'duplicate'."), - ), mcp.WithArray("labels", mcp.Description("New labels"), mcp.Items( @@ -1177,6 +1231,17 @@ func UpdateIssue(getClient GetClientFn, getGQLClient GetGQLClientFn, t translati 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") @@ -1194,8 +1259,7 @@ func UpdateIssue(getClient GetClientFn, getGQLClient GetGQLClientFn, t translati // Create the issue request with only provided fields issueRequest := &github.IssueRequest{} - restUpdateNeeded := false - gqlUpdateNeeded := false + hasNonStateUpdates := false // Set optional parameters if provided title, err := OptionalParam[string](request, "title") @@ -1204,7 +1268,7 @@ func UpdateIssue(getClient GetClientFn, getGQLClient GetGQLClientFn, t translati } if title != "" { issueRequest.Title = github.Ptr(title) - restUpdateNeeded = true + hasNonStateUpdates = true } body, err := OptionalParam[string](request, "body") @@ -1213,45 +1277,7 @@ func UpdateIssue(getClient GetClientFn, getGQLClient GetGQLClientFn, t translati } if body != "" { issueRequest.Body = github.Ptr(body) - restUpdateNeeded = true - } - - // Handle state and state_reason 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 - } - - // Validate state_reason usage - if stateReason != "" && state == "" { - return mcp.NewToolResultError("state_reason can only be used when state is also provided"), nil - } - if state == "open" && stateReason != "" && stateReason != "reopened" { - return mcp.NewToolResultError("when state is 'open', state_reason can only be 'reopened'"), nil - } - if state == "closed" && stateReason != "" && stateReason != "completed" && stateReason != "not_planned" && stateReason != "duplicate" { - return mcp.NewToolResultError("when state is 'closed', state_reason can only be 'completed', 'not_planned', or 'duplicate'"), nil - } - - // Use GraphQL for duplicate closure, REST for everything else - if state == "closed" && stateReason == "duplicate" { - gqlUpdateNeeded = true - } else if state != "" { - issueRequest.State = github.Ptr(state) - if stateReason != "" { - issueRequest.StateReason = github.Ptr(stateReason) - } - restUpdateNeeded = true - } - - duplicateOf, err := OptionalIntParam(request, "duplicate_of") - if err != nil { - return mcp.NewToolResultError(err.Error()), nil + hasNonStateUpdates = true } // Get labels @@ -1261,7 +1287,7 @@ func UpdateIssue(getClient GetClientFn, getGQLClient GetGQLClientFn, t translati } if len(labels) > 0 { issueRequest.Labels = &labels - restUpdateNeeded = true + hasNonStateUpdates = true } // Get assignees @@ -1271,7 +1297,7 @@ func UpdateIssue(getClient GetClientFn, getGQLClient GetGQLClientFn, t translati } if len(assignees) > 0 { issueRequest.Assignees = &assignees - restUpdateNeeded = true + hasNonStateUpdates = true } milestone, err := OptionalIntParam(request, "milestone") @@ -1281,7 +1307,7 @@ func UpdateIssue(getClient GetClientFn, getGQLClient GetGQLClientFn, t translati if milestone != 0 { milestoneNum := milestone issueRequest.Milestone = &milestoneNum - restUpdateNeeded = true + hasNonStateUpdates = true } // Get issue type @@ -1291,15 +1317,38 @@ func UpdateIssue(getClient GetClientFn, getGQLClient GetGQLClientFn, t translati } if issueType != "" { issueRequest.Type = github.Ptr(issueType) - restUpdateNeeded = true + hasNonStateUpdates = true + } + + // Handle state, state_reason and duplicateOf parameters + state, err := OptionalParam[string](request, "state") + if err != nil { + return mcp.NewToolResultError(err.Error()), nil } - if !restUpdateNeeded && !gqlUpdateNeeded { + stateReason, err := OptionalParam[string](request, "state_reason") + if err != nil { + return mcp.NewToolResultError(err.Error()), nil + } + if stateReason != "" && state == "" { + return mcp.NewToolResultError("state_reason can only be used when state is also provided"), 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 + } + + // Determine if there were any updates at all + if !hasNonStateUpdates && state == "" { return mcp.NewToolResultError("No update parameters provided."), nil } - // Handle REST API updates (title, body, state, labels, assignees, milestone, type, state_reason except "duplicate") - if restUpdateNeeded { + // Use REST API for non-state updates + if hasNonStateUpdates { client, err := getClient(ctx) if err != nil { return nil, fmt.Errorf("failed to get GitHub client: %w", err) @@ -1324,57 +1373,72 @@ func UpdateIssue(getClient GetClientFn, getGQLClient GetGQLClientFn, t translati } } - // Handle GraphQL API updates (state_reason = "duplicate") - if gqlUpdateNeeded { - if duplicateOf == 0 { - return mcp.NewToolResultError("duplicate_of must be provided when state_reason is 'duplicate'"), 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) } - var issueQuery struct { - Repository struct { - Issue struct { - ID githubv4.ID - } `graphql:"issue(number: $issueNumber)"` - DuplicateIssue struct { - ID githubv4.ID - } `graphql:"duplicateIssue: issue(number: $duplicateNumber)"` - } `graphql:"repository(owner: $owner, name: $repo)"` + // 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 } - err = gqlClient.Query(ctx, &issueQuery, map[string]interface{}{ - "owner": githubv4.String(owner), - "repo": githubv4.String(repo), - "issueNumber": githubv4.Int(issueNumber), // #nosec G115 - issue numbers are always small positive integers - "duplicateNumber": githubv4.Int(duplicateOf), // #nosec G115 - issue numbers are always small positive integers - }) + // 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 } - var mutation struct { - CloseIssue struct { - Issue struct { - ID githubv4.ID - Number githubv4.Int - URL githubv4.String - State githubv4.String - } - } `graphql:"closeIssue(input: $input)"` - } + // Do all logic + 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)"` + } - duplicateStateReason := IssueClosedStateReasonDuplicate - err = gqlClient.Mutate(ctx, &mutation, CloseIssueInput{ - IssueID: issueQuery.Repository.Issue.ID, - StateReason: &duplicateStateReason, - DuplicateIssueID: &issueQuery.Repository.DuplicateIssue.ID, - }, nil) - if err != nil { - return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "Failed to close issue as duplicate", err), nil + 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)"` + } + + closeInput := CloseIssueInput{ + IssueID: issueID, + StateReason: getCloseStateReason(stateReason), + } + + // 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 + } } } diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index 1c63b223e..d7ae2db9a 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -1044,22 +1044,22 @@ 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, "state_reason") - assert.Contains(t, tool.InputSchema.Properties, "duplicate_of") 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 issues for various test scenarios + // Mock issues for reuse across test cases mockUpdatedIssue := &github.Issue{ Number: github.Ptr(123), - Title: github.Ptr("Updated Issue Title"), - Body: github.Ptr("Updated issue description"), + Title: github.Ptr("Updated Title"), + Body: github.Ptr("Updated Description"), State: github.Ptr("closed"), - StateReason: github.Ptr("completed"), + 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")}}, @@ -1067,43 +1067,74 @@ func Test_UpdateIssue(t *testing.T) { Type: &github.IssueType{Name: github.Ptr("Bug")}, } - mockClosedNotPlannedIssue := &github.Issue{ - Number: github.Ptr(123), - Title: github.Ptr("Issue Title"), - State: github.Ptr("closed"), - StateReason: github.Ptr("not_planned"), - HTMLURL: github.Ptr("https://github.com/owner/repo/issues/123"), - } - - mockClosedIssue := &github.Issue{ + mockReopenedIssue := &github.Issue{ Number: github.Ptr(123), Title: github.Ptr("Issue Title"), - State: github.Ptr("closed"), + State: github.Ptr("open"), 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]interface{}{ - "title": "Updated Issue Title", - "body": "Updated issue description", - "state": "closed", - "state_reason": "completed", - "labels": []any{"bug", "priority"}, - "assignees": []any{"assignee1", "assignee2"}, - "milestone": float64(5), - "type": "Bug", + "title": "Updated Title", + "body": "Updated Description", }).andThen( mockResponse(t, http.StatusOK, mockUpdatedIssue), ), @@ -1113,77 +1144,84 @@ func Test_UpdateIssue(t *testing.T) { 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", - "state_reason": "completed", - "labels": []any{"bug", "priority"}, - "assignees": []any{"assignee1", "assignee2"}, - "milestone": float64(5), - "type": "Bug", + "title": "Updated Title", + "body": "Updated Description", }, expectError: false, expectedIssue: mockUpdatedIssue, }, { - name: "update issue fails with not found", - mockedClient: mock.NewMockedHTTPClient( + name: "issue not found when updating non-state fields only", + mockedRESTClient: mock.NewMockedHTTPClient( mock.WithRequestMatchHandler( mock.PatchReposIssuesByOwnerByRepoByIssueNumber, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusNotFound) - _, _ = w.Write([]byte(`{"message": "Issue not found"}`)) + _, _ = w.Write([]byte(`{"message": "Not Found"}`)) }), ), ), + mockedGQLClient: githubv4mock.NewMockedHTTPClient(), requestArgs: map[string]interface{}{ "owner": "owner", "repo": "repo", "issue_number": float64(999), - "title": "This issue doesn't exist", + "title": "Updated Title", }, expectError: true, expectedErrMsg: "failed to update issue", }, { - name: "update issue fails with validation error", - mockedClient: 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"}`)) - }), + name: "close issue as duplicate", + mockedRESTClient: mock.NewMockedHTTPClient( + mock.WithRequestMatch( + mock.GetReposIssuesByOwnerByRepoByIssueNumber, + mockUpdatedIssue, ), ), - requestArgs: map[string]interface{}{ - "owner": "owner", - "repo": "repo", - "issue_number": float64(123), - "state": "invalid_state", - }, - expectError: true, - expectedErrMsg: "failed to update issue", - }, - { - name: "close issue as not planned", - mockedClient: mock.NewMockedHTTPClient( - mock.WithRequestMatchHandler( - mock.PatchReposIssuesByOwnerByRepoByIssueNumber, - expectRequestBody(t, map[string]interface{}{ - "state": "closed", - "state_reason": "not_planned", - }).andThen( - mockResponse(t, http.StatusOK, mockClosedNotPlannedIssue), - ), + 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, ), - mock.WithRequestMatch( - mock.GetReposIssuesByOwnerByRepoByIssueNumber, - mockClosedNotPlannedIssue, + 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{}{ @@ -1191,150 +1229,160 @@ func Test_UpdateIssue(t *testing.T) { "repo": "repo", "issue_number": float64(123), "state": "closed", - "state_reason": "not_planned", + "state_reason": "duplicate", + "duplicate_of": float64(456), }, expectError: false, - expectedIssue: mockClosedNotPlannedIssue, + expectedIssue: mockUpdatedIssue, }, { - name: "close issue without state_reason", - mockedClient: mock.NewMockedHTTPClient( - mock.WithRequestMatchHandler( - mock.PatchReposIssuesByOwnerByRepoByIssueNumber, - expectRequestBody(t, map[string]interface{}{ - "state": "closed", - }).andThen( - mockResponse(t, http.StatusOK, mockClosedIssue), - ), - ), + name: "reopen issue", + mockedRESTClient: mock.NewMockedHTTPClient( mock.WithRequestMatch( mock.GetReposIssuesByOwnerByRepoByIssueNumber, - mockClosedIssue, + mockReopenedIssue, + ), + ), + 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": "closed", - // No state_reason provided + "state": "open", }, expectError: false, - expectedIssue: mockClosedIssue, + expectedIssue: mockReopenedIssue, }, { - name: "no update parameters provided", - mockedClient: mock.NewMockedHTTPClient(), + name: "main issue not found when trying to close it", + mockedRESTClient: mock.NewMockedHTTPClient(), + 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(123), - // No update fields + "issue_number": float64(999), + "state": "closed", + "state_reason": "not_planned", }, - expectError: false, // Error is returned in the result, not as Go error - expectedErrMsg: "No update parameters provided.", + expectError: true, + expectedErrMsg: "Failed to find issues", }, { - name: "state_reason without state should fail", - mockedClient: mock.NewMockedHTTPClient(), // No API calls expected + name: "duplicate issue not found when closing as duplicate", + mockedRESTClient: mock.NewMockedHTTPClient(), + 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), - // No state provided - "state_reason": "not_planned", + "state": "closed", + "state_reason": "duplicate", + "duplicate_of": float64(999), }, expectError: true, - expectedErrMsg: "state_reason can only be used when state is also provided", - }, - } - - 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), stubGetGQLClientFn(githubv4.NewClient(nil)), translations.NullTranslationHelper) - - // Create call request - request := createMCPRequest(tc.requestArgs) - - // Call handler - result, err := handler(context.Background(), request) - - // Verify results - 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 - textContent := getTextResult(t, result) - - // Unmarshal and verify the minimal result - var updateResp MinimalResponse - err = json.Unmarshal([]byte(textContent.Text), &updateResp) - require.NoError(t, err) - - assert.Equal(t, tc.expectedIssue.GetHTMLURL(), updateResp.URL) - }) - } -} - -func Test_UpdateIssue_CloseAsDuplicate(t *testing.T) { - // Setup mock issue for success case - mockClosedDuplicateIssue := &github.Issue{ - Number: github.Ptr(123), - Title: github.Ptr("Issue Title"), - Body: github.Ptr("Issue description"), - State: github.Ptr("closed"), - StateReason: github.Ptr("duplicate"), - HTMLURL: github.Ptr("https://github.com/owner/repo/issues/123"), - } - - // GraphQL response data for successful queries - successfulQueryResponse := githubv4mock.DataResponse(map[string]any{ - "repository": map[string]any{ - "issue": map[string]any{ - "id": "I_kwDOA0xdyM50BPaO", - }, - "duplicateIssue": map[string]any{ - "id": "I_kwDOA0xdyM50BPbP", - }, - }, - }) - - successfulMutationResponse := 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", - }, + expectedErrMsg: "Failed to find issues", }, - }) - - duplicateStateReason := IssueClosedStateReasonDuplicate - - tests := []struct { - name string - mockedClient *http.Client - requestArgs map[string]interface{} - expectError bool - expectedIssue *github.Issue - expectedErrMsg string - }{ { - name: "successful close as duplicate", - mockedClient: githubv4mock.NewMockedHTTPClient( + name: "close as duplicate with combined non-state updates", + mockedRESTClient: mock.NewMockedHTTPClient( + mock.WithRequestMatchHandler( + mock.PatchReposIssuesByOwnerByRepoByIssueNumber, + 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"), + }), + ), + ), + mock.WithRequestMatch( + mock.GetReposIssuesByOwnerByRepoByIssueNumber, + mockUpdatedIssue, + ), + ), + mockedGQLClient: githubv4mock.NewMockedHTTPClient( githubv4mock.NewQueryMatcher( struct { Repository struct { @@ -1343,16 +1391,16 @@ func Test_UpdateIssue_CloseAsDuplicate(t *testing.T) { } `graphql:"issue(number: $issueNumber)"` DuplicateIssue struct { ID githubv4.ID - } `graphql:"duplicateIssue: issue(number: $duplicateNumber)"` + } `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), - "duplicateNumber": githubv4.Int(456), + "owner": githubv4.String("owner"), + "repo": githubv4.String("repo"), + "issueNumber": githubv4.Int(123), + "duplicateOf": githubv4.Int(456), }, - successfulQueryResponse, + duplicateIssueIDQueryResponse, ), githubv4mock.NewMutationMatcher( struct { @@ -1371,81 +1419,74 @@ func Test_UpdateIssue_CloseAsDuplicate(t *testing.T) { DuplicateIssueID: githubv4.NewID("I_kwDOA0xdyM50BPbP"), }, nil, - successfulMutationResponse, + closeSuccessResponse, ), ), requestArgs: map[string]interface{}{ "owner": "owner", "repo": "repo", "issue_number": float64(123), + "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: mockClosedDuplicateIssue, + expectedIssue: mockUpdatedIssue, }, { - name: "fail to close as duplicate - duplicate_of missing", - mockedClient: mock.NewMockedHTTPClient(), // No API call expected + name: "no update params provided", + mockedRESTClient: mock.NewMockedHTTPClient(), + mockedGQLClient: githubv4mock.NewMockedHTTPClient(), requestArgs: map[string]interface{}{ "owner": "owner", "repo": "repo", "issue_number": float64(123), - "state": "closed", - "state_reason": "duplicate", - // Missing duplicate_of }, expectError: true, - expectedErrMsg: "duplicate_of must be provided when state_reason is 'duplicate'", + expectedErrMsg: "No update parameters provided.", }, { - name: "fail to close as duplicate - duplicate issue not found", - mockedClient: 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: $duplicateNumber)"` - } `graphql:"repository(owner: $owner, name: $repo)"` - }{}, - map[string]any{ - "owner": githubv4.String("owner"), - "repo": githubv4.String("repo"), - "issueNumber": githubv4.Int(123), - "duplicateNumber": githubv4.Int(999), - }, - githubv4mock.ErrorResponse("Could not resolve to an Issue with the number of 999."), - ), - ), + name: "state_reason without state should fail", + mockedRESTClient: mock.NewMockedHTTPClient(), + mockedGQLClient: githubv4mock.NewMockedHTTPClient(), + requestArgs: map[string]interface{}{ + "owner": "owner", + "repo": "repo", + "issue_number": float64(123), + "state_reason": "not_planned", + }, + expectError: true, + expectedErrMsg: "state_reason can only be used when state is also provided", + }, + { + 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": "duplicate", - "duplicate_of": float64(999), + "state_reason": "completed", + "duplicate_of": float64(456), }, expectError: true, - expectedErrMsg: "Failed to find issues", + expectedErrMsg: "duplicate_of can only be used when state_reason is 'duplicate'", }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - // Mock both GraphQL and the final REST API GET call - restClient := github.NewClient(mock.NewMockedHTTPClient( - mock.WithRequestMatch( - mock.GetReposIssuesByOwnerByRepoByIssueNumber, - mockClosedDuplicateIssue, - ), - )) - mockGQLClient := githubv4.NewClient(tc.mockedClient) - _, handler := UpdateIssue(stubGetClientFn(restClient), stubGetGQLClientFn(mockGQLClient), 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) From 0c9bc5644b464e25c654bb387854da70adb51262 Mon Sep 17 00:00:00 2001 From: kerobbi Date: Thu, 11 Sep 2025 15:17:58 +0100 Subject: [PATCH 6/7] fix marshal failures --- pkg/github/issues.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/github/issues.go b/pkg/github/issues.go index 31091944d..7a9717fc8 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -1466,7 +1466,7 @@ func UpdateIssue(getClient GetClientFn, getGQLClient GetGQLClientFn, t translati r, err := json.Marshal(minimalResponse) if err != nil { - return mcp.NewToolResultError(fmt.Sprintf("Failed to marshal response: %v", err)), nil + return nil, fmt.Errorf("failed to marshal response: %w", err) } return mcp.NewToolResultText(string(r)), nil From d66d3acee22801ae0bab92cf0a9a23bfe9e1c4e0 Mon Sep 17 00:00:00 2001 From: kerobbi Date: Fri, 12 Sep 2025 12:03:24 +0100 Subject: [PATCH 7/7] address latest feedback --- pkg/github/issues.go | 86 ++++++++++++--------------------------- pkg/github/issues_test.go | 80 ++++++++++++++++-------------------- 2 files changed, 60 insertions(+), 106 deletions(-) diff --git a/pkg/github/issues.go b/pkg/github/issues.go index 7a9717fc8..1c88a9fde 100644 --- a/pkg/github/issues.go +++ b/pkg/github/issues.go @@ -88,17 +88,14 @@ func fetchIssueIDs(ctx context.Context, gqlClient *githubv4.Client, owner, repo } // getCloseStateReason converts a string state reason to the appropriate enum value -func getCloseStateReason(stateReason string) *IssueClosedStateReason { +func getCloseStateReason(stateReason string) IssueClosedStateReason { switch stateReason { case "not_planned": - reason := IssueClosedStateReasonNotPlanned - return &reason + return IssueClosedStateReasonNotPlanned case "duplicate": - reason := IssueClosedStateReasonDuplicate - return &reason + return IssueClosedStateReasonDuplicate default: // Default to "completed" for empty or "completed" values - reason := IssueClosedStateReasonCompleted - return &reason + return IssueClosedStateReasonCompleted } } @@ -1259,7 +1256,6 @@ func UpdateIssue(getClient GetClientFn, getGQLClient GetGQLClientFn, t translati // Create the issue request with only provided fields issueRequest := &github.IssueRequest{} - hasNonStateUpdates := false // Set optional parameters if provided title, err := OptionalParam[string](request, "title") @@ -1268,7 +1264,6 @@ func UpdateIssue(getClient GetClientFn, getGQLClient GetGQLClientFn, t translati } if title != "" { issueRequest.Title = github.Ptr(title) - hasNonStateUpdates = true } body, err := OptionalParam[string](request, "body") @@ -1277,7 +1272,6 @@ func UpdateIssue(getClient GetClientFn, getGQLClient GetGQLClientFn, t translati } if body != "" { issueRequest.Body = github.Ptr(body) - hasNonStateUpdates = true } // Get labels @@ -1287,7 +1281,6 @@ func UpdateIssue(getClient GetClientFn, getGQLClient GetGQLClientFn, t translati } if len(labels) > 0 { issueRequest.Labels = &labels - hasNonStateUpdates = true } // Get assignees @@ -1297,7 +1290,6 @@ func UpdateIssue(getClient GetClientFn, getGQLClient GetGQLClientFn, t translati } if len(assignees) > 0 { issueRequest.Assignees = &assignees - hasNonStateUpdates = true } milestone, err := OptionalIntParam(request, "milestone") @@ -1307,7 +1299,6 @@ func UpdateIssue(getClient GetClientFn, getGQLClient GetGQLClientFn, t translati if milestone != 0 { milestoneNum := milestone issueRequest.Milestone = &milestoneNum - hasNonStateUpdates = true } // Get issue type @@ -1317,7 +1308,6 @@ func UpdateIssue(getClient GetClientFn, getGQLClient GetGQLClientFn, t translati } if issueType != "" { issueRequest.Type = github.Ptr(issueType) - hasNonStateUpdates = true } // Handle state, state_reason and duplicateOf parameters @@ -1330,9 +1320,6 @@ func UpdateIssue(getClient GetClientFn, getGQLClient GetGQLClientFn, t translati if err != nil { return mcp.NewToolResultError(err.Error()), nil } - if stateReason != "" && state == "" { - return mcp.NewToolResultError("state_reason can only be used when state is also provided"), nil - } duplicateOf, err := OptionalIntParam(request, "duplicate_of") if err != nil { @@ -1342,35 +1329,28 @@ func UpdateIssue(getClient GetClientFn, getGQLClient GetGQLClientFn, t translati return mcp.NewToolResultError("duplicate_of can only be used when state_reason is 'duplicate'"), nil } - // Determine if there were any updates at all - if !hasNonStateUpdates && state == "" { - return mcp.NewToolResultError("No update parameters provided."), 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) } - // Use REST API for non-state updates - if hasNonStateUpdates { - 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 ghErrors.NewGitHubAPIErrorResponse(ctx, + "failed to update issue", + resp, + err, + ), nil + } + defer func() { _ = resp.Body.Close() }() - _, resp, err := client.Issues.Edit(ctx, owner, repo, issueNumber, issueRequest) + if resp.StatusCode != http.StatusOK { + body, err := io.ReadAll(resp.Body) if err != nil { - return ghErrors.NewGitHubAPIErrorResponse(ctx, - "failed to update issue", - resp, - err, - ), nil - } - defer func() { _ = resp.Body.Close() }() - - if resp.StatusCode != http.StatusOK { - body, err := io.ReadAll(resp.Body) - if err != nil { - return nil, fmt.Errorf("failed to read response body: %w", err) - } - return mcp.NewToolResultError(fmt.Sprintf("failed to update issue: %s", string(body))), nil + return nil, fmt.Errorf("failed to read response body: %w", err) } + return mcp.NewToolResultError(fmt.Sprintf("failed to update issue: %s", string(body))), nil } // Use GraphQL API for state updates @@ -1391,7 +1371,6 @@ func UpdateIssue(getClient GetClientFn, getGQLClient GetGQLClientFn, t translati return ghErrors.NewGitHubGraphQLErrorResponse(ctx, "Failed to find issues", err), nil } - // Do all logic switch state { case "open": // Use ReopenIssue mutation for opening @@ -1425,9 +1404,10 @@ func UpdateIssue(getClient GetClientFn, getGQLClient GetGQLClientFn, t translati } `graphql:"closeIssue(input: $input)"` } + stateReasonValue := getCloseStateReason(stateReason) closeInput := CloseIssueInput{ IssueID: issueID, - StateReason: getCloseStateReason(stateReason), + StateReason: &stateReasonValue, } // Set duplicate issue ID if needed @@ -1442,26 +1422,10 @@ func UpdateIssue(getClient GetClientFn, getGQLClient GetGQLClientFn, t translati } } - // Get the final state of the issue to return - client, err := getClient(ctx) - if err != nil { - return nil, err - } - - finalIssue, resp, err := client.Issues.Get(ctx, owner, repo, issueNumber) - if err != nil { - return ghErrors.NewGitHubAPIErrorResponse(ctx, "Failed to get issue", resp, err), nil - } - defer func() { - if resp != nil && resp.Body != nil { - _ = resp.Body.Close() - } - }() - // Return minimal response with just essential information minimalResponse := MinimalResponse{ - ID: fmt.Sprintf("%d", finalIssue.GetID()), - URL: finalIssue.GetHTMLURL(), + ID: fmt.Sprintf("%d", updatedIssue.GetID()), + URL: updatedIssue.GetHTMLURL(), } r, err := json.Marshal(minimalResponse) diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index d7ae2db9a..cc1923df9 100644 --- a/pkg/github/issues_test.go +++ b/pkg/github/issues_test.go @@ -1054,6 +1054,18 @@ func Test_UpdateIssue(t *testing.T) { assert.ElementsMatch(t, tool.InputSchema.Required, []string{"owner", "repo", "issue_number"}) // Mock issues for reuse across test cases + mockBaseIssue := &github.Issue{ + Number: github.Ptr(123), + 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")}}, + Milestone: &github.Milestone{Number: github.Ptr(5)}, + Type: &github.IssueType{Name: github.Ptr("Bug")}, + } + mockUpdatedIssue := &github.Issue{ Number: github.Ptr(123), Title: github.Ptr("Updated Title"), @@ -1068,10 +1080,11 @@ func Test_UpdateIssue(t *testing.T) { } mockReopenedIssue := &github.Issue{ - Number: github.Ptr(123), - Title: github.Ptr("Issue Title"), - State: github.Ptr("open"), - HTMLURL: github.Ptr("https://github.com/owner/repo/issues/123"), + 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 @@ -1139,10 +1152,6 @@ func Test_UpdateIssue(t *testing.T) { mockResponse(t, http.StatusOK, mockUpdatedIssue), ), ), - mock.WithRequestMatch( - mock.GetReposIssuesByOwnerByRepoByIssueNumber, - mockUpdatedIssue, - ), ), mockedGQLClient: githubv4mock.NewMockedHTTPClient(), requestArgs: map[string]interface{}{ @@ -1180,8 +1189,8 @@ func Test_UpdateIssue(t *testing.T) { name: "close issue as duplicate", mockedRESTClient: mock.NewMockedHTTPClient( mock.WithRequestMatch( - mock.GetReposIssuesByOwnerByRepoByIssueNumber, - mockUpdatedIssue, + mock.PatchReposIssuesByOwnerByRepoByIssueNumber, + mockBaseIssue, ), ), mockedGQLClient: githubv4mock.NewMockedHTTPClient( @@ -1239,8 +1248,8 @@ func Test_UpdateIssue(t *testing.T) { name: "reopen issue", mockedRESTClient: mock.NewMockedHTTPClient( mock.WithRequestMatch( - mock.GetReposIssuesByOwnerByRepoByIssueNumber, - mockReopenedIssue, + mock.PatchReposIssuesByOwnerByRepoByIssueNumber, + mockBaseIssue, ), ), mockedGQLClient: githubv4mock.NewMockedHTTPClient( @@ -1287,8 +1296,13 @@ func Test_UpdateIssue(t *testing.T) { expectedIssue: mockReopenedIssue, }, { - name: "main issue not found when trying to close it", - mockedRESTClient: mock.NewMockedHTTPClient(), + name: "main issue not found when trying to close it", + mockedRESTClient: mock.NewMockedHTTPClient( + mock.WithRequestMatch( + mock.PatchReposIssuesByOwnerByRepoByIssueNumber, + mockBaseIssue, + ), + ), mockedGQLClient: githubv4mock.NewMockedHTTPClient( githubv4mock.NewQueryMatcher( struct { @@ -1317,8 +1331,13 @@ func Test_UpdateIssue(t *testing.T) { expectedErrMsg: "Failed to find issues", }, { - name: "duplicate issue not found when closing as duplicate", - mockedRESTClient: 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 { @@ -1377,10 +1396,6 @@ func Test_UpdateIssue(t *testing.T) { }), ), ), - mock.WithRequestMatch( - mock.GetReposIssuesByOwnerByRepoByIssueNumber, - mockUpdatedIssue, - ), ), mockedGQLClient: githubv4mock.NewMockedHTTPClient( githubv4mock.NewQueryMatcher( @@ -1439,31 +1454,6 @@ func Test_UpdateIssue(t *testing.T) { expectError: false, expectedIssue: mockUpdatedIssue, }, - { - name: "no update params provided", - mockedRESTClient: mock.NewMockedHTTPClient(), - mockedGQLClient: githubv4mock.NewMockedHTTPClient(), - requestArgs: map[string]interface{}{ - "owner": "owner", - "repo": "repo", - "issue_number": float64(123), - }, - expectError: true, - expectedErrMsg: "No update parameters provided.", - }, - { - name: "state_reason without state should fail", - mockedRESTClient: mock.NewMockedHTTPClient(), - mockedGQLClient: githubv4mock.NewMockedHTTPClient(), - requestArgs: map[string]interface{}{ - "owner": "owner", - "repo": "repo", - "issue_number": float64(123), - "state_reason": "not_planned", - }, - expectError: true, - expectedErrMsg: "state_reason can only be used when state is also provided", - }, { name: "duplicate_of without duplicate state_reason should fail", mockedRESTClient: mock.NewMockedHTTPClient(),