fix(issues): validate issue comment input modes

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Author: SamMorrowDrums

README.md

@@ -894,7 +894,7 @@ The following sets of tools are available:
 - **add_issue_comment** - Add comment to issue or pull request
   - **Required OAuth Scopes**: `repo`
   - `body`: Comment content. Required unless reaction is provided. (string, optional)
-  - `comment_id`: The numeric ID of the issue or pull request comment to react to. Use this for reactions to comments; omit it to react to the issue or pull request itself. Cannot be combined with body. (number, optional)
+  - `comment_id`: The numeric ID of the issue or pull request comment to react to. Use this for reactions to comments; omit it to react to the issue or pull request itself. Cannot be combined with body. (integer, optional)
   - `issue_number`: Issue or pull request number to comment on or react to. (number, required)
   - `owner`: Repository owner (string, required)
   - `reaction`: Emoji reaction to add. Required unless body is provided. (string, optional)

pkg/github/__toolsnaps__/add_issue_comment.snap

@@ -6,15 +6,40 @@
   },
   "description": "Add a comment and/or reaction to a specific issue or issue comment in a GitHub repository. Use this tool with pull requests as well (in this case pass pull request number as issue_number), but only if user is not asking specifically to add or react to review comments. At least one of body or reaction is required.",
   "inputSchema": {
+    "anyOf": [
+      {
+        "required": [
+          "body"
+        ]
+      },
+      {
+        "required": [
+          "reaction"
+        ]
+      }
+    ],
+    "dependentSchemas": {
+      "comment_id": {
+        "not": {
+          "required": [
+            "body"
+          ]
+        },
+        "required": [
+          "reaction"
+        ]
+      }
+    },
     "properties": {
       "body": {
         "description": "Comment content. Required unless reaction is provided.",
+        "minLength": 1,
         "type": "string"
       },
       "comment_id": {
         "description": "The numeric ID of the issue or pull request comment to react to. Use this for reactions to comments; omit it to react to the issue or pull request itself. Cannot be combined with body.",
         "minimum": 1,
-        "type": "number"
+        "type": "integer"
       },
       "issue_number": {
         "description": "Issue or pull request number to comment on or react to.",

pkg/github/issues.go

@@ -1212,13 +1212,14 @@ func AddIssueComment(t translations.TranslationHelperFunc) inventory.ServerTool
 						Description: "Issue or pull request number to comment on or react to.",
 					},
 					"comment_id": {
-						Type:        "number",
+						Type:        "integer",
 						Description: "The numeric ID of the issue or pull request comment to react to. Use this for reactions to comments; omit it to react to the issue or pull request itself. Cannot be combined with body.",
 						Minimum:     jsonschema.Ptr(1.0),
 					},
 					"body": {
 						Type:        "string",
 						Description: "Comment content. Required unless reaction is provided.",
+						MinLength:   jsonschema.Ptr(1),
 					},
 					"reaction": {
 						Type:        "string",
@@ -1227,6 +1228,16 @@ func AddIssueComment(t translations.TranslationHelperFunc) inventory.ServerTool
 					},
 				},
 				Required: []string{"owner", "repo", "issue_number"},
+				AnyOf: []*jsonschema.Schema{
+					{Required: []string{"body"}},
+					{Required: []string{"reaction"}},
+				},
+				DependentSchemas: map[string]*jsonschema.Schema{
+					"comment_id": {
+						Required: []string{"reaction"},
+						Not:      &jsonschema.Schema{Required: []string{"body"}},
+					},
+				},
 			},
 		},
 		[]scopes.Scope{scopes.Repo},
@@ -1245,10 +1256,10 @@ func AddIssueComment(t translations.TranslationHelperFunc) inventory.ServerTool
 			}
 			var commentID int64
 			hasCommentID := false
-			if _, ok := args["comment_id"]; ok {
-				commentID, err = RequiredBigInt(args, "comment_id")
+			if value, ok := args["comment_id"]; ok {
+				commentID, err = toInt64(value)
 				if err != nil {
-					return utils.NewToolResultError(err.Error()), nil, nil
+					return utils.NewToolResultError(fmt.Sprintf("parameter comment_id is not a valid number: %v", err)), nil, nil
 				}
 				if commentID < 1 {
 					return utils.NewToolResultError("comment_id must be greater than 0"), nil, nil
@@ -1278,6 +1289,9 @@ func AddIssueComment(t translations.TranslationHelperFunc) inventory.ServerTool
 			if hasReaction && reactionContent == "" {
 				return utils.NewToolResultError("reaction cannot be empty when provided"), nil, nil
 			}
+			if hasReaction && !isValidIssueReaction(reactionContent) {
+				return utils.NewToolResultError("reaction must be one of +1, -1, laugh, confused, heart, hooray, rocket, eyes"), nil, nil
+			}
 
 			client, err := deps.GetClient(ctx)
 			if err != nil {
@@ -1372,6 +1386,15 @@ func AddIssueComment(t translations.TranslationHelperFunc) inventory.ServerTool
 		})
 }
 
+func isValidIssueReaction(reaction string) bool {
+	switch reaction {
+	case "+1", "-1", "laugh", "confused", "heart", "hooray", "rocket", "eyes":
+		return true
+	default:
+		return false
+	}
+}
+
 func issueNumberFromIssueURL(issueURL string) (int, error) {
 	issueNumberString := issueURL[strings.LastIndex(issueURL, "/")+1:]
 	issueNumber, err := strconv.Atoi(issueNumberString)

pkg/github/issues_test.go

@@ -4738,11 +4738,10 @@ func Test_GetSubIssues(t *testing.T) {
 	}
 }
 
-func TestAddIssueComment(t *testing.T) {
+func TestAddIssueCommentSchema(t *testing.T) {
 	t.Parallel()
 
-	serverTool := AddIssueComment(translations.NullTranslationHelper)
-	tool := serverTool.Tool
+	tool := AddIssueComment(translations.NullTranslationHelper).Tool
 	require.NoError(t, toolsnaps.Test(tool.Name, tool))
 
 	assert.Equal(t, "add_issue_comment", tool.Name)
@@ -4756,6 +4755,101 @@ func TestAddIssueComment(t *testing.T) {
 	assert.Contains(t, schema.Properties, "reaction")
 	assert.ElementsMatch(t, schema.Required, []string{"owner", "repo", "issue_number"})
 
+	resolved, err := schema.Resolve(nil)
+	require.NoError(t, err)
+
+	baseArgs := map[string]any{
+		"owner":        "owner",
+		"repo":         "repo",
+		"issue_number": 42,
+	}
+	tests := []struct {
+		name    string
+		args    map[string]any
+		isValid bool
+	}{
+		{
+			name:    "body-only comment",
+			args:    map[string]any{"body": "This is a comment"},
+			isValid: true,
+		},
+		{
+			name:    "issue or pull request reaction",
+			args:    map[string]any{"reaction": "heart"},
+			isValid: true,
+		},
+		{
+			name:    "comment and issue or pull request reaction",
+			args:    map[string]any{"body": "This is a comment", "reaction": "heart"},
+			isValid: true,
+		},
+		{
+			name:    "existing comment reaction",
+			args:    map[string]any{"comment_id": 999, "reaction": "heart"},
+			isValid: true,
+		},
+		{
+			name:    "missing body and reaction",
+			args:    map[string]any{},
+			isValid: false,
+		},
+		{
+			name:    "empty body",
+			args:    map[string]any{"body": ""},
+			isValid: false,
+		},
+		{
+			name:    "comment_id without reaction",
+			args:    map[string]any{"comment_id": 999},
+			isValid: false,
+		},
+		{
+			name:    "comment_id with body",
+			args:    map[string]any{"comment_id": 999, "body": "This is a comment"},
+			isValid: false,
+		},
+		{
+			name:    "comment_id with body and reaction",
+			args:    map[string]any{"comment_id": 999, "body": "This is a comment", "reaction": "heart"},
+			isValid: false,
+		},
+		{
+			name:    "zero comment_id",
+			args:    map[string]any{"comment_id": 0, "reaction": "heart"},
+			isValid: false,
+		},
+		{
+			name:    "fractional comment_id",
+			args:    map[string]any{"comment_id": 1.5, "reaction": "heart"},
+			isValid: false,
+		},
+		{
+			name:    "invalid reaction",
+			args:    map[string]any{"reaction": "party"},
+			isValid: false,
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			args := maps.Clone(baseArgs)
+			maps.Copy(args, tc.args)
+			err := resolved.Validate(args)
+			if tc.isValid {
+				require.NoError(t, err)
+				return
+			}
+			require.Error(t, err)
+		})
+	}
+}
+
+func TestAddIssueCommentHandler(t *testing.T) {
+	t.Parallel()
+
+	serverTool := AddIssueComment(translations.NullTranslationHelper)
 	mockComment := &github.IssueComment{
 		ID:      github.Ptr(int64(456)),
 		Body:    github.Ptr("This is a comment"),
@@ -4908,6 +5002,18 @@ func TestAddIssueComment(t *testing.T) {
 			expectToolError:    true,
 			expectedToolErrMsg: "comment_id can only be provided when reaction is provided",
 		},
+		{
+			name: "zero comment_id",
+			requestArgs: map[string]any{
+				"owner":        "owner",
+				"repo":         "repo",
+				"issue_number": float64(42),
+				"comment_id":   float64(0),
+				"reaction":     "heart",
+			},
+			expectToolError:    true,
+			expectedToolErrMsg: "comment_id must be greater than 0",
+		},
 		{
 			name: "negative comment_id",
 			requestArgs: map[string]any{
@@ -4933,6 +5039,17 @@ func TestAddIssueComment(t *testing.T) {
 			expectToolError:    true,
 			expectedToolErrMsg: "comment_id cannot be combined with body",
 		},
+		{
+			name: "invalid reaction",
+			requestArgs: map[string]any{
+				"owner":        "owner",
+				"repo":         "repo",
+				"issue_number": float64(42),
+				"reaction":     "party",
+			},
+			expectToolError:    true,
+			expectedToolErrMsg: "reaction must be one of +1, -1, laugh, confused, heart, hooray, rocket, eyes",
+		},
 		{
 			name: "does not create comment when reaction fails",
 			mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{