move state and state reason handling to gql

Author: kerobbi

pkg/github/__toolsnaps__/update_issue.snap

@@ -57,8 +57,7 @@
         "enum": [
           "completed",
           "not_planned",
-          "duplicate",
-          "reopened"
+          "duplicate"
         ],
         "type": "string"
       },

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
+					}
 				}
 			}