fix state reason validation logic and update tests

Author: kerobbi

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")

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),
 			},