From 93d9077a1373b7a4b1ab6c66aa597e0f8671d263 Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Mon, 17 Aug 2026 15:22:09 +0200 Subject: [PATCH] fix(issues): validate issue comment input modes Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- README.md | 2 +- .../__toolsnaps__/add_issue_comment.snap | 27 +++- pkg/github/issues.go | 31 ++++- pkg/github/issues_test.go | 123 +++++++++++++++++- 4 files changed, 174 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index 32f8eb82bc..b8d35ca1da 100644 --- a/README.md +++ b/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) diff --git a/pkg/github/__toolsnaps__/add_issue_comment.snap b/pkg/github/__toolsnaps__/add_issue_comment.snap index f59431816e..708535ff58 100644 --- a/pkg/github/__toolsnaps__/add_issue_comment.snap +++ b/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.", diff --git a/pkg/github/issues.go b/pkg/github/issues.go index dfb823e26b..3f1aaf8489 100644 --- a/pkg/github/issues.go +++ b/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) diff --git a/pkg/github/issues_test.go b/pkg/github/issues_test.go index 77380e5e21..ea1e651466 100644 --- a/pkg/github/issues_test.go +++ b/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{